* Alan Stern | 2011-12-13 11:08:57 [-0500]:
>carefully. I don't like patch 5 at all, however. Your description
>says:
>
>> This is a shortcut and not essential for "working". This "shortcut"
>> requires for the gadget driver to drop its locks which are taken in the
>> complete callback.
>> I'm mainly for the removal because it needs fixing for the upcomming
>> stream implementation. Since there is no stream_id matching it would
>> need an additional exception (I don't think additional
>> stream_number * FIFO_SIZE buffer is a good idea). Once we add sg support
>> for the gadget side it would need additional fixup here.
>> I don't see the benefit of this "early" completion so I'm removing it.
>
>The benefit is that it provides better testing. That's what dummy-hcd
>was written for -- testing. In particular, this "early completion"
>path exists in the net2280 driver and hardware, so keeping it in
>dummy-hcd is worthwhile. If you like, you can restrict it to work only
>when the endpoint doesn't have any streams allocated.
Okay. So we do this in dummy because it is a code path in a driver. Fair
enough. Looking at net2280 I see one early complete call for ep0 if
length == 0 and for OUT ep where the fifo is read during enqueue in case
it is not empty. Nothing for IN.
So if we keep it, this raises two questions:
- This looks like every gadget has drop its locks while submitting
requests. From a quick grep I see that this is done. So I learned
something here.
- Isn't it premature to call complete for an IN endpoint before we know
it went through? For OUT I would understand, the data may already sit
in the FIFO but the IN transfer did not complete yet.
>One more thing: Why do you overuse u32 so much? For example, in patch
>3/7:
>
>> @@ -147,6 +148,8 @@ static const char *const ep_name [] = {
>> struct urbp {
>> struct urb *urb;
>> struct list_head urbp_list;
>> + struct sg_mapping_iter miter;
>> + u32 miter_started;
>> };
>
>There's no reason for miter_started to be a u32. In general, types
>that specify a particular size should be used only in situations where
>the size really matters -- which basically means whenever the value is
>embedded in a structure and a particular alignment is needed, or when
>the value is defined according to an external protocol.
So here I could use u32 miter_stared:1 to use only one bit. The compiler
tends to pad the structure to the next naturally aligned spot so adding
u8 here would hardly makes a difference. If it is followed by a pointer
for instance it will be padded by 3 or 7 bytes. So this here eats up the
same amount of memory.
Once a second variable of this kind is required this could change to the
bitfield (:1 thingy) notation or rename to flags and add a few defines.
I usually prefer the latter because I don't like bitfields like that and
they often produce worse code especially if you need to set both of them
to the same value at once.
>When neither of these holds, the best approach is to use a generic type
>(like int or unsigned) and allow the compiler the freedom to do what it
int is defined to be signed int on all architectures if it is not
specified. We don't have this for the char type for instance. This one is
different on powerpc vs x86. I like to be explicit here and I am lazy
as well so u32 is a good compromise :) Since negative values are not
required I pick the unsigned instead of the signed int. One could ask
himself why miter_started could be negative. Only "unsigned" looks
incomplete. I don't know how it is defined but I think it internally
handled as int. Atleast it uses 4 bytes which speaks for int (u32). So
if it is u32 why not state it clearly?
>wants. In this case, miter_started doesn't even need to be unsigned;
>since it merely stores a 0/1 value, it could be declared as bool.
>From the obvious point of view the goal goes clearly to bool. I have no
problem to change it to this type if you prefer.
>Nevertheless, stylistic concerns aside, for patches 1-4 and 6:
>
>Acked-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
Thank you.
>
>Alan Stern
Sebastian
--
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
