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 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? 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... > > + 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)... 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... > > + 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...) > > + 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. Sincerely, :) -- David Kubicek system specialist gedas CR s.r.o. TGM 840, Mlada Boleslav Czech Republic ------------------------------------------------------- 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
