Message ID | 20240821121539.374343-7-wojciech.drewek@intel.com |
---|---|
State | Changes Requested |
Headers | show |
Series | Add support for Rx timestamping for both ice and iavf drivers | expand |
From: Wojciech Drewek <wojciech.drewek@intel.com> Date: Wed, 21 Aug 2024 14:15:31 +0200 > From: Jacob Keller <jacob.e.keller@intel.com> > > Add the iavf_ptp.c file and fill it in with a skeleton framework to > allow registering the PTP clock device. > Add implementation of helper functions to check if a PTP capability > is supported and handle change in PTP capabilities. > Enabling virtual clock would be possible, though it would probably > perform poorly due to the lack of direct time access. [...] > +/** > + * iavf_ptp_register_clock - Register a new PTP for userspace > + * @adapter: private adapter structure > + * > + * Allocate and register a new PTP clock device if necessary. > + * > + * Return: 0 if success, error otherwise Period ('.') at the end is desired at the end of kdoc. > + */ > +static int iavf_ptp_register_clock(struct iavf_adapter *adapter) > +{ > + struct ptp_clock_info *ptp_info = &adapter->ptp.info; > + struct device *dev = &adapter->pdev->dev; > + > + memset(ptp_info, 0, sizeof(*ptp_info)); Is this needed? adapter is allocated using kzalloc() I think? > + > + snprintf(ptp_info->name, sizeof(ptp_info->name), "%s-%s-clk", > + dev_driver_string(dev), dev_name(dev)); dev_driver_string() can be just KBUILD_MODNAME when it's called inside the actual module. It's mostly used when you need to get a module name from a different module or core kernel code. > + ptp_info->owner = THIS_MODULE; > + > + adapter->ptp.clock = ptp_clock_register(ptp_info, dev); > + if (IS_ERR(adapter->ptp.clock)) { > + adapter->ptp.clock = NULL; > + > + return PTR_ERR(adapter->ptp.clock); Braino here. You first set ptp.clock to %NULL and then return PTR_ERR(ptp.clock). IOW, this error path will always return 0. I usually use temporary variables to avoid this. clock = ptp_clock_register(ptp_info, dev); if (IS_ERR(clock)) return PTR_ERR(clock); adapter->ptp.clock = clock; > + } > + > + dev_dbg(&adapter->pdev->dev, "PTP clock %s registered\n", > + adapter->ptp.info.name); > + > + return 0; > +} > + > +/** > + * iavf_ptp_init - Initialize PTP support if capability was negotiated > + * @adapter: private adapter structure > + * > + * Initialize PTP functionality, based on the capabilities that the PF has > + * enabled for this VF. > + */ > +void iavf_ptp_init(struct iavf_adapter *adapter) > +{ > + int err; > + > + if (!iavf_ptp_cap_supported(adapter, VIRTCHNL_1588_PTP_CAP_READ_PHC)) { > + pci_warn(adapter->pdev, > + "Device does not have PTP clock support\n"); I think it's pci_notice() or even pci_dbg(). A device can miss PTP clock, but it's not a failure. _warn() is when something went wrong, but not as wrong as _err() :D > + return; > + } > + > + err = iavf_ptp_register_clock(adapter); > + if (err) { > + pci_err(adapter->pdev, > + "Failed to register PTP clock device (%p)\n", > + ERR_PTR(err)); > + return; > + } Why does this function return void if there's an error path? To make sure the driver works even if PTP fails to register? But I think it's better to bail out if something failed than to work without certain functionality? > + > + adapter->ptp.initialized = true; > +} > + > +/** > + * iavf_ptp_release - Disable PTP support > + * @adapter: private adapter structure > + * > + * Release all PTP resources that were previously initialized. > + */ > +void iavf_ptp_release(struct iavf_adapter *adapter) > +{ > + adapter->ptp.initialized = false; > + > + if (!IS_ERR_OR_NULL(adapter->ptp.clock)) { Since you always assign clock to %NULL when the initialization failed, this could be just if (adapter->ptp.clock) > + dev_dbg(&adapter->pdev->dev, "removing PTP clock %s\n", > + adapter->ptp.info.name); pci_dbg() > + ptp_clock_unregister(adapter->ptp.clock); > + adapter->ptp.clock = NULL; > + } ...but I'd invert the condition to avoid +1 indent level. if (!adapter->ptp.clock) return; pci_dbg() ... > +} > + > +/** > + * iavf_ptp_process_caps - Handle change in PTP capabilities > + * @adapter: private adapter structure > + * > + * Handle any state changes necessary due to change in PTP capabilities, such > + * as after a device reset or change in configuration from the PF. > + */ > +void iavf_ptp_process_caps(struct iavf_adapter *adapter) > +{ > + bool read_phc = iavf_ptp_cap_supported(adapter, > + VIRTCHNL_1588_PTP_CAP_READ_PHC); Maybe split the declaration and initialization to avoid line break? My editor says it would fit in 80 if you make the variable name shorter, e.g. 'phc'. > + > + /* Check if the device gained or lost necessary access to support the > + * PTP hardware clock. If so, driver must respond appropriately by > + * creating or destroying the PTP clock device. > + */ > + if (adapter->ptp.initialized && !read_phc) > + iavf_ptp_release(adapter); > + else if (!adapter->ptp.initialized && read_phc) > + iavf_ptp_init(adapter); > +} Thanks, Olek
On 21.08.2024 16:20, Alexander Lobakin wrote: > From: Wojciech Drewek <wojciech.drewek@intel.com> > Date: Wed, 21 Aug 2024 14:15:31 +0200 > >> From: Jacob Keller <jacob.e.keller@intel.com> >> >> Add the iavf_ptp.c file and fill it in with a skeleton framework to >> allow registering the PTP clock device. >> Add implementation of helper functions to check if a PTP capability >> is supported and handle change in PTP capabilities. >> Enabling virtual clock would be possible, though it would probably >> perform poorly due to the lack of direct time access. > > [...] > >> +/** >> + * iavf_ptp_register_clock - Register a new PTP for userspace >> + * @adapter: private adapter structure >> + * >> + * Allocate and register a new PTP clock device if necessary. >> + * >> + * Return: 0 if success, error otherwise > > Period ('.') at the end is desired at the end of kdoc. Sure > >> + */ >> +static int iavf_ptp_register_clock(struct iavf_adapter *adapter) >> +{ >> + struct ptp_clock_info *ptp_info = &adapter->ptp.info; >> + struct device *dev = &adapter->pdev->dev; >> + >> + memset(ptp_info, 0, sizeof(*ptp_info)); > > Is this needed? adapter is allocated using kzalloc() I think? I think it's not needed, adapter is allocated using alloc_etherdev_mq since this is netdev's priv in iavf > >> + >> + snprintf(ptp_info->name, sizeof(ptp_info->name), "%s-%s-clk", >> + dev_driver_string(dev), dev_name(dev)); > > dev_driver_string() can be just KBUILD_MODNAME when it's called inside > the actual module. It's mostly used when you need to get a module name > from a different module or core kernel code. Makes sense > >> + ptp_info->owner = THIS_MODULE; >> + >> + adapter->ptp.clock = ptp_clock_register(ptp_info, dev); >> + if (IS_ERR(adapter->ptp.clock)) { >> + adapter->ptp.clock = NULL; >> + >> + return PTR_ERR(adapter->ptp.clock); > > Braino here. > You first set ptp.clock to %NULL and then return PTR_ERR(ptp.clock). > IOW, this error path will always return 0. > > I usually use temporary variables to avoid this. > > clock = ptp_clock_register(ptp_info, dev); > if (IS_ERR(clock)) > return PTR_ERR(clock); > > adapter->ptp.clock = clock; will fix > > >> + } >> + >> + dev_dbg(&adapter->pdev->dev, "PTP clock %s registered\n", >> + adapter->ptp.info.name); >> + >> + return 0; >> +} >> + >> +/** >> + * iavf_ptp_init - Initialize PTP support if capability was negotiated >> + * @adapter: private adapter structure >> + * >> + * Initialize PTP functionality, based on the capabilities that the PF has >> + * enabled for this VF. >> + */ >> +void iavf_ptp_init(struct iavf_adapter *adapter) >> +{ >> + int err; >> + >> + if (!iavf_ptp_cap_supported(adapter, VIRTCHNL_1588_PTP_CAP_READ_PHC)) { >> + pci_warn(adapter->pdev, >> + "Device does not have PTP clock support\n"); > > I think it's pci_notice() or even pci_dbg(). A device can miss PTP > clock, but it's not a failure. _warn() is when something went wrong, but > not as wrong as _err() :D sure > >> + return; >> + } >> + >> + err = iavf_ptp_register_clock(adapter); >> + if (err) { >> + pci_err(adapter->pdev, >> + "Failed to register PTP clock device (%p)\n", >> + ERR_PTR(err)); >> + return; >> + } > > Why does this function return void if there's an error path? To make > sure the driver works even if PTP fails to register? But I think it's > better to bail out if something failed than to work without certain > functionality? Most of the drivers don't bail out if ptp init failed, I'll stick to that. > >> + >> + adapter->ptp.initialized = true; >> +} >> + >> +/** >> + * iavf_ptp_release - Disable PTP support >> + * @adapter: private adapter structure >> + * >> + * Release all PTP resources that were previously initialized. >> + */ >> +void iavf_ptp_release(struct iavf_adapter *adapter) >> +{ >> + adapter->ptp.initialized = false; >> + >> + if (!IS_ERR_OR_NULL(adapter->ptp.clock)) { > > Since you always assign clock to %NULL when the initialization failed, > this could be just Yep > > if (adapter->ptp.clock) > >> + dev_dbg(&adapter->pdev->dev, "removing PTP clock %s\n", >> + adapter->ptp.info.name); > > pci_dbg() > >> + ptp_clock_unregister(adapter->ptp.clock); >> + adapter->ptp.clock = NULL; >> + } > > ...but I'd invert the condition to avoid +1 indent level. > > if (!adapter->ptp.clock) > return; > > pci_dbg() ... Agree > >> +} >> + >> +/** >> + * iavf_ptp_process_caps - Handle change in PTP capabilities >> + * @adapter: private adapter structure >> + * >> + * Handle any state changes necessary due to change in PTP capabilities, such >> + * as after a device reset or change in configuration from the PF. >> + */ >> +void iavf_ptp_process_caps(struct iavf_adapter *adapter) >> +{ >> + bool read_phc = iavf_ptp_cap_supported(adapter, >> + VIRTCHNL_1588_PTP_CAP_READ_PHC); > > Maybe split the declaration and initialization to avoid line break? My > editor says it would fit in 80 if you make the variable name shorter, > e.g. 'phc'. Sure, why not > >> + >> + /* Check if the device gained or lost necessary access to support the >> + * PTP hardware clock. If so, driver must respond appropriately by >> + * creating or destroying the PTP clock device. >> + */ >> + if (adapter->ptp.initialized && !read_phc) >> + iavf_ptp_release(adapter); >> + else if (!adapter->ptp.initialized && read_phc) >> + iavf_ptp_init(adapter); >> +} > > Thanks, > Olek
diff --git a/drivers/net/ethernet/intel/iavf/Makefile b/drivers/net/ethernet/intel/iavf/Makefile index 356ac9faa5bf..e13720a728ff 100644 --- a/drivers/net/ethernet/intel/iavf/Makefile +++ b/drivers/net/ethernet/intel/iavf/Makefile @@ -13,3 +13,5 @@ obj-$(CONFIG_IAVF) += iavf.o iavf-y := iavf_main.o iavf_ethtool.o iavf_virtchnl.o iavf_fdir.o \ iavf_adv_rss.o iavf_txrx.o iavf_common.o iavf_adminq.o + +iavf-$(CONFIG_PTP_1588_CLOCK) += iavf_ptp.o diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c index c26f6fc5250b..8a99aab5bf6f 100644 --- a/drivers/net/ethernet/intel/iavf/iavf_main.c +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c @@ -4,6 +4,7 @@ #include <linux/net/intel/libie/rx.h> #include "iavf.h" +#include "iavf_ptp.h" #include "iavf_prototype.h" /* All iavf tracepoints are defined by the include below, which must * be included exactly once across the whole kernel with @@ -2834,6 +2835,9 @@ static void iavf_init_config_adapter(struct iavf_adapter *adapter) /* request initial VLAN offload settings */ iavf_set_vlan_offload_features(adapter, 0, netdev->features); + /* Setup initial PTP configuration */ + iavf_ptp_init(adapter); + iavf_schedule_finish_config(adapter); return; @@ -5473,6 +5477,8 @@ static void iavf_remove(struct pci_dev *pdev) msleep(50); } + iavf_ptp_release(adapter); + iavf_misc_irq_disable(adapter); /* Shut down all the garbage mashers on the detention level */ cancel_work_sync(&adapter->reset_task); diff --git a/drivers/net/ethernet/intel/iavf/iavf_ptp.c b/drivers/net/ethernet/intel/iavf/iavf_ptp.c new file mode 100644 index 000000000000..045aa8690ac2 --- /dev/null +++ b/drivers/net/ethernet/intel/iavf/iavf_ptp.c @@ -0,0 +1,122 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright(c) 2024 Intel Corporation. */ + +#include "iavf.h" +#include "iavf_ptp.h" + +/** + * iavf_ptp_cap_supported - Check if a PTP capability is supported + * @adapter: private adapter structure + * @cap: the capability bitmask to check + * + * Return: true if every capability set in cap is also set in the enabled + * capabilities reported by the PF, false otherwise. + */ +bool iavf_ptp_cap_supported(const struct iavf_adapter *adapter, u32 cap) +{ + if (!IAVF_PTP_ALLOWED(adapter)) + return false; + + /* Only return true if every bit in cap is set in hw_caps.caps */ + return (adapter->ptp.hw_caps.caps & cap) == cap; +} + +/** + * iavf_ptp_register_clock - Register a new PTP for userspace + * @adapter: private adapter structure + * + * Allocate and register a new PTP clock device if necessary. + * + * Return: 0 if success, error otherwise + */ +static int iavf_ptp_register_clock(struct iavf_adapter *adapter) +{ + struct ptp_clock_info *ptp_info = &adapter->ptp.info; + struct device *dev = &adapter->pdev->dev; + + memset(ptp_info, 0, sizeof(*ptp_info)); + + snprintf(ptp_info->name, sizeof(ptp_info->name), "%s-%s-clk", + dev_driver_string(dev), dev_name(dev)); + ptp_info->owner = THIS_MODULE; + + adapter->ptp.clock = ptp_clock_register(ptp_info, dev); + if (IS_ERR(adapter->ptp.clock)) { + adapter->ptp.clock = NULL; + + return PTR_ERR(adapter->ptp.clock); + } + + dev_dbg(&adapter->pdev->dev, "PTP clock %s registered\n", + adapter->ptp.info.name); + + return 0; +} + +/** + * iavf_ptp_init - Initialize PTP support if capability was negotiated + * @adapter: private adapter structure + * + * Initialize PTP functionality, based on the capabilities that the PF has + * enabled for this VF. + */ +void iavf_ptp_init(struct iavf_adapter *adapter) +{ + int err; + + if (!iavf_ptp_cap_supported(adapter, VIRTCHNL_1588_PTP_CAP_READ_PHC)) { + pci_warn(adapter->pdev, + "Device does not have PTP clock support\n"); + return; + } + + err = iavf_ptp_register_clock(adapter); + if (err) { + pci_err(adapter->pdev, + "Failed to register PTP clock device (%p)\n", + ERR_PTR(err)); + return; + } + + adapter->ptp.initialized = true; +} + +/** + * iavf_ptp_release - Disable PTP support + * @adapter: private adapter structure + * + * Release all PTP resources that were previously initialized. + */ +void iavf_ptp_release(struct iavf_adapter *adapter) +{ + adapter->ptp.initialized = false; + + if (!IS_ERR_OR_NULL(adapter->ptp.clock)) { + dev_dbg(&adapter->pdev->dev, "removing PTP clock %s\n", + adapter->ptp.info.name); + ptp_clock_unregister(adapter->ptp.clock); + adapter->ptp.clock = NULL; + } +} + +/** + * iavf_ptp_process_caps - Handle change in PTP capabilities + * @adapter: private adapter structure + * + * Handle any state changes necessary due to change in PTP capabilities, such + * as after a device reset or change in configuration from the PF. + */ +void iavf_ptp_process_caps(struct iavf_adapter *adapter) +{ + bool read_phc = iavf_ptp_cap_supported(adapter, + VIRTCHNL_1588_PTP_CAP_READ_PHC); + + /* Check if the device gained or lost necessary access to support the + * PTP hardware clock. If so, driver must respond appropriately by + * creating or destroying the PTP clock device. + */ + if (adapter->ptp.initialized && !read_phc) + iavf_ptp_release(adapter); + else if (!adapter->ptp.initialized && read_phc) + iavf_ptp_init(adapter); +} diff --git a/drivers/net/ethernet/intel/iavf/iavf_ptp.h b/drivers/net/ethernet/intel/iavf/iavf_ptp.h index 65678e76c34f..c2ed24cef926 100644 --- a/drivers/net/ethernet/intel/iavf/iavf_ptp.h +++ b/drivers/net/ethernet/intel/iavf/iavf_ptp.h @@ -6,4 +6,9 @@ #include "iavf_types.h" +void iavf_ptp_init(struct iavf_adapter *adapter); +void iavf_ptp_release(struct iavf_adapter *adapter); +void iavf_ptp_process_caps(struct iavf_adapter *adapter); +bool iavf_ptp_cap_supported(const struct iavf_adapter *adapter, u32 cap); + #endif /* _IAVF_PTP_H_ */ diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c index 63ea30a20576..03e880d756de 100644 --- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c +++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c @@ -4,6 +4,7 @@ #include <linux/net/intel/libie/rx.h> #include "iavf.h" +#include "iavf_ptp.h" #include "iavf_prototype.h" /** @@ -387,6 +388,7 @@ void iavf_configure_queues(struct iavf_adapter *adapter) int pairs = adapter->num_active_queues; struct virtchnl_queue_pair_info *vqpi; u32 i, max_frame; + u8 rx_flags = 0; size_t len; max_frame = LIBIE_MAX_RX_FRM_LEN(adapter->rx_rings->pp->p.offset); @@ -404,6 +406,9 @@ void iavf_configure_queues(struct iavf_adapter *adapter) if (!vqci) return; + if (iavf_ptp_cap_supported(adapter, VIRTCHNL_1588_PTP_CAP_RX_TSTAMP)) + rx_flags |= VIRTCHNL_PTP_RX_TSTAMP; + vqci->vsi_id = adapter->vsi_res->vsi_id; vqci->num_queue_pairs = pairs; vqpi = vqci->qpair; @@ -426,6 +431,7 @@ void iavf_configure_queues(struct iavf_adapter *adapter) if (CRC_OFFLOAD_ALLOWED(adapter)) vqpi->rxq.crc_disable = !!(adapter->netdev->features & NETIF_F_RXFCS); + vqpi->rxq.flags = rx_flags; vqpi++; } @@ -2494,6 +2500,8 @@ void iavf_virtchnl_completion(struct iavf_adapter *adapter, case VIRTCHNL_OP_1588_PTP_GET_CAPS: memcpy(&adapter->ptp.hw_caps, msg, min_t(u16, msglen, sizeof(adapter->ptp.hw_caps))); + /* process any state change needed due to new capabilities */ + iavf_ptp_process_caps(adapter); break; case VIRTCHNL_OP_ENABLE_QUEUES: /* enable transmits */