| 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: <20230516115906.9d93685696ae7dc02faff752@hugovil.com>
Date: Tue, 16 May 2023 11:59:06 -0400
From: Hugo Villeneuve <hugo@...ovil.com>
To: Lech Perczak <lech.perczak@...lingroup.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>,
Hugo Villeneuve <hvilleneuve@...onoff.com>,
linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org,
Krzysztof Drobiński
<krzysztof.drobinski@...lingroup.com>, hugo@...ovil.com
Subject: Re: [RFC PATCH] Revert
"sc16is7xx: Separate GPIOs from modem control lines"
On Tue, 16 May 2023 10:50:11 +0200
Lech Perczak <lech.perczak@...lingroup.com> wrote:
> Hi Hugo,
>
> Please see my answers inline.
>
> W dniu 15.05.2023 o 18:51, Hugo Villeneuve pisze:
> > Hi Greg,
> >
> > On Mon, 15 May 2023 18:20:02 +0200
> > Greg Kroah-Hartman <gregkh@...uxfoundation.org> wrote:
> >
> >> On Mon, May 15, 2023 at 12:02:07PM -0400, Hugo Villeneuve wrote:
> >>> From: Hugo Villeneuve <hvilleneuve@...onoff.com>
> >>>
> >>> This reverts commit 679875d1d8802669590ef4d69b0e7d13207ebd61.
> >>>
> >>> Because of this commit, it is no longer possible to use the 16 GPIO
> >>> lines as dedicated GPIOs on the SC16IS752.
> >>>
> >>> Reverting it makes it work again.
> >>>
> >>> The log message of the original commit states:
> >>> "Export only the GPIOs that are not shared with hardware modem
> >>> control lines"
> >>>
> >>> But there is no explanation as to why this decision was taken to
> >>> permanently set the function of the GPIO lines as modem control
> >>> lines. AFAIK, there is no problem with using these lines as GPIO or modem
> >>> control lines.
> >>>
> >>> Maybe after reverting this commit, we could define a new
> >>> device-tree property named, for example,
> >>> "use-modem-control-lines", so that both options can be supported.
> >>>
> >>> Fixes: 679875d1d880 ("sc16is7xx: Separate GPIOs from modem control
> >>> lines")
> >> Please do not line-wrap these lines.
> > Ok.
> >
> >> Nor is a blank line needed here.
> > Ok.
> >
> >>> Signed-off-by: Hugo Villeneuve <hvilleneuve@...onoff.com>
> >>> ---
> >>> drivers/tty/serial/sc16is7xx.c | 14 ++++----------
> >>> 1 file changed, 4 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> >>> index 5bd98e4316f5..25f1b2f6ec51 100644
> >>> --- a/drivers/tty/serial/sc16is7xx.c
> >>> +++ b/drivers/tty/serial/sc16is7xx.c
> >>> @@ -306,7 +306,6 @@ struct sc16is7xx_devtype {
> >>> char name[10];
> >>> int nr_gpio;
> >>> int nr_uart;
> >>> - int has_mctrl;
> >>> };
> >>>
> >>> #define SC16IS7XX_RECONF_MD (1 << 0)
> >>> @@ -447,35 +446,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,
> >> I think this one line change is all you really need here, right? the
> >> otner changes do nothing in this patch, so you should just create a new
> >> one changing this value. Oh, and this one:
> >>
> >>> .nr_uart = 1,
> >>> - .has_mctrl = 1,
> >>> };
> >>>
> >>> static const struct sc16is7xx_devtype sc16is752_devtype = {
> >>> .name = "SC16IS752",
> >>> - .nr_gpio = 0,
> >>> + .nr_gpio = 8,
> >> right?
> >>
> >> Don't mess with the has_mctrl stuff, that's not relevant here.
> > Sorry, I just noticed that simply reverting commit 679875d1d880 is not sufficient (and will not compile). We must also revert part of commit:
> > 21144bab4f11 ("sc16is7xx: Handle modem status lines").
> >
> > The problem is that the commit 679875d1d880 was incomplete, and it was (unfortunately) completed by integrating it in commit 21144bab4f11 ("sc16is7xx: Handle modem status lines"). The relevant change was only these 5 new lines, burried deeply into the second commit:
> Just as you noticed, this was required to support full set of flow control lines on this device.
> The commit you're trying to revert was a preparation for it. Disabling has_mctrl will break it.
> I kindly suggest to suggest a fix, instead of hurrying a revert, and waiting for a proper fix later.
Hi Lech,
the [RFC] in the subject was there to discuss about a possible revert, and/or maybe a possible fix that would allow both modes to be supported. I am not hurrying anything and I am certainly not waiting for a later fix, as I very much want to help and maybe submit such a fix myself.
But the reality is that commits 679875d1d880/21144bab4f11 broke userspace by forcing GPIOs as modem control lines. I understand that reverting these patches could also potentially break things for applications depending on these patches. I am simply wondering what is the proper course of action here: revert patches and work on a fix to support both modes, or skip revert and work on a fix (my preference)?
> > @@ -1353,9 +1452,17 @@ static int sc16is7xx_probe(struct device *dev,
> > sc16is7xx_port_write(&s->p[i].port, SC16IS7XX_EFCR_REG,
> > 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);
> > +
> >
> > Therefore, I should also remove these lines if we go forward with a revert of the patch (should I add another tag "Fixes..." in that case?).
> >
> > And what do you think of my proposal to maybe replace has_mctrl with a device tree property so that both modes can be fully supported?
> I think the proper solution here, is not to invent a new device tree property for every single use case.
> I would start by looking for other drivers, if, and how they handle similar cases.
> For example, imx-serial driver respects "uart-has-rtscts" property, as do a lot of other controllers built into SoC-s.
> On the other hand, other devices which can also provide GPIOs, respect "gpio-controller" property.
I think that testing the presence of the "uart-has-rtscts" to force GPIOs as modem control lines would make a lot of sense.
> According to SC16IS752 datasheet [1], respecting one of those should be enough,
> as GPIOs can be enabled in groups of four pins even for dual UART version.
> Every group matches a single port, so probably this can be probably selected per UART even on dual-port versions.
I am trying to see how we could set "uart-has-rtscts" for only UART channel A or B in the device tree, but cannot find any example or documentation about that. How do you propose to do it?
From what I understand, the property "uart-has-rtscts" can be set only for the whole chip (channels A and B)...
Hugo.
> I'll be more than happy to assist with that.
>
> >
> > Thank you,
> > Hugo.
> >
> [1] https://www.nxp.com/docs/en/data-sheet/SC16IS752_SC16IS762.pdf
>
> --
> 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
>
>
--
Hugo Villeneuve
Powered by blists - more mailing lists