On Mon, 12 Dec 2011, Sebastian Andrzej Siewior wrote:
> Here I try to shrink my dummy_hcd queue. Patch 1-3 were posted here
> earlier and I fixed up all comments that were raised so far.
> Patch 3 also works arounds the fact that UAS (host side) does not work on
> hcd which do not support sg. This is not on top of my todo list but it on
> it :)
> Patch 5+ are new.
On the whole these look okay, although I haven't reviewed patch 7
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.
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.
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
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.
Nevertheless, stylistic concerns aside, for patches 1-4 and 6:
Acked-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
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
