netdev - Re: [net-next PATCH] net: dsa: add devm_dsa_register

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: <9ea17ec0-921f-4197-904e-52a91f6a5170@wanadoo.fr>
Date: Fri, 8 Nov 2024 23:22:37 +0100
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Christian Marangi <ansuelsmth@...il.com>, Andrew Lunn <andrew@...n.ch>,
 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>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [net-next PATCH] net: dsa: add devm_dsa_register_switch()


Le 08/11/2024 à 21:02, Christian Marangi a écrit :
> Some DSA driver can be simplified if devres takes care of unregistering
> the DSA switch. This permits to effectively drop the remove OP from
> driver that just execute the dsa_unregister_switch() and nothing else.

Nit: s/driver/drivers/

> Suggested-by: Marion & Christophe JAILLET <christophe.jaillet@...adoo.fr>

Please, remove the "Marion &"

> Signed-off-by: Christian Marangi <ansuelsmth@...il.com>
> ---
>   include/net/dsa.h |  1 +
>   net/dsa/dsa.c     | 19 +++++++++++++++++++
>   2 files changed, 20 insertions(+)
>
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 72ae65e7246a..c703d5dc3fb0 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -1355,6 +1355,7 @@ static inline void dsa_tag_generic_flow_dissect(const struct sk_buff *skb,
>   
>   void dsa_unregister_switch(struct dsa_switch *ds);
>   int dsa_register_switch(struct dsa_switch *ds);
> +int devm_dsa_register_switch(struct device *dev, struct dsa_switch *ds);
>   void dsa_switch_shutdown(struct dsa_switch *ds);
>   struct dsa_switch *dsa_switch_find(int tree_index, int sw_index);
>   void dsa_flush_workqueue(void);
> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> index 5a7c0e565a89..5cf1bac367ca 100644
> --- a/net/dsa/dsa.c
> +++ b/net/dsa/dsa.c
> @@ -1544,6 +1544,25 @@ int dsa_register_switch(struct dsa_switch *ds)
>   }
>   EXPORT_SYMBOL_GPL(dsa_register_switch);
>   
> +static void devm_dsa_unregister_switch(void *data)

I was also wondering if it would make sense to have callbacks used by 
devm_add_action_or_reset() have the __cold annotation.
(AFAIK, it is never used for that up to now)

CJ

> +{
> +	struct dsa_switch *ds = data;
> +
> +	dsa_unregister_switch(ds);
> +}
> +
> +int devm_dsa_register_switch(struct device *dev, struct dsa_switch *ds)
> +{
> +	int err;
> +
> +	err = dsa_register_switch(ds);
> +	if (err)
> +		return err;
> +
> +	return devm_add_action_or_reset(dev, devm_dsa_unregister_switch, ds);
> +}
> +EXPORT_SYMBOL_GPL(dsa_register_switch);
> +
>   static void dsa_switch_remove(struct dsa_switch *ds)
>   {
>   	struct dsa_switch_tree *dst = ds->dst;