If the imx driver cannot support RS485 it sets the ports rs485_supported
structure to NULL. But it still calls uart_get_rs485_mode() which may set
the RS485_ENABLED flag nevertheless.

This may lead to an attempt to configure RS485 even if it is not supported
when the flag is evaluated in uart_configure_port() at port startup.

Avoid this by bailing out of uart_get_rs485_mode() if the RS485_ENABLED
flag is not supported by the caller.

With this fix a check for RTS availability is now obsolete in the imx
driver, since it can not evaluate to true any more. Remove this check, too.

Fixes: 00d7a00e2a6f ("serial: imx: Fill in rs485_supported")
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: stable@vger.kernel.org
Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
---
 drivers/tty/serial/imx.c         | 4 ----
 drivers/tty/serial/serial_core.c | 3 +++
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 9cffeb23112b..98b78d360a74 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -2328,10 +2328,6 @@ static int imx_uart_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	if (sport->port.rs485.flags & SER_RS485_ENABLED &&
-	    (!sport->have_rtscts && !sport->have_rtsgpio))
-		dev_err(&pdev->dev, "no RTS control, disabling rs485\n");
-
 	/*
 	 * If using the i.MX UART RTS/CTS control then the RTS (CTS_B)
 	 * signal cannot be set low during transmission in case the
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 661074ab8edb..b418952c03df 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -3593,6 +3593,9 @@ int uart_get_rs485_mode(struct uart_port *port)
 	u32 rs485_delay[2];
 	int ret;
 
+	if (!(port->rs485_supported.flags & SER_RS485_ENABLED))
+		return 0;
+
 	ret = device_property_read_u32_array(dev, "rs485-rts-delay",
 					     rs485_delay, 2);
 	if (!ret) {
-- 
2.42.0

On Sat, 9 Dec 2023, Lino Sanfilippo wrote:

> If the imx driver cannot support RS485 it sets the ports rs485_supported
> structure to NULL.

No, an embedded struct inside struct uart_port cannot be set to NULL, 
it's always there.

Looking into the code, that setting of rs485_supported from imx_no_rs485 
is actually superfluous as it should be already cleared to zeros on alloc.

> But it still calls uart_get_rs485_mode() which may set
> the RS485_ENABLED flag nevertheless.
> 
> This may lead to an attempt to configure RS485 even if it is not supported
> when the flag is evaluated in uart_configure_port() at port startup.
> 
> Avoid this by bailing out of uart_get_rs485_mode() if the RS485_ENABLED
> flag is not supported by the caller.
> 
> With this fix a check for RTS availability is now obsolete in the imx
> driver, since it can not evaluate to true any more. Remove this check, too.
> 
> Fixes: 00d7a00e2a6f ("serial: imx: Fill in rs485_supported")
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: stable@vger.kernel.org
> Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>

-- 
 i.

On 11.12.23 12:00, Ilpo Järvinen wrote:
> On Sat, 9 Dec 2023, Lino Sanfilippo wrote:
>
>> If the imx driver cannot support RS485 it sets the ports rs485_supported
>> structure to NULL.
>
> No, an embedded struct inside struct uart_port cannot be set to NULL,
> it's always there.
>

Hmm, ok. What I meant was that the structure is nullified. "set to NULL" is maybe a bit
misleading. I will correct this.

> Looking into the code, that setting of rs485_supported from imx_no_rs485
> is actually superfluous as it should be already cleared to zeros on alloc.
>

Yes. BTW: Another "no_rs485" configuration setting can be found in the ar933x driver.
If we do not want to keep those assignments I can remove the one for the imx
driver with the next version of this patch...

>> But it still calls uart_get_rs485_mode() which may set
>> the RS485_ENABLED flag nevertheless.
>>
>> This may lead to an attempt to configure RS485 even if it is not supported
>> when the flag is evaluated in uart_configure_port() at port startup.
>>
>> Avoid this by bailing out of uart_get_rs485_mode() if the RS485_ENABLED
>> flag is not supported by the caller.
>>
>> With this fix a check for RTS availability is now obsolete in the imx
>> driver, since it can not evaluate to true any more. Remove this check, too.
>>
>> Fixes: 00d7a00e2a6f ("serial: imx: Fill in rs485_supported")
>> Cc: Shawn Guo <shawnguo@kernel.org>
>> Cc: Sascha Hauer <s.hauer@pengutronix.de>
>> Cc: stable@vger.kernel.org
>> Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>

On Wed, 13 Dec 2023, Lino Sanfilippo wrote:
> On 11.12.23 12:00, Ilpo Järvinen wrote:
> > On Sat, 9 Dec 2023, Lino Sanfilippo wrote:
> 
> > Looking into the code, that setting of rs485_supported from imx_no_rs485
> > is actually superfluous as it should be already cleared to zeros on alloc.
> >
> 
> Yes. BTW: Another "no_rs485" configuration setting can be found in the ar933x driver.
> If we do not want to keep those assignments I can remove the one for the imx
> driver with the next version of this patch...

I think they can just be dropped as it's normal in Linux code to assume 
that things are zeroed by default. Those "no"-variants originate from the 
time when supported_rs485 was not yet embedded but just a pointer to a 
const struct and I didn't realize I could have removed them when I ended 
up embedding the struct so it can be altered per port.

-- 
 i.