* [PATCH v4 0/9] serial/gpio: exar: Fixes and support for IOT2000
From: Jan Kiszka @ 2017-05-30 7:01 UTC (permalink / raw)
To: Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot
Cc: Linux Kernel Mailing List, linux-serial, linux-gpio,
Sudip Mukherjee, Andy Shevchenko, Sascha Weisenberger
This makes the gpio-exar driver usable, which was prevented by a number
of fatal bugs, and adds support for the SIMATIC IOT2040 to the 8250-exar
driver and, indirectly, to gpio-exar as well. It's a cross-subsystem
series, so I'm also cross-posting to the serial and gpio lists.
Changes in v4:
- fold v3 patches 8 and 9 together
- use DT-like property names
I would suggest to send this series as a whole through one branch,
either gpio after receiving acks from Greg or serial (most acks from
Linus are there).
Jan
Jan Kiszka (9):
serial: exar: Leave MPIOs as output for Commtech adapters
gpio-exar/8250-exar: Do not even instantiate a GPIO device for
Commtech cards
gpio-exar/8250-exar: Fix passing in of parent PCI device
gpio: exar: Allocate resources on behalf of the platform device
gpio: exar: Fix iomap request
gpio: exar: Fix reading of directions and values
gpio-exar/8250-exar: Rearrange gpiochip parenthood
gpio-exar/8250-exar: Make set of exported GPIOs configurable
serial: exar: Add support for IOT2040 device
drivers/gpio/gpio-exar.c | 75 ++++++++-------
drivers/tty/serial/8250/8250_exar.c | 180 ++++++++++++++++++++++++++++++++++--
2 files changed, 208 insertions(+), 47 deletions(-)
--
2.12.3
^ permalink raw reply
* [PATCH v4 1/9] serial: exar: Leave MPIOs as output for Commtech adapters
From: Jan Kiszka @ 2017-05-30 7:01 UTC (permalink / raw)
To: Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot
Cc: Linux Kernel Mailing List, linux-serial, linux-gpio,
Sudip Mukherjee, Andy Shevchenko, Sascha Weisenberger
In-Reply-To: <cover.1496127684.git.jan.kiszka@siemens.com>
Commtech adapters apparently need the original setting as outputs, see
https://marc.info/?l=linux-gpio&m=149557425201323&w=2. Account for that.
Fixes: 7dea8165f1d6 ("serial: exar: Preconfigure xr17v35x MPIOs as output")
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
drivers/tty/serial/8250/8250_exar.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index b4fa585156c7..8984e8b2d524 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -171,19 +171,26 @@ pci_xr17c154_setup(struct exar8250 *priv, struct pci_dev *pcidev,
return default_setup(priv, pcidev, idx, offset, port);
}
-static void setup_gpio(u8 __iomem *p)
+static void setup_gpio(struct pci_dev *pcidev, u8 __iomem *p)
{
+ /*
+ * The Commtech adapters required the MPIOs to be driven low. The Exar
+ * devices will export them as GPIOs, so we pre-configure them safely
+ * as inputs.
+ */
+ u8 dir = pcidev->vendor == PCI_VENDOR_ID_EXAR ? 0xff : 0x00;
+
writeb(0x00, p + UART_EXAR_MPIOINT_7_0);
writeb(0x00, p + UART_EXAR_MPIOLVL_7_0);
writeb(0x00, p + UART_EXAR_MPIO3T_7_0);
writeb(0x00, p + UART_EXAR_MPIOINV_7_0);
- writeb(0xff, p + UART_EXAR_MPIOSEL_7_0);
+ writeb(dir, p + UART_EXAR_MPIOSEL_7_0);
writeb(0x00, p + UART_EXAR_MPIOOD_7_0);
writeb(0x00, p + UART_EXAR_MPIOINT_15_8);
writeb(0x00, p + UART_EXAR_MPIOLVL_15_8);
writeb(0x00, p + UART_EXAR_MPIO3T_15_8);
writeb(0x00, p + UART_EXAR_MPIOINV_15_8);
- writeb(0xff, p + UART_EXAR_MPIOSEL_15_8);
+ writeb(dir, p + UART_EXAR_MPIOSEL_15_8);
writeb(0x00, p + UART_EXAR_MPIOOD_15_8);
}
@@ -236,7 +243,7 @@ pci_xr17v35x_setup(struct exar8250 *priv, struct pci_dev *pcidev,
if (idx == 0) {
/* Setup Multipurpose Input/Output pins. */
- setup_gpio(p);
+ setup_gpio(pcidev, p);
port->port.private_data = xr17v35x_register_gpio(pcidev);
}
--
2.12.3
^ permalink raw reply related
* [PATCH v4 2/9] gpio-exar/8250-exar: Do not even instantiate a GPIO device for Commtech cards
From: Jan Kiszka @ 2017-05-30 7:01 UTC (permalink / raw)
To: Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot
Cc: Linux Kernel Mailing List, linux-serial, linux-gpio,
Sudip Mukherjee, Andy Shevchenko, Sascha Weisenberger
In-Reply-To: <cover.1496127684.git.jan.kiszka@siemens.com>
Commtech adapters need the MPIOs for internal purposes, and the
gpio-exar driver already refused to pick them up. But there is actually
no point in even creating the underlying platform device.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
drivers/gpio/gpio-exar.c | 3 ---
drivers/tty/serial/8250/8250_exar.c | 4 +++-
2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c
index 081076771217..006a9a67c2a4 100644
--- a/drivers/gpio/gpio-exar.c
+++ b/drivers/gpio/gpio-exar.c
@@ -124,9 +124,6 @@ static int gpio_exar_probe(struct platform_device *pdev)
void __iomem *p;
int index, ret;
- if (pcidev->vendor != PCI_VENDOR_ID_EXAR)
- return -ENODEV;
-
/*
* Map the pci device to get the register addresses.
* We will need to read and write those registers to control
diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 8984e8b2d524..c29c7e675890 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -245,7 +245,9 @@ pci_xr17v35x_setup(struct exar8250 *priv, struct pci_dev *pcidev,
/* Setup Multipurpose Input/Output pins. */
setup_gpio(pcidev, p);
- port->port.private_data = xr17v35x_register_gpio(pcidev);
+ if (pcidev->vendor == PCI_VENDOR_ID_EXAR)
+ port->port.private_data =
+ xr17v35x_register_gpio(pcidev);
}
return 0;
--
2.12.3
^ permalink raw reply related
* [PATCH v4 3/9] gpio-exar/8250-exar: Fix passing in of parent PCI device
From: Jan Kiszka @ 2017-05-30 7:01 UTC (permalink / raw)
To: Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot
Cc: Linux Kernel Mailing List, linux-serial, linux-gpio,
Sudip Mukherjee, Andy Shevchenko, Sascha Weisenberger
In-Reply-To: <cover.1496127684.git.jan.kiszka@siemens.com>
This fixes reloading of the GPIO driver for the same platform device
instance as created by the exar UART driver: First of all, the driver
sets drvdata to its own value during probing and does not restore the
original value on exit. But this won't help anyway as the core clears
drvdata after the driver left.
Set the platform device parent instead.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
drivers/gpio/gpio-exar.c | 2 +-
drivers/tty/serial/8250/8250_exar.c | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c
index 006a9a67c2a4..da4550bb9939 100644
--- a/drivers/gpio/gpio-exar.c
+++ b/drivers/gpio/gpio-exar.c
@@ -119,7 +119,7 @@ static int exar_direction_input(struct gpio_chip *chip, unsigned int offset)
static int gpio_exar_probe(struct platform_device *pdev)
{
- struct pci_dev *pcidev = platform_get_drvdata(pdev);
+ struct pci_dev *pcidev = to_pci_dev(pdev->dev.parent);
struct exar_gpio_chip *exar_gpio;
void __iomem *p;
int index, ret;
diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index c29c7e675890..0f4b236d7e68 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -203,7 +203,8 @@ xr17v35x_register_gpio(struct pci_dev *pcidev)
if (!pdev)
return NULL;
- platform_set_drvdata(pdev, pcidev);
+ pdev->dev.parent = &pcidev->dev;
+
if (platform_device_add(pdev) < 0) {
platform_device_put(pdev);
return NULL;
--
2.12.3
^ permalink raw reply related
* [PATCH v4 4/9] gpio: exar: Allocate resources on behalf of the platform device
From: Jan Kiszka @ 2017-05-30 7:01 UTC (permalink / raw)
To: Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot
Cc: Linux Kernel Mailing List, linux-serial, linux-gpio,
Sudip Mukherjee, Andy Shevchenko, Sascha Weisenberger
In-Reply-To: <cover.1496127684.git.jan.kiszka@siemens.com>
Do not allocate resources on behalf of the parent device but on our own.
Otherwise, cleanup does not properly work if gpio-exar is removed but
not the parent device.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
drivers/gpio/gpio-exar.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c
index da4550bb9939..65126fa1e512 100644
--- a/drivers/gpio/gpio-exar.c
+++ b/drivers/gpio/gpio-exar.c
@@ -136,7 +136,7 @@ static int gpio_exar_probe(struct platform_device *pdev)
if (!p)
return -ENOMEM;
- exar_gpio = devm_kzalloc(&pcidev->dev, sizeof(*exar_gpio), GFP_KERNEL);
+ exar_gpio = devm_kzalloc(&pdev->dev, sizeof(*exar_gpio), GFP_KERNEL);
if (!exar_gpio)
return -ENOMEM;
@@ -157,7 +157,7 @@ static int gpio_exar_probe(struct platform_device *pdev)
exar_gpio->regs = p;
exar_gpio->index = index;
- ret = devm_gpiochip_add_data(&pcidev->dev,
+ ret = devm_gpiochip_add_data(&pdev->dev,
&exar_gpio->gpio_chip, exar_gpio);
if (ret)
goto err_destroy;
--
2.12.3
^ permalink raw reply related
* [PATCH v4 5/9] gpio: exar: Fix iomap request
From: Jan Kiszka @ 2017-05-30 7:01 UTC (permalink / raw)
To: Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot
Cc: Linux Kernel Mailing List, linux-serial, linux-gpio,
Sudip Mukherjee, Andy Shevchenko, Sascha Weisenberger
In-Reply-To: <cover.1496127684.git.jan.kiszka@siemens.com>
The UART driver already maps the resource for us. Trying to do this here
only fails and leaves us with a non-working device.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
drivers/gpio/gpio-exar.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c
index 65126fa1e512..b29890b143ce 100644
--- a/drivers/gpio/gpio-exar.c
+++ b/drivers/gpio/gpio-exar.c
@@ -125,14 +125,10 @@ static int gpio_exar_probe(struct platform_device *pdev)
int index, ret;
/*
- * Map the pci device to get the register addresses.
- * We will need to read and write those registers to control
- * the GPIO pins.
- * Using managed functions will save us from unmaping on exit.
- * As the device is enabled using managed functions by the
- * UART driver we can also use managed functions here.
+ * The UART driver must have mapped region 0 prior to registering this
+ * device - use it.
*/
- p = pcim_iomap(pcidev, 0, 0);
+ p = pcim_iomap_table(pcidev)[0];
if (!p)
return -ENOMEM;
--
2.12.3
^ permalink raw reply related
* [PATCH v4 6/9] gpio: exar: Fix reading of directions and values
From: Jan Kiszka @ 2017-05-30 7:01 UTC (permalink / raw)
To: Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot
Cc: Linux Kernel Mailing List, linux-serial, linux-gpio,
Sudip Mukherjee, Andy Shevchenko, Sascha Weisenberger
In-Reply-To: <cover.1496127684.git.jan.kiszka@siemens.com>
First, the logic for translating a register bit to the return code of
exar_get_direction and exar_get_value were wrong. And second, there was
a flip regarding the register bank in exar_get_direction.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
drivers/gpio/gpio-exar.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c
index b29890b143ce..f3585a184f39 100644
--- a/drivers/gpio/gpio-exar.c
+++ b/drivers/gpio/gpio-exar.c
@@ -68,7 +68,7 @@ static int exar_get(struct gpio_chip *chip, unsigned int reg)
value = readb(exar_gpio->regs + reg);
mutex_unlock(&exar_gpio->lock);
- return !!value;
+ return value;
}
static int exar_get_direction(struct gpio_chip *chip, unsigned int offset)
@@ -78,7 +78,7 @@ static int exar_get_direction(struct gpio_chip *chip, unsigned int offset)
int val;
addr = bank ? EXAR_OFFSET_MPIOSEL_HI : EXAR_OFFSET_MPIOSEL_LO;
- val = exar_get(chip, addr) >> (offset % 8);
+ val = exar_get(chip, addr) & BIT(offset % 8);
return !!val;
}
@@ -89,8 +89,8 @@ static int exar_get_value(struct gpio_chip *chip, unsigned int offset)
unsigned int addr;
int val;
- addr = bank ? EXAR_OFFSET_MPIOLVL_LO : EXAR_OFFSET_MPIOLVL_HI;
- val = exar_get(chip, addr) >> (offset % 8);
+ addr = bank ? EXAR_OFFSET_MPIOLVL_HI : EXAR_OFFSET_MPIOLVL_LO;
+ val = exar_get(chip, addr) & BIT(offset % 8);
return !!val;
}
--
2.12.3
^ permalink raw reply related
* [PATCH v4 7/9] gpio-exar/8250-exar: Rearrange gpiochip parenthood
From: Jan Kiszka @ 2017-05-30 7:01 UTC (permalink / raw)
To: Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot
Cc: Linux Kernel Mailing List, linux-serial, linux-gpio,
Sudip Mukherjee, Andy Shevchenko, Sascha Weisenberger
In-Reply-To: <cover.1496127684.git.jan.kiszka@siemens.com>
Set the parent of the exar gpiochip to its platform device, like other
gpiochips are doing it. In order to keep the relationship discoverable
for ACPI systems, set the platform device companion to the PCI device.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
drivers/gpio/gpio-exar.c | 2 +-
drivers/tty/serial/8250/8250_exar.c | 2 ++
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c
index f3585a184f39..1a629831d45b 100644
--- a/drivers/gpio/gpio-exar.c
+++ b/drivers/gpio/gpio-exar.c
@@ -142,7 +142,7 @@ static int gpio_exar_probe(struct platform_device *pdev)
sprintf(exar_gpio->name, "exar_gpio%d", index);
exar_gpio->gpio_chip.label = exar_gpio->name;
- exar_gpio->gpio_chip.parent = &pcidev->dev;
+ exar_gpio->gpio_chip.parent = &pdev->dev;
exar_gpio->gpio_chip.direction_output = exar_direction_output;
exar_gpio->gpio_chip.direction_input = exar_direction_input;
exar_gpio->gpio_chip.get_direction = exar_get_direction;
diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 0f4b236d7e68..ee4b142f3ed0 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -9,6 +9,7 @@
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation; either version 2 of the License.
*/
+#include <linux/acpi.h>
#include <linux/io.h>
#include <linux/kernel.h>
#include <linux/module.h>
@@ -204,6 +205,7 @@ xr17v35x_register_gpio(struct pci_dev *pcidev)
return NULL;
pdev->dev.parent = &pcidev->dev;
+ ACPI_COMPANION_SET(&pdev->dev, ACPI_COMPANION(&pcidev->dev));
if (platform_device_add(pdev) < 0) {
platform_device_put(pdev);
--
2.12.3
^ permalink raw reply related
* [PATCH v4 8/9] gpio-exar/8250-exar: Make set of exported GPIOs configurable
From: Jan Kiszka @ 2017-05-30 7:01 UTC (permalink / raw)
To: Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot
Cc: Linux Kernel Mailing List, linux-serial, linux-gpio,
Sudip Mukherjee, Andy Shevchenko, Sascha Weisenberger
In-Reply-To: <cover.1496127684.git.jan.kiszka@siemens.com>
On the SIMATIC, IOT2040 only a single pin is exportable as GPIO, the
rest is required to operate the UART. To allow modeling this case,
expand the platform device data structure to specify a (consecutive) pin
subset for exporting by the gpio-exar driver.
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
drivers/gpio/gpio-exar.c | 52 +++++++++++++++++++++----------------
drivers/tty/serial/8250/8250_exar.c | 14 +++++++---
2 files changed, 40 insertions(+), 26 deletions(-)
diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c
index 1a629831d45b..220e93ed9111 100644
--- a/drivers/gpio/gpio-exar.c
+++ b/drivers/gpio/gpio-exar.c
@@ -31,6 +31,7 @@ struct exar_gpio_chip {
int index;
void __iomem *regs;
char name[20];
+ unsigned int first_pin;
};
static void exar_update(struct gpio_chip *chip, unsigned int reg, int val,
@@ -51,11 +52,12 @@ static void exar_update(struct gpio_chip *chip, unsigned int reg, int val,
static int exar_set_direction(struct gpio_chip *chip, int direction,
unsigned int offset)
{
- unsigned int bank = offset / 8;
- unsigned int addr;
+ struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
+ unsigned int addr = (offset + exar_gpio->first_pin) / 8 ?
+ EXAR_OFFSET_MPIOSEL_HI : EXAR_OFFSET_MPIOSEL_LO;
+ unsigned int bit = (offset + exar_gpio->first_pin) % 8;
- addr = bank ? EXAR_OFFSET_MPIOSEL_HI : EXAR_OFFSET_MPIOSEL_LO;
- exar_update(chip, addr, direction, offset % 8);
+ exar_update(chip, addr, direction, bit);
return 0;
}
@@ -73,36 +75,33 @@ static int exar_get(struct gpio_chip *chip, unsigned int reg)
static int exar_get_direction(struct gpio_chip *chip, unsigned int offset)
{
- unsigned int bank = offset / 8;
- unsigned int addr;
- int val;
-
- addr = bank ? EXAR_OFFSET_MPIOSEL_HI : EXAR_OFFSET_MPIOSEL_LO;
- val = exar_get(chip, addr) & BIT(offset % 8);
+ struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
+ unsigned int addr = (offset + exar_gpio->first_pin) / 8 ?
+ EXAR_OFFSET_MPIOSEL_HI : EXAR_OFFSET_MPIOSEL_LO;
+ unsigned int bit = (offset + exar_gpio->first_pin) % 8;
- return !!val;
+ return !!(exar_get(chip, addr) & BIT(bit));
}
static int exar_get_value(struct gpio_chip *chip, unsigned int offset)
{
- unsigned int bank = offset / 8;
- unsigned int addr;
- int val;
-
- addr = bank ? EXAR_OFFSET_MPIOLVL_HI : EXAR_OFFSET_MPIOLVL_LO;
- val = exar_get(chip, addr) & BIT(offset % 8);
+ struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
+ unsigned int addr = (offset + exar_gpio->first_pin) / 8 ?
+ EXAR_OFFSET_MPIOLVL_HI : EXAR_OFFSET_MPIOLVL_LO;
+ unsigned int bit = (offset + exar_gpio->first_pin) % 8;
- return !!val;
+ return !!(exar_get(chip, addr) & BIT(bit));
}
static void exar_set_value(struct gpio_chip *chip, unsigned int offset,
int value)
{
- unsigned int bank = offset / 8;
- unsigned int addr;
+ struct exar_gpio_chip *exar_gpio = gpiochip_get_data(chip);
+ unsigned int addr = (offset + exar_gpio->first_pin) / 8 ?
+ EXAR_OFFSET_MPIOLVL_HI : EXAR_OFFSET_MPIOLVL_LO;
+ unsigned int bit = (offset + exar_gpio->first_pin) % 8;
- addr = bank ? EXAR_OFFSET_MPIOLVL_HI : EXAR_OFFSET_MPIOLVL_LO;
- exar_update(chip, addr, value, offset % 8);
+ exar_update(chip, addr, value, bit);
}
static int exar_direction_output(struct gpio_chip *chip, unsigned int offset,
@@ -121,6 +120,7 @@ static int gpio_exar_probe(struct platform_device *pdev)
{
struct pci_dev *pcidev = to_pci_dev(pdev->dev.parent);
struct exar_gpio_chip *exar_gpio;
+ u32 first_pin, ngpios;
void __iomem *p;
int index, ret;
@@ -132,6 +132,11 @@ static int gpio_exar_probe(struct platform_device *pdev)
if (!p)
return -ENOMEM;
+ if (device_property_read_u32(&pdev->dev, "gpio-exar,first-pin",
+ &first_pin) < 0 ||
+ device_property_read_u32(&pdev->dev, "ngpios", &ngpios) < 0)
+ return -EINVAL;
+
exar_gpio = devm_kzalloc(&pdev->dev, sizeof(*exar_gpio), GFP_KERNEL);
if (!exar_gpio)
return -ENOMEM;
@@ -149,9 +154,10 @@ static int gpio_exar_probe(struct platform_device *pdev)
exar_gpio->gpio_chip.get = exar_get_value;
exar_gpio->gpio_chip.set = exar_set_value;
exar_gpio->gpio_chip.base = -1;
- exar_gpio->gpio_chip.ngpio = 16;
+ exar_gpio->gpio_chip.ngpio = ngpios;
exar_gpio->regs = p;
exar_gpio->index = index;
+ exar_gpio->first_pin = first_pin;
ret = devm_gpiochip_add_data(&pdev->dev,
&exar_gpio->gpio_chip, exar_gpio);
diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index ee4b142f3ed0..1cdf125ffe30 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -14,6 +14,7 @@
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/pci.h>
+#include <linux/property.h>
#include <linux/serial_core.h>
#include <linux/serial_reg.h>
#include <linux/slab.h>
@@ -196,8 +197,14 @@ static void setup_gpio(struct pci_dev *pcidev, u8 __iomem *p)
}
static void *
-xr17v35x_register_gpio(struct pci_dev *pcidev)
+xr17v35x_register_gpio(struct pci_dev *pcidev, unsigned int first_pin,
+ unsigned int ngpios)
{
+ struct property_entry properties[] = {
+ PROPERTY_ENTRY_U32("gpio-exar,first-pin", first_pin),
+ PROPERTY_ENTRY_U32("ngpios", ngpios),
+ { }
+ };
struct platform_device *pdev;
pdev = platform_device_alloc("gpio_exar", PLATFORM_DEVID_AUTO);
@@ -207,7 +214,8 @@ xr17v35x_register_gpio(struct pci_dev *pcidev)
pdev->dev.parent = &pcidev->dev;
ACPI_COMPANION_SET(&pdev->dev, ACPI_COMPANION(&pcidev->dev));
- if (platform_device_add(pdev) < 0) {
+ if (platform_device_add_properties(pdev, properties) < 0 ||
+ platform_device_add(pdev) < 0) {
platform_device_put(pdev);
return NULL;
}
@@ -250,7 +258,7 @@ pci_xr17v35x_setup(struct exar8250 *priv, struct pci_dev *pcidev,
if (pcidev->vendor == PCI_VENDOR_ID_EXAR)
port->port.private_data =
- xr17v35x_register_gpio(pcidev);
+ xr17v35x_register_gpio(pcidev, 0, 16);
}
return 0;
--
2.12.3
^ permalink raw reply related
* [PATCH v4 9/9] serial: exar: Add support for IOT2040 device
From: Jan Kiszka @ 2017-05-30 7:01 UTC (permalink / raw)
To: Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot
Cc: Linux Kernel Mailing List, linux-serial, linux-gpio,
Sudip Mukherjee, Andy Shevchenko, Sascha Weisenberger
In-Reply-To: <cover.1496127684.git.jan.kiszka@siemens.com>
This implements the setup of RS232 and the switch-over to RS485 or RS422
for the Siemens IOT2040. That uses an EXAR XR17V352 with external logic
to switch between the different modes. The external logic is controlled
via MPIO pins of the EXAR controller.
Only pin 10 can be exported as GPIO on the IOT2040. It is connected to
an LED.
As the XR17V352 used on the IOT2040 is not equipped with an external
EEPROM, it cannot present itself as IOT2040-variant via subvendor/
subdevice IDs. Thus, we have to check via DMI for the target platform.
Co-developed with Sascha Weisenberger.
Signed-off-by: Sascha Weisenberger <sascha.weisenberger@siemens.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
drivers/tty/serial/8250/8250_exar.c | 154 ++++++++++++++++++++++++++++++++++--
1 file changed, 148 insertions(+), 6 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
index 1cdf125ffe30..25b138f43100 100644
--- a/drivers/tty/serial/8250/8250_exar.c
+++ b/drivers/tty/serial/8250/8250_exar.c
@@ -10,6 +10,7 @@
* the Free Software Foundation; either version 2 of the License.
*/
#include <linux/acpi.h>
+#include <linux/dmi.h>
#include <linux/io.h>
#include <linux/kernel.h>
#include <linux/module.h>
@@ -62,8 +63,50 @@
#define UART_EXAR_MPIOSEL_15_8 0x99 /* MPIOSEL[15:8] */
#define UART_EXAR_MPIOOD_15_8 0x9a /* MPIOOD[15:8] */
+#define UART_EXAR_RS485_DLY(x) ((x) << 4)
+
+/*
+ * IOT2040 MPIO wiring semantics:
+ *
+ * MPIO Port Function
+ * ---- ---- --------
+ * 0 2 Mode bit 0
+ * 1 2 Mode bit 1
+ * 2 2 Terminate bus
+ * 3 - <reserved>
+ * 4 3 Mode bit 0
+ * 5 3 Mode bit 1
+ * 6 3 Terminate bus
+ * 7 - <reserved>
+ * 8 2 Enable
+ * 9 3 Enable
+ * 10 - Red LED
+ * 11..15 - <unused>
+ */
+
+/* IOT2040 MPIOs 0..7 */
+#define IOT2040_UART_MODE_RS232 0x01
+#define IOT2040_UART_MODE_RS485 0x02
+#define IOT2040_UART_MODE_RS422 0x03
+#define IOT2040_UART_TERMINATE_BUS 0x04
+
+#define IOT2040_UART1_MASK 0x0f
+#define IOT2040_UART2_SHIFT 4
+
+#define IOT2040_UARTS_DEFAULT_MODE 0x11 /* both RS232 */
+#define IOT2040_UARTS_GPIO_LO_MODE 0x88 /* reserved pins as input */
+
+/* IOT2040 MPIOs 8..15 */
+#define IOT2040_UARTS_ENABLE 0x03
+#define IOT2040_UARTS_GPIO_HI_MODE 0xF8 /* enable & LED as outputs */
+
struct exar8250;
+struct exar8250_platform {
+ int (*rs485_config)(struct uart_port *, struct serial_rs485 *);
+ int (*register_gpio)(struct pci_dev *, struct uart_8250_port *);
+};
+
/**
* struct exar8250_board - board information
* @num_ports: number of serial ports
@@ -197,8 +240,8 @@ static void setup_gpio(struct pci_dev *pcidev, u8 __iomem *p)
}
static void *
-xr17v35x_register_gpio(struct pci_dev *pcidev, unsigned int first_pin,
- unsigned int ngpios)
+__xr17v35x_register_gpio(struct pci_dev *pcidev, unsigned int first_pin,
+ unsigned int ngpios)
{
struct property_entry properties[] = {
PROPERTY_ENTRY_U32("gpio-exar,first-pin", first_pin),
@@ -223,17 +266,118 @@ xr17v35x_register_gpio(struct pci_dev *pcidev, unsigned int first_pin,
return pdev;
}
+static int xr17v35x_register_gpio(struct pci_dev *pcidev,
+ struct uart_8250_port *port)
+{
+ if (pcidev->vendor == PCI_VENDOR_ID_EXAR)
+ port->port.private_data =
+ __xr17v35x_register_gpio(pcidev, 0, 16);
+
+ return 0;
+}
+
+static int iot2040_rs485_config(struct uart_port *port,
+ struct serial_rs485 *rs485)
+{
+ bool is_rs485 = !!(rs485->flags & SER_RS485_ENABLED);
+ u8 __iomem *p = port->membase;
+ u8 mask = IOT2040_UART1_MASK;
+ u8 mode, value;
+
+ if (is_rs485) {
+ if (rs485->flags & SER_RS485_RX_DURING_TX)
+ mode = IOT2040_UART_MODE_RS422;
+ else
+ mode = IOT2040_UART_MODE_RS485;
+
+ if (rs485->flags & SER_RS485_TERMINATE_BUS)
+ mode |= IOT2040_UART_TERMINATE_BUS;
+ } else {
+ mode = IOT2040_UART_MODE_RS232;
+ }
+
+ if (port->line == 3) {
+ mask <<= IOT2040_UART2_SHIFT;
+ mode <<= IOT2040_UART2_SHIFT;
+ }
+
+ value = readb(p + UART_EXAR_MPIOLVL_7_0);
+ value &= ~mask;
+ value |= mode;
+ writeb(value, p + UART_EXAR_MPIOLVL_7_0);
+
+ value = readb(p + UART_EXAR_FCTR);
+ if (is_rs485)
+ value |= UART_FCTR_EXAR_485;
+ else
+ value &= ~UART_FCTR_EXAR_485;
+ writeb(value, p + UART_EXAR_FCTR);
+
+ if (is_rs485)
+ writeb(UART_EXAR_RS485_DLY(4), p + UART_MSR);
+
+ port->rs485 = *rs485;
+
+ return 0;
+}
+
+static int iot2040_register_gpio(struct pci_dev *pcidev,
+ struct uart_8250_port *port)
+{
+ u8 __iomem *p = port->port.membase;
+
+ writeb(IOT2040_UARTS_DEFAULT_MODE, p + UART_EXAR_MPIOLVL_7_0);
+ writeb(IOT2040_UARTS_GPIO_LO_MODE, p + UART_EXAR_MPIOSEL_7_0);
+ writeb(IOT2040_UARTS_ENABLE, p + UART_EXAR_MPIOLVL_15_8);
+ writeb(IOT2040_UARTS_GPIO_HI_MODE, p + UART_EXAR_MPIOSEL_15_8);
+
+ port->port.private_data = __xr17v35x_register_gpio(pcidev, 10, 1);
+
+ return 0;
+}
+
+static const struct exar8250_platform exar8250_default_platform = {
+ .register_gpio = xr17v35x_register_gpio,
+};
+
+static const struct exar8250_platform iot2040_platform = {
+ .rs485_config = iot2040_rs485_config,
+ .register_gpio = iot2040_register_gpio,
+};
+
+static const struct dmi_system_id exar_platforms[] = {
+ {
+ .matches = {
+ DMI_EXACT_MATCH(DMI_BOARD_NAME, "SIMATIC IOT2000"),
+ DMI_EXACT_MATCH(DMI_BOARD_ASSET_TAG,
+ "6ES7647-0AA00-1YA2"),
+ },
+ .driver_data = (void *)&iot2040_platform,
+ },
+ {}
+};
+
static int
pci_xr17v35x_setup(struct exar8250 *priv, struct pci_dev *pcidev,
struct uart_8250_port *port, int idx)
{
const struct exar8250_board *board = priv->board;
+ const struct exar8250_platform *platform;
+ const struct dmi_system_id *dmi_match;
unsigned int offset = idx * 0x400;
unsigned int baud = 7812500;
u8 __iomem *p;
int ret;
+ dmi_match = dmi_first_match(exar_platforms);
+ if (dmi_match)
+ platform = dmi_match->driver_data;
+ else
+ platform = &exar8250_default_platform;
+
port->port.uartclk = baud * 16;
+ port->port.rs485_config = platform->rs485_config;
+
/*
* Setup the uart clock for the devices on expansion slot to
* half the clock speed of the main chip (which is 125MHz)
@@ -256,12 +400,10 @@ pci_xr17v35x_setup(struct exar8250 *priv, struct pci_dev *pcidev,
/* Setup Multipurpose Input/Output pins. */
setup_gpio(pcidev, p);
- if (pcidev->vendor == PCI_VENDOR_ID_EXAR)
- port->port.private_data =
- xr17v35x_register_gpio(pcidev, 0, 16);
+ ret = platform->register_gpio(pcidev, port);
}
- return 0;
+ return ret;
}
static void pci_xr17v35x_exit(struct pci_dev *pcidev)
--
2.12.3
^ permalink raw reply related
* Re: [GIT PULL] TTY/Serial driver fixes for 4.11-rc4
From: Dmitry Vyukov @ 2017-05-30 9:21 UTC (permalink / raw)
To: Greg KH
Cc: Vegard Nossum, Linus Torvalds, Jiri Slaby, Andrew Morton, LKML,
linux-serial
In-Reply-To: <20170503120101.GA21119@kroah.com>
On Wed, May 3, 2017 at 2:01 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>> >>> So the original problem is that the vmalloc() in n_tty_open() can
>> >>> fail, and that will panic in tty_set_ldisc()/tty_ldisc_restore()
>> >>> because of its unwillingness to proceed if the tty doesn't have an
>> >>> ldisc.
>> >>>
>> >>> Dmitry fixed this by allowing tty->ldisc == NULL in the case of memory
>> >>> allocation failure as we can see from the comment in tty_set_ldisc().
>> >>>
>> >>> Unfortunately, it would appear that some other bits of code do not
>> >>> like tty->ldisc == NULL (other than the crash in this thread, I saw
>> >>> 2-3 similar crashes in other functions, e.g. poll()). I see two
>> >>> possibilities:
>> >>>
>> >>> 1) make other code handle tty->ldisc == NULL.
>> >>>
>> >>> 2) don't close/free the old ldisc until the new one has been
>> >>> successfully created/initialised/opened/attached to the tty, and
>> >>> return an error to userspace if changing it failed.
>> >>>
>> >>> I'm leaning towards #2 as the more obviously correct fix, it makes
>> >>> tty_set_ldisc() transactional, the fix seems limited in scope to
>> >>> tty_set_ldisc() itself, and we don't need to make every other bit of
>> >>> code that uses tty->ldisc handle the NULL case.
>> >>
>> >> That sounds reasonable to me, care to work on a patch for this?
>> >
>> > Vegard, do you know how to do this?
>> > That was first thing that I tried, but I did not manage to make it
>> > work. disc is tied to tty, so it's not that one can create a fully
>> > initialized disc on the side and then simply swap pointers. Looking at
>> > the code now, there is at least TTY_LDISC_OPEN bit in tty. But as far
>> > as I remember there were more fundamental problems. Or maybe I just
>> > did not try too hard.
>>
>> I had a look at it but like you said, the tty/ldisc relationship is
>> complicated :-/
>>
>> Maybe we can split up ldisc initialisation into two methods so that
>> the first one (e.g. ->alloc) does all the allocation and is allowed to
>> fail and the second one (e.g. ->open) is not allowed to fail. Then you
>> can allocate a new ldisc without freeing the old one and only swap
>> them over if the allocation succeeded.
>>
>> That would require fixing up ->open for all the ldisc drivers though,
>> I'm not sure how easy/feasible it is.
>
> We don't have that many ldisc drivers, so it shouldn't be that hard to
> change to use this. It makes a lot more sense to fix this the correct
> way like this.
There are 26 drivers. This will require some careful surgery of them.
There is no way to test all of them without special hardware, right?
Also will require some changes to tty_ldisc.c.
So far I've filed https://github.com/google/syzkaller/issues/203 to
not lose track of it and collect all relevant details in one place.
>> I'll think about possible solutions, but I have no prior experience
>> with the tty code. In the meantime syzkaller also hit a couple of
>> other fun tty/pty bugs including a write/ioctl race that results in
>> buffer overflow :-/
>
> Ugh, let me know what they are and we'll work to fix them.
>
> Thanks for all of the work you all are doing in finding these issues, I
> might grumble about having to fix this and what a pain it is, but it's
> just me being grumpy about the tty code, not your effort. Your effort
> is much appreciated.
Thanks, Greg. Nice to hear.
^ permalink raw reply
* Re: [GIT PULL] TTY/Serial driver fixes for 4.11-rc4
From: Alan Cox @ 2017-05-30 12:09 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Greg KH, Vegard Nossum, Linus Torvalds, Jiri Slaby, Andrew Morton,
LKML, linux-serial
In-Reply-To: <CACT4Y+arELXuPr7psYFETt6NrDNgeS=tP=jGhP4a0_C_8Fa=LA@mail.gmail.com>
> >> I'll think about possible solutions, but I have no prior experience
> >> with the tty code. In the meantime syzkaller also hit a couple of
> >> other fun tty/pty bugs including a write/ioctl race that results in
> >> buffer overflow :-/
There are several of those, including some of that have been documented
for years but nobody ever volunteered to fix - in particular all the
interfaces that push characters to the tty other than via the normal
interrupt receive path are dodgy (console selection in particular)
The original tty model btw was that setting the ldisc to n_tty cannot
fail, and the structure allocated was smaller than a page size so was
safe.
The simple way to fix it is to restore that behaviour by adding a 'null'
ldisc that we can fail to instead of N_TTY since the N_TTY failback path
is long broken.
Something like this (untested)
commit 797035eaf800889287b0b176a11c89c0f1fbba30
Author: Alan Cox <alan@llwyncelyn.cymru>
Date: Tue May 30 12:59:45 2017 +0100
tty: handle the case where we cannot restore a line discipline
Historically the N_TTY driver could never fail but this has become broken over
time. Rather than trying to rewrite half the ldisc layer to fix the breakage
introduce a second level of fallback with an N_NULL ldisc which cannot fail,
and thus restore the guarantees required by the ldisc layer.
We still try and fail to N_TTY first. It's much more useful to find yourself
back in your old ldisc (first attempt) or in N_TTY (second attempt), and while
I'm not aware of any code out there that makes those assumptions it's good to
drive(r) defensively.
No signed off by: this is just a proposal!
diff --git a/drivers/tty/n_null.c b/drivers/tty/n_null.c
new file mode 100644
index 0000000..c5812cd
--- /dev/null
+++ b/drivers/tty/n_null.c
@@ -0,0 +1,67 @@
+/*
+ * n_null.c - Null line discipline used in the failure path
+ *
+ * Copyright (C) Intel 2017
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ * 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.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ */
+
+static int n_null_open(struct tty_struct *tty)
+{
+ return 0;
+}
+
+static void n_null_close(struct tty_struct *tty)
+{
+}
+
+static ssize_t n_null_read(struct tty_struct *tty, struct file *file,
+ unsigned char __user * buf, size_t nr)
+{
+ return -EOPNOTSUPP;
+}
+
+static ssize_t n_null_write(struct tty_struct *tty, struct file *file,
+ const unsigned char *buf, size_t nr)
+{
+ return -EOPNOTSUPP;
+}
+
+static ssize_t n_null_receivebuf(struct tty_struct *tty,
+ const unsigned char *cp, char *fp,
+ int cnt)
+{
+}
+
+static struct tty_ldisc_ops null_ldisc {
+ .owner = THIS_MODULE,
+ .magic = TTY_LDISC_MAGIC,
+ .name = "n_null",
+ .open = n_null_open,
+ .close = n_null_close,
+ .read = n_null_read,
+ .write = n_null_write,
+ .receive_buf = n_null_receivebuf
+};
+
+static int __init n_null_init(void)
+{
+ BUG_ON(tty_register_ldisc(N_NULL, &null_ldisc));
+ return 0;
+}
+
+static void __exit n_null_exit(void)
+{
+ tty_unregister_ldisc(N_NULL);
+}
diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
index f6ffe28..e80e05f 100644
--- a/drivers/tty/tty_ldisc.c
+++ b/drivers/tty/tty_ldisc.c
@@ -492,6 +492,29 @@ static void tty_ldisc_close(struct tty_struct *tty, struct tty_ldisc *ld)
}
/**
+ * tty_ldisc_failto - helper for ldisc failback
+ * @tty: tty to open the ldisc on
+ * @ld: ldisc we are trying to fail back to
+ *
+ * Helper to try and recover a tty when switching back to the old
+ * ldisc fails and we need something attached.
+ */
+
+static int tty_ldisc_failto(struct tty_struct *tty, int ld)
+{
+ struct tty_ldisc *disc = tty_ldisc_get(tty, ld);
+ int r;
+
+ if (IS_ERR(disc))
+ return PTR_ERR(disc);
+ tty->ldisc = disc;
+ tty_set_termios_ldisc(tty, ld);
+ if ((r = tty_ldisc_open(tty, disc)) < 0)
+ tty_ldisc_put(ld);
+ return r;
+}
+
+/**
* tty_ldisc_restore - helper for tty ldisc change
* @tty: tty to recover
* @old: previous ldisc
@@ -512,15 +535,14 @@ static void tty_ldisc_restore(struct tty_struct *tty, struct tty_ldisc *old)
tty_set_termios_ldisc(tty, old->ops->num);
if (tty_ldisc_open(tty, old) < 0) {
tty_ldisc_put(old);
- /* This driver is always present */
- new_ldisc = tty_ldisc_get(tty, N_TTY);
- if (IS_ERR(new_ldisc))
- panic("n_tty: get");
- tty->ldisc = new_ldisc;
- tty_set_termios_ldisc(tty, N_TTY);
- r = tty_ldisc_open(tty, new_ldisc);
+ /* The traditional behaviour is to fall back to N_TTY, we
+ want to avoid falling back to N_NULL unless we have no
+ choice to avoid the risk of breaking anything */
+ if (tty_ldisc_failto(tty, N_TTY) < 0 &&
+ tty_ldisc_failto(tty, N_NULL) < 0)
+ /* Fall back to null ldisc */
if (r < 0)
- panic("Couldn't open N_TTY ldisc for "
+ panic("Couldn't open N_NULL ldisc for "
"%s --- error %d.",
tty_name(tty), r);
}
diff --git a/include/uapi/linux/tty.h b/include/uapi/linux/tty.h
index e7855df..cf14553 100644
--- a/include/uapi/linux/tty.h
+++ b/include/uapi/linux/tty.h
@@ -36,5 +36,6 @@
#define N_TRACEROUTER 24 /* Trace data routing for MIPI P1149.7 */
#define N_NCI 25 /* NFC NCI UART */
#define N_SPEAKUP 26 /* Speakup communication with synths */
+#define N_NULL 27 /* Null ldisc used for error handling */
#endif /* _UAPI_LINUX_TTY_H */
^ permalink raw reply related
* [RFC, PATCH] imx: serial: Take tty->files_lock opportunistically
From: Andrey Smirnov @ 2017-05-30 12:37 UTC (permalink / raw)
To: linux-serial
Cc: Andrey Smirnov, cphealy, Peter Senna Tschudin, Greg Kroah-Hartman,
Jiri Slaby, linux-kernel
Trying to load a serdev driver agains a tty port on i.MX6Q results in
the following lockdep warning:
kworker/u8:1/100 [HC0[0]:SC0[0]:HE0:SE1] is trying to acquire:
(&(&tty->files_lock)->rlock){+.+...}, at: [<c04d3d4c>] imx_startup+0x2c0/0x50c
and this task is already holding:
(&port_lock_key){-.....}, at: [<c04d3b98>] imx_startup+0x10c/0x50c
which would create a new lock dependency:
(&port_lock_key){-.....} -> (&(&tty->files_lock)->rlock){+.+...}
but this new dependency connects a HARDIRQ-irq-safe lock:
(&port_lock_key){-.....}
... which became HARDIRQ-irq-safe at:
lock_acquire+0x74/0x94
_raw_spin_lock_irqsave+0x40/0x54
imx_txint+0x18/0x1d8
imx_int+0xc0/0x21c
__handle_irq_event_percpu+0x8c/0x124
handle_irq_event_percpu+0x24/0x60
handle_irq_event+0x40/0x64
handle_fasteoi_irq+0xd4/0x1ac
generic_handle_irq+0x28/0x3c
__handle_domain_irq+0x6c/0xe8
gic_handle_irq+0x58/0xb8
__irq_svc+0x70/0x98
cpuidle_enter_state+0x18c/0x2b8
cpuidle_enter+0x1c/0x20
call_cpuidle+0x28/0x44
do_idle+0x1b0/0x224
cpu_startup_entry+0x20/0x24
rest_init+0x128/0x168
start_kernel+0x328/0x39c
0x1000807c
to a HARDIRQ-irq-unsafe lock:
(&(&tty->files_lock)->rlock){+.+...}
... which became HARDIRQ-irq-unsafe at:
...
lock_acquire+0x74/0x94
_raw_spin_lock+0x30/0x40
tty_add_file+0x28/0x50
tty_open+0x9c/0x490
chrdev_open+0xa4/0x180
do_dentry_open+0x1f0/0x318
vfs_open+0x54/0x84
path_openat+0x32c/0xfbc
do_filp_open+0x68/0xcc
do_sys_open+0x108/0x1d0
SyS_open+0x20/0x24
kernel_init_freeable+0x15c/0x200
kernel_init+0x10/0x11c
ret_from_fork+0x14/0x24
other info that might help us debug this:
Possible interrupt unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&(&tty->files_lock)->rlock);
local_irq_disable();
lock(&port_lock_key);
lock(&(&tty->files_lock)->rlock);
<Interrupt>
lock(&port_lock_key);
*** DEADLOCK ***
In order to avoid this problem, change the code to opportunisticaly
take 'tty->files_lock' by using spin_trylock() in the offending
codepath instead.
Fixes: 18a4208826dd0a13eb06de724c86bba2c225f943 ("imx-serial: Reduce
RX DMA startup latency when opening for reading")
Cc: cphealy@gmail.com
Cc: Peter Senna Tschudin <peter.senna@collabora.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
Not sure if this is the best way to solve the problem (hence the RFC
tag). If anyone has a better idea, or if there's a better fix for this
already, please let me know.
Thanks,
Andrey Smirnov
drivers/tty/serial/imx.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 33509b4..24fe7fc 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1350,14 +1350,14 @@ static int imx_startup(struct uart_port *port)
struct tty_file_private *file_priv;
int readcnt = 0;
- spin_lock(&tty->files_lock);
+ if (spin_trylock(&tty->files_lock)) {
+ if (!list_empty(&tty->tty_files))
+ list_for_each_entry(file_priv, &tty->tty_files, list)
+ if (!(file_priv->file->f_flags & O_WRONLY))
+ readcnt++;
- if (!list_empty(&tty->tty_files))
- list_for_each_entry(file_priv, &tty->tty_files, list)
- if (!(file_priv->file->f_flags & O_WRONLY))
- readcnt++;
-
- spin_unlock(&tty->files_lock);
+ spin_unlock(&tty->files_lock);
+ }
if (readcnt > 0) {
imx_disable_rx_int(sport);
--
2.9.4
^ permalink raw reply related
* Re: [RFC, PATCH] imx: serial: Take tty->files_lock opportunistically
From: Rob Herring @ 2017-05-30 12:59 UTC (permalink / raw)
To: Andrey Smirnov
Cc: linux-serial@vger.kernel.org, Chris Healy, Peter Senna Tschudin,
Greg Kroah-Hartman, Jiri Slaby, linux-kernel@vger.kernel.org
In-Reply-To: <20170530123726.18598-1-andrew.smirnov@gmail.com>
On Tue, May 30, 2017 at 7:37 AM, Andrey Smirnov
<andrew.smirnov@gmail.com> wrote:
> Trying to load a serdev driver agains a tty port on i.MX6Q results in
> the following lockdep warning:
>
> kworker/u8:1/100 [HC0[0]:SC0[0]:HE0:SE1] is trying to acquire:
> (&(&tty->files_lock)->rlock){+.+...}, at: [<c04d3d4c>] imx_startup+0x2c0/0x50c
>
> and this task is already holding:
> (&port_lock_key){-.....}, at: [<c04d3b98>] imx_startup+0x10c/0x50c
> which would create a new lock dependency:
> (&port_lock_key){-.....} -> (&(&tty->files_lock)->rlock){+.+...}
>
> but this new dependency connects a HARDIRQ-irq-safe lock:
> (&port_lock_key){-.....}
>
> ... which became HARDIRQ-irq-safe at:
> lock_acquire+0x74/0x94
> _raw_spin_lock_irqsave+0x40/0x54
> imx_txint+0x18/0x1d8
> imx_int+0xc0/0x21c
> __handle_irq_event_percpu+0x8c/0x124
> handle_irq_event_percpu+0x24/0x60
> handle_irq_event+0x40/0x64
> handle_fasteoi_irq+0xd4/0x1ac
> generic_handle_irq+0x28/0x3c
> __handle_domain_irq+0x6c/0xe8
> gic_handle_irq+0x58/0xb8
> __irq_svc+0x70/0x98
> cpuidle_enter_state+0x18c/0x2b8
> cpuidle_enter+0x1c/0x20
> call_cpuidle+0x28/0x44
> do_idle+0x1b0/0x224
> cpu_startup_entry+0x20/0x24
> rest_init+0x128/0x168
> start_kernel+0x328/0x39c
> 0x1000807c
>
> to a HARDIRQ-irq-unsafe lock:
> (&(&tty->files_lock)->rlock){+.+...}
>
> ... which became HARDIRQ-irq-unsafe at:
> ...
> lock_acquire+0x74/0x94
> _raw_spin_lock+0x30/0x40
> tty_add_file+0x28/0x50
> tty_open+0x9c/0x490
> chrdev_open+0xa4/0x180
> do_dentry_open+0x1f0/0x318
> vfs_open+0x54/0x84
> path_openat+0x32c/0xfbc
> do_filp_open+0x68/0xcc
> do_sys_open+0x108/0x1d0
> SyS_open+0x20/0x24
> kernel_init_freeable+0x15c/0x200
> kernel_init+0x10/0x11c
> ret_from_fork+0x14/0x24
>
> other info that might help us debug this:
>
> Possible interrupt unsafe locking scenario:
>
> CPU0 CPU1
> ---- ----
> lock(&(&tty->files_lock)->rlock);
> local_irq_disable();
> lock(&port_lock_key);
> lock(&(&tty->files_lock)->rlock);
> <Interrupt>
> lock(&port_lock_key);
>
> *** DEADLOCK ***
>
> In order to avoid this problem, change the code to opportunisticaly
> take 'tty->files_lock' by using spin_trylock() in the offending
> codepath instead.
>
> Fixes: 18a4208826dd0a13eb06de724c86bba2c225f943 ("imx-serial: Reduce
> RX DMA startup latency when opening for reading")
>
> Cc: cphealy@gmail.com
> Cc: Peter Senna Tschudin <peter.senna@collabora.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jiri Slaby <jslaby@suse.com>
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
>
> Not sure if this is the best way to solve the problem (hence the RFC
> tag). If anyone has a better idea, or if there's a better fix for this
> already, please let me know.
IMO, the low level serial drivers shouldn't be accessing
tty->tty_files in the first place. Is being opened for write-only that
common and is skipping the DMA setup really necessary?
Rob
^ permalink raw reply
* Re: [RFC, PATCH] imx: serial: Take tty->files_lock opportunistically
From: Alan Cox @ 2017-05-30 13:42 UTC (permalink / raw)
To: Rob Herring
Cc: Andrey Smirnov, linux-serial@vger.kernel.org, Chris Healy,
Peter Senna Tschudin, Greg Kroah-Hartman, Jiri Slaby,
linux-kernel@vger.kernel.org
In-Reply-To: <CAL_JsqJUF8NhEhGY2FPPX5GUF3oP9XONQJy9sj42uPYATskz9A@mail.gmail.com>
> > Fixes: 18a4208826dd0a13eb06de724c86bba2c225f943 ("imx-serial: Reduce
> > RX DMA startup latency when opening for reading")
> >
> > Cc: cphealy@gmail.com
> > Cc: Peter Senna Tschudin <peter.senna@collabora.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Jiri Slaby <jslaby@suse.com>
> > Cc: linux-kernel@vger.kernel.org
> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> > ---
> >
> > Not sure if this is the best way to solve the problem (hence the RFC
> > tag). If anyone has a better idea, or if there's a better fix for this
> > already, please let me know.
>
> IMO, the low level serial drivers shouldn't be accessing
> tty->tty_files in the first place. Is being opened for write-only that
> common and is skipping the DMA setup really necessary?
Seconded - the Reduce RX DMA startup latency patch should just be
reverted (and shouldn't ever IMHO have gotten in).
Not all readers and writers to a tty have a file handle any more anyway,
so it's not only icky and layer violating it's fundamentally broken
beyond the locking.
Alan
^ permalink raw reply
* Re: [RFC, PATCH] imx: serial: Take tty->files_lock opportunistically
From: Peter Senna Tschudin @ 2017-05-30 13:44 UTC (permalink / raw)
To: Alan Cox
Cc: Rob Herring, Andrey Smirnov, linux-serial@vger.kernel.org,
Chris Healy, Greg Kroah-Hartman, Jiri Slaby,
linux-kernel@vger.kernel.org
In-Reply-To: <20170530144213.48df2a1c@alans-desktop>
On Tue, May 30, 2017 at 02:42:13PM +0100, Alan Cox wrote:
I sent a second patch recently:
https://patchwork.kernel.org/patch/9725625/
> > > Fixes: 18a4208826dd0a13eb06de724c86bba2c225f943 ("imx-serial: Reduce
> > > RX DMA startup latency when opening for reading")
> > >
> > > Cc: cphealy@gmail.com
> > > Cc: Peter Senna Tschudin <peter.senna@collabora.com>
> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Cc: Jiri Slaby <jslaby@suse.com>
> > > Cc: linux-kernel@vger.kernel.org
> > > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> > > ---
> > >
> > > Not sure if this is the best way to solve the problem (hence the RFC
> > > tag). If anyone has a better idea, or if there's a better fix for this
> > > already, please let me know.
> >
> > IMO, the low level serial drivers shouldn't be accessing
> > tty->tty_files in the first place. Is being opened for write-only that
> > common and is skipping the DMA setup really necessary?
>
> Seconded - the Reduce RX DMA startup latency patch should just be
> reverted (and shouldn't ever IMHO have gotten in).
>
> Not all readers and writers to a tty have a file handle any more anyway,
> so it's not only icky and layer violating it's fundamentally broken
> beyond the locking.
>
> Alan
^ permalink raw reply
* Re: [PATCH v8 net-next 00/17] net: qualcomm: add QCA7000 UART driver
From: David Miller @ 2017-05-30 18:03 UTC (permalink / raw)
To: stefan.wahren-eS4NqCHxEME
Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r, jslaby-IBi9RG/b67k,
LinoSanfilippo-Mmb7MZpHnFY, kubakici-5tc4TXWwyLM,
devicetree-u79uwXL29TY76Z2rM5mHXA, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-serial-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1496059045-13572-1-git-send-email-stefan.wahren-eS4NqCHxEME@public.gmane.org>
From: Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org>
Date: Mon, 29 May 2017 13:57:08 +0200
> The Qualcomm QCA7000 HomePlug GreenPHY supports two interfaces:
> UART and SPI. This patch series adds the missing support for UART.
>
> This driver based on the Qualcomm code [1], but contains some changes:
> * use random MAC address per default
> * use net_device_stats from device
> * share frame decoding between SPI and UART driver
> * improve error handling
> * reimplement tty_wakeup with work queue (based on slcan)
> * use new serial device bus instead of ldisc
>
> The patches 1 - 3 are just for clean up and are not related to
> the UART support. Patch 4 adds SET_NETDEV_DEV() to qca_spi.
> Patches 5 - 16 prepare the existing QCA7000 code for UART support.
> The last patch contains the new driver.
>
> The code itself has been tested on a Freescale i.MX28 board and
> a Raspberry Pi Zero.
Series applied, thank you.
--
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 v4 2/9] gpio-exar/8250-exar: Do not even instantiate a GPIO device for Commtech cards
From: Andy Shevchenko @ 2017-05-30 18:33 UTC (permalink / raw)
To: Jan Kiszka
Cc: Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot,
Linux Kernel Mailing List, linux-serial@vger.kernel.org,
linux-gpio@vger.kernel.org, Sudip Mukherjee, Sascha Weisenberger
In-Reply-To: <f7e7da1ae81aab7f53faa32260d4ee0f930048fe.1496127684.git.jan.kiszka@siemens.com>
On Tue, May 30, 2017 at 10:01 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> Commtech adapters need the MPIOs for internal purposes, and the
> gpio-exar driver already refused to pick them up. But there is actually
> no point in even creating the underlying platform device.
It still feels that partially you may do stuff here, like
renaming to
__xr17v35x_register_gpio()
and creating
xr17v35x_register_gpio() wrapper.
> diff --git a/drivers/gpio/gpio-exar.c b/drivers/gpio/gpio-exar.c
> index 081076771217..006a9a67c2a4 100644
> --- a/drivers/gpio/gpio-exar.c
> +++ b/drivers/gpio/gpio-exar.c
> @@ -124,9 +124,6 @@ static int gpio_exar_probe(struct platform_device *pdev)
> void __iomem *p;
> int index, ret;
>
> - if (pcidev->vendor != PCI_VENDOR_ID_EXAR)
> - return -ENODEV;
> -
> /*
> * Map the pci device to get the register addresses.
> * We will need to read and write those registers to control
> diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c
> index 8984e8b2d524..c29c7e675890 100644
> --- a/drivers/tty/serial/8250/8250_exar.c
> +++ b/drivers/tty/serial/8250/8250_exar.c
> @@ -245,7 +245,9 @@ pci_xr17v35x_setup(struct exar8250 *priv, struct pci_dev *pcidev,
> /* Setup Multipurpose Input/Output pins. */
> setup_gpio(pcidev, p);
>
> - port->port.private_data = xr17v35x_register_gpio(pcidev);
> + if (pcidev->vendor == PCI_VENDOR_ID_EXAR)
> + port->port.private_data =
> + xr17v35x_register_gpio(pcidev);
> }
>
> return 0;
> --
> 2.12.3
>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v4 8/9] gpio-exar/8250-exar: Make set of exported GPIOs configurable
From: Andy Shevchenko @ 2017-05-30 18:35 UTC (permalink / raw)
To: Jan Kiszka
Cc: Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot,
Linux Kernel Mailing List, linux-serial@vger.kernel.org,
linux-gpio@vger.kernel.org, Sudip Mukherjee, Sascha Weisenberger
In-Reply-To: <0dc397840e180777e412158f8e7057c313e75b34.1496127684.git.jan.kiszka@siemens.com>
On Tue, May 30, 2017 at 10:01 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On the SIMATIC, IOT2040 only a single pin is exportable as GPIO, the
> rest is required to operate the UART. To allow modeling this case,
> expand the platform device data structure to specify a (consecutive) pin
> subset for exporting by the gpio-exar driver.
> static void *
> -xr17v35x_register_gpio(struct pci_dev *pcidev)
> +xr17v35x_register_gpio(struct pci_dev *pcidev, unsigned int first_pin,
> + unsigned int ngpios)
I would rather to provide properties here as a parameter...
> {
> + struct property_entry properties[] = {
> + PROPERTY_ENTRY_U32("gpio-exar,first-pin", first_pin),
> + PROPERTY_ENTRY_U32("ngpios", ngpios),
> + { }
> + };
> if (pcidev->vendor == PCI_VENDOR_ID_EXAR)
...while defining them somewhere here.
> port->port.private_data =
> - xr17v35x_register_gpio(pcidev);
> + xr17v35x_register_gpio(pcidev, 0, 16);
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v4 9/9] serial: exar: Add support for IOT2040 device
From: Andy Shevchenko @ 2017-05-30 18:38 UTC (permalink / raw)
To: Jan Kiszka
Cc: Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot,
Linux Kernel Mailing List, linux-serial@vger.kernel.org,
linux-gpio@vger.kernel.org, Sudip Mukherjee, Sascha Weisenberger
In-Reply-To: <86bb088c2f8530930f68cc236f16569ac7be160b.1496127684.git.jan.kiszka@siemens.com>
On Tue, May 30, 2017 at 10:01 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> This implements the setup of RS232 and the switch-over to RS485 or RS422
> for the Siemens IOT2040. That uses an EXAR XR17V352 with external logic
> to switch between the different modes. The external logic is controlled
> via MPIO pins of the EXAR controller.
>
> Only pin 10 can be exported as GPIO on the IOT2040. It is connected to
> an LED.
>
> As the XR17V352 used on the IOT2040 is not equipped with an external
> EEPROM, it cannot present itself as IOT2040-variant via subvendor/
> subdevice IDs. Thus, we have to check via DMI for the target platform.
Take into consideration comments per patch 2.
Otherwise looks good to me.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v4 0/9] serial/gpio: exar: Fixes and support for IOT2000
From: Andy Shevchenko @ 2017-05-30 18:39 UTC (permalink / raw)
To: Jan Kiszka
Cc: Greg Kroah-Hartman, Linus Walleij, Alexandre Courbot,
Linux Kernel Mailing List, linux-serial@vger.kernel.org,
linux-gpio@vger.kernel.org, Sudip Mukherjee, Sascha Weisenberger
In-Reply-To: <cover.1496127684.git.jan.kiszka@siemens.com>
On Tue, May 30, 2017 at 10:01 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> This makes the gpio-exar driver usable, which was prevented by a number
> of fatal bugs, and adds support for the SIMATIC IOT2040 to the 8250-exar
> driver and, indirectly, to gpio-exar as well. It's a cross-subsystem
> series, so I'm also cross-posting to the serial and gpio lists.
Few comments here and there, overall looks good.
After addressing them, take my
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
to patches that are missing it.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v3 00/10] serial/gpio: exar: Fixes and support for IOT2000
From: Linus Walleij @ 2017-05-31 0:01 UTC (permalink / raw)
To: Jan Kiszka
Cc: Greg Kroah-Hartman, Alexandre Courbot, Linux Kernel Mailing List,
linux-serial@vger.kernel.org, linux-gpio@vger.kernel.org,
Sudip Mukherjee, Andy Shevchenko, Sascha Weisenberger
In-Reply-To: <835df887-2782-29f4-dfc9-a0fbbbb3019f@siemens.com>
On Mon, May 29, 2017 at 3:41 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2017-05-29 14:39, Linus Walleij wrote:
>> Should I merge them all? Should Greg merge them all?
>> Should one of us produce an immutable branch?
>
> Half of the patch are affecting both serial and GPIO subsystem, most
> have GPIO focus, though. Just patch 10 or more serial than GPIO. All in
> all, maybe Greg can ack and things can flow through the GPIO tree?
OK with me, let's see what Greg says.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit register support
From: Dong Aisheng @ 2017-05-31 7:47 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Nikita Yushchenko, Dong Aisheng, linux-serial@vger.kernel.org,
fugang.duan, Greg Kroah-Hartman, yangbo.lu,
linux-kernel@vger.kernel.org, Stefan Agner, Mingkai.Hu,
Jiri Slaby, linux-arm Mailing List
In-Reply-To: <CAHp75VdL=rGHuupGmgmAVonEQgpyPRgoMQrZK0yQiaM=1Vac-g@mail.gmail.com>
Hi Andy,
On Wed, May 17, 2017 at 12:55:59PM +0300, Andy Shevchenko wrote:
> On Wed, May 17, 2017 at 8:43 AM, Dong Aisheng <dongas86@gmail.com> wrote:
> > On Wed, May 17, 2017 at 08:37:41AM +0300, Nikita Yushchenko wrote:
> >>
> >>
> >> 17.05.2017 06:39, Dong Aisheng wrote:
> >> > On Tue, May 16, 2017 at 02:15:08PM +0300, Nikita Yushchenko wrote:
> >> >>> static u32 lpuart32_read(void __iomem *addr)
> >> >>> {
> >> >>> - return ioread32be(addr);
> >> >>> + return lpuart_is_be ? ioread32be(addr) : readl(addr);
> >> >>> }
> >> >>>
> >> >>> static void lpuart32_write(u32 val, void __iomem *addr)
> >> >>> {
> >> >>> - iowrite32be(val, addr);
> >> >>> + if (lpuart_is_be)
> >> >>> + iowrite32be(val, addr);
> >> >>> + else
> >> >>> + writel(val, addr);
> >> >>> }
> >> >>
> >> >> What if this is ever executed on big endian system?
> >> >>
> >> >
> >> > Sorry, not catching the point...
> >> >
> >> > What issues will meet?
> >>
> >> Isn't writel() in host endian?
> >
> > On big endian systems, it is supposed to run iowrite32be.
>
> It looks like you substituting *bus* side with CPU *side* of communication.
>
> If you are talking about CPU side
> __raw_readl() / __raw_writel() will do the trick.
>
Yes, you're right.
To be more accurate, for bus side, if it's BE lpuart it should use
ioread32be/iowrite32be. If it's LE lpuart, it should use readl/writel.
And for CPU side, endian is transparent to driver users as they're
hided in io accessors implementation.
Regards
Dong Aisheng
^ permalink raw reply
* Re: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit register support
From: Dong Aisheng @ 2017-05-31 8:07 UTC (permalink / raw)
To: Nikita Yushchenko
Cc: A.S. Dong, linux-serial@vger.kernel.org, Andy Duan,
gregkh@linuxfoundation.org, Y.B. Lu, linux-kernel@vger.kernel.org,
stefan@agner.ch, Mingkai Hu, jslaby@suse.com,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <2f873a62-0e4e-7da6-5c5e-be570fcceff1@cogentembedded.com>
Hi Nikita,
On Tue, May 23, 2017 at 08:24:18AM +0300, Nikita Yushchenko wrote:
> Hi,
>
> >>>> Alternative solution could be - have separate write path for earlycon.
> >>>
> >>> It looks to me having the same issue with a separate write patch
> >>> for earlycon as we still need distinguish Little or Big endian
> >>> for Layerscape and IMX.
> >>>
> >>>> At a glance, it is dozen lines of code.
> >>>
> >>> Would you please show some sample code?
> >>
> >> Do not reuse lpuart32_console_putchar() in earlycon code.
> >>
> >> Have two sets of early_setup/early_write/putchar - for BE and
> >> defaut-endian earlycon. And in these putchar's do not use
> >> lpuart_(read|write).
> >>
> >
> > Isn't that introducing another consistency break after fix one
> > consistency break?
> >
> > If doing that, we then have two register read/write APIs.
> > One for normal driver operation by dynamically checking lpuart_is_be
> > property to distinguish the endian difference problem.
> > Another is specifically implemented for only early console read/write
> > and use hardcoded way to read/write register directly instead of using
> > the standard API lpuart32_read/write, like follows:
> > e.g.
> > lpuart32_le_console_write() {
> > writel();
> > }
> >
> > lpuart32_be_console_write() {
> > iowrite32be()
> > }
> > This also makes the driver a bit strange and ugly.
> >
> > It looks to me both way are trade offs and the later one seems sacrifice
> > more. And i doubt if it's really necessary for probably a no real gain
> > purpose as the FPGA you mentioned is a theoretical case and less
> > possibility to exist.
> >
> > I'm still wondering how about keep using the exist way and adding more
> > information in code to explain why use a global var?
>
> I've checked other driver under drivers/tty/serial/, for examples of
> similar cases.
>
> Please look at serial8250_early_in() / serial8250_early_out() ?
> These do handle different endian, via port->iotype
>
Well, that does inspire me.
With struct uart_port's iotype member, the global lpuart_is_be can be
gone. We can update lpuart32_read/write API to take the reference of
of structure uart_port, then the API knows the endian information and
can use the correct further IO accessor accordingly.
And most importantly, it also works with earlycon.
> Another example is drivers/tty/serial/samsung.c, where
> port->private_data is initialized and used.
>
If using iotype, seems no need private_data anymore.
Will try and send the new series later with you CCed to help review.
Thanks for the advice.
Regards
Dong Aisheng
^ permalink raw reply
* Re: [GIT PULL] TTY/Serial driver fixes for 4.11-rc4
From: Dmitry Vyukov @ 2017-05-31 8:39 UTC (permalink / raw)
To: Alan Cox
Cc: Greg KH, Vegard Nossum, Linus Torvalds, Jiri Slaby, Andrew Morton,
LKML, linux-serial
In-Reply-To: <20170530130905.382487d5@alans-desktop>
On Tue, May 30, 2017 at 2:09 PM, Alan Cox <gnomes@lxorguk.ukuu.org.uk> wrote:
>> >> I'll think about possible solutions, but I have no prior experience
>> >> with the tty code. In the meantime syzkaller also hit a couple of
>> >> other fun tty/pty bugs including a write/ioctl race that results in
>> >> buffer overflow :-/
>
> There are several of those, including some of that have been documented
> for years but nobody ever volunteered to fix - in particular all the
> interfaces that push characters to the tty other than via the normal
> interrupt receive path are dodgy (console selection in particular)
>
> The original tty model btw was that setting the ldisc to n_tty cannot
> fail, and the structure allocated was smaller than a page size so was
> safe.
>
> The simple way to fix it is to restore that behaviour by adding a 'null'
> ldisc that we can fail to instead of N_TTY since the N_TTY failback path
> is long broken.
Greg, what do you think about this patch? Are you ready to accept
something like this?
Definitely shorter than changing all drivers.
> Something like this (untested)
>
> commit 797035eaf800889287b0b176a11c89c0f1fbba30
> Author: Alan Cox <alan@llwyncelyn.cymru>
> Date: Tue May 30 12:59:45 2017 +0100
>
> tty: handle the case where we cannot restore a line discipline
>
> Historically the N_TTY driver could never fail but this has become broken over
> time. Rather than trying to rewrite half the ldisc layer to fix the breakage
> introduce a second level of fallback with an N_NULL ldisc which cannot fail,
> and thus restore the guarantees required by the ldisc layer.
>
> We still try and fail to N_TTY first. It's much more useful to find yourself
> back in your old ldisc (first attempt) or in N_TTY (second attempt), and while
> I'm not aware of any code out there that makes those assumptions it's good to
> drive(r) defensively.
>
> No signed off by: this is just a proposal!
>
> diff --git a/drivers/tty/n_null.c b/drivers/tty/n_null.c
> new file mode 100644
> index 0000000..c5812cd
> --- /dev/null
> +++ b/drivers/tty/n_null.c
> @@ -0,0 +1,67 @@
> +/*
> + * n_null.c - Null line discipline used in the failure path
> + *
> + * Copyright (C) Intel 2017
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + *
> + * 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.
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + */
> +
> +static int n_null_open(struct tty_struct *tty)
> +{
> + return 0;
> +}
> +
> +static void n_null_close(struct tty_struct *tty)
> +{
> +}
> +
> +static ssize_t n_null_read(struct tty_struct *tty, struct file *file,
> + unsigned char __user * buf, size_t nr)
> +{
> + return -EOPNOTSUPP;
> +}
> +
> +static ssize_t n_null_write(struct tty_struct *tty, struct file *file,
> + const unsigned char *buf, size_t nr)
> +{
> + return -EOPNOTSUPP;
> +}
> +
> +static ssize_t n_null_receivebuf(struct tty_struct *tty,
> + const unsigned char *cp, char *fp,
> + int cnt)
> +{
> +}
> +
> +static struct tty_ldisc_ops null_ldisc {
> + .owner = THIS_MODULE,
> + .magic = TTY_LDISC_MAGIC,
> + .name = "n_null",
> + .open = n_null_open,
> + .close = n_null_close,
> + .read = n_null_read,
> + .write = n_null_write,
> + .receive_buf = n_null_receivebuf
> +};
> +
> +static int __init n_null_init(void)
> +{
> + BUG_ON(tty_register_ldisc(N_NULL, &null_ldisc));
> + return 0;
> +}
> +
> +static void __exit n_null_exit(void)
> +{
> + tty_unregister_ldisc(N_NULL);
> +}
> diff --git a/drivers/tty/tty_ldisc.c b/drivers/tty/tty_ldisc.c
> index f6ffe28..e80e05f 100644
> --- a/drivers/tty/tty_ldisc.c
> +++ b/drivers/tty/tty_ldisc.c
> @@ -492,6 +492,29 @@ static void tty_ldisc_close(struct tty_struct *tty, struct tty_ldisc *ld)
> }
>
> /**
> + * tty_ldisc_failto - helper for ldisc failback
> + * @tty: tty to open the ldisc on
> + * @ld: ldisc we are trying to fail back to
> + *
> + * Helper to try and recover a tty when switching back to the old
> + * ldisc fails and we need something attached.
> + */
> +
> +static int tty_ldisc_failto(struct tty_struct *tty, int ld)
> +{
> + struct tty_ldisc *disc = tty_ldisc_get(tty, ld);
> + int r;
> +
> + if (IS_ERR(disc))
> + return PTR_ERR(disc);
> + tty->ldisc = disc;
> + tty_set_termios_ldisc(tty, ld);
> + if ((r = tty_ldisc_open(tty, disc)) < 0)
> + tty_ldisc_put(ld);
> + return r;
> +}
> +
> +/**
> * tty_ldisc_restore - helper for tty ldisc change
> * @tty: tty to recover
> * @old: previous ldisc
> @@ -512,15 +535,14 @@ static void tty_ldisc_restore(struct tty_struct *tty, struct tty_ldisc *old)
> tty_set_termios_ldisc(tty, old->ops->num);
> if (tty_ldisc_open(tty, old) < 0) {
> tty_ldisc_put(old);
> - /* This driver is always present */
> - new_ldisc = tty_ldisc_get(tty, N_TTY);
> - if (IS_ERR(new_ldisc))
> - panic("n_tty: get");
> - tty->ldisc = new_ldisc;
> - tty_set_termios_ldisc(tty, N_TTY);
> - r = tty_ldisc_open(tty, new_ldisc);
> + /* The traditional behaviour is to fall back to N_TTY, we
> + want to avoid falling back to N_NULL unless we have no
> + choice to avoid the risk of breaking anything */
> + if (tty_ldisc_failto(tty, N_TTY) < 0 &&
> + tty_ldisc_failto(tty, N_NULL) < 0)
> + /* Fall back to null ldisc */
> if (r < 0)
> - panic("Couldn't open N_TTY ldisc for "
> + panic("Couldn't open N_NULL ldisc for "
> "%s --- error %d.",
> tty_name(tty), r);
> }
> diff --git a/include/uapi/linux/tty.h b/include/uapi/linux/tty.h
> index e7855df..cf14553 100644
> --- a/include/uapi/linux/tty.h
> +++ b/include/uapi/linux/tty.h
> @@ -36,5 +36,6 @@
> #define N_TRACEROUTER 24 /* Trace data routing for MIPI P1149.7 */
> #define N_NCI 25 /* NFC NCI UART */
> #define N_SPEAKUP 26 /* Speakup communication with synths */
> +#define N_NULL 27 /* Null ldisc used for error handling */
>
> #endif /* _UAPI_LINUX_TTY_H */
^ 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