* [PATCH staging 0/7] WF200 driver fixes
@ 2020-02-11 8:46 Michał Mirosław
2020-02-11 8:46 ` [[PATCH staging] 3/7] staging: wfx: fix init/remove vs IRQ race Michał Mirosław
` (6 more replies)
0 siblings, 7 replies; 20+ messages in thread
From: Michał Mirosław @ 2020-02-11 8:46 UTC (permalink / raw)
To: Jérôme Pouiller, Greg Kroah-Hartman; +Cc: devel, linux-kernel
A set of fixes for WF200 WiFi module driver. Three are bug fixes,
all the rest: cleanups and optimizations.
Michał Mirosław (7):
staging: wfx: add proper "compatible" string
staging: wfx: follow compatible = vendor,chip format
staging: wfx: fix init/remove vs IRQ race
staging: wfx: annotate nested gc_list vs tx queue locking
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 | 17 ++++----
drivers/staging/wfx/bus_spi.c | 41 ++++++++++++-------
drivers/staging/wfx/hwio.c | 2 +-
drivers/staging/wfx/main.c | 2 +-
drivers/staging/wfx/queue.c | 16 ++++----
7 files changed, 55 insertions(+), 42 deletions(-)
--
2.20.1
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* [[PATCH staging] 2/7] staging: wfx: follow compatible = vendor,chip format
2020-02-11 8:46 [PATCH staging 0/7] WF200 driver fixes Michał Mirosław
2020-02-11 8:46 ` [[PATCH staging] 3/7] staging: wfx: fix init/remove vs IRQ race Michał Mirosław
@ 2020-02-11 8:46 ` Michał Mirosław
2020-02-11 8:46 ` [[PATCH staging] 1/7] staging: wfx: add proper "compatible" string Michał Mirosław
` (4 subsequent siblings)
6 siblings, 0 replies; 20+ messages in thread
From: Michał Mirosław @ 2020-02-11 8:46 UTC (permalink / raw)
To: Jérôme Pouiller, Greg Kroah-Hartman; +Cc: devel, linux-kernel
As for SPI, follow "vendor,chip" format 'compatible' string also for
SDIO bus.
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 f8901164c206..29688b324b33 100644
--- a/drivers/staging/wfx/bus_sdio.c
+++ b/drivers/staging/wfx/bus_sdio.c
@@ -254,6 +254,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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [[PATCH staging] 3/7] staging: wfx: fix init/remove vs IRQ race
2020-02-11 8:46 [PATCH staging 0/7] WF200 driver fixes Michał Mirosław
@ 2020-02-11 8:46 ` Michał Mirosław
2020-02-11 9:23 ` Dan Carpenter
2020-02-11 8:46 ` [[PATCH staging] 2/7] staging: wfx: follow compatible = vendor,chip format Michał Mirosław
` (5 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Michał Mirosław @ 2020-02-11 8:46 UTC (permalink / raw)
To: Jérôme Pouiller, Greg Kroah-Hartman; +Cc: devel, linux-kernel
Move interrupt request and free so to avoid following WARN on probe
and possible use-after-free on remove.
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
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
drivers/staging/wfx/bus_sdio.c | 16 ++++++++--------
drivers/staging/wfx/bus_spi.c | 16 +++++++++-------
2 files changed, 17 insertions(+), 15 deletions(-)
diff --git a/drivers/staging/wfx/bus_sdio.c b/drivers/staging/wfx/bus_sdio.c
index 29688b324b33..2bfaf61e8f62 100644
--- a/drivers/staging/wfx/bus_sdio.c
+++ b/drivers/staging/wfx/bus_sdio.c
@@ -200,16 +200,16 @@ 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 err1;
+ }
+
+ ret = wfx_sdio_irq_subscribe(bus);
+ if (ret)
goto err2;
- }
ret = wfx_probe(bus->core);
if (ret)
@@ -218,9 +218,9 @@ static int wfx_sdio_probe(struct sdio_func *func,
return 0;
err3:
- wfx_free_common(bus->core);
+ wfx_sdio_irq_unsubscribe(bus);
err2:
- wfx_sdio_irq_unsubscribe(bus);
+ wfx_free_common(bus->core);
err1:
sdio_claim_host(func);
sdio_disable_func(func);
@@ -234,8 +234,8 @@ 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);
+ wfx_free_common(bus->core);
sdio_claim_host(func);
sdio_disable_func(func);
sdio_release_host(func);
diff --git a/drivers/staging/wfx/bus_spi.c b/drivers/staging/wfx/bus_spi.c
index 3ba705477ca8..2b108a9fa5ae 100644
--- a/drivers/staging/wfx/bus_spi.c
+++ b/drivers/staging/wfx/bus_spi.c
@@ -211,20 +211,22 @@ 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 = devm_request_irq(&func->dev, func->irq, wfx_spi_irq_handler,
+ IRQF_TRIGGER_RISING, "wfx", bus);
+ if (ret)
+ return ret;
+
ret = wfx_probe(bus->core);
- if (ret)
+ if (ret) {
+ devm_free_irq(&func->dev, func->irq, bus);
wfx_free_common(bus->core);
+ }
return ret;
}
@@ -234,11 +236,11 @@ 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);
+ wfx_free_common(bus->core);
return 0;
}
--
2.20.1
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [[PATCH staging] 1/7] staging: wfx: add proper "compatible" string
2020-02-11 8:46 [PATCH staging 0/7] WF200 driver fixes Michał Mirosław
2020-02-11 8:46 ` [[PATCH staging] 3/7] staging: wfx: fix init/remove vs IRQ race Michał Mirosław
2020-02-11 8:46 ` [[PATCH staging] 2/7] staging: wfx: follow compatible = vendor,chip format Michał Mirosław
@ 2020-02-11 8:46 ` Michał Mirosław
2020-02-11 8:46 ` [[PATCH staging] 5/7] staging: wfx: try 16-bit word mode first Michał Mirosław
` (3 subsequent siblings)
6 siblings, 0 replies; 20+ messages in thread
From: Michał Mirosław @ 2020-02-11 8:46 UTC (permalink / raw)
To: Jérôme Pouiller, Greg Kroah-Hartman; +Cc: devel, linux-kernel
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
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 40bc33035de2..3ba705477ca8 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",
@@ -201,9 +203,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);
}
@@ -244,14 +248,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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [[PATCH staging] 6/7] staging: wfx: use sleeping gpio accessors
2020-02-11 8:46 [PATCH staging 0/7] WF200 driver fixes Michał Mirosław
` (4 preceding siblings ...)
2020-02-11 8:46 ` [[PATCH staging] 4/7] staging: wfx: annotate nested gc_list vs tx queue locking Michał Mirosław
@ 2020-02-11 8:46 ` Michał Mirosław
2020-02-11 10:41 ` Jérôme Pouiller
2020-02-11 8:46 ` [[PATCH staging] 7/7] staging: wfx: use more power-efficient sleep for reset Michał Mirosław
6 siblings, 1 reply; 20+ messages in thread
From: Michał Mirosław @ 2020-02-11 8:46 UTC (permalink / raw)
To: Jérôme Pouiller, Greg Kroah-Hartman; +Cc: devel, linux-kernel
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 c5f78161234d..634b4e5bb055 100644
--- a/drivers/staging/wfx/bus_spi.c
+++ b/drivers/staging/wfx/bus_spi.c
@@ -208,9 +208,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 84adad64fc30..e8bdeb9aa3a9 100644
--- a/drivers/staging/wfx/main.c
+++ b/drivers/staging/wfx/main.c
@@ -420,7 +420,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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [[PATCH staging] 5/7] staging: wfx: try 16-bit word mode first
2020-02-11 8:46 [PATCH staging 0/7] WF200 driver fixes Michał Mirosław
` (2 preceding siblings ...)
2020-02-11 8:46 ` [[PATCH staging] 1/7] staging: wfx: add proper "compatible" string Michał Mirosław
@ 2020-02-11 8:46 ` Michał Mirosław
2020-02-11 8:46 ` [[PATCH staging] 4/7] staging: wfx: annotate nested gc_list vs tx queue locking Michał Mirosław
` (2 subsequent siblings)
6 siblings, 0 replies; 20+ messages in thread
From: Michał Mirosław @ 2020-02-11 8:46 UTC (permalink / raw)
To: Jérôme Pouiller, Greg Kroah-Hartman; +Cc: devel, linux-kernel
WF200 chip works with 16-bit words over SPI, so use that word
size if available. This avoids a bounce buffer usage for
little-endian hosts.
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
drivers/staging/wfx/bus_spi.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/wfx/bus_spi.c b/drivers/staging/wfx/bus_spi.c
index 2b108a9fa5ae..c5f78161234d 100644
--- a/drivers/staging/wfx/bus_spi.c
+++ b/drivers/staging/wfx/bus_spi.c
@@ -175,9 +175,12 @@ static int wfx_spi_probe(struct spi_device *func)
struct wfx_spi_priv *bus;
int ret;
- if (!func->bits_per_word)
- func->bits_per_word = 16;
+ func->bits_per_word = 16;
ret = spi_setup(func);
+ if (ret == -EINVAL) {
+ func->bits_per_word = 8;
+ ret = spi_setup(func);
+ }
if (ret)
return ret;
// Trace below is also displayed by spi_setup() if compiled with DEBUG
--
2.20.1
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [[PATCH staging] 4/7] staging: wfx: annotate nested gc_list vs tx queue locking
2020-02-11 8:46 [PATCH staging 0/7] WF200 driver fixes Michał Mirosław
` (3 preceding siblings ...)
2020-02-11 8:46 ` [[PATCH staging] 5/7] staging: wfx: try 16-bit word mode first Michał Mirosław
@ 2020-02-11 8:46 ` Michał Mirosław
2020-02-11 10:34 ` Jérôme Pouiller
2020-02-11 8:46 ` [[PATCH staging] 6/7] staging: wfx: use sleeping gpio accessors Michał Mirosław
2020-02-11 8:46 ` [[PATCH staging] 7/7] staging: wfx: use more power-efficient sleep for reset Michał Mirosław
6 siblings, 1 reply; 20+ messages in thread
From: Michał Mirosław @ 2020-02-11 8:46 UTC (permalink / raw)
To: Jérôme Pouiller, Greg Kroah-Hartman; +Cc: devel, linux-kernel
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
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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [[PATCH staging] 7/7] staging: wfx: use more power-efficient sleep for reset
2020-02-11 8:46 [PATCH staging 0/7] WF200 driver fixes Michał Mirosław
` (5 preceding siblings ...)
2020-02-11 8:46 ` [[PATCH staging] 6/7] staging: wfx: use sleeping gpio accessors Michał Mirosław
@ 2020-02-11 8:46 ` Michał Mirosław
2020-02-11 10:45 ` Jérôme Pouiller
6 siblings, 1 reply; 20+ messages in thread
From: Michał Mirosław @ 2020-02-11 8:46 UTC (permalink / raw)
To: Jérôme Pouiller, Greg Kroah-Hartman; +Cc: devel, linux-kernel
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 634b4e5bb055..14729af2c405 100644
--- a/drivers/staging/wfx/bus_spi.c
+++ b/drivers/staging/wfx/bus_spi.c
@@ -209,9 +209,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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [[PATCH staging] 3/7] staging: wfx: fix init/remove vs IRQ race
2020-02-11 8:46 ` [[PATCH staging] 3/7] staging: wfx: fix init/remove vs IRQ race Michał Mirosław
@ 2020-02-11 9:23 ` Dan Carpenter
0 siblings, 0 replies; 20+ messages in thread
From: Dan Carpenter @ 2020-02-11 9:23 UTC (permalink / raw)
To: Michał Mirosław
Cc: Jérôme Pouiller, Greg Kroah-Hartman, devel, linux-kernel
On Tue, Feb 11, 2020 at 09:46:54AM +0100, Michał Mirosław wrote:
> @@ -218,9 +218,9 @@ static int wfx_sdio_probe(struct sdio_func *func,
> return 0;
>
> err3:
> - wfx_free_common(bus->core);
> + wfx_sdio_irq_unsubscribe(bus);
> err2:
> - wfx_sdio_irq_unsubscribe(bus);
> + wfx_free_common(bus->core);
> err1:
> sdio_claim_host(func);
> sdio_disable_func(func);
> @@ -234,8 +234,8 @@ 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);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This calls sdio_release_host(func);
> + wfx_free_common(bus->core);
> sdio_claim_host(func);
> sdio_disable_func(func);
> sdio_release_host(func);
^^^^^^^^^^^^^^^^^^^^^^^^
so is this a double free?
> diff --git a/drivers/staging/wfx/bus_spi.c b/drivers/staging/wfx/bus_spi.c
> index 3ba705477ca8..2b108a9fa5ae 100644
> --- a/drivers/staging/wfx/bus_spi.c
> +++ b/drivers/staging/wfx/bus_spi.c
> @@ -211,20 +211,22 @@ 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 = devm_request_irq(&func->dev, func->irq, wfx_spi_irq_handler,
> + IRQF_TRIGGER_RISING, "wfx", bus);
> + if (ret)
> + return ret;
Shouldn't this call wfx_free_common(bus->core); before returning?
> +
> ret = wfx_probe(bus->core);
> - if (ret)
> + if (ret) {
> + devm_free_irq(&func->dev, func->irq, bus);
We shouldn't have to free devm_ resource.
> wfx_free_common(bus->core);
> + }
>
> return ret;
> }
> @@ -234,11 +236,11 @@ 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);
Is this devm_ free required?
> flush_work(&bus->request_rx);
> + wfx_free_common(bus->core);
> return 0;
> }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [[PATCH staging] 3/7] staging: wfx: fix init/remove vs IRQ race
@ 2020-02-11 9:23 ` Dan Carpenter
0 siblings, 0 replies; 20+ messages in thread
From: Dan Carpenter @ 2020-02-11 9:23 UTC (permalink / raw)
To: Michał Mirosław; +Cc: devel, Greg Kroah-Hartman, linux-kernel
On Tue, Feb 11, 2020 at 09:46:54AM +0100, Michał Mirosław wrote:
> @@ -218,9 +218,9 @@ static int wfx_sdio_probe(struct sdio_func *func,
> return 0;
>
> err3:
> - wfx_free_common(bus->core);
> + wfx_sdio_irq_unsubscribe(bus);
> err2:
> - wfx_sdio_irq_unsubscribe(bus);
> + wfx_free_common(bus->core);
> err1:
> sdio_claim_host(func);
> sdio_disable_func(func);
> @@ -234,8 +234,8 @@ 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);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This calls sdio_release_host(func);
> + wfx_free_common(bus->core);
> sdio_claim_host(func);
> sdio_disable_func(func);
> sdio_release_host(func);
^^^^^^^^^^^^^^^^^^^^^^^^
so is this a double free?
> diff --git a/drivers/staging/wfx/bus_spi.c b/drivers/staging/wfx/bus_spi.c
> index 3ba705477ca8..2b108a9fa5ae 100644
> --- a/drivers/staging/wfx/bus_spi.c
> +++ b/drivers/staging/wfx/bus_spi.c
> @@ -211,20 +211,22 @@ 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 = devm_request_irq(&func->dev, func->irq, wfx_spi_irq_handler,
> + IRQF_TRIGGER_RISING, "wfx", bus);
> + if (ret)
> + return ret;
Shouldn't this call wfx_free_common(bus->core); before returning?
> +
> ret = wfx_probe(bus->core);
> - if (ret)
> + if (ret) {
> + devm_free_irq(&func->dev, func->irq, bus);
We shouldn't have to free devm_ resource.
> wfx_free_common(bus->core);
> + }
>
> return ret;
> }
> @@ -234,11 +236,11 @@ 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);
Is this devm_ free required?
> flush_work(&bus->request_rx);
> + wfx_free_common(bus->core);
> return 0;
> }
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [[PATCH staging] 4/7] staging: wfx: annotate nested gc_list vs tx queue locking
2020-02-11 8:46 ` [[PATCH staging] 4/7] staging: wfx: annotate nested gc_list vs tx queue locking Michał Mirosław
@ 2020-02-11 10:34 ` Jérôme Pouiller
0 siblings, 0 replies; 20+ messages in thread
From: Jérôme Pouiller @ 2020-02-11 10:34 UTC (permalink / raw)
To: Michał Mirosław; +Cc: Greg Kroah-Hartman, devel, linux-kernel
On Tuesday 11 February 2020 09:46:55 CET Michał Mirosław wrote:
> 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
> 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
>
Reviewed-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
--
Jérôme Pouiller
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [[PATCH staging] 4/7] staging: wfx: annotate nested gc_list vs tx queue locking
@ 2020-02-11 10:34 ` Jérôme Pouiller
0 siblings, 0 replies; 20+ messages in thread
From: Jérôme Pouiller @ 2020-02-11 10:34 UTC (permalink / raw)
To: Michał Mirosław; +Cc: devel, Greg Kroah-Hartman, linux-kernel
On Tuesday 11 February 2020 09:46:55 CET Michał Mirosław wrote:
> 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
> 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
>
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [[PATCH staging] 3/7] staging: wfx: fix init/remove vs IRQ race
2020-02-11 9:23 ` Dan Carpenter
@ 2020-02-11 10:39 ` Michał Mirosław
-1 siblings, 0 replies; 20+ messages in thread
From: Michał Mirosław @ 2020-02-11 10:39 UTC (permalink / raw)
To: Dan Carpenter
Cc: Jérôme Pouiller, Greg Kroah-Hartman, devel, linux-kernel
On Tue, Feb 11, 2020 at 12:23:54PM +0300, Dan Carpenter wrote:
> On Tue, Feb 11, 2020 at 09:46:54AM +0100, Michał Mirosław wrote:
> > @@ -234,8 +234,8 @@ 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);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> This calls sdio_release_host(func);
>
> > + wfx_free_common(bus->core);
> > sdio_claim_host(func);
> > sdio_disable_func(func);
> > sdio_release_host(func);
> ^^^^^^^^^^^^^^^^^^^^^^^^
> so is this a double free?
This is paired with sdio_claim_host() just above.
Best Regards,
Michał Mirosław
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [[PATCH staging] 3/7] staging: wfx: fix init/remove vs IRQ race
@ 2020-02-11 10:39 ` Michał Mirosław
0 siblings, 0 replies; 20+ messages in thread
From: Michał Mirosław @ 2020-02-11 10:39 UTC (permalink / raw)
To: Dan Carpenter; +Cc: devel, Greg Kroah-Hartman, linux-kernel
On Tue, Feb 11, 2020 at 12:23:54PM +0300, Dan Carpenter wrote:
> On Tue, Feb 11, 2020 at 09:46:54AM +0100, Michał Mirosław wrote:
> > @@ -234,8 +234,8 @@ 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);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> This calls sdio_release_host(func);
>
> > + wfx_free_common(bus->core);
> > sdio_claim_host(func);
> > sdio_disable_func(func);
> > sdio_release_host(func);
> ^^^^^^^^^^^^^^^^^^^^^^^^
> so is this a double free?
This is paired with sdio_claim_host() just above.
Best Regards,
Michał Mirosław
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [[PATCH staging] 6/7] staging: wfx: use sleeping gpio accessors
2020-02-11 8:46 ` [[PATCH staging] 6/7] staging: wfx: use sleeping gpio accessors Michał Mirosław
@ 2020-02-11 10:41 ` Jérôme Pouiller
0 siblings, 0 replies; 20+ messages in thread
From: Jérôme Pouiller @ 2020-02-11 10:41 UTC (permalink / raw)
To: Michał Mirosław; +Cc: Greg Kroah-Hartman, devel, linux-kernel
On Tuesday 11 February 2020 09:46:55 CET Michał Mirosław wrote:
> 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 c5f78161234d..634b4e5bb055 100644
> --- a/drivers/staging/wfx/bus_spi.c
> +++ b/drivers/staging/wfx/bus_spi.c
> @@ -208,9 +208,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 84adad64fc30..e8bdeb9aa3a9 100644
> --- a/drivers/staging/wfx/main.c
> +++ b/drivers/staging/wfx/main.c
> @@ -420,7 +420,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
>
Reviewed-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
--
Jérôme Pouiller
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [[PATCH staging] 6/7] staging: wfx: use sleeping gpio accessors
@ 2020-02-11 10:41 ` Jérôme Pouiller
0 siblings, 0 replies; 20+ messages in thread
From: Jérôme Pouiller @ 2020-02-11 10:41 UTC (permalink / raw)
To: Michał Mirosław; +Cc: devel, Greg Kroah-Hartman, linux-kernel
On Tuesday 11 February 2020 09:46:55 CET Michał Mirosław wrote:
> 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 c5f78161234d..634b4e5bb055 100644
> --- a/drivers/staging/wfx/bus_spi.c
> +++ b/drivers/staging/wfx/bus_spi.c
> @@ -208,9 +208,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 84adad64fc30..e8bdeb9aa3a9 100644
> --- a/drivers/staging/wfx/main.c
> +++ b/drivers/staging/wfx/main.c
> @@ -420,7 +420,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
>
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [[PATCH staging] 7/7] staging: wfx: use more power-efficient sleep for reset
2020-02-11 8:46 ` [[PATCH staging] 7/7] staging: wfx: use more power-efficient sleep for reset Michał Mirosław
@ 2020-02-11 10:45 ` Jérôme Pouiller
0 siblings, 0 replies; 20+ messages in thread
From: Jérôme Pouiller @ 2020-02-11 10:45 UTC (permalink / raw)
To: Michał Mirosław; +Cc: Greg Kroah-Hartman, devel, linux-kernel
On Tuesday 11 February 2020 09:46:56 CET Michał Mirosław wrote:
>
> 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 634b4e5bb055..14729af2c405 100644
> --- a/drivers/staging/wfx/bus_spi.c
> +++ b/drivers/staging/wfx/bus_spi.c
> @@ -209,9 +209,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
>
Reviewed-by: Jérôme Pouiller <jerome.pouiller@silabs.com>
--
Jérôme Pouiller
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [[PATCH staging] 7/7] staging: wfx: use more power-efficient sleep for reset
@ 2020-02-11 10:45 ` Jérôme Pouiller
0 siblings, 0 replies; 20+ messages in thread
From: Jérôme Pouiller @ 2020-02-11 10:45 UTC (permalink / raw)
To: Michał Mirosław; +Cc: devel, Greg Kroah-Hartman, linux-kernel
On Tuesday 11 February 2020 09:46:56 CET Michał Mirosław wrote:
>
> 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 634b4e5bb055..14729af2c405 100644
> --- a/drivers/staging/wfx/bus_spi.c
> +++ b/drivers/staging/wfx/bus_spi.c
> @@ -209,9 +209,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
>
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [[PATCH staging] 3/7] staging: wfx: fix init/remove vs IRQ race
2020-02-11 10:39 ` Michał Mirosław
@ 2020-02-11 12:52 ` Dan Carpenter
-1 siblings, 0 replies; 20+ messages in thread
From: Dan Carpenter @ 2020-02-11 12:52 UTC (permalink / raw)
To: Michał Mirosław; +Cc: devel, Greg Kroah-Hartman, linux-kernel
On Tue, Feb 11, 2020 at 11:39:31AM +0100, Michał Mirosław wrote:
> On Tue, Feb 11, 2020 at 12:23:54PM +0300, Dan Carpenter wrote:
> > On Tue, Feb 11, 2020 at 09:46:54AM +0100, Michał Mirosław wrote:
> > > @@ -234,8 +234,8 @@ 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);
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >
> > This calls sdio_release_host(func);
> >
> > > + wfx_free_common(bus->core);
> > > sdio_claim_host(func);
> > > sdio_disable_func(func);
> > > sdio_release_host(func);
> > ^^^^^^^^^^^^^^^^^^^^^^^^
> > so is this a double free?
>
> This is paired with sdio_claim_host() just above.
Ah... I misread wfx_sdio_irq_unsubscribe(), sorry.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [[PATCH staging] 3/7] staging: wfx: fix init/remove vs IRQ race
@ 2020-02-11 12:52 ` Dan Carpenter
0 siblings, 0 replies; 20+ messages in thread
From: Dan Carpenter @ 2020-02-11 12:52 UTC (permalink / raw)
To: Michał Mirosław; +Cc: devel, Greg Kroah-Hartman, linux-kernel
On Tue, Feb 11, 2020 at 11:39:31AM +0100, Michał Mirosław wrote:
> On Tue, Feb 11, 2020 at 12:23:54PM +0300, Dan Carpenter wrote:
> > On Tue, Feb 11, 2020 at 09:46:54AM +0100, Michał Mirosław wrote:
> > > @@ -234,8 +234,8 @@ 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);
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >
> > This calls sdio_release_host(func);
> >
> > > + wfx_free_common(bus->core);
> > > sdio_claim_host(func);
> > > sdio_disable_func(func);
> > > sdio_release_host(func);
> > ^^^^^^^^^^^^^^^^^^^^^^^^
> > so is this a double free?
>
> This is paired with sdio_claim_host() just above.
Ah... I misread wfx_sdio_irq_unsubscribe(), sorry.
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2020-02-11 12:52 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-11 8:46 [PATCH staging 0/7] WF200 driver fixes Michał Mirosław
2020-02-11 8:46 ` [[PATCH staging] 3/7] staging: wfx: fix init/remove vs IRQ race Michał Mirosław
2020-02-11 9:23 ` Dan Carpenter
2020-02-11 9:23 ` Dan Carpenter
2020-02-11 10:39 ` Michał Mirosław
2020-02-11 10:39 ` Michał Mirosław
2020-02-11 12:52 ` Dan Carpenter
2020-02-11 12:52 ` Dan Carpenter
2020-02-11 8:46 ` [[PATCH staging] 2/7] staging: wfx: follow compatible = vendor,chip format Michał Mirosław
2020-02-11 8:46 ` [[PATCH staging] 1/7] staging: wfx: add proper "compatible" string Michał Mirosław
2020-02-11 8:46 ` [[PATCH staging] 5/7] staging: wfx: try 16-bit word mode first Michał Mirosław
2020-02-11 8:46 ` [[PATCH staging] 4/7] staging: wfx: annotate nested gc_list vs tx queue locking Michał Mirosław
2020-02-11 10:34 ` Jérôme Pouiller
2020-02-11 10:34 ` Jérôme Pouiller
2020-02-11 8:46 ` [[PATCH staging] 6/7] staging: wfx: use sleeping gpio accessors Michał Mirosław
2020-02-11 10:41 ` Jérôme Pouiller
2020-02-11 10:41 ` Jérôme Pouiller
2020-02-11 8:46 ` [[PATCH staging] 7/7] staging: wfx: use more power-efficient sleep for reset Michał Mirosław
2020-02-11 10:45 ` Jérôme Pouiller
2020-02-11 10:45 ` Jérôme Pouiller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.