linux-kernel - Re: [PATCH v6] serial: sh-sci: Update the suspend/resume support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMuHMdXis9-PjpKXP7eDGzdgox_jp8Gop6zgJBrnGrATdFTBTA@mail.gmail.com>
Date: Tue, 11 Feb 2025 09:16:38 +0100
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Claudiu <claudiu.beznea@...on.dev>
Cc: gregkh@...uxfoundation.org, jirislaby@...nel.org, p.zabel@...gutronix.de, 
	geert+renesas@...der.be, wsa+renesas@...g-engineering.com, 
	prabhakar.mahadev-lad.rj@...renesas.com, linux-kernel@...r.kernel.org, 
	linux-serial@...r.kernel.org, linux-renesas-soc@...r.kernel.org, 
	Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
Subject: Re: [PATCH v6] serial: sh-sci: Update the suspend/resume support

Hi Claudiu,

On Fri, 7 Feb 2025 at 12:33, Claudiu <claudiu.beznea@...on.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
>
> The Renesas RZ/G3S supports a power saving mode where power to most of the
> SoC components is turned off. When returning from this power saving mode,
> SoC components need to be re-configured.
>
> The SCIFs on the Renesas RZ/G3S need to be re-configured as well when
> returning from this power saving mode. The sh-sci code already configures
> the SCIF clocks, power domain and registers by calling uart_resume_port()
> in sci_resume(). On suspend path the SCIF UART ports are suspended
> accordingly (by calling uart_suspend_port() in sci_suspend()). The only
> missing setting is the reset signal. For this assert/de-assert the reset
> signal on driver suspend/resume.
>
> In case the no_console_suspend is specified by the user, the registers need
> to be saved on suspend path and restore on resume path. To do this the
> sci_console_save()/sci_console_restore() functions were added. There is no
> need to cache/restore the status or FIFO registers. Only the control
> registers. The registers that will be saved/restored on suspend/resume are
> specified by the struct sci_suspend_regs data structure.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
> ---
>
> The v4 of this patch was part of a series with 4 patches. As the rest of
> the patches were applied I dropped the cover letter. The v4 cover letter
> is located here:
> https://lore.kernel.org/all/20250120130936.1080069-1-claudiu.beznea.uj@bp.renesas.com
>
> Changes in v6:
> - used sci_getreg() before saving/restoring registers to avoid
>   WARN() on sci_serial_in()/sci_serial_out()
> - splitted sci_console_save_restore() in 2 functions:
>   sci_console_save()/sci_console_restore() as requested in the
>   review process
> - adjusted the patch description to reflect these changes

Thanks for the update!

Reviewed-by: Geert Uytterhoeven <geert+renesas@...der.be>

One philosophical comment below...

> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -3546,13 +3559,57 @@ static int sci_probe(struct platform_device *dev)
>         return 0;
>  }
>
> +static void sci_console_save(struct sci_port *s)
> +{
> +       struct sci_suspend_regs *regs = &s->suspend_regs;
> +       struct uart_port *port = &s->port;
> +
> +       if (sci_getreg(port, SCSMR)->size)
> +               regs->scsmr = sci_serial_in(port, SCSMR);
> +       if (sci_getreg(port, SCSCR)->size)
> +               regs->scscr = sci_serial_in(port, SCSCR);
> +       if (sci_getreg(port, SCFCR)->size)
> +               regs->scfcr = sci_serial_in(port, SCFCR);
> +       if (sci_getreg(port, SCSPTR)->size)
> +               regs->scsptr = sci_serial_in(port, SCSPTR);
> +       if (sci_getreg(port, SCBRR)->size)
> +               regs->scbrr = sci_serial_in(port, SCBRR);
> +       if (sci_getreg(port, SEMR)->size)
> +               regs->semr = sci_serial_in(port, SEMR);

IMHO, it does not make much sense to check for the presence of the
SCSMR, SCSCR, and SCBRR registers, as they exist on all variants,
and are always accessed unconditionally in the rest of the code.

Same for sci_console_restore().

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-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