Messages in this thread | | | Date | Wed, 27 Aug 2008 14:28:22 -0400 (EDT) | From | Alan Stern <> | Subject | Re: [PATCH] USB: add USB test and measurement class driver - round 2 |
| |
On Wed, 27 Aug 2008, Greg KH wrote:
> Here's an updated version of the usbtmc driver, with all of the > different issues that have been raised, hopefully addressed.
This is an example of what I was discussing with Oliver. In all likelihood you simply don't need usbtmc_mutex, and using it will cause a lockdep violation.
That's why so many of the other USB class drivers don't have an analogous static mutex.
> +static int usbtmc_open(struct inode *inode, struct file *filp) > +{ > + struct usb_interface *intf; > + struct usbtmc_device_data *data; > + int retval = -ENODEV; > + > + mutex_lock(&usbtmc_mutex);
You must never acquire a lock in your open method if it will be held by your disconnect method while unregistering the minor.
> +static int usbtmc_probe(struct usb_interface *intf, > + const struct usb_device_id *id) > +{ ... > + retcode = usb_register_dev(intf, &usbtmc_class); > + if (retcode) { > + dev_err(&intf->dev, "Not able to get a minor" > + " (base %u, slice default): %d\n", USBTMC_MINOR_BASE, > + retcode); > + goto error_register; > + } > + dev_dbg(&intf->dev, "Using minor number %d\n", intf->minor);
You do call usb_register_dev() during probe...
> + > + return 0; > + > +error_register: > + sysfs_remove_group(&intf->dev.kobj, &capability_attr_grp); > + sysfs_remove_group(&intf->dev.kobj, &data_attr_grp); > + kref_put(&data->kref, usbtmc_delete); > + return retcode; > +} > + > +static void usbtmc_disconnect(struct usb_interface *intf) > +{ > + struct usbtmc_device_data *data; > + > + dev_dbg(&intf->dev, "usbtmc_disconnect called\n"); > + > + mutex_lock(&usbtmc_mutex); > + data = usb_get_intfdata(intf); > + sysfs_remove_group(&intf->dev.kobj, &capability_attr_grp); > + sysfs_remove_group(&intf->dev.kobj, &data_attr_grp); > + kref_put(&data->kref, usbtmc_delete); > + mutex_unlock(&usbtmc_mutex); > +}
But you don't call usb_unregister_dev() during disconnect. An oversight?
Alan Stern
| |