Re: [PATCH v7 2/6] scsi: sr: support runtime pm

[Aaron Lu <aaron.lu@intel.com>]

On Mon, Sep 24, 2012 at 11:40:18PM +0200, Rafael J. Wysocki wrote:
> On Monday, September 24, 2012, Aaron Lu wrote:
> > On Mon, Sep 24, 2012 at 02:55:31PM +0200, Rafael J. Wysocki wrote:
> > > And I'd add a comment about the next poll.
> > > 
> > > This appears somewhat racy, though, because in theory a media may be inserted
> > > while we are removing power from the device.  Isn't that a problem?
> > 
> > Yes, this is a problem.
> > To avoid this race, I think the following needs to be done:
> > - For slot type ODD, make it such that user can't insert a disc;
> > - For tray type ODD, make it such that when user presses the eject
> >   button, the tray doesn't open.
> > I'll see if this is possible, thanks for the remind.
> 
> OK

Looks like this is not the right thing to do, if I lock the door user
will be confused.

> 
> > > > The poll runs periodically, until the device is powered off(reflected by
> > > > the powered_off flag), in which case, the poll will simply return
> > > > 0 without touching this device.
> > > 
> > > Is it useful to poll the device after it has been suspended, but not powered
> > > off?
> > 
> > Yes, it is necessary to poll the device when it has been suspended with
> > power left. The reason is, we need to check if there is a media change
> > event happened, and the way to check this is to issue a
> > GET_EVENT_STATUS_NOTIFICATION comand.
> > 
> > Please note that when the drive is not powered off, it will not send us
> > a notification when its eject button is pressed.
> 
> I'm not sure about that, actually.  If it doesn't notify us, that whole thing
> is inherently racy pretty much beyond fixing, because it is always possible
> that the user will press the eject button right after we've checked the
> status last time and right before power removal.  We're going to lose that
> event, so the user will have to push the button once again in that case.

I just checked the spec again and tested, when the ODD has power, it
will also send out notifications on pressing the eject button/inserting
a disc. So we should be able to capture such a event.

> 
> > > > That's correct.
> > > > AFAIK, user space does not care how often the device is polled, it
> > > > cares only one thing: when there is a media change event, please let me
> > > > know(through uevent).
> > > 
> > > So that's why we do the polling, right?
> > 
> > Yes.
> 
> Well, that's difficult.
> 
> If the remote wakeup is signaled through a GPE, it should be possible to
> enable it before we actually put the device into D3cold.  That may allow us
> to eliminate the races.

Thanks for the suggestion, I'll update the code.

I'm thinking of enabling this GPE in sr_suspend once we decided that it
is ready to be powered off, so the time frame between sr_suspend and
when the power is actually removed in libata should be taken care of by
the GPE. If GPE fires, the notification function will request a runtime
resume of the device. Does this sound OK?

Thanks,
Aaron