LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
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
Subject: Re: [PATCH RFC 1/3] staging: r8188eu: add proper rtw_read* error handling
Date: Sat, 21 Aug 2021 00:41:31 +0100	[thread overview]
Message-ID: <7a83e221a017e292bea8862191e48c6c95bb77da.camel@philpotter.co.uk> (raw)
In-Reply-To: <f4fb967c992c29c8d2e8f67e78835b52a60d2e52.1629479152.git.paskripkin@gmail.com>

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


  parent reply	other threads:[~2021-08-20 23:41 UTC|newest]

Thread overview: 118+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-20 17:07 [PATCH RFC 0/3] staging: r8188eu: avoid uninit value bugs Pavel Skripkin
2021-08-20 17:07 ` [PATCH RFC 1/3] staging: r8188eu: add proper rtw_read* error handling Pavel Skripkin
2021-08-20 21:50   ` Pavel Skripkin
2021-08-20 23:41   ` Phillip Potter [this message]
2021-08-21  5:55   ` Fabio M. De Francesco
2021-08-21 10:35     ` Pavel Skripkin
2021-08-21 12:11       ` Fabio M. De Francesco
2021-08-20 17:07 ` [PATCH RFC 2/3] staging: r8188eu: add error handling to ReadFuse Pavel Skripkin
2021-08-20 23:51   ` Phillip Potter
2021-08-21  3:59     ` Fabio M. De Francesco
2021-08-20 17:07 ` [PATCH RFC 3/3] staging: r8188eu: add error argument to read_macreg Pavel Skripkin
2021-08-20 23:18   ` Phillip Potter
2021-08-21 10:38     ` Pavel Skripkin
2021-08-20 23:12 ` [PATCH RFC 0/3] staging: r8188eu: avoid uninit value bugs Phillip Potter
2021-08-21 10:42   ` Pavel Skripkin
2021-08-22  9:53 ` Fabio M. De Francesco
2021-08-22 10:09   ` Pavel Skripkin
2021-08-22 10:59     ` Fabio M. De Francesco
2021-08-22 11:34       ` Fabio M. De Francesco
2021-08-22 12:10       ` Pavel Skripkin
2021-08-22 12:39         ` Greg KH
2021-08-22 12:50           ` Pavel Skripkin
2021-08-22 13:06             ` Greg KH
2021-08-22 13:21           ` Fabio M. De Francesco
2021-08-22 13:30             ` Greg KH
2021-08-22 13:31             ` Pavel Skripkin
2021-08-22 14:35               ` [PATCH RFC v2 0/6] " Pavel Skripkin
2021-08-22 14:35                 ` [PATCH RFC v2 1/6] staging: r8188eu: remove {read,write}_macreg Pavel Skripkin
2021-08-22 14:35                 ` [PATCH RFC v2 2/6] staging: r8188eu: add helper macro for printing registers Pavel Skripkin
2021-08-22 14:35                 ` [PATCH RFC v2 3/6] staging: r8188eu: add error handling of rtw_read8 Pavel Skripkin
2021-08-22 14:35                 ` [PATCH RFC v2 4/6] staging: r8188eu: add error handling of rtw_read16 Pavel Skripkin
2021-08-22 14:36                 ` [PATCH RFC v2 5/6] staging: r8188eu: add error handling of rtw_read32 Pavel Skripkin
2021-08-23 23:33                   ` Phillip Potter
2021-08-24  0:10                     ` Fabio M. De Francesco
2021-08-24  6:40                       ` Pavel Skripkin
2021-08-24  8:38                         ` Fabio M. De Francesco
2021-08-24  8:47                           ` Pavel Skripkin
2021-08-24  8:53                             ` Pavel Skripkin
2021-08-24  9:46                               ` Fabio M. De Francesco
2021-08-24 22:10                               ` Phillip Potter
2021-08-24 22:07                             ` Phillip Potter
2021-08-24  6:53                     ` Pavel Skripkin
2021-08-24  7:25                     ` [PATCH v3 0/6] staging: r8188eu: avoid uninit value bugs Pavel Skripkin
2021-08-24  7:27                       ` [PATCH v3 1/6] staging: r8188eu: remove {read,write}_macreg Pavel Skripkin
2021-08-26 10:39                         ` Greg KH
2021-08-26 10:40                         ` Greg KH
2021-08-24  7:27                       ` [PATCH v3 2/6] staging: r8188eu: add helper macro for printing registers Pavel Skripkin
2021-08-26 10:37                         ` Greg KH
2021-08-24  7:27                       ` [PATCH v3 3/6] staging: r8188eu: add error handling of rtw_read8 Pavel Skripkin
2021-08-25 12:05                         ` kernel test robot
2021-08-25 12:17                           ` Pavel Skripkin
2021-08-25 12:51                             ` Dan Carpenter
2021-08-25 13:02                               ` Pavel Skripkin
2021-08-25 13:34                                 ` Dan Carpenter
2021-08-25 13:44                                   ` Pavel Skripkin
2021-08-25 17:11                                     ` Nick Desaulniers
2021-08-26 11:08                                       ` Dan Carpenter
2021-08-25 23:45                         ` Fabio M. De Francesco
2021-08-26  5:13                           ` Pavel Skripkin
2021-08-26  8:21                         ` David Laight
2021-08-26  8:27                           ` Pavel Skripkin
2021-08-26 10:19                             ` David Laight
2021-08-26 11:21                               ` Dan Carpenter
2021-08-27  8:14                                 ` David Laight
2021-08-27  8:22                                   ` Pavel Skripkin
2021-08-27  9:07                         ` Dan Carpenter
2021-08-27  9:16                           ` Pavel Skripkin
2021-08-27  9:23                             ` Dan Carpenter
2021-08-30 11:21                         ` kernel test robot
2021-08-24  7:27                       ` [PATCH v3 4/6] staging: r8188eu: add error handling of rtw_read16 Pavel Skripkin
2021-08-25  4:35                         ` Fabio M. De Francesco
2021-08-25  8:22                           ` Pavel Skripkin
2021-08-25  9:48                             ` Fabio M. De Francesco
2021-08-25  9:55                               ` Pavel Skripkin
2021-08-25 10:06                                 ` Dan Carpenter
2021-08-25 10:13                                   ` Pavel Skripkin
2021-08-25 10:38                                     ` Dan Carpenter
2021-08-25 10:41                                       ` Pavel Skripkin
2021-08-25 11:06                                       ` Fabio M. De Francesco
2021-08-25 11:11                                         ` Fabio M. De Francesco
2021-08-25 11:31                                         ` Dan Carpenter
2021-08-25 12:11                                           ` Fabio M. De Francesco
2021-08-25 10:51                                 ` Fabio M. De Francesco
2021-08-26 10:50                         ` Greg KH
2021-08-26 10:58                           ` Pavel Skripkin
2021-08-24  7:27                       ` [PATCH v3 5/6] staging: r8188eu: add error handling of rtw_read32 Pavel Skripkin
2021-08-25  4:40                         ` Fabio M. De Francesco
2021-08-26  8:51                         ` David Laight
2021-08-26  9:22                           ` Pavel Skripkin
2021-08-26  9:27                             ` Pavel Skripkin
2021-08-26 10:22                               ` David Laight
2021-08-26 10:55                                 ` Pavel Skripkin
2021-08-26 10:59                                   ` David Laight
2021-08-26 20:03                                     ` Pavel Skripkin
2021-08-27  7:12                                       ` gregkh
2021-08-27  7:16                                         ` Pavel Skripkin
2021-08-24  7:27                       ` [PATCH v3 6/6] staging: r8188eu: make ReadEFuse return an int Pavel Skripkin
2021-08-25 10:13                       ` [PATCH v3 0/6] staging: r8188eu: avoid uninit value bugs Fabio M. De Francesco
2021-08-27  7:49                       ` Kari Argillander
2021-08-27  7:52                         ` Pavel Skripkin
2021-08-24  6:58                   ` [PATCH RFC v2 5/6] staging: r8188eu: add error handling of rtw_read32 Dan Carpenter
2021-08-24  7:01                     ` Pavel Skripkin
2021-08-24 15:07                       ` Fabio M. De Francesco
2021-08-22 14:36                 ` [PATCH RFC v2 6/6] staging: r8188eu: make ReadEFuse return an int Pavel Skripkin
2021-08-22 15:30                 ` [PATCH RFC v2 0/6] staging: r8188eu: avoid uninit value bugs Pavel Skripkin
2021-08-22 16:05                   ` Michael Straube
2021-08-22 16:26                     ` Pavel Skripkin
2021-08-22 23:52                       ` Phillip Potter
2021-08-22 17:36                 ` Fabio M. De Francesco
2021-08-22 17:38                   ` Pavel Skripkin
2021-08-22 20:06                     ` Fabio M. De Francesco
2021-08-22 20:19                       ` Pavel Skripkin
2021-08-23  0:12                 ` Phillip Potter
2021-08-23  6:38                   ` Pavel Skripkin
2021-08-23  6:44                   ` Pavel Skripkin
2021-08-22 16:03               ` [PATCH RFC 0/3] " Fabio M. De Francesco
2021-08-22 16:15                 ` Pavel Skripkin
2021-08-22 15:04             ` Phillip Potter

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=7a83e221a017e292bea8862191e48c6c95bb77da.camel@philpotter.co.uk \
    --to=phil@philpotter.co.uk \
    --cc=Larry.Finger@lwfinger.net \
    --cc=fmdefrancesco@gmail.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-staging@lists.linux.dev \
    --cc=paskripkin@gmail.com \
    --cc=straube.linux@gmail.com \
    --subject='Re: [PATCH RFC 1/3] staging: r8188eu: add proper rtw_read* 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 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).