LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
Cc: Larry Finger <Larry.Finger@lwfinger.net>,
	Phillip Potter <phil@philpotter.co.uk>,
	Pavel Skripkin <paskripkin@gmail.com>,
	linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org,
	David Laight <david.Laight@aculab.com>,
	Dan Carpenter <dan.carpenter@oracle.com>
Subject: Re: [PATCH v7 17/19] staging: r8188eu: shorten calls chain of rtw_read{8,16,32}()
Date: Fri, 17 Sep 2021 16:50:10 +0200	[thread overview]
Message-ID: <YUSrIqW5WSYuoa14@kroah.com> (raw)
In-Reply-To: <20210917071837.10926-18-fmdefrancesco@gmail.com>

On Fri, Sep 17, 2021 at 09:18:35AM +0200, Fabio M. De Francesco wrote:
> Shorten the calls chain of rtw_read8/16/32() down to the actual reads.
> For this purpose unify the three usb_read8/16/32 into the new
> usb_read(); make the latter parameterizable with 'size'; embed most of
> the code of usbctrl_vendorreq() into usb_read() and use in it the new
> usb_control_msg_recv() API of USB Core.
> 
> Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Co-developed-by: Pavel Skripkin <paskripkin@gmail.com>
> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
>  drivers/staging/r8188eu/hal/usb_ops_linux.c | 59 +++++++++++++++++++--
>  1 file changed, 56 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> index 2d5e9b3ba538..ef35358cf2d3 100644
> --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
> +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> @@ -89,6 +89,59 @@ static int usbctrl_vendorreq(struct intf_hdl *intfhdl, u16 value, void *data, u1
>  	return status;
>  }
>  
> +static int usb_read(struct intf_hdl *intfhdl, u16 addr, void *data, u8 size)
> +{
> +	struct adapter *adapt = intfhdl->padapter;
> +	struct dvobj_priv *dvobjpriv = adapter_to_dvobj(adapt);
> +	struct usb_device *udev = dvobjpriv->pusbdev;
> +	int status;
> +	u8 *io_buf; /* Pointer to I/O buffer */

As you "know" size is not going to be larger than 4 (hint, you should
prboably check it), just use bytes off of the stack here, and you can
ignore this buffer entirely.  That will hopefully allow you in the
future to get rid of that buffer as odds are it will not be needed
anymore.

> +
> +	if (adapt->bSurpriseRemoved || adapt->pwrctrlpriv.pnp_bstop_trx)
> +		return -EPERM;

How is it ok to check this outside of the lock?  What happens if these
values change right _after_ you check them?

Why check them at all, is this something that we even care about?

I know you are trying to make this just the same logic at is there
today, but why not just do it right the first time?

> +
> +	mutex_lock(&dvobjpriv->usb_vendor_req_mutex);
> +
> +	io_buf = dvobjpriv->usb_vendor_req_buf;
> +
> +	status = usb_control_msg_recv(udev, 0, REALTEK_USB_VENQT_CMD_REQ,
> +				      REALTEK_USB_VENQT_READ, addr,
> +				      REALTEK_USB_VENQT_CMD_IDX, io_buf,
> +				      size, RTW_USB_CONTROL_MSG_TIMEOUT,
> +				      GFP_KERNEL);
> +
> +	if (status == -ESHUTDOWN ||
> +	    status == -ENODEV ||
> +	    status == -ENOENT) {
> +		/*
> +		 * device or controller has been disabled due to
> +		 * some problem that could not be worked around,
> +		 * device or bus doesn’t exist, endpoint does not
> +		 * exist or is not enabled.
> +		 */
> +		adapt->bSurpriseRemoved = true;
> +		goto mutex_unlock;
> +	}
> +
> +	if (status < 0) {
> +		GET_HAL_DATA(adapt)->srestpriv.wifi_error_status =
> +			USB_VEN_REQ_CMD_FAIL;
> +
> +		if (rtw_inc_and_chk_continual_urb_error(dvobjpriv))
> +			adapt->bSurpriseRemoved = true;
> +
> +		goto mutex_unlock;
> +	}
> +
> +	rtw_reset_continual_urb_error(dvobjpriv);
> +	memcpy(data, io_buf, size);
> +
> +mutex_unlock:
> +	mutex_unlock(&dvobjpriv->usb_vendor_req_mutex);
> +
> +	return status;

No one cares about this value, is that ok?

thanks,

greg k-h

  reply	other threads:[~2021-09-17 14:50 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-17  7:18 [PATCH v7 00/19] staging: r8188eu: shorten and simplify calls chains Fabio M. De Francesco
2021-09-17  7:18 ` [PATCH v7 01/19] staging: r8188eu: remove usb_{read,write}_mem() Fabio M. De Francesco
2021-09-17  7:18 ` [PATCH v7 02/19] staging: r8188eu: remove the helpers of rtw_read8() Fabio M. De Francesco
2021-09-17  7:18 ` [PATCH v7 03/19] staging: r8188eu: remove the helpers of rtw_read16() Fabio M. De Francesco
2021-09-17  7:18 ` [PATCH v7 04/19] staging: r8188eu: remove the helpers of rtw_read32() Fabio M. De Francesco
2021-09-17  7:18 ` [PATCH v7 05/19] staging: r8188eu: remove the helpers of usb_write8() Fabio M. De Francesco
2021-09-17  7:18 ` [PATCH v7 06/19] staging: r8188eu: remove the helpers of usb_write16() Fabio M. De Francesco
2021-09-17  7:18 ` [PATCH v7 07/19] staging: r8188eu: remove the helpers of usb_write32() Fabio M. De Francesco
2021-09-17  7:18 ` [PATCH v7 08/19] staging: r8188eu: remove the helpers of usb_writeN() Fabio M. De Francesco
2021-09-17  7:18 ` [PATCH v7 09/19] staging: r8188eu: remove the helpers of usb_read_port() Fabio M. De Francesco
2021-09-17  7:18 ` [PATCH v7 10/19] staging: r8188eu: remove the helpers of usb_write_port() Fabio M. De Francesco
2021-09-17  7:18 ` [PATCH v7 11/19] staging: r8188eu: remove the helpers of usb_read_port_cancel() Fabio M. De Francesco
2021-09-17  7:18 ` [PATCH v7 12/19] staging: r8188eu: remove the helpers of usb_write_port_cancel() Fabio M. De Francesco
2021-09-17  7:18 ` [PATCH v7 13/19] staging: r8188eu: remove core/rtw_io.c Fabio M. De Francesco
2021-09-17  7:18 ` [PATCH v7 14/19] staging: r8188eu: remove struct _io_ops Fabio M. De Francesco
2021-09-17  7:18 ` [PATCH v7 15/19] staging: r8188eu: clean up usbctrl_vendorreq() Fabio M. De Francesco
2021-09-17 14:44   ` Greg Kroah-Hartman
2021-09-18 11:13     ` Fabio M. De Francesco
2021-09-17  7:18 ` [PATCH v7 16/19] staging: r8188eu: clean up rtw_read*() and rtw_write*() Fabio M. De Francesco
2021-09-17 14:44   ` Greg Kroah-Hartman
2021-09-17 14:45   ` Greg Kroah-Hartman
2021-09-18 11:41     ` Fabio M. De Francesco
2021-09-17  7:18 ` [PATCH v7 17/19] staging: r8188eu: shorten calls chain of rtw_read{8,16,32}() Fabio M. De Francesco
2021-09-17 14:50   ` Greg Kroah-Hartman [this message]
2021-09-17 15:01     ` David Laight
2021-09-17 15:17       ` 'Greg Kroah-Hartman'
2021-09-18 12:19     ` Fabio M. De Francesco
2021-09-17  7:18 ` [PATCH v7 18/19] staging: r8188eu: shorten calls chain of rtw_write{8,16,32,N}() Fabio M. De Francesco
2021-09-17  7:18 ` [PATCH v7 19/19] staging: r8188eu: remove shared buffer for usb requests Fabio M. De Francesco
2021-09-17 14:55   ` Greg Kroah-Hartman
2021-09-17 15:03     ` Pavel Skripkin
2021-09-17 15:06       ` Pavel Skripkin
2021-09-17 15:18       ` Greg Kroah-Hartman
2021-09-17 15:23         ` Pavel Skripkin
2021-09-17 11:07 ` [PATCH v7 00/19] staging: r8188eu: shorten and simplify calls chains Dan Carpenter

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=YUSrIqW5WSYuoa14@kroah.com \
    --to=gregkh@linuxfoundation.org \
    --cc=Larry.Finger@lwfinger.net \
    --cc=dan.carpenter@oracle.com \
    --cc=david.Laight@aculab.com \
    --cc=fmdefrancesco@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-staging@lists.linux.dev \
    --cc=paskripkin@gmail.com \
    --cc=phil@philpotter.co.uk \
    --subject='Re: [PATCH v7 17/19] staging: r8188eu: shorten calls chain of rtw_read{8,16,32}()' \
    /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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).