On Wed, 14 Dec 2011, Sebastian Andrzej Siewior wrote:
> * 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.
Maybe I'm remembering wrong. It has been many years since I looked at
this stuff. Perhaps this early-completion path is unique to dummy-hcd.
I still think it makes a good test case. Gadget drivers shouldn't
assume that request completions will always occur _after_ the
submission call has returned.
> 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.
Right. Or at least, they have to drop any locks that might be taken by
a completion handler.
> - 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.
This is an obscure technical point. I asked Dave Brownell about it
once. He said that the completion callback doesn't necessarily mean
that the host has received the data; it only means that the UDC driver
has taken over responsibility for the data, so that the usb_request
structure and data buffer are now available for the gadget driver to
reuse.
> >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.
I wasn't asking why you didn't use a bitfield; I was asking why you
didn't use unsigned or bool. Why go to the trouble of specifying a
32-bit quantity when the actual length doesn't matter?
> >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?
Because it confuses people. It implies strongly that there's a good
reason why the variable has to hold a 32-bit value.
> >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.
It's a small matter; I don't much care.
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
