Re: [PATCH v8 3/4] ata: Add APM X-Gene SoC SATA host controller driver

[Tejun Heo <tj@kernel.org>]

Hey,

On Tue, Jan 14, 2014 at 07:57:19AM -0800, Loc Ho wrote:
> > No, they don't and the comments in your driver don't really explain
> > what's going on.  Why are we having retry loops inside hardreset
> > itself?  This can prolong recovery time significantly in corner cases.
> > Why is this necessary?
> 
> There are two retry in the hardreset. The first retry is due to the
> requirement to have slightly modified PHY setting for different speed
> of the attached disk. The default is Gen3. If it is Gen1 or Gen2 disk
> attached, HW will require slightly modified PHY setting. The second
> retry is due to the fact that link may not link up the first time due
> to HW errata. For the initial version, I will remove the second retry
> and submit an separate patch.

And please explain what's going on in more detail in the comments.

> > * If your driver needs to append or prepent something to the existing
> >   one, export the existing one and then wrap that one with the extra
> >   logic around it.  Do not copy function body unnecessarily.  Even for
> >   the port_interrupt, it looks like you can just call the flush after
> >   invoking the normal ahci_handle_port_interrupt(), no?
> 
> As mentioned, the flush requires immediately after reading the CI.
> Otherwise, there is still an chance that the command is completed and
> the OS notified the upper layer while the data is still in flight. For
> the initial version, I will remove the flush (IRQ wrapper) and submit
> separate patch.

The function is called with ap->lock held.  Nothing can happen
inbetween.

Thanks.

-- 
tejun