[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Y8f4i2ablWnNO9Op@DEN-LT-70577>
Date: Wed, 18 Jan 2023 13:47:57 +0000
From: <Daniel.Machon@...rochip.com>
To: <petrm@...dia.com>
CC: <netdev@...r.kernel.org>, <davem@...emloft.net>,
<edumazet@...gle.com>, <kuba@...nel.org>, <pabeni@...hat.com>,
<Lars.Povlsen@...rochip.com>, <Steen.Hegelund@...rochip.com>,
<UNGLinuxDriver@...rochip.com>, <joe@...ches.com>,
<error27@...il.com>, <Horatiu.Vultur@...rochip.com>,
<Julia.Lawall@...ia.fr>, <vladimir.oltean@....com>,
<maxime.chevallier@...tlin.com>,
<linux-arm-kernel@...ts.infradead.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next v2 3/6] net: dcb: add new rewrite table
> > + rewr = nla_nest_start_noflag(skb, DCB_ATTR_DCB_REWR_TABLE);
> > + if (!rewr)
> > + return -EMSGSIZE;
>
> This being new code, don't use _noflag please.
Ack.
>
> > +
> > + spin_lock_bh(&dcb_lock);
> > + list_for_each_entry(itr, &dcb_rewr_list, list) {
> > + if (itr->ifindex == netdev->ifindex) {
> > + enum ieee_attrs_app type =
> > + dcbnl_app_attr_type_get(itr->app.selector);
> > + err = nla_put(skb, type, sizeof(itr->app), &itr->app);
> > + if (err) {
> > + spin_unlock_bh(&dcb_lock);
>
> This should cancel the nest started above.
Yes, it should.
>
> I wonder if it would be cleaner in a separate function, so that there
> can be a dedicated clean-up block to goto.
Well yes. That would make sense if the function were reused for both APP
and rewr.
Though in the APP equivalent code, nla_nest_start_noflag is used, and
dcbnl_ops->getdcbx() is called. Is there any userspace side-effect of
using nla_nest_start for APP too?
dcbnl_ops->getdcbx() would then be left outside of the shared function.
Does that call even have to hold the dcb_lock? Not as far as I can tell.
something like:
err = dcbnl_app_table_get(ndev, skb, &dcb_app_list,
DCB_ATTR_IEEE_APP_TABLE);
if (err)
return -EMSGSIZE;
err = dcbnl_app_table_get(ndev, skb, &dcb_rewr_list,
DCB_ATTR_DCB_REWR_TABLE);
if (err)
return -EMSGSIZE;
if (netdev->dcbnl_ops->getdcbx)
dcbx = netdev->dcbnl_ops->getdcbx(netdev); <-- without lock held
else
dcbx = -EOPNOTSUPP;
Let me hear your thoughts.
Powered by blists - more mailing lists