All of lore.kernel.org
 help / color / mirror / Atom feed
From: Dan Carpenter <dan.carpenter@oracle.com>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Martin Kaiser <martin@kaiser.cx>,
	Larry Finger <Larry.Finger@lwfinger.net>,
	linux-staging@lists.linux.dev, kernel-janitors@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/6] staging: rtl8188eu: fix usb_submit_urb error handling
Date: Tue, 15 Jun 2021 09:47:19 +0300	[thread overview]
Message-ID: <20210615064719.GA2120@kadam> (raw)
In-Reply-To: <YMdr0alJDEGfsqOA@kroah.com>

On Mon, Jun 14, 2021 at 04:46:41PM +0200, Greg Kroah-Hartman wrote:
> On Sat, Jun 12, 2021 at 08:00:15PM +0200, Martin Kaiser wrote:
> > -EPERM should be handled like any other error.
> 
> Why?  This is not "any other error" for the usb core, right?
> 

Yeah.  It's a fair point that this commit message doesn't say why to do
it or explain the implications.

> > 
> > Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> > ---
> >  drivers/staging/rtl8188eu/os_dep/usb_ops_linux.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/staging/rtl8188eu/os_dep/usb_ops_linux.c b/drivers/staging/rtl8188eu/os_dep/usb_ops_linux.c
> > index ec07b2017fb7..0ceb05f3884f 100644
> > --- a/drivers/staging/rtl8188eu/os_dep/usb_ops_linux.c
> > +++ b/drivers/staging/rtl8188eu/os_dep/usb_ops_linux.c
> > @@ -366,7 +366,6 @@ u32 usb_read_port(struct adapter *adapter, u32 addr, struct recv_buf *precvbuf)
> >  	struct usb_device *pusbd = pdvobj->pusbdev;
> >  	int err;
> >  	unsigned int pipe;
> > -	u32 ret = _SUCCESS;
> >  
> >  	if (adapter->bDriverStopped || adapter->bSurpriseRemoved ||
> >  	    adapter->pwrctrlpriv.pnp_bstop_trx) {
> > @@ -403,10 +402,10 @@ u32 usb_read_port(struct adapter *adapter, u32 addr, struct recv_buf *precvbuf)
> >  			  precvbuf);/* context is precvbuf */
> >  
> >  	err = usb_submit_urb(purb, GFP_ATOMIC);
> > -	if ((err) && (err != (-EPERM)))
> > -		ret = _FAIL;
> 
> if -EPERM returns from this function, someone set the "reject" bit on
> the urb.
> 
> Can this driver do that?  Where did this check originally come from, as
> it feels like this was added for a good reason.
> 

Yeah.  It can cancel urbs in rtw_hal_inirp_deinit().  That function used
to have a better name, "usb_read_port_cancel" and in retrospect the
original name was probably better.

I think the reason for that -EPERM was treated differently was because
originally there were some error messages printed if usb_submit_urb()
failed.  (They were't actually printed because this code is buggy).  The
authors probably didn't want to print the error messages but
accidentally made it return success as well.

There is only one caller that checks the return and it only affects the
behavior if we race against open.  Can that even happen?  I'm pretty
sure that returning a failure is the correct behavior but I'm going to
leave it to Martin to check for absolutely sure.  :P

regards,
dan carpenter


  reply	other threads:[~2021-06-15  6:47 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-12 18:00 [PATCH 1/6] staging: rtl8188eu: remove unused hal_data_8188e members Martin Kaiser
2021-06-12 18:00 ` [PATCH 2/6] staging: rtl8188eu: fix usb_submit_urb error handling Martin Kaiser
2021-06-14 14:46   ` Greg Kroah-Hartman
2021-06-15  6:47     ` Dan Carpenter [this message]
2021-06-12 18:00 ` [PATCH 3/6] staging: rtl8188eu: remove a write-only struct member Martin Kaiser
2021-06-12 18:00 ` [PATCH 4/6] staging: rtl8188eu: remove a write-only power-index members Martin Kaiser
2021-06-12 18:00 ` [PATCH 5/6] staging: rtl8188eu: remove another write-only member Martin Kaiser
2021-06-12 18:00 ` [PATCH 6/6] staging: rtl8188eu: remove RT_TRACE and DBG_88E prints from usb_intf.c Martin Kaiser
2021-06-14 11:34   ` Dan Carpenter
2021-06-14 14:58     ` Martin Kaiser
2021-06-20 17:40 ` [PATCH v2] staging: rtl8188eu: fix usb_submit_urb error handling Martin Kaiser

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210615064719.GA2120@kadam \
    --to=dan.carpenter@oracle.com \
    --cc=Larry.Finger@lwfinger.net \
    --cc=gregkh@linuxfoundation.org \
    --cc=kernel-janitors@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-staging@lists.linux.dev \
    --cc=martin@kaiser.cx \
    --subject='Re: [PATCH 2/6] staging: rtl8188eu: fix usb_submit_urb error handling' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.