| 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
| ||
|
Message-ID: <20240730122801.jzo5ahkurxaexwcm@ambiance>
Date: Tue, 30 Jul 2024 07:28:01 -0500
From: Nishanth Menon <nm@...com>
To: Markus Schneider-Pargmann <msp@...libre.com>
CC: Tero Kristo <kristo@...nel.org>, Santosh Shilimkar <ssantosh@...nel.org>,
Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Vignesh Raghavendra <vigneshr@...com>,
Vibhore Vardhan <vibhore@...com>, Kevin Hilman <khilman@...libre.com>,
Dhruva
Gole <d-gole@...com>, <linux-arm-kernel@...ts.infradead.org>,
<devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/6] firmware: ti_sci: Partial-IO support
On 10:00-20240729, Markus Schneider-Pargmann wrote:
> Add support for Partial-IO poweroff. In Partial-IO pins of a few modules
> can generate system wakeups while DDR memory is not powered resulting in
> a fresh boot of the system. The modules that can be wakeup sources are
> defined by the devicetree.
>
> Only wakeup sources that are actually enabled by the user will be
> considered as a an active wakeup source. If none of the wakeup sources
> are enabled the system will do a normal poweroff. If at least one wakeup
> source is enabled it will instead send a TI_SCI_MSG_PREPARE_SLEEP
> message from the sys_off handler. Sending this message will result in an
> immediate shutdown of the system. No execution is expected after this
> point. The code will enter an infinite loop.
>
> The wakeup source device nodes are gathered during probe. But they are
> only resolved to the actual devices in the sys_off handler, if they
> exist. If they do not exist, they are ignored.
>
> A short documentation about Partial-IO can be found in section 6.2.4.5
> of the TRM at
> https://www.ti.com/lit/pdf/spruiv7
>
> Signed-off-by: Markus Schneider-Pargmann <msp@...libre.com>
> ---
> drivers/firmware/ti_sci.c | 160 +++++++++++++++++++++++++++++++++-----
> drivers/firmware/ti_sci.h | 34 ++++++++
> 2 files changed, 175 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/firmware/ti_sci.c b/drivers/firmware/ti_sci.c
> index 160968301b1f..ba2e56da0215 100644
> --- a/drivers/firmware/ti_sci.c
> +++ b/drivers/firmware/ti_sci.c
> @@ -99,6 +99,9 @@ struct ti_sci_desc {
> * @node: list head
> * @host_id: Host ID
> * @users: Number of users of this instance
> + * @nr_wakeup_sources: Number of device nodes in wakeup_source_nodes
> + * @wakeup_source_nodes: Array of all device_nodes listed as wakeup sources in
> + * the devicetree
> */
> struct ti_sci_info {
> struct device *dev;
> @@ -116,6 +119,9 @@ struct ti_sci_info {
> u8 host_id;
> /* protected by ti_sci_list_mutex */
> int users;
> +
> + int nr_wakeup_sources;
> + struct device_node **wakeup_source_nodes;
> };
>
> #define cl_to_ti_sci_info(c) container_of(c, struct ti_sci_info, cl)
> @@ -392,10 +398,13 @@ static void ti_sci_put_one_xfer(struct ti_sci_xfers_info *minfo,
> static inline int ti_sci_do_xfer(struct ti_sci_info *info,
> struct ti_sci_xfer *xfer)
> {
> + struct ti_sci_msg_hdr *hdr = (struct ti_sci_msg_hdr *)xfer->tx_message.buf;
> int ret;
> int timeout;
> struct device *dev = info->dev;
> bool done_state = true;
> + bool response_expected = !!(hdr->flags & (TI_SCI_FLAG_REQ_ACK_ON_PROCESSED |
> + TI_SCI_FLAG_REQ_ACK_ON_RECEIVED));
I think a separate patch to introduce a no_response expected patch would
make sense on which we build tisci_sys_off_handler in the next patch?
>
> ret = mbox_send_message(info->chan_tx, &xfer->tx_message);
> if (ret < 0)
> @@ -403,25 +412,27 @@ static inline int ti_sci_do_xfer(struct ti_sci_info *info,
>
> ret = 0;
>
> - if (system_state <= SYSTEM_RUNNING) {
> - /* And we wait for the response. */
> - timeout = msecs_to_jiffies(info->desc->max_rx_timeout_ms);
> - if (!wait_for_completion_timeout(&xfer->done, timeout))
> - ret = -ETIMEDOUT;
> - } else {
> - /*
> - * If we are !running, we cannot use wait_for_completion_timeout
> - * during noirq phase, so we must manually poll the completion.
> - */
> - ret = read_poll_timeout_atomic(try_wait_for_completion, done_state,
> - done_state, 1,
> - info->desc->max_rx_timeout_ms * 1000,
> - false, &xfer->done);
> - }
> + if (response_expected) {
How about a goto?
if (!response_expected)
goto no_response;
> + if (system_state <= SYSTEM_RUNNING) {
> + /* And we wait for the response. */
> + timeout = msecs_to_jiffies(info->desc->max_rx_timeout_ms);
> + if (!wait_for_completion_timeout(&xfer->done, timeout))
> + ret = -ETIMEDOUT;
> + } else {
> + /*
> + * If we are !running, we cannot use wait_for_completion_timeout
> + * during noirq phase, so we must manually poll the completion.
> + */
> + ret = read_poll_timeout_atomic(try_wait_for_completion, done_state,
> + done_state, 1,
> + info->desc->max_rx_timeout_ms * 1000,
> + false, &xfer->done);
> + }
>
> - if (ret == -ETIMEDOUT)
> - dev_err(dev, "Mbox timedout in resp(caller: %pS)\n",
> - (void *)_RET_IP_);
> + if (ret == -ETIMEDOUT)
> + dev_err(dev, "Mbox timedout in resp(caller: %pS)\n",
> + (void *)_RET_IP_);
> + }
>
no_response:
> /*
> * NOTE: we might prefer not to need the mailbox ticker to manage the
> @@ -3262,6 +3273,82 @@ static int tisci_reboot_handler(struct sys_off_data *data)
> return NOTIFY_BAD;
> }
>
[...]
> +static int tisci_sys_off_handler(struct sys_off_data *data)
> +{
> + struct ti_sci_info *info = data->cb_data;
> + int i;
> + int ret;
> + bool enter_partial_io = false;
> +
> + for (i = 0; i != info->nr_wakeup_sources; ++i) {
> + struct platform_device *pdev =
> + of_find_device_by_node(info->wakeup_source_nodes[i]);
> +
> + if (!pdev)
> + continue;
> +
> + if (device_may_wakeup(&pdev->dev)) {
> + dev_dbg(info->dev, "%pOFp identified as wakeup source\n",
> + info->wakeup_source_nodes[i]);
> + enter_partial_io = true;
> + }
> + }
> +
> + if (!enter_partial_io)
> + return NOTIFY_DONE;
> +
> + ret = tisci_enter_partial_io(info);
> +
> + if (ret) {
> + dev_err(info->dev,
> + "Failed to enter Partial-IO %pe, trying to do an emergency restart\n",
> + ERR_PTR(ret));
> + emergency_restart();
> + }
> +
> + while (1);
Why not fall through OR go through emergency_restart (since there is
no fall through for shutdown path) if it acks, but actually fails to
enter LPM state after a dt described or a default timeout period?
> +
> + return NOTIFY_DONE;
> +}
> +
> /* Description for K2G */
> static const struct ti_sci_desc ti_sci_pmmc_k2g_desc = {
> .default_host_id = 2,
> @@ -3398,6 +3485,35 @@ static int ti_sci_probe(struct platform_device *pdev)
> goto out;
> }
>
> + if (of_property_read_bool(dev->of_node, "ti,partial-io-wakeup-sources")) {
You should probably check on TISCI_MSG_QUERY_FW_CAPS[1] if
Partial IO on low power mode is supported as well? if there is a
mismatch, report so?
> + info->nr_wakeup_sources =
> + of_count_phandle_with_args(dev->of_node,
> + "ti,partial-io-wakeup-sources",
> + NULL);
> + info->wakeup_source_nodes =
> + devm_kzalloc(dev, sizeof(*info->wakeup_source_nodes),
> + GFP_KERNEL);
> +
> + for (i = 0; i != info->nr_wakeup_sources; ++i) {
> + struct device_node *devnode =
> + of_parse_phandle(dev->of_node,
> + "ti,partial-io-wakeup-sources",
> + i);
> + info->wakeup_source_nodes[i] = devnode;
Curious: Don't we need to maintain reference counting for the devnode
if CONFIG_OF_DYNAMIC?
[...]
[1] https://software-dl.ti.com/tisci/esd/latest/2_tisci_msgs/general/core.html#tisci-msg-query-fw-caps
--
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D
Powered by blists - more mailing lists