| Message ID | 20230814132721.26608-1-ilpo.jarvinen@linux.intel.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [1/1] net/mlx5: Convert PCI error values to generic errnos | expand |
On Mon, Aug 14, 2023 at 04:27:20PM +0300, Ilpo Järvinen wrote: > mlx5_pci_link_toggle() returns mix PCI specific error codes and generic > errnos. > > Convert the PCI specific error values to generic errno using > pcibios_err_to_errno() before returning them. > > Fixes: eabe8e5e88f5 ("net/mlx5: Handle sync reset now event") > Fixes: 212b4d7251c1 ("net/mlx5: Wait for firmware to enable CRS before pci_restore_state") > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > --- > > Maintainers beware, this will conflict with read+write -> set/clear_word > fixes in pci.git/pcie-rmw. As such, it might be the easiest for Bjorn to > take it instead of net people. I provisionally rebased and applied it on pci/pcie-rmw. Take a look and make sure I didn't botch it -- I also found a case in mlx5_check_dev_ids() that looks like it needs the same conversion. The commit as applied is below. If networking folks would prefer to take this, let me know and I can drop it. > I wonder if these PCIBIOS_* error codes are useful at all? There's 1:1 > mapping into errno values so no information loss if the functions would just > return errnos directly. Perhaps this is just legacy nobody has bothered to > remove? If nobody opposes, I could take a look at getting rid of them. I don't think the PCIBIOS error codes are very useful outside of arch/x86. They're returned by x86 PCIBIOS functions, and I think we still use those calls, but I don't think there's value in exposing the x86 error codes outside arch/x86. Looks like a big job to clean it up though ;) > --- > drivers/net/ethernet/mellanox/mlx5/core/fw_reset.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fw_reset.c b/drivers/net/ethernet/mellanox/mlx5/core/fw_reset.c > index 4804990b7f22..0afd9dbfc471 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/fw_reset.c > +++ b/drivers/net/ethernet/mellanox/mlx5/core/fw_reset.c > @@ -371,7 +371,7 @@ static int mlx5_pci_link_toggle(struct mlx5_core_dev *dev) > > err = pci_read_config_word(dev->pdev, PCI_DEVICE_ID, &dev_id); > if (err) > - return err; > + return pcibios_err_to_errno(err); > err = mlx5_check_dev_ids(dev, dev_id); > if (err) > return err; > @@ -386,16 +386,16 @@ static int mlx5_pci_link_toggle(struct mlx5_core_dev *dev) > /* PCI link toggle */ > err = pci_read_config_word(bridge, cap + PCI_EXP_LNKCTL, ®16); > if (err) > - return err; > + return pcibios_err_to_errno(err); > reg16 |= PCI_EXP_LNKCTL_LD; > err = pci_write_config_word(bridge, cap + PCI_EXP_LNKCTL, reg16); > if (err) > - return err; > + return pcibios_err_to_errno(err); > msleep(500); > reg16 &= ~PCI_EXP_LNKCTL_LD; > err = pci_write_config_word(bridge, cap + PCI_EXP_LNKCTL, reg16); > if (err) > - return err; > + return pcibios_err_to_errno(err); > > /* Check link */ > if (!bridge->link_active_reporting) { > @@ -408,7 +408,7 @@ static int mlx5_pci_link_toggle(struct mlx5_core_dev *dev) > do { > err = pci_read_config_word(bridge, cap + PCI_EXP_LNKSTA, ®16); > if (err) > - return err; > + return pcibios_err_to_errno(err); > if (reg16 & PCI_EXP_LNKSTA_DLLLA) > break; > msleep(20); > @@ -426,7 +426,7 @@ static int mlx5_pci_link_toggle(struct mlx5_core_dev *dev) > do { > err = pci_read_config_word(dev->pdev, PCI_DEVICE_ID, ®16); > if (err) > - return err; > + return pcibios_err_to_errno(err); > if (reg16 == dev_id) > break; > msleep(20); commit a48c6af2d2a5 ("net/mlx5: Convert PCI error values to generic errnos") Author: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> Date: Mon Aug 14 16:27:20 2023 +0300 net/mlx5: Convert PCI error values to generic errnos mlx5_pci_link_toggle() returns a mix of PCI-specific error codes and generic errnos. Convert the PCI-specific error values to generic errno using pcibios_err_to_errno() before returning them. Fixes: eabe8e5e88f5 ("net/mlx5: Handle sync reset now event") Fixes: 212b4d7251c1 ("net/mlx5: Wait for firmware to enable CRS before pci_restore_state") Link: https://lore.kernel.org/r/20230814132721.26608-1-ilpo.jarvinen@linux.intel.com Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> [bhelgaas: rebase to pci/pcie-rmw, also convert in mlx5_check_dev_ids()] Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fw_reset.c b/drivers/net/ethernet/mellanox/mlx5/core/fw_reset.c index 99dcbd006357..85a2dfbb5c46 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/fw_reset.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/fw_reset.c @@ -311,7 +311,7 @@ static int mlx5_check_dev_ids(struct mlx5_core_dev *dev, u16 dev_id) list_for_each_entry(sdev, &bridge_bus->devices, bus_list) { err = pci_read_config_word(sdev, PCI_DEVICE_ID, &sdev_id); if (err) - return err; + return pcibios_err_to_errno(err); if (sdev_id != dev_id) { mlx5_core_warn(dev, "unrecognized dev_id (0x%x)\n", sdev_id); return -EPERM; @@ -371,7 +371,7 @@ static int mlx5_pci_link_toggle(struct mlx5_core_dev *dev) err = pci_read_config_word(dev->pdev, PCI_DEVICE_ID, &dev_id); if (err) - return err; + return pcibios_err_to_errno(err); err = mlx5_check_dev_ids(dev, dev_id); if (err) return err; @@ -386,11 +386,11 @@ static int mlx5_pci_link_toggle(struct mlx5_core_dev *dev) /* PCI link toggle */ err = pcie_capability_set_word(bridge, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_LD); if (err) - return err; + return pcibios_err_to_errno(err); msleep(500); err = pcie_capability_clear_word(bridge, PCI_EXP_LNKCTL, PCI_EXP_LNKCTL_LD); if (err) - return err; + return pcibios_err_to_errno(err); /* Check link */ if (!bridge->link_active_reporting) { @@ -403,7 +403,7 @@ static int mlx5_pci_link_toggle(struct mlx5_core_dev *dev) do { err = pci_read_config_word(bridge, cap + PCI_EXP_LNKSTA, ®16); if (err) - return err; + return pcibios_err_to_errno(err); if (reg16 & PCI_EXP_LNKSTA_DLLLA) break; msleep(20); @@ -421,7 +421,7 @@ static int mlx5_pci_link_toggle(struct mlx5_core_dev *dev) do { err = pci_read_config_word(dev->pdev, PCI_DEVICE_ID, ®16); if (err) - return err; + return pcibios_err_to_errno(err); if (reg16 == dev_id) break; msleep(20);
On Mon, 14 Aug 2023, Bjorn Helgaas wrote: > On Mon, Aug 14, 2023 at 04:27:20PM +0300, Ilpo Järvinen wrote: > > mlx5_pci_link_toggle() returns mix PCI specific error codes and generic > > errnos. > > > > Convert the PCI specific error values to generic errno using > > pcibios_err_to_errno() before returning them. > > > > Fixes: eabe8e5e88f5 ("net/mlx5: Handle sync reset now event") > > Fixes: 212b4d7251c1 ("net/mlx5: Wait for firmware to enable CRS before pci_restore_state") > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > > > --- > > > > Maintainers beware, this will conflict with read+write -> set/clear_word > > fixes in pci.git/pcie-rmw. As such, it might be the easiest for Bjorn to > > take it instead of net people. > > I provisionally rebased and applied it on pci/pcie-rmw. Take a look > and make sure I didn't botch it -- Looks okay. > I also found a case in > mlx5_check_dev_ids() that looks like it needs the same conversion. Ah, that where the one of them went (my first version had that fixed inside link_toggle but then when rebasing I didn't realize it had moved into another function). > The commit as applied is below. > > If networking folks would prefer to take this, let me know and I can > drop it. > > > I wonder if these PCIBIOS_* error codes are useful at all? There's 1:1 > > mapping into errno values so no information loss if the functions would just > > return errnos directly. Perhaps this is just legacy nobody has bothered to > > remove? If nobody opposes, I could take a look at getting rid of them. > > I don't think the PCIBIOS error codes are very useful outside of > arch/x86. They're returned by x86 PCIBIOS functions, and I think we > still use those calls, but I don't think there's value in exposing the > x86 error codes outside arch/x86. Looks like a big job to clean it up > though ;) Hmm... Do you mean pci_bios_read/write() in arch/x86/pci/pcbios.c? ...Because those functions are already inconsistent even with themselves, returning either -EINVAL or the PCI BIOS error code (or what I assume that masking of result to yield). And unfortunately, that's far from the only inconsistency within arch PCI read/write func return values...
On Tue, Aug 15, 2023 at 02:31:05PM +0300, Ilpo Järvinen wrote: > On Mon, 14 Aug 2023, Bjorn Helgaas wrote: > > On Mon, Aug 14, 2023 at 04:27:20PM +0300, Ilpo Järvinen wrote: > > > mlx5_pci_link_toggle() returns mix PCI specific error codes and generic > > > errnos. > > > ... > > > I wonder if these PCIBIOS_* error codes are useful at all? > > > There's 1:1 mapping into errno values so no information loss if > > > the functions would just return errnos directly. Perhaps this is > > > just legacy nobody has bothered to remove? If nobody opposes, I > > > could take a look at getting rid of them. > > > > I don't think the PCIBIOS error codes are very useful outside of > > arch/x86. They're returned by x86 PCIBIOS functions, and I think we > > still use those calls, but I don't think there's value in exposing the > > x86 error codes outside arch/x86. Looks like a big job to clean it up > > though ;) > > Hmm... Do you mean pci_bios_read/write() in arch/x86/pci/pcbios.c? > ...Because those functions are already inconsistent even with themselves, > returning either -EINVAL or the PCI BIOS error code (or what I assume that > masking of result to yield). I didn't look up the code; I just think we still use those PCIBIOS calls in some cases, so we need to know how to interpret the error values returned by the BIOS. IMHO it would be ideal if those PCIBIOS errors got converted to Linux errnos before generic code saw them. Bjorn
On 14 Aug 17:32, Bjorn Helgaas wrote: >On Mon, Aug 14, 2023 at 04:27:20PM +0300, Ilpo Järvinen wrote: >> mlx5_pci_link_toggle() returns mix PCI specific error codes and generic >> errnos. >> >> Convert the PCI specific error values to generic errno using >> pcibios_err_to_errno() before returning them. >> >> Fixes: eabe8e5e88f5 ("net/mlx5: Handle sync reset now event") >> Fixes: 212b4d7251c1 ("net/mlx5: Wait for firmware to enable CRS before pci_restore_state") >> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> >> >> --- >> >> Maintainers beware, this will conflict with read+write -> set/clear_word >> fixes in pci.git/pcie-rmw. As such, it might be the easiest for Bjorn to >> take it instead of net people. > >I provisionally rebased and applied it on pci/pcie-rmw. Take a look >and make sure I didn't botch it -- I also found a case in >mlx5_check_dev_ids() that looks like it needs the same conversion. > >The commit as applied is below. > >If networking folks would prefer to take this, let me know and I can >drop it. > I Just took this patch into my mlx5 submission queue and sent it to netdev tree, please drop it from your tree. Thanks for the patch, Saeed.
On Wed, Aug 16, 2023 at 02:45:15PM -0700, Saeed Mahameed wrote: > On 14 Aug 17:32, Bjorn Helgaas wrote: > > On Mon, Aug 14, 2023 at 04:27:20PM +0300, Ilpo Järvinen wrote: > > > mlx5_pci_link_toggle() returns mix PCI specific error codes and generic > > > errnos. > > > > > > Convert the PCI specific error values to generic errno using > > > pcibios_err_to_errno() before returning them. > > > > > > Fixes: eabe8e5e88f5 ("net/mlx5: Handle sync reset now event") > > > Fixes: 212b4d7251c1 ("net/mlx5: Wait for firmware to enable CRS before pci_restore_state") > > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > > > > > --- > > > > > > Maintainers beware, this will conflict with read+write -> set/clear_word > > > fixes in pci.git/pcie-rmw. As such, it might be the easiest for Bjorn to > > > take it instead of net people. > > > > I provisionally rebased and applied it on pci/pcie-rmw. Take a look > > and make sure I didn't botch it -- I also found a case in > > mlx5_check_dev_ids() that looks like it needs the same conversion. > > > > The commit as applied is below. > > > > If networking folks would prefer to take this, let me know and I can > > drop it. > > I Just took this patch into my mlx5 submission queue and sent it to netdev > tree, please drop it from your tree. OK, will do. Note that this will generate a conflict between netdev and the PCI tree during the merge window. Bjorn
On 16 Aug 17:04, Bjorn Helgaas wrote: >On Wed, Aug 16, 2023 at 02:45:15PM -0700, Saeed Mahameed wrote: >> On 14 Aug 17:32, Bjorn Helgaas wrote: >> > On Mon, Aug 14, 2023 at 04:27:20PM +0300, Ilpo Järvinen wrote: >> > > mlx5_pci_link_toggle() returns mix PCI specific error codes and generic >> > > errnos. >> > > >> > > Convert the PCI specific error values to generic errno using >> > > pcibios_err_to_errno() before returning them. >> > > >> > > Fixes: eabe8e5e88f5 ("net/mlx5: Handle sync reset now event") >> > > Fixes: 212b4d7251c1 ("net/mlx5: Wait for firmware to enable CRS before pci_restore_state") >> > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> >> > > >> > > --- >> > > >> > > Maintainers beware, this will conflict with read+write -> set/clear_word >> > > fixes in pci.git/pcie-rmw. As such, it might be the easiest for Bjorn to >> > > take it instead of net people. >> > >> > I provisionally rebased and applied it on pci/pcie-rmw. Take a look >> > and make sure I didn't botch it -- I also found a case in >> > mlx5_check_dev_ids() that looks like it needs the same conversion. >> > >> > The commit as applied is below. >> > >> > If networking folks would prefer to take this, let me know and I can >> > drop it. >> >> I Just took this patch into my mlx5 submission queue and sent it to netdev >> tree, please drop it from your tree. > >OK, will do. Note that this will generate a conflict between netdev >and the PCI tree during the merge window. > In such case let me drop it and you submit it, I was worried it will conflict with another ongoing feature in mlx5, but I am almost sure it won't be ready this cycle, so no reason to panic, you can take the patch to the pci tree and I will revise my PR, it's not too late. >Bjorn
On Wed, Aug 16, 2023 at 03:12:02PM -0700, Saeed Mahameed wrote: > On 16 Aug 17:04, Bjorn Helgaas wrote: > > On Wed, Aug 16, 2023 at 02:45:15PM -0700, Saeed Mahameed wrote: > > > On 14 Aug 17:32, Bjorn Helgaas wrote: > > > > On Mon, Aug 14, 2023 at 04:27:20PM +0300, Ilpo Järvinen wrote: > > > > > mlx5_pci_link_toggle() returns mix PCI specific error codes and generic > > > > > errnos. > > > > > > > > > > Convert the PCI specific error values to generic errno using > > > > > pcibios_err_to_errno() before returning them. > > > > > > > > > > Fixes: eabe8e5e88f5 ("net/mlx5: Handle sync reset now event") > > > > > Fixes: 212b4d7251c1 ("net/mlx5: Wait for firmware to enable CRS before pci_restore_state") > > > > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > > > > > > > > > > --- > > > > > > > > > > Maintainers beware, this will conflict with read+write -> set/clear_word > > > > > fixes in pci.git/pcie-rmw. As such, it might be the easiest for Bjorn to > > > > > take it instead of net people. > > > > > > > > I provisionally rebased and applied it on pci/pcie-rmw. Take a look > > > > and make sure I didn't botch it -- I also found a case in > > > > mlx5_check_dev_ids() that looks like it needs the same conversion. > > > > > > > > The commit as applied is below. > > > > > > > > If networking folks would prefer to take this, let me know and I can > > > > drop it. > > > > > > I Just took this patch into my mlx5 submission queue and sent it to netdev > > > tree, please drop it from your tree. > > > > OK, will do. Note that this will generate a conflict between netdev > > and the PCI tree during the merge window. > > > > In such case let me drop it and you submit it, I was worried it will > conflict with another ongoing feature in mlx5, but I am almost sure it > won't be ready this cycle, so no reason to panic, you can take the patch to > the pci tree and I will revise my PR, it's not too late. OK, I'll keep it, I think that will be easier all around. Bjorn
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fw_reset.c b/drivers/net/ethernet/mellanox/mlx5/core/fw_reset.c index 4804990b7f22..0afd9dbfc471 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/fw_reset.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/fw_reset.c @@ -371,7 +371,7 @@ static int mlx5_pci_link_toggle(struct mlx5_core_dev *dev) err = pci_read_config_word(dev->pdev, PCI_DEVICE_ID, &dev_id); if (err) - return err; + return pcibios_err_to_errno(err); err = mlx5_check_dev_ids(dev, dev_id); if (err) return err; @@ -386,16 +386,16 @@ static int mlx5_pci_link_toggle(struct mlx5_core_dev *dev) /* PCI link toggle */ err = pci_read_config_word(bridge, cap + PCI_EXP_LNKCTL, ®16); if (err) - return err; + return pcibios_err_to_errno(err); reg16 |= PCI_EXP_LNKCTL_LD; err = pci_write_config_word(bridge, cap + PCI_EXP_LNKCTL, reg16); if (err) - return err; + return pcibios_err_to_errno(err); msleep(500); reg16 &= ~PCI_EXP_LNKCTL_LD; err = pci_write_config_word(bridge, cap + PCI_EXP_LNKCTL, reg16); if (err) - return err; + return pcibios_err_to_errno(err); /* Check link */ if (!bridge->link_active_reporting) { @@ -408,7 +408,7 @@ static int mlx5_pci_link_toggle(struct mlx5_core_dev *dev) do { err = pci_read_config_word(bridge, cap + PCI_EXP_LNKSTA, ®16); if (err) - return err; + return pcibios_err_to_errno(err); if (reg16 & PCI_EXP_LNKSTA_DLLLA) break; msleep(20); @@ -426,7 +426,7 @@ static int mlx5_pci_link_toggle(struct mlx5_core_dev *dev) do { err = pci_read_config_word(dev->pdev, PCI_DEVICE_ID, ®16); if (err) - return err; + return pcibios_err_to_errno(err); if (reg16 == dev_id) break; msleep(20);
mlx5_pci_link_toggle() returns mix PCI specific error codes and generic errnos. Convert the PCI specific error values to generic errno using pcibios_err_to_errno() before returning them. Fixes: eabe8e5e88f5 ("net/mlx5: Handle sync reset now event") Fixes: 212b4d7251c1 ("net/mlx5: Wait for firmware to enable CRS before pci_restore_state") Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> --- Maintainers beware, this will conflict with read+write -> set/clear_word fixes in pci.git/pcie-rmw. As such, it might be the easiest for Bjorn to take it instead of net people. I wonder if these PCIBIOS_* error codes are useful at all? There's 1:1 mapping into errno values so no information loss if the functions would just return errnos directly. Perhaps this is just legacy nobody has bothered to remove? If nobody opposes, I could take a look at getting rid of them. --- drivers/net/ethernet/mellanox/mlx5/core/fw_reset.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)