Re: [PATCH] ide: Fix ata_id_has_dword_io to return DWORD I/O support properly

Re: [PATCH] ide: Fix ata_id_has_dword_io to return DWORD I/O support properly

[Jeff Garzik]

Mario Schwalbe wrote:
> This patch fixes ata_id_has_dword_io to return 1 (supported) if
> the drive is compliant to ATA2 or newer and evaluates the config
> word for older drives.
> 
> Signed-off-by: Mario Schwalbe <schwalbe@inf.tu-dresden.de>
> ---
>  include/linux/ata.h |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/ata.h b/include/linux/ata.h
> index a53318b..d0d6a9c 100644
> --- a/include/linux/ata.h
> +++ b/include/linux/ata.h
> @@ -700,12 +700,12 @@ static inline int ata_id_has_tpm(const u16 *id)
> 
>  static inline int ata_id_has_dword_io(const u16 *id)
>  {
> -	/* ATA 8 reuses this flag for "trusted" computing */
> -	if (ata_id_major_version(id) > 7)
> -		return 0;
> -	if (id[ATA_ID_DWORD_IO] & (1 << 0))
> -		return 1;
> -	return 0;
> +	/* This flag is defined up to ATA1 and deprecated since then
> +	   (ATA 8 reuses this flag for "trusted" computing). */
> +	if (ata_id_major_version(id) <= 1)
> +		return (id[ATA_ID_DWORD_IO] & (1 << 0)) != 0;
> +	/* later revision drives support DWORD I/O just fine */
> +	return 1;


This seems like a risky assumption...

	Jeff

Re: [PATCH] ide: Fix ata_id_has_dword_io to return DWORD I/O support properly

[Sergei Shtylyov]

Hello.

Jeff Garzik wrote:

>> This patch fixes ata_id_has_dword_io to return 1 (supported) if
>> the drive is compliant to ATA2 or newer and evaluates the config
>> word for older drives.

>> Signed-off-by: Mario Schwalbe <schwalbe@inf.tu-dresden.de>

>> diff --git a/include/linux/ata.h b/include/linux/ata.h
>> index a53318b..d0d6a9c 100644
>> --- a/include/linux/ata.h
>> +++ b/include/linux/ata.h
>> @@ -700,12 +700,12 @@ static inline int ata_id_has_tpm(const u16 *id)

>>  static inline int ata_id_has_dword_io(const u16 *id)
>>  {
>> -    /* ATA 8 reuses this flag for "trusted" computing */
>> -    if (ata_id_major_version(id) > 7)
>> -        return 0;
>> -    if (id[ATA_ID_DWORD_IO] & (1 << 0))
>> -        return 1;
>> -    return 0;
>> +    /* This flag is defined up to ATA1 and deprecated since then
>> +       (ATA 8 reuses this flag for "trusted" computing). */
>> +    if (ata_id_major_version(id) <= 1)
>> +        return (id[ATA_ID_DWORD_IO] & (1 << 0)) != 0;
>> +    /* later revision drives support DWORD I/O just fine */
>> +    return 1;

> This seems like a risky assumption...

    Not at all, I think. It's total mystery how "DWORD I/O" could've been even 
(not) supported by a *drive* at all. Support of "DWORD I/O" is a property of 
the PATA controller. Frankly speaking, I don't think that this function is at 
all useful...

>     Jeff

MBR, Sergei

Re: [PATCH] ide: Fix ata_id_has_dword_io to return DWORD I/O support properly

[Bartlomiej Zolnierkiewicz]

On Thursday 18 December 2008, Sergei Shtylyov wrote:
> Hello.
> 
> Jeff Garzik wrote:
> 
> >> This patch fixes ata_id_has_dword_io to return 1 (supported) if
> >> the drive is compliant to ATA2 or newer and evaluates the config
> >> word for older drives.
> 
> >> Signed-off-by: Mario Schwalbe <schwalbe@inf.tu-dresden.de>
> 
> >> diff --git a/include/linux/ata.h b/include/linux/ata.h
> >> index a53318b..d0d6a9c 100644
> >> --- a/include/linux/ata.h
> >> +++ b/include/linux/ata.h
> >> @@ -700,12 +700,12 @@ static inline int ata_id_has_tpm(const u16 *id)
> 
> >>  static inline int ata_id_has_dword_io(const u16 *id)
> >>  {
> >> -    /* ATA 8 reuses this flag for "trusted" computing */
> >> -    if (ata_id_major_version(id) > 7)
> >> -        return 0;
> >> -    if (id[ATA_ID_DWORD_IO] & (1 << 0))
> >> -        return 1;
> >> -    return 0;
> >> +    /* This flag is defined up to ATA1 and deprecated since then
> >> +       (ATA 8 reuses this flag for "trusted" computing). */
> >> +    if (ata_id_major_version(id) <= 1)
> >> +        return (id[ATA_ID_DWORD_IO] & (1 << 0)) != 0;
> >> +    /* later revision drives support DWORD I/O just fine */
> >> +    return 1;
> 
> > This seems like a risky assumption...
> 
>     Not at all, I think. It's total mystery how "DWORD I/O" could've been even 
> (not) supported by a *drive* at all. Support of "DWORD I/O" is a property of 
> the PATA controller. Frankly speaking, I don't think that this function is at 
> all useful...

Indeed, this function can be probably removed altogether later...

OTOH the current patch is safe even for 2.6.28 (based on years of experience
with the check that we had in IDE subsystem) and will fix some libata drivers
(pata_legacy, pata_qdi and pata_winbond) to use dword IO on >= ATA-2 devices.

[ In reality this a regression fix for IDE -> libata conversion as it is a
  huge performance improvement for the above mentioned DMA-less drivers. ]

Jeff, I would like to merge it through IDE tree since the other patch depends
on it but if you want to go ahead and push it to Linus earlier feel free to
do it (or I can include it into the next IDE fixes pull request if you like).

Thanks,
Bart

Re: [PATCH] ide: Fix ata_id_has_dword_io to return DWORD I/O support properly

[Alan Cox]

> OTOH the current patch is safe even for 2.6.28 (based on years of experience
> with the check that we had in IDE subsystem) and will fix some libata drivers
> (pata_legacy, pata_qdi and pata_winbond) to use dword IO on >= ATA-2 devices.
> 
> [ In reality this a regression fix for IDE -> libata conversion as it is a
>   huge performance improvement for the above mentioned DMA-less drivers. ]

The 32bit PIO support is already queued up and went to Jeff a while ago
so thats all in hand - its btw a big win on some suprising chipset cases
including Intel ICH chipsets.

> Jeff, I would like to merge it through IDE tree since the other patch depends
> on it but if you want to go ahead and push it to Linus earlier feel free to
> do it (or I can include it into the next IDE fixes pull request if you like).

Its still broken. You cannot use the version check for versions below 3.
I remain unconvinced we should be looking at it anywhere except specific
pure ISA cycle pass through hardware and thus it belongs as a helper for
those drivers not as ata_has_mumble stuff as its not ATA - eide_* maybe.

Doubly complicating it is the small detail that ATA1 devices don't
all respond to IDENTIFY in the first place....

Alan

Re: [PATCH] ide: Fix ata_id_has_dword_io to return DWORD I/O support properly

[Bartlomiej Zolnierkiewicz]

On Thursday 18 December 2008, Alan Cox wrote:
> > OTOH the current patch is safe even for 2.6.28 (based on years of experience
> > with the check that we had in IDE subsystem) and will fix some libata drivers
> > (pata_legacy, pata_qdi and pata_winbond) to use dword IO on >= ATA-2 devices.
> > 
> > [ In reality this a regression fix for IDE -> libata conversion as it is a
> >   huge performance improvement for the above mentioned DMA-less drivers. ]
> 
> The 32bit PIO support is already queued up and went to Jeff a while ago
> so thats all in hand - its btw a big win on some suprising chipset cases
> including Intel ICH chipsets.

It is a not exactly the same issue as the pata_{legacy,qdi,winbond} one
discussed above.  Anyway cool to see another IDE -> libata regression fixed.

> > Jeff, I would like to merge it through IDE tree since the other patch depends
> > on it but if you want to go ahead and push it to Linus earlier feel free to
> > do it (or I can include it into the next IDE fixes pull request if you like).
> 
> Its still broken. You cannot use the version check for versions below 3.

Hmm, this doesn't seem to be a problem w.r.t. to the patch we are discussing
because in such case ata_id_major_version() will just return 0 and the check
will behave in the identical way as it was before the patch.

> I remain unconvinced we should be looking at it anywhere except specific
> pure ISA cycle pass through hardware and thus it belongs as a helper for
> those drivers not as ata_has_mumble stuff as its not ATA - eide_* maybe.

Sure, we can always improve things further later.  However Mario's patch
is _definitely_ an improvement over the _current_ code.  Don't you agree?

[ There are no alternative patches to consider currently so... ]

Thanks,
Bart

Re: [PATCH] ide: Fix ata_id_has_dword_io to return DWORD I/O support properly

[Alan Cox]

> Sure, we can always improve things further later.  However Mario's patch
> is _definitely_ an improvement over the _current_ code.  Don't you agree?

No actually I don't. The current head dev code checks ATA > 7 as the
indicator that this bit is used for treacherous computing. That means the
behaviour for all the older drives is clearly back compatible with old
Linux behaviour.

Doing it that way around means we don't have any worries about changing
existing behaviours.

Re: [PATCH] ide: Fix ata_id_has_dword_io to return DWORD I/O support properly

[Bartlomiej Zolnierkiewicz]

On Friday 19 December 2008, Alan Cox wrote:
> > Sure, we can always improve things further later.  However Mario's patch
> > is _definitely_ an improvement over the _current_ code.  Don't you agree?
> 
> No actually I don't. The current head dev code checks ATA > 7 as the
> indicator that this bit is used for treacherous computing. That means the
> behaviour for all the older drives is clearly back compatible with old
> Linux behaviour.
> 
> Doing it that way around means we don't have any worries about changing
> existing behaviours.

Please try to verify old Linux behaviour compatiblity statement with
some ATA-[2..7] devices...  They have old ATA-1 bit set to _zero_...

Mario: I don't have anymore time to spend on fixing libata problems so
I'll be leaving ata_id_has_dword_io() issues up to libata maintainers,
sorry for inconvenience!

Thanks,
Bart

Re: [PATCH] ide: Fix ata_id_has_dword_io to return DWORD I/O support properly

[Alan Cox]

> Mario: I don't have anymore time to spend on fixing libata problems so
> I'll be leaving ata_id_has_dword_io() issues up to libata maintainers,
> sorry for inconvenience!

Having reviewed the docs I plan to either delete it or rename it and make
it reflect that anything ATA* (controller or device) doesn't need it.

The only conceivable case we might want to check it is pata_legacy (and
its equivalents in old IDE)

Alan

Re: [PATCH] ide: Fix ata_id_has_dword_io to return DWORD I/O support properly

[Sergei Shtylyov]

Hello.

Alan Cox wrote:

>> OTOH the current patch is safe even for 2.6.28 (based on years of experience
>> with the check that we had in IDE subsystem) and will fix some libata drivers
>> (pata_legacy, pata_qdi and pata_winbond) to use dword IO on >= ATA-2 devices.
>>
>> [ In reality this a regression fix for IDE -> libata conversion as it is a
>>   huge performance improvement for the above mentioned DMA-less drivers. ]
>>     
>
> The 32bit PIO support is already queued up and went to Jeff a while ago
> so thats all in hand - its btw a big win on some suprising chipset cases
> including Intel ICH chipsets.
>   

   This is indeed strange...

>> Jeff, I would like to merge it through IDE tree since the other patch depends
>> on it but if you want to go ahead and push it to Linus earlier feel free to
>> do it (or I can include it into the next IDE fixes pull request if you like).
>>     
>
> Its still broken. You cannot use the version check for versions below 3.
>   

   The word is reserved since ATA-2, and ata_id_major_version() should 
be returning 0 for pre-ATA-3 drives, so I don't know what else can be 
done here...

> I remain unconvinced we should be looking at it anywhere except specific
> pure ISA cycle pass through hardware and thus it belongs as a helper for
>   

   I'm still not getting how drive can support or not support "DWORD 
I/O" -- you certainly can't have 32-bit I/O cycle on ISA (only on EISA) 
and you certainly cannot translate 32-bit cycle to the ATA bus. I 
remember I had some hypotheses before but they turned out to be 
inconsistent.

> those drivers not as ata_has_mumble stuff as its not ATA - eide_* maybe.
>   

   Indeed, already ATA-1 says that the word 48 is "includedfor backwards 
compatible VU use".

> Alan
>   

MBR, Sergei

Re: [PATCH] ide: Fix ata_id_has_dword_io to return DWORD I/O support properly

[Sergei Shtylyov]

Hello, I just wrote:

>> I remain unconvinced we should be looking at it anywhere except specific
>> pure ISA cycle pass through hardware and thus it belongs as a helper for
>>   
>
>   I'm still not getting how drive can support or not support "DWORD 
> I/O" -- you certainly can't have 32-bit I/O cycle on ISA (only on 
> EISA) and you certainly cannot translate 32-bit cycle to the ATA bus. 
> I remember I had some hypotheses before but they turned out to be 
> inconsistent.

   If only two back-to-back I/O cycles weren't providing enough recovery 
time...

MBR, Sergei

Re: [PATCH] ide: Fix ata_id_has_dword_io to return DWORD I/O support properly

[Alan Cox]

> This seems like a risky assumption...

Its wrong on various counts

- ata_id_major_version can't tell early ATA versions apart
- on anything later than the early ISA IDE paddles (the ones that
basically were just bus decoders) its invisible to the drive

Except for legacy ISA bus controllers (and even there it is questionable)
I would favour simply ignoring it.

The reverse half of the problem (the fact the ATA8 drafts reuse it and
can cause confusion if your device is treacherous computing crippled) is
IMHO just amusing 8)

Re: [PATCH] ide: Fix ata_id_has_dword_io to return DWORD I/O support properly

[Robert Hancock]

Alan Cox wrote:
>> This seems like a risky assumption...
> 
> Its wrong on various counts
> 
> - ata_id_major_version can't tell early ATA versions apart
> - on anything later than the early ISA IDE paddles (the ones that
> basically were just bus decoders) its invisible to the drive
> 
> Except for legacy ISA bus controllers (and even there it is questionable)
> I would favour simply ignoring it.

32-bit IO wouldn't work on any ISA controller, would it? What happens if 
you do 32-bit IO port access on something on the ISA bus?

Re: [PATCH] ide: Fix ata_id_has_dword_io to return DWORD I/O support properly

[Sergei Shtylyov]

Hello.

Robert Hancock wrote:

>>> This seems like a risky assumption...
>>
>> Its wrong on various counts
>>
>> - ata_id_major_version can't tell early ATA versions apart
>> - on anything later than the early ISA IDE paddles (the ones that
>> basically were just bus decoders) its invisible to the drive
>>
>> Except for legacy ISA bus controllers (and even there it is 
>> questionable)
>> I would favour simply ignoring it.
>
> 32-bit IO wouldn't work on any ISA controller, would it? What happens 
> if you do 32-bit IO port access on something on the ISA bus?

   TTBOM, depending on what's driven by device on -IOCS16, this will 
translate into 2, 3, or 4 cycles at the successive addresses. In case of 
the IDE data register, this should translate into one 16-bit cycle at 
0x1x0, one 8-bit cycle at 0x1x1, and one 8-bit cycle at 0x1x2 which is 
of course not what anybody would want...

WBR, Sergei

Re: [PATCH] ide: Fix ata_id_has_dword_io to return DWORD I/O support properly

[Sergei Shtylyov]

Hello, I wrote:

>>>> This seems like a risky assumption...

>>> Its wrong on various counts

>>> - ata_id_major_version can't tell early ATA versions apart
>>> - on anything later than the early ISA IDE paddles (the ones that
>>> basically were just bus decoders) its invisible to the drive

>>> Except for legacy ISA bus controllers (and even there it is 
>>> questionable) I would favour simply ignoring it.

>> 32-bit IO wouldn't work on any ISA controller, would it? What happens 
>> if you do 32-bit IO port access on something on the ISA bus?

>   TTBOM, depending on what's driven by device on -IOCS16, this will 
> translate into 2, 3, or 4 cycles at the successive addresses. In case of 
> the IDE data register, this should translate into one 16-bit cycle at 
> 0x1x0, one 8-bit cycle at 0x1x1, and one 8-bit cycle at 0x1x2 which is 
> of course not what anybody would want...

    The 8-bit cycles would be at ports 0x1x2 and 0x1x3, of course. :-)

MBR, Sergei