Re: [Intel-wired-lan] [PATCH iwl-next v10 06/14] iavf: add initial framework for registering PTP clock

[Alexander Lobakin <aleksander.lobakin@intel.com>]

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