From: Hangbin Liu <liuhangbin@gmail.com>
To: Vladimir Oltean <vladimir.oltean@nxp.com>
Cc: Richard Cochran <richardcochran@gmail.com>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Andrew Lunn <andrew@lunn.ch>, David Miller <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>,
Jay Vosburgh <j.vosburgh@gmail.com>,
Veaceslav Falico <vfalico@gmail.com>,
Andy Gospodarek <andy@greyhouse.net>
Subject: Re: [PATCH RFC V1 net-next 1/4] net: ethtool: Refactor identical get_ts_info implementations.
Date: Fri, 21 Jan 2022 11:56:37 +0800 [thread overview]
Message-ID: <Yeou9TKzW1NcBOKW@Laptop-X1> (raw)
In-Reply-To: <20220120161329.fbniou5kzn2x4rp7@skbuf>
On Thu, Jan 20, 2022 at 04:13:29PM +0000, Vladimir Oltean wrote:
> > diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> > index b60e22f6394a..f28b88b67b9e 100644
> > --- a/drivers/net/bonding/bond_main.c
> > +++ b/drivers/net/bonding/bond_main.c
> > @@ -5353,23 +5353,13 @@ static int bond_ethtool_get_ts_info(struct net_device *bond_dev,
> > struct ethtool_ts_info *info)
> > {
> > struct bonding *bond = netdev_priv(bond_dev);
> > - const struct ethtool_ops *ops;
> > struct net_device *real_dev;
> > - struct phy_device *phydev;
> >
> > rcu_read_lock();
> > real_dev = bond_option_active_slave_get_rcu(bond);
> > rcu_read_unlock();
>
> Side note: I'm a bit confused about this rcu_read_lock() ->
> rcu_dereference_protected() -> rcu_read_unlock() pattern, and use of the
> real_dev outside the RCU critical section. Isn't ->get_ts_info()
> protected by the rtnl_mutex? Shouldn't there be a
> bond_option_active_slave_get() which uses rtnl_dereference()?
> I see the code has been recently added by Hangbin Liu.
Hi Vladimir,
Yes, it should be enough to use rtnl_dereference() since ->get_ts_info is
protected by the rtnl_lock. I just thought there is an existing get active
slave function and rcu read should be OK to be used here. So I just used the
existing one.
Hi Jay,
Do you think if there is a need to add a rtnl version of
bond_option_active_slave_get()?
Thanks
Hangbin
next prev parent reply other threads:[~2022-01-21 3:56 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-03 23:25 [PATCH RFC V1 net-next 1/4] net: ethtool: Refactor identical get_ts_info implementations Richard Cochran
2022-01-20 16:13 ` Vladimir Oltean
2022-01-21 3:56 ` Hangbin Liu [this message]
2022-01-21 5:20 ` Jay Vosburgh
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=Yeou9TKzW1NcBOKW@Laptop-X1 \
--to=liuhangbin@gmail.com \
--cc=andrew@lunn.ch \
--cc=andy@greyhouse.net \
--cc=davem@davemloft.net \
--cc=j.vosburgh@gmail.com \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=richardcochran@gmail.com \
--cc=vfalico@gmail.com \
--cc=vladimir.oltean@nxp.com \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).