Re: questions regarding possible violation of AHCI spec in AHCI driver

Tejun Heo
2011-01-11T18:23:31Z

Hello,

On Tue, Jan 11, 2011 at 10:09:08AM -0800, Jian Peng wrote:
> Happy Holiday! I want to revisit this issue and hopefully get
> consensus on it asap.

Happy new year!

> Here is the sequence that will cause problem if BSY|DRQ and SSTS.DET
> was not checked in start_engine:
>
> 1. SUD bit was set in ahci_power_up() to start communication between
> host and device
> 2. START bit was set in ahci_start_engine() to prepare for data
> transfer (should check condition and do not set START bit here
> per spec)
> By the time, host did not receive first FIS so BSY|DRQ was not
> cleared
> 3. inside ahci_hardreset(), call ahci_stop_engine() first, host
> controller will take time to clean up internal pipeline and
> transit to idle state
> 4. toggle SCTL.DET to reset interface, now COMRESET was not sent
> since internal state machine stuck at #2 and #3 (per> spec),
> BSY|DRQ bit was not cleared
> 5. call ahci_start_engine() again and try to read ID from device but
> interface is busy since BSY bit was not cleared, failed here
>
> At the end of section 10.1 of AHCI spec (rev 1.3), it states
>
> Software shall not set PxCMD.ST to '1' until it is determined that a
> functional device is present on the port as determined by
> PxTFD.STS.BSY = '0', PxTFD.STS.DRQ = '0', and PxSSTS.DET = 3h.
>
> It is likely used to prevent host controller from jumping into wrong
> state before first FIS was received.

I still have to say it's pretty silly of the controller to stall
inrecoverably.

> Please review this issue, and let me know how to resolve it by
> either adopting my previous patch, or creating a new patch.

You haven't really brought up any new issue so my comment stays the
same.

>> I'm not objecting to the change but you guys probably want to fix
>> the controller in following revisions. If we're gonna make the
>> change, I'd like to go with the previous version without the vendor
>> check. What is the timeframe for the controller release? Would it
>> be enough to merge the change during 2.6.38-rc1? After baking it
>> for some time in 2.6.38, we can propagate the change back through
>> -stable.

So, yeah, let's proceed with the patch. Please make the following
changes and resubmit.

* Please don't do readl() on the same line as variable declaration and
drop the unnecessary & 0xff.

* Hmmm... please consider adding a KERN_DEBUG message when
ahci_start_port() exits without starting the port. Just in case it
causes problems on other controllers.

Thank you.

--
tejun