| Message ID | 20201007101943.749898-1-Jerome.Pouiller@silabs.com |
|---|---|
| Headers | show |
| Series | wfx: move out from the staging area | expand |
Kalle Valo <kvalo@codeaurora.org> writes: > Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes: > >> On Wed, Oct 07, 2020 at 12:19:36PM +0200, Jerome Pouiller wrote: >>> From: Jérôme Pouiller <jerome.pouiller@silabs.com> >>> >>> I think the wfx driver is now mature enough to be accepted in the >>> drivers/net/wireless directory. >>> >>> There is still one item on the TODO list. It is an idea to improve the rate >>> control in some particular cases[1]. However, the current performances of the >>> driver seem to satisfy everyone. In add, the suggested change is large enough. >>> So, I would prefer to implement it only if it really solves an issue. I think it >>> is not an obstacle to move the driver out of the staging area. >>> >>> In order to comply with the last rules for the DT bindings, I have converted the >>> documentation to yaml. I am moderately happy with the result. Especially, for >>> the description of the binding. Any comments are welcome. >>> >>> The series also update the copyrights dates of the files. I don't know exactly >>> how this kind of changes should be sent. It's a bit weird to change all the >>> copyrights in one commit, but I do not see any better way. >>> >>> I also include a few fixes I have found these last weeks. >>> >>> [1] https://lore.kernel.org/lkml/3099559.gv3Q75KnN1@pc-42 >> >> I'll take the first 6 patches here, the last one you should work with >> the wireless maintainers to get reviewed. >> >> Maybe that might want to wait until after 5.10-rc1 is out, with all of >> these changes in it, making it an easier move. > > Yes, the driver needs to be reviewed in linux-wireless list. I recommend > submitting the whole driver in a patchset with one file per patch, which > seems to be the easiest way to review a full driver. The final move will > be in just one commit moving the driver, just like patch 7 does here. As > an example see how wilc1000 review was done. > > Device tree bindings needs to be reviewed by the DT maintainer so CC > devicetree on that patch. BTW, I wrote some instructions for new wireless drivers: https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#new_driver
Jérôme Pouiller <jerome.pouiller@silabs.com> writes: > On Thursday 8 October 2020 09:30:06 CEST Kalle Valo wrote: > [...] >> Yes, the driver needs to be reviewed in linux-wireless list. I recommend >> submitting the whole driver in a patchset with one file per patch, which >> seems to be the easiest way to review a full driver. The final move will >> be in just one commit moving the driver, just like patch 7 does here. As >> an example see how wilc1000 review was done. > > I see. I suppose it is still a bit complicated to review? Maybe I could > try to make things easier. > > For my submission to staging/ I had taken time to split the driver in an > understandable series of patches[1]. I think it was easier to review than > just sending files one by one. I could do the same thing for the > submission to linux-wireless. It would ask me a bit of work but, since I > already have a template, it is conceivable. > > Do you think it is worth it, or it would be an unnecessary effort? > > [1] > https://lore.kernel.org/driverdev-devel/20190919142527.31797-1-Jerome.Pouiller@silabs.com/ > or commits a7a91ca5a23d^..40115bbc40e2 I don't know how others think, but I prefer to review new drivers "one file per patch" style as I get to see the big picture easily. And besides, splitting the driver like that would be a huge job for you. I don't think it's worth your time in this case. And making changes in the driver during review process becomes even more complex. -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
There are some static checker warnings to look at from linux-next from
Tuesday.
drivers/staging/wfx/hif_tx.c:319 hif_join() error: we previously assumed 'channel' could be null (see line 315)
drivers/staging/wfx/main.c:228 wfx_send_pdata_pds() warn: potential NULL parameter dereference 'tmp_buf'
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'
drivers/staging/wfx/bus_spi.c:228 wfx_spi_probe() warn: 'bus->core' could be an error pointer
drivers/staging/wfx/bus_sdio.c:221 wfx_sdio_probe() warn: 'bus->core' could be an error pointer
drivers/staging/wfx/hif_rx.c:26 hif_generic_confirm() warn: negative user subtract: 0-u16max - 4
drivers/staging/wfx/hif_rx.c:98 hif_wakeup_indication() warn: 'gpiod_get_value(wdev->pdata.gpio_wakeup)' returns positive and negative
drivers/staging/wfx/bh.c:24 device_wakeup() warn: 'gpiod_get_value_cansleep(wdev->pdata.gpio_wakeup)' returns positive and negative
drivers/staging/wfx/hif_rx.c:235 hif_generic_indication() warn: format string contains non-ascii character '\xc2'
drivers/staging/wfx/hif_rx.c:235 hif_generic_indication() warn: format string contains non-ascii character '\xb0'
drivers/staging/wfx/data_tx.c:37 wfx_get_hw_rate() warn: constraint '(struct ieee80211_supported_band)->bitrates' overflow 'band->bitrates' 0 <= abs_rl '0-127' user_rl '' required = '(struct ieee80211_supported_band)->n_bitrates'
Some of these are unpublished checks that I haven't published because
they are too crap. The rest of the email is just long explanations.
Skip if not required.
regards,
dan carpenter
#1
drivers/staging/wfx/hif_tx.c:319 hif_join() error: we previously assumed 'channel' could be null (see line 315)
311 if (!hif)
312 return -ENOMEM;
313 body->infrastructure_bss_mode = !conf->ibss_joined;
314 body->short_preamble = conf->use_short_preamble;
315 if (channel && channel->flags & IEEE80211_CHAN_NO_IR)
^^^^^^^
Check for NULL.
316 body->probe_for_join = 0;
317 else
318 body->probe_for_join = 1;
319 body->channel_number = channel->hw_value;
^^^^^^^^^^^^^^^^^
Unchecked dereference.
320 body->beacon_interval = cpu_to_le32(conf->beacon_int);
321 body->basic_rate_set =
#2
drivers/staging/wfx/main.c:228 wfx_send_pdata_pds() warn: potential NULL parameter dereference 'tmp_buf'
227 tmp_buf = kmemdup(pds->data, pds->size, GFP_KERNEL);
No check for allocation failure.
228 ret = wfx_send_pds(wdev, tmp_buf, pds->size);
229 kfree(tmp_buf);
#3
drivers/staging/wfx/hif_rx.c:177 hif_scan_complete_indication() warn: potential NULL parameter dereference 'wvif'
170 static int hif_scan_complete_indication(struct wfx_dev *wdev,
171 const struct hif_msg *hif,
172 const void *buf)
173 {
174 struct wfx_vif *wvif = wdev_to_wvif(wdev, hif->interface);
^^^^^^^^^^^^^^^^^^^
Smatch thinks wdev_to_wvif() can return NULL.
175
176 WARN_ON(!wvif);
177 wfx_scan_complete(wvif);
178
179 return 0;
180 }
#4
drivers/staging/wfx/data_tx.c:576 wfx_flush() warn: potential NULL parameter dereference 'wvif'
572 while ((skb = skb_dequeue(&dropped)) != NULL) {
573 hif = (struct hif_msg *)skb->data;
574 wvif = wdev_to_wvif(wdev, hif->interface);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Same.
575 ieee80211_tx_info_clear_status(IEEE80211_SKB_CB(skb));
576 wfx_skb_dtor(wvif, skb);
577 }
578 }
#5 and #6
drivers/staging/wfx/bus_spi.c:228 wfx_spi_probe() warn: 'bus->core' could be an error pointer
drivers/staging/wfx/bus_sdio.c:221 wfx_sdio_probe() warn: 'bus->core' could be an error pointer
The wfx_init_common() function should return NULL instead of error
pointer if devm_gpiod_get_optional() fails.
#7
drivers/staging/wfx/hif_rx.c:26 hif_generic_confirm() warn: negative user subtract: 0-u16max - 4
20 static int hif_generic_confirm(struct wfx_dev *wdev,
21 const struct hif_msg *hif, const void *buf)
22 {
23 // All confirm messages start with status
24 int status = le32_to_cpup((__le32 *)buf);
25 int cmd = hif->id;
26 int len = le16_to_cpu(hif->len) - 4; // drop header
Can "len" get set to negative 4?
27
28 WARN(!mutex_is_locked(&wdev->hif_cmd.lock), "data locking error");
#8 and #9
drivers/staging/wfx/hif_rx.c:98 hif_wakeup_indication() warn: 'gpiod_get_value(wdev->pdata.gpio_wakeup)' returns positive and negative
drivers/staging/wfx/bh.c:24 device_wakeup() warn: 'gpiod_get_value_cansleep(wdev->pdata.gpio_wakeup)' returns positive and negative
94 static int hif_wakeup_indication(struct wfx_dev *wdev,
95 const struct hif_msg *hif, const void *buf)
96 {
97 if (!wdev->pdata.gpio_wakeup
98 || !gpiod_get_value(wdev->pdata.gpio_wakeup)) {
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Negative error codes from gpiod_get_value() should be treated as error.
99 dev_warn(wdev->dev, "unexpected wake-up indication\n");
100 return -EIO;
101 }
102 return 0;
103 }
#10 and #11
drivers/staging/wfx/hif_rx.c:235 hif_generic_indication() warn: format string contains non-ascii character '\xc2'
drivers/staging/wfx/hif_rx.c:235 hif_generic_indication() warn: format string contains non-ascii character '\xb0'
234 if (!wfx_api_older_than(wdev, 1, 4))
235 dev_info(wdev->dev, "Rx test ongoing. Temperature: %d°C\n",
^
Can we output non-ascii to dmesg? (I didn't add this Smatch check so I
don't really know the answer).
236 body->data.rx_stats.current_temp);
#12
drivers/staging/wfx/data_tx.c:37 wfx_get_hw_rate() warn: constraint '(struct ieee80211_supported_band)->bitrates' overflow 'band->bitrates' 0 <= abs_rl '0-127' user_rl '' required = '(struct ieee80211_supported_band)->n_bitrates'
20 static int wfx_get_hw_rate(struct wfx_dev *wdev,
21 const struct ieee80211_tx_rate *rate)
22 {
23 struct ieee80211_supported_band *band;
24
25 if (rate->idx < 0)
26 return -1;
27 if (rate->flags & IEEE80211_TX_RC_MCS) {
28 if (rate->idx > 7) {
29 WARN(1, "wrong rate->idx value: %d", rate->idx);
30 return -1;
31 }
32 return rate->idx + 14;
33 }
34 // WFx only support 2GHz, else band information should be retrieved
35 // from ieee80211_tx_info
36 band = wdev->hw->wiphy->bands[NL80211_BAND_2GHZ];
37 return band->bitrates[rate->idx].hw_value;
This code assumes that "rate->idx" can be all sort of invalid values
including negatives but it doesn't cap it against:
if (rate->idx >= band->n_bitrates)
return -1;
38 }
If you you read all the way down to the second end of the email then you
are a true hero. regards again,
dan carpenter
From: Jérôme Pouiller <jerome.pouiller@silabs.com> I think the wfx driver is now mature enough to be accepted in the drivers/net/wireless directory. There is still one item on the TODO list. It is an idea to improve the rate control in some particular cases[1]. However, the current performances of the driver seem to satisfy everyone. In add, the suggested change is large enough. So, I would prefer to implement it only if it really solves an issue. I think it is not an obstacle to move the driver out of the staging area. In order to comply with the last rules for the DT bindings, I have converted the documentation to yaml. I am moderately happy with the result. Especially, for the description of the binding. Any comments are welcome. The series also update the copyrights dates of the files. I don't know exactly how this kind of changes should be sent. It's a bit weird to change all the copyrights in one commit, but I do not see any better way. I also include a few fixes I have found these last weeks. [1] https://lore.kernel.org/lkml/3099559.gv3Q75KnN1@pc-42 Jérôme Pouiller (7): staging: wfx: fix handling of MMIC error staging: wfx: remove remaining code of 'secure link' feature staging: wfx: fix BA sessions for older firmwares staging: wfx: fix QoS priority for slow buses staging: wfx: update copyrights dates dt-bindings: staging: wfx: silabs,wfx yaml conversion wfx: move out from the staging area .../bindings/net/wireless/silabs,wfx.yaml | 125 ++++++++++++++++++ MAINTAINERS | 2 +- drivers/net/wireless/Kconfig | 1 + drivers/net/wireless/Makefile | 1 + drivers/net/wireless/silabs/Kconfig | 17 +++ drivers/net/wireless/silabs/Makefile | 3 + .../wireless/silabs}/wfx/Kconfig | 0 .../wireless/silabs}/wfx/Makefile | 0 .../{staging => net/wireless/silabs}/wfx/bh.c | 2 +- .../{staging => net/wireless/silabs}/wfx/bh.h | 2 +- .../wireless/silabs}/wfx/bus.h | 2 +- .../wireless/silabs}/wfx/bus_sdio.c | 2 +- .../wireless/silabs}/wfx/bus_spi.c | 2 +- .../wireless/silabs}/wfx/data_rx.c | 7 +- .../wireless/silabs}/wfx/data_rx.h | 2 +- .../wireless/silabs}/wfx/data_tx.c | 2 +- .../wireless/silabs}/wfx/data_tx.h | 2 +- .../wireless/silabs}/wfx/debug.c | 19 +-- .../wireless/silabs}/wfx/debug.h | 0 .../wireless/silabs}/wfx/fwio.c | 2 +- .../wireless/silabs}/wfx/fwio.h | 0 .../wireless/silabs}/wfx/hif_api_cmd.h | 2 +- .../wireless/silabs}/wfx/hif_api_general.h | 2 +- .../wireless/silabs}/wfx/hif_api_mib.h | 2 +- .../wireless/silabs}/wfx/hif_rx.c | 2 +- .../wireless/silabs}/wfx/hif_rx.h | 0 .../wireless/silabs}/wfx/hif_tx.c | 2 +- .../wireless/silabs}/wfx/hif_tx.h | 2 +- .../wireless/silabs}/wfx/hif_tx_mib.c | 2 +- .../wireless/silabs}/wfx/hif_tx_mib.h | 2 +- .../wireless/silabs}/wfx/hwio.c | 2 +- .../wireless/silabs}/wfx/hwio.h | 2 +- .../wireless/silabs}/wfx/key.c | 2 +- .../wireless/silabs}/wfx/key.h | 2 +- .../wireless/silabs}/wfx/main.c | 2 +- .../wireless/silabs}/wfx/main.h | 2 +- .../wireless/silabs}/wfx/queue.c | 16 ++- .../wireless/silabs}/wfx/queue.h | 3 +- .../wireless/silabs}/wfx/scan.c | 2 +- .../wireless/silabs}/wfx/scan.h | 2 +- .../wireless/silabs}/wfx/sta.c | 2 +- .../wireless/silabs}/wfx/sta.h | 2 +- .../wireless/silabs}/wfx/traces.h | 2 +- .../wireless/silabs}/wfx/wfx.h | 2 +- drivers/staging/Kconfig | 2 - drivers/staging/Makefile | 1 - .../bindings/net/wireless/siliabs,wfx.txt | 98 -------------- drivers/staging/wfx/TODO | 6 - 48 files changed, 198 insertions(+), 161 deletions(-) create mode 100644 Documentation/devicetree/bindings/net/wireless/silabs,wfx.yaml create mode 100644 drivers/net/wireless/silabs/Kconfig create mode 100644 drivers/net/wireless/silabs/Makefile rename drivers/{staging => net/wireless/silabs}/wfx/Kconfig (100%) rename drivers/{staging => net/wireless/silabs}/wfx/Makefile (100%) rename drivers/{staging => net/wireless/silabs}/wfx/bh.c (99%) rename drivers/{staging => net/wireless/silabs}/wfx/bh.h (92%) rename drivers/{staging => net/wireless/silabs}/wfx/bus.h (94%) rename drivers/{staging => net/wireless/silabs}/wfx/bus_sdio.c (99%) rename drivers/{staging => net/wireless/silabs}/wfx/bus_spi.c (99%) rename drivers/{staging => net/wireless/silabs}/wfx/data_rx.c (93%) rename drivers/{staging => net/wireless/silabs}/wfx/data_rx.h (86%) rename drivers/{staging => net/wireless/silabs}/wfx/data_tx.c (99%) rename drivers/{staging => net/wireless/silabs}/wfx/data_tx.h (96%) rename drivers/{staging => net/wireless/silabs}/wfx/debug.c (94%) rename drivers/{staging => net/wireless/silabs}/wfx/debug.h (100%) rename drivers/{staging => net/wireless/silabs}/wfx/fwio.c (99%) rename drivers/{staging => net/wireless/silabs}/wfx/fwio.h (100%) rename drivers/{staging => net/wireless/silabs}/wfx/hif_api_cmd.h (99%) rename drivers/{staging => net/wireless/silabs}/wfx/hif_api_general.h (99%) rename drivers/{staging => net/wireless/silabs}/wfx/hif_api_mib.h (99%) rename drivers/{staging => net/wireless/silabs}/wfx/hif_rx.c (99%) rename drivers/{staging => net/wireless/silabs}/wfx/hif_rx.h (100%) rename drivers/{staging => net/wireless/silabs}/wfx/hif_tx.c (99%) rename drivers/{staging => net/wireless/silabs}/wfx/hif_tx.h (97%) rename drivers/{staging => net/wireless/silabs}/wfx/hif_tx_mib.c (99%) rename drivers/{staging => net/wireless/silabs}/wfx/hif_tx_mib.h (97%) rename drivers/{staging => net/wireless/silabs}/wfx/hwio.c (99%) rename drivers/{staging => net/wireless/silabs}/wfx/hwio.h (98%) rename drivers/{staging => net/wireless/silabs}/wfx/key.c (99%) rename drivers/{staging => net/wireless/silabs}/wfx/key.h (87%) rename drivers/{staging => net/wireless/silabs}/wfx/main.c (99%) rename drivers/{staging => net/wireless/silabs}/wfx/main.h (95%) rename drivers/{staging => net/wireless/silabs}/wfx/queue.c (93%) rename drivers/{staging => net/wireless/silabs}/wfx/queue.h (94%) rename drivers/{staging => net/wireless/silabs}/wfx/scan.c (98%) rename drivers/{staging => net/wireless/silabs}/wfx/scan.h (90%) rename drivers/{staging => net/wireless/silabs}/wfx/sta.c (99%) rename drivers/{staging => net/wireless/silabs}/wfx/sta.h (98%) rename drivers/{staging => net/wireless/silabs}/wfx/traces.h (99%) rename drivers/{staging => net/wireless/silabs}/wfx/wfx.h (98%) delete mode 100644 drivers/staging/wfx/Documentation/devicetree/bindings/net/wireless/siliabs,wfx.txt delete mode 100644 drivers/staging/wfx/TODO