netdev - Re: [EXT] Prestera driver fail to probe twice

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240207153136.761ef376@kmaincent-XPS-13-7390>
Date: Wed, 7 Feb 2024 15:31:36 +0100
From: Köry Maincent <kory.maincent@...tlin.com>
To: Elad Nachman <enachman@...vell.com>
Cc: "netdev@...r.kernel.org" <netdev@...r.kernel.org>, Taras Chornyi
 <taras.chornyi@...ision.eu>, Thomas Petazzoni
 <thomas.petazzoni@...tlin.com>, Miquel Raynal <miquel.raynal@...tlin.com>
Subject: Re: [EXT] Prestera driver fail to probe twice

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

> > -----Original Message-----
> > From: Köry Maincent <kory.maincent@...tlin.com>
> > Sent: Wednesday, February 7, 2024 1:29 PM
> > To: Elad Nachman <enachman@...vell.com>
> > Cc: netdev@...r.kernel.org; Taras Chornyi <taras.chornyi@...ision.eu>;
> > Thomas Petazzoni <thomas.petazzoni@...tlin.com>; Miquel Raynal
> > <miquel.raynal@...tlin.com>
> > Subject: Re: [EXT] Prestera driver fail to probe twice
> > 
> > On Wed, 7 Feb 2024 10:56:29 +0000
> > Elad Nachman <enachman@...vell.com> wrote:
> >   
> > > > -----Original Message-----
> > > > From: Köry Maincent <kory.maincent@...tlin.com>
> > > > Sent: Wednesday, February 7, 2024 12:23 PM
> > > > To: Elad Nachman <enachman@...vell.com>
> > > > Cc: netdev@...r.kernel.org; Taras Chornyi
> > > > <taras.chornyi@...ision.eu>; Thomas Petazzoni
> > > > <thomas.petazzoni@...tlin.com>; Miquel Raynal
> > > > <miquel.raynal@...tlin.com>
> > > > Subject: Re: [EXT] Prestera driver fail to probe twice
> > > >
> > > > On Tue, 6 Feb 2024 18:30:33 +0000
> > > > Elad Nachman <enachman@...vell.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