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

Simon Horman
2023-04-10T10:24:01Z

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@amd.com>

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
>