On Mon, Jun 30, 2025 at 01:47:07PM -0700, Jacob Keller wrote:
> On 6/30/2025 11:56 AM, Jacob Keller wrote:
> > 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.
> >
> This is an artifact of the work to refactor ixgbe to support devlink:
>
> Both netdev and devlink want a private structure allocated as a flexible
> array member of their parent structure. They cannot both directly be
> ice_adapter, so we chose to have devlink be ice_adapter, and netdev gets
> the wrapper structure. I suspect the patches you wrote were based on a
> tree before this refactor, and/or you just did not spot the refactor
> happened.
>
> a0285236ab93 ("ixgbe: add initial devlink support") is where the change
> took place, which merged relatively recently.
>
> @Tony, I think this is a pretty trivial fixup on your tree if you want
> to handle it instead of forcing Vladimir to make a v2?
>
> its really just switching netdev_priv to ixgbe_from_netdev in these two
> functions.
Ugh :-/ sorry for the trouble, and thanks for doing the hard work of
characterizing this.
Indeed, my first conversion of ixgbe was in August 2023, as this commit
can attest:
https://github.com/vladimiroltean/linux/commit/0351ebf1eee3381ccfba9d31a924d1dd887a316f
At that time, Przemyslaw's commit fd5ef5203ce6 ("ixgbe: wrap
netdev_priv() usage") didn't exist, and "struct ixgbe_adapter *adapter =
netdev_priv(netdev);" was the de facto idiom in the driver, which I then
replicated two more times, in the new ixgbe_ptp_hwtstamp_set() and
ixgbe_ptp_hwtstamp_get() functions. Not only did I not notice that this
change took place, but it also compiled just fine, making me completely
unsuspecting...
Tony, let me know how you would like to proceed.
