[PATCH net-next 0/2] sfc: Implement ndo_hwtstamp_(get|set)

[PATCH net-next 0/2] sfc: Implement ndo_hwtstamp_(get|set)

[Alex Austin]

Implement ndo_hwtstamp_get and ndo_hwtstamp_set for sfc and sfc-siena.

Alex Austin (2):
  sfc: Implement ndo_hwtstamp_(get|set)
  sfc-siena: Implement ndo_hwtstamp_(get|set)

 drivers/net/ethernet/sfc/ef10.c             |  4 +--
 drivers/net/ethernet/sfc/efx.c              | 24 +++++++++++++----
 drivers/net/ethernet/sfc/net_driver.h       |  2 +-
 drivers/net/ethernet/sfc/ptp.c              | 30 ++++++++-------------
 drivers/net/ethernet/sfc/ptp.h              |  7 +++--
 drivers/net/ethernet/sfc/siena/efx.c        | 24 +++++++++++++----
 drivers/net/ethernet/sfc/siena/net_driver.h |  2 +-
 drivers/net/ethernet/sfc/siena/ptp.c        | 30 +++++++++------------
 drivers/net/ethernet/sfc/siena/ptp.h        |  7 +++--
 drivers/net/ethernet/sfc/siena/siena.c      |  2 +-
 10 files changed, 76 insertions(+), 56 deletions(-)

-- 
2.39.3

[PATCH net-next 1/2] sfc: Implement ndo_hwtstamp_(get|set)

[Alex Austin]

Update efx->ptp_data to use kernel_hwtstamp_config and implement
ndo_hwtstamp_(get|set). Remove SIOCGHWTSTAMP and SIOCSHWTSTAMP from
efx_ioctl.

Signed-off-by: Alex Austin <alex.austin@amd.com>
Acked-by: Martin Habets <habetsm.xilinx@gmail.com>
---
 drivers/net/ethernet/sfc/ef10.c       |  4 ++--
 drivers/net/ethernet/sfc/efx.c        | 24 ++++++++++++++++-----
 drivers/net/ethernet/sfc/net_driver.h |  2 +-
 drivers/net/ethernet/sfc/ptp.c        | 30 ++++++++++-----------------
 drivers/net/ethernet/sfc/ptp.h        |  7 +++++--
 5 files changed, 38 insertions(+), 29 deletions(-)

diff --git a/drivers/net/ethernet/sfc/ef10.c b/drivers/net/ethernet/sfc/ef10.c
index 6dfa062feebc..8fa6c0e9195b 100644
--- a/drivers/net/ethernet/sfc/ef10.c
+++ b/drivers/net/ethernet/sfc/ef10.c
@@ -3706,13 +3706,13 @@ static int efx_ef10_ptp_set_ts_sync_events(struct efx_nic *efx, bool en,
 }
 
 static int efx_ef10_ptp_set_ts_config_vf(struct efx_nic *efx,
-					 struct hwtstamp_config *init)
+					 struct kernel_hwtstamp_config *init)
 {
 	return -EOPNOTSUPP;
 }
 
 static int efx_ef10_ptp_set_ts_config(struct efx_nic *efx,
-				      struct hwtstamp_config *init)
+				      struct kernel_hwtstamp_config *init)
 {
 	int rc;
 
diff --git a/drivers/net/ethernet/sfc/efx.c b/drivers/net/ethernet/sfc/efx.c
index 19f4b4d0b851..e9d9de8e648a 100644
--- a/drivers/net/ethernet/sfc/efx.c
+++ b/drivers/net/ethernet/sfc/efx.c
@@ -495,11 +495,6 @@ static int efx_ioctl(struct net_device *net_dev, struct ifreq *ifr, int cmd)
 	struct efx_nic *efx = efx_netdev_priv(net_dev);
 	struct mii_ioctl_data *data = if_mii(ifr);
 
-	if (cmd == SIOCSHWTSTAMP)
-		return efx_ptp_set_ts_config(efx, ifr);
-	if (cmd == SIOCGHWTSTAMP)
-		return efx_ptp_get_ts_config(efx, ifr);
-
 	/* Convert phy_id from older PRTAD/DEVAD format */
 	if ((cmd == SIOCGMIIREG || cmd == SIOCSMIIREG) &&
 	    (data->phy_id & 0xfc00) == 0x0400)
@@ -581,6 +576,23 @@ static int efx_vlan_rx_kill_vid(struct net_device *net_dev, __be16 proto, u16 vi
 		return -EOPNOTSUPP;
 }
 
+static int efx_hwtstamp_set(struct net_device *net_dev,
+			    struct kernel_hwtstamp_config *config,
+			    struct netlink_ext_ack *extack)
+{
+	struct efx_nic *efx = efx_netdev_priv(net_dev);
+
+	return efx_ptp_set_ts_config(efx, config, extack);
+}
+
+static int efx_hwtstamp_get(struct net_device *net_dev,
+			    struct kernel_hwtstamp_config *config)
+{
+	struct efx_nic *efx = efx_netdev_priv(net_dev);
+
+	return efx_ptp_get_ts_config(efx, config);
+}
+
 static const struct net_device_ops efx_netdev_ops = {
 	.ndo_open		= efx_net_open,
 	.ndo_stop		= efx_net_stop,
@@ -596,6 +608,8 @@ static const struct net_device_ops efx_netdev_ops = {
 	.ndo_features_check	= efx_features_check,
 	.ndo_vlan_rx_add_vid	= efx_vlan_rx_add_vid,
 	.ndo_vlan_rx_kill_vid	= efx_vlan_rx_kill_vid,
+	.ndo_hwtstamp_set	= efx_hwtstamp_set,
+	.ndo_hwtstamp_get	= efx_hwtstamp_get,
 #ifdef CONFIG_SFC_SRIOV
 	.ndo_set_vf_mac		= efx_sriov_set_vf_mac,
 	.ndo_set_vf_vlan	= efx_sriov_set_vf_vlan,
diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h
index 27d86e90a3bb..f2dd7feb0e0c 100644
--- a/drivers/net/ethernet/sfc/net_driver.h
+++ b/drivers/net/ethernet/sfc/net_driver.h
@@ -1473,7 +1473,7 @@ struct efx_nic_type {
 	void (*ptp_write_host_time)(struct efx_nic *efx, u32 host_time);
 	int (*ptp_set_ts_sync_events)(struct efx_nic *efx, bool en, bool temp);
 	int (*ptp_set_ts_config)(struct efx_nic *efx,
-				 struct hwtstamp_config *init);
+				 struct kernel_hwtstamp_config *init);
 	int (*sriov_configure)(struct efx_nic *efx, int num_vfs);
 	int (*vlan_rx_add_vid)(struct efx_nic *efx, __be16 proto, u16 vid);
 	int (*vlan_rx_kill_vid)(struct efx_nic *efx, __be16 proto, u16 vid);
diff --git a/drivers/net/ethernet/sfc/ptp.c b/drivers/net/ethernet/sfc/ptp.c
index b04fdbb8aece..c3bffbf0ba2b 100644
--- a/drivers/net/ethernet/sfc/ptp.c
+++ b/drivers/net/ethernet/sfc/ptp.c
@@ -301,7 +301,7 @@ struct efx_ptp_data {
 	bool reset_required;
 	struct list_head rxfilters_mcast;
 	struct list_head rxfilters_ucast;
-	struct hwtstamp_config config;
+	struct kernel_hwtstamp_config config;
 	bool enabled;
 	unsigned int mode;
 	void (*ns_to_nic_time)(s64 ns, u32 *nic_major, u32 *nic_minor);
@@ -1848,7 +1848,7 @@ int efx_ptp_change_mode(struct efx_nic *efx, bool enable_wanted,
 	return 0;
 }
 
-static int efx_ptp_ts_init(struct efx_nic *efx, struct hwtstamp_config *init)
+static int efx_ptp_ts_init(struct efx_nic *efx, struct kernel_hwtstamp_config *init)
 {
 	int rc;
 
@@ -1895,33 +1895,25 @@ void efx_ptp_get_ts_info(struct efx_nic *efx, struct ethtool_ts_info *ts_info)
 	ts_info->rx_filters = ptp->efx->type->hwtstamp_filters;
 }
 
-int efx_ptp_set_ts_config(struct efx_nic *efx, struct ifreq *ifr)
+int efx_ptp_set_ts_config(struct efx_nic *efx,
+			  struct kernel_hwtstamp_config *config,
+			  struct netlink_ext_ack __always_unused *extack)
 {
-	struct hwtstamp_config config;
-	int rc;
-
 	/* Not a PTP enabled port */
 	if (!efx->ptp_data)
 		return -EOPNOTSUPP;
 
-	if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
-		return -EFAULT;
-
-	rc = efx_ptp_ts_init(efx, &config);
-	if (rc != 0)
-		return rc;
-
-	return copy_to_user(ifr->ifr_data, &config, sizeof(config))
-		? -EFAULT : 0;
+	return efx_ptp_ts_init(efx, config);
 }
 
-int efx_ptp_get_ts_config(struct efx_nic *efx, struct ifreq *ifr)
+int efx_ptp_get_ts_config(struct efx_nic *efx,
+			  struct kernel_hwtstamp_config *config)
 {
+	/* Not a PTP enabled port */
 	if (!efx->ptp_data)
 		return -EOPNOTSUPP;
-
-	return copy_to_user(ifr->ifr_data, &efx->ptp_data->config,
-			    sizeof(efx->ptp_data->config)) ? -EFAULT : 0;
+	*config = efx->ptp_data->config;
+	return 0;
 }
 
 static void ptp_event_failure(struct efx_nic *efx, int expected_frag_len)
diff --git a/drivers/net/ethernet/sfc/ptp.h b/drivers/net/ethernet/sfc/ptp.h
index 7b1ef7002b3f..2f30dbb490d2 100644
--- a/drivers/net/ethernet/sfc/ptp.h
+++ b/drivers/net/ethernet/sfc/ptp.h
@@ -18,8 +18,11 @@ void efx_ptp_defer_probe_with_channel(struct efx_nic *efx);
 struct efx_channel *efx_ptp_channel(struct efx_nic *efx);
 void efx_ptp_update_channel(struct efx_nic *efx, struct efx_channel *channel);
 void efx_ptp_remove(struct efx_nic *efx);
-int efx_ptp_set_ts_config(struct efx_nic *efx, struct ifreq *ifr);
-int efx_ptp_get_ts_config(struct efx_nic *efx, struct ifreq *ifr);
+int efx_ptp_set_ts_config(struct efx_nic *efx,
+			  struct kernel_hwtstamp_config *config,
+			  struct netlink_ext_ack *extack);
+int efx_ptp_get_ts_config(struct efx_nic *efx,
+			  struct kernel_hwtstamp_config *config);
 void efx_ptp_get_ts_info(struct efx_nic *efx, struct ethtool_ts_info *ts_info);
 bool efx_ptp_is_ptp_tx(struct efx_nic *efx, struct sk_buff *skb);
 int efx_ptp_get_mode(struct efx_nic *efx);
-- 
2.39.3

Re: [PATCH net-next 1/2] sfc: Implement ndo_hwtstamp_(get|set)

[Edward Cree]

On 30/11/2023 13:58, Alex Austin wrote:
> Update efx->ptp_data to use kernel_hwtstamp_config and implement
> ndo_hwtstamp_(get|set). Remove SIOCGHWTSTAMP and SIOCSHWTSTAMP from
> efx_ioctl.
> 
> Signed-off-by: Alex Austin <alex.austin@amd.com>
> Acked-by: Martin Habets <habetsm.xilinx@gmail.com>

Reviewed-by: Edward Cree <ecree.xilinx@gmail.com>

Re: [PATCH net-next 1/2] sfc: Implement ndo_hwtstamp_(get|set)

[Jakub Kicinski]

On Thu, 30 Nov 2023 13:58:25 +0000 Alex Austin wrote:
> -	struct hwtstamp_config config;
> +	struct kernel_hwtstamp_config config;

> +	*config = efx->ptp_data->config;

Do we have a lot of places which assign the new structure directly 
like this?

There's a bit of "request state" in it:

struct kernel_hwtstamp_config {
	int flags;
	int tx_type;
	int rx_filter;
	struct ifreq *ifr;             <<<
	bool copied_to_user;           <<<
	enum hwtstamp_source source;
};

Maybe keep the type of config as was, and use
hwtstamp_config_to_kernel() to set the right fields?

Re: [PATCH net-next 1/2] sfc: Implement ndo_hwtstamp_(get|set)

[Austin, Alex (DCCG)]

This seems like a good approach. I'll re-work into a v2.

Alex

On 02/12/2023 03:25, Jakub Kicinski wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Thu, 30 Nov 2023 13:58:25 +0000 Alex Austin wrote:
>> -     struct hwtstamp_config config;
>> +     struct kernel_hwtstamp_config config;
>> +     *config = efx->ptp_data->config;
> Do we have a lot of places which assign the new structure directly
> like this?
>
> There's a bit of "request state" in it:
>
> struct kernel_hwtstamp_config {
>          int flags;
>          int tx_type;
>          int rx_filter;
>          struct ifreq *ifr;             <<<
>          bool copied_to_user;           <<<
>          enum hwtstamp_source source;
> };
>
> Maybe keep the type of config as was, and use
> hwtstamp_config_to_kernel() to set the right fields?

Re: [PATCH net-next 1/2] sfc: Implement ndo_hwtstamp_(get|set)

[Vladimir Oltean]

On Mon, Dec 04, 2023 at 10:26:30AM +0000, Austin, Alex (DCCG) wrote:
> This seems like a good approach. I'll re-work into a v2.
> 
> Alex
> 
> On 02/12/2023 03:25, Jakub Kicinski wrote:
> > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> > 
> > 
> > On Thu, 30 Nov 2023 13:58:25 +0000 Alex Austin wrote:
> > > -     struct hwtstamp_config config;
> > > +     struct kernel_hwtstamp_config config;
> > > +     *config = efx->ptp_data->config;
> > Do we have a lot of places which assign the new structure directly
> > like this?
> > 
> > There's a bit of "request state" in it:
> > 
> > struct kernel_hwtstamp_config {
> >          int flags;
> >          int tx_type;
> >          int rx_filter;
> >          struct ifreq *ifr;             <<<
> >          bool copied_to_user;           <<<
> >          enum hwtstamp_source source;
> > };
> > 
> > Maybe keep the type of config as was, and use
> > hwtstamp_config_to_kernel() to set the right fields?
> 

If I may intervene. The "request state" will ultimately go away once all
drivers are converted. I know it's more fragile and not all fields are
valid, but I think I would like drivers to store the kernel_ variant of
the structure, because more stuff will be added to the kernel_ variant
in the future (the hwtstamp provider + qualifier), and doing this from
the beginning will avoid reworking them again.

Re: [PATCH net-next 1/2] sfc: Implement ndo_hwtstamp_(get|set)

[Jakub Kicinski]

On Mon, 4 Dec 2023 13:00:35 +0200 Vladimir Oltean wrote:
> If I may intervene. The "request state" will ultimately go away once all
> drivers are converted. I know it's more fragile and not all fields are
> valid, but I think I would like drivers to store the kernel_ variant of
> the structure, because more stuff will be added to the kernel_ variant
> in the future (the hwtstamp provider + qualifier), and doing this from
> the beginning will avoid reworking them again.

Okay, you know the direction of this work better, so:

pw-bot: under-review

Report-bugs-to: Vladimir Oltean <olteanv@gmail.com>

:P

Re: [PATCH net-next 1/2] sfc: Implement ndo_hwtstamp_(get|set)

[Vladimir Oltean]

On Mon, Dec 04, 2023 at 10:17:05AM -0800, Jakub Kicinski wrote:
> On Mon, 4 Dec 2023 13:00:35 +0200 Vladimir Oltean wrote:
> > If I may intervene. The "request state" will ultimately go away once all
> > drivers are converted. I know it's more fragile and not all fields are
> > valid, but I think I would like drivers to store the kernel_ variant of
> > the structure, because more stuff will be added to the kernel_ variant
> > in the future (the hwtstamp provider + qualifier), and doing this from
> > the beginning will avoid reworking them again.
> 
> Okay, you know the direction of this work better, so:
> 
> pw-bot: under-review

I mean your observation is in principle fair. If drivers save the struct
kernel_hwtstamp_config in the set() method and give it back in the get()
method (this is very widespread BTW), it's reasonable to question what
happens with the temporary fields, ifr and copied_to_user. Won't we
corrupt the teporary fields of the kernel_hwtstamp_config structure from
the set() with the previous ones from the get()?

The answer, I think, is that we do, but in a safe way. Because we implement
ndo_hwtstamp_set(), the copied_to_user that we save is false (aka "the
driver implementation didn't call copy_to_user()"). And when we give
this structure back in ndo_hwtstamp_get(), we overwrite false with false,
and a good ifr pointer with a bad one.

But the only reason we transport the ifr along with the
kernel_hwtstamp_config is for generic_hwtstamp_ioctl_lower() to work,
aka a new API upper driver on top of an old API real driver. Which is
not the case here, and no one looks at the stale ifr pointer.

It's a lot to think about to make sure that something bad won't happen,
I agree. I still don't believe it will break in subtle ways, but nonetheless
I do recognize the tradeoff. One approach is more straightforward
code-wise but more subtle behavior-wise, and the other is the opposite.

> 
> Report-bugs-to: Vladimir Oltean <olteanv@gmail.com>
> 
> :P

Hmm, I'm not sure if I want to go that far. Alex is free to choose
whichever implementation he sees fit, and so, he is also responsible
for the end result, in spite of any review feedback received. Please
don't consider my message as anything more than a suggestion.

Re: [PATCH net-next 1/2] sfc: Implement ndo_hwtstamp_(get|set)

[Paolo Abeni]

On Mon, 2023-12-04 at 20:45 +0200, Vladimir Oltean wrote:
> On Mon, Dec 04, 2023 at 10:17:05AM -0800, Jakub Kicinski wrote:
> > On Mon, 4 Dec 2023 13:00:35 +0200 Vladimir Oltean wrote:
> > > If I may intervene. The "request state" will ultimately go away once all
> > > drivers are converted. I know it's more fragile and not all fields are
> > > valid, but I think I would like drivers to store the kernel_ variant of
> > > the structure, because more stuff will be added to the kernel_ variant
> > > in the future (the hwtstamp provider + qualifier), and doing this from
> > > the beginning will avoid reworking them again.
> > 
> > Okay, you know the direction of this work better, so:
> > 
> > pw-bot: under-review
> 
> I mean your observation is in principle fair. If drivers save the struct
> kernel_hwtstamp_config in the set() method and give it back in the get()
> method (this is very widespread BTW), it's reasonable to question what
> happens with the temporary fields, ifr and copied_to_user. Won't we
> corrupt the teporary fields of the kernel_hwtstamp_config structure from
> the set() with the previous ones from the get()?
> 
> The answer, I think, is that we do, but in a safe way. Because we implement
> ndo_hwtstamp_set(), the copied_to_user that we save is false (aka "the
> driver implementation didn't call copy_to_user()"). And when we give
> this structure back in ndo_hwtstamp_get(), we overwrite false with false,
> and a good ifr pointer with a bad one.
> 
> But the only reason we transport the ifr along with the
> kernel_hwtstamp_config is for generic_hwtstamp_ioctl_lower() to work,
> aka a new API upper driver on top of an old API real driver. Which is
> not the case here, and no one looks at the stale ifr pointer.
> 
> It's a lot to think about to make sure that something bad won't happen,
> I agree. I still don't believe it will break in subtle ways, but nonetheless
> I do recognize the tradeoff. One approach is more straightforward
> code-wise but more subtle behavior-wise, and the other is the opposite.

I tried to dig into the relevant code as far as I can, and I tend to
agree with Vladimir: the current approach looks reasonably safe, and
forward looking. 

I think any eventual bugs (I could not find any) would be pre-existent
to this patch, rooted in dev_ioctl.c and to be addressed there.

I think this patches should go in the current form.

Cheers,

Paolo

Re: [PATCH net-next 1/2] sfc: Implement ndo_hwtstamp_(get|set)

[Austin, Alex (DCCG)]

Based on comments above, my preference is to keep these patches as they are.

Thanks,

Alex

On 05/12/2023 08:52, Paolo Abeni wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Mon, 2023-12-04 at 20:45 +0200, Vladimir Oltean wrote:
>> On Mon, Dec 04, 2023 at 10:17:05AM -0800, Jakub Kicinski wrote:
>>> On Mon, 4 Dec 2023 13:00:35 +0200 Vladimir Oltean wrote:
>>>> If I may intervene. The "request state" will ultimately go away once all
>>>> drivers are converted. I know it's more fragile and not all fields are
>>>> valid, but I think I would like drivers to store the kernel_ variant of
>>>> the structure, because more stuff will be added to the kernel_ variant
>>>> in the future (the hwtstamp provider + qualifier), and doing this from
>>>> the beginning will avoid reworking them again.
>>> Okay, you know the direction of this work better, so:
>>>
>>> pw-bot: under-review
>> I mean your observation is in principle fair. If drivers save the struct
>> kernel_hwtstamp_config in the set() method and give it back in the get()
>> method (this is very widespread BTW), it's reasonable to question what
>> happens with the temporary fields, ifr and copied_to_user. Won't we
>> corrupt the teporary fields of the kernel_hwtstamp_config structure from
>> the set() with the previous ones from the get()?
>>
>> The answer, I think, is that we do, but in a safe way. Because we implement
>> ndo_hwtstamp_set(), the copied_to_user that we save is false (aka "the
>> driver implementation didn't call copy_to_user()"). And when we give
>> this structure back in ndo_hwtstamp_get(), we overwrite false with false,
>> and a good ifr pointer with a bad one.
>>
>> But the only reason we transport the ifr along with the
>> kernel_hwtstamp_config is for generic_hwtstamp_ioctl_lower() to work,
>> aka a new API upper driver on top of an old API real driver. Which is
>> not the case here, and no one looks at the stale ifr pointer.
>>
>> It's a lot to think about to make sure that something bad won't happen,
>> I agree. I still don't believe it will break in subtle ways, but nonetheless
>> I do recognize the tradeoff. One approach is more straightforward
>> code-wise but more subtle behavior-wise, and the other is the opposite.
> I tried to dig into the relevant code as far as I can, and I tend to
> agree with Vladimir: the current approach looks reasonably safe, and
> forward looking.
>
> I think any eventual bugs (I could not find any) would be pre-existent
> to this patch, rooted in dev_ioctl.c and to be addressed there.
>
> I think this patches should go in the current form.
>
> Cheers,
>
> Paolo
>

Re: [PATCH net-next 1/2] sfc: Implement ndo_hwtstamp_(get|set)

[Vladimir Oltean]

On Thu, Nov 30, 2023 at 01:58:25PM +0000, Alex Austin wrote:
> Update efx->ptp_data to use kernel_hwtstamp_config and implement
> ndo_hwtstamp_(get|set). Remove SIOCGHWTSTAMP and SIOCSHWTSTAMP from
> efx_ioctl.
> 
> Signed-off-by: Alex Austin <alex.austin@amd.com>
> Acked-by: Martin Habets <habetsm.xilinx@gmail.com>
> ---

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

[PATCH net-next 2/2] sfc-siena: Implement ndo_hwtstamp_(get|set)

[Alex Austin]

Update efx->ptp_data to use kernel_hwtstamp_config and implement
ndo_hwtstamp_(get|set). Remove SIOCGHWTSTAMP and SIOCSHWTSTAMP from
efx_ioctl.

Signed-off-by: Alex Austin <alex.austin@amd.com>
Acked-by: Martin Habets <habetsm.xilinx@gmail.com>
---
 drivers/net/ethernet/sfc/siena/efx.c        | 24 +++++++++++++----
 drivers/net/ethernet/sfc/siena/net_driver.h |  2 +-
 drivers/net/ethernet/sfc/siena/ptp.c        | 30 +++++++++------------
 drivers/net/ethernet/sfc/siena/ptp.h        |  7 +++--
 drivers/net/ethernet/sfc/siena/siena.c      |  2 +-
 5 files changed, 38 insertions(+), 27 deletions(-)

diff --git a/drivers/net/ethernet/sfc/siena/efx.c b/drivers/net/ethernet/sfc/siena/efx.c
index 8c557f6a183c..59d3a6043379 100644
--- a/drivers/net/ethernet/sfc/siena/efx.c
+++ b/drivers/net/ethernet/sfc/siena/efx.c
@@ -495,11 +495,6 @@ static int efx_ioctl(struct net_device *net_dev, struct ifreq *ifr, int cmd)
 	struct efx_nic *efx = netdev_priv(net_dev);
 	struct mii_ioctl_data *data = if_mii(ifr);
 
-	if (cmd == SIOCSHWTSTAMP)
-		return efx_siena_ptp_set_ts_config(efx, ifr);
-	if (cmd == SIOCGHWTSTAMP)
-		return efx_siena_ptp_get_ts_config(efx, ifr);
-
 	/* Convert phy_id from older PRTAD/DEVAD format */
 	if ((cmd == SIOCGMIIREG || cmd == SIOCSMIIREG) &&
 	    (data->phy_id & 0xfc00) == 0x0400)
@@ -579,6 +574,23 @@ static int efx_vlan_rx_kill_vid(struct net_device *net_dev, __be16 proto, u16 vi
 		return -EOPNOTSUPP;
 }
 
+static int efx_siena_hwtstamp_set(struct net_device *net_dev,
+				  struct kernel_hwtstamp_config *config,
+				  struct netlink_ext_ack *extack)
+{
+	struct efx_nic *efx = netdev_priv(net_dev);
+
+	return efx_siena_ptp_set_ts_config(efx, config, extack);
+}
+
+static int efx_siena_hwtstamp_get(struct net_device *net_dev,
+				  struct kernel_hwtstamp_config *config)
+{
+	struct efx_nic *efx = netdev_priv(net_dev);
+
+	return efx_siena_ptp_get_ts_config(efx, config);
+}
+
 static const struct net_device_ops efx_netdev_ops = {
 	.ndo_open		= efx_net_open,
 	.ndo_stop		= efx_net_stop,
@@ -594,6 +606,8 @@ static const struct net_device_ops efx_netdev_ops = {
 	.ndo_features_check	= efx_siena_features_check,
 	.ndo_vlan_rx_add_vid	= efx_vlan_rx_add_vid,
 	.ndo_vlan_rx_kill_vid	= efx_vlan_rx_kill_vid,
+	.ndo_hwtstamp_set	= efx_siena_hwtstamp_set,
+	.ndo_hwtstamp_get	= efx_siena_hwtstamp_get,
 #ifdef CONFIG_SFC_SIENA_SRIOV
 	.ndo_set_vf_mac		= efx_sriov_set_vf_mac,
 	.ndo_set_vf_vlan	= efx_sriov_set_vf_vlan,
diff --git a/drivers/net/ethernet/sfc/siena/net_driver.h b/drivers/net/ethernet/sfc/siena/net_driver.h
index ff7bbc325952..94152f595acd 100644
--- a/drivers/net/ethernet/sfc/siena/net_driver.h
+++ b/drivers/net/ethernet/sfc/siena/net_driver.h
@@ -1424,7 +1424,7 @@ struct efx_nic_type {
 	void (*ptp_write_host_time)(struct efx_nic *efx, u32 host_time);
 	int (*ptp_set_ts_sync_events)(struct efx_nic *efx, bool en, bool temp);
 	int (*ptp_set_ts_config)(struct efx_nic *efx,
-				 struct hwtstamp_config *init);
+				 struct kernel_hwtstamp_config *init);
 	int (*sriov_configure)(struct efx_nic *efx, int num_vfs);
 	int (*vlan_rx_add_vid)(struct efx_nic *efx, __be16 proto, u16 vid);
 	int (*vlan_rx_kill_vid)(struct efx_nic *efx, __be16 proto, u16 vid);
diff --git a/drivers/net/ethernet/sfc/siena/ptp.c b/drivers/net/ethernet/sfc/siena/ptp.c
index 38e666561bcd..4b5e2f0ba350 100644
--- a/drivers/net/ethernet/sfc/siena/ptp.c
+++ b/drivers/net/ethernet/sfc/siena/ptp.c
@@ -297,7 +297,7 @@ struct efx_ptp_data {
 	u32 rxfilter_event;
 	u32 rxfilter_general;
 	bool rxfilter_installed;
-	struct hwtstamp_config config;
+	struct kernel_hwtstamp_config config;
 	bool enabled;
 	unsigned int mode;
 	void (*ns_to_nic_time)(s64 ns, u32 *nic_major, u32 *nic_minor);
@@ -1762,7 +1762,8 @@ int efx_siena_ptp_change_mode(struct efx_nic *efx, bool enable_wanted,
 	return 0;
 }
 
-static int efx_ptp_ts_init(struct efx_nic *efx, struct hwtstamp_config *init)
+static int efx_ptp_ts_init(struct efx_nic *efx,
+			   struct kernel_hwtstamp_config *init)
 {
 	int rc;
 
@@ -1799,33 +1800,26 @@ void efx_siena_ptp_get_ts_info(struct efx_nic *efx,
 	ts_info->rx_filters = ptp->efx->type->hwtstamp_filters;
 }
 
-int efx_siena_ptp_set_ts_config(struct efx_nic *efx, struct ifreq *ifr)
+int efx_siena_ptp_set_ts_config(struct efx_nic *efx,
+				struct kernel_hwtstamp_config *config,
+				struct netlink_ext_ack __always_unused *extack)
 {
-	struct hwtstamp_config config;
-	int rc;
-
 	/* Not a PTP enabled port */
 	if (!efx->ptp_data)
 		return -EOPNOTSUPP;
 
-	if (copy_from_user(&config, ifr->ifr_data, sizeof(config)))
-		return -EFAULT;
-
-	rc = efx_ptp_ts_init(efx, &config);
-	if (rc != 0)
-		return rc;
-
-	return copy_to_user(ifr->ifr_data, &config, sizeof(config))
-		? -EFAULT : 0;
+	return efx_ptp_ts_init(efx, config);
 }
 
-int efx_siena_ptp_get_ts_config(struct efx_nic *efx, struct ifreq *ifr)
+int efx_siena_ptp_get_ts_config(struct efx_nic *efx,
+				struct kernel_hwtstamp_config *config)
 {
+	/* Not a PTP enabled port */
 	if (!efx->ptp_data)
 		return -EOPNOTSUPP;
 
-	return copy_to_user(ifr->ifr_data, &efx->ptp_data->config,
-			    sizeof(efx->ptp_data->config)) ? -EFAULT : 0;
+	*config = efx->ptp_data->config;
+	return 0;
 }
 
 static void ptp_event_failure(struct efx_nic *efx, int expected_frag_len)
diff --git a/drivers/net/ethernet/sfc/siena/ptp.h b/drivers/net/ethernet/sfc/siena/ptp.h
index 4172f90e9f6f..6352f84424f6 100644
--- a/drivers/net/ethernet/sfc/siena/ptp.h
+++ b/drivers/net/ethernet/sfc/siena/ptp.h
@@ -15,8 +15,11 @@
 struct ethtool_ts_info;
 void efx_siena_ptp_defer_probe_with_channel(struct efx_nic *efx);
 struct efx_channel *efx_siena_ptp_channel(struct efx_nic *efx);
-int efx_siena_ptp_set_ts_config(struct efx_nic *efx, struct ifreq *ifr);
-int efx_siena_ptp_get_ts_config(struct efx_nic *efx, struct ifreq *ifr);
+int efx_siena_ptp_set_ts_config(struct efx_nic *efx,
+				struct kernel_hwtstamp_config *config,
+				struct netlink_ext_ack *extack);
+int efx_siena_ptp_get_ts_config(struct efx_nic *efx,
+				struct kernel_hwtstamp_config *config);
 void efx_siena_ptp_get_ts_info(struct efx_nic *efx,
 			       struct ethtool_ts_info *ts_info);
 bool efx_siena_ptp_is_ptp_tx(struct efx_nic *efx, struct sk_buff *skb);
diff --git a/drivers/net/ethernet/sfc/siena/siena.c b/drivers/net/ethernet/sfc/siena/siena.c
index a44c8fa25748..ca33dc08e555 100644
--- a/drivers/net/ethernet/sfc/siena/siena.c
+++ b/drivers/net/ethernet/sfc/siena/siena.c
@@ -136,7 +136,7 @@ static void siena_ptp_write_host_time(struct efx_nic *efx, u32 host_time)
 }
 
 static int siena_ptp_set_ts_config(struct efx_nic *efx,
-				   struct hwtstamp_config *init)
+				   struct kernel_hwtstamp_config *init)
 {
 	int rc;
 
-- 
2.39.3

Re: [PATCH net-next 2/2] sfc-siena: Implement ndo_hwtstamp_(get|set)

[Edward Cree]

On 30/11/2023 13:58, Alex Austin wrote:
> Update efx->ptp_data to use kernel_hwtstamp_config and implement
> ndo_hwtstamp_(get|set). Remove SIOCGHWTSTAMP and SIOCSHWTSTAMP from
> efx_ioctl.
> 
> Signed-off-by: Alex Austin <alex.austin@amd.com>
> Acked-by: Martin Habets <habetsm.xilinx@gmail.com>

Reviewed-by: Edward Cree <ecree.xilinx@gmail.com>

Re: [PATCH net-next 2/2] sfc-siena: Implement ndo_hwtstamp_(get|set)

[Vladimir Oltean]

On Thu, Nov 30, 2023 at 01:58:26PM +0000, Alex Austin wrote:
> Update efx->ptp_data to use kernel_hwtstamp_config and implement
> ndo_hwtstamp_(get|set). Remove SIOCGHWTSTAMP and SIOCSHWTSTAMP from
> efx_ioctl.
> 
> Signed-off-by: Alex Austin <alex.austin@amd.com>
> Acked-by: Martin Habets <habetsm.xilinx@gmail.com>
> ---

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Re: [PATCH net-next 0/2] sfc: Implement ndo_hwtstamp_(get|set)

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 30 Nov 2023 13:58:24 +0000 you wrote:
> Implement ndo_hwtstamp_get and ndo_hwtstamp_set for sfc and sfc-siena.
> 
> Alex Austin (2):
>   sfc: Implement ndo_hwtstamp_(get|set)
>   sfc-siena: Implement ndo_hwtstamp_(get|set)
> 
>  drivers/net/ethernet/sfc/ef10.c             |  4 +--
>  drivers/net/ethernet/sfc/efx.c              | 24 +++++++++++++----
>  drivers/net/ethernet/sfc/net_driver.h       |  2 +-
>  drivers/net/ethernet/sfc/ptp.c              | 30 ++++++++-------------
>  drivers/net/ethernet/sfc/ptp.h              |  7 +++--
>  drivers/net/ethernet/sfc/siena/efx.c        | 24 +++++++++++++----
>  drivers/net/ethernet/sfc/siena/net_driver.h |  2 +-
>  drivers/net/ethernet/sfc/siena/ptp.c        | 30 +++++++++------------
>  drivers/net/ethernet/sfc/siena/ptp.h        |  7 +++--
>  drivers/net/ethernet/sfc/siena/siena.c      |  2 +-
>  10 files changed, 76 insertions(+), 56 deletions(-)

Here is the summary with links:
  - [net-next,1/2] sfc: Implement ndo_hwtstamp_(get|set)
    https://git.kernel.org/netdev/net-next/c/1ac23674a971
  - [net-next,2/2] sfc-siena: Implement ndo_hwtstamp_(get|set)
    https://git.kernel.org/netdev/net-next/c/d82afc800c1e

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html