| Message ID | Pine.LNX.4.64.0911181151140.5068@hs20-bc2-1.build.redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
> 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
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
> > 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
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
> 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
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
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
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
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
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
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
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 = {