| 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: <SN6PR12MB273486EC7A958CFA427C0B37C3A00@SN6PR12MB2734.namprd12.prod.outlook.com>
Date: Tue, 27 Aug 2019 09:30:42 +0000
From: Krishna Yarlagadda <kyarlagadda@...dia.com>
To: Thierry Reding <thierry.reding@...il.com>
CC: "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
"robh+dt@...nel.org" <robh+dt@...nel.org>,
"mark.rutland@....com" <mark.rutland@....com>,
Jonathan Hunter <jonathanh@...dia.com>,
"Laxman Dewangan" <ldewangan@...dia.com>,
"jslaby@...e.com" <jslaby@...e.com>,
"linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"Shardar Mohammed" <smohammed@...dia.com>
Subject: RE: [PATCH 09/14] serial: tegra: set maximum num of uart ports to 8
> -----Original Message-----
> From: Thierry Reding <thierry.reding@...il.com>
> Sent: Tuesday, August 13, 2019 3:50 PM
> To: Krishna Yarlagadda <kyarlagadda@...dia.com>
> Cc: gregkh@...uxfoundation.org; robh+dt@...nel.org;
> mark.rutland@....com; Jonathan Hunter <jonathanh@...dia.com>; Laxman
> Dewangan <ldewangan@...dia.com>; jslaby@...e.com; linux-
> serial@...r.kernel.org; devicetree@...r.kernel.org; linux-
> tegra@...r.kernel.org; linux-kernel@...r.kernel.org; Shardar Mohammed
> <smohammed@...dia.com>
> Subject: Re: [PATCH 09/14] serial: tegra: set maximum num of uart
> ports to 8
>
> On Mon, Aug 12, 2019 at 04:58:18PM +0530, Krishna Yarlagadda wrote:
> > From: Shardar Shariff Md <smohammed@...dia.com>
> >
> > Set maximum number of UART ports to 8 as older chips have 7 ports
> > and
> > Tergra194 and later chips will have 8 ports. Add this info to chip
> > data and register uart driver in platform driver probe.
> >
> > Signed-off-by: Shardar Shariff Md <smohammed@...dia.com>
> > Signed-off-by: Krishna Yarlagadda <kyarlagadda@...dia.com>
> > ---
> > drivers/tty/serial/serial-tegra.c | 21 +++++++++++++--------
> > 1 file changed, 13 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/tty/serial/serial-tegra.c
> > b/drivers/tty/serial/serial-tegra.c
> > index e0379d9..329923c 100644
> > --- a/drivers/tty/serial/serial-tegra.c
> > +++ b/drivers/tty/serial/serial-tegra.c
> > @@ -62,7 +62,7 @@
> > #define TEGRA_UART_TX_TRIG_4B 0x20
> > #define TEGRA_UART_TX_TRIG_1B 0x30
> >
> > -#define TEGRA_UART_MAXIMUM 5
> > +#define TEGRA_UART_MAXIMUM 8
> >
> > /* Default UART setting when started: 115200 no parity, stop, 8 data bits */
> > #define TEGRA_UART_DEFAULT_BAUD 115200
> > @@ -87,6 +87,7 @@ struct tegra_uart_chip_data {
> > bool allow_txfifo_reset_fifo_mode;
> > bool support_clk_src_div;
> > bool fifo_mode_enable_status;
> > + int uart_max_port;
> > };
> >
> > struct tegra_uart_port {
> > @@ -1323,6 +1324,7 @@ static struct tegra_uart_chip_data
> tegra20_uart_chip_data = {
> > .allow_txfifo_reset_fifo_mode = true,
> > .support_clk_src_div = false,
> > .fifo_mode_enable_status = false,
> > + .uart_max_port = 5,
> > };
> >
> > static struct tegra_uart_chip_data tegra30_uart_chip_data = { @@
> > -1330,6 +1332,7 @@ static struct tegra_uart_chip_data
> tegra30_uart_chip_data = {
> > .allow_txfifo_reset_fifo_mode = false,
> > .support_clk_src_div = true,
> > .fifo_mode_enable_status = false,
> > + .uart_max_port = 5,
> > };
> >
> > static struct tegra_uart_chip_data tegra186_uart_chip_data = { @@
> > -1337,6 +1340,7 @@ static struct tegra_uart_chip_data
> tegra186_uart_chip_data = {
> > .allow_txfifo_reset_fifo_mode = false,
> > .support_clk_src_div = true,
> > .fifo_mode_enable_status = true,
> > + .uart_max_port = 5,
>
> You say in the commit message that the older chips have 7 ports, but
> here you say they have 5. Which one is it?
>
> > };
> >
> > static const struct of_device_id tegra_uart_of_match[] = { @@
> > -1386,6
> > +1390,7 @@ static int tegra_uart_probe(struct platform_device *pdev)
> > u->type = PORT_TEGRA;
> > u->fifosize = 32;
> > tup->cdata = cdata;
> > + tegra_uart_driver.nr = cdata->uart_max_port;
> >
> > platform_set_drvdata(pdev, tup);
> > resource = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> @@
> > -1411,6 +1416,13 @@ static int tegra_uart_probe(struct
> > platform_device
> *pdev)
> > return PTR_ERR(tup->rst);
> > }
> >
> > + ret = uart_register_driver(&tegra_uart_driver);
> > + if (ret < 0) {
> > + pr_err("Could not register %s driver\n",
> > + tegra_uart_driver.driver_name);
> > + return ret;
> > + }
>
> I don't think this is the right place for this. You're going to try to
> register the driver once for each instance of the Tegra UART that will be probed.
>
> I'm surprised that this works at all because there's a BUG_ON() early
> in
> uart_register_driver() that checks for the existence of drv->state,
> which means that the second instance of tegra_uart_probe() should
> trigger that and cause the kernel to crash.
>
> I think it's better to either create an additional of_device_id table
> that is used to match on the top-level node's compatible string and
> which only contains the maximum number of ports for the given SoC, or
> you could add code to
> tegra_uart_init() that counts the number of ports that do match and
> initialize tegra_uart_driver.nr using that number. It would something like this:
>
> unsigned int count = 0;
>
> for_each_matching_node(np, &tegra_uart_of_match)
> count++;
>
> tegra_uart_driver.nr = count;
>
> You could also add additional checks in the loop, perhaps something
> like:
>
> for_each_matching_node(np, &tegra_uart_of_match)
> if (of_device_is_available(np))
> count++
>
> Though that would prevent any UARTs from getting added via dynamic
> device tree manipulation.
>
> Thierry
>
Multiple port entries does result in failures which I missed. I will fix this
as suggested.
KY
> > +
> > u->iotype = UPIO_MEM32;
> > ret = platform_get_irq(pdev, 0);
> > if (ret < 0) {
> > @@ -1472,13 +1484,6 @@ static int __init tegra_uart_init(void) {
> > int ret;
> >
> > - ret = uart_register_driver(&tegra_uart_driver);
> > - if (ret < 0) {
> > - pr_err("Could not register %s driver\n",
> > - tegra_uart_driver.driver_name);
> > - return ret;
> > - }
> > -
> > ret = platform_driver_register(&tegra_uart_platform_driver);
> > if (ret < 0) {
> > pr_err("Uart platform driver register failed, e = %d\n", ret);
> > --
> > 2.7.4
> >
Powered by blists - more mailing lists