[PATCH] ata.h: Don't do a SATA check for 40wire_relaxed

[PATCH] ata.h: Don't do a SATA check for 40wire_relaxed

[Alan Cox]

Without the valid bits at least one set of TSScorp drives report 0 in
word 93 for PATA 40 wire, which we (and the specs) say actually means
SATA. (The SATA version seems to report 80 wire...)

Signed-off-by: Alan Cox <alan@redhat.com>

diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.24-rc2-mm1/include/linux/ata.h linux-2.6.24-rc2-mm1/include/linux/ata.h
--- linux.vanilla-2.6.24-rc2-mm1/include/linux/ata.h	2007-11-16 17:55:20.000000000 +0000
+++ linux-2.6.24-rc2-mm1/include/linux/ata.h	2007-11-16 18:42:20.000000000 +0000
@@ -560,8 +560,6 @@
 
 static inline int ata_drive_40wire_relaxed(const u16 *dev_id)
 {
-	if (ata_id_is_sata(dev_id))
-		return 0;	/* SATA */
 	if ((dev_id[93] & 0x2000) == 0x2000)
 		return 0;	/* 80 wire */
 	return 1;

Re: [PATCH] ata.h: Don't do a SATA check for 40wire_relaxed

[Jeff Garzik]

Alan Cox wrote:
> Without the valid bits at least one set of TSScorp drives report 0 in
> word 93 for PATA 40 wire, which we (and the specs) say actually means
> SATA. (The SATA version seems to report 80 wire...)
> 
> Signed-off-by: Alan Cox <alan@redhat.com>

SATA version on what controller?  Have you verified where the bridge is, 
if it's not reporting word93==0 ?

Is this for 2.6.24-rc, considering that we are late in -rc?

	Jeff

Re: [PATCH] ata.h: Don't do a SATA check for 40wire_relaxed

[Alan Cox]

On Fri, 23 Nov 2007 20:23:39 -0500
Jeff Garzik <jeff@garzik.org> wrote:

> Alan Cox wrote:
> > Without the valid bits at least one set of TSScorp drives report 0 in
> > word 93 for PATA 40 wire, which we (and the specs) say actually means
> > SATA. (The SATA version seems to report 80 wire...)
> > 
> > Signed-off-by: Alan Cox <alan@redhat.com>
> 
> SATA version on what controller?  Have you verified where the bridge is, 
> if it's not reporting word93==0 ?

PATA you see 0x0000 for 40 wire, 0x2000 for 80 wire (no valid bits), and
for SATA (no bridges or if present in the drive) 0x2000

Alan

Re: [PATCH] ata.h: Don't do a SATA check for 40wire_relaxed

[Jeff Garzik]

Alan Cox wrote:
> Without the valid bits at least one set of TSScorp drives report 0 in
> word 93 for PATA 40 wire, which we (and the specs) say actually means
> SATA. (The SATA version seems to report 80 wire...)
> 
> Signed-off-by: Alan Cox <alan@redhat.com>
> 
> diff -u --new-file --recursive --exclude-from /usr/src/exclude linux.vanilla-2.6.24-rc2-mm1/include/linux/ata.h linux-2.6.24-rc2-mm1/include/linux/ata.h
> --- linux.vanilla-2.6.24-rc2-mm1/include/linux/ata.h	2007-11-16 17:55:20.000000000 +0000
> +++ linux-2.6.24-rc2-mm1/include/linux/ata.h	2007-11-16 18:42:20.000000000 +0000
> @@ -560,8 +560,6 @@
>  
>  static inline int ata_drive_40wire_relaxed(const u16 *dev_id)
>  {
> -	if (ata_id_is_sata(dev_id))
> -		return 0;	/* SATA */
>  	if ((dev_id[93] & 0x2000) == 0x2000)
>  		return 0;	/* 80 wire */
>  	return 1;

I've been thinking a lot about this, and I am really wondering if we 
should fix up the IDENTIFY DEVICE page instead?

Re: [PATCH] ata.h: Don't do a SATA check for 40wire_relaxed

[Alan Cox]

> > -	if (ata_id_is_sata(dev_id))
> > -		return 0;	/* SATA */
> >  	if ((dev_id[93] & 0x2000) == 0x2000)
> >  		return 0;	/* 80 wire */
> >  	return 1;
> 
> I've been thinking a lot about this, and I am really wondering if we 
> should fix up the IDENTIFY DEVICE page instead?

You want to intercept all the SG_IO calls to do an ATAPI identify and
make them lie ? I'm not personally sure thats a good idea.

Alan

Re: [PATCH] ata.h: Don't do a SATA check for 40wire_relaxed

[Peter Missel]

Hi all!

As the originator of this patch, please let me comment.

Jeff Garzik wrote:
>Alan Cox wrote:
>> Without the valid bits at least one set of TSScorp drives report 0 in
>> word 93 for PATA 40 wire, which we (and the specs) say actually means
>> SATA. (The SATA version seems to report 80 wire...)
>> 
>> Signed-off-by: Alan Cox <alan@redhat.com>

>SATA version on what controller?  Have you verified where the bridge is, 
>if it's not reporting word93==0 ?

>Is this for 2.6.24-rc, considering that we are late in -rc?

The offending drives are native IDE, but their word93 is broken, in that the 
validation bits are missing. Hence, if connected to a 40-wire cable, they 
return word93=0x0000 and make themselves look like a SATA drive.

This makes the 'relaxed' cable check do the exact wrong thing - allow fast 
UDMA modes.

The patch discussed here corrects the 'relaxed' cable check, so that it 
actually does what it's supposed to - look at nothing but the cable type bit 
in word93 and exactly NOT draw any other conclusions from word93. 

Due the dangerous behaviour of the unpatched code, I strongly vote for 
inclusion in 2.6.24.

regards,
Peter

Re: [PATCH] ata.h: Don't do a SATA check for 40wire_relaxed

[Peter Missel]

Pinging ... don't take this personal guys, but are we going to keep the 
original, dangerously broken implementation in 2.6.24, or will my patch go 
upstream in time?

cheers,
Peter

---------------
Hi all!

As the originator of this patch, please let me comment.

Jeff Garzik wrote:
>Alan Cox wrote:
>> Without the valid bits at least one set of TSScorp drives report 0 in
>> word 93 for PATA 40 wire, which we (and the specs) say actually means
>> SATA. (The SATA version seems to report 80 wire...)
>> 
>> Signed-off-by: Alan Cox <alan@redhat.com>

>SATA version on what controller?  Have you verified where the bridge is, 
>if it's not reporting word93==0 ?

>Is this for 2.6.24-rc, considering that we are late in -rc?

The offending drives are native IDE, but their word93 is broken, in that the 
validation bits are missing. Hence, if connected to a 40-wire cable, they 
return word93=0x0000 and make themselves look like a SATA drive.

This makes the 'relaxed' cable check do the exact wrong thing - allow fast 
UDMA modes.

The patch discussed here corrects the 'relaxed' cable check, so that it 
actually does what it's supposed to - look at nothing but the cable type bit 
in word93 and exactly NOT draw any other conclusions from word93. 

Due the dangerous behaviour of the unpatched code, I strongly vote for 
inclusion in 2.6.24.

regards,
Peter

Re: [PATCH] ata.h: Don't do a SATA check for 40wire_relaxed

[Alan Cox]

On Sat, Dec 15, 2007 at 06:35:32PM +0100, Peter Missel wrote:
> Pinging ... don't take this personal guys, but are we going to keep the 
> original, dangerously broken implementation in 2.6.24, or will my patch go 
> upstream in time?

Up to Jeff/Linus.

Alan (on holiday)