[PATCH net] net: lan966x: Remove ptp traps in case the ptp is not enabled.

[PATCH net] net: lan966x: Remove ptp traps in case the ptp is not enabled.

[Horatiu Vultur]

Lan966x is adding ptp traps to redirect the ptp frames to the CPU such
that the HW will not forward these frames anywhere. The issue is that in
case ptp is not enabled and the timestamping source is et to
HWTSTAMP_SOURCE_NETDEV then these traps would not be removed on the
error path.
Fix this by removing the traps in this case as they are not needed.

Fixes: 54e1ed69c40a ("net: lan966x: convert to ndo_hwtstamp_get() and ndo_hwtstamp_set()")
Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 drivers/net/ethernet/microchip/lan966x/lan966x_main.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
index 2635ef8958c80..318676e42bb62 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
@@ -479,8 +479,10 @@ static int lan966x_port_hwtstamp_set(struct net_device *dev,
 		return err;
 
 	if (cfg->source == HWTSTAMP_SOURCE_NETDEV) {
-		if (!port->lan966x->ptp)
+		if (!port->lan966x->ptp) {
+			lan966x_ptp_del_traps(port);
 			return -EOPNOTSUPP;
+		}
 
 		err = lan966x_ptp_hwtstamp_set(port, cfg, extack);
 		if (err) {
-- 
2.34.1

Re: [PATCH net] net: lan966x: Remove ptp traps in case the ptp is not enabled.

[Vladimir Oltean]

On Tue, May 14, 2024 at 09:35:00PM +0200, Horatiu Vultur wrote:
> Lan966x is adding ptp traps to redirect the ptp frames to the CPU such
> that the HW will not forward these frames anywhere. The issue is that in
> case ptp is not enabled and the timestamping source is et to
> HWTSTAMP_SOURCE_NETDEV then these traps would not be removed on the
> error path.
> Fix this by removing the traps in this case as they are not needed.
> 
> Fixes: 54e1ed69c40a ("net: lan966x: convert to ndo_hwtstamp_get() and ndo_hwtstamp_set()")
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
>  drivers/net/ethernet/microchip/lan966x/lan966x_main.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> index 2635ef8958c80..318676e42bb62 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> @@ -479,8 +479,10 @@ static int lan966x_port_hwtstamp_set(struct net_device *dev,
>  		return err;
>  
>  	if (cfg->source == HWTSTAMP_SOURCE_NETDEV) {
> -		if (!port->lan966x->ptp)
> +		if (!port->lan966x->ptp) {
> +			lan966x_ptp_del_traps(port);
>  			return -EOPNOTSUPP;
> +		}
>  
>  		err = lan966x_ptp_hwtstamp_set(port, cfg, extack);
>  		if (err) {
> -- 
> 2.34.1
>

Alternatively, the -EOPNOTSUPP check could be moved before programming
the traps in the first place.

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Re: [PATCH net] net: lan966x: Remove ptp traps in case the ptp is not enabled.

[Horatiu Vultur]

The 05/15/2024 01:21, Vladimir Oltean wrote:

Hi Vladimir,

> 
> On Tue, May 14, 2024 at 09:35:00PM +0200, Horatiu Vultur wrote:
> > Lan966x is adding ptp traps to redirect the ptp frames to the CPU such
> > that the HW will not forward these frames anywhere. The issue is that in
> > case ptp is not enabled and the timestamping source is et to
> > HWTSTAMP_SOURCE_NETDEV then these traps would not be removed on the
> > error path.
> > Fix this by removing the traps in this case as they are not needed.
> >
> > Fixes: 54e1ed69c40a ("net: lan966x: convert to ndo_hwtstamp_get() and ndo_hwtstamp_set()")
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> > ---
> >  drivers/net/ethernet/microchip/lan966x/lan966x_main.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> > index 2635ef8958c80..318676e42bb62 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> > @@ -479,8 +479,10 @@ static int lan966x_port_hwtstamp_set(struct net_device *dev,
> >               return err;
> >
> >       if (cfg->source == HWTSTAMP_SOURCE_NETDEV) {
> > -             if (!port->lan966x->ptp)
> > +             if (!port->lan966x->ptp) {
> > +                     lan966x_ptp_del_traps(port);
> >                       return -EOPNOTSUPP;
> > +             }
> >
> >               err = lan966x_ptp_hwtstamp_set(port, cfg, extack);
> >               if (err) {
> > --
> > 2.34.1
> >
> 
> Alternatively, the -EOPNOTSUPP check could be moved before programming
> the traps in the first place.

Thanks for the review.
Actually I don't think this alternative will work. In case of PHY
timestamping, we would still like to add those rules regardless if
ptp is enabled on lan966x.

> 
> Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

-- 
/Horatiu

Re: [PATCH net] net: lan966x: Remove ptp traps in case the ptp is not enabled.

[Vladimir Oltean]

On Thu, May 16, 2024 at 08:48:55AM +0200, Horatiu Vultur wrote:
> > Alternatively, the -EOPNOTSUPP check could be moved before programming
> > the traps in the first place.
> 
> Thanks for the review.
> Actually I don't think this alternative will work. In case of PHY
> timestamping, we would still like to add those rules regardless if
> ptp is enabled on lan966x.
> 
> > 
> > Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> -- 
> /Horatiu

I don't understand why this would not have worked?

diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
index b12d3b8a64fd..1439a36e8394 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
@@ -474,14 +474,14 @@ static int lan966x_port_hwtstamp_set(struct net_device *dev,
 	    cfg->source != HWTSTAMP_SOURCE_PHYLIB)
 		return -EOPNOTSUPP;
 
+	if (cfg->source == HWTSTAMP_SOURCE_NETDEV && !port->lan966x->ptp)
+		return -EOPNOTSUPP;
+
 	err = lan966x_ptp_setup_traps(port, cfg);
 	if (err)
 		return err;
 
 	if (cfg->source == HWTSTAMP_SOURCE_NETDEV) {
-		if (!port->lan966x->ptp)
-			return -EOPNOTSUPP;
-
 		err = lan966x_ptp_hwtstamp_set(port, cfg, extack);
 		if (err) {
 			lan966x_ptp_del_traps(port);

Re: [PATCH net] net: lan966x: Remove ptp traps in case the ptp is not enabled.

[Horatiu Vultur]

The 05/17/2024 13:04, Vladimir Oltean wrote:
> 
> On Thu, May 16, 2024 at 08:48:55AM +0200, Horatiu Vultur wrote:
> > > Alternatively, the -EOPNOTSUPP check could be moved before programming
> > > the traps in the first place.
> >
> > Thanks for the review.
> > Actually I don't think this alternative will work. In case of PHY
> > timestamping, we would still like to add those rules regardless if
> > ptp is enabled on lan966x.
> >
> > >
> > > Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> >
> > --
> > /Horatiu
> 
> I don't understand why this would not have worked?
> 
> diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> index b12d3b8a64fd..1439a36e8394 100644
> --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> @@ -474,14 +474,14 @@ static int lan966x_port_hwtstamp_set(struct net_device *dev,
>             cfg->source != HWTSTAMP_SOURCE_PHYLIB)
>                 return -EOPNOTSUPP;
> 
> +       if (cfg->source == HWTSTAMP_SOURCE_NETDEV && !port->lan966x->ptp)
> +               return -EOPNOTSUPP;
> +

This should also work.
Initially I thought you wanted to have only the check for
port->lan966x->ptp here. And that is why I said it would not work.

>         err = lan966x_ptp_setup_traps(port, cfg);
>         if (err)
>                 return err;
> 
>         if (cfg->source == HWTSTAMP_SOURCE_NETDEV) {
> -               if (!port->lan966x->ptp)
> -                       return -EOPNOTSUPP;
> -
>                 err = lan966x_ptp_hwtstamp_set(port, cfg, extack);
>                 if (err) {
>                         lan966x_ptp_del_traps(port);

-- 
/Horatiu

Re: [PATCH net] net: lan966x: Remove ptp traps in case the ptp is not enabled.

[Vladimir Oltean]

On Fri, May 17, 2024 at 12:18:11PM +0200, Horatiu Vultur wrote:
> The 05/17/2024 13:04, Vladimir Oltean wrote:
> > 
> > On Thu, May 16, 2024 at 08:48:55AM +0200, Horatiu Vultur wrote:
> > > > Alternatively, the -EOPNOTSUPP check could be moved before programming
> > > > the traps in the first place.
> > >
> > > Thanks for the review.
> > > Actually I don't think this alternative will work. In case of PHY
> > > timestamping, we would still like to add those rules regardless if
> > > ptp is enabled on lan966x.
> > >
> > > >
> > > > Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > >
> > > --
> > > /Horatiu
> > 
> > I don't understand why this would not have worked?
> > 
> > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> > index b12d3b8a64fd..1439a36e8394 100644
> > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> > @@ -474,14 +474,14 @@ static int lan966x_port_hwtstamp_set(struct net_device *dev,
> >             cfg->source != HWTSTAMP_SOURCE_PHYLIB)
> >                 return -EOPNOTSUPP;
> > 
> > +       if (cfg->source == HWTSTAMP_SOURCE_NETDEV && !port->lan966x->ptp)
> > +               return -EOPNOTSUPP;
> > +
> 
> This should also work.
> Initially I thought you wanted to have only the check for
> port->lan966x->ptp here. And that is why I said it would not work.

Ok. I see the patch was marked as "changes requested". I think the
second alternative would be better anyway, because a requested
configuration which cannot be supported will be rejected outright,
rather than doing some stuff, figuring out it cannot be done, then
undoing what was done. Would you mind sending a v2 like this?

Re: [PATCH net] net: lan966x: Remove ptp traps in case the ptp is not enabled.

[Horatiu Vultur]

The 05/17/2024 13:23, Vladimir Oltean wrote:
> 
> On Fri, May 17, 2024 at 12:18:11PM +0200, Horatiu Vultur wrote:
> > The 05/17/2024 13:04, Vladimir Oltean wrote:
> > >
> > > On Thu, May 16, 2024 at 08:48:55AM +0200, Horatiu Vultur wrote:
> > > > > Alternatively, the -EOPNOTSUPP check could be moved before programming
> > > > > the traps in the first place.
> > > >
> > > > Thanks for the review.
> > > > Actually I don't think this alternative will work. In case of PHY
> > > > timestamping, we would still like to add those rules regardless if
> > > > ptp is enabled on lan966x.
> > > >
> > > > >
> > > > > Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > >
> > > > --
> > > > /Horatiu
> > >
> > > I don't understand why this would not have worked?
> > >
> > > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> > > index b12d3b8a64fd..1439a36e8394 100644
> > > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> > > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
> > > @@ -474,14 +474,14 @@ static int lan966x_port_hwtstamp_set(struct net_device *dev,
> > >             cfg->source != HWTSTAMP_SOURCE_PHYLIB)
> > >                 return -EOPNOTSUPP;
> > >
> > > +       if (cfg->source == HWTSTAMP_SOURCE_NETDEV && !port->lan966x->ptp)
> > > +               return -EOPNOTSUPP;
> > > +
> >
> > This should also work.
> > Initially I thought you wanted to have only the check for
> > port->lan966x->ptp here. And that is why I said it would not work.
> 
> Ok. I see the patch was marked as "changes requested". I think the
> second alternative would be better anyway, because a requested
> configuration which cannot be supported will be rejected outright,
> rather than doing some stuff, figuring out it cannot be done, then
> undoing what was done. Would you mind sending a v2 like this?

I will send a v2 as you suggested.

-- 
/Horatiu