[PATCH] libata: share PIO limits among devices sharing a channel

[PATCH] libata: share PIO limits among devices sharing a channel

[Tejun Heo]

PIO xfermask limits should be shared by all devices on the same
channel to avoid violating device selection timing.  libata used to
guarantee this at core level but I mistakenly dropped the code during
conversion to new EH.  This patch revives that guarantee.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
Jeff, IMHO, this is a regression and should go into #upstream-fixes.
Thanks.

 drivers/ata/libata-core.c |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Index: work/drivers/ata/libata-core.c
===================================================================
--- work.orig/drivers/ata/libata-core.c
+++ work/drivers/ata/libata-core.c
@@ -3390,6 +3390,7 @@ static void ata_dev_xfermask(struct ata_
 	struct ata_port *ap = dev->ap;
 	struct ata_host *host = ap->host;
 	unsigned long xfer_mask;
+	int i;
 
 	/* controller modes available */
 	xfer_mask = ata_pack_xfermask(ap->pio_mask,
@@ -3412,6 +3413,22 @@ static void ata_dev_xfermask(struct ata_
 				       dev->mwdma_mask, dev->udma_mask);
 	xfer_mask &= ata_id_xfermask(dev->id);
 
+	/* PIO xfermask limits are shared by all devices on the same
+	 * channel to avoid violating device selection timing.
+	 */
+	for (i = 0; i < ATA_MAX_DEVICES; i++) {
+		struct ata_device *d = &ap->device[i];
+		unsigned int pio_mask;
+
+		if (ata_dev_absent(d))
+			continue;
+
+		ata_unpack_xfermask(ata_id_xfermask(d->id),
+				    &pio_mask, NULL, NULL);
+		pio_mask &= d->pio_mask;
+		xfer_mask &= ata_pack_xfermask(pio_mask, UINT_MAX, UINT_MAX);
+	}
+
 	/*
 	 *	CFA Advanced TrueIDE timings are not allowed on a shared
 	 *	cable

Re: [PATCH] libata: share PIO limits among devices sharing a channel

[Alan]

On Thu, 25 Jan 2007 20:29:47 +0900
Tejun Heo <htejun@gmail.com> wrote:

> PIO xfermask limits should be shared by all devices on the same
> channel to avoid violating device selection timing.  libata used to

NAK, this is totally wrong

> +	/* PIO xfermask limits are shared by all devices on the same
> +	 * channel to avoid violating device selection timing.
> +	 */
> +	for (i = 0; i < ATA_MAX_DEVICES; i++) {
> +		struct ata_device *d = &ap->device[i];
> +		unsigned int pio_mask;
> +
> +		if (ata_dev_absent(d))
> +			continue;
> +
> +		ata_unpack_xfermask(ata_id_xfermask(d->id),
> +				    &pio_mask, NULL, NULL);
> +		pio_mask &= d->pio_mask;
> +		xfer_mask &= ata_pack_xfermask(pio_mask, UINT_MAX, UINT_MAX);
> +	}

NAK

This "guarantee" was deliberately removed long ago and is completely
bogus.

The good ATA chipsets do not suffer from selection timing limits of this
form. The less smart ones do and the drivers correctly merge the timing
values, including a whole chunk of functionality in the ata_timing
interface to get it right when doing DMA modes.

Adding this patch is the regression. Even the ancient drivers/ide code
does this properly.

Alan

Re: [PATCH] libata: share PIO limits among devices sharing a channel

[Jeff Garzik]

Alan wrote:
> On Thu, 25 Jan 2007 20:29:47 +0900
> Tejun Heo <htejun@gmail.com> wrote:
> 
>> PIO xfermask limits should be shared by all devices on the same
>> channel to avoid violating device selection timing.  libata used to
> 
> NAK, this is totally wrong
> 
>> +	/* PIO xfermask limits are shared by all devices on the same
>> +	 * channel to avoid violating device selection timing.
>> +	 */
>> +	for (i = 0; i < ATA_MAX_DEVICES; i++) {
>> +		struct ata_device *d = &ap->device[i];
>> +		unsigned int pio_mask;
>> +
>> +		if (ata_dev_absent(d))
>> +			continue;
>> +
>> +		ata_unpack_xfermask(ata_id_xfermask(d->id),
>> +				    &pio_mask, NULL, NULL);
>> +		pio_mask &= d->pio_mask;
>> +		xfer_mask &= ata_pack_xfermask(pio_mask, UINT_MAX, UINT_MAX);
>> +	}
> 
> NAK
> 
> This "guarantee" was deliberately removed long ago and is completely
> bogus.
> 
> The good ATA chipsets do not suffer from selection timing limits of this
> form. The less smart ones do and the drivers correctly merge the timing

Agreed.

I'm curious what the motivation of this patch was?

	Jeff

Re: [PATCH] libata: share PIO limits among devices sharing a channel

[Tejun Heo]

Jeff Garzik wrote:
> Alan wrote:
>> On Thu, 25 Jan 2007 20:29:47 +0900
>> Tejun Heo <htejun@gmail.com> wrote:
>>
>>> PIO xfermask limits should be shared by all devices on the same
>>> channel to avoid violating device selection timing.  libata used to
>>
>> NAK, this is totally wrong

Eek.. You actually said this is/was documented and relied upon all over.

http://thread.gmane.org/gmane.linux.ide/13184/focus=14486

(searching mailboxes more...)  Oh, and the dropping of common PIO mode
selection was agreed upon by Jeff and Alan.

One way or the other, this only affects PATA devices and as long as all
PATA LLDs guarantee that selection timing isn't violated, this can go.
I thought PATA drivers still relied on this common denominator PIO mode
configuration.  I think it's Alan's call.

> I'm curious what the motivation of this patch was?

Hope my intentions are explained.

Thanks.

-- 
tejun

Re: [PATCH] libata: share PIO limits among devices sharing a channel

[Alan]

> >> NAK, this is totally wrong
> 
> Eek.. You actually said this is/was documented and relied upon all over.
> 
> http://thread.gmane.org/gmane.linux.ide/13184/focus=14486

What I said (well tried to say since it was apparently vague and caused
confusion) was relied upon was the hardware being in PIO0 during probe
(because we don't call the PIO setup methods during probe/reset) and that
we decide which modes both devices are using *before* we set either of
them (which the current code appears to do correctly). This is because
the device may need to know both to apply limits - such as merging
address setup time.

> 
> (searching mailboxes more...)  Oh, and the dropping of common PIO mode
> selection was agreed upon by Jeff and Alan.
> 
> One way or the other, this only affects PATA devices and as long as all
> PATA LLDs guarantee that selection timing isn't violated, this can go.
> I thought PATA drivers still relied on this common denominator PIO mode
> configuration.  I think it's Alan's call.
> 
> > I'm curious what the motivation of this patch was?
> 
> Hope my intentions are explained.

Yep. I think it's a simple case of confusion about the meaning of an
email.

Alan

Re: [PATCH] libata: share PIO limits among devices sharing a channel

[Tejun Heo]

Alan wrote:
>>>> NAK, this is totally wrong
>> Eek.. You actually said this is/was documented and relied upon all over.
>>
>> http://thread.gmane.org/gmane.linux.ide/13184/focus=14486
> 
> What I said (well tried to say since it was apparently vague and caused
> confusion) was relied upon was the hardware being in PIO0 during probe
> (because we don't call the PIO setup methods during probe/reset) and that
> we decide which modes both devices are using *before* we set either of
> them (which the current code appears to do correctly). This is because
> the device may need to know both to apply limits - such as merging
> address setup time.

I see.

>>> I'm curious what the motivation of this patch was?
>> Hope my intentions are explained.
> 
> Yep. I think it's a simple case of confusion about the meaning of an
> email.

Okay, then.  We can just ignore this whole thread.  Thanks for clarifying.

-- 
tejun

Re: [PATCH] libata: share PIO limits among devices sharing a channel

[Sergei Shtylyov]

Alan wrote:

>>PIO xfermask limits should be shared by all devices on the same
>>channel to avoid violating device selection timing.  libata used to

    This is not a good way to deal with this. Only command block (8-bit) 
timings  should be the same for both drives on channel, data register (16-bit) 
timings may be different.

> NAK, this is totally wrong

>>+	/* PIO xfermask limits are shared by all devices on the same
>>+	 * channel to avoid violating device selection timing.
>>+	 */
>>+	for (i = 0; i < ATA_MAX_DEVICES; i++) {
>>+		struct ata_device *d = &ap->device[i];
>>+		unsigned int pio_mask;
>>+
>>+		if (ata_dev_absent(d))
>>+			continue;
>>+
>>+		ata_unpack_xfermask(ata_id_xfermask(d->id),
>>+				    &pio_mask, NULL, NULL);
>>+		pio_mask &= d->pio_mask;
>>+		xfer_mask &= ata_pack_xfermask(pio_mask, UINT_MAX, UINT_MAX);
>>+	}

> NAK

> This "guarantee" was deliberately removed long ago and is completely
> bogus.

    I'd agree...

> The good ATA chipsets do not suffer from selection timing limits of this
> form. The less smart ones do and the drivers correctly merge the timing
> values, including a whole chunk of functionality in the ata_timing
> interface to get it right when doing DMA modes.

    I need to have a look at this (when I have time :-)...

> Adding this patch is the regression. Even the ancient drivers/ide code
> does this properly.

    Not really, at least not all drivers.  Namely, hpt366.c (still) doesn't 
merge 8-bit timings (maybe this is handled in hardware but the datasheets 
don't tell about it then) -- I need to look at fixing this... Well, it was 
even worse before "the grand rewrite" since even setting UltraDMA modes 
changed PIO timings (both 8- and 16-bit) to match PIO4 -- piix/slc90e66 are 
still doing this kind of crap (I'm going to fix this at last).

MBR, Sergei

Re: [PATCH] libata: share PIO limits among devices sharing a channel

[Alan]

>     Not really, at least not all drivers.  Namely, hpt366.c (still) doesn't 
> merge 8-bit timings (maybe this is handled in hardware but the datasheets 
> don't tell about it then) -- I need to look at fixing this... Well, it was 

I've never been able to find out - but HPT's own drivers don't seem to do
it under any OS so I assume its ok. The timing stuff should be pretty
much the same for both ide and libata as libata lifted most of the rather
nice ide-timing stuff directly from the old IDE layer.

Alan

Re: [PATCH] libata: share PIO limits among devices sharing a channel

[Sergei Shtylyov]

Hello.

Alan wrote:

>>    Not really, at least not all drivers.  Namely, hpt366.c (still) doesn't 
>>merge 8-bit timings (maybe this is handled in hardware but the datasheets 
>>don't tell about it then) -- I need to look at fixing this... Well, it was 

> I've never been able to find out - but HPT's own drivers don't seem to do
> it under any OS so I assume its ok.

    From their drivers/BIOSes/chips I can only assume their engeneers' serious 
brain damage and incompetence. :-)
    Well, Linux driver was hardly better.

> The timing stuff should be pretty
> much the same for both ide and libata as libata lifted most of the rather
> nice ide-timing stuff directly from the old IDE layer.

    Yeah, pata_hpt3*.c still have the same issues as the old hpt366 driver, 
namely changing PIO timings when setting DMA modes.

> Alan

WBR, Sergei