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

[Leon Romanovsky <leon@kernel.org>]

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