Re: [PATCH v3] usb: dwc3: gadget: Move null pinter check after window closed

[Greg KH]

On Fri, May 13, 2022 at 02:57:09PM +0800, Albert Wang wrote:
> After inspecting further, we do see the locking is implicit, with the
> main gotcha being the unlock/re-lock.

This sentance makes no sense at all.

Who is "we"?  What is the gotcha?  What is the subject of the sentance?
What is going on?

> That creates a window for a race to happen.

What is "that"?

> This change moves the NULL check to be adjacent to where
> to it's used and after the window is "closed".

What is "this"?

Please read Documentation/process/submitting-patches.rst for how to
properly write a changelog text so that others can understand what is
going on.

thanks,

greg k-h


> 
> Fixes: 26288448120b ("usb: dwc3: gadget: Fix null pointer exception")
> Signed-off-by: Albert Wang <albertccwang@xxxxxxxxxx>
> ---
>  v3: Add change log to be compliant with the canonical patch format
>  v2: Remove redundant 'else' and add additional comments and more
>      descriptive commit text
> 
>  drivers/usb/dwc3/gadget.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 19477f4bbf54..fda58951cf27 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -3366,14 +3366,19 @@ static bool dwc3_gadget_endpoint_trbs_complete(struct dwc3_ep *dep,
>  	struct dwc3		*dwc = dep->dwc;
>  	bool			no_started_trb = true;
>  
> -	if (!dep->endpoint.desc)
> -		return no_started_trb;
> -
> +	/*
> +	 * This function eventually leads to dwc3_giveback() which unlocks
> +	 * the dwc->lock and relocks afterwards. This actually creates a
> +	 * a window for a race to happen.

What race?  Why mention it here?  Why not fix it instead of documenting
it?

this comment does not make sense, sorry.

thanks,

greg k-h