On Mon, Jan 13, 2014 at 12:37:01PM -0800, Dan Williams wrote:
> On Mon, Jan 13, 2014 at 12:06 PM, Todd E Brandt
> <todd.e.brandt@xxxxxxxxxxxxxxx> wrote:
> > On Fri, Jan 10, 2014 at 07:13:11PM -0800, Dan Williams wrote:
> >> On Fri, Jan 10, 2014 at 6:13 PM, Phillip Susi <psusi@xxxxxxxxxx> wrote:
> >> > -----BEGIN PGP SIGNED MESSAGE-----
> >> > Hash: SHA512
> >> >
> >> > On 01/10/2014 06:11 PM, Brandt, Todd E wrote:
> >> >> Yes yours is simpler, but it also opens a potential memory issue
> >> >> by passing a static int as the return location for the error value.
> >> >> I think it's just safer to tell the callback to attempt no return
> >> >> value at all, and for that you need to expand it into two
> >> >> arguments, one for selection, the other for the output address.
> >> >
> >> > What sort of memory issue? Also isn't there a system NULL page
> >> > somewhere that could be used?
> >> >
> >>
> >> I think the static variable is ok. We can be sure that all eh threads
> >> are torn down before libata.ko is unloaded.
> >
> > Actually there's one other reason. In the ata_port_request_pm function it
> > checks to see if there's a previous resume operation pending, and if so
> > it calls ata_port_wait_eh in order to wait for it to complete before
> > issuing the new suspend. If you just use the (int*)async parameter it
> > will return immediately and defer to the caller to try again, like is does
> > with SAS. But in our case we *don't* try again, so it would result in the
> > resume being skipped. There needs to be a new case where the caller wants
> > the call to be asynchronous, and it wants ata_port_request_pm to do its
> > own waiting, but doesn't care about the return value. Thus the additional
> > parameter.
>
> I think that is specifically for the libata case of a suspend request
> arriving while an async resume is still in flight. Given libata
Accoring to the comments it's for a previous resume, not a previous suspend.
/* Previous resume operation might still be in
* progress. Wait for PM_PENDING to clear.
*/
if (ap->pflags & ATA_PFLAG_PM_PENDING) {
if (async) {
*async = -EAGAIN;
return 0;
}
ata_port_wait_eh(ap);
WARN_ON(ap->pflags & ATA_PFLAG_PM_PENDING);
}
I'm going to assume that it was added for a reason, so I've structured my
patch in such a way that it doesn't alter the existing logic. Removing that
particular check would be a completely different discussion and is out of
the scope of this patch.
> suspends are synchronous I do not think we have the reverse problem of
> resume requests arriving while a suspend is in flight. However, it
> might be worth a WARN_ON_ONCE() to document that assumption.
>
> In the libsas case suspends are asynchronous, but they are flushed by
> libsas before any resumes are processed, so there should not be
> conflicts.
--
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
