[v1] net: stmmac: misc fixes

[PATCH net 3/3] net: stmmac: check if interface is running before TC block setup

Posted by Konrad Leszczynski 1 month ago

From: Karol Jurczenia <karol.jurczenia@intel.com>

If the interface is down before setting a TC block, the queues are already
disabled and setup cannot proceed.

Fixes: 4dbbe8dde8485b89 ("net: stmmac: Add support for U32 TC filter using Flexible RX Parser")
Reviewed-by: Sebastian Basierski <sebastian.basierski@intel.com>
Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com>
Signed-off-by: Karol Jurczenia <karol.jurczenia@intel.com>
Signed-off-by: Konrad Leszczynski <konrad.leszczynski@intel.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 70c3dd88a749..202a157a1c90 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -6247,6 +6247,9 @@ static int stmmac_setup_tc_block_cb(enum tc_setup_type type, void *type_data,
 	struct stmmac_priv *priv = cb_priv;
 	int ret = -EOPNOTSUPP;
 
+	if (!netif_running(priv->dev))
+		return -EINVAL;
+
 	if (!tc_cls_can_offload_and_chain0(priv->dev, type_data))
 		return ret;
 
-- 
2.34.1

Re: [PATCH net 3/3] net: stmmac: check if interface is running before TC block setup

Posted by Jakub Kicinski 1 month ago

On Thu, 28 Aug 2025 12:02:37 +0200 Konrad Leszczynski wrote:
> If the interface is down before setting a TC block, the queues are already
> disabled and setup cannot proceed.

More context would be useful. What's the user-visible behavior before
and after? Can the device handle installing the filters while down? 
Is it just an issue of us restarting the queues when we shouldn't?
-- 
pw-bot: cr

Re: [PATCH net 3/3] net: stmmac: check if interface is running before TC block setup

Posted by Sebastian Basierski 4 weeks ago

On 9/1/2025 10:03 PM, Jakub Kicinski wrote:
> More context would be useful. What's the user-visible behavior before
> and after? Can the device handle installing the filters while down?
> Is it just an issue of us restarting the queues when we shouldn't?

Before this patch driver couldn't be unloaded with tc filter applied.

Running those commands is enough to reproduce the issue:
   tc qdisc add dev enp0s29f2 ingress
   tc filter add dev enp0s29f2 ingress protocol all prio 1 u32
   rmmod dwmac_intel

in effect module would not unload.

Re: [PATCH net 3/3] net: stmmac: check if interface is running before TC block setup

Posted by Jakub Kicinski 4 weeks ago

On Thu, 4 Sep 2025 21:01:49 +0200 Sebastian Basierski wrote:
> On 9/1/2025 10:03 PM, Jakub Kicinski wrote:
> > More context would be useful. What's the user-visible behavior before
> > and after? Can the device handle installing the filters while down?
> > Is it just an issue of us restarting the queues when we shouldn't?  
> 
> Before this patch driver couldn't be unloaded with tc filter applied.
> 
> Running those commands is enough to reproduce the issue:
>    tc qdisc add dev enp0s29f2 ingress
>    tc filter add dev enp0s29f2 ingress protocol all prio 1 u32
>    rmmod dwmac_intel
> 
> in effect module would not unload.

Makes sense. Could you also confirm that the offload doesn't in fact
work if set up when device is down? I think block setup is when qdisc
is installed?

ip link set dev $x down
tc qdisc add dev enp0s29f2 ingress
ip link set dev $x up
tc filter add dev enp0s29f2 ingress protocol all prio 1 u32 ...

If it doesn't work we can feel safe we're not breaking anyone's
scripts, however questionable.

Re: [PATCH net 3/3] net: stmmac: check if interface is running before TC block setup

Posted by Sebastian Basierski 3 weeks, 2 days ago

On 9/5/2025 3:56 AM, Jakub Kicinski wrote:
> On Thu, 4 Sep 2025 21:01:49 +0200 Sebastian Basierski wrote:
>> On 9/1/2025 10:03 PM, Jakub Kicinski wrote:
>>> More context would be useful. What's the user-visible behavior before
>>> and after? Can the device handle installing the filters while down?
>>> Is it just an issue of us restarting the queues when we shouldn't?
>> Before this patch driver couldn't be unloaded with tc filter applied.
>>
>> Running those commands is enough to reproduce the issue:
>>     tc qdisc add dev enp0s29f2 ingress
>>     tc filter add dev enp0s29f2 ingress protocol all prio 1 u32
>>     rmmod dwmac_intel
>>
>> in effect module would not unload.
> Makes sense. Could you also confirm that the offload doesn't in fact
> work if set up when device is down? I think block setup is when qdisc
> is installed?
>
> ip link set dev $x down
> tc qdisc add dev enp0s29f2 ingress
> ip link set dev $x up
> tc filter add dev enp0s29f2 ingress protocol all prio 1 u32 ...
>
> If it doesn't work we can feel safe we're not breaking anyone's
> scripts, however questionable.
Sorry for late response.
I just checked what you asked for.
   x="enp129s29f0"
   ip link set dev $x down
   tc qdisc add dev $x ingress
   ip link set dev $x up
   tc filter add dev $x ingress protocol ip flower ip_proto 1 action drop
Looks like with and without patch ICMP packets are dropped.

Re: [PATCH net 3/3] net: stmmac: check if interface is running before TC block setup

Posted by Jakub Kicinski 2 weeks, 2 days ago

On Tue, 9 Sep 2025 20:47:01 +0200 Sebastian Basierski wrote:
> >> Before this patch driver couldn't be unloaded with tc filter applied.
> >>
> >> Running those commands is enough to reproduce the issue:
> >>     tc qdisc add dev enp0s29f2 ingress
> >>     tc filter add dev enp0s29f2 ingress protocol all prio 1 u32
> >>     rmmod dwmac_intel
> >>
> >> in effect module would not unload.  
> > Makes sense. Could you also confirm that the offload doesn't in fact
> > work if set up when device is down? I think block setup is when qdisc
> > is installed?
> >
> > ip link set dev $x down
> > tc qdisc add dev enp0s29f2 ingress
> > ip link set dev $x up
> > tc filter add dev enp0s29f2 ingress protocol all prio 1 u32 ...
> >
> > If it doesn't work we can feel safe we're not breaking anyone's
> > scripts, however questionable.  
> Sorry for late response.
> I just checked what you asked for.
>    x="enp129s29f0"
>    ip link set dev $x down
>    tc qdisc add dev $x ingress
>    ip link set dev $x up
>    tc filter add dev $x ingress protocol ip flower ip_proto 1 action drop
> Looks like with and without patch ICMP packets are dropped.

Aren't you testing non-offloaded filter?
Test with skip_sw, if it works it means that some order of commands may
have indeed worked.

Re: [PATCH net 3/3] net: stmmac: check if interface is running before TC block setup

Posted by Vadim Fedorenko 1 month ago

On 28/08/2025 11:02, Konrad Leszczynski wrote:
> From: Karol Jurczenia <karol.jurczenia@intel.com>
> 
> If the interface is down before setting a TC block, the queues are already
> disabled and setup cannot proceed.
> 
> Fixes: 4dbbe8dde8485b89 ("net: stmmac: Add support for U32 TC filter using Flexible RX Parser")
> Reviewed-by: Sebastian Basierski <sebastian.basierski@intel.com>
> Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com>
> Signed-off-by: Karol Jurczenia <karol.jurczenia@intel.com>
> Signed-off-by: Konrad Leszczynski <konrad.leszczynski@intel.com>
> ---
>   drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 70c3dd88a749..202a157a1c90 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -6247,6 +6247,9 @@ static int stmmac_setup_tc_block_cb(enum tc_setup_type type, void *type_data,
>   	struct stmmac_priv *priv = cb_priv;
>   	int ret = -EOPNOTSUPP;
>   
> +	if (!netif_running(priv->dev))
> +		return -EINVAL;
> +

The check looks valid, but I'm not quite sure of the error code.
Anyways,
Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>