Re: [RFC] usbcore: compare and release one bos descriptor in usb_reset_and_verify_device()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, 26 Aug 2013, Xenia Ragiadakou wrote:

> In usb_reset_and_verify_device(), hub_port_init() allocates a new bos
> descriptor to hold the value read by the device. The new bos descriptor
> has to be compared with the old one in order to figure out if device 's
> firmware has changed in which case the device has to be reenumerated.
> In the original code, none of the two descriptors was deallocated leading
> to memory leaks.
> 
> This patch compares the old bos descriptor with the new one to detect change
> in firmware and releases the newly allocated bos descriptor to prevent memory
> leak.

Pretty good, but a few things need to be changed.

> Signed-off-by: Xenia Ragiadakou <burzalodowa@xxxxxxxxx>
> ---
>  drivers/usb/core/hub.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 175179e..2801619 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -4939,7 +4939,8 @@ void usb_hub_cleanup(void)
>  } /* usb_hub_cleanup() */
>  
>  static int descriptors_changed(struct usb_device *udev,
> -		struct usb_device_descriptor *old_device_descriptor)
> +			struct usb_device_descriptor *old_device_descriptor,
> +			struct usb_host_bos *old_bos)

Please leave the indentation the same as it was (two extra tab stops in 
the continuation lines).

>  {
>  	int		changed = 0;
>  	unsigned	index;
> @@ -4953,6 +4954,19 @@ static int descriptors_changed(struct usb_device *udev,
>  			sizeof(*old_device_descriptor)) != 0)
>  		return 1;
>  
> +	if (!udev->wusb && le16_to_cpu(udev->descriptor.bcdUSB) >= 0x0201) {

This test shouldn't be here.

> +		if ((old_bos && !udev->bos) || (!old_bos && udev->bos))
> +			return 1;
> +		if (udev->bos) {
> +			len = udev->bos->desc->wTotalLength;
> +			if (len != old_bos->desc->wTotalLength)
> +				return 1;
> +			if (memcmp(udev->bos->desc, old_bos->desc,
> +					le16_to_cpu(len)))
> +				return 1;
> +		}
> +	}
> +
>  	/* Since the idVendor, idProduct, and bcdDevice values in the
>  	 * device descriptor haven't changed, we will assume the
>  	 * Manufacturer and Product strings haven't changed either.
> @@ -5049,6 +5063,7 @@ static int usb_reset_and_verify_device(struct usb_device *udev)
>  	struct usb_hub			*parent_hub;
>  	struct usb_hcd			*hcd = bus_to_hcd(udev->bus);
>  	struct usb_device_descriptor	descriptor = udev->descriptor;
> +	struct usb_host_bos		*bos = udev->bos;
>  	int 				i, ret = 0;
>  	int				port1 = udev->portnum;
>  
> @@ -5099,12 +5114,17 @@ static int usb_reset_and_verify_device(struct usb_device *udev)
>  		goto re_enumerate;
>   
>  	/* Device might have changed firmware (DFU or similar) */
> -	if (descriptors_changed(udev, &descriptor)) {
> +	if (descriptors_changed(udev, &descriptor, bos)) {
>  		dev_info(&udev->dev, "device firmware changed\n");
>  		udev->descriptor = descriptor;	/* for disconnect() calls */
> +		usb_release_bos_descriptor(udev);
> +		udev->bos = bos;

Don't do this here...

>  		goto re_enumerate;
>    	}
>  
> +	usb_release_bos_descriptor(udev);
> +	udev->bos = bos;
> +

and don't do it here either.  This is tricky because we need to make 
sure the right thing happens no matter what pathway gets followed.  
Your patch misses the "if (ret < 0)" path before the 
descriptors_changed() call.

I think the safest approach is to save the old bos value and set 
udev->bos to NULL at the same time, before the call to
usb_unlocked_disable_lpm().  Then do the release and assignment just 
after the "done:" and "re_enumerate:" statement labels.  That way they 
will always be executed, on every pathway.

Alan Stern

--
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




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux