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]
Date: Wed, 7 Feb 2024 15:06:34 +0000
From: Elad Nachman <enachman@...vell.com>
To: Köry Maincent <kory.maincent@...tlin.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



> -----Original Message-----
> From: Köry Maincent <kory.maincent@...tlin.com>
> Sent: Wednesday, February 7, 2024 4:32 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 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.

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=