Am Dienstag, 15. Januar 2008 16:30:34 schrieb Alan Stern:
> On Tue, 15 Jan 2008, Oliver Neukum wrote:
>
> > Hi Alan,
> >
> > here's a simple implementation to handle ioctl() by blocking
> > autosuspend until the device is closed again.
> >
> > It is relative to your patch set.
>
> A few comments are below.
>
> > --- linux-as/drivers/scsi/sd.c 2008-01-15 14:17:05.000000000 +0100
> > +++ linux-2.6.24-scsi-pm/drivers/scsi/sd.c 2008-01-15 14:20:13.000000000
> > +0100
> > @@ -711,6 +718,19 @@ static int sd_ioctl(struct inode * inode
> > case SCSI_IOCTL_GET_BUS_NUMBER:
> > return scsi_ioctl(sdp, cmd, p);
> > default:
>
> Do all ioctls filter through this routine? It looks like requests
> coming through block/scsi_ioctl.c will bypass this code. Have you
> decided to ignore those requests for now?
I found no way to deal with them without pushing the autosuspend code
into the generic code.
> > + /* closer filtering should go here */
> > +#ifdef CONFIG_SCSI_DYNAMIC_PM
> > + if (!sdp->autosuspend_ioctl_blocked) {
> > + error = scsi_autoresume_device(sdp);
> > + if (error < 0)
> > + return error;
> > + /* check for lost race due to drop of BKL */
> > + if (sdp->autosuspend_ioctl_blocked)
> > + scsi_autosuspend_device(sdp);
> > + else
> > + sdp->autosuspend_ioctl_blocked = 1;
>
> This is still racy; you need a real synchronization mechanism. For
> instance, you could use sdp->pm_mutex.
How is this racy? We hold BKL in ioctl().
> > + }
> > +#endif
> > error = scsi_cmd_ioctl(filp, disk->queue, disk, cmd, p);
> > if (error != -ENOTTY)
> > return error;
> > --- linux-as/include/scsi/scsi_device.h 2008-01-15 14:17:05.000000000
> > +0100
> > +++ linux-2.6.24-scsi-pm/include/scsi/scsi_device.h 2008-01-14
> > 12:45:12.000000000 +0100
> > @@ -177,6 +177,7 @@ struct scsi_device {
> > unsigned auto_pm:1; /* doing autosuspend or autoresume */
> > unsigned autosuspend_disabled:1; /* autosuspend & autoresume */
> > unsigned autoresume_disabled:1; /* disabled by the user */
> > + unsigned autosuspend_ioctl_blocked:1; /* disabled due to ioctl use */
> > unsigned skip_sys_resume:1; /* skip the next system resume */
> > unsigned use_ULD_pm:1; /* call the Upper-Level Driver's
> > * suspend/resume methods */
>
> The new flag is present for all SCSI devices, but you added code to use
> it only in the sd driver. What about the other upper-level SCSI
> drivers?
To be added once I figure out how to handle cd drives used to play audio.
Adding a timer for CDs' maximum play time seems a bit gross.
Regards
Oliver
-
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
