[net-next,v14,2/7] net: dsa: Add convenience functions for frame handling

Commit Message

Mattias Forsblad Sept. 19, 2022, 11:08 a.m. UTC

Add common control functions for drivers that need
to send and wait for control frames.

Reviewed-by: Florian Fainelli <[e-mail]>
Signed-off-by: Mattias Forsblad <[e-mail]>
---
 include/net/dsa.h | 11 +++++++++++
 net/dsa/dsa.c     | 17 +++++++++++++++++
 net/dsa/dsa2.c    |  2 ++
 3 files changed, 30 insertions(+)

Comments

Vladimir Oltean Sept. 19, 2022, 10:14 p.m. UTC | #1

On Mon, Sep 19, 2022 at 01:08:42PM +0200, Mattias Forsblad wrote:
> +int dsa_switch_inband_tx(struct dsa_switch *ds, struct sk_buff *skb,
> +			 struct completion *completion, unsigned long timeout)
> +{
> +	struct completion *com;
> +
> +	/* Custom completion? */
> +	com = completion ? : &ds->inband_done;
> +
> +	reinit_completion(com);
> +
> +	if (skb)
> +		dev_queue_xmit(skb);

Does it make sense from an API perspective to call dsa_switch_inband_tx()
with a NULL skb? If yes, please add a comment explaining why. If not,
please delete this check.

> +
> +	return wait_for_completion_timeout(com, msecs_to_jiffies(timeout));

If this is going to be provided as a DSA common layer "helper" function,
at least make an effort to document the expected return code.

Hint, wait_for_completion_timeout() returns an unsigned long time_left,
you return an int. What does it mean?!

At the bare minimum, leave a comment, especially when it's not obvious
(DSA typically uses negative integer values as error codes, and zero on
success. Here, zero is an error - timeout. If the amount of time left
does not matter, do translate this into 0 for success, and -ETIMEDOUT
for timeout). If you're also feeling generous, do please also update
Documentation/networking/dsa/dsa.rst with the info about the flow of
Ethernet-based register access that you wish was available while you
were figuring out how things worked.

> +}
> +EXPORT_SYMBOL_GPL(dsa_switch_inband_tx);

Andrew Lunn Sept. 19, 2022, 10:22 p.m. UTC | #2

> > +
> > +	return wait_for_completion_timeout(com, msecs_to_jiffies(timeout));
> 
> If this is going to be provided as a DSA common layer "helper" function,
> at least make an effort to document the expected return code.
> 
> Hint, wait_for_completion_timeout() returns an unsigned long time_left,
> you return an int. What does it mean?!
> 
> At the bare minimum, leave a comment, especially when it's not obvious
> (DSA typically uses negative integer values as error codes, and zero on
> success. Here, zero is an error - timeout. If the amount of time left
> does not matter, do translate this into 0 for success, and -ETIMEDOUT
> for timeout). If you're also feeling generous, do please also update
> Documentation/networking/dsa/dsa.rst with the info about the flow of
> Ethernet-based register access that you wish was available while you
> were figuring out how things worked.

Hi Vladimir

I just posted an alternative to this patch. One of the patches
addresses this point. I convert the return value to the more normal
-ve for error, 0 for success, including -ETIMEOUT if the completion
times out.

My patchset is however missing documentation. Once we get the basic
API agreed on, i can extend Documentation/networking/dsa/dsa.rst.

       Andrew

Patch

diff --git a/include/net/dsa.h b/include/net/dsa.h
index f2ce12860546..08f3fff5f4df 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -495,6 +495,8 @@  struct dsa_switch {
 	unsigned int		max_num_bridges;
 
 	unsigned int		num_ports;
+
+	struct completion	inband_done;
 };
 
 static inline struct dsa_port *dsa_to_port(struct dsa_switch *ds, int p)
@@ -1390,6 +1392,15 @@  void dsa_tag_drivers_register(struct dsa_tag_driver *dsa_tag_driver_array[],
 void dsa_tag_drivers_unregister(struct dsa_tag_driver *dsa_tag_driver_array[],
 				unsigned int count);
 
+int dsa_switch_inband_tx(struct dsa_switch *ds, struct sk_buff *skb,
+			 struct completion *completion, unsigned long timeout);
+
+static inline void dsa_switch_inband_complete(struct dsa_switch *ds, struct completion *completion)
+{
+	/* Custom completion? */
+	complete(completion ?: &ds->inband_done);
+}
+
 #define dsa_tag_driver_module_drivers(__dsa_tag_drivers_array, __count)	\
 static int __init dsa_tag_driver_module_init(void)			\
 {									\
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index be7b320cda76..ad870494d68b 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -324,6 +324,23 @@  int dsa_switch_resume(struct dsa_switch *ds)
 EXPORT_SYMBOL_GPL(dsa_switch_resume);
 #endif
 
+int dsa_switch_inband_tx(struct dsa_switch *ds, struct sk_buff *skb,
+			 struct completion *completion, unsigned long timeout)
+{
+	struct completion *com;
+
+	/* Custom completion? */
+	com = completion ? : &ds->inband_done;
+
+	reinit_completion(com);
+
+	if (skb)
+		dev_queue_xmit(skb);
+
+	return wait_for_completion_timeout(com, msecs_to_jiffies(timeout));
+}
+EXPORT_SYMBOL_GPL(dsa_switch_inband_tx);
+
 static struct packet_type dsa_pack_type __read_mostly = {
 	.type	= cpu_to_be16(ETH_P_XDSA),
 	.func	= dsa_switch_rcv,
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index ed56c7a554b8..a048a6200789 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -874,6 +874,8 @@  static int dsa_switch_setup(struct dsa_switch *ds)
 	if (ds->setup)
 		return 0;
 
+	init_completion(&ds->inband_done);
+
 	/* Initialize ds->phys_mii_mask before registering the slave MDIO bus
 	 * driver and before ops->setup() has run, since the switch drivers and
 	 * the slave MDIO bus driver rely on these values for probing PHY