Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   <Daniel.Machon@microchip.com>
To:     <petrm@nvidia.com>
CC:     <netdev@vger.kernel.org>, <davem@davemloft.net>,
        <edumazet@google.com>, <kuba@kernel.org>, <pabeni@redhat.com>,
        <Lars.Povlsen@microchip.com>, <Steen.Hegelund@microchip.com>,
        <UNGLinuxDriver@microchip.com>, <joe@perches.com>,
        <error27@gmail.com>, <Horatiu.Vultur@microchip.com>,
        <Julia.Lawall@inria.fr>, <vladimir.oltean@nxp.com>,
        <maxime.chevallier@bootlin.com>,
        <linux-arm-kernel@lists.infradead.org>,
        <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH net-next v2 3/6] net: dcb: add new rewrite table
Thread-Topic: [PATCH net-next v2 3/6] net: dcb: add new rewrite table
Thread-Index: AQHZKbm5BtwmhtpMEECQnUzYRY/XFK6kAv+AgAAwfIA=
Date:   Wed, 18 Jan 2023 13:47:57 +0000
Message-ID: <Y8f4i2ablWnNO9Op@DEN-LT-70577>
References: <20230116144853.2446315-1-daniel.machon@microchip.com>
 <20230116144853.2446315-4-daniel.machon@microchip.com>
 <87lem0w1k3.fsf@nvidia.com>
In-Reply-To: <87lem0w1k3.fsf@nvidia.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?Hwv4GjxP2m9k1JjYIMmYBiHI6b4YydfCxH0ymboER8GYNEi5X6OeqrHEMCm1?=
 =?us-ascii?Q?C75gllJARblz1HHjiNNRY1nkTLxBdLTD2dHqqIhy/tXg2UyXzCspDosle6+H?=
 =?us-ascii?Q?GaYd+f8wUFaVKKhvlLIC3gLQA29gK7x8rh8TyG2Wwn9y25ZkwFqSPtOp61Rb?=
 =?us-ascii?Q?ckmNf414JSh8Mu251cV3zq0kkIiRLQTis0TF1ax8Flt9T/y3py7qPKcK/ZwY?=
 =?us-ascii?Q?1/iCbggeXrmsDFI3Src45NcmmyYM7POQ/TjCezV2k/KzreMMoifZpqluZO50?=
 =?us-ascii?Q?5phGKYO3nCMfNjSHMkV1FxGJMmpBktu9VfChmT6budm+Fq2YVXnslZqmOCEG?=
 =?us-ascii?Q?rCGPNcuCJgrdMl8yL5i42ghwDgzJKBVoh/TbZFB51UYwE6eBp85nYIhbpcwB?=
 =?us-ascii?Q?TOlhDrfdofjJbzFAXrBjm73lUf1UhPDwsGRlOBwKTMSDs74iQMkV3UloodRY?=
 =?us-ascii?Q?OaWQNOwb2wF5GIg/ITJmk66+NMPWlMZnc9UvajQdMYBBa3hj4ZSUR7k+FD49?=
 =?us-ascii?Q?n9KH7NM3EFhaokYveKgjNvLuYmcglS9LLzdJeFTg6Kba0O9CmqEN+RWaACly?=
 =?us-ascii?Q?0tXiU7/dMX6G8NXSRYHPGoK5frdzQCzXPXmvnc85rVoo5JEJOXhRv7kFLB+c?=
 =?us-ascii?Q?GECfI6SuktNFsg48I2Ov3kMoh3S9wvUSPYLlRWTgjiMj13V/J69bMsI5YF6B?=
 =?us-ascii?Q?dA7/Gr4SAvuxzlkYJ3NYpC+4ERwXZUblnEhUxVK9CroWCx1CWAn4S8n9+qa4?=
 =?us-ascii?Q?C1ISO+GDddFYYd6EnM9MgicJplN6I0cQBhzXatnkTzOqA2nnnf5ooSj679Vg?=
 =?us-ascii?Q?1uM0voqxeExI3GPT4L/gZwPCU9en/SOd3V8zJVo13+LFFBp0ob/hCEJoJJnw?=
 =?us-ascii?Q?zsP1oJKnOo8wrfH1b/tBO32z7HxY+dGIjOXXyFhNwF936RFKks4dpeNUtTjt?=
 =?us-ascii?Q?FcUr2hbD5bomq46tJ6OX+N2wti3f8KRjgtGbXsUzKSYK0QpO5RTeovd2wJGB?=
 =?us-ascii?Q?TWlguhZCYJ21Wnf3yl21tFhGmX5PP4iKX8PsNUSbvXZPFUcSPadbgUx+Vl5o?=
 =?us-ascii?Q?TGPIDnJSent4hSZ0bc/cXJRCjx2JDATs4Ah06IcGJP7badR+kwLvXEHYWkzO?=
 =?us-ascii?Q?eq2l0Gfv7zaPempm1gYBrUrYkKKufqKr2j+65N2JYtqQXEoTe5juzLhCBzfa?=
 =?us-ascii?Q?i1WpNi88V8s1+Bbd++L2sUZErM8Kk3XjTPaR4Yu95wTpERDOYCrEHnwBcL14?=
 =?us-ascii?Q?9XQ6ShPmCKKlNbbF2iVjRpeUcpR5SCZ6tkoIkbVSxOro0rUIHcZOYXy8usOu?=
 =?us-ascii?Q?iSaSSkc/YABo7iDLJKDAk964A3zAYxsCM9rF26g7N3oXKLpfHtp57rva0W/N?=
 =?us-ascii?Q?s6HSmLvs/TKorws3EFGg3Qs/0rKtFz8k3qY984JxkSAVxBEM7RFVqiqEPASN?=
 =?us-ascii?Q?0S5rTykTXRY+8iQQR828peMJtRsCekRKki/dB3tA+m7jLe28V7LWXrI2RwiA?=
 =?us-ascii?Q?1dPa9PNCuSgJyhJFkXPGz3+ixMEmxQzbvS8+5b5xLeuS4+yLkRKT+2v3lU7Q?=
 =?us-ascii?Q?pg3bqT5qdUoWeGYae17w02weyZ4F7SGld2tdeRSCpCTbs5XfywM1YroZAlAw?=
 =?us-ascii?Q?1Br9MuMKvEJLEQLJOZ0nJpU=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <CAFD1C91A3E425458FEB53CCF15CE29E@namprd11.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: CO6PR11MB5569.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b025c496-875a-4107-1db4-08daf95a9b6a
X-MS-Exchange-CrossTenant-originalarrivaltime: 18 Jan 2023 13:47:57.5517
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 3f4057f3-b418-4d4e-ba84-d55b4e897d88
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: JTSPfS/1zAAix1LnieDkXrethudKD/H1gy8mxmZPM4lKK415cYEnzcKCL4jpeY7MyNBAfbBh0vsxOggPw59+foOJGsXJEoylje5S06ODIeM=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB5552
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> > +     rewr =3D nla_nest_start_noflag(skb, DCB_ATTR_DCB_REWR_TABLE);
> > +     if (!rewr)
> > +             return -EMSGSIZE;
>=20
> This being new code, don't use _noflag please.

Ack.

>=20
> > +
> > +     spin_lock_bh(&dcb_lock);
> > +     list_for_each_entry(itr, &dcb_rewr_list, list) {
> > +             if (itr->ifindex =3D=3D netdev->ifindex) {
> > +                     enum ieee_attrs_app type =3D
> > +                             dcbnl_app_attr_type_get(itr->app.selector=
);
> > +                     err =3D nla_put(skb, type, sizeof(itr->app), &itr=
->app);
> > +                     if (err) {
> > +                             spin_unlock_bh(&dcb_lock);
>=20
> This should cancel the nest started above.

Yes, it should.

>=20
> 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 =3D dcbnl_app_table_get(ndev, skb, &dcb_app_list,
			  DCB_ATTR_IEEE_APP_TABLE);
if (err)
	return -EMSGSIZE;

err =3D dcbnl_app_table_get(ndev, skb, &dcb_rewr_list,
			  DCB_ATTR_DCB_REWR_TABLE);
if (err)
        return -EMSGSIZE;

if (netdev->dcbnl_ops->getdcbx)
	dcbx =3D netdev->dcbnl_ops->getdcbx(netdev); <-- without lock held
else
	dcbx =3D -EOPNOTSUPP;

Let me hear your thoughts.