netdev - Re: [PATCH net-next 3/4] mlx5: convert to ndo_hwtstamp_get() and ndo_hwtstamp

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <3a6ce0da-9f3f-4630-8c01-43ae980828d1@linux.dev>
Date: Tue, 23 Sep 2025 21:48:23 +0100
From: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
To: Carolina Jubran <cjubran@...dia.com>
Cc: netdev@...r.kernel.org, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>,
 Richard Cochran <richardcochran@...il.com>,
 Andrew Lunn <andrew+netdev@...n.ch>, Michael Chan
 <michael.chan@...adcom.com>, Pavan Chebbi <pavan.chebbi@...adcom.com>,
 Tariq Toukan <tariqt@...dia.com>, Saeed Mahameed <saeedm@...dia.com>,
 Mark Bloch <mbloch@...dia.com>
Subject: Re: [PATCH net-next 3/4] mlx5: convert to ndo_hwtstamp_get() and
 ndo_hwtstamp_set()

On 23/09/2025 18:44, Carolina Jubran wrote:
> 
> On 22/09/2025 19:51, Vadim Fedorenko wrote:
> Hi Vadim, thanks for the patch!
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/ 
>> drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> index 5e007bb3bad1..74a63371ab69 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
>> @@ -4755,9 +4755,11 @@ static int mlx5e_hwstamp_config_ptp_rx(struct 
>> mlx5e_priv *priv, bool ptp_rx)
>>                       &new_params.ptp_rx, true);
>>   }
>> -int mlx5e_hwstamp_set(struct mlx5e_priv *priv, struct ifreq *ifr)
>> +int mlx5e_hwstamp_set(struct net_device *dev,
>> +              struct kernel_hwtstamp_config *config,
>> +              struct netlink_ext_ack *extack)
>>   {
>> -    struct hwtstamp_config config;
>> +    struct mlx5e_priv *priv = netdev_priv(dev);
>>       bool rx_cqe_compress_def;
>>       bool ptp_rx;
>>       int err;
>> @@ -4766,11 +4768,8 @@ int mlx5e_hwstamp_set(struct mlx5e_priv *priv, 
>> struct ifreq *ifr)
>>           (mlx5_clock_get_ptp_index(priv->mdev) == -1))
> 
> 
> I would add an |extack| message here.

Yeah, but the !MLX5_CAP_GEN(priv->mdev, device_frequency_khz) check
looks redundant as mdev->clock->ptp will be null in case of absent of
device_frequency_khz, according to mlx5_init_clock()

> 
>> @@ -4814,47 +4813,34 @@ int mlx5e_hwstamp_set(struct mlx5e_priv *priv, 
>> struct ifreq *ifr)
>>       if (!mlx5e_profile_feature_cap(priv->profile, PTP_RX))
>>           err = mlx5e_hwstamp_config_no_ptp_rx(priv,
>> -                             config.rx_filter != HWTSTAMP_FILTER_NONE);
>> +                             config->rx_filter != HWTSTAMP_FILTER_NONE);
>>       else
>>           err = mlx5e_hwstamp_config_ptp_rx(priv, ptp_rx);
>>       if (err)
>>           goto err_unlock;
>> -    memcpy(&priv->tstamp, &config, sizeof(config));
>> +    memcpy(&priv->tstamp, config, sizeof(*config));
> 
> 
> A direct assignment would be cleaner.

Just wanted to follow original style. I'll change it in the next ver

> 
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/ipoib/ipoib.c b/ 
>> drivers/net/ethernet/mellanox/mlx5/core/ipoib/ipoib.c
>> index 79ae3a51a4b3..ff8ffd997b17 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/ipoib/ipoib.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/ipoib/ipoib.c
>> @@ -52,7 +52,8 @@ static const struct net_device_ops mlx5i_netdev_ops = {
>>       .ndo_init                = mlx5i_dev_init,
>>       .ndo_uninit              = mlx5i_dev_cleanup,
>>       .ndo_change_mtu          = mlx5i_change_mtu,
>> -    .ndo_eth_ioctl            = mlx5i_ioctl,
>> +    .ndo_hwtstamp_get        = mlx5e_hwstamp_get,
>> +    .ndo_hwtstamp_set        = mlx5e_hwstamp_set,
>>   };
>>   /* IPoIB mlx5 netdev profile */
>> @@ -557,20 +558,6 @@ int mlx5i_dev_init(struct net_device *dev)
>>       return 0;
>>   }
>> -int mlx5i_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
>> -{
>> -    struct mlx5e_priv *priv = mlx5i_epriv(dev);
>> -
> 
> 
> mlx5i_epriv should still be used here. on IPoIB netdev_priv gives you a
> struct mlx5i_priv .

Oh, I see... we have to have slightly different hwstamp functions for
mlx5i and mlx5e because of different netdev->priv type. Let me see how
it can be factorized.