[PATCH libata-dev#upstream-fixes] pata_amd: fix an obvious bug in cable detection

[PATCH libata-dev#upstream-fixes] pata_amd: fix an obvious bug in cable detection

[Tejun Heo]

80c test mask is at bits 18 and 19 of EIDE Controller Configuration
not 22 and 23.  Fix it.

Signed-off-by: Tejun Heo <htejun@gmail.com>
---
This makes unreliable cable detection even more unreliable.  Please
consider for -stable.  Thanks.

 drivers/ata/pata_amd.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: work/drivers/ata/pata_amd.c
===================================================================
--- work.orig/drivers/ata/pata_amd.c
+++ work/drivers/ata/pata_amd.c
@@ -128,7 +128,7 @@ static void timing_setup(struct ata_port
 
 static int amd_pre_reset(struct ata_port *ap)
 {
-	static const u32 bitmask[2] = {0x03, 0xC0};
+	static const u32 bitmask[2] = {0x03, 0x0C};
 	static const struct pci_bits amd_enable_bits[] = {
 		{ 0x40, 1, 0x02, 0x02 },
 		{ 0x40, 1, 0x01, 0x01 }
@@ -247,7 +247,7 @@ static void amd133_set_dmamode(struct at
  */
 
 static int nv_pre_reset(struct ata_port *ap) {
-	static const u8 bitmask[2] = {0x03, 0xC0};
+	static const u8 bitmask[2] = {0x03, 0x0C};
 	static const struct pci_bits nv_enable_bits[] = {
 		{ 0x50, 1, 0x02, 0x02 },
 		{ 0x50, 1, 0x01, 0x01 }

Re: [PATCH libata-dev#upstream-fixes] pata_amd: fix an obvious bug in cable detection

[Alan]

On Mon, 5 Feb 2007 17:01:28 +0900
Tejun Heo <htejun@gmail.com> wrote:

> 80c test mask is at bits 18 and 19 of EIDE Controller Configuration
> not 22 and 23.  Fix it.

ACK - verified against data sheet.

Alan

Re: [PATCH libata-dev#upstream-fixes] pata_amd: fix an obvious bug in cable detection

[Jeff Garzik]

Tejun Heo wrote:
> 80c test mask is at bits 18 and 19 of EIDE Controller Configuration
> not 22 and 23.  Fix it.
> 
> Signed-off-by: Tejun Heo <htejun@gmail.com>
> ---
> This makes unreliable cable detection even more unreliable.  Please
> consider for -stable.  Thanks.
> 
>  drivers/ata/pata_amd.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Index: work/drivers/ata/pata_amd.c
> ===================================================================
> --- work.orig/drivers/ata/pata_amd.c
> +++ work/drivers/ata/pata_amd.c
> @@ -128,7 +128,7 @@ static void timing_setup(struct ata_port
>  
>  static int amd_pre_reset(struct ata_port *ap)
>  {
> -	static const u32 bitmask[2] = {0x03, 0xC0};
> +	static const u32 bitmask[2] = {0x03, 0x0C};
>  	static const struct pci_bits amd_enable_bits[] = {
>  		{ 0x40, 1, 0x02, 0x02 },
>  		{ 0x40, 1, 0x01, 0x01 }
> @@ -247,7 +247,7 @@ static void amd133_set_dmamode(struct at
>   */
>  
>  static int nv_pre_reset(struct ata_port *ap) {
> -	static const u8 bitmask[2] = {0x03, 0xC0};
> +	static const u8 bitmask[2] = {0x03, 0x0C};
>  	static const struct pci_bits nv_enable_bits[] = {

applied

Re: [PATCH libata-dev#upstream-fixes] pata_amd: fix an obvious bug in cable detection

[Alan]

On Sat, 24 Feb 2007 20:52:22 -0500
Jeff Garzik <jeff@garzik.org> wrote:

> Tejun Heo wrote:
> > 80c test mask is at bits 18 and 19 of EIDE Controller Configuration
> > not 22 and 23.  Fix it.
> > 
> > Signed-off-by: Tejun Heo <htejun@gmail.com>
> > ---
> > This makes unreliable cable detection even more unreliable.  Please
> > consider for -stable.  Thanks.

At minimum you also need to stop doing drive side detect for PATA_CBL_80
as many controllers don't do both so if you've got host side detect that
is what you must trust.

Fixed that in my internal tree and it seems happier

Re: [PATCH libata-dev#upstream-fixes] pata_amd: fix an obvious bug in cable detection

[Tejun Heo]

[cc'ing Mark, Bart and Sergei. Hi]

Alan wrote:
>>> This makes unreliable cable detection even more unreliable.  Please
>>> consider for -stable.  Thanks.
> 
> At minimum you also need to stop doing drive side detect for PATA_CBL_80
> as many controllers don't do both so if you've got host side detect that
> is what you must trust.
> 
> Fixed that in my internal tree and it seems happier

It seems libata is wrong about device side cable detection.  Device side
cable detection tries to detect host side capacitor connected to PDIAG-,
depends on the slave device releasing PDIAG- signal.

According to Annex B of ATA/ATAPI-5, IDENTIFY should be issued to the
slave device first to ensure that it releases PDIAG- and then use the
cable detection result from the master device.  As we IDENTIFY master
first right after reset, slave if present is driving PDIAG-, so the
master on IDENTIFY always thinks the capacitor is present and 40c limit
is always applied.

IDE is better off because it doesn't reset before IDENTIFYing and it's
likely that BIOS has issued some commands to the slave device prior to
passing control to OS thus making it release PDIAG-.

1. Device side cable detection can only be used to limit speed because
if motherboard doesn't have capacitor attached to PDIAG-, the test
result always indicates 80c.

2. Host side detection involves issuing commands to attached devices
because it depends on the slave device releasing PDIAG-, so most
controllers do cable detection in BIOS and just put the result in PCI
config region or wherever.  We dunno how BIOS does it exactly but many
of them probably consider device side detection result as well.

So, considering #1 and #2, it might be best to just believe what the
controller (BIOS) says and not bother about device side detection.  In
fact, we've been effectively ignoring device side detection result
before my "fix" for device side cable detection.

What do you think?

-- 
tejun

Re: [PATCH libata-dev#upstream-fixes] pata_amd: fix an obvious bug in cable detection

[Bartlomiej Zolnierkiewicz]

On Sunday 25 February 2007, Tejun Heo wrote:
> [cc'ing Mark, Bart and Sergei. Hi]
> 
> Alan wrote:
> >>> This makes unreliable cable detection even more unreliable.  Please
> >>> consider for -stable.  Thanks.
> > 
> > At minimum you also need to stop doing drive side detect for PATA_CBL_80
> > as many controllers don't do both so if you've got host side detect that
> > is what you must trust.
> > 
> > Fixed that in my internal tree and it seems happier
> 
> It seems libata is wrong about device side cable detection.  Device side
> cable detection tries to detect host side capacitor connected to PDIAG-,
> depends on the slave device releasing PDIAG- signal.
> 
> According to Annex B of ATA/ATAPI-5, IDENTIFY should be issued to the
> slave device first to ensure that it releases PDIAG- and then use the
> cable detection result from the master device.  As we IDENTIFY master
> first right after reset, slave if present is driving PDIAG-, so the
> master on IDENTIFY always thinks the capacitor is present and 40c limit
> is always applied.
> 
> IDE is better off because it doesn't reset before IDENTIFYing and it's
> likely that BIOS has issued some commands to the slave device prior to
> passing control to OS thus making it release PDIAG-.
> 
> 1. Device side cable detection can only be used to limit speed because
> if motherboard doesn't have capacitor attached to PDIAG-, the test
> result always indicates 80c.
> 
> 2. Host side detection involves issuing commands to attached devices
> because it depends on the slave device releasing PDIAG-, so most
> controllers do cable detection in BIOS and just put the result in PCI
> config region or wherever.  We dunno how BIOS does it exactly but many
> of them probably consider device side detection result as well.
> 
> So, considering #1 and #2, it might be best to just believe what the
> controller (BIOS) says and not bother about device side detection.  In
> fact, we've been effectively ignoring device side detection result
> before my "fix" for device side cable detection.
> 
> What do you think?

Why can't libata do the right thing and just send IDENTIFY command
to the slave device first?

Bart

Re: [PATCH libata-dev#upstream-fixes] pata_amd: fix an obvious bug in cable detection

[Tejun Heo]

Bartlomiej Zolnierkiewicz wrote:
> Why can't libata do the right thing and just send IDENTIFY command
> to the slave device first?

That's my plan B.  I'm just not sure whether it would do any good, would it?

-- 
tejun

Re: [PATCH libata-dev#upstream-fixes] pata_amd: fix an obvious bug in cable detection

[Bartlomiej Zolnierkiewicz]

On Sunday 25 February 2007, Tejun Heo wrote:
> Bartlomiej Zolnierkiewicz wrote:
> > Why can't libata do the right thing and just send IDENTIFY command
> > to the slave device first?
> 
> That's my plan B.  I'm just not sure whether it would do any good, would it?

Probably it would but to know for sure you would need to verify it on some
of the affected system (shouldn't be problem given number of the bugreports).

Bart

Re: [PATCH libata-dev#upstream-fixes] pata_amd: fix an obvious bug in cable detection

[Alan]

> According to Annex B of ATA/ATAPI-5, IDENTIFY should be issued to the
> slave device first to ensure that it releases PDIAG- and then use the
> cable detection result from the master device.  As we IDENTIFY master
> first right after reset, slave if present is driving PDIAG-, so the
> master on IDENTIFY always thinks the capacitor is present and 40c limit
> is always applied.

I had to go read this and its news to me but nice to know what is going
on.

> So, considering #1 and #2, it might be best to just believe what the
> controller (BIOS) says and not bother about device side detection.  In

The BIOS is run once at boot, on an x86 box, maybe only for some
controllers.

> What do you think?

I think we need to do the identify first. We could actually tidy up a lot
of duplicate driver code if there was a ->cable_detect() method, as many
of them could use the standard ata error handler without hooks. Right now
it is mostly hooked just to do cable type setup.

There are far too many cases from non-x86 through to hotplug we can't
trust any BIOS bits.

Alan

Re: [PATCH libata-dev#upstream-fixes] pata_amd: fix an obvious bug in cable detection

[Alan]

On Mon, 26 Feb 2007 02:41:22 +0900
Tejun Heo <htejun@gmail.com> wrote:

> Bartlomiej Zolnierkiewicz wrote:
> > Why can't libata do the right thing and just send IDENTIFY command
> > to the slave device first?
> 
> That's my plan B.  I'm just not sure whether it would do any good, would it?

Should find out soon. I've updated my tree to add the ->cable_detect()
method, along with finishing off the (ap, adev) cleanup by passing only
adev to set_pio_mode and friends, also killing the un-needed
->post_set_mode() now that set_mode can be wrapped.

Having a separate ->cable_detect() method cleans stuff up a ton btw. Will
post my tree soon for discussion.

Alan