Re: [net-next PATCH] net: dsa: add devm_dsa_register_switch()

[Christophe JAILLET]

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@xxxxxxxxxx>

Please, remove the "Marion &"

> Signed-off-by: Christian Marangi <ansuelsmth@xxxxxxxxx>
> ---
>   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;