* Re: [PATCH 1/2] acpi, spcr: Make SPCR avialable to other architectures
From: Prarit Bhargava @ 2017-12-08 19:42 UTC (permalink / raw)
To: Timur Tabi
Cc: ACPI Devel Maling List, Jonathan Corbet, the arch/x86 maintainers,
linux-pm, Catalin Marinas, Bhupesh Sharma, linux-doc, Will Deacon,
lkml, Ingo Molnar, Rafael J. Wysocki, Lv Zheng, linux-serial,
H. Peter Anvin, Thomas Gleixner,
linux-arm-kernel@lists.infradead.org, Jeffrey Hugo
In-Reply-To: <CAOZdJXV4E6gu=Ed6nOCcEp2afe_g8Ot-H5oO5=6+ye51cv9hUg@mail.gmail.com>
On 12/07/2017 01:43 PM, Timur Tabi wrote:
> On Thu, Dec 7, 2017 at 11:29 AM, Prarit Bhargava <prarit@redhat.com> wrote:
>> Other architectures can use SPCR to setup an early console or console but
>> the current code is ARM64 specific.
>>
>> Change the name of parse_spcr() to acpi_parse_spcr(). Add a weak
>> function acpi_arch_setup_console() that can be used for arch-specific
>> setup. Move SPCR initialization flag into ACPI code. Update the
>> Documention on the use of the SPCR earlycon.
>>
>> This patch does not change arm64 behaviour.
>
> Let's hope so. Our E44 erratum work-around has caused lots of
> problems in the past, so the code is a bit fragile.
So it does look like I broke something :( I'll fix this and do more testing.
One thing that I just learned (relative to x86) is that you have to test much
further and wider than on x86.
P.
>
>> +static bool qdf2400_erratum_44_present(struct acpi_table_header *h)
>> +{
>> + if (memcmp(h->oem_id, "QCOM ", ACPI_OEM_ID_SIZE))
>> + return false;
>> +
>> + if (!memcmp(h->oem_table_id, "QDF2432 ", ACPI_OEM_TABLE_ID_SIZE))
>> + return true;
>> +
>> + if (!memcmp(h->oem_table_id, "QDF2400 ", ACPI_OEM_TABLE_ID_SIZE) &&
>> + h->oem_revision == 1)
>> + return true;
>> +
>> + return false;
>> +}
>
> Please give us a chance to test this patch before merging. We've had a
> lot of problems with our E44 work-around, and we need to make sure
> that qdf2400_erratum_44_present function is called before the pl011
> driver is probed.
>
>> +
>> +/*
>> + * APM X-Gene v1 and v2 UART hardware is an 16550 like device but has its
>> + * register aligned to 32-bit. In addition, the BIOS also encoded the
>> + * access width to be 8 bits. This function detects this errata condition.
>> + */
>> +static bool xgene_8250_erratum_present(struct acpi_table_spcr *tb)
>> +{
>> + bool xgene_8250 = false;
>> +
>> + if (tb->interface_type != ACPI_DBG2_16550_COMPATIBLE)
>> + return false;
>> +
>> + if (memcmp(tb->header.oem_id, "APMC0D", ACPI_OEM_ID_SIZE) &&
>> + memcmp(tb->header.oem_id, "HPE ", ACPI_OEM_ID_SIZE))
>> + return false;
>> +
>> + if (!memcmp(tb->header.oem_table_id, "XGENESPC",
>> + ACPI_OEM_TABLE_ID_SIZE) && tb->header.oem_revision == 0)
>> + xgene_8250 = true;
>> +
>> + if (!memcmp(tb->header.oem_table_id, "ProLiant",
>> + ACPI_OEM_TABLE_ID_SIZE) && tb->header.oem_revision == 1)
>> + xgene_8250 = true;
>> +
>> + return xgene_8250;
>> +}
>
> I suspect that this function has the same issues as
> qdf2400_erratum_44_present().
>
>> +config ACPI_SPCR_TABLE
>> + bool "ACPI Serial Port Console Redirection Support"
>> + default y if ARM64
>> + help
>> + Enable support for Serial Port Console Redirection (SPCR) Table.
>> + This table provides information about the configuration of the
>> + earlycon console.
>> +
>
> So ACPI without SPCR has never been tested by us. Making it optional
> makes me a little nervous. We'll have to evaluate this change.
>
^ permalink raw reply
* Re: Have my PA8800 back online... (serial port missing on v4.14)
From: Helge Deller @ 2017-12-08 19:06 UTC (permalink / raw)
To: Frank Scheiner, linux-parisc, John David Anglin, Andy Shevchenko,
linux-serial
Cc: debian-hppa
In-Reply-To: <53815372-58e8-70e2-bab4-1777e848cf5e@web.de>
Adding the linux-serial mailing list:
* Frank Scheiner <frank.scheiner@web.de>:
> On 12/04/2017 03:54 PM, Helge Deller wrote:
> > Anyway, the *only* problem we have right now is, that the Linux kernel 4.14 doesn't detect all serial ports which were detected in earlier kernels.
> > Thus the kernel will talk to the non-existant serial port at 0xfffffffff4050010 instead of 0xfffffffff4050000.
> >
> > 4.13:
> > [ 28.882849] Serial: 8250/16550 driver, 4 ports, IRQ sharing enabled
> > [ 28.898720] 0000:e0:01.0: ttyS0 at MMIO 0xfffffffff4051000 (irq = 73, base_baud = 115200) is a 16450
> > [ 28.934669] 0000:e0:01.1: ttyS1 at MMIO 0xfffffffff4050000 (irq = 73, base_baud = 115200) is a 16550A
> > [ 28.963031] 0000:e0:01.1: ttyS2 at MMIO 0xfffffffff4050010 (irq = 73, base_baud = 115200) is a 16550A
> > [ 28.984946] 0000:e0:01.1: ttyS3 at MMIO 0xfffffffff4050038 (irq = 73, base_baud = 115200) is a 16550A
>
> >
> > ...but for v4.14.x only the following serial ports are detected:
> > [ 28.671984] Serial: 8250/16550 driver, 4 ports, IRQ sharing enabled
> > [ 28.708902] 0000:e0:01.1: ttyS0 at MMIO 0xfffffffff4050000 (irq = 73, base_baud = 115200) is a 16550A
> > [ 28.731145] 0000:e0:01.1: ttyS1 at MMIO 0xfffffffff4050010 (irq = 73, base_baud = 115200) is a 16550A
> >
> >
> > Maybe reverting this commit brings back the old behavior:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7d8905d064058f4b65057e0101588f362f288bc0
>
> I'm unsure about this commit, it speaks more of avoiding duplicate messages
> for device enabling.
Reverting this commit:
commit 7d8905d064058f4b65057e0101588f362f288bc0
Author: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Date: Mon Jul 24 20:28:32 2017 +0300
serial: 8250_pci: Enable device after we check black list
indeed fixes the problem.
After reverting, the serial port from the Diva card shows up as ttyS0 (as before).
With that patch applied, the serial port from the Diva card gets
ignored and the previous ttyS1 port becomes ttyS0 which then breaks
booting the parisc machine because the kernel expects the serial port on
ttyS1.
I'm not sure what the best way forward is.
Fact is, that the patch above changes the behaviour and serial ports
which were existant before suddenly vanish with kernel 4.14.
This following patch does work, and adds back the Diva serial port on parisc.
Not sure if it's acceptable though.
Helge
diff --git a/drivers/tty/serial/8250/8250_pci.c b/drivers/tty/serial/8250/8250_pci.c
index 0c101a7470b0..61319e968e8c 100644
--- a/drivers/tty/serial/8250/8250_pci.c
+++ b/drivers/tty/serial/8250/8250_pci.c
@@ -3393,6 +3393,10 @@ static int serial_pci_is_class_communication(struct pci_dev *dev)
* (Should we try to make guesses for multiport serial devices
* later?)
*/
+ if (IS_ENABLED(CONFIG_PARISC) &&
+ (dev->class >> 8) == PCI_CLASS_COMMUNICATION_OTHER)
+ return 0;
+
if ((((dev->class >> 8) != PCI_CLASS_COMMUNICATION_SERIAL) &&
((dev->class >> 8) != PCI_CLASS_COMMUNICATION_MODEM)) ||
(dev->class & 0xff) > 6)
^ permalink raw reply related
* [PATCH] serial: max310x: Delete an error message for a failed memory allocation in max310x_probe()
From: SF Markus Elfring @ 2017-12-08 19:00 UTC (permalink / raw)
To: linux-serial, Alexander Shiyan, Greg Kroah-Hartman, Jiri Slaby
Cc: LKML, kernel-janitors
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 8 Dec 2017 19:53:10 +0100
Omit an extra message for a memory allocation failure in this function.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/tty/serial/max310x.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index ecb6513a6505..2aad36f402fd 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -1102,10 +1102,8 @@ static int max310x_probe(struct device *dev, struct max310x_devtype *devtype,
/* Alloc port structure */
s = devm_kzalloc(dev, sizeof(*s) +
sizeof(struct max310x_one) * devtype->nr, GFP_KERNEL);
- if (!s) {
- dev_err(dev, "Error allocating port structure\n");
+ if (!s)
return -ENOMEM;
- }
clk_osc = devm_clk_get(dev, "osc");
clk_xtal = devm_clk_get(dev, "xtal");
--
2.15.1
^ permalink raw reply related
* [PATCH 4/4] serial: pch_uart: Improve a size determination in pch_uart_init_port()
From: SF Markus Elfring @ 2017-12-08 18:12 UTC (permalink / raw)
To: linux-serial, Greg Kroah-Hartman, Jiri Slaby; +Cc: LKML, kernel-janitors
In-Reply-To: <f8220e08-da4c-65d6-ecc7-9405cab6f375@users.sourceforge.net>
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 8 Dec 2017 18:53:21 +0100
Replace the specification of a data structure by a pointer dereference
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/tty/serial/pch_uart.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/pch_uart.c b/drivers/tty/serial/pch_uart.c
index 790a7aae331f..1c30309ef02c 100644
--- a/drivers/tty/serial/pch_uart.c
+++ b/drivers/tty/serial/pch_uart.c
@@ -1747,7 +1747,7 @@ static struct eg20t_port *pch_uart_init_port(struct pci_dev *pdev,
board = &drv_dat[id->driver_data];
port_type = board->port_type;
- priv = kzalloc(sizeof(struct eg20t_port), GFP_KERNEL);
+ priv = kzalloc(sizeof(*priv), GFP_KERNEL);
if (priv == NULL)
goto init_port_alloc_err;
--
2.15.1
^ permalink raw reply related
* [PATCH 3/4] serial: pch_uart: Delete an unnecessary return statement in two functions
From: SF Markus Elfring @ 2017-12-08 18:11 UTC (permalink / raw)
To: linux-serial, Greg Kroah-Hartman, Jiri Slaby; +Cc: LKML, kernel-janitors
In-Reply-To: <f8220e08-da4c-65d6-ecc7-9405cab6f375@users.sourceforge.net>
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 8 Dec 2017 18:48:10 +0100
The script "checkpatch.pl" pointed information out like the following.
WARNING: void function return statements are not generally useful
Thus remove such a statement in the affected functions.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/tty/serial/pch_uart.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/tty/serial/pch_uart.c b/drivers/tty/serial/pch_uart.c
index 53cdf23b83c6..790a7aae331f 100644
--- a/drivers/tty/serial/pch_uart.c
+++ b/drivers/tty/serial/pch_uart.c
@@ -693,8 +693,6 @@ static void pch_free_dma(struct uart_port *port)
priv->rx_buf_virt = NULL;
priv->rx_buf_dma = 0;
}
-
- return;
}
static bool filter(struct dma_chan *chan, void *slave)
@@ -1861,8 +1859,8 @@ static void pch_uart_pci_remove(struct pci_dev *pdev)
pch_uart_exit_port(priv);
pci_disable_device(pdev);
kfree(priv);
- return;
}
+
#ifdef CONFIG_PM
static int pch_uart_pci_suspend(struct pci_dev *pdev, pm_message_t state)
{
--
2.15.1
^ permalink raw reply related
* [PATCH 2/4] serial: pch_uart: Use kcalloc() in dma_handle_tx()
From: SF Markus Elfring @ 2017-12-08 18:10 UTC (permalink / raw)
To: linux-serial, Greg Kroah-Hartman, Jiri Slaby; +Cc: LKML, kernel-janitors
In-Reply-To: <f8220e08-da4c-65d6-ecc7-9405cab6f375@users.sourceforge.net>
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 8 Dec 2017 18:38:15 +0100
* A multiplication for the size determination of a memory allocation
indicated that an array data structure should be processed.
Thus use the corresponding function "kcalloc".
This issue was detected by using the Coccinelle software.
* Replace the specification of a data structure by a pointer dereference
to make the corresponding size determination a bit safer according to
the Linux coding style convention.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/tty/serial/pch_uart.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tty/serial/pch_uart.c b/drivers/tty/serial/pch_uart.c
index 0fc72c491c9b..53cdf23b83c6 100644
--- a/drivers/tty/serial/pch_uart.c
+++ b/drivers/tty/serial/pch_uart.c
@@ -991,7 +991,7 @@ static unsigned int dma_handle_tx(struct eg20t_port *priv)
priv->tx_dma_use = 1;
- priv->sg_tx_p = kzalloc(sizeof(struct scatterlist)*num, GFP_ATOMIC);
+ priv->sg_tx_p = kcalloc(num, sizeof(*priv->sg_tx_p), GFP_ATOMIC);
if (!priv->sg_tx_p)
return 0;
--
2.15.1
^ permalink raw reply related
* [PATCH 1/4] serial: pch_uart: Delete an error message for a failed memory allocation in dma_handle_tx()
From: SF Markus Elfring @ 2017-12-08 18:09 UTC (permalink / raw)
To: linux-serial, Greg Kroah-Hartman, Jiri Slaby; +Cc: LKML, kernel-janitors
In-Reply-To: <f8220e08-da4c-65d6-ecc7-9405cab6f375@users.sourceforge.net>
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 8 Dec 2017 18:28:20 +0100
Omit an extra message for a memory allocation failure in this function.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/tty/serial/pch_uart.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/tty/serial/pch_uart.c b/drivers/tty/serial/pch_uart.c
index 760d5dd0aada..0fc72c491c9b 100644
--- a/drivers/tty/serial/pch_uart.c
+++ b/drivers/tty/serial/pch_uart.c
@@ -992,10 +992,8 @@ static unsigned int dma_handle_tx(struct eg20t_port *priv)
priv->tx_dma_use = 1;
priv->sg_tx_p = kzalloc(sizeof(struct scatterlist)*num, GFP_ATOMIC);
- if (!priv->sg_tx_p) {
- dev_err(priv->port.dev, "%s:kzalloc Failed\n", __func__);
+ if (!priv->sg_tx_p)
return 0;
- }
sg_init_table(priv->sg_tx_p, num); /* Initialize SG table */
sg = priv->sg_tx_p;
--
2.15.1
^ permalink raw reply related
* [PATCH 0/4] tty/serial/pch_uart: Adjustments for four function implementations
From: SF Markus Elfring @ 2017-12-08 18:08 UTC (permalink / raw)
To: linux-serial, Greg Kroah-Hartman, Jiri Slaby; +Cc: LKML, kernel-janitors
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 8 Dec 2017 19:03:45 +0100
A few update suggestions were taken into account
from static source code analysis.
Markus Elfring (4):
Delete an error message for a failed memory allocation in dma_handle_tx()
Use kcalloc() in dma_handle_tx()
Delete an unnecessary return statement in two functions
Improve a size determination in pch_uart_init_port()
drivers/tty/serial/pch_uart.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
--
2.15.1
^ permalink raw reply
* [PATCH] serial: sc16is7xx: Delete an error message for a failed memory allocation in sc16is7xx_probe()
From: SF Markus Elfring @ 2017-12-08 16:42 UTC (permalink / raw)
To: linux-serial, Greg Kroah-Hartman, Jiri Slaby; +Cc: LKML, kernel-janitors
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 8 Dec 2017 17:27:50 +0100
Omit an extra message for a memory allocation failure in this function.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/tty/serial/sc16is7xx.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 65792a3539d0..8b3c35f77e7c 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1156,10 +1156,8 @@ static int sc16is7xx_probe(struct device *dev,
s = devm_kzalloc(dev, sizeof(*s) +
sizeof(struct sc16is7xx_one) * devtype->nr_uart,
GFP_KERNEL);
- if (!s) {
- dev_err(dev, "Error allocating port structure\n");
+ if (!s)
return -ENOMEM;
- }
s->clk = devm_clk_get(dev, NULL);
if (IS_ERR(s->clk)) {
--
2.15.1
^ permalink raw reply related
* Re: [PATCH 0/2] acpi, x86: Add SPCR table support
From: Prarit Bhargava @ 2017-12-08 16:02 UTC (permalink / raw)
To: Jeffrey Hugo, Ingo Molnar
Cc: Jonathan Corbet, x86, linux-pm, Catalin Marinas, Bhupesh Sharma,
linux-doc, Will Deacon, linux-kernel, linux-acpi, Ingo Molnar,
Rafael J. Wysocki, Lv Zheng, linux-serial, H. Peter Anvin,
Thomas Gleixner, linux-arm-kernel
In-Reply-To: <bd727a15-c697-741f-864b-8b48b8e5e272@codeaurora.org>
On 12/08/2017 10:31 AM, Jeffrey Hugo wrote:
> On 12/8/2017 7:29 AM, Prarit Bhargava wrote:
>>
>>
>> On 12/08/2017 01:29 AM, Ingo Molnar wrote:
>>>
>>> * Prarit Bhargava <prarit@redhat.com> wrote:
>>>
>>>> The SPCR (Serial Port Console Redirection) Table provides information
>>>> about the configuration of serial port. This information can be used
>>>> to configure the early console.
>>>
>>> s/about the configuration of serial port
>>> /about the configuration of the serial port
>>>
>>>> SPCR support was added for arm64 and is made available across all arches
>>>> in this patchset. The first patch adds a weak per-arch configuration function
>>>> and moves the SPCR code into ACPI. The second patch adds support to x86.
>>>>
>>>> The existing behaviour on arm64 is maintained. If the SPCR exists the
>>>> earlycon and console are automatically configured.
>>>
>>> s/arm64
>>> /ARM64
>>>
>>> which is easier to read and it's also the prevalent spelling:
>>>
>>> triton:~/tip> for N in $(git grep -ih arm64 arch/arm64/ | sed
>>> 's/[[:punct:]]/ /g'); do echo $N | grep -iw arm64; done | sort | uniq -c
>>> 412 arm64
>>> 1 Arm64
>>> 854 ARM64
>>>
>>>> The existing default behaviour on x86 is also maintained. If no console or
>>>> earlycon parameter is defined and the SPCR exists , the serial port is not
>>>> configured. If the earlycon parameter is used both the early console
>>>> and the console are configured using the data from the SPCR.
>>>
>>> s/exists , the
>>> /exists, the
>>>
>>> But, the logic to not use the SPCR looks confusing to me.
>>>
>>> The SPCR is only present if the user has explicitly configured a serial console
>>> for that machine, either in the firmware, or remotely via IPMI, correct? I.e.
>>> SPCR
>>> will not be spuriously present by default on systems that have a serial console
>>> but the user never expressed any interest for them, right?
>>
>> If I disable "Serial Port Console Debug" in my BIOS I still see the SPCR
>> configured:
>>
>> [root@prarit-lab ~]# dmesg | grep SPCR
>> [ 0.000000] ACPI: SPCR 0x0000000069031000 000050 (v01
>> 00000000 00000000)
>>
>> AFAICT the SPCR is always enabled on some systems.
>
> "Serial Port Console Debug" sounds like DBG2 to me, although I am unsure of the
> specific platform you are referencing.
>
>>
>>>
>>> If so then we should pick up that serial console configuration and activate it,
>>> regardless of any kernel boot options!
>>
>> I'm worried about someone who doesn't want a console on ttyS0 suddenly ending up
>> with one. The SPCR could contain incorrect data, etc.
>>
>> I originally wanted this on by default, but the chances of breaking someone's
>> setup seems significant doesn't it? Maybe I'm being too cautious. Anyone else
>> want to weigh in? I'm not ignoring your idea Ingo, I'm just worried about being
>> yelled at by a user :) because I broke their default console setup.
>>
>
> SPCR is always present on QDF2400 (ARM64 platform). Firmware will automatically
> update the SPCR table to point to the correct console (IE Serial-over-LAN or SOL
> if configured via IPMI). Unless a user specifically chooses to override the
> SPCR via "console=", we expect that a console will be present based on the data
> in SPCR.
Hey Jeffrey -- I think Ingo's & my conversation is x86-specific. I am *NOT*
changing the behaviour on ARM64.
P.
>
^ permalink raw reply
* Re: [PATCH 0/2] acpi, x86: Add SPCR table support
From: Jeffrey Hugo @ 2017-12-08 15:31 UTC (permalink / raw)
To: Prarit Bhargava, Ingo Molnar
Cc: Jonathan Corbet, x86, linux-pm, Catalin Marinas, Bhupesh Sharma,
linux-doc, Will Deacon, linux-kernel, linux-acpi, Ingo Molnar,
Rafael J. Wysocki, Lv Zheng, linux-serial, H. Peter Anvin,
Thomas Gleixner, linux-arm-kernel
In-Reply-To: <f078ada7-1e30-41cc-28dc-077ed2a5986d@redhat.com>
On 12/8/2017 7:29 AM, Prarit Bhargava wrote:
>
>
> On 12/08/2017 01:29 AM, Ingo Molnar wrote:
>>
>> * Prarit Bhargava <prarit@redhat.com> wrote:
>>
>>> The SPCR (Serial Port Console Redirection) Table provides information
>>> about the configuration of serial port. This information can be used
>>> to configure the early console.
>>
>> s/about the configuration of serial port
>> /about the configuration of the serial port
>>
>>> SPCR support was added for arm64 and is made available across all arches
>>> in this patchset. The first patch adds a weak per-arch configuration function
>>> and moves the SPCR code into ACPI. The second patch adds support to x86.
>>>
>>> The existing behaviour on arm64 is maintained. If the SPCR exists the
>>> earlycon and console are automatically configured.
>>
>> s/arm64
>> /ARM64
>>
>> which is easier to read and it's also the prevalent spelling:
>>
>> triton:~/tip> for N in $(git grep -ih arm64 arch/arm64/ | sed 's/[[:punct:]]/ /g'); do echo $N | grep -iw arm64; done | sort | uniq -c
>> 412 arm64
>> 1 Arm64
>> 854 ARM64
>>
>>> The existing default behaviour on x86 is also maintained. If no console or
>>> earlycon parameter is defined and the SPCR exists , the serial port is not
>>> configured. If the earlycon parameter is used both the early console
>>> and the console are configured using the data from the SPCR.
>>
>> s/exists , the
>> /exists, the
>>
>> But, the logic to not use the SPCR looks confusing to me.
>>
>> The SPCR is only present if the user has explicitly configured a serial console
>> for that machine, either in the firmware, or remotely via IPMI, correct? I.e. SPCR
>> will not be spuriously present by default on systems that have a serial console
>> but the user never expressed any interest for them, right?
>
> If I disable "Serial Port Console Debug" in my BIOS I still see the SPCR configured:
>
> [root@prarit-lab ~]# dmesg | grep SPCR
> [ 0.000000] ACPI: SPCR 0x0000000069031000 000050 (v01
> 00000000 00000000)
>
> AFAICT the SPCR is always enabled on some systems.
"Serial Port Console Debug" sounds like DBG2 to me, although I am unsure
of the specific platform you are referencing.
>
>>
>> If so then we should pick up that serial console configuration and activate it,
>> regardless of any kernel boot options!
>
> I'm worried about someone who doesn't want a console on ttyS0 suddenly ending up
> with one. The SPCR could contain incorrect data, etc.
>
> I originally wanted this on by default, but the chances of breaking someone's
> setup seems significant doesn't it? Maybe I'm being too cautious. Anyone else
> want to weigh in? I'm not ignoring your idea Ingo, I'm just worried about being
> yelled at by a user :) because I broke their default console setup.
>
SPCR is always present on QDF2400 (ARM64 platform). Firmware will
automatically update the SPCR table to point to the correct console (IE
Serial-over-LAN or SOL if configured via IPMI). Unless a user
specifically chooses to override the SPCR via "console=", we expect that
a console will be present based on the data in SPCR.
--
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply
* Re: [PATCH v3 05/33] nds32: Exception handling
From: Al Viro @ 2017-12-08 15:05 UTC (permalink / raw)
To: Greentime Hu
Cc: greentime, linux-kernel, arnd, linux-arch, tglx, jason,
marc.zyngier, robh+dt, netdev, deanbo422, devicetree, dhowells,
will.deacon, daniel.lezcano, linux-serial, geert.uytterhoeven,
linus.walleij, mark.rutland, greg, Vincent Chen
In-Reply-To: <8fad3e13c85cd90bd038cac7ead0e97e4438a0e0.1512723245.git.green.hu@gmail.com>
On Fri, Dec 08, 2017 at 05:11:48PM +0800, Greentime Hu wrote:
> diff --git a/arch/nds32/kernel/traps.c b/arch/nds32/kernel/traps.c
> new file mode 100644
> index 0000000..30a275d
> --- /dev/null
> +++ b/arch/nds32/kernel/traps.c
> @@ -0,0 +1,441 @@
> +/*
> + * Copyright (C) 2005-2017 Andes Technology Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/personality.h>
> +#include <linux/kallsyms.h>
> +#include <linux/hardirq.h>
> +#include <linux/kdebug.h>
> +#include <linux/sched/task_stack.h>
> +
> +#include <asm/proc-fns.h>
> +#include <asm/uaccess.h>
The only include of asm/uaccess.h should be in linux/uaccess.h; everything
else should include linux/uaccess.h. The same goes for other patches in
the series.
^ permalink raw reply
* Re: [PATCH 0/2] acpi, x86: Add SPCR table support
From: Prarit Bhargava @ 2017-12-08 14:29 UTC (permalink / raw)
To: Ingo Molnar
Cc: linux-acpi, linux-doc, linux-kernel, linux-arm-kernel, linux-pm,
linux-serial, Bhupesh Sharma, Lv Zheng, Thomas Gleixner,
Ingo Molnar, H. Peter Anvin, x86, Jonathan Corbet,
Catalin Marinas, Will Deacon, Rafael J. Wysocki
In-Reply-To: <20171208062925.64ov23u7oxmdfzyk@gmail.com>
On 12/08/2017 01:29 AM, Ingo Molnar wrote:
>
> * Prarit Bhargava <prarit@redhat.com> wrote:
>
>> The SPCR (Serial Port Console Redirection) Table provides information
>> about the configuration of serial port. This information can be used
>> to configure the early console.
>
> s/about the configuration of serial port
> /about the configuration of the serial port
>
>> SPCR support was added for arm64 and is made available across all arches
>> in this patchset. The first patch adds a weak per-arch configuration function
>> and moves the SPCR code into ACPI. The second patch adds support to x86.
>>
>> The existing behaviour on arm64 is maintained. If the SPCR exists the
>> earlycon and console are automatically configured.
>
> s/arm64
> /ARM64
>
> which is easier to read and it's also the prevalent spelling:
>
> triton:~/tip> for N in $(git grep -ih arm64 arch/arm64/ | sed 's/[[:punct:]]/ /g'); do echo $N | grep -iw arm64; done | sort | uniq -c
> 412 arm64
> 1 Arm64
> 854 ARM64
>
>> The existing default behaviour on x86 is also maintained. If no console or
>> earlycon parameter is defined and the SPCR exists , the serial port is not
>> configured. If the earlycon parameter is used both the early console
>> and the console are configured using the data from the SPCR.
>
> s/exists , the
> /exists, the
>
> But, the logic to not use the SPCR looks confusing to me.
>
> The SPCR is only present if the user has explicitly configured a serial console
> for that machine, either in the firmware, or remotely via IPMI, correct? I.e. SPCR
> will not be spuriously present by default on systems that have a serial console
> but the user never expressed any interest for them, right?
If I disable "Serial Port Console Debug" in my BIOS I still see the SPCR configured:
[root@prarit-lab ~]# dmesg | grep SPCR
[ 0.000000] ACPI: SPCR 0x0000000069031000 000050 (v01
00000000 00000000)
AFAICT the SPCR is always enabled on some systems.
>
> If so then we should pick up that serial console configuration and activate it,
> regardless of any kernel boot options!
I'm worried about someone who doesn't want a console on ttyS0 suddenly ending up
with one. The SPCR could contain incorrect data, etc.
I originally wanted this on by default, but the chances of breaking someone's
setup seems significant doesn't it? Maybe I'm being too cautious. Anyone else
want to weigh in? I'm not ignoring your idea Ingo, I'm just worried about being
yelled at by a user :) because I broke their default console setup.
P.
^ permalink raw reply
* Re: [PATCH v3 23/33] nds32: Generic timers support
From: Linus Walleij @ 2017-12-08 13:43 UTC (permalink / raw)
To: Greentime Hu
Cc: greentime, linux-kernel@vger.kernel.org, Arnd Bergmann,
linux-arch, Thomas Gleixner, Jason Cooper, Marc Zyngier,
Rob Herring, netdev, deanbo422,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Al Viro, David Howells, Will Deacon, Daniel Lezcano, linux-serial,
Geert Uytterhoeven, Mark Rutland, Greg KH, Vincent Chen
In-Reply-To: <9f074c94b56617d99f9c6741176bb2ee9dfc4331.1512723245.git.green.hu@gmail.com>
On Fri, Dec 8, 2017 at 10:12 AM, Greentime Hu <green.hu@gmail.com> wrote:
> From: Greentime Hu <greentime@andestech.com>
>
> This patch adds support for timer.
>
> Signed-off-by: Vincent Chen <vincentc@andestech.com>
> Signed-off-by: Greentime Hu <greentime@andestech.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH v3 04/33] nds32: Kernel booting and initialization
From: Greentime Hu @ 2017-12-08 13:25 UTC (permalink / raw)
To: Philippe Ombredanne
Cc: Greentime, LKML, Arnd Bergmann, linux-arch, Thomas Gleixner,
Jason Cooper, Marc Zyngier, Rob Herring, netdev, Vincent Chen,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Al Viro, David Howells, Will Deacon, Daniel Lezcano, linux-serial,
Geert Uytterhoeven, Linus Walleij,
Mark Rutland <mark.rutlan>
In-Reply-To: <CAOFm3uF5BpxpAAH4eq-OSY1bS2cSGS27NY524JoAMzJ1F4bbrA@mail.gmail.com>
Hi, Philippe:
2017-12-08 21:19 GMT+08:00 Philippe Ombredanne <pombredanne@nexb.com>:
> Dear Greentime,
>
> On Fri, Dec 8, 2017 at 10:11 AM, Greentime Hu <green.hu@gmail.com> wrote:
>> From: Greentime Hu <greentime@andestech.com>
>>
>> This patch includes the kernel startup code. It can get dtb pointer
>> passed from bootloader. It will create a temp mapping by tlb
>> instructions at beginning and goto start_kernel.
>>
>> Signed-off-by: Vincent Chen <vincentc@andestech.com>
>> Signed-off-by: Greentime Hu <greentime@andestech.com>
> []
>> --- /dev/null
>> +++ b/arch/nds32/kernel/head.S
>> @@ -0,0 +1,202 @@
>> +/*
>> + * Copyright (C) 2005-2017 Andes Technology Corporation
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
>> + */
>
>
> Have you considered using the new SPDX ids instead of this fine legalese?
> e.g.:
>
> // SPDX-License-Identifier: GPL-2.0
> // Copyright (C) 2005-2017 Andes Technology Corporation
>
> This is much shorter and neater (unless you are a legalese lover of course!)
>
> Check also Thomas doc patches and Linus comments on why he prefers the
> C++ comment style for these.
Thanks for your suggestions. We'd like to do this change.
I will apply it in the next version patch.
^ permalink raw reply
* Re: [PATCH v3 04/33] nds32: Kernel booting and initialization
From: Philippe Ombredanne @ 2017-12-08 13:19 UTC (permalink / raw)
To: Greentime Hu
Cc: greentime, LKML, Arnd Bergmann, linux-arch, Thomas Gleixner,
jason, marc.zyngier, Rob Herring, netdev, deanbo422,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, viro,
dhowells, Will Deacon, daniel.lezcano, linux-serial,
geert.uytterhoeven, linus.walleij, Mark Rutland, greg,
Vincent Chen
In-Reply-To: <7131f15434d17eb64a83999bff9e96a7e9d4b5f5.1512723245.git.green.hu@gmail.com>
Dear Greentime,
On Fri, Dec 8, 2017 at 10:11 AM, Greentime Hu <green.hu@gmail.com> wrote:
> From: Greentime Hu <greentime@andestech.com>
>
> This patch includes the kernel startup code. It can get dtb pointer
> passed from bootloader. It will create a temp mapping by tlb
> instructions at beginning and goto start_kernel.
>
> Signed-off-by: Vincent Chen <vincentc@andestech.com>
> Signed-off-by: Greentime Hu <greentime@andestech.com>
[]
> --- /dev/null
> +++ b/arch/nds32/kernel/head.S
> @@ -0,0 +1,202 @@
> +/*
> + * Copyright (C) 2005-2017 Andes Technology Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
Have you considered using the new SPDX ids instead of this fine legalese?
e.g.:
// SPDX-License-Identifier: GPL-2.0
// Copyright (C) 2005-2017 Andes Technology Corporation
This is much shorter and neater (unless you are a legalese lover of course!)
Check also Thomas doc patches and Linus comments on why he prefers the
C++ comment style for these.
--
Cordially
Philippe Ombredanne, your friendly licensing scruffy bot
^ permalink raw reply
* Re: [PATCH v3 17/33] nds32: VDSO support
From: Greentime Hu @ 2017-12-08 12:46 UTC (permalink / raw)
To: Marc Zyngier
Cc: Mark Rutland, Greentime, Linux Kernel Mailing List, Arnd Bergmann,
linux-arch, Thomas Gleixner, Jason Cooper, Rob Herring, netdev,
Vincent Chen, DTML, Al Viro, David Howells, Will Deacon,
Daniel Lezcano, linux-serial-u79uwXL29TY76Z2rM5mHXA,
Geert Uytterhoeven, Linus Walleij, Greg KH, Vincent Chen
In-Reply-To: <f58c7052-c2fe-5704-a03b-41bf2e3b20b9-5wv7dgnIgG8@public.gmane.org>
Hi, Marc:
2017-12-08 20:29 GMT+08:00 Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>:
> On 08/12/17 11:54, Greentime Hu wrote:
>> Hi, Mark:
>>
>> 2017-12-08 18:21 GMT+08:00 Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>:
>>> On Fri, Dec 08, 2017 at 05:12:00PM +0800, Greentime Hu wrote:
>>>> From: Greentime Hu <greentime-MUIXKm3Oiri1Z/+hSey0Gg@public.gmane.org>
>>>>
>>>> This patch adds VDSO support. The VDSO code is currently used for
>>>> sys_rt_sigreturn() and optimised gettimeofday() (using the SoC timer counter).
>>>
>>> [...]
>>>
>>>> +static int grab_timer_node_info(void)
>>>> +{
>>>> + struct device_node *timer_node;
>>>> +
>>>> + timer_node = of_find_node_by_name(NULL, "timer");
>>>
>>> Please use a compatible string, rather than matching the timer by name.
>>>
>>> It's plausible that you have multiple nodes called "timer" in the DT,
>>> under different parent nodes, and this might not be the device you
>>> think it is. I see your dt in patch 24 has two timer nodes.
>>>
>>> It would be best if your clocksource driver exposed some stuct that you
>>> looked at here, so that you're guaranteed to user the same device.
>>
>> We'd like to use "timer" here because there are 2 different timer IPs
>> and we are sure that they won't be in the same SoC.
>> We think this implementation in VDSO should be platform independent to
>> get cycle-count register.
>> Our customer or other SoC provider who can use "timer" and define
>> cycle-count-offset or cycle-count-down then we can get the correct
>> cycle-count.
>>
>> We sent atcpit100 patch last time along with our arch, however we'd
>> like to send it to its sub system this time and my colleague is still
>> working on it.
>> He may send the timer patch next week.
>>
>>
>>>> + of_property_read_u32(timer_node, "cycle-count-offset",
>>>> + &vdso_data->cycle_count_offset);
>>>> + vdso_data->cycle_count_down =
>>>> + of_property_read_bool(timer_node, "cycle-count-down");
>>>
>>> ... and then you'd only need to parse these in one place, too.
>>>
>>> IIUC these are proeprties for the atcpit device, which has no
>>> documentation or driver in this series.
>>>
>>> So I'm rather confused as to what's going on here.
>>>
>>
>> These properties are defined in dts which can provide the cycle count
>> register offset address of that timer, so that we can get cycle-count.
>>
>>>> + return of_address_to_resource(timer_node, 0, &timer_res);
>>>> +}
>>>
>>>> +int arch_setup_additional_pages(struct linux_binprm *bprm, int uses_interp)
>>>> +{
>>>
>>>> + /*Map timer to user space */
>>>> + vdso_base += PAGE_SIZE;
>>>> + prot = __pgprot(_PAGE_V | _PAGE_M_UR_KR | _PAGE_D |
>>>> + _PAGE_G | _PAGE_C_DEV);
>>>> + ret = io_remap_pfn_range(vma, vdso_base, timer_res.start >> PAGE_SHIFT,
>>>> + PAGE_SIZE, prot);
>>>> + if (ret)
>>>> + goto up_fail;
>>>
>>> Maybe this is fine, but it looks a bit suspicious.
>>>
>>> Is it safe to map IO memory to a userspace process like this?
>>>
>>> In general that isn't safe, since userspace could access other registers
>>> (if those exist), perform accesses that change the state of hardware, or
>>> make unsupported access types (e.g. unaligned, atomic) that result in
>>> errors the kernel can't handle.
>>>
>>> Does none of that apply here?
>>
>> We only provide read permission to this page so hareware state won't
>> be chagned. It will trigger exception if we try to write.
>> We will check about the alignment/atomic issue of this region.
>
> It still feels a bit odd. A hostile userspace could potentially find out
> about what the kernel is doing. For example, if the deadline of the next
> timer is accessible by reading that page, userspace could infer a lot of
> things that we'd normally want to keep hidden. Not knowing this HW, I
> cannot answer that question, but maybe you can.
>
> Another question: MMIO accesses can be quite slow. How much do you gain
> by having a vdso compared to executing a system call?
>
I think the rest of the timer registers should be fine to be read.
Anyway we will discuss about the security issue.
Based on our previous experiments.
Decrease 4,519,021 (47%) cycle count for executing gettimeofday()
with: without vDSO(using syscall) = 5,091,342 : 9,610,363
The cycle count was get by CPU performance monitor.
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 17/33] nds32: VDSO support
From: Marc Zyngier @ 2017-12-08 12:29 UTC (permalink / raw)
To: Greentime Hu, Mark Rutland
Cc: Greentime, Linux Kernel Mailing List, Arnd Bergmann, linux-arch,
Thomas Gleixner, Jason Cooper, Rob Herring, netdev, Vincent Chen,
DTML, Al Viro, David Howells, Will Deacon, Daniel Lezcano,
linux-serial, Geert Uytterhoeven, Linus Walleij, Greg KH,
Vincent Chen
In-Reply-To: <CAEbi=3e9Ep4_DL4SSwp15as1t7ALvw-s2gqv+NsuRZiebNGFAQ@mail.gmail.com>
On 08/12/17 11:54, Greentime Hu wrote:
> Hi, Mark:
>
> 2017-12-08 18:21 GMT+08:00 Mark Rutland <mark.rutland@arm.com>:
>> On Fri, Dec 08, 2017 at 05:12:00PM +0800, Greentime Hu wrote:
>>> From: Greentime Hu <greentime@andestech.com>
>>>
>>> This patch adds VDSO support. The VDSO code is currently used for
>>> sys_rt_sigreturn() and optimised gettimeofday() (using the SoC timer counter).
>>
>> [...]
>>
>>> +static int grab_timer_node_info(void)
>>> +{
>>> + struct device_node *timer_node;
>>> +
>>> + timer_node = of_find_node_by_name(NULL, "timer");
>>
>> Please use a compatible string, rather than matching the timer by name.
>>
>> It's plausible that you have multiple nodes called "timer" in the DT,
>> under different parent nodes, and this might not be the device you
>> think it is. I see your dt in patch 24 has two timer nodes.
>>
>> It would be best if your clocksource driver exposed some stuct that you
>> looked at here, so that you're guaranteed to user the same device.
>
> We'd like to use "timer" here because there are 2 different timer IPs
> and we are sure that they won't be in the same SoC.
> We think this implementation in VDSO should be platform independent to
> get cycle-count register.
> Our customer or other SoC provider who can use "timer" and define
> cycle-count-offset or cycle-count-down then we can get the correct
> cycle-count.
>
> We sent atcpit100 patch last time along with our arch, however we'd
> like to send it to its sub system this time and my colleague is still
> working on it.
> He may send the timer patch next week.
>
>
>>> + of_property_read_u32(timer_node, "cycle-count-offset",
>>> + &vdso_data->cycle_count_offset);
>>> + vdso_data->cycle_count_down =
>>> + of_property_read_bool(timer_node, "cycle-count-down");
>>
>> ... and then you'd only need to parse these in one place, too.
>>
>> IIUC these are proeprties for the atcpit device, which has no
>> documentation or driver in this series.
>>
>> So I'm rather confused as to what's going on here.
>>
>
> These properties are defined in dts which can provide the cycle count
> register offset address of that timer, so that we can get cycle-count.
>
>>> + return of_address_to_resource(timer_node, 0, &timer_res);
>>> +}
>>
>>> +int arch_setup_additional_pages(struct linux_binprm *bprm, int uses_interp)
>>> +{
>>
>>> + /*Map timer to user space */
>>> + vdso_base += PAGE_SIZE;
>>> + prot = __pgprot(_PAGE_V | _PAGE_M_UR_KR | _PAGE_D |
>>> + _PAGE_G | _PAGE_C_DEV);
>>> + ret = io_remap_pfn_range(vma, vdso_base, timer_res.start >> PAGE_SHIFT,
>>> + PAGE_SIZE, prot);
>>> + if (ret)
>>> + goto up_fail;
>>
>> Maybe this is fine, but it looks a bit suspicious.
>>
>> Is it safe to map IO memory to a userspace process like this?
>>
>> In general that isn't safe, since userspace could access other registers
>> (if those exist), perform accesses that change the state of hardware, or
>> make unsupported access types (e.g. unaligned, atomic) that result in
>> errors the kernel can't handle.
>>
>> Does none of that apply here?
>
> We only provide read permission to this page so hareware state won't
> be chagned. It will trigger exception if we try to write.
> We will check about the alignment/atomic issue of this region.
It still feels a bit odd. A hostile userspace could potentially find out
about what the kernel is doing. For example, if the deadline of the next
timer is accessible by reading that page, userspace could infer a lot of
things that we'd normally want to keep hidden. Not knowing this HW, I
cannot answer that question, but maybe you can.
Another question: MMIO accesses can be quite slow. How much do you gain
by having a vdso compared to executing a system call?
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* Re: [PATCH v3 17/33] nds32: VDSO support
From: Mark Rutland @ 2017-12-08 12:14 UTC (permalink / raw)
To: Greentime Hu
Cc: Greentime, Linux Kernel Mailing List, Arnd Bergmann, linux-arch,
Thomas Gleixner, Jason Cooper, Marc Zyngier, Rob Herring, netdev,
Vincent Chen, DTML, Al Viro, David Howells, Will Deacon,
Daniel Lezcano, linux-serial, Geert Uytterhoeven, Linus Walleij,
Greg KH, Vincent Chen
In-Reply-To: <CAEbi=3e9Ep4_DL4SSwp15as1t7ALvw-s2gqv+NsuRZiebNGFAQ@mail.gmail.com>
On Fri, Dec 08, 2017 at 07:54:42PM +0800, Greentime Hu wrote:
> 2017-12-08 18:21 GMT+08:00 Mark Rutland <mark.rutland@arm.com>:
> > On Fri, Dec 08, 2017 at 05:12:00PM +0800, Greentime Hu wrote:
> >> +static int grab_timer_node_info(void)
> >> +{
> >> + struct device_node *timer_node;
> >> +
> >> + timer_node = of_find_node_by_name(NULL, "timer");
> >
> > Please use a compatible string, rather than matching the timer by name.
> >
> > It's plausible that you have multiple nodes called "timer" in the DT,
> > under different parent nodes, and this might not be the device you
> > think it is. I see your dt in patch 24 has two timer nodes.
> >
> > It would be best if your clocksource driver exposed some stuct that you
> > looked at here, so that you're guaranteed to user the same device.
>
> We'd like to use "timer" here because there are 2 different timer IPs
> and we are sure that they won't be in the same SoC.
> We think this implementation in VDSO should be platform independent to
> get cycle-count register.
> Our customer or other SoC provider who can use "timer" and define
> cycle-count-offset or cycle-count-down then we can get the correct
> cycle-count.
This is not the right way to do things.
So from a DT perspective, NAK.
You should not add properties to arbitrary DT bindings to handle a Linux
implementation detail.
Please remove this DT code, and have the drivers for those timer blocks
export this information to your vdso code somehow.
> We sent atcpit100 patch last time along with our arch, however we'd
> like to send it to its sub system this time and my colleague is still
> working on it.
> He may send the timer patch next week.
I think that it would make sense for that patch to be part of the arch
port, especially given that (AFAICT) there is no dirver for the other
timer IP that you mention.
[...]
> >> +int arch_setup_additional_pages(struct linux_binprm *bprm, int uses_interp)
> >> +{
> >
> >> + /*Map timer to user space */
> >> + vdso_base += PAGE_SIZE;
> >> + prot = __pgprot(_PAGE_V | _PAGE_M_UR_KR | _PAGE_D |
> >> + _PAGE_G | _PAGE_C_DEV);
> >> + ret = io_remap_pfn_range(vma, vdso_base, timer_res.start >> PAGE_SHIFT,
> >> + PAGE_SIZE, prot);
> >> + if (ret)
> >> + goto up_fail;
> >
> > Maybe this is fine, but it looks a bit suspicious.
> >
> > Is it safe to map IO memory to a userspace process like this?
> >
> > In general that isn't safe, since userspace could access other registers
> > (if those exist), perform accesses that change the state of hardware, or
> > make unsupported access types (e.g. unaligned, atomic) that result in
> > errors the kernel can't handle.
> >
> > Does none of that apply here?
>
> We only provide read permission to this page so hareware state won't
> be chagned. It will trigger exception if we try to write.
> We will check about the alignment/atomic issue of this region.
Ok, thanks.
This is another reason to only do this for devices/drivers that we have
drivers for, since we can't know that this is safe in general.
Thanks,
Mark.
^ permalink raw reply
* Re: [PATCH v3 17/33] nds32: VDSO support
From: Greentime Hu @ 2017-12-08 11:54 UTC (permalink / raw)
To: Mark Rutland
Cc: Greentime, Linux Kernel Mailing List, Arnd Bergmann, linux-arch,
Thomas Gleixner, Jason Cooper, Marc Zyngier, Rob Herring, netdev,
Vincent Chen, DTML, Al Viro, David Howells, Will Deacon,
Daniel Lezcano, linux-serial, Geert Uytterhoeven, Linus Walleij,
Greg KH, Vincent Chen
In-Reply-To: <20171208102149.iqiieszktwzorkuw@lakrids.cambridge.arm.com>
Hi, Mark:
2017-12-08 18:21 GMT+08:00 Mark Rutland <mark.rutland@arm.com>:
> On Fri, Dec 08, 2017 at 05:12:00PM +0800, Greentime Hu wrote:
>> From: Greentime Hu <greentime@andestech.com>
>>
>> This patch adds VDSO support. The VDSO code is currently used for
>> sys_rt_sigreturn() and optimised gettimeofday() (using the SoC timer counter).
>
> [...]
>
>> +static int grab_timer_node_info(void)
>> +{
>> + struct device_node *timer_node;
>> +
>> + timer_node = of_find_node_by_name(NULL, "timer");
>
> Please use a compatible string, rather than matching the timer by name.
>
> It's plausible that you have multiple nodes called "timer" in the DT,
> under different parent nodes, and this might not be the device you
> think it is. I see your dt in patch 24 has two timer nodes.
>
> It would be best if your clocksource driver exposed some stuct that you
> looked at here, so that you're guaranteed to user the same device.
We'd like to use "timer" here because there are 2 different timer IPs
and we are sure that they won't be in the same SoC.
We think this implementation in VDSO should be platform independent to
get cycle-count register.
Our customer or other SoC provider who can use "timer" and define
cycle-count-offset or cycle-count-down then we can get the correct
cycle-count.
We sent atcpit100 patch last time along with our arch, however we'd
like to send it to its sub system this time and my colleague is still
working on it.
He may send the timer patch next week.
>> + of_property_read_u32(timer_node, "cycle-count-offset",
>> + &vdso_data->cycle_count_offset);
>> + vdso_data->cycle_count_down =
>> + of_property_read_bool(timer_node, "cycle-count-down");
>
> ... and then you'd only need to parse these in one place, too.
>
> IIUC these are proeprties for the atcpit device, which has no
> documentation or driver in this series.
>
> So I'm rather confused as to what's going on here.
>
These properties are defined in dts which can provide the cycle count
register offset address of that timer, so that we can get cycle-count.
>> + return of_address_to_resource(timer_node, 0, &timer_res);
>> +}
>
>> +int arch_setup_additional_pages(struct linux_binprm *bprm, int uses_interp)
>> +{
>
>> + /*Map timer to user space */
>> + vdso_base += PAGE_SIZE;
>> + prot = __pgprot(_PAGE_V | _PAGE_M_UR_KR | _PAGE_D |
>> + _PAGE_G | _PAGE_C_DEV);
>> + ret = io_remap_pfn_range(vma, vdso_base, timer_res.start >> PAGE_SHIFT,
>> + PAGE_SIZE, prot);
>> + if (ret)
>> + goto up_fail;
>
> Maybe this is fine, but it looks a bit suspicious.
>
> Is it safe to map IO memory to a userspace process like this?
>
> In general that isn't safe, since userspace could access other registers
> (if those exist), perform accesses that change the state of hardware, or
> make unsupported access types (e.g. unaligned, atomic) that result in
> errors the kernel can't handle.
>
> Does none of that apply here?
We only provide read permission to this page so hareware state won't
be chagned. It will trigger exception if we try to write.
We will check about the alignment/atomic issue of this region.
Thanks.
^ permalink raw reply
* Re: [PATCH 2/2] serial: tegra: Fix a typo in a comment line
From: Jon Hunter @ 2017-12-08 10:42 UTC (permalink / raw)
To: Laxman Dewangan, SF Markus Elfring, linux-serial, linux-tegra,
Greg Kroah-Hartman, Jiri Slaby, Thierry Reding
Cc: LKML, kernel-janitors, trivial
In-Reply-To: <b7bcd402-1fc2-85aa-bbd6-89e2e465e855@nvidia.com>
On 08/12/17 09:37, Laxman Dewangan wrote:
>
>
> On Friday 08 December 2017 01:51 AM, SF Markus Elfring wrote:
>> From: Markus Elfring <elfring@users.sourceforge.net>
>> Date: Thu, 7 Dec 2017 21:06:25 +0100
>>
>> Delete a duplicate character in a word of this description.
>>
>
> Acked-by: Laxman Dewangan <ldewangan@nvidia.com>
Acked-by: Jon Hunter <jonathanh@nvidia.com>
Thanks!
Jon
--
nvpublic
^ permalink raw reply
* Re: [PATCH 1/2] serial: tegra: Delete an error message for a failed memory allocation in tegra_uart_probe()
From: Jon Hunter @ 2017-12-08 10:42 UTC (permalink / raw)
To: Laxman Dewangan, SF Markus Elfring, linux-serial, linux-tegra,
Greg Kroah-Hartman, Jiri Slaby, Thierry Reding
Cc: LKML, kernel-janitors
In-Reply-To: <cced0e90-61b1-2182-a2b3-327ee9394190@nvidia.com>
On 08/12/17 09:37, Laxman Dewangan wrote:
> On Friday 08 December 2017 01:49 AM, SF Markus Elfring wrote:
>> From: Markus Elfring <elfring@users.sourceforge.net>
>> Date: Thu, 7 Dec 2017 21:00:05 +0100
>>
>> Omit an extra message for a memory allocation failure in this function.
>>
>> This issue was detected by using the Coccinelle software.
>>
>
> Acked-by: Laxman Dewangan <ldewangan@nvidia.com>
Acked-by: Jon Hunter <jonathanh@nvidia.com>
Thanks!
Jon
--
nvpublic
^ permalink raw reply
* Re: [PATCH v3 24/33] nds32: Device tree support
From: Mark Rutland @ 2017-12-08 10:27 UTC (permalink / raw)
To: Greentime Hu
Cc: greentime, linux-kernel, arnd, linux-arch, tglx, jason,
marc.zyngier, robh+dt, netdev, deanbo422, devicetree, viro,
dhowells, will.deacon, daniel.lezcano, linux-serial,
geert.uytterhoeven, linus.walleij, greg, Vincent Chen
In-Reply-To: <c1083b6f603aeeb13c72c501214e82549404fa5c.1512723245.git.green.hu@gmail.com>
On Fri, Dec 08, 2017 at 05:12:07PM +0800, Greentime Hu wrote:
> + timer0: timer@98400000 {
> + compatible = "andestech,atftmr010";
> + reg = <0x98400000 0x1000>;
> + interrupts = <19>;
> + clocks = <&clk_pll>;
> + clock-names = "apb_pclk";
> + cycle-count-offset = <0x20>;
> + };
Looking through the series, I can't find a binding or driver for this
timer, either.
Does timekeeping work with this series, or are additional patches
necessary?
Thanks,
Mark.
^ permalink raw reply
* Re: [PATCH v3 24/33] nds32: Device tree support
From: Mark Rutland @ 2017-12-08 10:23 UTC (permalink / raw)
To: Greentime Hu
Cc: greentime, linux-kernel, arnd, linux-arch, tglx, jason,
marc.zyngier, robh+dt, netdev, deanbo422, devicetree, viro,
dhowells, will.deacon, daniel.lezcano, linux-serial,
geert.uytterhoeven, linus.walleij, greg, Vincent Chen
In-Reply-To: <c1083b6f603aeeb13c72c501214e82549404fa5c.1512723245.git.green.hu@gmail.com>
On Fri, Dec 08, 2017 at 05:12:07PM +0800, Greentime Hu wrote:
> + timer0: timer@f0400000 {
> + compatible = "andestech,atcpit100";
> + reg = <0xf0400000 0x1000>;
> + interrupts = <2>;
> + clocks = <&clk_pll>;
> + clock-names = "apb_pclk";
> + cycle-count-offset = <0x38>;
> + cycle-count-down;
> + };
The cover leter mentioned the atcpit100 driver had been removed, but the
node is still here, and the vdso code seems to look for this node.
This binding needs documentation.
Thanks,
Mark.
^ permalink raw reply
* Re: [PATCH v3 17/33] nds32: VDSO support
From: Mark Rutland @ 2017-12-08 10:21 UTC (permalink / raw)
To: Greentime Hu
Cc: greentime, linux-kernel, arnd, linux-arch, tglx, jason,
marc.zyngier, robh+dt, netdev, deanbo422, devicetree, viro,
dhowells, will.deacon, daniel.lezcano, linux-serial,
geert.uytterhoeven, linus.walleij, greg, Vincent Chen
In-Reply-To: <921ccfa97c4c13f12c7b22b9554f52dcce51f87e.1512723245.git.green.hu@gmail.com>
On Fri, Dec 08, 2017 at 05:12:00PM +0800, Greentime Hu wrote:
> From: Greentime Hu <greentime@andestech.com>
>
> This patch adds VDSO support. The VDSO code is currently used for
> sys_rt_sigreturn() and optimised gettimeofday() (using the SoC timer counter).
[...]
> +static int grab_timer_node_info(void)
> +{
> + struct device_node *timer_node;
> +
> + timer_node = of_find_node_by_name(NULL, "timer");
Please use a compatible string, rather than matching the timer by name.
It's plausible that you have multiple nodes called "timer" in the DT,
under different parent nodes, and this might not be the device you
think it is. I see your dt in patch 24 has two timer nodes.
It would be best if your clocksource driver exposed some stuct that you
looked at here, so that you're guaranteed to user the same device.
> + of_property_read_u32(timer_node, "cycle-count-offset",
> + &vdso_data->cycle_count_offset);
> + vdso_data->cycle_count_down =
> + of_property_read_bool(timer_node, "cycle-count-down");
... and then you'd only need to parse these in one place, too.
IIUC these are proeprties for the atcpit device, which has no
documentation or driver in this series.
So I'm rather confused as to what's going on here.
> + return of_address_to_resource(timer_node, 0, &timer_res);
> +}
> +int arch_setup_additional_pages(struct linux_binprm *bprm, int uses_interp)
> +{
> + /*Map timer to user space */
> + vdso_base += PAGE_SIZE;
> + prot = __pgprot(_PAGE_V | _PAGE_M_UR_KR | _PAGE_D |
> + _PAGE_G | _PAGE_C_DEV);
> + ret = io_remap_pfn_range(vma, vdso_base, timer_res.start >> PAGE_SHIFT,
> + PAGE_SIZE, prot);
> + if (ret)
> + goto up_fail;
Maybe this is fine, but it looks a bit suspicious.
Is it safe to map IO memory to a userspace process like this?
In general that isn't safe, since userspace could access other registers
(if those exist), perform accesses that change the state of hardware, or
make unsupported access types (e.g. unaligned, atomic) that result in
errors the kernel can't handle.
Does none of that apply here?
Thanks,
Mark.
^ permalink raw reply
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