| Message ID | 20230526102841.2226553-15-jiri@resnulli.us (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 216ba9f4adc8f2e452edb9a58d2dfbfc11608c00 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | devlink: move port ops into separate structure | expand |
On Fri, 26 May 2023 12:28:40 +0200 Jiri Pirko wrote:
> Move port_del() from devlink_ops into newly introduced devlink_port_ops.
I didn't think this thru last time, I thought port_new will move
in another patch, but that's impossible (obviously?).
Isn't it kinda weird that the new callback is in one place and del
callback is in another? Asymmetric ?
Sat, May 27, 2023 at 06:10:08AM CEST, kuba@kernel.org wrote: >On Fri, 26 May 2023 12:28:40 +0200 Jiri Pirko wrote: >> Move port_del() from devlink_ops into newly introduced devlink_port_ops. > >I didn't think this thru last time, I thought port_new will move >in another patch, but that's impossible (obviously?). > >Isn't it kinda weird that the new callback is in one place and del >callback is in another? Asymmetric ? Yeah, I don't know how to do it differently. port_new() has to be devlink op, as it operates not on the port but on the device. However, port_del() operates on device. I was thinking about changing the name of port_del() to port_destructor() or something like that which would make the symmetricity issue bit less visible. IDK, up to you. One way or another, I think this could be easily done as a follow-up (I have 15 patches now already anyway). Thanks!
On Sat, 27 May 2023 09:42:45 +0200 Jiri Pirko wrote: > >I didn't think this thru last time, I thought port_new will move > >in another patch, but that's impossible (obviously?). > > > >Isn't it kinda weird that the new callback is in one place and del > >callback is in another? Asymmetric ? > > Yeah, I don't know how to do it differently. port_new() has to be > devlink op, as it operates not on the port but on the device. However, > port_del() operates on device. I was thinking about changing the name of > port_del() to port_destructor() or something like that which would make > the symmetricity issue bit less visible. IDK, up to you. One way or > another, I think this could be easily done as a follow-up (I have 15 > patches now already anyway). One could argue logically removing a port is also an operation of the parent (i.e. the devlink instance). The fact that the port gets destroyed in the process is secondary. Ergo maybe we should skip this patch?
Mon, May 29, 2023 at 08:33:34AM CEST, kuba@kernel.org wrote: >On Sat, 27 May 2023 09:42:45 +0200 Jiri Pirko wrote: >> >I didn't think this thru last time, I thought port_new will move >> >in another patch, but that's impossible (obviously?). >> > >> >Isn't it kinda weird that the new callback is in one place and del >> >callback is in another? Asymmetric ? >> >> Yeah, I don't know how to do it differently. port_new() has to be >> devlink op, as it operates not on the port but on the device. However, >> port_del() operates on device. I was thinking about changing the name of >> port_del() to port_destructor() or something like that which would make >> the symmetricity issue bit less visible. IDK, up to you. One way or >> another, I think this could be easily done as a follow-up (I have 15 >> patches now already anyway). > >One could argue logically removing a port is also an operation of >the parent (i.e. the devlink instance). The fact that the port gets >destroyed in the process is secondary. Ergo maybe we should skip >this patch? Well, the port_del() could differ for different port flavours. The embedding structure of struct devlink_port is also different. Makes sense to me to skip the flavour switch and have one port_del() for each port.
On Mon, 29 May 2023 10:31:14 +0200 Jiri Pirko wrote: > >One could argue logically removing a port is also an operation of > >the parent (i.e. the devlink instance). The fact that the port gets > >destroyed in the process is secondary. Ergo maybe we should skip > >this patch? > > Well, the port_del() could differ for different port flavours. The > embedding structure of struct devlink_port is also different. > > Makes sense to me to skip the flavour switch and have one port_del() for > each port. The asymmetry bothers me. It's hard to comment on what the best approach is given this series shows no benefit of moving port_del(). Maybe even a loss, as mlx5 now has an ifdef in two places: > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/devlink.c b/drivers/net/ethernet/mellanox/mlx5/core/devlink.c > index e39fd85ea2f9..63635cc44479 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/devlink.c > +++ b/drivers/net/ethernet/mellanox/mlx5/core/devlink.c > @@ -320,7 +320,6 @@ static const struct devlink_ops mlx5_devlink_ops = { > #endif > #ifdef CONFIG_MLX5_SF_MANAGER > .port_new = mlx5_devlink_sf_port_new, > - .port_del = mlx5_devlink_sf_port_del, > #endif > .flash_update = mlx5_devlink_flash_update, > .info_get = mlx5_devlink_info_get, > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/esw/devlink_port.c b/drivers/net/ethernet/mellanox/mlx5/core/esw/devlink_port.c > index 76c5d6e9d47f..f370f67d9e33 100644 > --- a/drivers/net/ethernet/mellanox/mlx5/core/esw/devlink_port.c > +++ b/drivers/net/ethernet/mellanox/mlx5/core/esw/devlink_port.c > @@ -145,6 +145,9 @@ struct devlink_port *mlx5_esw_offloads_devlink_port(struct mlx5_eswitch *esw, u1 > } > > static const struct devlink_port_ops mlx5_esw_dl_sf_port_ops = { > +#ifdef CONFIG_MLX5_SF_MANAGER > + .port_del = mlx5_devlink_sf_port_del, > +#endif > .port_fn_hw_addr_get = mlx5_devlink_port_fn_hw_addr_get, > .port_fn_hw_addr_set = mlx5_devlink_port_fn_hw_addr_set, > .port_fn_roce_get = mlx5_devlink_port_fn_roce_get, Is it okay if we deferred the port_del() patch until there's some clear benefit?
Tue, May 30, 2023 at 03:41:19AM CEST, kuba@kernel.org wrote: >On Mon, 29 May 2023 10:31:14 +0200 Jiri Pirko wrote: >> >One could argue logically removing a port is also an operation of >> >the parent (i.e. the devlink instance). The fact that the port gets >> >destroyed in the process is secondary. Ergo maybe we should skip >> >this patch? >> >> Well, the port_del() could differ for different port flavours. The >> embedding structure of struct devlink_port is also different. >> >> Makes sense to me to skip the flavour switch and have one port_del() for >> each port. > >The asymmetry bothers me. It's hard to comment on what the best >approach is given this series shows no benefit of moving port_del(). >Maybe even a loss, as mlx5 now has an ifdef in two places: > >> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/devlink.c b/drivers/net/ethernet/mellanox/mlx5/core/devlink.c >> index e39fd85ea2f9..63635cc44479 100644 >> --- a/drivers/net/ethernet/mellanox/mlx5/core/devlink.c >> +++ b/drivers/net/ethernet/mellanox/mlx5/core/devlink.c >> @@ -320,7 +320,6 @@ static const struct devlink_ops mlx5_devlink_ops = { >> #endif >> #ifdef CONFIG_MLX5_SF_MANAGER >> .port_new = mlx5_devlink_sf_port_new, >> - .port_del = mlx5_devlink_sf_port_del, >> #endif >> .flash_update = mlx5_devlink_flash_update, >> .info_get = mlx5_devlink_info_get, >> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/esw/devlink_port.c b/drivers/net/ethernet/mellanox/mlx5/core/esw/devlink_port.c >> index 76c5d6e9d47f..f370f67d9e33 100644 >> --- a/drivers/net/ethernet/mellanox/mlx5/core/esw/devlink_port.c >> +++ b/drivers/net/ethernet/mellanox/mlx5/core/esw/devlink_port.c >> @@ -145,6 +145,9 @@ struct devlink_port *mlx5_esw_offloads_devlink_port(struct mlx5_eswitch *esw, u1 >> } >> >> static const struct devlink_port_ops mlx5_esw_dl_sf_port_ops = { >> +#ifdef CONFIG_MLX5_SF_MANAGER >> + .port_del = mlx5_devlink_sf_port_del, >> +#endif >> .port_fn_hw_addr_get = mlx5_devlink_port_fn_hw_addr_get, >> .port_fn_hw_addr_set = mlx5_devlink_port_fn_hw_addr_set, >> .port_fn_roce_get = mlx5_devlink_port_fn_roce_get, > >Is it okay if we deferred the port_del() patch until there's some >clear benefit? Sure.
Tue, May 30, 2023 at 03:41:19AM CEST, kuba@kernel.org wrote: >On Mon, 29 May 2023 10:31:14 +0200 Jiri Pirko wrote: >> >One could argue logically removing a port is also an operation of >> >the parent (i.e. the devlink instance). The fact that the port gets >> >destroyed in the process is secondary. Ergo maybe we should skip >> >this patch? >> >> Well, the port_del() could differ for different port flavours. The >> embedding structure of struct devlink_port is also different. >> >> Makes sense to me to skip the flavour switch and have one port_del() for >> each port. > >The asymmetry bothers me. It's hard to comment on what the best Yeah, I had the same problem with that, but after a lots of thinking, it is a best I could think of. Please see below for the reasoning. >approach is given this series shows no benefit of moving port_del(). >Maybe even a loss, as mlx5 now has an ifdef in two places: > >> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/devlink.c b/drivers/net/ethernet/mellanox/mlx5/core/devlink.c >> index e39fd85ea2f9..63635cc44479 100644 >> --- a/drivers/net/ethernet/mellanox/mlx5/core/devlink.c >> +++ b/drivers/net/ethernet/mellanox/mlx5/core/devlink.c >> @@ -320,7 +320,6 @@ static const struct devlink_ops mlx5_devlink_ops = { >> #endif >> #ifdef CONFIG_MLX5_SF_MANAGER >> .port_new = mlx5_devlink_sf_port_new, >> - .port_del = mlx5_devlink_sf_port_del, >> #endif >> .flash_update = mlx5_devlink_flash_update, >> .info_get = mlx5_devlink_info_get, >> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/esw/devlink_port.c b/drivers/net/ethernet/mellanox/mlx5/core/esw/devlink_port.c >> index 76c5d6e9d47f..f370f67d9e33 100644 >> --- a/drivers/net/ethernet/mellanox/mlx5/core/esw/devlink_port.c >> +++ b/drivers/net/ethernet/mellanox/mlx5/core/esw/devlink_port.c >> @@ -145,6 +145,9 @@ struct devlink_port *mlx5_esw_offloads_devlink_port(struct mlx5_eswitch *esw, u1 >> } >> >> static const struct devlink_port_ops mlx5_esw_dl_sf_port_ops = { >> +#ifdef CONFIG_MLX5_SF_MANAGER >> + .port_del = mlx5_devlink_sf_port_del, >> +#endif Btw, this ifdef is going to go away in a follow-up patchset. >> .port_fn_hw_addr_get = mlx5_devlink_port_fn_hw_addr_get, >> .port_fn_hw_addr_set = mlx5_devlink_port_fn_hw_addr_set, >> .port_fn_roce_get = mlx5_devlink_port_fn_roce_get, > >Is it okay if we deferred the port_del() patch until there's some >clear benefit? Well actually, there is a clear benefit even in this patchset: We have 2 flavours of ports each with different ops in mlx5: VF: static const struct devlink_port_ops mlx5_esw_dl_port_ops = { .port_fn_hw_addr_get = mlx5_devlink_port_fn_hw_addr_get, .port_fn_hw_addr_set = mlx5_devlink_port_fn_hw_addr_set, .port_fn_roce_get = mlx5_devlink_port_fn_roce_get, .port_fn_roce_set = mlx5_devlink_port_fn_roce_set, .port_fn_migratable_get = mlx5_devlink_port_fn_migratable_get, .port_fn_migratable_set = mlx5_devlink_port_fn_migratable_set, }; SF: static const struct devlink_port_ops mlx5_esw_dl_sf_port_ops = { .port_del = mlx5_devlink_sf_port_del, .port_fn_hw_addr_get = mlx5_devlink_port_fn_hw_addr_get, .port_fn_hw_addr_set = mlx5_devlink_port_fn_hw_addr_set, .port_fn_roce_get = mlx5_devlink_port_fn_roce_get, .port_fn_roce_set = mlx5_devlink_port_fn_roce_set, .port_fn_state_get = mlx5_devlink_sf_port_fn_state_get, .port_fn_state_set = mlx5_devlink_sf_port_fn_state_set, }; You can see that the port_del() op is supported only on the SF flavour. VF does not support it and therefore port_del() is not defined on it. Without this patch, I would have to have a helper mlx5_devlink_port_del() that would check if the port is SF and call mlx5_devlink_sf_port_del() in that case. This patch reduces the boilerplate. Btw if you look at the cmd line api, it also aligns: $ devlink port add pci/0000:08:00.0 flavour pcisf pfnum 0 sfnum 101 pci/0000:08:00.0/32768: type eth netdev eth4 flavour pcisf controller 0 pfnum 0 sfnum 101 splittable false function: hw_addr 00:00:00:00:00:00 state inactive opstate detached $ devlink port del pci/0000:08:00.0/32768 You use pci/0000:08:00.0/32768 as a delete handle. port_del() is basically an object destructor. Would it perhaps help to rename is to .port_destructor()? That would somehow ease the asymmetry :) IDK. I would leave the name as it is a and move to port_ops.
On Tue, 30 May 2023 08:58:47 +0200 Jiri Pirko wrote: > >> .port_fn_hw_addr_get = mlx5_devlink_port_fn_hw_addr_get, > >> .port_fn_hw_addr_set = mlx5_devlink_port_fn_hw_addr_set, > >> .port_fn_roce_get = mlx5_devlink_port_fn_roce_get, > > > >Is it okay if we deferred the port_del() patch until there's some > >clear benefit? > > Well actually, there is a clear benefit even in this patchset: > > We have 2 flavours of ports each with different ops in mlx5: > VF: > static const struct devlink_port_ops mlx5_esw_dl_port_ops = { > .port_fn_hw_addr_get = mlx5_devlink_port_fn_hw_addr_get, > .port_fn_hw_addr_set = mlx5_devlink_port_fn_hw_addr_set, > .port_fn_roce_get = mlx5_devlink_port_fn_roce_get, > .port_fn_roce_set = mlx5_devlink_port_fn_roce_set, > .port_fn_migratable_get = mlx5_devlink_port_fn_migratable_get, > .port_fn_migratable_set = mlx5_devlink_port_fn_migratable_set, > }; > > SF: > static const struct devlink_port_ops mlx5_esw_dl_sf_port_ops = { > .port_del = mlx5_devlink_sf_port_del, > .port_fn_hw_addr_get = mlx5_devlink_port_fn_hw_addr_get, > .port_fn_hw_addr_set = mlx5_devlink_port_fn_hw_addr_set, > .port_fn_roce_get = mlx5_devlink_port_fn_roce_get, > .port_fn_roce_set = mlx5_devlink_port_fn_roce_set, > .port_fn_state_get = mlx5_devlink_sf_port_fn_state_get, > .port_fn_state_set = mlx5_devlink_sf_port_fn_state_set, > }; > > You can see that the port_del() op is supported only on the SF flavour. > VF does not support it and therefore port_del() is not defined on it. This is what I started thinking as well yesterday. Is there any reason to delete a port which isn't an SF? Similarly - is there any reason to delete a port which wasn't allocated via port_new? > Without this patch, I would have to have a helper > mlx5_devlink_port_del() that would check if the port is SF and call > mlx5_devlink_sf_port_del() in that case. This patch reduces the > boilerplate. ... Because if port_del can only happen on port_new'd ports, we should try to move that check into the core. It'd prevent misuse of the API. > Btw if you look at the cmd line api, it also aligns: > $ devlink port add pci/0000:08:00.0 flavour pcisf pfnum 0 sfnum 101 > pci/0000:08:00.0/32768: type eth netdev eth4 flavour pcisf controller 0 pfnum 0 sfnum 101 splittable false > function: > hw_addr 00:00:00:00:00:00 state inactive opstate detached > $ devlink port del pci/0000:08:00.0/32768 > > You use pci/0000:08:00.0/32768 as a delete handle. > > port_del() is basically an object destructor. Would it perhaps help to > rename is to .port_destructor()? That would somehow ease the asymmetry > :) IDK. I would leave the name as it is a and move to port_ops. Meh.
Tue, May 30, 2023 at 07:34:00PM CEST, kuba@kernel.org wrote: >On Tue, 30 May 2023 08:58:47 +0200 Jiri Pirko wrote: >> >> .port_fn_hw_addr_get = mlx5_devlink_port_fn_hw_addr_get, >> >> .port_fn_hw_addr_set = mlx5_devlink_port_fn_hw_addr_set, >> >> .port_fn_roce_get = mlx5_devlink_port_fn_roce_get, >> > >> >Is it okay if we deferred the port_del() patch until there's some >> >clear benefit? >> >> Well actually, there is a clear benefit even in this patchset: >> >> We have 2 flavours of ports each with different ops in mlx5: >> VF: >> static const struct devlink_port_ops mlx5_esw_dl_port_ops = { >> .port_fn_hw_addr_get = mlx5_devlink_port_fn_hw_addr_get, >> .port_fn_hw_addr_set = mlx5_devlink_port_fn_hw_addr_set, >> .port_fn_roce_get = mlx5_devlink_port_fn_roce_get, >> .port_fn_roce_set = mlx5_devlink_port_fn_roce_set, >> .port_fn_migratable_get = mlx5_devlink_port_fn_migratable_get, >> .port_fn_migratable_set = mlx5_devlink_port_fn_migratable_set, >> }; >> >> SF: >> static const struct devlink_port_ops mlx5_esw_dl_sf_port_ops = { >> .port_del = mlx5_devlink_sf_port_del, >> .port_fn_hw_addr_get = mlx5_devlink_port_fn_hw_addr_get, >> .port_fn_hw_addr_set = mlx5_devlink_port_fn_hw_addr_set, >> .port_fn_roce_get = mlx5_devlink_port_fn_roce_get, >> .port_fn_roce_set = mlx5_devlink_port_fn_roce_set, >> .port_fn_state_get = mlx5_devlink_sf_port_fn_state_get, >> .port_fn_state_set = mlx5_devlink_sf_port_fn_state_set, >> }; >> >> You can see that the port_del() op is supported only on the SF flavour. >> VF does not support it and therefore port_del() is not defined on it. > >This is what I started thinking as well yesterday. Is there any reason >to delete a port which isn't an SF? Similarly - is there any reason to >delete a port which wasn't allocated via port_new? Good question. Not possible atm. For SR-IOV VFs it probably does not make sense as there are PCI sysfs knobs to do that. Not sure about SIOV. > >> Without this patch, I would have to have a helper >> mlx5_devlink_port_del() that would check if the port is SF and call >> mlx5_devlink_sf_port_del() in that case. This patch reduces the >> boilerplate. > >... Because if port_del can only happen on port_new'd ports, we should >try to move that check into the core. It'd prevent misuse of the API. Okay, that is fair. Will make this change now. If the future brings different needs, easy to change. > >> Btw if you look at the cmd line api, it also aligns: >> $ devlink port add pci/0000:08:00.0 flavour pcisf pfnum 0 sfnum 101 >> pci/0000:08:00.0/32768: type eth netdev eth4 flavour pcisf controller 0 pfnum 0 sfnum 101 splittable false >> function: >> hw_addr 00:00:00:00:00:00 state inactive opstate detached >> $ devlink port del pci/0000:08:00.0/32768 >> >> You use pci/0000:08:00.0/32768 as a delete handle. >> >> port_del() is basically an object destructor. Would it perhaps help to >> rename is to .port_destructor()? That would somehow ease the asymmetry >> :) IDK. I would leave the name as it is a and move to port_ops. > >Meh. Yeah :)
On 5/30/2023 11:33 PM, Jiri Pirko wrote: > Tue, May 30, 2023 at 07:34:00PM CEST, kuba@kernel.org wrote: >> On Tue, 30 May 2023 08:58:47 +0200 Jiri Pirko wrote: >>>>> .port_fn_hw_addr_get = mlx5_devlink_port_fn_hw_addr_get, >>>>> .port_fn_hw_addr_set = mlx5_devlink_port_fn_hw_addr_set, >>>>> .port_fn_roce_get = mlx5_devlink_port_fn_roce_get, >>>> >>>> Is it okay if we deferred the port_del() patch until there's some >>>> clear benefit? >>> >>> Well actually, there is a clear benefit even in this patchset: >>> >>> We have 2 flavours of ports each with different ops in mlx5: >>> VF: >>> static const struct devlink_port_ops mlx5_esw_dl_port_ops = { >>> .port_fn_hw_addr_get = mlx5_devlink_port_fn_hw_addr_get, >>> .port_fn_hw_addr_set = mlx5_devlink_port_fn_hw_addr_set, >>> .port_fn_roce_get = mlx5_devlink_port_fn_roce_get, >>> .port_fn_roce_set = mlx5_devlink_port_fn_roce_set, >>> .port_fn_migratable_get = mlx5_devlink_port_fn_migratable_get, >>> .port_fn_migratable_set = mlx5_devlink_port_fn_migratable_set, >>> }; >>> >>> SF: >>> static const struct devlink_port_ops mlx5_esw_dl_sf_port_ops = { >>> .port_del = mlx5_devlink_sf_port_del, >>> .port_fn_hw_addr_get = mlx5_devlink_port_fn_hw_addr_get, >>> .port_fn_hw_addr_set = mlx5_devlink_port_fn_hw_addr_set, >>> .port_fn_roce_get = mlx5_devlink_port_fn_roce_get, >>> .port_fn_roce_set = mlx5_devlink_port_fn_roce_set, >>> .port_fn_state_get = mlx5_devlink_sf_port_fn_state_get, >>> .port_fn_state_set = mlx5_devlink_sf_port_fn_state_set, >>> }; >>> >>> You can see that the port_del() op is supported only on the SF flavour. >>> VF does not support it and therefore port_del() is not defined on it. >> >> This is what I started thinking as well yesterday. Is there any reason >> to delete a port which isn't an SF? Similarly - is there any reason to >> delete a port which wasn't allocated via port_new? > > Good question. Not possible atm. For SR-IOV VFs it probably does not make > sense as there are PCI sysfs knobs to do that. Not sure about SIOV. > > At least for ice, the current plan for Scalable IOV I had was creating ports via port_new, so port_del makes sense there. I don't know how else others were planning on doing this. We're a bit delayed on being able to post actual patches just yet though :( >> >>> Without this patch, I would have to have a helper >>> mlx5_devlink_port_del() that would check if the port is SF and call >>> mlx5_devlink_sf_port_del() in that case. This patch reduces the >>> boilerplate. >> >> ... Because if port_del can only happen on port_new'd ports, we should >> try to move that check into the core. It'd prevent misuse of the API. > > Okay, that is fair. Will make this change now. If the future brings > different needs, easy to change. > > >> >>> Btw if you look at the cmd line api, it also aligns: >>> $ devlink port add pci/0000:08:00.0 flavour pcisf pfnum 0 sfnum 101 >>> pci/0000:08:00.0/32768: type eth netdev eth4 flavour pcisf controller 0 pfnum 0 sfnum 101 splittable false >>> function: >>> hw_addr 00:00:00:00:00:00 state inactive opstate detached >>> $ devlink port del pci/0000:08:00.0/32768 >>> >>> You use pci/0000:08:00.0/32768 as a delete handle. >>> >>> port_del() is basically an object destructor. Would it perhaps help to >>> rename is to .port_destructor()? That would somehow ease the asymmetry >>> :) IDK. I would leave the name as it is a and move to port_ops. >> >> Meh. > i like thinking about it in terms of destructor, but that sort of implies its called whenever the port is removed (i.e. even if removed by the driver via something like devlink_port_unregister or whatever its called). > Yeah :) >
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/devlink.c b/drivers/net/ethernet/mellanox/mlx5/core/devlink.c index e39fd85ea2f9..63635cc44479 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/devlink.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/devlink.c @@ -320,7 +320,6 @@ static const struct devlink_ops mlx5_devlink_ops = { #endif #ifdef CONFIG_MLX5_SF_MANAGER .port_new = mlx5_devlink_sf_port_new, - .port_del = mlx5_devlink_sf_port_del, #endif .flash_update = mlx5_devlink_flash_update, .info_get = mlx5_devlink_info_get, diff --git a/drivers/net/ethernet/mellanox/mlx5/core/esw/devlink_port.c b/drivers/net/ethernet/mellanox/mlx5/core/esw/devlink_port.c index 76c5d6e9d47f..f370f67d9e33 100644 --- a/drivers/net/ethernet/mellanox/mlx5/core/esw/devlink_port.c +++ b/drivers/net/ethernet/mellanox/mlx5/core/esw/devlink_port.c @@ -145,6 +145,9 @@ struct devlink_port *mlx5_esw_offloads_devlink_port(struct mlx5_eswitch *esw, u1 } static const struct devlink_port_ops mlx5_esw_dl_sf_port_ops = { +#ifdef CONFIG_MLX5_SF_MANAGER + .port_del = mlx5_devlink_sf_port_del, +#endif .port_fn_hw_addr_get = mlx5_devlink_port_fn_hw_addr_get, .port_fn_hw_addr_set = mlx5_devlink_port_fn_hw_addr_set, .port_fn_roce_get = mlx5_devlink_port_fn_roce_get, diff --git a/include/net/devlink.h b/include/net/devlink.h index 21254d6884ee..aeccab044358 100644 --- a/include/net/devlink.h +++ b/include/net/devlink.h @@ -1447,23 +1447,6 @@ struct devlink_ops { int (*port_new)(struct devlink *devlink, const struct devlink_port_new_attrs *attrs, struct netlink_ext_ack *extack); - /** - * port_del() - Delete a port function - * @devlink: Devlink instance - * @port: The devlink port - * @extack: extack for reporting error messages - * - * Devlink core will call this device driver function upon user request - * to delete a previously created port function - * - * Notes: - * - On success, drivers must unregister the corresponding devlink - * port - * - * Return: 0 on success, negative value otherwise. - */ - int (*port_del)(struct devlink *devlink, struct devlink_port *port, - struct netlink_ext_ack *extack); /** * Rate control callbacks. @@ -1560,6 +1543,9 @@ void devlink_free(struct devlink *devlink); * @port_unsplit: Callback used to unsplit the port group back into * a single port. * @port_type_set: Callback used to set a type of a port. + * @port_del: Callback used to delete selected port along with related function. + * Devlink core calls this upon user request to delete + * a port previously created by devlink_ops->port_new(). * @port_fn_hw_addr_get: Callback used to set port function's hardware address. * Should be used by device drivers to report * the hardware address of a function managed @@ -1602,6 +1588,8 @@ struct devlink_port_ops { struct netlink_ext_ack *extack); int (*port_type_set)(struct devlink_port *devlink_port, enum devlink_port_type port_type); + int (*port_del)(struct devlink *devlink, struct devlink_port *port, + struct netlink_ext_ack *extack); int (*port_fn_hw_addr_get)(struct devlink_port *port, u8 *hw_addr, int *hw_addr_len, struct netlink_ext_ack *extack); diff --git a/net/devlink/leftover.c b/net/devlink/leftover.c index 03ce24a1397e..52aaa439caa5 100644 --- a/net/devlink/leftover.c +++ b/net/devlink/leftover.c @@ -1348,7 +1348,7 @@ static int devlink_nl_cmd_port_new_doit(struct sk_buff *skb, struct devlink_port_new_attrs new_attrs = {}; struct devlink *devlink = info->user_ptr[0]; - if (!devlink->ops->port_new || !devlink->ops->port_del) + if (!devlink->ops->port_new) return -EOPNOTSUPP; if (!info->attrs[DEVLINK_ATTR_PORT_FLAVOUR] || @@ -1387,10 +1387,10 @@ static int devlink_nl_cmd_port_del_doit(struct sk_buff *skb, struct netlink_ext_ack *extack = info->extack; struct devlink *devlink = info->user_ptr[0]; - if (!devlink->ops->port_del) + if (!devlink_port->ops->port_del) return -EOPNOTSUPP; - return devlink->ops->port_del(devlink, devlink_port, extack); + return devlink_port->ops->port_del(devlink, devlink_port, extack); } static int