Prestera driver fail to probe twice

Prestera driver fail to probe twice

[Köry Maincent]

Hello,

It seems the prestera driver has never been tested as a module or in a
probe defer case:

# insmod /mnt/prestera.ko 
# insmod /mnt/prestera_pci.ko
[   87.927343] Prestera DX 0000:01:00.0: Loading mrvl/prestera/mvsw_prestera_fw-v4.1.img ...
[   87.935600] Prestera DX 0000:01:00.0: FW version '4.1.0'
[   91.556693] Prestera DX 0000:01:00.0: Prestera FW is ready
[   99.431226] Prestera DX 0000:01:00.0: using random base mac address
# rmmod /mnt/prestera_pci.ko 
# insmod /mnt/prestera_pci.ko
[  122.938273] Prestera DX 0000:01:00.0: waiting for FW loader is timed out
[  122.945163] prestera_pci_probe : 944, err -110
[  122.949704] Prestera DX: probe of 0000:01:00.0 failed with error -110

We have to find a way to detect if the firmware has already been flashed.
I am not familiar with this controller and don't have its datasheet.
Could someone handle it?

Thanks,
Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

RE: [EXT] Prestera driver fail to probe twice

[Elad Nachman]

Sorry, that's not how this works.

The firmware CPU loader will only reload if the firmware crashed or exit.

Hence, insmod on the host side will fail, as the firmware side loader is not waiting
For the host to send a new firmware, but first for the existing firmware to exit.

By design, the way to load new firmware is by resetting both CPUs (this should be covered by CPLD).

Can you please explain:

1. What kind of HW / board are you trying this on?
2. Who is the customer requesting  this?
3. What is the design purpose of this change?

Elad.

-----Original Message-----
From: Köry Maincent <kory.maincent@bootlin.com> 
Sent: Tuesday, February 6, 2024 5:54 PM
To: netdev@vger.kernel.org; Taras Chornyi <taras.chornyi@plvision.eu>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>; Miquel Raynal <miquel.raynal@bootlin.com>
Subject: [EXT] Prestera driver fail to probe twice

External Email

----------------------------------------------------------------------
Hello,

It seems the prestera driver has never been tested as a module or in a probe defer case:

# insmod /mnt/prestera.ko
# insmod /mnt/prestera_pci.ko
[   87.927343] Prestera DX 0000:01:00.0: Loading mrvl/prestera/mvsw_prestera_fw-v4.1.img ...
[   87.935600] Prestera DX 0000:01:00.0: FW version '4.1.0'
[   91.556693] Prestera DX 0000:01:00.0: Prestera FW is ready
[   99.431226] Prestera DX 0000:01:00.0: using random base mac address
# rmmod /mnt/prestera_pci.ko
# insmod /mnt/prestera_pci.ko
[  122.938273] Prestera DX 0000:01:00.0: waiting for FW loader is timed out [  122.945163] prestera_pci_probe : 944, err -110 [  122.949704] Prestera DX: probe of 0000:01:00.0 failed with error -110

We have to find a way to detect if the firmware has already been flashed.
I am not familiar with this controller and don't have its datasheet.
Could someone handle it?

Thanks,
Regards,
--
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://urldefense.proofpoint.com/v2/url?u=https-3A__bootlin.com&d=DwIFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=eTeNTLEK5-TxXczjOcKPhANIFtlB9pP4lq9qhdlFrwQ&m=ttkSGM_1augs7U16EjsCCiDgsAPWM7GXv5u28w9zcStA49DC0bLarpXqDmoDngBu&s=9gSVvceZTEdo5VHefj6KdETqcGq-dFjEcSauSJ8OLi8&e= 

Re: [EXT] Prestera driver fail to probe twice

[Köry Maincent]

On Tue, 6 Feb 2024 18:30:33 +0000
Elad Nachman <enachman@marvell.com> wrote:

> Sorry, that's not how this works.
> 
> The firmware CPU loader will only reload if the firmware crashed or exit.
> 
> Hence, insmod on the host side will fail, as the firmware side loader is not
> waiting For the host to send a new firmware, but first for the existing
> firmware to exit.

With the current implementation we can't rmmod/insmod the driver.
Also, in case of deferring probe the same problem appears and the driver will
never probe. I don't think this is a good behavior.

Isn't it possible to verify that the firmware has already been sent and is
working well at the probe time? Then we wouldn't try to flash it.

Or stop the firmware at remove time and in case of probe defer.
 
> By design, the way to load new firmware is by resetting both CPUs (this
> should be covered by CPLD).

Are you saying the only way to stop the firmware is to shutdown the CPU?
As ask above, can't we know if the firmware has already been load?

> Can you please explain:
> 
> 1. What kind of HW / board are you trying this on?
> 2. Who is the customer requesting  this?
> 3. What is the design purpose of this change?

I have no particular request for that.
I was debugging a PoE controller driver that the prestera was depending on, I
was playing with these in a module state to verify the kref free.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

RE: [EXT] Prestera driver fail to probe twice

[Elad Nachman]

> -----Original Message-----
> From: Köry Maincent <kory.maincent@bootlin.com>
> Sent: Wednesday, February 7, 2024 12:23 PM
> To: Elad Nachman <enachman@marvell.com>
> Cc: netdev@vger.kernel.org; Taras Chornyi <taras.chornyi@plvision.eu>;
> Thomas Petazzoni <thomas.petazzoni@bootlin.com>; Miquel Raynal
> <miquel.raynal@bootlin.com>
> Subject: Re: [EXT] Prestera driver fail to probe twice
> 
> On Tue, 6 Feb 2024 18:30:33 +0000
> Elad Nachman <enachman@marvell.com> wrote:
> 
> > Sorry, that's not how this works.
> >
> > The firmware CPU loader will only reload if the firmware crashed or exit.
> >
> > Hence, insmod on the host side will fail, as the firmware side loader
> > is not waiting For the host to send a new firmware, but first for the
> > existing firmware to exit.
> 
> With the current implementation we can't rmmod/insmod the driver.
> Also, in case of deferring probe the same problem appears and the driver will
> never probe. I don't think this is a good behavior.
> 
> Isn't it possible to verify that the firmware has already been sent and is
> working well at the probe time? Then we wouldn't try to flash it.

Everything is possible, but that is the way the firmware interface was initially designed.
Changing this will break compatibility with board already deployed in the field.

> 
> Or stop the firmware at remove time and in case of probe defer.
> 
> > By design, the way to load new firmware is by resetting both CPUs
> > (this should be covered by CPLD).
> 
> Are you saying the only way to stop the firmware is to shutdown the CPU?

Yes, that is the current design.

> As ask above, can't we know if the firmware has already been load?

Once again, this will break compatibility with boards deployed on the field.

> 
> > Can you please explain:
> >
> > 1. What kind of HW / board are you trying this on?
> > 2. Who is the customer requesting  this?
> > 3. What is the design purpose of this change?
> 
> I have no particular request for that.
> I was debugging a PoE controller driver that the prestera was depending on, I
> was playing with these in a module state to verify the kref free.
> 
> Regards,
> --
> Köry Maincent, Bootlin
> Embedded Linux and kernel engineering
> https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__bootlin.com&d=DwIFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=eTeNTLEK5-
> TxXczjOcKPhANIFtlB9pP4lq9qhdlFrwQ&m=piWh4FT_ehOSBh66-
> WPooytouhKWszNmhcmZntCWpXSN-
> bUQK8Rts1fLTasbqFlu&s=F4Le7TOKKleZMOYJWQtVbUOZlXRh9TeK9JP_hLDDln
> c&e=

Re: [EXT] Prestera driver fail to probe twice

[Köry Maincent]

On Wed, 7 Feb 2024 10:56:29 +0000
Elad Nachman <enachman@marvell.com> wrote:

> > -----Original Message-----
> > From: Köry Maincent <kory.maincent@bootlin.com>
> > Sent: Wednesday, February 7, 2024 12:23 PM
> > To: Elad Nachman <enachman@marvell.com>
> > Cc: netdev@vger.kernel.org; Taras Chornyi <taras.chornyi@plvision.eu>;
> > Thomas Petazzoni <thomas.petazzoni@bootlin.com>; Miquel Raynal
> > <miquel.raynal@bootlin.com>
> > Subject: Re: [EXT] Prestera driver fail to probe twice
> > 
> > On Tue, 6 Feb 2024 18:30:33 +0000
> > Elad Nachman <enachman@marvell.com> wrote:
> >   
> > > Sorry, that's not how this works.
> > >
> > > The firmware CPU loader will only reload if the firmware crashed or exit.
> > >
> > > Hence, insmod on the host side will fail, as the firmware side loader
> > > is not waiting For the host to send a new firmware, but first for the
> > > existing firmware to exit.  
> > 
> > With the current implementation we can't rmmod/insmod the driver.
> > Also, in case of deferring probe the same problem appears and the driver
> > will never probe. I don't think this is a good behavior.
> > 
> > Isn't it possible to verify that the firmware has already been sent and is
> > working well at the probe time? Then we wouldn't try to flash it.  
> 
> Everything is possible, but that is the way the firmware interface was
> initially designed. Changing this will break compatibility with board already
> deployed in the field.

I don't understand, why fixing the probe by not flashing the firmware if it is
already flashed, will break compatibility?
Do I miss something?

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

RE: [EXT] Prestera driver fail to probe twice

[Elad Nachman]

> -----Original Message-----
> From: Köry Maincent <kory.maincent@bootlin.com>
> Sent: Wednesday, February 7, 2024 1:29 PM
> To: Elad Nachman <enachman@marvell.com>
> Cc: netdev@vger.kernel.org; Taras Chornyi <taras.chornyi@plvision.eu>;
> Thomas Petazzoni <thomas.petazzoni@bootlin.com>; Miquel Raynal
> <miquel.raynal@bootlin.com>
> Subject: Re: [EXT] Prestera driver fail to probe twice
> 
> On Wed, 7 Feb 2024 10:56:29 +0000
> Elad Nachman <enachman@marvell.com> wrote:
> 
> > > -----Original Message-----
> > > From: Köry Maincent <kory.maincent@bootlin.com>
> > > Sent: Wednesday, February 7, 2024 12:23 PM
> > > To: Elad Nachman <enachman@marvell.com>
> > > Cc: netdev@vger.kernel.org; Taras Chornyi
> > > <taras.chornyi@plvision.eu>; Thomas Petazzoni
> > > <thomas.petazzoni@bootlin.com>; Miquel Raynal
> > > <miquel.raynal@bootlin.com>
> > > Subject: Re: [EXT] Prestera driver fail to probe twice
> > >
> > > On Tue, 6 Feb 2024 18:30:33 +0000
> > > Elad Nachman <enachman@marvell.com> wrote:
> > >
> > > > Sorry, that's not how this works.
> > > >
> > > > The firmware CPU loader will only reload if the firmware crashed or exit.
> > > >
> > > > Hence, insmod on the host side will fail, as the firmware side
> > > > loader is not waiting For the host to send a new firmware, but
> > > > first for the existing firmware to exit.
> > >
> > > With the current implementation we can't rmmod/insmod the driver.
> > > Also, in case of deferring probe the same problem appears and the
> > > driver will never probe. I don't think this is a good behavior.
> > >
> > > Isn't it possible to verify that the firmware has already been sent
> > > and is working well at the probe time? Then we wouldn't try to flash it.
> >
> > Everything is possible, but that is the way the firmware interface was
> > initially designed. Changing this will break compatibility with board
> > already deployed in the field.
> 
> I don't understand, why fixing the probe by not flashing the firmware if it is
> already flashed, will break compatibility?
> Do I miss something?

First, firmware is loaded to RAM and not flashed.
Second, there is a certain control loop which dictates when the firmware loader expects new firmware by ABI, and that can only happen when the previous firmware code has terminated.

> 
> Regards,
> --
> Köry Maincent, Bootlin
> Embedded Linux and kernel engineering
> https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__bootlin.com&d=DwIFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=eTeNTLEK5-
> TxXczjOcKPhANIFtlB9pP4lq9qhdlFrwQ&m=T44g75Yk0UF0uqOlZrOea036_lOrf
> eH5g4oVTTZABDLVL9Yb06pSSH8oxsRgmt7g&s=L3_zgzJvce6HESMAg1UWPecI-
> Hqu9uOGd8N0AaZ1Jwg&e=

Re: [EXT] Prestera driver fail to probe twice

[Köry Maincent]

On Wed, 7 Feb 2024 12:24:16 +0000
Elad Nachman <enachman@marvell.com> wrote:

> > -----Original Message-----
> > From: Köry Maincent <kory.maincent@bootlin.com>
> > Sent: Wednesday, February 7, 2024 1:29 PM
> > To: Elad Nachman <enachman@marvell.com>
> > Cc: netdev@vger.kernel.org; Taras Chornyi <taras.chornyi@plvision.eu>;
> > Thomas Petazzoni <thomas.petazzoni@bootlin.com>; Miquel Raynal
> > <miquel.raynal@bootlin.com>
> > Subject: Re: [EXT] Prestera driver fail to probe twice
> > 
> > On Wed, 7 Feb 2024 10:56:29 +0000
> > Elad Nachman <enachman@marvell.com> wrote:
> >   
> > > > -----Original Message-----
> > > > From: Köry Maincent <kory.maincent@bootlin.com>
> > > > Sent: Wednesday, February 7, 2024 12:23 PM
> > > > To: Elad Nachman <enachman@marvell.com>
> > > > Cc: netdev@vger.kernel.org; Taras Chornyi
> > > > <taras.chornyi@plvision.eu>; Thomas Petazzoni
> > > > <thomas.petazzoni@bootlin.com>; Miquel Raynal
> > > > <miquel.raynal@bootlin.com>
> > > > Subject: Re: [EXT] Prestera driver fail to probe twice
> > > >
> > > > On Tue, 6 Feb 2024 18:30:33 +0000
> > > > Elad Nachman <enachman@marvell.com> wrote:
> > > >  
> > > > > Sorry, that's not how this works.
> > > > >
> > > > > The firmware CPU loader will only reload if the firmware crashed or
> > > > > exit.
> > > > >
> > > > > Hence, insmod on the host side will fail, as the firmware side
> > > > > loader is not waiting For the host to send a new firmware, but
> > > > > first for the existing firmware to exit.  
> > > >
> > > > With the current implementation we can't rmmod/insmod the driver.
> > > > Also, in case of deferring probe the same problem appears and the
> > > > driver will never probe. I don't think this is a good behavior.
> > > >
> > > > Isn't it possible to verify that the firmware has already been sent
> > > > and is working well at the probe time? Then we wouldn't try to flash
> > > > it.  
> > >
> > > Everything is possible, but that is the way the firmware interface was
> > > initially designed. Changing this will break compatibility with board
> > > already deployed in the field.  
> > 
> > I don't understand, why fixing the probe by not flashing the firmware if it
> > is already flashed, will break compatibility?
> > Do I miss something?  
> 
> First, firmware is loaded to RAM and not flashed.
> Second, there is a certain control loop which dictates when the firmware
> loader expects new firmware by ABI, and that can only happen when the
> previous firmware code has terminated.

I still don't understand why it will break the compatibility.
You never entered the second times probe as you would have faced this issue.

I haven't tested it yet but wouldn't this do the job:

--- a/drivers/net/ethernet/marvell/prestera/prestera_pci.c
+++ b/drivers/net/ethernet/marvell/prestera/prestera_pci.c
@@ -457,16 +457,21 @@ static int prestera_fw_init(struct prestera_fw *fw)
        fw->dev.send_req = prestera_fw_send_req;
        fw->ldr_regs = fw->dev.ctl_regs;
 
-       err = prestera_fw_load(fw);
-       if (err)
-               return err;
-
        err = prestera_fw_wait_reg32(fw, PRESTERA_FW_READY_REG,
                                     PRESTERA_FW_READY_MAGIC,
                                     PRESTERA_FW_READY_WAIT_MS);
        if (err) {
-               dev_err(fw->dev.dev, "FW failed to start\n");
-               return err;
+               err = prestera_fw_load(fw);
+               if (err)
+                       return err;
+
+               err = prestera_fw_wait_reg32(fw, PRESTERA_FW_READY_REG,
+                                            PRESTERA_FW_READY_MAGIC,
+                                            PRESTERA_FW_READY_WAIT_MS);
+               if (err) {
+                       dev_err(fw->dev.dev, "FW failed to start\n");
+                       return err;
+               }
        }


Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

RE: [EXT] Prestera driver fail to probe twice

[Elad Nachman]

> -----Original Message-----
> From: Köry Maincent <kory.maincent@bootlin.com>
> Sent: Wednesday, February 7, 2024 4:32 PM
> To: Elad Nachman <enachman@marvell.com>
> Cc: netdev@vger.kernel.org; Taras Chornyi <taras.chornyi@plvision.eu>;
> Thomas Petazzoni <thomas.petazzoni@bootlin.com>; Miquel Raynal
> <miquel.raynal@bootlin.com>
> Subject: Re: [EXT] Prestera driver fail to probe twice
> 
> On Wed, 7 Feb 2024 12:24:16 +0000
> Elad Nachman <enachman@marvell.com> wrote:
> 
> > > -----Original Message-----
> > > From: Köry Maincent <kory.maincent@bootlin.com>
> > > Sent: Wednesday, February 7, 2024 1:29 PM
> > > To: Elad Nachman <enachman@marvell.com>
> > > Cc: netdev@vger.kernel.org; Taras Chornyi
> > > <taras.chornyi@plvision.eu>; Thomas Petazzoni
> > > <thomas.petazzoni@bootlin.com>; Miquel Raynal
> > > <miquel.raynal@bootlin.com>
> > > Subject: Re: [EXT] Prestera driver fail to probe twice
> > >
> > > On Wed, 7 Feb 2024 10:56:29 +0000
> > > Elad Nachman <enachman@marvell.com> wrote:
> > >
> > > > > -----Original Message-----
> > > > > From: Köry Maincent <kory.maincent@bootlin.com>
> > > > > Sent: Wednesday, February 7, 2024 12:23 PM
> > > > > To: Elad Nachman <enachman@marvell.com>
> > > > > Cc: netdev@vger.kernel.org; Taras Chornyi
> > > > > <taras.chornyi@plvision.eu>; Thomas Petazzoni
> > > > > <thomas.petazzoni@bootlin.com>; Miquel Raynal
> > > > > <miquel.raynal@bootlin.com>
> > > > > Subject: Re: [EXT] Prestera driver fail to probe twice
> > > > >
> > > > > On Tue, 6 Feb 2024 18:30:33 +0000 Elad Nachman
> > > > > <enachman@marvell.com> wrote:
> > > > >
> > > > > > Sorry, that's not how this works.
> > > > > >
> > > > > > The firmware CPU loader will only reload if the firmware
> > > > > > crashed or exit.
> > > > > >
> > > > > > Hence, insmod on the host side will fail, as the firmware side
> > > > > > loader is not waiting For the host to send a new firmware, but
> > > > > > first for the existing firmware to exit.
> > > > >
> > > > > With the current implementation we can't rmmod/insmod the driver.
> > > > > Also, in case of deferring probe the same problem appears and
> > > > > the driver will never probe. I don't think this is a good behavior.
> > > > >
> > > > > Isn't it possible to verify that the firmware has already been
> > > > > sent and is working well at the probe time? Then we wouldn't try
> > > > > to flash it.
> > > >
> > > > Everything is possible, but that is the way the firmware interface
> > > > was initially designed. Changing this will break compatibility
> > > > with board already deployed in the field.
> > >
> > > I don't understand, why fixing the probe by not flashing the
> > > firmware if it is already flashed, will break compatibility?
> > > Do I miss something?
> >
> > First, firmware is loaded to RAM and not flashed.
> > Second, there is a certain control loop which dictates when the
> > firmware loader expects new firmware by ABI, and that can only happen
> > when the previous firmware code has terminated.
> 
> I still don't understand why it will break the compatibility.

Please read below regarding your code assumptions.
There is a complex state machine present today, and your changes will break it for existing boards.

> You never entered the second times probe as you would have faced this
> issue.
> 
> I haven't tested it yet but wouldn't this do the job:
> 
> --- a/drivers/net/ethernet/marvell/prestera/prestera_pci.c
> +++ b/drivers/net/ethernet/marvell/prestera/prestera_pci.c
> @@ -457,16 +457,21 @@ static int prestera_fw_init(struct prestera_fw *fw)
>         fw->dev.send_req = prestera_fw_send_req;
>         fw->ldr_regs = fw->dev.ctl_regs;
> 
> -       err = prestera_fw_load(fw);
> -       if (err)
> -               return err;
> -
>         err = prestera_fw_wait_reg32(fw, PRESTERA_FW_READY_REG,
>                                      PRESTERA_FW_READY_MAGIC,
>                                      PRESTERA_FW_READY_WAIT_MS);

This assumes wrongly that the MAGIC is correct when the firmware is running.

>         if (err) {
> -               dev_err(fw->dev.dev, "FW failed to start\n");
> -               return err;
> +               err = prestera_fw_load(fw);
> +               if (err)
> +                       return err;
> +
> +               err = prestera_fw_wait_reg32(fw, PRESTERA_FW_READY_REG,
> +                                            PRESTERA_FW_READY_MAGIC,
> +                                            PRESTERA_FW_READY_WAIT_MS);
> +               if (err) {
> +                       dev_err(fw->dev.dev, "FW failed to start\n");
> +                       return err;
> +               }
>         }

MAGIC value is cleared after FW is downloaded to RAM, just before it is executed.
So checking the MAGIC value to determine if the firmware is running is useless.

You will get the MAGIC value read correctly only when firmware is during the download process, which does not help you implementing the logic you want.

> 
> 
> Regards,
> --
> Köry Maincent, Bootlin
> Embedded Linux and kernel engineering
> https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__bootlin.com&d=DwIFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=eTeNTLEK5-
> TxXczjOcKPhANIFtlB9pP4lq9qhdlFrwQ&m=noNhHaknkUrd6D7SACyfoufA6a5U
> RJzyiZtY9e1W5aHqUHHCa5baEnyEtZMQU0Aq&s=OFTH-
> xoeBFGu02uVCdaHSWcGdKX9dbu950jxaO5rjOc&e=

Re: [EXT] Prestera driver fail to probe twice

[Jakub Kicinski]

On Wed, 7 Feb 2024 15:06:34 +0000 Elad Nachman wrote:
> MAGIC value is cleared after FW is downloaded to RAM, just before it is executed.
> So checking the MAGIC value to determine if the firmware is running is useless.
> 
> You will get the MAGIC value read correctly only when firmware is
> during the download process, which does not help you implementing the
> logic you want.

I'm not aware of other networking drivers which cannot be reloaded /
rebound. I think it's part of base expectations for upstream drivers.
Could you work on fixing this?

IIRC we have enum devlink_param_reset_dev_on_drv_probe_value to control
whether device is always reset on driver load. You can make resetting
an opt-in if you're concerned about backward compatibility.

Re: Prestera driver fail to probe twice

[Andrew Lunn]

On Tue, Feb 06, 2024 at 04:54:06PM +0100, Köry Maincent wrote:
> Hello,
> 
> It seems the prestera driver has never been tested as a module or in a
> probe defer case:

Hi Köry

Could you hack a -EPROBE_DEFER failure? If you can show that does not
work, the driver is clearly broken because phylink could return that.

      Andrew

RE: [EXT] Prestera driver fail to probe twice

[Elad Nachman]

Once again, existing deployed firmware does not check the enum below , so this does not ensure backward compatibility.

-----Original Message-----
From: Jakub Kicinski <kuba@kernel.org> 
Sent: Wednesday, February 7, 2024 8:32 PM
To: Elad Nachman <enachman@marvell.com>
Cc: Köry Maincent <kory.maincent@bootlin.com>; netdev@vger.kernel.org; Taras Chornyi <taras.chornyi@plvision.eu>; Thomas Petazzoni <thomas.petazzoni@bootlin.com>; Miquel Raynal <miquel.raynal@bootlin.com>
Subject: Re: [EXT] Prestera driver fail to probe twice

On Wed, 7 Feb 2024 15:06:34 +0000 Elad Nachman wrote:
> MAGIC value is cleared after FW is downloaded to RAM, just before it is executed.
> So checking the MAGIC value to determine if the firmware is running is useless.
> 
> You will get the MAGIC value read correctly only when firmware is 
> during the download process, which does not help you implementing the 
> logic you want.

I'm not aware of other networking drivers which cannot be reloaded / rebound. I think it's part of base expectations for upstream drivers.
Could you work on fixing this?

IIRC we have enum devlink_param_reset_dev_on_drv_probe_value to control whether device is always reset on driver load. You can make resetting an opt-in if you're concerned about backward compatibility.

Re: Prestera driver fail to probe twice

[Köry Maincent]

Hello Andrew,

On Thu, 8 Feb 2024 00:32:43 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> On Tue, Feb 06, 2024 at 04:54:06PM +0100, Köry Maincent wrote:
> > Hello,
> > 
> > It seems the prestera driver has never been tested as a module or in a
> > probe defer case:  
> 
> Hi Köry
> 
> Could you hack a -EPROBE_DEFER failure? If you can show that does not
> work, the driver is clearly broken because phylink could return that.

That is how I have noticed the issue. I was trying to test my PSE pd692x0
driver as a module that prestera depends on (I will drop the PoE v3 soon).
The PSE core is returning -EPROBE_DEFER in case we can not find the PSE
provider.

Here is a boot dmesg for example:

[    1.898897] Prestera DX 0000:01:00.0: Loading mrvl/prestera/mvsw_prestera_fw-v4.1.img ...
[    1.907155] Prestera DX 0000:01:00.0: FW version '4.1.0'
[    5.535427] Prestera DX 0000:01:00.0: Prestera FW is ready
[   13.458823] Prestera DX 0000:01:00.0: using random base mac address
[   13.596594] prestera_port_sfp_bind : 403 -EPROBE_DEFER (The hack to get the PSE is in this function)
[   13.632517] ahci f2540000.sata: supply ahci not found, using dummy regulator
[   13.639759] ahci f2540000.sata: supply phy not found, using dummy regulator
[   13.646846] platform f2540000.sata:sata-port@0: supply target not found, using dummy regulator
[   13.655866] platform f2540000.sata:sata-port@1: supply target not found, using dummy regulator
[   13.666105] ahci f2540000.sata: masking port_map 0x3 -> 0x3
[   13.671960] ahci f2540000.sata: AHCI 0001.0000 32 slots 2 ports 6 Gbps 0x3 impl platform mode
[   13.680598] ahci f2540000.sata: flags: 64bit ncq sntf led only pmp fbs pio slum part sxs 
[   13.690393] scsi host1: ahci
[   13.693744] scsi host2: ahci
[   13.696789] ata1: SATA max UDMA/133 mmio [mem 0xf2540000-0xf256ffff] port 0x100 irq 40 lpm-pol 0
[   13.705640] ata2: SATA max UDMA/133 mmio [mem 0xf2540000-0xf256ffff] port 0x180 irq 40 lpm-pol 0
[   13.787389] mvpp2 f2000000.ethernet: using 8 per-cpu buffers
[   13.813450] mvpp2 f2000000.ethernet eth0: Using random mac address a2:00:a8:8c:7f:ca
[   13.862023] i2c i2c-2: Added multiplexed i2c bus 3
[   13.867101] i2c i2c-2: Added multiplexed i2c bus 4
[   13.872104] i2c i2c-2: Added multiplexed i2c bus 5
[   13.877181] i2c i2c-2: Added multiplexed i2c bus 6
[   13.882034] i2c-mux-gpio i2c-mux: 4 port mux on mv64xxx_i2c adapter adapter
[   14.030349] ata2: SATA link down (SStatus 0 SControl 300)
[   14.195606] ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
[   14.201982] ata1.00: ATA-9: M.2 (S42) 3TE7, S20730A, max UDMA/133
[   14.208142] ata1.00: 53742528 sectors, multi 16: LBA48 NCQ (depth 32)
[   14.214832] ata1.00: configured for UDMA/133
[   14.219477] scsi 1:0:0:0: Direct-Access     ATA      M.2 (S42) 3TE7   30A  PQ: 0 ANSI: 5
[   14.228723] sd 1:0:0:0: [sdb] 53742528 512-byte logical blocks: (27.5 GB/25.6 GiB)
[   14.236465] sd 1:0:0:0: [sdb] Write Protect is off
[   14.241341] sd 1:0:0:0: [sdb] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
[   14.250491] sd 1:0:0:0: [sdb] Preferred minimum I/O size 512 bytes
[   14.259197]  sdb: sdb1 sdb2 sdb3 sdb4
[   14.263962] sd 1:0:0:0: [sdb] Attached SCSI removable disk
[   18.896377] Prestera DX 0000:01:00.0: waiting for FW loader is timed out
[   18.903242] Prestera DX: probe of 0000:01:00.0 failed with error -110


Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: Prestera driver fail to probe twice

[Andrew Lunn]

On Thu, Feb 08, 2024 at 10:10:05AM +0100, Köry Maincent wrote:
> Hello Andrew,
> 
> On Thu, 8 Feb 2024 00:32:43 +0100
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
> > On Tue, Feb 06, 2024 at 04:54:06PM +0100, Köry Maincent wrote:
> > > Hello,
> > > 
> > > It seems the prestera driver has never been tested as a module or in a
> > > probe defer case:  
> > 
> > Hi Köry
> > 
> > Could you hack a -EPROBE_DEFER failure? If you can show that does not
> > work, the driver is clearly broken because phylink could return that.
> 
> That is how I have noticed the issue. I was trying to test my PSE pd692x0
> driver as a module that prestera depends on (I will drop the PoE v3 soon).
> The PSE core is returning -EPROBE_DEFER in case we can not find the PSE
> provider.
> 
> Here is a boot dmesg for example:
> 
> [    1.898897] Prestera DX 0000:01:00.0: Loading mrvl/prestera/mvsw_prestera_fw-v4.1.img ...
> [    1.907155] Prestera DX 0000:01:00.0: FW version '4.1.0'
> [    5.535427] Prestera DX 0000:01:00.0: Prestera FW is ready
> [   13.458823] Prestera DX 0000:01:00.0: using random base mac address
> [   13.596594] prestera_port_sfp_bind : 403 -EPROBE_DEFER (The hack to get the PSE is in this function)
> [   13.632517] ahci f2540000.sata: supply ahci not found, using dummy regulator
> [   13.639759] ahci f2540000.sata: supply phy not found, using dummy regulator
> [   13.646846] platform f2540000.sata:sata-port@0: supply target not found, using dummy regulator
> [   13.655866] platform f2540000.sata:sata-port@1: supply target not found, using dummy regulator
> [   13.666105] ahci f2540000.sata: masking port_map 0x3 -> 0x3
> [   13.671960] ahci f2540000.sata: AHCI 0001.0000 32 slots 2 ports 6 Gbps 0x3 impl platform mode
> [   13.680598] ahci f2540000.sata: flags: 64bit ncq sntf led only pmp fbs pio slum part sxs 
> [   13.690393] scsi host1: ahci
> [   13.693744] scsi host2: ahci
> [   13.696789] ata1: SATA max UDMA/133 mmio [mem 0xf2540000-0xf256ffff] port 0x100 irq 40 lpm-pol 0
> [   13.705640] ata2: SATA max UDMA/133 mmio [mem 0xf2540000-0xf256ffff] port 0x180 irq 40 lpm-pol 0
> [   13.787389] mvpp2 f2000000.ethernet: using 8 per-cpu buffers
> [   13.813450] mvpp2 f2000000.ethernet eth0: Using random mac address a2:00:a8:8c:7f:ca
> [   13.862023] i2c i2c-2: Added multiplexed i2c bus 3
> [   13.867101] i2c i2c-2: Added multiplexed i2c bus 4
> [   13.872104] i2c i2c-2: Added multiplexed i2c bus 5
> [   13.877181] i2c i2c-2: Added multiplexed i2c bus 6
> [   13.882034] i2c-mux-gpio i2c-mux: 4 port mux on mv64xxx_i2c adapter adapter
> [   14.030349] ata2: SATA link down (SStatus 0 SControl 300)
> [   14.195606] ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
> [   14.201982] ata1.00: ATA-9: M.2 (S42) 3TE7, S20730A, max UDMA/133
> [   14.208142] ata1.00: 53742528 sectors, multi 16: LBA48 NCQ (depth 32)
> [   14.214832] ata1.00: configured for UDMA/133
> [   14.219477] scsi 1:0:0:0: Direct-Access     ATA      M.2 (S42) 3TE7   30A  PQ: 0 ANSI: 5
> [   14.228723] sd 1:0:0:0: [sdb] 53742528 512-byte logical blocks: (27.5 GB/25.6 GiB)
> [   14.236465] sd 1:0:0:0: [sdb] Write Protect is off
> [   14.241341] sd 1:0:0:0: [sdb] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
> [   14.250491] sd 1:0:0:0: [sdb] Preferred minimum I/O size 512 bytes
> [   14.259197]  sdb: sdb1 sdb2 sdb3 sdb4
> [   14.263962] sd 1:0:0:0: [sdb] Attached SCSI removable disk
> [   18.896377] Prestera DX 0000:01:00.0: waiting for FW loader is timed out
> [   18.903242] Prestera DX: probe of 0000:01:00.0 failed with error -110

Thanks. That clearly shows that the Prestera driver is broken.

Either there needs to be a way to determine if the firmware has been
downloaded, or the firmware download needs to be moved to the end of
the probe function once all resources are available and its no longer
possible to have an -EPROBE_DEFER.

   Andrew

Re: [EXT] Prestera driver fail to probe twice

[Jakub Kicinski]

On Thu, 8 Feb 2024 07:36:54 +0000 Elad Nachman wrote:
> Once again, existing deployed firmware does not check the enum below,
> so this does not ensure backward compatibility.

Okay, if it's a bigger rework - how much time do you need to get
it fixed?

Re: [EXT] Prestera driver fail to probe twice

[Jakub Kicinski]

On Thu, 8 Feb 2024 07:37:18 -0800 Jakub Kicinski wrote:
> On Thu, 8 Feb 2024 07:36:54 +0000 Elad Nachman wrote:
> > Once again, existing deployed firmware does not check the enum below,
> > so this does not ensure backward compatibility.  
> 
> Okay, if it's a bigger rework - how much time do you need to get
> it fixed?

This question is hogging a spot on my todo list.
I'll earmark the driver for removal in three months if there's 
no progress. Please LMK if you have a different estimate.
It's just a question of it not falling thru the cracks.