On 5/13/2025 3:11 AM, Vladimir Oltean wrote:
> New timestamping API was introduced in commit 66f7223039c0 ("net: add
> NDOs for configuring hardware timestamping") from kernel v6.6.
>
> It is time to convert the Intel ixgbe driver to the new API, so that
> timestamping configuration can be removed from the ndo_eth_ioctl() path
> completely.
>
> Signed-off-by: Vladimir Oltean <[email protected]>
> ---
Ugh. Apologies for the late reply here, but this took for ever to track
down what was wrong in our testing.
The ixgbe patch has a somewhat subtle bug which lead to failed timestamp
configuration and likely other forms of memory corruption.
> drivers/net/ethernet/intel/ixgbe/ixgbe.h | 9 ++--
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 6 +--
> drivers/net/ethernet/intel/ixgbe/ixgbe_ptp.c | 42 +++++++++----------
> 3 files changed, 29 insertions(+), 28 deletions(-)
>
>
> /**
> - * ixgbe_ptp_get_ts_config - get current hardware timestamping configuration
> - * @adapter: pointer to adapter structure
> - * @ifr: ioctl data
> + * ixgbe_ptp_hwtstamp_get - get current hardware timestamping configuration
> + * @netdev: pointer to net device structure
> + * @config: timestamping configuration structure
> *
> * This function returns the current timestamping settings. Rather than
> * attempt to deconstruct registers to fill in the values, simply keep a copy
> * of the old settings around, and return a copy when requested.
> */
> -int ixgbe_ptp_get_ts_config(struct ixgbe_adapter *adapter, struct ifreq *ifr)
> +int ixgbe_ptp_hwtstamp_get(struct net_device *netdev,
> + struct kernel_hwtstamp_config *config)
> {
> - struct hwtstamp_config *config = &adapter->tstamp_config;
> + struct ixgbe_adapter *adapter = netdev_priv(netdev);
>
ixgbe doesn't directly assign the adapter to netdev_priv and this needs
to be ixgbe_from_netdev, since there is a wrapper ixgbe_netdev_priv
structure. I didn't dig into why, but both get and set are wrong here,
and are misinterpreting the ixgbe_netdev_priv structure as
ixgbe_adapter, which is obviously wrong.
See its definition quoted here:
> static inline struct ixgbe_adapter *ixgbe_from_netdev(struct net_device
> *netdev)
> {
> struct ixgbe_netdevice_priv *priv = netdev_priv(netdev);
>
> return priv->adapter;
> }
>
Whats odd is that the netdev priv structure is just a wrapper around a
pointer to the adapter:
> struct ixgbe_netdevice_priv {
> struct ixgbe_adapter *adapter;
> };
> - return copy_to_user(ifr->ifr_data, config,
> - sizeof(*config)) ? -EFAULT : 0;
> + *config = adapter->tstamp_config;
> +
> + return 0;
> }
Because we're completely pointing to the wrong memory, this overwrites
who knows what since the ixgbe_netdev_priv is just the pointer address.
>
> /**
> @@ -978,7 +980,7 @@ int ixgbe_ptp_get_ts_config(struct ixgbe_adapter
> *adapter, struct ifreq *ifr)
> * mode, if required to support the specifically requested mode.
> */
> static int ixgbe_ptp_set_timestamp_mode(struct ixgbe_adapter *adapter,
> - struct hwtstamp_config *config)
> + struct kernel_hwtstamp_config *config)
> {
> struct ixgbe_hw *hw = &adapter->hw;
> u32 tsync_tx_ctl = IXGBE_TSYNCTXCTL_ENABLED;
> @@ -1129,31 +1131,29 @@ static int ixgbe_ptp_set_timestamp_mode(struct
> ixgbe_adapter *adapter,
> }
>
> /**
> - * ixgbe_ptp_set_ts_config - user entry point for timestamp mode
> - * @adapter: pointer to adapter struct
> - * @ifr: ioctl data
> + * ixgbe_ptp_hwtstamp_set - user entry point for timestamp mode
> + * @netdev: pointer to net device structure
> + * @config: timestamping configuration structure
> + * @extack: netlink extended ack structure for error reporting
> *
> * Set hardware to requested mode. If unsupported, return an error with no
> * changes. Otherwise, store the mode for future reference.
> */
> -int ixgbe_ptp_set_ts_config(struct ixgbe_adapter *adapter, struct ifreq *ifr)
> +int ixgbe_ptp_hwtstamp_set(struct net_device *netdev,
> + struct kernel_hwtstamp_config *config,
> + struct netlink_ext_ack *extack)
> {
> - struct hwtstamp_config config;
> + struct ixgbe_adapter *adapter = netdev_priv(netdev);
> int err;
Same here, the current code would need ixgbe_from_netdev()
I think the right thing to do is submit a patch/fix which drops the
completely useless ixgbe_netdevice_priv structure, but I'll have to dig
into git history to see why it was ever there in the first place.
OpenPGP_signature.asc
Description: OpenPGP digital signature
