On Fri, 11 Sep 2009, Michal Nazarewicz wrote:
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.
On Fri, 11 Sep 2009 16:36:28 +0200, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote:
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,
That's the plan. ;)
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.
I would have swore I compiled the kernel after each patch. I'll check
that once again on Monday then. Also, if two macro definitions are
identical GCC won't issue a warning so I guess it's not such a big
deal since those are removed in the final version.
Although you changed the stor_ and STOR_ prefixes to fsg_ and FSG_ in
the code, you missed some occurrences in the comments.
Ah, thanks, that'll be an easy fix.
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.
I change it that way because FSG and MSF use different structures and
so the functions would have to have access to the definition of the
structure passed as drvdata -- it's even worse since it's fsg_dev
in FSG and fsg_common in MSF (so there would have to be a typedef or
#define). I decided that'll be better if the drvdata will point to
the filesem.
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.
I'm not sure if I understand what you mean. I tried to modify FSG as
little as possible -- most of the modifications involved merely moving
the code to storage_common.c or storage_common2.c. There are only a
few changes in the way FSG works which were made to make common files
independent from mod_data object.
Patch 19/24 has several misspellings in the new comments. It also
fails to remove all mention of the interrupt endpoint.
Yes, I only recently notices that there are some more references
which I have overlooked. I'm still not sure if I tracked all of them.
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?
I'm not sure if all of it can be shared. What I'm thinking about are
endpoints. Currently I'm working on moving the rest of the data to the
common structure but still enpoints need to be specific for instance
I think. This may take me a few days though plus I'm not sure if I'll
have the time to finish it properly.
Currently my idea is to create an fsg pointer in common structure which
will be updated when configuration is changed (in fsg_set_alt() function)
and then bulk_in and bulk_out would be accessed via this pointer
(ie. common->fsg->bulk_{in,out}). The whole fsg_dev structure will be
reduced to a usb_function structure, pointer to fsg_common,
bulk_{in,out}_enabled bit fields and bulk_{in,out} pointers to usb_ep.
Anyway, again, thank you for the review. :) All comments are welcome
and helpful.
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michał "mina86" Nazarewicz (o o)
ooo +-<m.nazarewicz@xxxxxxxxxxx>-<mina86@xxxxxxxxxx>-ooO--(_)--Ooo--
--
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
