Message ID | 20091130132248.27236.35287.stgit@localhost.localdomain |
---|---|
State | Not Applicable |
Delegated to: | David Miller |
Headers | show |
Hello. Alan Cox wrote: > Bartlomiej pointed out that while this got fixed in the old driver whoever > did it didn't port it across. > Signed-off-by: Alan Cox <alan@linux.intel.com> [...] > +static int mwdma_clip_to_pio(struct ata_device *adev) > +{ > + const int mwdma_to_pio[3] = { > + XFER_PIO_0, XFER_PIO_3, XFER_PIO_4 > + }; > + return min(mwdma_to_pio[adev->dma_mode - XFER_MW_DMA_0], > + adev->pio_mode - XFER_PIO_0); > +} You call min() on uncomparables, i.e. mwdma_to_pio[] contains XFER_PIO_* and adev->pio_mode - XFER_PIO_0 yields you a mode number. Thus the second argument will always "win" as a minimal one. MBR, Sergei -- 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 Monday 07 December 2009 02:26:00 pm Sergei Shtylyov wrote: > Hello. > > Alan Cox wrote: > > > Bartlomiej pointed out that while this got fixed in the old driver whoever > > did it didn't port it across. > > > Signed-off-by: Alan Cox <alan@linux.intel.com> > > [...] > > > +static int mwdma_clip_to_pio(struct ata_device *adev) > > +{ > > + const int mwdma_to_pio[3] = { > > + XFER_PIO_0, XFER_PIO_3, XFER_PIO_4 > > + }; > > + return min(mwdma_to_pio[adev->dma_mode - XFER_MW_DMA_0], > > + adev->pio_mode - XFER_PIO_0); > > +} > > You call min() on uncomparables, i.e. mwdma_to_pio[] contains XFER_PIO_* > and adev->pio_mode - XFER_PIO_0 yields you a mode number. Thus the second > argument will always "win" as a minimal one. There are more issues with the patch related to mwdma_clip_to_pio(). The function can return values between 0 and 4 which obviously won't work well for the new code below for values > 2 (i.e. resulting in out-of-bounds array access for the common-case of dev->pio_mode == 4). @@ -400,21 +439,25 @@ static void sis_100_set_dmamode (struct ata_port *ap, struct ata_device *adev) struct pci_dev *pdev = to_pci_dev(ap->host->dev); int speed = adev->dma_mode - XFER_MW_DMA_0; int drive_pci = sis_old_port_base(adev); - u8 timing; + u16 timing; - const u8 udma_bits[] = { 0x8B, 0x87, 0x85, 0x83, 0x82, 0x81}; + const u16 udma_bits[] = { + 0x8B00, 0x8700, 0x8500, 0x8300, 0x8200, 0x8100}; + const u8 mwdma_bits[] = { 0x08, 0x32, 0x31 }; - pci_read_config_byte(pdev, drive_pci + 1, &timing); + pci_read_config_word(pdev, drive_pci, &timing); if (adev->dma_mode < XFER_UDMA_0) { - /* NOT SUPPORTED YET: NEED DATA SHEET. DITTO IN OLD DRIVER */ + speed = mwdma_clip_to_pio(adev); + timing &= ~0x80FF; + timing |= mwdma_bits[speed]; } else { /* Bit 7 is UDMA on/off, bit 0-3 are cycle time */ speed = adev->dma_mode - XFER_UDMA_0; - timing &= ~0x8F; + timing &= ~0x8F00; timing |= udma_bits[speed]; } - pci_write_config_byte(pdev, drive_pci + 1, timing); + pci_write_config_word(pdev, drive_pci, timing); } /** @@ -434,21 +477,26 @@ static void sis_133_early_set_dmamode (struct ata_port *ap, struct ata_device *a struct pci_dev *pdev = to_pci_dev(ap->host->dev); int speed = adev->dma_mode - XFER_MW_DMA_0; int drive_pci = sis_old_port_base(adev); - u8 timing; - /* Low 4 bits are timing */ - static const u8 udma_bits[] = { 0x8F, 0x8A, 0x87, 0x85, 0x83, 0x82, 0x81}; + u16 timing; + /* Bits 15-12 are timing */ + static const u16 udma_bits[] = { + 0x8F00, 0x8A00, 0x8700, 0x8500, 0x8300, 0x8200, 0x8100 + }; + static const u8 mwdma_bits[] = { 0x08, 0x32, 0x31 }; - pci_read_config_byte(pdev, drive_pci + 1, &timing); + pci_read_config_word(pdev, drive_pci, &timing); if (adev->dma_mode < XFER_UDMA_0) { - /* NOT SUPPORTED YET: NEED DATA SHEET. DITTO IN OLD DRIVER */ + speed = mwdma_clip_to_pio(adev); + timing &= ~0x80FF; + timing = mwdma_bits[speed]; } else { /* Bit 7 is UDMA on/off, bit 0-3 are cycle time */ speed = adev->dma_mode - XFER_UDMA_0; - timing &= ~0x8F; + timing &= ~0x8F00; timing |= udma_bits[speed]; } - pci_write_config_byte(pdev, drive_pci + 1, timing); + pci_write_config_word(pdev, drive_pci, timing); } /** -- Bartlomiej Zolnierkiewicz -- 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 Monday 07 December 2009 04:05:28 pm Bartlomiej Zolnierkiewicz wrote:
> array access for the common-case of dev->pio_mode == 4).
s/4/XFER_PIO_4/
[ Looking at the buggy code is bad for your health. ]
--
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 Monday 07 December 2009 04:05:28 pm Bartlomiej Zolnierkiewicz wrote: > On Monday 07 December 2009 02:26:00 pm Sergei Shtylyov wrote: > > Hello. > > > > Alan Cox wrote: > > > > > Bartlomiej pointed out that while this got fixed in the old driver whoever > > > did it didn't port it across. > > > > > Signed-off-by: Alan Cox <alan@linux.intel.com> > > > > [...] > > > > > +static int mwdma_clip_to_pio(struct ata_device *adev) > > > +{ > > > + const int mwdma_to_pio[3] = { > > > + XFER_PIO_0, XFER_PIO_3, XFER_PIO_4 > > > + }; > > > + return min(mwdma_to_pio[adev->dma_mode - XFER_MW_DMA_0], > > > + adev->pio_mode - XFER_PIO_0); > > > +} > > > > You call min() on uncomparables, i.e. mwdma_to_pio[] contains XFER_PIO_* > > and adev->pio_mode - XFER_PIO_0 yields you a mode number. Thus the second > > argument will always "win" as a minimal one. > > There are more issues with the patch related to mwdma_clip_to_pio(). > > The function can return values between 0 and 4 which obviously won't work > well for the new code below for values > 2 (i.e. resulting in out-of-bounds Fortunately above bugs won't be triggered because the patch forgot to update MWDMA masks.. ;) What is even more interesting is that the driver currently also lacks support for MWDMA modes on UDMA66 chips (though it was implemented in the driver). static const struct ata_port_info sis_info66 = { .flags = ATA_FLAG_SLAVE_POSS, .pio_mask = ATA_PIO4, /* No MWDMA */ .udma_mask = ATA_UDMA4, .port_ops = &sis_66_ops, }; static const struct ata_port_info sis_info100 = { .flags = ATA_FLAG_SLAVE_POSS, .pio_mask = ATA_PIO4, /* No MWDMA */ .udma_mask = ATA_UDMA5, .port_ops = &sis_100_ops, }; static const struct ata_port_info sis_info100_early = { .flags = ATA_FLAG_SLAVE_POSS, .pio_mask = ATA_PIO4, /* No MWDMA */ .udma_mask = ATA_UDMA5, .port_ops = &sis_66_ops, }; static const struct ata_port_info sis_info133 = { .flags = ATA_FLAG_SLAVE_POSS, .pio_mask = ATA_PIO4, /* No MWDMA */ .udma_mask = ATA_UDMA6, .port_ops = &sis_133_ops, }; const struct ata_port_info sis_info133_for_sata = { .flags = ATA_FLAG_SLAVE_POSS | ATA_FLAG_SRST, .pio_mask = ATA_PIO4, /* No MWDMA */ .udma_mask = ATA_UDMA6, .port_ops = &sis_133_for_sata_ops, }; static const struct ata_port_info sis_info133_early = { .flags = ATA_FLAG_SLAVE_POSS, .pio_mask = ATA_PIO4, /* No MWDMA */ .udma_mask = ATA_UDMA6, .port_ops = &sis_133_early_ops, }; -- Bartlomiej Zolnierkiewicz -- 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 Monday 07 December 2009 04:36:31 pm Bartlomiej Zolnierkiewicz wrote: > On Monday 07 December 2009 04:05:28 pm Bartlomiej Zolnierkiewicz wrote: > > On Monday 07 December 2009 02:26:00 pm Sergei Shtylyov wrote: > > > Hello. > > > > > > Alan Cox wrote: > > > > > > > Bartlomiej pointed out that while this got fixed in the old driver whoever > > > > did it didn't port it across. > > > > > > > Signed-off-by: Alan Cox <alan@linux.intel.com> > > > > > > [...] One more thing: @@ -479,13 +527,12 @@ static void sis_133_set_dmamode (struct ata_port *ap, struct ata_device *adev) if (reg54 & 0x40000000) port = 0x70; port += (8 * ap->port_no) + (4 * adev->devno); - pci_read_config_dword(pdev, port, &t1); if (adev->dma_mode < XFER_UDMA_0) { - t1 &= ~0x00000004; - /* FIXME: need data sheet to add MWDMA here. Also lacking on - ide/pci driver */ + speed = mwdma_clip_to_pio(adev); + sis_133_do_piomode(ap, adev, speed); + t1 &= ~4; /* UDMA off */ } else { speed = adev->dma_mode - XFER_UDMA_0; /* if & 8 no UDMA133 - need info for ... */ This way of doing things results in under-clocked timings for MWDMA0 mode, (they're not the same as ones for PIO0 mode), old driver gets it correctly. -- Bartlomiej Zolnierkiewicz -- 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 Monday 07 December 2009 04:05:28 pm Bartlomiej Zolnierkiewicz wrote: > On Monday 07 December 2009 02:26:00 pm Sergei Shtylyov wrote: > > Hello. > > > > Alan Cox wrote: > > > > > Bartlomiej pointed out that while this got fixed in the old driver whoever > > > did it didn't port it across. > > > > > Signed-off-by: Alan Cox <alan@linux.intel.com> > > > > [...] > > > > > +static int mwdma_clip_to_pio(struct ata_device *adev) > > > +{ > > > + const int mwdma_to_pio[3] = { > > > + XFER_PIO_0, XFER_PIO_3, XFER_PIO_4 > > > + }; > > > + return min(mwdma_to_pio[adev->dma_mode - XFER_MW_DMA_0], > > > + adev->pio_mode - XFER_PIO_0); > > > +} > > > > You call min() on uncomparables, i.e. mwdma_to_pio[] contains XFER_PIO_* > > and adev->pio_mode - XFER_PIO_0 yields you a mode number. Thus the second > > argument will always "win" as a minimal one. > > There are more issues with the patch related to mwdma_clip_to_pio(). Hmm.. why cannot ->qc_issue be used here (like in all other host drivers with shared PIO/MWDMA timings) instead of some new 'clipping'? ->qc_issue is the standard way to do it, takes care of more corner cases and would also allow for faster MWDMA0 timings. -- Bartlomiej Zolnierkiewicz -- 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
> Hmm.. why cannot ->qc_issue be used here (like in all other host drivers with > shared PIO/MWDMA timings) instead of some new 'clipping'? Agreed it would probably work providing the 8bit timings are not affected by any of this. > ->qc_issue is the standard way to do it, takes care of more corner cases > and would also allow for faster MWDMA0 timings. All yours.. just revert the buggy patch and fix it. I won't have time to look at it until next year and I freecycled all my SiS hardware some time ago. Alan -- 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 Monday 07 December 2009 06:53:14 pm Alan Cox wrote: > > Hmm.. why cannot ->qc_issue be used here (like in all other host drivers with > > shared PIO/MWDMA timings) instead of some new 'clipping'? > > Agreed it would probably work providing the 8bit timings are not affected > by any of this. If 8bit timings are a problem ->bmdma_{start,stop} approach can be used instead (as done for similar reason in it821x IIRC). > > ->qc_issue is the standard way to do it, takes care of more corner cases > > and would also allow for faster MWDMA0 timings. > > All yours.. just revert the buggy patch and fix it. I won't have time to > look at it until next year and I freecycled all my SiS hardware some time > ago. Thank you but this is quite a bit of work and I'm not interested personally. -- Bartlomiej Zolnierkiewicz -- 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
diff --git a/drivers/ata/pata_sis.c b/drivers/ata/pata_sis.c index 488e77b..d70ecec 100644 --- a/drivers/ata/pata_sis.c +++ b/drivers/ata/pata_sis.c @@ -252,24 +252,25 @@ static void sis_100_set_piomode (struct ata_port *ap, struct ata_device *adev) } /** - * sis_133_set_piomode - Initialize host controller PATA PIO timings + * sis_133_do_piomode - Initialize host controller PATA PIO/DMA timings * @ap: Port whose timings we are configuring * @adev: Device we are configuring for. * * Set PIO mode for device, in host controller PCI config space. This - * function handles PIO set up for the later ATA133 devices. + * function handles PIO set up for the later ATA133 devices. The same + * timings are used for MWDMA. * * LOCKING: * None (inherited from caller). */ -static void sis_133_set_piomode (struct ata_port *ap, struct ata_device *adev) +static void sis_133_do_piomode(struct ata_port *ap, struct ata_device *adev, + int speed) { struct pci_dev *pdev = to_pci_dev(ap->host->dev); int port = 0x40; u32 t1; u32 reg54; - int speed = adev->pio_mode - XFER_PIO_0; const u32 timing133[] = { 0x28269000, /* Recovery << 24 | Act << 16 | Ini << 12 */ @@ -305,6 +306,42 @@ static void sis_133_set_piomode (struct ata_port *ap, struct ata_device *adev) } /** + * sis_133_set_piomode - Initialize host controller PATA PIO timings + * @ap: Port whose timings we are configuring + * @adev: Device we are configuring for. + * + * Set PIO mode for device, in host controller PCI config space. This + * function handles PIO set up for the later ATA133 devices. + * + * LOCKING: + * None (inherited from caller). + */ + +static void sis_133_set_piomode (struct ata_port *ap, struct ata_device *adev) +{ + + sis_133_do_piomode(ap, adev, adev->pio_mode - XFER_PIO_0); +} + +/** + * mwdma_clip_to_pio - clip MWDMA mode + * @adev: device + * + * As the SiS shared MWDMA and PIO timings we must program the equivalent + * PIO timing for the MWDMA mode but we must not program one higher than + * the permitted PIO timing of the device. + */ + +static int mwdma_clip_to_pio(struct ata_device *adev) +{ + const int mwdma_to_pio[3] = { + XFER_PIO_0, XFER_PIO_3, XFER_PIO_4 + }; + return min(mwdma_to_pio[adev->dma_mode - XFER_MW_DMA_0], + adev->pio_mode - XFER_PIO_0); +} + +/** * sis_old_set_dmamode - Initialize host controller PATA DMA timings * @ap: Port whose timings we are configuring * @adev: Device to program @@ -332,6 +369,7 @@ static void sis_old_set_dmamode (struct ata_port *ap, struct ata_device *adev) if (adev->dma_mode < XFER_UDMA_0) { /* bits 3-0 hold recovery timing bits 8-10 active timing and the higher bits are dependant on the device */ + speed = mwdma_clip_to_pio(adev); timing &= ~0x870F; timing |= mwdma_bits[speed]; } else { @@ -372,6 +410,7 @@ static void sis_66_set_dmamode (struct ata_port *ap, struct ata_device *adev) if (adev->dma_mode < XFER_UDMA_0) { /* bits 3-0 hold recovery timing bits 8-10 active timing and the higher bits are dependant on the device, bit 15 udma */ + speed = mwdma_clip_to_pio(adev); timing &= ~0x870F; timing |= mwdma_bits[speed]; } else { @@ -389,7 +428,7 @@ static void sis_66_set_dmamode (struct ata_port *ap, struct ata_device *adev) * @adev: Device to program * * Set UDMA/MWDMA mode for device, in host controller PCI config space. - * Handles UDMA66 and early UDMA100 devices. + * Handles later UDMA100 devices. * * LOCKING: * None (inherited from caller). @@ -400,21 +439,25 @@ static void sis_100_set_dmamode (struct ata_port *ap, struct ata_device *adev) struct pci_dev *pdev = to_pci_dev(ap->host->dev); int speed = adev->dma_mode - XFER_MW_DMA_0; int drive_pci = sis_old_port_base(adev); - u8 timing; + u16 timing; - const u8 udma_bits[] = { 0x8B, 0x87, 0x85, 0x83, 0x82, 0x81}; + const u16 udma_bits[] = { + 0x8B00, 0x8700, 0x8500, 0x8300, 0x8200, 0x8100}; + const u8 mwdma_bits[] = { 0x08, 0x32, 0x31 }; - pci_read_config_byte(pdev, drive_pci + 1, &timing); + pci_read_config_word(pdev, drive_pci, &timing); if (adev->dma_mode < XFER_UDMA_0) { - /* NOT SUPPORTED YET: NEED DATA SHEET. DITTO IN OLD DRIVER */ + speed = mwdma_clip_to_pio(adev); + timing &= ~0x80FF; + timing |= mwdma_bits[speed]; } else { /* Bit 7 is UDMA on/off, bit 0-3 are cycle time */ speed = adev->dma_mode - XFER_UDMA_0; - timing &= ~0x8F; + timing &= ~0x8F00; timing |= udma_bits[speed]; } - pci_write_config_byte(pdev, drive_pci + 1, timing); + pci_write_config_word(pdev, drive_pci, timing); } /** @@ -434,21 +477,26 @@ static void sis_133_early_set_dmamode (struct ata_port *ap, struct ata_device *a struct pci_dev *pdev = to_pci_dev(ap->host->dev); int speed = adev->dma_mode - XFER_MW_DMA_0; int drive_pci = sis_old_port_base(adev); - u8 timing; - /* Low 4 bits are timing */ - static const u8 udma_bits[] = { 0x8F, 0x8A, 0x87, 0x85, 0x83, 0x82, 0x81}; + u16 timing; + /* Bits 15-12 are timing */ + static const u16 udma_bits[] = { + 0x8F00, 0x8A00, 0x8700, 0x8500, 0x8300, 0x8200, 0x8100 + }; + static const u8 mwdma_bits[] = { 0x08, 0x32, 0x31 }; - pci_read_config_byte(pdev, drive_pci + 1, &timing); + pci_read_config_word(pdev, drive_pci, &timing); if (adev->dma_mode < XFER_UDMA_0) { - /* NOT SUPPORTED YET: NEED DATA SHEET. DITTO IN OLD DRIVER */ + speed = mwdma_clip_to_pio(adev); + timing &= ~0x80FF; + timing = mwdma_bits[speed]; } else { /* Bit 7 is UDMA on/off, bit 0-3 are cycle time */ speed = adev->dma_mode - XFER_UDMA_0; - timing &= ~0x8F; + timing &= ~0x8F00; timing |= udma_bits[speed]; } - pci_write_config_byte(pdev, drive_pci + 1, timing); + pci_write_config_word(pdev, drive_pci, timing); } /** @@ -479,13 +527,12 @@ static void sis_133_set_dmamode (struct ata_port *ap, struct ata_device *adev) if (reg54 & 0x40000000) port = 0x70; port += (8 * ap->port_no) + (4 * adev->devno); - pci_read_config_dword(pdev, port, &t1); if (adev->dma_mode < XFER_UDMA_0) { - t1 &= ~0x00000004; - /* FIXME: need data sheet to add MWDMA here. Also lacking on - ide/pci driver */ + speed = mwdma_clip_to_pio(adev); + sis_133_do_piomode(ap, adev, speed); + t1 &= ~4; /* UDMA off */ } else { speed = adev->dma_mode - XFER_UDMA_0; /* if & 8 no UDMA133 - need info for ... */
Bartlomiej pointed out that while this got fixed in the old driver whoever did it didn't port it across. Signed-off-by: Alan Cox <alan@linux.intel.com> --- drivers/ata/pata_sis.c | 91 ++++++++++++++++++++++++++++++++++++------------ 1 files changed, 69 insertions(+), 22 deletions(-) -- 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