netdev - Re: [net PATCH v2 7/8] fbnic: Pull fbnic_fw_xmit_cap

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: <CAKgT0Uf=ykmYjxywZzQ3f1RhveSTA1_s-8-GpevcGWV37gr5PA@mail.gmail.com>
Date: Tue, 6 May 2025 13:14:26 -0700
From: Alexander Duyck <alexander.duyck@...il.com>
To: Jacob Keller <jacob.e.keller@...el.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org, 
	pabeni@...hat.com, horms@...nel.org
Subject: Re: [net PATCH v2 7/8] fbnic: Pull fbnic_fw_xmit_cap_msg use out of
 interrupt context

On Tue, May 6, 2025 at 11:57 AM Jacob Keller <jacob.e.keller@...el.com> wrote:
>
>
>
> On 5/6/2025 9:00 AM, Alexander Duyck wrote:
> > From: Alexander Duyck <alexanderduyck@...com>
> >
> > This change pulls the call to fbnic_fw_xmit_cap_msg out of
> > fbnic_mbx_init_desc_ring and instead places it in the polling function for
> > getting the Tx ready. Doing that we can avoid the potential issue with an
> > interrupt coming in later from the firmware that causes it to get fired in
> > interrupt context.
> >
> > Fixes: 20d2e88cc746 ("eth: fbnic: Add initial messaging to notify FW of our presence")
> > Signed-off-by: Alexander Duyck <alexanderduyck@...com>
>
>
> Reviewed-by: Jacob Keller <jacob.e.keller@...el.com>
>
> > @@ -393,15 +375,6 @@ static void fbnic_mbx_init_desc_ring(struct fbnic_dev *fbd, int mbx_idx)
> >               /* Enable DMA reads from the device */
> >               wr32(fbd, FBNIC_PUL_OB_TLP_HDR_AR_CFG,
> >                    FBNIC_PUL_OB_TLP_HDR_AR_CFG_BME);
> > -
> > -             /* Force version to 1 if we successfully requested an update
> > -              * from the firmware. This should be overwritten once we get
> > -              * the actual version from the firmware in the capabilities
> > -              * request message.
> > -              */
> > -             if (!fbnic_fw_xmit_cap_msg(fbd) &&
> > -                 !fbd->fw_cap.running.mgmt.version)
> > -                     fbd->fw_cap.running.mgmt.version = 1;
>
> ...
>
> >
> > +     /* Request an update from the firmware. This should overwrite
> > +      * mgmt.version once we get the actual version from the firmware
> > +      * in the capabilities request message.
> > +      */
> > +     err = fbnic_fw_xmit_simple_msg(fbd, FBNIC_TLV_MSG_ID_HOST_CAP_REQ);
> > +     if (err)
> > +             goto clean_mbx;
> > +
> > +     /* Use "1" to indicate we entered the state waiting for a response */
> > +     fbd->fw_cap.running.mgmt.version = 1;
> > +
>
> Curious about the comment rewording here. I guess the extra information
> about forcing and the value being updated to the actual version later
> isn't as relevant in the new location?

Well the "force" wasn't really the correct wording to begin with. It
wasn't as if we were really forcing anything, and really we should
have been resetting the management version every time we restarted the
mailbox anyway since it is possible for the FW to reboot into a newer
or older version after we have flashed the device.

All we were doing is using a non-zero version to indicate that the
mailbox woke up, was sent a capabilities request, but didn't send a
capability response. We use it for debugging as we can identify cases
where the FW is there, but doesn't respond to the capabilities
request, or it sends back a malformed capabilities response.