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

[Lorenzo Bianconi <lorenzo.bianconi@redhat.com>]

[-- 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 --]