On Fri, 14 Aug 2009, [iso-8859-2] Micha³ Nazarewicz wrote:
> This patch provides also a per-LUN configuration of
> removable, CD-ROM and read-only flags so that gadget
> may provide several LUNs each with different
> configuration.
>
> It adds more configuration options to configuration as
> one may turn off various features (removable and CD-ROM
> support, turn read only always on) which may come in
> handy for small systems.
>
> The "can_stall" flag is also configurable and turned off
> by default as it seemd to break the mass storage functions
> while it was tested.
I don't like the approach taken in these two patches. You should leave
the existing file_storage.c alone and create new source files for the
composite version of the driver. Or if you prefer, create a new file
with common code for both versions, leaving the rest of the existing
code in file_storage.c untouched.
Also, you seem to have gone wild with configuration options. They are
a bad thing in general; they force people to make choices before
understanding all the issues involved. IMO you should leave out the
FILE_STORAGE_TEST config option entirely from the composite version.
It isn't needed for ordinary use, only for specialized testing which
can be done using FSG. The other options probably should be enabled
always, since disabling them saves very little code. If you want to
make some of them configurable, make them configurable at runtime via
module parameters or sysfs attributes.
("can_stall" should not be disabled by default. The fact that it
didn't work when you tried it indicates only that you were using
unsuitable hardware. The default should be to follow the spec, which
says that the gadget must stall.)
In addition, the patch overuses macros. Macros should be used only
when they make the code easier to read and understand; they shouldn't
be used merely to save typing.
There were one or two other odd things. For example, what's the point
of this business about making fsf_bufflen an alias so that it can't be
assigned to? If you don't want it assigned to, then don't assign
anything to it! If you like, declare it as "const" -- everybody will
understand what that means. There's no need to obscure the code.
And on top of everything else, you based your work on an old version of
file_storage.c. I hope you'll redo it based on the current version.
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
