From: Leon Romanovsky <leon@kernel.org>
To: intel-wired-lan@osuosl.org
Subject: [Intel-wired-lan] [PATCH net-next 5/6] devlink: Reshuffle resource registration logic
Date: Fri, 19 Nov 2021 17:38:53 +0200 [thread overview]
Message-ID: <YZfFDSnnjOG+wSyK@unreal> (raw)
In-Reply-To: <20211118174813.54c3731f@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
On Thu, Nov 18, 2021 at 05:48:13PM -0800, Jakub Kicinski wrote:
> On Thu, 18 Nov 2021 09:50:20 +0200 Leon Romanovsky wrote:
> > And it shouldn't. devlink_resource_find() will return valid resource only
> > if there driver is completely bogus with races or incorrect allocations of
> > resource_id.
> >
> > devlink_*_register(..)
> > mutex_lock(&devlink->lock);
> > if (devlink_*_find(...)) {
> > mutex_unlock(&devlink->lock);
> > return ....;
> > }
> > .....
> >
> > It is almost always wrong from locking and layering perspective the pattern above,
> > as it is racy by definition if not protected by top layer.
> >
> > There are exceptions from the rule above, but devlink is clearly not the
> > one of such exceptions.
>
> Just drop the unnecessary "cleanup" patches and limit the amount
> of driver code we'll have to revert if your approach fails.
My approach works, exactly like it works in other subsystems.
https://lore.kernel.org/netdev/cover.1636390483.git.leonro at nvidia.com/
We are waiting to see your proposal extended to support parallel devlink
execution and to be applied to real drivers.
https://lore.kernel.org/netdev/20211030231254.2477599-1-kuba at kernel.org/
Anyway, you are maintainer, you want half work, you will get half work.
>
> I spent enough time going back and forth with you.
>
> Please.
Disagreements are hard for everyone, not only for you.
Thanks
WARNING: multiple messages have this Message-ID (diff)
From: Leon Romanovsky <leon@kernel.org>
To: Jakub Kicinski <kuba@kernel.org>
Cc: "David S . Miller" <davem@davemloft.net>,
Alexandre Belloni <alexandre.belloni@bootlin.com>,
Andrew Lunn <andrew@lunn.ch>, Aya Levin <ayal@mellanox.com>,
Claudiu Manoil <claudiu.manoil@nxp.com>,
drivers@pensando.io, Florian Fainelli <f.fainelli@gmail.com>,
Ido Schimmel <idosch@nvidia.com>,
intel-wired-lan@lists.osuosl.org,
Ioana Ciornei <ioana.ciornei@nxp.com>,
Jesse Brandeburg <jesse.brandeburg@intel.com>,
Jiri Pirko <jiri@nvidia.com>,
linux-kernel@vger.kernel.org, linux-rdma@vger.kernel.org,
Michael Chan <michael.chan@broadcom.com>,
netdev@vger.kernel.org, oss-drivers@corigine.com,
Saeed Mahameed <saeedm@nvidia.com>,
Shannon Nelson <snelson@pensando.io>,
Simon Horman <simon.horman@corigine.com>,
Taras Chornyi <tchornyi@marvell.com>,
Tariq Toukan <tariqt@nvidia.com>,
Tony Nguyen <anthony.l.nguyen@intel.com>,
UNGLinuxDriver@microchip.com,
Vivien Didelot <vivien.didelot@gmail.com>,
Vladimir Oltean <vladimir.oltean@nxp.com>
Subject: Re: [PATCH net-next 5/6] devlink: Reshuffle resource registration logic
Date: Fri, 19 Nov 2021 17:38:53 +0200 [thread overview]
Message-ID: <YZfFDSnnjOG+wSyK@unreal> (raw)
In-Reply-To: <20211118174813.54c3731f@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
On Thu, Nov 18, 2021 at 05:48:13PM -0800, Jakub Kicinski wrote:
> On Thu, 18 Nov 2021 09:50:20 +0200 Leon Romanovsky wrote:
> > And it shouldn't. devlink_resource_find() will return valid resource only
> > if there driver is completely bogus with races or incorrect allocations of
> > resource_id.
> >
> > devlink_*_register(..)
> > mutex_lock(&devlink->lock);
> > if (devlink_*_find(...)) {
> > mutex_unlock(&devlink->lock);
> > return ....;
> > }
> > .....
> >
> > It is almost always wrong from locking and layering perspective the pattern above,
> > as it is racy by definition if not protected by top layer.
> >
> > There are exceptions from the rule above, but devlink is clearly not the
> > one of such exceptions.
>
> Just drop the unnecessary "cleanup" patches and limit the amount
> of driver code we'll have to revert if your approach fails.
My approach works, exactly like it works in other subsystems.
https://lore.kernel.org/netdev/cover.1636390483.git.leonro@nvidia.com/
We are waiting to see your proposal extended to support parallel devlink
execution and to be applied to real drivers.
https://lore.kernel.org/netdev/20211030231254.2477599-1-kuba@kernel.org/
Anyway, you are maintainer, you want half work, you will get half work.
>
> I spent enough time going back and forth with you.
>
> Please.
Disagreements are hard for everyone, not only for you.
Thanks
next prev parent reply other threads:[~2021-11-19 15:38 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-17 18:26 [Intel-wired-lan] [PATCH net-next 0/6] Devlink cleanups Leon Romanovsky
2021-11-17 18:26 ` Leon Romanovsky
2021-11-17 18:26 ` [Intel-wired-lan] [PATCH net-next 1/6] devlink: Remove misleading internal_flags from health reporter dump Leon Romanovsky
2021-11-17 18:26 ` Leon Romanovsky
2021-11-17 18:26 ` [Intel-wired-lan] [PATCH net-next 2/6] devlink: Delete useless checks of holding devlink lock Leon Romanovsky
2021-11-17 18:26 ` Leon Romanovsky
2021-11-17 18:26 ` [Intel-wired-lan] [PATCH net-next 3/6] devlink: Simplify devlink resources unregister call Leon Romanovsky
2021-11-17 18:26 ` Leon Romanovsky
2021-11-17 18:26 ` [Intel-wired-lan] [PATCH net-next 4/6] devlink: Clean registration of devlink port Leon Romanovsky
2021-11-17 18:26 ` Leon Romanovsky
2021-11-18 4:49 ` [Intel-wired-lan] " Jakub Kicinski
2021-11-18 4:49 ` Jakub Kicinski
2021-11-18 7:32 ` [Intel-wired-lan] " Leon Romanovsky
2021-11-18 7:32 ` Leon Romanovsky
2021-11-17 18:26 ` [Intel-wired-lan] [PATCH net-next 5/6] devlink: Reshuffle resource registration logic Leon Romanovsky
2021-11-17 18:26 ` Leon Romanovsky
2021-11-18 4:49 ` [Intel-wired-lan] " Jakub Kicinski
2021-11-18 4:49 ` Jakub Kicinski
2021-11-18 7:50 ` [Intel-wired-lan] " Leon Romanovsky
2021-11-18 7:50 ` Leon Romanovsky
2021-11-19 1:48 ` [Intel-wired-lan] " Jakub Kicinski
2021-11-19 1:48 ` Jakub Kicinski
2021-11-19 15:38 ` Leon Romanovsky [this message]
2021-11-19 15:38 ` Leon Romanovsky
2021-11-19 16:10 ` [Intel-wired-lan] " Jakub Kicinski
2021-11-19 16:10 ` Jakub Kicinski
2021-11-21 8:45 ` [Intel-wired-lan] " Leon Romanovsky
2021-11-21 8:45 ` Leon Romanovsky
2021-11-23 2:27 ` [Intel-wired-lan] " Jakub Kicinski
2021-11-23 2:27 ` Jakub Kicinski
2021-11-23 8:33 ` [Intel-wired-lan] " Leon Romanovsky
2021-11-23 8:33 ` Leon Romanovsky
2021-11-23 23:33 ` [Intel-wired-lan] " Jakub Kicinski
2021-11-23 23:33 ` Jakub Kicinski
2021-11-25 9:02 ` [Intel-wired-lan] " Leon Romanovsky
2021-11-25 9:02 ` Leon Romanovsky
2021-11-17 18:26 ` [Intel-wired-lan] [PATCH net-next 6/6] devlink: Inline sb related functions Leon Romanovsky
2021-11-17 18:26 ` Leon Romanovsky
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=YZfFDSnnjOG+wSyK@unreal \
--to=leon@kernel.org \
--cc=intel-wired-lan@osuosl.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.