On Fri, 2021-08-20 at 20:07 +0300, Pavel Skripkin wrote: > 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. Regards, Phil
