diff mbox

[v8,10/11] scsi: sr: support (un)block events

Message ID 1351501298-3716-11-git-send-email-aaron.lu@intel.com (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Aaron Lu Oct. 29, 2012, 9:01 a.m. UTC 
2 interfaces are added to block/unblock events for the disk sr manages.
This is used by SATA ZPODD, when ODD is runtime powered off, the events
poll is no longer needed so better be blocked. And once powered on,
events poll will be unblocked.

These 2 interfaces are needed here because SATA layer does not have
access to the gendisk structure sr manages.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/scsi/Makefile   |  1 +
 drivers/scsi/sr_zpodd.c | 21 +++++++++++++++++++++
 drivers/scsi/sr_zpodd.h |  9 +++++++++
 3 files changed, 31 insertions(+)
 create mode 100644 drivers/scsi/sr_zpodd.c
 create mode 100644 drivers/scsi/sr_zpodd.h

Comments

James Bottomley Oct. 29, 2012, 6:11 p.m. UTC | #1 
On Mon, 2012-10-29 at 17:01 +0800, Aaron Lu wrote:
> 2 interfaces are added to block/unblock events for the disk sr manages.
> This is used by SATA ZPODD, when ODD is runtime powered off, the events
> poll is no longer needed so better be blocked. And once powered on,
> events poll will be unblocked.
> 
> These 2 interfaces are needed here because SATA layer does not have
> access to the gendisk structure sr manages.

I'm afraid this is a nasty layering violation.  You can't have a low
level driver have knowledge of a call back in an upper layer one (in
this case sr_block_events).

This is all done it looks like because of the problem of getting access
to the scsi_cd structure from libata, but it's not the right way to
solve it.

I also don't really think you need to do any blocking or unblocking.  As
I said in the previous thread, just fake the events the standard tells
you to, so userspace can probe to its heart's content and you can keep
the device powered off.

James


> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> ---
>  drivers/scsi/Makefile   |  1 +
>  drivers/scsi/sr_zpodd.c | 21 +++++++++++++++++++++
>  drivers/scsi/sr_zpodd.h |  9 +++++++++
>  3 files changed, 31 insertions(+)
>  create mode 100644 drivers/scsi/sr_zpodd.c
>  create mode 100644 drivers/scsi/sr_zpodd.h
> 
> diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
> index 888f73a..474efe2 100644
> --- a/drivers/scsi/Makefile
> +++ b/drivers/scsi/Makefile
> @@ -177,6 +177,7 @@ sd_mod-objs	:= sd.o
>  sd_mod-$(CONFIG_BLK_DEV_INTEGRITY) += sd_dif.o
>  
>  sr_mod-objs	:= sr.o sr_ioctl.o sr_vendor.o
> +sr_mod-$(CONFIG_SATA_ZPODD) += sr_zpodd.o
>  ncr53c8xx-flags-$(CONFIG_SCSI_ZALON) \
>  		:= -DCONFIG_NCR53C8XX_PREFETCH -DSCSI_NCR_BIG_ENDIAN \
>  			-DCONFIG_SCSI_NCR53C8XX_NO_WORD_TRANSFERS
> diff --git a/drivers/scsi/sr_zpodd.c b/drivers/scsi/sr_zpodd.c
> new file mode 100644
> index 0000000..0686e8c
> --- /dev/null
> +++ b/drivers/scsi/sr_zpodd.c
> @@ -0,0 +1,21 @@
> +#include <linux/module.h>
> +#include <linux/genhd.h>
> +#include <linux/cdrom.h>
> +#include "sr.h"
> +
> +bool sr_block_events(struct device *dev)
> +{
> +	struct scsi_cd *cd = dev_get_drvdata(dev);
> +	if (cd)
> +		return disk_try_block_events(cd->disk);
> +	return false;
> +}
> +EXPORT_SYMBOL(sr_block_events);
> +
> +void sr_unblock_events(struct device *dev)
> +{
> +	struct scsi_cd *cd = dev_get_drvdata(dev);
> +	if (cd)
> +		disk_unblock_events(cd->disk);
> +}
> +EXPORT_SYMBOL(sr_unblock_events);
> diff --git a/drivers/scsi/sr_zpodd.h b/drivers/scsi/sr_zpodd.h
> new file mode 100644
> index 0000000..49c6a55
> --- /dev/null
> +++ b/drivers/scsi/sr_zpodd.h
> @@ -0,0 +1,9 @@
> +#ifndef __SR_ZPODD_H__
> +#define __SR_ZPODD_H__
> +
> +#include <linux/device.h>
> +
> +extern bool sr_block_events(struct device *dev);
> +extern void sr_unblock_events(struct device *dev);
> +
> +#endif


--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alan Stern Oct. 29, 2012, 10:22 p.m. UTC | #2 
On Mon, 29 Oct 2012, James Bottomley wrote:

> On Mon, 2012-10-29 at 17:01 +0800, Aaron Lu wrote:
> > 2 interfaces are added to block/unblock events for the disk sr manages.
> > This is used by SATA ZPODD, when ODD is runtime powered off, the events
> > poll is no longer needed so better be blocked. And once powered on,
> > events poll will be unblocked.
> > 
> > These 2 interfaces are needed here because SATA layer does not have
> > access to the gendisk structure sr manages.
> 
> I'm afraid this is a nasty layering violation.  You can't have a low
> level driver have knowledge of a call back in an upper layer one (in
> this case sr_block_events).
> 
> This is all done it looks like because of the problem of getting access
> to the scsi_cd structure from libata, but it's not the right way to
> solve it.
> 
> I also don't really think you need to do any blocking or unblocking.  As
> I said in the previous thread, just fake the events the standard tells
> you to, so userspace can probe to its heart's content and you can keep
> the device powered off.

James, one of us has misunderstood the event-polling mechanism.  As far
as I know, the polling calls that Aaron is concerned with are generated
by the kernel, not by userspace.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
James Bottomley Oct. 30, 2012, 4:34 a.m. UTC | #3 
On Mon, 2012-10-29 at 18:22 -0400, Alan Stern wrote:
> On Mon, 29 Oct 2012, James Bottomley wrote:
> 
> > On Mon, 2012-10-29 at 17:01 +0800, Aaron Lu wrote:
> > > 2 interfaces are added to block/unblock events for the disk sr manages.
> > > This is used by SATA ZPODD, when ODD is runtime powered off, the events
> > > poll is no longer needed so better be blocked. And once powered on,
> > > events poll will be unblocked.
> > > 
> > > These 2 interfaces are needed here because SATA layer does not have
> > > access to the gendisk structure sr manages.
> > 
> > I'm afraid this is a nasty layering violation.  You can't have a low
> > level driver have knowledge of a call back in an upper layer one (in
> > this case sr_block_events).
> > 
> > This is all done it looks like because of the problem of getting access
> > to the scsi_cd structure from libata, but it's not the right way to
> > solve it.
> > 
> > I also don't really think you need to do any blocking or unblocking.  As
> > I said in the previous thread, just fake the events the standard tells
> > you to, so userspace can probe to its heart's content and you can keep
> > the device powered off.
> 
> James, one of us has misunderstood the event-polling mechanism.  As far
> as I know, the polling calls that Aaron is concerned with are generated
> by the kernel, not by userspace.

I don't think the origin of the poll changes anything in what I said
above, does it?

James


--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Aaron Lu Oct. 30, 2012, 5:02 a.m. UTC | #4 
On 10/30/2012 02:11 AM, James Bottomley wrote:
> On Mon, 2012-10-29 at 17:01 +0800, Aaron Lu wrote:
>> 2 interfaces are added to block/unblock events for the disk sr manages.
>> This is used by SATA ZPODD, when ODD is runtime powered off, the events
>> poll is no longer needed so better be blocked. And once powered on,
>> events poll will be unblocked.
>>
>> These 2 interfaces are needed here because SATA layer does not have
>> access to the gendisk structure sr manages.
> 
> I'm afraid this is a nasty layering violation.  You can't have a low
> level driver have knowledge of a call back in an upper layer one (in
> this case sr_block_events).

I agree. But since ZPODD is now implemented in ATA layer now, this seems
the only way to do it.

> 
> This is all done it looks like because of the problem of getting access
> to the scsi_cd structure from libata, but it's not the right way to
> solve it.

Indeed, and any suggestion on a right way?

> 
> I also don't really think you need to do any blocking or unblocking.  As
> I said in the previous thread, just fake the events the standard tells
> you to, so userspace can probe to its heart's content and you can keep
> the device powered off.

No problem for the device, the problem is for its ancestor devices in my
current implementation. If the event is not blocked, the ODD will be
runtime resumed every 2 seconds. And along the way, all its ancestor
devices will have to be runtime resumed every 2 seconds, even I did some
trick to avoid power on the ODD itself. This is not a good thing from
power management perspective.

And there are two reasons I want to align runtime PM with ZPODD:

1 Simpler code.
And if I do not align ZPODD with runtime PM, I will need to maintain the
ODD's software status myself, and deal with some race conditions like
when I was to power off the ODD, a new command may arrive; or when the
ODD is powering on, all arriving commands need to be failed for the
moment untill the ODD is fully recovered from EH, etc. And all these
things can be avoided if I rely on runtime PM framework.

2 Integrate with the whole system
With runtime PM, when ODD is suspended, its ancestor devices will have a
chance to enter low power state.

Basically, I think power off is a special suspend state, it should
happen after the device enters runtime suspended state, and this is the
idea I used to implement ZPODD.

Thanks,
Aaron

> 
> James
> 
> 
>> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
>> ---
>>  drivers/scsi/Makefile   |  1 +
>>  drivers/scsi/sr_zpodd.c | 21 +++++++++++++++++++++
>>  drivers/scsi/sr_zpodd.h |  9 +++++++++
>>  3 files changed, 31 insertions(+)
>>  create mode 100644 drivers/scsi/sr_zpodd.c
>>  create mode 100644 drivers/scsi/sr_zpodd.h
>>
>> diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
>> index 888f73a..474efe2 100644
>> --- a/drivers/scsi/Makefile
>> +++ b/drivers/scsi/Makefile
>> @@ -177,6 +177,7 @@ sd_mod-objs	:= sd.o
>>  sd_mod-$(CONFIG_BLK_DEV_INTEGRITY) += sd_dif.o
>>  
>>  sr_mod-objs	:= sr.o sr_ioctl.o sr_vendor.o
>> +sr_mod-$(CONFIG_SATA_ZPODD) += sr_zpodd.o
>>  ncr53c8xx-flags-$(CONFIG_SCSI_ZALON) \
>>  		:= -DCONFIG_NCR53C8XX_PREFETCH -DSCSI_NCR_BIG_ENDIAN \
>>  			-DCONFIG_SCSI_NCR53C8XX_NO_WORD_TRANSFERS
>> diff --git a/drivers/scsi/sr_zpodd.c b/drivers/scsi/sr_zpodd.c
>> new file mode 100644
>> index 0000000..0686e8c
>> --- /dev/null
>> +++ b/drivers/scsi/sr_zpodd.c
>> @@ -0,0 +1,21 @@
>> +#include <linux/module.h>
>> +#include <linux/genhd.h>
>> +#include <linux/cdrom.h>
>> +#include "sr.h"
>> +
>> +bool sr_block_events(struct device *dev)
>> +{
>> +	struct scsi_cd *cd = dev_get_drvdata(dev);
>> +	if (cd)
>> +		return disk_try_block_events(cd->disk);
>> +	return false;
>> +}
>> +EXPORT_SYMBOL(sr_block_events);
>> +
>> +void sr_unblock_events(struct device *dev)
>> +{
>> +	struct scsi_cd *cd = dev_get_drvdata(dev);
>> +	if (cd)
>> +		disk_unblock_events(cd->disk);
>> +}
>> +EXPORT_SYMBOL(sr_unblock_events);
>> diff --git a/drivers/scsi/sr_zpodd.h b/drivers/scsi/sr_zpodd.h
>> new file mode 100644
>> index 0000000..49c6a55
>> --- /dev/null
>> +++ b/drivers/scsi/sr_zpodd.h
>> @@ -0,0 +1,9 @@
>> +#ifndef __SR_ZPODD_H__
>> +#define __SR_ZPODD_H__
>> +
>> +#include <linux/device.h>
>> +
>> +extern bool sr_block_events(struct device *dev);
>> +extern void sr_unblock_events(struct device *dev);
>> +
>> +#endif
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/scsi/Makefile b/drivers/scsi/Makefile
index 888f73a..474efe2 100644
--- a/drivers/scsi/Makefile
+++ b/drivers/scsi/Makefile
@@ -177,6 +177,7 @@  sd_mod-objs	:= sd.o
 sd_mod-$(CONFIG_BLK_DEV_INTEGRITY) += sd_dif.o
 
 sr_mod-objs	:= sr.o sr_ioctl.o sr_vendor.o
+sr_mod-$(CONFIG_SATA_ZPODD) += sr_zpodd.o
 ncr53c8xx-flags-$(CONFIG_SCSI_ZALON) \
 		:= -DCONFIG_NCR53C8XX_PREFETCH -DSCSI_NCR_BIG_ENDIAN \
 			-DCONFIG_SCSI_NCR53C8XX_NO_WORD_TRANSFERS
diff --git a/drivers/scsi/sr_zpodd.c b/drivers/scsi/sr_zpodd.c
new file mode 100644
index 0000000..0686e8c
--- /dev/null
+++ b/drivers/scsi/sr_zpodd.c
@@ -0,0 +1,21 @@ 
+#include <linux/module.h>
+#include <linux/genhd.h>
+#include <linux/cdrom.h>
+#include "sr.h"
+
+bool sr_block_events(struct device *dev)
+{
+	struct scsi_cd *cd = dev_get_drvdata(dev);
+	if (cd)
+		return disk_try_block_events(cd->disk);
+	return false;
+}
+EXPORT_SYMBOL(sr_block_events);
+
+void sr_unblock_events(struct device *dev)
+{
+	struct scsi_cd *cd = dev_get_drvdata(dev);
+	if (cd)
+		disk_unblock_events(cd->disk);
+}
+EXPORT_SYMBOL(sr_unblock_events);
diff --git a/drivers/scsi/sr_zpodd.h b/drivers/scsi/sr_zpodd.h
new file mode 100644
index 0000000..49c6a55
--- /dev/null
+++ b/drivers/scsi/sr_zpodd.h
@@ -0,0 +1,9 @@ 
+#ifndef __SR_ZPODD_H__
+#define __SR_ZPODD_H__
+
+#include <linux/device.h>
+
+extern bool sr_block_events(struct device *dev);
+extern void sr_unblock_events(struct device *dev);
+
+#endif