Re: [PATCH 4/5] powerpc/zImage: Rework serial driver probing

[Oliver]

On Tue, Mar 20, 2018 at 8:20 PM, Michael Ellerman  wrote:
> Oliver O'Halloran  writes:
>
>> Rather than checking the compatible string in serial_driver_init()
>> we call into the driver's init function and wait for a driver to
>> inidicate it bound to the device by returning zero.
>>
>> The pointers to each driver probe functions are stored in the
>> ".serialdrv" section of the zImage, similar to how initcalls and
>> whatnot are handled inside Linux. This structure allows us to
>> conditionally compile specific driver files based on the KConfig
>> settings. This is needed because we don't have access to the
>> KConfig #defines in the zImage source files (it's a giant #include
>> headache) so conditional compilation is the only way to eliminate
>> unnecessary code.
>
> Did you actually get the config.h include working? Or was it such a big
> headache that it doesn't even work?

I had a half-hearted go at it and ran into some problems with include
paths. The usual workaround for that is copying the files into the
build directory and using sed scripts to replace the <> include with a
"" include, so I said screw it and went with this instead. I'm not too
suprised about there being a few linker errors. I tested a few
different defconfigs before posting, but I figured either your CI or
the 0day bot would catch the rest ;)

> I'm pretty happy with this patch regardless, but it would be simpler and
> less bug prone if the CONFIG_ symbols were usable in the boot code.
>
> cheers

Re: [PATCH 4/5] powerpc/zImage: Rework serial driver probing

[Michael Ellerman]

Oliver O'Halloran  writes:

> Rather than checking the compatible string in serial_driver_init()
> we call into the driver's init function and wait for a driver to
> inidicate it bound to the device by returning zero.
>
> The pointers to each driver probe functions are stored in the
> ".serialdrv" section of the zImage, similar to how initcalls and
> whatnot are handled inside Linux. This structure allows us to
> conditionally compile specific driver files based on the KConfig
> settings. This is needed because we don't have access to the
> KConfig #defines in the zImage source files (it's a giant #include
> headache) so conditional compilation is the only way to eliminate
> unnecessary code.

Did you actually get the config.h include working? Or was it such a big
headache that it doesn't even work?

I'm pretty happy with this patch regardless, but it would be simpler and
less bug prone if the CONFIG_ symbols were usable in the boot code.

cheers

Re: [PATCH 4/5] powerpc/zImage: Rework serial driver probing

[kbuild test robot]

Hi Oliver,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on v4.16-rc4]
[also build test ERROR on next-20180319]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Oliver-O-Halloran/powerpc-zImage-Remove-ifdef-in-opal-c/20180320-023705
config: powerpc-ppc64_defconfig (attached as .config)
compiler: powerpc64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=powerpc 

All errors (new ones prefixed by >>):

>> arch/powerpc/boot/wrapper.a(serial.o):(.got2+0x1c): undefined reference to 
>> `_serial_drv_start'
>> arch/powerpc/boot/wrapper.a(serial.o):(.got2+0x24): undefined reference to 
>> `_serial_drv_end'

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

Re: [PATCH 4/5] powerpc/zImage: Rework serial driver probing

[kbuild test robot]

Hi Oliver,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on v4.16-rc4]
[also build test ERROR on next-20180319]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Oliver-O-Halloran/powerpc-zImage-Remove-ifdef-in-opal-c/20180320-023705
config: powerpc64-alldefconfig (attached as .config)
compiler: powerpc-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=powerpc64 

All errors (new ones prefixed by >>):

   arch/powerpc/boot/wrapper.a(crt0.o): In function `_zimage_start_opd':
>> (.text+0x0): multiple definition of `_zimage_start_opd'
   arch/powerpc/boot/crt0.o:(.text+0x0): first defined here
   arch/powerpc/boot/wrapper.a(crt0.o): In function `_zimage_start':
>> (.text+0x24): multiple definition of `_zimage_start_lib'
   arch/powerpc/boot/crt0.o:(.text+0x24): first defined here

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

[PATCH 4/5] powerpc/zImage: Rework serial driver probing

[Oliver O'Halloran]

Rather than checking the compatible string in serial_driver_init()
we call into the driver's init function and wait for a driver to
inidicate it bound to the device by returning zero.

The pointers to each driver probe functions are stored in the
".serialdrv" section of the zImage, similar to how initcalls and
whatnot are handled inside Linux. This structure allows us to
conditionally compile specific driver files based on the KConfig
settings. This is needed because we don't have access to the
KConfig #defines in the zImage source files (it's a giant #include
headache) so conditional compilation is the only way to eliminate
unnecessary code.

Signed-off-by: Oliver O'Halloran 
---
 arch/powerpc/boot/cpm-serial.c  |  1 +
 arch/powerpc/boot/mpc52xx-psc.c |  1 +
 arch/powerpc/boot/mpsc.c|  1 +
 arch/powerpc/boot/ns16550.c |  2 ++
 arch/powerpc/boot/opal.c|  1 +
 arch/powerpc/boot/ops.h | 16 +--
 arch/powerpc/boot/serial.c  | 41 +++--
 arch/powerpc/boot/uartlite.c|  1 +
 arch/powerpc/boot/wrapper   |  2 +-
 arch/powerpc/boot/zImage.coff.lds.S |  4 
 arch/powerpc/boot/zImage.lds.S  |  8 
 11 files changed, 42 insertions(+), 36 deletions(-)

diff --git a/arch/powerpc/boot/cpm-serial.c b/arch/powerpc/boot/cpm-serial.c
index f6b1cf23946e..5ebf3dfd810a 100644
--- a/arch/powerpc/boot/cpm-serial.c
+++ b/arch/powerpc/boot/cpm-serial.c
@@ -295,3 +295,4 @@ int cpm_console_init(void *devp, struct serial_console_data 
*scdp)
 
return 0;
 }
+SERIAL_DRIVER(cpm_console_init);
diff --git a/arch/powerpc/boot/mpc52xx-psc.c b/arch/powerpc/boot/mpc52xx-psc.c
index 75470936e661..c09e82eaf006 100644
--- a/arch/powerpc/boot/mpc52xx-psc.c
+++ b/arch/powerpc/boot/mpc52xx-psc.c
@@ -66,3 +66,4 @@ int mpc5200_psc_console_init(void *devp, struct 
serial_console_data *scdp)
 
return 0;
 }
+SERIAL_DRIVER(mpc5200_psc_console_init);
diff --git a/arch/powerpc/boot/mpsc.c b/arch/powerpc/boot/mpsc.c
index 59e23e886440..2d0a72522b3c 100644
--- a/arch/powerpc/boot/mpsc.c
+++ b/arch/powerpc/boot/mpsc.c
@@ -170,3 +170,4 @@ int mpsc_console_init(void *devp, struct 
serial_console_data *scdp)
 err_out:
return -1;
 }
+SERIAL_DRIVER(mpsc_console_init);
diff --git a/arch/powerpc/boot/ns16550.c b/arch/powerpc/boot/ns16550.c
index 0e7bd5284255..49e8c299b829 100644
--- a/arch/powerpc/boot/ns16550.c
+++ b/arch/powerpc/boot/ns16550.c
@@ -81,3 +81,5 @@ int ns16550_console_init(void *devp, struct 
serial_console_data *scdp)
 
return 0;
 }
+
+SERIAL_DRIVER(ns16550_console_init);
diff --git a/arch/powerpc/boot/opal.c b/arch/powerpc/boot/opal.c
index 0e92537536b9..dc345ff63422 100644
--- a/arch/powerpc/boot/opal.c
+++ b/arch/powerpc/boot/opal.c
@@ -102,3 +102,4 @@ int opal_console_init(void *devp, struct 
serial_console_data *scdp)
 
return 0;
 }
+SERIAL_DRIVER(opal_console_init);
diff --git a/arch/powerpc/boot/ops.h b/arch/powerpc/boot/ops.h
index fad1862f4b2d..06151ecb2f45 100644
--- a/arch/powerpc/boot/ops.h
+++ b/arch/powerpc/boot/ops.h
@@ -84,13 +84,17 @@ extern struct loader_info loader_info;
 
 void start(void);
 void fdt_init(void *blob);
+
+struct driver_ptr {
+   int (*ptr)(void *node, struct serial_console_data *scdp);
+};
+
+#define SERIAL_DRIVER(init_function) static const struct driver_ptr \
+   __attribute__((__section__(".serialdrv"))) __attribute__((used)) \
+   init_function##_ptr = { .ptr = _function, }
+
 int serial_console_init(void);
-int ns16550_console_init(void *devp, struct serial_console_data *scdp);
-int mpsc_console_init(void *devp, struct serial_console_data *scdp);
-int cpm_console_init(void *devp, struct serial_console_data *scdp);
-int mpc5200_psc_console_init(void *devp, struct serial_console_data *scdp);
-int uartlite_console_init(void *devp, struct serial_console_data *scdp);
-int opal_console_init(void *devp, struct serial_console_data *scdp);
+
 void *simple_alloc_init(char *base, unsigned long heap_size,
unsigned long granularity, unsigned long max_allocs);
 extern void flush_cache(void *, unsigned long);
diff --git a/arch/powerpc/boot/serial.c b/arch/powerpc/boot/serial.c
index 88955095ec07..a7f9ac9a27b5 100644
--- a/arch/powerpc/boot/serial.c
+++ b/arch/powerpc/boot/serial.c
@@ -105,11 +105,15 @@ static void *serial_get_stdout_devp(void)
return NULL;
 }
 
+extern unsigned long _serial_drv_start;
+extern unsigned long _serial_drv_end;
+
 static struct serial_console_data serial_cd;
 
 /* Node's "compatible" property determines which serial driver to use */
 int serial_console_init(void)
 {
+   struct driver_ptr *start, *end, *probe;
void *devp;
int rc = -1;
 
@@ -117,35 +121,14 @@ int serial_console_init(void)
if (devp == NULL)
goto err_out;
 
-   if (dt_is_compatible(devp, "ns16550") ||
-   dt_is_compatible(devp, "pnpPNP,501"))
-