Re: [PATCH] brcmfmac: unlink URB when request timed out

[Oliver Neukum <oneukum@suse.de>]

On Sun, 2014-11-09 at 13:10 -0500, Mathy Vanhoef wrote:
> From: Mathy Vanhoef <vanhoefm@gmail.com>
> 
> Unlink the submitted URB in brcmf_usb_dl_cmd if the request timed out. This
> assures the URB is never submitted twice, preventing a driver crash.

Hi,

I am afrad this patch is no good. The diagnosis is good,
but the fix introduces serious problems.

> diff --git a/drivers/net/wireless/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/brcm80211/brcmfmac/usb.c
> index 5265aa7..1bc7858 100644
> --- a/drivers/net/wireless/brcm80211/brcmfmac/usb.c
> +++ b/drivers/net/wireless/brcm80211/brcmfmac/usb.c
> @@ -738,10 +738,12 @@ static int brcmf_usb_dl_cmd(struct brcmf_usbdev_info *devinfo, u8 cmd,
>  		goto finalize;
>  	}
>  
> -	if (!brcmf_usb_ioctl_resp_wait(devinfo))
> +	if (!brcmf_usb_ioctl_resp_wait(devinfo)) {
> +		usb_unlink_urb(devinfo->ctl_urb);

This is the asynchronous unlink. You have no guarantee it is finished
after this point.

>  		ret = -ETIMEDOUT;
> -	else
> +	} else {
>  		memcpy(buffer, tmpbuf, buflen);
> +	}
>  
>  finalize:
>  	kfree(tmpbuf);

Which means that you are freeing memory that may still be used by DMA
at this time.
In addition you have no guarantee that the unlink is indeed finished
by the time the URB is reused.
If you wish to take this approach you better forget about this URB
and allocate a new one and free the buffer from the callback.

	Regards
		Oliver

-- 
Oliver Neukum <oneukum@suse.de>