[PATCH v2 net-next] net: ethernet: mtk_wed: fix possible deadlock if mtk_wed_wo_init fails

[PATCH v2 net-next] net: ethernet: mtk_wed: fix possible deadlock if mtk_wed_wo_init fails

[Lorenzo Bianconi]

Introduce __mtk_wed_detach() in order to avoid a possible deadlock in
mtk_wed_attach routine if mtk_wed_wo_init fails.
Check wo pointer is properly allocated before running mtk_wed_wo_reset()
and mtk_wed_wo_deinit() in __mtk_wed_detach routine.
Honor mtk_wed_mcu_send_msg return value in mtk_wed_wo_reset().

Fixes: 4c5de09eb0d0 ("net: ethernet: mtk_wed: add configure wed wo support")
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
Changes since v1:
- move wo pointer checks in __mtk_wed_detach()
---
 drivers/net/ethernet/mediatek/mtk_wed.c     | 30 ++++++++++++++-------
 drivers/net/ethernet/mediatek/mtk_wed_mcu.c |  3 +++
 2 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_wed.c b/drivers/net/ethernet/mediatek/mtk_wed.c
index d041615b2bac..2ce9fbb1c66d 100644
--- a/drivers/net/ethernet/mediatek/mtk_wed.c
+++ b/drivers/net/ethernet/mediatek/mtk_wed.c
@@ -174,9 +174,10 @@ mtk_wed_wo_reset(struct mtk_wed_device *dev)
 	mtk_wdma_tx_reset(dev);
 	mtk_wed_reset(dev, MTK_WED_RESET_WED);
 
-	mtk_wed_mcu_send_msg(wo, MTK_WED_MODULE_ID_WO,
-			     MTK_WED_WO_CMD_CHANGE_STATE, &state,
-			     sizeof(state), false);
+	if (mtk_wed_mcu_send_msg(wo, MTK_WED_MODULE_ID_WO,
+				 MTK_WED_WO_CMD_CHANGE_STATE, &state,
+				 sizeof(state), false))
+		return;
 
 	if (readx_poll_timeout(mtk_wed_wo_read_status, dev, val,
 			       val == MTK_WED_WOIF_DISABLE_DONE,
@@ -576,12 +577,10 @@ mtk_wed_deinit(struct mtk_wed_device *dev)
 }
 
 static void
-mtk_wed_detach(struct mtk_wed_device *dev)
+__mtk_wed_detach(struct mtk_wed_device *dev)
 {
 	struct mtk_wed_hw *hw = dev->hw;
 
-	mutex_lock(&hw_lock);
-
 	mtk_wed_deinit(dev);
 
 	mtk_wdma_rx_reset(dev);
@@ -590,9 +589,11 @@ mtk_wed_detach(struct mtk_wed_device *dev)
 	mtk_wed_free_tx_rings(dev);
 
 	if (mtk_wed_get_rx_capa(dev)) {
-		mtk_wed_wo_reset(dev);
+		if (hw->wed_wo)
+			mtk_wed_wo_reset(dev);
 		mtk_wed_free_rx_rings(dev);
-		mtk_wed_wo_deinit(hw);
+		if (hw->wed_wo)
+			mtk_wed_wo_deinit(hw);
 	}
 
 	if (dev->wlan.bus_type == MTK_WED_BUS_PCIE) {
@@ -612,6 +613,13 @@ mtk_wed_detach(struct mtk_wed_device *dev)
 	module_put(THIS_MODULE);
 
 	hw->wed_dev = NULL;
+}
+
+static void
+mtk_wed_detach(struct mtk_wed_device *dev)
+{
+	mutex_lock(&hw_lock);
+	__mtk_wed_detach(dev);
 	mutex_unlock(&hw_lock);
 }
 
@@ -1490,8 +1498,10 @@ mtk_wed_attach(struct mtk_wed_device *dev)
 		ret = mtk_wed_wo_init(hw);
 	}
 out:
-	if (ret)
-		mtk_wed_detach(dev);
+	if (ret) {
+		dev_err(dev->hw->dev, "failed to attach wed device\n");
+		__mtk_wed_detach(dev);
+	}
 unlock:
 	mutex_unlock(&hw_lock);
 
diff --git a/drivers/net/ethernet/mediatek/mtk_wed_mcu.c b/drivers/net/ethernet/mediatek/mtk_wed_mcu.c
index f9539e6233c9..3dd02889d972 100644
--- a/drivers/net/ethernet/mediatek/mtk_wed_mcu.c
+++ b/drivers/net/ethernet/mediatek/mtk_wed_mcu.c
@@ -207,6 +207,9 @@ int mtk_wed_mcu_msg_update(struct mtk_wed_device *dev, int id, void *data,
 	if (dev->hw->version == 1)
 		return 0;
 
+	if (!wo)
+		return -ENODEV;
+
 	return mtk_wed_mcu_send_msg(wo, MTK_WED_MODULE_ID_WO, id, data, len,
 				    true);
 }
-- 
2.38.1

Re: [PATCH v2 net-next] net: ethernet: mtk_wed: fix possible deadlock if mtk_wed_wo_init fails

[Leon Romanovsky]

On Mon, Dec 05, 2022 at 12:14:41PM +0100, Lorenzo Bianconi wrote:
> Introduce __mtk_wed_detach() in order to avoid a possible deadlock in
> mtk_wed_attach routine if mtk_wed_wo_init fails.
> Check wo pointer is properly allocated before running mtk_wed_wo_reset()
> and mtk_wed_wo_deinit() in __mtk_wed_detach routine.
> Honor mtk_wed_mcu_send_msg return value in mtk_wed_wo_reset().
> 
> Fixes: 4c5de09eb0d0 ("net: ethernet: mtk_wed: add configure wed wo support")
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
> Changes since v1:
> - move wo pointer checks in __mtk_wed_detach()
> ---
>  drivers/net/ethernet/mediatek/mtk_wed.c     | 30 ++++++++++++++-------
>  drivers/net/ethernet/mediatek/mtk_wed_mcu.c |  3 +++
>  2 files changed, 23 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mediatek/mtk_wed.c b/drivers/net/ethernet/mediatek/mtk_wed.c
> index d041615b2bac..2ce9fbb1c66d 100644
> --- a/drivers/net/ethernet/mediatek/mtk_wed.c
> +++ b/drivers/net/ethernet/mediatek/mtk_wed.c
> @@ -174,9 +174,10 @@ mtk_wed_wo_reset(struct mtk_wed_device *dev)
>  	mtk_wdma_tx_reset(dev);
>  	mtk_wed_reset(dev, MTK_WED_RESET_WED);
>  
> -	mtk_wed_mcu_send_msg(wo, MTK_WED_MODULE_ID_WO,
> -			     MTK_WED_WO_CMD_CHANGE_STATE, &state,
> -			     sizeof(state), false);
> +	if (mtk_wed_mcu_send_msg(wo, MTK_WED_MODULE_ID_WO,
> +				 MTK_WED_WO_CMD_CHANGE_STATE, &state,
> +				 sizeof(state), false))
> +		return;
>  
>  	if (readx_poll_timeout(mtk_wed_wo_read_status, dev, val,
>  			       val == MTK_WED_WOIF_DISABLE_DONE,
> @@ -576,12 +577,10 @@ mtk_wed_deinit(struct mtk_wed_device *dev)
>  }
>  
>  static void
> -mtk_wed_detach(struct mtk_wed_device *dev)
> +__mtk_wed_detach(struct mtk_wed_device *dev)
>  {
>  	struct mtk_wed_hw *hw = dev->hw;
>  
> -	mutex_lock(&hw_lock);
> -
>  	mtk_wed_deinit(dev);
>  
>  	mtk_wdma_rx_reset(dev);
> @@ -590,9 +589,11 @@ mtk_wed_detach(struct mtk_wed_device *dev)
>  	mtk_wed_free_tx_rings(dev);
>  
>  	if (mtk_wed_get_rx_capa(dev)) {
> -		mtk_wed_wo_reset(dev);
> +		if (hw->wed_wo)
> +			mtk_wed_wo_reset(dev);
>  		mtk_wed_free_rx_rings(dev);
> -		mtk_wed_wo_deinit(hw);
> +		if (hw->wed_wo)
> +			mtk_wed_wo_deinit(hw);
>  	}
>  
>  	if (dev->wlan.bus_type == MTK_WED_BUS_PCIE) {
> @@ -612,6 +613,13 @@ mtk_wed_detach(struct mtk_wed_device *dev)
>  	module_put(THIS_MODULE);
>  
>  	hw->wed_dev = NULL;
> +}
> +
> +static void
> +mtk_wed_detach(struct mtk_wed_device *dev)
> +{
> +	mutex_lock(&hw_lock);
> +	__mtk_wed_detach(dev);
>  	mutex_unlock(&hw_lock);
>  }
>  
> @@ -1490,8 +1498,10 @@ mtk_wed_attach(struct mtk_wed_device *dev)
>  		ret = mtk_wed_wo_init(hw);
>  	}
>  out:
> -	if (ret)
> -		mtk_wed_detach(dev);
> +	if (ret) {
> +		dev_err(dev->hw->dev, "failed to attach wed device\n");
> +		__mtk_wed_detach(dev);
> +	}
>  unlock:
>  	mutex_unlock(&hw_lock);
>  
> diff --git a/drivers/net/ethernet/mediatek/mtk_wed_mcu.c b/drivers/net/ethernet/mediatek/mtk_wed_mcu.c
> index f9539e6233c9..3dd02889d972 100644
> --- a/drivers/net/ethernet/mediatek/mtk_wed_mcu.c
> +++ b/drivers/net/ethernet/mediatek/mtk_wed_mcu.c
> @@ -207,6 +207,9 @@ int mtk_wed_mcu_msg_update(struct mtk_wed_device *dev, int id, void *data,
>  	if (dev->hw->version == 1)
>  		return 0;
>  
> +	if (!wo)
> +		return -ENODEV;
> +

Can you please help me to understand how and when this mtk_wed_mcu_msg_update()
function is called?

I see this line .msg_update = mtk_wed_mcu_msg_update, and
relevant mtk_wed_device_update_msg() define, but nothing calls to this
define.



>  	return mtk_wed_mcu_send_msg(wo, MTK_WED_MODULE_ID_WO, id, data, len,
>  				    true);
>  }
> -- 
> 2.38.1
> 

Re: [PATCH v2 net-next] net: ethernet: mtk_wed: fix possible deadlock if mtk_wed_wo_init fails

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 4292 bytes --]

> On Mon, Dec 05, 2022 at 12:14:41PM +0100, Lorenzo Bianconi wrote:
> > Introduce __mtk_wed_detach() in order to avoid a possible deadlock in
> > mtk_wed_attach routine if mtk_wed_wo_init fails.
> > Check wo pointer is properly allocated before running mtk_wed_wo_reset()
> > and mtk_wed_wo_deinit() in __mtk_wed_detach routine.
> > Honor mtk_wed_mcu_send_msg return value in mtk_wed_wo_reset().
> > 
> > Fixes: 4c5de09eb0d0 ("net: ethernet: mtk_wed: add configure wed wo support")
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> > Changes since v1:
> > - move wo pointer checks in __mtk_wed_detach()
> > ---
> >  drivers/net/ethernet/mediatek/mtk_wed.c     | 30 ++++++++++++++-------
> >  drivers/net/ethernet/mediatek/mtk_wed_mcu.c |  3 +++
> >  2 files changed, 23 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/mediatek/mtk_wed.c b/drivers/net/ethernet/mediatek/mtk_wed.c
> > index d041615b2bac..2ce9fbb1c66d 100644
> > --- a/drivers/net/ethernet/mediatek/mtk_wed.c
> > +++ b/drivers/net/ethernet/mediatek/mtk_wed.c
> > @@ -174,9 +174,10 @@ mtk_wed_wo_reset(struct mtk_wed_device *dev)
> >  	mtk_wdma_tx_reset(dev);
> >  	mtk_wed_reset(dev, MTK_WED_RESET_WED);
> >  
> > -	mtk_wed_mcu_send_msg(wo, MTK_WED_MODULE_ID_WO,
> > -			     MTK_WED_WO_CMD_CHANGE_STATE, &state,
> > -			     sizeof(state), false);
> > +	if (mtk_wed_mcu_send_msg(wo, MTK_WED_MODULE_ID_WO,
> > +				 MTK_WED_WO_CMD_CHANGE_STATE, &state,
> > +				 sizeof(state), false))
> > +		return;
> >  
> >  	if (readx_poll_timeout(mtk_wed_wo_read_status, dev, val,
> >  			       val == MTK_WED_WOIF_DISABLE_DONE,
> > @@ -576,12 +577,10 @@ mtk_wed_deinit(struct mtk_wed_device *dev)
> >  }
> >  
> >  static void
> > -mtk_wed_detach(struct mtk_wed_device *dev)
> > +__mtk_wed_detach(struct mtk_wed_device *dev)
> >  {
> >  	struct mtk_wed_hw *hw = dev->hw;
> >  
> > -	mutex_lock(&hw_lock);
> > -
> >  	mtk_wed_deinit(dev);
> >  
> >  	mtk_wdma_rx_reset(dev);
> > @@ -590,9 +589,11 @@ mtk_wed_detach(struct mtk_wed_device *dev)
> >  	mtk_wed_free_tx_rings(dev);
> >  
> >  	if (mtk_wed_get_rx_capa(dev)) {
> > -		mtk_wed_wo_reset(dev);
> > +		if (hw->wed_wo)
> > +			mtk_wed_wo_reset(dev);
> >  		mtk_wed_free_rx_rings(dev);
> > -		mtk_wed_wo_deinit(hw);
> > +		if (hw->wed_wo)
> > +			mtk_wed_wo_deinit(hw);
> >  	}
> >  
> >  	if (dev->wlan.bus_type == MTK_WED_BUS_PCIE) {
> > @@ -612,6 +613,13 @@ mtk_wed_detach(struct mtk_wed_device *dev)
> >  	module_put(THIS_MODULE);
> >  
> >  	hw->wed_dev = NULL;
> > +}
> > +
> > +static void
> > +mtk_wed_detach(struct mtk_wed_device *dev)
> > +{
> > +	mutex_lock(&hw_lock);
> > +	__mtk_wed_detach(dev);
> >  	mutex_unlock(&hw_lock);
> >  }
> >  
> > @@ -1490,8 +1498,10 @@ mtk_wed_attach(struct mtk_wed_device *dev)
> >  		ret = mtk_wed_wo_init(hw);
> >  	}
> >  out:
> > -	if (ret)
> > -		mtk_wed_detach(dev);
> > +	if (ret) {
> > +		dev_err(dev->hw->dev, "failed to attach wed device\n");
> > +		__mtk_wed_detach(dev);
> > +	}
> >  unlock:
> >  	mutex_unlock(&hw_lock);
> >  
> > diff --git a/drivers/net/ethernet/mediatek/mtk_wed_mcu.c b/drivers/net/ethernet/mediatek/mtk_wed_mcu.c
> > index f9539e6233c9..3dd02889d972 100644
> > --- a/drivers/net/ethernet/mediatek/mtk_wed_mcu.c
> > +++ b/drivers/net/ethernet/mediatek/mtk_wed_mcu.c
> > @@ -207,6 +207,9 @@ int mtk_wed_mcu_msg_update(struct mtk_wed_device *dev, int id, void *data,
> >  	if (dev->hw->version == 1)
> >  		return 0;
> >  
> > +	if (!wo)
> > +		return -ENODEV;
> > +
> 
> Can you please help me to understand how and when this mtk_wed_mcu_msg_update()
> function is called?
> 
> I see this line .msg_update = mtk_wed_mcu_msg_update, and
> relevant mtk_wed_device_update_msg() define, but nothing calls to this
> define.

mtk_wed_device_update_msg() is currently run by mt7915 driver in
mt7915_mcu_wed_enable_rx_stats() and in mt76_connac_mcu_sta_wed_update().
At the moment we always run mtk_wed_mcu_msg_update with non-NULL wo pointer,
but I would prefer to add this safety check.

Regards,
Lorenzo

> 
> 
> 
> >  	return mtk_wed_mcu_send_msg(wo, MTK_WED_MODULE_ID_WO, id, data, len,
> >  				    true);
> >  }
> > -- 
> > 2.38.1
> > 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 net-next] net: ethernet: mtk_wed: fix possible deadlock if mtk_wed_wo_init fails

[Leon Romanovsky]

On Mon, Dec 05, 2022 at 06:05:49PM +0100, Lorenzo Bianconi wrote:
> > On Mon, Dec 05, 2022 at 12:14:41PM +0100, Lorenzo Bianconi wrote:
> > > Introduce __mtk_wed_detach() in order to avoid a possible deadlock in
> > > mtk_wed_attach routine if mtk_wed_wo_init fails.
> > > Check wo pointer is properly allocated before running mtk_wed_wo_reset()
> > > and mtk_wed_wo_deinit() in __mtk_wed_detach routine.
> > > Honor mtk_wed_mcu_send_msg return value in mtk_wed_wo_reset().
> > > 
> > > Fixes: 4c5de09eb0d0 ("net: ethernet: mtk_wed: add configure wed wo support")
> > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > ---
> > > Changes since v1:
> > > - move wo pointer checks in __mtk_wed_detach()
> > > ---
> > >  drivers/net/ethernet/mediatek/mtk_wed.c     | 30 ++++++++++++++-------
> > >  drivers/net/ethernet/mediatek/mtk_wed_mcu.c |  3 +++
> > >  2 files changed, 23 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/net/ethernet/mediatek/mtk_wed.c b/drivers/net/ethernet/mediatek/mtk_wed.c
> > > index d041615b2bac..2ce9fbb1c66d 100644
> > > --- a/drivers/net/ethernet/mediatek/mtk_wed.c
> > > +++ b/drivers/net/ethernet/mediatek/mtk_wed.c
> > > @@ -174,9 +174,10 @@ mtk_wed_wo_reset(struct mtk_wed_device *dev)
> > >  	mtk_wdma_tx_reset(dev);
> > >  	mtk_wed_reset(dev, MTK_WED_RESET_WED);
> > >  
> > > -	mtk_wed_mcu_send_msg(wo, MTK_WED_MODULE_ID_WO,
> > > -			     MTK_WED_WO_CMD_CHANGE_STATE, &state,
> > > -			     sizeof(state), false);
> > > +	if (mtk_wed_mcu_send_msg(wo, MTK_WED_MODULE_ID_WO,
> > > +				 MTK_WED_WO_CMD_CHANGE_STATE, &state,
> > > +				 sizeof(state), false))
> > > +		return;
> > >  
> > >  	if (readx_poll_timeout(mtk_wed_wo_read_status, dev, val,
> > >  			       val == MTK_WED_WOIF_DISABLE_DONE,
> > > @@ -576,12 +577,10 @@ mtk_wed_deinit(struct mtk_wed_device *dev)
> > >  }
> > >  
> > >  static void
> > > -mtk_wed_detach(struct mtk_wed_device *dev)
> > > +__mtk_wed_detach(struct mtk_wed_device *dev)
> > >  {
> > >  	struct mtk_wed_hw *hw = dev->hw;
> > >  
> > > -	mutex_lock(&hw_lock);
> > > -
> > >  	mtk_wed_deinit(dev);
> > >  
> > >  	mtk_wdma_rx_reset(dev);
> > > @@ -590,9 +589,11 @@ mtk_wed_detach(struct mtk_wed_device *dev)
> > >  	mtk_wed_free_tx_rings(dev);
> > >  
> > >  	if (mtk_wed_get_rx_capa(dev)) {
> > > -		mtk_wed_wo_reset(dev);
> > > +		if (hw->wed_wo)
> > > +			mtk_wed_wo_reset(dev);
> > >  		mtk_wed_free_rx_rings(dev);
> > > -		mtk_wed_wo_deinit(hw);
> > > +		if (hw->wed_wo)
> > > +			mtk_wed_wo_deinit(hw);
> > >  	}
> > >  
> > >  	if (dev->wlan.bus_type == MTK_WED_BUS_PCIE) {
> > > @@ -612,6 +613,13 @@ mtk_wed_detach(struct mtk_wed_device *dev)
> > >  	module_put(THIS_MODULE);
> > >  
> > >  	hw->wed_dev = NULL;
> > > +}
> > > +
> > > +static void
> > > +mtk_wed_detach(struct mtk_wed_device *dev)
> > > +{
> > > +	mutex_lock(&hw_lock);
> > > +	__mtk_wed_detach(dev);
> > >  	mutex_unlock(&hw_lock);
> > >  }
> > >  
> > > @@ -1490,8 +1498,10 @@ mtk_wed_attach(struct mtk_wed_device *dev)
> > >  		ret = mtk_wed_wo_init(hw);
> > >  	}
> > >  out:
> > > -	if (ret)
> > > -		mtk_wed_detach(dev);
> > > +	if (ret) {
> > > +		dev_err(dev->hw->dev, "failed to attach wed device\n");
> > > +		__mtk_wed_detach(dev);
> > > +	}
> > >  unlock:
> > >  	mutex_unlock(&hw_lock);
> > >  
> > > diff --git a/drivers/net/ethernet/mediatek/mtk_wed_mcu.c b/drivers/net/ethernet/mediatek/mtk_wed_mcu.c
> > > index f9539e6233c9..3dd02889d972 100644
> > > --- a/drivers/net/ethernet/mediatek/mtk_wed_mcu.c
> > > +++ b/drivers/net/ethernet/mediatek/mtk_wed_mcu.c
> > > @@ -207,6 +207,9 @@ int mtk_wed_mcu_msg_update(struct mtk_wed_device *dev, int id, void *data,
> > >  	if (dev->hw->version == 1)
> > >  		return 0;
> > >  
> > > +	if (!wo)
> > > +		return -ENODEV;
> > > +
> > 
> > Can you please help me to understand how and when this mtk_wed_mcu_msg_update()
> > function is called?
> > 
> > I see this line .msg_update = mtk_wed_mcu_msg_update, and
> > relevant mtk_wed_device_update_msg() define, but nothing calls to this
> > define.
> 
> mtk_wed_device_update_msg() is currently run by mt7915 driver in
> mt7915_mcu_wed_enable_rx_stats() and in mt76_connac_mcu_sta_wed_update().

I see, I didn't fetch latest net-next.

> At the moment we always run mtk_wed_mcu_msg_update with non-NULL wo pointer,
> but I would prefer to add this safety check.

If it is impossible, please add WARN_ON(!wo) instead of "if ...". It
describes better what is expected here.

Thanks

> 
> Regards,
> Lorenzo
> 
> > 
> > 
> > 
> > >  	return mtk_wed_mcu_send_msg(wo, MTK_WED_MODULE_ID_WO, id, data, len,
> > >  				    true);
> > >  }
> > > -- 
> > > 2.38.1
> > > 
> >