Re: [PATCH v2 5/9] serial: qcom-geni: Use geni_se_set_rate() for source clock configuration

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Device clocks are enabled before restoring the OPP performance state during resume, violating hardware constraints.
- [High] Returning an error from runtime_suspend after hardware is disabled causes state desynchronization and system crashes.
--

commit 23fad340d403394b171aaa614c65cbbd0f718351
Author: Praveen Talari <[email protected]>

    serial: qcom-geni: Use geni_se_set_rate() for source clock configuration
    
    On the SA8255P platform there is no Linux clock handler for the SE
    source clock, so the driver has no way to compute the divider needed
    for a requested baud rate. To work around this, firmware instead
    exposes the SE HW supported source clock frequencies as OPPs on the
    perf domain device.

> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 3633723acef8d..23ecd137c5210 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c

[ ... ]

> @@ -2039,15 +2041,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]
Does returning an error from geni_se_set_rate() here cause a state
desynchronization?

If power_off() succeeds, the hardware (interconnects, clocks, and pin
control) is physically disabled. If geni_se_set_rate() then fails and
we return that error to the PM core, the PM core will leave the device
state as RPM_ACTIVE.

Subsequent accesses from the console or user space would then attempt
to read or write hardware registers without calling runtime_resume.
Because the hardware is already powered off, wouldn't these unclocked
accesses lead to a synchronous external abort and crash the system?

>  }
>  
>  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) {
> @@ -2056,8 +2062,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: Critical]
Are the device clocks being enabled before restoring the OPP performance
state here?

In qcom_geni_serial_runtime_suspend(), the driver disables hardware via
power_off() first, then drops the OPP performance state to 0.

However, in qcom_geni_serial_runtime_resume(), power_on() is called
before geni_se_set_rate(). The power_on() function enables the serial
and core clocks at their full target rate while the power domain is
still at a 0 voltage vote.

Could enabling clocks without sufficient voltage violate hardware
constraints and cause a synchronous external abort on resume? Should
geni_se_set_rate() be called to restore the OPP performance state
before we power on the clocks?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827-derive_clk_perf_tbl_from_perf_domain_opp_table-v2-0-091697dbeb02@oss.qualcomm.com?part=5