Re: [PATCH v2 4/5] pm: use callbacks from dev_pm_ops for scsi devices

[Aaron Lu <aaron.lu@intel.com>]

On Thu, Oct 11, 2012 at 10:57:26AM -0400, Alan Stern wrote:
> I have a couple of small changes to suggest.  Nothing major.
> 
> On Thu, 11 Oct 2012, Aaron Lu wrote:
> 
> > @@ -102,26 +77,87 @@ static int scsi_bus_prepare(struct device *dev)
> >  
> >  static int scsi_bus_suspend(struct device *dev)
> >  {
> > -	return scsi_bus_suspend_common(dev, PMSG_SUSPEND);
> > +	int err = 0;
> > +	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> > +
> > +	if (scsi_is_sdev_device(dev)) {
> > +		/*
> > +		 * sd is the only high-level SCSI driver to implement runtime
> > +		 * PM, and sd treats runtime suspend, system suspend, and
> > +		 * system hibernate identically.
> > +		 */
> 
> It would be better if we don't refer to sd specifically.  The comment 
> could be changed to something like this:
> 
> 		/*
> 		 * All the high-level SCSI drivers that implement runtime PM
> 		 * treat runtime suspend, system suspend, and system
> 		 * hibernate identically.
> 		 */

OK, will do this.

> 
> >  static int scsi_bus_freeze(struct device *dev)
> >  {
> > -	return scsi_bus_suspend_common(dev, PMSG_FREEZE);
> > +	int err = 0;
> > +	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> > +
> > +	if (scsi_is_sdev_device(dev)) {
> > +		/* wake up device so that FREEZE will succeed */
> > +		if (pm_runtime_suspended(dev))
> > +			pm_runtime_resume(dev);
> 
> I'm not sure why this is here.  If none of the high-level drivers 
> implement FREEZE then it's not needed.

Agree.
I should have checked the git log to see why it is here.
>From the log, it doesn't seem to have a reason.
Thanks for pointing this out.

> 
> 
> >  static int scsi_bus_poweroff(struct device *dev)
> >  {
> > -	return scsi_bus_suspend_common(dev, PMSG_HIBERNATE);
> > +	int err = 0;
> > +	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> > +
> > +	if (scsi_is_sdev_device(dev)) {
> > +		/*
> > +		 * sd is the only high-level SCSI driver to implement runtime
> > +		 * PM, and sd treats runtime suspend, system suspend, and
> > +		 * system hibernate identically.
> > +		 */
> 
> Same change as above for this comment.

OK.

And thanks for reviewing.

-Aaron