* [PATCH net-next 0/2] devlink port attr cleanup @ 2025-08-12 3:51 Parav Pandit 2025-08-12 3:51 ` [PATCH net-next 1/2] devlink/port: Check attributes early and constify Parav Pandit 2025-08-12 3:51 ` [PATCH net-next 2/2] devlink/port: Simplify return checks Parav Pandit 0 siblings, 2 replies; 10+ messages in thread From: Parav Pandit @ 2025-08-12 3:51 UTC (permalink / raw) To: davem, edumazet, kuba, pabeni, horms, netdev; +Cc: jiri, Parav Pandit Hi, This two small patches simplifies the devlink port attributes set functions. Summary: patch-1 constifies the attributes and moves the checks early patch-2 removes the return 0 check at several places and simplfies caller Please review. Parav Pandit (2): devlink/port: Check attributes early and constify devlink/port: Simplify return checks include/net/devlink.h | 2 +- net/devlink/port.c | 33 ++++++++------------------------- 2 files changed, 9 insertions(+), 26 deletions(-) -- 2.26.2 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH net-next 1/2] devlink/port: Check attributes early and constify 2025-08-12 3:51 [PATCH net-next 0/2] devlink port attr cleanup Parav Pandit @ 2025-08-12 3:51 ` Parav Pandit 2025-08-12 12:56 ` Vadim Fedorenko 2025-08-12 14:46 ` Jakub Kicinski 2025-08-12 3:51 ` [PATCH net-next 2/2] devlink/port: Simplify return checks Parav Pandit 1 sibling, 2 replies; 10+ messages in thread From: Parav Pandit @ 2025-08-12 3:51 UTC (permalink / raw) To: davem, edumazet, kuba, pabeni, horms, netdev Cc: jiri, Parav Pandit, Jiri Pirko Constify the devlink port attributes to indicate they are read only and does not depend on anything else. Therefore, validate it early before setting in the devlink port. Reviewed-by: Jiri Pirko <jiri@nvidia.com> Signed-off-by: Parav Pandit <parav@nvidia.com> --- include/net/devlink.h | 2 +- net/devlink/port.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/include/net/devlink.h b/include/net/devlink.h index 93640a29427c..c6f3afa92c8f 100644 --- a/include/net/devlink.h +++ b/include/net/devlink.h @@ -1739,7 +1739,7 @@ void devlink_port_type_ib_set(struct devlink_port *devlink_port, struct ib_device *ibdev); void devlink_port_type_clear(struct devlink_port *devlink_port); void devlink_port_attrs_set(struct devlink_port *devlink_port, - struct devlink_port_attrs *devlink_port_attrs); + const struct devlink_port_attrs *dl_port_attrs); void devlink_port_attrs_pci_pf_set(struct devlink_port *devlink_port, u32 controller, u16 pf, bool external); void devlink_port_attrs_pci_vf_set(struct devlink_port *devlink_port, u32 controller, diff --git a/net/devlink/port.c b/net/devlink/port.c index 939081a0e615..1033b9ad2af4 100644 --- a/net/devlink/port.c +++ b/net/devlink/port.c @@ -1357,17 +1357,17 @@ static int __devlink_port_attrs_set(struct devlink_port *devlink_port, * @attrs: devlink port attrs */ void devlink_port_attrs_set(struct devlink_port *devlink_port, - struct devlink_port_attrs *attrs) + const struct devlink_port_attrs *attrs) { int ret; ASSERT_DEVLINK_PORT_NOT_REGISTERED(devlink_port); + WARN_ON(attrs->splittable && attrs->split); devlink_port->attrs = *attrs; ret = __devlink_port_attrs_set(devlink_port, attrs->flavour); if (ret) return; - WARN_ON(attrs->splittable && attrs->split); } EXPORT_SYMBOL_GPL(devlink_port_attrs_set); -- 2.26.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net-next 1/2] devlink/port: Check attributes early and constify 2025-08-12 3:51 ` [PATCH net-next 1/2] devlink/port: Check attributes early and constify Parav Pandit @ 2025-08-12 12:56 ` Vadim Fedorenko 2025-08-12 14:46 ` Jakub Kicinski 1 sibling, 0 replies; 10+ messages in thread From: Vadim Fedorenko @ 2025-08-12 12:56 UTC (permalink / raw) To: Parav Pandit, davem, edumazet, kuba, pabeni, horms, netdev Cc: jiri, Jiri Pirko On 12/08/2025 04:51, Parav Pandit wrote: > Constify the devlink port attributes to indicate they are read only > and does not depend on anything else. Therefore, validate it early > before setting in the devlink port. > > Reviewed-by: Jiri Pirko <jiri@nvidia.com> > Signed-off-by: Parav Pandit <parav@nvidia.com> > --- > include/net/devlink.h | 2 +- > net/devlink/port.c | 4 ++-- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/include/net/devlink.h b/include/net/devlink.h > index 93640a29427c..c6f3afa92c8f 100644 > --- a/include/net/devlink.h > +++ b/include/net/devlink.h > @@ -1739,7 +1739,7 @@ void devlink_port_type_ib_set(struct devlink_port *devlink_port, > struct ib_device *ibdev); > void devlink_port_type_clear(struct devlink_port *devlink_port); > void devlink_port_attrs_set(struct devlink_port *devlink_port, > - struct devlink_port_attrs *devlink_port_attrs); > + const struct devlink_port_attrs *dl_port_attrs); > void devlink_port_attrs_pci_pf_set(struct devlink_port *devlink_port, u32 controller, > u16 pf, bool external); > void devlink_port_attrs_pci_vf_set(struct devlink_port *devlink_port, u32 controller, > diff --git a/net/devlink/port.c b/net/devlink/port.c > index 939081a0e615..1033b9ad2af4 100644 > --- a/net/devlink/port.c > +++ b/net/devlink/port.c > @@ -1357,17 +1357,17 @@ static int __devlink_port_attrs_set(struct devlink_port *devlink_port, > * @attrs: devlink port attrs > */ > void devlink_port_attrs_set(struct devlink_port *devlink_port, > - struct devlink_port_attrs *attrs) > + const struct devlink_port_attrs *attrs) > { > int ret; > > ASSERT_DEVLINK_PORT_NOT_REGISTERED(devlink_port); > + WARN_ON(attrs->splittable && attrs->split); > > devlink_port->attrs = *attrs; > ret = __devlink_port_attrs_set(devlink_port, attrs->flavour); > if (ret) > return; > - WARN_ON(attrs->splittable && attrs->split); After this change there is no need for local variable and the "if" block > } > EXPORT_SYMBOL_GPL(devlink_port_attrs_set); > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next 1/2] devlink/port: Check attributes early and constify 2025-08-12 3:51 ` [PATCH net-next 1/2] devlink/port: Check attributes early and constify Parav Pandit 2025-08-12 12:56 ` Vadim Fedorenko @ 2025-08-12 14:46 ` Jakub Kicinski 2025-08-12 16:20 ` Parav Pandit 1 sibling, 1 reply; 10+ messages in thread From: Jakub Kicinski @ 2025-08-12 14:46 UTC (permalink / raw) To: Parav Pandit; +Cc: davem, edumazet, pabeni, horms, netdev, jiri, Jiri Pirko On Tue, 12 Aug 2025 06:51:05 +0300 Parav Pandit wrote: > void devlink_port_attrs_set(struct devlink_port *devlink_port, > - struct devlink_port_attrs *devlink_port_attrs); > + const struct devlink_port_attrs *dl_port_attrs); Why not rename to attr, which is what the implementation uses? > void devlink_port_attrs_set(struct devlink_port *devlink_port, > - struct devlink_port_attrs *attrs) > + const struct devlink_port_attrs *attrs) ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH net-next 1/2] devlink/port: Check attributes early and constify 2025-08-12 14:46 ` Jakub Kicinski @ 2025-08-12 16:20 ` Parav Pandit 0 siblings, 0 replies; 10+ messages in thread From: Parav Pandit @ 2025-08-12 16:20 UTC (permalink / raw) To: Jakub Kicinski Cc: davem@davemloft.net, edumazet@google.com, pabeni@redhat.com, horms@kernel.org, netdev@vger.kernel.org, jiri@resnulli.us, Jiri Pirko > From: Jakub Kicinski <kuba@kernel.org> > Sent: 12 August 2025 08:17 PM > > On Tue, 12 Aug 2025 06:51:05 +0300 Parav Pandit wrote: > > void devlink_port_attrs_set(struct devlink_port *devlink_port, > > - struct devlink_port_attrs *devlink_port_attrs); > > + const struct devlink_port_attrs *dl_port_attrs); > > Why not rename to attr, which is what the implementation uses? > Make sense. Will change in v2. > > void devlink_port_attrs_set(struct devlink_port *devlink_port, > > - struct devlink_port_attrs *attrs) > > + const struct devlink_port_attrs *attrs) ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH net-next 2/2] devlink/port: Simplify return checks 2025-08-12 3:51 [PATCH net-next 0/2] devlink port attr cleanup Parav Pandit 2025-08-12 3:51 ` [PATCH net-next 1/2] devlink/port: Check attributes early and constify Parav Pandit @ 2025-08-12 3:51 ` Parav Pandit 2025-08-12 12:59 ` Vadim Fedorenko 1 sibling, 1 reply; 10+ messages in thread From: Parav Pandit @ 2025-08-12 3:51 UTC (permalink / raw) To: davem, edumazet, kuba, pabeni, horms, netdev Cc: jiri, Parav Pandit, Jiri Pirko Drop always returning 0 from the helper routine and simplify its callers. Reviewed-by: Jiri Pirko <jiri@nvidia.com> Signed-off-by: Parav Pandit <parav@nvidia.com> --- net/devlink/port.c | 29 ++++++----------------------- 1 file changed, 6 insertions(+), 23 deletions(-) diff --git a/net/devlink/port.c b/net/devlink/port.c index 1033b9ad2af4..93f2969b9cf3 100644 --- a/net/devlink/port.c +++ b/net/devlink/port.c @@ -1333,8 +1333,8 @@ int devlink_port_netdevice_event(struct notifier_block *nb, return NOTIFY_OK; } -static int __devlink_port_attrs_set(struct devlink_port *devlink_port, - enum devlink_port_flavour flavour) +static void __devlink_port_attrs_set(struct devlink_port *devlink_port, + enum devlink_port_flavour flavour) { struct devlink_port_attrs *attrs = &devlink_port->attrs; @@ -1347,7 +1347,6 @@ static int __devlink_port_attrs_set(struct devlink_port *devlink_port, } else { devlink_port->switch_port = false; } - return 0; } /** @@ -1359,15 +1358,11 @@ static int __devlink_port_attrs_set(struct devlink_port *devlink_port, void devlink_port_attrs_set(struct devlink_port *devlink_port, const struct devlink_port_attrs *attrs) { - int ret; - ASSERT_DEVLINK_PORT_NOT_REGISTERED(devlink_port); WARN_ON(attrs->splittable && attrs->split); devlink_port->attrs = *attrs; - ret = __devlink_port_attrs_set(devlink_port, attrs->flavour); - if (ret) - return; + __devlink_port_attrs_set(devlink_port, attrs->flavour); } EXPORT_SYMBOL_GPL(devlink_port_attrs_set); @@ -1383,14 +1378,10 @@ void devlink_port_attrs_pci_pf_set(struct devlink_port *devlink_port, u32 contro u16 pf, bool external) { struct devlink_port_attrs *attrs = &devlink_port->attrs; - int ret; ASSERT_DEVLINK_PORT_NOT_REGISTERED(devlink_port); - ret = __devlink_port_attrs_set(devlink_port, - DEVLINK_PORT_FLAVOUR_PCI_PF); - if (ret) - return; + __devlink_port_attrs_set(devlink_port, DEVLINK_PORT_FLAVOUR_PCI_PF); attrs->pci_pf.controller = controller; attrs->pci_pf.pf = pf; attrs->pci_pf.external = external; @@ -1411,14 +1402,10 @@ void devlink_port_attrs_pci_vf_set(struct devlink_port *devlink_port, u32 contro u16 pf, u16 vf, bool external) { struct devlink_port_attrs *attrs = &devlink_port->attrs; - int ret; ASSERT_DEVLINK_PORT_NOT_REGISTERED(devlink_port); - ret = __devlink_port_attrs_set(devlink_port, - DEVLINK_PORT_FLAVOUR_PCI_VF); - if (ret) - return; + __devlink_port_attrs_set(devlink_port, DEVLINK_PORT_FLAVOUR_PCI_VF); attrs->pci_vf.controller = controller; attrs->pci_vf.pf = pf; attrs->pci_vf.vf = vf; @@ -1439,14 +1426,10 @@ void devlink_port_attrs_pci_sf_set(struct devlink_port *devlink_port, u32 contro u16 pf, u32 sf, bool external) { struct devlink_port_attrs *attrs = &devlink_port->attrs; - int ret; ASSERT_DEVLINK_PORT_NOT_REGISTERED(devlink_port); - ret = __devlink_port_attrs_set(devlink_port, - DEVLINK_PORT_FLAVOUR_PCI_SF); - if (ret) - return; + __devlink_port_attrs_set(devlink_port, DEVLINK_PORT_FLAVOUR_PCI_SF); attrs->pci_sf.controller = controller; attrs->pci_sf.pf = pf; attrs->pci_sf.sf = sf; -- 2.26.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net-next 2/2] devlink/port: Simplify return checks 2025-08-12 3:51 ` [PATCH net-next 2/2] devlink/port: Simplify return checks Parav Pandit @ 2025-08-12 12:59 ` Vadim Fedorenko 2025-08-12 13:45 ` Parav Pandit 0 siblings, 1 reply; 10+ messages in thread From: Vadim Fedorenko @ 2025-08-12 12:59 UTC (permalink / raw) To: Parav Pandit, davem, edumazet, kuba, pabeni, horms, netdev Cc: jiri, Jiri Pirko On 12/08/2025 04:51, Parav Pandit wrote: > Drop always returning 0 from the helper routine and simplify > its callers. Oh, I see, you split it into 2 patches, but I'm not sure it's actually needed, because the first patch doesn't look logical on its own... ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH net-next 2/2] devlink/port: Simplify return checks 2025-08-12 12:59 ` Vadim Fedorenko @ 2025-08-12 13:45 ` Parav Pandit 2025-08-12 14:47 ` Jakub Kicinski 0 siblings, 1 reply; 10+ messages in thread From: Parav Pandit @ 2025-08-12 13:45 UTC (permalink / raw) To: Vadim Fedorenko, davem@davemloft.net, edumazet@google.com, kuba@kernel.org, pabeni@redhat.com, horms@kernel.org, netdev@vger.kernel.org Cc: jiri@resnulli.us, Jiri Pirko > From: Vadim Fedorenko <vadim.fedorenko@linux.dev> > Sent: 12 August 2025 06:30 PM > > On 12/08/2025 04:51, Parav Pandit wrote: > > Drop always returning 0 from the helper routine and simplify its > > callers. > > > Oh, I see, you split it into 2 patches, but I'm not sure it's actually needed, > because the first patch doesn't look logical on its own... I can squash the two patches to one. The general guidance was to not do multiple different types of changes in single patch. No strong preference to me. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next 2/2] devlink/port: Simplify return checks 2025-08-12 13:45 ` Parav Pandit @ 2025-08-12 14:47 ` Jakub Kicinski 2025-08-12 16:21 ` Parav Pandit 0 siblings, 1 reply; 10+ messages in thread From: Jakub Kicinski @ 2025-08-12 14:47 UTC (permalink / raw) To: Parav Pandit Cc: Vadim Fedorenko, davem@davemloft.net, edumazet@google.com, pabeni@redhat.com, horms@kernel.org, netdev@vger.kernel.org, jiri@resnulli.us, Jiri Pirko On Tue, 12 Aug 2025 13:45:09 +0000 Parav Pandit wrote: > The general guidance was to not do multiple different types of changes in single patch. > No strong preference to me. It would probably make more sense if the order of the patches was inverted. ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH net-next 2/2] devlink/port: Simplify return checks 2025-08-12 14:47 ` Jakub Kicinski @ 2025-08-12 16:21 ` Parav Pandit 0 siblings, 0 replies; 10+ messages in thread From: Parav Pandit @ 2025-08-12 16:21 UTC (permalink / raw) To: Jakub Kicinski Cc: Vadim Fedorenko, davem@davemloft.net, edumazet@google.com, pabeni@redhat.com, horms@kernel.org, netdev@vger.kernel.org, jiri@resnulli.us, Jiri Pirko > From: Jakub Kicinski <kuba@kernel.org> > Sent: 12 August 2025 08:18 PM > > On Tue, 12 Aug 2025 13:45:09 +0000 Parav Pandit wrote: > > The general guidance was to not do multiple different types of changes in > single patch. > > No strong preference to me. > > It would probably make more sense if the order of the patches was inverted. Right. Will send v2 with reverse order and addressing other comment. Thanks. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2025-08-12 16:21 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-08-12 3:51 [PATCH net-next 0/2] devlink port attr cleanup Parav Pandit 2025-08-12 3:51 ` [PATCH net-next 1/2] devlink/port: Check attributes early and constify Parav Pandit 2025-08-12 12:56 ` Vadim Fedorenko 2025-08-12 14:46 ` Jakub Kicinski 2025-08-12 16:20 ` Parav Pandit 2025-08-12 3:51 ` [PATCH net-next 2/2] devlink/port: Simplify return checks Parav Pandit 2025-08-12 12:59 ` Vadim Fedorenko 2025-08-12 13:45 ` Parav Pandit 2025-08-12 14:47 ` Jakub Kicinski 2025-08-12 16:21 ` Parav Pandit
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).