Patchwork [v9,06/10] ata: zpodd: check zero power ready status

login
register
mail settings
Submitter Aaron Lu
Date Dec. 3, 2012, 8:13 a.m.
Message ID <20121203081321.GA9990@mint-spring.sh.intel.com>
Download mbox | patch
Permalink /patch/203297/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Aaron Lu - Dec. 3, 2012, 8:13 a.m.
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:



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.
> 
> 
> 
--
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
James Bottomley - Dec. 3, 2012, 8:25 a.m.
On Mon, 2012-12-03 at 16:13 +0800, Aaron Lu wrote:
> 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 */

Yes, but if we can get away with doing that, it should be in genhd
because it's completely generic.

I was imagining we'd have to fake the reply to the test unit ready or
some other commands, which is why it would need to be in sr.c

The check events code is Tejun's baby, if he's OK with it then just do
it in genhd.c

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

Right.

James


--
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
Aaron Lu - Dec. 3, 2012, 8:59 a.m.
On Mon, Dec 03, 2012 at 08:25:43AM +0000, James Bottomley wrote:
> On Mon, 2012-12-03 at 16:13 +0800, Aaron Lu wrote:
> > 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 */
> 
> Yes, but if we can get away with doing that, it should be in genhd
> because it's completely generic.
> 
> I was imagining we'd have to fake the reply to the test unit ready or
> some other commands, which is why it would need to be in sr.c
> 
> The check events code is Tejun's baby, if he's OK with it then just do
> it in genhd.c

I agree that it better be done in genhd, the only concern is, in that
case, this flag will need to be in struct device. I'm not sure if this
is acceptible though, since the whole events checking thing is for block
based devices only.

Ideally, it should be in struct disk_events, but I don't see a way for
libata to access that structure... so any suggestion of doing this? or
it is OK to add such a field to struct device?

Thanks,
Aaron

> 
> > Then when ZPODD is powered off, it will set this flag; and unset it when
> > it is powered up.
> 
> Right.
> 
> James
> 
> 
--
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
Tejun Heo - Dec. 3, 2012, 4:23 p.m.
Hello, James.

On Mon, Dec 03, 2012 at 08:25:43AM +0000, James Bottomley wrote:
> > 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 */
> 
> Yes, but if we can get away with doing that, it should be in genhd
> because it's completely generic.
> 
> I was imagining we'd have to fake the reply to the test unit ready or
> some other commands, which is why it would need to be in sr.c
> 
> The check events code is Tejun's baby, if he's OK with it then just do
> it in genhd.c

The problem here is there's no easy to reach genhd from libata (or the
other way around) without going through sr.  I think we're gonna have
to have something in sr one way or the other.

Thanks.
Jeff Garzik - Dec. 3, 2012, 6:56 p.m.
On 12/03/2012 11:23 AM, Tejun Heo wrote:
> Hello, James.
>
> On Mon, Dec 03, 2012 at 08:25:43AM +0000, James Bottomley wrote:
>>> 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 */
>>
>> Yes, but if we can get away with doing that, it should be in genhd
>> because it's completely generic.
>>
>> I was imagining we'd have to fake the reply to the test unit ready or
>> some other commands, which is why it would need to be in sr.c
>>
>> The check events code is Tejun's baby, if he's OK with it then just do
>> it in genhd.c
>
> The problem here is there's no easy to reach genhd from libata (or the
> other way around) without going through sr.  I think we're gonna have
> to have something in sr one way or the other.

  ...which is precisely as I said when v1 of this ZPODD patchset appeared.

sr modifications cannot be avoided.

	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
Aaron Lu - Dec. 4, 2012, 5:04 a.m.
On 12/04/2012 02:56 AM, Jeff Garzik wrote:
> On 12/03/2012 11:23 AM, Tejun Heo wrote:
>> Hello, James.
>>
>> On Mon, Dec 03, 2012 at 08:25:43AM +0000, James Bottomley wrote:
>>>> 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 */
>>>
>>> Yes, but if we can get away with doing that, it should be in genhd
>>> because it's completely generic.
>>>
>>> I was imagining we'd have to fake the reply to the test unit ready or
>>> some other commands, which is why it would need to be in sr.c
>>>
>>> The check events code is Tejun's baby, if he's OK with it then just do
>>> it in genhd.c
>>
>> The problem here is there's no easy to reach genhd from libata (or the
>> other way around) without going through sr.  I think we're gonna have
>> to have something in sr one way or the other.
> 
>   ...which is precisely as I said when v1 of this ZPODD patchset appeared.
> 
> sr modifications cannot be avoided.

So I'm gonna use the above code to silence the poll when ODD is powered
off. I suppose everybody is OK with this, right? James, please let me
know if you disagree.

Thanks,
Aaron

> 
> 	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
James Bottomley - Dec. 4, 2012, 12:11 p.m.
On Mon, 2012-12-03 at 08:23 -0800, Tejun Heo wrote:
> Hello, James.
> 
> On Mon, Dec 03, 2012 at 08:25:43AM +0000, James Bottomley wrote:
> > > 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 */
> > 
> > Yes, but if we can get away with doing that, it should be in genhd
> > because it's completely generic.
> > 
> > I was imagining we'd have to fake the reply to the test unit ready or
> > some other commands, which is why it would need to be in sr.c
> > 
> > The check events code is Tejun's baby, if he's OK with it then just do
> > it in genhd.c
> 
> The problem here is there's no easy to reach genhd from libata (or the
> other way around) without going through sr.  I think we're gonna have
> to have something in sr one way or the other.

Can't we do that via an event? It's a bit clunky because we need the
callback in the layer that sees the sdev, which is libata-scsi, we just
need an analogue of ata_scsi_media_change_notify, but ignoring and
allowing polling is essentially event driven as well, so it should all
work.  We'll need a listener in genhd, which might be trickier.

This may also work as the more generic route for stuff where we can't
connect the bottom to the top of the stack (which looks like a problem
we'll begin wrestling with a lot now).

James


--
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
Aaron Lu - Dec. 7, 2012, 6:13 a.m.
On 12/04/2012 08:11 PM, James Bottomley wrote:
> On Mon, 2012-12-03 at 08:23 -0800, Tejun Heo wrote:
>> Hello, James.
>>
>> On Mon, Dec 03, 2012 at 08:25:43AM +0000, James Bottomley wrote:
>>>> 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 */
>>>
>>> Yes, but if we can get away with doing that, it should be in genhd
>>> because it's completely generic.
>>>
>>> I was imagining we'd have to fake the reply to the test unit ready or
>>> some other commands, which is why it would need to be in sr.c
>>>
>>> The check events code is Tejun's baby, if he's OK with it then just do
>>> it in genhd.c
>>
>> The problem here is there's no easy to reach genhd from libata (or the
>> other way around) without going through sr.  I think we're gonna have
>> to have something in sr one way or the other.
> 
> Can't we do that via an event? It's a bit clunky because we need the
> callback in the layer that sees the sdev, which is libata-scsi, we just
> need an analogue of ata_scsi_media_change_notify, but ignoring and
> allowing polling is essentially event driven as well, so it should all
> work.  We'll need a listener in genhd, which might be trickier.

Hi Tejun,

Do you have any comments on James' suggestion? I want to know your
opinion, thanks.

Hi James,

Do you mean we start a thread in genhd that listen to events, so that
some driver can send an event to genhd about if the device should be
polled? I'm thinking where to store such information. If store it in
struct disk_events, then we will need to know which gendisk is for
the device, but how? Maybe by loop scan all gendisk's driverfs_dev?
If store it in struct device, then we do not need to send the event but
just set a flag in sturct device in libata, and let genhd check this
flag when poll. So I don't quite understand this, can you please
elaborate? Thanks.

Thanks,
Aaron

> 
> This may also work as the more generic route for stuff where we can't
> connect the bottom to the top of the stack (which looks like a problem
> we'll begin wrestling with a lot now).
> 
> James
> 
> 

--
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
Aaron Lu - Dec. 10, 2012, 3:26 a.m.
On 12/07/2012 02:13 PM, Aaron Lu wrote:
> On 12/04/2012 08:11 PM, James Bottomley wrote:
>> On Mon, 2012-12-03 at 08:23 -0800, Tejun Heo wrote:
>>> Hello, James.
>>>
>>> On Mon, Dec 03, 2012 at 08:25:43AM +0000, James Bottomley wrote:
>>>>> 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 */
>>>>
>>>> Yes, but if we can get away with doing that, it should be in genhd
>>>> because it's completely generic.
>>>>
>>>> I was imagining we'd have to fake the reply to the test unit ready or
>>>> some other commands, which is why it would need to be in sr.c
>>>>
>>>> The check events code is Tejun's baby, if he's OK with it then just do
>>>> it in genhd.c
>>>
>>> The problem here is there's no easy to reach genhd from libata (or the
>>> other way around) without going through sr.  I think we're gonna have
>>> to have something in sr one way or the other.
>>
>> Can't we do that via an event? It's a bit clunky because we need the
>> callback in the layer that sees the sdev, which is libata-scsi, we just
>> need an analogue of ata_scsi_media_change_notify, but ignoring and
>> allowing polling is essentially event driven as well, so it should all
>> work.  We'll need a listener in genhd, which might be trickier.
> 
> Hi Tejun,
> 
> Do you have any comments on James' suggestion? I want to know your
> opinion, thanks.

Hi Tejun,

A colleague of mine reminded me that it's impolite to write something
like this, and here is my apology. I didn't mean to be rude, I'm sorry
for this if it made you feel uncomfortable.

If possible, can you please shed some light on this problem and James'
suggestion? I need your opinions to make progress, thanks.

-Aaron

> 
> Hi James,
> 
> Do you mean we start a thread in genhd that listen to events, so that
> some driver can send an event to genhd about if the device should be
> polled? I'm thinking where to store such information. If store it in
> struct disk_events, then we will need to know which gendisk is for
> the device, but how? Maybe by loop scan all gendisk's driverfs_dev?
> If store it in struct device, then we do not need to send the event but
> just set a flag in sturct device in libata, and let genhd check this
> flag when poll. So I don't quite understand this, can you please
> elaborate? Thanks.
> 
> Thanks,
> Aaron
> 
>>
>> This may also work as the more generic route for stuff where we can't
>> connect the bottom to the top of the stack (which looks like a problem
>> we'll begin wrestling with a lot now).
>>
>> James
>>
>>
> 

--
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
Tejun Heo - Dec. 11, 2012, 5:10 a.m.
Hello, guys.

On Mon, Dec 10, 2012 at 11:26:07AM +0800, Aaron Lu wrote:
> >>> The problem here is there's no easy to reach genhd from libata (or the
> >>> other way around) without going through sr.  I think we're gonna have
> >>> to have something in sr one way or the other.
> >>
> >> Can't we do that via an event? It's a bit clunky because we need the
> >> callback in the layer that sees the sdev, which is libata-scsi, we just
> >> need an analogue of ata_scsi_media_change_notify, but ignoring and
> >> allowing polling is essentially event driven as well, so it should all
> >> work.  We'll need a listener in genhd, which might be trickier.

I'm not really following what you mean.  Can you please elaborate?
One way or the other, doesn't the notification have to bubble up
through SCSI?

> A colleague of mine reminded me that it's impolite to write something
> like this, and here is my apology. I didn't mean to be rude, I'm sorry
> for this if it made you feel uncomfortable.

Oh, no, it's not rude at all.  I was just being distracted w/ other
stuff and lazy.  Sorry about that.

Thanks.
Aaron Lu - Dec. 18, 2012, 8:30 a.m.
Hi Everyone,

Due to lack of information, I think I'll go ahead and pick up a simple
solution for this(i.e. the code I attached previously to set a flag
event_driven in scsi_device to let sr skip events poll) and send a new
series out for you to review. After all, I can't wait forever...

Please feel free to let me know if you don't think I should send a new
series with the above mentioned solution.

Thanks a lot for your(all of you) kind help and comments.

-Aaron

On 12/11/2012 01:10 PM, Tejun Heo wrote:
> Hello, guys.
> 
> On Mon, Dec 10, 2012 at 11:26:07AM +0800, Aaron Lu wrote:
>>>>> The problem here is there's no easy to reach genhd from libata (or the
>>>>> other way around) without going through sr.  I think we're gonna have
>>>>> to have something in sr one way or the other.
>>>>
>>>> Can't we do that via an event? It's a bit clunky because we need the
>>>> callback in the layer that sees the sdev, which is libata-scsi, we just
>>>> need an analogue of ata_scsi_media_change_notify, but ignoring and
>>>> allowing polling is essentially event driven as well, so it should all
>>>> work.  We'll need a listener in genhd, which might be trickier.
> 
> I'm not really following what you mean.  Can you please elaborate?
> One way or the other, doesn't the notification have to bubble up
> through SCSI?
> 
>> A colleague of mine reminded me that it's impolite to write something
>> like this, and here is my apology. I didn't mean to be rude, I'm sorry
>> for this if it made you feel uncomfortable.
> 
> Oh, no, it's not rude at all. I was just being distracted w/ other
> stuff and lazy.  Sorry about that.
> 
> Thanks.
> 

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

Patch

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