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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YxdAhDHy1V22HFw+@lunn.ch>
Date:   Tue, 6 Sep 2022 14:43:48 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Mattias Forsblad <mattias.forsblad@...il.com>
Cc:     netdev@...r.kernel.org, Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net-next v4 2/6] net: dsa: Add convenience functions for
 frame handling

On Tue, Sep 06, 2022 at 08:34:46AM +0200, Mattias Forsblad wrote:
> Add common control functions for drivers that need
> to send and wait for control frames.

It would be nice to explain why a custom complete is needed. Ideally,
it should not be needed at all.

> Signed-off-by: Mattias Forsblad <mattias.forsblad@...il.com>
> ---
>  include/net/dsa.h | 13 +++++++++++++
>  net/dsa/dsa.c     | 28 ++++++++++++++++++++++++++++
>  net/dsa/dsa2.c    |  2 ++
>  3 files changed, 43 insertions(+)
> 
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index f2ce12860546..70a358641235 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,17 @@ 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);

Blank line please.

> +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);
> +}
> +
>  #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..2d7add779b6f 100644
> --- a/net/dsa/dsa.c
> +++ b/net/dsa/dsa.c
> @@ -324,6 +324,34 @@ 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)
> +{
> +	int ret;
> +	struct completion *com;

Reverse christmas tree. Longest lines first.

> +
> +	/* Custom completion? */
> +	if (completion)
> +		com = completion;
> +	else
> +		com = &ds->inband_done;
> +
> +	reinit_completion(com);
> +
> +	if (skb)
> +		dev_queue_xmit(skb);
> +
> +	ret = wait_for_completion_timeout(com, msecs_to_jiffies(timeout));
> +	if (ret <= 0) {
> +		dev_dbg(ds->dev, "DSA inband: timeout waiting for answer\n");
> +
> +		return -ETIMEDOUT;
> +	}

It looks like wait_for_completion_timeout() can return a negative
error code. You should return that error code, not replace it with
-ETIMEDOUT. If it returns 0, then it has timed out, and returning
-ETIMEDOUT does make sense. If the completion is indicated before the
timeout, the return value is the remaining time. So you can return a
positive number here. It is worth documenting that, since a common
patterns is:

	err = dsa_switch_inband_tx()
	if (err)
		return err;

does not work in this case.

     Andrew