Re: [PATCH] usb-hid-core: Set intfdata to NULL if probe fails

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 05/22/2012 09:56 AM, Jiri Slaby wrote:
> On 05/21/2012 09:39 PM, Hans de Goede wrote:
>> other drivers which rely on frameworks which only call dev_set_drvdata
>> on the interface's device if no drvdata has been set
>
> This looks very broken as it relies on an undocumented behavior.

I don't see how expecting intfdata to be NULL when an USB driver's
probe function gets called is broken, esp. since most
USB drivers will unconditionally set intfdata to something from
their probe functions, so it seems reasonable to assume that it is
not pointing to anything before probe gets called.

Also note that on disconnect / forced unbind, the USB core
will set intfdata to NULL. itself, enforcing it to be NULL
for unbound interfaces, except when a probe function
screws up and sets intfdata even though the probe failed.

> If they
> want to do that they should:
> * set intfdata to NULL
> * call some hook that may set intfdata
> * set intfdata to whatever they want if it is still NULL
>
> Right?

Wrong, why would they need to explictly NULL intfdata? it should
be NULL when their probe gets called. I cannot believe we are
even having this discussion, are you really trying to argue
that leaving intfdata as a dangling pointer, rather then setting
it to NULL (*) is better???

*) or moving the setting of it to a point where probe can no longer
fail.

> What are those frameworks?
v4l2-device.c does this, the call sequence is:

-some driver's probe method gets called
-that driver can either call dev_set_drvdata itself, if it has its
  own uses for it, or leave it as is (which should be NULL at this point)
-v4l2_device_register gets called on a v4l2_device struct, with a
  device argument, if that device does not have any drvdata set, it will
  set drvdata to point to the v4l2_device struct.

The purpose of this is to try and bring some standardization to what
drvdata will point to for v4l2 devices. But this whole shebang is not
really relevant.

Leaving around a dangling pointer in any circumstancces is just BAD,
very very BAD.

Regards,

Hans





>
>> Signed-off-by: Hans de Goede<hdegoede@redhat.com>
>> ---
>>   drivers/hid/usbhid/hid-core.c |    1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
>> index 5bf91db..70d760f 100644
>> --- a/drivers/hid/usbhid/hid-core.c
>> +++ b/drivers/hid/usbhid/hid-core.c
>> @@ -1296,6 +1296,7 @@ err_free:
>>   	kfree(usbhid);
>>   err:
>>   	hid_destroy_device(hid);
>> +	usb_set_intfdata(intf, NULL);
>>   	return ret;
>>   }
>>
>>