Re: [PATCH net-next v8 2/6] net: dsa: Add convenience functions for frame handling

[Vladimir Oltean <olteanv@gmail.com>]

On Fri, Sep 09, 2022 at 10:51:34AM +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>
> 

Please no new lines between tags.

> Signed-off-by: Mattias Forsblad <mattias.forsblad@gmail.com>
> ---
>  include/net/dsa.h | 14 ++++++++++++++
>  net/dsa/dsa.c     | 17 +++++++++++++++++
>  net/dsa/dsa2.c    |  2 ++
>  3 files changed, 33 insertions(+)
> 
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index f2ce12860546..0e8a7ef17490 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,18 @@ 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? */
> +	if (completion)
> +		complete(completion);
> +	else
> +		complete(&ds->inband_done);

Could you please do the same thing here as what Paolo suggested:
	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..a1b3ecfdffb8 100644
> --- a/net/dsa/dsa2.c
> +++ b/net/dsa/dsa2.c
> @@ -1746,6 +1746,8 @@ static int dsa_switch_probe(struct dsa_switch *ds)
>  		dsa_tree_put(dst);
>  	}
>  
> +	init_completion(&ds->inband_done);
> +

This is actually done relatively late here (later than ds->ops->setup()
is called), considering that we could use Ethernet based management even
for general purpose register access. I would consider putting it right
at the beginning of dsa_switch_setup(), after the "if (ds->setup)" check.

>  	return err;
>  }
>  
> -- 
> 2.25.1
>