| Message ID | 20231130135826.19018-2-alex.austin@amd.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 1ac23674a971d8596648695f72168815c3f52e11 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | sfc: Implement ndo_hwtstamp_(get|set) | expand |
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>
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?
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?
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.
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
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.
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
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 >
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);