As of now, initial storage MAC setup (in bnx2x_init_one) is not checked.

This can lead to unexpected FCoE behavior (as address will be in unexpected
state) without notice.

Check dev_addr_add for storage MAC and if it failes produce error message
and turn off FCoE feature.

Signed-off-by: Nikita Kiryushin <kiryushin@ancud.ru>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index 678829646cec..c5d5e85777d4 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -13988,8 +13988,12 @@ static int bnx2x_init_one(struct pci_dev *pdev,
 	if (!NO_FCOE(bp)) {
 		/* Add storage MAC address */
 		rtnl_lock();
-		dev_addr_add(bp->dev, bp->fip_mac, NETDEV_HW_ADDR_T_SAN);
+		rc = dev_addr_add(bp->dev, bp->fip_mac, NETDEV_HW_ADDR_T_SAN);
 		rtnl_unlock();
+		if (rc) {
+			dev_err(&pdev->dev, "Cannot add storage MAC address\n");
+			bp->flags |= NO_FCOE_FLAG;
+		}
 	}
 	BNX2X_DEV_INFO(
 	       "%s (%c%d) PCI-E found at mem %lx, IRQ %d, node addr %pM\n",
-- 
2.34.1

On Fri, Jul 12, 2024 at 04:29:15PM +0300, Nikita Kiryushin wrote:
> As of now, initial storage MAC setup (in bnx2x_init_one) is not checked.
> 
> This can lead to unexpected FCoE behavior (as address will be in unexpected
> state) without notice.
> 
> Check dev_addr_add for storage MAC and if it failes produce error message
> and turn off FCoE feature.

How broken is it when this happens? This is called from .probe. So
returning the error code will fail the probe and the device will not
be created. Is that a better solution?

	Andrew

> How broken is it when this happens?
I can not say what would happen exactly, if the address is not assigned
the way it should. But there would be at least an attempt to free unallocated
address (in __bnx2x_remove).

> This is called from .probe. So
> returning the error code will fail the probe and the device will not
> be created. Is that a better solution?
To me, it does not seem fatal, that is why I am not returning error,
just print it and disable FCoE. The "rc" set will not be returned (unless
jumped to error handlers, which we are not doing). Would it be better, if
I used some other result variable other than "rc"? The check could be the call,
but than handling would be inside a lock, which I think is a bad idea.

On 7/15/24 17:10, Nikita Kiryushin wrote:
>> How broken is it when this happens?
> I can not say what would happen exactly, if the address is not assigned
> the way it should. But there would be at least an attempt to free unallocated
> address (in __bnx2x_remove).
>
>> This is called from .probe. So
>> returning the error code will fail the probe and the device will not
>> be created. Is that a better solution?
> To me, it does not seem fatal, that is why I am not returning error,
> just print it and disable FCoE. The "rc" set will not be returned (unless
> jumped to error handlers, which we are not doing). Would it be better, if
> I used some other result variable other than "rc"? The check could be the call,
> but than handling would be inside a lock, which I think is a bad idea.

The patch is marked as "Changes Requested" at the Patchwork,
but I am not sure, what has to be done with it.