Re: [PATCH v3 4/8] serial: qcom-geni: Use geni_se_set_rate() for source clock configuration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] serial: PM state desync in qcom_geni_serial_runtime_suspend() error path
- [High] DVFS sequencing violation: clock ungated before voltage restored
- [Medium] tty: serial: qcom-geni: missing power_off rollback on geni_se_set_rate failure
- [Medium] tty: qcom_geni_serial: Inconsistent state on geni_se_set_rate() failure
--

commit c05165e9ed1c294def3b3c9c4244bbde141fcad4
Author: Praveen Talari <praveen.talari@oss.qualcomm.com>

serial: qcom-geni: Use geni_se_set_rate() for source clock configuration

This commit transitions the QCOM GENI serial driver to use
geni_se_set_rate() for source clock configuration. This allows
firmware-managed platforms to handle clock programming centrally by the
GENI core.

> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 4a6f526340b8d..23e423009255b 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
[ ... ]
> @@ -1461,7 +1460,10 @@ static int geni_serial_set_rate(struct geni_se *se, unsigned long baud)
>  
>  	uport->uartclk = clk_rate;
>  	port->clk_rate = clk_rate;
> -	dev_pm_opp_set_rate(uport->dev, clk_rate);
> +	ret = geni_se_set_rate(&port->se, clk_rate);
> +	if (ret)
> +		return ret;
> +

[Severity: Medium]
If geni_se_set_rate() fails here, the hardware clock divider remains
unchanged, but uport->uartclk and port->clk_rate are updated without being
rolled back.

Does this leave the software tracking variables advertising a new baud rate
while the hardware remains at the old rate? On a subsequent
qcom_geni_serial_runtime_resume(), the system might restore the new
port->clk_rate to the hardware, resulting in the source clock running at
the new rate while the hardware still uses the old clock divider from
before the failed termios update, permanently garbling the UART baud rate.

[ ... ]
> @@ -2051,15 +2053,19 @@ static void qcom_geni_serial_remove(struct platform_device *pdev)
>  static int __maybe_unused qcom_geni_serial_runtime_suspend(struct device *dev)
>  {
>  	struct qcom_geni_serial_port *port = dev_get_drvdata(dev);
> +	int ret;
>  
> -	return port->dev_data->power_off ?
> -	       port->dev_data->power_off(&port->se) : 0;
> +	ret = port->dev_data->power_off ?
> +	      port->dev_data->power_off(&port->se) : 0;
> +	if (ret)
> +		return ret;
> +
> +	return geni_se_set_rate(&port->se, 0);
>  }

[Severity: High]
If geni_se_set_rate(&port->se, 0) fails, this returns an error after the
device was successfully powered off.

When a runtime_suspend callback returns an error, the PM core assumes the
suspend failed and keeps the device's runtime state as RPM_ACTIVE. Since the
PM core considers the device logically active, will subsequent UART accesses
bypass runtime_resume() and directly access the physically powered off
hardware, resulting in a fatal bus error (synchronous external abort)?

[ ... ]
>  static int __maybe_unused qcom_geni_serial_runtime_resume(struct device *dev)
>  {
>  	struct qcom_geni_serial_port *port = dev_get_drvdata(dev);
> -	struct uart_port *uport = &port->uport;
>  	int ret;
>  
>  	if (port->dev_data->power_on) {
> @@ -2068,8 +2074,8 @@ static int __maybe_unused qcom_geni_serial_runtime_resume(struct device *dev)
>  			return ret;
>  	}
>  
> -	if (port->se.has_opp && port->clk_rate)
> -		return dev_pm_opp_set_rate(uport->dev, port->clk_rate);
> +	if (port->clk_rate)
> +		return geni_se_set_rate(&port->se, port->clk_rate);

[Severity: High]
Does this sequence violate DVFS sequencing constraints?

During runtime_suspend, the OPP performance state drops via
geni_se_set_rate(&port->se, 0), which drops the voltage but leaves the
underlying clock frequency unchanged. Here during runtime_resume,
port->dev_data->power_on() (which invokes clk_prepare_enable()) is called
before geni_se_set_rate(&port->se, port->clk_rate) restores the voltage.

Could this result in the clock being ungated at its original high frequency
while the voltage is still dropped, leading to hardware lockups or system
reboots?


[Severity: Medium]
Also, if geni_se_set_rate() fails here, it returns an error to the PM core
without calling port->dev_data->power_off() to roll back the resources
enabled by power_on().

Since the PM core marks the device as RPM_SUSPENDED upon a failed resume
without invoking the suspend callback, does this permanently leak
interconnect bandwidth votes and clock enable counts, and leave the device
physically powered?

>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-derive_clk_perf_tbl_from_perf_domain_opp_table-v3-0-5da1096243c9@oss.qualcomm.com?part=4