Re: [PATCH v2 1/7] serial: qcom-geni: fix premature receiver enable

[Bjorn Andersson]

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@xxxxxxxxxxxxxxx	# 6.2
> > Cc: Aniket Randive<quic_arandive@xxxxxxxxxxx>
> > Signed-off-by: Johan Hovold<johan+linaro@xxxxxxxxxx>
> > ---
> >   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