Fwd: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

Fwd: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Guntsche Michael]

Hello list,

Forgot to CC: linux-ide when writing this.

Please put me on CC when replying since I am not subscribed to the list.

Kind regards,
Michael

Begin forwarded message:

> From: Michael Guntsche <mike@it-loops.com>
> Date: October 27, 2008 11:21:58 GMT+01:00
> To: tj@kernel.org
> Subject: Re: [PATCH #upstream-fixes 1/4] libata: fix device  
> iteration bugs
>
> Hello,
>
> I just browsed through the linux-ide archives and found this patch.  
> I am
> wondering if this also helps with a problem I am facing on one of my  
> router
> boxes since a few kernel versions.
>
> I have an ancient router box with a  "Intel Corporation 82371SB  
> PIIX3 IDE
> [Natoma/Triton II]" controller. I switched to the new libata code a  
> long
> time ago using the ata_piix driver. I have one harddisk and a CD- 
> burner i
> not use very often. The Harddisk is on Primary master the burner on
> secondary master. with at least the last two kernel versions, I see  
> this
> during bootup
>
> ata_piix 0000:00:07.1: version 2.12
> scsi0 : ata_piix
> scsi1 : ata_piix
> ata1: PATA max MWDMA2 cmd 0x1f0 ctl 0x3f6 bmdma 0xe800 irq 14
> ata2: PATA max MWDMA2 cmd 0x170 ctl 0x376 bmdma 0xe808 irq 15
> ata1.00: ATA-5: IC35L040AVER07-0, ER4OA45A, max UDMA/100
> ata1.00: 66055248 sectors, multi 16: LBA
> ata1.00: configured for MWDMA2
> ata2.01: failed to IDENTIFY (I/O error, err_mask=0x1)
> ata2.01: failed to IDENTIFY (I/O error, err_mask=0x1)
> ata2.01: failed to IDENTIFY (I/O error, err_mask=0x1)
> ata2.00: ATAPI: SAMSUNG CD-R/RW SW-408B, M300, max MWDMA2
>
> As you can see for some reason ata2.01 (secondary slave??) gets probed
> during startup although there is nothing attached to it. It is tried  
> three
> times with a 5 seconds pause in between. I did not notice this in  
> the past
> since a do not reboot that machine very often.
> For testing purposes I just recently did this a few times in a row and
> noticed this. It is not a biggie since harddisk and burner are  
> working but
> nevertheless it wasn't happening with older kernel versions so I  
> though you
> should know.
>
> Kind regards,
> Michael

Re: Fwd: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Tejun Heo]

[-- Attachment #1: Type: text/plain, Size: 1886 bytes --]

Hello,

Guntsche Michael wrote:
>> I just browsed through the linux-ide archives and found this patch. I am
>> wondering if this also helps with a problem I am facing on one of my
>> router
>> boxes since a few kernel versions.
>>
>> I have an ancient router box with a  "Intel Corporation 82371SB PIIX3 IDE
>> [Natoma/Triton II]" controller. I switched to the new libata code a long
>> time ago using the ata_piix driver. I have one harddisk and a CD-burner i
>> not use very often. The Harddisk is on Primary master the burner on
>> secondary master. with at least the last two kernel versions, I see this
>> during bootup
>>
>> ata_piix 0000:00:07.1: version 2.12
>> scsi0 : ata_piix
>> scsi1 : ata_piix
>> ata1: PATA max MWDMA2 cmd 0x1f0 ctl 0x3f6 bmdma 0xe800 irq 14
>> ata2: PATA max MWDMA2 cmd 0x170 ctl 0x376 bmdma 0xe808 irq 15
>> ata1.00: ATA-5: IC35L040AVER07-0, ER4OA45A, max UDMA/100
>> ata1.00: 66055248 sectors, multi 16: LBA
>> ata1.00: configured for MWDMA2
>> ata2.01: failed to IDENTIFY (I/O error, err_mask=0x1)
>> ata2.01: failed to IDENTIFY (I/O error, err_mask=0x1)
>> ata2.01: failed to IDENTIFY (I/O error, err_mask=0x1)
>> ata2.00: ATAPI: SAMSUNG CD-R/RW SW-408B, M300, max MWDMA2
>>
>> As you can see for some reason ata2.01 (secondary slave??) gets
>> probed during startup although there is nothing attached to it. It
>> is tried three times with a 5 seconds pause in between. I did not
>> notice this in the past since a do not reboot that machine very
>> often.  For testing purposes I just recently did this a few times
>> in a row and noticed this. It is not a biggie since harddisk and
>> burner are working but nevertheless it wasn't happening with older
>> kernel versions so I though you should know.

Looks like our phantom device detection logic is somehow broken.  Can
you please apply the attached patch and report boot log?

Thanks.

-- 
tejun

[-- Attachment #2: phantom-debug.patch --]
[-- Type: text/x-patch, Size: 488 bytes --]

diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index 4b47394..709bbb4 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -1226,6 +1226,8 @@ fsm_start:
 		} else {
 			/* ATA PIO protocol */
 			if (unlikely((status & ATA_DRQ) == 0)) {
+				ata_dev_printk(qc->dev, KERN_INFO,
+					       "XXX status=0x%x\n", status);
 				/* handle BSY=0, DRQ=0 as error */
 				if (likely(status & (ATA_ERR | ATA_DF)))
 					/* device stops HSM for abort/error */

Re: Fwd: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Michael Guntsche]

On Mon, 10 Nov 2008 15:52:02 +0900, Tejun Heo <tj@kernel.org> wrote:
> Hello,

> Looks like our phantom device detection logic is somehow broken.  Can
> you please apply the attached patch and report boot log?

Here is the output after adding the debug line. Hopefully this helps.

ata_piix 0000:00:07.1: version 2.12
scsi0 : ata_piix
scsi1 : ata_piix
ata1: PATA max MWDMA2 cmd 0x1f0 ctl 0x3f6 bmdma 0xe800 irq 14
ata2: PATA max MWDMA2 cmd 0x170 ctl 0x376 bmdma 0xe808 irq 15
ata1.00: ATA-5: IC35L040AVER07-0, ER4OA45A, max UDMA/100
ata1.00: 66055248 sectors, multi 16: LBA
ata1.00: configured for MWDMA2
ata2.01: XXX status=0x1
ata2.01: failed to IDENTIFY (I/O error, err_mask=0x1)
ata2.01: XXX status=0x1
ata2.01: failed to IDENTIFY (I/O error, err_mask=0x1)
ata2.01: XXX status=0x1
ata2.01: failed to IDENTIFY (I/O error, err_mask=0x1)
ata2.00: ATAPI: SAMSUNG CD-R/RW SW-408B, M300, max MWDMA2
ata2.00: configured for MWDMA2

Kind regards,
Michael

Re: Fwd: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Tejun Heo]

(cc'ing Alan, Jeff and Mark).

Hello, guys.

Michael Guntsche wrote:
> On Mon, 10 Nov 2008 15:52:02 +0900, Tejun Heo <tj@kernel.org> wrote:
>> Hello,
> 
>> Looks like our phantom device detection logic is somehow broken.  Can
>> you please apply the attached patch and report boot log?
> 
> Here is the output after adding the debug line. Hopefully this helps.
> 
> ata_piix 0000:00:07.1: version 2.12
> scsi0 : ata_piix
> scsi1 : ata_piix
> ata1: PATA max MWDMA2 cmd 0x1f0 ctl 0x3f6 bmdma 0xe800 irq 14
> ata2: PATA max MWDMA2 cmd 0x170 ctl 0x376 bmdma 0xe808 irq 15
> ata1.00: ATA-5: IC35L040AVER07-0, ER4OA45A, max UDMA/100
> ata1.00: 66055248 sectors, multi 16: LBA
> ata1.00: configured for MWDMA2
> ata2.01: XXX status=0x1
> ata2.01: failed to IDENTIFY (I/O error, err_mask=0x1)
> ata2.01: XXX status=0x1
> ata2.01: failed to IDENTIFY (I/O error, err_mask=0x1)
> ata2.01: XXX status=0x1
> ata2.01: failed to IDENTIFY (I/O error, err_mask=0x1)
> ata2.00: ATAPI: SAMSUNG CD-R/RW SW-408B, M300, max MWDMA2
> ata2.00: configured for MWDMA2

On Michael's real old ata_piix, somehow the ERR bit is set on phantom
device defeating our phantom device detection logic.  We can relax
phantom device condition but that increases the risk of misdetection on
actual IDENTIFY errors.  Any ideas how we can nicely work around this one?

Thanks.

-- 
tejun

Re: Fwd: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Mark Lord]

Tejun Heo wrote:
> (cc'ing Alan, Jeff and Mark).
> 
> Hello, guys.
> 
> Michael Guntsche wrote:
>> On Mon, 10 Nov 2008 15:52:02 +0900, Tejun Heo <tj@kernel.org> wrote:
>>> Hello,
>>> Looks like our phantom device detection logic is somehow broken.  Can
>>> you please apply the attached patch and report boot log?
>> Here is the output after adding the debug line. Hopefully this helps.
>>
>> ata_piix 0000:00:07.1: version 2.12
>> scsi0 : ata_piix
>> scsi1 : ata_piix
>> ata1: PATA max MWDMA2 cmd 0x1f0 ctl 0x3f6 bmdma 0xe800 irq 14
>> ata2: PATA max MWDMA2 cmd 0x170 ctl 0x376 bmdma 0xe808 irq 15
>> ata1.00: ATA-5: IC35L040AVER07-0, ER4OA45A, max UDMA/100
>> ata1.00: 66055248 sectors, multi 16: LBA
>> ata1.00: configured for MWDMA2
>> ata2.01: XXX status=0x1
>> ata2.01: failed to IDENTIFY (I/O error, err_mask=0x1)
>> ata2.01: XXX status=0x1
>> ata2.01: failed to IDENTIFY (I/O error, err_mask=0x1)
>> ata2.01: XXX status=0x1
>> ata2.01: failed to IDENTIFY (I/O error, err_mask=0x1)
>> ata2.00: ATAPI: SAMSUNG CD-R/RW SW-408B, M300, max MWDMA2
>> ata2.00: configured for MWDMA2
> 
> On Michael's real old ata_piix, somehow the ERR bit is set on phantom
> device defeating our phantom device detection logic.  We can relax
> phantom device condition but that increases the risk of misdetection on
> actual IDENTIFY errors.  Any ideas how we can nicely work around this one?
..

Mmm.. I'm way out of the loop on this now,
so please pardon me suggesting things known not to work, but..

1. Pay attention to ATA status register on this chipset?
   Eg. if it has BUSY, or reads 0x7f, then don't IDENTIFY?
2. Check for device signature before trying IDENTIFY?
3. Try a r/w test on the data register first, to see if there's
   really hardware attached to it?

Cheers

Re: Fwd: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Tejun Heo]

Mark Lord wrote:
>> On Michael's real old ata_piix, somehow the ERR bit is set on phantom
>> device defeating our phantom device detection logic.  We can relax
>> phantom device condition but that increases the risk of misdetection on
>> actual IDENTIFY errors.  Any ideas how we can nicely work around this
>> one?
> ..
> 
> Mmm.. I'm way out of the loop on this now,
> so please pardon me suggesting things known not to work, but..
> 
> 1. Pay attention to ATA status register on this chipset?
>   Eg. if it has BUSY, or reads 0x7f, then don't IDENTIFY?
> 2. Check for device signature before trying IDENTIFY?
> 3. Try a r/w test on the data register first, to see if there's
>   really hardware attached to it?

We already do 1, 2 and 3.  The problem with phantom device is that the
existent device on the channel replies to acesses to the other
non-existent device and the NODEV_HINT detection is the last line of
defense which successfully evades all the other mechanisms.  And now
we have this controller/device combination which successfully tricks
it too.  :-(

-- 
tejun

Re: Fwd: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Mark Lord]

Tejun Heo wrote:
> Mark Lord wrote:
>>> On Michael's real old ata_piix, somehow the ERR bit is set on phantom
>>> device defeating our phantom device detection logic.  We can relax
>>> phantom device condition but that increases the risk of misdetection on
>>> actual IDENTIFY errors.  Any ideas how we can nicely work around this
>>> one?
>> ..
>>
>> Mmm.. I'm way out of the loop on this now,
>> so please pardon me suggesting things known not to work, but..
>>
>> 1. Pay attention to ATA status register on this chipset?
>>   Eg. if it has BUSY, or reads 0x7f, then don't IDENTIFY?
>> 2. Check for device signature before trying IDENTIFY?
>> 3. Try a r/w test on the data register first, to see if there's
>>   really hardware attached to it?
> 
> We already do 1, 2 and 3.  The problem with phantom device is that the
> existent device on the channel replies to acesses to the other
> non-existent device and the NODEV_HINT detection is the last line of
> defense which successfully evades all the other mechanisms.  And now
> we have this controller/device combination which successfully tricks
> it too.  :-(
..

Mmm.. but he's using "really old ata_piix" hardware, as in what Intel
once called the "Triton" (or Triton II) chipset.  Which I wrote support
for in drivers/ide, way back when.. and we never had this problem.

Something weird here.

Re: Fwd: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Sergei Shtylyov]

Hello.

Mark Lord wrote:

>>>> On Michael's real old ata_piix, somehow the ERR bit is set on phantom
>>>> device defeating our phantom device detection logic.  We can relax
>>>> phantom device condition but that increases the risk of 
>>>> misdetection on
>>>> actual IDENTIFY errors.  Any ideas how we can nicely work around this
>>>> one?
>>> ..
>>>
>>> Mmm.. I'm way out of the loop on this now,
>>> so please pardon me suggesting things known not to work, but..
>>>
>>> 1. Pay attention to ATA status register on this chipset?
>>>   Eg. if it has BUSY, or reads 0x7f, then don't IDENTIFY?
>>> 2. Check for device signature before trying IDENTIFY?
>>> 3. Try a r/w test on the data register first, to see if there's
>>>   really hardware attached to it?
>>
>> We already do 1, 2 and 3.  The problem with phantom device is that the
>> existent device on the channel replies to acesses to the other
>> non-existent device and the NODEV_HINT detection is the last line of
>> defense which successfully evades all the other mechanisms.  And now
>> we have this controller/device combination which successfully tricks
>> it too.  :-(
> ..
>
> Mmm.. but he's using "really old ata_piix" hardware, as in what Intel
> once called the "Triton" (or Triton II) chipset.

   The "original Triton" IDE is supported by pata_oldpiix.

>   Which I wrote support
> for in drivers/ide, way back when.. and we never had this problem.

   The support for the "original Triton" is drivers/ide/ is still broken 
after all these years. The driver assumes that the slave IDE timing 
register always present -- which the origina 82371FB didn't have. :-(

WBR, Sergei

Re: Fwd: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Michael Guntsche]

On Tue, 11 Nov 2008 12:19:24 +0300, Sergei Shtylyov
<sshtylyov@ru.mvista.com> wrote:
> Hello.
> 

>> Mmm.. but he's using "really old ata_piix" hardware, as in what Intel
>> once called the "Triton" (or Triton II) chipset.
> 
>    The "original Triton" IDE is supported by pata_oldpiix.
> 
>>   Which I wrote support
>> for in drivers/ide, way back when.. and we never had this problem.
> 
>    The support for the "original Triton" is drivers/ide/ is still broken 
> after all these years. The driver assumes that the slave IDE timing 
> register always present -- which the origina 82371FB didn't have. :-(

00:07.1 IDE interface: Intel Corporation 82371SB PIIX3 IDE [Natoma/Triton
II]

So apparently I have an "SB" Triton II. Looking at the description of the
drivers I thought that ata_piix would be the correct driver for it.

... support for PATA on the Intel ESB/ICH/PIIX3/PIIX4 series host
controllers.

Also looking the the comments in ata_piix it looks like my chipset already
supports independent timing and therefore ata_piix is the correct driver to
use.

Please correct me if I am wrong or misunderstood your remarks.

Kind regards,
Michael

Re: Fwd: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Mark Lord]

Michael Guntsche wrote:
>
> So apparently I have an "SB" Triton II. Looking at the description of the
> drivers I thought that ata_piix would be the correct driver for it.
> 
> ... support for PATA on the Intel ESB/ICH/PIIX3/PIIX4 series host
> controllers.
> 
> Also looking the the comments in ata_piix it looks like my chipset already
> supports independent timing and therefore ata_piix is the correct driver to
> use.
..

Yeah, that's the right one.  But if you are technically apt,
then perhaps you could try a brief experiment for us:

Use drivers/ide instead, and see what it reports in the boot log.

One simple way to do this, is to boot a Knoppix CD, say v5.01 or so
(but not the newest version, which probably uses libata instead).

Then just grab the boot log with dmesg, and post it here.

Thanks

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Guntsche Michael]

On Nov 11, 2008, at 15:29, Mark Lord wrote:
> .
>
> Yeah, that's the right one.  But if you are technically apt,
> then perhaps you could try a brief experiment for us:
>
> Use drivers/ide instead, and see what it reports in the boot log.
>
> One simple way to do this, is to boot a Knoppix CD, say v5.01 or so
> (but not the newest version, which probably uses libata instead).
>
> Then just grab the boot log with dmesg, and post it here.

Ok, recompiled the kernel with the old driver, here the relevant dmesg  
output.

Uniform Multi-Platform E-IDE driver
piix 0000:00:07.1: IDE controller (0x8086:0x7010 rev 0x00)
piix 0000:00:07.1: not 100% native mode: will probe irqs later
     ide0: BM-DMA at 0xe800-0xe807
     ide1: BM-DMA at 0xe808-0xe80f
Probing IDE interface ide0...
hda: IC35L040AVER07-0, ATA DISK drive
hda: host max PIO4 wanted PIO255(auto-tune) selected PIO4
hda: MWDMA2 mode selected
Probing IDE interface ide1...
hdc: SAMSUNG CD-R/RW SW-408B, ATAPI CD/DVD-ROM drive
hdc: host max PIO4 wanted PIO255(auto-tune) selected PIO4
hdc: MWDMA2 mode selected
ide0 at 0x1f0-0x1f7,0x3f6 on irq 14
ide1 at 0x170-0x177,0x376 on irq 15

Hope that helps,
Mike

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Mark Lord]

Guntsche Michael wrote:
> 
> On Nov 11, 2008, at 15:29, Mark Lord wrote:
>> .
>>
>> Yeah, that's the right one.  But if you are technically apt,
>> then perhaps you could try a brief experiment for us:
>>
>> Use drivers/ide instead, and see what it reports in the boot log.
>>
>> One simple way to do this, is to boot a Knoppix CD, say v5.01 or so
>> (but not the newest version, which probably uses libata instead).
>>
>> Then just grab the boot log with dmesg, and post it here.
> 
> Ok, recompiled the kernel with the old driver, here the relevant dmesg 
> output.
> 
> Uniform Multi-Platform E-IDE driver
> piix 0000:00:07.1: IDE controller (0x8086:0x7010 rev 0x00)
> piix 0000:00:07.1: not 100% native mode: will probe irqs later
>     ide0: BM-DMA at 0xe800-0xe807
>     ide1: BM-DMA at 0xe808-0xe80f
> Probing IDE interface ide0...
> hda: IC35L040AVER07-0, ATA DISK drive
> hda: host max PIO4 wanted PIO255(auto-tune) selected PIO4
> hda: MWDMA2 mode selected
> Probing IDE interface ide1...
> hdc: SAMSUNG CD-R/RW SW-408B, ATAPI CD/DVD-ROM drive
> hdc: host max PIO4 wanted PIO255(auto-tune) selected PIO4
> hdc: MWDMA2 mode selected
> ide0 at 0x1f0-0x1f7,0x3f6 on irq 14
> ide1 at 0x170-0x177,0x376 on irq 15
..

Mmm.. no phantom device showing up there,
so I suppose what's left of my ancient device probing
is still mostly working (does any of it remain now?).

Tejun?

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Tejun Heo]

[-- Attachment #1: Type: text/plain, Size: 1589 bytes --]

Mark Lord wrote:
> Guntsche Michael wrote:
>>
>> On Nov 11, 2008, at 15:29, Mark Lord wrote:
>>> .
>>>
>>> Yeah, that's the right one.  But if you are technically apt,
>>> then perhaps you could try a brief experiment for us:
>>>
>>> Use drivers/ide instead, and see what it reports in the boot log.
>>>
>>> One simple way to do this, is to boot a Knoppix CD, say v5.01 or so
>>> (but not the newest version, which probably uses libata instead).
>>>
>>> Then just grab the boot log with dmesg, and post it here.
>>
>> Ok, recompiled the kernel with the old driver, here the relevant dmesg
>> output.
>>
>> Uniform Multi-Platform E-IDE driver
>> piix 0000:00:07.1: IDE controller (0x8086:0x7010 rev 0x00)
>> piix 0000:00:07.1: not 100% native mode: will probe irqs later
>>     ide0: BM-DMA at 0xe800-0xe807
>>     ide1: BM-DMA at 0xe808-0xe80f
>> Probing IDE interface ide0...
>> hda: IC35L040AVER07-0, ATA DISK drive
>> hda: host max PIO4 wanted PIO255(auto-tune) selected PIO4
>> hda: MWDMA2 mode selected
>> Probing IDE interface ide1...
>> hdc: SAMSUNG CD-R/RW SW-408B, ATAPI CD/DVD-ROM drive
>> hdc: host max PIO4 wanted PIO255(auto-tune) selected PIO4
>> hdc: MWDMA2 mode selected
>> ide0 at 0x1f0-0x1f7,0x3f6 on irq 14
>> ide1 at 0x170-0x177,0x376 on irq 15
> ..
> 
> Mmm.. no phantom device showing up there,
> so I suppose what's left of my ancient device probing
> is still mostly working (does any of it remain now?).

Looks like we screwed up phantom device detection somewhere.  Michael,
can you please apply the attached patch and report kernel boot log?

Thanks.

-- 
tejun

[-- Attachment #2: phantom-debug.patch --]
[-- Type: text/x-patch, Size: 1639 bytes --]

diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index 4b47394..abbb5f0 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -1226,11 +1226,16 @@ fsm_start:
 		} else {
 			/* ATA PIO protocol */
 			if (unlikely((status & ATA_DRQ) == 0)) {
+				ata_dev_printk(qc->dev, KERN_INFO, "XXX status=0x%x hdiag=%d\n",
+					       status, qc->dev->horkage & ATA_HORKAGE_DIAGNOSTIC);
+
 				/* handle BSY=0, DRQ=0 as error */
-				if (likely(status & (ATA_ERR | ATA_DF)))
+				if (likely(status & (ATA_ERR | ATA_DF))) {
 					/* device stops HSM for abort/error */
 					qc->err_mask |= AC_ERR_DEV;
-				else {
+					if (qc->dev->horkage & ATA_HORKAGE_DIAGNOSTIC)
+						qc->err_mask |= AC_ERR_NODEV_HINT;
+				} else {
 					/* HSM violation. Let EH handle this.
 					 * Phantom devices also trigger this
 					 * condition.  Mark hint.
@@ -1838,6 +1843,8 @@ unsigned int ata_sff_dev_classify(struct ata_device *dev, int present,
 
 	/* determine if device is ATA or ATAPI */
 	class = ata_dev_classify(&tf);
+	ata_dev_printk(dev, KERN_INFO, "XXX sff_dev_classify present=%d hdiag=%d tf=%02x:%02x:%02x class=%d\n",
+		       present, dev->horkage & ATA_HORKAGE_DIAGNOSTIC, tf.lbal, tf.lbam, tf.lbah, class);
 
 	if (class == ATA_DEV_UNKNOWN) {
 		/* If the device failed diagnostic, it's likely to
@@ -1981,6 +1988,7 @@ int ata_sff_softreset(struct ata_link *link, unsigned int *classes,
 		devmask |= (1 << 0);
 	if (slave_possible && ata_devchk(ap, 1))
 		devmask |= (1 << 1);
+	ata_link_printk(link, KERN_INFO, "XXX devmask=%x\n", devmask);
 
 	/* select device 0 again */
 	ap->ops->sff_dev_select(ap, 0);

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Michael Guntsche]

On Wed, 12 Nov 2008 11:34:19 +0900, Tejun Heo <tj@kernel.org> wrote:

> Looks like we screwed up phantom device detection somewhere.  Michael,
> can you please apply the attached patch and report kernel boot log?

Here the relevant dmesg output

ata_piix 0000:00:07.1: version 2.12
scsi0 : ata_piix
scsi1 : ata_piix
ata1: PATA max MWDMA2 cmd 0x1f0 ctl 0x3f6 bmdma 0xe800 irq 14
ata2: PATA max MWDMA2 cmd 0x170 ctl 0x376 bmdma 0xe808 irq 15
ata1: XXX devmask=3
ata1.00: XXX sff_dev_classify present=1 hdiag=0 tf=01:00:00 class=1
ata1.01: XXX sff_dev_classify present=2 hdiag=0 tf=01:00:00 class=1
ata1.00: ATA-5: IC35L040AVER07-0, ER4OA45A, max UDMA/100
ata1.00: 66055248 sectors, multi 16: LBA
ata1.00: configured for MWDMA2
ata2: XXX devmask=1
ata2.00: XXX sff_dev_classify present=1 hdiag=0 tf=01:14:eb class=3
ata2.01: XXX sff_dev_classify present=0 hdiag=1 tf=ff:00:00 class=1
ata2.01: XXX status=0x1 hdiag=1
ata2.01: NODEV after polling detection
ata2.00: ATAPI: SAMSUNG CD-R/RW SW-408B, M300, max MWDMA2
ata2.00: configured for MWDMA2

Hope this helps,
Michael

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Tejun Heo]

[-- Attachment #1: Type: text/plain, Size: 1174 bytes --]

Michael Guntsche wrote:
> On Wed, 12 Nov 2008 11:34:19 +0900, Tejun Heo <tj@kernel.org> wrote:
> 
>> Looks like we screwed up phantom device detection somewhere.  Michael,
>> can you please apply the attached patch and report kernel boot log?
> 
> Here the relevant dmesg output
> 
> ata_piix 0000:00:07.1: version 2.12
> scsi0 : ata_piix
> scsi1 : ata_piix
> ata1: PATA max MWDMA2 cmd 0x1f0 ctl 0x3f6 bmdma 0xe800 irq 14
> ata2: PATA max MWDMA2 cmd 0x170 ctl 0x376 bmdma 0xe808 irq 15
> ata1: XXX devmask=3
> ata1.00: XXX sff_dev_classify present=1 hdiag=0 tf=01:00:00 class=1
> ata1.01: XXX sff_dev_classify present=2 hdiag=0 tf=01:00:00 class=1
> ata1.00: ATA-5: IC35L040AVER07-0, ER4OA45A, max UDMA/100
> ata1.00: 66055248 sectors, multi 16: LBA
> ata1.00: configured for MWDMA2
> ata2: XXX devmask=1
> ata2.00: XXX sff_dev_classify present=1 hdiag=0 tf=01:14:eb class=3
> ata2.01: XXX sff_dev_classify present=0 hdiag=1 tf=ff:00:00 class=1
> ata2.01: XXX status=0x1 hdiag=1
> ata2.01: NODEV after polling detection
> ata2.00: ATAPI: SAMSUNG CD-R/RW SW-408B, M300, max MWDMA2
> ata2.00: configured for MWDMA2

Can you please test the attached patch?

Thanks.

-- 
tejun

[-- Attachment #2: phantom-fix.patch --]
[-- Type: text/x-patch, Size: 5782 bytes --]

diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index 4b47394..9859705 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -1083,7 +1083,6 @@ static void ata_hsm_qc_complete(struct ata_queued_cmd *qc, int in_wq)
 
 /**
  *	ata_sff_hsm_move - move the HSM to the next state.
- *	@ap: the target ata_port
  *	@qc: qc on going
  *	@status: current device status
  *	@in_wq: 1 if called from workqueue, 0 otherwise
@@ -1091,9 +1090,11 @@ static void ata_hsm_qc_complete(struct ata_queued_cmd *qc, int in_wq)
  *	RETURNS:
  *	1 when poll next status needed, 0 otherwise.
  */
-int ata_sff_hsm_move(struct ata_port *ap, struct ata_queued_cmd *qc,
-		     u8 status, int in_wq)
+int ata_sff_hsm_move(struct ata_queued_cmd *qc, u8 status, int in_wq)
 {
+	struct ata_port *ap = qc->ap;
+	struct ata_device *dev = qc->dev;
+	struct ata_eh_context *ehc = &ap->link.eh_context;
 	struct ata_eh_info *ehi = &ap->link.eh_info;
 	unsigned long flags = 0;
 	int poll_next;
@@ -1227,10 +1228,15 @@ fsm_start:
 			/* ATA PIO protocol */
 			if (unlikely((status & ATA_DRQ) == 0)) {
 				/* handle BSY=0, DRQ=0 as error */
-				if (likely(status & (ATA_ERR | ATA_DF)))
+				if (likely(status & (ATA_ERR | ATA_DF))) {
 					/* device stops HSM for abort/error */
 					qc->err_mask |= AC_ERR_DEV;
-				else {
+
+					if (!(ehc->sff_devmask &
+					      (1 << dev->devno)))
+						qc->err_mask |=
+							AC_ERR_NODEV_HINT;
+				} else {
 					/* HSM violation. Let EH handle this.
 					 * Phantom devices also trigger this
 					 * condition.  Mark hint.
@@ -1359,7 +1365,7 @@ fsm_start:
 	}
 
 	/* move the HSM */
-	poll_next = ata_sff_hsm_move(ap, qc, status, 1);
+	poll_next = ata_sff_hsm_move(qc, status, 1);
 
 	/* another command or interrupt handler
 	 * may be running at this point.
@@ -1593,7 +1599,7 @@ inline unsigned int ata_sff_host_intr(struct ata_port *ap,
 	/* ack bmdma irq events */
 	ap->ops->sff_irq_clear(ap);
 
-	ata_sff_hsm_move(ap, qc, status, 0);
+	ata_sff_hsm_move(qc, status, 0);
 
 	if (unlikely(qc->err_mask) && (qc->tf.protocol == ATA_PROT_DMA ||
 				       qc->tf.protocol == ATAPI_PROT_DMA))
@@ -1969,25 +1975,26 @@ int ata_sff_softreset(struct ata_link *link, unsigned int *classes,
 		      unsigned long deadline)
 {
 	struct ata_port *ap = link->ap;
+	struct ata_eh_context *ehc = &link->eh_context;
 	unsigned int slave_possible = ap->flags & ATA_FLAG_SLAVE_POSS;
-	unsigned int devmask = 0;
 	int rc;
 	u8 err;
 
 	DPRINTK("ENTER\n");
 
 	/* determine if device 0/1 are present */
+	ehc->sff_devmask = 0;
 	if (ata_devchk(ap, 0))
-		devmask |= (1 << 0);
+		ehc->sff_devmask |= (1 << 0);
 	if (slave_possible && ata_devchk(ap, 1))
-		devmask |= (1 << 1);
+		ehc->sff_devmask |= (1 << 1);
 
 	/* select device 0 again */
 	ap->ops->sff_dev_select(ap, 0);
 
 	/* issue bus reset */
-	DPRINTK("about to softreset, devmask=%x\n", devmask);
-	rc = ata_bus_softreset(ap, devmask, deadline);
+	DPRINTK("about to softreset, devmask=%x\n", ehc->sff_devmask);
+	rc = ata_bus_softreset(ap, ehc->sff_devmask, deadline);
 	/* if link is occupied, -ENODEV too is an error */
 	if (rc && (rc != -ENODEV || sata_scr_valid(link))) {
 		ata_link_printk(link, KERN_ERR, "SRST failed (errno=%d)\n", rc);
@@ -1996,10 +2003,10 @@ int ata_sff_softreset(struct ata_link *link, unsigned int *classes,
 
 	/* determine by signature whether we have ATA or ATAPI devices */
 	classes[0] = ata_sff_dev_classify(&link->device[0],
-					  devmask & (1 << 0), &err);
+					  ehc->sff_devmask & (1 << 0), &err);
 	if (slave_possible && err != 0x81)
 		classes[1] = ata_sff_dev_classify(&link->device[1],
-						  devmask & (1 << 1), &err);
+					ehc->sff_devmask & (1 << 1), &err);
 
 	DPRINTK("EXIT, classes[0]=%u [1]=%u\n", classes[0], classes[1]);
 	return 0;
diff --git a/drivers/ata/pata_bf54x.c b/drivers/ata/pata_bf54x.c
index 1266924..1458ce8 100644
--- a/drivers/ata/pata_bf54x.c
+++ b/drivers/ata/pata_bf54x.c
@@ -1406,7 +1406,7 @@ static unsigned int bfin_ata_host_intr(struct ata_port *ap,
 	/* ack bmdma irq events */
 	ap->ops->sff_irq_clear(ap);
 
-	ata_sff_hsm_move(ap, qc, status, 0);
+	ata_sff_hsm_move(qc, status, 0);
 
 	if (unlikely(qc->err_mask) && (qc->tf.protocol == ATA_PROT_DMA ||
 				       qc->tf.protocol == ATAPI_PROT_DMA))
diff --git a/drivers/ata/sata_sil.c b/drivers/ata/sata_sil.c
index 031d7b7..061f471 100644
--- a/drivers/ata/sata_sil.c
+++ b/drivers/ata/sata_sil.c
@@ -413,7 +413,7 @@ static void sil_host_intr(struct ata_port *ap, u32 bmdma2)
 	ata_sff_irq_clear(ap);
 
 	/* kick HSM in the ass */
-	ata_sff_hsm_move(ap, qc, status, 0);
+	ata_sff_hsm_move(qc, status, 0);
 
 	if (unlikely(qc->err_mask) && ata_is_dma(qc->tf.protocol))
 		ata_ehi_push_desc(ehi, "BMDMA2 stat 0x%x", bmdma2);
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 59b0f1c..0a14022 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -639,6 +639,9 @@ struct ata_eh_context {
 	u8			saved_xfer_mode[ATA_MAX_DEVICES];
 	/* timestamp for the last reset attempt or success */
 	unsigned long		last_reset;
+#ifdef CONFIG_ATA_SFF
+	unsigned int		sff_devmask;
+#endif
 };
 
 struct ata_acpi_drive
@@ -1511,8 +1514,7 @@ extern unsigned int ata_sff_data_xfer_noirq(struct ata_device *dev,
 			unsigned char *buf, unsigned int buflen, int rw);
 extern u8 ata_sff_irq_on(struct ata_port *ap);
 extern void ata_sff_irq_clear(struct ata_port *ap);
-extern int ata_sff_hsm_move(struct ata_port *ap, struct ata_queued_cmd *qc,
-			    u8 status, int in_wq);
+extern int ata_sff_hsm_move(struct ata_queued_cmd *qc, u8 status, int in_wq);
 extern unsigned int ata_sff_qc_issue(struct ata_queued_cmd *qc);
 extern bool ata_sff_qc_fill_rtf(struct ata_queued_cmd *qc);
 extern unsigned int ata_sff_host_intr(struct ata_port *ap,

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Michael Guntsche]

On Wed, 12 Nov 2008 17:15:58 +0900, Tejun Heo <tj@kernel.org> wrote:

> Can you please test the attached patch?


ata_piix 0000:00:07.1: version 2.12
scsi0 : ata_piix
scsi1 : ata_piix
ata1: PATA max MWDMA2 cmd 0x1f0 ctl 0x3f6 bmdma 0xe800 irq 14
ata2: PATA max MWDMA2 cmd 0x170 ctl 0x376 bmdma 0xe808 irq 15
ata1.00: ATA-5: IC35L040AVER07-0, ER4OA45A, max UDMA/100
ata1.00: 66055248 sectors, multi 16: LBA
ata1.00: configured for MWDMA2
ata2.01: NODEV after polling detection
ata2.00: ATAPI: SAMSUNG CD-R/RW SW-408B, M300, max MWDMA2
ata2.00: configured for MWDMA2

This is the output I get. I do not know if I have any stalls there since I
am not in front of the machine right now.
But if I understand "NODEV after polling detection" correctly, the code is
no able to see that there is nothing attached there.
I will give it another reboot when I am back home to see if the stall is
still happening.

On a side note, since we are already talking about old chipsets and mobos.
Since this is a rather old hardware it was not possible to get the 40GB IBM
disk running in the first place. 
What I did back then (2.4 kernel days) was to use "ibmsetmax" to clamp the
disk to a smaller size and enabled the relevent option in the kernel. This
way I got past the BIOS (it stopped if the harddisk was saying it was >
32GB??) but the linux kernel correctly saw the right size.
Looking ad the dmesg output right now I think that it is only seeing the
smaller size.

sd 0:0:0:0: [sda] 66055248 512-byte hardware sectors (33820 MB)
sd 0:0:0:0: [sda] Write Protect is off
sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00
sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't
support DPO

ibmsetmax output:
 ./ibmsetmax /dev/sda
Using device /dev/sda
native max address: 66055247
that is 33820286976 bytes, 33.8 GB
lba capacity: 66055248 sectors (33820286976 bytes)

Is the clamping feature present in 2.6 by default? I cannot test this with
a 2.4 kernel right now, since the libc no longer supports it. :)

Anyway thank you all very much for your time and support,
Michael

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Tejun Heo]

Hello,

Michael Guntsche wrote:
> ata_piix 0000:00:07.1: version 2.12
> scsi0 : ata_piix
> scsi1 : ata_piix
> ata1: PATA max MWDMA2 cmd 0x1f0 ctl 0x3f6 bmdma 0xe800 irq 14
> ata2: PATA max MWDMA2 cmd 0x170 ctl 0x376 bmdma 0xe808 irq 15
> ata1.00: ATA-5: IC35L040AVER07-0, ER4OA45A, max UDMA/100
> ata1.00: 66055248 sectors, multi 16: LBA
> ata1.00: configured for MWDMA2
> ata2.01: NODEV after polling detection
> ata2.00: ATAPI: SAMSUNG CD-R/RW SW-408B, M300, max MWDMA2
> ata2.00: configured for MWDMA2
> 
> This is the output I get. I do not know if I have any stalls there since I
> am not in front of the machine right now.
> But if I understand "NODEV after polling detection" correctly, the code is
> no able to see that there is nothing attached there.
> I will give it another reboot when I am back home to see if the stall is
> still happening.

Yeap, it's working correctly now.

> On a side note, since we are already talking about old chipsets and mobos.
> Since this is a rather old hardware it was not possible to get the 40GB IBM
> disk running in the first place. 
> What I did back then (2.4 kernel days) was to use "ibmsetmax" to clamp the
> disk to a smaller size and enabled the relevent option in the kernel. This
> way I got past the BIOS (it stopped if the harddisk was saying it was >
> 32GB??) but the linux kernel correctly saw the right size.
> Looking ad the dmesg output right now I think that it is only seeing the
> smaller size.
> 
> sd 0:0:0:0: [sda] 66055248 512-byte hardware sectors (33820 MB)
> sd 0:0:0:0: [sda] Write Protect is off
> sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00
> sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't
> support DPO
> 
> ibmsetmax output:
>  ./ibmsetmax /dev/sda
> Using device /dev/sda
> native max address: 66055247
> that is 33820286976 bytes, 33.8 GB
> lba capacity: 66055248 sectors (33820286976 bytes)
> 
> Is the clamping feature present in 2.6 by default? I cannot test this with
> a 2.4 kernel right now, since the libc no longer supports it. :)

Does libata.ignore_hpa=1 help?

-- 
tejun

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Michael Guntsche]

On Wed, 12 Nov 2008 18:27:38 +0900, Tejun Heo <tj@kernel.org> wrote:
> Hello,
> Yeap, it's working correctly now.

Good to know

> Does libata.ignore_hpa=1 help?

No it does not.

/proc/cmdline:
==============

auto BOOT_IMAGE=Linux ro root=802 libata.ignore_hpa=1

dmesg:
======

ata1.00: ATA-5: IC35L040AVER07-0, ER4OA45A, max UDMA/100
ata1.00: 66055248 sectors, multi 16: LBA
ata1.00: configured for MWDMA2
ata2.01: NODEV after polling detection
ata2.00: ATAPI: SAMSUNG CD-R/RW SW-408B, M300, max MWDMA2
ata2.00: configured for MWDMA2
scsi 0:0:0:0: Direct-Access     ATA      IC35L040AVER07-0 ER4O PQ: 0 ANSI:
5
sd 0:0:0:0: [sda] 66055248 512-byte hardware sectors (33820 MB)
sd 0:0:0:0: [sda] Write Protect is off
sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00
sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't
support DPO or FUA
sd 0:0:0:0: [sda] 66055248 512-byte hardware sectors (33820 MB)
sd 0:0:0:0: [sda] Write Protect is off
sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00
sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't
support DPO or FUA

ibmsetmax:
==========

Using device /dev/sda
native max address: 66055247
that is 33820286976 bytes, 33.8 GB
lba capacity: 66055248 sectors (33820286976 bytes)

Kind regards,
Michael

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Tejun Heo]

Michael Guntsche wrote:
> On Wed, 12 Nov 2008 18:27:38 +0900, Tejun Heo <tj@kernel.org> wrote:
>> Hello,
>> Yeap, it's working correctly now.
> 
> Good to know
> 
>> Does libata.ignore_hpa=1 help?
> 
> No it does not.
> 
> /proc/cmdline:
> ==============
> 
> auto BOOT_IMAGE=Linux ro root=802 libata.ignore_hpa=1
> 
> dmesg:
> ======
> 
> ata1.00: ATA-5: IC35L040AVER07-0, ER4OA45A, max UDMA/100
> ata1.00: 66055248 sectors, multi 16: LBA
> ata1.00: configured for MWDMA2
> ata2.01: NODEV after polling detection
> ata2.00: ATAPI: SAMSUNG CD-R/RW SW-408B, M300, max MWDMA2
> ata2.00: configured for MWDMA2
> scsi 0:0:0:0: Direct-Access     ATA      IC35L040AVER07-0 ER4O PQ: 0 ANSI:
> 5
> sd 0:0:0:0: [sda] 66055248 512-byte hardware sectors (33820 MB)
> sd 0:0:0:0: [sda] Write Protect is off
> sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00
> sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't
> support DPO or FUA
> sd 0:0:0:0: [sda] 66055248 512-byte hardware sectors (33820 MB)
> sd 0:0:0:0: [sda] Write Protect is off
> sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00
> sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't
> support DPO or FUA
> 
> ibmsetmax:
> ==========
> 
> Using device /dev/sda
> native max address: 66055247
> that is 33820286976 bytes, 33.8 GB
> lba capacity: 66055248 sectors (33820286976 bytes)

Care to post "hdparm -I /dev/sda"?

-- 
tejun

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Michael Guntsche]

On Wed, 12 Nov 2008 18:48:50 +0900, Tejun Heo <tj@kernel.org> wrote:

> Care to post "hdparm -I /dev/sda"?

Here it is.

/dev/sda:

ATA device, with non-removable media
powers-up in standby; SET FEATURES subcmd spins-up.
        Model Number:       IC35L040AVER07-0
        Serial Number:      SXPTXGK0136
        Firmware Revision:  ER4OA45A
Standards:
        Used: ATA/ATAPI-5 T13 1321D revision 1
        Supported: 5 4 3 & some of 6
Configuration:
        Logical         max     current
        cylinders       16383   16383
        heads           16      16
        sectors/track   63      63
        --
        CHS current addressable sectors:   16514064
        LBA    user addressable sectors:   66055248
        device size with M = 1024*1024:       32253 MBytes
        device size with M = 1000*1000:       33820 MBytes (33 GB)
Capabilities:
        LBA, IORDY(can be disabled)
        bytes avail on r/w long: 40     Queue depth: 32
        Standby timer values: spec'd by Standard, no device specific
minimum
        R/W multiple sector transfer: Max = 16  Current = 16
        Advanced power management level: disabled
        Recommended acoustic management value: 128, current value: 254
        DMA: mdma0 mdma1 *mdma2 udma0 udma1 udma2 udma3 udma4 udma5
             Cycle time: min=120ns recommended=120ns
        PIO: pio0 pio1 pio2 pio3 pio4
             Cycle time: no flow control=240ns  IORDY flow control=120ns
Commands/features:
        Enabled Supported:
           *    SMART feature set
                Security Mode feature set
           *    Power Management feature set
           *    Write cache
           *    Look-ahead
                Release interrupt
           *    Host Protected Area feature set
           *    WRITE_BUFFER command
           *    READ_BUFFER command
           *    NOP cmd
           *    READ/WRITE_DMA_QUEUED
                Advanced Power Management feature set
                Power-Up In Standby feature set
                SET_FEATURES required to spinup after power up
                Address Offset Reserved Area Boot
                SET_MAX security extension
           *    Automatic Acoustic Management feature set
           *    Device Configuration Overlay feature set
Security:
        Master password revision code = 65534
                supported
        not     enabled
        not     locked
        not     frozen
        not     expired: security count
        not     supported: enhanced erase
        30min for SECURITY ERASE UNIT.
HW reset results:
        CBLID- above Vih
        Device num = 0 determined by the jumper
Checksum: correct


/Mike

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Mark Lord]

Michael Guntsche wrote:
> On Wed, 12 Nov 2008 18:48:50 +0900, Tejun Heo <tj@kernel.org> wrote:
> 
>> Care to post "hdparm -I /dev/sda"?
> 
> Here it is.
> 
> /dev/sda:
> 
> ATA device, with non-removable media
> powers-up in standby; SET FEATURES subcmd spins-up.
>         Model Number:       IC35L040AVER07-0
>         Serial Number:      SXPTXGK0136
>         Firmware Revision:  ER4OA45A
> Standards:
>         Used: ATA/ATAPI-5 T13 1321D revision 1
>         Supported: 5 4 3 & some of 6
> Configuration:
>         Logical         max     current
>         cylinders       16383   16383
>         heads           16      16
>         sectors/track   63      63
>         --
>         CHS current addressable sectors:   16514064
>         LBA    user addressable sectors:   66055248
>         device size with M = 1024*1024:       32253 MBytes
>         device size with M = 1000*1000:       33820 MBytes (33 GB)
> Capabilities:
>         LBA, IORDY(can be disabled)
>         bytes avail on r/w long: 40     Queue depth: 32
>         Standby timer values: spec'd by Standard, no device specific
> minimum
>         R/W multiple sector transfer: Max = 16  Current = 16
>         Advanced power management level: disabled
>         Recommended acoustic management value: 128, current value: 254
>         DMA: mdma0 mdma1 *mdma2 udma0 udma1 udma2 udma3 udma4 udma5
>              Cycle time: min=120ns recommended=120ns
>         PIO: pio0 pio1 pio2 pio3 pio4
>              Cycle time: no flow control=240ns  IORDY flow control=120ns
> Commands/features:
>         Enabled Supported:
>            *    SMART feature set
>                 Security Mode feature set
>            *    Power Management feature set
>            *    Write cache
>            *    Look-ahead
>                 Release interrupt
>            *    Host Protected Area feature set
>            *    WRITE_BUFFER command
>            *    READ_BUFFER command
>            *    NOP cmd
>            *    READ/WRITE_DMA_QUEUED
>                 Advanced Power Management feature set
>                 Power-Up In Standby feature set
>                 SET_FEATURES required to spinup after power up
>                 Address Offset Reserved Area Boot
>                 SET_MAX security extension
>            *    Automatic Acoustic Management feature set
>            *    Device Configuration Overlay feature set
> Security:
>         Master password revision code = 65534
>                 supported
>         not     enabled
>         not     locked
>         not     frozen
>         not     expired: security count
>         not     supported: enhanced erase
>         30min for SECURITY ERASE UNIT.
> HW reset results:
>         CBLID- above Vih
>         Device num = 0 determined by the jumper
> Checksum: correct
..

One more:  Let's see "hdparm -N" for that drive
(note to self: add -N functionality to -I someday..).

Thanks

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Michael Guntsche]

On Thu, 13 Nov 2008 21:38:30 -0500, Mark Lord <liml@rtr.ca> wrote:

> One more:  Let's see "hdparm -N" for that drive
> (note to self: add -N functionality to -I someday..).

/dev/sda:
 max sectors   = 66055248/15723600, HPA setting seems invalid

Hmm, is the "HPA setting seems invalid" an indication that this drive is
clamped?

Hope that helps,
Mike

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Mark Lord]

Michael Guntsche wrote:
> On Thu, 13 Nov 2008 21:38:30 -0500, Mark Lord <liml@rtr.ca> wrote:
> 
>> One more:  Let's see "hdparm -N" for that drive
>> (note to self: add -N functionality to -I someday..).
> 
> /dev/sda:
>  max sectors   = 66055248/15723600, HPA setting seems invalid
> 
> Hmm, is the "HPA setting seems invalid" an indication that this drive is clamped?
..

It's definitely clamped, but that output is a sign that you
may need a newer version of hdparm -- could you try that again
using hdparm-9.3 from sourceforge?

Thanks

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Mark Lord]

Mark Lord wrote:
> Michael Guntsche wrote:
>> On Thu, 13 Nov 2008 21:38:30 -0500, Mark Lord <liml@rtr.ca> wrote:
>>
>>> One more:  Let's see "hdparm -N" for that drive
>>> (note to self: add -N functionality to -I someday..).
>>
>> /dev/sda:
>>  max sectors   = 66055248/15723600, HPA setting seems invalid
>>
>> Hmm, is the "HPA setting seems invalid" an indication that this drive 
>> is clamped?
> ..
> 
> It's definitely clamped, but that output is a sign that you
> may need a newer version of hdparm -- could you try that again
> using hdparm-9.3 from sourceforge?
..

To clarify, the second number reported should NEVER be less than
the first number.  If it is, then there's a bug in the low-level
libata driver for your SATA/IDE controller -- it's not returning
the high-order-bytes from the SAT command.  Several drivers had
this bug until 2.6.27 or so, and some may still misbehave.

A newer version of hdparm will verify that, and also show the correct
value (hopefully) obtained via a completely different mechanism.

Thanks

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Guntsche Michael]

On Nov 14, 2008, at 18:24, Mark Lord wrote:

> To clarify, the second number reported should NEVER be less than
> the first number.  If it is, then there's a bug in the low-level
> libata driver for your SATA/IDE controller -- it's not returning
> the high-order-bytes from the SAT command.  Several drivers had
> this bug until 2.6.27 or so, and some may still misbehave.
>
> A newer version of hdparm will verify that, and also show the correct
> value (hopefully) obtained via a completely different mechanism.
>
> Thanks

Here the new output, it still does not look right, or am I wrong?

/dev/sda:
  max sectors   = 66055248/15723600(66055248?), HPA setting seems  
invalid (buggy kernel device driver?)

Kind regards,
Michael

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Mark Lord]

Guntsche Michael wrote:
> 
> On Nov 14, 2008, at 18:24, Mark Lord wrote:
> 
>> To clarify, the second number reported should NEVER be less than
>> the first number.  If it is, then there's a bug in the low-level
>> libata driver for your SATA/IDE controller -- it's not returning
>> the high-order-bytes from the SAT command.  Several drivers had
>> this bug until 2.6.27 or so, and some may still misbehave.
>>
>> A newer version of hdparm will verify that, and also show the correct
>> value (hopefully) obtained via a completely different mechanism.
>>
>> Thanks
> 
> Here the new output, it still does not look right, or am I wrong?
> 
> /dev/sda:
>  max sectors   = 66055248/15723600(66055248?), HPA setting seems invalid 
> (buggy kernel device driver?)
..

Ahh, okay, no HPA in effect.

   max sectors = 66055248/ ... (66055248)

That's the entire drive, ignoring the buggy libata driver
that reports 15723600 (returns zeros for hob_lba[mhl]).

Michael Guntsche wrote:
> On a side note, since we are already talking about old chipsets and mobos.
> Since this is a rather old hardware it was not possible to get the 40GB IBM
> disk running in the first place. 
> What I did back then (2.4 kernel days) was to use "ibmsetmax" to clamp the
> disk to a smaller size and enabled the relevent option in the kernel. This
> way I got past the BIOS (it stopped if the harddisk was saying it was >
> 32GB??) but the linux kernel correctly saw the right size.
> Looking ad the dmesg output right now I think that it is only seeing the
> smaller size.
> 
> sd 0:0:0:0: [sda] 66055248 512-byte hardware sectors (33820 MB)
..

Well, according to the drive, it *is* a 33GB drive, not a 40GB drive.
So whatever "ibmsetmax" did appears to be outside of the ATA standard
way of doing it.  Unless I've got a bug in hdparm that I don't know about. :)

> /dev/sda:
> 
> ATA device, with non-removable media
> powers-up in standby; SET FEATURES subcmd spins-up.
>         Model Number:       IC35L040AVER07-0
>         Serial Number:      SXPTXGK0136
>         Firmware Revision:  ER4OA45A
> Standards:
>         Used: ATA/ATAPI-5 T13 1321D revision 1
>         Supported: 5 4 3 & some of 6
> Configuration:
>         Logical         max     current
>         cylinders       16383   16383
>         heads           16      16
>         sectors/track   63      63
>         --
>         CHS current addressable sectors:   16514064
>         LBA    user addressable sectors:   66055248
>         device size with M = 1024*1024:       32253 MBytes
>         device size with M = 1000*1000:       33820 MBytes (33 GB)
..
>           *    Host Protected Area feature set
..

That last line bothers me -- the '*' indicates that an HPA is in effect,
but the -N output indicates otherwise.. mmm... 

Let's see:  66055248 = 0x3efec50.
If we mask off all but the lower 24 bits,
we get 0xefec50 = 15723600,
which matches what the buggy device driver
returned when it dropped the top 24 bits.

So probably no bug in hdparm.
Which is odd, because google tells me that
the IC35L040AVER07-0 really is a 40GB drive.

Mmm..

  

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Mark Lord]

Mark Lord wrote:
..
>>           *    Host Protected Area feature set
> ..
> 
> That last line bothers me -- the '*' indicates that an HPA is in effect,
> but the -N output indicates otherwise.. mmm...
> Let's see:  66055248 = 0x3efec50.
> If we mask off all but the lower 24 bits,
> we get 0xefec50 = 15723600,
> which matches what the buggy device driver
> returned when it dropped the top 24 bits.
> 
> So probably no bug in hdparm.
> Which is odd, because google tells me that
> the IC35L040AVER07-0 really is a 40GB drive.
> 
> Mmm..
..

Oh..
Since you now have hdparm-9.3, what do you get from "hdparm --dco-identify" ??

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Guntsche Michael]

On Nov 15, 2008, at 5:17, Mark Lord wrote:
>
> Oh..
> Since you now have hdparm-9.3, what do you get from "hdparm --dco- 
> identify" ??

DCO Revision: 0x0001
The following features can be selectively disabled via DCO:
	Transfer modes:
		 mdma0 mdma1 mdma2
		 udma0 udma1 udma2 udma3 udma4 udma5
	Real max sectors: 66055248
	ATA command/feature sets:
		 SMART self_test error_log security PUIS TCQ AAM HPA

Hmm, still the same max sectors.

Kind regards,
Michael

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Guntsche Michael]

On Nov 15, 2008, at 5:17, Mark Lord wrote:

> Mark Lord wrote:
>> Which is odd, because google tells me that
>> the IC35L040AVER07-0 really is a 40GB drive.
>> Mmm..

I found the problem Mark and I am SOOOOOO sorry for wasting your time  
here.
Your remark about the drive being 40GB made me think about it again.  
As you know this is a quite old router that's running here.
When moving to 2.6 I also replaced the IBM disk with a new one (had  
several of them lying around back then).
The first one was CLAMPED with ibmsetmax. During the install of the  
new harddisk I clamped the new one with the JUMPERS on the DRIVE ond  
not with ibmsetmax.
I know remember that back then I thought that this would easier and  
more hassle free.
The libata stack, hdparm and everything else is working correctly it's  
the drive that's reporting 32GB!!!!!!!

Once again sorry for troubling you (all) with this non-issue.


Kind regards,
Michael

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Mark Lord]

Guntsche Michael wrote:
> 
> On Nov 15, 2008, at 5:17, Mark Lord wrote:
> 
>> Mark Lord wrote:
>>> Which is odd, because google tells me that
>>> the IC35L040AVER07-0 really is a 40GB drive.
>>> Mmm..
> 
> I found the problem Mark and I am SOOOOOO sorry for wasting your time here.
> Your remark about the drive being 40GB made me think about it again. As 
> you know this is a quite old router that's running here.
> When moving to 2.6 I also replaced the IBM disk with a new one (had 
> several of them lying around back then).
> The first one was CLAMPED with ibmsetmax. During the install of the new 
> harddisk I clamped the new one with the JUMPERS on the DRIVE ond not 
> with ibmsetmax.
> I know remember that back then I thought that this would easier and more 
> hassle free.
> The libata stack, hdparm and everything else is working correctly it's 
> the drive that's reporting 32GB!!!!!!!
..

Ahh.. that's better.  Thanks for figuring this one out for us all!

Cheers

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Tejun Heo]

Mark Lord wrote:
>> The libata stack, hdparm and everything else is working correctly it's
>> the drive that's reporting 32GB!!!!!!!
> ..
> 
> Ahh.. that's better.  Thanks for figuring this one out for us all!

So, the only real bug here is ata_piix not returning HOB registers?
That's weird.  It's ata_piix.  It's not likely to be broken.  :-(

-- 
tejun

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Mark Lord]

Tejun Heo wrote:
> Mark Lord wrote:
>>> The libata stack, hdparm and everything else is working correctly it's
>>> the drive that's reporting 32GB!!!!!!!
>> ..
>>
>> Ahh.. that's better.  Thanks for figuring this one out for us all!
> 
> So, the only real bug here is ata_piix not returning HOB registers?
> That's weird.  It's ata_piix.  It's not likely to be broken.  :-(
..

ata_piix works here for me here, too.
But I think he's using pata_piix, not ata_piix.

Fyi:  sata_sil24 had the HOB bug a few kernels ago,
but it's also now fine in 2.6.27.

-ml

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Michael Guntsche]

On Nov 16, 2008, at 6:49, Mark Lord wrote:

> ata_piix works here for me here, too.
> But I think he's using pata_piix, not ata_piix.
>

No, I am using ata_piix here.

Kind regards,
Michael

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Michael Guntsche]

On Nov 16, 2008, at 6:49, Mark Lord wrote:
> ata_piix works here for me here, too.
> But I think he's using pata_piix, not ata_piix.
>
> Fyi:  sata_sil24 had the HOB bug a few kernels ago,
> but it's also now fine in 2.6.27.

Forgot to mention that. I also tried it with the old IDE code and got  
the same results.
The reason why I am clamping the drive in the first place is that my  
BIOS/chipset hangs during boot if a larger disk is connected.
So maybe the results I am seeing here is "normal" since the hardware  
seems to be at it's limit. I know that clamping with ibmsetmax worked  
back then, but could it be that the Jumper on the Harddisk is doing it  
in a different way?

Kind regards,
Michael

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Sergei Shtylyov]

Hello.

Mark Lord wrote:

>>>> The libata stack, hdparm and everything else is working correctly it's
>>>> the drive that's reporting 32GB!!!!!!!
>>> ..
>>>
>>> Ahh.. that's better.  Thanks for figuring this one out for us all!
>>
>> So, the only real bug here is ata_piix not returning HOB registers?
>> That's weird.  It's ata_piix.  It's not likely to be broken.  :-(
> ..
>
> ata_piix works here for me here, too.
> But I think he's using pata_piix, not ata_piix.

   Huh? IIRC, there has never been pata_piix, just pata_oldpiix that 
only drives 82371FB.

WBR, Sergei

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Alan Cox]

> ata_piix works here for me here, too.
> But I think he's using pata_piix, not ata_piix.

There is no pata_piix ...

Re: Fwd: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Mark Lord]

Sergei Shtylyov wrote:
> Hello.
> 
> Mark Lord wrote:
..
>> Mmm.. but he's using "really old ata_piix" hardware, as in what Intel
>> once called the "Triton" (or Triton II) chipset.
> 
>   The "original Triton" IDE is supported by pata_oldpiix.
> 
>>   Which I wrote support
>> for in drivers/ide, way back when.. and we never had this problem.
> 
>   The support for the "original Triton" is drivers/ide/ is still broken 
> after all these years. The driver assumes that the slave IDE timing 
> register always present -- which the origina 82371FB didn't have. :-(
..

Hey, don't complain to me -- I didn't break that!
Heck, the Linux support for Triton DMA made us the first
widely available O/S with UDMA support for that (or any) chipset!
Predating even Redmond by a good six months or more!

But back then, we still relied upon the BIOS to manage timings
(since it had to set them up to boot anyway).  The b0rked timing
in drivers/ide came from a subsequent "maintainer".  :)

Cheers

Re: Fwd: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Alan Cox]

> Heck, the Linux support for Triton DMA made us the first
> widely available O/S with UDMA support for that (or any) chipset!
> Predating even Redmond by a good six months or more!

Triton 1 didn't have UDMA - are you thinking of the later chipsets ?

Alan

Re: Fwd: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Mark Lord]

Alan Cox wrote:
>> Heck, the Linux support for Triton DMA made us the first
>> widely available O/S with UDMA support for that (or any) chipset!
>> Predating even Redmond by a good six months or more!
> 
> Triton 1 didn't have UDMA - are you thinking of the later chipsets ?
..

Ooops.. poor memory here for terminology.

Triton was the first Intel IDE controller with PCI bus-master DMA,
and Linux was the first O/S to take advantage of it:  82371FB.

[PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Tejun Heo]

There were several places where only enabled devices should be
iterated over but device enabledness wasn't checked.

* IDENTIFY data 40 wire check in cable_is_40wire()
* xfer_mode/ncq_enabled saving in ata_scsi_error()
* DUBIOUS_XFER handling in ata_set_mode()

While at it, reformat comment in cable_is_40wire().

Signed-off-by: Tejun Heo <tj@kernel.org>
---
Jeff, this four patch series fixes iteration bugs and improves
iterators and fix ata_port_detach() related bugs.  The iterator change
is a bit big for #upstream-fixes but it's fairly straight forward and
I verified them in most combinations, so it should be pretty safe.

Thanks.

 drivers/ata/libata-core.c |   18 ++++++++----------
 drivers/ata/libata-eh.c   |    9 +++++++++
 2 files changed, 17 insertions(+), 10 deletions(-)

Index: work/drivers/ata/libata-core.c
===================================================================
--- work.orig/drivers/ata/libata-core.c
+++ work/drivers/ata/libata-core.c
@@ -4169,18 +4169,16 @@ static int cable_is_40wire(struct ata_po
 	if (ap->cbl == ATA_CBL_PATA40_SHORT)
 		return 0;
 	/* If the controller doesn't know we scan
-
-	   - Note: We look for all 40 wire detects at this point.
-	     Any 80 wire detect is taken to be 80 wire cable
-	     because
-	     - In many setups only the one drive (slave if present)
-               will give a valid detect
-             - If you have a non detect capable drive you don't
-               want it to colour the choice
-        */
+	 *
+	 * - Note: We look for all 40 wire detects at this point.  Any
+	 *   80 wire detect is taken to be 80 wire cable because - In
+	 *   many setups only the one drive (slave if present) will
+	 *   give a valid detect - If you have a non detect capable
+	 *   drive you don't want it to colour the choice
+	 */
 	ata_port_for_each_link(link, ap) {
 		ata_link_for_each_dev(dev, link) {
-			if (!ata_is_40wire(dev))
+			if (ata_dev_enabled(dev) && !ata_is_40wire(dev))
 				return 0;
 		}
 	}
Index: work/drivers/ata/libata-eh.c
===================================================================
--- work.orig/drivers/ata/libata-eh.c
+++ work/drivers/ata/libata-eh.c
@@ -603,6 +603,9 @@ void ata_scsi_error(struct Scsi_Host *ho
 			ata_link_for_each_dev(dev, link) {
 				int devno = dev->devno;
 
+				if (!ata_dev_enabled(dev))
+					continue;
+
 				ehc->saved_xfer_mode[devno] = dev->xfer_mode;
 				if (ata_ncq_enabled(dev))
 					ehc->saved_ncq_enabled |= 1 << devno;
@@ -2790,6 +2793,9 @@ int ata_set_mode(struct ata_link *link,
 
 	/* if data transfer is verified, clear DUBIOUS_XFER on ering top */
 	ata_link_for_each_dev(dev, link) {
+		if (!ata_dev_enabled(dev))
+			continue;
+
 		if (!(dev->flags & ATA_DFLAG_DUBIOUS_XFER)) {
 			struct ata_ering_entry *ent;
 
@@ -2811,6 +2817,9 @@ int ata_set_mode(struct ata_link *link,
 		u8 saved_xfer_mode = ehc->saved_xfer_mode[dev->devno];
 		u8 saved_ncq = !!(ehc->saved_ncq_enabled & (1 << dev->devno));
 
+		if (!ata_dev_enabled(dev))
+			continue;
+
 		if (dev->xfer_mode != saved_xfer_mode ||
 		    ata_ncq_enabled(dev) != saved_ncq)
 			dev->flags |= ATA_DFLAG_DUBIOUS_XFER;

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Sergei Shtylyov]

Hello.

Tejun Heo wrote:

> There were several places where only enabled devices should be
> iterated over but device enabledness wasn't checked.
>
> * IDENTIFY data 40 wire check in cable_is_40wire()
> * xfer_mode/ncq_enabled saving in ata_scsi_error()
> * DUBIOUS_XFER handling in ata_set_mode()
>
> While at it, reformat comment in cable_is_40wire().
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
>   

   More nitpicking... :-)

> Index: work/drivers/ata/libata-core.c
> ===================================================================
> --- work.orig/drivers/ata/libata-core.c
> +++ work/drivers/ata/libata-core.c
> @@ -4169,18 +4169,16 @@ static int cable_is_40wire(struct ata_po
>  	if (ap->cbl == ATA_CBL_PATA40_SHORT)
>  		return 0;
>  	/* If the controller doesn't know we scan
> -
> -	   - Note: We look for all 40 wire detects at this point.
> -	     Any 80 wire detect is taken to be 80 wire cable
> -	     because
> -	     - In many setups only the one drive (slave if present)
> -               will give a valid detect
> -             - If you have a non detect capable drive you don't
> -               want it to colour the choice
> -        */
> +	 *
> +	 * - Note: We look for all 40 wire detects at this point.  Any
> +	 *   80 wire detect is taken to be 80 wire cable because - In
> +	 *   many setups only the one drive (slave if present) will
> +	 *   give a valid detect - If you have a non detect capable
> +	 *   drive you don't want it to colour the choice
> +	 */
>   

   The comment formatting  now is worse than it was... I'd drop hyphen 
before "Note:" and align the bullets under this note (also doing s/If/if/).

MBR, Sergei

Re: [PATCH #upstream-fixes 1/4] libata: fix device iteration bugs

[Tejun Heo]

Sergei Shtylyov wrote:
> Hello.
> 
> Tejun Heo wrote:
> 
>> There were several places where only enabled devices should be
>> iterated over but device enabledness wasn't checked.
>>
>> * IDENTIFY data 40 wire check in cable_is_40wire()
>> * xfer_mode/ncq_enabled saving in ata_scsi_error()
>> * DUBIOUS_XFER handling in ata_set_mode()
>>
>> While at it, reformat comment in cable_is_40wire().
>>
>> Signed-off-by: Tejun Heo <tj@kernel.org>
>>   
> 
>   More nitpicking... :-)
> 
>> Index: work/drivers/ata/libata-core.c
>> ===================================================================
>> --- work.orig/drivers/ata/libata-core.c
>> +++ work/drivers/ata/libata-core.c
>> @@ -4169,18 +4169,16 @@ static int cable_is_40wire(struct ata_po
>>      if (ap->cbl == ATA_CBL_PATA40_SHORT)
>>          return 0;
>>      /* If the controller doesn't know we scan
>> -
>> -       - Note: We look for all 40 wire detects at this point.
>> -         Any 80 wire detect is taken to be 80 wire cable
>> -         because
>> -         - In many setups only the one drive (slave if present)
>> -               will give a valid detect
>> -             - If you have a non detect capable drive you don't
>> -               want it to colour the choice
>> -        */
>> +     *
>> +     * - Note: We look for all 40 wire detects at this point.  Any
>> +     *   80 wire detect is taken to be 80 wire cable because - In
>> +     *   many setups only the one drive (slave if present) will
>> +     *   give a valid detect - If you have a non detect capable
>> +     *   drive you don't want it to colour the choice
>> +     */
>>   
> 
>   The comment formatting  now is worse than it was... I'd drop hyphen
> before "Note:" and align the bullets under this note (also doing s/If/if/).

Yeah, somehow I missed those bullets altogether.  Will regenerate the
patch.  Thanks.

-- 
tejun