| Message ID | 20221125071958.276454-1-shaozhengchao@huawei.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] sfc: fix error process in efx_ef100_pci_sriov_enable() | expand |
On 25/11/2022 07:19, Zhengchao Shao wrote: > There are two issues in efx_ef100_pci_sriov_enable(): > 1. When it doesn't have MAE Privilege, it doesn't disable pci sriov. > 2. When creating VF successfully, it should return vf nums instead of 0. A single patch should do one thing. If these two issues were valid, they ought to be fixed separately by two commits. > Compiled test only. > > Fixes: 08135eecd07f ("sfc: add skeleton ef100 VF representors") > Fixes: 78a9b3c47bef ("sfc: add EF100 VF support via a write to sriov_numvfs") > Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> > --- > drivers/net/ethernet/sfc/ef100_sriov.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/sfc/ef100_sriov.c b/drivers/net/ethernet/sfc/ef100_sriov.c > index 94bdbfcb47e8..adf7fb09940e 100644 > --- a/drivers/net/ethernet/sfc/ef100_sriov.c > +++ b/drivers/net/ethernet/sfc/ef100_sriov.c > @@ -25,15 +25,17 @@ static int efx_ef100_pci_sriov_enable(struct efx_nic *efx, int num_vfs) > if (rc) > goto fail1; > > - if (!nic_data->grp_mae) > + if (!nic_data->grp_mae) { > + pci_disable_sriov(dev); > return 0; > + } NACK to this part; if we don't have MAE privilege, that means someone else (e.g. the embedded SoC) is in charge of the MAE and is responsible for configuring switching behaviour for any VFs we create. Thus, the existing behaviour — create the VFs without creating any corresponding representors — is intended. > > for (i = 0; i < num_vfs; i++) { > rc = efx_ef100_vfrep_create(efx, i); > if (rc) > goto fail2; > } > - return 0; > + return num_vfs; NACK to this too: this is not returned directly to the PCI core but to ef100_pci_sriov_configure(), which already does the translation from 0 (success) to num_vfs. So changing it here is unnecessary. -ed > > fail2: > list_for_each_entry_safe(efv, next, &efx->vf_reps, list) >
On Fri, Nov 25, 2022 at 03:19:58PM +0800, Zhengchao Shao wrote: > There are two issues in efx_ef100_pci_sriov_enable(): > 1. When it doesn't have MAE Privilege, it doesn't disable pci sriov. > 2. When creating VF successfully, it should return vf nums instead of 0. This function returns 0 on success, or an error code. It is the higher level function ef100_pci_sriov_configure() in ef100.c that returns num_vfs if things are ok. Martin > Compiled test only. > > Fixes: 08135eecd07f ("sfc: add skeleton ef100 VF representors") > Fixes: 78a9b3c47bef ("sfc: add EF100 VF support via a write to sriov_numvfs") > Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> > --- > drivers/net/ethernet/sfc/ef100_sriov.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/sfc/ef100_sriov.c b/drivers/net/ethernet/sfc/ef100_sriov.c > index 94bdbfcb47e8..adf7fb09940e 100644 > --- a/drivers/net/ethernet/sfc/ef100_sriov.c > +++ b/drivers/net/ethernet/sfc/ef100_sriov.c > @@ -25,15 +25,17 @@ static int efx_ef100_pci_sriov_enable(struct efx_nic *efx, int num_vfs) > if (rc) > goto fail1; > > - if (!nic_data->grp_mae) > + if (!nic_data->grp_mae) { > + pci_disable_sriov(dev); > return 0; > + } > > for (i = 0; i < num_vfs; i++) { > rc = efx_ef100_vfrep_create(efx, i); > if (rc) > goto fail2; > } > - return 0; > + return num_vfs; > > fail2: > list_for_each_entry_safe(efv, next, &efx->vf_reps, list) > -- > 2.17.1
On 2022/11/29 16:19, Edward Cree wrote: > On 25/11/2022 07:19, Zhengchao Shao wrote: >> There are two issues in efx_ef100_pci_sriov_enable(): >> 1. When it doesn't have MAE Privilege, it doesn't disable pci sriov. >> 2. When creating VF successfully, it should return vf nums instead of 0. > > A single patch should do one thing. If these two issues were > valid, they ought to be fixed separately by two commits. > >> Compiled test only. >> >> Fixes: 08135eecd07f ("sfc: add skeleton ef100 VF representors") >> Fixes: 78a9b3c47bef ("sfc: add EF100 VF support via a write to sriov_numvfs") >> Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> >> --- >> drivers/net/ethernet/sfc/ef100_sriov.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/ethernet/sfc/ef100_sriov.c b/drivers/net/ethernet/sfc/ef100_sriov.c >> index 94bdbfcb47e8..adf7fb09940e 100644 >> --- a/drivers/net/ethernet/sfc/ef100_sriov.c >> +++ b/drivers/net/ethernet/sfc/ef100_sriov.c >> @@ -25,15 +25,17 @@ static int efx_ef100_pci_sriov_enable(struct efx_nic *efx, int num_vfs) >> if (rc) >> goto fail1; >> >> - if (!nic_data->grp_mae) >> + if (!nic_data->grp_mae) { >> + pci_disable_sriov(dev); >> return 0; >> + } > > NACK to this part; if we don't have MAE privilege, that means > someone else (e.g. the embedded SoC) is in charge of the MAE > and is responsible for configuring switching behaviour for any > VFs we create. > Thus, the existing behaviour — create the VFs without creating > any corresponding representors — is intended. > >> >> for (i = 0; i < num_vfs; i++) { >> rc = efx_ef100_vfrep_create(efx, i); >> if (rc) >> goto fail2; >> } >> - return 0; >> + return num_vfs; > > NACK to this too: this is not returned directly to the PCI > core but to ef100_pci_sriov_configure(), which already does > the translation from 0 (success) to num_vfs. So changing it > here is unnecessary. > > -ed > Thank you for your review. My misunderstood. You are right. >> >> fail2: >> list_for_each_entry_safe(efv, next, &efx->vf_reps, list) >>
diff --git a/drivers/net/ethernet/sfc/ef100_sriov.c b/drivers/net/ethernet/sfc/ef100_sriov.c index 94bdbfcb47e8..adf7fb09940e 100644 --- a/drivers/net/ethernet/sfc/ef100_sriov.c +++ b/drivers/net/ethernet/sfc/ef100_sriov.c @@ -25,15 +25,17 @@ static int efx_ef100_pci_sriov_enable(struct efx_nic *efx, int num_vfs) if (rc) goto fail1; - if (!nic_data->grp_mae) + if (!nic_data->grp_mae) { + pci_disable_sriov(dev); return 0; + } for (i = 0; i < num_vfs; i++) { rc = efx_ef100_vfrep_create(efx, i); if (rc) goto fail2; } - return 0; + return num_vfs; fail2: list_for_each_entry_safe(efv, next, &efx->vf_reps, list)
There are two issues in efx_ef100_pci_sriov_enable(): 1. When it doesn't have MAE Privilege, it doesn't disable pci sriov. 2. When creating VF successfully, it should return vf nums instead of 0. Compiled test only. Fixes: 08135eecd07f ("sfc: add skeleton ef100 VF representors") Fixes: 78a9b3c47bef ("sfc: add EF100 VF support via a write to sriov_numvfs") Signed-off-by: Zhengchao Shao <shaozhengchao@huawei.com> --- drivers/net/ethernet/sfc/ef100_sriov.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)