Patchwork Don't use UDMA on VIA UDMA33 controller with Transcend SSD

login
register
mail settings
Submitter Mikulas Patocka
Date Nov. 18, 2009, 5:09 p.m.
Message ID <Pine.LNX.4.64.0911181151140.5068@hs20-bc2-1.build.redhat.com>
Download mbox | patch
Permalink /patch/38764/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Mikulas Patocka - Nov. 18, 2009, 5:09 p.m.
On Tue, 17 Nov 2009, David Miller wrote:

> From: Mikulas Patocka <mpatocka@redhat.com>
> Date: Wed, 4 Nov 2009 20:25:21 -0500 (EST)
> 
> > Don't use UDMA on VIA UDMA33 controller with Transcend SSD
> > 
> > The computer locks up if Transcend SSD runs in any of UDMA modes.
> > It doesn't lockup with different brand SSD, so this is specific to Transcend.
> > 
> > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> 
> Mikulas, I'm happy to apply this if you match on the full
> ID string, not just "TS".
> 
> Please update your patch and I'll push it upstream.
> 
> Thank you.

Transcend makes various versions of their SSDs and all begin with TS. You 
can assume that Transcend SSDs with different capacity or format won't 
work too because they likely use the same controller. The naming is this:

TS64GSSD25-M
TS: Transcend
64G: capacity
SSD25: 2.5" format
-M: MLC

So the problem is that if you match against the full string, you are going 
to miss the other Transcend devices and the patch becomes quite useless.

If you want to harden it against false negatives, you can grep the string 
for "SSD", as in the patch below, but there is not anything better to do 
--- if you include "64G" in the string, you fail on non-64G devices, if 
you include "SSD25", you fail on 1.8" devices, if you include "-M", you 
fail on SLC.

Mikulas

---

Don't use UDMA on VIA UDMA33 controller with Transcend SSD

The computer locks up after if Transcend SSD runs in any of UDMA modes.
It doesn't lockup with different brand SSD, so this is specific to Transcend.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/ide/via82cxxx.c |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Cox - Nov. 18, 2009, 5:22 p.m.
> Transcend makes various versions of their SSDs and all begin with TS. You 
> can assume that Transcend SSDs with different capacity or format won't 
> work too because they likely use the same controller. The naming is this:

You don't want to assume that because then you will never find out if
there is no problem.

> So the problem is that if you match against the full string, you are going 
> to miss the other Transcend devices and the patch becomes quite useless.

Quite the reverse. It's not implausible that only one or two devices are
affected, but match TS SSD is going to match zillions of devices of many
generations for years to come. Thats not good.

Experience is also that very few of the existing blacklist entries we
have grow to be large scale wildcards.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Nov. 18, 2009, 5:32 p.m.
From: Alan Cox <alan@lxorguk.ukuu.org.uk>
Date: Wed, 18 Nov 2009 17:22:58 +0000

>> Transcend makes various versions of their SSDs and all begin with TS. You 
>> can assume that Transcend SSDs with different capacity or format won't 
>> work too because they likely use the same controller. The naming is this:
> 
> You don't want to assume that because then you will never find out if
> there is no problem.

Agreed.

And if Alan's patch for the ATA layer is going to use an exact string
match, which it is, and we should do the same in the IDE layer to be
consistent.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mikulas Patocka - Nov. 18, 2009, 5:37 p.m.
> > Transcend makes various versions of their SSDs and all begin with TS. You 
> > can assume that Transcend SSDs with different capacity or format won't 
> > work too because they likely use the same controller. The naming is this:
> 
> You don't want to assume that because then you will never find out if
> there is no problem.

I can be certain that no company would be stupid enough to design a 
separate controller for each capacity and form factor of their devices. 
They use one or a very few chips.

Furthemore, even if they upgrade the chip and produce 64G 2.5" MLC device 
with the new working chip, you can't tell it from the ID string at all.

> > So the problem is that if you match against the full string, you are going 
> > to miss the other Transcend devices and the patch becomes quite useless.
> 
> Quite the reverse. It's not implausible that only one or two devices are
> affected, but match TS SSD is going to match zillions of devices of many
> generations for years to come. Thats not good.

Why no good? People just get lower performance (on an old motherboard, 
where no one expects high performance anyway).

On the other hand, if you miss a device from the blacklist, people get 
crashes.

Crashes are worse than lower performance.

> Experience is also that very few of the existing blacklist entries we
> have grow to be large scale wildcards.

Mikulas
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mikulas Patocka - Nov. 18, 2009, 5:46 p.m.
On Wed, 18 Nov 2009, David Miller wrote:

> From: Alan Cox <alan@lxorguk.ukuu.org.uk>
> Date: Wed, 18 Nov 2009 17:22:58 +0000
> 
> >> Transcend makes various versions of their SSDs and all begin with TS. You 
> >> can assume that Transcend SSDs with different capacity or format won't 
> >> work too because they likely use the same controller. The naming is this:
> > 
> > You don't want to assume that because then you will never find out if
> > there is no problem.
> 
> Agreed.
> 
> And if Alan's patch for the ATA layer is going to use an exact string
> match, which it is, and we should do the same in the IDE layer to be
> consistent.

And what will you do with 16G or 32G versions of the same device? You 
really think they use different controllers and are not affected?

Mikulas
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Cox - Nov. 18, 2009, 5:50 p.m.
> I can be certain that no company would be stupid enough to design a 
> separate controller for each capacity and form factor of their devices. 
> They use one or a very few chips.

But they do fix the firmware, and you see that with hard disks as well.

> Why no good? People just get lower performance (on an old motherboard, 
> where no one expects high performance anyway).

Which they don't notice or report
> 
> On the other hand, if you miss a device from the blacklist, people get 
> crashes.

Which they do notice or report (and in the meantime can boot with
libata.dma=0 or one of the MWDMA settings).

> Crashes are worse than lower performance.

Not always. Inflicting poor performance (and also *much* lower data
integrity) on people isn't a good idea in this case IMHO. That's based
upon experience with disks and what gets fixed.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Nov. 18, 2009, 5:53 p.m.
From: Mikulas Patocka <mpatocka@redhat.com>
Date: Wed, 18 Nov 2009 12:46:37 -0500 (EST)

> And what will you do with 16G or 32G versions of the same device? You 
> really think they use different controllers and are not affected?

Firmware changes often.
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mikulas Patocka - Nov. 18, 2009, 6:02 p.m.
On Wed, 18 Nov 2009, Alan Cox wrote:

> > I can be certain that no company would be stupid enough to design a 
> > separate controller for each capacity and form factor of their devices. 
> > They use one or a very few chips.
> 
> But they do fix the firmware, and you see that with hard disks as well.

So match it against a firmware revision (V0826 in my case, so blacklist 
all lower).

Anyway, I think it is bug on the motherboard because similar lockups can 
be seen with one WD 40G disk operating at UDMA0 or UDMA1 (UDMA2 is OK). 
And if this is old buggy motherboard, Transcend will fix nothing.

> > Why no good? People just get lower performance (on an old motherboard, 
> > where no one expects high performance anyway).
> 
> Which they don't notice or report

If they don't notice it, it is not a problem :)

Mikulas
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mikulas Patocka - Nov. 18, 2009, 6:04 p.m.
On Wed, 18 Nov 2009, David Miller wrote:

> From: Mikulas Patocka <mpatocka@redhat.com>
> Date: Wed, 18 Nov 2009 12:46:37 -0500 (EST)
> 
> > And what will you do with 16G or 32G versions of the same device? You 
> > really think they use different controllers and are not affected?
> 
> Firmware changes often.

So use the firmware revision field to match it.

Mikulas
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bartlomiej Zolnierkiewicz - Oct. 11, 2011, 5:12 p.m.
Mikulas Patocka wrote:

> On Tue, 17 Nov 2009, David Miller wrote:
> 
> > From: Mikulas Patocka <mpatocka@redhat.com>
> > Date: Wed, 4 Nov 2009 20:25:21 -0500 (EST)
> > 
> > > Don't use UDMA on VIA UDMA33 controller with Transcend SSD
> > > 
> > > The computer locks up if Transcend SSD runs in any of UDMA modes.
> > > It doesn't lockup with different brand SSD, so this is specific to Transcend.
> > > 
> > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > 
> > Mikulas, I'm happy to apply this if you match on the full
> > ID string, not just "TS".
> > 
> > Please update your patch and I'll push it upstream.
> > 
> > Thank you.
> 
> Transcend makes various versions of their SSDs and all begin with TS. You 
> can assume that Transcend SSDs with different capacity or format won't 
> work too because they likely use the same controller. The naming is this:
> 
> TS64GSSD25-M
> TS: Transcend
> 64G: capacity
> SSD25: 2.5" format
> -M: MLC
> 
> So the problem is that if you match against the full string, you are going 
> to miss the other Transcend devices and the patch becomes quite useless.
> 
> If you want to harden it against false negatives, you can grep the string 
> for "SSD", as in the patch below, but there is not anything better to do 
> --- if you include "64G" in the string, you fail on non-64G devices, if 
> you include "SSD25", you fail on 1.8" devices, if you include "-M", you 
> fail on SLC.
> 
> Mikulas
> 
> ---
> 
> Don't use UDMA on VIA UDMA33 controller with Transcend SSD
> 
> The computer locks up after if Transcend SSD runs in any of UDMA modes.
> It doesn't lockup with different brand SSD, so this is specific to Transcend.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

Reviewed-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>

Still valid for 3.1.  Dave, ping?

> ---
>  drivers/ide/via82cxxx.c |   17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> Index: linux-2.6.31.6-fast/drivers/ide/via82cxxx.c
> ===================================================================
> --- linux-2.6.31.6-fast.orig/drivers/ide/via82cxxx.c	2009-11-16 13:08:04.000000000 +0100
> +++ linux-2.6.31.6-fast/drivers/ide/via82cxxx.c	2009-11-18 17:57:22.000000000 +0100
> @@ -195,6 +195,22 @@ static void via_set_pio_mode(ide_drive_t
>  	via_set_drive(drive, XFER_PIO_0 + pio);
>  }
>  
> +static u8 via_udma_filter(ide_drive_t *drive)
> +{
> +	char *m = (char *)&drive->id[ATA_ID_PROD];
> +
> +	/*
> +	 * Restrict UDMA for Transcend flash cards.
> +	 * On VIA 33, UDMA locks up. On VIA 133, it works. I can't test other
> +	 * controllers.
> +	 */
> +	if (!memcmp(m, "TS", 2) && strstr(m, "SSD") &&
> +	    drive->hwif->ultra_mask == ATA_UDMA2)
> +		return 0;
> +
> +	return drive->hwif->ultra_mask;
> +}
> +
>  static struct via_isa_bridge *via_config_find(struct pci_dev **isa)
>  {
>  	struct via_isa_bridge *via_config;
> @@ -372,6 +388,7 @@ static const struct ide_port_ops via_por
>  	.set_pio_mode		= via_set_pio_mode,
>  	.set_dma_mode		= via_set_drive,
>  	.cable_detect		= via82cxxx_cable_detect,
> +	.udma_filter		= via_udma_filter,
>  };
>  
>  static const struct ide_port_info via82cxxx_chipset __devinitdata = {
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Oct. 11, 2011, 7:05 p.m.
From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Date: Tue, 11 Oct 2011 19:12:01 +0200

> Still valid for 3.1.  Dave, ping?

Feedback given and changes requested/required, patch was not updated
and resubmitted by the author:

http://patchwork.ozlabs.org/patch/38764/
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Cox - Oct. 11, 2011, 7:39 p.m.
On Tue, 11 Oct 2011 15:05:04 -0400 (EDT)
David Miller <davem@davemloft.net> wrote:

> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Date: Tue, 11 Oct 2011 19:12:01 +0200
> 
> > Still valid for 3.1.  Dave, ping?
> 
> Feedback given and changes requested/required, patch was not updated
> and resubmitted by the author:
> 
> http://patchwork.ozlabs.org/patch/38764/

The last state on this I saw it seemd that exactly one person had seen it
on exactly one machine ? Has that changed ?
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

Index: linux-2.6.31.6-fast/drivers/ide/via82cxxx.c
===================================================================
--- linux-2.6.31.6-fast.orig/drivers/ide/via82cxxx.c	2009-11-16 13:08:04.000000000 +0100
+++ linux-2.6.31.6-fast/drivers/ide/via82cxxx.c	2009-11-18 17:57:22.000000000 +0100
@@ -195,6 +195,22 @@  static void via_set_pio_mode(ide_drive_t
 	via_set_drive(drive, XFER_PIO_0 + pio);
 }
 
+static u8 via_udma_filter(ide_drive_t *drive)
+{
+	char *m = (char *)&drive->id[ATA_ID_PROD];
+
+	/*
+	 * Restrict UDMA for Transcend flash cards.
+	 * On VIA 33, UDMA locks up. On VIA 133, it works. I can't test other
+	 * controllers.
+	 */
+	if (!memcmp(m, "TS", 2) && strstr(m, "SSD") &&
+	    drive->hwif->ultra_mask == ATA_UDMA2)
+		return 0;
+
+	return drive->hwif->ultra_mask;
+}
+
 static struct via_isa_bridge *via_config_find(struct pci_dev **isa)
 {
 	struct via_isa_bridge *via_config;
@@ -372,6 +388,7 @@  static const struct ide_port_ops via_por
 	.set_pio_mode		= via_set_pio_mode,
 	.set_dma_mode		= via_set_drive,
 	.cable_detect		= via82cxxx_cable_detect,
+	.udma_filter		= via_udma_filter,
 };
 
 static const struct ide_port_info via82cxxx_chipset __devinitdata = {