MIME-Version: 1.0
References: <20230307113621.64153-1-gautam.dawar@amd.com> <20230307113621.64153-2-gautam.dawar@amd.com>
In-Reply-To: <20230307113621.64153-2-gautam.dawar@amd.com>
From:   Jason Wang <jasowang@redhat.com>
Date:   Fri, 10 Mar 2023 13:04:14 +0800
Message-ID: <CACGkMEubKv-CGgTdTbt=Ja=pbazXT3nOGY9f_VtRwrOsmf8-rw@mail.gmail.com>
Subject: Re: [PATCH net-next v2 01/14] sfc: add function personality support
 for EF100 devices
To:     Gautam Dawar <gautam.dawar@amd.com>
Cc:     linux-net-drivers@amd.com, Edward Cree <ecree.xilinx@gmail.com>,
        Martin Habets <habetsm.xilinx@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Richard Cochran <richardcochran@gmail.com>,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        eperezma@redhat.com, harpreet.anand@amd.com, tanuj.kamde@amd.com,
        koushik.dutta@amd.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Mar 7, 2023 at 7:36=E2=80=AFPM Gautam Dawar <gautam.dawar@amd.com> =
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@gmail.com>
> Signed-off-by: Martin Habets <habetsm.xilinx@gmail.com>
> Signed-off-by: Gautam Dawar <gautam.dawar@amd.com>
> ---
>  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 =3D 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 =3D 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 =3D STATE_PROBED;
> -       rc =3D ef100_probe_netdev(probe_data);
> +       rc =3D 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 (whi=
ch
> + * 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[] =3D {
> +       [EF100_BAR_CONFIG_EF100] =3D {
> +               .init =3D ef100_probe_netdev,
> +               .fini =3D ef100_remove_netdev
> +       },
> +#ifdef CONFIG_SFC_VDPA
> +       [EF100_BAR_CONFIG_VDPA] =3D {
> +               .init =3D NULL,
> +               .fini =3D NULL
> +       },
> +#endif
> +       [EF100_BAR_CONFIG_NONE] =3D {
> +               .init =3D NULL,
> +               .fini =3D 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. It might work like, during PCI probe, register an aux device
ef100_nic.net . Then we will have two drivers that could be bound:

1) netdev
2) vdpa

So the bar config setup could be delayed to the auxiliary driver
probe. And we can register the mgmt device during probing the vdpa aux
device. This complies with the sysfs based mgmt interface for driver
core and allows more policy to be added on top e.g autoprobe/override
etc.

Thanks