On Fri, 11 Sep 2009, Michal Nazarewicz wrote:
> Hello again!
>
> The following patchset is yet another version of the Mass Storage
> Function (MSF) which is a composite function based on File-backed
> Storage Gadget (FSG). This patchset takes into consideration all
> comments received after sending two previous patchsets.
>
> I tried to split the whole patchset into small diffs which will be
> easy to review so don't get scared by those 24 patches -- 7 of them
> are less then 100 and another 11 are less then 500 lines so it should
> not take so much time looking through them.
>
>
> Patches from 1 to 8 change the file_storage.c file and create
> storage_common.c and storage_common2.c files (the former depends on
> nothing from the main file whereas the later requires fsg_dev
> structure to be defined). I tried to keep functionality changes to
> the minimum as to minimise probability of introducing any bugs to
> already working FSG.
>
> Patch 9 simply copies the file_storage.c to f_mass_storage.c. From
> this point further, only patch 19 changes file_storage.c modifying
> a single line there (a constness issue).
>
> Patch 10 adds a Kconfig and Makefile options to compile the newly
> created file as a gadget. This is a temporary patch introduced for
> testing purpose -- it is reverted by patch 18.
>
> Patches from 11 to 17 modifies the f_mass_storage.c preparing it to be
> a composite function which will be able to be used in many USB
> configurations. This includes removing some unused parts as well as
> extracting parts common for a single MSF in different configurations.
>
> Patch 19 finally converts f_mass_storage.c into a USB composite
> function.
>
> Next two patches change the way configuration and module parameters
> are handled to a more flexible. Last three patches add minor features
> and fix comments.
On the whole this looks good. I did not spend much time reviewing the
new MSF code; instead I concentrated on the changes to the existing
code.
A few things stood out. If you're going to reorganize all these
patches for submission then fine, but you should strive to make sure
that each intermediate stage can build without errors. I noticed at
one point you had a typo ("lunx" instead of "luns") and NUM_BUFFERS
was defined twice.
Although you changed the stor_ and STOR_ prefixes to fsg_ and FSG_ in
the code, you missed some occurrences in the comments.
Making the drvdata for the LUN devices point to fsg->filesem is just
plain weird. Make it point to fsg and get filesem from there.
This also depends on how you decide to organize the patches for
submission: It might make sense to do more of the code motion first,
in file_storage.c, before making the copy.
Patch 19/24 has several misspellings in the new comments. It also
fails to remove all mention of the interrupt endpoint.
Would it be reasonable to consider all of fsg as common rather than
only part of it? After all, if you're going to have copies of
essentially the same driver in multiple configurations, shouldn't they
share everything?
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
