diff mbox series

[v8,net-next,06/12] net: fec: convert to ndo_hwtstamp_get() and ndo_hwtstamp_set()

Message ID 20230717152709.574773-7-vladimir.oltean@nxp.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series Introduce ndo_hwtstamp_get() and ndo_hwtstamp_set() | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1342 this patch: 1342
netdev/cc_maintainers success CCed 10 of 10 maintainers
netdev/build_clang success Errors and warnings before: 1365 this patch: 1365
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1365 this patch: 1365
netdev/checkpatch warning WARNING: line length of 81 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Vladimir Oltean July 17, 2023, 3:27 p.m. UTC 
The hardware timestamping through ndo_eth_ioctl() is going away.
Convert the FEC driver to the new API before that can be removed.

After removing the timestamping logic from fec_enet_ioctl(), the rest
is equivalent to phy_do_ioctl_running().

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
Reviewed-by: Wei Fang <wei.fang@nxp.com>
---
Changes in v8:
- Use phy_do_ioctl_running()
Changes in v7:
- Patch is new

 drivers/net/ethernet/freescale/fec.h      |  5 +-
 drivers/net/ethernet/freescale/fec_main.c | 67 +++++++++++++----------
 drivers/net/ethernet/freescale/fec_ptp.c  | 31 ++++-------
 3 files changed, 53 insertions(+), 50 deletions(-)

Comments

Russell King (Oracle) July 18, 2023, 2:20 p.m. UTC | #1 
On Mon, Jul 17, 2023 at 06:27:03PM +0300, Vladimir Oltean wrote:
> -static int fec_enet_ioctl(struct net_device *ndev, struct ifreq *rq, int cmd)
> -{
> -	struct fec_enet_private *fep = netdev_priv(ndev);
> -	struct phy_device *phydev = ndev->phydev;
> -
> -	if (!netif_running(ndev))
> -		return -EINVAL;
> -
> -	if (!phydev)
> -		return -ENODEV;
> -
... process hwtstamp calls

So if the network device is not running, ioctl() returns -EINVAL. From
what I can see in fec_enet_mii_probe() called from fec_enet_open(), we
guarantee that phydev will not be NULL once the first open has
succeeded, so I don't think the second if() statement has any effect.

> +static int fec_hwtstamp_get(struct net_device *ndev,
> +			    struct kernel_hwtstamp_config *config)
> +{
> +	struct fec_enet_private *fep = netdev_priv(ndev);
> +	struct phy_device *phydev = ndev->phydev;
> +
> +	if (phy_has_hwtstamp(phydev))
> +		return phy_mii_ioctl(phydev, config->ifr, SIOCGHWTSTAMP);
> +
> +	if (!fep->bufdesc_ex)
> +		return -EOPNOTSUPP;

If the interface hasn't been brought up at least once, then phydev
here will be NULL, and we'll drop through to this test. If the FEC
doesn't support extended buffer descriptors, userspace will see
-EOPNOTSUPP rather than -EINVAL. This could be misleading to userspace.

Does this need something like:

	if (!netif_running(ndev))
		return -EINVAL;

before, or maybe just after phy_has_hwtstamp() to give equivalent
behaviour?

> +static int fec_hwtstamp_set(struct net_device *ndev,
> +			    struct kernel_hwtstamp_config *config,
> +			    struct netlink_ext_ack *extack)
> +{
> +	struct fec_enet_private *fep = netdev_priv(ndev);
> +	struct phy_device *phydev = ndev->phydev;
> +
> +	if (phy_has_hwtstamp(phydev)) {
> +		fec_ptp_disable_hwts(ndev);
> +
> +		return phy_mii_ioctl(phydev, config->ifr, SIOCSHWTSTAMP);
> +	}
> +
> +	if (!fep->bufdesc_ex)
> +		return -EOPNOTSUPP;

Same comment here.
Vladimir Oltean Aug. 1, 2023, 1:12 p.m. UTC | #2 
On Tue, Jul 18, 2023 at 03:20:13PM +0100, Russell King (Oracle) wrote:
> > +static int fec_hwtstamp_get(struct net_device *ndev,
> > +			    struct kernel_hwtstamp_config *config)
> > +{
> > +	struct fec_enet_private *fep = netdev_priv(ndev);
> > +	struct phy_device *phydev = ndev->phydev;
> > +
> > +	if (phy_has_hwtstamp(phydev))
> > +		return phy_mii_ioctl(phydev, config->ifr, SIOCGHWTSTAMP);
> > +
> > +	if (!fep->bufdesc_ex)
> > +		return -EOPNOTSUPP;
> 
> If the interface hasn't been brought up at least once, then phydev
> here will be NULL, and we'll drop through to this test. If the FEC
> doesn't support extended buffer descriptors, userspace will see
> -EOPNOTSUPP rather than -EINVAL. This could be misleading to userspace.
> 
> Does this need something like:
> 
> 	if (!netif_running(ndev))
> 		return -EINVAL;
> 
> before, or maybe just after phy_has_hwtstamp() to give equivalent
> behaviour?
> 
> > +static int fec_hwtstamp_set(struct net_device *ndev,
> > +			    struct kernel_hwtstamp_config *config,
> > +			    struct netlink_ext_ack *extack)
> > +{
> > +	struct fec_enet_private *fep = netdev_priv(ndev);
> > +	struct phy_device *phydev = ndev->phydev;
> > +
> > +	if (phy_has_hwtstamp(phydev)) {
> > +		fec_ptp_disable_hwts(ndev);
> > +
> > +		return phy_mii_ioctl(phydev, config->ifr, SIOCSHWTSTAMP);
> > +	}
> > +
> > +	if (!fep->bufdesc_ex)
> > +		return -EOPNOTSUPP;
> 
> Same comment here.

I will add back the netif_running() check to continue to not permit the
operation to go through, as before.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index 63a053dea819..5d7b76f0c829 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -704,8 +704,9 @@  void fec_ptp_init(struct platform_device *pdev, int irq_idx);
 void fec_ptp_stop(struct platform_device *pdev);
 void fec_ptp_start_cyclecounter(struct net_device *ndev);
 void fec_ptp_disable_hwts(struct net_device *ndev);
-int fec_ptp_set(struct net_device *ndev, struct ifreq *ifr);
-int fec_ptp_get(struct net_device *ndev, struct ifreq *ifr);
+int fec_ptp_set(struct net_device *ndev, struct kernel_hwtstamp_config *config,
+		struct netlink_ext_ack *extack);
+void fec_ptp_get(struct net_device *ndev, struct kernel_hwtstamp_config *config);
 
 /****************************************************************************/
 #endif /* FEC_H */
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 1b990a486059..c35b569d848a 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -3237,33 +3237,6 @@  static const struct ethtool_ops fec_enet_ethtool_ops = {
 	.self_test		= net_selftest,
 };
 
-static int fec_enet_ioctl(struct net_device *ndev, struct ifreq *rq, int cmd)
-{
-	struct fec_enet_private *fep = netdev_priv(ndev);
-	struct phy_device *phydev = ndev->phydev;
-
-	if (!netif_running(ndev))
-		return -EINVAL;
-
-	if (!phydev)
-		return -ENODEV;
-
-	if (fep->bufdesc_ex) {
-		bool use_fec_hwts = !phy_has_hwtstamp(phydev);
-
-		if (cmd == SIOCSHWTSTAMP) {
-			if (use_fec_hwts)
-				return fec_ptp_set(ndev, rq);
-			fec_ptp_disable_hwts(ndev);
-		} else if (cmd == SIOCGHWTSTAMP) {
-			if (use_fec_hwts)
-				return fec_ptp_get(ndev, rq);
-		}
-	}
-
-	return phy_mii_ioctl(phydev, rq, cmd);
-}
-
 static void fec_enet_free_buffers(struct net_device *ndev)
 {
 	struct fec_enet_private *fep = netdev_priv(ndev);
@@ -3927,6 +3900,42 @@  static int fec_enet_xdp_xmit(struct net_device *dev,
 	return sent_frames;
 }
 
+static int fec_hwtstamp_get(struct net_device *ndev,
+			    struct kernel_hwtstamp_config *config)
+{
+	struct fec_enet_private *fep = netdev_priv(ndev);
+	struct phy_device *phydev = ndev->phydev;
+
+	if (phy_has_hwtstamp(phydev))
+		return phy_mii_ioctl(phydev, config->ifr, SIOCGHWTSTAMP);
+
+	if (!fep->bufdesc_ex)
+		return -EOPNOTSUPP;
+
+	fec_ptp_get(ndev, config);
+
+	return 0;
+}
+
+static int fec_hwtstamp_set(struct net_device *ndev,
+			    struct kernel_hwtstamp_config *config,
+			    struct netlink_ext_ack *extack)
+{
+	struct fec_enet_private *fep = netdev_priv(ndev);
+	struct phy_device *phydev = ndev->phydev;
+
+	if (phy_has_hwtstamp(phydev)) {
+		fec_ptp_disable_hwts(ndev);
+
+		return phy_mii_ioctl(phydev, config->ifr, SIOCSHWTSTAMP);
+	}
+
+	if (!fep->bufdesc_ex)
+		return -EOPNOTSUPP;
+
+	return fec_ptp_set(ndev, config, extack);
+}
+
 static const struct net_device_ops fec_netdev_ops = {
 	.ndo_open		= fec_enet_open,
 	.ndo_stop		= fec_enet_close,
@@ -3936,13 +3945,15 @@  static const struct net_device_ops fec_netdev_ops = {
 	.ndo_validate_addr	= eth_validate_addr,
 	.ndo_tx_timeout		= fec_timeout,
 	.ndo_set_mac_address	= fec_set_mac_address,
-	.ndo_eth_ioctl		= fec_enet_ioctl,
+	.ndo_eth_ioctl		= phy_do_ioctl_running,
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller	= fec_poll_controller,
 #endif
 	.ndo_set_features	= fec_set_features,
 	.ndo_bpf		= fec_enet_bpf,
 	.ndo_xdp_xmit		= fec_enet_xdp_xmit,
+	.ndo_hwtstamp_get	= fec_hwtstamp_get,
+	.ndo_hwtstamp_set	= fec_hwtstamp_set,
 };
 
 static const unsigned short offset_des_active_rxq[] = {
diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index afc658d2c271..50943db40f2d 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -618,16 +618,12 @@  void fec_ptp_disable_hwts(struct net_device *ndev)
 	fep->hwts_rx_en = 0;
 }
 
-int fec_ptp_set(struct net_device *ndev, struct ifreq *ifr)
+int fec_ptp_set(struct net_device *ndev, struct kernel_hwtstamp_config *config,
+		struct netlink_ext_ack *extack)
 {
 	struct fec_enet_private *fep = netdev_priv(ndev);
 
-	struct hwtstamp_config config;
-
-	if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
-		return -EFAULT;
-
-	switch (config.tx_type) {
+	switch (config->tx_type) {
 	case HWTSTAMP_TX_OFF:
 		fep->hwts_tx_en = 0;
 		break;
@@ -638,33 +634,28 @@  int fec_ptp_set(struct net_device *ndev, struct ifreq *ifr)
 		return -ERANGE;
 	}
 
-	switch (config.rx_filter) {
+	switch (config->rx_filter) {
 	case HWTSTAMP_FILTER_NONE:
 		fep->hwts_rx_en = 0;
 		break;
 
 	default:
 		fep->hwts_rx_en = 1;
-		config.rx_filter = HWTSTAMP_FILTER_ALL;
+		config->rx_filter = HWTSTAMP_FILTER_ALL;
 		break;
 	}
 
-	return copy_to_user(ifr->ifr_data, &config, sizeof(config)) ?
-	    -EFAULT : 0;
+	return 0;
 }
 
-int fec_ptp_get(struct net_device *ndev, struct ifreq *ifr)
+void fec_ptp_get(struct net_device *ndev, struct kernel_hwtstamp_config *config)
 {
 	struct fec_enet_private *fep = netdev_priv(ndev);
-	struct hwtstamp_config config;
-
-	config.flags = 0;
-	config.tx_type = fep->hwts_tx_en ? HWTSTAMP_TX_ON : HWTSTAMP_TX_OFF;
-	config.rx_filter = (fep->hwts_rx_en ?
-			    HWTSTAMP_FILTER_ALL : HWTSTAMP_FILTER_NONE);
 
-	return copy_to_user(ifr->ifr_data, &config, sizeof(config)) ?
-		-EFAULT : 0;
+	config->flags = 0;
+	config->tx_type = fep->hwts_tx_en ? HWTSTAMP_TX_ON : HWTSTAMP_TX_OFF;
+	config->rx_filter = (fep->hwts_rx_en ?
+			     HWTSTAMP_FILTER_ALL : HWTSTAMP_FILTER_NONE);
 }
 
 /*