| Message ID | 20201007101943.749898-1-Jerome.Pouiller@silabs.com |
|---|---|
| Headers | show |
| Series | wfx: move out from the staging area | expand |
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. thanks, greg k-h
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. But do note that there's currently one new driver in review queue, so it will most likely take some time before wfx is reviewed.
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
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
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.
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