[PATCH] libata: warn if speed limited due to 40-wire cable (v2)

[PATCH] libata: warn if speed limited due to 40-wire cable (v2)

[Robert Hancock]

Here's a revised version of my previous patch to warn the user if a
drive's transfer rate is limited because of a 40-wire cable detection.
This one hopefully addresses Alan's previous comments - we now do this
at the very end of the function, and the ugly if condition has been
cleaned up somewhat. Also, it's been inadvertently tested (it seems that
pata_amd Nvidia cable detection is broken in current -git..)

Signed-off-by: Robert Hancock <hancockr@shaw.ca>

--- linux-2.6.20-git6/drivers/ata/libata-core.c	2007-02-11 17:31:19.000000000 -0600
+++ linux-2.6.20-git6edit/drivers/ata/libata-core.c	2007-02-13 19:15:58.000000000 -0600
@@ -3305,19 +3305,7 @@ static void ata_dev_xfermask(struct ata_
 	xfer_mask = ata_pack_xfermask(ap->pio_mask,
 				      ap->mwdma_mask, ap->udma_mask);
 
-	/* Apply cable rule here.  Don't apply it early because when
-	 * we handle hot plug the cable type can itself change.
-	 */
-	if (ap->cbl == ATA_CBL_PATA40)
-		xfer_mask &= ~(0xF8 << ATA_SHIFT_UDMA);
-	/* Apply drive side cable rule. Unknown or 80 pin cables reported
-	 * host side are checked drive side as well. Cases where we know a
-	 * 40wire cable is used safely for 80 are not checked here.
-	 */
-        if (ata_drive_40wire(dev->id) && (ap->cbl == ATA_CBL_PATA_UNK || ap->cbl == ATA_CBL_PATA80))
-		xfer_mask &= ~(0xF8 << ATA_SHIFT_UDMA);
-
-
+	/* drive modes available */
 	xfer_mask &= ata_pack_xfermask(dev->pio_mask,
 				       dev->mwdma_mask, dev->udma_mask);
 	xfer_mask &= ata_id_xfermask(dev->id);
@@ -3348,6 +3336,25 @@ static void ata_dev_xfermask(struct ata_
 	if (ap->ops->mode_filter)
 		xfer_mask = ap->ops->mode_filter(ap, dev, xfer_mask);
 
+	/* Apply cable rule here.  Don't apply it early because when
+	 * we handle hot plug the cable type can itself change.
+	 * Check this last so that we know if the transfer rate was
+	 * solely limited by the cable.
+	 * Unknown or 80 wire cables reported host side are checked
+	 * drive side as well. Cases where we know a 40wire cable
+	 * is used safely for 80 are not checked here.
+	 */
+	if (xfer_mask & (0xF8 << ATA_SHIFT_UDMA))
+		/* UDMA/44 or higher would be available */
+		if((ap->cbl == ATA_CBL_PATA40) ||
+   		    (ata_drive_40wire(dev->id) &&
+		     (ap->cbl == ATA_CBL_PATA_UNK ||
+                     ap->cbl == ATA_CBL_PATA80))) {
+		      	ata_dev_printk(dev, KERN_WARNING,
+				 "limited to UDMA/33 due to 40-wire cable\n");
+			xfer_mask &= ~(0xF8 << ATA_SHIFT_UDMA);
+		}
+
 	ata_unpack_xfermask(xfer_mask, &dev->pio_mask,
 			    &dev->mwdma_mask, &dev->udma_mask);
 }

Re: [PATCH] libata: warn if speed limited due to 40-wire cable (v2)

[Alan]

> cleaned up somewhat. Also, it's been inadvertently tested (it seems that
> pata_amd Nvidia cable detection is broken in current -git..)

Yes. The pata_acpi driver fixes that one, but got kicked out as it didn't
match other merges at the same time. It'll be back soon

> 
> Signed-off-by: Robert Hancock <hancockr@shaw.ca>

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

Re: [PATCH] libata: warn if speed limited due to 40-wire cable (v2)

[Jeff Garzik]

Robert Hancock wrote:
> Here's a revised version of my previous patch to warn the user if a
> drive's transfer rate is limited because of a 40-wire cable detection.
> This one hopefully addresses Alan's previous comments - we now do this
> at the very end of the function, and the ugly if condition has been
> cleaned up somewhat. Also, it's been inadvertently tested (it seems that
> pata_amd Nvidia cable detection is broken in current -git..)
> 
> Signed-off-by: Robert Hancock <hancockr@shaw.ca>
> 
> --- linux-2.6.20-git6/drivers/ata/libata-core.c    2007-02-11 
> 17:31:19.000000000 -0600
> +++ linux-2.6.20-git6edit/drivers/ata/libata-core.c    2007-02-13 
> 19:15:58.000000000 -0600
> @@ -3305,19 +3305,7 @@ static void ata_dev_xfermask(struct ata_
>     xfer_mask = ata_pack_xfermask(ap->pio_mask,
>                       ap->mwdma_mask, ap->udma_mask);
> 
> -    /* Apply cable rule here.  Don't apply it early because when
> -     * we handle hot plug the cable type can itself change.
> -     */
> -    if (ap->cbl == ATA_CBL_PATA40)
> -        xfer_mask &= ~(0xF8 << ATA_SHIFT_UDMA);
> -    /* Apply drive side cable rule. Unknown or 80 pin cables reported
> -     * host side are checked drive side as well. Cases where we know a
> -     * 40wire cable is used safely for 80 are not checked here.
> -     */
> -        if (ata_drive_40wire(dev->id) && (ap->cbl == ATA_CBL_PATA_UNK 
> || ap->cbl == ATA_CBL_PATA80))
> -        xfer_mask &= ~(0xF8 << ATA_SHIFT_UDMA);
> -
> -
> +    /* drive modes available */
>     xfer_mask &= ata_pack_xfermask(dev->pio_mask,
>                        dev->mwdma_mask, dev->udma_mask);
>     xfer_mask &= ata_id_xfermask(dev->id);
> @@ -3348,6 +3336,25 @@ static void ata_dev_xfermask(struct ata_
>     if (ap->ops->mode_filter)
>         xfer_mask = ap->ops->mode_filter(ap, dev, xfer_mask);
> 
> +    /* Apply cable rule here.  Don't apply it early because when
> +     * we handle hot plug the cable type can itself change.
> +     * Check this last so that we know if the transfer rate was
> +     * solely limited by the cable.
> +     * Unknown or 80 wire cables reported host side are checked
> +     * drive side as well. Cases where we know a 40wire cable
> +     * is used safely for 80 are not checked here.
> +     */
> +    if (xfer_mask & (0xF8 << ATA_SHIFT_UDMA))
> +        /* UDMA/44 or higher would be available */
> +        if((ap->cbl == ATA_CBL_PATA40) ||
> +               (ata_drive_40wire(dev->id) &&
> +             (ap->cbl == ATA_CBL_PATA_UNK ||
> +                     ap->cbl == ATA_CBL_PATA80))) {
> +                  ata_dev_printk(dev, KERN_WARNING,
> +                 "limited to UDMA/33 due to 40-wire cable\n");
> +            xfer_mask &= ~(0xF8 << ATA_SHIFT_UDMA);
> +        }

it seems broken to manipulate xfer_mask after returning from the 
driver's ->mode_filter hook.

this patch is more than just a speed-limited warning printk, afaics

Re: [PATCH] libata: warn if speed limited due to 40-wire cable (v2)

[Jeff Garzik]

Alan Cox wrote:
>> it seems broken to manipulate xfer_mask after returning from the 
>> driver's ->mode_filter hook.
>>
>> this patch is more than just a speed-limited warning printk, afaics
> 
> I actually suggested that order because the only way the printk can be
> done correctly is for it to be the very last test made. Since the mode
> filter is not told what mode will be used but just subtracts modes that
> are not allowed this should be safe.

OK

Re: [PATCH] libata: warn if speed limited due to 40-wire cable (v2)

[Alan Cox]

> it seems broken to manipulate xfer_mask after returning from the 
> driver's ->mode_filter hook.
> 
> this patch is more than just a speed-limited warning printk, afaics

I actually suggested that order because the only way the printk can be
done correctly is for it to be the very last test made. Since the mode
filter is not told what mode will be used but just subtracts modes that
are not allowed this should be safe.

I don't otherwise see how we deal with the case where a 40wire cable is
used and the mode filter function decides itself to drop to UDMA33 or
lower due to incompatibilities or errata.

Alan

Re: [PATCH] libata: warn if speed limited due to 40-wire cable (v2)

[Bill Davidsen]

Alan Cox wrote:
>> it seems broken to manipulate xfer_mask after returning from the 
>> driver's ->mode_filter hook.
>>
>> this patch is more than just a speed-limited warning printk, afaics
> 
> I actually suggested that order because the only way the printk can be
> done correctly is for it to be the very last test made. Since the mode
> filter is not told what mode will be used but just subtracts modes that
> are not allowed this should be safe.

Far better to have a drive which works slowly than one which works 
unreliably.

-- 
Bill Davidsen <davidsen@tmr.com>
   "We have more to fear from the bungling of the incompetent than from
the machinations of the wicked."  - from Slashdot

Re: [PATCH] libata: warn if speed limited due to 40-wire cable (v2)

[Stephen Clark]

Bill Davidsen wrote:

>Alan Cox wrote:
>  
>
>>>it seems broken to manipulate xfer_mask after returning from the 
>>>driver's ->mode_filter hook.
>>>
>>>this patch is more than just a speed-limited warning printk, afaics
>>>      
>>>
>>I actually suggested that order because the only way the printk can be
>>done correctly is for it to be the very last test made. Since the mode
>>filter is not told what mode will be used but just subtracts modes that
>>are not allowed this should be safe.
>>    
>>
>
>Far better to have a drive which works slowly than one which works 
>unreliably.
>
>  
>
That would be true if the 40 wire detection was 100% accurate!

-- 

"They that give up essential liberty to obtain temporary safety, 
deserve neither liberty nor safety."  (Ben Franklin)

"The course of history shows that as a government grows, liberty 
decreases."  (Thomas Jefferson)

Re: [PATCH] libata: warn if speed limited due to 40-wire cable (v2)

[Bill Davidsen]

Stephen Clark wrote:
> Bill Davidsen wrote:
>
>> Alan Cox wrote:
>>  
>>
>>>> it seems broken to manipulate xfer_mask after returning from the 
>>>> driver's ->mode_filter hook.
>>>>
>>>> this patch is more than just a speed-limited warning printk, afaics
>>>>     
>>> I actually suggested that order because the only way the printk can be
>>> done correctly is for it to be the very last test made. Since the mode
>>> filter is not told what mode will be used but just subtracts modes that
>>> are not allowed this should be safe.
>>>   
>>
>> Far better to have a drive which works slowly than one which works 
>> unreliably.
>>
>>  
>>
> That would be true if the 40 wire detection was 100% accurate!
The statement is completely correct, even though the detection may not 
be. ;-)

With the current set(s) of patches to do better detection, cable 
evaluation should be better. But even if not, a slow system is more 
useful than one which doesn't work, crashes because of swap i/o errors, etc.

-- 
bill davidsen <davidsen@tmr.com>
  CTO TMR Associates, Inc
  Doing interesting things with small computers since 1979

Re: [PATCH] libata: warn if speed limited due to 40-wire cable (v2)

[Stephen Clark]

Bill Davidsen wrote:

>Stephen Clark wrote:
>  
>
>>Bill Davidsen wrote:
>>
>>    
>>
>>>Alan Cox wrote:
>>> 
>>>
>>>      
>>>
>>>>>it seems broken to manipulate xfer_mask after returning from the 
>>>>>driver's ->mode_filter hook.
>>>>>
>>>>>this patch is more than just a speed-limited warning printk, afaics
>>>>>    
>>>>>          
>>>>>
>>>>I actually suggested that order because the only way the printk can be
>>>>done correctly is for it to be the very last test made. Since the mode
>>>>filter is not told what mode will be used but just subtracts modes that
>>>>are not allowed this should be safe.
>>>>  
>>>>        
>>>>
>>>Far better to have a drive which works slowly than one which works 
>>>unreliably.
>>>
>>> 
>>>
>>>      
>>>
>>That would be true if the 40 wire detection was 100% accurate!
>>    
>>
>The statement is completely correct, even though the detection may not 
>be. ;-)
>
>With the current set(s) of patches to do better detection, cable 
>evaluation should be better. But even if not, a slow system is more 
>useful than one which doesn't work, crashes because of swap i/o errors, etc.
>
>  
>
I have had problems with cable detection on my previous laptop and my 
current laptop. It almost made
my systems unusable. On my current laptop I was getting a thruput of a 
little over 1 mbps instead
of the 44 mbps I get with udma set to the correct value. It took hours 
to upgrade my laptop from
fc5 to fc6 because of this mis detection.

-- 

"They that give up essential liberty to obtain temporary safety, 
deserve neither liberty nor safety."  (Ben Franklin)

"The course of history shows that as a government grows, liberty 
decreases."  (Thomas Jefferson)

Re: [PATCH] libata: warn if speed limited due to 40-wire cable (v2)

[Bill Davidsen]

Stephen Clark wrote:
> Bill Davidsen wrote:
>
>> Stephen Clark wrote:
>>  
>>
>>> Bill Davidsen wrote:
>>>
>>>   
>>>> Alan Cox wrote:
>>>>
>>>>
>>>>     
>>>>>> it seems broken to manipulate xfer_mask after returning from the 
>>>>>> driver's ->mode_filter hook.
>>>>>>
>>>>>> this patch is more than just a speed-limited warning printk, afaics
>>>>>>            
>>>>> I actually suggested that order because the only way the printk 
>>>>> can be
>>>>> done correctly is for it to be the very last test made. Since the 
>>>>> mode
>>>>> filter is not told what mode will be used but just subtracts modes 
>>>>> that
>>>>> are not allowed this should be safe.
>>>>>  
>>>>>       
>>>> Far better to have a drive which works slowly than one which works 
>>>> unreliably.
>>>>
>>>>
>>>>
>>>>     
>>> That would be true if the 40 wire detection was 100% accurate!
>>>   
>> The statement is completely correct, even though the detection may 
>> not be. ;-)
>>
>> With the current set(s) of patches to do better detection, cable 
>> evaluation should be better. But even if not, a slow system is more 
>> useful than one which doesn't work, crashes because of swap i/o 
>> errors, etc.
>>
>>  
>>
> I have had problems with cable detection on my previous laptop and my 
> current laptop. It almost made
> my systems unusable. On my current laptop I was getting a thruput of a 
> little over 1 mbps instead
> of the 44 mbps I get with udma set to the correct value. It took hours 
> to upgrade my laptop from
> fc5 to fc6 because of this mis detection.
>
As far as I can see, if you are getting that low a speed, you have other 
problems. I have a system with old slow drives which are really on a 40 
pin cable, and they run at UDMA(33). One of the experts in this can 
undoubtedly tell us more, but your system should run faster than that, 
mine does, and I really HAVE a 40 pin cable (and drive).

If your system drops to PIO modes, I doubt cable is the only issue, I 
think there are other issues (acpi comes to mind).

-- 
bill davidsen <davidsen@tmr.com>
  CTO TMR Associates, Inc
  Doing interesting things with small computers since 1979