| 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: <24h2xx6plhjtdsuntyygghalo4cqxdoh6ncexzrxk4kpj5coy2@5jwdqxixdwto>
Date: Fri, 3 Jan 2025 11:11:23 +0100
From: Markus Schneider-Pargmann <msp@...libre.com>
To: Dhruva Gole <d-gole@...com>
Cc: Nishanth Menon <nm@...com>, Tero Kristo <kristo@...nel.org>,
Santosh Shilimkar <ssantosh@...nel.org>, Vignesh Raghavendra <vigneshr@...com>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, Anand Gadiyar <gadiyar@...com>,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
Vishal Mahaveer <vishalm@...com>, Kevin Hilman <khilman@...libre.com>
Subject: Re: [PATCH v4 2/4] firmware: ti_sci: Partial-IO support
Hi Dhruva,
On Fri, Dec 20, 2024 at 05:20:43PM +0530, Dhruva Gole wrote:
> Hi Markus,
>
> On Dec 19, 2024 at 21:02:13 +0100, Markus Schneider-Pargmann wrote:
> > Add support for Partial-IO poweroff. In Partial-IO pins of a few
>
> Maybe add a comma after the In partial-IO, a few pins in this SOC can generate....
>
> > hardware units can generate system wakeups while DDR memory is not
> > powered resulting in a fresh boot of the system. These hardware units in
> > the SoC are always powered so that some logic can detect pin activity.
> >
> > If the system supports Partial-IO as described in the fw capabilities, a
> > sys_off handler is added. This sys_off handler decides if the poweroff
> > is executed by entering normal poweroff or Partial-IO instead. The
> > decision is made by checking if wakeup is enabled on all devices that
> > may wake up the SoC from Partial-IO.
> >
> > The possible wakeup devices are found by checking which devices have the
> > "poweroff" in the list of wakeup-source power states. Only devices that
> > are actually enabled by the user will be considered as an active wakeup
> > source. If none of the wakeup sources is 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
> > wait for 5s and do an emergency_restart afterwards if Partial-IO wasn't
> > entered at that point.
> >
> > 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 | 115 +++++++++++++++++++++++++++++++++++++++++++++-
> > drivers/firmware/ti_sci.h | 5 ++
> > 2 files changed, 119 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/firmware/ti_sci.c b/drivers/firmware/ti_sci.c
> > index ec0c54935ac0d667323d98b86ac9d288b73be6aa..693ac816f8ba3941a9156bd39524099ca476d712 100644
> > --- a/drivers/firmware/ti_sci.c
> > +++ b/drivers/firmware/ti_sci.c
> > @@ -3746,6 +3746,100 @@ static const struct dev_pm_ops ti_sci_pm_ops = {
> > #endif
> > };
> >
> > +/*
> > + * Enter Partial-IO, which disables everything including DDR with only a small
> > + * logic being active for wakeup.
> > + */
> > +static int tisci_enter_partial_io(struct ti_sci_info *info)
>
> Isn't the function naming style in the driver ti_sci_XXX ? You're
> missing one `_` I guess
>
> > +{
> > + struct ti_sci_msg_req_prepare_sleep *req;
> > + struct ti_sci_xfer *xfer;
> > + struct device *dev = info->dev;
> > + int ret = 0;
> > +
> > + xfer = ti_sci_get_one_xfer(info, TI_SCI_MSG_PREPARE_SLEEP,
> > + TI_SCI_FLAG_REQ_GENERIC_NORESPONSE,
> > + sizeof(*req), sizeof(struct ti_sci_msg_hdr));
> > + 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 = TISCI_MSG_VALUE_SLEEP_MODE_PARTIAL_IO;
> > + req->ctx_lo = 0;
> > + req->ctx_hi = 0;
> > + req->debug_flags = 0;
> > +
> > + dev_info(dev, "Entering Partial-IO because a powered wakeup-enabled device was found.\n");
> > +
> > + ret = ti_sci_do_xfer(info, xfer);
> > + if (ret) {
> > + dev_err(dev, "Mbox send fail %d\n", ret);
> > + goto fail;
> > + }
> > +
> > +fail:
> > + ti_sci_put_one_xfer(&info->minfo, xfer);
> > +
> > + return ret;
> > +}
> > +
> > +static bool tisci_canuart_wakeup_enabled(struct ti_sci_info *info)
>
> Add some documentation around this please.
>
> > +{
> > + struct device_node *wakeup_node = NULL;
> > +
> > + for (wakeup_node = of_find_node_with_property(NULL, "wakeup-source");
> > + wakeup_node;
> > + wakeup_node = of_find_node_with_property(wakeup_node, "wakeup-source")) {
> > + struct platform_device *pdev;
> > + int index;
> > +
> > + index = of_property_match_string(wakeup_node, "wakeup-source", "poweroff");
> > + if (index < 0)
> > + continue;
>
> Doesn't the fact that we're inside the for loop already ensure this is > 0?
No, this can return several error codes if the property doesn't have a
value or the string is not found in the property array.
>
> > +
> > + pdev = of_find_device_by_node(wakeup_node);
> > + if (!pdev)
> > + break;
>
> Same here? Would we otherwise be in the loop?
> Just having a quick look here, I could be wrong, please just check once.
The devicetree node doesn't necessarily have a device associated with
it. So I need to check if there is a device for this devicetree node.
I fixed the rest of your comments. Thanks for your review.
Best
Markus
Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)
Powered by blists - more mailing lists