Re: [PATCH] ahci.c: fix ati sb600 sata IRQ_TF_ERR

Conke Hu
2007-03-15T12:00:52Z

On 3/14/07, Tejun Heo <htejun@gmail.com> wrote:
> Hello,
>
> Conke Hu wrote:
> > When there is no media in SATA CD/DVD drive or media is not ready,
> > AHCI controller fails to execute the ATAPI commands TEST_UNIT_READY,
> > READ_CAPACITY or READ_TOC and reports PORT_IRQ_TF_ERR. But ATI SB600
> > SATA controller sets SERR_INTERNAL bit in the error register at the
> > same time, which is not necessary. This patch is just to ignore the
> > INTERNAL ERROR in such case. Without this patch, ahci error handler
> > will report many errors as below:
>
> Whoa, SERR_INTERNAL on ATAPI check condition? Just for fun, here's what
> the spec says about SERR_INTERNAL.
>

When media is not ready, command TEST_UNIT_READY fails with ahci irq
status == 0x40000001(IRQ_TF_ERROR) and serror == SERR_INTERNEL, then
ahci error handler calls atapi_eh_request_sense() and sets
ATA_QCFLAG_SENSE_VALID. Command REQUEST_SENSE executes successfully
and atapi_qc_complete() sets result = SAM_STAT_CHECK_CONDITION, and
now the whole TEST_UNIT_READY request is done and returns .

> E Internal error: The host bus adapter experienced an internal error
> that caused the operation to fail and may have put the host bus adapter
> into an error state. Host software should reset the interface before
> re-trying the operation. If the condition persists, the host bus adapter
> may suffer from a design issue rendering it incompatible with the
> attached device.
>

Yes, I saw this too :) and I am contacting the hardware engineers to
check if there is any hardware bug.
But, even though this were a hardware bug and could be fixed, we would
still need this patch since many SB600 boards have already come into
the market and those ASICs can never be fixed :(
So, if no errors in this patch, could Jeff please apply it ASAP?

> Anyways thanks for fixing this. Just a few comments.
>
> > --- linux-2.6.21-rc3-git8/drivers/ata/ahci.c.orig 2007-03-25
> > 20:57:31.000000000 +0800
> > +++ linux-2.6.21-rc3-git8/drivers/ata/ahci.c 2007-03-25
> > 21:03:54.000000000 +0800
> > @@ -80,6 +80,7 @@ enum {
> > board_ahci_pi = 1,
> > board_ahci_vt8251 = 2,
> > board_ahci_ign_iferr = 3,
> > + board_ahci_ati = 4,
> >
> > /* global controller registers */
> > HOST_CAP = 0x00, /* host capabilities */
> > @@ -168,6 +169,7 @@ enum {
> > AHCI_FLAG_NO_NCQ = (1 << 24),
> > AHCI_FLAG_IGN_IRQ_IF_ERR = (1 << 25), /* ignore IRQ_IF_ERR */
> > AHCI_FLAG_HONOR_PI = (1 << 26), /* honor PORTS_IMPL */
> > + AHCI_FLAG_TF_ERR_FIX = (1 << 27), /* ignore INTERNAL ERROR on
> > IRQ_TF_ERROR */
>
> Can we use board_ahci_ign_interr and AHCI_FLAG_IGN_SERR_INTERNAL to keep
> it more consistent with the other IGN flag?
>
> > - { PCI_VDEVICE(ATI, 0x4380), board_ahci }, /* ATI SB600 non-raid */
> > + { PCI_VDEVICE(ATI, 0x4380), board_ahci_ati }, /* ATI SB600 non-raid */
> > { PCI_VDEVICE(ATI, 0x4381), board_ahci }, /* ATI SB600 raid */
>
> 4381 isn't affected while 4380 is?

I never see such an ID, and plan to remove 0x4381.
The patch which added the PCI IDs was not sent out by myself. I
checked all SB600 boards, and not found any 0x4381 controller, only
0x4380 instead. In fact, SB600 RAID and Non-RAID share the same PCI
device ID, only with class code different.

>
> Hmmm... Okay, this is weird. I'm feeling very strong deja vu.
>
> Well, I must be getting alzheimer. I did almost the same patch a month
> ago and was waiting for verification to properly submit the patch.
>
> http://thread.gmane.org/gmane.linux.ide/16049/focus=16437
>
> Anyways, Conke Hu, can you please take a look at my patch from a month
> ago? It's almost identical but SERR_INTERNAL is always ignored on both
> SB600 PCI IDs, which I think is safer. Does this fix what you're seeing?
>

I just read your patch. Another difference is that my patch ignores
SERR_INTERNAL only when the command is ATAPI and IRQ_TF_ERR occurs. In
other cases, I think, we'd better not ignore the SERR_INTERNEL. Right?
The following is some detail:
// your patch:
+ if (ap->flags & AHCI_FLAG_IGN_SERR_INTERNAL)
+ serr &= ~SERR_INTERNAL;

// mine:
- if (irq_stat & PORT_IRQ_TF_ERR)
+ if (irq_stat & PORT_IRQ_TF_ERR) {
err_mask |= AC_ERR_DEV;
+
+ /* some controllers set INTERNAL ERROR on ATAPI
IRQ_TF_ERROR, ignore it */
+ if ((serror & SERR_INTERNAL) &&
+ (ap->flags & AHCI_FLAG_TF_ERR_FIX) &&
+ qc && qc->dev->class == ATA_DEV_ATAPI) {
+ serror &= ~SERR_INTERNAL;
+ }
+ }

Tejun, you do me a great favor, thank you so much! for your previous
help, too :)

Conke