LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
To: Larry.Finger@lwfinger.net, phil@philpotter.co.uk,
	gregkh@linuxfoundation.org, straube.linux@gmail.com,
	Pavel Skripkin <paskripkin@gmail.com>
Cc: linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org,
	Pavel Skripkin <paskripkin@gmail.com>
Subject: Re: [PATCH RFC 1/3] staging: r8188eu: add proper rtw_read* error handling
Date: Sat, 21 Aug 2021 07:55:38 +0200	[thread overview]
Message-ID: <5720270.rXTAdOU5UK@localhost.localdomain> (raw)
In-Reply-To: <f4fb967c992c29c8d2e8f67e78835b52a60d2e52.1629479152.git.paskripkin@gmail.com>

On Friday, August 20, 2021 7:07:36 PM CEST 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

Dear Pavel,

Please, achive --> achieve.

> 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(-)

I agree with Philip: please, split this long patch. If I were you, I'd make 
one patch for each of the three rtw_read*() and a fourth patch for usb_read*().

> --- a/drivers/staging/r8188eu/core/rtw_io.c
> +++ b/drivers/staging/r8188eu/core/rtw_io.c
> @@ -34,44 +34,44 @@ jackson@realtek.com.tw
>  #define rtw_cpu_to_le16(val)		cpu_to_le16(val)
>  #define rtw_cpu_to_le32(val)		cpu_to_le32(val)

Not related to your patch, these macros are useless and misleading.

> -u8 _rtw_read8(struct adapter *adapter, u32 addr)
> +u8 _rtw_read8(struct adapter *adapter, u32 addr, int *error)
>  {
>  	u8 r_val;
>  	struct io_priv *pio_priv = &adapter->iopriv;
>  	struct	intf_hdl *pintfhdl = &pio_priv->intf;
> -	u8 (*_read8)(struct intf_hdl *pintfhdl, u32 addr);
> +	u8 (*_read8)(struct intf_hdl *pintfhdl, u32 addr, int *error);
>  
>  
>  	_read8 = pintfhdl->io_ops._read8;
> -	r_val = _read8(pintfhdl, addr);
> +	r_val = _read8(pintfhdl, addr, error);
>  
>  	return r_val;
>  }

I really don't like passing errors through arguments. Why don't you pass 
a storage location where the function save the byte read and instead use the 
return for errors? I think that this would result in a cleaner design. Furthermore,
it is used everywhere in the kernel.
  
> -u16 _rtw_read16(struct adapter *adapter, u32 addr)
> +u16 _rtw_read16(struct adapter *adapter, u32 addr, int *error)
>  {
>  	u16 r_val;
>  	struct io_priv *pio_priv = &adapter->iopriv;
>  	struct	intf_hdl		*pintfhdl = &pio_priv->intf;
> -	u16 (*_read16)(struct intf_hdl *pintfhdl, u32 addr);
> +	u16 (*_read16)(struct intf_hdl *pintfhdl, u32 addr, int *error);
>  
>  	_read16 = pintfhdl->io_ops._read16;
>  
> -	r_val = _read16(pintfhdl, addr);
> +	r_val = _read16(pintfhdl, addr, error);
>  
>  	return r_val;
>  }

Same.

> -u32 _rtw_read32(struct adapter *adapter, u32 addr)
> +u32 _rtw_read32(struct adapter *adapter, u32 addr, int *error)
>  {
>  	u32 r_val;
>  	struct io_priv *pio_priv = &adapter->iopriv;
>  	struct	intf_hdl		*pintfhdl = &pio_priv->intf;
> -	u32	(*_read32)(struct intf_hdl *pintfhdl, u32 addr);
> +	u32	(*_read32)(struct intf_hdl *pintfhdl, u32 addr, int *error);
>  
>  	_read32 = pintfhdl->io_ops._read32;
>  
> -	r_val = _read32(pintfhdl, addr);
> +	r_val = _read32(pintfhdl, addr, error);
>  
>  	return r_val;
>  }

Same.

I'm done for now: too many lines to read all at once :)

Regards,

Fabio



  parent reply	other threads:[~2021-08-21  5:59 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
2021-08-21  5:55   ` Fabio M. De Francesco [this message]
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=5720270.rXTAdOU5UK@localhost.localdomain \
    --to=fmdefrancesco@gmail.com \
    --cc=Larry.Finger@lwfinger.net \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-staging@lists.linux.dev \
    --cc=paskripkin@gmail.com \
    --cc=phil@philpotter.co.uk \
    --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).