[PATCH 0/5] Fix for ZPODD

[PATCH 0/5] Fix for ZPODD

[Aaron Lu]

Here are some patches to make ZPODD easier to use for end users and
a fix for using ZPODD with system suspend.

Aaron Lu (5):
  scsi: sr: fix for sr suspend and resume
  scsi: sr: runtime pm when ODD is open/closed
  scsi: sr: block events when runtime suspended
  scsi: pm: use runtime resume callback if available
  block: genhd: add an interface to set disk's poll interval

 block/genhd.c          | 25 ++++++++++++++++++------
 drivers/scsi/scsi_pm.c | 14 ++++++++-----
 drivers/scsi/sr.c      | 53 ++++++++++++++++++++++++++++++++++++--------------
 include/linux/genhd.h  |  1 +
 4 files changed, 67 insertions(+), 26 deletions(-)

-- 
1.7.11.3

[PATCH 1/5] scsi: sr: fix for sr suspend and resume

[Aaron Lu]

In sr_suspend, we do not need to do anything if it is not a runtime pm
request, so just return by checking the PM_EVENT_AUTO flag.
And in sr_resume, only reset the suspend_count back to 1 if the ODD is
waken up by the user, or the usage count of the scsi device will not
balance.

Signed-off-by: Aaron Lu <aaron.lu@amd.com>
---
 drivers/scsi/sr.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 3da0879..5f4d19a 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -178,8 +178,8 @@ static int sr_suspend(struct device *dev, pm_message_t msg)
 	struct scsi_sense_hdr sshdr;
 	struct scsi_cd *cd = dev_get_drvdata(dev);
 
-	/* no action for system suspend */
-	if (msg.event == PM_EVENT_SUSPEND)
+	/* no action for system pm operations */
+	if (!(msg.event & PM_EVENT_AUTO))
 		return 0;
 
 	/* do another TUR to see if the ODD is still ready to be powered off */
@@ -217,9 +217,9 @@ static int sr_resume(struct device *dev)
 		cd->device->wakeup_by_user = 0;
 		if (!(cd->cdi.mask & CDC_CLOSE_TRAY))
 			sr_tray_move(&cd->cdi, 1);
-	}
 
-	atomic_set(&cd->suspend_count, 1);
+		atomic_set(&cd->suspend_count, 1);
+	}
 
 	return 0;
 }
-- 
1.7.11.3

Re: [PATCH 1/5] scsi: sr: fix for sr suspend and resume

[Alan Stern]

On Mon, 23 Jul 2012, Aaron Lu wrote:

> In sr_suspend, we do not need to do anything if it is not a runtime pm
> request, so just return by checking the PM_EVENT_AUTO flag.
> And in sr_resume, only reset the suspend_count back to 1 if the ODD is
> waken up by the user, or the usage count of the scsi device will not
> balance.
> 
> Signed-off-by: Aaron Lu <aaron.lu@amd.com>
> ---
>  drivers/scsi/sr.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> index 3da0879..5f4d19a 100644
> --- a/drivers/scsi/sr.c
> +++ b/drivers/scsi/sr.c
> @@ -178,8 +178,8 @@ static int sr_suspend(struct device *dev, pm_message_t msg)
>  	struct scsi_sense_hdr sshdr;
>  	struct scsi_cd *cd = dev_get_drvdata(dev);
>  
> -	/* no action for system suspend */
> -	if (msg.event == PM_EVENT_SUSPEND)
> +	/* no action for system pm operations */
> +	if (!(msg.event & PM_EVENT_AUTO))

Please use the PMSG_IS_AUTO macro.

Alan Stern

[PATCH 2/5] scsi: sr: runtime pm when ODD is open/closed

[Aaron Lu]

The ODD can either be runtime resumed by the user or by a software
request. And for the latter part, we only support runtime resume the ODD
when the eject request is received. We did this in sr's block ioctl
function, this looks ugly.

Change this by runtime resuming the ODD in its open function and runtime
suspending it in its release function.

The downside of this approach is that in old distros with old udisk
daemon, the ODD will be polled by udisk daemon so open/close will
constantly be called, which will cause the ODD frequently resume from
suspend state, breaking the effect of power saving. User with such a
distro is advised to issue a udisk command to inhibit polling of the
disk like this:
$ udisks --inhibit-polling /dev/sr0
And since newer kernel has in kernel polling, there is no problem
regarding ODD's event report.

Signed-off-by: Aaron Lu <aaron.lu@amd.com>
---
 drivers/scsi/sr.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 5f4d19a..f7a7635 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -152,8 +152,15 @@ static inline struct scsi_cd *scsi_cd_get(struct gendisk *disk)
 	kref_get(&cd->kref);
 	if (scsi_device_get(cd->device))
 		goto out_put;
+	if (pm_runtime_get_sync(&cd->device->sdev_gendev) < 0) {
+		pm_runtime_put_noidle(&cd->device->sdev_gendev);
+		goto out_pm;
+	}
 	goto out;
 
+ out_pm:
+	scsi_device_put(cd->device);
+
  out_put:
 	kref_put(&cd->kref, sr_kref_release);
 	cd = NULL;
@@ -169,6 +176,8 @@ static void scsi_cd_put(struct scsi_cd *cd)
 	mutex_lock(&sr_ref_mutex);
 	kref_put(&cd->kref, sr_kref_release);
 	scsi_device_put(sdev);
+	pm_runtime_mark_last_busy(&cd->device->sdev_gendev);
+	pm_runtime_put_autosuspend(&cd->device->sdev_gendev);
 	mutex_unlock(&sr_ref_mutex);
 }
 
@@ -654,13 +663,6 @@ static int sr_block_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
 	void __user *argp = (void __user *)arg;
 	int ret;
 
-	/* Make sure the ODD is not suspended */
-	ret = pm_runtime_get_sync(&sdev->sdev_gendev);
-	if (ret < 0) {
-		pm_runtime_put_noidle(&sdev->sdev_gendev);
-		return -EACCES;
-	}
-
 	mutex_lock(&sr_mutex);
 
 	/*
@@ -692,8 +694,6 @@ static int sr_block_ioctl(struct block_device *bdev, fmode_t mode, unsigned cmd,
 
 out:
 	mutex_unlock(&sr_mutex);
-	pm_runtime_mark_last_busy(&cd->device->sdev_gendev);
-	pm_runtime_put_autosuspend(&cd->device->sdev_gendev);
 	return ret;
 }
 
-- 
1.7.11.3

Re: [PATCH 2/5] scsi: sr: runtime pm when ODD is open/closed

[Alan Stern]

On Mon, 23 Jul 2012, Aaron Lu wrote:

> The ODD can either be runtime resumed by the user or by a software
> request. And for the latter part, we only support runtime resume the ODD
> when the eject request is received. We did this in sr's block ioctl
> function, this looks ugly.
> 
> Change this by runtime resuming the ODD in its open function and runtime
> suspending it in its release function.
> 
> The downside of this approach is that in old distros with old udisk
> daemon, the ODD will be polled by udisk daemon so open/close will
> constantly be called, which will cause the ODD frequently resume from
> suspend state, breaking the effect of power saving. User with such a
> distro is advised to issue a udisk command to inhibit polling of the
> disk like this:
> $ udisks --inhibit-polling /dev/sr0
> And since newer kernel has in kernel polling, there is no problem
> regarding ODD's event report.
> 
> Signed-off-by: Aaron Lu <aaron.lu@amd.com>
> ---
>  drivers/scsi/sr.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> index 5f4d19a..f7a7635 100644
> --- a/drivers/scsi/sr.c
> +++ b/drivers/scsi/sr.c
> @@ -152,8 +152,15 @@ static inline struct scsi_cd *scsi_cd_get(struct gendisk *disk)
>  	kref_get(&cd->kref);
>  	if (scsi_device_get(cd->device))
>  		goto out_put;
> +	if (pm_runtime_get_sync(&cd->device->sdev_gendev) < 0) {
> +		pm_runtime_put_noidle(&cd->device->sdev_gendev);
> +		goto out_pm;
> +	}

You should use scsi_autopm_get_device instead of bypassing the SCSI 
layer.  Similarly for the _put call.

I know the existing calls do this already.  They shouldn't.

Alan Stern

[PATCH 3/5] scsi: sr: block events when runtime suspended

[Aaron Lu]

When the ODD is runtime suspended, there is no need to poll it for
events, so block events poll for it and unblock when resumed.

Signed-off-by: Aaron Lu <aaron.lu@amd.com>
---
 block/genhd.c     | 2 ++
 drivers/scsi/sr.c | 7 ++++---
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index 9cf5583..bdb3682 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -1458,6 +1458,7 @@ void disk_block_events(struct gendisk *disk)
 
 	mutex_unlock(&ev->block_mutex);
 }
+EXPORT_SYMBOL(disk_block_events);
 
 static void __disk_unblock_events(struct gendisk *disk, bool check_now)
 {
@@ -1502,6 +1503,7 @@ void disk_unblock_events(struct gendisk *disk)
 	if (disk->ev)
 		__disk_unblock_events(disk, false);
 }
+EXPORT_SYMBOL(disk_unblock_events);
 
 /**
  * disk_flush_events - schedule immediate event checking and flushing
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index f7a7635..69c9e22 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -208,6 +208,8 @@ static int sr_suspend(struct device *dev, pm_message_t msg)
 		return -EBUSY;
 	}
 
+	disk_block_events(cd->disk);
+
 	return 0;
 }
 
@@ -230,6 +232,8 @@ static int sr_resume(struct device *dev)
 		atomic_set(&cd->suspend_count, 1);
 	}
 
+	disk_unblock_events(cd->disk);
+
 	return 0;
 }
 
@@ -318,9 +322,6 @@ static unsigned int sr_check_events(struct cdrom_device_info *cdi,
 	if (CDSL_CURRENT != slot)
 		return 0;
 
-	if (pm_runtime_suspended(&cd->device->sdev_gendev))
-		return 0;
-
 	/* if the logical unit just finished loading/unloading, do a TUR */
 	if (cd->device->can_power_off && cd->dbml && sr_unit_load_done(cd)) {
 		events = 0;
-- 
1.7.11.3

[PATCH 4/5] scsi: pm: use runtime resume callback if available

[Aaron Lu]

When runtime resume a scsi device, if the device's driver has
implemented runtime resume callback, use that instead of the resume
callback.

sr driver needs this to properly do different things for system resume
and runtime resume.

Signed-off-by: Aaron Lu <aaron.lu@amd.com>
---
 drivers/scsi/scsi_pm.c | 14 +++++++++-----
 drivers/scsi/sr.c      | 21 +++++++++++++++++++++
 2 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index d4201de..19bba47 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -34,14 +34,18 @@ static int scsi_dev_type_suspend(struct device *dev, pm_message_t msg)
 	return err;
 }
 
-static int scsi_dev_type_resume(struct device *dev)
+static int scsi_dev_type_resume(struct device *dev, bool runtime)
 {
 	struct device_driver *drv;
 	int err = 0;
+	int (*resume)(struct device *);
 
 	drv = dev->driver;
-	if (drv && drv->resume)
-		err = drv->resume(dev);
+	if (runtime && drv && drv->pm && drv->pm->runtime_resume)
+		resume = drv->pm->runtime_resume;
+	else
+		resume = drv ? drv->resume : NULL;
+	err = resume(dev);
 	scsi_device_resume(to_scsi_device(dev));
 	dev_dbg(dev, "scsi resume: %d\n", err);
 	return err;
@@ -84,7 +88,7 @@ static int scsi_bus_resume_common(struct device *dev)
 		 * Resume it on behalf of child.
 		 */
 		pm_runtime_get_sync(dev->parent);
-		err = scsi_dev_type_resume(dev);
+		err = scsi_dev_type_resume(dev, false);
 		pm_runtime_put_sync(dev->parent);
 	}
 
@@ -159,7 +163,7 @@ static int scsi_runtime_resume(struct device *dev)
 
 	dev_dbg(dev, "scsi_runtime_resume\n");
 	if (scsi_is_sdev_device(dev))
-		err = scsi_dev_type_resume(dev);
+		err = scsi_dev_type_resume(dev, true);
 
 	/* Insert hooks here for targets, hosts, and transport classes */
 
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 69c9e22..2f159aa 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -82,6 +82,11 @@ static int sr_remove(struct device *);
 static int sr_done(struct scsi_cmnd *);
 static int sr_suspend(struct device *dev, pm_message_t msg);
 static int sr_resume(struct device *dev);
+static int sr_runtime_resume(struct device *dev);
+
+static struct dev_pm_ops sr_pm_ops = {
+	.runtime_resume = sr_runtime_resume,
+};
 
 static struct scsi_driver sr_template = {
 	.owner			= THIS_MODULE,
@@ -91,6 +96,7 @@ static struct scsi_driver sr_template = {
 		.remove		= sr_remove,
 		.suspend	= sr_suspend,
 		.resume		= sr_resume,
+		.pm		= &sr_pm_ops,
 	},
 	.done			= sr_done,
 };
@@ -215,6 +221,21 @@ static int sr_suspend(struct device *dev, pm_message_t msg)
 
 static int sr_resume(struct device *dev)
 {
+	struct scsi_cd *cd = dev_get_drvdata(dev);
+
+	/*
+	 * If ODD is runtime suspended before system pm, unblock disk
+	 * events now since on system resume, we will fully resume it
+	 * and set its rumtime status to active.
+	 */
+	if (pm_runtime_suspended(dev))
+		disk_unblock_events(cd->disk);
+
+	return 0;
+}
+
+static int sr_runtime_resume(struct device *dev)
+{
 	struct scsi_cd *cd;
 	struct scsi_sense_hdr sshdr;
 
-- 
1.7.11.3

Re: [PATCH 4/5] scsi: pm: use runtime resume callback if available

[Sergei Shtylyov]

Hello.

On 07/23/2012 10:49 AM, Aaron Lu wrote:

> When runtime resume a scsi device, if the device's driver has
> implemented runtime resume callback, use that instead of the resume
> callback.

> sr driver needs this to properly do different things for system resume
> and runtime resume.

> Signed-off-by: Aaron Lu <aaron.lu@amd.com>
> ---
>  drivers/scsi/scsi_pm.c | 14 +++++++++-----
>  drivers/scsi/sr.c      | 21 +++++++++++++++++++++
>  2 files changed, 30 insertions(+), 5 deletions(-)

> diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
> index d4201de..19bba47 100644
> --- a/drivers/scsi/scsi_pm.c
> +++ b/drivers/scsi/scsi_pm.c
> @@ -34,14 +34,18 @@ static int scsi_dev_type_suspend(struct device *dev, pm_message_t msg)
>  	return err;
>  }
>  
> -static int scsi_dev_type_resume(struct device *dev)
> +static int scsi_dev_type_resume(struct device *dev, bool runtime)
>  {
>  	struct device_driver *drv;
>  	int err = 0;
> +	int (*resume)(struct device *);
>  
>  	drv = dev->driver;
> -	if (drv && drv->resume)
> -		err = drv->resume(dev);
> +	if (runtime && drv && drv->pm && drv->pm->runtime_resume)
> +		resume = drv->pm->runtime_resume;
> +	else
> +		resume = drv ? drv->resume : NULL;

   Call thru NULL pointer below will cause kernel oops. Is it your intention?

> +	err = resume(dev);

WBR, Sergei

Re: [PATCH 4/5] scsi: pm: use runtime resume callback if available

[Aaron Lu]

Hi,

On Mon, Jul 23, 2012 at 06:36:12PM +0400, Sergei Shtylyov wrote:
> Hello.
> 
> On 07/23/2012 10:49 AM, Aaron Lu wrote:
> 
> > When runtime resume a scsi device, if the device's driver has
> > implemented runtime resume callback, use that instead of the resume
> > callback.
> 
> > sr driver needs this to properly do different things for system resume
> > and runtime resume.
> 
> > Signed-off-by: Aaron Lu <aaron.lu@amd.com>
> > ---
> >  drivers/scsi/scsi_pm.c | 14 +++++++++-----
> >  drivers/scsi/sr.c      | 21 +++++++++++++++++++++
> >  2 files changed, 30 insertions(+), 5 deletions(-)
> 
> > diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
> > index d4201de..19bba47 100644
> > --- a/drivers/scsi/scsi_pm.c
> > +++ b/drivers/scsi/scsi_pm.c
> > @@ -34,14 +34,18 @@ static int scsi_dev_type_suspend(struct device *dev, pm_message_t msg)
> >  	return err;
> >  }
> >  
> > -static int scsi_dev_type_resume(struct device *dev)
> > +static int scsi_dev_type_resume(struct device *dev, bool runtime)
> >  {
> >  	struct device_driver *drv;
> >  	int err = 0;
> > +	int (*resume)(struct device *);
> >  
> >  	drv = dev->driver;
> > -	if (drv && drv->resume)
> > -		err = drv->resume(dev);
> > +	if (runtime && drv && drv->pm && drv->pm->runtime_resume)
> > +		resume = drv->pm->runtime_resume;
> > +	else
> > +		resume = drv ? drv->resume : NULL;
> 
>    Call thru NULL pointer below will cause kernel oops. Is it your intention?

Oops, I forgot the if check here, thanks for pointing this out.

-Aaron

> 
> > +	err = resume(dev);
> 

[PATCH 5/5] block: genhd: add an interface to set disk's poll interval

[Aaron Lu]

Set the ODD's in kernel poll interval to 2s for the user in case the
user is using an old distro on which udev will not set the system wide
block parameter events_dfl_poll_msecs.

Signed-off-by: Aaron Lu <aaron.lu@amd.com>
---
 block/genhd.c         | 23 +++++++++++++++++------
 drivers/scsi/sr.c     |  1 +
 include/linux/genhd.h |  1 +
 3 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index bdb3682..de9b9d9 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -1619,6 +1619,19 @@ static void disk_events_workfn(struct work_struct *work)
 		kobject_uevent_env(&disk_to_dev(disk)->kobj, KOBJ_CHANGE, envp);
 }
 
+int disk_events_set_poll_msecs(struct gendisk *disk, long intv)
+{
+	if (intv < 0 && intv != -1)
+		return -EINVAL;
+
+	disk_block_events(disk);
+	disk->ev->poll_msecs = intv;
+	__disk_unblock_events(disk, true);
+
+	return 0;
+}
+EXPORT_SYMBOL(disk_events_set_poll_msecs);
+
 /*
  * A disk events enabled device has the following sysfs nodes under
  * its /sys/block/X/ directory.
@@ -1675,16 +1688,14 @@ static ssize_t disk_events_poll_msecs_store(struct device *dev,
 {
 	struct gendisk *disk = dev_to_disk(dev);
 	long intv;
+	int ret;
 
 	if (!count || !sscanf(buf, "%ld", &intv))
 		return -EINVAL;
 
-	if (intv < 0 && intv != -1)
-		return -EINVAL;
-
-	disk_block_events(disk);
-	disk->ev->poll_msecs = intv;
-	__disk_unblock_events(disk, true);
+	ret = disk_events_set_poll_msecs(disk, intv);
+	if (ret)
+		return ret;
 
 	return count;
 }
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 2f159aa..78c4226 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -869,6 +869,7 @@ static int sr_probe(struct device *dev)
 	dev_set_drvdata(dev, cd);
 	disk->flags |= GENHD_FL_REMOVABLE;
 	add_disk(disk);
+	disk_events_set_poll_msecs(disk, 2000);
 
 	sdev_printk(KERN_DEBUG, sdev,
 		    "Attached scsi CD-ROM %s\n", cd->cdi.name);
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 017a7fb..7414fb5 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -418,6 +418,7 @@ extern void disk_block_events(struct gendisk *disk);
 extern void disk_unblock_events(struct gendisk *disk);
 extern void disk_flush_events(struct gendisk *disk, unsigned int mask);
 extern unsigned int disk_clear_events(struct gendisk *disk, unsigned int mask);
+extern int disk_events_set_poll_msecs(struct gendisk *disk, long intv);
 
 /* drivers/char/random.c */
 extern void add_disk_randomness(struct gendisk *disk);
-- 
1.7.11.3

Re: [PATCH 5/5] block: genhd: add an interface to set disk's poll interval

[Betty Dall]

Hi Aaron,

On Mon, 2012-07-23 at 14:49 +0800, Aaron Lu wrote:
> Set the ODD's in kernel poll interval to 2s for the user in case the
> user is using an old distro on which udev will not set the system wide
> block parameter events_dfl_poll_msecs.
Why did you pick 2 seconds? 

> 
> Signed-off-by: Aaron Lu <aaron.lu@amd.com>
> ---
>  block/genhd.c         | 23 +++++++++++++++++------
>  drivers/scsi/sr.c     |  1 +
>  include/linux/genhd.h |  1 +
>  3 files changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/block/genhd.c b/block/genhd.c
> index bdb3682..de9b9d9 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -1619,6 +1619,19 @@ static void disk_events_workfn(struct work_struct *work)
>  		kobject_uevent_env(&disk_to_dev(disk)->kobj, KOBJ_CHANGE, envp);
>  }
>  
> +int disk_events_set_poll_msecs(struct gendisk *disk, long intv)
> +{
> +	if (intv < 0 && intv != -1)
> +		return -EINVAL;
> +
> +	disk_block_events(disk);
> +	disk->ev->poll_msecs = intv;
> +	__disk_unblock_events(disk, true);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(disk_events_set_poll_msecs);
> +
>  /*
>   * A disk events enabled device has the following sysfs nodes under
>   * its /sys/block/X/ directory.
> @@ -1675,16 +1688,14 @@ static ssize_t disk_events_poll_msecs_store(struct device *dev,
>  {
>  	struct gendisk *disk = dev_to_disk(dev);
>  	long intv;
> +	int ret;
>  
>  	if (!count || !sscanf(buf, "%ld", &intv))
>  		return -EINVAL;
>  
> -	if (intv < 0 && intv != -1)
> -		return -EINVAL;
> -
> -	disk_block_events(disk);
> -	disk->ev->poll_msecs = intv;
> -	__disk_unblock_events(disk, true);
> +	ret = disk_events_set_poll_msecs(disk, intv);
> +	if (ret)
> +		return ret;
>  
>  	return count;
>  }
> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> index 2f159aa..78c4226 100644
> --- a/drivers/scsi/sr.c
> +++ b/drivers/scsi/sr.c
> @@ -869,6 +869,7 @@ static int sr_probe(struct device *dev)
>  	dev_set_drvdata(dev, cd);
>  	disk->flags |= GENHD_FL_REMOVABLE;
>  	add_disk(disk);
> +	disk_events_set_poll_msecs(disk, 2000);

Could you check that disk event's poll_msecs is the default (-1) before
setting it to 2s? I am thinking of a case when the probe happens after
the call to disk_events_poll_msecs_store() and this code would overwrite
the user specified value.

>  
>  	sdev_printk(KERN_DEBUG, sdev,
>  		    "Attached scsi CD-ROM %s\n", cd->cdi.name);
> diff --git a/include/linux/genhd.h b/include/linux/genhd.h
> index 017a7fb..7414fb5 100644
> --- a/include/linux/genhd.h
> +++ b/include/linux/genhd.h
> @@ -418,6 +418,7 @@ extern void disk_block_events(struct gendisk *disk);
>  extern void disk_unblock_events(struct gendisk *disk);
>  extern void disk_flush_events(struct gendisk *disk, unsigned int mask);
>  extern unsigned int disk_clear_events(struct gendisk *disk, unsigned int mask);
> +extern int disk_events_set_poll_msecs(struct gendisk *disk, long intv);
>  
>  /* drivers/char/random.c */
>  extern void add_disk_randomness(struct gendisk *disk);

-Betty

Re: [PATCH 5/5] block: genhd: add an interface to set disk's poll interval

[Aaron Lu]

On Mon, Jul 23, 2012 at 12:43:34PM -0600, Betty Dall wrote:
> Hi Aaron,

Hi,

> 
> On Mon, 2012-07-23 at 14:49 +0800, Aaron Lu wrote:
> > Set the ODD's in kernel poll interval to 2s for the user in case the
> > user is using an old distro on which udev will not set the system wide
> > block parameter events_dfl_poll_msecs.
> Why did you pick 2 seconds? 

Just a random pick, no special meaning here.
On newer distros, udev will also pick 2s for the events_dfl_poll_msecs
parameter, so I just followed that :-)
Do you see any problem with this setting?

> 
> > 
> > Signed-off-by: Aaron Lu <aaron.lu@amd.com>
> > ---
> >  block/genhd.c         | 23 +++++++++++++++++------
> >  drivers/scsi/sr.c     |  1 +
> >  include/linux/genhd.h |  1 +
> >  3 files changed, 19 insertions(+), 6 deletions(-)
> > 
> > diff --git a/block/genhd.c b/block/genhd.c
> > index bdb3682..de9b9d9 100644
> > --- a/block/genhd.c
> > +++ b/block/genhd.c
> > @@ -1619,6 +1619,19 @@ static void disk_events_workfn(struct work_struct *work)
> >  		kobject_uevent_env(&disk_to_dev(disk)->kobj, KOBJ_CHANGE, envp);
> >  }
> >  
> > +int disk_events_set_poll_msecs(struct gendisk *disk, long intv)
> > +{
> > +	if (intv < 0 && intv != -1)
> > +		return -EINVAL;
> > +
> > +	disk_block_events(disk);
> > +	disk->ev->poll_msecs = intv;
> > +	__disk_unblock_events(disk, true);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(disk_events_set_poll_msecs);
> > +
> >  /*
> >   * A disk events enabled device has the following sysfs nodes under
> >   * its /sys/block/X/ directory.
> > @@ -1675,16 +1688,14 @@ static ssize_t disk_events_poll_msecs_store(struct device *dev,
> >  {
> >  	struct gendisk *disk = dev_to_disk(dev);
> >  	long intv;
> > +	int ret;
> >  
> >  	if (!count || !sscanf(buf, "%ld", &intv))
> >  		return -EINVAL;
> >  
> > -	if (intv < 0 && intv != -1)
> > -		return -EINVAL;
> > -
> > -	disk_block_events(disk);
> > -	disk->ev->poll_msecs = intv;
> > -	__disk_unblock_events(disk, true);
> > +	ret = disk_events_set_poll_msecs(disk, intv);
> > +	if (ret)
> > +		return ret;
> >  
> >  	return count;
> >  }
> > diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> > index 2f159aa..78c4226 100644
> > --- a/drivers/scsi/sr.c
> > +++ b/drivers/scsi/sr.c
> > @@ -869,6 +869,7 @@ static int sr_probe(struct device *dev)
> >  	dev_set_drvdata(dev, cd);
> >  	disk->flags |= GENHD_FL_REMOVABLE;
> >  	add_disk(disk);
> > +	disk_events_set_poll_msecs(disk, 2000);
> 
> Could you check that disk event's poll_msecs is the default (-1) before
> setting it to 2s? I am thinking of a case when the probe happens after
> the call to disk_events_poll_msecs_store() and this code would overwrite
> the user specified value.

The block device sr0 is created by this driver in this probe function,
so the user should not be able to set the poll interval before probe,
right?

Thanks,
Aaron

Re: [PATCH 5/5] block: genhd: add an interface to set disk's poll interval

[Betty Dall]

Hi Aaron,

On Tue, 2012-07-24 at 07:52 +0800, Aaron Lu wrote:
> On Mon, Jul 23, 2012 at 12:43:34PM -0600, Betty Dall wrote:
> > Hi Aaron,
> 
> Hi,
> 
> > 
> > On Mon, 2012-07-23 at 14:49 +0800, Aaron Lu wrote:
> > > Set the ODD's in kernel poll interval to 2s for the user in case the
> > > user is using an old distro on which udev will not set the system wide
> > > block parameter events_dfl_poll_msecs.
> > Why did you pick 2 seconds? 
> 
> Just a random pick, no special meaning here.
> On newer distros, udev will also pick 2s for the events_dfl_poll_msecs
> parameter, so I just followed that :-)
> Do you see any problem with this setting?

No problem, and I was curious as to why 2s, and the fact that is it used
in udev for events_dfl_poll_msecs is a good reason.

> > 
> > > 
> > > Signed-off-by: Aaron Lu <aaron.lu@amd.com>
> > > ---
> > >  block/genhd.c         | 23 +++++++++++++++++------
> > >  drivers/scsi/sr.c     |  1 +
> > >  include/linux/genhd.h |  1 +
> > >  3 files changed, 19 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/block/genhd.c b/block/genhd.c
> > > index bdb3682..de9b9d9 100644
> > > --- a/block/genhd.c
> > > +++ b/block/genhd.c
> > > @@ -1619,6 +1619,19 @@ static void disk_events_workfn(struct work_struct *work)
> > >  		kobject_uevent_env(&disk_to_dev(disk)->kobj, KOBJ_CHANGE, envp);
> > >  }
> > >  
> > > +int disk_events_set_poll_msecs(struct gendisk *disk, long intv)
> > > +{
> > > +	if (intv < 0 && intv != -1)
> > > +		return -EINVAL;
> > > +
> > > +	disk_block_events(disk);
> > > +	disk->ev->poll_msecs = intv;
> > > +	__disk_unblock_events(disk, true);
> > > +
> > > +	return 0;
> > > +}
> > > +EXPORT_SYMBOL(disk_events_set_poll_msecs);
> > > +
> > >  /*
> > >   * A disk events enabled device has the following sysfs nodes under
> > >   * its /sys/block/X/ directory.
> > > @@ -1675,16 +1688,14 @@ static ssize_t disk_events_poll_msecs_store(struct device *dev,
> > >  {
> > >  	struct gendisk *disk = dev_to_disk(dev);
> > >  	long intv;
> > > +	int ret;
> > >  
> > >  	if (!count || !sscanf(buf, "%ld", &intv))
> > >  		return -EINVAL;
> > >  
> > > -	if (intv < 0 && intv != -1)
> > > -		return -EINVAL;
> > > -
> > > -	disk_block_events(disk);
> > > -	disk->ev->poll_msecs = intv;
> > > -	__disk_unblock_events(disk, true);
> > > +	ret = disk_events_set_poll_msecs(disk, intv);
> > > +	if (ret)
> > > +		return ret;
> > >  
> > >  	return count;
> > >  }
> > > diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> > > index 2f159aa..78c4226 100644
> > > --- a/drivers/scsi/sr.c
> > > +++ b/drivers/scsi/sr.c
> > > @@ -869,6 +869,7 @@ static int sr_probe(struct device *dev)
> > >  	dev_set_drvdata(dev, cd);
> > >  	disk->flags |= GENHD_FL_REMOVABLE;
> > >  	add_disk(disk);
> > > +	disk_events_set_poll_msecs(disk, 2000);
> > 
> > Could you check that disk event's poll_msecs is the default (-1) before
> > setting it to 2s? I am thinking of a case when the probe happens after
> > the call to disk_events_poll_msecs_store() and this code would overwrite
> > the user specified value.
> 
> The block device sr0 is created by this driver in this probe function,
> so the user should not be able to set the poll interval before probe,
> right?

The add_disk() call happens immediately before the new
disk_events_set_poll_msecs() call. add_disk() is what eventually creates
the sysfs files and calls your new disk_events_set_poll_msecs(). It
makes more sense to me to have the new call to
disk_events_set_poll_msecs(disk, 2000) before the call to add_disk().
That way add_disk()could override the 2 second default based on user
input. If a distro doesn't support udev setting events_dfl_poll_msecs,
the add_disk() won't ever make a call to disk_events_set_poll_msecs()
and the 2 second default will stand.

-Betty

Re: [PATCH 5/5] block: genhd: add an interface to set disk's poll interval

[Aaron Lu]

Hi Betty,

On Tue, Jul 24, 2012 at 12:55:06PM -0600, Betty Dall wrote:
> > > > diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> > > > index 2f159aa..78c4226 100644
> > > > --- a/drivers/scsi/sr.c
> > > > +++ b/drivers/scsi/sr.c
> > > > @@ -869,6 +869,7 @@ static int sr_probe(struct device *dev)
> > > >  	dev_set_drvdata(dev, cd);
> > > >  	disk->flags |= GENHD_FL_REMOVABLE;
> > > >  	add_disk(disk);
> > > > +	disk_events_set_poll_msecs(disk, 2000);
> > > 
> > > Could you check that disk event's poll_msecs is the default (-1) before
> > > setting it to 2s? I am thinking of a case when the probe happens after
> > > the call to disk_events_poll_msecs_store() and this code would overwrite
> > > the user specified value.
> > 
> > The block device sr0 is created by this driver in this probe function,
> > so the user should not be able to set the poll interval before probe,
> > right?
> 
> The add_disk() call happens immediately before the new
> disk_events_set_poll_msecs() call. add_disk() is what eventually creates
> the sysfs files

Right, and it's disk_add_events in add_disk that adds these sysfs files.

> and calls your new disk_events_set_poll_msecs().

No... there is no call to disk_events_set_poll_msecs in add_disk, when
the events for the disk is created by disk_alloc_events, the poll_msecs
of the event is initialized to the default value -1. And then
disk_add_events will create the sysfs files and add_disk will return,
and I'll change the default value of -1 to 2000 with the new function
I've made.

> It makes more sense to me to have the new call to
> disk_events_set_poll_msecs(disk, 2000) before the call to add_disk().

This is too early, since the events of the disk is not allocated yet.

I hope I've explained this clearly, if you see a problem, please let me
know, thanks.

-Aaron