On Monday 30 May 2005 17:16, Edouard TISSERANT wrote: > Staphane Voltz has fixed this points: > - use usb_kill_urb > - Leaking an urb in the error path > - use SLAB_KERNEL > > Please download last version of the patch on SF. > > Regarding resource freeing in usb_acecad_disconnect() all other driver > do the same. Are they all wrong ? > > Regarding Open/Close serialization, would anyone have the "right code" > so that we could know how to do that correctly ? >
Check out my tree here: http://www.kernel.org/git/?p=linux/kernel/git/dtor/input.git;a=summary - I think I have converted all drivers. Basically you just remove the counter altogether since input core keeps track of usage and calls these functions only for the first/last user: static int usb_mouse_open(struct input_dev *dev) { struct usb_mouse *mouse = dev->private; mouse->irq->dev = mouse->usbdev; if (usb_submit_urb(mouse->irq, GFP_KERNEL)) return -EIO; return 0; } static void usb_mouse_close(struct input_dev *dev) { struct usb_mouse *mouse = dev->private; usb_kill_urb(mouse->irq); } I also have some more comments: > +static ssize_t show_inputDevice(struct device *dev, char *buf) > +{ > + struct usb_acecad *acecad = dev_get_drvdata(dev); > + struct input_dev *inputdev; > + struct input_handle *inputhandle; > + struct list_head *node, *next; > + int retval=-ENOENT; > + > + inputdev = &acecad->dev; > + list_for_each_safe(node, next, &inputdev->h_list) { > + inputhandle = to_handle(node); > + if (strncmp(inputhandle->name, "event", 5) == 0) { > + retval = snprintf(buf, PAGE_SIZE, "/dev/input/%s\n", > inputhandle->name); > + break; > + } > + } > + return retval; > +} 1. Why list_for_each_safe? The code does not delete list elements here. 2. What protects the list from changes by other thread? 3. Why individual driver picks in the input core? 4. Why is it needed at all? You can get this information from sysfs: /sys/class/input/eventX have 'device' symlink, you can use that to track back to acecad, then use /sys/class/input/eventX/dev to get device node. > +/* > + * sysfs files removal > + */ > +static void acecad_delete_files(struct device *dev) > +{ > + device_remove_file(dev, &dev_attr_size); > + device_remove_file(dev, &dev_attr_product_id); > + device_remove_file(dev, &dev_attr_vendor_id); > + device_remove_file(dev, &dev_attr_vendor); > + device_remove_file(dev, &dev_attr_product); > + device_remove_file(dev, &dev_attr_input_path); > +} > Would be nice if attribute_group was used to correctly handle failures when creating attributes. > + /* Make sure the evdev module is loaded */ > + if (request_module("evdev") != 0) > + printk(KERN_INFO "acecad: error loading 'evdev' module"); > Don't do that from kernel. If it is needed hotplug scripts will take care of that. I know aiptek does the same and I think it should be removed from there too. > + if (dev->descriptor.iProduct && > + usb_string(dev, dev->descriptor.iProduct, buf, 63) > 0) > + { > Formatting - brace should go one the same line with "if". > + switch(id->driver_info) > + { Same. > +static ssize_t show_tabletVendorId(struct device *dev, char *buf) > +{ > + struct usb_acecad *acecad = dev_get_drvdata(dev); > + > + if (acecad == NULL) > + return 0; > + > + return snprintf(buf, PAGE_SIZE, "0x%04x\n", acecad->dev.id.vendor); > +} Do we need this attribute (and ProductId one)? You set this data in input_dev structure and we better make it available from there for all drivers. -- Dmitry ------------------------------------------------------- This SF.Net email is sponsored by Yahoo. Introducing Yahoo! Search Developer Network - Create apps using Yahoo! Search APIs Find out how you can build Yahoo! directly into your own Applications - visit http://developer.yahoo.net/?fr=offad-ysdn-ostg-q22005 _______________________________________________ linux-usb-devel@lists.sourceforge.net To unsubscribe, use the last form field at: https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
