[PATCH v2 0/6] staging: WF200 driver fixes

[PATCH v2 0/6] staging: WF200 driver fixes

[Michał Mirosław]

A set of fixes for WF200 WiFi module driver. Three are bug fixes,
all the rest: cleanups and optimizations.

v2:
  use devres for fixing init/interrupt race, as suggested by Dan Carpenter
  add Fixes: tags dropped by accident
  remove 8-bit SPI fall-back patch

Michał Mirosław (6):
  staging: wfx: fix init/remove vs IRQ race
  staging: wfx: annotate nested gc_list vs tx queue locking
  staging: wfx: add proper "compatible" string
  staging: wfx: follow compatible = vendor,chip format
  staging: wfx: use sleeping gpio accessors
  staging: wfx: use more power-efficient sleep for reset

 .../bindings/net/wireless/siliabs,wfx.txt     | 11 ++---
 drivers/staging/wfx/bh.c                      |  8 ++--
 drivers/staging/wfx/bus_sdio.c                | 16 +++----
 drivers/staging/wfx/bus_spi.c                 | 45 +++++++++++--------
 drivers/staging/wfx/hwio.c                    |  2 +-
 drivers/staging/wfx/main.c                    | 23 ++++++----
 drivers/staging/wfx/main.h                    |  1 -
 drivers/staging/wfx/queue.c                   | 16 +++----
 8 files changed, 66 insertions(+), 56 deletions(-)

-- 
2.20.1


From 08a65da2e2ce94be8a7d00cc692166404ea3e08d Mon Sep 17 00:00:00 2001
Message-Id: <cover.1581416380.git.mirq-linux@rere.qmqm.pl>
From: =?UTF-8?q?Micha=C5=82=20Miros=C5=82aw?= <mirq-linux@rere.qmqm.pl>
Date: Tue, 11 Feb 2020 11:19:40 +0100
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
To:	=?UTF-8?B?SsOpcsO0bWU=?= Pouiller <jerome.pouiller@silabs.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:	devel@driverdev.osuosl.org,
	linux-kernel@vger.kernel.org

Michał Mirosław (7):
  staging: wfx: fix init/remove vs IRQ race
  staging: wfx: annotate nested gc_list vs tx queue locking
  staging: wfx: add proper "compatible" string
  staging: wfx: follow compatible = vendor,chip format
  staging: wfx: try 16-bit word mode first
  staging: wfx: use sleeping gpio accessors
  staging: wfx: use more power-efficient sleep for reset

 .../bindings/net/wireless/siliabs,wfx.txt     | 11 ++--
 drivers/staging/wfx/bh.c                      |  8 +--
 drivers/staging/wfx/bus_sdio.c                | 16 +++---
 drivers/staging/wfx/bus_spi.c                 | 52 +++++++++++--------
 drivers/staging/wfx/hwio.c                    |  2 +-
 drivers/staging/wfx/main.c                    | 23 ++++----
 drivers/staging/wfx/main.h                    |  1 -
 drivers/staging/wfx/queue.c                   | 16 +++---
 8 files changed, 71 insertions(+), 58 deletions(-)

-- 
2.20.1

[PATCH v2 0/6] staging: WF200 driver fixes

[Michał Mirosław]

A set of fixes for WF200 WiFi module driver. Three are bug fixes,
all the rest: cleanups and optimizations.

v2:
  use devres for fixing init/interrupt race, as suggested by Dan Carpenter
  add Fixes: tags dropped by accident
  remove 8-bit SPI fall-back patch

Michał Mirosław (6):
  staging: wfx: fix init/remove vs IRQ race
  staging: wfx: annotate nested gc_list vs tx queue locking
  staging: wfx: add proper "compatible" string
  staging: wfx: follow compatible = vendor,chip format
  staging: wfx: use sleeping gpio accessors
  staging: wfx: use more power-efficient sleep for reset

 .../bindings/net/wireless/siliabs,wfx.txt     | 11 ++---
 drivers/staging/wfx/bh.c                      |  8 ++--
 drivers/staging/wfx/bus_sdio.c                | 16 +++----
 drivers/staging/wfx/bus_spi.c                 | 45 +++++++++++--------
 drivers/staging/wfx/hwio.c                    |  2 +-
 drivers/staging/wfx/main.c                    | 23 ++++++----
 drivers/staging/wfx/main.h                    |  1 -
 drivers/staging/wfx/queue.c                   | 16 +++----
 8 files changed, 66 insertions(+), 56 deletions(-)

-- 
2.20.1


From 08a65da2e2ce94be8a7d00cc692166404ea3e08d Mon Sep 17 00:00:00 2001
Message-Id: <cover.1581416380.git.mirq-linux@rere.qmqm.pl>
From: =?UTF-8?q?Micha=C5=82=20Miros=C5=82aw?= <mirq-linux@rere.qmqm.pl>
Date: Tue, 11 Feb 2020 11:19:40 +0100
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
To:	=?UTF-8?B?SsOpcsO0bWU=?= Pouiller <jerome.pouiller@silabs.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:	devel@driverdev.osuosl.org,
	linux-kernel@vger.kernel.org

Michał Mirosław (7):
  staging: wfx: fix init/remove vs IRQ race
  staging: wfx: annotate nested gc_list vs tx queue locking
  staging: wfx: add proper "compatible" string
  staging: wfx: follow compatible = vendor,chip format
  staging: wfx: try 16-bit word mode first
  staging: wfx: use sleeping gpio accessors
  staging: wfx: use more power-efficient sleep for reset

 .../bindings/net/wireless/siliabs,wfx.txt     | 11 ++--
 drivers/staging/wfx/bh.c                      |  8 +--
 drivers/staging/wfx/bus_sdio.c                | 16 +++---
 drivers/staging/wfx/bus_spi.c                 | 52 +++++++++++--------
 drivers/staging/wfx/hwio.c                    |  2 +-
 drivers/staging/wfx/main.c                    | 23 ++++----
 drivers/staging/wfx/main.h                    |  1 -
 drivers/staging/wfx/queue.c                   | 16 +++---
 8 files changed, 71 insertions(+), 58 deletions(-)

-- 
2.20.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 1/6] staging: wfx: fix init/remove vs IRQ race

[Michał Mirosław]

Current code races in init/exit with interrupt handlers. This is noticed
by the warning below. Fix it by using devres for ordering allocations and
IRQ de/registration.

WARNING: CPU: 0 PID: 827 at drivers/staging/wfx/bus_spi.c:142 wfx_spi_irq_handler+0x5c/0x64 [wfx]
race condition in driver init/deinit

Cc: stable@vger.kernel.org
Fixes: 0096214a59a7 ("staging: wfx: add support for I/O access")
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
v2: use devres to fix the races
---
 drivers/staging/wfx/bus_sdio.c | 15 ++++++---------
 drivers/staging/wfx/bus_spi.c  | 27 ++++++++++++++-------------
 drivers/staging/wfx/main.c     | 21 +++++++++++++--------
 drivers/staging/wfx/main.h     |  1 -
 4 files changed, 33 insertions(+), 31 deletions(-)

diff --git a/drivers/staging/wfx/bus_sdio.c b/drivers/staging/wfx/bus_sdio.c
index f8901164c206..5450bd5e1b5d 100644
--- a/drivers/staging/wfx/bus_sdio.c
+++ b/drivers/staging/wfx/bus_sdio.c
@@ -200,25 +200,23 @@ static int wfx_sdio_probe(struct sdio_func *func,
 	if (ret)
 		goto err0;
 
-	ret = wfx_sdio_irq_subscribe(bus);
-	if (ret)
-		goto err1;
-
 	bus->core = wfx_init_common(&func->dev, &wfx_sdio_pdata,
 				    &wfx_sdio_hwbus_ops, bus);
 	if (!bus->core) {
 		ret = -EIO;
-		goto err2;
+		goto err1;
 	}
 
+	ret = wfx_sdio_irq_subscribe(bus);
+	if (ret)
+		goto err1;
+
 	ret = wfx_probe(bus->core);
 	if (ret)
-		goto err3;
+		goto err2;
 
 	return 0;
 
-err3:
-	wfx_free_common(bus->core);
 err2:
 	wfx_sdio_irq_unsubscribe(bus);
 err1:
@@ -234,7 +232,6 @@ static void wfx_sdio_remove(struct sdio_func *func)
 	struct wfx_sdio_priv *bus = sdio_get_drvdata(func);
 
 	wfx_release(bus->core);
-	wfx_free_common(bus->core);
 	wfx_sdio_irq_unsubscribe(bus);
 	sdio_claim_host(func);
 	sdio_disable_func(func);
diff --git a/drivers/staging/wfx/bus_spi.c b/drivers/staging/wfx/bus_spi.c
index 40bc33035de2..605ad74068b7 100644
--- a/drivers/staging/wfx/bus_spi.c
+++ b/drivers/staging/wfx/bus_spi.c
@@ -154,6 +154,11 @@ static void wfx_spi_request_rx(struct work_struct *work)
 	wfx_bh_request_rx(bus->core);
 }
 
+static void wfx_flush_irq_work(void *w)
+{
+	flush_work(w);
+}
+
 static size_t wfx_spi_align_size(void *priv, size_t size)
 {
 	// Most of SPI controllers avoid DMA if buffer size is not 32bit aligned
@@ -207,22 +212,23 @@ static int wfx_spi_probe(struct spi_device *func)
 		udelay(2000);
 	}
 
-	ret = devm_request_irq(&func->dev, func->irq, wfx_spi_irq_handler,
-			       IRQF_TRIGGER_RISING, "wfx", bus);
-	if (ret)
-		return ret;
-
 	INIT_WORK(&bus->request_rx, wfx_spi_request_rx);
 	bus->core = wfx_init_common(&func->dev, &wfx_spi_pdata,
 				    &wfx_spi_hwbus_ops, bus);
 	if (!bus->core)
 		return -EIO;
 
-	ret = wfx_probe(bus->core);
+	ret = devm_add_action_or_reset(&func->dev, wfx_flush_irq_work,
+				       &bus->request_rx);
 	if (ret)
-		wfx_free_common(bus->core);
+		return ret;
 
-	return ret;
+	ret = devm_request_irq(&func->dev, func->irq, wfx_spi_irq_handler,
+			       IRQF_TRIGGER_RISING, "wfx", bus);
+	if (ret)
+		return ret;
+
+	return wfx_probe(bus->core);
 }
 
 static int wfx_spi_remove(struct spi_device *func)
@@ -230,11 +236,6 @@ static int wfx_spi_remove(struct spi_device *func)
 	struct wfx_spi_priv *bus = spi_get_drvdata(func);
 
 	wfx_release(bus->core);
-	wfx_free_common(bus->core);
-	// A few IRQ will be sent during device release. Hopefully, no IRQ
-	// should happen after wdev/wvif are released.
-	devm_free_irq(&func->dev, func->irq, bus);
-	flush_work(&bus->request_rx);
 	return 0;
 }
 
diff --git a/drivers/staging/wfx/main.c b/drivers/staging/wfx/main.c
index 84adad64fc30..76b2ff7fc7fe 100644
--- a/drivers/staging/wfx/main.c
+++ b/drivers/staging/wfx/main.c
@@ -262,6 +262,16 @@ static int wfx_send_pdata_pds(struct wfx_dev *wdev)
 	return ret;
 }
 
+static void wfx_free_common(void *data)
+{
+	struct wfx_dev *wdev = data;
+
+	mutex_destroy(&wdev->rx_stats_lock);
+	mutex_destroy(&wdev->conf_mutex);
+	wfx_tx_queues_deinit(wdev);
+	ieee80211_free_hw(wdev->hw);
+}
+
 struct wfx_dev *wfx_init_common(struct device *dev,
 				const struct wfx_platform_data *pdata,
 				const struct hwbus_ops *hwbus_ops,
@@ -332,17 +342,12 @@ struct wfx_dev *wfx_init_common(struct device *dev,
 	wfx_init_hif_cmd(&wdev->hif_cmd);
 	wfx_tx_queues_init(wdev);
 
+	if (devm_add_action_or_reset(dev, wfx_free_common, wdev))
+		return NULL;
+
 	return wdev;
 }
 
-void wfx_free_common(struct wfx_dev *wdev)
-{
-	mutex_destroy(&wdev->rx_stats_lock);
-	mutex_destroy(&wdev->conf_mutex);
-	wfx_tx_queues_deinit(wdev);
-	ieee80211_free_hw(wdev->hw);
-}
-
 int wfx_probe(struct wfx_dev *wdev)
 {
 	int i;
diff --git a/drivers/staging/wfx/main.h b/drivers/staging/wfx/main.h
index 875f8c227803..9c9410072def 100644
--- a/drivers/staging/wfx/main.h
+++ b/drivers/staging/wfx/main.h
@@ -34,7 +34,6 @@ struct wfx_dev *wfx_init_common(struct device *dev,
 				const struct wfx_platform_data *pdata,
 				const struct hwbus_ops *hwbus_ops,
 				void *hwbus_priv);
-void wfx_free_common(struct wfx_dev *wdev);
 
 int wfx_probe(struct wfx_dev *wdev);
 void wfx_release(struct wfx_dev *wdev);
-- 
2.20.1

[PATCH v2 1/6] staging: wfx: fix init/remove vs IRQ race

[Michał Mirosław]

Current code races in init/exit with interrupt handlers. This is noticed
by the warning below. Fix it by using devres for ordering allocations and
IRQ de/registration.

WARNING: CPU: 0 PID: 827 at drivers/staging/wfx/bus_spi.c:142 wfx_spi_irq_handler+0x5c/0x64 [wfx]
race condition in driver init/deinit

Cc: stable@vger.kernel.org
Fixes: 0096214a59a7 ("staging: wfx: add support for I/O access")
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
v2: use devres to fix the races
---
 drivers/staging/wfx/bus_sdio.c | 15 ++++++---------
 drivers/staging/wfx/bus_spi.c  | 27 ++++++++++++++-------------
 drivers/staging/wfx/main.c     | 21 +++++++++++++--------
 drivers/staging/wfx/main.h     |  1 -
 4 files changed, 33 insertions(+), 31 deletions(-)

diff --git a/drivers/staging/wfx/bus_sdio.c b/drivers/staging/wfx/bus_sdio.c
index f8901164c206..5450bd5e1b5d 100644
--- a/drivers/staging/wfx/bus_sdio.c
+++ b/drivers/staging/wfx/bus_sdio.c
@@ -200,25 +200,23 @@ static int wfx_sdio_probe(struct sdio_func *func,
 	if (ret)
 		goto err0;
 
-	ret = wfx_sdio_irq_subscribe(bus);
-	if (ret)
-		goto err1;
-
 	bus->core = wfx_init_common(&func->dev, &wfx_sdio_pdata,
 				    &wfx_sdio_hwbus_ops, bus);
 	if (!bus->core) {
 		ret = -EIO;
-		goto err2;
+		goto err1;
 	}
 
+	ret = wfx_sdio_irq_subscribe(bus);
+	if (ret)
+		goto err1;
+
 	ret = wfx_probe(bus->core);
 	if (ret)
-		goto err3;
+		goto err2;
 
 	return 0;
 
-err3:
-	wfx_free_common(bus->core);
 err2:
 	wfx_sdio_irq_unsubscribe(bus);
 err1:
@@ -234,7 +232,6 @@ static void wfx_sdio_remove(struct sdio_func *func)
 	struct wfx_sdio_priv *bus = sdio_get_drvdata(func);
 
 	wfx_release(bus->core);
-	wfx_free_common(bus->core);
 	wfx_sdio_irq_unsubscribe(bus);
 	sdio_claim_host(func);
 	sdio_disable_func(func);
diff --git a/drivers/staging/wfx/bus_spi.c b/drivers/staging/wfx/bus_spi.c
index 40bc33035de2..605ad74068b7 100644
--- a/drivers/staging/wfx/bus_spi.c
+++ b/drivers/staging/wfx/bus_spi.c
@@ -154,6 +154,11 @@ static void wfx_spi_request_rx(struct work_struct *work)
 	wfx_bh_request_rx(bus->core);
 }
 
+static void wfx_flush_irq_work(void *w)
+{
+	flush_work(w);
+}
+
 static size_t wfx_spi_align_size(void *priv, size_t size)
 {
 	// Most of SPI controllers avoid DMA if buffer size is not 32bit aligned
@@ -207,22 +212,23 @@ static int wfx_spi_probe(struct spi_device *func)
 		udelay(2000);
 	}
 
-	ret = devm_request_irq(&func->dev, func->irq, wfx_spi_irq_handler,
-			       IRQF_TRIGGER_RISING, "wfx", bus);
-	if (ret)
-		return ret;
-
 	INIT_WORK(&bus->request_rx, wfx_spi_request_rx);
 	bus->core = wfx_init_common(&func->dev, &wfx_spi_pdata,
 				    &wfx_spi_hwbus_ops, bus);
 	if (!bus->core)
 		return -EIO;
 
-	ret = wfx_probe(bus->core);
+	ret = devm_add_action_or_reset(&func->dev, wfx_flush_irq_work,
+				       &bus->request_rx);
 	if (ret)
-		wfx_free_common(bus->core);
+		return ret;
 
-	return ret;
+	ret = devm_request_irq(&func->dev, func->irq, wfx_spi_irq_handler,
+			       IRQF_TRIGGER_RISING, "wfx", bus);
+	if (ret)
+		return ret;
+
+	return wfx_probe(bus->core);
 }
 
 static int wfx_spi_remove(struct spi_device *func)
@@ -230,11 +236,6 @@ static int wfx_spi_remove(struct spi_device *func)
 	struct wfx_spi_priv *bus = spi_get_drvdata(func);
 
 	wfx_release(bus->core);
-	wfx_free_common(bus->core);
-	// A few IRQ will be sent during device release. Hopefully, no IRQ
-	// should happen after wdev/wvif are released.
-	devm_free_irq(&func->dev, func->irq, bus);
-	flush_work(&bus->request_rx);
 	return 0;
 }
 
diff --git a/drivers/staging/wfx/main.c b/drivers/staging/wfx/main.c
index 84adad64fc30..76b2ff7fc7fe 100644
--- a/drivers/staging/wfx/main.c
+++ b/drivers/staging/wfx/main.c
@@ -262,6 +262,16 @@ static int wfx_send_pdata_pds(struct wfx_dev *wdev)
 	return ret;
 }
 
+static void wfx_free_common(void *data)
+{
+	struct wfx_dev *wdev = data;
+
+	mutex_destroy(&wdev->rx_stats_lock);
+	mutex_destroy(&wdev->conf_mutex);
+	wfx_tx_queues_deinit(wdev);
+	ieee80211_free_hw(wdev->hw);
+}
+
 struct wfx_dev *wfx_init_common(struct device *dev,
 				const struct wfx_platform_data *pdata,
 				const struct hwbus_ops *hwbus_ops,
@@ -332,17 +342,12 @@ struct wfx_dev *wfx_init_common(struct device *dev,
 	wfx_init_hif_cmd(&wdev->hif_cmd);
 	wfx_tx_queues_init(wdev);
 
+	if (devm_add_action_or_reset(dev, wfx_free_common, wdev))
+		return NULL;
+
 	return wdev;
 }
 
-void wfx_free_common(struct wfx_dev *wdev)
-{
-	mutex_destroy(&wdev->rx_stats_lock);
-	mutex_destroy(&wdev->conf_mutex);
-	wfx_tx_queues_deinit(wdev);
-	ieee80211_free_hw(wdev->hw);
-}
-
 int wfx_probe(struct wfx_dev *wdev)
 {
 	int i;
diff --git a/drivers/staging/wfx/main.h b/drivers/staging/wfx/main.h
index 875f8c227803..9c9410072def 100644
--- a/drivers/staging/wfx/main.h
+++ b/drivers/staging/wfx/main.h
@@ -34,7 +34,6 @@ struct wfx_dev *wfx_init_common(struct device *dev,
 				const struct wfx_platform_data *pdata,
 				const struct hwbus_ops *hwbus_ops,
 				void *hwbus_priv);
-void wfx_free_common(struct wfx_dev *wdev);
 
 int wfx_probe(struct wfx_dev *wdev);
 void wfx_release(struct wfx_dev *wdev);
-- 
2.20.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 1/6] staging: wfx: fix init/remove vs IRQ race

[Jérôme Pouiller]

On Tuesday 11 February 2020 11:35:01 CET Michał Mirosław wrote:
> Current code races in init/exit with interrupt handlers. This is noticed
> by the warning below. Fix it by using devres for ordering allocations and
> IRQ de/registration.
> 
> WARNING: CPU: 0 PID: 827 at drivers/staging/wfx/bus_spi.c:142 wfx_spi_irq_handler+0x5c/0x64 [wfx]
> race condition in driver init/deinit
> 
> Cc: stable@vger.kernel.org
> Fixes: 0096214a59a7 ("staging: wfx: add support for I/O access")
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
> v2: use devres to fix the races
> ---
>  drivers/staging/wfx/bus_sdio.c | 15 ++++++---------
>  drivers/staging/wfx/bus_spi.c  | 27 ++++++++++++++-------------
>  drivers/staging/wfx/main.c     | 21 +++++++++++++--------
>  drivers/staging/wfx/main.h     |  1 -
>  4 files changed, 33 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/staging/wfx/bus_sdio.c b/drivers/staging/wfx/bus_sdio.c
> index f8901164c206..5450bd5e1b5d 100644
> --- a/drivers/staging/wfx/bus_sdio.c
> +++ b/drivers/staging/wfx/bus_sdio.c
> @@ -200,25 +200,23 @@ static int wfx_sdio_probe(struct sdio_func *func,
>         if (ret)
>                 goto err0;
> 
> -       ret = wfx_sdio_irq_subscribe(bus);
> -       if (ret)
> -               goto err1;
> -
>         bus->core = wfx_init_common(&func->dev, &wfx_sdio_pdata,
>                                     &wfx_sdio_hwbus_ops, bus);
>         if (!bus->core) {
>                 ret = -EIO;
> -               goto err2;
> +               goto err1;
>         }
> 
> +       ret = wfx_sdio_irq_subscribe(bus);
> +       if (ret)
> +               goto err1;
> +
>         ret = wfx_probe(bus->core);
>         if (ret)
> -               goto err3;
> +               goto err2;
> 
>         return 0;
> 
> -err3:
> -       wfx_free_common(bus->core);
>  err2:
>         wfx_sdio_irq_unsubscribe(bus);
>  err1:
> @@ -234,7 +232,6 @@ static void wfx_sdio_remove(struct sdio_func *func)
>         struct wfx_sdio_priv *bus = sdio_get_drvdata(func);
> 
>         wfx_release(bus->core);
> -       wfx_free_common(bus->core);
>         wfx_sdio_irq_unsubscribe(bus);
>         sdio_claim_host(func);
>         sdio_disable_func(func);
> diff --git a/drivers/staging/wfx/bus_spi.c b/drivers/staging/wfx/bus_spi.c
> index 40bc33035de2..605ad74068b7 100644
> --- a/drivers/staging/wfx/bus_spi.c
> +++ b/drivers/staging/wfx/bus_spi.c
> @@ -154,6 +154,11 @@ static void wfx_spi_request_rx(struct work_struct *work)
>         wfx_bh_request_rx(bus->core);
>  }
> 
> +static void wfx_flush_irq_work(void *w)
> +{
> +       flush_work(w);
> +}
> +
>  static size_t wfx_spi_align_size(void *priv, size_t size)
>  {
>         // Most of SPI controllers avoid DMA if buffer size is not 32bit aligned
> @@ -207,22 +212,23 @@ static int wfx_spi_probe(struct spi_device *func)
>                 udelay(2000);
>         }
> 
> -       ret = devm_request_irq(&func->dev, func->irq, wfx_spi_irq_handler,
> -                              IRQF_TRIGGER_RISING, "wfx", bus);
> -       if (ret)
> -               return ret;
> -
>         INIT_WORK(&bus->request_rx, wfx_spi_request_rx);
>         bus->core = wfx_init_common(&func->dev, &wfx_spi_pdata,
>                                     &wfx_spi_hwbus_ops, bus);
>         if (!bus->core)
>                 return -EIO;
> 
> -       ret = wfx_probe(bus->core);
> +       ret = devm_add_action_or_reset(&func->dev, wfx_flush_irq_work,
> +                                      &bus->request_rx);
>         if (ret)
> -               wfx_free_common(bus->core);
> +               return ret;
> 
> -       return ret;
> +       ret = devm_request_irq(&func->dev, func->irq, wfx_spi_irq_handler,
> +                              IRQF_TRIGGER_RISING, "wfx", bus);
> +       if (ret)
> +               return ret;
> +
> +       return wfx_probe(bus->core);
>  }
> 
>  static int wfx_spi_remove(struct spi_device *func)
> @@ -230,11 +236,6 @@ static int wfx_spi_remove(struct spi_device *func)
>         struct wfx_spi_priv *bus = spi_get_drvdata(func);
> 
>         wfx_release(bus->core);
> -       wfx_free_common(bus->core);
> -       // A few IRQ will be sent during device release. Hopefully, no IRQ
> -       // should happen after wdev/wvif are released.
> -       devm_free_irq(&func->dev, func->irq, bus);
> -       flush_work(&bus->request_rx);
>         return 0;
>  }
> 
> diff --git a/drivers/staging/wfx/main.c b/drivers/staging/wfx/main.c
> index 84adad64fc30..76b2ff7fc7fe 100644
> --- a/drivers/staging/wfx/main.c
> +++ b/drivers/staging/wfx/main.c
> @@ -262,6 +262,16 @@ static int wfx_send_pdata_pds(struct wfx_dev *wdev)
>         return ret;
>  }
> 
> +static void wfx_free_common(void *data)
> +{
> +       struct wfx_dev *wdev = data;
> +
> +       mutex_destroy(&wdev->rx_stats_lock);
> +       mutex_destroy(&wdev->conf_mutex);
> +       wfx_tx_queues_deinit(wdev);
> +       ieee80211_free_hw(wdev->hw);
> +}
> +
>  struct wfx_dev *wfx_init_common(struct device *dev,
>                                 const struct wfx_platform_data *pdata,
>                                 const struct hwbus_ops *hwbus_ops,
> @@ -332,17 +342,12 @@ struct wfx_dev *wfx_init_common(struct device *dev,
>         wfx_init_hif_cmd(&wdev->hif_cmd);
>         wfx_tx_queues_init(wdev);
> 
> +       if (devm_add_action_or_reset(dev, wfx_free_common, wdev))
> +               return NULL;
> +
>         return wdev;
>  }
> 
> -void wfx_free_common(struct wfx_dev *wdev)
> -{
> -       mutex_destroy(&wdev->rx_stats_lock);
> -       mutex_destroy(&wdev->conf_mutex);
> -       wfx_tx_queues_deinit(wdev);
> -       ieee80211_free_hw(wdev->hw);
> -}
> -
>  int wfx_probe(struct wfx_dev *wdev)
>  {
>         int i;
> diff --git a/drivers/staging/wfx/main.h b/drivers/staging/wfx/main.h
> index 875f8c227803..9c9410072def 100644
> --- a/drivers/staging/wfx/main.h
> +++ b/drivers/staging/wfx/main.h
> @@ -34,7 +34,6 @@ struct wfx_dev *wfx_init_common(struct device *dev,
>                                 const struct wfx_platform_data *pdata,
>                                 const struct hwbus_ops *hwbus_ops,
>                                 void *hwbus_priv);
> -void wfx_free_common(struct wfx_dev *wdev);
> 
>  int wfx_probe(struct wfx_dev *wdev);
>  void wfx_release(struct wfx_dev *wdev);
> --
> 2.20.1
> 

Are you sure you can completely drop wfx_free_common()? If you want to
use devres, I think that wfx_flush_irq_work() should call
wfx_free_common(), no?

-- 
Jérôme Pouiller

Re: [PATCH v2 1/6] staging: wfx: fix init/remove vs IRQ race

[Jérôme Pouiller]

On Tuesday 11 February 2020 11:35:01 CET Michał Mirosław wrote:
> Current code races in init/exit with interrupt handlers. This is noticed
> by the warning below. Fix it by using devres for ordering allocations and
> IRQ de/registration.
> 
> WARNING: CPU: 0 PID: 827 at drivers/staging/wfx/bus_spi.c:142 wfx_spi_irq_handler+0x5c/0x64 [wfx]
> race condition in driver init/deinit
> 
> Cc: stable@vger.kernel.org
> Fixes: 0096214a59a7 ("staging: wfx: add support for I/O access")
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
> v2: use devres to fix the races
> ---
>  drivers/staging/wfx/bus_sdio.c | 15 ++++++---------
>  drivers/staging/wfx/bus_spi.c  | 27 ++++++++++++++-------------
>  drivers/staging/wfx/main.c     | 21 +++++++++++++--------
>  drivers/staging/wfx/main.h     |  1 -
>  4 files changed, 33 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/staging/wfx/bus_sdio.c b/drivers/staging/wfx/bus_sdio.c
> index f8901164c206..5450bd5e1b5d 100644
> --- a/drivers/staging/wfx/bus_sdio.c
> +++ b/drivers/staging/wfx/bus_sdio.c
> @@ -200,25 +200,23 @@ static int wfx_sdio_probe(struct sdio_func *func,
>         if (ret)
>                 goto err0;
> 
> -       ret = wfx_sdio_irq_subscribe(bus);
> -       if (ret)
> -               goto err1;
> -
>         bus->core = wfx_init_common(&func->dev, &wfx_sdio_pdata,
>                                     &wfx_sdio_hwbus_ops, bus);
>         if (!bus->core) {
>                 ret = -EIO;
> -               goto err2;
> +               goto err1;
>         }
> 
> +       ret = wfx_sdio_irq_subscribe(bus);
> +       if (ret)
> +               goto err1;
> +
>         ret = wfx_probe(bus->core);
>         if (ret)
> -               goto err3;
> +               goto err2;
> 
>         return 0;
> 
> -err3:
> -       wfx_free_common(bus->core);
>  err2:
>         wfx_sdio_irq_unsubscribe(bus);
>  err1:
> @@ -234,7 +232,6 @@ static void wfx_sdio_remove(struct sdio_func *func)
>         struct wfx_sdio_priv *bus = sdio_get_drvdata(func);
> 
>         wfx_release(bus->core);
> -       wfx_free_common(bus->core);
>         wfx_sdio_irq_unsubscribe(bus);
>         sdio_claim_host(func);
>         sdio_disable_func(func);
> diff --git a/drivers/staging/wfx/bus_spi.c b/drivers/staging/wfx/bus_spi.c
> index 40bc33035de2..605ad74068b7 100644
> --- a/drivers/staging/wfx/bus_spi.c
> +++ b/drivers/staging/wfx/bus_spi.c
> @@ -154,6 +154,11 @@ static void wfx_spi_request_rx(struct work_struct *work)
>         wfx_bh_request_rx(bus->core);
>  }
> 
> +static void wfx_flush_irq_work(void *w)
> +{
> +       flush_work(w);
> +}
> +
>  static size_t wfx_spi_align_size(void *priv, size_t size)
>  {
>         // Most of SPI controllers avoid DMA if buffer size is not 32bit aligned
> @@ -207,22 +212,23 @@ static int wfx_spi_probe(struct spi_device *func)
>                 udelay(2000);
>         }
> 
> -       ret = devm_request_irq(&func->dev, func->irq, wfx_spi_irq_handler,
> -                              IRQF_TRIGGER_RISING, "wfx", bus);
> -       if (ret)
> -               return ret;
> -
>         INIT_WORK(&bus->request_rx, wfx_spi_request_rx);
>         bus->core = wfx_init_common(&func->dev, &wfx_spi_pdata,
>                                     &wfx_spi_hwbus_ops, bus);
>         if (!bus->core)
>                 return -EIO;
> 
> -       ret = wfx_probe(bus->core);
> +       ret = devm_add_action_or_reset(&func->dev, wfx_flush_irq_work,
> +                                      &bus->request_rx);
>         if (ret)
> -               wfx_free_common(bus->core);
> +               return ret;
> 
> -       return ret;
> +       ret = devm_request_irq(&func->dev, func->irq, wfx_spi_irq_handler,
> +                              IRQF_TRIGGER_RISING, "wfx", bus);
> +       if (ret)
> +               return ret;
> +
> +       return wfx_probe(bus->core);
>  }
> 
>  static int wfx_spi_remove(struct spi_device *func)
> @@ -230,11 +236,6 @@ static int wfx_spi_remove(struct spi_device *func)
>         struct wfx_spi_priv *bus = spi_get_drvdata(func);
> 
>         wfx_release(bus->core);
> -       wfx_free_common(bus->core);
> -       // A few IRQ will be sent during device release. Hopefully, no IRQ
> -       // should happen after wdev/wvif are released.
> -       devm_free_irq(&func->dev, func->irq, bus);
> -       flush_work(&bus->request_rx);
>         return 0;
>  }
> 
> diff --git a/drivers/staging/wfx/main.c b/drivers/staging/wfx/main.c
> index 84adad64fc30..76b2ff7fc7fe 100644
> --- a/drivers/staging/wfx/main.c
> +++ b/drivers/staging/wfx/main.c
> @@ -262,6 +262,16 @@ static int wfx_send_pdata_pds(struct wfx_dev *wdev)
>         return ret;
>  }
> 
> +static void wfx_free_common(void *data)
> +{
> +       struct wfx_dev *wdev = data;
> +
> +       mutex_destroy(&wdev->rx_stats_lock);
> +       mutex_destroy(&wdev->conf_mutex);
> +       wfx_tx_queues_deinit(wdev);
> +       ieee80211_free_hw(wdev->hw);
> +}
> +
>  struct wfx_dev *wfx_init_common(struct device *dev,
>                                 const struct wfx_platform_data *pdata,
>                                 const struct hwbus_ops *hwbus_ops,
> @@ -332,17 +342,12 @@ struct wfx_dev *wfx_init_common(struct device *dev,
>         wfx_init_hif_cmd(&wdev->hif_cmd);
>         wfx_tx_queues_init(wdev);
> 
> +       if (devm_add_action_or_reset(dev, wfx_free_common, wdev))
> +               return NULL;
> +
>         return wdev;
>  }
> 
> -void wfx_free_common(struct wfx_dev *wdev)
> -{
> -       mutex_destroy(&wdev->rx_stats_lock);
> -       mutex_destroy(&wdev->conf_mutex);
> -       wfx_tx_queues_deinit(wdev);
> -       ieee80211_free_hw(wdev->hw);
> -}
> -
>  int wfx_probe(struct wfx_dev *wdev)
>  {
>         int i;
> diff --git a/drivers/staging/wfx/main.h b/drivers/staging/wfx/main.h
> index 875f8c227803..9c9410072def 100644
> --- a/drivers/staging/wfx/main.h
> +++ b/drivers/staging/wfx/main.h
> @@ -34,7 +34,6 @@ struct wfx_dev *wfx_init_common(struct device *dev,
>                                 const struct wfx_platform_data *pdata,
>                                 const struct hwbus_ops *hwbus_ops,
>                                 void *hwbus_priv);
> -void wfx_free_common(struct wfx_dev *wdev);
> 
>  int wfx_probe(struct wfx_dev *wdev);
>  void wfx_release(struct wfx_dev *wdev);
> --
> 2.20.1
> 

Are you sure you can completely drop wfx_free_common()? If you want to
use devres, I think that wfx_flush_irq_work() should call
wfx_free_common(), no?

-- 
Jérôme Pouiller

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 1/6] staging: wfx: fix init/remove vs IRQ race

[Michał Mirosław]

On Tue, Feb 11, 2020 at 11:19:18AM +0000, Jérôme Pouiller wrote:
> On Tuesday 11 February 2020 11:35:01 CET Michał Mirosław wrote:
> > Current code races in init/exit with interrupt handlers. This is noticed
> > by the warning below. Fix it by using devres for ordering allocations and
> > IRQ de/registration.
> > 
> > WARNING: CPU: 0 PID: 827 at drivers/staging/wfx/bus_spi.c:142 wfx_spi_irq_handler+0x5c/0x64 [wfx]
> > race condition in driver init/deinit
[...]
> > --- a/drivers/staging/wfx/bus_spi.c
> > +++ b/drivers/staging/wfx/bus_spi.c
> > @@ -154,6 +154,11 @@ static void wfx_spi_request_rx(struct work_struct *work)
> >         wfx_bh_request_rx(bus->core);
> >  }
> > 
> > +static void wfx_flush_irq_work(void *w)
> > +{
> > +       flush_work(w);
> > +}
> > +
> >  static size_t wfx_spi_align_size(void *priv, size_t size)
> >  {
> >         // Most of SPI controllers avoid DMA if buffer size is not 32bit aligned
> > @@ -207,22 +212,23 @@ static int wfx_spi_probe(struct spi_device *func)
> >                 udelay(2000);
> >         }
> > 
> > -       ret = devm_request_irq(&func->dev, func->irq, wfx_spi_irq_handler,
> > -                              IRQF_TRIGGER_RISING, "wfx", bus);
> > -       if (ret)
> > -               return ret;
> > -
> >         INIT_WORK(&bus->request_rx, wfx_spi_request_rx);
> >         bus->core = wfx_init_common(&func->dev, &wfx_spi_pdata,
> >                                     &wfx_spi_hwbus_ops, bus);
> >         if (!bus->core)
> >                 return -EIO;
> > 
> > -       ret = wfx_probe(bus->core);
> > +       ret = devm_add_action_or_reset(&func->dev, wfx_flush_irq_work,
> > +                                      &bus->request_rx);
> >         if (ret)
> > -               wfx_free_common(bus->core);
> > +               return ret;
> > 
> > -       return ret;
> > +       ret = devm_request_irq(&func->dev, func->irq, wfx_spi_irq_handler,
> > +                              IRQF_TRIGGER_RISING, "wfx", bus);
> > +       if (ret)
> > +               return ret;
> > +
> > +       return wfx_probe(bus->core);
> >  }
> > 
> >  static int wfx_spi_remove(struct spi_device *func)
> > @@ -230,11 +236,6 @@ static int wfx_spi_remove(struct spi_device *func)
> >         struct wfx_spi_priv *bus = spi_get_drvdata(func);
> > 
> >         wfx_release(bus->core);
> > -       wfx_free_common(bus->core);
> > -       // A few IRQ will be sent during device release. Hopefully, no IRQ
> > -       // should happen after wdev/wvif are released.
> > -       devm_free_irq(&func->dev, func->irq, bus);
> > -       flush_work(&bus->request_rx);
> >         return 0;
> >  }
> > 
> > diff --git a/drivers/staging/wfx/main.c b/drivers/staging/wfx/main.c
> > index 84adad64fc30..76b2ff7fc7fe 100644
> > --- a/drivers/staging/wfx/main.c
> > +++ b/drivers/staging/wfx/main.c
> > @@ -262,6 +262,16 @@ static int wfx_send_pdata_pds(struct wfx_dev *wdev)
> >         return ret;
> >  }
> > 
> > +static void wfx_free_common(void *data)
> > +{
> > +       struct wfx_dev *wdev = data;
> > +
> > +       mutex_destroy(&wdev->rx_stats_lock);
> > +       mutex_destroy(&wdev->conf_mutex);
> > +       wfx_tx_queues_deinit(wdev);
> > +       ieee80211_free_hw(wdev->hw);
> > +}
> > +
> >  struct wfx_dev *wfx_init_common(struct device *dev,
> >                                 const struct wfx_platform_data *pdata,
> >                                 const struct hwbus_ops *hwbus_ops,
> > @@ -332,17 +342,12 @@ struct wfx_dev *wfx_init_common(struct device *dev,
> >         wfx_init_hif_cmd(&wdev->hif_cmd);
> >         wfx_tx_queues_init(wdev);
> > 
> > +       if (devm_add_action_or_reset(dev, wfx_free_common, wdev))
> > +               return NULL;
> > +
> >         return wdev;
> >  }
> > 
> > -void wfx_free_common(struct wfx_dev *wdev)
> > -{
> > -       mutex_destroy(&wdev->rx_stats_lock);
> > -       mutex_destroy(&wdev->conf_mutex);
> > -       wfx_tx_queues_deinit(wdev);
> > -       ieee80211_free_hw(wdev->hw);
> > -}
> > -
> >  int wfx_probe(struct wfx_dev *wdev)
> >  {
> >         int i;
> > diff --git a/drivers/staging/wfx/main.h b/drivers/staging/wfx/main.h
> > index 875f8c227803..9c9410072def 100644
> > --- a/drivers/staging/wfx/main.h
> > +++ b/drivers/staging/wfx/main.h
> > @@ -34,7 +34,6 @@ struct wfx_dev *wfx_init_common(struct device *dev,
> >                                 const struct wfx_platform_data *pdata,
> >                                 const struct hwbus_ops *hwbus_ops,
> >                                 void *hwbus_priv);
> > -void wfx_free_common(struct wfx_dev *wdev);
> > 
> >  int wfx_probe(struct wfx_dev *wdev);
> >  void wfx_release(struct wfx_dev *wdev);
> > --
> > 2.20.1
> > 
> 
> Are you sure you can completely drop wfx_free_common()? If you want to
> use devres, I think that wfx_flush_irq_work() should call
> wfx_free_common(), no?

wfx_free_common() will be called by devres in the right order. That's ensured
by devm_add_action_or_reset() at the end of wfx_init_common().

Best Regards,
Michał Mirosław

Re: [PATCH v2 1/6] staging: wfx: fix init/remove vs IRQ race

[Michał Mirosław]

On Tue, Feb 11, 2020 at 11:19:18AM +0000, Jérôme Pouiller wrote:
> On Tuesday 11 February 2020 11:35:01 CET Michał Mirosław wrote:
> > Current code races in init/exit with interrupt handlers. This is noticed
> > by the warning below. Fix it by using devres for ordering allocations and
> > IRQ de/registration.
> > 
> > WARNING: CPU: 0 PID: 827 at drivers/staging/wfx/bus_spi.c:142 wfx_spi_irq_handler+0x5c/0x64 [wfx]
> > race condition in driver init/deinit
[...]
> > --- a/drivers/staging/wfx/bus_spi.c
> > +++ b/drivers/staging/wfx/bus_spi.c
> > @@ -154,6 +154,11 @@ static void wfx_spi_request_rx(struct work_struct *work)
> >         wfx_bh_request_rx(bus->core);
> >  }
> > 
> > +static void wfx_flush_irq_work(void *w)
> > +{
> > +       flush_work(w);
> > +}
> > +
> >  static size_t wfx_spi_align_size(void *priv, size_t size)
> >  {
> >         // Most of SPI controllers avoid DMA if buffer size is not 32bit aligned
> > @@ -207,22 +212,23 @@ static int wfx_spi_probe(struct spi_device *func)
> >                 udelay(2000);
> >         }
> > 
> > -       ret = devm_request_irq(&func->dev, func->irq, wfx_spi_irq_handler,
> > -                              IRQF_TRIGGER_RISING, "wfx", bus);
> > -       if (ret)
> > -               return ret;
> > -
> >         INIT_WORK(&bus->request_rx, wfx_spi_request_rx);
> >         bus->core = wfx_init_common(&func->dev, &wfx_spi_pdata,
> >                                     &wfx_spi_hwbus_ops, bus);
> >         if (!bus->core)
> >                 return -EIO;
> > 
> > -       ret = wfx_probe(bus->core);
> > +       ret = devm_add_action_or_reset(&func->dev, wfx_flush_irq_work,
> > +                                      &bus->request_rx);
> >         if (ret)
> > -               wfx_free_common(bus->core);
> > +               return ret;
> > 
> > -       return ret;
> > +       ret = devm_request_irq(&func->dev, func->irq, wfx_spi_irq_handler,
> > +                              IRQF_TRIGGER_RISING, "wfx", bus);
> > +       if (ret)
> > +               return ret;
> > +
> > +       return wfx_probe(bus->core);
> >  }
> > 
> >  static int wfx_spi_remove(struct spi_device *func)
> > @@ -230,11 +236,6 @@ static int wfx_spi_remove(struct spi_device *func)
> >         struct wfx_spi_priv *bus = spi_get_drvdata(func);
> > 
> >         wfx_release(bus->core);
> > -       wfx_free_common(bus->core);
> > -       // A few IRQ will be sent during device release. Hopefully, no IRQ
> > -       // should happen after wdev/wvif are released.
> > -       devm_free_irq(&func->dev, func->irq, bus);
> > -       flush_work(&bus->request_rx);
> >         return 0;
> >  }
> > 
> > diff --git a/drivers/staging/wfx/main.c b/drivers/staging/wfx/main.c
> > index 84adad64fc30..76b2ff7fc7fe 100644
> > --- a/drivers/staging/wfx/main.c
> > +++ b/drivers/staging/wfx/main.c
> > @@ -262,6 +262,16 @@ static int wfx_send_pdata_pds(struct wfx_dev *wdev)
> >         return ret;
> >  }
> > 
> > +static void wfx_free_common(void *data)
> > +{
> > +       struct wfx_dev *wdev = data;
> > +
> > +       mutex_destroy(&wdev->rx_stats_lock);
> > +       mutex_destroy(&wdev->conf_mutex);
> > +       wfx_tx_queues_deinit(wdev);
> > +       ieee80211_free_hw(wdev->hw);
> > +}
> > +
> >  struct wfx_dev *wfx_init_common(struct device *dev,
> >                                 const struct wfx_platform_data *pdata,
> >                                 const struct hwbus_ops *hwbus_ops,
> > @@ -332,17 +342,12 @@ struct wfx_dev *wfx_init_common(struct device *dev,
> >         wfx_init_hif_cmd(&wdev->hif_cmd);
> >         wfx_tx_queues_init(wdev);
> > 
> > +       if (devm_add_action_or_reset(dev, wfx_free_common, wdev))
> > +               return NULL;
> > +
> >         return wdev;
> >  }
> > 
> > -void wfx_free_common(struct wfx_dev *wdev)
> > -{
> > -       mutex_destroy(&wdev->rx_stats_lock);
> > -       mutex_destroy(&wdev->conf_mutex);
> > -       wfx_tx_queues_deinit(wdev);
> > -       ieee80211_free_hw(wdev->hw);
> > -}
> > -
> >  int wfx_probe(struct wfx_dev *wdev)
> >  {
> >         int i;
> > diff --git a/drivers/staging/wfx/main.h b/drivers/staging/wfx/main.h
> > index 875f8c227803..9c9410072def 100644
> > --- a/drivers/staging/wfx/main.h
> > +++ b/drivers/staging/wfx/main.h
> > @@ -34,7 +34,6 @@ struct wfx_dev *wfx_init_common(struct device *dev,
> >                                 const struct wfx_platform_data *pdata,
> >                                 const struct hwbus_ops *hwbus_ops,
> >                                 void *hwbus_priv);
> > -void wfx_free_common(struct wfx_dev *wdev);
> > 
> >  int wfx_probe(struct wfx_dev *wdev);
> >  void wfx_release(struct wfx_dev *wdev);
> > --
> > 2.20.1
> > 
> 
> Are you sure you can completely drop wfx_free_common()? If you want to
> use devres, I think that wfx_flush_irq_work() should call
> wfx_free_common(), no?

wfx_free_common() will be called by devres in the right order. That's ensured
by devm_add_action_or_reset() at the end of wfx_init_common().

Best Regards,
Michał Mirosław
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2 1/6] staging: wfx: fix init/remove vs IRQ race

[Jérôme Pouiller]

On Tuesday 11 February 2020 13:49:22 CET Michał Mirosław 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.
> 
> 
> On Tue, Feb 11, 2020 at 11:19:18AM +0000, Jérôme Pouiller wrote:
> > On Tuesday 11 February 2020 11:35:01 CET Michał Mirosław wrote:
> > > Current code races in init/exit with interrupt handlers. This is noticed
> > > by the warning below. Fix it by using devres for ordering allocations and
> > > IRQ de/registration.
> > >
> > > WARNING: CPU: 0 PID: 827 at drivers/staging/wfx/bus_spi.c:142 wfx_spi_irq_handler+0x5c/0x64 [wfx]
> > > race condition in driver init/deinit
> [...]
> > > --- a/drivers/staging/wfx/bus_spi.c
> > > +++ b/drivers/staging/wfx/bus_spi.c
> > > @@ -154,6 +154,11 @@ static void wfx_spi_request_rx(struct work_struct *work)
> > >         wfx_bh_request_rx(bus->core);
> > >  }
> > >
> > > +static void wfx_flush_irq_work(void *w)
> > > +{
> > > +       flush_work(w);
> > > +}
> > > +
> > >  static size_t wfx_spi_align_size(void *priv, size_t size)
> > >  {
> > >         // Most of SPI controllers avoid DMA if buffer size is not 32bit aligned
> > > @@ -207,22 +212,23 @@ static int wfx_spi_probe(struct spi_device *func)
> > >                 udelay(2000);
> > >         }
> > >
> > > -       ret = devm_request_irq(&func->dev, func->irq, wfx_spi_irq_handler,
> > > -                              IRQF_TRIGGER_RISING, "wfx", bus);
> > > -       if (ret)
> > > -               return ret;
> > > -
> > >         INIT_WORK(&bus->request_rx, wfx_spi_request_rx);
> > >         bus->core = wfx_init_common(&func->dev, &wfx_spi_pdata,
> > >                                     &wfx_spi_hwbus_ops, bus);
> > >         if (!bus->core)
> > >                 return -EIO;
> > >
> > > -       ret = wfx_probe(bus->core);
> > > +       ret = devm_add_action_or_reset(&func->dev, wfx_flush_irq_work,
> > > +                                      &bus->request_rx);
> > >         if (ret)
> > > -               wfx_free_common(bus->core);
> > > +               return ret;
> > >
> > > -       return ret;
> > > +       ret = devm_request_irq(&func->dev, func->irq, wfx_spi_irq_handler,
> > > +                              IRQF_TRIGGER_RISING, "wfx", bus);
> > > +       if (ret)
> > > +               return ret;
> > > +
> > > +       return wfx_probe(bus->core);
> > >  }
> > >
> > >  static int wfx_spi_remove(struct spi_device *func)
> > > @@ -230,11 +236,6 @@ static int wfx_spi_remove(struct spi_device *func)
> > >         struct wfx_spi_priv *bus = spi_get_drvdata(func);
> > >
> > >         wfx_release(bus->core);
> > > -       wfx_free_common(bus->core);
> > > -       // A few IRQ will be sent during device release. Hopefully, no IRQ
> > > -       // should happen after wdev/wvif are released.
> > > -       devm_free_irq(&func->dev, func->irq, bus);
> > > -       flush_work(&bus->request_rx);
> > >         return 0;
> > >  }
> > >
> > > diff --git a/drivers/staging/wfx/main.c b/drivers/staging/wfx/main.c
> > > index 84adad64fc30..76b2ff7fc7fe 100644
> > > --- a/drivers/staging/wfx/main.c
> > > +++ b/drivers/staging/wfx/main.c
> > > @@ -262,6 +262,16 @@ static int wfx_send_pdata_pds(struct wfx_dev *wdev)
> > >         return ret;
> > >  }
> > >
> > > +static void wfx_free_common(void *data)
> > > +{
> > > +       struct wfx_dev *wdev = data;
> > > +
> > > +       mutex_destroy(&wdev->rx_stats_lock);
> > > +       mutex_destroy(&wdev->conf_mutex);
> > > +       wfx_tx_queues_deinit(wdev);
> > > +       ieee80211_free_hw(wdev->hw);
> > > +}
> > > +
> > >  struct wfx_dev *wfx_init_common(struct device *dev,
> > >                                 const struct wfx_platform_data *pdata,
> > >                                 const struct hwbus_ops *hwbus_ops,
> > > @@ -332,17 +342,12 @@ struct wfx_dev *wfx_init_common(struct device *dev,
> > >         wfx_init_hif_cmd(&wdev->hif_cmd);
> > >         wfx_tx_queues_init(wdev);
> > >
> > > +       if (devm_add_action_or_reset(dev, wfx_free_common, wdev))
> > > +               return NULL;
> > > +
> > >         return wdev;
> > >  }
> > >
> > > -void wfx_free_common(struct wfx_dev *wdev)
> > > -{
> > > -       mutex_destroy(&wdev->rx_stats_lock);
> > > -       mutex_destroy(&wdev->conf_mutex);
> > > -       wfx_tx_queues_deinit(wdev);
> > > -       ieee80211_free_hw(wdev->hw);
> > > -}
> > > -
> > >  int wfx_probe(struct wfx_dev *wdev)
> > >  {
> > >         int i;
> > > diff --git a/drivers/staging/wfx/main.h b/drivers/staging/wfx/main.h
> > > index 875f8c227803..9c9410072def 100644
> > > --- a/drivers/staging/wfx/main.h
> > > +++ b/drivers/staging/wfx/main.h
> > > @@ -34,7 +34,6 @@ struct wfx_dev *wfx_init_common(struct device *dev,
> > >                                 const struct wfx_platform_data *pdata,
> > >                                 const struct hwbus_ops *hwbus_ops,
> > >                                 void *hwbus_priv);
> > > -void wfx_free_common(struct wfx_dev *wdev);
> > >
> > >  int wfx_probe(struct wfx_dev *wdev);
> > >  void wfx_release(struct wfx_dev *wdev);
> > > --
> > > 2.20.1
> > >
> >
> > Are you sure you can completely drop wfx_free_common()? If you want to
> > use devres, I think that wfx_flush_irq_work() should call
> > wfx_free_common(), no?
> 
> wfx_free_common() will be called by devres in the right order. That's ensured
> by devm_add_action_or_reset() at the end of wfx_init_common().
Indeed.

Reviewed-by: Jérôme Pouiller <jerome.pouiller@silabs.com>

-- 
Jérôme Pouiller

Re: [PATCH v2 1/6] staging: wfx: fix init/remove vs IRQ race

[Jérôme Pouiller]

On Tuesday 11 February 2020 13:49:22 CET Michał Mirosław 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.
> 
> 
> On Tue, Feb 11, 2020 at 11:19:18AM +0000, Jérôme Pouiller wrote:
> > On Tuesday 11 February 2020 11:35:01 CET Michał Mirosław wrote:
> > > Current code races in init/exit with interrupt handlers. This is noticed
> > > by the warning below. Fix it by using devres for ordering allocations and
> > > IRQ de/registration.
> > >
> > > WARNING: CPU: 0 PID: 827 at drivers/staging/wfx/bus_spi.c:142 wfx_spi_irq_handler+0x5c/0x64 [wfx]
> > > race condition in driver init/deinit
> [...]
> > > --- a/drivers/staging/wfx/bus_spi.c
> > > +++ b/drivers/staging/wfx/bus_spi.c
> > > @@ -154,6 +154,11 @@ static void wfx_spi_request_rx(struct work_struct *work)
> > >         wfx_bh_request_rx(bus->core);
> > >  }
> > >
> > > +static void wfx_flush_irq_work(void *w)
> > > +{
> > > +       flush_work(w);
> > > +}
> > > +
> > >  static size_t wfx_spi_align_size(void *priv, size_t size)
> > >  {
> > >         // Most of SPI controllers avoid DMA if buffer size is not 32bit aligned
> > > @@ -207,22 +212,23 @@ static int wfx_spi_probe(struct spi_device *func)
> > >                 udelay(2000);
> > >         }
> > >
> > > -       ret = devm_request_irq(&func->dev, func->irq, wfx_spi_irq_handler,
> > > -                              IRQF_TRIGGER_RISING, "wfx", bus);
> > > -       if (ret)
> > > -               return ret;
> > > -
> > >         INIT_WORK(&bus->request_rx, wfx_spi_request_rx);
> > >         bus->core = wfx_init_common(&func->dev, &wfx_spi_pdata,
> > >                                     &wfx_spi_hwbus_ops, bus);
> > >         if (!bus->core)
> > >                 return -EIO;
> > >
> > > -       ret = wfx_probe(bus->core);
> > > +       ret = devm_add_action_or_reset(&func->dev, wfx_flush_irq_work,
> > > +                                      &bus->request_rx);
> > >         if (ret)
> > > -               wfx_free_common(bus->core);
> > > +               return ret;
> > >
> > > -       return ret;
> > > +       ret = devm_request_irq(&func->dev, func->irq, wfx_spi_irq_handler,
> > > +                              IRQF_TRIGGER_RISING, "wfx", bus);
> > > +       if (ret)
> > > +               return ret;
> > > +
> > > +       return wfx_probe(bus->core);
> > >  }
> > >
> > >  static int wfx_spi_remove(struct spi_device *func)
> > > @@ -230,11 +236,6 @@ static int wfx_spi_remove(struct spi_device *func)
> > >         struct wfx_spi_priv *bus = spi_get_drvdata(func);
> > >
> > >         wfx_release(bus->core);
> > > -       wfx_free_common(bus->core);
> > > -       // A few IRQ will be sent during device release. Hopefully, no IRQ
> > > -       // should happen after wdev/wvif are released.
> > > -       devm_free_irq(&func->dev, func->irq, bus);
> > > -       flush_work(&bus->request_rx);
> > >         return 0;
> > >  }
> > >
> > > diff --git a/drivers/staging/wfx/main.c b/drivers/staging/wfx/main.c
> > > index 84adad64fc30..76b2ff7fc7fe 100644
> > > --- a/drivers/staging/wfx/main.c
> > > +++ b/drivers/staging/wfx/main.c
> > > @@ -262,6 +262,16 @@ static int wfx_send_pdata_pds(struct wfx_dev *wdev)
> > >         return ret;
> > >  }
> > >
> > > +static void wfx_free_common(void *data)
> > > +{
> > > +       struct wfx_dev *wdev = data;
> > > +
> > > +       mutex_destroy(&wdev->rx_stats_lock);
> > > +       mutex_destroy(&wdev->conf_mutex);
> > > +       wfx_tx_queues_deinit(wdev);
> > > +       ieee80211_free_hw(wdev->hw);
> > > +}
> > > +
> > >  struct wfx_dev *wfx_init_common(struct device *dev,
> > >                                 const struct wfx_platform_data *pdata,
> > >                                 const struct hwbus_ops *hwbus_ops,
> > > @@ -332,17 +342,12 @@ struct wfx_dev *wfx_init_common(struct device *dev,
> > >         wfx_init_hif_cmd(&wdev->hif_cmd);
> > >         wfx_tx_queues_init(wdev);
> > >
> > > +       if (devm_add_action_or_reset(dev, wfx_free_common, wdev))
> > > +               return NULL;
> > > +
> > >         return wdev;
> > >  }
> > >
> > > -void wfx_free_common(struct wfx_dev *wdev)
> > > -{
> > > -       mutex_destroy(&wdev->rx_stats_lock);
> > > -       mutex_destroy(&wdev->conf_mutex);
> > > -       wfx_tx_queues_deinit(wdev);
> > > -       ieee80211_free_hw(wdev->hw);
> > > -}
> > > -
> > >  int wfx_probe(struct wfx_dev *wdev)
> > >  {
> > >         int i;
> > > diff --git a/drivers/staging/wfx/main.h b/drivers/staging/wfx/main.h
> > > index 875f8c227803..9c9410072def 100644
> > > --- a/drivers/staging/wfx/main.h
> > > +++ b/drivers/staging/wfx/main.h
> > > @@ -34,7 +34,6 @@ struct wfx_dev *wfx_init_common(struct device *dev,
> > >                                 const struct wfx_platform_data *pdata,
> > >                                 const struct hwbus_ops *hwbus_ops,
> > >                                 void *hwbus_priv);
> > > -void wfx_free_common(struct wfx_dev *wdev);
> > >
> > >  int wfx_probe(struct wfx_dev *wdev);
> > >  void wfx_release(struct wfx_dev *wdev);
> > > --
> > > 2.20.1
> > >
> >
> > Are you sure you can completely drop wfx_free_common()? If you want to
> > use devres, I think that wfx_flush_irq_work() should call
> > wfx_free_common(), no?
> 
> wfx_free_common() will be called by devres in the right order. That's ensured
> by devm_add_action_or_reset() at the end of wfx_init_common().
Indeed.

Reviewed-by: Jérôme Pouiller <jerome.pouiller@silabs.com>

-- 
Jérôme Pouiller

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 2/6] staging: wfx: annotate nested gc_list vs tx queue locking

[Michał Mirosław]

Lockdep is complaining about recursive locking, because it can't make
a difference between locked skb_queues. Annotate nested locks and avoid
double bh_disable/enable.

[...]
insmod/815 is trying to acquire lock:
cb7d6418 (&(&list->lock)->rlock){+...}, at: wfx_tx_queues_clear+0xfc/0x198 [wfx]

but task is already holding lock:
cb7d61f4 (&(&list->lock)->rlock){+...}, at: wfx_tx_queues_clear+0xa0/0x198 [wfx]

[...]
Possible unsafe locking scenario:

      CPU0
      ----
 lock(&(&list->lock)->rlock);
 lock(&(&list->lock)->rlock);

Cc: stable@vger.kernel.org
Fixes: 9bca45f3d692 ("staging: wfx: allow to send 802.11 frames")
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/staging/wfx/queue.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/wfx/queue.c b/drivers/staging/wfx/queue.c
index 0bcc61feee1d..51d6c55ae91f 100644
--- a/drivers/staging/wfx/queue.c
+++ b/drivers/staging/wfx/queue.c
@@ -130,12 +130,12 @@ static void wfx_tx_queue_clear(struct wfx_dev *wdev, struct wfx_queue *queue,
 	spin_lock_bh(&queue->queue.lock);
 	while ((item = __skb_dequeue(&queue->queue)) != NULL)
 		skb_queue_head(gc_list, item);
-	spin_lock_bh(&stats->pending.lock);
+	spin_lock_nested(&stats->pending.lock, 1);
 	for (i = 0; i < ARRAY_SIZE(stats->link_map_cache); ++i) {
 		stats->link_map_cache[i] -= queue->link_map_cache[i];
 		queue->link_map_cache[i] = 0;
 	}
-	spin_unlock_bh(&stats->pending.lock);
+	spin_unlock(&stats->pending.lock);
 	spin_unlock_bh(&queue->queue.lock);
 }
 
@@ -207,9 +207,9 @@ void wfx_tx_queue_put(struct wfx_dev *wdev, struct wfx_queue *queue,
 
 	++queue->link_map_cache[tx_priv->link_id];
 
-	spin_lock_bh(&stats->pending.lock);
+	spin_lock_nested(&stats->pending.lock, 1);
 	++stats->link_map_cache[tx_priv->link_id];
-	spin_unlock_bh(&stats->pending.lock);
+	spin_unlock(&stats->pending.lock);
 	spin_unlock_bh(&queue->queue.lock);
 }
 
@@ -237,11 +237,11 @@ static struct sk_buff *wfx_tx_queue_get(struct wfx_dev *wdev,
 		__skb_unlink(skb, &queue->queue);
 		--queue->link_map_cache[tx_priv->link_id];
 
-		spin_lock_bh(&stats->pending.lock);
+		spin_lock_nested(&stats->pending.lock, 1);
 		__skb_queue_tail(&stats->pending, skb);
 		if (!--stats->link_map_cache[tx_priv->link_id])
 			wakeup_stats = true;
-		spin_unlock_bh(&stats->pending.lock);
+		spin_unlock(&stats->pending.lock);
 	}
 	spin_unlock_bh(&queue->queue.lock);
 	if (wakeup_stats)
@@ -259,10 +259,10 @@ int wfx_pending_requeue(struct wfx_dev *wdev, struct sk_buff *skb)
 	spin_lock_bh(&queue->queue.lock);
 	++queue->link_map_cache[tx_priv->link_id];
 
-	spin_lock_bh(&stats->pending.lock);
+	spin_lock_nested(&stats->pending.lock, 1);
 	++stats->link_map_cache[tx_priv->link_id];
 	__skb_unlink(skb, &stats->pending);
-	spin_unlock_bh(&stats->pending.lock);
+	spin_unlock(&stats->pending.lock);
 	__skb_queue_tail(&queue->queue, skb);
 	spin_unlock_bh(&queue->queue.lock);
 	return 0;
-- 
2.20.1

[PATCH v2 2/6] staging: wfx: annotate nested gc_list vs tx queue locking

[Michał Mirosław]

Lockdep is complaining about recursive locking, because it can't make
a difference between locked skb_queues. Annotate nested locks and avoid
double bh_disable/enable.

[...]
insmod/815 is trying to acquire lock:
cb7d6418 (&(&list->lock)->rlock){+...}, at: wfx_tx_queues_clear+0xfc/0x198 [wfx]

but task is already holding lock:
cb7d61f4 (&(&list->lock)->rlock){+...}, at: wfx_tx_queues_clear+0xa0/0x198 [wfx]

[...]
Possible unsafe locking scenario:

      CPU0
      ----
 lock(&(&list->lock)->rlock);
 lock(&(&list->lock)->rlock);

Cc: stable@vger.kernel.org
Fixes: 9bca45f3d692 ("staging: wfx: allow to send 802.11 frames")
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/staging/wfx/queue.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/wfx/queue.c b/drivers/staging/wfx/queue.c
index 0bcc61feee1d..51d6c55ae91f 100644
--- a/drivers/staging/wfx/queue.c
+++ b/drivers/staging/wfx/queue.c
@@ -130,12 +130,12 @@ static void wfx_tx_queue_clear(struct wfx_dev *wdev, struct wfx_queue *queue,
 	spin_lock_bh(&queue->queue.lock);
 	while ((item = __skb_dequeue(&queue->queue)) != NULL)
 		skb_queue_head(gc_list, item);
-	spin_lock_bh(&stats->pending.lock);
+	spin_lock_nested(&stats->pending.lock, 1);
 	for (i = 0; i < ARRAY_SIZE(stats->link_map_cache); ++i) {
 		stats->link_map_cache[i] -= queue->link_map_cache[i];
 		queue->link_map_cache[i] = 0;
 	}
-	spin_unlock_bh(&stats->pending.lock);
+	spin_unlock(&stats->pending.lock);
 	spin_unlock_bh(&queue->queue.lock);
 }
 
@@ -207,9 +207,9 @@ void wfx_tx_queue_put(struct wfx_dev *wdev, struct wfx_queue *queue,
 
 	++queue->link_map_cache[tx_priv->link_id];
 
-	spin_lock_bh(&stats->pending.lock);
+	spin_lock_nested(&stats->pending.lock, 1);
 	++stats->link_map_cache[tx_priv->link_id];
-	spin_unlock_bh(&stats->pending.lock);
+	spin_unlock(&stats->pending.lock);
 	spin_unlock_bh(&queue->queue.lock);
 }
 
@@ -237,11 +237,11 @@ static struct sk_buff *wfx_tx_queue_get(struct wfx_dev *wdev,
 		__skb_unlink(skb, &queue->queue);
 		--queue->link_map_cache[tx_priv->link_id];
 
-		spin_lock_bh(&stats->pending.lock);
+		spin_lock_nested(&stats->pending.lock, 1);
 		__skb_queue_tail(&stats->pending, skb);
 		if (!--stats->link_map_cache[tx_priv->link_id])
 			wakeup_stats = true;
-		spin_unlock_bh(&stats->pending.lock);
+		spin_unlock(&stats->pending.lock);
 	}
 	spin_unlock_bh(&queue->queue.lock);
 	if (wakeup_stats)
@@ -259,10 +259,10 @@ int wfx_pending_requeue(struct wfx_dev *wdev, struct sk_buff *skb)
 	spin_lock_bh(&queue->queue.lock);
 	++queue->link_map_cache[tx_priv->link_id];
 
-	spin_lock_bh(&stats->pending.lock);
+	spin_lock_nested(&stats->pending.lock, 1);
 	++stats->link_map_cache[tx_priv->link_id];
 	__skb_unlink(skb, &stats->pending);
-	spin_unlock_bh(&stats->pending.lock);
+	spin_unlock(&stats->pending.lock);
 	__skb_queue_tail(&queue->queue, skb);
 	spin_unlock_bh(&queue->queue.lock);
 	return 0;
-- 
2.20.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 3/6] staging: wfx: add proper "compatible" string

[Michał Mirosław]

Add "compatible" string matching "vendor,chip" template and proper
GPIO flags handling. Keep support for old name and reset polarity
for older devicetrees.

Cc: stable@vger.kernel.org   # d3a5bcb4a17f ("gpio: add gpiod_toggle_active_low()")
Cc: stable@vger.kernel.org
Fixes: 0096214a59a7 ("staging: wfx: add support for I/O access")
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 .../bindings/net/wireless/siliabs,wfx.txt          |  7 ++++---
 drivers/staging/wfx/bus_spi.c                      | 14 ++++++++++----
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/wfx/Documentation/devicetree/bindings/net/wireless/siliabs,wfx.txt b/drivers/staging/wfx/Documentation/devicetree/bindings/net/wireless/siliabs,wfx.txt
index 26de6762b942..52f97673da1e 100644
--- a/drivers/staging/wfx/Documentation/devicetree/bindings/net/wireless/siliabs,wfx.txt
+++ b/drivers/staging/wfx/Documentation/devicetree/bindings/net/wireless/siliabs,wfx.txt
@@ -6,7 +6,7 @@ SPI
 You have to declare the WFxxx chip in your device tree.
 
 Required properties:
- - compatible: Should be "silabs,wfx-spi"
+ - compatible: Should be "silabs,wf200"
  - reg: Chip select address of device
  - spi-max-frequency: Maximum SPI clocking speed of device in Hz
  - interrupts-extended: Should contain interrupt line (interrupt-parent +
@@ -15,6 +15,7 @@ Required properties:
 Optional properties:
  - reset-gpios: phandle of gpio that will be used to reset chip during probe.
    Without this property, you may encounter issues with warm boot.
+   (Legacy: when compatible == "silabs,wfx-spi", the gpio is inverted.)
 
 Please consult Documentation/devicetree/bindings/spi/spi-bus.txt for optional
 SPI connection related properties,
@@ -23,12 +24,12 @@ Example:
 
 &spi1 {
 	wfx {
-		compatible = "silabs,wfx-spi";
+		compatible = "silabs,wf200";
 		pinctrl-names = "default";
 		pinctrl-0 = <&wfx_irq &wfx_gpios>;
 		interrupts-extended = <&gpio 16 IRQ_TYPE_EDGE_RISING>;
 		wakeup-gpios = <&gpio 12 GPIO_ACTIVE_HIGH>;
-		reset-gpios = <&gpio 13 GPIO_ACTIVE_HIGH>;
+		reset-gpios = <&gpio 13 GPIO_ACTIVE_LOW>;
 		reg = <0>;
 		spi-max-frequency = <42000000>;
 	};
diff --git a/drivers/staging/wfx/bus_spi.c b/drivers/staging/wfx/bus_spi.c
index 605ad74068b7..d6a75bd61595 100644
--- a/drivers/staging/wfx/bus_spi.c
+++ b/drivers/staging/wfx/bus_spi.c
@@ -27,6 +27,8 @@ MODULE_PARM_DESC(gpio_reset, "gpio number for reset. -1 for none.");
 #define SET_WRITE 0x7FFF        /* usage: and operation */
 #define SET_READ 0x8000         /* usage: or operation */
 
+#define WFX_RESET_INVERTED 1
+
 static const struct wfx_platform_data wfx_spi_pdata = {
 	.file_fw = "wfm_wf200",
 	.file_pds = "wf200.pds",
@@ -206,9 +208,11 @@ static int wfx_spi_probe(struct spi_device *func)
 	if (!bus->gpio_reset) {
 		dev_warn(&func->dev, "try to load firmware anyway\n");
 	} else {
-		gpiod_set_value(bus->gpio_reset, 0);
-		udelay(100);
+		if (spi_get_device_id(func)->driver_data & WFX_RESET_INVERTED)
+			gpiod_toggle_active_low(bus->gpio_reset);
 		gpiod_set_value(bus->gpio_reset, 1);
+		udelay(100);
+		gpiod_set_value(bus->gpio_reset, 0);
 		udelay(2000);
 	}
 
@@ -245,14 +249,16 @@ static int wfx_spi_remove(struct spi_device *func)
  * stripped.
  */
 static const struct spi_device_id wfx_spi_id[] = {
-	{ "wfx-spi", 0 },
+	{ "wfx-spi", WFX_RESET_INVERTED },
+	{ "wf200", 0 },
 	{ },
 };
 MODULE_DEVICE_TABLE(spi, wfx_spi_id);
 
 #ifdef CONFIG_OF
 static const struct of_device_id wfx_spi_of_match[] = {
-	{ .compatible = "silabs,wfx-spi" },
+	{ .compatible = "silabs,wfx-spi", .data = (void *)WFX_RESET_INVERTED },
+	{ .compatible = "silabs,wf200" },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, wfx_spi_of_match);
-- 
2.20.1

[PATCH v2 3/6] staging: wfx: add proper "compatible" string

[Michał Mirosław]

Add "compatible" string matching "vendor,chip" template and proper
GPIO flags handling. Keep support for old name and reset polarity
for older devicetrees.

Cc: stable@vger.kernel.org   # d3a5bcb4a17f ("gpio: add gpiod_toggle_active_low()")
Cc: stable@vger.kernel.org
Fixes: 0096214a59a7 ("staging: wfx: add support for I/O access")
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 .../bindings/net/wireless/siliabs,wfx.txt          |  7 ++++---
 drivers/staging/wfx/bus_spi.c                      | 14 ++++++++++----
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/wfx/Documentation/devicetree/bindings/net/wireless/siliabs,wfx.txt b/drivers/staging/wfx/Documentation/devicetree/bindings/net/wireless/siliabs,wfx.txt
index 26de6762b942..52f97673da1e 100644
--- a/drivers/staging/wfx/Documentation/devicetree/bindings/net/wireless/siliabs,wfx.txt
+++ b/drivers/staging/wfx/Documentation/devicetree/bindings/net/wireless/siliabs,wfx.txt
@@ -6,7 +6,7 @@ SPI
 You have to declare the WFxxx chip in your device tree.
 
 Required properties:
- - compatible: Should be "silabs,wfx-spi"
+ - compatible: Should be "silabs,wf200"
  - reg: Chip select address of device
  - spi-max-frequency: Maximum SPI clocking speed of device in Hz
  - interrupts-extended: Should contain interrupt line (interrupt-parent +
@@ -15,6 +15,7 @@ Required properties:
 Optional properties:
  - reset-gpios: phandle of gpio that will be used to reset chip during probe.
    Without this property, you may encounter issues with warm boot.
+   (Legacy: when compatible == "silabs,wfx-spi", the gpio is inverted.)
 
 Please consult Documentation/devicetree/bindings/spi/spi-bus.txt for optional
 SPI connection related properties,
@@ -23,12 +24,12 @@ Example:
 
 &spi1 {
 	wfx {
-		compatible = "silabs,wfx-spi";
+		compatible = "silabs,wf200";
 		pinctrl-names = "default";
 		pinctrl-0 = <&wfx_irq &wfx_gpios>;
 		interrupts-extended = <&gpio 16 IRQ_TYPE_EDGE_RISING>;
 		wakeup-gpios = <&gpio 12 GPIO_ACTIVE_HIGH>;
-		reset-gpios = <&gpio 13 GPIO_ACTIVE_HIGH>;
+		reset-gpios = <&gpio 13 GPIO_ACTIVE_LOW>;
 		reg = <0>;
 		spi-max-frequency = <42000000>;
 	};
diff --git a/drivers/staging/wfx/bus_spi.c b/drivers/staging/wfx/bus_spi.c
index 605ad74068b7..d6a75bd61595 100644
--- a/drivers/staging/wfx/bus_spi.c
+++ b/drivers/staging/wfx/bus_spi.c
@@ -27,6 +27,8 @@ MODULE_PARM_DESC(gpio_reset, "gpio number for reset. -1 for none.");
 #define SET_WRITE 0x7FFF        /* usage: and operation */
 #define SET_READ 0x8000         /* usage: or operation */
 
+#define WFX_RESET_INVERTED 1
+
 static const struct wfx_platform_data wfx_spi_pdata = {
 	.file_fw = "wfm_wf200",
 	.file_pds = "wf200.pds",
@@ -206,9 +208,11 @@ static int wfx_spi_probe(struct spi_device *func)
 	if (!bus->gpio_reset) {
 		dev_warn(&func->dev, "try to load firmware anyway\n");
 	} else {
-		gpiod_set_value(bus->gpio_reset, 0);
-		udelay(100);
+		if (spi_get_device_id(func)->driver_data & WFX_RESET_INVERTED)
+			gpiod_toggle_active_low(bus->gpio_reset);
 		gpiod_set_value(bus->gpio_reset, 1);
+		udelay(100);
+		gpiod_set_value(bus->gpio_reset, 0);
 		udelay(2000);
 	}
 
@@ -245,14 +249,16 @@ static int wfx_spi_remove(struct spi_device *func)
  * stripped.
  */
 static const struct spi_device_id wfx_spi_id[] = {
-	{ "wfx-spi", 0 },
+	{ "wfx-spi", WFX_RESET_INVERTED },
+	{ "wf200", 0 },
 	{ },
 };
 MODULE_DEVICE_TABLE(spi, wfx_spi_id);
 
 #ifdef CONFIG_OF
 static const struct of_device_id wfx_spi_of_match[] = {
-	{ .compatible = "silabs,wfx-spi" },
+	{ .compatible = "silabs,wfx-spi", .data = (void *)WFX_RESET_INVERTED },
+	{ .compatible = "silabs,wf200" },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, wfx_spi_of_match);
-- 
2.20.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 4/6] staging: wfx: follow compatible = vendor,chip format

[Michał Mirosław]

As for SPI, follow "vendor,chip" format 'compatible' string also for
SDIO bus.

Fixes: 0096214a59a7 ("staging: wfx: add support for I/O access")
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 .../devicetree/bindings/net/wireless/siliabs,wfx.txt          | 4 ++--
 drivers/staging/wfx/bus_sdio.c                                | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/wfx/Documentation/devicetree/bindings/net/wireless/siliabs,wfx.txt b/drivers/staging/wfx/Documentation/devicetree/bindings/net/wireless/siliabs,wfx.txt
index 52f97673da1e..ffec79c14786 100644
--- a/drivers/staging/wfx/Documentation/devicetree/bindings/net/wireless/siliabs,wfx.txt
+++ b/drivers/staging/wfx/Documentation/devicetree/bindings/net/wireless/siliabs,wfx.txt
@@ -45,7 +45,7 @@ case. Thus declaring WFxxx chip in device tree is strongly recommended (and may
 become mandatory in the future).
 
 Required properties:
- - compatible: Should be "silabs,wfx-sdio"
+ - compatible: Should be "silabs,wf200"
  - reg: Should be 1
 
 In addition, it is recommended to declare a mmc-pwrseq on SDIO host above WFx.
@@ -71,7 +71,7 @@ Example:
 	#size = <0>;
 
 	mmc@1 {
-		compatible = "silabs,wfx-sdio";
+		compatible = "silabs,wf200";
 		reg = <1>;
 		pinctrl-names = "default";
 		pinctrl-0 = <&wfx_wakeup>;
diff --git a/drivers/staging/wfx/bus_sdio.c b/drivers/staging/wfx/bus_sdio.c
index 5450bd5e1b5d..dedc3ff58d3e 100644
--- a/drivers/staging/wfx/bus_sdio.c
+++ b/drivers/staging/wfx/bus_sdio.c
@@ -251,6 +251,7 @@ MODULE_DEVICE_TABLE(sdio, wfx_sdio_ids);
 #ifdef CONFIG_OF
 static const struct of_device_id wfx_sdio_of_match[] = {
 	{ .compatible = "silabs,wfx-sdio" },
+	{ .compatible = "silabs,wf200" },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, wfx_sdio_of_match);
-- 
2.20.1

[PATCH v2 4/6] staging: wfx: follow compatible = vendor,chip format

[Michał Mirosław]

As for SPI, follow "vendor,chip" format 'compatible' string also for
SDIO bus.

Fixes: 0096214a59a7 ("staging: wfx: add support for I/O access")
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 .../devicetree/bindings/net/wireless/siliabs,wfx.txt          | 4 ++--
 drivers/staging/wfx/bus_sdio.c                                | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/wfx/Documentation/devicetree/bindings/net/wireless/siliabs,wfx.txt b/drivers/staging/wfx/Documentation/devicetree/bindings/net/wireless/siliabs,wfx.txt
index 52f97673da1e..ffec79c14786 100644
--- a/drivers/staging/wfx/Documentation/devicetree/bindings/net/wireless/siliabs,wfx.txt
+++ b/drivers/staging/wfx/Documentation/devicetree/bindings/net/wireless/siliabs,wfx.txt
@@ -45,7 +45,7 @@ case. Thus declaring WFxxx chip in device tree is strongly recommended (and may
 become mandatory in the future).
 
 Required properties:
- - compatible: Should be "silabs,wfx-sdio"
+ - compatible: Should be "silabs,wf200"
  - reg: Should be 1
 
 In addition, it is recommended to declare a mmc-pwrseq on SDIO host above WFx.
@@ -71,7 +71,7 @@ Example:
 	#size = <0>;
 
 	mmc@1 {
-		compatible = "silabs,wfx-sdio";
+		compatible = "silabs,wf200";
 		reg = <1>;
 		pinctrl-names = "default";
 		pinctrl-0 = <&wfx_wakeup>;
diff --git a/drivers/staging/wfx/bus_sdio.c b/drivers/staging/wfx/bus_sdio.c
index 5450bd5e1b5d..dedc3ff58d3e 100644
--- a/drivers/staging/wfx/bus_sdio.c
+++ b/drivers/staging/wfx/bus_sdio.c
@@ -251,6 +251,7 @@ MODULE_DEVICE_TABLE(sdio, wfx_sdio_ids);
 #ifdef CONFIG_OF
 static const struct of_device_id wfx_sdio_of_match[] = {
 	{ .compatible = "silabs,wfx-sdio" },
+	{ .compatible = "silabs,wf200" },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, wfx_sdio_of_match);
-- 
2.20.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 5/6] staging: wfx: use sleeping gpio accessors

[Michał Mirosław]

Driver calls GPIO get/set only from non-atomic context and so can use any
GPIOs.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/staging/wfx/bh.c      | 6 +++---
 drivers/staging/wfx/bus_spi.c | 4 ++--
 drivers/staging/wfx/main.c    | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/wfx/bh.c b/drivers/staging/wfx/bh.c
index 983c41d1fe7c..c6319ab7e71a 100644
--- a/drivers/staging/wfx/bh.c
+++ b/drivers/staging/wfx/bh.c
@@ -20,10 +20,10 @@ static void device_wakeup(struct wfx_dev *wdev)
 {
 	if (!wdev->pdata.gpio_wakeup)
 		return;
-	if (gpiod_get_value(wdev->pdata.gpio_wakeup))
+	if (gpiod_get_value_cansleep(wdev->pdata.gpio_wakeup))
 		return;
 
-	gpiod_set_value(wdev->pdata.gpio_wakeup, 1);
+	gpiod_set_value_cansleep(wdev->pdata.gpio_wakeup, 1);
 	if (wfx_api_older_than(wdev, 1, 4)) {
 		if (!completion_done(&wdev->hif.ctrl_ready))
 			udelay(2000);
@@ -45,7 +45,7 @@ static void device_release(struct wfx_dev *wdev)
 	if (!wdev->pdata.gpio_wakeup)
 		return;
 
-	gpiod_set_value(wdev->pdata.gpio_wakeup, 0);
+	gpiod_set_value_cansleep(wdev->pdata.gpio_wakeup, 0);
 }
 
 static int rx_helper(struct wfx_dev *wdev, size_t read_len, int *is_cnf)
diff --git a/drivers/staging/wfx/bus_spi.c b/drivers/staging/wfx/bus_spi.c
index d6a75bd61595..e3cd12592662 100644
--- a/drivers/staging/wfx/bus_spi.c
+++ b/drivers/staging/wfx/bus_spi.c
@@ -210,9 +210,9 @@ static int wfx_spi_probe(struct spi_device *func)
 	} else {
 		if (spi_get_device_id(func)->driver_data & WFX_RESET_INVERTED)
 			gpiod_toggle_active_low(bus->gpio_reset);
-		gpiod_set_value(bus->gpio_reset, 1);
+		gpiod_set_value_cansleep(bus->gpio_reset, 1);
 		udelay(100);
-		gpiod_set_value(bus->gpio_reset, 0);
+		gpiod_set_value_cansleep(bus->gpio_reset, 0);
 		udelay(2000);
 	}
 
diff --git a/drivers/staging/wfx/main.c b/drivers/staging/wfx/main.c
index 76b2ff7fc7fe..3c4c240229ad 100644
--- a/drivers/staging/wfx/main.c
+++ b/drivers/staging/wfx/main.c
@@ -425,7 +425,7 @@ int wfx_probe(struct wfx_dev *wdev)
 			"enable 'quiescent' power mode with gpio %d and PDS file %s\n",
 			desc_to_gpio(wdev->pdata.gpio_wakeup),
 			wdev->pdata.file_pds);
-		gpiod_set_value(wdev->pdata.gpio_wakeup, 1);
+		gpiod_set_value_cansleep(wdev->pdata.gpio_wakeup, 1);
 		control_reg_write(wdev, 0);
 		hif_set_operational_mode(wdev, HIF_OP_POWER_MODE_QUIESCENT);
 	} else {
-- 
2.20.1

[PATCH v2 5/6] staging: wfx: use sleeping gpio accessors

[Michał Mirosław]

Driver calls GPIO get/set only from non-atomic context and so can use any
GPIOs.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/staging/wfx/bh.c      | 6 +++---
 drivers/staging/wfx/bus_spi.c | 4 ++--
 drivers/staging/wfx/main.c    | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/wfx/bh.c b/drivers/staging/wfx/bh.c
index 983c41d1fe7c..c6319ab7e71a 100644
--- a/drivers/staging/wfx/bh.c
+++ b/drivers/staging/wfx/bh.c
@@ -20,10 +20,10 @@ static void device_wakeup(struct wfx_dev *wdev)
 {
 	if (!wdev->pdata.gpio_wakeup)
 		return;
-	if (gpiod_get_value(wdev->pdata.gpio_wakeup))
+	if (gpiod_get_value_cansleep(wdev->pdata.gpio_wakeup))
 		return;
 
-	gpiod_set_value(wdev->pdata.gpio_wakeup, 1);
+	gpiod_set_value_cansleep(wdev->pdata.gpio_wakeup, 1);
 	if (wfx_api_older_than(wdev, 1, 4)) {
 		if (!completion_done(&wdev->hif.ctrl_ready))
 			udelay(2000);
@@ -45,7 +45,7 @@ static void device_release(struct wfx_dev *wdev)
 	if (!wdev->pdata.gpio_wakeup)
 		return;
 
-	gpiod_set_value(wdev->pdata.gpio_wakeup, 0);
+	gpiod_set_value_cansleep(wdev->pdata.gpio_wakeup, 0);
 }
 
 static int rx_helper(struct wfx_dev *wdev, size_t read_len, int *is_cnf)
diff --git a/drivers/staging/wfx/bus_spi.c b/drivers/staging/wfx/bus_spi.c
index d6a75bd61595..e3cd12592662 100644
--- a/drivers/staging/wfx/bus_spi.c
+++ b/drivers/staging/wfx/bus_spi.c
@@ -210,9 +210,9 @@ static int wfx_spi_probe(struct spi_device *func)
 	} else {
 		if (spi_get_device_id(func)->driver_data & WFX_RESET_INVERTED)
 			gpiod_toggle_active_low(bus->gpio_reset);
-		gpiod_set_value(bus->gpio_reset, 1);
+		gpiod_set_value_cansleep(bus->gpio_reset, 1);
 		udelay(100);
-		gpiod_set_value(bus->gpio_reset, 0);
+		gpiod_set_value_cansleep(bus->gpio_reset, 0);
 		udelay(2000);
 	}
 
diff --git a/drivers/staging/wfx/main.c b/drivers/staging/wfx/main.c
index 76b2ff7fc7fe..3c4c240229ad 100644
--- a/drivers/staging/wfx/main.c
+++ b/drivers/staging/wfx/main.c
@@ -425,7 +425,7 @@ int wfx_probe(struct wfx_dev *wdev)
 			"enable 'quiescent' power mode with gpio %d and PDS file %s\n",
 			desc_to_gpio(wdev->pdata.gpio_wakeup),
 			wdev->pdata.file_pds);
-		gpiod_set_value(wdev->pdata.gpio_wakeup, 1);
+		gpiod_set_value_cansleep(wdev->pdata.gpio_wakeup, 1);
 		control_reg_write(wdev, 0);
 		hif_set_operational_mode(wdev, HIF_OP_POWER_MODE_QUIESCENT);
 	} else {
-- 
2.20.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v2 6/6] staging: wfx: use more power-efficient sleep for reset

[Michał Mirosław]

Replace udelay() with usleep_range() as all uses are in a sleepable context.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/staging/wfx/bh.c      | 2 +-
 drivers/staging/wfx/bus_spi.c | 4 ++--
 drivers/staging/wfx/hwio.c    | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/wfx/bh.c b/drivers/staging/wfx/bh.c
index c6319ab7e71a..9fcab00a3733 100644
--- a/drivers/staging/wfx/bh.c
+++ b/drivers/staging/wfx/bh.c
@@ -26,7 +26,7 @@ static void device_wakeup(struct wfx_dev *wdev)
 	gpiod_set_value_cansleep(wdev->pdata.gpio_wakeup, 1);
 	if (wfx_api_older_than(wdev, 1, 4)) {
 		if (!completion_done(&wdev->hif.ctrl_ready))
-			udelay(2000);
+			usleep_range(2000, 2500);
 	} else {
 		// completion.h does not provide any function to wait
 		// completion without consume it (a kind of
diff --git a/drivers/staging/wfx/bus_spi.c b/drivers/staging/wfx/bus_spi.c
index e3cd12592662..61e99b09decb 100644
--- a/drivers/staging/wfx/bus_spi.c
+++ b/drivers/staging/wfx/bus_spi.c
@@ -211,9 +211,9 @@ static int wfx_spi_probe(struct spi_device *func)
 		if (spi_get_device_id(func)->driver_data & WFX_RESET_INVERTED)
 			gpiod_toggle_active_low(bus->gpio_reset);
 		gpiod_set_value_cansleep(bus->gpio_reset, 1);
-		udelay(100);
+		usleep_range(100, 150);
 		gpiod_set_value_cansleep(bus->gpio_reset, 0);
-		udelay(2000);
+		usleep_range(2000, 2500);
 	}
 
 	INIT_WORK(&bus->request_rx, wfx_spi_request_rx);
diff --git a/drivers/staging/wfx/hwio.c b/drivers/staging/wfx/hwio.c
index 47e04c59ed93..d3a141d95a0e 100644
--- a/drivers/staging/wfx/hwio.c
+++ b/drivers/staging/wfx/hwio.c
@@ -142,7 +142,7 @@ static int indirect_read(struct wfx_dev *wdev, int reg, u32 addr, void *buf,
 			goto err;
 		if (!(cfg & prefetch))
 			break;
-		udelay(200);
+		usleep_range(200, 250);
 	}
 	if (i == 20) {
 		ret = -ETIMEDOUT;
-- 
2.20.1

[PATCH v2 6/6] staging: wfx: use more power-efficient sleep for reset

[Michał Mirosław]

Replace udelay() with usleep_range() as all uses are in a sleepable context.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/staging/wfx/bh.c      | 2 +-
 drivers/staging/wfx/bus_spi.c | 4 ++--
 drivers/staging/wfx/hwio.c    | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/wfx/bh.c b/drivers/staging/wfx/bh.c
index c6319ab7e71a..9fcab00a3733 100644
--- a/drivers/staging/wfx/bh.c
+++ b/drivers/staging/wfx/bh.c
@@ -26,7 +26,7 @@ static void device_wakeup(struct wfx_dev *wdev)
 	gpiod_set_value_cansleep(wdev->pdata.gpio_wakeup, 1);
 	if (wfx_api_older_than(wdev, 1, 4)) {
 		if (!completion_done(&wdev->hif.ctrl_ready))
-			udelay(2000);
+			usleep_range(2000, 2500);
 	} else {
 		// completion.h does not provide any function to wait
 		// completion without consume it (a kind of
diff --git a/drivers/staging/wfx/bus_spi.c b/drivers/staging/wfx/bus_spi.c
index e3cd12592662..61e99b09decb 100644
--- a/drivers/staging/wfx/bus_spi.c
+++ b/drivers/staging/wfx/bus_spi.c
@@ -211,9 +211,9 @@ static int wfx_spi_probe(struct spi_device *func)
 		if (spi_get_device_id(func)->driver_data & WFX_RESET_INVERTED)
 			gpiod_toggle_active_low(bus->gpio_reset);
 		gpiod_set_value_cansleep(bus->gpio_reset, 1);
-		udelay(100);
+		usleep_range(100, 150);
 		gpiod_set_value_cansleep(bus->gpio_reset, 0);
-		udelay(2000);
+		usleep_range(2000, 2500);
 	}
 
 	INIT_WORK(&bus->request_rx, wfx_spi_request_rx);
diff --git a/drivers/staging/wfx/hwio.c b/drivers/staging/wfx/hwio.c
index 47e04c59ed93..d3a141d95a0e 100644
--- a/drivers/staging/wfx/hwio.c
+++ b/drivers/staging/wfx/hwio.c
@@ -142,7 +142,7 @@ static int indirect_read(struct wfx_dev *wdev, int reg, u32 addr, void *buf,
 			goto err;
 		if (!(cfg & prefetch))
 			break;
-		udelay(200);
+		usleep_range(200, 250);
 	}
 	if (i == 20) {
 		ret = -ETIMEDOUT;
-- 
2.20.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel