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