Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756925Ab0GNOXL (ORCPT ); Wed, 14 Jul 2010 10:23:11 -0400 Received: from mailout4.w1.samsung.com ([210.118.77.14]:34520 "EHLO mailout4.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750943Ab0GNOXJ convert rfc822-to-8bit (ORCPT ); Wed, 14 Jul 2010 10:23:09 -0400 MIME-version: 1.0 Content-type: text/plain; charset=utf-8; format=flowed; delsp=yes Date: Wed, 14 Jul 2010 16:24:23 +0200 From: =?utf-8?B?TWljaGHFgiBOYXphcmV3aWN6?= Subject: Re: [PATCHv2] usb: gadget: storage: optional SCSI WRITE FUA bit In-reply-to: To: Andy Shevchenko Cc: linux-kernel@vger.kernel.org, Andy Shevchenko , Denis Karpov , Adrian Hunter , Alan Stern , David Brownell , Greg Kroah-Hartman , linux-usb@vger.kernel.org Message-id: Organization: Samsung Electronics Content-transfer-encoding: 8BIT User-Agent: Opera Mail/10.60 (Linux) References: <1279098331-7872-1-git-send-email-ext-andriy.shevchenko@nokia.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2834 Lines: 77 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) +----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo-- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/