[PATCH] usb: core: Call disconnect() only if it is provided by driver

[PATCH] usb: core: Call disconnect() only if it is provided by driver

[Dmytro Bagrii]

A driver may use devres allocations. Disconnect handler is not needed in
this case. Allow such driver to leave .disconnect field uninitialized in
struct usb_driver instead of providing empty stub function.

Signed-off-by: Dmytro Bagrii <dimich.dmb@gmail.com>
---
 drivers/usb/core/driver.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index 355ed33a2179..d7fe440b033c 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -455,7 +455,8 @@ static int usb_unbind_interface(struct device *dev)
 	if (!driver->soft_unbind || udev->state == USB_STATE_NOTATTACHED)
 		usb_disable_interface(udev, intf, false);
 
-	driver->disconnect(intf);
+	if (driver->disconnect)
+		driver->disconnect(intf);
 
 	/* Free streams */
 	for (i = 0, j = 0; i < intf->cur_altsetting->desc.bNumEndpoints; i++) {
-- 
2.36.1

Re: [PATCH] usb: core: Call disconnect() only if it is provided by driver

[Greg KH]

On Thu, May 19, 2022 at 04:29:00PM +0300, Dmytro Bagrii wrote:
> A driver may use devres allocations. Disconnect handler is not needed in
> this case. Allow such driver to leave .disconnect field uninitialized in
> struct usb_driver instead of providing empty stub function.
> 
> Signed-off-by: Dmytro Bagrii <dimich.dmb@gmail.com>
> ---
>  drivers/usb/core/driver.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index 355ed33a2179..d7fe440b033c 100644
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -455,7 +455,8 @@ static int usb_unbind_interface(struct device *dev)
>  	if (!driver->soft_unbind || udev->state == USB_STATE_NOTATTACHED)
>  		usb_disable_interface(udev, intf, false);
>  
> -	driver->disconnect(intf);
> +	if (driver->disconnect)
> +		driver->disconnect(intf);
>  
>  	/* Free streams */
>  	for (i = 0, j = 0; i < intf->cur_altsetting->desc.bNumEndpoints; i++) {
> -- 
> 2.36.1
> 

What in-kernel driver has this issue and does not have a disconnect
callback?

thanks,

greg k-h

Re: [PATCH] usb: core: Call disconnect() only if it is provided by driver

[Alan Stern]

On Thu, May 19, 2022 at 04:29:00PM +0300, Dmytro Bagrii wrote:
> A driver may use devres allocations. Disconnect handler is not needed in
> this case. Allow such driver to leave .disconnect field uninitialized in
> struct usb_driver instead of providing empty stub function.
> 
> Signed-off-by: Dmytro Bagrii <dimich.dmb@gmail.com>
> ---
>  drivers/usb/core/driver.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index 355ed33a2179..d7fe440b033c 100644
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -455,7 +455,8 @@ static int usb_unbind_interface(struct device *dev)
>  	if (!driver->soft_unbind || udev->state == USB_STATE_NOTATTACHED)
>  		usb_disable_interface(udev, intf, false);
>  
> -	driver->disconnect(intf);
> +	if (driver->disconnect)
> +		driver->disconnect(intf);
>  
>  	/* Free streams */
>  	for (i = 0, j = 0; i < intf->cur_altsetting->desc.bNumEndpoints; i++) {

I'm very dubious about this change.  Disconnect routines generally do 
more than just deallocation.

Can you point to any drivers that would actually benefit from this?

Alan Stern

Re: [PATCH] usb: core: Call disconnect() only if it is provided by driver

[Oliver Neukum]

On 19.05.22 15:29, Dmytro Bagrii wrote:
> A driver may use devres allocations. Disconnect handler is not needed in
> this case. Allow such driver to leave .disconnect field uninitialized in
> struct usb_driver instead of providing empty stub function.
It is needed. disconnect() means that you have to give up
ownership of the interface and must cease IO. That cannot
be done with devres.
I am pretty sure that a driver without disconnect() is buggy.
disconnect() is the one method that is really not optional.

    Regards
        Oliver

Re: [PATCH] usb: core: Call disconnect() only if it is provided by driver

[Dmytro Bagrii]

On 19.05.22 16:45, Greg KH wrote:
> On Thu, May 19, 2022 at 04:29:00PM +0300, Dmytro Bagrii wrote:
>> A driver may use devres allocations. Disconnect handler is not needed in
>> this case. Allow such driver to leave .disconnect field uninitialized in
>> struct usb_driver instead of providing empty stub function.
>>
>> Signed-off-by: Dmytro Bagrii <dimich.dmb@gmail.com>
>> ---
>>  drivers/usb/core/driver.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
>> index 355ed33a2179..d7fe440b033c 100644
>> --- a/drivers/usb/core/driver.c
>> +++ b/drivers/usb/core/driver.c
>> @@ -455,7 +455,8 @@ static int usb_unbind_interface(struct device *dev)
>>  	if (!driver->soft_unbind || udev->state == USB_STATE_NOTATTACHED)
>>  		usb_disable_interface(udev, intf, false);
>>  
>> -	driver->disconnect(intf);
>> +	if (driver->disconnect)
>> +		driver->disconnect(intf);
>>  
>>  	/* Free streams */
>>  	for (i = 0, j = 0; i < intf->cur_altsetting->desc.bNumEndpoints; i++) {
>> -- 
>> 2.36.1
>>
> 
> What in-kernel driver has this issue and does not have a disconnect
> callback?

I don't see such in-kernel USB drivers yet. I develop an out-of-tree driver
and use devres for all initialization including controllers registration
etc. For actions which aren't available with devm_*, e.g. URB allocation i
use devm_add_action_or_reset() for deinit. I realized my _disconnect()
function is empty, nothing to do there. All deinitialization is done during
usb interface release. If i leave .disconnect field uninitialized in struct
usb_driver, obviously i get NULL pointer dereference bug. Thus i decided
that allowing a driver not to provide disconnect() is a good idea, as it is
done for remove() in SPI and I2C subsystems.

-- 
Best Regards,
Dmytro Bagrii.

Re: [PATCH] usb: core: Call disconnect() only if it is provided by driver

[Greg KH]

On Thu, May 19, 2022 at 06:27:17PM +0300, Dmytro Bagrii wrote:
> On 19.05.22 16:45, Greg KH wrote:
> > On Thu, May 19, 2022 at 04:29:00PM +0300, Dmytro Bagrii wrote:
> >> A driver may use devres allocations. Disconnect handler is not needed in
> >> this case. Allow such driver to leave .disconnect field uninitialized in
> >> struct usb_driver instead of providing empty stub function.
> >>
> >> Signed-off-by: Dmytro Bagrii <dimich.dmb@gmail.com>
> >> ---
> >>  drivers/usb/core/driver.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> >> index 355ed33a2179..d7fe440b033c 100644
> >> --- a/drivers/usb/core/driver.c
> >> +++ b/drivers/usb/core/driver.c
> >> @@ -455,7 +455,8 @@ static int usb_unbind_interface(struct device *dev)
> >>  	if (!driver->soft_unbind || udev->state == USB_STATE_NOTATTACHED)
> >>  		usb_disable_interface(udev, intf, false);
> >>  
> >> -	driver->disconnect(intf);
> >> +	if (driver->disconnect)
> >> +		driver->disconnect(intf);
> >>  
> >>  	/* Free streams */
> >>  	for (i = 0, j = 0; i < intf->cur_altsetting->desc.bNumEndpoints; i++) {
> >> -- 
> >> 2.36.1
> >>
> > 
> > What in-kernel driver has this issue and does not have a disconnect
> > callback?
> 
> I don't see such in-kernel USB drivers yet.

Great, then all is well.  We can not make kernel changes for out-of-tree
drivers for obvious reasons.

When you submit your driver, we will be glad to consider this change.
But as others changed, odds are your driver is incorrect and should have
a disconnect call.  Unless it is a very simple driver that could be done
instead in userspace with usbfs/libusb?

thanks,

greg k-h

Re: [PATCH] usb: core: Call disconnect() only if it is provided by driver

[Dmytro Bagrii]

On 19.05.22 19:38, Greg KH wrote:
> On Thu, May 19, 2022 at 06:27:17PM +0300, Dmytro Bagrii wrote:
>> On 19.05.22 16:45, Greg KH wrote:
>>> On Thu, May 19, 2022 at 04:29:00PM +0300, Dmytro Bagrii wrote:
>>>> A driver may use devres allocations. Disconnect handler is not needed in
>>>> this case. Allow such driver to leave .disconnect field uninitialized in
>>>> struct usb_driver instead of providing empty stub function.
>>>>
>>>> Signed-off-by: Dmytro Bagrii <dimich.dmb@gmail.com>
>>>> ---
>>>>  drivers/usb/core/driver.c | 3 ++-
>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
>>>> index 355ed33a2179..d7fe440b033c 100644
>>>> --- a/drivers/usb/core/driver.c
>>>> +++ b/drivers/usb/core/driver.c
>>>> @@ -455,7 +455,8 @@ static int usb_unbind_interface(struct device *dev)
>>>>  	if (!driver->soft_unbind || udev->state == USB_STATE_NOTATTACHED)
>>>>  		usb_disable_interface(udev, intf, false);
>>>>  
>>>> -	driver->disconnect(intf);
>>>> +	if (driver->disconnect)
>>>> +		driver->disconnect(intf);
>>>>  
>>>>  	/* Free streams */
>>>>  	for (i = 0, j = 0; i < intf->cur_altsetting->desc.bNumEndpoints; i++) {
>>>> -- 
>>>> 2.36.1
>>>>
>>>
>>> What in-kernel driver has this issue and does not have a disconnect
>>> callback?
>>
>> I don't see such in-kernel USB drivers yet.
> 
> Great, then all is well.  We can not make kernel changes for out-of-tree
> drivers for obvious reasons.
> 
> When you submit your driver, we will be glad to consider this change.
> But as others changed, odds are your driver is incorrect and should have
> a disconnect call.  Unless it is a very simple driver that could be done
> instead in userspace with usbfs/libusb?

Ok, i agree, my propoposed change is premature.
Of course, i'm checking the driver for memory, refcounts and other
resources leakage during development and not going to publish it until make
sure it works correctly.
There are some limitations with libusb in my case, e.g. it is unable to
bind existing in-tree drivers to a bus provided by hardware over USB.
Thank you for explanation.

-- 
Best Regards,
Dmytro Bagrii.