[lkml]   [2010]   [Jul]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCHv2] usb: gadget: storage: optional SCSI WRITE FUA bit
    On Wed, 14 Jul 2010 15:44:58 +0200, Andy Shevchenko <> wrote:
    >>> @@ -93,6 +93,8 @@
    >>> * removable Default false, boolean for removable media
    >>> * luns=N Default N = number of filenames, number of
    >>> * LUNs to support
    >>> + * fua=b[,b...] Default false, booleans for ignore FUA
    >>> flag
    >>> + * in SCSI WRITE(6,10,12) commands
    >> I wonder if it makes sense to make it per-LUN. I would imagine
    >> that it's great to ignore FUA if the device has its own power supply
    >> in which case after disconnect the data won't be lost. This is a
    >> per-device property not really per-LUN. As such I'd make this option
    >> global for the gadget.

    > Make sense only for removable media with one partition.
    > Otherwise. why we have sync option per partition f.e., not per device?

    Ah, OK, I see why this is per LUN. You want to be able not to ignore
    FUA if the backing storage is a removable media, right?

    >>> + ssize_t rc = count;

    >> Not really needed here.

    > See below

    This still stands. The “rc” is not needed.

    >>> + if (sscanf(buf, "%d", &i) != 1)
    >>> + return -EINVAL;

    >> Why not simple_strtol() directly?

    > I did it in the same way like fsg_store_ro() does.
    > I have no objections to back to previous solution.

    OK. I'd use simpre_strol() myself. Maybe even patched fsg_store_ro().

    >>> +
    >>> + if (curlun->fua)
    >>> + fsg_lun_fsync_sub(curlun);

    >> Shouldn't that read something like:
    >> + if (!curlun->fua && i)
    >> + fsg_lun_fsync_sub(curlun);
    >> ie. there is no sense in syncing if FUA was active (in which case all
    >> writes were synced already, right?) or if the new value is false (since
    >> then user does not won't syncing).

    > The idea is to sync data before switching from async mode.

    But there can be a case of switching from async to async when syncing
    is not necessary. That's why I proposed the &&. With fua = 1 meaning
    ignore the flag my proposal would be:

    + if (!i && curlun->fua)
    + fsg_lun_fsync_sub(curlun);

    > Actually fua = 1 means ignorance of that flag.

    ignore_fua would be better name then I think. This also stands for
    module parameter.

    Best regards, _ _
    | Humble Liege of Serenely Enlightened Majesty of o' \,=./ `o
    | Computer Science, Michał "mina86" Nazarewicz (o o)
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to
    More majordomo info at
    Please read the FAQ at

     \ /
      Last update: 2010-07-14 16:25    [W:0.023 / U:4.532 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site