netdev - Re: [net-next PATCH v4 07/10] fbnic: Add logic to track PMD state via MAC/PCS signals

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] [day] [month] [year] [list]

Message-ID: <CAKgT0Ued4fG9_CEdjJRUnBgUaCxr39eJ16y1g+Pdmq+znQOVNA@mail.gmail.com>
Date: Tue, 18 Nov 2025 10:11:14 -0800
From: Alexander Duyck <alexander.duyck@...il.com>
To: Paolo Abeni <pabeni@...hat.com>
Cc: netdev@...r.kernel.org, kuba@...nel.org, kernel-team@...a.com, 
	andrew+netdev@...n.ch, hkallweit1@...il.com, linux@...linux.org.uk, 
	davem@...emloft.net
Subject: Re: [net-next PATCH v4 07/10] fbnic: Add logic to track PMD state via
 MAC/PCS signals

On Tue, Nov 18, 2025 at 3:07 AM Paolo Abeni <pabeni@...hat.com> wrote:
>
> On 11/13/25 5:33 PM, Alexander Duyck wrote:
> > +/**
> > + * fbnic_phylink_pmd_training_complete_notify - PMD training complete notifier
> > + * @netdev: Netdev struct phylink device attached to
> > + *
> > + * When the link first comes up the PMD will have a period of 2 to 3 seconds
> > + * where the link will flutter due to link training. To avoid spamming the
> > + * kernel log with messages about this we add a delay of 4 seconds from the
> > + * time of the last PCS report of link so that we can guarantee we are unlikely
> > + * to see any further link loss events due to link training.
> > + **/
> > +void fbnic_phylink_pmd_training_complete_notify(struct net_device *netdev)
> > +{
> > +     struct fbnic_net *fbn = netdev_priv(netdev);
> > +     struct fbnic_dev *fbd = fbn->fbd;
> > +
> > +     if (fbd->pmd_state != FBNIC_PMD_TRAINING)
> > +             return;
> > +
> > +     if (!time_before(fbd->end_of_pmd_training, jiffies))
> > +             return;
> > +
> > +     fbd->pmd_state = FBNIC_PMD_SEND_DATA;
> > +
> > +     phylink_pcs_change(&fbn->phylink_pcs, false);
>
> AFAICS the above runs with no lock and can race with
> pcs_get_state()/fbnic_pmd_update_state(). Is there some logic safeguard
> logic I'm missing? Why 'pmd_state' does not need ONCE annotation?

We shouldn't need to worry about the pcs_get_state as the
phylink_pcs_change call will trigger a follow-on so if the link is up
it will be brought down in the next call. So the worst case scenario
is that it would appear as a link flap.

I will take a look at the pmd_state setup. I don't know if we need to
use the ONCE annotations, but I think we may be relying a bit on the
sanity of x86 ordering and may need to make a few tweaks.