[patch net-next 0/3] net: devlink: devl_* cosmetic fixes

[patch net-next 0/3] net: devlink: devl_* cosmetic fixes

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Hi. This patches just fixes some small cosmetic issues of devl_* related
functions which I found on the way.

Jiri Pirko (3):
  net: devlink: move unlocked function prototypes alongside the locked
    ones
  net: devlink: call lockdep_assert_held() for devlink->lock directly
  net: devlink: fix unlocked vs locked functions descriptions

 include/net/devlink.h | 16 +++++++---------
 net/core/devlink.c    | 42 +++++++++++++++++++++++++++++++++++++++---
 2 files changed, 46 insertions(+), 12 deletions(-)

-- 
2.35.3

[patch net-next 1/3] net: devlink: move unlocked function prototypes alongside the locked ones

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Maintain the same order as it is in devlink.c for function prototypes.
The most of the locked variants would very likely soon be removed
and the unlocked version would be the only one.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 include/net/devlink.h | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/include/net/devlink.h b/include/net/devlink.h
index b8f54a8e9c82..edbfe6daa3b5 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -1520,15 +1520,6 @@ void devl_unlock(struct devlink *devlink);
 void devl_assert_locked(struct devlink *devlink);
 bool devl_lock_is_held(struct devlink *devlink);
 
-int devl_port_register(struct devlink *devlink,
-		       struct devlink_port *devlink_port,
-		       unsigned int port_index);
-void devl_port_unregister(struct devlink_port *devlink_port);
-
-int devl_rate_leaf_create(struct devlink_port *port, void *priv);
-void devl_rate_leaf_destroy(struct devlink_port *devlink_port);
-void devl_rate_nodes_destroy(struct devlink *devlink);
-
 struct ib_device;
 
 struct net *devlink_net(const struct devlink *devlink);
@@ -1550,9 +1541,13 @@ void devlink_set_features(struct devlink *devlink, u64 features);
 void devlink_register(struct devlink *devlink);
 void devlink_unregister(struct devlink *devlink);
 void devlink_free(struct devlink *devlink);
+int devl_port_register(struct devlink *devlink,
+		       struct devlink_port *devlink_port,
+		       unsigned int port_index);
 int devlink_port_register(struct devlink *devlink,
 			  struct devlink_port *devlink_port,
 			  unsigned int port_index);
+void devl_port_unregister(struct devlink_port *devlink_port);
 void devlink_port_unregister(struct devlink_port *devlink_port);
 void devlink_port_type_eth_set(struct devlink_port *devlink_port,
 			       struct net_device *netdev);
@@ -1568,8 +1563,11 @@ void devlink_port_attrs_pci_vf_set(struct devlink_port *devlink_port, u32 contro
 void devlink_port_attrs_pci_sf_set(struct devlink_port *devlink_port,
 				   u32 controller, u16 pf, u32 sf,
 				   bool external);
+int devl_rate_leaf_create(struct devlink_port *port, void *priv);
 int devlink_rate_leaf_create(struct devlink_port *port, void *priv);
+void devl_rate_leaf_destroy(struct devlink_port *devlink_port);
 void devlink_rate_leaf_destroy(struct devlink_port *devlink_port);
+void devl_rate_nodes_destroy(struct devlink *devlink);
 void devlink_rate_nodes_destroy(struct devlink *devlink);
 void devlink_port_linecard_set(struct devlink_port *devlink_port,
 			       struct devlink_linecard *linecard);
-- 
2.35.3

[patch net-next 2/3] net: devlink: call lockdep_assert_held() for devlink->lock directly

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

In devlink.c there is direct access to whole struct devlink so there is
no need to use helper. So obey the customs and work with lock directly
avoiding helpers which might obfuscate things a bit.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 net/core/devlink.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/core/devlink.c b/net/core/devlink.c
index 25b481dd1709..a7477addbd59 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -10185,7 +10185,7 @@ int devl_rate_leaf_create(struct devlink_port *devlink_port, void *priv)
 	struct devlink *devlink = devlink_port->devlink;
 	struct devlink_rate *devlink_rate;
 
-	devl_assert_locked(devlink_port->devlink);
+	lockdep_assert_held(&devlink_port->devlink->lock);
 
 	if (WARN_ON(devlink_port->devlink_rate))
 		return -EBUSY;
@@ -10224,7 +10224,7 @@ void devl_rate_leaf_destroy(struct devlink_port *devlink_port)
 {
 	struct devlink_rate *devlink_rate = devlink_port->devlink_rate;
 
-	devl_assert_locked(devlink_port->devlink);
+	lockdep_assert_held(&devlink_port->devlink->lock);
 	if (!devlink_rate)
 		return;
 
@@ -10270,7 +10270,7 @@ void devl_rate_nodes_destroy(struct devlink *devlink)
 	static struct devlink_rate *devlink_rate, *tmp;
 	const struct devlink_ops *ops = devlink->ops;
 
-	devl_assert_locked(devlink);
+	lockdep_assert_held(&devlink->lock);
 
 	list_for_each_entry(devlink_rate, &devlink->rate_list, list) {
 		if (!devlink_rate->parent)
-- 
2.35.3

Re: [patch net-next 2/3] net: devlink: call lockdep_assert_held() for devlink->lock directly

[Jakub Kicinski]

On Fri,  1 Jul 2022 11:59:25 +0200 Jiri Pirko wrote:
> In devlink.c there is direct access to whole struct devlink so there is
> no need to use helper. So obey the customs and work with lock directly
> avoiding helpers which might obfuscate things a bit.

> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index 25b481dd1709..a7477addbd59 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -10185,7 +10185,7 @@ int devl_rate_leaf_create(struct devlink_port *devlink_port, void *priv)
>  	struct devlink *devlink = devlink_port->devlink;
>  	struct devlink_rate *devlink_rate;
>  
> -	devl_assert_locked(devlink_port->devlink);
> +	lockdep_assert_held(&devlink_port->devlink->lock);

I don't understand why. Do we use lockdep asserts directly on rtnl_mutex
in rtnetlink.c?

Re: [patch net-next 2/3] net: devlink: call lockdep_assert_held() for devlink->lock directly

[Jiri Pirko]

Fri, Jul 01, 2022 at 06:33:16PM CEST, kuba@kernel.org wrote:
>On Fri,  1 Jul 2022 11:59:25 +0200 Jiri Pirko wrote:
>> In devlink.c there is direct access to whole struct devlink so there is
>> no need to use helper. So obey the customs and work with lock directly
>> avoiding helpers which might obfuscate things a bit.
>
>> diff --git a/net/core/devlink.c b/net/core/devlink.c
>> index 25b481dd1709..a7477addbd59 100644
>> --- a/net/core/devlink.c
>> +++ b/net/core/devlink.c
>> @@ -10185,7 +10185,7 @@ int devl_rate_leaf_create(struct devlink_port *devlink_port, void *priv)
>>  	struct devlink *devlink = devlink_port->devlink;
>>  	struct devlink_rate *devlink_rate;
>>  
>> -	devl_assert_locked(devlink_port->devlink);
>> +	lockdep_assert_held(&devlink_port->devlink->lock);
>
>I don't understand why. Do we use lockdep asserts directly on rtnl_mutex
>in rtnetlink.c?

Well:

1) it's been a long time policy not to use helpers for locks if not
   needed. There reason is that the reader has easier job in seeing what
   the code is doing. And here, it is not needed to use helper (we can
   access the containing struct)
2) lock/unlock for devlink->lock is done here w/o helpers as well
3) there is really no gain of using helper here.
4) rtnl_mutex is probably not good example, it has a lot of ancient
   history behind it.

Re: [patch net-next 2/3] net: devlink: call lockdep_assert_held() for devlink->lock directly

[Jakub Kicinski]

On Sat, 2 Jul 2022 17:58:06 +0200 Jiri Pirko wrote:
> Fri, Jul 01, 2022 at 06:33:16PM CEST, kuba@kernel.org wrote:
> >On Fri,  1 Jul 2022 11:59:25 +0200 Jiri Pirko wrote:  
> >> In devlink.c there is direct access to whole struct devlink so there is
> >> no need to use helper. So obey the customs and work with lock directly
> >> avoiding helpers which might obfuscate things a bit.  
> >  
> >> diff --git a/net/core/devlink.c b/net/core/devlink.c
> >> index 25b481dd1709..a7477addbd59 100644
> >> --- a/net/core/devlink.c
> >> +++ b/net/core/devlink.c
> >> @@ -10185,7 +10185,7 @@ int devl_rate_leaf_create(struct devlink_port *devlink_port, void *priv)
> >>  	struct devlink *devlink = devlink_port->devlink;
> >>  	struct devlink_rate *devlink_rate;
> >>  
> >> -	devl_assert_locked(devlink_port->devlink);
> >> +	lockdep_assert_held(&devlink_port->devlink->lock);  
> >
> >I don't understand why. Do we use lockdep asserts directly on rtnl_mutex
> >in rtnetlink.c?  
> 
> Well:
> 
> 1) it's been a long time policy not to use helpers for locks if not
>    needed. There reason is that the reader has easier job in seeing what
>    the code is doing. And here, it is not needed to use helper (we can
>    access the containing struct)

AFAIU the policy is not to _create_ helpers for locks for no good
reason. If the helper already exists it's better to consistently use
it.

> 2) lock/unlock for devlink->lock is done here w/o helpers as well

Existing code, I didn't want to cause major code churn until the
transition is finished.

> 3) there is really no gain of using helper here.

Shorter, easier to type and remember, especially if the author is
already using the exported assert in the driver.

> 4) rtnl_mutex is probably not good example, it has a lot of ancient
>    history behind it.

It's our main lock so we know it best. Do you have other examples?

Look, I don't really care, I just want to make sure we document the
rules of engagement clearly for everyone to see and uniformly enforce. 
So we either need to bash out exactly what we want (and I think our
views differ) or you should switch the commit message to say "I feel
like" rather than referring to "customs" 😁

Re: [patch net-next 2/3] net: devlink: call lockdep_assert_held() for devlink->lock directly

[Jiri Pirko]

Sat, Jul 02, 2022 at 09:29:46PM CEST, kuba@kernel.org wrote:
>On Sat, 2 Jul 2022 17:58:06 +0200 Jiri Pirko wrote:
>> Fri, Jul 01, 2022 at 06:33:16PM CEST, kuba@kernel.org wrote:
>> >On Fri,  1 Jul 2022 11:59:25 +0200 Jiri Pirko wrote:  
>> >> In devlink.c there is direct access to whole struct devlink so there is
>> >> no need to use helper. So obey the customs and work with lock directly
>> >> avoiding helpers which might obfuscate things a bit.  
>> >  
>> >> diff --git a/net/core/devlink.c b/net/core/devlink.c
>> >> index 25b481dd1709..a7477addbd59 100644
>> >> --- a/net/core/devlink.c
>> >> +++ b/net/core/devlink.c
>> >> @@ -10185,7 +10185,7 @@ int devl_rate_leaf_create(struct devlink_port *devlink_port, void *priv)
>> >>  	struct devlink *devlink = devlink_port->devlink;
>> >>  	struct devlink_rate *devlink_rate;
>> >>  
>> >> -	devl_assert_locked(devlink_port->devlink);
>> >> +	lockdep_assert_held(&devlink_port->devlink->lock);  
>> >
>> >I don't understand why. Do we use lockdep asserts directly on rtnl_mutex
>> >in rtnetlink.c?  
>> 
>> Well:
>> 
>> 1) it's been a long time policy not to use helpers for locks if not
>>    needed. There reason is that the reader has easier job in seeing what
>>    the code is doing. And here, it is not needed to use helper (we can
>>    access the containing struct)
>
>AFAIU the policy is not to _create_ helpers for locks for no good
>reason. If the helper already exists it's better to consistently use
>it.
>
>> 2) lock/unlock for devlink->lock is done here w/o helpers as well
>
>Existing code, I didn't want to cause major code churn until the
>transition is finished.
>
>> 3) there is really no gain of using helper here.
>
>Shorter, easier to type and remember, especially if the author is
>already using the exported assert in the driver.
>
>> 4) rtnl_mutex is probably not good example, it has a lot of ancient
>>    history behind it.
>
>It's our main lock so we know it best. Do you have other examples?
>
>Look, I don't really care, I just want to make sure we document the
>rules of engagement clearly for everyone to see and uniformly enforce. 
>So we either need to bash out exactly what we want (and I think our
>views differ) or you should switch the commit message to say "I feel
>like" rather than referring to "customs" 😁

Jakub, I don't really care. If you say we should do it differently, I
will do it differently. I just want the use to be consistent. From the
earlier reactions of DaveM on such helpers, I got an impression we don't
want them if possible. If this is no longer true, I'm fine with it. Just
tell me what I should do.

Thanks!

Re: [patch net-next 2/3] net: devlink: call lockdep_assert_held() for devlink->lock directly

[Jakub Kicinski]

On Mon, 4 Jul 2022 08:19:17 +0200 Jiri Pirko wrote:
> Jakub, I don't really care. If you say we should do it differently, I
> will do it differently. I just want the use to be consistent. From the
> earlier reactions of DaveM on such helpers, I got an impression we don't
> want them if possible. If this is no longer true, I'm fine with it. Just
> tell me what I should do.

As I said - my understanding is that we want to discourage (driver)
authors from wrapping locks in lock/unlock helpers. Which was very
fashionable at some point (IDK why, but it seem to have mostly gone
away?).

If the helper already exists I think consistency wins and we should use
it.

Re: [patch net-next 2/3] net: devlink: call lockdep_assert_held() for devlink->lock directly

[Jiri Pirko]

Tue, Jul 05, 2022 at 04:58:39AM CEST, kuba@kernel.org wrote:
>On Mon, 4 Jul 2022 08:19:17 +0200 Jiri Pirko wrote:
>> Jakub, I don't really care. If you say we should do it differently, I
>> will do it differently. I just want the use to be consistent. From the
>> earlier reactions of DaveM on such helpers, I got an impression we don't
>> want them if possible. If this is no longer true, I'm fine with it. Just
>> tell me what I should do.
>
>As I said - my understanding is that we want to discourage (driver)
>authors from wrapping locks in lock/unlock helpers. Which was very
>fashionable at some point (IDK why, but it seem to have mostly gone
>away?).
>
>If the helper already exists I think consistency wins and we should use
>it.

Okay, will send a patch to convert devlink.c to use these helpers.
Thanks!

[patch net-next 3/3] net: devlink: fix unlocked vs locked functions descriptions

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

To be unified with the rest of the code, the unlocked version (devl_*)
of function should have the same description in documentation as the
locked one. Add the missing documentation. Also, add "Context"
annotation for the locked versions where it is missing.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 net/core/devlink.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/net/core/devlink.c b/net/core/devlink.c
index a7477addbd59..cdb33125cd1e 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -9877,6 +9877,19 @@ static void devlink_port_type_warn_cancel(struct devlink_port *devlink_port)
 	cancel_delayed_work_sync(&devlink_port->type_warn_dw);
 }
 
+/**
+ *	devl_port_register - Register devlink port
+ *
+ *	@devlink: devlink
+ *	@devlink_port: devlink port
+ *	@port_index: driver-specific numerical identifier of the port
+ *
+ *	Register devlink port with provided port index. User can use
+ *	any indexing, even hw-related one. devlink_port structure
+ *	is convenient to be embedded inside user driver private structure.
+ *	Note that the caller should take care of zeroing the devlink_port
+ *	structure.
+ */
 int devl_port_register(struct devlink *devlink,
 		       struct devlink_port *devlink_port,
 		       unsigned int port_index)
@@ -9915,6 +9928,8 @@ EXPORT_SYMBOL_GPL(devl_port_register);
  *	is convenient to be embedded inside user driver private structure.
  *	Note that the caller should take care of zeroing the devlink_port
  *	structure.
+ *
+ *	Context: Takes and release devlink->lock <mutex>.
  */
 int devlink_port_register(struct devlink *devlink,
 			  struct devlink_port *devlink_port,
@@ -9929,6 +9944,11 @@ int devlink_port_register(struct devlink *devlink,
 }
 EXPORT_SYMBOL_GPL(devlink_port_register);
 
+/**
+ *	devlink_port_unregister - Unregister devlink port
+ *
+ *	@devlink_port: devlink port
+ */
 void devl_port_unregister(struct devlink_port *devlink_port)
 {
 	lockdep_assert_held(&devlink_port->devlink->lock);
@@ -9946,6 +9966,8 @@ EXPORT_SYMBOL_GPL(devl_port_unregister);
  *	devlink_port_unregister - Unregister devlink port
  *
  *	@devlink_port: devlink port
+ *
+ *	Context: Takes and release devlink->lock <mutex>.
  */
 void devlink_port_unregister(struct devlink_port *devlink_port)
 {
@@ -10206,6 +10228,15 @@ int devl_rate_leaf_create(struct devlink_port *devlink_port, void *priv)
 }
 EXPORT_SYMBOL_GPL(devl_rate_leaf_create);
 
+/**
+ * devlink_rate_leaf_create - create devlink rate leaf
+ * @devlink_port: devlink port object to create rate object on
+ * @priv: driver private data
+ *
+ * Create devlink rate object of type leaf on provided @devlink_port.
+ *
+ * Context: Takes and release devlink->lock <mutex>.
+ */
 int
 devlink_rate_leaf_create(struct devlink_port *devlink_port, void *priv)
 {
@@ -10220,6 +10251,11 @@ devlink_rate_leaf_create(struct devlink_port *devlink_port, void *priv)
 }
 EXPORT_SYMBOL_GPL(devlink_rate_leaf_create);
 
+/**
+ * devl_rate_leaf_destroy - destroy devlink rate leaf
+ *
+ * @devlink_port: devlink port linked to the rate object
+ */
 void devl_rate_leaf_destroy(struct devlink_port *devlink_port)
 {
 	struct devlink_rate *devlink_rate = devlink_port->devlink_rate;
-- 
2.35.3

Re: [patch net-next 3/3] net: devlink: fix unlocked vs locked functions descriptions

[Jakub Kicinski]

On Fri,  1 Jul 2022 11:59:26 +0200 Jiri Pirko wrote:

> + *	Register devlink port with provided port index. User can use
> + *	any indexing, even hw-related one. devlink_port structure
> + *	is convenient to be embedded inside user driver private structure.
> + *	Note that the caller should take care of zeroing the devlink_port
> + *	structure.

Should we also mention that the port type has to be set later?
I guess that may be beyond the scope.

> + */

> +/**
> + *	devlink_port_unregister - Unregister devlink port

devl_

> + *
> + *	@devlink_port: devlink port
> + */

I wonder if we should use this as an opportunity to start following 
the more modern kdoc format:

No tab indentation, and () after the function's name.

At least for the new kdoc we add.

>  void devl_port_unregister(struct devlink_port *devlink_port)
>  {
>  	lockdep_assert_held(&devlink_port->devlink->lock);

Re: [patch net-next 3/3] net: devlink: fix unlocked vs locked functions descriptions

[Jiri Pirko]

Fri, Jul 01, 2022 at 06:30:37PM CEST, kuba@kernel.org wrote:
>On Fri,  1 Jul 2022 11:59:26 +0200 Jiri Pirko wrote:
>
>> + *	Register devlink port with provided port index. User can use
>> + *	any indexing, even hw-related one. devlink_port structure
>> + *	is convenient to be embedded inside user driver private structure.
>> + *	Note that the caller should take care of zeroing the devlink_port
>> + *	structure.
>
>Should we also mention that the port type has to be set later?
>I guess that may be beyond the scope.

Let's do that in a separate patch. This is just to keep consistency
between devlink_ and devl_

>
>> + */
>
>> +/**
>> + *	devlink_port_unregister - Unregister devlink port
>
>devl_

Right.


>
>> + *
>> + *	@devlink_port: devlink port
>> + */
>
>I wonder if we should use this as an opportunity to start following 
>the more modern kdoc format:
>
>No tab indentation, and () after the function's name.
>
>At least for the new kdoc we add.

Okay. Makes sense.


>
>>  void devl_port_unregister(struct devlink_port *devlink_port)
>>  {
>>  	lockdep_assert_held(&devlink_port->devlink->lock);
>