Messages in this thread Patch in this message | | | Date | Mon, 10 Apr 2023 12:22:42 +0200 | From | Simon Horman <> | Subject | Re: [PATCH net-next v4 06/14] sfc: implement vDPA management device operations |
| |
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 >
| |