| Message ID | 20201009171307.864608-1-Jerome.Pouiller@silabs.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | staging: wfx: fix issues reported by Smatch | expand |
On Saturday 10 October 2020 14:40:34 CEST Greg Kroah-Hartman wrote: > On Sat, Oct 10, 2020 at 02:22:13PM +0200, Jérôme Pouiller wrote: > > On Friday 9 October 2020 20:52:47 CEST Kalle Valo wrote: > > > Jerome Pouiller <Jerome.Pouiller@silabs.com> writes: > > > > > > > From: Jérôme Pouiller <jerome.pouiller@silabs.com> > > > > > > > > Smatch complains: > > > > > > > > drivers/staging/wfx/hif_rx.c:177 hif_scan_complete_indication() warn: potential NULL parameter dereference 'wvif' > > > > drivers/staging/wfx/data_tx.c:576 wfx_flush() warn: potential NULL parameter dereference 'wvif' > > > > > > > > Indeed, if the vif id returned by the device does not exist anymore, > > > > wdev_to_wvif() could return NULL. > > > > > > > > In add, the error is not handled uniformly in the code, sometime a > > > > WARN() is displayed but code continue, sometime a dev_warn() is > > > > displayed, sometime it is just not tested, ... > > > > > > > > This patch standardize that. > > > > > > > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > Signed-off-by: Jérôme Pouiller <jerome.pouiller@silabs.com> > > > > --- > > > > drivers/staging/wfx/data_tx.c | 5 ++++- > > > > drivers/staging/wfx/hif_rx.c | 34 ++++++++++++++++++++++++---------- > > > > drivers/staging/wfx/sta.c | 4 ++++ > > > > 3 files changed, 32 insertions(+), 11 deletions(-) > > > > > > > > diff --git a/drivers/staging/wfx/data_tx.c b/drivers/staging/wfx/data_tx.c > > > > index b4d5dd3d2d23..8db0be08daf8 100644 > > > > --- a/drivers/staging/wfx/data_tx.c > > > > +++ b/drivers/staging/wfx/data_tx.c > > > > @@ -431,7 +431,10 @@ static void wfx_skb_dtor(struct wfx_vif *wvif, struct sk_buff *skb) > > > > sizeof(struct hif_req_tx) + > > > > req->fc_offset; > > > > > > > > - WARN_ON(!wvif); > > > > + if (!wvif) { > > > > + pr_warn("%s: vif associated with the skb does not exist anymore\n", __func__); > > > > + return; > > > > + } > > > > > > I'm not really a fan of using function names in warning or error > > > messages as it clutters the log. In debug messages I think they are ok. > > > > In the initial code, I used WARN() that far more clutters the log (I > > have stated that a backtrace won't provide any useful information, so > > pr_warn() was better suited). > > > > In add, in my mind, these warnings are debug messages. If they appears, > > the user should probably report a bug. > > > > Finally, in this patch, I use the same message several times (ok, not > > this particular one). So the function name is a way to differentiate > > them. > > You should use dev_*() for these, that way you can properly determine > the exact device as well. Totally agree. I initially did that. However, the device is a field of wvif which is NULL in this case. I could have changed the code to get the real pointer to the device. But I didn't want to clutter the code just for a debug message (and also because I was a bit lazy).
From: Jérôme Pouiller <jerome.pouiller@silabs.com> Fix the issues reported by Dan using Smatch[1]. [1] https://lore.kernel.org/lkml/20201008131320.GA1042@kadam/ Jérôme Pouiller (8): staging: wfx: improve error handling of hif_join() staging: wfx: check memory allocation staging: wfx: standardize the error when vif does not exist staging: wfx: wfx_init_common() returns NULL on error staging: wfx: increase robustness of hif_generic_confirm() staging: wfx: gpiod_get_value() can return an error staging: wfx: drop unicode characters from strings staging: wfx: improve robustness of wfx_get_hw_rate() drivers/staging/wfx/bh.c | 2 +- drivers/staging/wfx/data_tx.c | 9 ++++++- drivers/staging/wfx/hif_rx.c | 44 +++++++++++++++++++++++------------ drivers/staging/wfx/hif_tx.c | 4 +++- drivers/staging/wfx/main.c | 10 ++++++-- drivers/staging/wfx/sta.c | 4 ++++ 6 files changed, 53 insertions(+), 20 deletions(-)