Re: [PATCH net-next v4 06/14] sfc: implement vDPA management device operations

[Simon Horman]

On Fri, Apr 07, 2023 at 01:40:07PM +0530, Gautam Dawar wrote:
> To allow vDPA device creation and deletion, add a vDPA management
> device per function. Currently, the vDPA devices can be created
> only on a VF. Also, for now only network class of vDPA devices
> are supported.
> 
> Signed-off-by: Gautam Dawar <gautam.dawar@xxxxxxx>

Hi Gautam,

some minor feedback from my side is inline.

> diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c

...

> @@ -1286,13 +1286,35 @@ int ef100_probe_netdev_pf(struct efx_nic *efx)
>  
>  int ef100_probe_vf(struct efx_nic *efx)
>  {
> -	return ef100_probe_main(efx);
> +	struct ef100_nic_data *nic_data __maybe_unused;
> +	int rc;
> +
> +	rc = ef100_probe_main(efx);
> +	if (rc)
> +		return rc;
> +
> +#ifdef CONFIG_SFC_VDPA
> +	nic_data = efx->nic_data;
> +	if (nic_data->vdpa_supported) {
> +		rc = ef100_vdpa_register_mgmtdev(efx);
> +		if (rc)
> +			pci_warn(efx->pci_dev,
> +				 "register_mgmtdev failed, rc: %d\n", rc);
> +	}
> +#endif

I think it would be nicer to factor the #ifdef coded out, perhaps into a
helper like this (completely untested!).

void ef100_probe_vf_vdpa(struct efx_nic *efx)
{
#ifdef CONFIG_SFC_VDPA
	struct ef100_nic_data *nic_data = efx->nic_data;

	nic_data = efx->nic_data;
	if (nic_data->vdpa_supported) {
		int rc = ef100_vdpa_register_mgmtdev(efx);
		if (rc)
			pci_warn(efx->pci_dev,
				 "register_mgmtdev failed, rc: %d\n", rc);
	}
#endif
}

Or perhaps an approach similar to the one you have taken,
but using IS_ENABLED() rather than #ifdef

> +
> +	return 0;
>  }
>  
>  void ef100_remove(struct efx_nic *efx)
>  {
>  	struct ef100_nic_data *nic_data = efx->nic_data;
>  
> +#ifdef CONFIG_SFC_VDPA
> +	if (nic_data->vdpa_supported)

nic_data is dereferenced here.
But a bit futher down this function there is a check for nic_data being NULL.

Reported by Smatch as:

drivers/net/ethernet/sfc/ef100_nic.c:1325 ef100_remove() warn: variable dereferenced before check 'nic_data' (see line 1314)

> +		ef100_vdpa_unregister_mgmtdev(efx);
> +#endif

Again, I think it would be nice to factor this #ifdef out somehow.

> +
>  	if (IS_ENABLED(CONFIG_SFC_SRIOV) && efx->mae) {
>  		efx_ef100_fini_reps(efx);
>  		efx_fini_mae(efx);
> diff --git a/drivers/net/ethernet/sfc/ef100_nic.h b/drivers/net/ethernet/sfc/ef100_nic.h
> index a01e9d643ccd..e63ea555116c 100644
> --- a/drivers/net/ethernet/sfc/ef100_nic.h
> +++ b/drivers/net/ethernet/sfc/ef100_nic.h
> @@ -69,6 +69,13 @@ enum ef100_bar_config {
>  	EF100_BAR_CONFIG_VDPA,
>  };
>  
> +#ifdef CONFIG_SFC_VDPA
> +enum ef100_vdpa_class {
> +	EF100_VDPA_CLASS_NONE,
> +	EF100_VDPA_CLASS_NET,
> +};
> +#endif
> +

I don't think there is any need to guard this with an #ifdef

>  struct ef100_nic_data {
>  	struct efx_nic *efx;
>  	struct efx_buffer mcdi_buf;
> @@ -76,9 +83,11 @@ struct ef100_nic_data {
>  	u32 datapath_caps2;
>  	u32 datapath_caps3;
>  	unsigned int pf_index;
> +	unsigned int vf_index;
>  	u16 warm_boot_count;
>  #ifdef CONFIG_SFC_VDPA
>  	bool vdpa_supported; /* true if vdpa is supported on this PCIe FN */
> +	enum ef100_vdpa_class vdpa_class;
>  #endif
>  	u8 port_id[ETH_ALEN];
>  	DECLARE_BITMAP(evq_phases, EFX_MAX_CHANNELS);

...

> diff --git a/drivers/net/ethernet/sfc/ef100_vdpa.h b/drivers/net/ethernet/sfc/ef100_vdpa.h

...

> +static inline bool efx_vdpa_is_little_endian(struct ef100_vdpa_nic *vdpa_nic)
> +{
> +	return virtio_legacy_is_little_endian() ||
> +		(vdpa_nic->features & (1ULL << VIRTIO_F_VERSION_1));
> +}

Using BIT_ULL seems appropriate here.

...

> diff --git a/drivers/net/ethernet/sfc/net_driver.h b/drivers/net/ethernet/sfc/net_driver.h
> index 3dc9eae5a81d..1da71deac71c 100644
> --- a/drivers/net/ethernet/sfc/net_driver.h
> +++ b/drivers/net/ethernet/sfc/net_driver.h
> @@ -1090,6 +1090,12 @@ struct efx_nic {
>  	int rx_packet_len_offset;
>  	int rx_packet_ts_offset;
>  	bool rx_scatter;
> +#ifdef CONFIG_SFC_VDPA
> +	/** @mgmt_dev: vDPA Management device */
> +	struct vdpa_mgmt_dev *mgmt_dev;
> +	/** @vdpa_nic: vDPA device structure (EF100) */
> +	struct ef100_vdpa_nic *vdpa_nic;
> +#endif

I think the commends belong in the kdoc immediately above the structure
definition. Or is this the way it is done for conditionally present
fields (I don't know) ?

>  	struct efx_rss_context rss_context;
>  	struct mutex rss_lock;
>  	u32 vport_id;
> -- 
> 2.30.1
>