Zitat von Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>:
@@ -122,7 +141,7 @@ static int usbtmc_open(struct inode *inode,
struct file *filp)
{
struct usb_interface *intf;
struct usbtmc_device_data *data;
- int retval = 0;
+ struct usbtmc_file_data *file_data;
intf = usb_find_interface(&usbtmc_driver, iminor(inode));
if (!intf) {
@@ -130,21 +149,54 @@ static int usbtmc_open(struct inode *inode,
struct file *filp)
return -ENODEV;
}
+ file_data = kzalloc(sizeof(*file_data), GFP_KERNEL);
+ if (!file_data)
+ return -ENOMEM;
+
+ pr_debug("%s - called\n", __func__);
Please do not add "tracing" functions like this. The kernel has a
wonderful built-in function tracing functionality, don't try to write
your own. These lines should just be removed.
+
data = usb_get_intfdata(intf);
/* Protect reference to data from file structure until release */
kref_get(&data->kref);
+ mutex_lock(&data->io_mutex);
+ file_data->data = data;
+
+ /* copy default values from device settings */
+ file_data->TermChar = data->TermChar;
+ file_data->TermCharEnabled = data->TermCharEnabled;
+ file_data->auto_abort = data->auto_abort;
+
+ INIT_LIST_HEAD(&file_data->file_elem);
+ spin_lock_irq(&data->dev_lock);
+ list_add_tail(&file_data->file_elem, &data->file_list);
+ spin_unlock_irq(&data->dev_lock);
+ mutex_unlock(&data->io_mutex);
+
/* Store pointer in file structure's private data field */
- filp->private_data = data;
+ filp->private_data = file_data;
- return retval;
+ return 0;
}
static int usbtmc_release(struct inode *inode, struct file *file)
{
- struct usbtmc_device_data *data = file->private_data;
+ struct usbtmc_file_data *file_data = file->private_data;
- kref_put(&data->kref, usbtmc_delete);
+ pr_debug("%s - called\n", __func__);
Again, these all need to be dropped.
+
+ /* prevent IO _AND_ usbtmc_interrupt */
+ mutex_lock(&file_data->data->io_mutex);
+ spin_lock_irq(&file_data->data->dev_lock);
+
+ list_del(&file_data->file_elem);
+
+ spin_unlock_irq(&file_data->data->dev_lock);
+ mutex_unlock(&file_data->data->io_mutex);
You protect the list, but what about removing the data itself?
+
+ kref_put(&file_data->data->kref, usbtmc_delete);
What protects this from being called at the same time a kref_get is
being called?
Yeah, it previously probably already had this race, sorry I never
noticed that.
I looked for a race here, but I do not find a race between open and release,
since a refcount of "file_data->data->kref" is always hold by
usbtmc_probe/disconnect.
However I see a race between usbtmc_open and usbtmc_disconnect. Are these
callback functions called mutual exclusive?
I'm not sure, but if not, then I think we have the same problem in
usb-skeleton.c
+ file_data->data = NULL;
+ kfree(file_data);
return 0;
}
thanks,
greg k-h
--
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
