Re: [PATCH RFT 4/6] serial: sh-sci: Do not probe the serial port if its slot in sci_ports[] is in use

[Geert Uytterhoeven]

Hi Claudiu,

On Wed, Dec 4, 2024 at 4:58 PM Claudiu <claudiu.beznea@xxxxxxxxx> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
>
> In the sh-sci driver, sci_ports[0] is used by earlycon. If the earlycon is
> still active when sci_probe() is called and the new serial port is supposed
> to map to sci_ports[0], return -EBUSY to prevent breaking the earlycon.
>
> This situation should occurs in debug scenarios, and users should be
> aware of the potential conflict.
>
> Fixes: 0b0cced19ab1 ("serial: sh-sci: Add CONFIG_SERIAL_EARLYCON support")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>

Thanks for your patch!

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

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
> @@ -3443,6 +3444,7 @@ static int sci_probe_single(struct platform_device *dev,
>  static int sci_probe(struct platform_device *dev)
>  {
>         struct plat_sci_port *p;
> +       struct resource *res;
>         struct sci_port *sp;
>         unsigned int dev_id;
>         int ret;
> @@ -3472,6 +3474,26 @@ static int sci_probe(struct platform_device *dev)
>         }
>
>         sp = &sci_ports[dev_id];
> +
> +       /*
> +        * In case:
> +        * - the probed port alias is zero (as the one used by earlycon), and
> +        * - the earlycon is still active (e.g., "earlycon keep_bootcon" in
> +        *   bootargs)

This is even true without "keep_bootcon", as nothing ever clears the
sci_port.earlycon flag once it is set.

> +        *
> +        * defer the probe of this serial. This is a debug scenario and the user
> +        * must be aware of it.
> +        *
> +        * Except when the probed port is the same as the earlycon port.
> +        */
> +
> +       res = platform_get_resource(dev, IORESOURCE_MEM, 0);
> +       if (!res)
> +               return -ENODEV;
> +
> +       if (sp->earlycon && res->start != sp->port.mapbase)
> +               return dev_err_probe(&dev->dev, -EBUSY, "sci_port[0] is used by earlycon!\n");
> +
>         platform_set_drvdata(dev, sp);
>
>         ret = sci_probe_single(dev, dev_id, p, sp);
> @@ -3568,6 +3590,7 @@ static int __init early_console_setup(struct earlycon_device *device,
>         port_cfg.type = type;
>         sci_ports[0].cfg = &port_cfg;
>         sci_ports[0].params = sci_probe_regmap(&port_cfg);
> +       sci_ports[0].earlycon = true;
>         port_cfg.scscr = sci_serial_in(&sci_ports[0].port, SCSCR);
>         sci_serial_out(&sci_ports[0].port, SCSCR,
>                        SCSCR_RE | SCSCR_TE | port_cfg.scscr);

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

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