diff mbox series

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

Message ID 20220916121817.4061532-3-mattias.forsblad@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series net: dsa: qca8k, mv88e6xxx: rmon: Add RMU support | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 19 this patch: 19
netdev/cc_maintainers success CCed 9 of 9 maintainers
netdev/build_clang success Errors and warnings before: 13 this patch: 13
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 19 this patch: 19
netdev/checkpatch warning WARNING: line length of 99 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Mattias Forsblad Sept. 16, 2022, 12:18 p.m. UTC 
Add common control functions for drivers that need
to send and wait for control frames.

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
Signed-off-by: Mattias Forsblad <mattias.forsblad@gmail.com>
---
 include/net/dsa.h | 11 +++++++++++
 net/dsa/dsa.c     | 17 +++++++++++++++++
 net/dsa/dsa2.c    |  2 ++
 3 files changed, 30 insertions(+)

Comments

Christian Marangi Sept. 16, 2022, 6:06 a.m. UTC | #1 
On Fri, Sep 16, 2022 at 02:18:13PM +0200, Mattias Forsblad wrote:
> Add common control functions for drivers that need
> to send and wait for control frames.
> 
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> Signed-off-by: Mattias Forsblad <mattias.forsblad@gmail.com>
> ---
>  include/net/dsa.h | 11 +++++++++++
>  net/dsa/dsa.c     | 17 +++++++++++++++++
>  net/dsa/dsa2.c    |  2 ++
>  3 files changed, 30 insertions(+)
> 
> 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);

Missing handling for custom completion!

Should be 

complete(completion ? 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;

Missing handling for custom completion!

Should be

com = completion ? 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
> -- 
> 2.25.1
>
Christian Marangi Sept. 16, 2022, 6:26 a.m. UTC | #2 
On Fri, Sep 16, 2022 at 04:47:57PM +0300, Vladimir Oltean wrote:
> On Fri, Sep 16, 2022 at 08:06:38AM +0200, Christian Marangi wrote:
> > > +static inline void dsa_switch_inband_complete(struct dsa_switch *ds, struct completion *completion)
> > > +{
> > > +	/* Custom completion? */
> > > +	complete(completion ?: &ds->inband_done);
> >
> > Missing handling for custom completion!
> >
> > Should be
> >
> > complete(completion ? completion : &ds->inband_done);
> 
> !!!!
> 
> https://en.wikipedia.org/wiki/%3F:#C
> https://en.wikipedia.org/wiki/Elvis_operator
> 
> | A GNU extension to C allows omitting the second operand, and using
> | implicitly the first operand as the second also:
> |
> | a == x ? : y;
> |
> | The expression is equivalent to
> |
> | a == x ? (a == x) : y;
> |
> | except that if x is an expression, it is evaluated only once. The
> | difference is significant if evaluating the expression has side effects.
> | This shorthand form is sometimes known as the Elvis operator in other
> | languages.
> 
> cat ternary.c
> #include <stdio.h>
> 
> int main(void)
> {
> 	printf("%d\n", 3 ?: 4);
> 	return 0;
> }
> 
> make ternary
> ./ternary
> 3

Oh well! The more you know ahahha.

Then sorry for the wrong review but still wouldn't be more
clear/readable with full syntax?
Vladimir Oltean Sept. 16, 2022, 1:47 p.m. UTC | #3 
On Fri, Sep 16, 2022 at 08:06:38AM +0200, Christian Marangi wrote:
> > +static inline void dsa_switch_inband_complete(struct dsa_switch *ds, struct completion *completion)
> > +{
> > +	/* Custom completion? */
> > +	complete(completion ?: &ds->inband_done);
>
> Missing handling for custom completion!
>
> Should be
>
> complete(completion ? completion : &ds->inband_done);

!!!!

https://en.wikipedia.org/wiki/%3F:#C
https://en.wikipedia.org/wiki/Elvis_operator

| A GNU extension to C allows omitting the second operand, and using
| implicitly the first operand as the second also:
|
| a == x ? : y;
|
| The expression is equivalent to
|
| a == x ? (a == x) : y;
|
| except that if x is an expression, it is evaluated only once. The
| difference is significant if evaluating the expression has side effects.
| This shorthand form is sometimes known as the Elvis operator in other
| languages.

cat ternary.c
#include <stdio.h>

int main(void)
{
	printf("%d\n", 3 ?: 4);
	return 0;
}

make ternary
./ternary
3
diff mbox series

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