[PATCH net-next 0/2] devlink port attr cleanup

[PATCH net-next 0/2] devlink port attr cleanup

[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

[PATCH net-next 1/2] devlink/port: Check attributes early and constify

[Parav Pandit]

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

[PATCH net-next 2/2] devlink/port: Simplify return checks

[Parav Pandit]

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

Re: [PATCH net-next 1/2] devlink/port: Check attributes early and constify

[Vadim Fedorenko]

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);
>   

Re: [PATCH net-next 2/2] devlink/port: Simplify return checks

[Vadim Fedorenko]

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...

RE: [PATCH net-next 2/2] devlink/port: Simplify return checks

[Parav Pandit]

> 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.

Re: [PATCH net-next 1/2] devlink/port: Check attributes early and constify

[Jakub Kicinski]

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)

Re: [PATCH net-next 2/2] devlink/port: Simplify return checks

[Jakub Kicinski]

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.

RE: [PATCH net-next 1/2] devlink/port: Check attributes early and constify

[Parav Pandit]

> 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)

RE: [PATCH net-next 2/2] devlink/port: Simplify return checks

[Parav Pandit]

> 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.