Re: [PATCH net-next v2 01/14] sfc: add function personality support for EF100 devices

[Jason Wang]

在 2023/3/13 19:50, Martin Habets 写道:
> On Fri, Mar 10, 2023 at 01:04:14PM +0800, Jason Wang wrote:
> > On Tue, Mar 7, 2023 at 7:36 PM Gautam Dawar <gautam.dawar@xxxxxxx> wrote:
> > > A function personality defines the location and semantics of
> > > registers in the BAR. EF100 NICs allow different personalities
> > > of a PCIe function and changing it at run-time. A total of three
> > > function personalities are defined as of now: EF100, vDPA and
> > > None with EF100 being the default.
> > > For now, vDPA net devices can be created on a EF100 virtual
> > > function and the VF personality will be changed to vDPA in the
> > > process.
> > >
> > > Co-developed-by: Martin Habets <habetsm.xilinx@xxxxxxxxx>
> > > Signed-off-by: Martin Habets <habetsm.xilinx@xxxxxxxxx>
> > > Signed-off-by: Gautam Dawar <gautam.dawar@xxxxxxx>
> > > ---
> > >   drivers/net/ethernet/sfc/ef100.c     |  6 +-
> > >   drivers/net/ethernet/sfc/ef100_nic.c | 98 +++++++++++++++++++++++++++-
> > >   drivers/net/ethernet/sfc/ef100_nic.h | 11 ++++
> > >   3 files changed, 111 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/sfc/ef100.c b/drivers/net/ethernet/sfc/ef100.c
> > > index 71aab3d0480f..c1c69783db7b 100644
> > > --- a/drivers/net/ethernet/sfc/ef100.c
> > > +++ b/drivers/net/ethernet/sfc/ef100.c
> > > @@ -429,8 +429,7 @@ static void ef100_pci_remove(struct pci_dev *pci_dev)
> > >          if (!efx)
> > >                  return;
> > >
> > > -       probe_data = container_of(efx, struct efx_probe_data, efx);
> > > -       ef100_remove_netdev(probe_data);
> > > +       efx_ef100_set_bar_config(efx, EF100_BAR_CONFIG_NONE);
> > >   #ifdef CONFIG_SFC_SRIOV
> > >          efx_fini_struct_tc(efx);
> > >   #endif
> > > @@ -443,6 +442,7 @@ static void ef100_pci_remove(struct pci_dev *pci_dev)
> > >          pci_disable_pcie_error_reporting(pci_dev);
> > >
> > >          pci_set_drvdata(pci_dev, NULL);
> > > +       probe_data = container_of(efx, struct efx_probe_data, efx);
> > >          efx_fini_struct(efx);
> > >          kfree(probe_data);
> > >   };
> > > @@ -508,7 +508,7 @@ static int ef100_pci_probe(struct pci_dev *pci_dev,
> > >                  goto fail;
> > >
> > >          efx->state = STATE_PROBED;
> > > -       rc = ef100_probe_netdev(probe_data);
> > > +       rc = efx_ef100_set_bar_config(efx, EF100_BAR_CONFIG_EF100);
> > >          if (rc)
> > >                  goto fail;
> > >
> > > diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c
> > > index 4dc643b0d2db..8cbe5e0f4bdf 100644
> > > --- a/drivers/net/ethernet/sfc/ef100_nic.c
> > > +++ b/drivers/net/ethernet/sfc/ef100_nic.c
> > > @@ -772,6 +772,99 @@ static int efx_ef100_get_base_mport(struct efx_nic *efx)
> > >          return 0;
> > >   }
> > >
> > > +/* BAR configuration.
> > > + * To change BAR configuration, tear down the current configuration (which
> > > + * leaves the hardware in the PROBED state), and then initialise the new
> > > + * BAR state.
> > > + */
> > > +struct ef100_bar_config_ops {
> > > +       int (*init)(struct efx_probe_data *probe_data);
> > > +       void (*fini)(struct efx_probe_data *probe_data);
> > > +};
> > > +
> > > +static const struct ef100_bar_config_ops bar_config_ops[] = {
> > > +       [EF100_BAR_CONFIG_EF100] = {
> > > +               .init = ef100_probe_netdev,
> > > +               .fini = ef100_remove_netdev
> > > +       },
> > > +#ifdef CONFIG_SFC_VDPA
> > > +       [EF100_BAR_CONFIG_VDPA] = {
> > > +               .init = NULL,
> > > +               .fini = NULL
> > > +       },
> > > +#endif
> > > +       [EF100_BAR_CONFIG_NONE] = {
> > > +               .init = NULL,
> > > +               .fini = NULL
> > > +       },
> > > +};
> > This looks more like a mini bus implementation. I wonder if we can
> > reuse an auxiliary bus here which is more user friendly for management
> > tools.
> When we were in the design phase of vDPA for EF100 it was still called
> virtbus, and the virtbus discussion was in full swing at that time.
> We could not afford to add risk to the project by depending on it, as
> it might not have been merged at all.


Right.


> If we were doing the same design now I would definitely consider using
> the auxiliary bus.
>
> Martin


But it's not late to do the change now. Auxiliary bus has been used by a 
lot of devices (even with vDPA device). The change looks not too 
complicated.

This looks more scalable and convenient for management layer.

Thanks