Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Fri, 13 May 2022 10:16:38 -0700
From:   Jack Pham <quic_jackp@quicinc.com>
To:     Greg KH <gregkh@linuxfoundation.org>
CC:     Albert Wang <albertccwang@google.com>, <balbi@kernel.org>,
        <badhri@google.com>, <linux-usb@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3] usb: dwc3: gadget: Move null pinter check after
 window closed
Message-ID: <20220513171638.GC5637@jackp-linux.qualcomm.com>
References: <20220513065709.1606634-1-albertccwang@google.com>
 <Yn4E0XHlsPjaEgMw@kroah.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <Yn4E0XHlsPjaEgMw@kroah.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On Fri, May 13, 2022 at 09:12:17AM +0200, Greg KH wrote:
> 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.

Albert, it looks like you took my reply comments verbatim.  These were
in context to questions Greg asked about locking or apparent lack
thereof.  But on their own they really don't make sense as Greg poitns
out.  Could you please write up the text (in your own words) in a way
that would be clear to a person seeing this patch for the first time?

Jack

> > Fixes: 26288448120b ("usb: dwc3: gadget: Fix null pointer exception")
> > Signed-off-by: Albert Wang <albertccwang@google.com>
> > ---
> >  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