From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754978AbZKTSnk (ORCPT ); Fri, 20 Nov 2009 13:43:40 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1753392AbZKTSnk (ORCPT ); Fri, 20 Nov 2009 13:43:40 -0500 Received: from smtp-out002.kontent.com ([81.88.40.216]:53633 "EHLO smtp-out002.kontent.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753760AbZKTSnj (ORCPT ); Fri, 20 Nov 2009 13:43:39 -0500 From: Oliver Neukum To: Ondrej Zary Subject: Re: [PATCH v4] [RFC] NEXIO (or iNexio) support for usbtouchscreen Date: Fri, 20 Nov 2009 19:43:46 +0100 User-Agent: KMail/1.12.2 (Linux/2.6.32-rc6-0.1-default; KDE/4.3.1; x86_64; ; ) Cc: linux-usb@vger.kernel.org, daniel.ritz@gmx.ch, linux-kernel@vger.kernel.org References: <200911161515.00907.linux@rainbow-software.org> <200911191756.04615.oliver@neukum.org> <200911201021.45410.linux@rainbow-software.org> In-Reply-To: <200911201021.45410.linux@rainbow-software.org> MIME-Version: 1.0 Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Message-Id: <200911201943.46696.oliver@neukum.org> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 . 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