| Message ID | 20220825103400.1356995-1-jiri@resnulli.us (mailing list archive) |
|---|---|
| Headers | show |
| Series | devlink: sanitize per-port region creation/destruction | expand |
On Thu, 25 Aug 2022 12:33:53 +0200 Jiri Pirko wrote: > Currently the only user of per-port devlink regions is DSA. All drivers > that use regions register them before devlink registration. For DSA, > this was not possible as the internals of struct devlink_port needed for > region creation are initialized during port registration. > > This introduced a mismatch in creation flow of devlink and devlink port > regions. As you can see, it causes the DSA driver to make the port > init/exit flow a bit cumbersome. > > Fix this by introducing port_init/fini() which could be optionally > called by drivers like DSA, to prepare struct devlink_port to be used > for region creation purposes before devlink port register is called. > > Force similar limitation as for devlink params and disallow to create > regions after devlink or devlink port are registered. That allows to > simplify the devlink region internal API to not to rely on > devlink->lock. The point of exposing the devlink lock was to avoid forcing drivers to order object registration in a specific way. I don't like. Please provide some solid motivation here 'cause if it's you like vs I like... Also the patches don't apply.
Fri, Aug 26, 2022 at 12:01:32AM CEST, kuba@kernel.org wrote: >On Thu, 25 Aug 2022 12:33:53 +0200 Jiri Pirko wrote: >> Currently the only user of per-port devlink regions is DSA. All drivers >> that use regions register them before devlink registration. For DSA, >> this was not possible as the internals of struct devlink_port needed for >> region creation are initialized during port registration. >> >> This introduced a mismatch in creation flow of devlink and devlink port >> regions. As you can see, it causes the DSA driver to make the port >> init/exit flow a bit cumbersome. >> >> Fix this by introducing port_init/fini() which could be optionally >> called by drivers like DSA, to prepare struct devlink_port to be used >> for region creation purposes before devlink port register is called. >> >> Force similar limitation as for devlink params and disallow to create >> regions after devlink or devlink port are registered. That allows to >> simplify the devlink region internal API to not to rely on >> devlink->lock. > >The point of exposing the devlink lock was to avoid forcing drivers >to order object registration in a specific way. I don't like. Well for params, we are also forcing them in a specific way. The regions, with the DSA exception which is not voluntary, don't need to be created/destroyed during devlink/port being registered. I try to bring some order to the a bit messy devlink world. The intention is to make everyone's live happier :) > >Please provide some solid motivation here 'cause if it's you like vs >I like... > >Also the patches don't apply.
On Fri, 26 Aug 2022 10:21:25 +0200 Jiri Pirko wrote: >> The point of exposing the devlink lock was to avoid forcing drivers >> to order object registration in a specific way. I don't like. > > Well for params, we are also forcing them in a specific way. The > regions, with the DSA exception which is not voluntary, don't need to be > created/destroyed during devlink/port being registered. > > I try to bring some order to the a bit messy devlink world. The > intention is to make everyone's live happier :) The way I remember it - we had to keep the ordering on resources for mlx4 because of complicated locking/async nature of events, and since it's a driver for a part which is much EoL we won't go back now and do major surgery, that's fine. But that shouldn't mean that the recommended way of using resources is "hook them up before register". The overall devlink locking ordering should converge towards the "hold devl_lock() around registration of your components, or whenever the device goes out of consistent state".
Sat, Aug 27, 2022 at 02:22:43AM CEST, kuba@kernel.org wrote: >On Fri, 26 Aug 2022 10:21:25 +0200 Jiri Pirko wrote: >>> The point of exposing the devlink lock was to avoid forcing drivers >>> to order object registration in a specific way. I don't like. >> >> Well for params, we are also forcing them in a specific way. The >> regions, with the DSA exception which is not voluntary, don't need to be >> created/destroyed during devlink/port being registered. >> >> I try to bring some order to the a bit messy devlink world. The >> intention is to make everyone's live happier :) > >The way I remember it - we had to keep the ordering on resources for >mlx4 because of complicated locking/async nature of events, and since >it's a driver for a part which is much EoL we won't go back now and do >major surgery, that's fine. > >But that shouldn't mean that the recommended way of using resources is >"hook them up before register". The overall devlink locking ordering >should converge towards the "hold devl_lock() around registration of >your components, or whenever the device goes out of consistent state". As you wish.
From: Jiri Pirko <jiri@nvidia.com> Currently the only user of per-port devlink regions is DSA. All drivers that use regions register them before devlink registration. For DSA, this was not possible as the internals of struct devlink_port needed for region creation are initialized during port registration. This introduced a mismatch in creation flow of devlink and devlink port regions. As you can see, it causes the DSA driver to make the port init/exit flow a bit cumbersome. Fix this by introducing port_init/fini() which could be optionally called by drivers like DSA, to prepare struct devlink_port to be used for region creation purposes before devlink port register is called. Force similar limitation as for devlink params and disallow to create regions after devlink or devlink port are registered. That allows to simplify the devlink region internal API to not to rely on devlink->lock. Tested with dsa_look with out-of-tree patch implementing devlink port regions there kindly provided by Vladimir Oltean. Jiri Pirko (7): netdevsim: don't re-create dummy region during devlink reload net: devlink: introduce port registered assert helper and use it net: devlink: introduce a flag to indicate devlink port being registered net: devlink: add port_init/fini() helpers to allow pre-register/post-unregister functions net: dsa: move port_setup/teardown to be called outside devlink port registered area net: dsa: don't do devlink port setup early net: devlink: convert region create/destroy() to be forbidden on registered devlink/port drivers/net/ethernet/mellanox/mlx4/crdump.c | 20 +- drivers/net/netdevsim/dev.c | 18 +- include/net/devlink.h | 12 +- net/core/devlink.c | 154 ++++++++-------- net/dsa/dsa2.c | 191 ++++++++------------ 5 files changed, 178 insertions(+), 217 deletions(-)