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

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

[Christian Marangi]

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.

Suggested-by: Marion & Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Signed-off-by: Christian Marangi <ansuelsmth@gmail.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)
+{
+	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;
-- 
2.45.2

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

[Andrew Lunn]

> +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);

This looks to be the wrong function name.

    Andrew

---
pw-bot: cr

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

[Christian Marangi]

On Fri, Nov 08, 2024 at 09:35:32PM +0100, Andrew Lunn wrote:
> > +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);
> 
> This looks to be the wrong function name.
>

Ah... Anyway aside from this, is the feature OK? Questioning why it
wasn't proposed early...

> 
> ---
> pw-bot: cr
> 

-- 
	Ansuel

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

[Andrew Lunn]

On Fri, Nov 08, 2024 at 09:53:48PM +0100, Christian Marangi wrote:
> On Fri, Nov 08, 2024 at 09:35:32PM +0100, Andrew Lunn wrote:
> > > +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);
> > 
> > This looks to be the wrong function name.
> >
> 
> Ah... Anyway aside from this, is the feature OK? Questioning why it
> wasn't proposed early...

Some people blindly make use of devm_ without thinking about
ordering. These helpers can introduce bugs. So they are not always
liked.

It would be best if you added the helper at the same time as its user.

	Andrew

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@wanadoo.fr>

Please, remove the "Marion &"

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

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

[kernel test robot]

Hi Christian,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Christian-Marangi/net-dsa-add-devm_dsa_register_switch/20241109-040405
base:   net-next/main
patch link:    https://lore.kernel.org/r/20241108200217.2761-1-ansuelsmth%40gmail.com
patch subject: [net-next PATCH] net: dsa: add devm_dsa_register_switch()
config: s390-allmodconfig (https://download.01.org/0day-ci/archive/20241109/202411091011.vIDu9Nhl-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 592c0fe55f6d9a811028b5f3507be91458ab2713)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241109/202411091011.vIDu9Nhl-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411091011.vIDu9Nhl-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from net/dsa/dsa.c:10:
   In file included from include/linux/device.h:32:
   In file included from include/linux/device/driver.h:21:
   In file included from include/linux/module.h:19:
   In file included from include/linux/elf.h:6:
   In file included from arch/s390/include/asm/elf.h:181:
   In file included from arch/s390/include/asm/mmu_context.h:11:
   In file included from arch/s390/include/asm/pgalloc.h:18:
   In file included from include/linux/mm.h:2213:
   include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     504 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     505 |                            item];
         |                            ~~~~
   include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     511 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     512 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     518 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
   include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     524 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     525 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   In file included from net/dsa/dsa.c:14:
   In file included from include/linux/netdevice.h:38:
   In file included from include/net/net_namespace.h:43:
   In file included from include/linux/skbuff.h:28:
   In file included from include/linux/dma-mapping.h:11:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:95:
   include/asm-generic/io.h:548:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     548 |         val = __raw_readb(PCI_IOBASE + addr);
         |                           ~~~~~~~~~~ ^
   include/asm-generic/io.h:561:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     561 |         val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu'
      37 | #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
         |                                                           ^
   include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
     102 | #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
         |                                                      ^
   In file included from net/dsa/dsa.c:14:
   In file included from include/linux/netdevice.h:38:
   In file included from include/net/net_namespace.h:43:
   In file included from include/linux/skbuff.h:28:
   In file included from include/linux/dma-mapping.h:11:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:95:
   include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     574 |         val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu'
      35 | #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
         |                                                           ^
   include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
     115 | #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
         |                                                      ^
   In file included from net/dsa/dsa.c:14:
   In file included from include/linux/netdevice.h:38:
   In file included from include/net/net_namespace.h:43:
   In file included from include/linux/skbuff.h:28:
   In file included from include/linux/dma-mapping.h:11:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:95:
   include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     585 |         __raw_writeb(value, PCI_IOBASE + addr);
         |                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     595 |         __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     605 |         __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:693:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     693 |         readsb(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:701:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     701 |         readsw(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:709:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     709 |         readsl(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:718:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     718 |         writesb(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
   include/asm-generic/io.h:727:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     727 |         writesw(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
   include/asm-generic/io.h:736:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     736 |         writesl(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
>> <inline asm>:4:33: error: symbol '__export_symbol_dsa_register_switch' is already defined
       4 | .section ".export_symbol","a" ; __export_symbol_dsa_register_switch: ; .asciz "GPL" ; .asciz "" ; .balign 8 ; .quad dsa_register_switch ; .previous
         |                                 ^
   16 warnings and 1 error generated.

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[kernel test robot]

Hi Christian,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Christian-Marangi/net-dsa-add-devm_dsa_register_switch/20241109-040405
base:   net-next/main
patch link:    https://lore.kernel.org/r/20241108200217.2761-1-ansuelsmth%40gmail.com
patch subject: [net-next PATCH] net: dsa: add devm_dsa_register_switch()
config: i386-buildonly-randconfig-001-20241109 (https://download.01.org/0day-ci/archive/20241109/202411091122.phMAmman-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241109/202411091122.phMAmman-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411091122.phMAmman-lkp@intel.com/

All errors (new ones prefixed by >>):

   /tmp/cc81Xoq7.s: Assembler messages:
>> /tmp/cc81Xoq7.s:10: Error: symbol `__export_symbol_dsa_register_switch' is already defined

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki