[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <071329fe-7215-235c-06b7-f17bf69d872b@redhat.com>
Date: Wed, 15 Mar 2023 13:11:23 +0800
From: Jason Wang <jasowang@...hat.com>
To: Gautam Dawar <gautam.dawar@....com>, linux-net-drivers@....com,
Edward Cree <ecree.xilinx@...il.com>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
Richard Cochran <richardcochran@...il.com>,
linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
eperezma@...hat.com, harpreet.anand@....com, tanuj.kamde@....com,
koushik.dutta@....com
Subject: Re: [PATCH net-next v2 01/14] sfc: add function personality support
for EF100 devices
在 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@....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@...il.com>
>>> Signed-off-by: Martin Habets <habetsm.xilinx@...il.com>
>>> Signed-off-by: Gautam Dawar <gautam.dawar@....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 = 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
Powered by blists - more mailing lists