Re: [patch net-next RFC] net: dsa: move port_setup/teardown to be called outside devlink port registered area

Jiri Pirko
2022-07-26T14:46:31Z

Tue, Jul 26, 2022 at 03:43:09PM CEST, olteanv@gmail.com wrote:
>Hi Jiri,
>
>On Tue, Jul 26, 2022 at 02:41:05PM +0200, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@nvidia.com>
>>
>> Move port_setup() op to be called before devlink_port_register() and
>> port_teardown() after devlink_port_unregister().
>>
>> RFC note: I don't see why this would not work, but I have no way to
>> test this does not break things. But I think it makes sense to move this
>> alongside the rest of the devlink port code, the reinit() function
>> also gets much nicer, as clearly the fact that
>> port_setup()->devlink_port_region_create() was called in dsa_port_setup
>> did not fit the flow.
>>
>> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
>> ---
>
>devlink_port->devlink isn't set (it's set in devl_port_register), so
>when devlink_port_region_create() calls devl_lock(devlink), it blasts
>right through that NULL pointer:
>
>[ 4.966960] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000320
>[ 5.009201] [0000000000000320] user address but active_mm is swapper
>[ 5.015616] Internal error: Oops: 96000004 [#1] PREEMPT SMP
>[ 5.024244] CPU: 1 PID: 8 Comm: kworker/u4:0 Not tainted 5.19.0-rc7-07010-ga9b9500ffaac-dirty #3395
>[ 5.033281] Hardware name: CZ.NIC Turris Mox Board (DT)
>[ 5.038499] Workqueue: events_unbound deferred_probe_work_func
>[ 5.044342] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>[ 5.051297] pc : __mutex_lock+0x5c/0x460
>[ 5.055220] lr : __mutex_lock+0x50/0x460
>[ 5.133818] Call trace:
>[ 5.136258] __mutex_lock+0x5c/0x460
>[ 5.139831] mutex_lock_nested+0x40/0x50
>[ 5.143750] devlink_port_region_create+0x54/0x15c
>[ 5.148542] dsa_devlink_port_region_create+0x64/0x90
>[ 5.153592] mv88e6xxx_setup_devlink_regions_port+0x30/0x60
>[ 5.159165] mv88e6xxx_port_setup+0x10/0x20
>[ 5.163345] dsa_port_devlink_setup+0x60/0x150
>[ 5.167786] dsa_register_switch+0x938/0x119c
>[ 5.172138] mv88e6xxx_probe+0x714/0x770
>[ 5.176058] mdio_probe+0x34/0x70
>[ 5.179370] really_probe.part.0+0x9c/0x2ac
>[ 5.183550] __driver_probe_device+0x98/0x144
>[ 5.187902] driver_probe_device+0xac/0x14c

Oh yes, could you please try together with following patch? (nevermind
chicken-egg problem you may spot now)

Subject: [patch net-next RFC] net: devlink: convert region creation/destroy()
to be forbidden on registered devlink/port

No need to create or destroy region when devlink or devlink ports are
registered. Limit the possibility to call the region create/destroy()
only for non-registered devlink or devlink port. Benefit from that and
avoid need to take devl_lock.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
drivers/net/netdevsim/dev.c | 8 ++--
include/net/devlink.h | 5 ---
net/core/devlink.c | 78 ++++++++-----------------------------
3 files changed, 20 insertions(+), 71 deletions(-)

diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index 925dc8a5254d..3f0c19e30650 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c
@@ -557,15 +557,15 @@ static int nsim_dev_dummy_region_init(struct nsim_dev *nsim_dev,
struct devlink *devlink)
{
nsim_dev->dummy_region =
- devl_region_create(devlink, &dummy_region_ops,
- NSIM_DEV_DUMMY_REGION_SNAPSHOT_MAX,
- NSIM_DEV_DUMMY_REGION_SIZE);
+ devlink_region_create(devlink, &dummy_region_ops,
+ NSIM_DEV_DUMMY_REGION_SNAPSHOT_MAX,
+ NSIM_DEV_DUMMY_REGION_SIZE);
return PTR_ERR_OR_ZERO(nsim_dev->dummy_region);
}

static void nsim_dev_dummy_region_exit(struct nsim_dev *nsim_dev)
{
- devl_region_destroy(nsim_dev->dummy_region);
+ devlink_region_destroy(nsim_dev->dummy_region);
}

static int
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 9edb4a28cf30..2416750e050d 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -1666,10 +1666,6 @@ int devlink_param_driverinit_value_get(struct devlink *devlink, u32 param_id,
int devlink_param_driverinit_value_set(struct devlink *devlink, u32 param_id,
union devlink_param_value init_val);
void devlink_param_value_changed(struct devlink *devlink, u32 param_id);
-struct devlink_region *devl_region_create(struct devlink *devlink,
- const struct devlink_region_ops *ops,
- u32 region_max_snapshots,
- u64 region_size);
struct devlink_region *
devlink_region_create(struct devlink *devlink,
const struct devlink_region_ops *ops,
@@ -1678,7 +1674,6 @@ struct devlink_region *
devlink_port_region_create(struct devlink_port *port,
const struct devlink_port_region_ops *ops,
u32 region_max_snapshots, u64 region_size);
-void devl_region_destroy(struct devlink_region *region);
void devlink_region_destroy(struct devlink_region *region);
void devlink_port_region_destroy(struct devlink_region *region);

diff --git a/net/core/devlink.c b/net/core/devlink.c
index 4e0c4f9265e8..15d28aba69fc 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -5701,8 +5701,7 @@ static void devlink_nl_region_notify(struct devlink_region *region,
struct sk_buff *msg;

WARN_ON(cmd != DEVLINK_CMD_REGION_NEW && cmd != DEVLINK_CMD_REGION_DEL);
- if (!xa_get_mark(&devlinks, devlink->index, DEVLINK_REGISTERED))
- return;
+ ASSERT_DEVLINK_REGISTERED(devlink);

msg = devlink_nl_region_notify_build(region, snapshot, cmd, 0, 0);
if (IS_ERR(msg))
@@ -11131,21 +11130,22 @@ void devlink_param_value_changed(struct devlink *devlink, u32 param_id)
EXPORT_SYMBOL_GPL(devlink_param_value_changed);

/**
- * devl_region_create - create a new address region
+ * devlink_region_create - create a new address region
*
* @devlink: devlink
* @ops: region operations and name
* @region_max_snapshots: Maximum supported number of snapshots for region
* @region_size: size of region
*/
-struct devlink_region *devl_region_create(struct devlink *devlink,
- const struct devlink_region_ops *ops,
- u32 region_max_snapshots,
- u64 region_size)
+struct devlink_region *
+devlink_region_create(struct devlink *devlink,
+ const struct devlink_region_ops *ops,
+ u32 region_max_snapshots,
+ u64 region_size)
{
struct devlink_region *region;

- devl_assert_locked(devlink);
+ ASSERT_DEVLINK_NOT_REGISTERED(devlink);

if (WARN_ON(!ops) || WARN_ON(!ops->destructor))
return ERR_PTR(-EINVAL);
@@ -11164,35 +11164,9 @@ struct devlink_region *devl_region_create(struct devlink *devlink,
INIT_LIST_HEAD(&region->snapshot_list);
mutex_init(&region->snapshot_lock);
list_add_tail(&region->list, &devlink->region_list);
- devlink_nl_region_notify(region, NULL, DEVLINK_CMD_REGION_NEW);

return region;
}
-EXPORT_SYMBOL_GPL(devl_region_create);
-
-/**
- * devlink_region_create - create a new address region
- *
- * @devlink: devlink
- * @ops: region operations and name
- * @region_max_snapshots: Maximum supported number of snapshots for region
- * @region_size: size of region
- *
- * Context: Takes and release devlink->lock <mutex>.
- */
-struct devlink_region *
-devlink_region_create(struct devlink *devlink,
- const struct devlink_region_ops *ops,
- u32 region_max_snapshots, u64 region_size)
-{
- struct devlink_region *region;
-
- devl_lock(devlink);
- region = devl_region_create(devlink, ops, region_max_snapshots,
- region_size);
- devl_unlock(devlink);
- return region;
-}
EXPORT_SYMBOL_GPL(devlink_region_create);

/**
@@ -11202,8 +11176,6 @@ EXPORT_SYMBOL_GPL(devlink_region_create);
* @ops: region operations and name
* @region_max_snapshots: Maximum supported number of snapshots for region
* @region_size: size of region
- *
- * Context: Takes and release devlink->lock <mutex>.
*/
struct devlink_region *
devlink_port_region_create(struct devlink_port *port,
@@ -11214,11 +11186,11 @@ devlink_port_region_create(struct devlink_port *port,
struct devlink_region *region;
int err = 0;

+ ASSERT_DEVLINK_PORT_NOT_REGISTERED(port);
+
if (WARN_ON(!ops) || WARN_ON(!ops->destructor))
return ERR_PTR(-EINVAL);

- devl_lock(devlink);
-
if (devlink_port_region_get_by_name(port, ops->name)) {
err = -EEXIST;
goto unlock;
@@ -11238,9 +11210,7 @@ devlink_port_region_create(struct devlink_port *port,
INIT_LIST_HEAD(&region->snapshot_list);
mutex_init(&region->snapshot_lock);
list_add_tail(&region->list, &port->region_list);
- devlink_nl_region_notify(region, NULL, DEVLINK_CMD_REGION_NEW);

- devl_unlock(devlink);
return region;

unlock:
@@ -11250,16 +11220,18 @@ devlink_port_region_create(struct devlink_port *port,
EXPORT_SYMBOL_GPL(devlink_port_region_create);

/**
- * devl_region_destroy - destroy address region
+ * devlink_region_destroy - destroy address region
*
* @region: devlink region to destroy
*/
-void devl_region_destroy(struct devlink_region *region)
+void devlink_region_destroy(struct devlink_region *region)
{
- struct devlink *devlink = region->devlink;
struct devlink_snapshot *snapshot, *ts;

- devl_assert_locked(devlink);
+ if (region->port)
+ ASSERT_DEVLINK_PORT_NOT_REGISTERED(region->port);
+ else
+ ASSERT_DEVLINK_NOT_REGISTERED(region->devlink);

/* Free all snapshots of region */
list_for_each_entry_safe(snapshot, ts, &region->snapshot_list, list)
@@ -11268,26 +11240,8 @@ void devl_region_destroy(struct devlink_region *region)
list_del(&region->list);
mutex_destroy(&region->snapshot_lock);

- devlink_nl_region_notify(region, NULL, DEVLINK_CMD_REGION_DEL);
kfree(region);
}
-EXPORT_SYMBOL_GPL(devl_region_destroy);
-
-/**
- * devlink_region_destroy - destroy address region
- *
- * @region: devlink region to destroy
- *
- * Context: Takes and release devlink->lock <mutex>.
- */
-void devlink_region_destroy(struct devlink_region *region)
-{
- struct devlink *devlink = region->devlink;
-
- devl_lock(devlink);
- devl_region_destroy(region);
- devl_unlock(devlink);
-}
EXPORT_SYMBOL_GPL(devlink_region_destroy);

/**
--
2.35.3