Re: [PATCH v7 0/6] ZPODD patches

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Monday, September 24, 2012, Aaron Lu wrote:
> On Fri, Sep 21, 2012 at 11:18:27PM +0200, Rafael J. Wysocki wrote:
> > On Friday, September 21, 2012, Aaron Lu wrote:
> > > On Thu, Sep 20, 2012 at 10:00:51PM +0200, Rafael J. Wysocki wrote:
> > > > On Wednesday, September 19, 2012, Aaron Lu wrote:
> > > > > Thanks Rafael, and if there is any question/problem,
> > > > > please kindly let me know.
> > > > 
> > > > Well, unfortunately my initial review indicates that the patchset is not
> > > > quite ready to go upstream yet.
> > > > 
> > > > I'll send comments in replies to the individual patches, but overall I can
> > > > say that at this stage of development, when I look at the patches, it should
> > > > be clear to me not only what is being changed, but _why_ it is being changed
> > > > in the first place and, secondly, why it is being changed in this particular
> > > > way.  It's far from that, though.
> > > 
> > > I'm adding zero power support for optical disk drive(ZPODD), which is
> > > made possible with the newly defined device attention(DA) pin introduced
> > > in SATA 3.1 spec.
> > > 
> > > The idea here is to use runtime pm to achieve this, so I basically did 2
> > > things:
> > > 1 Add runtime pm support for ODD;
> > > 2 Add power off support for ODD after it is runtime suspended.
> > > 
> > > Patch 2 is runtime pm support for ODD, the reason it is done this way is
> > > discussed here:
> > > http://www.spinics.net/lists/linux-scsi/msg61551.html
> > 
> > Why isn't it explained in the patch changelog, then?  People should be able
> > to learn why things are done the way they are done from git logs.
> > 
> > > The basic idea is, the ODD will be runtime suspended as long as there is
> > > nobody using it, that is, no programs opening the block device.
> > > 
> > > The ODD will be polled periodically, so it will be runtime resumed
> > > before checking if there is any events pending and suspended when done.
> > 
> > OK.  So what happens if we power off the drive via runtime PM.  Does it
> > it really make sense to resumie it through polling in that case?
> 
> No, this is the reason I introduced the powered_off flag. If set, the
> poll will simply return without touching the device.
> 
> I've tried to do a disk_block_events call on its suspend callback when
> it is ready to be powered off, but there is a race that I don't know how
> to solve:
> pm_runtime_suspend			disk_events_workfn		
>   scsi_dev_type_suspend			  sr_block_check_events
>     sr_suspend				    cdrom_check_events
>       disk_block_events		      	      cdrom_update_events
>       (this call waits for all		        sr_check_events
>       running events_checking function	          scsi_autopm_get_device
>       to return)
> 
> Suppose sr_suspend runs first, and then sr_check_events comes in.
> sr_suspend calls disk_block_events, which waits for sr_check_events,
> while scsi_autopm_get_device wait for suspend callback to finish,
> deadlock.

I need some more time to think about this, stay tuned.

> > > The only exception is, if we found a disc is just inserted, we will not
> > > idle it immediately at the end of the poll, reason explained in another
> > > mail.
> > > 
> > > This is the rational I wrote patch 2, and patch 1 is used by patch 2.
> > > 
> > > Patch 3 is adding power off support for ODD after it is runtime
> > > suspended, the condition is specified in section 15:
> > > ftp://ftp.seagate.com/sff/INF-8090.PDF
> > > 
> > > That is, for tray type ODD: no media inside and door closed; for slot
> > > type ODD: no media inside.
> > > 
> > > The is the reason sr_suspend is written, for non-ZPODD capable devices,
> > > it does nothing; for ZPODD devices, it will check the above condition to
> > > see if it is ready to be powered off. The ready_to_power_off flag will be
> > > used by ATA layer to decide if power can be removed.
> > 
> > Now, James says he doesn't like the way ready_to_power_off is used.  Sure
> > enough, it is totally irrelevant to the majority of SCSI devices.  It actually
> > is totally irrelevant to everything in the SCSI subsystem except for the sr
> > driver and libata.  So I wonder if you have considered any alternative
> > way to address the use case at hand?
> > 
> > > When in powered off state, if user presses the eject button or insert a
> > > disc, an ACPI event will be generated and our acpi wake handler will
> > > pm_runtime_resume the ODD. And if it is a tray type ODD, its tray should
> > > be ejected(need_eject flag) after powered on. This is patch 3.
> > 
> > That sounds reasonable enough, but the role of the powered_off and
> > need_eject flags could be explained a bit better.  In particular, it would
> 
> powered_off: set when the device is powered off, clear otherwise.

That's pretty clear, but I think this flag should be called no_polling
or something like this, because that's what it means to the SCSI layer.

> need_eject:
> First consider how the device will be runtime resumed:
> 1 Some program opens the block device;
> 2 Events checking poll when it's not powered off yet;
> 3 User presses the eject button or inserts a disc into the slot when the
>   device is in powered off state.
> And the need_eject flag is for case 3, when the device is in powered off
> state and user presses the eject button, it will be powered on(through
> acpi wake notification function) and runtime resumed. In its runtime
> resume callback, its tray needs to be ejected since user just presses
> the eject button. The whole process of ZPODD is opaque to the user,
> he/she doesn't know the ODD lost power so the ODD has to behave exactly
> like it doesn't lose power.

Do you think it can be useful for other types of devices, not necessarily
handled through ACPI?

> Hi Oliver,
> This flag is really to say the tray needs to be ejected after runtime
> resumed, it's not that media change detected. It is possible that user
> ejects the tray without putting any disc inside and simply close the
> tray, which doesn't qualify a media change event. And if user does
> put a disc in, the sr_check_events will find that and report the media
> change event to user space. Agree?
> 
> > be nice to have explained why they have to be present in struct scsi_device,
> > because they don't seem to be particularly useful for many SCSI devices
> > that aren't CD drives (the need_eject one in particular).
> 
> With your suggestion of pm_platform_power_off_allowed, I suppose
> powered_off can be eliminated similarly with something like
> pm_platform_powered_off returning true or false(for ACPI platform,
> return true when device is in D3 cold state).

I'm currently thinking that using PM QoS may be a better approach here.

> And for the need_eject flag, I don't know if there is a better place for
> it. The acpi wake notification code resides in libata(where we need to
> record that this resume is due to user presses the eject button and the
> tray needs to be ejected after resumed), and the runtime resume callback
> resides in scsi driver(where we actually eject the tray). Ideally, this
> flag should sit in scsi_cd structure, but libata does not have access to
> it.

Yes, that's the problem that James mentioned.

Thanks,
Rafael