ide: Fix ata_id_has_dword_io to return DWORD I/O support properly
PATCH 1/1 ide: Fix ata_id_has_dword_io to return DWORD I/O support properly
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.
--- include/linux/ata.h | 12 ++++++------ 1 files changed, 6 insertions(+), 6 deletions(-)
include/linux/ata.h +6 -6
@@ -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 ; }static inline int ata_id_has_unload (const u16 *id)
Signature · 1 line
1.5.6.3
Quoted 19 lines · Mario Schwalbe
This patch fixes
ata_id_has_dword_ioto 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 <[email protected]>--- include/linux/ata.h | 12 ++++++------ 1 files changed, 6 insertions(+), 6 deletions(-)include/linux/ata.h +0
-2@@ -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 )
include/linux/ata.h static inline int ata_id_has_tpm(const u16 *id) · line 708
Quoted 10 lines
include/linux/ata.h +6
-4-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
Hello.
Quoted 27 lines · Jeff Garzik
Quoted 16 lines
This patch fixes
ata_id_has_dword_ioto 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 <[email protected]>include/linux/ata.h +0
-2@@ -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 )include/linux/ata.h static inline int ata_id_has_tpm(const u16 *id) · line 708
Quoted 10 lines
include/linux/ata.h +6
-4-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 ;
Quoted 1 line
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...
Quoted 1 line
Jeff
MBR, Sergei
Quoted 31 lines · Sergei Shtylyov
Hello.
Quoted 26 lines · Jeff Garzik
Quoted 26 lines
This patch fixes
ata_id_has_dword_ioto 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 <[email protected]>include/linux/ata.h +6
-6@@ -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 ;
Quoted 6 lines
Quoted 1 line
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
Quoted 6 lines
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_qdiandpata_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.
Quoted 3 lines
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
Quoted 10 lines · Alan Cox
Quoted 6 lines
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_qdiandpata_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.
Quoted 5 lines
Quoted 3 lines
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.
Quoted 3 lines
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_mumblestuff 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
Quoted 2 lines
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.
Quoted 10 lines · Alan Cox
Quoted 2 lines
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
Quoted 3 lines
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
Hello.
Quoted 4 lines · Alan Cox
Quoted 4 lines
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_qdiandpata_winbond) to use dword IO on >= ATA-2 devices.
Quoted 8 lines
Quoted 3 lines
[ 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...
Quoted 7 lines
Quoted 4 lines
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...
Quoted 3 lines
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.
Quoted 2 lines
those drivers not as
ata_has_mumblestuff as its not ATA - eide_* maybe.
Indeed, already ATA-1 says that the word 48 is "includedfor backwards
compatible VU use".
Quoted 2 lines
Alan
MBR, Sergei
Quoted 6 lines · I just
Quoted 3 lines
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
Quoted 1 line
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)
Quoted 10 lines · Alan Cox
Quoted 1 line
This seems like a risky assumption...
Its wrong on various counts
-
ata_id_major_versioncan'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 driveExcept 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?
Hello.
Quoted 4 lines · Robert Hancock
Quoted 4 lines
Quoted 1 line
This seems like a risky assumption...
Its wrong on various counts
Quoted 8 lines
Quoted 6 lines
-
ata_id_major_versioncan'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 driveExcept 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
Quoted 8 lines · Hello, I wrote:
Quoted 7 lines
Quoted 7 lines
Quoted 1 line
This seems like a risky assumption...
Its wrong on various counts
-
ata_id_major_versioncan'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
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
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