From: Doug Berger <opendmb@gmail.com>
To: Florian Fainelli <florian.fainelli@broadcom.com>,
"David S. Miller" <davem@davemloft.net>,
Eric Dumazet <edumazet@google.com>,
Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>
Cc: bcm-kernel-feedback-list@broadcom.com, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, stable@vger.kernel.org
Subject: Re: [PATCH net v2 3/3] net: bcmgenet: synchronize UMAC_CMD access
Date: Fri, 26 Apr 2024 12:43:15 -0700 [thread overview]
Message-ID: <50522abe-57d0-47df-9917-e0cd6848650f@gmail.com> (raw)
In-Reply-To: <07b4cb83-08db-449d-9d73-88e84fa570bd@broadcom.com>
On 4/26/2024 11:19 AM, Florian Fainelli wrote:
> On 4/25/24 15:27, Doug Berger wrote:
>> The UMAC_CMD register is written from different execution
>> contexts and has insufficient synchronization protections to
>> prevent possible corruption. Of particular concern are the
>> acceses from the phy_device delayed work context used by the
>> adjust_link call and the BH context that may be used by the
>> ndo_set_rx_mode call.
>>
>> A spinlock is added to the driver to protect contended register
>> accesses (i.e. reg_lock) and it is used to synchronize accesses
>> to UMAC_CMD.
>>
>> Fixes: 1c1008c793fa ("net: bcmgenet: add main driver file")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Doug Berger <opendmb@gmail.com>
>
> Acked-by: Florian Fainelli <florian.fainelli@broadcom.com>
>
> As a bug fix this is totally fine. I believe there could be an
> improvement made in 'net-next' whereby we introduce an
> unimac_rmw_locked() or something that essentially does:
>
> void unimac_rmw_locked(struct bcmgenet_priv *priv, u32 offset, u32
> mask_clear, u32 mask_set)
> {
> u32 reg;
> spin_lock_bh(&priv->reg_lock);
> reg = bcmgenet_umac_readl(priv, offset);
> reg &= ~mask_clear;
> reg |= mask_set;
> bcmgenet_umac_writel(priv, reg, offset);
> spin_unlock_bh(&priv->reg_lock);
> }
>
> At least a couple of callers could benefit from it. Thanks!
The only issue I see is enforcing the 2us delay in reset_umac(). A
scenario where a different context might attempt to modify UMAC_CMD
during that window is admittedly contrived, but the approach of this
commit provides better protection.
-Doug
next prev parent reply other threads:[~2024-04-26 19:43 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-25 22:27 [PATCH net v2 0/3] net: bcmgenet: protect contended accesses Doug Berger
2024-04-25 22:27 ` [PATCH net v2 1/3] net: bcmgenet: synchronize EXT_RGMII_OOB_CTRL access Doug Berger
2024-04-26 18:16 ` Florian Fainelli
2024-04-25 22:27 ` [PATCH net v2 2/3] net: bcmgenet: synchronize use of bcmgenet_set_rx_mode() Doug Berger
2024-04-26 18:16 ` Florian Fainelli
2024-04-25 22:27 ` [PATCH net v2 3/3] net: bcmgenet: synchronize UMAC_CMD access Doug Berger
2024-04-26 18:19 ` Florian Fainelli
2024-04-26 19:43 ` Doug Berger [this message]
2024-04-26 19:46 ` Doug Berger
2024-04-29 5:30 ` [PATCH net v2 0/3] net: bcmgenet: protect contended accesses patchwork-bot+netdevbpf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=50522abe-57d0-47df-9917-e0cd6848650f@gmail.com \
--to=opendmb@gmail.com \
--cc=bcm-kernel-feedback-list@broadcom.com \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=florian.fainelli@broadcom.com \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=stable@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.