Re: [Intel-wired-lan] [PATCH net-next v2 1/3] ice: Auxbus devices & driver for E822 TS

[Tony Nguyen <anthony.l.nguyen@intel.com>]

On 7/21/2023 8:44 AM, Michal Michalik wrote:
> There is a problem in HW in E822-based devices leading to race
> condition.
> It might happen that, in order:
> - PF0 (which owns the PHC) requests few timestamps,
> - PF1 requests a timestamp,
> - interrupt is being triggered and both PF0 and PF1 threads are woken
> up,
> - PF0 got one timestamp, still waiting for others so not going to sleep,
> - PF1 gets it's timestamp, process it and go to sleep,
> - PF1 requests a timestamp again,
> - just before PF0 goes to sleep timestamp of PF1 appear,
> - PF0 finishes all it's timestamps and go to sleep (PF1 also sleeping).
> That leaves PF1 timestamp memory not read, which lead to blocking the
> next interrupt from arriving.
> 
> Fix it by adding auxiliary devices and only one driver to handle all the
> timestamps for all PF's by PHC owner. In the past each PF requested it's
> own timestamps and process it from the start till the end which causes
> problem described above. Currently each PF requests the timestamps as
> before, but the actual reading of the completed timestamps is being done
> by the PTP auxilary driver, which is registered by the PF which owns PHC.

WARNING: 'auxilary' may be misspelled - perhaps 'auxiliary'?

...

> +/**
> + * ice_ptp_auxbus_create_id_table - Create auxiliary device ID table
> + * @pf: Board private structure
> + * @name: auxiliary bus driver name
> + */
> +static struct auxiliary_device_id *
> +ice_ptp_auxbus_create_id_table(struct ice_pf *pf, const char *name)
> +{
> +	struct auxiliary_device_id *ids;
> +
> +	/* Second id left empty to terminate the array */
> +	ids = (struct auxiliary_device_id *)
> +	       devm_kcalloc(ice_pf_to_dev(pf), 2,
> +			    sizeof(struct auxiliary_device_id), GFP_KERNEL);

This casting is no needed. Also, some void* casts below that aren't 
needed either.

> +	if (!ids)
> +		return NULL;
> +
> +	snprintf(ids[0].name, sizeof(ids[0].name), "ice.%s", name);
> +
> +	return ids;
> +}
> +
> +/**
> + * ice_ptp_register_auxbus_driver - Register PTP auxiliary bus driver
> + * @pf: Board private structure
> + */
> +static int ice_ptp_register_auxbus_driver(struct ice_pf *pf)
> +{
> +	struct auxiliary_driver *aux_driver;
> +	struct ice_ptp *ptp;
> +	struct device *dev;
> +	char *name;
> +	int err;
> +
> +	ptp = &pf->ptp;
> +	dev = ice_pf_to_dev(pf);
> +	aux_driver = &ptp->ports_owner.aux_driver;
> +	INIT_LIST_HEAD(&ptp->ports_owner.ports);
> +	mutex_init(&ptp->ports_owner.lock);
> +	name = devm_kasprintf(dev, GFP_KERNEL, "ptp_aux_dev_%u_%u_clk%u",
> +			      pf->pdev->bus->number, PCI_SLOT(pf->pdev->devfn),
> +			      ice_get_ptp_src_clock_index(&pf->hw));
> +
> +	aux_driver->name = name;
> +	aux_driver->shutdown = ice_ptp_auxbus_shutdown;
> +	aux_driver->suspend = ice_ptp_auxbus_suspend;
> +	aux_driver->remove = ice_ptp_auxbus_remove;
> +	aux_driver->resume = ice_ptp_auxbus_resume;
> +	aux_driver->probe = ice_ptp_auxbus_probe;
> +	aux_driver->id_table = ice_ptp_auxbus_create_id_table(pf, name);
> +	if (!aux_driver->id_table)
> +		return -ENOMEM;
> +
> +	err = auxiliary_driver_register(aux_driver);
> +	if (err) {
> +		devm_kfree(dev, (void *)aux_driver->id_table);
> +		dev_err(dev, "Failed registering aux_driver, name <%s>\n",
> +			name);
> +	}
> +
> +	return err;
> +}
> +
> +/**
> + * ice_ptp_unregister_auxbus_driver - Unregister PTP auxiliary bus driver
> + * @pf: Board private structure
> + */
> +static void ice_ptp_unregister_auxbus_driver(struct ice_pf *pf)
> +{
> +	struct auxiliary_driver *aux_driver = &pf->ptp.ports_owner.aux_driver;
> +
> +	auxiliary_driver_unregister(aux_driver);
> +	devm_kfree(ice_pf_to_dev(pf), (void *)aux_driver->id_table);
> +
> +	mutex_destroy(&pf->ptp.ports_owner.lock);
> +}
> +
> +/**
>    * ice_ptp_prepare_for_reset - Prepare PTP for reset
>    * @pf: Board private structure
>    */
> @@ -2652,7 +2890,15 @@ static int ice_ptp_init_owner(struct ice_pf *pf)
>   	/* Release the global hardware lock */
>   	ice_ptp_unlock(hw);
>   
> -	if (!ice_is_e810(hw)) {
> +	if (pf->ptp.tx_interrupt_mode == ICE_PTP_TX_INTERRUPT_ALL) {
> +		/* The clock owner for this device type handles the timestamp
> +		 * interrupt for all ports.
> +		 */
> +		ice_ptp_configure_tx_tstamp(pf, true);
> +
> +		/* React on all quads interrupts for E82x */
> +		wr32(hw, PFINT_TSYN_MSK + (0x4 * hw->pf_id), (u32)0x1f);
> +
>   		/* Enable quad interrupts */
>   		err = ice_ptp_tx_ena_intr(pf, true, itr);
>   		if (err)
> @@ -2667,8 +2913,16 @@ static int ice_ptp_init_owner(struct ice_pf *pf)
>   	/* Store the PTP clock index for other PFs */
>   	ice_set_ptp_clock_index(pf);
>   
> -	return 0;
> +	err = ice_ptp_register_auxbus_driver(pf);
> +	if (err) {
> +		dev_err(ice_pf_to_dev(pf), "Failed to register PTP auxbus driver");
> +		goto err_aux;
> +	}
>   
> +	return 0;
> +err_aux:
> +	ice_clear_ptp_clock_index(pf);
> +	ptp_clock_unregister(pf->ptp.clock);
>   err_clk:
>   	pf->ptp.clock = NULL;
>   err_exit:
> @@ -2718,8 +2972,15 @@ static int ice_ptp_init_port(struct ice_pf *pf, struct ice_ptp_port *ptp_port)
>   	case ICE_PHY_E810:
>   		return ice_ptp_init_tx_e810(pf, &ptp_port->tx);
>   	case ICE_PHY_E822:
> +		/* Non-owner PFs don't react to any interrupts on E82x,
> +		 * neither on own quad nor on others
> +		 */
> +		if (!ice_ptp_pf_handles_tx_interrupt(pf)) {
> +			ice_ptp_configure_tx_tstamp(pf, false);
> +			wr32(hw, PFINT_TSYN_MSK + (0x4 * hw->pf_id), (u32)0x0);
> +		}
>   		kthread_init_delayed_work(&ptp_port->ov_work,
> -					  ice_ptp_wait_for_offsets);
> +				ice_ptp_wait_for_offsets);

I believe the original alignment is correct/no need for this change.

>   
>   		return ice_ptp_init_tx_e822(pf, &ptp_port->tx,
>   					    ptp_port->port_num);
> @@ -2729,6 +2990,102 @@ static int ice_ptp_init_port(struct ice_pf *pf, struct ice_ptp_port *ptp_port)
>   }
>   
>   /**
> + * ice_ptp_release_auxbus_device
> + * @dev: device that utilizes the auxbus
> + */
> +static void ice_ptp_release_auxbus_device(struct device *dev)
> +{
> +	/* Doing nothing here, but handle to auxbux device must be satisfied */
> +}
> +
> +/**
> + * ice_ptp_create_auxbus_device - Create PTP auxiliary bus device
> + * @pf: Board private structure
> + */
> +static int ice_ptp_create_auxbus_device(struct ice_pf *pf)
> +{
> +	struct auxiliary_device *aux_dev;
> +	struct ice_ptp *ptp;
> +	struct device *dev;
> +	char *name;
> +	int err;
> +	u32 id;
> +
> +	ptp = &pf->ptp;
> +	id = ptp->port.port_num;
> +	dev = ice_pf_to_dev(pf);
> +
> +	aux_dev = &ptp->port.aux_dev;
> +
> +	name = devm_kasprintf(dev, GFP_KERNEL, "ptp_aux_dev_%u_%u_clk%u",
> +			      pf->pdev->bus->number, PCI_SLOT(pf->pdev->devfn),
> +			      ice_get_ptp_src_clock_index(&pf->hw));
> +
> +	aux_dev->name = name;
> +	aux_dev->id = id;
> +	aux_dev->dev.release = ice_ptp_release_auxbus_device;
> +	aux_dev->dev.parent = ice_pf_to_dev(pf);

You already have dev as a local var.

> +
> +	err = auxiliary_device_init(aux_dev);
> +	if (err)
> +		goto aux_err;
> +
> +	err = auxiliary_device_add(aux_dev);
> +	if (err) {
> +		auxiliary_device_uninit(aux_dev);
> +		goto aux_err;
> +	}
> +
> +	return 0;
> +aux_err:
> +	dev_err(ice_pf_to_dev(pf), "Failed to create PTP auxiliary bus device <%s>\n",
> +		name);

Same here.

> +	devm_kfree(dev, name);
> +	return err;
> +}
> +
> +/**
> + * ice_ptp_remove_auxbus_device - Remove PTP auxiliary bus device
> + * @pf: Board private structure
> + */
> +static void ice_ptp_remove_auxbus_device(struct ice_pf *pf)
> +{
> +	struct auxiliary_device *aux_dev = &pf->ptp.port.aux_dev;
> +
> +	auxiliary_device_delete(aux_dev);
> +	auxiliary_device_uninit(aux_dev);
> +
> +	memset(aux_dev, 0, sizeof(*aux_dev));
> +}
> +
> +/**
> + * ice_ptp_init_tx_interrupt_mode - Initialize device Tx interrupt mode
> + * @pf: Board private structure
> + *
> + * Initialize the Tx timestamp interrupt mode for this device. For most device
> + * types, each PF processes the interrupt and manages its own timestamps. For
> + * E822-based devices, only the clock owner processes the timestamps. Other
> + * PFs disable the interrupt and do not process their own timestamps.
> + */
> +static void ice_ptp_init_tx_interrupt_mode(struct ice_pf *pf)
> +{
> +	switch (pf->hw.phy_model) {
> +	case ICE_PHY_E822:
> +		/* E822 based PHY has the clock owner process the interrupt
> +		 * for all ports.
> +		 */
> +		if (ice_pf_src_tmr_owned(pf))
> +			pf->ptp.tx_interrupt_mode = ICE_PTP_TX_INTERRUPT_ALL;
> +		else
> +			pf->ptp.tx_interrupt_mode = ICE_PTP_TX_INTERRUPT_NONE;
> +		break;
> +	default:
> +		/* other PHY types handle their own Tx interrupt */
> +		pf->ptp.tx_interrupt_mode = ICE_PTP_TX_INTERRUPT_SELF;
> +	}
> +}
> +
> +/**
>    * ice_ptp_init - Initialize PTP hardware clock support
>    * @pf: Board private structure
>    *
> @@ -2748,10 +3105,12 @@ void ice_ptp_init(struct ice_pf *pf)
>   
>   	ice_ptp_init_phy_model(hw);
>   
> +	ice_ptp_init_tx_interrupt_mode(pf);
> +
>   	/* If this function owns the clock hardware, it must allocate and
>   	 * configure the PTP clock device to represent it.
>   	 */
> -	if (hw->func_caps.ts_func_info.src_tmr_owned) {
> +	if (ice_pf_src_tmr_owned(pf)) {
>   		err = ice_ptp_init_owner(pf);
>   		if (err)
>   			goto err;
> @@ -2770,6 +3129,10 @@ void ice_ptp_init(struct ice_pf *pf)
>   	if (err)
>   		goto err;
>   
> +	err = ice_ptp_create_auxbus_device(pf);
> +	if (err)
> +		goto err;
> +
>   	dev_info(ice_pf_to_dev(pf), "PTP init successful\n");
>   	return;
>   
> @@ -2798,6 +3161,8 @@ void ice_ptp_release(struct ice_pf *pf)
>   	/* Disable timestamping for both Tx and Rx */
>   	ice_ptp_cfg_timestamp(pf, false);
>   
> +	ice_ptp_remove_auxbus_device(pf);
> +
>   	ice_ptp_release_tx_tracker(pf, &pf->ptp.port.tx);
>   
>   	clear_bit(ICE_FLAG_PTP, pf->flags);
> @@ -2821,5 +3186,7 @@ void ice_ptp_release(struct ice_pf *pf)
>   	ptp_clock_unregister(pf->ptp.clock);
>   	pf->ptp.clock = NULL;
>   
> +	ice_ptp_unregister_auxbus_driver(pf);
> +
>   	dev_info(ice_pf_to_dev(pf), "Removed PTP clock\n");
>   }
> diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.h b/drivers/net/ethernet/intel/ice/ice_ptp.h
> index 995a570..d94c223 100644
> --- a/drivers/net/ethernet/intel/ice/ice_ptp.h
> +++ b/drivers/net/ethernet/intel/ice/ice_ptp.h
> @@ -157,7 +157,9 @@ struct ice_ptp_tx {
>    * ready for PTP functionality. It is used to track the port initialization
>    * and determine when the port's PHY offset is valid.
>    *
> + * @list_member: list member structure of auxiliary device
>    * @tx: Tx timestamp tracking for this port
> + * @aux_dev: auxiliary device associated with this port
>    * @ov_work: delayed work task for tracking when PHY offset is valid
>    * @ps_lock: mutex used to protect the overall PTP PHY start procedure
>    * @link_up: indicates whether the link is up
> @@ -165,7 +167,9 @@ struct ice_ptp_tx {
>    * @port_num: the port number this structure represents
>    */
>   struct ice_ptp_port {
> +	struct list_head list_member;
>   	struct ice_ptp_tx tx;
> +	struct auxiliary_device aux_dev;
>   	struct kthread_delayed_work ov_work;
>   	struct mutex ps_lock; /* protects overall PTP PHY start procedure */
>   	bool link_up;
> @@ -173,11 +177,35 @@ struct ice_ptp_port {
>   	u8 port_num;
>   };
>   
> +enum ice_ptp_tx_interrupt {
> +	ICE_PTP_TX_INTERRUPT_NONE = 0,
> +	ICE_PTP_TX_INTERRUPT_SELF,
> +	ICE_PTP_TX_INTERRUPT_ALL,
> +};
> +
> +/**
> + * struct ice_ptp_port_owner - data used to handle the PTP clock owner info
> + *
> + * This structure contains data necessary for the PTP clock owner to correctly
> + * handle the timestamping feature for all attached ports.
> + *
> + * @aux_driver: the structure carring the auxiliary driver information
> + * @ports: list of porst handled by this port owner
> + * @lock: protect access to ports list
> + */
> +struct ice_ptp_port_owner {
> +	struct auxiliary_driver aux_driver;
> +	struct list_head ports;
> +	struct mutex lock;
> +};
> +
>   #define GLTSYN_TGT_H_IDX_MAX		4
>   
>   /**
>    * struct ice_ptp - data used for integrating with CONFIG_PTP_1588_CLOCK
> + * @tx_interrupt_mode: the TX interrupt mode for the PTP clock
>    * @port: data for the PHY port initialization procedure
> + * @ports_owner: data for the auxiliary driver owner
>    * @work: delayed work function for periodic tasks
>    * @cached_phc_time: a cached copy of the PHC time for timestamp extension
>    * @cached_phc_jiffies: jiffies when cached_phc_time was last updated
> @@ -197,7 +225,9 @@ struct ice_ptp_port {
>    * @late_cached_phc_updates: number of times cached PHC update is late
>    */
>   struct ice_ptp {
> +	enum ice_ptp_tx_interrupt tx_interrupt_mode;
>   	struct ice_ptp_port port;
> +	struct ice_ptp_port_owner ports_owner;
>   	struct kthread_delayed_work work;
>   	u64 cached_phc_time;
>   	unsigned long cached_phc_jiffies;
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan