linux-kernel - Re: [PATCH v2 2/2] remoteproc: imx_rproc: handle system off for i.MX7ULP

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CANLsYky_WPo6UXZXAMMLpQ-duf9OxZYvb4PRw4zUgisMcjWOvQ@mail.gmail.com>
Date: Wed, 21 Aug 2024 10:58:10 -0600
From: Mathieu Poirier <mathieu.poirier@...aro.org>
To: Daniel Baluta <daniel.baluta@...il.com>
Cc: "Peng Fan (OSS)" <peng.fan@....nxp.com>, Bjorn Andersson <andersson@...nel.org>, 
	Shawn Guo <shawnguo@...nel.org>, Sascha Hauer <s.hauer@...gutronix.de>, 
	Pengutronix Kernel Team <kernel@...gutronix.de>, Fabio Estevam <festevam@...il.com>, 
	Daniel Baluta <daniel.baluta@....com>, Iuliana Prodan <iuliana.prodan@....com>, 
	Marek Vasut <marex@...x.de>, linux-remoteproc@...r.kernel.org, imx@...ts.linux.dev, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	Peng Fan <peng.fan@....com>
Subject: Re: [PATCH v2 2/2] remoteproc: imx_rproc: handle system off for i.MX7ULP

On Wed, 21 Aug 2024 at 02:32, Daniel Baluta <daniel.baluta@...il.com> wrote:
>
> Hello Mathieu,
>
> I've talked to Peng and if my understanding is correct I think the patch is OK.
> Maybe we can split the patch in two:
> * first, adding the power off callback with explanations.
> * second, adding the restart callback with explanations.
>
> And also add a more detailed explanation.
>
> Power off and restart are totally different operations and are not complementary
> as I thought in the beginning. There are not like suspend/resume for example.
>
> > >  static int imx_rproc_probe(struct platform_device *pdev)
> > >  {
> > >       struct device *dev = &pdev->dev;
> > > @@ -1104,6 +1122,24 @@ static int imx_rproc_probe(struct platform_device *pdev)
> > >       if (rproc->state != RPROC_DETACHED)
> > >               rproc->auto_boot = of_property_read_bool(np, "fsl,auto-boot");
> > >
> > > +     if (of_device_is_compatible(dev->of_node, "fsl,imx7ulp-cm4")) {
> > > +             ret = devm_register_sys_off_handler(dev, SYS_OFF_MODE_POWER_OFF_PREPARE,
> > > +                                                 SYS_OFF_PRIO_DEFAULT,
> > > +                                                 imx_rproc_sys_off_handler, rproc);
> >
> > Why does the mailbox needs to be set up again when the system is going down...
>
> Scenario: We call Linux *shutdown -P * command to power off the machine.
>
> At this point mailbox TX operation is configured as *blocking*. Power
> off is done via
> an atomic notifier call which doesn't allow blocking. If we do so we
> will endup in a kernel crash.
>
> So, at this moment we setup again the mailboxes configuring them with
> *non-blocking* option.
>
> >
> > > +             if (ret) {
> > > +                     dev_err(dev, "register power off handler failure\n");
> > > +                     goto err_put_clk;
> > > +             }
> > > +
> > > +             ret = devm_register_sys_off_handler(dev, SYS_OFF_MODE_RESTART_PREPARE,
> > > +                                                 SYS_OFF_PRIO_DEFAULT,
> > > +                                                 imx_rproc_sys_off_handler, rproc);
> >
> > ... and why does it need to be free'd when the system is going up?
>
> System is not going up here. System is running and we do a reboot.
>

Ah!  This is still on the downward path - I thought
"SYS_OFF_MODE_RESTART_PREPARE" was associated with the upward path,
when the system is restarted after a shutdown or a reboot.  That is
where the confusion came from.

> Scenario: We call Linux *shutdown -r* command to reboot the machine.
>
> Similarly, mailboxes are already set and configured as *blocking*. We
> cannot use the mailboxes
> as they are because reboot is done via an atomic notifier which if we
> call a blocking function it will endup in crash.
>
> So, we need to free the existing mailbox and create new ones with the
> *non-blocking* options.
>
> I think this is really fair to me. The one thing, I admit we must work
> on, create a better commit message.
>
> What do you say? Does this work for you?
>

Things are clear now and I agree with the implementation.  No need for
two separate patches, just a re-worked changelog.

Thanks,
Mathieu

> Thanks a lot for your help!