[PATCH 1/1] net/mlx5: Convert PCI error values to generic errnos

[PATCH 1/1] net/mlx5: Convert PCI error values to generic errnos

[Ilpo Järvinen]

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(-)

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, &reg16);
 	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, &reg16);
 		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, &reg16);
 		if (err)
-			return err;
+			return pcibios_err_to_errno(err);
 		if (reg16 == dev_id)
 			break;
 		msleep(20);
-- 
2.30.2

Re: [PATCH 1/1] net/mlx5: Convert PCI error values to generic errnos

[Bjorn Helgaas]

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, &reg16);
>  	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, &reg16);
>  		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, &reg16);
>  		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, &reg16);
 		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, &reg16);
 		if (err)
-			return err;
+			return pcibios_err_to_errno(err);
 		if (reg16 == dev_id)
 			break;
 		msleep(20);

Re: [PATCH 1/1] net/mlx5: Convert PCI error values to generic errnos

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2239 bytes --]

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...


-- 
 i.

Re: [PATCH 1/1] net/mlx5: Convert PCI error values to generic errnos

[Bjorn Helgaas]

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

Re: [PATCH 1/1] net/mlx5: Convert PCI error values to generic errnos

[Saeed Mahameed]

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.

Re: [PATCH 1/1] net/mlx5: Convert PCI error values to generic errnos

[Bjorn Helgaas]

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

Re: [PATCH 1/1] net/mlx5: Convert PCI error values to generic errnos

[Saeed Mahameed]

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

Re: [PATCH 1/1] net/mlx5: Convert PCI error values to generic errnos

[Bjorn Helgaas]

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