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.
OpenPGP_signature.asc
Description: OpenPGP digital signature
