On Friday 20 November 2009 19:43:46 Oliver Neukum wrote:
> 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?
Oh yes, that might be a problem - I'll add a NULL check before kstrdup. And
maybe it should not be hardcoded to 3 reads - had to check this.
> > > > + 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.
As there's no datasheet and I have only one device (with one firmware
version), ignoring it looks like the best "solution".
I'll also want to remove NEXIO_INPUT_EP and NEXIO_OUTPUT_EP constants - the
endpoint addresses can be found at runtine (there's only one input and one
output endpoint). I think that to do this in nexio_init(), it needs to
know "struct usb_interface *" instead of "struct usb_device *". I have a
patch ready (but forgot to take it so it needs to wait until next week) that
replaces "struct usb_device *udev" in struct usbtouch_usb with "struct
usb_interface *interface" - is it a good idea?
>
> > 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-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Ondrej Zary
--
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
