Re: [PATCH v2] serial: qcom-geni: Fix blocked task

["Alexey Klimov" <alexey.klimov@linaro.org>]

On Wed Sep 17, 2025 at 2:04 AM BST, Krzysztof Kozlowski wrote:
> Revert commit 1afa70632c39 ("serial: qcom-geni: Enable PM runtime for
> serial driver") and its dependent commit 86fa39dd6fb7 ("serial:
> qcom-geni: Enable Serial on SA8255p Qualcomm platforms") because the
> first one causes regression - hang task on Qualcomm RB1 board (QRB2210)
> and unable to use serial at all during normal boot:
>
>   INFO: task kworker/u16:0:12 blocked for more than 42 seconds.
>         Not tainted 6.17.0-rc1-00004-g53e760d89498 #9
>   "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
>   task:kworker/u16:0   state:D stack:0     pid:12    tgid:12    ppid:2      task_flags:0x4208060 flags:0x00000010
>   Workqueue: async async_run_entry_fn
>   Call trace:
>    __switch_to+0xe8/0x1a0 (T)
>    __schedule+0x290/0x7c0
>    schedule+0x34/0x118
>    rpm_resume+0x14c/0x66c
>    rpm_resume+0x2a4/0x66c
>    rpm_resume+0x2a4/0x66c
>    rpm_resume+0x2a4/0x66c
>    __pm_runtime_resume+0x50/0x9c
>    __driver_probe_device+0x58/0x120
>    driver_probe_device+0x3c/0x154
>    __driver_attach_async_helper+0x4c/0xc0
>    async_run_entry_fn+0x34/0xe0
>    process_one_work+0x148/0x290
>    worker_thread+0x2c4/0x3e0
>    kthread+0x118/0x1c0
>    ret_from_fork+0x10/0x20
>
> The issue was reported on 12th of August and was ignored by author of
> commits introducing issue for two weeks.  Only after complaining author
> produced a fix which did not work, so if original commits cannot be
> reliably fixed for 5 weeks, they obviously are buggy and need to be
> dropped.
>
> Fixes: 1afa70632c39 ("serial: qcom-geni: Enable PM runtime for serial driver")
> Reported-by: Alexey Klimov <alexey.klimov@linaro.org>
> Closes: https://lore.kernel.org/all/DC0D53ZTNOBU.E8LSD5E5Z8TX@linaro.org/
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Tested-by: Alexey Klimov <alexey.klimov@linaro.org>
Reviewed-by: Alexey Klimov <alexey.klimov@linaro.org>



> ---
>
> Changes in v2:
> 1. Correct reference to cause (proper commit) in the commit msg.
> ---
>  drivers/tty/serial/qcom_geni_serial.c | 176 +++-----------------------
>  1 file changed, 16 insertions(+), 160 deletions(-)
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> index 0fdda3a1e70b..7c5befe5490d 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -14,7 +14,6 @@
>  #include <linux/irq.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> -#include <linux/pm_domain.h>
>  #include <linux/pm_opp.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
> @@ -102,16 +101,10 @@
>  #define DMA_RX_BUF_SIZE		2048
>  
>  static DEFINE_IDA(port_ida);
> -#define DOMAIN_IDX_POWER	0
> -#define DOMAIN_IDX_PERF		1
>  
>  struct qcom_geni_device_data {
>  	bool console;
>  	enum geni_se_xfer_mode mode;
> -	struct dev_pm_domain_attach_data pd_data;
> -	int (*resources_init)(struct uart_port *uport);
> -	int (*set_rate)(struct uart_port *uport, unsigned int baud);
> -	int (*power_state)(struct uart_port *uport, bool state);
>  };
>  
>  struct qcom_geni_private_data {
> @@ -149,7 +142,6 @@ struct qcom_geni_serial_port {
>  
>  	struct qcom_geni_private_data private_data;
>  	const struct qcom_geni_device_data *dev_data;
> -	struct dev_pm_domain_list *pd_list;
>  };
>  
>  static const struct uart_ops qcom_geni_console_pops;
> @@ -1301,42 +1293,6 @@ static int geni_serial_set_rate(struct uart_port *uport, unsigned int baud)
>  	return 0;
>  }
>  
> -static int geni_serial_set_level(struct uart_port *uport, unsigned int baud)
> -{
> -	struct qcom_geni_serial_port *port = to_dev_port(uport);
> -	struct device *perf_dev = port->pd_list->pd_devs[DOMAIN_IDX_PERF];
> -
> -	/*
> -	 * The performance protocol sets UART communication
> -	 * speeds by selecting different performance levels
> -	 * through the OPP framework.
> -	 *
> -	 * Supported perf levels for baudrates in firmware are below
> -	 * +---------------------+--------------------+
> -	 * |  Perf level value   |  Baudrate values   |
> -	 * +---------------------+--------------------+
> -	 * |      300            |      300           |
> -	 * |      1200           |      1200          |
> -	 * |      2400           |      2400          |
> -	 * |      4800           |      4800          |
> -	 * |      9600           |      9600          |
> -	 * |      19200          |      19200         |
> -	 * |      38400          |      38400         |
> -	 * |      57600          |      57600         |
> -	 * |      115200         |      115200        |
> -	 * |      230400         |      230400        |
> -	 * |      460800         |      460800        |
> -	 * |      921600         |      921600        |
> -	 * |      2000000        |      2000000       |
> -	 * |      3000000        |      3000000       |
> -	 * |      3200000        |      3200000       |
> -	 * |      4000000        |      4000000       |
> -	 * +---------------------+--------------------+
> -	 */
> -
> -	return dev_pm_opp_set_level(perf_dev, baud);
> -}
> -
>  static void qcom_geni_serial_set_termios(struct uart_port *uport,
>  					 struct ktermios *termios,
>  					 const struct ktermios *old)
> @@ -1355,7 +1311,7 @@ static void qcom_geni_serial_set_termios(struct uart_port *uport,
>  	/* baud rate */
>  	baud = uart_get_baud_rate(uport, termios, old, 300, 8000000);
>  
> -	ret = port->dev_data->set_rate(uport, baud);
> +	ret = geni_serial_set_rate(uport, baud);
>  	if (ret)
>  		return;
>  
> @@ -1642,27 +1598,8 @@ static int geni_serial_resources_off(struct uart_port *uport)
>  	return 0;
>  }
>  
> -static int geni_serial_resource_state(struct uart_port *uport, bool power_on)
> +static int geni_serial_resource_init(struct qcom_geni_serial_port *port)
>  {
> -	return power_on ? geni_serial_resources_on(uport) : geni_serial_resources_off(uport);
> -}
> -
> -static int geni_serial_pwr_init(struct uart_port *uport)
> -{
> -	struct qcom_geni_serial_port *port = to_dev_port(uport);
> -	int ret;
> -
> -	ret = dev_pm_domain_attach_list(port->se.dev,
> -					&port->dev_data->pd_data, &port->pd_list);
> -	if (ret <= 0)
> -		return -EINVAL;
> -
> -	return 0;
> -}
> -
> -static int geni_serial_resource_init(struct uart_port *uport)
> -{
> -	struct qcom_geni_serial_port *port = to_dev_port(uport);
>  	int ret;
>  
>  	port->se.clk = devm_clk_get(port->se.dev, "se");
> @@ -1707,10 +1644,10 @@ static void qcom_geni_serial_pm(struct uart_port *uport,
>  		old_state = UART_PM_STATE_OFF;
>  
>  	if (new_state == UART_PM_STATE_ON && old_state == UART_PM_STATE_OFF)
> -		pm_runtime_resume_and_get(uport->dev);
> +		geni_serial_resources_on(uport);
>  	else if (new_state == UART_PM_STATE_OFF &&
>  		 old_state == UART_PM_STATE_ON)
> -		pm_runtime_put_sync(uport->dev);
> +		geni_serial_resources_off(uport);
>  
>  }
>  
> @@ -1813,16 +1750,13 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
>  	port->se.dev = &pdev->dev;
>  	port->se.wrapper = dev_get_drvdata(pdev->dev.parent);
>  
> -	ret = port->dev_data->resources_init(uport);
> +	ret = geni_serial_resource_init(port);
>  	if (ret)
>  		return ret;
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (!res) {
> -		ret = -EINVAL;
> -		goto error;
> -	}
> -
> +	if (!res)
> +		return -EINVAL;
>  	uport->mapbase = res->start;
>  
>  	uport->rs485_config = qcom_geni_rs485_config;
> @@ -1834,26 +1768,19 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
>  	if (!data->console) {
>  		port->rx_buf = devm_kzalloc(uport->dev,
>  					    DMA_RX_BUF_SIZE, GFP_KERNEL);
> -		if (!port->rx_buf) {
> -			ret = -ENOMEM;
> -			goto error;
> -		}
> +		if (!port->rx_buf)
> +			return -ENOMEM;
>  	}
>  
>  	port->name = devm_kasprintf(uport->dev, GFP_KERNEL,
>  			"qcom_geni_serial_%s%d",
>  			uart_console(uport) ? "console" : "uart", uport->line);
> -	if (!port->name) {
> -		ret = -ENOMEM;
> -		goto error;
> -	}
> +	if (!port->name)
> +		return -ENOMEM;
>  
>  	irq = platform_get_irq(pdev, 0);
> -	if (irq < 0) {
> -		ret = irq;
> -		goto error;
> -	}
> -
> +	if (irq < 0)
> +		return irq;
>  	uport->irq = irq;
>  	uport->has_sysrq = IS_ENABLED(CONFIG_SERIAL_QCOM_GENI_CONSOLE);
>  
> @@ -1875,18 +1802,16 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
>  			IRQF_TRIGGER_HIGH, port->name, uport);
>  	if (ret) {
>  		dev_err(uport->dev, "Failed to get IRQ ret %d\n", ret);
> -		goto error;
> +		return ret;
>  	}
>  
>  	ret = uart_get_rs485_mode(uport);
>  	if (ret)
>  		return ret;
>  
> -	devm_pm_runtime_enable(port->se.dev);
> -
>  	ret = uart_add_one_port(drv, uport);
>  	if (ret)
> -		goto error;
> +		return ret;
>  
>  	if (port->wakeup_irq > 0) {
>  		device_init_wakeup(&pdev->dev, true);
> @@ -1896,15 +1821,11 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
>  			device_init_wakeup(&pdev->dev, false);
>  			ida_free(&port_ida, uport->line);
>  			uart_remove_one_port(drv, uport);
> -			goto error;
> +			return ret;
>  		}
>  	}
>  
>  	return 0;
> -
> -error:
> -	dev_pm_domain_detach_list(port->pd_list);
> -	return ret;
>  }
>  
>  static void qcom_geni_serial_remove(struct platform_device *pdev)
> @@ -1917,31 +1838,6 @@ static void qcom_geni_serial_remove(struct platform_device *pdev)
>  	device_init_wakeup(&pdev->dev, false);
>  	ida_free(&port_ida, uport->line);
>  	uart_remove_one_port(drv, &port->uport);
> -	dev_pm_domain_detach_list(port->pd_list);
> -}
> -
> -static int __maybe_unused qcom_geni_serial_runtime_suspend(struct device *dev)
> -{
> -	struct qcom_geni_serial_port *port = dev_get_drvdata(dev);
> -	struct uart_port *uport = &port->uport;
> -	int ret = 0;
> -
> -	if (port->dev_data->power_state)
> -		ret = port->dev_data->power_state(uport, false);
> -
> -	return ret;
> -}
> -
> -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 = 0;
> -
> -	if (port->dev_data->power_state)
> -		ret = port->dev_data->power_state(uport, true);
> -
> -	return ret;
>  }
>  
>  static int qcom_geni_serial_suspend(struct device *dev)
> @@ -1979,46 +1875,14 @@ static int qcom_geni_serial_resume(struct device *dev)
>  static const struct qcom_geni_device_data qcom_geni_console_data = {
>  	.console = true,
>  	.mode = GENI_SE_FIFO,
> -	.resources_init = geni_serial_resource_init,
> -	.set_rate = geni_serial_set_rate,
> -	.power_state = geni_serial_resource_state,
>  };
>  
>  static const struct qcom_geni_device_data qcom_geni_uart_data = {
>  	.console = false,
>  	.mode = GENI_SE_DMA,
> -	.resources_init = geni_serial_resource_init,
> -	.set_rate = geni_serial_set_rate,
> -	.power_state = geni_serial_resource_state,
> -};
> -
> -static const struct qcom_geni_device_data sa8255p_qcom_geni_console_data = {
> -	.console = true,
> -	.mode = GENI_SE_FIFO,
> -	.pd_data = {
> -		.pd_flags = PD_FLAG_DEV_LINK_ON,
> -		.pd_names = (const char*[]) { "power", "perf" },
> -		.num_pd_names = 2,
> -	},
> -	.resources_init = geni_serial_pwr_init,
> -	.set_rate = geni_serial_set_level,
> -};
> -
> -static const struct qcom_geni_device_data sa8255p_qcom_geni_uart_data = {
> -	.console = false,
> -	.mode = GENI_SE_DMA,
> -	.pd_data = {
> -		.pd_flags = PD_FLAG_DEV_LINK_ON,
> -		.pd_names = (const char*[]) { "power", "perf" },
> -		.num_pd_names = 2,
> -	},
> -	.resources_init = geni_serial_pwr_init,
> -	.set_rate = geni_serial_set_level,
>  };
>  
>  static const struct dev_pm_ops qcom_geni_serial_pm_ops = {
> -	SET_RUNTIME_PM_OPS(qcom_geni_serial_runtime_suspend,
> -			   qcom_geni_serial_runtime_resume, NULL)
>  	SYSTEM_SLEEP_PM_OPS(qcom_geni_serial_suspend, qcom_geni_serial_resume)
>  };
>  
> @@ -2027,18 +1891,10 @@ static const struct of_device_id qcom_geni_serial_match_table[] = {
>  		.compatible = "qcom,geni-debug-uart",
>  		.data = &qcom_geni_console_data,
>  	},
> -	{
> -		.compatible = "qcom,sa8255p-geni-debug-uart",
> -		.data = &sa8255p_qcom_geni_console_data,
> -	},
>  	{
>  		.compatible = "qcom,geni-uart",
>  		.data = &qcom_geni_uart_data,
>  	},
> -	{
> -		.compatible = "qcom,sa8255p-geni-uart",
> -		.data = &sa8255p_qcom_geni_uart_data,
> -	},
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, qcom_geni_serial_match_table);