[linux-next:master 11606/12643] arm-linux-gnueabi-ld: arch/arm/mach-pxa/devices.o:undefined reference to `gpio_set_wake'

[linux-next:master 11606/12643] arm-linux-gnueabi-ld: arch/arm/mach-pxa/devices.o:undefined reference to `gpio_set_wake'

[kernel test robot]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
head:   f477dd6eede3ecedc8963478571d99ec3bf3f762
commit: 917195d6f8293c35fcf9ed4bfe18f52cc1c9d2e6 [11606/12643] ARM: pxa: consolidate GPIO chip platform data
config: arm-pxa3xx_defconfig (https://download.01.org/0day-ci/archive/20240711/202407112039.cyyIQ3Js-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 13.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240711/202407112039.cyyIQ3Js-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/202407112039.cyyIQ3Js-lkp@intel.com/

All errors (new ones prefixed by >>):

>> arm-linux-gnueabi-ld: arch/arm/mach-pxa/devices.o:(.data+0x167c): undefined reference to `gpio_set_wake'

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

Re: [linux-next:master 11606/12643] arm-linux-gnueabi-ld: arch/arm/mach-pxa/devices.o:undefined reference to `gpio_set_wake'

[Dmitry Torokhov]

Hi Arnd,

On Thu, Jul 11, 2024 at 09:02:07PM +0800, kernel test robot wrote:
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
> head:   f477dd6eede3ecedc8963478571d99ec3bf3f762
> commit: 917195d6f8293c35fcf9ed4bfe18f52cc1c9d2e6 [11606/12643] ARM: pxa: consolidate GPIO chip platform data
> config: arm-pxa3xx_defconfig (https://download.01.org/0day-ci/archive/20240711/202407112039.cyyIQ3Js-lkp@intel.com/config)
> compiler: arm-linux-gnueabi-gcc (GCC) 13.3.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240711/202407112039.cyyIQ3Js-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/202407112039.cyyIQ3Js-lkp@intel.com/
> 
> All errors (new ones prefixed by >>):
> 
> >> arm-linux-gnueabi-ld: arch/arm/mach-pxa/devices.o:(.data+0x167c): undefined reference to `gpio_set_wake'

Sorry about this. I think the easiest way of fixing this is:

diff --git a/arch/arm/mach-pxa/devices.c b/arch/arm/mach-pxa/devices.c
index d050a4c78f97..7695cfce01a1 100644
--- a/arch/arm/mach-pxa/devices.c
+++ b/arch/arm/mach-pxa/devices.c
@@ -633,6 +633,7 @@ struct platform_device pxa27x_device_pwm1 = {
 };
 #endif /* CONFIG_PXA27x || CONFIG_PXA3xx */
 
+#if defined(CONFIG_PXA25x) || defined(CONFIG_PXA27x)
 const struct software_node pxa2xx_gpiochip_node = {
 	.name	= "gpio-pxa",
 };
@@ -684,6 +685,7 @@ struct platform_device pxa27x_device_gpio = {
 		.platform_data	= &pxa2xx_gpio_info,
 	},
 };
+#endif /* CONFIG_PXA25x || CONFIG_PXA27x */
 
 static struct resource pxa_dma_resource[] = {
 	[0] = {

If you are OK with this I'll submit proper patch.

Thanks!

-- 
Dmitry

Re: [linux-next:master 11606/12643] arm-linux-gnueabi-ld: arch/arm/mach-pxa/devices.o:undefined reference to `gpio_set_wake'

[Arnd Bergmann]

On Fri, Jul 12, 2024, at 07:31, Dmitry Torokhov wrote:
> @@ -684,6 +685,7 @@ struct platform_device pxa27x_device_gpio = {
>  		.platform_data	= &pxa2xx_gpio_info,
>  	},
>  };
> +#endif /* CONFIG_PXA25x || CONFIG_PXA27x */
> 
>  static struct resource pxa_dma_resource[] = {
>  	[0] = {
>
> If you are OK with this I'll submit proper patch.

Right, that's probably the easiest way. I was trying a more
elaborate change (see below), but there is a good chance I
introduce another regression in that, so let's go with
your patch. Please send it directly to soc@kernel.org
Cc:linux-arm-kernel and I'll pick it up.

One more thing I noticed: if you are doing more patches
to convert devices in board files to use software nodes,
we should probably try to move them away from static
platform_device definitions towards a dynamic
platform_device_register_simple() or similar interface
at the same time.

    Arnd


diff --git a/arch/arm/mach-pxa/devices.c b/arch/arm/mach-pxa/devices.c
index d050a4c78f97..8813a5d179ed 100644
--- a/arch/arm/mach-pxa/devices.c
+++ b/arch/arm/mach-pxa/devices.c
@@ -633,58 +633,6 @@ struct platform_device pxa27x_device_pwm1 = {
 };
 #endif /* CONFIG_PXA27x || CONFIG_PXA3xx */
 
-const struct software_node pxa2xx_gpiochip_node = {
-	.name	= "gpio-pxa",
-};
-
-struct resource pxa_resource_gpio[] = {
-	{
-		.start	= 0x40e00000,
-		.end	= 0x40e0ffff,
-		.flags	= IORESOURCE_MEM,
-	}, {
-		.start	= IRQ_GPIO0,
-		.end	= IRQ_GPIO0,
-		.name	= "gpio0",
-		.flags	= IORESOURCE_IRQ,
-	}, {
-		.start	= IRQ_GPIO1,
-		.end	= IRQ_GPIO1,
-		.name	= "gpio1",
-		.flags	= IORESOURCE_IRQ,
-	}, {
-		.start	= IRQ_GPIO_2_x,
-		.end	= IRQ_GPIO_2_x,
-		.name	= "gpio_mux",
-		.flags	= IORESOURCE_IRQ,
-	},
-};
-
-static struct pxa_gpio_platform_data pxa2xx_gpio_info = {
-	.irq_base	= PXA_GPIO_TO_IRQ(0),
-	.gpio_set_wake	= gpio_set_wake,
-};
-
-struct platform_device pxa25x_device_gpio = {
-	.name		= "pxa25x-gpio",
-	.id		= -1,
-	.num_resources	= ARRAY_SIZE(pxa_resource_gpio),
-	.resource	= pxa_resource_gpio,
-	.dev		= {
-		.platform_data	= &pxa2xx_gpio_info,
-	},
-};
-
-struct platform_device pxa27x_device_gpio = {
-	.name		= "pxa27x-gpio",
-	.id		= -1,
-	.num_resources	= ARRAY_SIZE(pxa_resource_gpio),
-	.resource	= pxa_resource_gpio,
-	.dev		= {
-		.platform_data	= &pxa2xx_gpio_info,
-	},
-};
-
 static struct resource pxa_dma_resource[] = {
 	[0] = {
 		.start	= 0x40000000,
diff --git a/arch/arm/mach-pxa/mfp-pxa2xx.c b/arch/arm/mach-pxa/mfp-pxa2xx.c
index f5a3d890f682..e0ac1f525f77 100644
--- a/arch/arm/mach-pxa/mfp-pxa2xx.c
+++ b/arch/arm/mach-pxa/mfp-pxa2xx.c
@@ -15,12 +15,16 @@
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/io.h>
+#include <linux/ioport.h>
+#include <linux/property.h>
+#include <linux/platform_device.h>
 #include <linux/syscore_ops.h>
 #include <linux/soc/pxa/cpu.h>
 
 #include "pxa2xx-regs.h"
 #include "mfp-pxa2xx.h"
 #include "mfp-pxa27x.h"
+#include "irqs.h"
 
 #include "generic.h"
 
@@ -151,24 +155,6 @@ void pxa2xx_mfp_config(unsigned long *mfp_cfgs, int num)
 	}
 }
 
-void pxa2xx_mfp_set_lpm(int mfp, unsigned long lpm)
-{
-	unsigned long flags, c;
-	int gpio;
-
-	gpio = __mfp_validate(mfp);
-	if (gpio < 0)
-		return;
-
-	local_irq_save(flags);
-
-	c = gpio_desc[gpio].config;
-	c = (c & ~MFP_LPM_STATE_MASK) | lpm;
-	__mfp_config_gpio(gpio, c);
-
-	local_irq_restore(flags);
-}
-
 int gpio_set_wake(unsigned int gpio, unsigned int on)
 {
 	struct gpio_desc *d;
@@ -221,7 +207,49 @@ int gpio_set_wake(unsigned int gpio, unsigned int on)
 	return 0;
 }
 
+const struct software_node pxa2xx_gpiochip_node = {
+	.name	= "gpio-pxa",
+};
+
+struct resource pxa_resource_gpio[] = {
+	{
+		.start	= 0x40e00000,
+		.end	= 0x40e0ffff,
+		.flags	= IORESOURCE_MEM,
+	}, {
+		.start	= IRQ_GPIO0,
+		.end	= IRQ_GPIO0,
+		.name	= "gpio0",
+		.flags	= IORESOURCE_IRQ,
+	}, {
+		.start	= IRQ_GPIO1,
+		.end	= IRQ_GPIO1,
+		.name	= "gpio1",
+		.flags	= IORESOURCE_IRQ,
+	}, {
+		.start	= IRQ_GPIO_2_x,
+		.end	= IRQ_GPIO_2_x,
+		.name	= "gpio_mux",
+		.flags	= IORESOURCE_IRQ,
+	},
+};
+
+static struct pxa_gpio_platform_data pxa2xx_gpio_info = {
+	.irq_base	= PXA_GPIO_TO_IRQ(0),
+	.gpio_set_wake	= gpio_set_wake,
+};
+
 #ifdef CONFIG_PXA25x
+struct platform_device pxa25x_device_gpio = {
+	.name		= "pxa25x-gpio",
+	.id		= -1,
+	.num_resources	= ARRAY_SIZE(pxa_resource_gpio),
+	.resource	= pxa_resource_gpio,
+	.dev		= {
+		.platform_data	= &pxa2xx_gpio_info,
+	},
+};
+
 static void __init pxa25x_mfp_init(void)
 {
 	int i;
@@ -247,6 +275,16 @@ static inline void pxa25x_mfp_init(void) {}
 #endif /* CONFIG_PXA25x */
 
 #ifdef CONFIG_PXA27x
+struct platform_device pxa27x_device_gpio = {
+	.name		= "pxa27x-gpio",
+	.id		= -1,
+	.num_resources	= ARRAY_SIZE(pxa_resource_gpio),
+	.resource	= pxa_resource_gpio,
+	.dev		= {
+		.platform_data	= &pxa2xx_gpio_info,
+	},
+};
+
 static int pxa27x_pkwr_gpio[] = {
 	13, 16, 17, 34, 36, 37, 38, 39, 90, 91, 93, 94,
 	95, 96, 97, 98, 99, 100, 101, 102

Re: [linux-next:master 11606/12643] arm-linux-gnueabi-ld: arch/arm/mach-pxa/devices.o:undefined reference to `gpio_set_wake'

[Dmitry Torokhov]

On Fri, Jul 12, 2024 at 08:41:58AM +0200, Arnd Bergmann wrote:
> On Fri, Jul 12, 2024, at 07:31, Dmitry Torokhov wrote:
> > @@ -684,6 +685,7 @@ struct platform_device pxa27x_device_gpio = {
> >  		.platform_data	= &pxa2xx_gpio_info,
> >  	},
> >  };
> > +#endif /* CONFIG_PXA25x || CONFIG_PXA27x */
> > 
> >  static struct resource pxa_dma_resource[] = {
> >  	[0] = {
> >
> > If you are OK with this I'll submit proper patch.
> 
> Right, that's probably the easiest way. I was trying a more
> elaborate change (see below), but there is a good chance I
> introduce another regression in that, so let's go with
> your patch. Please send it directly to soc@kernel.org
> Cc:linux-arm-kernel and I'll pick it up.

Done.

> 
> One more thing I noticed: if you are doing more patches
> to convert devices in board files to use software nodes,
> we should probably try to move them away from static
> platform_device definitions towards a dynamic
> platform_device_register_simple() or similar interface
> at the same time.

Sometimes platform_device_register_full() which operates on
platform_device_info structure is very handy, but in general
what is the issue with statically defined devices (when they
are never unregistered)?

Thanks.

-- 
Dmitry

Re: [linux-next:master 11606/12643] arm-linux-gnueabi-ld: arch/arm/mach-pxa/devices.o:undefined reference to `gpio_set_wake'

[Arnd Bergmann]

On Sat, Jul 13, 2024, at 05:38, Dmitry Torokhov wrote:
> On Fri, Jul 12, 2024 at 08:41:58AM +0200, Arnd Bergmann wrote:
>> On Fri, Jul 12, 2024, at 07:31, Dmitry Torokhov wrote:
>> > @@ -684,6 +685,7 @@ struct platform_device pxa27x_device_gpio = {
>> >  		.platform_data	= &pxa2xx_gpio_info,
>> >  	},
>> >  };
>> > +#endif /* CONFIG_PXA25x || CONFIG_PXA27x */
>> > 
>> >  static struct resource pxa_dma_resource[] = {
>> >  	[0] = {
>> >
>> > If you are OK with this I'll submit proper patch.
>> 
>> Right, that's probably the easiest way. I was trying a more
>> elaborate change (see below), but there is a good chance I
>> introduce another regression in that, so let's go with
>> your patch. Please send it directly to soc@kernel.org
>> Cc:linux-arm-kernel and I'll pick it up.
>
> Done.

Merged now in preparation for my pull requests, thanks!

>> One more thing I noticed: if you are doing more patches
>> to convert devices in board files to use software nodes,
>> we should probably try to move them away from static
>> platform_device definitions towards a dynamic
>> platform_device_register_simple() or similar interface
>> at the same time.
>
> Sometimes platform_device_register_full() which operates on
> platform_device_info structure is very handy, but in general
> what is the issue with statically defined devices (when they
> are never unregistered)?

Greg has been asking for this for many years, but it has
been unrealistic in the past because of the large number
of board files using static platform devices. As far as I
understand it, having static platform devices prevents us
from better enforcing lifetime management rules on device
objects. Adding Greg to Cc in case he has something to add.

The other point of course is static memory usage: a platform
device structure in .data is much larger than a
platform_device_info, and this adds up in a kernel that
tries to support lots of boards.

     Arnd

Re: [linux-next:master 11606/12643] arm-linux-gnueabi-ld: arch/arm/mach-pxa/devices.o:undefined reference to `gpio_set_wake'

[Greg Kroah-Hartman]

On Mon, Jul 15, 2024 at 02:42:06PM +0200, Arnd Bergmann wrote:
> On Sat, Jul 13, 2024, at 05:38, Dmitry Torokhov wrote:
> >> One more thing I noticed: if you are doing more patches
> >> to convert devices in board files to use software nodes,
> >> we should probably try to move them away from static
> >> platform_device definitions towards a dynamic
> >> platform_device_register_simple() or similar interface
> >> at the same time.
> >
> > Sometimes platform_device_register_full() which operates on
> > platform_device_info structure is very handy, but in general
> > what is the issue with statically defined devices (when they
> > are never unregistered)?
> 
> Greg has been asking for this for many years, but it has
> been unrealistic in the past because of the large number
> of board files using static platform devices. As far as I
> understand it, having static platform devices prevents us
> from better enforcing lifetime management rules on device
> objects. Adding Greg to Cc in case he has something to add.

Not much more to add other than "please don't do that."

thanks,

greg k-h