[02/12] usb: usbtmc: Support Read Status Byte with SRQ per file handle

[Guido Kiener <guido@kiener-muenchen.de>]

Zitat von Greg KH <gregkh@linuxfoundation.org>:

>> @@ -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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html