netdev - Re: [net-next PATCH v2 11/15] eth: fbnic: Add link detection

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: <CAKgT0UfTAk=tNejVLSFth6aSeUhHYSmAErc84mQojXtT9n2GDg@mail.gmail.com>
Date: Tue, 25 Jun 2024 09:29:01 -0700
From: Alexander Duyck <alexander.duyck@...il.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: netdev@...r.kernel.org, Alexander Duyck <alexanderduyck@...com>, kuba@...nel.org, 
	davem@...emloft.net, pabeni@...hat.com
Subject: Re: [net-next PATCH v2 11/15] eth: fbnic: Add link detection

On Tue, Jun 25, 2024 at 8:25 AM Andrew Lunn <andrew@...n.ch> wrote:
>
> > +     /* Tri-state value indicating state of link.
> > +      *  0 - Up
> > +      *  1 - Down
> > +      *  2 - Event - Requires checking as link state may have changed
> > +      */
> > +     s8 link_state;
>
> Maybe add an enum?

Doesn't that default to a 32b size? The general thought was to just
keep this small since it only needs to be a few bits.

> > +static irqreturn_t fbnic_mac_msix_intr(int __always_unused irq, void *data)
> > +{
> > +     struct fbnic_dev *fbd = data;
> > +     struct fbnic_net *fbn;
> > +
> > +     if (!fbd->mac->get_link_event(fbd)) {
> > +             fbnic_wr32(fbd, FBNIC_INTR_MASK_CLEAR(0),
> > +                        1u << FBNIC_MAC_MSIX_ENTRY);
> > +             return IRQ_HANDLED;
> > +     }
> > +
> > +     fbd->link_state = FBNIC_LINK_EVENT;
> > +     fbn = netdev_priv(fbd->netdev);
> > +
> > +     phylink_mac_change(fbn->phylink, fbd->link_state == FBNIC_LINK_UP);
>
> Can fbd->link_state == FBNIC_LINK_UP given that you have just done:
>     fbd->link_state = FBNIC_LINK_EVENT ?

My bad. I will need to fix that. I think that was some fallout from an
earlier refactor.

> > +static u32 __fbnic_mac_config_asic(struct fbnic_dev *fbd)
> > +{
> > +     /* Enable MAC Promiscuous mode and Tx padding */
> > +     u32 command_config = FBNIC_MAC_COMMAND_CONFIG_TX_PAD_EN |
> > +                          FBNIC_MAC_COMMAND_CONFIG_PROMISC_EN;
> > +     struct fbnic_net *fbn = netdev_priv(fbd->netdev);
> > +     u32 rxb_pause_ctrl;
> > +
> > +     /* Set class 0 Quanta and refresh */
> > +     wr32(fbd, FBNIC_MAC_CL01_PAUSE_QUANTA, 0xffff);
> > +     wr32(fbd, FBNIC_MAC_CL01_QUANTA_THRESH, 0x7fff);
> > +
> > +     /* Enable generation of pause frames if enabled */
> > +     rxb_pause_ctrl = rd32(fbd, FBNIC_RXB_PAUSE_DROP_CTRL);
> > +     rxb_pause_ctrl &= ~FBNIC_RXB_PAUSE_DROP_CTRL_PAUSE_ENABLE;
> > +     if (!fbn->tx_pause)
> > +             command_config |= FBNIC_MAC_COMMAND_CONFIG_TX_PAUSE_DIS;
> > +     else
> > +             rxb_pause_ctrl |=
> > +                     FIELD_PREP(FBNIC_RXB_PAUSE_DROP_CTRL_PAUSE_ENABLE,
> > +                                FBNIC_PAUSE_EN_MASK);
> > +     wr32(fbd, FBNIC_RXB_PAUSE_DROP_CTRL, rxb_pause_ctrl);
> > +
> > +     if (!fbn->rx_pause)
> > +             command_config |= FBNIC_MAC_COMMAND_CONFIG_RX_PAUSE_DIS;
>
> Everybody gets pause wrong. To try to combat that it has mostly been
> moved into phylink. When phylink calls your mac_config() callback it
> passes const struct phylink_link_state *state. Within state is the
> pause member. That tells you how to configure the hardware. phylink
> will then deal with the differences between forced pause configuration
> and negotiated pause configuration, etc. Your current mac_config() is
> empty...
>
>         Andrew

So the pause setup for now is stored in fbn->[tr]x_pause. So if we
were to configure it via the mac_config call and then likely call into
this function. We end up having to reuse it in a few spots to avoid
having to read/modify the MAC register and instead just set the data
based on our stored config. Although I think we might be able to pare
this down as the command_config is the only piece we really need to
carry. The rest of this setup is essentially just a pause config which
could be done once instead of on every link up/down transition. I will
look at splitting this up.