| 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: <20240826163455.bp3a6lxtimiquzq5@ascertain>
Date: Mon, 26 Aug 2024 11:34:55 -0500
From: Nishanth Menon <nm@...com>
To: Kevin Hilman <khilman@...libre.com>
CC: Tero Kristo <kristo@...nel.org>, Santosh Shilimkar <ssantosh@...nel.org>,
<linux-arm-kernel@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
Vibhore Vardhan <vibhore@...com>, Dhruva Gole <d-gole@...com>,
Akashdeep Kaur
<a-kaur@...com>,
Markus Schneider-Pargmann <msp@...libre.com>
Subject: Re: [PATCH v10 2/4] firmware: ti_sci: Add system suspend and resume
call
On 08:39-20240814, Kevin Hilman wrote:
> From: Vibhore Vardhan <vibhore@...com>
>
[...]
> + *
> + * Return: 0 if all went well, else returns appropriate error value.
> + */
> +static int ti_sci_cmd_prepare_sleep(const struct ti_sci_handle *handle, u8 mode,
> + u32 ctx_lo, u32 ctx_hi, u32 debug_flags)
> +{
> + struct ti_sci_info *info;
> + struct ti_sci_msg_req_prepare_sleep *req;
> + struct ti_sci_msg_hdr *resp;
> + struct ti_sci_xfer *xfer;
> + struct device *dev;
> + int ret = 0;
> +
> + if (IS_ERR(handle))
> + return PTR_ERR(handle);
> + if (!handle)
> + return -EINVAL;
> +
> + info = handle_to_ti_sci_info(handle);
> + dev = info->dev;
> +
> + xfer = ti_sci_get_one_xfer(info, TI_SCI_MSG_PREPARE_SLEEP,
> + TI_SCI_FLAG_REQ_ACK_ON_PROCESSED,
> + sizeof(*req), sizeof(*resp));
> + if (IS_ERR(xfer)) {
> + ret = PTR_ERR(xfer);
> + dev_err(dev, "Message alloc failed(%d)\n", ret);
> + return ret;
> + }
> +
> + req = (struct ti_sci_msg_req_prepare_sleep *)xfer->xfer_buf;
> + req->mode = mode;
> + req->ctx_lo = ctx_lo;
> + req->ctx_hi = ctx_hi;
> + req->debug_flags = debug_flags;
> +
> + ret = ti_sci_do_xfer(info, xfer);
> + if (ret) {
> + dev_err(dev, "Mbox send fail %d\n", ret);
> + goto fail;
> + }
> +
> + resp = (struct ti_sci_msg_hdr *)xfer->xfer_buf;
> +
> + ret = ti_sci_is_response_ack(resp) ? 0 : -ENODEV;
Here and other functions:
I think adding a dev_err for a NAKed message is probably useful for debug.
> +
> +fail:
> + ti_sci_put_one_xfer(&info->minfo, xfer);
> +
> + return ret;
> +}
> +
> /**
> * ti_sci_msg_cmd_query_fw_caps() - Get the FW/SoC capabilities
> * @handle: Pointer to TI SCI handle
> @@ -1710,6 +1770,58 @@ static int ti_sci_msg_cmd_query_fw_caps(const struct ti_sci_handle *handle,
> return ret;
> }
>
> +/**
> + * ti_sci_cmd_set_io_isolation() - Enable IO isolation in LPM
> + * @handle: Pointer to TI SCI handle
> + * @state: The desired state of the IO isolation
> + *
> + * Return: 0 if all went well, else returns appropriate error value.
> + */
> +static int ti_sci_cmd_set_io_isolation(const struct ti_sci_handle *handle,
> + u8 state)
I probably might have preferred to have set_io_isolation as a
different patch, but OK, it is not too hard to see the hookup
commonality.
> +{
> + struct ti_sci_info *info;
> + struct ti_sci_msg_req_set_io_isolation *req;
> + struct ti_sci_msg_hdr *resp;
> + struct ti_sci_xfer *xfer;
> + struct device *dev;
> + int ret = 0;
> +
> + if (IS_ERR(handle))
> + return PTR_ERR(handle);
> + if (!handle)
> + return -EINVAL;
> +
> + info = handle_to_ti_sci_info(handle);
> + dev = info->dev;
> +
> + xfer = ti_sci_get_one_xfer(info, TI_SCI_MSG_SET_IO_ISOLATION,
> + TI_SCI_FLAG_REQ_ACK_ON_PROCESSED,
> + sizeof(*req), sizeof(*resp));
> + if (IS_ERR(xfer)) {
> + ret = PTR_ERR(xfer);
> + dev_err(dev, "Message alloc failed(%d)\n", ret);
> + return ret;
> + }
> + req = (struct ti_sci_msg_req_set_io_isolation *)xfer->xfer_buf;
> + req->state = state;
> +
> + ret = ti_sci_do_xfer(info, xfer);
> + if (ret) {
> + dev_err(dev, "Mbox send fail %d\n", ret);
> + goto fail;
> + }
> +
> + resp = (struct ti_sci_msg_hdr *)xfer->xfer_buf;
> +
> + ret = ti_sci_is_response_ack(resp) ? 0 : -ENODEV;
here as well.
> +
> +fail:
> + ti_sci_put_one_xfer(&info->minfo, xfer);
> +
> + return ret;
> +}
> +
> static int ti_sci_cmd_core_reboot(const struct ti_sci_handle *handle)
> {
> struct ti_sci_info *info;
> @@ -3321,6 +3433,79 @@ static int tisci_reboot_handler(struct sys_off_data *data)
> return NOTIFY_BAD;
> }
>
> +#ifdef CONFIG_SUSPEND
> +static int ti_sci_prepare_system_suspend(struct ti_sci_info *info)
> +{
> + u8 mode;
> +
> + /*
> + * Map and validate the target Linux suspend state to TISCI LPM.
> + * Default is to let Device Manager select the low power mode.
> + */
> + switch (pm_suspend_target_state) {
> + case PM_SUSPEND_MEM:
> + if (info->fw_caps & MSG_FLAG_CAPS_LPM_DM_MANAGED)
> + mode = TISCI_MSG_VALUE_SLEEP_MODE_DM_MANAGED;
> + else
> + /* DM Managed is not supported by the firmware. */
dev_err print is useful for debug?
> + return -EOPNOTSUPP;
> + break;
> + default:
> + /*
> + * Do not fail if we don't have action to take for a
> + * specific suspend mode.
> + */
> + return 0;
> + }
> +
> + return ti_sci_cmd_prepare_sleep(&info->handle, mode, 0, 0, 0);
Umm.. context address is 0x0?
> +}
> +
> +static int ti_sci_suspend(struct device *dev)
> +{
> + struct ti_sci_info *info = dev_get_drvdata(dev);
> + int ret;
> +
> + ret = ti_sci_prepare_system_suspend(info);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int ti_sci_suspend_noirq(struct device *dev)
> +{
> + struct ti_sci_info *info = dev_get_drvdata(dev);
> + int ret = 0;
> +
> + ret = ti_sci_cmd_set_io_isolation(&info->handle, TISCI_MSG_VALUE_IO_ENABLE);
> + if (ret)
> + return ret;
Does this add any value here?
> + dev_dbg(dev, "%s: set isolation: %d\n", __func__, ret);
> +
> + return 0;
> +}
> +
> +static int ti_sci_resume_noirq(struct device *dev)
> +{
> + struct ti_sci_info *info = dev_get_drvdata(dev);
> + int ret = 0;
> +
> + ret = ti_sci_cmd_set_io_isolation(&info->handle, TISCI_MSG_VALUE_IO_DISABLE);
> + if (ret)
> + return ret;
Same..
> + dev_dbg(dev, "%s: disable isolation: %d\n", __func__, ret);
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops ti_sci_pm_ops = {
> + .suspend = ti_sci_suspend,
> + .suspend_noirq = ti_sci_suspend_noirq,
> + .resume_noirq = ti_sci_resume_noirq,
> +};
> +#endif /* CONFIG_SUSPEND */
Mostly, I am seeing usage with CONFIG_PM_SLEEP instead of
CONFIG_SUSPEND. is that a better option? and further, usage tends to be
as follows:
static int __maybe_unused ti_sci_resume_noirq(struct device *dev)
static const struct dev_pm_ops ti_sci_pm_ops = {
#ifdef CONFIG_PM_SLEEP
.suspend = ti_sci_suspend,
.suspend_noirq = ti_sci_suspend_noirq,
.resume_noirq = ti_sci_resume_noirq,
#endif
};
Keeps the #ifdeffery just localized to one place.
Thoughts?
> +
> /* Description for K2G */
> static const struct ti_sci_desc ti_sci_pmmc_k2g_desc = {
> .default_host_id = 2,
> @@ -3490,6 +3675,9 @@ static struct platform_driver ti_sci_driver = {
> .name = "ti-sci",
> .of_match_table = of_match_ptr(ti_sci_of_match),
> .suppress_bind_attrs = true,
> +#ifdef CONFIG_SUSPEND
> + .pm = &ti_sci_pm_ops,
> +#endif
> },
> };
> module_platform_driver(ti_sci_driver);
[...]
--
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D
Powered by blists - more mailing lists