Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <7a83e221a017e292bea8862191e48c6c95bb77da.camel@philpotter.co.uk>
Subject: Re: [PATCH RFC 1/3] staging: r8188eu: add proper rtw_read* error
 handling
From:   Phillip Potter <phil@philpotter.co.uk>
To:     Pavel Skripkin <paskripkin@gmail.com>
Cc:     linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org,
        Larry.Finger@lwfinger.net, gregkh@linuxfoundation.org,
        straube.linux@gmail.com, fmdefrancesco@gmail.com
Date:   Sat, 21 Aug 2021 00:41:31 +0100
In-Reply-To: <f4fb967c992c29c8d2e8f67e78835b52a60d2e52.1629479152.git.paskripkin@gmail.com>
References: <cover.1629479152.git.paskripkin@gmail.com>
         <f4fb967c992c29c8d2e8f67e78835b52a60d2e52.1629479152.git.paskripkin@gmail.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.40.3 (3.40.3-1.fc34) 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Fri, 2021-08-20 at 20:07 +0300, Pavel Skripkin wrote:
> rtw_read*() functions call usb_read* inside. These functions could
> fail
> in some cases; for example: failed to receive control message. These
> cases should be handled to prevent uninit value bugs, since usb_read*
> functions blindly return stack variable without checking if this
> value
> _actualy_ initialized.
> 
> To achive it, all usb_read* and rtw_read*() argument list is expanded
> with pointer to error and added error usbctrl_vendorreq() error
> checking.
> If transfer is successful error will be initialized to 0 otherwise to
> error returned from usb_control_msg().
> 
> To not break the build, added error checking for rtw_read*() call all
> across the driver.
> 
> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
> ---
>  drivers/staging/r8188eu/core/rtw_debug.c      |  79 +++-
>  drivers/staging/r8188eu/core/rtw_efuse.c      |  83 +++-
>  drivers/staging/r8188eu/core/rtw_io.c         |  18 +-
>  drivers/staging/r8188eu/core/rtw_mp.c         |  37 +-
>  drivers/staging/r8188eu/core/rtw_mp_ioctl.c   |  20 +-
>  drivers/staging/r8188eu/core/rtw_pwrctrl.c    |   6 +-
>  drivers/staging/r8188eu/core/rtw_sreset.c     |   7 +-
>  drivers/staging/r8188eu/hal/HalPwrSeqCmd.c    |   9 +-
>  drivers/staging/r8188eu/hal/hal_com.c         |  22 +-
>  drivers/staging/r8188eu/hal/odm_interface.c   |  12 +-
>  drivers/staging/r8188eu/hal/rtl8188e_cmd.c    |  37 +-
>  drivers/staging/r8188eu/hal/rtl8188e_dm.c     |   6 +-
>  .../staging/r8188eu/hal/rtl8188e_hal_init.c   | 198 +++++++--
>  drivers/staging/r8188eu/hal/rtl8188e_phycfg.c |  26 +-
>  drivers/staging/r8188eu/hal/rtl8188e_sreset.c |  20 +-
>  drivers/staging/r8188eu/hal/rtl8188eu_led.c   |  17 +-
>  drivers/staging/r8188eu/hal/usb_halinit.c     | 394 ++++++++++++++--
> --
>  drivers/staging/r8188eu/hal/usb_ops_linux.c   |  16 +-
>  drivers/staging/r8188eu/include/rtw_io.h      |  18 +-
>  drivers/staging/r8188eu/os_dep/ioctl_linux.c  | 168 +++++---
>  20 files changed, 941 insertions(+), 252 deletions(-)
> 
...
> diff --git a/drivers/staging/r8188eu/hal/usb_ops_linux.c
> b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> index 953fa05dc30c..980af6c02be5 100644
> --- a/drivers/staging/r8188eu/hal/usb_ops_linux.c
> +++ b/drivers/staging/r8188eu/hal/usb_ops_linux.c
> @@ -101,29 +101,31 @@ static int usbctrl_vendorreq(struct intf_hdl
> *pintfhdl, u16 value, void *pdata,
>         return status;
>  }
>  
> -static u8 usb_read8(struct intf_hdl *pintfhdl, u32 addr)
> +static u8 usb_read8(struct intf_hdl *pintfhdl, u32 addr, int *error)
>  {
>         u8 requesttype;
>         u16 wvalue;
>         u16 len;
>         u8 data = 0;
> +       int res;
>  
> -
> +       if (unlikely(!error))
> +               WARN_ON_ONCE("r8188eu: Reading w/o error checking is
> bad idea\n");
>  
>         requesttype = 0x01;/* read_in */
>  
>         wvalue = (u16)(addr & 0x0000ffff);
>         len = 1;
>  
> -       usbctrl_vendorreq(pintfhdl, wvalue, &data, len, requesttype);
> -
> -
> +       res = usbctrl_vendorreq(pintfhdl, wvalue, &data, len,
> requesttype);
> +       if (likely(error))
> +               *error = res < 0? res: 0;
>  
>         return data;
>  
>  }
>  
> -static u16 usb_read16(struct intf_hdl *pintfhdl, u32 addr)
> +static u16 usb_read16(struct intf_hdl *pintfhdl, u32 addr, int
> *error)
>  {
>         u8 requesttype;
>         u16 wvalue;
> @@ -138,7 +140,7 @@ static u16 usb_read16(struct intf_hdl *pintfhdl,
> u32 addr)
>         return (u16)(le32_to_cpu(data) & 0xffff);
>  }
>  
> -static u32 usb_read32(struct intf_hdl *pintfhdl, u32 addr)
> +static u32 usb_read32(struct intf_hdl *pintfhdl, u32 addr, int
> *error)
>  {
>         u8 requesttype;
>         u16 wvalue;

Dear Pavel,

For this patch, you modify the signature of the usb_read*() functions,
but only introduce the usbctrl_vendorreq checks for usb_read8.

I can see from the following patch that you have done the others too
later on, but really I would say for changes like this, they should be
grouped in the same patch. Also, just me nitpicking probably, but the
patch is rather large - sometimes unavoidable I know, but perhaps
better to group logically into areas (files or types of change) in
order to get the patches smaller and thus more easily reviewable. Many
thanks.

In terms of what we do with the errors, I would invite comments from
others on this too due to my inexperience, but I like your thinking.

Regards,
Phil