Message-ID: <071329fe-7215-235c-06b7-f17bf69d872b@redhat.com>
Date:   Wed, 15 Mar 2023 13:11:23 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.8.0
Subject: Re: [PATCH net-next v2 01/14] sfc: add function personality support
 for EF100 devices
Content-Language: en-US
To:     Gautam Dawar <gautam.dawar@amd.com>, linux-net-drivers@amd.com,
        Edward Cree <ecree.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
References: <20230307113621.64153-1-gautam.dawar@amd.com>
 <20230307113621.64153-2-gautam.dawar@amd.com>
 <CACGkMEubKv-CGgTdTbt=Ja=pbazXT3nOGY9f_VtRwrOsmf8-rw@mail.gmail.com>
 <ZA8OBEDECFI4grXG@gmail.com>
From:   Jason Wang <jasowang@redhat.com>
In-Reply-To: <ZA8OBEDECFI4grXG@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


在 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@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 = 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