Hello Oliver,
I know I can count on you ;-) Thank you for reviewing.
On Sunday 12 January 2003 16:47, Oliver Neukum wrote:
> void auerbuf_free(struct auerbuf *bp)
> - simply do kfree. It's officially allowed to pass NULL. There's no need
> to burn cycles
OK. I have changed all occurances. I didn't know about that.
Are you shure? Kfree() lacks some documentation in "Writing device drivers",
and in fact, there are some examples in this book which do the null-check
before kfree().
> static void auerchain_complete(struct urb *urb)
> - Wow. I never saw a recursive interrupt handler. The logic is beyond me.
> - Simply use spin_lock(). You already are in interrupt. It even saves stack
> space.
Uh no. auerchain_complete() may be called from user space ... see
auerchain_submit_urb_list() below. The sense of the recursive call is simple:
I want to concentrate error handling at one place, and I want my chained
submit() to mimic the original behaviour of usb_submit_urb(). So obviously
auerchain_submit_urb_list() can not be the same as usb_submit_urb(), because
the return codes from usb_submit_urb() can not go back into time to the
auerchain_submit_urb_list() call.
So I have decided to do all error handling at a central place: in the
completion handler. Its easy and safe.
(In fact, it might be a good idea to do all error handling in the completion
handler. Now, you have two points in time for error handling: the return
value of usb_submit_urb(), and the completion handler. So you have to
consider which errror is reported on which place. Weak.)
The whole auerchain() calls will vanish in 2.5.x, if it is allowed to submit
more than one control message at a time.
> int auerchain_submit_urb_list(struct auerchain *acp, struct urb *urb,
> + urb->status = -EINPROGRESS; /* usb_submit_urb does this, too */
> Yes, it does. And no driver has any business poking there. This is too ugly
> to let live.
I recall that it was neccessary to mimic the behaviour of usb_submit_urb().
I don't view auerchain as a "driver" module. It is simply adding the missing
chaining of urbs to usb_submit_urb(). As such, this code *has* to do the same
things as usb_submit_urb(). This will make it easy to replace by
usb_submit_urb() in 2.5.x.
> int auerchain_unlink_urb(struct auerchain *acp, struct urb *urb)
> + spin_unlock_irqrestore(&acp->lock, flags);
> + dbg("unlink waiting urb");
> + urb->status = -ENOENT;
> + urb->complete(urb);
> Please no. Don't do this.
>
> I am afraid there are severe layering violations in this code.
Can you explain? What do you mean? What should I do instead?
best regards
Wolfgang Mües
-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
