On Wed, 2 Nov 2011, Yuping Luo wrote:
> On Sat, Oct 29, 2011 at 5:48 AM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
> > On Fri, 28 Oct 2011, Felipe Balbi wrote:
> >
> >> On Fri, Oct 28, 2011 at 03:22:16PM -0400, Alan Stern wrote:
> >> > On Fri, 28 Oct 2011, Alan Stern wrote:
> >> >
> >> > > The important questions are:
> >> > >
> >> > > � When are the right times to flush the page cache?
> >> > >
> >> > > � Why should the MSC gadget have to worry about this in the
> >> > > � first place? �That is, why doesn't the rest of the kernel
> >> > > � take care of this automatically?
> >> >
> >> > I just went back and looked at the SCSI spec. �The description of START
> >> > STOP UNIT says this:
> >> >
> >> > � � Targets that contain cache memory shall implicitly perform a
> >> > � � SYNCHRONIZE CACHE command for the entire medium prior to
> >> > � � executing the STOP UNIT command.
> >> >
> >> > That's pretty clear. �Currently the do_start_stop() routine just calls
> >> > fsg_lun_close(); it looks like we should put an fsg_lun_fsync_sub()
> >> > call there too if the backing storage is not a regular file (and maybe
> >> > even if it _is_ a regular file, if we want to obey the spec strictly).
> >> >
> >> > Then there won't be any need for fsg_lun_fsync_sub() in
> >> > do_prevent_allow().
> >>
> >> Makes sense to me. Care to send a patch to be queued for this -rc cycle?
> >
> > If Yuping or Barry would like to do it, that's fine with me. �Otherwise
> > I can send a patch.
> >
> > Alan Stern
> >
> >
> just revert my previous change, and add following, will test with
> windows os and linux.
>
> diff --git a/drivers/usb/gadget/f_mass_storage.c
> b/drivers/usb/gadget/f_mass_storage.c
> index 52583a2..2f79416 100644
> --- a/drivers/usb/gadget/f_mass_storage.c
> +++ b/drivers/usb/gadget/f_mass_storage.c
> @@ -1497,8 +1497,6 @@ static int do_prevent_allow(struct fsg_common *common)
> return -EINVAL;
> }
>
> - if (curlun->prevent_medium_removal && !prevent)
> - fsg_lun_fsync_sub(curlun);
> curlun->prevent_medium_removal = prevent;
> return 0;
> }
> diff --git a/drivers/usb/gadget/file_storage.c
> b/drivers/usb/gadget/file_storage.c
> index 3ac4f51..9a7f7f5 100644
> --- a/drivers/usb/gadget/file_storage.c
> +++ b/drivers/usb/gadget/file_storage.c
> @@ -1880,8 +1880,6 @@ static int do_prevent_allow(struct fsg_dev *fsg)
> return -EINVAL;
> }
>
> - if (curlun->prevent_medium_removal && !prevent)
> - fsg_lun_fsync_sub(curlun);
> curlun->prevent_medium_removal = prevent;
> return 0;
> }
> diff --git a/drivers/usb/gadget/storage_common.c
> b/drivers/usb/gadget/storage_common.c
> index c7f291a..2cef9de 100644
> --- a/drivers/usb/gadget/storage_common.c
> +++ b/drivers/usb/gadget/storage_common.c
> @@ -765,6 +765,7 @@ static void fsg_lun_close(struct fsg_lun *curlun)
> {
> if (curlun->filp) {
> LDBG(curlun, "close backing file\n");
> + fsg_lun_fsync_sub(curlun);
> fput(curlun->filp);
> curlun->filp = NULL;
> }
You know what? I finally remembered to look at the SCSI spec's
description of PREVENT ALLOW MEDIUM REMOVAL:
The prevention of medium removal shall begin when any initiator
issues a PREVENT ALLOW MEDIUM REMOVAL command with a prevent
bit of one (medium removal prevented). The prevention of medium
removal for the logical unit shall terminate:
a) after all initiators that have medium removal
prevented issue PREVENT ALLOW MEDIUM REMOVAL commands
with a prevent bit of zero, and the target has
successfully performed a synchronize cache operation;
b) upon the receipt of a BUS DEVICE RESET message from any
initiator; or
c) upon a hard RESET condition.
The last line of a) indicates that we really do need to keep the
fsg_lun_fsync_sub() calls where they are. We probably could skip the
sync if the backing storage is a regular file, because in that case it
shouldn't be removed until its filesystem is unmounted, and unmounting
syncs the page cache.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
