linux-kernel - Re: [PATCH v11 3/5] tty: serial: sh-sci: Use port ops callbacks

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: <CAMuHMdUU-EyU3DgqP9KDmeT_A4i-xaE9hAUOvYFQcbYmpJc2ng@mail.gmail.com>
Date: Tue, 17 Jun 2025 10:44:16 +0200
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Prabhakar <prabhakar.csengg@...il.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Jiri Slaby <jirislaby@...nel.org>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Magnus Damm <magnus.damm@...il.com>, Wolfram Sang <wsa+renesas@...g-engineering.com>, 
	linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-renesas-soc@...r.kernel.org, 
	Biju Das <biju.das.jz@...renesas.com>, 
	Fabrizio Castro <fabrizio.castro.jz@...esas.com>, 
	Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Subject: Re: [PATCH v11 3/5] tty: serial: sh-sci: Use port ops callbacks

Hi Prabhakar,

On Mon, 16 Jun 2025 at 23:39, Prabhakar <prabhakar.csengg@...il.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
>
> Replace direct calls to internal helpers such as sci_stop_tx(),
> sci_start_tx(), sci_stop_rx(), sci_set_mctrl(), sci_enable_ms(), and
> sci_request_port() with their corresponding port ops callbacks.
>
> This change improves consistency and abstraction across the driver and
> prepares the codebase for adding support for the RSCI driver on the
> Renesas RZ/T2H SoC, which heavily reuses the existing SCI driver.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>

Thanks for your patch!

I am a bit reluctant to increase the number of indirect calls in a
driver that is also used on (old and slow) SH systems...

> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -880,7 +880,7 @@ static void sci_transmit_chars(struct uart_port *port)
>                         sci_serial_out(port, SCSCR, ctrl);
>                 }
>
> -               sci_stop_tx(port);
> +               s->port.ops->stop_tx(port);

RSCI has its own implementation of sci_port_ops.transmit_chars(), so
I think it is better to avoid the overhead of an indirect call, and keep
calling sci_stop_tx() directly.

         }
>  }
>
> @@ -1497,7 +1497,7 @@ static void sci_dma_tx_work_fn(struct work_struct *work)
>  switch_to_pio:
>         uart_port_lock_irqsave(port, &flags);
>         s->chan_tx = NULL;
> -       sci_start_tx(port);
> +       s->port.ops->start_tx(port);

This function is indeed shared by sh-sci and rsci, but still unused
by the latter as it does not support DMA yet.

>         uart_port_unlock_irqrestore(port, flags);
>         return;
>  }
> @@ -2289,8 +2289,8 @@ void sci_shutdown(struct uart_port *port)
>         mctrl_gpio_disable_ms_sync(to_sci_port(port)->gpios);
>
>         uart_port_lock_irqsave(port, &flags);
> -       sci_stop_rx(port);
> -       sci_stop_tx(port);
> +       s->port.ops->stop_rx(port);
> +       s->port.ops->stop_tx(port);

OK.

>         s->ops->shutdown_complete(port);
>         uart_port_unlock_irqrestore(port, flags);
>
> @@ -2684,7 +2684,7 @@ static void sci_set_termios(struct uart_port *port, struct ktermios *termios,
>         }
>         if (port->flags & UPF_HARD_FLOW) {
>                 /* Refresh (Auto) RTS */
> -               sci_set_mctrl(port, port->mctrl);
> +               s->port.ops->set_mctrl(port, port->mctrl);

RSCI has its own implementation of uart_ops.set_termios(), so please
keep the direct call.

>         }
>
>         /*
> @@ -2721,7 +2721,7 @@ static void sci_set_termios(struct uart_port *port, struct ktermios *termios,
>         sci_port_disable(s);
>
>         if (UART_ENABLE_MS(port, termios->c_cflag))
> -               sci_enable_ms(port);
> +               s->port.ops->enable_ms(port);

Likewise.
And once RSCI fully implements uart_ops.set_termios(), I think
it can just reuse sci_enable_ms().

>  }
>
>  void sci_pm(struct uart_port *port, unsigned int state,
> @@ -2827,7 +2827,7 @@ void sci_config_port(struct uart_port *port, int flags)
>                 struct sci_port *sport = to_sci_port(port);
>
>                 port->type = sport->cfg->type;
> -               sci_request_port(port);
> +               sport->port.ops->request_port(port);

Both sh-sci and rsci use sci_request_port() as their
uart_ops.request_port() callbacks, so please use a direct call.

>         }
>  }

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