netdev - Re: [PATCH v5 net-next 5/5] net: ethernet: mtk_wed: add reset/reset

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <02cfb1dd78f6efb1ae3077de24fa357091168d39.camel@gmail.com>
Date:   Thu, 12 Jan 2023 08:15:20 -0800
From:   Alexander H Duyck <alexander.duyck@...il.com>
To:     Lorenzo Bianconi <lorenzo@...nel.org>, netdev@...r.kernel.org
Cc:     davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        pabeni@...hat.com, lorenzo.bianconi@...hat.com, nbd@....name,
        john@...ozen.org, sean.wang@...iatek.com, Mark-MC.Lee@...iatek.com,
        sujuan.chen@...iatek.com, daniel@...rotopia.org, leon@...nel.org
Subject: Re: [PATCH v5 net-next 5/5] net: ethernet: mtk_wed: add
 reset/reset_complete callbacks

On Wed, 2023-01-11 at 18:22 +0100, Lorenzo Bianconi wrote:
> Introduce reset and reset_complete wlan callback to schedule WLAN driver
> reset when ethernet/wed driver is resetting.
> 
> Tested-by: Daniel Golle <daniel@...rotopia.org>
> Co-developed-by: Sujuan Chen <sujuan.chen@...iatek.com>
> Signed-off-by: Sujuan Chen <sujuan.chen@...iatek.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo@...nel.org>
> ---
>  drivers/net/ethernet/mediatek/mtk_eth_soc.c |  7 ++++
>  drivers/net/ethernet/mediatek/mtk_wed.c     | 40 +++++++++++++++++++++
>  drivers/net/ethernet/mediatek/mtk_wed.h     |  8 +++++
>  include/linux/soc/mediatek/mtk_wed.h        |  2 ++
>  4 files changed, 57 insertions(+)
> 

Do we have any updates on the implementation that would be making use
of this? It looks like there was a discussion for the v2 of this set to
include a link to an RFC posting that would make use of this set.

> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> index 1af74e9a6cd3..0147e98009c2 100644
> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> @@ -3924,6 +3924,11 @@ static void mtk_pending_work(struct work_struct *work)
>  	set_bit(MTK_RESETTING, &eth->state);
>  
>  	mtk_prepare_for_reset(eth);
> +	mtk_wed_fe_reset();
> +	/* Run again reset preliminary configuration in order to avoid any
> +	 * possible race during FE reset since it can run releasing RTNL lock.
> +	 */
> +	mtk_prepare_for_reset(eth);
>  
>  	/* stop all devices to make sure that dma is properly shut down */
>  	for (i = 0; i < MTK_MAC_COUNT; i++) {
> @@ -3961,6 +3966,8 @@ static void mtk_pending_work(struct work_struct *work)
>  
>  	clear_bit(MTK_RESETTING, &eth->state);
>  
> +	mtk_wed_fe_reset_complete();
> +
>  	rtnl_unlock();
>  }
>  
> diff --git a/drivers/net/ethernet/mediatek/mtk_wed.c b/drivers/net/ethernet/mediatek/mtk_wed.c
> index a6271449617f..4854993f2941 100644
> --- a/drivers/net/ethernet/mediatek/mtk_wed.c
> +++ b/drivers/net/ethernet/mediatek/mtk_wed.c
> @@ -206,6 +206,46 @@ mtk_wed_wo_reset(struct mtk_wed_device *dev)
>  	iounmap(reg);
>  }
>  
> +void mtk_wed_fe_reset(void)
> +{
> +	int i;
> +
> +	mutex_lock(&hw_lock);
> +
> +	for (i = 0; i < ARRAY_SIZE(hw_list); i++) {
> +		struct mtk_wed_hw *hw = hw_list[i];
> +		struct mtk_wed_device *dev = hw->wed_dev;
> +
> +		if (!dev || !dev->wlan.reset)
> +			continue;
> +
> +		/* reset callback blocks until WLAN reset is completed */
> +		if (dev->wlan.reset(dev))
> +			dev_err(dev->dev, "wlan reset failed\n");

The reason why having the consumer would be useful are cases like this.
My main concern is if the error value might be useful to actually
expose rather than just treating it as a boolean. Usually for things
like this I prefer to see the result captured and if it indicates error
we return the error value since this could be one of several possible
causes for the error assuming this returns an int and not a bool.

> +	}
> +
> +	mutex_unlock(&hw_lock);
> +}
> +
> +void mtk_wed_fe_reset_complete(void)
> +{
> +	int i;
> +
> +	mutex_lock(&hw_lock);
> +
> +	for (i = 0; i < ARRAY_SIZE(hw_list); i++) {
> +		struct mtk_wed_hw *hw = hw_list[i];
> +		struct mtk_wed_device *dev = hw->wed_dev;
> +
> +		if (!dev || !dev->wlan.reset_complete)
> +			continue;
> +
> +		dev->wlan.reset_complete(dev);
> +	}
> +
> +	mutex_unlock(&hw_lock);
> +}
> +
>  static struct mtk_wed_hw *
>  mtk_wed_assign(struct mtk_wed_device *dev)
>  {