| Message ID | 20240730091509.18846-7-mateusz.polchlopek@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add support for Rx timestamping for both ice and iavf drivers. | expand |
From: Mateusz Polchlopek <mateusz.polchlopek@intel.com> Date: Tue, 30 Jul 2024 05:15:01 -0400 > 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. > > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> > Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com> > Reviewed-by: Sai Krishna <saikrishnag@marvell.com> > Reviewed-by: Simon Horman <horms@kernel.org> > Co-developed-by: Ahmed Zaki <ahmed.zaki@intel.com> > Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com> > Co-developed-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com> > Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com> > --- > drivers/net/ethernet/intel/iavf/Makefile | 1 + > drivers/net/ethernet/intel/iavf/iavf_main.c | 5 + > drivers/net/ethernet/intel/iavf/iavf_ptp.c | 126 ++++++++++++++++++ > drivers/net/ethernet/intel/iavf/iavf_ptp.h | 10 ++ > .../net/ethernet/intel/iavf/iavf_virtchnl.c | 2 + > 5 files changed, 144 insertions(+) > create mode 100644 drivers/net/ethernet/intel/iavf/iavf_ptp.c > > diff --git a/drivers/net/ethernet/intel/iavf/Makefile b/drivers/net/ethernet/intel/iavf/Makefile > index 356ac9faa5bf..cff88cb49540 100644 > --- a/drivers/net/ethernet/intel/iavf/Makefile > +++ b/drivers/net/ethernet/intel/iavf/Makefile > @@ -13,3 +13,4 @@ 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 Newline before this one? > diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c > index 11599d62d15a..3961c540c1c4 100644 > --- a/drivers/net/ethernet/intel/iavf/iavf_main.c > +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c > @@ -2847,6 +2847,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; > > @@ -5474,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..1344298481d4 > --- /dev/null > +++ b/drivers/net/ethernet/intel/iavf/iavf_ptp.c > @@ -0,0 +1,126 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Copyright(c) 2024 Intel Corporation. */ > + > +#include "iavf.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(struct iavf_adapter *adapter, u32 cap) > +{ > + if (!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; Aren't these parenthesis redundant? > +} > + > +/** > + * 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) - 1, "%s-%s-clk", sizeof(ptp_info->name) without `-1`, snprintf() takes care of it. > + dev_driver_string(dev), > + dev_name(dev)); This can be placed in one line. > + ptp_info->owner = THIS_MODULE; > + > + adapter->ptp.clock = ptp_clock_register(ptp_info, dev); > + if (IS_ERR(adapter->ptp.clock)) > + return PTR_ERR(adapter->ptp.clock); Shouldn't ptp.clock be nullified when this happens? > + > + dev_info(&adapter->pdev->dev, "PTP clock %s registered\n", > + adapter->ptp.info.name); 1. dev_dbg()? 2. pci_* for printing messages from PCI drivers. 3. Empty newline before return. > + 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) > +{ > + struct device *dev = &adapter->pdev->dev; > + int err; > + > + if (WARN_ON(adapter->ptp.initialized)) { > + dev_err(dev, "PTP functionality was already initialized!\n"); Is this needed? > + return; > + } > + > + if (!iavf_ptp_cap_supported(adapter, VIRTCHNL_1588_PTP_CAP_READ_PHC)) { > + dev_dbg(dev, "Device does not have PTP clock support\n"); This is pci_warn(), not dbg I guess? > + return; > + } > + > + err = iavf_ptp_register_clock(adapter); > + if (err) { > + dev_warn(dev, "Failed to register PTP clock device (%d)\n", > + err); pci_warn(pdev, "failed to ... device, %pe\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_info(&adapter->pdev->dev, "removing PTP clock %s\n", > + adapter->ptp.info.name); _dbg(). > + 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) > +{ > + struct device *dev = &adapter->pdev->dev; > + > + dev_dbg(dev, "PTP capabilities changed at runtime\n"); Is this needed at all? > + > + /* 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 && > + !iavf_ptp_cap_supported(adapter, VIRTCHNL_1588_PTP_CAP_READ_PHC)) > + iavf_ptp_release(adapter); > + else if (!adapter->ptp.initialized && > + iavf_ptp_cap_supported(adapter, > + VIRTCHNL_1588_PTP_CAP_READ_PHC)) You can check for the cap support once before the whole block. > + 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 aee4e2da0b9a..4939c219bd18 100644 > --- a/drivers/net/ethernet/intel/iavf/iavf_ptp.h > +++ b/drivers/net/ethernet/intel/iavf/iavf_ptp.h > @@ -4,9 +4,19 @@ > #ifndef _IAVF_PTP_H_ > #define _IAVF_PTP_H_ > > +#include <linux/ptp_clock_kernel.h> > + > /* fields used for PTP support */ > struct iavf_ptp { > struct virtchnl_ptp_caps hw_caps; > + bool initialized; Holes. > + struct ptp_clock_info info; > + struct ptp_clock *clock; > }; > > +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(struct iavf_adapter *adapter, u32 cap); > + > #endif /* _IAVF_PTP_H_ */ Thanks, Olek
On 7/30/2024 3:40 PM, Alexander Lobakin wrote: > From: Mateusz Polchlopek <mateusz.polchlopek@intel.com> > Date: Tue, 30 Jul 2024 05:15:01 -0400 > >> 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. >> >> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com> >> Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com> >> Reviewed-by: Sai Krishna <saikrishnag@marvell.com> >> Reviewed-by: Simon Horman <horms@kernel.org> >> Co-developed-by: Ahmed Zaki <ahmed.zaki@intel.com> >> Signed-off-by: Ahmed Zaki <ahmed.zaki@intel.com> >> Co-developed-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com> >> Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com> >> --- >> drivers/net/ethernet/intel/iavf/Makefile | 1 + >> drivers/net/ethernet/intel/iavf/iavf_main.c | 5 + >> drivers/net/ethernet/intel/iavf/iavf_ptp.c | 126 ++++++++++++++++++ >> drivers/net/ethernet/intel/iavf/iavf_ptp.h | 10 ++ >> .../net/ethernet/intel/iavf/iavf_virtchnl.c | 2 + >> 5 files changed, 144 insertions(+) >> create mode 100644 drivers/net/ethernet/intel/iavf/iavf_ptp.c >> >> diff --git a/drivers/net/ethernet/intel/iavf/Makefile b/drivers/net/ethernet/intel/iavf/Makefile >> index 356ac9faa5bf..cff88cb49540 100644 >> --- a/drivers/net/ethernet/intel/iavf/Makefile >> +++ b/drivers/net/ethernet/intel/iavf/Makefile >> @@ -13,3 +13,4 @@ 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 > > Newline before this one? > >> diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c b/drivers/net/ethernet/intel/iavf/iavf_main.c >> index 11599d62d15a..3961c540c1c4 100644 >> --- a/drivers/net/ethernet/intel/iavf/iavf_main.c >> +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c >> @@ -2847,6 +2847,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; >> >> @@ -5474,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..1344298481d4 >> --- /dev/null >> +++ b/drivers/net/ethernet/intel/iavf/iavf_ptp.c >> @@ -0,0 +1,126 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* Copyright(c) 2024 Intel Corporation. */ >> + >> +#include "iavf.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(struct iavf_adapter *adapter, u32 cap) >> +{ >> + if (!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; > > Aren't these parenthesis redundant? > I think they are not. They wrap bit operation and also I checked it with checkpatch script and it doesn't complain about reduntant parenthesis. >> +} >> + >> +/** >> + * 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) - 1, "%s-%s-clk", > > sizeof(ptp_info->name) without `-1`, snprintf() takes care of it. > >> + dev_driver_string(dev), >> + dev_name(dev)); > > This can be placed in one line. > >> + ptp_info->owner = THIS_MODULE; >> + >> + adapter->ptp.clock = ptp_clock_register(ptp_info, dev); >> + if (IS_ERR(adapter->ptp.clock)) >> + return PTR_ERR(adapter->ptp.clock); > > Shouldn't ptp.clock be nullified when this happens? > >> + >> + dev_info(&adapter->pdev->dev, "PTP clock %s registered\n", >> + adapter->ptp.info.name); > > 1. dev_dbg()? > 2. pci_* for printing messages from PCI drivers. > 3. Empty newline before return. > >> + 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) >> +{ >> + struct device *dev = &adapter->pdev->dev; >> + int err; >> + >> + if (WARN_ON(adapter->ptp.initialized)) { >> + dev_err(dev, "PTP functionality was already initialized!\n"); > > Is this needed? > >> + return; >> + } >> + >> + if (!iavf_ptp_cap_supported(adapter, VIRTCHNL_1588_PTP_CAP_READ_PHC)) { >> + dev_dbg(dev, "Device does not have PTP clock support\n"); > > This is pci_warn(), not dbg I guess? > >> + return; >> + } >> + >> + err = iavf_ptp_register_clock(adapter); >> + if (err) { >> + dev_warn(dev, "Failed to register PTP clock device (%d)\n", >> + err); > > pci_warn(pdev, "failed to ... device, %pe\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_info(&adapter->pdev->dev, "removing PTP clock %s\n", >> + adapter->ptp.info.name); > > _dbg(). > >> + 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) >> +{ >> + struct device *dev = &adapter->pdev->dev; >> + >> + dev_dbg(dev, "PTP capabilities changed at runtime\n"); > > Is this needed at all? > >> + >> + /* 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 && >> + !iavf_ptp_cap_supported(adapter, VIRTCHNL_1588_PTP_CAP_READ_PHC)) >> + iavf_ptp_release(adapter); >> + else if (!adapter->ptp.initialized && >> + iavf_ptp_cap_supported(adapter, >> + VIRTCHNL_1588_PTP_CAP_READ_PHC)) > > You can check for the cap support once before the whole block. > >> + 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 aee4e2da0b9a..4939c219bd18 100644 >> --- a/drivers/net/ethernet/intel/iavf/iavf_ptp.h >> +++ b/drivers/net/ethernet/intel/iavf/iavf_ptp.h >> @@ -4,9 +4,19 @@ >> #ifndef _IAVF_PTP_H_ >> #define _IAVF_PTP_H_ >> >> +#include <linux/ptp_clock_kernel.h> >> + >> /* fields used for PTP support */ >> struct iavf_ptp { >> struct virtchnl_ptp_caps hw_caps; >> + bool initialized; > > Holes. > >> + struct ptp_clock_info info; >> + struct ptp_clock *clock; >> }; >> >> +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(struct iavf_adapter *adapter, u32 cap); >> + >> #endif /* _IAVF_PTP_H_ */ > > Thanks, > Olek
From: Mateusz Polchlopek <mateusz.polchlopek@intel.com> Date: Thu, 8 Aug 2024 13:04:29 +0200 > > > On 7/30/2024 3:40 PM, Alexander Lobakin wrote: >> From: Mateusz Polchlopek <mateusz.polchlopek@intel.com> >> Date: Tue, 30 Jul 2024 05:15:01 -0400 [...] >>> +bool iavf_ptp_cap_supported(struct iavf_adapter *adapter, u32 cap) >>> +{ >>> + if (!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; >> >> Aren't these parenthesis redundant? >> > > I think they are not. They wrap bit operation and also I checked it > with checkpatch script and it doesn't complain about reduntant > parenthesis. If the object code doesn't change when compiling without them, there are no compiler complains etc, then they are :D checkpatch doesn't always catch things, but I don't remember whether the compiler won't complain or change the object code / logic. Could you please check? Thanks, Olek
> -----Original Message----- > From: Lobakin, Aleksander <aleksander.lobakin@intel.com> > Sent: Thursday, August 8, 2024 5:25 AM > To: Polchlopek, Mateusz <mateusz.polchlopek@intel.com> > Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; Keller, Jacob E > <jacob.e.keller@intel.com>; Drewek, Wojciech <wojciech.drewek@intel.com>; Sai > Krishna <saikrishnag@marvell.com>; Simon Horman <horms@kernel.org>; Zaki, > Ahmed <ahmed.zaki@intel.com> > Subject: Re: [Intel-wired-lan] [PATCH iwl-next v8 06/14] iavf: add initial framework > for registering PTP clock > > From: Mateusz Polchlopek <mateusz.polchlopek@intel.com> > Date: Thu, 8 Aug 2024 13:04:29 +0200 > > > > > > > On 7/30/2024 3:40 PM, Alexander Lobakin wrote: > >> From: Mateusz Polchlopek <mateusz.polchlopek@intel.com> > >> Date: Tue, 30 Jul 2024 05:15:01 -0400 > > [...] > > >>> +bool iavf_ptp_cap_supported(struct iavf_adapter *adapter, u32 cap) > >>> +{ > >>> + if (!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; > >> > >> Aren't these parenthesis redundant? > >> > > > > I think they are not. They wrap bit operation and also I checked it > > with checkpatch script and it doesn't complain about reduntant > > parenthesis. > > If the object code doesn't change when compiling without them, there are > no compiler complains etc, then they are :D checkpatch doesn't always > catch things, but I don't remember whether the compiler won't complain > or change the object code / logic. Could you please check? > > Thanks, > Olek They may be technically redundant in that the parenthesis don't matter.. but sometimes they can help code legibility by making it more obvious to a human reviewer who isn't immediately going to think like a compiler and realize that & is not && for example...
On 8/8/2024 2:24 PM, Alexander Lobakin wrote: > From: Mateusz Polchlopek <mateusz.polchlopek@intel.com> > Date: Thu, 8 Aug 2024 13:04:29 +0200 > >> >> >> On 7/30/2024 3:40 PM, Alexander Lobakin wrote: >>> From: Mateusz Polchlopek <mateusz.polchlopek@intel.com> >>> Date: Tue, 30 Jul 2024 05:15:01 -0400 > > [...] > >>>> +bool iavf_ptp_cap_supported(struct iavf_adapter *adapter, u32 cap) >>>> +{ >>>> + if (!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; >>> >>> Aren't these parenthesis redundant? >>> >> >> I think they are not. They wrap bit operation and also I checked it >> with checkpatch script and it doesn't complain about reduntant >> parenthesis. > > If the object code doesn't change when compiling without them, there are > no compiler complains etc, then they are :D checkpatch doesn't always > catch things, but I don't remember whether the compiler won't complain > or change the object code / logic. Could you please check? > > Thanks, > Olek Okay, good point. I checked that and they are not redundant. If I remove them then compiler complains and object code changes so - parenthesis stay with us :D thanks
From: Mateusz Polchlopek <mateusz.polchlopek@intel.com> Date: Mon, 12 Aug 2024 12:01:34 +0200 > > > On 8/8/2024 2:24 PM, Alexander Lobakin wrote: >> From: Mateusz Polchlopek <mateusz.polchlopek@intel.com> >> Date: Thu, 8 Aug 2024 13:04:29 +0200 >> >>> >>> >>> On 7/30/2024 3:40 PM, Alexander Lobakin wrote: >>>> From: Mateusz Polchlopek <mateusz.polchlopek@intel.com> >>>> Date: Tue, 30 Jul 2024 05:15:01 -0400 >> >> [...] >> >>>>> +bool iavf_ptp_cap_supported(struct iavf_adapter *adapter, u32 cap) >>>>> +{ >>>>> + if (!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; >>>> >>>> Aren't these parenthesis redundant? >>>> >>> >>> I think they are not. They wrap bit operation and also I checked it >>> with checkpatch script and it doesn't complain about reduntant >>> parenthesis. >> >> If the object code doesn't change when compiling without them, there are >> no compiler complains etc, then they are :D checkpatch doesn't always >> catch things, but I don't remember whether the compiler won't complain >> or change the object code / logic. Could you please check? >> >> Thanks, >> Olek > > Okay, good point. I checked that and they are not redundant. If I remove > them then compiler complains and object code changes so - parenthesis > stay with us :D Nice, thanks for checking! It's always better and faster to just check and make sure. > > thanks Olek
diff --git a/drivers/net/ethernet/intel/iavf/Makefile b/drivers/net/ethernet/intel/iavf/Makefile index 356ac9faa5bf..cff88cb49540 100644 --- a/drivers/net/ethernet/intel/iavf/Makefile +++ b/drivers/net/ethernet/intel/iavf/Makefile @@ -13,3 +13,4 @@ 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 11599d62d15a..3961c540c1c4 100644 --- a/drivers/net/ethernet/intel/iavf/iavf_main.c +++ b/drivers/net/ethernet/intel/iavf/iavf_main.c @@ -2847,6 +2847,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; @@ -5474,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..1344298481d4 --- /dev/null +++ b/drivers/net/ethernet/intel/iavf/iavf_ptp.c @@ -0,0 +1,126 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright(c) 2024 Intel Corporation. */ + +#include "iavf.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(struct iavf_adapter *adapter, u32 cap) +{ + if (!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) - 1, "%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)) + return PTR_ERR(adapter->ptp.clock); + + dev_info(&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) +{ + struct device *dev = &adapter->pdev->dev; + int err; + + if (WARN_ON(adapter->ptp.initialized)) { + dev_err(dev, "PTP functionality was already initialized!\n"); + return; + } + + if (!iavf_ptp_cap_supported(adapter, VIRTCHNL_1588_PTP_CAP_READ_PHC)) { + dev_dbg(dev, "Device does not have PTP clock support\n"); + return; + } + + err = iavf_ptp_register_clock(adapter); + if (err) { + dev_warn(dev, "Failed to register PTP clock device (%d)\n", + 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_info(&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) +{ + struct device *dev = &adapter->pdev->dev; + + dev_dbg(dev, "PTP capabilities changed at runtime\n"); + + /* 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 && + !iavf_ptp_cap_supported(adapter, VIRTCHNL_1588_PTP_CAP_READ_PHC)) + iavf_ptp_release(adapter); + else if (!adapter->ptp.initialized && + iavf_ptp_cap_supported(adapter, + VIRTCHNL_1588_PTP_CAP_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 aee4e2da0b9a..4939c219bd18 100644 --- a/drivers/net/ethernet/intel/iavf/iavf_ptp.h +++ b/drivers/net/ethernet/intel/iavf/iavf_ptp.h @@ -4,9 +4,19 @@ #ifndef _IAVF_PTP_H_ #define _IAVF_PTP_H_ +#include <linux/ptp_clock_kernel.h> + /* fields used for PTP support */ struct iavf_ptp { struct virtchnl_ptp_caps hw_caps; + bool initialized; + struct ptp_clock_info info; + struct ptp_clock *clock; }; +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(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 0f338f337778..a75a9cf46591 100644 --- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c +++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c @@ -2510,6 +2510,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 */