| Message ID | 1357809870-18816-8-git-send-email-aaron.lu@intel.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
On Thu, Jan 10, 2013 at 05:24:28PM +0800, Aaron Lu wrote: > @@ -182,6 +182,13 @@ void zpodd_enable_run_wake(struct ata_device *dev) > { > struct zpodd *zpodd = dev->zpodd; > > + /* > + * Silence the media change poll, as we will be notified when > + * user wants to use the ODD so there is no meaning to poll > + * it when it is powered off > + */ > + dev->sdev->disable_disk_events = true; What's the synchronization rule for this field? Thanks.
On Thu, Jan 10, 2013 at 11:56:10AM -0800, Tejun Heo wrote: > On Thu, Jan 10, 2013 at 05:24:28PM +0800, Aaron Lu wrote: > > @@ -182,6 +182,13 @@ void zpodd_enable_run_wake(struct ata_device *dev) > > { > > struct zpodd *zpodd = dev->zpodd; > > > > + /* > > + * Silence the media change poll, as we will be notified when > > + * user wants to use the ODD so there is no meaning to poll > > + * it when it is powered off > > + */ > > + dev->sdev->disable_disk_events = true; > > What's the synchronization rule for this field? I documented the rule in include/scsi/scsi_device.h. This field is modified in the ata port's runtime suspend and resume callback, and is read accessed in the check_events callback of the sr block driver. The runtime PM callback is synchronized by PM core, in that the two callbacks will never run concurrently. So I guess saying synchronized by PM core is enough for this field? This is what I've added in v12 for scsi_device structure: + bool disable_disk_events; /* disable poll for disk events, used in + * ATA layer, sychronized by PM core */ + Or do you mean I should add a comment explaining the sync rule when it is modifed, like in the above code? Thanks, Aaron > > Thanks. > > -- > tejun -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, Aaron. On Fri, Jan 11, 2013 at 10:11:10AM +0800, Aaron Lu wrote: > > What's the synchronization rule for this field? > > I documented the rule in include/scsi/scsi_device.h. > > This field is modified in the ata port's runtime suspend and resume > callback, and is read accessed in the check_events callback of the sr > block driver. The runtime PM callback is synchronized by PM core, in > that the two callbacks will never run concurrently. So I guess saying > synchronized by PM core is enough for this field? > > This is what I've added in v12 for scsi_device structure: > > + bool disable_disk_events; /* disable poll for disk events, used in > + * ATA layer, sychronized by PM core */ > + > > Or do you mean I should add a comment explaining the sync rule when it > is modifed, like in the above code? The thing is that disabling disk events doesn't necessarily have anything to do with PM, so tying synchronization to PM subsystem is a bit unexpected. How about making it an atomic_t? That way, disabling can stack and synchronization dependency to PM is removed. Thanks.
Hi Tejun, On Thu, Jan 10, 2013 at 06:17:01PM -0800, Tejun Heo wrote: > Hello, Aaron. > > On Fri, Jan 11, 2013 at 10:11:10AM +0800, Aaron Lu wrote: > > > What's the synchronization rule for this field? > > > > I documented the rule in include/scsi/scsi_device.h. > > > > This field is modified in the ata port's runtime suspend and resume > > callback, and is read accessed in the check_events callback of the sr > > block driver. The runtime PM callback is synchronized by PM core, in > > that the two callbacks will never run concurrently. So I guess saying > > synchronized by PM core is enough for this field? > > > > This is what I've added in v12 for scsi_device structure: > > > > + bool disable_disk_events; /* disable poll for disk events, used in > > + * ATA layer, sychronized by PM core */ > > + > > > > Or do you mean I should add a comment explaining the sync rule when it > > is modifed, like in the above code? > > The thing is that disabling disk events doesn't necessarily have > anything to do with PM, so tying synchronization to PM subsystem is a > bit unexpected. How about making it an atomic_t? That way, disabling > can stack and synchronization dependency to PM is removed. OK, will make it atomic in next version, thanks for the advice. Perhaps I can add two scsi helper functions in scsi_lib.c like: void sdev_disable_disk_events(struct scsi_device *sdev) { atomic_inc(&sdev->disk_events_disable_depth); } void sdev_enable_disk_events(struct scsi_device *sdev) { if (WARN_ON_ONCE(atomic_read(&sdev->disk_events_disable_depth) <= 0)) return; atomic_dec(&sdev->disk_events_disable_depth); } And call them in ATA layer. Do you like this? Thanks, Aaron -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Fri, Jan 11, 2013 at 11:16:26AM +0800, Aaron Lu wrote: > OK, will make it atomic in next version, thanks for the advice. > > Perhaps I can add two scsi helper functions in scsi_lib.c like: > > void sdev_disable_disk_events(struct scsi_device *sdev) > { > atomic_inc(&sdev->disk_events_disable_depth); > } > > void sdev_enable_disk_events(struct scsi_device *sdev) > { > if (WARN_ON_ONCE(atomic_read(&sdev->disk_events_disable_depth) <= 0)) > return; > atomic_dec(&sdev->disk_events_disable_depth); > } > > And call them in ATA layer. Do you like this? Sounds good to me. James, how does the series look to you? Thanks!
On 01/11/2013 01:44 PM, Tejun Heo wrote: > Hello, > > On Fri, Jan 11, 2013 at 11:16:26AM +0800, Aaron Lu wrote: >> OK, will make it atomic in next version, thanks for the advice. >> >> Perhaps I can add two scsi helper functions in scsi_lib.c like: >> >> void sdev_disable_disk_events(struct scsi_device *sdev) >> { >> atomic_inc(&sdev->disk_events_disable_depth); >> } >> >> void sdev_enable_disk_events(struct scsi_device *sdev) >> { >> if (WARN_ON_ONCE(atomic_read(&sdev->disk_events_disable_depth) <= 0)) >> return; >> atomic_dec(&sdev->disk_events_disable_depth); >> } >> >> And call them in ATA layer. Do you like this? > > Sounds good to me. James, how does the series look to you? Indeed. Want James' Acked-by for patch #1. I think it's ready. It can go into libata-dev.git #upstream, and be reverted prior to Linus push if James NAKs. Jeff -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 14, 2013 at 01:01:47PM -0500, Jeff Garzik wrote: > On 01/11/2013 01:44 PM, Tejun Heo wrote: > >Hello, > > > >On Fri, Jan 11, 2013 at 11:16:26AM +0800, Aaron Lu wrote: > >>OK, will make it atomic in next version, thanks for the advice. > >> > >>Perhaps I can add two scsi helper functions in scsi_lib.c like: > >> > >>void sdev_disable_disk_events(struct scsi_device *sdev) > >>{ > >> atomic_inc(&sdev->disk_events_disable_depth); > >>} > >> > >>void sdev_enable_disk_events(struct scsi_device *sdev) > >>{ > >> if (WARN_ON_ONCE(atomic_read(&sdev->disk_events_disable_depth) <= 0)) > >> return; > >> atomic_dec(&sdev->disk_events_disable_depth); > >>} > >> > >>And call them in ATA layer. Do you like this? > > > >Sounds good to me. James, how does the series look to you? > > Indeed. Want James' Acked-by for patch #1. > > I think it's ready. It can go into libata-dev.git #upstream, and be > reverted prior to Linus push if James NAKs. Great! Thanks. I'll incorporate Tejun's suggestions into v13 and send them out soon. -Aaron -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/ata/libata-zpodd.c b/drivers/ata/libata-zpodd.c index 1f5d52a..11fcd58 100644 --- a/drivers/ata/libata-zpodd.c +++ b/drivers/ata/libata-zpodd.c @@ -182,6 +182,13 @@ void zpodd_enable_run_wake(struct ata_device *dev) { struct zpodd *zpodd = dev->zpodd; + /* + * Silence the media change poll, as we will be notified when + * user wants to use the ODD so there is no meaning to poll + * it when it is powered off + */ + dev->sdev->disable_disk_events = true; + zpodd->powered_off = true; device_set_run_wake(&dev->sdev->sdev_gendev, true); acpi_pm_device_run_wake(&dev->sdev->sdev_gendev, true); @@ -230,6 +237,7 @@ void zpodd_post_poweron(struct ata_device *dev) zpodd->zp_sampled = false; zpodd->zp_ready = false; + dev->sdev->disable_disk_events = false; } static void zpodd_wake_dev(acpi_handle handle, u32 event, void *context) diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c index 4d1a610..c9c7fbb 100644 --- a/drivers/scsi/sr.c +++ b/drivers/scsi/sr.c @@ -594,7 +594,11 @@ 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->disable_disk_events) + 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..6e9781f 100644 --- a/include/scsi/scsi_device.h +++ b/include/scsi/scsi_device.h @@ -161,6 +161,9 @@ struct scsi_device { unsigned wce_default_on:1; /* Cache is ON by default */ unsigned no_dif:1; /* T10 PI (DIF) should be disabled */ + bool disable_disk_events; /* disable poll for disk events, used in + * ATA layer, sychronized by PM core */ + DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */ struct list_head event_list; /* asserted events */ struct work_struct event_work;
When the ODD is powered off, any action the user did to the ODD that would generate a media event will trigger an ACPI interrupt, so the poll for media event is no longer necessary. And the poll will also cause a runtime status change, which will stop the ODD from staying in powered off state, so the poll should better be stopped. But since we don't have access to the gendisk structure in LLDs, here comes the disable_disk_events flag for scsi device. This flag serves as a capability of the device, conveyed by the LLDs to upper layer. It is set when LLDs know that this device will no longer generate any media related events, so that the check_events can simply return 0 without bothering the device, effectively silence the poll. Signed-off-by: Aaron Lu <aaron.lu@intel.com> --- drivers/ata/libata-zpodd.c | 8 ++++++++ drivers/scsi/sr.c | 6 +++++- include/scsi/scsi_device.h | 3 +++ 3 files changed, 16 insertions(+), 1 deletion(-)