| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <a83b8550-6a1e-f156-2eb8-e6b62d87c425@camlingroup.com>
Date: Fri, 19 May 2023 18:56:00 +0200
From: Lech Perczak <lech.perczak@...lingroup.com>
To: Hugo Villeneuve <hugo@...ovil.com>
CC: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Jiri Slaby <jirislaby@...nel.org>,
Lech Perczak <l.perczak@...lintechnologies.com>,
Tomasz Moń <tomasz.mon@...lingroup.com>,
linux-gpio@...r.kernel.org,
Hugo Villeneuve <hvilleneuve@...onoff.com>,
linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH v2 2/2] serial: sc16is7xx: fix regression with GPIO
configuration
W dniu 19.05.2023 o 18:38, Hugo Villeneuve pisze:
> On Fri, 19 May 2023 18:00:54 +0200
> Lech Perczak <lech.perczak@...lingroup.com> wrote:
>
>> W dniu 19.05.2023 o 15:30, Hugo Villeneuve pisze:
>>> On Fri, 19 May 2023 15:00:58 +0200
>>> Lech Perczak <lech.perczak@...lingroup.com> wrote:
>>>
>>>> Hello Hugo,
>>>>
>>>> A couple of remarks inline.
>>>>
>>>> W dniu 18.05.2023 o 15:29, Hugo Villeneuve pisze:
>>>>> From: Hugo Villeneuve <hvilleneuve@...onoff.com>
>>>>>
>>>>> Commit 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")
>>>>> and commit 21144bab4f11 ("sc16is7xx: Handle modem status lines")
>>>>> changed the function of the GPIOs pins to act as modem control
>>>>> lines without any possibility of selecting GPIO function.
>>>>>
>>>>> As a consequence, applications that depends on GPIO lines configured
>>>>> by default as GPIO pins no longer work as expected.
>>>>>
>>>>> Also, the change to select modem control lines function was done only
>>>>> for channel A of dual UART variants (752/762). This was not documented
>>>>> in the log message.
>>>>>
>>>>> This new patch allows to specify GPIO or modem control line function
>>>>> in the device tree, and for each of the ports (A or B).
>>>>>
>>>>> This is done by using the new device-tree property named
>>>>> "modem-control-line-ports" (property added in separate patch).
>>>>>
>>>>> Boards that need to have GPIOS configured as modem control lines
>>>>> should add that property to their device tree. Here is a list of
>>>>> boards using the sc16is7xx driver in their device tree and that may
>>>>> need to be modified:
>>>>> arm64/boot/dts/freescale/fsl-ls1012a-frdm.dts
>>>>> mips/boot/dts/ingenic/cu1830-neo.dts
>>>>> mips/boot/dts/ingenic/cu1000-neo.dts
>>>>>
>>>>> Fixes: 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control lines")
>>>>> Fixes: 21144bab4f11 ("sc16is7xx: Handle modem status lines")
>>>>> Signed-off-by: Hugo Villeneuve <hvilleneuve@...onoff.com>
>>>>> ---
>>>>> v1 -> v2: Rebase because original patch did not apply properly
>>>>> Add git base-commit
>>>>>
>>>>> drivers/tty/serial/sc16is7xx.c | 42 ++++++++++++++++++++--------------
>>>>> 1 file changed, 25 insertions(+), 17 deletions(-)
>>>>>
>>>>> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
>>>>> index abad091baeea..4e3d2325ef6e 100644
>>>>> --- a/drivers/tty/serial/sc16is7xx.c
>>>>> +++ b/drivers/tty/serial/sc16is7xx.c
>>>>> @@ -236,7 +236,8 @@
>>>>>
>>>>> /* IOControl register bits (Only 750/760) */
>>>>> #define SC16IS7XX_IOCONTROL_LATCH_BIT (1 << 0) /* Enable input latching */
>>>>> -#define SC16IS7XX_IOCONTROL_MODEM_BIT (1 << 1) /* Enable GPIO[7:4] as modem pins */
>>>>> +#define SC16IS7XX_IOCONTROL_MODEM_A_BIT (1 << 1) /* Enable GPIO[7:4] as modem A pins */
>>>>> +#define SC16IS7XX_IOCONTROL_MODEM_B_BIT (1 << 2) /* Enable GPIO[3:0] as modem B pins */
>>>>> #define SC16IS7XX_IOCONTROL_SRESET_BIT (1 << 3) /* Software Reset */
>>>>>
>>>>> /* EFCR register bits */
>>>>> @@ -306,7 +307,6 @@ struct sc16is7xx_devtype {
>>>>> char name[10];
>>>>> int nr_gpio;
>>>>> int nr_uart;
>>>>> - int has_mctrl;
>>>>> };
>>>>>
>>>>> #define SC16IS7XX_RECONF_MD (1 << 0)
>>>>> @@ -447,35 +447,30 @@ static const struct sc16is7xx_devtype sc16is74x_devtype = {
>>>>> .name = "SC16IS74X",
>>>>> .nr_gpio = 0,
>>>>> .nr_uart = 1,
>>>>> - .has_mctrl = 0,
>>>>> };
>>>>>
>>>>> static const struct sc16is7xx_devtype sc16is750_devtype = {
>>>>> .name = "SC16IS750",
>>>>> - .nr_gpio = 4,
>>>>> + .nr_gpio = 8,
>>>>> .nr_uart = 1,
>>>>> - .has_mctrl = 1,
>>>>> };
>>>>>
>>>>> static const struct sc16is7xx_devtype sc16is752_devtype = {
>>>>> .name = "SC16IS752",
>>>>> - .nr_gpio = 0,
>>>>> + .nr_gpio = 8,
>>>>> .nr_uart = 2,
>>>>> - .has_mctrl = 1,
>>>>> };
>>>>>
>>>>> static const struct sc16is7xx_devtype sc16is760_devtype = {
>>>>> .name = "SC16IS760",
>>>>> - .nr_gpio = 4,
>>>>> + .nr_gpio = 8,
>>>>> .nr_uart = 1,
>>>>> - .has_mctrl = 1,
>>>>> };
>>>>>
>>>>> static const struct sc16is7xx_devtype sc16is762_devtype = {
>>>>> .name = "SC16IS762",
>>>>> - .nr_gpio = 0,
>>>>> + .nr_gpio = 8,
>>>>> .nr_uart = 2,
>>>>> - .has_mctrl = 1,
>>>>> };
>>>>>
>>>>> static bool sc16is7xx_regmap_volatile(struct device *dev, unsigned int reg)
>>>>> @@ -1456,12 +1451,6 @@ static int sc16is7xx_probe(struct device *dev,
>>>>> SC16IS7XX_EFCR_RXDISABLE_BIT |
>>>>> SC16IS7XX_EFCR_TXDISABLE_BIT);
>>>>>
>>>>> - /* Use GPIO lines as modem status registers */
>>>>> - if (devtype->has_mctrl)
>>>>> - sc16is7xx_port_write(&s->p[i].port,
>>>>> - SC16IS7XX_IOCONTROL_REG,
>>>>> - SC16IS7XX_IOCONTROL_MODEM_BIT);
>>>>> -
>>>>> /* Initialize kthread work structs */
>>>>> kthread_init_work(&s->p[i].tx_work, sc16is7xx_tx_proc);
>>>>> kthread_init_work(&s->p[i].reg_work, sc16is7xx_reg_proc);
>>>>> @@ -1497,6 +1486,25 @@ static int sc16is7xx_probe(struct device *dev,
>>>>> prop, p, u)
>>>>> if (u < devtype->nr_uart)
>>>>> s->p[u].irda_mode = true;
>>>>> +
>>>>> + val = 0;
>>>>> +
>>>>> + of_property_for_each_u32(dev->of_node, "nxp,modem-control-line-ports",
>>>>> + prop, p, u)
>>>> The general idea looks solid to me.
>>>> I think, that with this property set, we should also reduce number of GPIOs exported by gpiochip.
>>> OK, I will look into that.
>>>
>>>>> + if (u < devtype->nr_uart) {
>>>>> + /* Use GPIO lines as modem control lines */
>>>>> + if (u == 0)
>>>>> + val |= SC16IS7XX_IOCONTROL_MODEM_A_BIT;
>>>>> + else if (u == 1)
>>>>> + val |= SC16IS7XX_IOCONTROL_MODEM_B_BIT;
>>>>> + }
>>>>> +
>>>>> + if (val)
>>>>> + regmap_update_bits(
>>>>> + s->regmap,
>>>>> + SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT,
>>>>> + SC16IS7XX_IOCONTROL_MODEM_A_BIT |
>>>>> + SC16IS7XX_IOCONTROL_MODEM_B_BIT, val);
>>>> I just had a chance to test this on my hardware (with SC16IS760), and something still doesn't work.
>>>> When looking into /sys/kernel/debug/regmap/<device>/registers,
>>>> it seems like the proper bits are set, also taking the regmap shift into account.
>>>> For reference - I did my test basing on v6.4-rc2.
>>>>
>>>> I used a loopback plug and statserial to perform the test.
>>>> Without your patch, when DTR is set, all other pins (DCD, DSR, RI) are set. With it - they are cleared, according to statserial.
>>> It is strange, because the only conceptual change is the moment in time when we set the IOControl register. Before, IOControl was set within the for/loop for initializing each port. Now it is done after.
>>>
>>> Maybe you could try to put the section that reads the DT property just after resetting the device.
>>>
>>> While suggesting this, I just noticed that when the device is reset, we do not wait before initiating communication with the device. Thefore, I would suggest the following change (that I will submit as a separate patch):
>>>
>>> regmap_write(s->regmap, SC16IS7XX_IOCONTROL_REG << SC16IS7XX_REG_SHIFT,
>>> SC16IS7XX_IOCONTROL_SRESET_BIT);
>>>
>>> + /* After reset, the host must wait at least 3us before initializing a
>>> + * communication with the device: */
>>> + usleep_range(3, 5);
>>>
>>> Then like I said, insert the code that read the modem-control-line-port DT property right after that to see if it helps.
>>>
>>> Hugo.
>> Hello,
>>
>> I tried those suggestions, together with the patch you mentioned in the other email - all to no avail.
>> I added a debug print to confirm, that the property is detected, and regmap access is executed - and it is. This is so strange.
>> Maybe the offsets in regmap are still wrong, for some reason?
>> I also double checked if it was possible to alter and check GPIO states through GPIO subsystem - it is.
> Then it seems that the remaining difference is the number of GPIOs that are configured, which for the moment is always the value set in constant nr_gpio (for each device).
>
> I already implemented and tested your suggestion of lowering the number of GPIOs exported when using some pins as modem status lines. It seems to be working good, altough I have only tested that the correct number of GPIOs are now exported.
>
> I will send you the test patch and let me know if it helps.
>
> Hugo.
Hello Hugo,
I applied the patch marking IOControl register as volatile - and this one fixed the issue. Modem line states are now reported correctly.
I think it'll be a good idea to bundle all the fixes in the series, when they become ready for submission.
--
Pozdrawiam/With kind regards,
Lech Perczak
Sr. Software Engineer
Camlin Technologies Poland Limited Sp. z o.o.
Strzegomska 54,
53-611 Wroclaw
Tel: (+48) 71 75 000 16
Email: lech.perczak@...lingroup.com
Website: http://www.camlingroup.com
Powered by blists - more mailing lists