Hello Tejun,
Thanks for your review comments. Please find my answers below :
>> Broke the sata_fsl.c driver in 2.6.26. I know the following patch
>> fixes the issue, it clearly also adds port multipler support. I'm
not
>> sure if you are willing to take that as part of 2.6.26 or not, but
the
>> current
>> 2.6.26 driver is broken.
> Would it be possible to break the patch into two pieces? One to fix
the problem and the other to add PMP support?
Actually, the boot-time hang issue was caused due to my handling of
command completion interrupt and detecting
the queued commands being completed. I was not finding the correct
"active" link and hence causing command completion
interrupts not being ack'ed correctly and locking the machine as they
remain pending forever.
Now the fix I added works for both PMP and direct device attach cases,
and is actually part of the PMP patch because I
had changed command completion interrupt handling initially for PMP and
addition of links in the libata core,
and this initial change had introduced the bug which caused hangs in
direct device attach cases. Therefore, this fix was added
to my initial PMP patch. For the same reason, I would wish to keep this
as a single patch.
>> @@ -771,7 +803,8 @@ try_offline_again:
>> ata_port_printk(ap, KERN_WARNING,
>> "No Device OR PHYRDY change,Hstatus =
0x%x\n",
>> ioread32(hcr_base + HSTATUS));
>> - goto err;
>> + *class = ATA_DEV_NONE;
>> + goto out;
>> }
>>
>> /*
>> @@ -783,7 +816,8 @@ try_offline_again:
>>
>> if ((temp & 0xFF) != 0x18) {
>> ata_port_printk(ap, KERN_WARNING, "No Signature
Update\n");
>> - goto err;
>> + *class = ATA_DEV_NONE;
>> + goto out;
> Is setting class to ATA_DEV_NONE necessary? What happens if you drop
the above two chunks?
> Also, it looks to me that currently sata_fsl_softreset() does both
hard and soft resets. Is it possible to split it into
sata_fsl_hardreset() and softreset()?
>> /*
>> - * We should consider this as non fatal error, and TF
must
>> - * be updated as done below.
>> + * Ignore serror in case of fatal errors as we always
want
>> + * to do a soft-reset of the FSL SATA controller.
Analyzing
>> + * serror may cause libata to schedule a hard-reset
action,
>> + * and hard-reset currently does not do controller
>> + * offline/online, causing command timeouts and leads to
an
>> + * un-recoverable state, hence make libATA ignore
>> + * autopsy in case of fatal errors.
>> */
>>
>> - err_mask |= AC_ERR_DEV;
>> - }
>> + ehi->flags |= ATA_EHI_NO_AUTOPSY;
> This is really fishy. NO_AUTOPSY isn't for stuff like this and libata
EH as of the current #usptream and #upstream-fixes default to hardreset,
so it will hardreset no > matter what you do.
> What do you mean by hardreset does'nt do controller offline/online?
Is this PHY sequence in sata_fsl_softreset()? I think what should be
done is...
> * Separate out PHY diddling from sata_fsl_softreset() into
sata_fsl_hardreset().
> * If sata_fsl_hardreset() alone doesn't leave the controller in usable
state. Return -EAGAIN from it to request follow-up SRST on success.
Both of the above cases ( ATA_DEV_NONE & NO_AUTOPSY ) are basically
hacks because as you have mentioned, currently sata_fsl_softreset() does
both
PHY-reset and softreset handling. This split of sata_fsl_softreset() has
been on my list of things to do for some time, and I will work on it
next
week and send you an updated patch for review.
Thanks,
Ashish
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
