mbox series

[0/7] wfx: move out from the staging area

Message ID 20201007101943.749898-1-Jerome.Pouiller@silabs.com
Headers show
Series wfx: move out from the staging area | expand

Message

Jérôme Pouiller Oct. 7, 2020, 10:19 a.m. UTC
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

Comments

Greg KH Oct. 7, 2020, 10:55 a.m. UTC | #1
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
Kalle Valo Oct. 8, 2020, 7:30 a.m. UTC | #2
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 Oct. 8, 2020, 9:50 a.m. UTC | #3
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 Oct. 8, 2020, 10:10 a.m. UTC | #4
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
Dan Carpenter Oct. 8, 2020, 1:13 p.m. UTC | #5
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