diff mbox series

[RFT,5/6] serial: sh-sci: Clean sci_ports[0] after at earlycon exit

Message ID 20241204155806.3781200-6-claudiu.beznea.uj@bp.renesas.com
State New
Headers show
Series serial: sh-sci: Fixes for earlycon and keep_bootcon | expand

Commit Message

Claudiu Beznea Dec. 4, 2024, 3:58 p.m. UTC 
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

The early_console_setup() function initializes sci_ports[0].port with an
object of type struct uart_port obtained from the struct earlycon_device
passed as an argument to early_console_setup().

Later, during serial port probing, the serial port used as earlycon
(e.g., port A) might be remapped to a different position in the sci_ports[]
array, and a different serial port (e.g., port B) might be assigned to slot
0. For example:

sci_ports[0] = port B
sci_ports[X] = port A

In this scenario, the new port mapped at index zero (port B) retains the
data associated with the earlycon configuration. Consequently, after the
Linux boot process, any access to the serial port now mapped to
sci_ports[0] (port B) will block the original earlycon port (port A).

To address this, introduce an early_console_exit() function to clean up
sci_ports[0] when earlycon is exited.

To prevent the cleanup of sci_ports[0] while the serial device is still
being used by earlycon, introduce the struct sci_port::probing flag and
account for it in early_console_exit().

Fixes: 0b0cced19ab1 ("serial: sh-sci: Add CONFIG_SERIAL_EARLYCON support")
Cc: stable@vger.kernel.org
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes since the integrated patch:
- adjust the commit message to address Geert comments at [1]
- Introduce the struct sci_port::probing flag to prevent the cleanup
  of sci_ports[0] while the serial device is still being used by earlycon


[1] https://lore.kernel.org/all/CAMuHMdX57_AEYC_6CbrJn-+B+ivU8oFiXR0FXF7Lrqv5dWZWYA@mail.gmail.com/

 drivers/tty/serial/sh-sci.c | 33 +++++++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 6 deletions(-)

Comments

Geert Uytterhoeven Dec. 19, 2024, 2:26 p.m. UTC | #1 
Hi Claudiu,

On Wed, Dec 4, 2024 at 4:58 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> The early_console_setup() function initializes sci_ports[0].port with an
> object of type struct uart_port obtained from the struct earlycon_device
> passed as an argument to early_console_setup().
>
> Later, during serial port probing, the serial port used as earlycon
> (e.g., port A) might be remapped to a different position in the sci_ports[]
> array, and a different serial port (e.g., port B) might be assigned to slot
> 0. For example:
>
> sci_ports[0] = port B
> sci_ports[X] = port A
>
> In this scenario, the new port mapped at index zero (port B) retains the
> data associated with the earlycon configuration. Consequently, after the
> Linux boot process, any access to the serial port now mapped to
> sci_ports[0] (port B) will block the original earlycon port (port A).
>
> To address this, introduce an early_console_exit() function to clean up
> sci_ports[0] when earlycon is exited.
>
> To prevent the cleanup of sci_ports[0] while the serial device is still
> being used by earlycon, introduce the struct sci_port::probing flag and
> account for it in early_console_exit().
>
> Fixes: 0b0cced19ab1 ("serial: sh-sci: Add CONFIG_SERIAL_EARLYCON support")
> Cc: stable@vger.kernel.org
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
>
> Changes since the integrated patch:
> - adjust the commit message to address Geert comments at [1]
> - Introduce the struct sci_port::probing flag to prevent the cleanup
>   of sci_ports[0] while the serial device is still being used by earlycon

Thanks for the update!

> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -159,6 +159,7 @@ struct sci_port {
>         bool autorts;
>         bool tx_occurred;
>         bool earlycon;
> +       bool probing;

This is only used in sci_ports[0], so it can be a single global flag,
instead of a flag embedded in each sci_port structure.

>  };
>
>  #define SCI_NPORTS CONFIG_SERIAL_SH_SCI_NR_UARTS
> @@ -3386,7 +3387,8 @@ static struct plat_sci_port *sci_parse_dt(struct platform_device *pdev,
>  static int sci_probe_single(struct platform_device *dev,
>                                       unsigned int index,
>                                       struct plat_sci_port *p,
> -                                     struct sci_port *sciport)
> +                                     struct sci_port *sciport,
> +                                     struct resource *sci_res)
>  {
>         int ret;
>
> @@ -3433,12 +3435,15 @@ static int sci_probe_single(struct platform_device *dev,
>                 sciport->port.flags |= UPF_HARD_FLOW;
>         }
>
> -       ret = uart_add_one_port(&sci_uart_driver, &sciport->port);
> -       if (ret) {
> -               return ret;
> +       if (sci_ports[0].earlycon && sci_ports[0].port.mapbase == sci_res->start) {
> +               /*
> +                * Skip cleanup up the sci_port[0] in early_console_exit(), this

Double up

> +                * port is the same as the earlycon one.
> +                */
> +               sci_ports[0].probing = true;
>         }
>
> -       return 0;
> +       return uart_add_one_port(&sci_uart_driver, &sciport->port);
>  }
>
>  static int sci_probe(struct platform_device *dev)
> @@ -3496,7 +3501,7 @@ static int sci_probe(struct platform_device *dev)
>
>         platform_set_drvdata(dev, sp);
>
> -       ret = sci_probe_single(dev, dev_id, p, sp);
> +       ret = sci_probe_single(dev, dev_id, p, sp, res);
>         if (ret)
>                 return ret;
>
> @@ -3579,6 +3584,20 @@ sh_early_platform_init_buffer("earlyprintk", &sci_driver,
>  #ifdef CONFIG_SERIAL_SH_SCI_EARLYCON
>  static struct plat_sci_port port_cfg;
>
> +static int early_console_exit(struct console *co)
> +{
> +       struct sci_port *sci_port = &sci_ports[0];
> +
> +       /*
> +        * Clean the slot used by earlycon. A new SCI device might
> +        * map to this slot.
> +        */
> +       if (sci_port->earlycon && !sci_port->probing)
> +               memset(sci_port, 0, sizeof(*sci_port));

Aha, so this clears sci_port.earlycon, too (cfr. my comment on
PATCH 4/6). Still, I don't think this is sufficient: shouldn't
sci_port.earlycon be cleared unconditionally?

> +
> +       return 0;
> +}
> +
>  static int __init early_console_setup(struct earlycon_device *device,
>                                       int type)
>  {
> @@ -3596,6 +3615,8 @@ static int __init early_console_setup(struct earlycon_device *device,
>                        SCSCR_RE | SCSCR_TE | port_cfg.scscr);
>
>         device->con->write = serial_console_write;
> +       device->con->exit = early_console_exit;
> +
>         return 0;
>  }
>  static int __init sci_early_console_setup(struct earlycon_device *device,

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Claudiu Beznea Dec. 21, 2024, 9:39 a.m. UTC | #2 
Hi, Geert,

On 19.12.2024 16:26, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Wed, Dec 4, 2024 at 4:58 PM Claudiu <claudiu.beznea@tuxon.dev> wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> The early_console_setup() function initializes sci_ports[0].port with an
>> object of type struct uart_port obtained from the struct earlycon_device
>> passed as an argument to early_console_setup().
>>
>> Later, during serial port probing, the serial port used as earlycon
>> (e.g., port A) might be remapped to a different position in the sci_ports[]
>> array, and a different serial port (e.g., port B) might be assigned to slot
>> 0. For example:
>>
>> sci_ports[0] = port B
>> sci_ports[X] = port A
>>
>> In this scenario, the new port mapped at index zero (port B) retains the
>> data associated with the earlycon configuration. Consequently, after the
>> Linux boot process, any access to the serial port now mapped to
>> sci_ports[0] (port B) will block the original earlycon port (port A).
>>
>> To address this, introduce an early_console_exit() function to clean up
>> sci_ports[0] when earlycon is exited.
>>
>> To prevent the cleanup of sci_ports[0] while the serial device is still
>> being used by earlycon, introduce the struct sci_port::probing flag and
>> account for it in early_console_exit().
>>
>> Fixes: 0b0cced19ab1 ("serial: sh-sci: Add CONFIG_SERIAL_EARLYCON support")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> ---
>>
>> Changes since the integrated patch:
>> - adjust the commit message to address Geert comments at [1]
>> - Introduce the struct sci_port::probing flag to prevent the cleanup
>>   of sci_ports[0] while the serial device is still being used by earlycon
> 
> Thanks for the update!
> 
>> --- a/drivers/tty/serial/sh-sci.c
>> +++ b/drivers/tty/serial/sh-sci.c
>> @@ -159,6 +159,7 @@ struct sci_port {
>>         bool autorts;
>>         bool tx_occurred;
>>         bool earlycon;
>> +       bool probing;
> 
> This is only used in sci_ports[0], so it can be a single global flag,
> instead of a flag embedded in each sci_port structure.
> 
>>  };
>>
>>  #define SCI_NPORTS CONFIG_SERIAL_SH_SCI_NR_UARTS
>> @@ -3386,7 +3387,8 @@ static struct plat_sci_port *sci_parse_dt(struct platform_device *pdev,
>>  static int sci_probe_single(struct platform_device *dev,
>>                                       unsigned int index,
>>                                       struct plat_sci_port *p,
>> -                                     struct sci_port *sciport)
>> +                                     struct sci_port *sciport,
>> +                                     struct resource *sci_res)
>>  {
>>         int ret;
>>
>> @@ -3433,12 +3435,15 @@ static int sci_probe_single(struct platform_device *dev,
>>                 sciport->port.flags |= UPF_HARD_FLOW;
>>         }
>>
>> -       ret = uart_add_one_port(&sci_uart_driver, &sciport->port);
>> -       if (ret) {
>> -               return ret;
>> +       if (sci_ports[0].earlycon && sci_ports[0].port.mapbase == sci_res->start) {
>> +               /*
>> +                * Skip cleanup up the sci_port[0] in early_console_exit(), this
> 
> Double up
> 
>> +                * port is the same as the earlycon one.
>> +                */
>> +               sci_ports[0].probing = true;
>>         }
>>
>> -       return 0;
>> +       return uart_add_one_port(&sci_uart_driver, &sciport->port);
>>  }
>>
>>  static int sci_probe(struct platform_device *dev)
>> @@ -3496,7 +3501,7 @@ static int sci_probe(struct platform_device *dev)
>>
>>         platform_set_drvdata(dev, sp);
>>
>> -       ret = sci_probe_single(dev, dev_id, p, sp);
>> +       ret = sci_probe_single(dev, dev_id, p, sp, res);
>>         if (ret)
>>                 return ret;
>>
>> @@ -3579,6 +3584,20 @@ sh_early_platform_init_buffer("earlyprintk", &sci_driver,
>>  #ifdef CONFIG_SERIAL_SH_SCI_EARLYCON
>>  static struct plat_sci_port port_cfg;
>>
>> +static int early_console_exit(struct console *co)
>> +{
>> +       struct sci_port *sci_port = &sci_ports[0];
>> +
>> +       /*
>> +        * Clean the slot used by earlycon. A new SCI device might
>> +        * map to this slot.
>> +        */
>> +       if (sci_port->earlycon && !sci_port->probing)
>> +               memset(sci_port, 0, sizeof(*sci_port));
> 
> Aha, so this clears sci_port.earlycon, too (cfr. my comment on
> PATCH 4/6). Still, I don't think this is sufficient: shouldn't
> sci_port.earlycon be cleared unconditionally?

I remember I had failures with unconditional clear. I'll double check it
though and adjust accordingly, if needed.

Thank you,
Claudiu

> 
>> +
>> +       return 0;
>> +}
>> +
>>  static int __init early_console_setup(struct earlycon_device *device,
>>                                       int type)
>>  {
>> @@ -3596,6 +3615,8 @@ static int __init early_console_setup(struct earlycon_device *device,
>>                        SCSCR_RE | SCSCR_TE | port_cfg.scscr);
>>
>>         device->con->write = serial_console_write;
>> +       device->con->exit = early_console_exit;
>> +
>>         return 0;
>>  }
>>  static int __init sci_early_console_setup(struct earlycon_device *device,
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
diff mbox series

Patch

diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
index e12fbc71082a..f74eb68774ca 100644
--- a/drivers/tty/serial/sh-sci.c
+++ b/drivers/tty/serial/sh-sci.c
@@ -159,6 +159,7 @@  struct sci_port {
 	bool autorts;
 	bool tx_occurred;
 	bool earlycon;
+	bool probing;
 };
 
 #define SCI_NPORTS CONFIG_SERIAL_SH_SCI_NR_UARTS
@@ -3386,7 +3387,8 @@  static struct plat_sci_port *sci_parse_dt(struct platform_device *pdev,
 static int sci_probe_single(struct platform_device *dev,
 				      unsigned int index,
 				      struct plat_sci_port *p,
-				      struct sci_port *sciport)
+				      struct sci_port *sciport,
+				      struct resource *sci_res)
 {
 	int ret;
 
@@ -3433,12 +3435,15 @@  static int sci_probe_single(struct platform_device *dev,
 		sciport->port.flags |= UPF_HARD_FLOW;
 	}
 
-	ret = uart_add_one_port(&sci_uart_driver, &sciport->port);
-	if (ret) {
-		return ret;
+	if (sci_ports[0].earlycon && sci_ports[0].port.mapbase == sci_res->start) {
+		/*
+		 * Skip cleanup up the sci_port[0] in early_console_exit(), this
+		 * port is the same as the earlycon one.
+		 */
+		sci_ports[0].probing = true;
 	}
 
-	return 0;
+	return uart_add_one_port(&sci_uart_driver, &sciport->port);
 }
 
 static int sci_probe(struct platform_device *dev)
@@ -3496,7 +3501,7 @@  static int sci_probe(struct platform_device *dev)
 
 	platform_set_drvdata(dev, sp);
 
-	ret = sci_probe_single(dev, dev_id, p, sp);
+	ret = sci_probe_single(dev, dev_id, p, sp, res);
 	if (ret)
 		return ret;
 
@@ -3579,6 +3584,20 @@  sh_early_platform_init_buffer("earlyprintk", &sci_driver,
 #ifdef CONFIG_SERIAL_SH_SCI_EARLYCON
 static struct plat_sci_port port_cfg;
 
+static int early_console_exit(struct console *co)
+{
+	struct sci_port *sci_port = &sci_ports[0];
+
+	/*
+	 * Clean the slot used by earlycon. A new SCI device might
+	 * map to this slot.
+	 */
+	if (sci_port->earlycon && !sci_port->probing)
+		memset(sci_port, 0, sizeof(*sci_port));
+
+	return 0;
+}
+
 static int __init early_console_setup(struct earlycon_device *device,
 				      int type)
 {
@@ -3596,6 +3615,8 @@  static int __init early_console_setup(struct earlycon_device *device,
 		       SCSCR_RE | SCSCR_TE | port_cfg.scscr);
 
 	device->con->write = serial_console_write;
+	device->con->exit = early_console_exit;
+
 	return 0;
 }
 static int __init sci_early_console_setup(struct earlycon_device *device,