The receiver should not be enabled until the port is opened so drop the
bogus call to start rx from the setup code which is shared with the
console implementation.

This was added for some confused implementation of hibernation support,
but the receiver must not be started unconditionally as the port may not
have been open when hibernating the system.

Fixes: 35781d8356a2 ("tty: serial: qcom-geni-serial: Add support for Hibernation feature")
Cc: stable@vger.kernel.org	# 6.2
Cc: Aniket Randive <quic_arandive@quicinc.com>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/tty/serial/qcom_geni_serial.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 6f0db310cf69..9ea6bd09e665 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -1152,7 +1152,6 @@ static int qcom_geni_serial_port_setup(struct uart_port *uport)
 			       false, true, true);
 	geni_se_init(&port->se, UART_RX_WM, port->rx_fifo_depth - 2);
 	geni_se_select_mode(&port->se, port->dev_data->mode);
-	qcom_geni_serial_start_rx(uport);
 	port->setup = true;
 
 	return 0;
-- 
2.45.2

Hi,

On Tue, Oct 1, 2024 at 5:51 AM Johan Hovold <johan+linaro@kernel.org> wrote:
>
> The receiver should not be enabled until the port is opened so drop the
> bogus call to start rx from the setup code which is shared with the
> console implementation.
>
> This was added for some confused implementation of hibernation support,
> but the receiver must not be started unconditionally as the port may not
> have been open when hibernating the system.

Could you provide a motivation for your patch in the description? Is
patch needed for something (perhaps a future patch in the series)? Is
it fixing a bug? Does it save power? Is the call harmless but cleaner
to get rid of?


> Fixes: 35781d8356a2 ("tty: serial: qcom-geni-serial: Add support for Hibernation feature")
> Cc: stable@vger.kernel.org      # 6.2
> Cc: Aniket Randive <quic_arandive@quicinc.com>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  drivers/tty/serial/qcom_geni_serial.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 6f0db310cf69..9ea6bd09e665 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -1152,7 +1152,6 @@ static int qcom_geni_serial_port_setup(struct uart_port *uport)
>                                false, true, true);
>         geni_se_init(&port->se, UART_RX_WM, port->rx_fifo_depth - 2);
>         geni_se_select_mode(&port->se, port->dev_data->mode);
> -       qcom_geni_serial_start_rx(uport);

FWIW, I found at least one thing that's broken by your patch. If you
enable kgdb (but _not_ "kgdboc_earlycon") and then add "kgdbwait" to
the kernel command line parameters then things will be broken after
your patch. You'll drop into the debugger but can't interact with it.
The "kgdboc_earlycon" path handles this because of
"qcom_geni_serial_enable_early_read()" but it doesn't seem like
there's anything that handles it for normal kgdb. If you drop in the
debugger later it'll probably work if you've got an "agetty" running
because that'll enable the RX path.


-Doug

On Thu, Oct 03, 2024 at 11:29:58AM -0700, Doug Anderson wrote:
> On Tue, Oct 1, 2024 at 5:51 AM Johan Hovold <johan+linaro@kernel.org> wrote:
> >
> > The receiver should not be enabled until the port is opened so drop the
> > bogus call to start rx from the setup code which is shared with the
> > console implementation.
> >
> > This was added for some confused implementation of hibernation support,
> > but the receiver must not be started unconditionally as the port may not
> > have been open when hibernating the system.
> 
> Could you provide a motivation for your patch in the description? Is
> patch needed for something (perhaps a future patch in the series)? Is
> it fixing a bug? Does it save power? Is the call harmless but cleaner
> to get rid of?

I was trying to bring some order to this driver so that the receiver is
enabled when the port is opened and disabled when it is closed again as
expected, and get rid of the random calls added in places where they do
not belong (e.g., as Bjorn also mentioned, why was the call to start rx
added in the port setup code if it was needed for hibernation?).

Data "received" over the wire before opening the port should not be
processed, but it also turns out that enabling the receiver before the
port is opened can confuse the firmware and break the "stale" rx timer
handling so that data is only forwarded in chunks of 12 bytes instead of
when each char is received.

> > Fixes: 35781d8356a2 ("tty: serial: qcom-geni-serial: Add support for Hibernation feature")
> > Cc: stable@vger.kernel.org      # 6.2
> > Cc: Aniket Randive <quic_arandive@quicinc.com>
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
> >  drivers/tty/serial/qcom_geni_serial.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> > index 6f0db310cf69..9ea6bd09e665 100644
> > --- a/drivers/tty/serial/qcom_geni_serial.c
> > +++ b/drivers/tty/serial/qcom_geni_serial.c
> > @@ -1152,7 +1152,6 @@ static int qcom_geni_serial_port_setup(struct uart_port *uport)
> >                                false, true, true);
> >         geni_se_init(&port->se, UART_RX_WM, port->rx_fifo_depth - 2);
> >         geni_se_select_mode(&port->se, port->dev_data->mode);
> > -       qcom_geni_serial_start_rx(uport);
> 
> FWIW, I found at least one thing that's broken by your patch. If you
> enable kgdb (but _not_ "kgdboc_earlycon") and then add "kgdbwait" to
> the kernel command line parameters then things will be broken after
> your patch. You'll drop into the debugger but can't interact with it.
> The "kgdboc_earlycon" path handles this because of
> "qcom_geni_serial_enable_early_read()" but it doesn't seem like
> there's anything that handles it for normal kgdb. If you drop in the
> debugger later it'll probably work if you've got an "agetty" running
> because that'll enable the RX path.

Ok, so the kgdb has started relying on this call since d8851a96ba25
("tty: serial: qcom-geni-serial: Add a poll_init() function"). Thanks
for pointing that out.

The polled console code should not be calling the port setup code
unconditionally anyway so I'll fix this up as well in v3.

Johan

Thanks Johan for the fixes.

On 10/1/2024 6:20 PM, Johan Hovold wrote:
> The receiver should not be enabled until the port is opened so drop the
> bogus call to start rx from the setup code which is shared with the
> console implementation.
> 
> This was added for some confused implementation of hibernation support,
> but the receiver must not be started unconditionally as the port may not
> have been open when hibernating the system.
> 
> Fixes: 35781d8356a2 ("tty: serial: qcom-geni-serial: Add support for Hibernation feature")
> Cc:stable@vger.kernel.org	# 6.2
> Cc: Aniket Randive<quic_arandive@quicinc.com>
> Signed-off-by: Johan Hovold<johan+linaro@kernel.org>
> ---
>   drivers/tty/serial/qcom_geni_serial.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 6f0db310cf69..9ea6bd09e665 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -1152,7 +1152,6 @@ static int qcom_geni_serial_port_setup(struct uart_port *uport)
>   			       false, true, true);
>   	geni_se_init(&port->se, UART_RX_WM, port->rx_fifo_depth - 2);
>   	geni_se_select_mode(&port->se, port->dev_data->mode);
> -	qcom_geni_serial_start_rx(uport);
Does it mean hibernation will break now ? Not sure if its tested with 
hibernation. I can see this call was added to port_setup specifically 
for hibernation but now after removing it, where is it getting fixed ?
I think RX will not be initialized after hibernation.
>   	port->setup = true;
>   
>   	return 0;
> -- 2.45.2

On Tue, Oct 01, 2024 at 07:20:36PM +0530, Mukesh Kumar Savaliya wrote:
> Thanks Johan for the fixes.

Thanks for taking a look.

> On 10/1/2024 6:20 PM, Johan Hovold wrote:
> > The receiver should not be enabled until the port is opened so drop the
> > bogus call to start rx from the setup code which is shared with the
> > console implementation.
> > 
> > This was added for some confused implementation of hibernation support,
> > but the receiver must not be started unconditionally as the port may not
> > have been open when hibernating the system.
> > 
> > Fixes: 35781d8356a2 ("tty: serial: qcom-geni-serial: Add support for Hibernation feature")
> > Cc:stable@vger.kernel.org	# 6.2
> > Cc: Aniket Randive<quic_arandive@quicinc.com>
> > Signed-off-by: Johan Hovold<johan+linaro@kernel.org>

> > @@ -1152,7 +1152,6 @@ static int qcom_geni_serial_port_setup(struct uart_port *uport)
> >   			       false, true, true);
> >   	geni_se_init(&port->se, UART_RX_WM, port->rx_fifo_depth - 2);
> >   	geni_se_select_mode(&port->se, port->dev_data->mode);
> > -	qcom_geni_serial_start_rx(uport);

> Does it mean hibernation will break now ? Not sure if its tested with 
> hibernation. I can see this call was added to port_setup specifically 
> for hibernation but now after removing it, where is it getting fixed ?
> I think RX will not be initialized after hibernation.

Correct. As I alluded to in the commit message this "hibernation
support" is quite broken already, but I was trying to avoid spending
more time on this driver than I already have and just look the other way
for the time being.

Note that rx is enabled by the serial core resume code, but then this
hibernation hack added a call to the setup the port after resuming it,
which would disable rx again were it not for this random call to
start rx, which should never have been added here in the first place.

But as these platforms do not support hibernation in mainline, and the
code broken anyway, I'll just rip it all out for v3.

Johan

On Tue, Oct 01, 2024 at 07:20:36PM GMT, Mukesh Kumar Savaliya wrote:
> Thanks Johan for the fixes.
> 
> On 10/1/2024 6:20 PM, Johan Hovold wrote:
> > The receiver should not be enabled until the port is opened so drop the
> > bogus call to start rx from the setup code which is shared with the
> > console implementation.
> > 
> > This was added for some confused implementation of hibernation support,
> > but the receiver must not be started unconditionally as the port may not
> > have been open when hibernating the system.
> > 
> > Fixes: 35781d8356a2 ("tty: serial: qcom-geni-serial: Add support for Hibernation feature")
> > Cc:stable@vger.kernel.org	# 6.2
> > Cc: Aniket Randive<quic_arandive@quicinc.com>
> > Signed-off-by: Johan Hovold<johan+linaro@kernel.org>
> > ---
> >   drivers/tty/serial/qcom_geni_serial.c | 1 -
> >   1 file changed, 1 deletion(-)
> > 
> > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> > index 6f0db310cf69..9ea6bd09e665 100644
> > --- a/drivers/tty/serial/qcom_geni_serial.c
> > +++ b/drivers/tty/serial/qcom_geni_serial.c
> > @@ -1152,7 +1152,6 @@ static int qcom_geni_serial_port_setup(struct uart_port *uport)
> >   			       false, true, true);
> >   	geni_se_init(&port->se, UART_RX_WM, port->rx_fifo_depth - 2);
> >   	geni_se_select_mode(&port->se, port->dev_data->mode);
> > -	qcom_geni_serial_start_rx(uport);
> Does it mean hibernation will break now ? Not sure if its tested with
> hibernation. I can see this call was added to port_setup specifically for
> hibernation but now after removing it, where is it getting fixed ?

Can you explain how you're testing hibernation and on which platform
this is done? I'd like to add this to my set of tests, but last time I
tested I couldn't find a platform where we survived the restore
processes (it's been a while though).

> I think RX will not be initialized after hibernation.

qcom_geni_serial_port_setup() is invoked in multiple places, how come
we don't perform this hibernation-specific operation in
qcom_geni_serial_sys_hib_resume()? (And why is it called hib_resume when
the kernel nomenclature for what it does is "restore"?)

Regards,
Bjorn

> >   	port->setup = true;
> >   	return 0;
> > -- 2.45.2