Re: [PATCH v6 1/7] scsi: sr: support runtime pm for ODD

From: Oliver Neukum <oliver@neukum.org>
To: Aaron Lu <aaron.lu@intel.com>
Cc: Alan Stern <stern@rowland.harvard.edu>,
	Aaron Lu <aaron.lwe@gmail.com>,
	James Bottomley <James.Bottomley@hansenpartnership.com>,
	Jeff Garzik <jgarzik@pobox.com>,
	linux-scsi@vger.kernel.org, linux-ide@vger.kernel.org,
	linux-pm@vger.kernel.org, linux-acpi@vger.kernel.org
Subject: Re: [PATCH v6 1/7] scsi: sr: support runtime pm for ODD
Date: Tue, 11 Sep 2012 10:55:44 +0200	[thread overview]
Message-ID: <3578472.dQIepChrCX@linux-lqwf.site> (raw)
In-Reply-To: <20120911064429.GA2196@mint-spring.sh.intel.com>

On Tuesday 11 September 2012 14:44:30 Aaron Lu wrote:
> On Mon, Sep 10, 2012 at 12:45:51PM +0200, Oliver Neukum wrote:
> > On Monday 10 September 2012 17:16:22 Aaron Lu wrote:
> > 
> > > +static int sr_resume(struct device *dev)
> > > +{
> > > +	struct scsi_cd *cd;
> > > +	struct scsi_sense_hdr sshdr;
> > > +
> > > +	cd = dev_get_drvdata(dev);
> > > +
> > > +	if (!cd->device->powered_off)
> > > +		return 0;
> > > +
> > > +	/* get the disk ready */
> > > +	scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr);
> > > +
> > > +	/* if user wakes up the ODD, eject the tray */
> > > +	if (cd->device->need_eject) {
> > > +		cd->device->need_eject = 0;
> > > +		if (!(cd->cdi.mask & CDC_CLOSE_TRAY))
> > > +			sr_tray_move(&cd->cdi, 1);
> > 
> > 1. Even if the door is locked?
> 
> This piece of code of sr_resume is intended for ZPODD devices, for
> normal devices it will simply return as the powered_off flag will never
> be set for them.
> 
> And for ZPODD devices, the door shouldn't be in locked state here since
> for it to enter power off state, "no medium inside" has to be satisfied

This is true only if the device is autosuspended. But sr_resume()
will also be called if the whole system was suspended.
What happens if the user presses the door button on the drive
while the system was suspended?

> and when there is no medium inside, we do not lock its door.
> 
> > 2. sr_tray_move allocates memory with GFP_KERNEL. This smells of a deadlock.
> 
> Sorry, don't get this. Can you please elaborate?

Suppose you need to wake up two devices for the same reason, a ZPODD and a hard
drive. The ZPODD is woken first and sr_resume() requests memory with GFP_KERNEL.
The VM layer decides to page out to the hard drive to be woken up -> deadlock.

	Regards
		Oliver