Message ID | 20220726124105.495652-1-jiri@resnulli.us (mailing list archive) |
---|---|
State | RFC |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | [net-next,RFC] net: dsa: move port_setup/teardown to be called outside devlink port registered area | expand |
Context | Check | Description |
---|---|---|
netdev/tree_selection | success | Clearly marked for net-next |
netdev/fixes_present | success | Fixes tag not required for -next series |
netdev/subject_prefix | success | Link |
netdev/cover_letter | success | Single patches do not need cover letters |
netdev/patch_count | success | Link |
netdev/header_inline | success | No static functions without inline keyword in header files |
netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
netdev/cc_maintainers | success | CCed 9 of 9 maintainers |
netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
netdev/module_param | success | Was 0 now: 0 |
netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
netdev/check_selftest | success | No net selftest shell script |
netdev/verify_fixes | success | No Fixes tag |
netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
netdev/checkpatch | warning | WARNING: Duplicate signature |
netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
netdev/source_inline | success | Was 0 now: 0 |
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
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(®ion->snapshot_list); mutex_init(®ion->snapshot_lock); list_add_tail(®ion->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(®ion->snapshot_list); mutex_init(®ion->snapshot_lock); list_add_tail(®ion->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, ®ion->snapshot_list, list) @@ -11268,26 +11240,8 @@ void devl_region_destroy(struct devlink_region *region) list_del(®ion->list); mutex_destroy(®ion->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); /**
On Tue, Jul 26, 2022 at 04:45:44PM +0200, Jiri Pirko wrote: > Oh yes, could you please try together with following patch? (nevermind > chicken-egg problem you may spot now) Duly ignoring ;) > 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(®ion->snapshot_list); > mutex_init(®ion->snapshot_lock); > list_add_tail(®ion->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); > > /** Were you on net-next when you generated this patch? Here's what I have at 11164: devlink_port_region_create: if (devlink_port_region_get_by_name(port, ops->name)) { err = -EEXIST; goto unlock; } region = kzalloc(sizeof(*region), GFP_KERNEL); if (!region) { My end of devl_region_create() and start of devlink_region_create() are at 11106, but notice how I lack the mutex_init(®ion->snapshot_lock) that you have in your context: INIT_LIST_HEAD(®ion->snapshot_list); list_add_tail(®ion->list, &devlink->region_list); devlink_nl_region_notify(region, NULL, DEVLINK_CMD_REGION_NEW); return region; } EXPORT_SYMBOL_GPL(devl_region_create); Would you mind regenerating this patch rather than letting me bodge it?
Tue, Jul 26, 2022 at 05:20:59PM CEST, olteanv@gmail.com wrote: >On Tue, Jul 26, 2022 at 04:45:44PM +0200, Jiri Pirko wrote: >> Oh yes, could you please try together with following patch? (nevermind >> chicken-egg problem you may spot now) > >Duly ignoring ;) > >> 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(®ion->snapshot_list); >> mutex_init(®ion->snapshot_lock); >> list_add_tail(®ion->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); >> >> /** > >Were you on net-next when you generated this patch? Here's what I have >at 11164: > >devlink_port_region_create: > if (devlink_port_region_get_by_name(port, ops->name)) { > err = -EEXIST; > goto unlock; > } > > region = kzalloc(sizeof(*region), GFP_KERNEL); > if (!region) { > >My end of devl_region_create() and start of devlink_region_create() are >at 11106, but notice how I lack the mutex_init(®ion->snapshot_lock) >that you have in your context: > > INIT_LIST_HEAD(®ion->snapshot_list); > list_add_tail(®ion->list, &devlink->region_list); > devlink_nl_region_notify(region, NULL, DEVLINK_CMD_REGION_NEW); > > return region; >} >EXPORT_SYMBOL_GPL(devl_region_create); > >Would you mind regenerating this patch rather than letting me bodge it? Darn, wait. I will fixup a squash for you. Sorry.
Tue, Jul 26, 2022 at 05:57:59PM CEST, jiri@resnulli.us wrote: >Tue, Jul 26, 2022 at 05:20:59PM CEST, olteanv@gmail.com wrote: >>On Tue, Jul 26, 2022 at 04:45:44PM +0200, Jiri Pirko wrote: [..] > >Darn, wait. I will fixup a squash for you. Sorry. Here it is: diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c index 5802e80e8fe1..ddfc03722ab5 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 780744b550b8..6885c8928327 100644 --- a/include/net/devlink.h +++ b/include/net/devlink.h @@ -1665,10 +1665,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, @@ -1677,7 +1673,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 98d79feeb3dc..71219f66da4e 100644 --- a/net/core/devlink.c +++ b/net/core/devlink.c @@ -310,6 +310,11 @@ static struct devlink *devlink_get_from_attrs(struct net *net, return devlink; } +#define ASSERT_DEVLINK_PORT_REGISTERED(devlink_port) \ + WARN_ON_ONCE(!devlink_port->devlink) +#define ASSERT_DEVLINK_PORT_NOT_REGISTERED(devlink_port) \ + WARN_ON_ONCE(devlink_port->devlink) + static struct devlink_port *devlink_port_get_by_index(struct devlink *devlink, unsigned int port_index) { @@ -5646,8 +5651,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)) @@ -9704,7 +9708,8 @@ int devl_port_register(struct devlink *devlink, if (devlink_port_index_exists(devlink, port_index)) return -EEXIST; - WARN_ON(devlink_port->devlink); + ASSERT_DEVLINK_PORT_NOT_REGISTERED(devlink_port); + devlink_port->devlink = devlink; devlink_port->index = port_index; spin_lock_init(&devlink_port->type_lock); @@ -9788,8 +9793,8 @@ static void __devlink_port_type_set(struct devlink_port *devlink_port, enum devlink_port_type type, void *type_dev) { - if (WARN_ON(!devlink_port->devlink)) - return; + ASSERT_DEVLINK_PORT_REGISTERED(devlink_port); + devlink_port_type_warn_cancel(devlink_port); spin_lock_bh(&devlink_port->type_lock); devlink_port->type = type; @@ -9908,8 +9913,8 @@ void devlink_port_attrs_set(struct devlink_port *devlink_port, { int ret; - if (WARN_ON(devlink_port->devlink)) - return; + ASSERT_DEVLINK_PORT_NOT_REGISTERED(devlink_port); + devlink_port->attrs = *attrs; ret = __devlink_port_attrs_set(devlink_port, attrs->flavour); if (ret) @@ -9932,8 +9937,8 @@ void devlink_port_attrs_pci_pf_set(struct devlink_port *devlink_port, u32 contro struct devlink_port_attrs *attrs = &devlink_port->attrs; int ret; - if (WARN_ON(devlink_port->devlink)) - return; + ASSERT_DEVLINK_PORT_NOT_REGISTERED(devlink_port); + ret = __devlink_port_attrs_set(devlink_port, DEVLINK_PORT_FLAVOUR_PCI_PF); if (ret) @@ -9959,8 +9964,8 @@ void devlink_port_attrs_pci_vf_set(struct devlink_port *devlink_port, u32 contro struct devlink_port_attrs *attrs = &devlink_port->attrs; int ret; - if (WARN_ON(devlink_port->devlink)) - return; + ASSERT_DEVLINK_PORT_NOT_REGISTERED(devlink_port); + ret = __devlink_port_attrs_set(devlink_port, DEVLINK_PORT_FLAVOUR_PCI_VF); if (ret) @@ -9987,8 +9992,8 @@ void devlink_port_attrs_pci_sf_set(struct devlink_port *devlink_port, u32 contro struct devlink_port_attrs *attrs = &devlink_port->attrs; int ret; - if (WARN_ON(devlink_port->devlink)) - return; + ASSERT_DEVLINK_PORT_NOT_REGISTERED(devlink_port); + ret = __devlink_port_attrs_set(devlink_port, DEVLINK_PORT_FLAVOUR_PCI_SF); if (ret) @@ -10103,8 +10108,8 @@ EXPORT_SYMBOL_GPL(devl_rate_nodes_destroy); void devlink_port_linecard_set(struct devlink_port *devlink_port, struct devlink_linecard *linecard) { - if (WARN_ON(devlink_port->devlink)) - return; + ASSERT_DEVLINK_PORT_NOT_REGISTERED(devlink_port); + devlink_port->linecard = linecard; } EXPORT_SYMBOL_GPL(devlink_port_linecard_set); @@ -11073,21 +11078,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); @@ -11105,35 +11111,9 @@ struct devlink_region *devl_region_create(struct devlink *devlink, region->size = region_size; INIT_LIST_HEAD(®ion->snapshot_list); list_add_tail(®ion->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); /** @@ -11143,8 +11123,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, @@ -11155,11 +11133,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; @@ -11178,9 +11156,7 @@ devlink_port_region_create(struct devlink_port *port, region->size = region_size; INIT_LIST_HEAD(®ion->snapshot_list); list_add_tail(®ion->list, &port->region_list); - devlink_nl_region_notify(region, NULL, DEVLINK_CMD_REGION_NEW); - devl_unlock(devlink); return region; unlock: @@ -11190,16 +11166,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, ®ion->snapshot_list, list) @@ -11207,26 +11185,8 @@ void devl_region_destroy(struct devlink_region *region) list_del(®ion->list); - 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); /** diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c index cac48a741f27..a8b6c6434df2 100644 --- a/net/dsa/dsa2.c +++ b/net/dsa/dsa2.c @@ -451,19 +451,12 @@ static int dsa_port_setup(struct dsa_port *dp) { struct devlink_port *dlp = &dp->devlink_port; bool dsa_port_link_registered = false; - struct dsa_switch *ds = dp->ds; bool dsa_port_enabled = false; int err = 0; if (dp->setup) return 0; - if (ds->ops->port_setup) { - err = ds->ops->port_setup(ds, dp->index); - if (err) - return err; - } - switch (dp->type) { case DSA_PORT_TYPE_UNUSED: dsa_port_disable(dp); @@ -506,11 +499,6 @@ static int dsa_port_setup(struct dsa_port *dp) dsa_port_disable(dp); if (err && dsa_port_link_registered) dsa_port_link_unregister_of(dp); - if (err) { - if (ds->ops->port_teardown) - ds->ops->port_teardown(ds, dp->index); - return err; - } dp->setup = true; @@ -523,10 +511,17 @@ static int dsa_port_devlink_setup(struct dsa_port *dp) struct dsa_switch_tree *dst = dp->ds->dst; struct devlink_port_attrs attrs = {}; struct devlink *dl = dp->ds->devlink; + struct dsa_switch *ds = dp->ds; const unsigned char *id; unsigned char len; int err; + if (ds->ops->port_setup) { + err = ds->ops->port_setup(ds, dp->index); + if (err) + return err; + } + id = (const unsigned char *)&dst->index; len = sizeof(dst->index); @@ -552,24 +547,23 @@ static int dsa_port_devlink_setup(struct dsa_port *dp) devlink_port_attrs_set(dlp, &attrs); err = devlink_port_register(dl, dlp, dp->index); + if (err) { + if (ds->ops->port_teardown) + ds->ops->port_teardown(ds, dp->index); + return err; + } + dp->devlink_port_setup = true; - if (!err) - dp->devlink_port_setup = true; - - return err; + return 0; } static void dsa_port_teardown(struct dsa_port *dp) { struct devlink_port *dlp = &dp->devlink_port; - struct dsa_switch *ds = dp->ds; if (!dp->setup) return; - if (ds->ops->port_teardown) - ds->ops->port_teardown(ds, dp->index); - devlink_port_type_clear(dlp); switch (dp->type) { @@ -597,40 +591,24 @@ static void dsa_port_teardown(struct dsa_port *dp) static void dsa_port_devlink_teardown(struct dsa_port *dp) { struct devlink_port *dlp = &dp->devlink_port; + struct dsa_switch *ds = dp->ds; - if (dp->devlink_port_setup) + if (dp->devlink_port_setup) { devlink_port_unregister(dlp); + if (ds->ops->port_teardown) + ds->ops->port_teardown(ds, dp->index); + } dp->devlink_port_setup = false; } /* Destroy the current devlink port, and create a new one which has the UNUSED - * flavour. At this point, any call to ds->ops->port_setup has been already - * balanced out by a call to ds->ops->port_teardown, so we know that any - * devlink port regions the driver had are now unregistered. We then call its - * ds->ops->port_setup again, in order for the driver to re-create them on the - * new devlink port. + * flavour. */ static int dsa_port_reinit_as_unused(struct dsa_port *dp) { - struct dsa_switch *ds = dp->ds; - int err; - dsa_port_devlink_teardown(dp); dp->type = DSA_PORT_TYPE_UNUSED; - err = dsa_port_devlink_setup(dp); - if (err) - return err; - - if (ds->ops->port_setup) { - /* On error, leave the devlink port registered, - * dsa_switch_teardown will clean it up later. - */ - err = ds->ops->port_setup(ds, dp->index); - if (err) - return err; - } - - return 0; + return dsa_port_devlink_setup(dp); } static int dsa_devlink_info_get(struct devlink *dl,
On Tue, Jul 26, 2022 at 06:13:11PM +0200, Jiri Pirko wrote:
> Here it is:
Thanks, this one does apply.
We have the same problem, except now it's with port->region_list
(region_create does list_add_tail, port_register does INIT_LIST_HEAD).
I don't think you need to see this anymore, but anyway, here it is.
[ 4.949727] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000010
[ 5.020395] CPU: 1 PID: 8 Comm: kworker/u4:0 Not tainted 5.19.0-rc7-07010-ga9b9500ffaac-dirty #3397
[ 5.047447] pc : devlink_port_region_create+0x6c/0x150
[ 5.052587] lr : dsa_devlink_port_region_create+0x64/0x90
[ 5.057983] sp : ffff80000c17b8b0
[ 5.132669] Call trace:
[ 5.135109] devlink_port_region_create+0x6c/0x150
[ 5.139899] dsa_devlink_port_region_create+0x64/0x90
[ 5.144946] mv88e6xxx_setup_devlink_regions_port+0x30/0x60
[ 5.150520] mv88e6xxx_port_setup+0x10/0x20
[ 5.154700] dsa_port_devlink_setup+0x60/0x150
[ 5.159141] dsa_register_switch+0x938/0x119c
[ 5.163496] mv88e6xxx_probe+0x714/0x770
[ 5.167416] mdio_probe+0x34/0x70
Tue, Jul 26, 2022 at 06:55:20PM CEST, olteanv@gmail.com wrote: >On Tue, Jul 26, 2022 at 06:13:11PM +0200, Jiri Pirko wrote: >> Here it is: > >Thanks, this one does apply. > >We have the same problem, except now it's with port->region_list >(region_create does list_add_tail, port_register does INIT_LIST_HEAD). > >I don't think you need to see this anymore, but anyway, here it is. Thanks, will fix this and send it to you off-list, to avoid spamming people, sorry. Thanks! > >[ 4.949727] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000010 >[ 5.020395] CPU: 1 PID: 8 Comm: kworker/u4:0 Not tainted 5.19.0-rc7-07010-ga9b9500ffaac-dirty #3397 >[ 5.047447] pc : devlink_port_region_create+0x6c/0x150 >[ 5.052587] lr : dsa_devlink_port_region_create+0x64/0x90 >[ 5.057983] sp : ffff80000c17b8b0 >[ 5.132669] Call trace: >[ 5.135109] devlink_port_region_create+0x6c/0x150 >[ 5.139899] dsa_devlink_port_region_create+0x64/0x90 >[ 5.144946] mv88e6xxx_setup_devlink_regions_port+0x30/0x60 >[ 5.150520] mv88e6xxx_port_setup+0x10/0x20 >[ 5.154700] dsa_port_devlink_setup+0x60/0x150 >[ 5.159141] dsa_register_switch+0x938/0x119c >[ 5.163496] mv88e6xxx_probe+0x714/0x770 >[ 5.167416] mdio_probe+0x34/0x70
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c index cac48a741f27..a8b6c6434df2 100644 --- a/net/dsa/dsa2.c +++ b/net/dsa/dsa2.c @@ -451,19 +451,12 @@ static int dsa_port_setup(struct dsa_port *dp) { struct devlink_port *dlp = &dp->devlink_port; bool dsa_port_link_registered = false; - struct dsa_switch *ds = dp->ds; bool dsa_port_enabled = false; int err = 0; if (dp->setup) return 0; - if (ds->ops->port_setup) { - err = ds->ops->port_setup(ds, dp->index); - if (err) - return err; - } - switch (dp->type) { case DSA_PORT_TYPE_UNUSED: dsa_port_disable(dp); @@ -506,11 +499,6 @@ static int dsa_port_setup(struct dsa_port *dp) dsa_port_disable(dp); if (err && dsa_port_link_registered) dsa_port_link_unregister_of(dp); - if (err) { - if (ds->ops->port_teardown) - ds->ops->port_teardown(ds, dp->index); - return err; - } dp->setup = true; @@ -523,10 +511,17 @@ static int dsa_port_devlink_setup(struct dsa_port *dp) struct dsa_switch_tree *dst = dp->ds->dst; struct devlink_port_attrs attrs = {}; struct devlink *dl = dp->ds->devlink; + struct dsa_switch *ds = dp->ds; const unsigned char *id; unsigned char len; int err; + if (ds->ops->port_setup) { + err = ds->ops->port_setup(ds, dp->index); + if (err) + return err; + } + id = (const unsigned char *)&dst->index; len = sizeof(dst->index); @@ -552,24 +547,23 @@ static int dsa_port_devlink_setup(struct dsa_port *dp) devlink_port_attrs_set(dlp, &attrs); err = devlink_port_register(dl, dlp, dp->index); + if (err) { + if (ds->ops->port_teardown) + ds->ops->port_teardown(ds, dp->index); + return err; + } + dp->devlink_port_setup = true; - if (!err) - dp->devlink_port_setup = true; - - return err; + return 0; } static void dsa_port_teardown(struct dsa_port *dp) { struct devlink_port *dlp = &dp->devlink_port; - struct dsa_switch *ds = dp->ds; if (!dp->setup) return; - if (ds->ops->port_teardown) - ds->ops->port_teardown(ds, dp->index); - devlink_port_type_clear(dlp); switch (dp->type) { @@ -597,40 +591,24 @@ static void dsa_port_teardown(struct dsa_port *dp) static void dsa_port_devlink_teardown(struct dsa_port *dp) { struct devlink_port *dlp = &dp->devlink_port; + struct dsa_switch *ds = dp->ds; - if (dp->devlink_port_setup) + if (dp->devlink_port_setup) { devlink_port_unregister(dlp); + if (ds->ops->port_teardown) + ds->ops->port_teardown(ds, dp->index); + } dp->devlink_port_setup = false; } /* Destroy the current devlink port, and create a new one which has the UNUSED - * flavour. At this point, any call to ds->ops->port_setup has been already - * balanced out by a call to ds->ops->port_teardown, so we know that any - * devlink port regions the driver had are now unregistered. We then call its - * ds->ops->port_setup again, in order for the driver to re-create them on the - * new devlink port. + * flavour. */ static int dsa_port_reinit_as_unused(struct dsa_port *dp) { - struct dsa_switch *ds = dp->ds; - int err; - dsa_port_devlink_teardown(dp); dp->type = DSA_PORT_TYPE_UNUSED; - err = dsa_port_devlink_setup(dp); - if (err) - return err; - - if (ds->ops->port_setup) { - /* On error, leave the devlink port registered, - * dsa_switch_teardown will clean it up later. - */ - err = ds->ops->port_setup(ds, dp->index); - if (err) - return err; - } - - return 0; + return dsa_port_devlink_setup(dp); } static int dsa_devlink_info_get(struct devlink *dl,