[PATCH net-next] net: dsa: mv88e6xxx: Allow hwstamping on the master port

[PATCH net-next] net: dsa: mv88e6xxx: Allow hwstamping on the master port

[Fabio Estevam]

From: Steffen Bätz <steffen@innosonix.de>

Currently, it is not possible to run SIOCGHWTSTAMP or SIOCSHWTSTAMP
ioctls on the dsa master interface if the port_hwtstamp_set/get()
hooks are present, as implemented in net/dsa/master.c:

static int dsa_master_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
{
...
	case SIOCGHWTSTAMP:
	case SIOCSHWTSTAMP:
		/* Deny PTP operations on master if there is at least one
		 * switch in the tree that is PTP capable.
		 */
		list_for_each_entry(dp, &dst->ports, list)
			if (dp->ds->ops->port_hwtstamp_get ||
			    dp->ds->ops->port_hwtstamp_set)
				return -EBUSY;
		break;
	}

Even if the hwtstamping functionality is disabled in the mv88e6xxx driver
by not setting CONFIG_NET_DSA_MV88E6XXX_PTP, the functions port_hwtstamp_set()
port_hwtstamp_get() are still present due to their stub declarations.

Fix this problem, by removing the stub declarations and guarding these
functions wih CONFIG_NET_DSA_MV88E6XXX_PTP.

Without this change:

 # hwstamp_ctl -i eth0
SIOCGHWTSTAMP failed: Device or resource busy

With the change applied, it is possible to set and get the timestamping
options:

 # hwstamp_ctl -i eth0
current settings:
tx_type 0
rx_filter 0

 # hwstamp_ctl -i eth0 -r 1 -t 1
current settings:
tx_type 0
rx_filter 0
new settings:
tx_type 1
rx_filter 1

Tested on a custom i.MX8MN board with a 88E6320 switch.

Signed-off-by: Steffen Bätz <steffen@innosonix.de>
Signed-off-by: Fabio Estevam <festevam@denx.de>
---
 drivers/net/dsa/mv88e6xxx/chip.c     |  2 ++
 drivers/net/dsa/mv88e6xxx/hwtstamp.h | 12 ------------
 2 files changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index bf34c942db99..cfa0168cab03 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -6925,8 +6925,10 @@ static const struct dsa_switch_ops mv88e6xxx_switch_ops = {
 	.port_mirror_del	= mv88e6xxx_port_mirror_del,
 	.crosschip_bridge_join	= mv88e6xxx_crosschip_bridge_join,
 	.crosschip_bridge_leave	= mv88e6xxx_crosschip_bridge_leave,
+#ifdef CONFIG_NET_DSA_MV88E6XXX_PTP
 	.port_hwtstamp_set	= mv88e6xxx_port_hwtstamp_set,
 	.port_hwtstamp_get	= mv88e6xxx_port_hwtstamp_get,
+#endif
 	.port_txtstamp		= mv88e6xxx_port_txtstamp,
 	.port_rxtstamp		= mv88e6xxx_port_rxtstamp,
 	.get_ts_info		= mv88e6xxx_get_ts_info,
diff --git a/drivers/net/dsa/mv88e6xxx/hwtstamp.h b/drivers/net/dsa/mv88e6xxx/hwtstamp.h
index cf7fb6d660b1..86c6213c2ae8 100644
--- a/drivers/net/dsa/mv88e6xxx/hwtstamp.h
+++ b/drivers/net/dsa/mv88e6xxx/hwtstamp.h
@@ -132,18 +132,6 @@ int mv88e6165_global_disable(struct mv88e6xxx_chip *chip);
 
 #else /* !CONFIG_NET_DSA_MV88E6XXX_PTP */
 
-static inline int mv88e6xxx_port_hwtstamp_set(struct dsa_switch *ds,
-					      int port, struct ifreq *ifr)
-{
-	return -EOPNOTSUPP;
-}
-
-static inline int mv88e6xxx_port_hwtstamp_get(struct dsa_switch *ds,
-					      int port, struct ifreq *ifr)
-{
-	return -EOPNOTSUPP;
-}
-
 static inline bool mv88e6xxx_port_rxtstamp(struct dsa_switch *ds, int port,
 					   struct sk_buff *clone,
 					   unsigned int type)
-- 
2.25.1

Re: [PATCH net-next] net: dsa: mv88e6xxx: Allow hwstamping on the master port

[Vladimir Oltean]

On Thu, Nov 10, 2022 at 09:43:45AM -0300, Fabio Estevam wrote:
> From: Steffen Bätz <steffen@innosonix.de>
> 
> Currently, it is not possible to run SIOCGHWTSTAMP or SIOCSHWTSTAMP
> ioctls on the dsa master interface if the port_hwtstamp_set/get()
> hooks are present, as implemented in net/dsa/master.c:
> 
> static int dsa_master_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
> {
> ...
> 	case SIOCGHWTSTAMP:
> 	case SIOCSHWTSTAMP:
> 		/* Deny PTP operations on master if there is at least one
> 		 * switch in the tree that is PTP capable.
> 		 */
> 		list_for_each_entry(dp, &dst->ports, list)
> 			if (dp->ds->ops->port_hwtstamp_get ||
> 			    dp->ds->ops->port_hwtstamp_set)
> 				return -EBUSY;
> 		break;
> 	}
> 
> Even if the hwtstamping functionality is disabled in the mv88e6xxx driver
> by not setting CONFIG_NET_DSA_MV88E6XXX_PTP, the functions port_hwtstamp_set()
> port_hwtstamp_get() are still present due to their stub declarations.
> 
> Fix this problem, by removing the stub declarations and guarding these
> functions wih CONFIG_NET_DSA_MV88E6XXX_PTP.
> 
> Without this change:
> 
>  # hwstamp_ctl -i eth0
> SIOCGHWTSTAMP failed: Device or resource busy
> 
> With the change applied, it is possible to set and get the timestamping
> options:
> 
>  # hwstamp_ctl -i eth0
> current settings:
> tx_type 0
> rx_filter 0
> 
>  # hwstamp_ctl -i eth0 -r 1 -t 1
> current settings:
> tx_type 0
> rx_filter 0
> new settings:
> tx_type 1
> rx_filter 1
> 
> Tested on a custom i.MX8MN board with a 88E6320 switch.
> 
> Signed-off-by: Steffen Bätz <steffen@innosonix.de>
> Signed-off-by: Fabio Estevam <festevam@denx.de>
> ---

NACK.

Please extend dsa_master_ioctl() to "see through" the dp->ds->ops->port_hwtstamp_get()
pointer, similar to what dsa_port_can_configure_learning() does. Create
a fake struct ifreq, call port_hwtstamp_get(), see if it returned
-EOPNOTSUPP, and wrap that into a dsa_port_supports_hwtstamp() function,
and call that instead of the current simplistic checks for the function
pointers.

Re: [PATCH net-next] net: dsa: mv88e6xxx: Allow hwstamping on the master port

[Fabio Estevam]

[-- Attachment #1: Type: text/plain, Size: 701 bytes --]

Hi Vladimir,

On Thu, Nov 10, 2022 at 9:53 AM Vladimir Oltean <olteanv@gmail.com> wrote:

> NACK.
>
> Please extend dsa_master_ioctl() to "see through" the dp->ds->ops->port_hwtstamp_get()
> pointer, similar to what dsa_port_can_configure_learning() does. Create
> a fake struct ifreq, call port_hwtstamp_get(), see if it returned
> -EOPNOTSUPP, and wrap that into a dsa_port_supports_hwtstamp() function,
> and call that instead of the current simplistic checks for the function
> pointers.

I tried to implement what you suggested in the attached patch, but I
am not sure it is correct.

If it doesn't work, please cook a patch so we can try it.

I am not familiar with the DSA code, sorry.

Thanks

[-- Attachment #2: 0001-net-dsa-Introduce-dsa_port_supports_hwtstamp.patch --]
[-- Type: text/x-patch, Size: 3179 bytes --]

From 6f45968dd2e4585818ea6acf8420bfed38246604 Mon Sep 17 00:00:00 2001
From: Fabio Estevam <festevam@denx.de>
Date: Thu, 10 Nov 2022 18:50:59 -0300
Subject: [PATCH net-next] net: dsa: Introduce dsa_port_supports_hwtstamp()

Currently, during SIOCGHWTSTAMP/SIOCSHWTSTAMP ioctl operations, the
criteria for deciding whether PTP operations on the master port is allowed
or not is simply based on the presence of the port_hwtstamp_get/set()
operations.

This is not a robust method because on the mv88e6xxx driver, for example,
the port_hwtstamp_get()/set() hooks are always implemented.

Even when CONFIG_NET_DSA_MV88E6XXX_PTP is disabled there are still stub
implementations for these functions that return -EOPNOTSUPP.

Instead of relying on the presence of port_hwtstamp_get/set(), introduce
the dsa_port_supports_hwtstamp() function, which checks the return
value of port_hwtstamp_get/set() to decide.

Without this change:

 # hwstamp_ctl -i eth0
SIOCGHWTSTAMP failed: Device or resource busy

With this change applied, it is possible to set and get the timestamping
options:

 # hwstamp_ctl -i eth0
current settings:
tx_type 0
rx_filter 0

 # hwstamp_ctl -i eth0 -r 1 -t 1
current settings:
tx_type 0
rx_filter 0
new settings:
tx_type 1
rx_filter 1

Tested on a custom i.MX8MN board with a 88E6320 switch.

Signed-off-by: Fabio Estevam <festevam@denx.de>
---
 net/dsa/master.c | 40 ++++++++++++++++++++++++++++------------
 1 file changed, 28 insertions(+), 12 deletions(-)

diff --git a/net/dsa/master.c b/net/dsa/master.c
index 2851e44c4cf0..f078db44a8b3 100644
--- a/net/dsa/master.c
+++ b/net/dsa/master.c
@@ -187,29 +187,45 @@ static void dsa_master_get_strings(struct net_device *dev, uint32_t stringset,
 	}
 }
 
-static int dsa_master_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
+static bool dsa_port_supports_hwtstamp(struct net_device *dev, struct ifreq *ifr, int cmd)
 {
 	struct dsa_port *cpu_dp = dev->dsa_ptr;
 	struct dsa_switch *ds = cpu_dp->ds;
-	struct dsa_switch_tree *dst;
+	int port = cpu_dp->index;
 	int err = -EOPNOTSUPP;
-	struct dsa_port *dp;
-
-	dst = ds->dst;
 
 	switch (cmd) {
 	case SIOCGHWTSTAMP:
+		if (ds->ops->port_hwtstamp_get) {
+			err = ds->ops->port_hwtstamp_get(ds, port, ifr);
+			if (err == -EOPNOTSUPP)
+				return false;
+		}
+		break;
 	case SIOCSHWTSTAMP:
-		/* Deny PTP operations on master if there is at least one
-		 * switch in the tree that is PTP capable.
-		 */
-		list_for_each_entry(dp, &dst->ports, list)
-			if (dp->ds->ops->port_hwtstamp_get ||
-			    dp->ds->ops->port_hwtstamp_set)
-				return -EBUSY;
+		if (ds->ops->port_hwtstamp_set) {
+			err = ds->ops->port_hwtstamp_set(ds, port, ifr);
+			if (err == -EOPNOTSUPP)
+				return false;
+		}
 		break;
 	}
 
+	return !!err;
+}
+
+static int dsa_master_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
+{
+	struct dsa_port *cpu_dp = dev->dsa_ptr;
+	struct dsa_switch *ds = cpu_dp->ds;
+	struct dsa_switch_tree *dst;
+	int err = -EOPNOTSUPP;
+
+	dst = ds->dst;
+
+	if (dsa_port_supports_hwtstamp(dev, ifr, cmd))
+		return -EBUSY;
+
 	if (dev->netdev_ops->ndo_eth_ioctl)
 		err = dev->netdev_ops->ndo_eth_ioctl(dev, ifr, cmd);
 
-- 
2.25.1

Re: [PATCH net-next] net: dsa: mv88e6xxx: Allow hwstamping on the master port

[Vladimir Oltean]

On Fri, Nov 11, 2022 at 08:13:56AM -0300, Fabio Estevam wrote:
> Hi Vladimir,
> 
> On Thu, Nov 10, 2022 at 9:53 AM Vladimir Oltean <olteanv@gmail.com> wrote:
> 
> > NACK.
> >
> > Please extend dsa_master_ioctl() to "see through" the dp->ds->ops->port_hwtstamp_get()
> > pointer, similar to what dsa_port_can_configure_learning() does. Create
> > a fake struct ifreq, call port_hwtstamp_get(), see if it returned
> > -EOPNOTSUPP, and wrap that into a dsa_port_supports_hwtstamp() function,
> > and call that instead of the current simplistic checks for the function
> > pointers.
> 
> I tried to implement what you suggested in the attached patch, but I
> am not sure it is correct.
> 
> If it doesn't work, please cook a patch so we can try it.

Yeah, this is not what I meant. I posted a patch illustrating what I
meant here:
https://patchwork.kernel.org/project/netdevbpf/patch/20221111211020.543540-1-vladimir.oltean@nxp.com/

Re: [PATCH net-next] net: dsa: mv88e6xxx: Allow hwstamping on the master port

[Fabio Estevam]

Hi Vladimir,

On Fri, Nov 11, 2022 at 6:41 PM Vladimir Oltean <olteanv@gmail.com> wrote:

> Yeah, this is not what I meant. I posted a patch illustrating what I
> meant here:
> https://patchwork.kernel.org/project/netdevbpf/patch/20221111211020.543540-1-vladimir.oltean@nxp.com/

Thanks for your patch, appreciated.

I tested your patch and it works fine.

Cheers