From: Wojciech Drewek <wojciech.dre...@intel.com> Date: Wed, 21 Aug 2024 14:15:31 +0200
> From: Jacob Keller <jacob.e.kel...@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