Message-ID: <213347f1-7af5-45a5-a9ce-448d98867d35@intel.com>
Date: Tue, 27 Aug 2024 12:59:50 +0200
From: Wojciech Drewek <wojciech.drewek@...el.com>
To: Alexander Lobakin <aleksander.lobakin@...el.com>
CC: <netdev@...r.kernel.org>, <intel-wired-lan@...ts.osuosl.org>,
	<horms@...nel.org>, <anthony.l.nguyen@...el.com>, <kuba@...nel.org>,
	<alexandr.lobakin@...el.com>
Subject: Re: [PATCH iwl-next v10 06/14] iavf: add initial framework for
 registering PTP clock



On 21.08.2024 16:20, Alexander Lobakin wrote:
> From: Wojciech Drewek <wojciech.drewek@...el.com>
> Date: Wed, 21 Aug 2024 14:15:31 +0200
> 
>> From: Jacob Keller <jacob.e.keller@...el.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

Powered by blists - more mailing lists