Re: [PATCH v9 06/10] ata: zpodd: check zero power ready status

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

On Wed, Nov 28, 2012 at 08:56:09AM +0000, James Bottomley wrote:
> On Tue, 2012-11-27 at 17:39 -0800, Tejun Heo wrote:
> > Hey, Rafael.
> > 
> > On Wed, Nov 28, 2012 at 01:51:00AM +0100, Rafael J. Wysocki wrote:
> > > Having considered that a bit more I'm now thinking that in fact the power state
> > > the device is in at the moment doesn't really matter, so the polling code need
> > > not really know what PM is doing.  What it needs to know is that the device
> > > will generate a hardware event when something interesting happens, so it is not
> > > necessary to poll it.
> > > 
> > > In this particular case it is related to power management (apparently, hardware
> > > events will only be generated after the device has been put into ACPI D3cold,
> > > or so Aaron seems to be claiming), but it need not be in general, at least in
> > > principle.
> > > 
> > > It looks like we need an "event driven" flag that the can be set in the lower
> > > layers and read by the upper layers.  I suppose this means it would need to be
> > > in struct device, but not necessarily in the PM-specific part of it.
> > 
> > We already have that.  That's what gendisk->async_events is for (as
> > opposed to gendisk->events).  If all events are async_events, block
> > won't poll for events, but I'm not sure that's the golden bullet.
> > 
> > * None implements async_events yet and an API is missing -
> >   disk_check_events() - which is trivial to add, but it's the same
> >   story.  We'll need a mechanism to shoot up notification from libata
> >   to block layer.  It's admittedly easier to justify routing through
> >   SCSI tho.  So, we're mostly shifting the problem.  Given that async
> >   events is nice to have, so it isn't a bad idea.
> 
> Could we drive it in the polling code?  As in, if we set a flag to say
> we're event driven and please don't bother us, we could just respond to
> the poll with the last known state (this would probably have to be in
> SCSI somewhere since most polls are Test Unit Readys).  That way ZPODD
> sets this flag when the device powers down and unsets it when it powers
> up.

Does it mean I can do something like this:

diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 5fc97d2..219820c 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -578,7 +578,10 @@ static unsigned int sr_block_check_events(struct gendisk *disk,
 					  unsigned int clearing)
 {
 	struct scsi_cd *cd = scsi_cd(disk);
-	return cdrom_check_events(&cd->cdi, clearing);
+	if (!cd->device->event_driven)
+		return cdrom_check_events(&cd->cdi, clearing);
+	else
+		return 0;
 }
 
 static int sr_block_revalidate_disk(struct gendisk *disk)
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index e65c62e..1756151 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -160,6 +160,7 @@ struct scsi_device {
 	unsigned can_power_off:1; /* Device supports runtime power off */
 	unsigned wce_default_on:1;	/* Cache is ON by default */
 	unsigned no_dif:1;	/* T10 PI (DIF) should be disabled */
+	unsigned event_driven:1; /* No need to poll the device */
 
 	DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
 	struct list_head event_list;	/* asserted events */


Then when ZPODD is powered off, it will set this flag; and unset it when
it is powered up.

Thanks,
Aaron

> 
> James
> 
> > * Still dunno much about zpodd but IIUC the notification from
> >   zero-power is via ACPI.  To advertise that the device doesn't need
> >   polling, it should also be able to do async notification while
> >   powered up, which isn't covered by zpodd but ATA async notification.
> >   So, ummm... that's another obstacle.  If zpodd requires the device
> >   to support ATA async notification, it might not be too bad tho.
> 
> 
>