* Re: [PATCH v5] earlycon: Use a pointer table to fix __earlycon_table stride
From: Rob Herring @ 2018-03-26 22:24 UTC (permalink / raw)
To: Daniel Kurtz
Cc: Greg Kroah-Hartman, Guenter Roeck, adurbin, linux-kernel,
Frank Rowand, Jiri Slaby, Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE,
open list:SERIAL DRIVERS,
open list:GENERIC INCLUDE/ASM HEADER FILES
In-Reply-To: <20180320220536.20245-1-djkurtz@chromium.org>
On Tue, Mar 20, 2018 at 04:05:36PM -0600, Daniel Kurtz wrote:
> Commit 99492c39f39f ("earlycon: Fix __earlycon_table stride") tried to fix
> __earlycon_table stride by forcing the earlycon_id struct alignment to 32
> and asking the linker to 32-byte align the __earlycon_table symbol. This
> fix was based on commit 07fca0e57fca92 ("tracing: Properly align linker
> defined symbols") which tried a similar fix for the tracing subsystem.
>
> However, this fix doesn't quite work because there is no guarantee that
> gcc will place structures packed into an array format. In fact, gcc 4.9
> chooses to 64-byte align these structs by inserting additional padding
> between the entries because it has no clue that they are supposed to be in
> an array. If we are unlucky, the linker will assign symbol
> "__earlycon_table" to a 32-byte aligned address which does not correspond
> to the 64-byte aligned contents of section "__earlycon_table".
>
> To address this same problem, the fix to the tracing system was
> subsequently re-implemented using a more robust table of pointers approach
> by commits:
> 3d56e331b653 ("tracing: Replace syscall_meta_data struct array with pointer array")
> 654986462939 ("tracepoints: Fix section alignment using pointer array")
> e4a9ea5ee7c8 ("tracing: Replace trace_event struct array with pointer array")
>
> Let's use this same "array of pointers to structs" approach for
> EARLYCON_TABLE.
Wouldn't every use of linker sections with structs have this problem? We
use them for clocks, irqs, timers unless those have been fixed.
>
> Fixes: 99492c39f39f ("earlycon: Fix __earlycon_table stride")
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> Suggested-by: Aaron Durbin <adurbin@chromium.org>
> ---
> Changes since v2:
> * Use __initconst instead of __initdata to avoid h8300 and alpha kbuild errors
>
> Changes since v3:
> * Fixed typos in commit message
>
> Changes since v4:
> * removed Change-Id: from commit message
>
> drivers/of/fdt.c | 7 +++++--
> drivers/tty/serial/earlycon.c | 6 ++++--
> include/asm-generic/vmlinux.lds.h | 2 +-
> include/linux/serial_core.h | 21 ++++++++++++++-------
> 4 files changed, 24 insertions(+), 12 deletions(-)
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* [PATCH] Bluetooth: hci_bcm: Remove DMI quirk for the MINIX Z83-4
From: ianwmorrison @ 2018-03-26 22:09 UTC (permalink / raw)
To: marcel, gustavo, johan.hedberg
Cc: linux-bluetooth, linux-serial, linux-acpi, stable, hdegoede,
ianwmorrison
As Interrupt resource specified IRQs are now assumed to be always
active-low the DMI quirk for the MINIX Z83-4 is no longer required.
Cc: stable@vger.kernel.org
Signed-off-by: Ian W MORRISON <ianwmorrison@gmail.com>
---
drivers/bluetooth/hci_bcm.c | 7 -------
1 file changed, 7 deletions(-)
diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
index 6828e485bc24..8a011267a4f6 100644
--- a/drivers/bluetooth/hci_bcm.c
+++ b/drivers/bluetooth/hci_bcm.c
@@ -804,13 +804,6 @@ static const struct dmi_system_id bcm_active_low_irq_dmi_table[] = {
DMI_EXACT_MATCH(DMI_PRODUCT_VERSION, "ThinkPad 8"),
},
},
- {
- .ident = "MINIX Z83-4",
- .matches = {
- DMI_EXACT_MATCH(DMI_SYS_VENDOR, "MINIX"),
- DMI_MATCH(DMI_PRODUCT_NAME, "Z83-4"),
- },
- },
{ }
};
--
2.11.0
^ permalink raw reply related
* Re: [PATCH 1/3] serial: 8250_early: Add earlycon support for AMD Carrizo / Stoneyridge
From: Alan Cox @ 2018-03-26 18:24 UTC (permalink / raw)
To: Aaron Durbin
Cc: Andy Shevchenko, Daniel Kurtz, Ricardo Ribalda Delgado,
Greg Kroah-Hartman, Jiri Slaby, Marc Gonzalez, Doug Anderson,
Matt Redfearn, Jeffy, open list:SERIAL DRIVERS,
Linux Kernel Mailing List
In-Reply-To: <CAE2855tGe0i9XoEPe9iLFpCds3fXyj9uGhDACvK06KaqvhvYBg@mail.gmail.com>
> Sadly, this situation
> is not unique to this hardware. There is hardware all over that does
> not meet the current assumptions being made in the early uart drivers
> within the kernel.
Is there any fundamental reason you can't just embed dt entries in the
ACPI table to describe the other features you need. I appreciate it
doesn't solve the generic PC case but it ought to help for anything where
the firmware cares about Linux ?
Alan
^ permalink raw reply
* Re: [PATCH 1/5] Bluetooth: hci_bcm: Add irq_polarity module option
From: Marcel Holtmann @ 2018-03-23 20:51 UTC (permalink / raw)
To: Hans de Goede
Cc: Gustavo F. Padovan, Johan Hedberg, linux-bluetooth, linux-serial,
linux-acpi, stable
In-Reply-To: <20180316202811.5678-1-hdegoede@redhat.com>
Hi Hans,
> Add irq_polarity module option for easier troubleshooting of irq-polarity
> issues.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> drivers/bluetooth/hci_bcm.c | 20 +++++++++++++++-----
> 1 file changed, 15 insertions(+), 5 deletions(-)
all 5 patches have been applied to bluetooth-next tree.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 1/5] Bluetooth: hci_bcm: Add irq_polarity module option
From: Hans de Goede @ 2018-03-23 11:30 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Gustavo F. Padovan, Johan Hedberg, linux-bluetooth, linux-serial,
linux-acpi, stable
In-Reply-To: <D1968341-EFFB-4D3D-94D4-521DB1BD01B1@holtmann.org>
Hi Marcel,
On 23-03-18 12:23, Marcel Holtmann wrote:
> Hi Hans,
>
>>>>> Add irq_polarity module option for easier troubleshooting of irq-polarity
>>>>> issues.
>>>>> Cc: stable@vger.kernel.org
>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>
>>>> Marcel, is there any reason why this (bug-fix) series is not merged
>>>> yet? Any review remarks I missed?
>>> I was waiting for net to be merged back into net-next so that I don’t have to deal with conflicts.
>>
>> Ah ok, no problem. I was just wondering if there was anything
>> I needed to do.
>
> I would really like to move over to using btuart.c that I posted a few days ago, but that will be obviously more work and maybe it is good to stabilize and simplify this inside hci_bcm.c first.
Yes I already saw your remark about this a few days ago. I do think that
moving to btuart.c eventually is a good idea, but your RFC seemed somewhat
feature incomplete. Mainly lacking the runtime-pm stuff, which is not
only needed for powersaving, but on some machines the shutdown gpio
defaults to "off", so the bluetooth won't work until we at a minimum
have added gpio support to btuart.c.
So for now I would like to get things fixed in hci_bcm.c first, as
you suggest. This will also allow easily getting the IRQ polarity and
missing IDs (at least those IDs which are confirmed to be actually
used) fixed in current stable kernels.
I'm afraid I don't have time to help much with btuart.c my hci_bcm.c
work is a small part of a project I'm working on to better support
Bay and Cherry Trail devices and that project itself is a personal
side-project. But when btuart.c is more feature complete I would be
happy to test it on various devices I own which have an uart attach
broadcom bt device.
Regards,
Hans
^ permalink raw reply
* Re: [PATCH 1/5] Bluetooth: hci_bcm: Add irq_polarity module option
From: Marcel Holtmann @ 2018-03-23 11:23 UTC (permalink / raw)
To: Hans de Goede
Cc: Gustavo F. Padovan, Johan Hedberg, linux-bluetooth, linux-serial,
linux-acpi, stable
In-Reply-To: <564f71d3-c9ab-c383-cd40-fe3edf92e9cf@redhat.com>
Hi Hans,
>>>> Add irq_polarity module option for easier troubleshooting of irq-polarity
>>>> issues.
>>>> Cc: stable@vger.kernel.org
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>
>>> Marcel, is there any reason why this (bug-fix) series is not merged
>>> yet? Any review remarks I missed?
>> I was waiting for net to be merged back into net-next so that I don’t have to deal with conflicts.
>
> Ah ok, no problem. I was just wondering if there was anything
> I needed to do.
I would really like to move over to using btuart.c that I posted a few days ago, but that will be obviously more work and maybe it is good to stabilize and simplify this inside hci_bcm.c first.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 1/5] Bluetooth: hci_bcm: Add irq_polarity module option
From: Hans de Goede @ 2018-03-23 11:17 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Gustavo F. Padovan, Johan Hedberg, linux-bluetooth, linux-serial,
linux-acpi, stable
In-Reply-To: <3ECCC2D8-93CF-4A37-AA12-EE991EF1B7E3@holtmann.org>
Hi,
On 23-03-18 12:16, Marcel Holtmann wrote:
> Hi Hans,
>
>>> Add irq_polarity module option for easier troubleshooting of irq-polarity
>>> issues.
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>
>> Marcel, is there any reason why this (bug-fix) series is not merged
>> yet? Any review remarks I missed?
>
> I was waiting for net to be merged back into net-next so that I don’t have to deal with conflicts.
Ah ok, no problem. I was just wondering if there was anything
I needed to do.
Regards,
Hans
^ permalink raw reply
* Re: [PATCH 1/5] Bluetooth: hci_bcm: Add irq_polarity module option
From: Marcel Holtmann @ 2018-03-23 11:16 UTC (permalink / raw)
To: Hans de Goede
Cc: Gustavo F. Padovan, Johan Hedberg, linux-bluetooth, linux-serial,
linux-acpi, stable
In-Reply-To: <a5553246-d39c-8d3a-430e-2b8fea3b7732@redhat.com>
Hi Hans,
>> Add irq_polarity module option for easier troubleshooting of irq-polarity
>> issues.
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>
> Marcel, is there any reason why this (bug-fix) series is not merged
> yet? Any review remarks I missed?
I was waiting for net to be merged back into net-next so that I don’t have to deal with conflicts.
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 1/5] Bluetooth: hci_bcm: Add irq_polarity module option
From: Hans de Goede @ 2018-03-23 10:56 UTC (permalink / raw)
To: Marcel Holtmann, Gustavo Padovan, Johan Hedberg
Cc: linux-bluetooth, linux-serial, linux-acpi, stable
In-Reply-To: <20180316202811.5678-1-hdegoede@redhat.com>
Hi Marcel, et al.,
On 16-03-18 21:28, Hans de Goede wrote:
> Add irq_polarity module option for easier troubleshooting of irq-polarity
> issues.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Marcel, is there any reason why this (bug-fix) series is not merged
yet? Any review remarks I missed?
Regards,
Hans
> ---
> drivers/bluetooth/hci_bcm.c | 20 +++++++++++++++-----
> 1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
> index 5e3aba3c3185..bb6cd1623132 100644
> --- a/drivers/bluetooth/hci_bcm.c
> +++ b/drivers/bluetooth/hci_bcm.c
> @@ -126,6 +126,10 @@ struct bcm_data {
> static DEFINE_MUTEX(bcm_device_lock);
> static LIST_HEAD(bcm_device_list);
>
> +static int irq_polarity = -1;
> +module_param(irq_polarity, int, 0444);
> +MODULE_PARM_DESC(irq_polarity, "IRQ polarity 0: active-high 1: active-low");
> +
> static inline void host_set_baudrate(struct hci_uart *hu, unsigned int speed)
> {
> if (hu->serdev)
> @@ -988,11 +992,17 @@ static int bcm_acpi_probe(struct bcm_device *dev)
> }
> acpi_dev_free_resource_list(&resources);
>
> - dmi_id = dmi_first_match(bcm_active_low_irq_dmi_table);
> - if (dmi_id) {
> - dev_warn(dev->dev, "%s: Overwriting IRQ polarity to active low",
> - dmi_id->ident);
> - dev->irq_active_low = true;
> + if (irq_polarity != -1) {
> + dev->irq_active_low = irq_polarity;
> + dev_warn(dev->dev, "Overwriting IRQ polarity to active %s by module-param\n",
> + dev->irq_active_low ? "low" : "high");
> + } else {
> + dmi_id = dmi_first_match(bcm_active_low_irq_dmi_table);
> + if (dmi_id) {
> + dev_warn(dev->dev, "%s: Overwriting IRQ polarity to active low",
> + dmi_id->ident);
> + dev->irq_active_low = true;
> + }
> }
>
> return 0;
>
^ permalink raw reply
* Re: [PATCH v2 2/2] earlycon: Use a pointer table to fix __earlycon_table stride
From: kbuild test robot @ 2018-03-23 2:54 UTC (permalink / raw)
Cc: kbuild-all, Greg Kroah-Hartman, Matthias Kaehlcke, Guenter Roeck,
adurbin, linux-kernel, Daniel Kurtz, Rob Herring, Frank Rowand,
Jiri Slaby, Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE,
open list:SERIAL DRIVERS,
open list:GENERIC INCLUDE/ASM HEADER FILES
In-Reply-To: <20180320175712.201572-3-djkurtz@chromium.org>
[-- Attachment #1: Type: text/plain, Size: 2462 bytes --]
Hi Daniel,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on tty/tty-testing]
[also build test ERROR on v4.16-rc6 next-20180322]
[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/Daniel-Kurtz/serial-sh-sci-Remove-__initdata-attribute-for-struct-port_cfg/20180323-085004
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: i386-allyesconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
>> drivers/tty/serial/sh-sci.c:3293:81: error: early_driver causes a section type conflict with __UNIQUE_ID___earlycon_hscif44
early_platform_init_buffer("earlyprintk", &sci_driver,
^
drivers/tty/serial/sh-sci.c:3349:33: note: '__UNIQUE_ID___earlycon_hscif44' was declared here
OF_EARLYCON_DECLARE(hscif, "renesas,hscif", hscif_early_console_setup);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +3293 drivers/tty/serial/sh-sci.c
^1da177e drivers/serial/sh-sci.c Linus Torvalds 2005-04-16 3291
7b6fd3bf drivers/serial/sh-sci.c Magnus Damm 2009-12-14 3292 #ifdef CONFIG_SERIAL_SH_SCI_CONSOLE
7b6fd3bf drivers/serial/sh-sci.c Magnus Damm 2009-12-14 @3293 early_platform_init_buffer("earlyprintk", &sci_driver,
7b6fd3bf drivers/serial/sh-sci.c Magnus Damm 2009-12-14 3294 early_serial_buf, ARRAY_SIZE(early_serial_buf));
7b6fd3bf drivers/serial/sh-sci.c Magnus Damm 2009-12-14 3295 #endif
0b0cced1 drivers/tty/serial/sh-sci.c Yoshinori Sato 2015-12-24 3296 #ifdef CONFIG_SERIAL_SH_SCI_EARLYCON
2897809d drivers/tty/serial/sh-sci.c Daniel Kurtz 2018-03-20 3297 static struct plat_sci_port port_cfg;
0b0cced1 drivers/tty/serial/sh-sci.c Yoshinori Sato 2015-12-24 3298
:::::: The code at line 3293 was first introduced by commit
:::::: 7b6fd3bf82c4901f6ba0101ba71a5c507c24f9cf sh-sci: Extend sh-sci driver with early console V2
:::::: TO: Magnus Damm <damm@opensource.se>
:::::: CC: Paul Mundt <lethal@linux-sh.org>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 62176 bytes --]
^ permalink raw reply
* [PATCH v4] serial: mvebu-uart: fix tx lost characters
From: Gabriel Matni @ 2018-03-22 19:15 UTC (permalink / raw)
To: gregkh@linuxfoundation.org
Cc: Miquel Raynal, Grégory Clement, linux-serial@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, stable@vger.kernel.org
From: Gabriel Matni <gabriel.matni@exfo.com>
Fixes missing characters on kernel console at low baud rates (i.e.9600).
The driver should poll TX_RDY or TX_FIFO_EMP instead of TX_EMP to ensure
that the transmitter holding register (THR) is ready to receive a new byte.
TX_EMP tells us when it is possible to send a break sequence via
SND_BRK_SEQ. While this also indicates that both the THR and the TSR are
empty, it does not guarantee that a new byte can be written just yet.
Fixes: 30530791a7a0 ("serial: mvebu-uart: initial support for Armada-3700 serial port")
Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
Acked-by: Gregory CLEMENT <gregory.clement@bootlin.com>
Signed-off-by: Gabriel Matni <gabriel.matni@exfo.com>
---
Changes since v3:
- mail the patch in a clean email
- fix the subject line accordingly (no Re:)
Changes since v2:
- use one line for the "Fixes" entry
- removed trailing space between Signed-off-by entry and ---
- start using versioning, previous fixes in v1
Changes since v1:
- patch was corrupt, could not be applied
- fixed line indent
---
drivers/tty/serial/mvebu-uart.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/mvebu-uart.c b/drivers/tty/serial/mvebu-uart.c
index a100e98259d7..f0df0640208e 100644
--- a/drivers/tty/serial/mvebu-uart.c
+++ b/drivers/tty/serial/mvebu-uart.c
@@ -618,7 +618,7 @@ static void wait_for_xmitr(struct uart_port *port)
u32 val;
readl_poll_timeout_atomic(port->membase + UART_STAT, val,
- (val & STAT_TX_EMP), 1, 10000);
+ (val & STAT_TX_RDY(port)), 1, 10000);
}
static void mvebu_uart_console_putchar(struct uart_port *port, int ch)
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v3] serial: mvebu-uart: fix tx lost characters
From: gregkh @ 2018-03-22 17:35 UTC (permalink / raw)
To: Gabriel Matni
Cc: Miquel Raynal, linux-serial@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, stable@vger.kernel.org,
Grégory Clement, Thomas Petazzoni
In-Reply-To: <3B588D51285A4A4D8D39C94212E07826279E39@SPQCMBX02.exfo.com>
On Tue, Mar 20, 2018 at 04:09:38PM +0000, Gabriel Matni wrote:
> From: Gabriel Matni <gabriel.matni@exfo.com>
>
> Fixes missing characters on kernel console at low baud rates (i.e.9600).
> The driver should poll TX_RDY or TX_FIFO_EMP instead of TX_EMP to ensure
> that the transmitter holding register (THR) is ready to receive a new byte.
>
> TX_EMP tells us when it is possible to send a break sequence via
> SND_BRK_SEQ. While this also indicates that both the THR and the TSR are
> empty, it does not guarantee that a new byte can be written just yet.
>
> Fixes: 30530791a7a0 ("serial: mvebu-uart: initial support for Armada-3700 serial port")
> Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Acked-by: Gregory CLEMENT <gregory.clement@bootlin.com>
> Signed-off-by: Gabriel Matni <gabriel.matni@exfo.com>
> ---
> Changes since v2:
> - use one line for the "Fixes" entry
> - removed trailing space between Signed-off-by entry and ---
> - start using versioning, previous fixes in v1
>
> Changes since v1:
> - patch was corrupt, could not be applied
> - fixed line indent
> ---
> drivers/tty/serial/mvebu-uart.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/mvebu-uart.c b/drivers/tty/serial/mvebu-uart.c
> index a100e98259d7..f0df0640208e 100644
> --- a/drivers/tty/serial/mvebu-uart.c
> +++ b/drivers/tty/serial/mvebu-uart.c
> @@ -618,7 +618,7 @@ static void wait_for_xmitr(struct uart_port *port)
> u32 val;
>
> readl_poll_timeout_atomic(port->membase + UART_STAT, val,
> - (val & STAT_TX_EMP), 1, 10000);
> + (val & STAT_TX_RDY(port)), 1, 10000);
> }
>
> static void mvebu_uart_console_putchar(struct uart_port *port, int ch)
> --
> 2.7.4
>
>
> > -----Original Message-----
> > From: gregkh@linuxfoundation.org <gregkh@linuxfoundation.org>
> > Sent: March 20, 2018 5:32 AM
> > To: Gabriel Matni <gabriel.matni@exfo.com>
> > Cc: Miquel Raynal <miquel.raynal@bootlin.com>; linux-
> > serial@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > stable@vger.kernel.org; Grégory Clement <gregory.clement@bootlin.com>;
> > Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> > Subject: Re: [PATCH] serial: mvebu-uart: fix tx lost characters
<snip>
What is all of this below the patch for?
Please clean up and send this properly, in a clean email, with no "Re:"
on the subject line, as a new patch/email.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH] serial: samsung: macros with complex values should be enclosed in parentheses
From: YOUNGKEUN OH @ 2018-03-22 9:04 UTC (permalink / raw)
To: Greg KH; +Cc: jslaby, linux-serial, linux-kernel, kernel-janitors
In-Reply-To: <20180322084511.GA6211@kroah.com>
On 03/22/2018 05:45 PM, Greg KH wrote:
> On Thu, Mar 22, 2018 at 01:37:45PM +0900, y.k.oh wrote:
>>
>>
>> On 03/14/2018 10:58 PM, Greg KH wrote:
>>> On Wed, Mar 14, 2018 at 11:17:05AM +0900, YOUNGKEUN OH wrote:
>>>> Cleanup checkpatch error:
>>>> ERROR: Macros with complex values should be enclosed in parentheses
>>>>
>>>> Signed-off-by: YOUNGKEUN OH <y.k.oh@samsung.com>
>>>> ---
>>>> drivers/tty/serial/samsung.c | 16 ++++++++--------
>>>> 1 file changed, 8 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
>>>> index 3f2f8c1..da9bddb1 100644
>>>> --- a/drivers/tty/serial/samsung.c
>>>> +++ b/drivers/tty/serial/samsung.c
>>>> @@ -1455,7 +1455,7 @@ static int __init s3c24xx_serial_console_init(void)
>>>> }
>>>> console_initcall(s3c24xx_serial_console_init);
>>>>
>>>> -#define S3C24XX_SERIAL_CONSOLE &s3c24xx_serial_console
>>>> +#define S3C24XX_SERIAL_CONSOLE (&s3c24xx_serial_console)
>>>
>>> That's not a complex macro!
>>>
>>> Please use checkpatch as a hint, not the "truth".
>>>
>>> thanks,
>>>
>>> greg k-h
>>>
>> Dear Greg.
>>
>> Yes! Of course that's not a pretty complex macro. However, I think it is
>> important to make a small effort to modify the minimum standard,
>> Checkpatch ERROR, to ensure consistency in the Linux code.
>> Starting with this Checkpatch error of samsung.c, I would like to start
>> my efforts on contribution from a small point. Please review it again.
>
> Why would I review something again that is not correct?
>
> If you wish to start out in kernel development, please start in
> drivers/staging/*/TODO, not in "core" kernel code. That is what the
> drivers/staging/ code is there for.
>
> good luck!
>
> greg k-h
>
Dear Greg.
Thank you for your comment. I'll see you soon with a good patch.
Sincerely,
Youngkeun Oh
^ permalink raw reply
* Re: [PATCH] serial: samsung: macros with complex values should be enclosed in parentheses
From: Greg KH @ 2018-03-22 8:45 UTC (permalink / raw)
To: y.k.oh; +Cc: jslaby, linux-serial, linux-kernel, kernel-janitors
In-Reply-To: <2f2dcf3c-2432-3dc7-5fd4-f9326ef48455@samsung.com>
On Thu, Mar 22, 2018 at 01:37:45PM +0900, y.k.oh wrote:
>
>
> On 03/14/2018 10:58 PM, Greg KH wrote:
> > On Wed, Mar 14, 2018 at 11:17:05AM +0900, YOUNGKEUN OH wrote:
> >> Cleanup checkpatch error:
> >> ERROR: Macros with complex values should be enclosed in parentheses
> >>
> >> Signed-off-by: YOUNGKEUN OH <y.k.oh@samsung.com>
> >> ---
> >> drivers/tty/serial/samsung.c | 16 ++++++++--------
> >> 1 file changed, 8 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
> >> index 3f2f8c1..da9bddb1 100644
> >> --- a/drivers/tty/serial/samsung.c
> >> +++ b/drivers/tty/serial/samsung.c
> >> @@ -1455,7 +1455,7 @@ static int __init s3c24xx_serial_console_init(void)
> >> }
> >> console_initcall(s3c24xx_serial_console_init);
> >>
> >> -#define S3C24XX_SERIAL_CONSOLE &s3c24xx_serial_console
> >> +#define S3C24XX_SERIAL_CONSOLE (&s3c24xx_serial_console)
> >
> > That's not a complex macro!
> >
> > Please use checkpatch as a hint, not the "truth".
> >
> > thanks,
> >
> > greg k-h
> >
> Dear Greg.
>
> Yes! Of course that's not a pretty complex macro. However, I think it is
> important to make a small effort to modify the minimum standard,
> Checkpatch ERROR, to ensure consistency in the Linux code.
> Starting with this Checkpatch error of samsung.c, I would like to start
> my efforts on contribution from a small point. Please review it again.
Why would I review something again that is not correct?
If you wish to start out in kernel development, please start in
drivers/staging/*/TODO, not in "core" kernel code. That is what the
drivers/staging/ code is there for.
good luck!
greg k-h
^ permalink raw reply
* Re: [PATCH] serial: samsung: macros with complex values should be enclosed in parentheses
From: Joe Perches @ 2018-03-22 4:45 UTC (permalink / raw)
To: y.k.oh, Greg KH; +Cc: jslaby, linux-serial, linux-kernel, kernel-janitors
In-Reply-To: <2f2dcf3c-2432-3dc7-5fd4-f9326ef48455@samsung.com>
On Thu, 2018-03-22 at 13:37 +0900, y.k.oh wrote:
>
> On 03/14/2018 10:58 PM, Greg KH wrote:
> > On Wed, Mar 14, 2018 at 11:17:05AM +0900, YOUNGKEUN OH wrote:
> > > Cleanup checkpatch error:
> > > ERROR: Macros with complex values should be enclosed in parentheses
> > >
> > > Signed-off-by: YOUNGKEUN OH <y.k.oh@samsung.com>
> > > ---
> > > drivers/tty/serial/samsung.c | 16 ++++++++--------
> > > 1 file changed, 8 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
> > > index 3f2f8c1..da9bddb1 100644
> > > --- a/drivers/tty/serial/samsung.c
> > > +++ b/drivers/tty/serial/samsung.c
> > > @@ -1455,7 +1455,7 @@ static int __init s3c24xx_serial_console_init(void)
> > > }
> > > console_initcall(s3c24xx_serial_console_init);
> > >
> > > -#define S3C24XX_SERIAL_CONSOLE &s3c24xx_serial_console
> > > +#define S3C24XX_SERIAL_CONSOLE (&s3c24xx_serial_console)
> >
> > That's not a complex macro!
> >
> > Please use checkpatch as a hint, not the "truth".
> >
> > thanks,
> >
> > greg k-h
> >
>
> Dear Greg.
>
> Yes! Of course that's not a pretty complex macro. However, I think it is
> important to make a small effort to modify the minimum standard,
> Checkpatch ERROR, to ensure consistency in the Linux code.
> Starting with this Checkpatch error of samsung.c, I would like to start
> my efforts on contribution from a small point. Please review it again.
> If there are any points that require modification, such as comment,
> then I'll modify it.
It might be better to remove the #defines and use
an #ifdef in the one place S2C24XX_SERIAL_CONSOLE
is referenced instead.
^ permalink raw reply
* Re: [PATCH] serial: samsung: macros with complex values should be enclosed in parentheses
From: y.k.oh @ 2018-03-22 4:37 UTC (permalink / raw)
To: Greg KH; +Cc: jslaby, linux-serial, linux-kernel, kernel-janitors
In-Reply-To: <20180314135804.GA21350@kroah.com>
On 03/14/2018 10:58 PM, Greg KH wrote:
> On Wed, Mar 14, 2018 at 11:17:05AM +0900, YOUNGKEUN OH wrote:
>> Cleanup checkpatch error:
>> ERROR: Macros with complex values should be enclosed in parentheses
>>
>> Signed-off-by: YOUNGKEUN OH <y.k.oh@samsung.com>
>> ---
>> drivers/tty/serial/samsung.c | 16 ++++++++--------
>> 1 file changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
>> index 3f2f8c1..da9bddb1 100644
>> --- a/drivers/tty/serial/samsung.c
>> +++ b/drivers/tty/serial/samsung.c
>> @@ -1455,7 +1455,7 @@ static int __init s3c24xx_serial_console_init(void)
>> }
>> console_initcall(s3c24xx_serial_console_init);
>>
>> -#define S3C24XX_SERIAL_CONSOLE &s3c24xx_serial_console
>> +#define S3C24XX_SERIAL_CONSOLE (&s3c24xx_serial_console)
>
> That's not a complex macro!
>
> Please use checkpatch as a hint, not the "truth".
>
> thanks,
>
> greg k-h
>
Dear Greg.
Yes! Of course that's not a pretty complex macro. However, I think it is
important to make a small effort to modify the minimum standard,
Checkpatch ERROR, to ensure consistency in the Linux code.
Starting with this Checkpatch error of samsung.c, I would like to start
my efforts on contribution from a small point. Please review it again.
If there are any points that require modification, such as comment,
then I'll modify it.
Thank you.
Best regards,
Youngkeun Oh
^ permalink raw reply
* Re: [PATCH v3] earlycon: Use a pointer table to fix __earlycon_table stride
From: Guenter Roeck @ 2018-03-20 22:12 UTC (permalink / raw)
To: Daniel Kurtz
Cc: Greg Kroah-Hartman, Guenter Roeck, Aaron Durbin, linux-kernel,
Rob Herring, Frank Rowand, Jiri Slaby, Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE,
open list:SERIAL DRIVERS,
open list:GENERIC INCLUDE/ASM HEADER FILES
In-Reply-To: <20180320220202.18464-1-djkurtz@chromium.org>
On Tue, Mar 20, 2018 at 3:02 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
> Commit 99492c39f39f ("earlycon: Fix __earlycon_table stride") tried to fix
> __earlycon_table stride by forcing the earlycon_id struct alignment to 32
> and asking the linker to 32-byte align the __earlycon_table symbol. This
> fix was based on commit 07fca0e57fca92 ("tracing: Properly align linker
> defined symbols") which tried a similar fix for the tracing subsystem.
>
> However, this fix doesn't quite work because there is no guarantee that
> gcc will place structures packed into an array format. In fact, gcc 4.9
> chooses to 64-byte align these structs by inserting additional padding
> between the entries because it has no clue that they are supposed to be in
> an array. If we are unlucky, the linker will assign symbol
> "__earlycon_table" to a 32-byte aligned address which does not correpsond
> to the 64-byte alignbed contents of section "__earlycon_table".
>
> To address this same problem, the fix to the tracing system was
> subsequently re-implemented using a more robust table of pointers approach
> by commits:
> 3d56e331b653 ("tracing: Replace syscall_meta_data struct array with pointer array")
> 654986462939 ("tracepoints: Fix section alignment using pointer array")
> e4a9ea5ee7c8 ("tracing: Replace trace_event struct array with pointer array")
>
> Let's use this same "array of pointers to structs" approach for
> EARLYCON_TABLE.
>
> Fixes: 99492c39f39f ("earlycon: Fix __earlycon_table stride")
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> Suggested-by: Aaron Durbin <adurbin@chromium.org>
Build tested on sh4, alpha, and h8300 to ensure that the build problem
reported by 0day is gone.
For that,
Tested-by: Guenter Roeck <groeck@chromium.org>
I am not an expert in this area, but the change makes sense to me. As such,
Reviewed-by: Guenter Roeck <groeck@chromium.org>
but take it with a grain of salt.
Guenter
> ---
> Changes since v2:
> * Use __initconst instead of __initdata to avoid h8300 and alpha kbuild errors
>
> drivers/of/fdt.c | 7 +++++--
> drivers/tty/serial/earlycon.c | 6 ++++--
> include/asm-generic/vmlinux.lds.h | 2 +-
> include/linux/serial_core.h | 21 ++++++++++++++-------
> 4 files changed, 24 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 84aa9d676375..6da20b9688f7 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -942,7 +942,7 @@ int __init early_init_dt_scan_chosen_stdout(void)
> int offset;
> const char *p, *q, *options = NULL;
> int l;
> - const struct earlycon_id *match;
> + const struct earlycon_id **p_match;
> const void *fdt = initial_boot_params;
>
> offset = fdt_path_offset(fdt, "/chosen");
> @@ -969,7 +969,10 @@ int __init early_init_dt_scan_chosen_stdout(void)
> return 0;
> }
>
> - for (match = __earlycon_table; match < __earlycon_table_end; match++) {
> + for (p_match = __earlycon_table; p_match < __earlycon_table_end;
> + p_match++) {
> + const struct earlycon_id *match = *p_match;
> +
> if (!match->compatible[0])
> continue;
>
> diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
> index a24278380fec..22683393a0f2 100644
> --- a/drivers/tty/serial/earlycon.c
> +++ b/drivers/tty/serial/earlycon.c
> @@ -169,7 +169,7 @@ static int __init register_earlycon(char *buf, const struct earlycon_id *match)
> */
> int __init setup_earlycon(char *buf)
> {
> - const struct earlycon_id *match;
> + const struct earlycon_id **p_match;
>
> if (!buf || !buf[0])
> return -EINVAL;
> @@ -177,7 +177,9 @@ int __init setup_earlycon(char *buf)
> if (early_con.flags & CON_ENABLED)
> return -EALREADY;
>
> - for (match = __earlycon_table; match < __earlycon_table_end; match++) {
> + for (p_match = __earlycon_table; p_match < __earlycon_table_end;
> + p_match++) {
> + const struct earlycon_id *match = *p_match;
> size_t len = strlen(match->name);
>
> if (strncmp(buf, match->name, len))
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index 1ab0e520d6fc..e17de55c2542 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -179,7 +179,7 @@
> #endif
>
> #ifdef CONFIG_SERIAL_EARLYCON
> -#define EARLYCON_TABLE() STRUCT_ALIGN(); \
> +#define EARLYCON_TABLE() . = ALIGN(8); \
> VMLINUX_SYMBOL(__earlycon_table) = .; \
> KEEP(*(__earlycon_table)) \
> VMLINUX_SYMBOL(__earlycon_table_end) = .;
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index b32df49a3bd5..c4219b9cbb70 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -351,10 +351,10 @@ struct earlycon_id {
> char name[16];
> char compatible[128];
> int (*setup)(struct earlycon_device *, const char *options);
> -} __aligned(32);
> +};
>
> -extern const struct earlycon_id __earlycon_table[];
> -extern const struct earlycon_id __earlycon_table_end[];
> +extern const struct earlycon_id *__earlycon_table[];
> +extern const struct earlycon_id *__earlycon_table_end[];
>
> #if defined(CONFIG_SERIAL_EARLYCON) && !defined(MODULE)
> #define EARLYCON_USED_OR_UNUSED __used
> @@ -362,12 +362,19 @@ extern const struct earlycon_id __earlycon_table_end[];
> #define EARLYCON_USED_OR_UNUSED __maybe_unused
> #endif
>
> -#define OF_EARLYCON_DECLARE(_name, compat, fn) \
> - static const struct earlycon_id __UNIQUE_ID(__earlycon_##_name) \
> - EARLYCON_USED_OR_UNUSED __section(__earlycon_table) \
> +#define _OF_EARLYCON_DECLARE(_name, compat, fn, unique_id) \
> + static const struct earlycon_id unique_id \
> + EARLYCON_USED_OR_UNUSED __initconst \
> = { .name = __stringify(_name), \
> .compatible = compat, \
> - .setup = fn }
> + .setup = fn }; \
> + static const struct earlycon_id EARLYCON_USED_OR_UNUSED \
> + __section(__earlycon_table) \
> + * const __PASTE(__p, unique_id) = &unique_id
> +
> +#define OF_EARLYCON_DECLARE(_name, compat, fn) \
> + _OF_EARLYCON_DECLARE(_name, compat, fn, \
> + __UNIQUE_ID(__earlycon_##_name))
>
> #define EARLYCON_DECLARE(_name, fn) OF_EARLYCON_DECLARE(_name, "", fn)
>
> --
> 2.16.2.804.g6dcf76e118-goog
>
^ permalink raw reply
* [PATCH v5] earlycon: Use a pointer table to fix __earlycon_table stride
From: Daniel Kurtz @ 2018-03-20 22:05 UTC (permalink / raw)
To: Greg Kroah-Hartman, Guenter Roeck
Cc: adurbin, linux-kernel, Daniel Kurtz, Rob Herring, Frank Rowand,
Jiri Slaby, Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE,
open list:SERIAL DRIVERS,
open list:GENERIC INCLUDE/ASM HEADER FILES
Commit 99492c39f39f ("earlycon: Fix __earlycon_table stride") tried to fix
__earlycon_table stride by forcing the earlycon_id struct alignment to 32
and asking the linker to 32-byte align the __earlycon_table symbol. This
fix was based on commit 07fca0e57fca92 ("tracing: Properly align linker
defined symbols") which tried a similar fix for the tracing subsystem.
However, this fix doesn't quite work because there is no guarantee that
gcc will place structures packed into an array format. In fact, gcc 4.9
chooses to 64-byte align these structs by inserting additional padding
between the entries because it has no clue that they are supposed to be in
an array. If we are unlucky, the linker will assign symbol
"__earlycon_table" to a 32-byte aligned address which does not correspond
to the 64-byte aligned contents of section "__earlycon_table".
To address this same problem, the fix to the tracing system was
subsequently re-implemented using a more robust table of pointers approach
by commits:
3d56e331b653 ("tracing: Replace syscall_meta_data struct array with pointer array")
654986462939 ("tracepoints: Fix section alignment using pointer array")
e4a9ea5ee7c8 ("tracing: Replace trace_event struct array with pointer array")
Let's use this same "array of pointers to structs" approach for
EARLYCON_TABLE.
Fixes: 99492c39f39f ("earlycon: Fix __earlycon_table stride")
Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
Suggested-by: Aaron Durbin <adurbin@chromium.org>
---
Changes since v2:
* Use __initconst instead of __initdata to avoid h8300 and alpha kbuild errors
Changes since v3:
* Fixed typos in commit message
Changes since v4:
* removed Change-Id: from commit message
drivers/of/fdt.c | 7 +++++--
drivers/tty/serial/earlycon.c | 6 ++++--
include/asm-generic/vmlinux.lds.h | 2 +-
include/linux/serial_core.h | 21 ++++++++++++++-------
4 files changed, 24 insertions(+), 12 deletions(-)
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 84aa9d676375..6da20b9688f7 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -942,7 +942,7 @@ int __init early_init_dt_scan_chosen_stdout(void)
int offset;
const char *p, *q, *options = NULL;
int l;
- const struct earlycon_id *match;
+ const struct earlycon_id **p_match;
const void *fdt = initial_boot_params;
offset = fdt_path_offset(fdt, "/chosen");
@@ -969,7 +969,10 @@ int __init early_init_dt_scan_chosen_stdout(void)
return 0;
}
- for (match = __earlycon_table; match < __earlycon_table_end; match++) {
+ for (p_match = __earlycon_table; p_match < __earlycon_table_end;
+ p_match++) {
+ const struct earlycon_id *match = *p_match;
+
if (!match->compatible[0])
continue;
diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
index a24278380fec..22683393a0f2 100644
--- a/drivers/tty/serial/earlycon.c
+++ b/drivers/tty/serial/earlycon.c
@@ -169,7 +169,7 @@ static int __init register_earlycon(char *buf, const struct earlycon_id *match)
*/
int __init setup_earlycon(char *buf)
{
- const struct earlycon_id *match;
+ const struct earlycon_id **p_match;
if (!buf || !buf[0])
return -EINVAL;
@@ -177,7 +177,9 @@ int __init setup_earlycon(char *buf)
if (early_con.flags & CON_ENABLED)
return -EALREADY;
- for (match = __earlycon_table; match < __earlycon_table_end; match++) {
+ for (p_match = __earlycon_table; p_match < __earlycon_table_end;
+ p_match++) {
+ const struct earlycon_id *match = *p_match;
size_t len = strlen(match->name);
if (strncmp(buf, match->name, len))
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 1ab0e520d6fc..e17de55c2542 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -179,7 +179,7 @@
#endif
#ifdef CONFIG_SERIAL_EARLYCON
-#define EARLYCON_TABLE() STRUCT_ALIGN(); \
+#define EARLYCON_TABLE() . = ALIGN(8); \
VMLINUX_SYMBOL(__earlycon_table) = .; \
KEEP(*(__earlycon_table)) \
VMLINUX_SYMBOL(__earlycon_table_end) = .;
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index b32df49a3bd5..c4219b9cbb70 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -351,10 +351,10 @@ struct earlycon_id {
char name[16];
char compatible[128];
int (*setup)(struct earlycon_device *, const char *options);
-} __aligned(32);
+};
-extern const struct earlycon_id __earlycon_table[];
-extern const struct earlycon_id __earlycon_table_end[];
+extern const struct earlycon_id *__earlycon_table[];
+extern const struct earlycon_id *__earlycon_table_end[];
#if defined(CONFIG_SERIAL_EARLYCON) && !defined(MODULE)
#define EARLYCON_USED_OR_UNUSED __used
@@ -362,12 +362,19 @@ extern const struct earlycon_id __earlycon_table_end[];
#define EARLYCON_USED_OR_UNUSED __maybe_unused
#endif
-#define OF_EARLYCON_DECLARE(_name, compat, fn) \
- static const struct earlycon_id __UNIQUE_ID(__earlycon_##_name) \
- EARLYCON_USED_OR_UNUSED __section(__earlycon_table) \
+#define _OF_EARLYCON_DECLARE(_name, compat, fn, unique_id) \
+ static const struct earlycon_id unique_id \
+ EARLYCON_USED_OR_UNUSED __initconst \
= { .name = __stringify(_name), \
.compatible = compat, \
- .setup = fn }
+ .setup = fn }; \
+ static const struct earlycon_id EARLYCON_USED_OR_UNUSED \
+ __section(__earlycon_table) \
+ * const __PASTE(__p, unique_id) = &unique_id
+
+#define OF_EARLYCON_DECLARE(_name, compat, fn) \
+ _OF_EARLYCON_DECLARE(_name, compat, fn, \
+ __UNIQUE_ID(__earlycon_##_name))
#define EARLYCON_DECLARE(_name, fn) OF_EARLYCON_DECLARE(_name, "", fn)
--
2.16.2.804.g6dcf76e118-goog
^ permalink raw reply related
* [PATCH v4] earlycon: Use a pointer table to fix __earlycon_table stride
From: Daniel Kurtz @ 2018-03-20 22:04 UTC (permalink / raw)
To: Greg Kroah-Hartman, Guenter Roeck
Cc: adurbin, linux-kernel, Daniel Kurtz, Rob Herring, Frank Rowand,
Jiri Slaby, Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE,
open list:SERIAL DRIVERS,
open list:GENERIC INCLUDE/ASM HEADER FILES
Commit 99492c39f39f ("earlycon: Fix __earlycon_table stride") tried to fix
__earlycon_table stride by forcing the earlycon_id struct alignment to 32
and asking the linker to 32-byte align the __earlycon_table symbol. This
fix was based on commit 07fca0e57fca92 ("tracing: Properly align linker
defined symbols") which tried a similar fix for the tracing subsystem.
However, this fix doesn't quite work because there is no guarantee that
gcc will place structures packed into an array format. In fact, gcc 4.9
chooses to 64-byte align these structs by inserting additional padding
between the entries because it has no clue that they are supposed to be in
an array. If we are unlucky, the linker will assign symbol
"__earlycon_table" to a 32-byte aligned address which does not correspond
to the 64-byte aligned contents of section "__earlycon_table".
To address this same problem, the fix to the tracing system was
subsequently re-implemented using a more robust table of pointers approach
by commits:
3d56e331b653 ("tracing: Replace syscall_meta_data struct array with pointer array")
654986462939 ("tracepoints: Fix section alignment using pointer array")
e4a9ea5ee7c8 ("tracing: Replace trace_event struct array with pointer array")
Let's use this same "array of pointers to structs" approach for
EARLYCON_TABLE.
Change-Id: Ic42c4db0c8b034fa6aa2bf02eef0fdc159478ac4
Fixes: 99492c39f39f ("earlycon: Fix __earlycon_table stride")
Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
Suggested-by: Aaron Durbin <adurbin@chromium.org>
---
drivers/of/fdt.c | 7 +++++--
drivers/tty/serial/earlycon.c | 6 ++++--
include/asm-generic/vmlinux.lds.h | 2 +-
include/linux/serial_core.h | 21 ++++++++++++++-------
4 files changed, 24 insertions(+), 12 deletions(-)
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 84aa9d676375..6da20b9688f7 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -942,7 +942,7 @@ int __init early_init_dt_scan_chosen_stdout(void)
int offset;
const char *p, *q, *options = NULL;
int l;
- const struct earlycon_id *match;
+ const struct earlycon_id **p_match;
const void *fdt = initial_boot_params;
offset = fdt_path_offset(fdt, "/chosen");
@@ -969,7 +969,10 @@ int __init early_init_dt_scan_chosen_stdout(void)
return 0;
}
- for (match = __earlycon_table; match < __earlycon_table_end; match++) {
+ for (p_match = __earlycon_table; p_match < __earlycon_table_end;
+ p_match++) {
+ const struct earlycon_id *match = *p_match;
+
if (!match->compatible[0])
continue;
diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
index a24278380fec..22683393a0f2 100644
--- a/drivers/tty/serial/earlycon.c
+++ b/drivers/tty/serial/earlycon.c
@@ -169,7 +169,7 @@ static int __init register_earlycon(char *buf, const struct earlycon_id *match)
*/
int __init setup_earlycon(char *buf)
{
- const struct earlycon_id *match;
+ const struct earlycon_id **p_match;
if (!buf || !buf[0])
return -EINVAL;
@@ -177,7 +177,9 @@ int __init setup_earlycon(char *buf)
if (early_con.flags & CON_ENABLED)
return -EALREADY;
- for (match = __earlycon_table; match < __earlycon_table_end; match++) {
+ for (p_match = __earlycon_table; p_match < __earlycon_table_end;
+ p_match++) {
+ const struct earlycon_id *match = *p_match;
size_t len = strlen(match->name);
if (strncmp(buf, match->name, len))
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 1ab0e520d6fc..e17de55c2542 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -179,7 +179,7 @@
#endif
#ifdef CONFIG_SERIAL_EARLYCON
-#define EARLYCON_TABLE() STRUCT_ALIGN(); \
+#define EARLYCON_TABLE() . = ALIGN(8); \
VMLINUX_SYMBOL(__earlycon_table) = .; \
KEEP(*(__earlycon_table)) \
VMLINUX_SYMBOL(__earlycon_table_end) = .;
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index b32df49a3bd5..c4219b9cbb70 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -351,10 +351,10 @@ struct earlycon_id {
char name[16];
char compatible[128];
int (*setup)(struct earlycon_device *, const char *options);
-} __aligned(32);
+};
-extern const struct earlycon_id __earlycon_table[];
-extern const struct earlycon_id __earlycon_table_end[];
+extern const struct earlycon_id *__earlycon_table[];
+extern const struct earlycon_id *__earlycon_table_end[];
#if defined(CONFIG_SERIAL_EARLYCON) && !defined(MODULE)
#define EARLYCON_USED_OR_UNUSED __used
@@ -362,12 +362,19 @@ extern const struct earlycon_id __earlycon_table_end[];
#define EARLYCON_USED_OR_UNUSED __maybe_unused
#endif
-#define OF_EARLYCON_DECLARE(_name, compat, fn) \
- static const struct earlycon_id __UNIQUE_ID(__earlycon_##_name) \
- EARLYCON_USED_OR_UNUSED __section(__earlycon_table) \
+#define _OF_EARLYCON_DECLARE(_name, compat, fn, unique_id) \
+ static const struct earlycon_id unique_id \
+ EARLYCON_USED_OR_UNUSED __initconst \
= { .name = __stringify(_name), \
.compatible = compat, \
- .setup = fn }
+ .setup = fn }; \
+ static const struct earlycon_id EARLYCON_USED_OR_UNUSED \
+ __section(__earlycon_table) \
+ * const __PASTE(__p, unique_id) = &unique_id
+
+#define OF_EARLYCON_DECLARE(_name, compat, fn) \
+ _OF_EARLYCON_DECLARE(_name, compat, fn, \
+ __UNIQUE_ID(__earlycon_##_name))
#define EARLYCON_DECLARE(_name, fn) OF_EARLYCON_DECLARE(_name, "", fn)
--
2.16.2.804.g6dcf76e118-goog
^ permalink raw reply related
* [PATCH v3] earlycon: Use a pointer table to fix __earlycon_table stride
From: Daniel Kurtz @ 2018-03-20 22:02 UTC (permalink / raw)
To: Greg Kroah-Hartman, Guenter Roeck
Cc: adurbin, linux-kernel, Daniel Kurtz, Rob Herring, Frank Rowand,
Jiri Slaby, Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE,
open list:SERIAL DRIVERS,
open list:GENERIC INCLUDE/ASM HEADER FILES
Commit 99492c39f39f ("earlycon: Fix __earlycon_table stride") tried to fix
__earlycon_table stride by forcing the earlycon_id struct alignment to 32
and asking the linker to 32-byte align the __earlycon_table symbol. This
fix was based on commit 07fca0e57fca92 ("tracing: Properly align linker
defined symbols") which tried a similar fix for the tracing subsystem.
However, this fix doesn't quite work because there is no guarantee that
gcc will place structures packed into an array format. In fact, gcc 4.9
chooses to 64-byte align these structs by inserting additional padding
between the entries because it has no clue that they are supposed to be in
an array. If we are unlucky, the linker will assign symbol
"__earlycon_table" to a 32-byte aligned address which does not correpsond
to the 64-byte alignbed contents of section "__earlycon_table".
To address this same problem, the fix to the tracing system was
subsequently re-implemented using a more robust table of pointers approach
by commits:
3d56e331b653 ("tracing: Replace syscall_meta_data struct array with pointer array")
654986462939 ("tracepoints: Fix section alignment using pointer array")
e4a9ea5ee7c8 ("tracing: Replace trace_event struct array with pointer array")
Let's use this same "array of pointers to structs" approach for
EARLYCON_TABLE.
Fixes: 99492c39f39f ("earlycon: Fix __earlycon_table stride")
Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
Suggested-by: Aaron Durbin <adurbin@chromium.org>
---
Changes since v2:
* Use __initconst instead of __initdata to avoid h8300 and alpha kbuild errors
drivers/of/fdt.c | 7 +++++--
drivers/tty/serial/earlycon.c | 6 ++++--
include/asm-generic/vmlinux.lds.h | 2 +-
include/linux/serial_core.h | 21 ++++++++++++++-------
4 files changed, 24 insertions(+), 12 deletions(-)
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 84aa9d676375..6da20b9688f7 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -942,7 +942,7 @@ int __init early_init_dt_scan_chosen_stdout(void)
int offset;
const char *p, *q, *options = NULL;
int l;
- const struct earlycon_id *match;
+ const struct earlycon_id **p_match;
const void *fdt = initial_boot_params;
offset = fdt_path_offset(fdt, "/chosen");
@@ -969,7 +969,10 @@ int __init early_init_dt_scan_chosen_stdout(void)
return 0;
}
- for (match = __earlycon_table; match < __earlycon_table_end; match++) {
+ for (p_match = __earlycon_table; p_match < __earlycon_table_end;
+ p_match++) {
+ const struct earlycon_id *match = *p_match;
+
if (!match->compatible[0])
continue;
diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
index a24278380fec..22683393a0f2 100644
--- a/drivers/tty/serial/earlycon.c
+++ b/drivers/tty/serial/earlycon.c
@@ -169,7 +169,7 @@ static int __init register_earlycon(char *buf, const struct earlycon_id *match)
*/
int __init setup_earlycon(char *buf)
{
- const struct earlycon_id *match;
+ const struct earlycon_id **p_match;
if (!buf || !buf[0])
return -EINVAL;
@@ -177,7 +177,9 @@ int __init setup_earlycon(char *buf)
if (early_con.flags & CON_ENABLED)
return -EALREADY;
- for (match = __earlycon_table; match < __earlycon_table_end; match++) {
+ for (p_match = __earlycon_table; p_match < __earlycon_table_end;
+ p_match++) {
+ const struct earlycon_id *match = *p_match;
size_t len = strlen(match->name);
if (strncmp(buf, match->name, len))
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 1ab0e520d6fc..e17de55c2542 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -179,7 +179,7 @@
#endif
#ifdef CONFIG_SERIAL_EARLYCON
-#define EARLYCON_TABLE() STRUCT_ALIGN(); \
+#define EARLYCON_TABLE() . = ALIGN(8); \
VMLINUX_SYMBOL(__earlycon_table) = .; \
KEEP(*(__earlycon_table)) \
VMLINUX_SYMBOL(__earlycon_table_end) = .;
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index b32df49a3bd5..c4219b9cbb70 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -351,10 +351,10 @@ struct earlycon_id {
char name[16];
char compatible[128];
int (*setup)(struct earlycon_device *, const char *options);
-} __aligned(32);
+};
-extern const struct earlycon_id __earlycon_table[];
-extern const struct earlycon_id __earlycon_table_end[];
+extern const struct earlycon_id *__earlycon_table[];
+extern const struct earlycon_id *__earlycon_table_end[];
#if defined(CONFIG_SERIAL_EARLYCON) && !defined(MODULE)
#define EARLYCON_USED_OR_UNUSED __used
@@ -362,12 +362,19 @@ extern const struct earlycon_id __earlycon_table_end[];
#define EARLYCON_USED_OR_UNUSED __maybe_unused
#endif
-#define OF_EARLYCON_DECLARE(_name, compat, fn) \
- static const struct earlycon_id __UNIQUE_ID(__earlycon_##_name) \
- EARLYCON_USED_OR_UNUSED __section(__earlycon_table) \
+#define _OF_EARLYCON_DECLARE(_name, compat, fn, unique_id) \
+ static const struct earlycon_id unique_id \
+ EARLYCON_USED_OR_UNUSED __initconst \
= { .name = __stringify(_name), \
.compatible = compat, \
- .setup = fn }
+ .setup = fn }; \
+ static const struct earlycon_id EARLYCON_USED_OR_UNUSED \
+ __section(__earlycon_table) \
+ * const __PASTE(__p, unique_id) = &unique_id
+
+#define OF_EARLYCON_DECLARE(_name, compat, fn) \
+ _OF_EARLYCON_DECLARE(_name, compat, fn, \
+ __UNIQUE_ID(__earlycon_##_name))
#define EARLYCON_DECLARE(_name, fn) OF_EARLYCON_DECLARE(_name, "", fn)
--
2.16.2.804.g6dcf76e118-goog
^ permalink raw reply related
* Re: [PATCH v2 06/21] fpga: Remove depends on HAS_DMA in case of platform dependency
From: Alan Tull @ 2018-03-20 18:20 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Ulf Hansson, Wolfram Sang, linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-fpga-u79uwXL29TY76Z2rM5mHXA,
linux-remoteproc-u79uwXL29TY76Z2rM5mHXA,
ALSA Development Mailing List, Bjorn Andersson, Eric Anholt,
netdev, MTD Maling List, Linux I2C,
linux1394-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f,
Christoph Hellwig, Stefan Wahren, Boris Brezillon,
James E . J . Bottomley, Herbert Xu, scsi, Richard Weinberger,
Jassi Brar, Marek Vasut, linux-serial-u79uwXL29TY76Z2rM5mHXA,
Matias Bjorling
In-Reply-To: <CAMuHMdU-0VOs6MYrCaCrtRfBqGgaQfox1AgbExNNcYxVC6Uh-Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Tue, Mar 20, 2018 at 5:04 AM, Geert Uytterhoeven
<geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
Hi Geert,
> Hi Alan,
>
> On Mon, Mar 19, 2018 at 5:06 PM, Alan Tull <atull-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>> On Fri, Mar 16, 2018 at 8:51 AM, Geert Uytterhoeven
>> <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
>> This essentially removes this commit
>>
>> commit 1c8cb409491403036919dd1c6b45013dc8835a44
>> Author: Sudip Mukherjee <sudipm.mukherjee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> Date: Wed Aug 3 13:45:46 2016 -0700
>>
>> drivers/fpga/Kconfig: fix build failure
>>
>> While building m32r allmodconfig the build is failing with the error:
>>
>> ERROR: "bad_dma_ops" [drivers/fpga/zynq-fpga.ko] undefined!
>>
>> Xilinx Zynq FPGA is using DMA but there was no dependency while
>> building.
>>
>> Link: http://lkml.kernel.org/r/1464346526-13913-1-git-send-email-sudipm.mukherjee-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
>> Signed-off-by: Sudip Mukherjee <sudip.mukherjee-4yDnlxn2s6sWdaTGBSpHTA@public.gmane.org>
>> Acked-by: Moritz Fischer <moritz.fischer-+aYTwkv1SeIAvxtiuMwx3w@public.gmane.org>
>> Cc: Alan Tull <atull-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx@public.gmane.org>
>> Signed-off-by: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
>> Signed-off-by: Linus Torvalds <torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
>
> Yes it does. The major change is that the first (core) series introduces
> all needed dummies to do successful compile-testing on NO_DMA=y platforms.
OK yes, I looked at the first patch that does the fix. Looks good.
Thanks for doing this.
>
>>> Remove dependencies on HAS_DMA where a Kconfig symbol depends on another
>>> symbol that implies HAS_DMA, and, optionally, on "|| COMPILE_TEST".
>>> In most cases this other symbol is an architecture or platform specific
>>> symbol, or PCI.
>>>
>>> Generic symbols and drivers without platform dependencies keep their
>>> dependencies on HAS_DMA, to prevent compiling subsystems or drivers that
>>> cannot work anyway.
>>>
>>> This simplifies the dependencies, and allows to improve compile-testing.
>>>
>>> Signed-off-by: Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>
>>> Reviewed-by: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>> Acked-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
Acked-by: Alan Tull <atull-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Regards,
Alan
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
^ permalink raw reply
* Re: [PATCH v2 2/2] earlycon: Use a pointer table to fix __earlycon_table stride
From: Guenter Roeck @ 2018-03-20 18:19 UTC (permalink / raw)
To: Daniel Kurtz
Cc: Greg Kroah-Hartman, Matthias Kaehlcke, Guenter Roeck,
Aaron Durbin, linux-kernel, Rob Herring, Frank Rowand, Jiri Slaby,
Arnd Bergmann, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE,
open list:SERIAL DRIVERS,
open list:GENERIC INCLUDE/ASM HEADER FILES
In-Reply-To: <20180320175712.201572-3-djkurtz@chromium.org>
On Tue, Mar 20, 2018 at 10:57 AM, Daniel Kurtz <djkurtz@chromium.org> wrote:
> Commit 99492c39f39f ("earlycon: Fix __earlycon_table stride") tried to fix
> __earlycon_table stride by forcing the earlycon_id struct alignment to 32
> and asking the linker to 32-byte align the __earlycon_table symbol. This
> fix was based on commit 07fca0e57fca92 ("tracing: Properly align linker
> defined symbols") which tried a similar fix for the tracing subsystem.
>
> However, this fix doesn't quite work because there is no guarantee that
> gcc will place structures packed into an array format. In fact, gcc 4.9
> chooses to 64-byte align these structs by inserting additional padding
> between the entries because it has no clue that they are supposed to be in
> an array. If we are unlucky, the linker will assign symbol
> "__earlycon_table" to a 32-byte aligned address which does not correpsond
correspond
> to the 64-byte alignbed contents of section "__earlycon_table".
aligned
>
> To address this same problem, the fix to the tracing system was
> subsequently re-implemented using a more robust table of pointers approach
> by commits:
> 3d56e331b653 ("tracing: Replace syscall_meta_data struct array with pointer array")
> 654986462939 ("tracepoints: Fix section alignment using pointer array")
> e4a9ea5ee7c8 ("tracing: Replace trace_event struct array with pointer array")
>
> Let's use this same "array of pointers to structs" approach for
> EARLYCON_TABLE.
>
> Fixes: 99492c39f39f ("earlycon: Fix __earlycon_table stride")
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> Suggested-by: Aaron Durbin <adurbin@chromium.org>
> ---
> Changes since v1:
> * added Suggested-by and Fixes, and reworded the commit message per Randy.
>
> drivers/of/fdt.c | 7 +++++--
> drivers/tty/serial/earlycon.c | 6 ++++--
> include/asm-generic/vmlinux.lds.h | 2 +-
> include/linux/serial_core.h | 21 ++++++++++++++-------
> 4 files changed, 24 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 84aa9d676375..6da20b9688f7 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -942,7 +942,7 @@ int __init early_init_dt_scan_chosen_stdout(void)
> int offset;
> const char *p, *q, *options = NULL;
> int l;
> - const struct earlycon_id *match;
> + const struct earlycon_id **p_match;
> const void *fdt = initial_boot_params;
>
> offset = fdt_path_offset(fdt, "/chosen");
> @@ -969,7 +969,10 @@ int __init early_init_dt_scan_chosen_stdout(void)
> return 0;
> }
>
> - for (match = __earlycon_table; match < __earlycon_table_end; match++) {
> + for (p_match = __earlycon_table; p_match < __earlycon_table_end;
> + p_match++) {
> + const struct earlycon_id *match = *p_match;
> +
> if (!match->compatible[0])
> continue;
>
> diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
> index a24278380fec..22683393a0f2 100644
> --- a/drivers/tty/serial/earlycon.c
> +++ b/drivers/tty/serial/earlycon.c
> @@ -169,7 +169,7 @@ static int __init register_earlycon(char *buf, const struct earlycon_id *match)
> */
> int __init setup_earlycon(char *buf)
> {
> - const struct earlycon_id *match;
> + const struct earlycon_id **p_match;
>
> if (!buf || !buf[0])
> return -EINVAL;
> @@ -177,7 +177,9 @@ int __init setup_earlycon(char *buf)
> if (early_con.flags & CON_ENABLED)
> return -EALREADY;
>
> - for (match = __earlycon_table; match < __earlycon_table_end; match++) {
> + for (p_match = __earlycon_table; p_match < __earlycon_table_end;
> + p_match++) {
> + const struct earlycon_id *match = *p_match;
> size_t len = strlen(match->name);
>
> if (strncmp(buf, match->name, len))
> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index 1ab0e520d6fc..e17de55c2542 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -179,7 +179,7 @@
> #endif
>
> #ifdef CONFIG_SERIAL_EARLYCON
> -#define EARLYCON_TABLE() STRUCT_ALIGN(); \
> +#define EARLYCON_TABLE() . = ALIGN(8); \
> VMLINUX_SYMBOL(__earlycon_table) = .; \
> KEEP(*(__earlycon_table)) \
> VMLINUX_SYMBOL(__earlycon_table_end) = .;
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index b32df49a3bd5..93b7add47087 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -351,10 +351,10 @@ struct earlycon_id {
> char name[16];
> char compatible[128];
> int (*setup)(struct earlycon_device *, const char *options);
> -} __aligned(32);
> +};
>
> -extern const struct earlycon_id __earlycon_table[];
> -extern const struct earlycon_id __earlycon_table_end[];
> +extern const struct earlycon_id *__earlycon_table[];
> +extern const struct earlycon_id *__earlycon_table_end[];
>
> #if defined(CONFIG_SERIAL_EARLYCON) && !defined(MODULE)
> #define EARLYCON_USED_OR_UNUSED __used
> @@ -362,12 +362,19 @@ extern const struct earlycon_id __earlycon_table_end[];
> #define EARLYCON_USED_OR_UNUSED __maybe_unused
> #endif
>
> -#define OF_EARLYCON_DECLARE(_name, compat, fn) \
> - static const struct earlycon_id __UNIQUE_ID(__earlycon_##_name) \
> - EARLYCON_USED_OR_UNUSED __section(__earlycon_table) \
> +#define _OF_EARLYCON_DECLARE(_name, compat, fn, unique_id) \
> + static const struct earlycon_id unique_id \
> + EARLYCON_USED_OR_UNUSED __initdata \
> = { .name = __stringify(_name), \
> .compatible = compat, \
> - .setup = fn }
> + .setup = fn }; \
> + static const struct earlycon_id EARLYCON_USED_OR_UNUSED \
> + __section(__earlycon_table) \
> + * const __PASTE(__p, unique_id) = &unique_id
> +
> +#define OF_EARLYCON_DECLARE(_name, compat, fn) \
> + _OF_EARLYCON_DECLARE(_name, compat, fn, \
> + __UNIQUE_ID(__earlycon_##_name))
>
> #define EARLYCON_DECLARE(_name, fn) OF_EARLYCON_DECLARE(_name, "", fn)
>
> --
> 2.16.2.804.g6dcf76e118-goog
>
^ permalink raw reply
* [PATCH v2 2/2] earlycon: Use a pointer table to fix __earlycon_table stride
From: Daniel Kurtz @ 2018-03-20 17:57 UTC (permalink / raw)
To: Greg Kroah-Hartman, Matthias Kaehlcke, Guenter Roeck
Cc: adurbin, linux-kernel, Daniel Kurtz, Rob Herring, Frank Rowand,
Jiri Slaby, Arnd Bergmann,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE,
open list:SERIAL DRIVERS,
open list:GENERIC INCLUDE/ASM HEADER FILES
In-Reply-To: <20180320175712.201572-1-djkurtz@chromium.org>
Commit 99492c39f39f ("earlycon: Fix __earlycon_table stride") tried to fix
__earlycon_table stride by forcing the earlycon_id struct alignment to 32
and asking the linker to 32-byte align the __earlycon_table symbol. This
fix was based on commit 07fca0e57fca92 ("tracing: Properly align linker
defined symbols") which tried a similar fix for the tracing subsystem.
However, this fix doesn't quite work because there is no guarantee that
gcc will place structures packed into an array format. In fact, gcc 4.9
chooses to 64-byte align these structs by inserting additional padding
between the entries because it has no clue that they are supposed to be in
an array. If we are unlucky, the linker will assign symbol
"__earlycon_table" to a 32-byte aligned address which does not correpsond
to the 64-byte alignbed contents of section "__earlycon_table".
To address this same problem, the fix to the tracing system was
subsequently re-implemented using a more robust table of pointers approach
by commits:
3d56e331b653 ("tracing: Replace syscall_meta_data struct array with pointer array")
654986462939 ("tracepoints: Fix section alignment using pointer array")
e4a9ea5ee7c8 ("tracing: Replace trace_event struct array with pointer array")
Let's use this same "array of pointers to structs" approach for
EARLYCON_TABLE.
Fixes: 99492c39f39f ("earlycon: Fix __earlycon_table stride")
Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
Suggested-by: Aaron Durbin <adurbin@chromium.org>
---
Changes since v1:
* added Suggested-by and Fixes, and reworded the commit message per Randy.
drivers/of/fdt.c | 7 +++++--
drivers/tty/serial/earlycon.c | 6 ++++--
include/asm-generic/vmlinux.lds.h | 2 +-
include/linux/serial_core.h | 21 ++++++++++++++-------
4 files changed, 24 insertions(+), 12 deletions(-)
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 84aa9d676375..6da20b9688f7 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -942,7 +942,7 @@ int __init early_init_dt_scan_chosen_stdout(void)
int offset;
const char *p, *q, *options = NULL;
int l;
- const struct earlycon_id *match;
+ const struct earlycon_id **p_match;
const void *fdt = initial_boot_params;
offset = fdt_path_offset(fdt, "/chosen");
@@ -969,7 +969,10 @@ int __init early_init_dt_scan_chosen_stdout(void)
return 0;
}
- for (match = __earlycon_table; match < __earlycon_table_end; match++) {
+ for (p_match = __earlycon_table; p_match < __earlycon_table_end;
+ p_match++) {
+ const struct earlycon_id *match = *p_match;
+
if (!match->compatible[0])
continue;
diff --git a/drivers/tty/serial/earlycon.c b/drivers/tty/serial/earlycon.c
index a24278380fec..22683393a0f2 100644
--- a/drivers/tty/serial/earlycon.c
+++ b/drivers/tty/serial/earlycon.c
@@ -169,7 +169,7 @@ static int __init register_earlycon(char *buf, const struct earlycon_id *match)
*/
int __init setup_earlycon(char *buf)
{
- const struct earlycon_id *match;
+ const struct earlycon_id **p_match;
if (!buf || !buf[0])
return -EINVAL;
@@ -177,7 +177,9 @@ int __init setup_earlycon(char *buf)
if (early_con.flags & CON_ENABLED)
return -EALREADY;
- for (match = __earlycon_table; match < __earlycon_table_end; match++) {
+ for (p_match = __earlycon_table; p_match < __earlycon_table_end;
+ p_match++) {
+ const struct earlycon_id *match = *p_match;
size_t len = strlen(match->name);
if (strncmp(buf, match->name, len))
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 1ab0e520d6fc..e17de55c2542 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -179,7 +179,7 @@
#endif
#ifdef CONFIG_SERIAL_EARLYCON
-#define EARLYCON_TABLE() STRUCT_ALIGN(); \
+#define EARLYCON_TABLE() . = ALIGN(8); \
VMLINUX_SYMBOL(__earlycon_table) = .; \
KEEP(*(__earlycon_table)) \
VMLINUX_SYMBOL(__earlycon_table_end) = .;
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index b32df49a3bd5..93b7add47087 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -351,10 +351,10 @@ struct earlycon_id {
char name[16];
char compatible[128];
int (*setup)(struct earlycon_device *, const char *options);
-} __aligned(32);
+};
-extern const struct earlycon_id __earlycon_table[];
-extern const struct earlycon_id __earlycon_table_end[];
+extern const struct earlycon_id *__earlycon_table[];
+extern const struct earlycon_id *__earlycon_table_end[];
#if defined(CONFIG_SERIAL_EARLYCON) && !defined(MODULE)
#define EARLYCON_USED_OR_UNUSED __used
@@ -362,12 +362,19 @@ extern const struct earlycon_id __earlycon_table_end[];
#define EARLYCON_USED_OR_UNUSED __maybe_unused
#endif
-#define OF_EARLYCON_DECLARE(_name, compat, fn) \
- static const struct earlycon_id __UNIQUE_ID(__earlycon_##_name) \
- EARLYCON_USED_OR_UNUSED __section(__earlycon_table) \
+#define _OF_EARLYCON_DECLARE(_name, compat, fn, unique_id) \
+ static const struct earlycon_id unique_id \
+ EARLYCON_USED_OR_UNUSED __initdata \
= { .name = __stringify(_name), \
.compatible = compat, \
- .setup = fn }
+ .setup = fn }; \
+ static const struct earlycon_id EARLYCON_USED_OR_UNUSED \
+ __section(__earlycon_table) \
+ * const __PASTE(__p, unique_id) = &unique_id
+
+#define OF_EARLYCON_DECLARE(_name, compat, fn) \
+ _OF_EARLYCON_DECLARE(_name, compat, fn, \
+ __UNIQUE_ID(__earlycon_##_name))
#define EARLYCON_DECLARE(_name, fn) OF_EARLYCON_DECLARE(_name, "", fn)
--
2.16.2.804.g6dcf76e118-goog
^ permalink raw reply related
* [PATCH v2 1/2] serial: sh-sci: Remove __initdata attribute for struct 'port_cfg'
From: Daniel Kurtz @ 2018-03-20 17:57 UTC (permalink / raw)
To: Greg Kroah-Hartman, Matthias Kaehlcke, Guenter Roeck
Cc: adurbin, linux-kernel, Daniel Kurtz, Jiri Slaby,
open list:SERIAL DRIVERS
In-Reply-To: <20180320175712.201572-1-djkurtz@chromium.org>
Commit dd076cffb8cd ("serial: sh-sci: Fix init data attribute for struct
'port_cfg'") properly removed the __init attribute, and changed it to
__initdata.
The __init function early_console_setup() takes the address of global
port_cfg and assigns it to a field in another global, sci_ports:
static int __init early_console_setup(struct earlycon_device *device,
int type)
{
...
port_cfg.type = type;
sci_ports[0].cfg = &port_cfg;
...
}
port_cfg, however, is now in __initdata:
static struct plat_sci_port port_cfg __initdata;
... but sci_ports is not:
static struct sci_port sci_ports[SCI_NPORTS];
Thus, there is a non-__initdata variable containing the address of a
__initdata struct.
Fix this section type conflict by just removing the __initdata attribute.
Fixes: dd076cffb8cd ("serial: sh-sci: Fix init data attribute for struct 'port_cfg'")
Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
drivers/tty/serial/sh-sci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index 44adf9db38f8..ff4e1012ed76 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -3277,7 +3277,7 @@ early_platform_init_buffer("earlyprintk", &sci_driver,
early_serial_buf, ARRAY_SIZE(early_serial_buf));
#endif
#ifdef CONFIG_SERIAL_SH_SCI_EARLYCON
-static struct plat_sci_port port_cfg __initdata;
+static struct plat_sci_port port_cfg;
static int __init early_console_setup(struct earlycon_device *device,
int type)
--
2.16.2.804.g6dcf76e118-goog
^ permalink raw reply related
* RE: [PATCH v3] serial: mvebu-uart: fix tx lost characters
From: Gabriel Matni @ 2018-03-20 16:09 UTC (permalink / raw)
To: gregkh@linuxfoundation.org
Cc: Miquel Raynal, linux-serial@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, stable@vger.kernel.org,
Grégory Clement, Thomas Petazzoni
From: Gabriel Matni <gabriel.matni@exfo.com>
Fixes missing characters on kernel console at low baud rates (i.e.9600).
The driver should poll TX_RDY or TX_FIFO_EMP instead of TX_EMP to ensure
that the transmitter holding register (THR) is ready to receive a new byte.
TX_EMP tells us when it is possible to send a break sequence via
SND_BRK_SEQ. While this also indicates that both the THR and the TSR are
empty, it does not guarantee that a new byte can be written just yet.
Fixes: 30530791a7a0 ("serial: mvebu-uart: initial support for Armada-3700 serial port")
Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
Acked-by: Gregory CLEMENT <gregory.clement@bootlin.com>
Signed-off-by: Gabriel Matni <gabriel.matni@exfo.com>
---
Changes since v2:
- use one line for the "Fixes" entry
- removed trailing space between Signed-off-by entry and ---
- start using versioning, previous fixes in v1
Changes since v1:
- patch was corrupt, could not be applied
- fixed line indent
---
drivers/tty/serial/mvebu-uart.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/mvebu-uart.c b/drivers/tty/serial/mvebu-uart.c
index a100e98259d7..f0df0640208e 100644
--- a/drivers/tty/serial/mvebu-uart.c
+++ b/drivers/tty/serial/mvebu-uart.c
@@ -618,7 +618,7 @@ static void wait_for_xmitr(struct uart_port *port)
u32 val;
readl_poll_timeout_atomic(port->membase + UART_STAT, val,
- (val & STAT_TX_EMP), 1, 10000);
+ (val & STAT_TX_RDY(port)), 1, 10000);
}
static void mvebu_uart_console_putchar(struct uart_port *port, int ch)
--
2.7.4
> -----Original Message-----
> From: gregkh@linuxfoundation.org <gregkh@linuxfoundation.org>
> Sent: March 20, 2018 5:32 AM
> To: Gabriel Matni <gabriel.matni@exfo.com>
> Cc: Miquel Raynal <miquel.raynal@bootlin.com>; linux-
> serial@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> stable@vger.kernel.org; Grégory Clement <gregory.clement@bootlin.com>;
> Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> Subject: Re: [PATCH] serial: mvebu-uart: fix tx lost characters
>
> On Fri, Mar 16, 2018 at 01:45:32PM +0000, Gabriel Matni wrote:
> > From: Gabriel Matni <gabriel.matni@exfo.com>
> >
> > Fixes missing characters on kernel console at low baud rates (i.e.9600).
> > The driver should poll TX_RDY or TX_FIFO_EMP instead of TX_EMP to
> ensure
> > that the transmitter holding register (THR) is ready to receive a new byte.
> >
> > TX_EMP tells us when it is possible to send a break sequence via
> > SND_BRK_SEQ. While this also indicates that both the THR and the TSR are
> > empty, it does not guarantee that a new byte can be written just yet.
> >
> > Fixes: 30530791a7a0 ("serial: mvebu-uart: initial support for Armada-3700
> > serial port")
>
> Can all be on one line.
>
> And should this go to the stable trees?
>
> > Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > Acked-by: Gregory CLEMENT <gregory.clement@bootlin.com>
> > Signed-off-by: Gabriel Matni <gabriel.matni@exfo.com>
>
> Trailing whitespace?
>
> >
> > ---
> > drivers/tty/serial/mvebu-uart.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
>
> What version of this patch is this? How do I know which to accept?
>
> Please properly version your patch, and include the changes below the
> --- line like the documentation says to do.
>
> thanks,
>
> greg k-h
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox