Re: [[PATCH staging] 3/7] staging: wfx: fix init/remove vs IRQ race

Re: [[PATCH staging] 3/7] staging: wfx: fix init/remove vs IRQ race

[Dan Carpenter]

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

Re: [[PATCH staging] 3/7] staging: wfx: fix init/remove vs IRQ race

[Michał Mirosław]

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

Re: [[PATCH staging] 3/7] staging: wfx: fix init/remove vs IRQ race

[Dan Carpenter]

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

Re: [[PATCH staging] 4/7] staging: wfx: annotate nested gc_list vs tx queue locking

[Jérôme Pouiller]

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

Re: [[PATCH staging] 6/7] staging: wfx: use sleeping gpio accessors

[Jérôme Pouiller]

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

Re: [[PATCH staging] 7/7] staging: wfx: use more power-efficient sleep for reset

[Jérôme Pouiller]

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