Re: [PATCH] net: stmmac:fix system hang when setting up tag_8021q VLAN for DSA ports

From: Jacob Keller <jacob.e.keller@intel.com>
To: Yan Wang <rk.code@outlook.com>, <davem@davemloft.net>,
	<edumazet@google.com>, <pabeni@redhat.com>, <kuba@kernel.org>,
	<mcoquelin.stm32@gmail.com>
Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>,
	Alexandre Torgue <alexandre.torgue@foss.st.com>,
	Jose Abreu <joabreu@synopsys.com>,
	"open list:STMMAC ETHERNET DRIVER" <netdev@vger.kernel.org>,
	"moderated list:ARM/STM32 ARCHITECTURE"
	<linux-stm32@st-md-mailman.stormreply.com>,
	"moderated list:ARM/STM32 ARCHITECTURE"
	<linux-arm-kernel@lists.infradead.org>,
	open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] net: stmmac:fix system hang when setting up tag_8021q VLAN for DSA ports
Date: Thu, 13 Apr 2023 10:00:41 -0700	[thread overview]
Message-ID: <de8e3387-6224-d367-d897-a4f61450cd7e@intel.com> (raw)
In-Reply-To: <KL1PR01MB5448020DE191340AE64530B0E6989@KL1PR01MB5448.apcprd01.prod.exchangelabs.com>

On 4/13/2023 6:38 AM, Yan Wang wrote:
> The system hang because of dsa_tag_8021q_port_setup()->
> 				stmmac_vlan_rx_add_vid().
> 
> I found in stmmac_drv_probe() that cailing pm_runtime_put()
> disabled the clock.
> 
> First, when the kernel is compiled with CONFIG_PM=y,The stmmac's
> resume/suspend is active.
> 
> Secondly,stmmac as DSA master,the dsa_tag_8021q_port_setup() function
> will callback stmmac_vlan_rx_add_vid when DSA dirver starts. However,
> The system is hanged for the stmmac_vlan_rx_add_vid() accesses its
> registers after stmmac's clock is closed.
> 

And access to the registers basically just waits because its clock is
stopped?

> I would suggest adding the pm_runtime_resume_and_get() to the
> stmmac_vlan_rx_add_vid().This guarantees that resuming clock output
> while in use.
> 

It feels a bit odd to need this within the VLAN function, but these can
be entered at an odd time where its possible that the clock has been
closed... Ok.

> Signed-off-by: Yan Wang <rk.code@outlook.com>
> ---

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index d7fcab057032..f9cd063f1fe3 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -6350,6 +6350,10 @@ static int stmmac_vlan_rx_add_vid(struct net_device *ndev, __be16 proto, u16 vid
>  	bool is_double = false;
>  	int ret;
>  
> +	ret = pm_runtime_resume_and_get(priv->device);
> +	if (ret < 0)
> +		return ret;
> +
>  	if (be16_to_cpu(proto) == ETH_P_8021AD)
>  		is_double = true;
>  
> @@ -6357,16 +6361,18 @@ static int stmmac_vlan_rx_add_vid(struct net_device *ndev, __be16 proto, u16 vid
>  	ret = stmmac_vlan_update(priv, is_double);
>  	if (ret) {
>  		clear_bit(vid, priv->active_vlans);
> -		return ret;
> +		goto err_pm_put;
>  	}
>  
>  	if (priv->hw->num_vlan) {
>  		ret = stmmac_add_hw_vlan_rx_fltr(priv, ndev, priv->hw, proto, vid);
>  		if (ret)
> -			return ret;
> +			goto err_pm_put;
>  	}
> +err_pm_put:
> +	pm_runtime_put(priv->device);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int stmmac_vlan_rx_kill_vid(struct net_device *ndev, __be16 proto, u16 vid)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel