Re: [PATCH] staging: wfx: silence symbol 'wfx_get_ps_timeout' was not declared warning
On Monday 19 April 2021 17:33:48 CEST Ashish Kalra wrote: > > Upon running sparse, "warning: symbol 'wfx_get_ps_timeout' was not declared. > Should it be static?" and "warning: symbol 'wfx_update_pm' was not declared. > Should it be static?" is brought to notice for this file. static keyword > should be added to prevent this warning. let's add it to make it cleaner and > silence the Sparse warning. Hi Ashish, Thank you for your contribution. It seems that this issue is already fixed by commit ce59858bbc10 "staging: wfx: make methods 'wfx_get_ps_timeout' and 'wfx_update_pm' static" (merged in master in version 5.8). Can you check you are working on the last tree? -- Jérôme Pouiller
Re: [PATCH v5 08/24] wfx: add bus_sdio.c
On Tuesday 23 March 2021 15:11:56 CET Ulf Hansson wrote:
> On Mon, 22 Mar 2021 at 18:14, Jérôme Pouiller
> wrote:
> > On Monday 22 March 2021 13:20:35 CET Ulf Hansson wrote:
> > > On Mon, 15 Mar 2021 at 14:25, Jerome Pouiller
> > > wrote:
> > > >
> > > > From: Jérôme Pouiller
> > > >
> > > > Signed-off-by: Jérôme Pouiller
> > > > ---
> > > > drivers/net/wireless/silabs/wfx/bus_sdio.c | 259 +
> > > > 1 file changed, 259 insertions(+)
> > > > create mode 100644 drivers/net/wireless/silabs/wfx/bus_sdio.c
> > >
> > > [...]
> > >
> > > > +static const struct sdio_device_id wfx_sdio_ids[] = {
> > > > + { SDIO_DEVICE(SDIO_VENDOR_ID_SILABS,
> > > > SDIO_DEVICE_ID_SILABS_WF200) },
> > > > + { },
> > > > +};
> > > > +MODULE_DEVICE_TABLE(sdio, wfx_sdio_ids);
> > > > +
> > > > +struct sdio_driver wfx_sdio_driver = {
> > > > + .name = "wfx-sdio",
> > > > + .id_table = wfx_sdio_ids,
> > > > + .probe = wfx_sdio_probe,
> > > > + .remove = wfx_sdio_remove,
> > > > + .drv = {
> > > > + .owner = THIS_MODULE,
> > > > + .of_match_table = wfx_sdio_of_match,
> > >
> > > It's not mandatory to support power management, like system
> > > suspend/resume. However, as this looks like this is a driver for an
> > > embedded SDIO device, you probably want this.
> > >
> > > If that is the case, please assign the dev_pm_ops here and implement
> > > the ->suspend|resume() callbacks.
> >
> > I have no platform to test suspend/resume, so I have only a
> > theoretical understanding of this subject.
>
> I see.
>
> >
> > I understanding is that with the current implementation, the
> > device will be powered off on suspend and then totally reset
> > (including reloading of the firmware) on resume. I am wrong?
>
> You are correct, for a *removable* SDIO card. In this case, the
> mmc/sdio core will remove the corresponding SDIO card/device and its
> corresponding SDIO func devices at system suspend. It will then be
> redetected at system resume (and the SDIO func driver re-probed).
>
> Although, as this is an embedded SDIO device, per definition it's not
> a removable card (MMC_CAP_NONREMOVABLE should be set for the
> corresponding mmc host), the SDIO card will stick around and instead
> the ->suspend|resume() callback needs to be implemented for the SDIO
> func driver.
If I follow what has been done in other drivers I would write something
like:
static int wfx_sdio_suspend(struct device *dev)
{
struct sdio_func *func = dev_to_sdio_func(dev);
struct wfx_sdio_priv *bus = sdio_get_drvdata(func);
config_reg_write_bits(bus->core, CFG_IRQ_ENABLE_DATA, 0);
// Necessary to keep device firmware in RAM
return sdio_set_host_pm_flags(func, MMC_PM_KEEP_POWER);
}
However, why not the implementation below?
static int wfx_sdio_suspend(struct device *dev)
{
struct sdio_func *func = dev_to_sdio_func(dev);
wfx_sdio_remove(func);
return 0;
}
In both cases, I worry to provide these functions without being able to
test them.
--
Jérôme Pouiller
Re: [PATCH v5 08/24] wfx: add bus_sdio.c
Hello Ulf,
On Monday 22 March 2021 13:20:35 CET Ulf Hansson wrote:
> On Mon, 15 Mar 2021 at 14:25, Jerome Pouiller
> wrote:
> >
> > From: Jérôme Pouiller
> >
> > Signed-off-by: Jérôme Pouiller
> > ---
> > drivers/net/wireless/silabs/wfx/bus_sdio.c | 259 +
> > 1 file changed, 259 insertions(+)
> > create mode 100644 drivers/net/wireless/silabs/wfx/bus_sdio.c
>
> [...]
>
> > +static const struct sdio_device_id wfx_sdio_ids[] = {
> > + { SDIO_DEVICE(SDIO_VENDOR_ID_SILABS, SDIO_DEVICE_ID_SILABS_WF200) },
> > + { },
> > +};
> > +MODULE_DEVICE_TABLE(sdio, wfx_sdio_ids);
> > +
> > +struct sdio_driver wfx_sdio_driver = {
> > + .name = "wfx-sdio",
> > + .id_table = wfx_sdio_ids,
> > + .probe = wfx_sdio_probe,
> > + .remove = wfx_sdio_remove,
> > + .drv = {
> > + .owner = THIS_MODULE,
> > + .of_match_table = wfx_sdio_of_match,
>
> It's not mandatory to support power management, like system
> suspend/resume. However, as this looks like this is a driver for an
> embedded SDIO device, you probably want this.
>
> If that is the case, please assign the dev_pm_ops here and implement
> the ->suspend|resume() callbacks.
I have no platform to test suspend/resume, so I have only a
theoretical understanding of this subject.
I understanding is that with the current implementation, the
device will be powered off on suspend and then totally reset
(including reloading of the firmware) on resume. I am wrong?
This behavior sounds correct to me. You would expect something
more?
--
Jérôme Pouiller
Re: [PATCH] wfx: fix irqf_oneshot.cocci warnings
Hello,
On Monday 15 March 2021 22:09:20 CET kernel test robot wrote:
>
> From: kernel test robot
>
> drivers/net/wireless/silabs/wfx/bus_sdio.c:134:8-33: ERROR: Threaded IRQ with
> no primary handler requested without IRQF_ONESHOT
>
> Since commit 1c6c69525b40 ("genirq: Reject bogus threaded irq requests")
> threaded IRQs without a primary handler need to be requested with
> IRQF_ONESHOT, otherwise the request will fail.
>
> So pass the IRQF_ONESHOT flag in this case.
>
> Generated by: scripts/coccinelle/misc/irqf_oneshot.cocci
>
> CC: Jérôme Pouiller
> Reported-by: kernel test robot
> Signed-off-by: kernel test robot
> ---
>
> url:
> https://github.com/0day-ci/linux/commits/Jerome-Pouiller/wfx-get-out-from-the-staging-area/20210315-212855
> base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
> b828324bba8f575fde487a91fec07303789dda8a
>
> bus_sdio.c |3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> --- a/drivers/net/wireless/silabs/wfx/bus_sdio.c
> +++ b/drivers/net/wireless/silabs/wfx/bus_sdio.c
> @@ -132,7 +132,8 @@ static int wfx_sdio_irq_subscribe(void *
> flags = IRQF_TRIGGER_HIGH;
> flags |= IRQF_ONESHOT;
> return devm_request_threaded_irq(&bus->func->dev, bus->of_irq, NULL,
> -wfx_sdio_irq_handler_ext, flags,
> +wfx_sdio_irq_handler_ext,
> +flags | IRQF_ONESHOT,
> "wfx", bus);
> }
>
>
Obviously, "flags" always contains IRQF_ONESHOT. So, it is a false positive.
--
Jérôme Pouiller
Re: [PATCH v5 03/24] wfx: add Makefile/Kconfig
Hi Leon, On Monday 15 March 2021 16:11:52 CET Leon Romanovsky wrote: > On Mon, Mar 15, 2021 at 02:24:40PM +0100, Jerome Pouiller wrote: > > From: Jérôme Pouiller > > > > Signed-off-by: Jérôme Pouiller > > --- > > drivers/net/wireless/silabs/wfx/Kconfig | 12 +++ > > drivers/net/wireless/silabs/wfx/Makefile | 26 > > 2 files changed, 38 insertions(+) > > create mode 100644 drivers/net/wireless/silabs/wfx/Kconfig > > create mode 100644 drivers/net/wireless/silabs/wfx/Makefile > > > > diff --git a/drivers/net/wireless/silabs/wfx/Kconfig > > b/drivers/net/wireless/silabs/wfx/Kconfig > > new file mode 100644 > > index ..3be4b1e735e1 > > --- /dev/null > > +++ b/drivers/net/wireless/silabs/wfx/Kconfig > > @@ -0,0 +1,12 @@ > > +config WFX > > + tristate "Silicon Labs wireless chips WF200 and further" > > + depends on MAC80211 > > + depends on MMC || !MMC # do not allow WFX=y if MMC=m > > + depends on (SPI || MMC) > > + help > > + This is a driver for Silicons Labs WFxxx series (WF200 and further) > > + chipsets. This chip can be found on SPI or SDIO buses. > > + > > + Silabs does not use a reliable SDIO vendor ID. So, to avoid > > conflicts, > > + the driver won't probe the device if it is not also declared in the > > + Device Tree. > > diff --git a/drivers/net/wireless/silabs/wfx/Makefile > > b/drivers/net/wireless/silabs/wfx/Makefile > > new file mode 100644 > > index ..f399962c8619 > > --- /dev/null > > +++ b/drivers/net/wireless/silabs/wfx/Makefile > > @@ -0,0 +1,26 @@ > > +# SPDX-License-Identifier: GPL-2.0 > > + > > +# Necessary for CREATE_TRACE_POINTS > > +CFLAGS_debug.o = -I$(src) > > I wonder if it is still relevant outside of the staging tree. It seems this pattern is common in the main tree. You suggest to relocate trace.h to include/trace/events? -- Jérôme Pouiller
Re: [PATCH] staging: wfx: remove unused included header files
On Tuesday 9 March 2021 14:07:43 CET Greg KH wrote:
> On Thu, Mar 04, 2021 at 10:43:45AM +0100, Jérôme Pouiller wrote:
> > Hello Greg,
> >
> > On Tuesday 2 March 2021 16:01:25 CET Jérôme Pouiller wrote:
> > > Hello Muhammad,
> > >
> > > Sorry, I am a bit late for the review of this patch. Thank you for
your
> > > contribution.
> > >
> > > On Thursday 11 February 2021 15:36:37 CET Muhammad Usama Anjum
wrote:
> > > >
> > > > Many header files have been included, but never used. Those
header
> > > > files have been removed.
> > > >
> > > > Signed-off-by: Muhammad Usama Anjum
> > > > ---
> > > > drivers/staging/wfx/bh.c | 1 -
> > > > drivers/staging/wfx/bh.h | 4
> > > > drivers/staging/wfx/bus.h | 3 ---
> > > > drivers/staging/wfx/bus_sdio.c| 6 --
> > > > drivers/staging/wfx/bus_spi.c | 7 ---
> > > > drivers/staging/wfx/data_rx.c | 5 -
> > > > drivers/staging/wfx/data_tx.c | 5 -
> > > > drivers/staging/wfx/data_tx.h | 3 ---
> > > > drivers/staging/wfx/debug.c | 6 --
> > > > drivers/staging/wfx/fwio.c| 2 --
> > > > drivers/staging/wfx/hif_api_cmd.h | 4
> > > > drivers/staging/wfx/hif_api_general.h | 9 -
> > > > drivers/staging/wfx/hif_tx.c | 4
> > > > drivers/staging/wfx/hif_tx_mib.c | 5 -
> > > > drivers/staging/wfx/hwio.c| 3 ---
> > > > drivers/staging/wfx/hwio.h| 2 --
> > > > drivers/staging/wfx/key.c | 2 --
> > > > drivers/staging/wfx/key.h | 2 --
> > > > drivers/staging/wfx/main.c| 7 ---
> > > > drivers/staging/wfx/main.h| 3 ---
> > > > drivers/staging/wfx/queue.c | 4
> > > > drivers/staging/wfx/queue.h | 3 ---
> > > > drivers/staging/wfx/scan.h| 2 --
> > > > drivers/staging/wfx/sta.c | 6 --
> > > > drivers/staging/wfx/sta.h | 2 --
> > > > drivers/staging/wfx/traces.h | 3 ---
> > > > drivers/staging/wfx/wfx.h | 3 ---
> > > > 27 files changed, 106 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/wfx/bh.c b/drivers/staging/wfx/bh.c
> > > > index ed53d0b45592..cd6bcfdfbe9a 100644
> > > > --- a/drivers/staging/wfx/bh.c
> > > > +++ b/drivers/staging/wfx/bh.c
> > > > @@ -5,7 +5,6 @@
> > > > * Copyright (c) 2017-2020, Silicon Laboratories, Inc.
> > > > * Copyright (c) 2010, ST-Ericsson
> > > > */
> > > > -#include
> > > > #include
> > >
> > > Though bh.c refers to gpiod_set_value_cansleep()
> > >
> > >
> > > > #include "bh.h"
> > > > diff --git a/drivers/staging/wfx/bh.h b/drivers/staging/wfx/bh.h
> > > > index 78c49329e22a..92ef3298d4ac 100644
> > > > --- a/drivers/staging/wfx/bh.h
> > > > +++ b/drivers/staging/wfx/bh.h
> > > > @@ -8,10 +8,6 @@
> > > > #ifndef WFX_BH_H
> > > > #define WFX_BH_H
> > > >
> > > > -#include
> > > > -#include
> > > > -#include
> > > > -
> > > > struct wfx_dev;
> > > >
> > > > struct wfx_hif {
> > >
> > > Ditto, bh.h refers to atomic_t, struct work_struct and struct
> > > completion. If you try to compile bh.h alone (with something like
> > > gcc -xc .../bh.h) it won't work.
> > >
> > > Maybe it works now because we are lucky in the order the headers
are
> > > included, but I think it is not sufficient.
> > >
> > > [... same problem repeats multiple times in the following ...]
> > >
> >
> > Greg, if nobody has any opinion on that, I think that this patch
should
> > be reverted.
>
> Nothing is breaking, why should it be reverted?
Because it is less maintainable?
> You never build a .h file alone :)
hmmm... notwithstanding, I thing that the code should not depend on the
order the headers are included. You don't?
> Anyway, sure, I'll revert it, what is the commit id?
commit 314fd52f01ea "staging: wfx: remove unused included header files"
Thank you!
--
Jérôme Pouiller
Re: [PATCH] staging: wfx: remove unused included header files
Hello Greg,
On Tuesday 2 March 2021 16:01:25 CET Jérôme Pouiller wrote:
> Hello Muhammad,
>
> Sorry, I am a bit late for the review of this patch. Thank you for your
> contribution.
>
> On Thursday 11 February 2021 15:36:37 CET Muhammad Usama Anjum wrote:
> >
> > Many header files have been included, but never used. Those header
> > files have been removed.
> >
> > Signed-off-by: Muhammad Usama Anjum
> > ---
> > drivers/staging/wfx/bh.c | 1 -
> > drivers/staging/wfx/bh.h | 4
> > drivers/staging/wfx/bus.h | 3 ---
> > drivers/staging/wfx/bus_sdio.c| 6 --
> > drivers/staging/wfx/bus_spi.c | 7 ---
> > drivers/staging/wfx/data_rx.c | 5 -
> > drivers/staging/wfx/data_tx.c | 5 -
> > drivers/staging/wfx/data_tx.h | 3 ---
> > drivers/staging/wfx/debug.c | 6 --
> > drivers/staging/wfx/fwio.c| 2 --
> > drivers/staging/wfx/hif_api_cmd.h | 4
> > drivers/staging/wfx/hif_api_general.h | 9 -
> > drivers/staging/wfx/hif_tx.c | 4
> > drivers/staging/wfx/hif_tx_mib.c | 5 -
> > drivers/staging/wfx/hwio.c| 3 ---
> > drivers/staging/wfx/hwio.h| 2 --
> > drivers/staging/wfx/key.c | 2 --
> > drivers/staging/wfx/key.h | 2 --
> > drivers/staging/wfx/main.c| 7 ---
> > drivers/staging/wfx/main.h| 3 ---
> > drivers/staging/wfx/queue.c | 4
> > drivers/staging/wfx/queue.h | 3 ---
> > drivers/staging/wfx/scan.h| 2 --
> > drivers/staging/wfx/sta.c | 6 --
> > drivers/staging/wfx/sta.h | 2 --
> > drivers/staging/wfx/traces.h | 3 ---
> > drivers/staging/wfx/wfx.h | 3 ---
> > 27 files changed, 106 deletions(-)
> >
> > diff --git a/drivers/staging/wfx/bh.c b/drivers/staging/wfx/bh.c
> > index ed53d0b45592..cd6bcfdfbe9a 100644
> > --- a/drivers/staging/wfx/bh.c
> > +++ b/drivers/staging/wfx/bh.c
> > @@ -5,7 +5,6 @@
> > * Copyright (c) 2017-2020, Silicon Laboratories, Inc.
> > * Copyright (c) 2010, ST-Ericsson
> > */
> > -#include
> > #include
>
> Though bh.c refers to gpiod_set_value_cansleep()
>
>
> > #include "bh.h"
> > diff --git a/drivers/staging/wfx/bh.h b/drivers/staging/wfx/bh.h
> > index 78c49329e22a..92ef3298d4ac 100644
> > --- a/drivers/staging/wfx/bh.h
> > +++ b/drivers/staging/wfx/bh.h
> > @@ -8,10 +8,6 @@
> > #ifndef WFX_BH_H
> > #define WFX_BH_H
> >
> > -#include
> > -#include
> > -#include
> > -
> > struct wfx_dev;
> >
> > struct wfx_hif {
>
> Ditto, bh.h refers to atomic_t, struct work_struct and struct
> completion. If you try to compile bh.h alone (with something like
> gcc -xc .../bh.h) it won't work.
>
> Maybe it works now because we are lucky in the order the headers are
> included, but I think it is not sufficient.
>
> [... same problem repeats multiple times in the following ...]
>
Greg, if nobody has any opinion on that, I think that this patch should
be reverted.
--
Jérôme Pouiller
Re: [PATCH] staging: wfx: remove unused included header files
Hello Muhammad,
Sorry, I am a bit late for the review of this patch. Thank you for your
contribution.
On Thursday 11 February 2021 15:36:37 CET Muhammad Usama Anjum wrote:
>
> Many header files have been included, but never used. Those header
> files have been removed.
>
> Signed-off-by: Muhammad Usama Anjum
> ---
> drivers/staging/wfx/bh.c | 1 -
> drivers/staging/wfx/bh.h | 4
> drivers/staging/wfx/bus.h | 3 ---
> drivers/staging/wfx/bus_sdio.c| 6 --
> drivers/staging/wfx/bus_spi.c | 7 ---
> drivers/staging/wfx/data_rx.c | 5 -
> drivers/staging/wfx/data_tx.c | 5 -
> drivers/staging/wfx/data_tx.h | 3 ---
> drivers/staging/wfx/debug.c | 6 --
> drivers/staging/wfx/fwio.c| 2 --
> drivers/staging/wfx/hif_api_cmd.h | 4
> drivers/staging/wfx/hif_api_general.h | 9 -
> drivers/staging/wfx/hif_tx.c | 4
> drivers/staging/wfx/hif_tx_mib.c | 5 -
> drivers/staging/wfx/hwio.c| 3 ---
> drivers/staging/wfx/hwio.h| 2 --
> drivers/staging/wfx/key.c | 2 --
> drivers/staging/wfx/key.h | 2 --
> drivers/staging/wfx/main.c| 7 ---
> drivers/staging/wfx/main.h| 3 ---
> drivers/staging/wfx/queue.c | 4
> drivers/staging/wfx/queue.h | 3 ---
> drivers/staging/wfx/scan.h| 2 --
> drivers/staging/wfx/sta.c | 6 --
> drivers/staging/wfx/sta.h | 2 --
> drivers/staging/wfx/traces.h | 3 ---
> drivers/staging/wfx/wfx.h | 3 ---
> 27 files changed, 106 deletions(-)
>
> diff --git a/drivers/staging/wfx/bh.c b/drivers/staging/wfx/bh.c
> index ed53d0b45592..cd6bcfdfbe9a 100644
> --- a/drivers/staging/wfx/bh.c
> +++ b/drivers/staging/wfx/bh.c
> @@ -5,7 +5,6 @@
> * Copyright (c) 2017-2020, Silicon Laboratories, Inc.
> * Copyright (c) 2010, ST-Ericsson
> */
> -#include
> #include
Though bh.c refers to gpiod_set_value_cansleep()
> #include "bh.h"
> diff --git a/drivers/staging/wfx/bh.h b/drivers/staging/wfx/bh.h
> index 78c49329e22a..92ef3298d4ac 100644
> --- a/drivers/staging/wfx/bh.h
> +++ b/drivers/staging/wfx/bh.h
> @@ -8,10 +8,6 @@
> #ifndef WFX_BH_H
> #define WFX_BH_H
>
> -#include
> -#include
> -#include
> -
> struct wfx_dev;
>
> struct wfx_hif {
Ditto, bh.h refers to atomic_t, struct work_struct and struct
completion. If you try to compile bh.h alone (with something like
gcc -xc .../bh.h) it won't work.
Maybe it works now because we are lucky in the order the headers are
included, but I think it is not sufficient.
[... same problem repeats multiple times in the following ...]
--
Jérôme Pouiller
Re: [PATCH] staging: wfx: avoid defining array of flexible struct
On Thursday 11 February 2021 11:50:26 CET Muhammad Usama Anjum wrote:
>
> In this particular case, the struct element is already flexible struct.
> Thus struct element ie[] is ambiguous inside another struct. The members
> of struct element ie aren't being accessed in code anywhere. The data of
> u8 type is copied in it. So it has been changed to u8 ie[] to make the
> sparse happy and code simple.
You may also mention that the driver code does not care of the type of
the ie attribute. It is only accessed from one place with memcpy().
>
> Warning from sparse:
> drivers/stagingwfx/hif_tx.c: note: in included file (through
> drivers/stagingwfx/data_tx.h, drivers/staging//wfx/wfx.h):
> drivers/staging//wfx/hif_api_cmd.h:103:26: warning: array of flexible
> structures
>
> Signed-off-by: Muhammad Usama Anjum
> ---
> drivers/staging/wfx/hif_api_cmd.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/wfx/hif_api_cmd.h
> b/drivers/staging/wfx/hif_api_cmd.h
> index 11bc1a58edae..58c9bb036011 100644
> --- a/drivers/staging/wfx/hif_api_cmd.h
> +++ b/drivers/staging/wfx/hif_api_cmd.h
> @@ -100,7 +100,7 @@ struct hif_req_update_ie {
> u8 reserved1:5;
> u8 reserved2;
> __le16 num_ies;
> - struct element ie[];
> + u8 ie[];
> } __packed;
>
> struct hif_cnf_update_ie {
> --
> 2.25.1
>
>
I think that "#include " is no more necessary.
Else:
Reviewed-by: Jérôme Pouiller
--
Jérôme Pouiller
Re: [PATCH v3 05/24] wfx: add main.c/main.h
On Tuesday 22 December 2020 16:44:05 CET Kalle Valo wrote:
> Jerome Pouiller writes:
>
> > +/* NOTE: wfx_send_pds() destroy buf */
> > +int wfx_send_pds(struct wfx_dev *wdev, u8 *buf, size_t len)
> > +{
> > + int ret;
> > + int start, brace_level, i;
> > +
> > + start = 0;
> > + brace_level = 0;
> > + if (buf[0] != '{') {
> > + dev_err(wdev->dev, "valid PDS start with '{'. Did you forget to
> > compress it?\n");
> > + return -EINVAL;
> > + }
> > + for (i = 1; i < len - 1; i++) {
> > + if (buf[i] == '{')
> > + brace_level++;
> > + if (buf[i] == '}')
> > + brace_level--;
> > + if (buf[i] == '}' && !brace_level) {
> > + i++;
> > + if (i - start + 1 > WFX_PDS_MAX_SIZE)
> > + return -EFBIG;
> > + buf[start] = '{';
> > + buf[i] = 0;
> > + dev_dbg(wdev->dev, "send PDS '%s}'\n", buf + start);
> > + buf[i] = '}';
> > + ret = hif_configuration(wdev, buf + start,
> > + i - start + 1);
> > + if (ret > 0) {
> > + dev_err(wdev->dev, "PDS bytes %d to %d: invalid data (unsupported
> > options?)\n",
> > + start, i);
> > + return -EINVAL;
> > + }
> > + if (ret == -ETIMEDOUT) {
> > + dev_err(wdev->dev, "PDS bytes %d to %d: chip didn't reply (corrupted
> > file?)\n",
> > + start, i);
> > + return ret;
> > + }
> > + if (ret) {
> > + dev_err(wdev->dev, "PDS bytes %d to %d: chip returned an unknown
> > error\n",
> > + start, i);
> > + return -EIO;
> > + }
> > + buf[i] = ',';
> > + start = i;
> > + }
> > + }
> > + return 0;
> > +}
>
> What does this function do? Looks very strange.
I am going to add this comment:
The device need data about the antenna configuration. This information in
provided by PDS (Platform Data Set, this is the wording used in WF200
documentation) files. For hardware integrators, the full process to create
PDS files is described here:
https://github.com/SiliconLabs/wfx-firmware/blob/master/PDS/README.md
So this function aims to send PDS to the device. However, the PDS file is
often bigger than Rx buffers of the chip, so it has to be sent in multiple
parts.
In add, the PDS data cannot be split anywhere. The PDS files contains tree
structures. Braces are used to enter/leave a level of the tree (in a JSON
fashion). PDS files can only been split between root nodes.
--
Jérôme Pouiller
Re: [PATCH v3 09/24] wfx: add hwio.c/hwio.h
On Tuesday 22 December 2020 16:27:01 CET Greg Kroah-Hartman wrote: > On Tue, Dec 22, 2020 at 05:10:11PM +0200, Kalle Valo wrote: > > Jerome Pouiller writes: > > > > > +/* > > > + * Internal helpers. > > > + * > > > + * About CONFIG_VMAP_STACK: > > > + * When CONFIG_VMAP_STACK is enabled, it is not possible to run DMA on > > > stack > > > + * allocated data. Functions below that work with registers (aka > > > functions > > > + * ending with "32") automatically reallocate buffers with kmalloc. > > > However, > > > + * functions that work with arbitrary length buffers let's caller to > > > handle > > > + * memory location. In doubt, enable CONFIG_DEBUG_SG to detect badly > > > located > > > + * buffer. > > > + */ > > > > This sounds very hacky to me, I have understood that you should never > > use stack with DMA. > > You should never do that because some platforms do not support it, so no > driver should ever try to do that as they do not know what platform they > are running on. Just to be curious, why these platforms don't support DMA in a stack allocated area? If the memory is contiguous (= not vmalloced), correctly aligned and in the first 4GB of physical memory, it should be sufficient, shouldn't? -- Jérôme Pouiller
Re: [PATCH v3 03/24] wfx: add Makefile/Kconfig
On Tuesday 22 December 2020 16:02:38 CET Kalle Valo wrote:
> Jerome Pouiller writes:
>
> > From: Jérôme Pouiller
> >
> > Signed-off-by: Jérôme Pouiller
>
> [...]
>
> > +wfx-$(CONFIG_SPI) += bus_spi.o
> > +wfx-$(subst m,y,$(CONFIG_MMC)) += bus_sdio.o
>
> Why this subst? And why only for MMC?
CONFIG_SPI is a boolean (y or empty). The both values make senses.
CONFIG_MMC is a tristate (y, m or empty). The substitution above
ensure that bus_sdio.o will included in wfx.ko if CONFIG_MMC is 'm'
("wfx-$(CONFIG_MMC) += bus_sdio.o" wouldn't make the job).
You may want to know what it happens if CONFIG_MMC=m while CONFIG_WFX=y.
This line in Kconfig prevents to compile wfx statically if MMC is a
module:
depends on MMC || !MMC # do not allow WFX=y if MMC=m
--
Jérôme Pouiller
Re: [PATCH v3 12/24] wfx: add hif_api_*.h
On Tuesday 22 December 2020 16:20:46 CET Kalle Valo wrote: > Jerome Pouiller writes: > > > --- /dev/null > > +++ b/drivers/net/wireless/silabs/wfx/hif_api_general.h > > @@ -0,0 +1,267 @@ > > +/* SPDX-License-Identifier: Apache-2.0 */ > > +/* > > + * WFx hardware interface definitions > > + * > > + * Copyright (c) 2018-2020, Silicon Laboratories Inc. > > + */ > > + > > +#ifndef WFX_HIF_API_GENERAL_H > > +#define WFX_HIF_API_GENERAL_H > > + > > +#ifdef __KERNEL__ > > +#include > > +#include > > +#else > > +#include > > +#include > > +#define __packed __attribute__((__packed__)) > > +#endif > > Why check for __KERNEL__ and redefined __packed? These don't belong to a > wireless driver. In the old days, this file was shared with other projects. I though I had cleaned all these things. -- Jérôme Pouiller
Re: [PATCH v3 09/24] wfx: add hwio.c/hwio.h
On Tuesday 22 December 2020 16:27:01 CET Greg Kroah-Hartman wrote: > > On Tue, Dec 22, 2020 at 05:10:11PM +0200, Kalle Valo wrote: > > Jerome Pouiller writes: > > > > > +/* > > > + * Internal helpers. > > > + * > > > + * About CONFIG_VMAP_STACK: > > > + * When CONFIG_VMAP_STACK is enabled, it is not possible to run DMA on > > > stack > > > + * allocated data. Functions below that work with registers (aka > > > functions > > > + * ending with "32") automatically reallocate buffers with kmalloc. > > > However, > > > + * functions that work with arbitrary length buffers let's caller to > > > handle > > > + * memory location. In doubt, enable CONFIG_DEBUG_SG to detect badly > > > located > > > + * buffer. > > > + */ > > > > This sounds very hacky to me, I have understood that you should never > > use stack with DMA. > > You should never do that because some platforms do not support it, so no > driver should ever try to do that as they do not know what platform they > are running on. Yes, I have learned this rule the hard way. There is no better way than a comment to warn the user that the argument will be used with a DMA? A Sparse annotation, for example? -- Jérôme Pouiller
Re: [PATCH] Staging: silabs si4455 serial driver
+
> + ret = si4455_send_command_get_response(port,
> + sizeof(dataOut),
> + dataOut,
> + sizeof(dataIn),
> + dataIn);
Why not:
ret = si4455_send_command_get_response(port,
sizeof(*result), result,
sizeof(dataIn), dataIn);
> + if (ret == 0) {
> + result->CHIPREV = dataIn[0];
> + memcpy(&result->PART, &dataIn[1],sizeof(result->PART));
> + result->PBUILD = dataIn[3];
> + memcpy(&result->ID, &dataIn[4], sizeof(result->ID));
> + result->CUSTOMER = dataIn[6];
> + result->ROMID = dataIn[7];
> + result->BOND = dataIn[8];
... it would avoid all these lines.
> + } else {
> + dev_err(port->dev,
> + "%s: si4455_send_command_get_response error(%i)",
> + __func__,
> + ret);
> + }
> + return ret;
> +}
[...]
--
Jérôme Pouiller
Re: [PATCH v3 02/24] dt-bindings: introduce silabs,wfx.yaml
On Wednesday 4 November 2020 20:15:54 CET Rob Herring wrote: > On Wed, 04 Nov 2020 16:51:45 +0100, Jerome Pouiller wrote: > > From: Jérôme Pouiller > > > > Signed-off-by: Jérôme Pouiller > > --- > > .../bindings/net/wireless/silabs,wfx.yaml | 131 ++ > > 1 file changed, 131 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/net/wireless/silabs,wfx.yaml > > > > > My bot found errors running 'make dt_binding_check' on your patch: > > yamllint warnings/errors: > > dtschema/dtc warnings/errors: > /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/net/wireless/silabs,wfx.yaml: > 'additionalProperties' is a required property > /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/net/wireless/silabs,wfx.yaml: > ignoring, error in schema: > warning: no schema found in file: > ./Documentation/devicetree/bindings/net/wireless/silabs,wfx.yaml > > > See https://patchwork.ozlabs.org/patch/1394182 > > The base for the patch is generally the last rc1. Any dependencies > should be noted. > > If you already ran 'make dt_binding_check' and didn't see the above > error(s), then make sure 'yamllint' is installed and dt-schema is up to > date: > > pip3 install dtschema --upgrade Weird, I don't have any error. Yet, yamllint is installed (1.24.2-1) and pip says that dts-schema is up-to-date (2020.8.2.dev2+gd63b653). I have also tried after rebased on v5.10-rc2, then on v5.10-rc1 without success. -- Jérôme Pouiller
Re: [PATCH] staging: wfx: avoid uninitialized variable use
On Monday 26 October 2020 17:02:22 CET Arnd Bergmann wrote:
>
> From: Arnd Bergmann
>
> Move the added check of the 'band' variable after the
> initialization. Pointed out by clang with
>
> drivers/staging/wfx/data_tx.c:34:19: warning: variable 'band' is
> uninitialized when used here [-Wuninitialized]
> if (rate->idx >= band->n_bitrates) {
Hello Arnd,
This patch has already been submitted[1]. I think it is going to be
applied to staging very soon.
Sorry for the disturbing.
[1]
https://lore.kernel.org/driverdev-devel/20201019160604.1609180-1-jerome.pouil...@silabs.com/
--
Jérôme Pouiller
Re: [PATCH] staging: wfx: make a const array static, makes object smaller
On Monday 19 October 2020 10:09:19 CEST David Laight wrote: > From: Joe Perches > > Sent: 17 October 2020 01:12 > > > > On Fri, 2020-10-16 at 23:33 +0100, Colin King wrote: > > > From: Colin Ian King > > > > > > Don't populate const array filter_ies on the stack but instead > > > make it static. Makes the object code smaller by 261 bytes. > > > > > > Before: > > >textdata bss dec hex filename > > > 216743166 448 2528862c8 drivers/staging/wfx/sta.o > > > > > > After: > > >textdata bss dec hex filename > > > 213493230 448 2502761c3 drivers/staging/wfx/sta.o > > > > Thanks. > > > > It's odd to me it's so large a change as it's only > > 24 bytes of initialization. (3 entries, each 8 bytes) > > Perhaps the 'stack protector' crap? > > Interestingly, loading the data from the 'readonly' section > is probably a data cache miss. > Which might end up being slower than the extra code to > update the on-stack data. > The extra code might get prefetched... I had never realized the difference between "const" and "static const" in this case. With my gcc fro arm, the output of "objdump -h sta.o" gives: Before: 0 .text 19fc 0034 2**2 CONTENTS, ALLOC, LOAD, READONLY, CODE [...] 7 .rodata 0015 1e78 2**2 CONTENTS, ALLOC, LOAD, READONLY, DATA After: 0 .text 1974 0034 2**2 CONTENTS, ALLOC, LOAD, READONLY, CODE [...] 7 .rodata 002d 1dd4 2**2 CONTENTS, ALLOC, LOAD, READONLY, DATA The difference of .rodata is exactly what is expected (24 bytes) and we save 115 bytes of code. Reviewed-by: Jérôme Pouiller -- Jérôme Pouiller
Re: [PATCH 07/23] wfx: add bus_sdio.c
Hello Ulf,
On Friday 16 October 2020 13:30:30 CEST Ulf Hansson wrote:
> On Mon, 12 Oct 2020 at 12:47, Jerome Pouiller
> wrote:
> >
> > From: Jérôme Pouiller
>
> Please fill out this commit message to explain a bit more about the
> patch and the HW it enables support for.
This patch belongs to a series[1] that will squashed before to be
committed (Kalle Valo prefer to process like that for this review). So,
I didn't bother to write real commit messages. For the v2, I will take
care to add linux-mmc in copy of the whole series.
[1]
https://lore.kernel.org/lkml/20201012104648.985256-1-jerome.pouil...@silabs.com/
> > Signed-off-by: Jérôme Pouiller
> > ---
> > drivers/net/wireless/silabs/wfx/bus_sdio.c | 269 +
> > 1 file changed, 269 insertions(+)
> > create mode 100644 drivers/net/wireless/silabs/wfx/bus_sdio.c
> >
> > diff --git a/drivers/net/wireless/silabs/wfx/bus_sdio.c
> > b/drivers/net/wireless/silabs/wfx/bus_sdio.c
> > new file mode 100644
> > index ..e06d7e1ebe9c
[...]
> > +struct sdio_driver wfx_sdio_driver = {
> > + .name = "wfx-sdio",
> > + .id_table = wfx_sdio_ids,
> > + .probe = wfx_sdio_probe,
> > + .remove = wfx_sdio_remove,
> > + .drv = {
> > + .owner = THIS_MODULE,
> > + .of_match_table = wfx_sdio_of_match,
> > + }
> > +};
>
> I couldn't find where you call sdio_register|unregister_driver(), but
> maybe that's done from another patch in series?
Indeed, it is here[2].
[2]
https://lore.kernel.org/lkml/20201012104648.985256-5-jerome.pouil...@silabs.com/
--
Jérôme Pouiller
Re: [PATCH 07/23] wfx: add bus_sdio.c
On Wednesday 14 October 2020 14:43:34 CEST Pali Rohár wrote:
> On Wednesday 14 October 2020 13:52:15 Jérôme Pouiller wrote:
> > On Tuesday 13 October 2020 22:11:56 CEST Pali Rohár wrote:
> > > On Monday 12 October 2020 12:46:32 Jerome Pouiller wrote:
> > > > +#define SDIO_VENDOR_ID_SILABS0x
> > > > +#define SDIO_DEVICE_ID_SILABS_WF200 0x1000
> > > > +static const struct sdio_device_id wfx_sdio_ids[] = {
> > > > + { SDIO_DEVICE(SDIO_VENDOR_ID_SILABS, SDIO_DEVICE_ID_SILABS_WF200)
> > > > },
> > >
> > > Please move ids into common include file include/linux/mmc/sdio_ids.h
> > > where are all SDIO ids. Now all drivers have ids defined in that file.
> > >
> > > > + // FIXME: ignore VID/PID and only rely on device tree
> > > > + // { SDIO_DEVICE(SDIO_ANY_ID, SDIO_ANY_ID) },
> > >
> > > What is the reason for ignoring vendor and device ids?
> >
> > The device has a particularity, its VID/PID is :1000 (as you can see
> > above). This value is weird. The risk of collision with another device is
> > high.
>
> Those ids looks strange. You are from Silabs, can you check internally
> in Silabs if ids are really correct? And which sdio vendor id you in
> Silabs got assigned for your products?
I confirm these ids are the ones burned in the WF200. We have to deal with
that :( .
> I know that sdio devices with multiple functions may have different sdio
> vendor/device id particular function and in common CIS (function 0).
>
> Could not be a problem that on one place is vendor/device id correct and
> on other place is that strange value?
>
> I have sent following patch (now part of upstream kernel) which exports
> these ids to userspace:
> https://lore.kernel.org/linux-mmc/20200527110858.17504-2-p...@kernel.org/T/#u
>
> Also for debugging ids and information about sdio cards, I sent another
> patch which export additional data:
> https://lore.kernel.org/linux-mmc/20200727133837.19086-1-p...@kernel.org/T/#u
>
> Could you try them and look at /sys/class/mmc_host/ attribute outputs?
Here is:
# cd /sys/class/mmc_host/ && grep -r . mmc1/
mmc1/power/runtime_suspended_time:0
grep: mmc1/power/autosuspend_delay_ms: Input/output error
mmc1/power/runtime_active_time:0
mmc1/power/control:auto
mmc1/power/runtime_status:unsupported
mmc1/mmc1:0001/vendor:0x
mmc1/mmc1:0001/rca:0x0001
mmc1/mmc1:0001/device:0x1000
mmc1/mmc1:0001/mmc1:0001:1/vendor:0x
mmc1/mmc1:0001/mmc1:0001:1/device:0x1000
grep: mmc1/mmc1:0001/mmc1:0001:1/info4: No data available
mmc1/mmc1:0001/mmc1:0001:1/power/runtime_suspended_time:0
grep: mmc1/mmc1:0001/mmc1:0001:1/power/autosuspend_delay_ms: Input/output
error
mmc1/mmc1:0001/mmc1:0001:1/power/runtime_active_time:0
mmc1/mmc1:0001/mmc1:0001:1/power/control:auto
mmc1/mmc1:0001/mmc1:0001:1/power/runtime_status:unsupported
mmc1/mmc1:0001/mmc1:0001:1/class:0x00
grep: mmc1/mmc1:0001/mmc1:0001:1/info2: No data available
mmc1/mmc1:0001/mmc1:0001:1/modalias:sdio:c00vd1000
mmc1/mmc1:0001/mmc1:0001:1/revision:0.0
mmc1/mmc1:0001/mmc1:0001:1/uevent:OF_NAME=mmc
mmc1/mmc1:0001/mmc1:0001:1/uevent:OF_FULLNAME=/soc/sdhci@7e30/mmc@1
mmc1/mmc1:0001/mmc1:0001:1/uevent:OF_COMPATIBLE_0=silabs,wfx-sdio
mmc1/mmc1:0001/mmc1:0001:1/uevent:OF_COMPATIBLE_N=1
mmc1/mmc1:0001/mmc1:0001:1/uevent:SDIO_CLASS=00
mmc1/mmc1:0001/mmc1:0001:1/uevent:SDIO_ID=:1000
mmc1/mmc1:0001/mmc1:0001:1/uevent:SDIO_REVISION=0.0
mmc1/mmc1:0001/mmc1:0001:1/uevent:MODALIAS=sdio:c00vd1000
grep: mmc1/mmc1:0001/mmc1:0001:1/info3: No data available
grep: mmc1/mmc1:0001/mmc1:0001:1/info1: No data available
mmc1/mmc1:0001/ocr:0x0020
grep: mmc1/mmc1:0001/info4: No data available
mmc1/mmc1:0001/power/runtime_suspended_time:0
grep: mmc1/mmc1:0001/power/autosuspend_delay_ms: Input/output error
mmc1/mmc1:0001/power/runtime_active_time:0
mmc1/mmc1:0001/power/control:auto
mmc1/mmc1:0001/power/runtime_status:unsupported
grep: mmc1/mmc1:0001/info2: No data available
mmc1/mmc1:0001/type:SDIO
mmc1/mmc1:0001/revision:0.0
mmc1/mmc1:0001/uevent:MMC_TYPE=SDIO
mmc1/mmc1:0001/uevent:SDIO_ID=:1000
mmc1/mmc1:0001/uevent:SDIO_REVISION=0.0
grep: mmc1/mmc1:0001/info3: No data available
grep: mmc1/mmc1:0001/info1: No data available
--
Jérôme Pouiller
Re: [PATCH 01/23] dt-bindings: introduce silabs,wfx.yaml
On Tuesday 13 October 2020 18:49:35 CEST Rob Herring wrote: > On Mon, Oct 12, 2020 at 12:46:26PM +0200, Jerome Pouiller wrote: > > From: Jérôme Pouiller [...] > > + Note that in add of the properties below, the WFx driver also supports > > + `mac-address` and `local-mac-address` as described in > > + Documentation/devicetree/bindings/net/ethernet.txt > > Note what ethernet.txt contains... This should have a $ref to > ethernet-controller.yaml to express the above. > > You can add 'mac-address: true' if you want to be explicit about what > properties are used. Here, only mac-address and local-mac-address are supported. So, would the code below do the job? local-mac-address: $ref: ethernet-controller.yaml#/properties/local-mac-address mac-address: $ref: ethernet-controller.yaml#/properties/mac-address [...] > > + spi-max-frequency: > > +description: (SPI only) Maximum SPI clocking speed of device in Hz. > > No need to redefine a common property. When a property is specific to a bus, I would have like to explicitly say it. That's why I redefined the description. [...] > > + config-file: > > +description: Use an alternative file as PDS. Default is `wf200.pds`. > > Only > > + necessary for development/debug purpose. > > 'firmware-name' is typically what we'd use here. Though if just for > debug/dev, perhaps do a debugfs interface for this instead. As DT should > come from the firmware/bootloader, requiring changing the DT for > dev/debug is not the easiest workflow compared to doing something from > userspace. This file is not a firmware. It mainly contains data related to the antenna. At the beginning, this property has been added for development. With the time, I think it can be used to have one disk image for several devices that differ only in antenna. I am going to remove the part about development/debug purpose. [...] > Will need additionalProperties or unevaluatedProperties depending on > whether you list out properties from ethernet-controller.yaml or not. I think I need to specify "additionalProperties: true" since the user can also use properties defined for the SPI devices. In fact, I would like to write something like: allOf: $ref: spi-controller.yaml#/patternProperties/^.*@[0-9a-f]+$/properties -- Jérôme Pouiller
Re: [PATCH 07/23] wfx: add bus_sdio.c
Hello Pali,
On Tuesday 13 October 2020 22:11:56 CEST Pali Rohár wrote:
> Hello!
>
> On Monday 12 October 2020 12:46:32 Jerome Pouiller wrote:
> > +#define SDIO_VENDOR_ID_SILABS0x
> > +#define SDIO_DEVICE_ID_SILABS_WF200 0x1000
> > +static const struct sdio_device_id wfx_sdio_ids[] = {
> > + { SDIO_DEVICE(SDIO_VENDOR_ID_SILABS, SDIO_DEVICE_ID_SILABS_WF200) },
>
> Please move ids into common include file include/linux/mmc/sdio_ids.h
> where are all SDIO ids. Now all drivers have ids defined in that file.
>
> > + // FIXME: ignore VID/PID and only rely on device tree
> > + // { SDIO_DEVICE(SDIO_ANY_ID, SDIO_ANY_ID) },
>
> What is the reason for ignoring vendor and device ids?
The device has a particularity, its VID/PID is :1000 (as you can see
above). This value is weird. The risk of collision with another device is
high.
So, maybe the device should be probed only if it appears in the DT. Since
WF200 targets embedded platforms, I don't think it is a problem to rely on
DT. You will find another FIXME further in the code about that:
+ dev_warn(&func->dev,
+"device is not declared in DT, features will be
limited\n");
+ // FIXME: ignore VID/PID and only rely on device tree
+ // return -ENODEV;
However, it wouldn't be usual way to manage SDIO devices (and it is the
reason why the code is commented out).
Anyway, if we choose to rely on the DT, should we also check the VID/PID?
Personally, I am in favor to probe the device only if VID/PID match and if
a DT node is found, even if it is not the usual way.
--
Jérôme Pouiller
Re: [PATCH] staging: wfx: Spacing and alignment cleanup
On Monday 12 October 2020 10:26:30 CEST izabela.bakoll...@gmail.com wrote:
> From: Izabela Bakollari
>
> This patch fixes minor issue with spacing and alignment.
>
> checkpatch message:
> CHECK: Alignment should match open parenthesis
>
> Signed-off-by: Izabela Bakollari
> ---
> drivers/staging/wfx/debug.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/wfx/debug.c b/drivers/staging/wfx/debug.c
> index 3f1712b7c919..83ccbab50899 100644
> --- a/drivers/staging/wfx/debug.c
> +++ b/drivers/staging/wfx/debug.c
> @@ -32,7 +32,7 @@ static const struct trace_print_flags wfx_reg_print_map[] =
> {
> };
>
> static const char *get_symbol(unsigned long val,
> - const struct trace_print_flags *symbol_array)
> + const struct trace_print_flags *symbol_array)
> {
> int i;
>
> --
> 2.18.4
>
Hello Izabela,
Thank you for your contribution.
This issue has already been fixed in commit d61c0848100c ("staging: wfx:
clear alignment style issues").
For your next contribution, ensure your patch applies on the last
development tree.
To get the development tree to use, you can run:
./scripts/get_maintainer.pl --scm -f drivers/staging/wfx/debug.c
--
Jérôme Pouiller
Re: [PATCH 3/8] staging: wfx: standardize the error when vif does not exist
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 writes:
> > >
> > > > From: Jérôme Pouiller
> > > >
> > > > 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
> > > > Signed-off-by: Jérôme Pouiller
> > > > ---
> > > > 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).
--
Jérôme Pouiller
Re: [PATCH 2/8] staging: wfx: check memory allocation
On Friday 9 October 2020 20:51:01 CEST Kalle Valo wrote:
> CAUTION: This email originated from outside of the organization. Do not click
> links or open attachments unless you recognize the sender and know the
> content is safe.
>
>
> Jerome Pouiller writes:
>
> > From: Jérôme Pouiller
> >
> > Smatch complains:
> >
> >main.c:228 wfx_send_pdata_pds() warn: potential NULL parameter
> > dereference 'tmp_buf'
> >227 tmp_buf = kmemdup(pds->data, pds->size, GFP_KERNEL);
> >228 ret = wfx_send_pds(wdev, tmp_buf, pds->size);
> > ^^^
> > 229 kfree(tmp_buf);
> >
> > Reported-by: Dan Carpenter
> > Signed-off-by: Jérôme Pouiller
> > ---
> > drivers/staging/wfx/main.c | 8 +++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/wfx/main.c b/drivers/staging/wfx/main.c
> > index df11c091e094..a8dc2c033410 100644
> > --- a/drivers/staging/wfx/main.c
> > +++ b/drivers/staging/wfx/main.c
> > @@ -222,12 +222,18 @@ static int wfx_send_pdata_pds(struct wfx_dev *wdev)
> > if (ret) {
> > dev_err(wdev->dev, "can't load PDS file %s\n",
> > wdev->pdata.file_pds);
> > - return ret;
> > + goto err1;
> > }
> > tmp_buf = kmemdup(pds->data, pds->size, GFP_KERNEL);
> > + if (!tmp_buf) {
> > + ret = -ENOMEM;
> > + goto err2;
> > + }
> > ret = wfx_send_pds(wdev, tmp_buf, pds->size);
> > kfree(tmp_buf);
> > +err2:
> > release_firmware(pds);
> > +err1:
> > return ret;
> > }
>
> A minor style issue but using more descriptive error labels make the
> code more readable and maintainable, especially in a bigger function.
> For example, err2 could be called err_release_firmware.
>
> And actually err1 could be removed and the goto replaced with just
> "return ret;". Then err2 could be renamed to a simple err.
It was the case in the initial code. However, I have preferred to not
mix 'return' and 'goto' inside the same function. Probably a matter of
taste.
Greg has already applied the series, but I don't forget this review. I
will take it into account in the series I am going to send you (probably
in the v2, in order to not defer the v1).
--
Jérôme Pouiller
Re: [PATCH 3/8] staging: wfx: standardize the error when vif does not exist
On Friday 9 October 2020 20:52:47 CEST Kalle Valo wrote:
> Jerome Pouiller writes:
>
> > From: Jérôme Pouiller
> >
> > 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
> > Signed-off-by: Jérôme Pouiller
> > ---
> > 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.
--
Jérôme Pouiller
Re: [PATCH 0/7] wfx: move out from the staging area
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.pouil...@silabs.com/ or commits a7a91ca5a23d^..40115bbc40e2 -- Jérôme Pouiller
Re: [PATCH] staging: wfx: simplify virt_addr_valid call
On Wednesday 16 September 2020 13:11:59 CEST Greg KH wrote: > On Sat, Sep 12, 2020 at 07:47:19AM -0700, t...@redhat.com wrote: > > From: Tom Rix > > > > Reviewing sram_write_dma_safe(), there are two > > identical calls to virt_addr_valid(). The second > > call can be simplified by a comparison of variables > > set from the first call. > > > > Signed-off-by: Tom Rix > > --- > > drivers/staging/wfx/fwio.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/staging/wfx/fwio.c b/drivers/staging/wfx/fwio.c > > index 22d3b684f04f..c99adb0c99f1 100644 > > --- a/drivers/staging/wfx/fwio.c > > +++ b/drivers/staging/wfx/fwio.c > > @@ -94,7 +94,7 @@ static int sram_write_dma_safe(struct wfx_dev *wdev, u32 > > addr, const u8 *buf, > > tmp = buf; > > } > > ret = sram_buf_write(wdev, addr, tmp, len); > > - if (!virt_addr_valid(buf)) > > + if (tmp != buf) > > kfree(tmp); > > return ret; > > } > > Jerome, any thoughts? Looks correct. Reviewed-by: Jérôme Pouiller -- Jérôme Pouiller
Re: [PATCH 01/12] staging: wfx: fix BA when device is AP and MFP is enabled
On Monday 24 August 2020 11:50:42 CEST Dan Carpenter wrote:
> On Thu, Aug 20, 2020 at 05:58:47PM +0200, Jerome Pouiller wrote:
> > From: Jérôme Pouiller
> >
> > The protection of the management frames is mainly done by mac80211.
> > However, frames for the management of the BlockAck sessions are directly
> > sent by the device. These frames have to be protected if MFP is in use.
> > So the driver has to pass the MFP configuration to the device.
> >
> > Until now, the BlockAck management frames were completely unprotected
> > whatever the status of the MFP negotiation. So, some devices dropped
> > these frames.
> >
> > The device has two knobs to control the MFP. One global and one per
> > station. Normally, the driver should always enable global MFP. Then it
> > should enable MFP on every station with which MFP was successfully
> > negotiated. Unfortunately, the older firmwares only provide the
> > global control.
> >
> > So, this patch enable global MFP as it is exposed in the beacon. Then it
> > marks every station with which the MFP is effective.
> >
> > Thus, the support for the old firmwares is not so bad. It may only
> > encounter some difficulties to negotiate BA sessions when the local
> > device (the AP) is MFP capable (ieee80211w=1) but the station is not.
> > The only solution for this case is to upgrade the firmware.
> >
> > Signed-off-by: Jérôme Pouiller
> > ---
> > drivers/staging/wfx/sta.c | 22 +-
> > 1 file changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c
> > index ad63332f690c..9c1c8223a49f 100644
> > --- a/drivers/staging/wfx/sta.c
> > +++ b/drivers/staging/wfx/sta.c
> > @@ -434,7 +434,7 @@ int wfx_sta_add(struct ieee80211_hw *hw, struct
> > ieee80211_vif *vif,
> > wvif->link_id_map |= BIT(sta_priv->link_id);
> > WARN_ON(!sta_priv->link_id);
> > WARN_ON(sta_priv->link_id >= HIF_LINK_ID_MAX);
> > - hif_map_link(wvif, sta->addr, 0, sta_priv->link_id);
> > + hif_map_link(wvif, sta->addr, sta->mfp ? 2 : 0, sta_priv->link_id);
> >
> > return 0;
> > }
> > @@ -474,6 +474,25 @@ static int wfx_upload_ap_templates(struct wfx_vif
> > *wvif)
> > return 0;
> > }
> >
> > +static void wfx_set_mfp_ap(struct wfx_vif *wvif)
> > +{
> > + struct sk_buff *skb = ieee80211_beacon_get(wvif->wdev->hw, wvif->vif);
> > + const int ieoffset = offsetof(struct ieee80211_mgmt,
> > u.beacon.variable);
> > + const u16 *ptr = (u16 *)cfg80211_find_ie(WLAN_EID_RSN,
> > + skb->data + ieoffset,
> > + skb->len - ieoffset);
> > + const int pairwise_cipher_suite_count_offset = 8 / sizeof(u16);
> > + const int pairwise_cipher_suite_size = 4 / sizeof(u16);
> > + const int akm_suite_size = 4 / sizeof(u16);
> > +
> > + if (ptr) {
> > + ptr += pairwise_cipher_suite_count_offset;
> > + ptr += 1 + pairwise_cipher_suite_size * *ptr;
>
> The value of "*ptr" comes from skb->data. How do we know that it
> doesn't point to something beyond the end of the skb->data buffer?
I think the beacon come from hostapd (or any userspace application with
the necessary permissions). Indeed, it could be corrupted.
I have noticed that WLAN_EID_RSN is parsed at multiple places in the
kernel and I haven't seen any particular check :( (and WLAN_EID_RSN is
probably not the only dangerous IE).
Anyway, I am going to add a few checks on values of ptr.
> > + ptr += 1 + akm_suite_size * *ptr;
> > + hif_set_mfp(wvif, *ptr & BIT(7), *ptr & BIT(6));
> > + }
> > +}
--
Jérôme Pouiller
Re: [PATCH 1/6] staging: wfx/main.h: fix a spelling and grammar mistake
On Thursday 20 August 2020 10:49:33 CEST Suraj Upadhyay wrote:
>
> Fix spelling mistake "extention" => "extension".
> And correct the statement in passive-voice by adding "be".
>
> Signed-off-by: Suraj Upadhyay
> ---
> drivers/staging/wfx/main.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/wfx/main.h b/drivers/staging/wfx/main.h
> index c59d375dd3ad..2457cb595b0f 100644
> --- a/drivers/staging/wfx/main.h
> +++ b/drivers/staging/wfx/main.h
> @@ -19,7 +19,7 @@ struct wfx_dev;
> struct hwbus_ops;
>
> struct wfx_platform_data {
> - /* Keyset and ".sec" extention will appended to this string */
> + /* Keyset and ".sec" extension will be appended to this string */
Already fixed by commit c9638363f02d ("staging: wfx: fix a handful of
spelling mistakes"). But, I am sure there still are spell mistakes in code
of wfx driver :)
--
Jérôme Pouiller
Re: [PATCH 4/6] staging: wfx/debug.c: Fix spelling mistake "carefull" => "careful"
On Thursday 20 August 2020 10:50:57 CEST Suraj Upadhyay wrote:
>
> Signed-off-by: Suraj Upadhyay
> ---
> drivers/staging/wfx/debug.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/wfx/debug.c b/drivers/staging/wfx/debug.c
> index 3f1712b7c919..5772e2375370 100644
> --- a/drivers/staging/wfx/debug.c
> +++ b/drivers/staging/wfx/debug.c
> @@ -299,7 +299,7 @@ static ssize_t wfx_send_hif_msg_read(struct file *file,
> char __user *user_buf,
> return ret;
> if (context->ret < 0)
> return context->ret;
> - // Be carefull, write() is waiting for a full message while read()
> + // Be careful, write() is waiting for a full message while read()
Already fixed by commit c9638363f02d ("staging: wfx: fix a handful of
spelling mistakes").
--
Jérôme Pouiller
Re: [PATCH 5/6] staging: wfx/hif_rx.c: Fix spelling mistake "negociation" => "negotiation"
Hello Suraj,
Thank you for your contribution.
On Thursday 20 August 2020 10:51:27 CEST Suraj Upadhyay wrote:
>
> Signed-off-by: Suraj Upadhyay
Even for a small change, you have to fill the commit log body.
> ---
> drivers/staging/wfx/hif_rx.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/wfx/hif_rx.c b/drivers/staging/wfx/hif_rx.c
> index cc7c0cf226ba..1d32973d8ec1 100644
> --- a/drivers/staging/wfx/hif_rx.c
> +++ b/drivers/staging/wfx/hif_rx.c
> @@ -118,7 +118,7 @@ static int hif_keys_indication(struct wfx_dev *wdev,
>
> // SL_PUB_KEY_EXCHANGE_STATUS_SUCCESS is used by legacy secure link
> if (body->status && body->status != HIF_STATUS_SLK_NEGO_SUCCESS)
> - dev_warn(wdev->dev, "secure link negociation error\n");
> + dev_warn(wdev->dev, "secure link negotiation error\n");
This change is already in the staging tree. See commit c9638363f02d
("staging: wfx: fix a handful of spelling mistakes").
Globally, if you post changes for drivers/staging you should rebase your
work on the staging tree before to send it. You can use the script
get_maintainer.pl to get this information:
$ ./scripts/get_maintainer.pl --scm drivers/staging/wfx
[...]
git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
--
Jérôme Pouiller
Re: [PATCH v2] staging: wfx: refactor to avoid duplication at hif_tx.c
Hello Tomer,
On Wednesday 5 August 2020 14:14:42 CEST Tomer Samara wrote:
>
> Add functions wfx_full_send(), wfx_full_send_no_reply_async(),
> wfx_full_send_no_reply() and wfx_full_send_no_reply_free()
> which works as follow:
> wfx_full_send() - simple wrapper for both wfx_fill_header()
> and wfx_cmd_send().
> wfx_full_send_no_reply_async() - wrapper for both but with
> NULL as reply and size zero.
> wfx_full_send_no_reply() - same as wfx_full_send_no_reply_async()
>but with false async value
> wfx_full_send_no_reply_free() - same as wfx_full_send_no_reply()
> but also free the struct hif_msg.
>
> Signed-off-by: Tomer Samara
> ---
> Changes in v2:
> - Changed these functions to static
>
> drivers/staging/wfx/hif_tx.c | 180 ---
> 1 file changed, 80 insertions(+), 100 deletions(-)
>
> diff --git a/drivers/staging/wfx/hif_tx.c b/drivers/staging/wfx/hif_tx.c
> index 5110f9b93762..17f668fa40a0 100644
> --- a/drivers/staging/wfx/hif_tx.c
> +++ b/drivers/staging/wfx/hif_tx.c
> @@ -40,7 +40,7 @@ static void wfx_fill_header(struct hif_msg *hif, int if_id,
>
> static void *wfx_alloc_hif(size_t body_len, struct hif_msg **hif)
> {
> - *hif = kzalloc(sizeof(struct hif_msg) + body_len, GFP_KERNEL);
> + *hif = kzalloc(sizeof(*hif) + body_len, GFP_KERNEL);
This change is not related to the rest of the patch. It should probably be
split out.
> if (*hif)
> return (*hif)->body;
> else
> @@ -123,9 +123,39 @@ int wfx_cmd_send(struct wfx_dev *wdev, struct hif_msg
> *request,
> return ret;
> }
>
> +static int wfx_full_send(struct wfx_dev *wdev, struct hif_msg *hif, void
> *reply,
> +size_t reply_len, bool async, int if_id, unsigned
> int cmd,
> +int size)
> +{
> + wfx_fill_header(hif, if_id, cmd, size);
> + return wfx_cmd_send(wdev, hif, reply, reply_len, async);
> +}
This function takes 8 parameters. Are you sure it simplifies the code?
In add, it does two actions: modify hif and send it. I would keep these
two actions separated.
> +
> +static int wfx_full_send_no_reply_async(struct wfx_dev *wdev, struct hif_msg
> *hif, int if_id,
> + unsigned int cmd, int size, bool
> async)
> +{
> + return wfx_full_send(wdev, hif, NULL, 0, async, if_id, cmd, size);
> +}
Does it make sense to have a parameter 'async' to
wfx_full_send_no_reply_async()? It is weird to call this function with
async=false, no?
> +
> +static int wfx_full_send_no_reply(struct wfx_dev *wdev, struct hif_msg *hif,
> int if_id,
> + unsigned int cmd, int size)
> +{
> + return wfx_full_send_no_reply_async(wdev, hif, if_id, cmd, size,
> false);
> +}
> +
> +static int wfx_full_send_no_reply_free(struct wfx_dev *wdev, struct hif_msg
> *hif, int if_id,
> + unsigned int cmd, int size)
> +{
> + int ret;
> +
> + ret = wfx_full_send_no_reply(wdev, hif, if_id, cmd, size);
> + kfree(hif);
> + return ret;
> +}
One more time, sending the data and releasing are unrelated actions.
Indeed, it saves a few lines of code, but is it really an improvement?
> +
> // This function is special. After HIF_REQ_ID_SHUT_DOWN, chip won't reply to
> any
> // request anymore. We need to slightly hack struct wfx_hif_cmd for that
> job. Be
> -// carefull to only call this funcion during device unregister.
> +// careful to only call this function during device unregister.
Not related to the rest of the patch.
[...]
As it stands, I think this change does not improve the code. Obviously, it
is a subjective opinion. What the other developers think about it?
--
Jérôme Pouiller
Re: [PATCH][next][V2] staging: wfx: fix a handful of spelling mistakes
is function is special. After HIF_REQ_ID_SHUT_DOWN, chip won't reply to
> any
> // request anymore. We need to slightly hack struct wfx_hif_cmd for that
> job. Be
> -// carefull to only call this funcion during device unregister.
> +// careful to only call this function during device unregister.
> int hif_shutdown(struct wfx_dev *wdev)
> {
> int ret;
> diff --git a/drivers/staging/wfx/main.c b/drivers/staging/wfx/main.c
> index 11dfa088fc86..4263f912760b 100644
> --- a/drivers/staging/wfx/main.c
> +++ b/drivers/staging/wfx/main.c
> @@ -384,7 +384,7 @@ int wfx_probe(struct wfx_dev *wdev)
> err = wfx_sl_init(wdev);
> if (err && wdev->hw_caps.capabilities.link_mode == SEC_LINK_ENFORCED)
> {
> dev_err(wdev->dev,
> - "chip require secure_link, but can't negociate it\n");
> + "chip require secure_link, but can't negotiate it\n");
> goto err0;
> }
>
> diff --git a/drivers/staging/wfx/main.h b/drivers/staging/wfx/main.h
> index c59d375dd3ad..2457cb595b0f 100644
> --- a/drivers/staging/wfx/main.h
> +++ b/drivers/staging/wfx/main.h
> @@ -19,7 +19,7 @@ struct wfx_dev;
> struct hwbus_ops;
>
> struct wfx_platform_data {
> - /* Keyset and ".sec" extention will appended to this string */
> + /* Keyset and ".sec" extension will be appended to this string */
> const char *file_fw;
> const char *file_pds;
> struct gpio_desc *gpio_wakeup;
> diff --git a/drivers/staging/wfx/sta.c b/drivers/staging/wfx/sta.c
> index 4e30ab17a93d..ad63332f690c 100644
> --- a/drivers/staging/wfx/sta.c
> +++ b/drivers/staging/wfx/sta.c
> @@ -214,7 +214,7 @@ static int wfx_get_ps_timeout(struct wfx_vif *wvif, bool
> *enable_ps)
> if (chan0 && chan1 && chan0->hw_value != chan1->hw_value &&
> wvif->vif->type != NL80211_IFTYPE_AP) {
> // It is necessary to enable powersave if channels
> - // are differents.
> + // are different.
> if (enable_ps)
> *enable_ps = true;
> if (wvif->wdev->force_ps_timeout > -1)
> --
> 2.27.0
>
>
Reviewed-by: Jérôme Pouiller
--
Jérôme Pouiller
Re: [PATCH][next] staging: wfx: fix uninitialized variable bytes_done
On Monday 6 July 2020 15:20:17 CEST Colin King wrote:
>
> From: Colin Ian King
>
> The variable bytes_done is not initialized and hence the first
> FIFO size check on bytes_done may be breaking prematurely from
> the loop if bytes_done contains a large bogus uninitialized value.
> Fix this by initializing bytes_done to zero.
>
> Addresses-Coverity: ("Uninitialized scalar variable")
> Fixes: a9408ad79ff3 ("staging: wfx: load the firmware faster")
> Signed-off-by: Colin Ian King
Good catch!
I am surprised that my gcc hasn't caught that.
Reviewed-by: Jérôme Pouiller
> ---
> drivers/staging/wfx/fwio.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/wfx/fwio.c b/drivers/staging/wfx/fwio.c
> index d9a886f3e64b..206c6cf6511c 100644
> --- a/drivers/staging/wfx/fwio.c
> +++ b/drivers/staging/wfx/fwio.c
> @@ -177,7 +177,7 @@ static int wait_ncp_status(struct wfx_dev *wdev, u32
> status)
> static int upload_firmware(struct wfx_dev *wdev, const u8 *data, size_t len)
> {
> int ret;
> - u32 offs, bytes_done;
> + u32 offs, bytes_done = 0;
> ktime_t now, start;
>
> if (len % DNLD_BLOCK_SIZE) {
> --
> 2.27.0
>
>
--
Jérôme Pouiller
Re: [staging-testing] drivers/staging/wfx/hif_tx.c
On Wednesday 10 June 2020 08:53:13 CEST Mohamed Dawod wrote: > Hello, > > I read this point in staging/wfx/TODO file > > - In wfx_cmd_send(), "async" allow to send command without waiting the reply. > It may help in some situation, but it is not yet used. In add, it may > cause > some trouble: > > https://lore.kernel.org/driverdev-devel/alpine.DEB.2.21.1910041317381.2992@hadrien/ > So, fix it (by replacing the mutex with a semaphore) or drop it. > > I think that this issue has already been fixed in hif_shutdown() function, > hasn't it ? Indeed, when I wrote the TODO file, the function hif_shutdown() didn't exist yet. > I have another question. How can (replacing the mutex with a semaphore) solve > the problem? My understanding is that a mutex aims to be acquired and released from the same context. In some specific usages (RT-mutex? lock proving?), acquire mutex from a context and release it from another can produce some errors. In contrary a boolean semaphore does not have this restriction. (can anyone confirm that?) -- Jérôme Pouiller
Re: drivers/staging/wfx/main.c:47:14-21: ERROR: PTR_ERR applied after initialization to constant on line 42
On Monday 8 June 2020 15:31:36 CEST kernel test robot wrote:
>
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> master
> head: af7b4801030c07637840191c69eb666917e4135d
> commit: 0096214a59a72b3c3c943e27bd03307324d3ce0f staging: wfx: add support
> for I/O access
> date: 8 months ago
> config: i386-randconfig-c024-20200607 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-13) 9.3.0
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot
>
>
> coccinelle warnings: (new ones prefixed by >>)
>
> >> drivers/staging/wfx/main.c:47:14-21: ERROR: PTR_ERR applied after
> >> initialization to constant on line 42
>
> vim +47 drivers/staging/wfx/main.c
>
> 30
> 31 struct gpio_desc *wfx_get_gpio(struct device *dev, int override,
> const char *label)
> 32 {
> 33 struct gpio_desc *ret;
> 34 char label_buf[256];
> 35
> 36 if (override >= 0) {
> 37 snprintf(label_buf, sizeof(label_buf), "wfx_%s",
> label);
> 38 ret = ERR_PTR(devm_gpio_request_one(dev, override,
> GPIOF_OUT_INIT_LOW, label_buf));
> 39 if (!ret)
> 40 ret = gpio_to_desc(override);
> 41 } else if (override == -1) {
> > 42 ret = NULL;
> 43 } else {
> 44 ret = devm_gpiod_get(dev, label, GPIOD_OUT_LOW);
> 45 }
> 46 if (IS_ERR(ret) || !ret) {
> > 47 if (!ret || PTR_ERR(ret) == -ENOENT)
> 48 dev_warn(dev, "gpio %s is not defined\n",
> label);
> 49 else
> 50 dev_warn(dev, "error while requesting gpio
> %s\n", label);
> 51 ret = NULL;
> 52 } else {
> 53 dev_dbg(dev, "using gpio %d for %s\n",
> desc_to_gpio(ret), label);
> 54 }
> 55 return ret;
> 56 }
> 57
Hello,
This warning seems to be a false positive (the variable "ret" is affected in
all branches of the if/else).
--
Jérôme Pouiller
Re: [PATCH 00/10] staging: wfx: introduce nl80211 vendor extensions
On Wednesday 27 May 2020 14:34:37 CEST Kalle Valo wrote:
> Jerome Pouiller writes:
>
> > This series introduces some nl80211 vendor extensions to the wfx driver.
> >
> > This series may lead to some discussions:
> >
> > 1. Patch 7 allows to change the dynamic PS timeout. I have found
> > an API in wext (cfg80211_wext_siwpower()) that do more or less the
> > same thing. However, I have not found any equivalent in nl80211. Is it
> > expected or this API should be ported to nl80211?
>
> struct wireless_dev::ps_timeout doesn't work for you?
Indeed, cfg80211_wext_siwpower() modify wireless_dev::ps_timeout, but
there is no equivalent in nl80211, no?
Else, I choose to not directly change wireless_dev::ps_timeout because I
worried about interactions with other parts of cfg80211/mac80211.
> >
> > 2. The device The device allows to do Packet Traffic Arbitration (PTA or
> > also Coex). This feature allows the device to communicate with another
> > RF device in order to share the access to the RF. The patch 9 provides
> > a way to configure that. However, I think that this chip is not the
> > only one to provide this feature. Maybe a standard way to change
> > these parameters should be provided?
> >
> > 3. For these vendor extensions, I have used the new policy introduced by
> > the commit 901bb989185516 ("nl80211: require and validate vendor
> > command policy"). However, it seems that my version of 'iw' is not
> > able to follow this new policy (it does not pack the netlink
> > attributes into a NLA_NESTED). I could develop a tool specifically for
> > that API, but it is not very handy. So, in patch 10, I have also
> > introduced an API for compatibility with iw. Any comments about this?
>
> If you want the driver out of staging I recommend not adding any vendor
> commands until the driver is moved to drivers/net/wireless. Also do note
> that we have special rules for nl80211 vendor commands:
>
> https://wireless.wiki.kernel.org/en/developers/documentation/nl80211#vendor-specific_api
I hoped to suggest the move of this driver outside of staging in some
weeks (the last items in TODO list are either non-essential or easy to
fix). So, you suggest me to resend these patches after that change?
--
Jérôme Pouiller
Re: [PATCH] staging: wfx: typo fix
On Saturday 16 May 2020 14:43:59 CEST Mohamed Dawod wrote:
> Fixing some typo errors in traces.h file
>
> Signed-off-by: Mohamed Dawod
> ---
> drivers/staging/wfx/traces.h | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/wfx/traces.h b/drivers/staging/wfx/traces.h
> index bb9f7e9..80e131c 100644
> --- a/drivers/staging/wfx/traces.h
> +++ b/drivers/staging/wfx/traces.h
> @@ -32,16 +32,16 @@
> * xxx_name(XXX) \
> * ...
> *
> - * 3. Instanciate that list_names:
> + * 3. Instantiate that list_names:
> *
> * list_names
> *
> - * 4. Redefine xxx_name() as a entry of array for __print_symbolic()
> + * 4. Redefine xxx_name() as an entry of array for __print_symbolic()
> *
> * #undef xxx_name
> * #define xxx_name(msg) { msg, #msg },
> *
> - * 5. list_name can now nearlu be used with __print_symbolic() but,
> + * 5. list_name can now nearly be used with __print_symbolic() but,
> * __print_symbolic() dislike last comma of list. So we define a new
> list
> * with a dummy element:
> *
The subject of the mail should have been "[PATCH v2]" (it is automatic if
use the -v option of "git send-email").
Apart from that:
Reviewed-by: Jérôme Pouiller
--
Jérôme Pouiller
Re: [PATCH 05/19] staging: wfx: fix coherency of hif_scan() prototype
On Friday 15 May 2020 15:53:59 CEST Greg Kroah-Hartman wrote: > On Fri, May 15, 2020 at 10:33:11AM +0200, Jerome Pouiller wrote: > > From: Jérôme Pouiller > > > > The function hif_scan() return the timeout for the completion of the > > scan request. It is the only function from hif_tx.c that return another > > thing than just an error code. This behavior is not coherent with the > > rest of file. Worse, if value returned is positive, the caller can't > > make say if it is a timeout or the value returned by the hardware. > > > > Uniformize API with other HIF functions, only return the error code and > > pass timeout with parameters. > > > > Signed-off-by: Jérôme Pouiller > > --- > > drivers/staging/wfx/hif_tx.c | 6 -- > > drivers/staging/wfx/hif_tx.h | 2 +- > > drivers/staging/wfx/scan.c | 6 +++--- > > 3 files changed, 8 insertions(+), 6 deletions(-) > > This patch fails to apply to my branch, so I've stopped here in the > patch series. Hello Greg, Did you applied the patch called "staging: wfx: unlock on error path" from Dan? (I wrote that information in the introduction letter, but maybe I would had include the Dan's patch in my PR?) -- Jérôme Pouiller
Re: [PATCH] wfx: typo fix
Hello,
On Wednesday 13 May 2020 09:24:33 CEST Mohamed Dawod wrote:
>
> fixing some typo errors in traces.h file
>
> Signed-off-by: Mohamed Dawod
> ---
> drivers/staging/wfx/traces.h | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
It would be great if the subject started with "staging: wfx: " (as
the other patches of this directory).
Else, I like when commit messages start with a capital letter, but it is not big
deal.
> diff --git a/drivers/staging/wfx/traces.h b/drivers/staging/wfx/traces.h
> index bb9f7e9..80e131c 100644
> --- a/drivers/staging/wfx/traces.h
> +++ b/drivers/staging/wfx/traces.h
> @@ -32,16 +32,16 @@
> * xxx_name(XXX) \
> * ...
> *
> - * 3. Instanciate that list_names:
> + * 3. Instantiate that list_names:
> *
> * list_names
> *
> - * 4. Redefine xxx_name() as a entry of array for __print_symbolic()
> + * 4. Redefine xxx_name() as an entry of array for __print_symbolic()
> *
> * #undef xxx_name
> * #define xxx_name(msg) { msg, #msg },
> *
> - * 5. list_name can now nearlu be used with __print_symbolic() but,
> + * 5. list_name can now nearly be used with __print_symbolic() but,
> * __print_symbolic() dislike last comma of list. So we define a new
> list
> * with a dummy element:
Ok.
Thank you for your contribution.
--
Jérôme Pouiller
Re: [PATCH 13/17] staging: wfx: fix endianness of the field 'len'
On Tuesday 12 May 2020 09:43:34 CEST Geert Uytterhoeven wrote:
> Hi Jerome,
>
> On Mon, May 11, 2020 at 5:53 PM Jerome Pouiller
> wrote:
> > From: Jérôme Pouiller
> >
> > The struct hif_msg is received from the hardware. So, it declared as
> > little endian. However, it is also accessed from many places in the
> > driver. Sparse complains about that:
> >
> > drivers/staging/wfx/bh.c:88:32: warning: restricted __le16 degrades to
> > integer
> > drivers/staging/wfx/bh.c:88:32: warning: restricted __le16 degrades to
> > integer
> > drivers/staging/wfx/bh.c:93:32: warning: restricted __le16 degrades to
> > integer
> > drivers/staging/wfx/bh.c:93:32: warning: cast to restricted __le16
> > drivers/staging/wfx/bh.c:93:32: warning: restricted __le16 degrades to
> > integer
> > drivers/staging/wfx/bh.c:121:25: warning: incorrect type in argument 2
> > (different base types)
> > drivers/staging/wfx/bh.c:121:25:expected unsigned int len
> > drivers/staging/wfx/bh.c:121:25:got restricted __le16 [usertype] len
> > drivers/staging/wfx/hif_rx.c:27:22: warning: restricted __le16 degrades
> > to integer
> > drivers/staging/wfx/hif_rx.c:347:39: warning: incorrect type in
> > argument 7 (different base types)
> > drivers/staging/wfx/hif_rx.c:347:39:expected unsigned int
> > [usertype] len
> > drivers/staging/wfx/hif_rx.c:347:39:got restricted __le16 const
> > [usertype] len
> > drivers/staging/wfx/hif_rx.c:365:39: warning: incorrect type in
> > argument 7 (different base types)
> > drivers/staging/wfx/hif_rx.c:365:39:expected unsigned int
> > [usertype] len
> > drivers/staging/wfx/hif_rx.c:365:39:got restricted __le16 const
> > [usertype] len
> > drivers/staging/wfx/./traces.h:195:1: warning: incorrect type in
> > assignment (different base types)
> > drivers/staging/wfx/./traces.h:195:1:expected int msg_len
> > drivers/staging/wfx/./traces.h:195:1:got restricted __le16 const
> > [usertype] len
> > drivers/staging/wfx/./traces.h:195:1: warning: incorrect type in
> > assignment (different base types)
> > drivers/staging/wfx/./traces.h:195:1:expected int msg_len
> > drivers/staging/wfx/./traces.h:195:1:got restricted __le16 const
> > [usertype] len
> > drivers/staging/wfx/debug.c:319:20: warning: restricted __le16 degrades
> > to integer
> > drivers/staging/wfx/secure_link.c:85:27: warning: restricted __le16
> > degrades to integer
> > drivers/staging/wfx/secure_link.c:85:27: warning: restricted __le16
> > degrades to integer
>
> Thanks for your patch!
>
> > In order to make Sparse happy and to keep access from the driver easy,
> > this patch declare 'len' with native endianness.
> >
> > On reception of hardware data, this patch takes care to do byte-swap and
> > keep Sparse happy.
>
> Which means sparse can no longer do any checking on the field,
> and new bugs may/will creep in in the future, unnoticed.
>
> > --- a/drivers/staging/wfx/hif_api_general.h
> > +++ b/drivers/staging/wfx/hif_api_general.h
> > @@ -23,7 +23,10 @@
> > #define HIF_COUNTER_MAX 7
> >
> > struct hif_msg {
> > - __le16 len;
> > + // len is in fact little endian. However, it is widely used in the
> > + // driver, so we declare it in native byte order and we reorder just
> > + // before/after send/receive it (see bh.c).
> > + u16len;
>
> While there's a small penalty associated with always doing the conversion
> on big-endian platforms, it will probably be lost in the noise anyway.
I have made the changes to show you that the code is far more complicated
with a le16... and the result was not as complicated as I expected...
I am going to post a v2.
--
Jérôme Pouiller
[PATCH] ARM: dts: imx6dl: Fix typo in Engicam i.CoreM6 DualLite/Solo RQS
Obviously "Engicam i.CoreM6 DualLite/Solo RQS" include an imx6dl, not
an imx6q.
Signed-off-by: Jérôme Pouiller
---
arch/arm/boot/dts/imx6dl-icore-rqs.dts | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/imx6dl-icore-rqs.dts
b/arch/arm/boot/dts/imx6dl-icore-rqs.dts
index cf42c2f5cdc7..1281bc39b7ab 100644
--- a/arch/arm/boot/dts/imx6dl-icore-rqs.dts
+++ b/arch/arm/boot/dts/imx6dl-icore-rqs.dts
@@ -42,7 +42,7 @@
/dts-v1/;
-#include "imx6q.dtsi"
+#include "imx6dl.dtsi"
#include "imx6qdl-icore-rqs.dtsi"
/ {
--
2.11.0
