netdev - Re: [net PATCH 5/6] fbnic: Cleanup handling of completions

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: <CAKgT0UcnUEY9JMajO-18vJ7wNCXt6LV0WM61imwPi=y=d_TEqg@mail.gmail.com>
Date: Sun, 4 May 2025 07:37:15 -0700
From: Alexander Duyck <alexander.duyck@...il.com>
To: Simon Horman <horms@...nel.org>
Cc: netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org, 
	pabeni@...hat.com
Subject: Re: [net PATCH 5/6] fbnic: Cleanup handling of completions

On Fri, May 2, 2025 at 3:45 AM Simon Horman <horms@...nel.org> wrote:
>
> On Thu, May 01, 2025 at 04:30:22PM -0700, Alexander Duyck wrote:
> > From: Alexander Duyck <alexanderduyck@...com>
> >
> > There was an issue in that if we were to shutdown we could be left with
> > a completion in flight as the mailbox went away. To address that I have
> > added an fbnic_mbx_evict_all_cmpl function that is meant to essentially
> > create a "broken pipe" type response so that all callers will receive an
> > error indicating that the connection has been broken as a result of us
> > shutting down the mailbox.
> >
> > In addition the naming was inconsistent between the creation and clearing
> > of completions. Since the cmpl seems to be the common suffix to use for the
> > handling of cmpl_data I went through and renamed fbnic_fw_clear_compl to
> > fbnic_fw_clear_cmpl.
>
> I do see this is somehow related to the fix described in the first
> paragraph. But I don't think renaming functions like this is appropriate
> for net.

I can drop this. As it stands we have a follow-up patch for net-next
which I had split these fixes from so we can most likely leave it for
there.

> > Fixes: 378e5cc1c6c6 ("eth: fbnic: hwmon: Add completion infrastructure for firmware requests")
> >
> > Signed-off-by: Alexander Duyck <alexanderduyck@...com>
> > ---
> >  drivers/net/ethernet/meta/fbnic/fbnic_fw.c  |   22 +++++++++++++++++++++-
> >  drivers/net/ethernet/meta/fbnic/fbnic_fw.h  |    2 +-
> >  drivers/net/ethernet/meta/fbnic/fbnic_mac.c |    2 +-
> >  3 files changed, 23 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> > index d019191d6ae9..efc0176f1a9a 100644
> > --- a/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> > +++ b/drivers/net/ethernet/meta/fbnic/fbnic_fw.c
> > @@ -928,6 +928,23 @@ int fbnic_mbx_poll_tx_ready(struct fbnic_dev *fbd)
> >       return attempts ? 0 : -ETIMEDOUT;
> >  }
> >
> > +static void __fbnic_fw_evict_cmpl(struct fbnic_fw_completion *cmpl_data)
> > +{
> > +     cmpl_data->result = -EPIPE;
> > +     complete(&cmpl_data->done);
> > +}
> > +
> > +static void fbnic_mbx_evict_all_cmpl(struct fbnic_dev *fbd)
> > +{
> > +     struct fbnic_fw_completion *cmpl_data;
> > +
> > +     cmpl_data = fbd->cmpl_data;
> > +     if (cmpl_data)
> > +             __fbnic_fw_evict_cmpl(cmpl_data);
> > +
> > +     memset(&fbd->cmpl_data, 0, sizeof(fbd->cmpl_data));
>
> Maybe I've been staring at my screen for too long, but could this
> be expressed more succinctly as:
>
>         fbd->cmpl_data = NULL;
>
> And if so, it seems that step can be skipped if cmpl_data is already
> NULL, which is already checked.
>
> Leading to the following, which is somehow easier on my brain.
>
> static void fbnic_mbx_evict_all_cmpl(struct fbnic_dev *fbd)
> {
>         if (fbd->cmpl_data) {
>                 __fbnic_fw_evict_cmpl(fbd->cmpl_data);
>                 fbd->cmpl_data = NULL;
>         }
> }

I'll make the change. There is a follow-on patch for net-next that
will add support for multiple completions. As such the code may roll
back a bit to the original form, but it will make more sense if
contained within a for loop.