Am Freitag, 20. November 2009 10:21:43 schrieb Ondrej Zary:
> On Thursday 19 November 2009, Oliver Neukum wrote:
> > > +struct nexio_priv {
> > > + struct urb *ack;
> > > + char ack_buf[2];
> > > +};
> >
> > No. Every buffer needs to have an exclusive cache line for DMA
> > to work on the incoherent archotectures. Therefore you must allocate
> > each buffer with its own kmalloc.
>
> OK, thanks for your patience.
No problem. I should have explained better.
> > > + /* read replies */
> > > + for (i = 0; i < 3; i++) {
> > > + memset(buf, 0, NEXIO_BUFSIZE);
> > > + ret = usb_bulk_msg(dev, usb_rcvbulkpipe(dev, NEXIO_INPUT_EP),
> > > + buf, NEXIO_BUFSIZE, &actual_len,
> > > + NEXIO_TIMEOUT);
> > > + if (ret < 0 || actual_len < 1 || buf[1] != actual_len)
> > > + continue;
> > > + switch (buf[0]) {
> > > + case 0x83: /* firmware version */
> > > + firmware_ver = kstrdup(&buf[2], GFP_KERNEL);
On second thought this is not nice. If a device is broken enough to report
a name or a firmware version twice, you produce a memory leak.
Do you know very buggy devices to exist?
> > > + break;
> > > + case 0x84: /* device name */
> > > + device_name = kstrdup(&buf[2], GFP_KERNEL);
> > > + break;
> > > + }
> > > + }
> > > + printk(KERN_INFO "Nexio device: %s, firmware version: %s\n",
> > > + device_name, firmware_ver);
> >
> > How do you know device_name and firmware_ver are not NULL?
>
> printk works fine with NULL, it prints <NULL>. Is it necessary to add more
> code only to make the output nice?
No, for niceness it is not necessary. The question is whether you want
to treat this as an error or print a warning. That is a matter of taste.
> Looks like a bug in the original usbtouchscreen code. There's no locking.
> Will a spinlock in usbtouch_open() and usbtouch_disconnect() fix it? Or
> do you see more problems here?
You must not call usb_kill_urb() with a spinlock held.
I'll lokk at the usbtouchscreen code.
The new version looks good to me.
Regards
Oliver
--
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
