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:

quoted

Hey, Rafael.

On Wed, Nov 28, 2012 at 01:51:00AM +0100, Rafael J. Wysocki wrote:

quoted

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

quoted

* 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.