Am Donnerstag, 16. Juni 2005 20:27 schrieb David Kubicek:
> Hello,
>
> thank you for reviewing my changes to this driver! I was hoping for
> somebody with better USB/kernel programming experience to do that. I'm
> a complete newbie in kernel programming - this update took four days to
> complete (including studying of kernel/USB internals:) and I sure didn't
> expect it to make it into official kernel. It's just that many ppl in
> Czech Republic needed this thing for CDMA wireless broadband internet
> access and when I purchased the thing, I've decided to do it myself for
> us all to have full speed - many ppl switched to windows for that... :-/
>
> On Thu, Jun 16, 2005 at 04:41:25PM +0200, Oliver Neukum wrote:
> > Am Donnerstag, 16. Juni 2005 13:01 schrieb Petr Pisar:
> > > David Kubicek has rewritten cdc-acm module for 2.6 kernel. It is still
> > > in testing but it seems stable. Patch can be downloaded from
> > > [http://dave.ok.cz/cdc-acm_release/].
> >
> > extremely cool and definitely 2.6.13 material
> >
> > a few remarks:
> >
> > - if (urb->status)
> > - dev_dbg(&acm->data->dev, "bulk rx status %d\n", urb->status);
> >
> > You can't ignore errors.
>
> Ok, can anyone suggest what to do? Original driver just logged the
At least test for the terminating trio ECONNRESET, ENOENT and ESHUTDOWN.
The handler should terminate and not schedule a tasklet if any of those
happens.
> failure and proceeded. In my patch, bad status is "considered" 0+ len
> buffer and the work of the driver continues. (urb->status) usually
> happens when a transfer is in progress and USB cable is plugged out.
> Does it really implicate actual_length==0 or not? If not, are the
> data in the buffer correct or bogus/to-be-ignored?
actual_length should be believed.
> If len==0 or data in buffer are valid, no special handling should be
> required. If you mean that the fact should be logged as in original,
> no problem...
Anything unhandled should be logged, yes.
>
>
> > + if (!rcv || urb <= 0) {
> >
> > What is this supposed to do?
>
> (!rcv) means incorrect URB (no .context set up, which shouldn't happen as
> all URBs are initialized with context=acm driver data)...
If it cannot happen make it BUG_ON()
> and (urb<=0) is forgotten piece from earliers stages of patch devel. :)
> Perhaps it could be replaced with that urb->status check? Because if
> true, URB and the related buffer are marked as free to use and routine
> ends...
Checking a pointer for < 0 is too ugly to let live.
> > + if ((err = usb_submit_urb(rcv->urb, GFP_ATOMIC)) < 0) {
> >
> > Can this kill the driver by ENOMEM?
>
> Don't know - this is the same handling as in original driver. I added
> marking URB&buff free for use (original just called dev_dbg)...
>
> Anyway, as the update is programmed, if the submit would for any reason
> fail (even consequently) transfers would be suspended just until the
> reason disappeared (enough memory again). Original driver would stop
> working (no URB in the queue, no acm_read_bulk called, no tasklet
> scheduled...)
Yes, I see.
> > + for (i = 0; i < ACM_MAX_READ_BUFS; i++) {
> > + struct acm_read_buffer *buf = &(acm->read_bufs[i]);
> > +
> > + if (!(buf->base = kmalloc(readsize, GFP_KERNEL))) {
> > + dev_dbg(&intf->dev, "out of memory (read_bufs
> > kmalloc)\n");
> > + goto alloc_fail7;
> > + }
> > }
> >
> > This should alloc usb buffers, not use kmalloc.
>
> I used the same allocation as is in some other drivers I looked into for
> inspiration. :) The other way I know of is usb_buffer_alloc. If you
> think it should be changed to alloc buffers using that, I can do it.
We are looking at potentially 2MBit/s. It would be a good idea.
Not strictly necessary, but a good idea.
Please send me a patch for 2.6.13.
Regards
Oliver
-------------------------------------------------------
SF.Net email is sponsored by: Discover Easy Linux Migration Strategies
from IBM. Find simple to follow Roadmaps, straightforward articles,
informative Webcasts and more! Get everything you need to get up to
speed, fast. http://ads.osdn.com/?ad_id=7477&alloc_id=16492&op=click
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
