On 8/21/21 2:12 AM, Phillip Potter wrote:

On Fri, 2021-08-20 at 20:07 +0300, Pavel Skripkin wrote:

quoted

Hi, Greg, Larry and Phillip!

I noticed, that new staging driver was added like 3 weeks ago and I
decided
to look at the code, because drivers in staging directory are always
buggy.

The first thing I noticed is *no one* was checking read operations
result, but
it can fail and driver may start writing random stack values into
registers. It
can cause driver misbehavior or device misbehavior.

To avoid this type of bugs, i've expanded read() API with error
parametr,
which will be initialized to error if read fails. It helps callers to
break/return earlier and don't write random values to registers or to
rely
on random values.

Why is this pacth series RFC?
  1. I don't have this device and I cannot test these changes.
  2. I don't know how to handle errors in each particular case. For
now, function
     just returns or returns an error. That's all. I hope, driver
maintainers will
     help with these bits.
  3. I guess, I handled not all uninit value bugs here. I hope, I
fixed
     at least half of them

This series was build-tested and made on top of staging-testing
branch


With regards,
Pavel Skripkin

Pavel Skripkin (3):
  staging: r8188eu: add proper rtw_read* error handling
  staging: r8188eu: add error handling to ReadFuse
  staging: r8188eu: add error argument to read_macreg

 drivers/staging/r8188eu/core/rtw_debug.c      |  79 +++-
 drivers/staging/r8188eu/core/rtw_efuse.c      | 119 +++--
 drivers/staging/r8188eu/core/rtw_io.c         |  18 +-
 drivers/staging/r8188eu/core/rtw_mp.c         |  38 +-
 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/hal_intf.c        |   6 +-
 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   | 260 ++++++++---
 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     | 412 ++++++++++++++--
--
 drivers/staging/r8188eu/hal/usb_ops_linux.c   |  55 ++-
 drivers/staging/r8188eu/include/hal_intf.h    |   6 +-
 .../staging/r8188eu/include/rtl8188e_hal.h    |   2 +-
 drivers/staging/r8188eu/include/rtw_efuse.h   |   4 +-
 drivers/staging/r8188eu/include/rtw_io.h      |  18 +-
 drivers/staging/r8188eu/include/rtw_mp.h      |   2 +-
 drivers/staging/r8188eu/os_dep/ioctl_linux.c  | 168 +++++--
 drivers/staging/r8188eu/os_dep/usb_intf.c     |   4 +-
 26 files changed, 1072 insertions(+), 301 deletions(-)

Dear Pavel,

Firstly, thank you for this contribution, it is much appreciated.
Whilst I'm still learning myself when it comes to this driver and to
kernel code in general, I can certainly say the code looks pretty good
in general so far. I will try and offer individual comments on each
patch.

Thank you for your feedback. So, I will prepare a v2 version in few days 
and, I think, I will leave RFC prefix.

Also, I want to receive some feedback from Larry about error handling in 
each particular case, I guess, he can help us with it.

So, I will split each rtw_read* changes into separate patche and make 
them return an error instead of read value.


Again, big thanks to you and Fabio for feedback, I appreciate it :)




With regards,
Pavel Skripkin