[v7,3/6] scsi: sr: support zero power ODD(ZPODD)

On 09/22/2012 05:02 AM, Rafael J. Wysocki wrote:
> On Friday, September 21, 2012, Aaron Lu wrote:
>> On Fri, Sep 21, 2012 at 12:07:23AM +0200, Rafael J. Wysocki wrote:
>>> On Wednesday, September 12, 2012, Aaron Lu wrote:
>>>>  static struct scsi_driver sr_template = {
>>>>  	.owner			= THIS_MODULE,
>>>> @@ -87,6 +89,8 @@ static struct scsi_driver sr_template = {
>>>>  		.name   	= "sr",
>>>>  		.probe		= sr_probe,
>>>>  		.remove		= sr_remove,
>>>> +		.suspend        = sr_suspend,
>>>> +		.resume         = sr_resume,
>>>>  	},
>>>>  	.done			= sr_done,
>>>>  };
>>>> @@ -172,6 +176,52 @@ static void scsi_cd_put(struct scsi_cd *cd)
>>>>  	mutex_unlock(&sr_ref_mutex);
>>>>  }
>>>
>>> Besides, I need some help to understand how this is supposed to work.
>>>
>>> Do I think correctly that sr_suspend(), for example, will be run by the
>>> SCSI bus type layer in case of a CD device runtime suspend?  However,
>>
>> Yes.
>>
>>> won't this routine be used during system suspend as well and won't it cause
>>> problems to happen if so?
>>
>> On system suspend, nothing needs to be done.
>> I'll add the following code in next version.
>>
>> 	if (!PMSG_IS_AUTO(msg))
>> 		return 0;
> 
> Please don't.  The pm_message_t thing is obsolete and shoulnd't really be
> used by device drivers.  I know that ATA relies on it internally, but that's
> just something that needs to be changed at one point.
> 
> Moreover, I'd like to migrate SCSI drivers to the PM handling based on struct
> dev_pm_ops eventually and your change is kind of going in the opposite
> direction.  I don't know how much effort the migration is going to take,
> though, so perhaps we can just make this change first.

Does the following change look OK?


And I'll define runtime callbacks for sr and sd.

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

On Thu, 27 Sep 2012, Aaron Lu wrote:

> > Moreover, I'd like to migrate SCSI drivers to the PM handling based on struct
> > dev_pm_ops eventually and your change is kind of going in the opposite
> > direction.  I don't know how much effort the migration is going to take,
> > though, so perhaps we can just make this change first.
> 
> Does the following change look OK?
> 
> diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
> index dc0ad85..1fb7ccc 100644
> --- a/drivers/scsi/scsi_pm.c
> +++ b/drivers/scsi/scsi_pm.c
> @@ -143,7 +143,15 @@ static int scsi_runtime_suspend(struct device *dev)
>  
>  	dev_dbg(dev, "scsi_runtime_suspend\n");
>  	if (scsi_is_sdev_device(dev)) {
> -		err = scsi_dev_type_suspend(dev, PMSG_AUTO_SUSPEND);
> +		err = scsi_device_quiesce(to_scsi_device(dev));
> +		if (err)
> +			goto out;
> +
> +		err = pm_generic_runtime_suspend(dev);
> +		if (!err)
> +			goto out;
> +
> +		scsi_device_resume(to_scsi_device(dev));
>  		if (err == -EAGAIN)
>  			pm_schedule_suspend(dev, jiffies_to_msecs(
>  				round_jiffies_up_relative(HZ/10)));

Maybe in the end it will be better, but for now this looks like 
unnecessary code duplication.  Basically you are copying 
scsi_dev_type_suspend() into scsi_runtime_suspend(), except that you 
omitted the debugging statement.

Alan Stern

--
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 09/27/2012 10:42 PM, Alan Stern wrote:
> On Thu, 27 Sep 2012, Aaron Lu wrote:
> 
>>> Moreover, I'd like to migrate SCSI drivers to the PM handling based on struct
>>> dev_pm_ops eventually and your change is kind of going in the opposite
>>> direction.  I don't know how much effort the migration is going to take,
>>> though, so perhaps we can just make this change first.
>>
>> Does the following change look OK?
>>
>> diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
>> index dc0ad85..1fb7ccc 100644
>> --- a/drivers/scsi/scsi_pm.c
>> +++ b/drivers/scsi/scsi_pm.c
>> @@ -143,7 +143,15 @@ static int scsi_runtime_suspend(struct device *dev)
>>  
>>  	dev_dbg(dev, "scsi_runtime_suspend\n");
>>  	if (scsi_is_sdev_device(dev)) {
>> -		err = scsi_dev_type_suspend(dev, PMSG_AUTO_SUSPEND);
>> +		err = scsi_device_quiesce(to_scsi_device(dev));
>> +		if (err)
>> +			goto out;
>> +
>> +		err = pm_generic_runtime_suspend(dev);
>> +		if (!err)
>> +			goto out;
>> +
>> +		scsi_device_resume(to_scsi_device(dev));
>>  		if (err == -EAGAIN)
>>  			pm_schedule_suspend(dev, jiffies_to_msecs(
>>  				round_jiffies_up_relative(HZ/10)));
> 
> Maybe in the end it will be better, but for now this looks like 
> unnecessary code duplication.  Basically you are copying 
> scsi_dev_type_suspend() into scsi_runtime_suspend(), except that you 
> omitted the debugging statement.

And I've used pm_generic_runtime_suspend :-)
That would allow me to write runtime callbacks of dev_pm_ops for
indivisual scsi drivers, like sr.

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

On Thursday, September 27, 2012, Aaron Lu wrote:
> On 09/27/2012 10:42 PM, Alan Stern wrote:
> > On Thu, 27 Sep 2012, Aaron Lu wrote:
> > 
> >>> Moreover, I'd like to migrate SCSI drivers to the PM handling based on struct
> >>> dev_pm_ops eventually and your change is kind of going in the opposite
> >>> direction.  I don't know how much effort the migration is going to take,
> >>> though, so perhaps we can just make this change first.
> >>
> >> Does the following change look OK?
> >>
> >> diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
> >> index dc0ad85..1fb7ccc 100644
> >> --- a/drivers/scsi/scsi_pm.c
> >> +++ b/drivers/scsi/scsi_pm.c
> >> @@ -143,7 +143,15 @@ static int scsi_runtime_suspend(struct device *dev)
> >>  
> >>  	dev_dbg(dev, "scsi_runtime_suspend\n");
> >>  	if (scsi_is_sdev_device(dev)) {
> >> -		err = scsi_dev_type_suspend(dev, PMSG_AUTO_SUSPEND);
> >> +		err = scsi_device_quiesce(to_scsi_device(dev));
> >> +		if (err)
> >> +			goto out;
> >> +
> >> +		err = pm_generic_runtime_suspend(dev);
> >> +		if (!err)
> >> +			goto out;
> >> +
> >> +		scsi_device_resume(to_scsi_device(dev));
> >>  		if (err == -EAGAIN)
> >>  			pm_schedule_suspend(dev, jiffies_to_msecs(
> >>  				round_jiffies_up_relative(HZ/10)));
> > 
> > Maybe in the end it will be better, but for now this looks like 
> > unnecessary code duplication.  Basically you are copying 
> > scsi_dev_type_suspend() into scsi_runtime_suspend(), except that you 
> > omitted the debugging statement.
> 
> And I've used pm_generic_runtime_suspend :-)
> That would allow me to write runtime callbacks of dev_pm_ops for
> indivisual scsi drivers, like sr.

Well, actually, I meant something different.

The above patch would make it possible for drivers to provide runtime PM
callbacks through dev->driver.pm, but drv->suspend and drv->resume would
still be used for system suspend/resume (and hibernation).

The transition to struct dev_pm_ops I meant, however, would be to start using
callbacks from dev->driver.pm for _all_ device PM operations by default and
fall back to drv->suspend and drv->resume for the drivers whose dev->driver.pm
is NULL (along the lines of what PCI does).  The next step would be to
convert all drivers to use dev->driver.pm only.

So I wouldn't like any drivers to use dev->driver.pm callbacks for some
operations and drv->suspend and drv->resume for the remaining ones at the
same time.

Thanks,
Rafael
--
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