linux-kernel - Re: [PATCH v3 1/5] PM: sleep: Resume children after resuming the parent

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJZ5v0gMHU71drwOYatFhUcDFKXb9=vTo=JFFYfDabYBdrqWLg@mail.gmail.com>
Date: Wed, 7 May 2025 18:43:47 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Jon Hunter <jonathanh@...dia.com>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, "Rafael J. Wysocki" <rjw@...ysocki.net>, 
	Linux PM <linux-pm@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>, 
	Alan Stern <stern@...land.harvard.edu>, Ulf Hansson <ulf.hansson@...aro.org>, 
	Johan Hovold <johan@...nel.org>, Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>, 
	Saravana Kannan <saravanak@...gle.com>, 
	"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>
Subject: Re: [PATCH v3 1/5] PM: sleep: Resume children after resuming the parent

On Wed, May 7, 2025 at 5:40 PM Jon Hunter <jonathanh@...dia.com> wrote:
>
>
>
> On 07/05/2025 15:56, Rafael J. Wysocki wrote:
>
> ...
>
> > So apparently one of the children has not been suspended yet when this
> > happens.  That's fine because it should be suspended at one point and
> > the parent suspend should be unblocked, so it looks like the child
> > suspend doesn't complete for some reason.
> >
> >> I will enable the PM_ADVANCED_DEBUG and confirm that making the I2C
> >> itself non-async works.
> >
> > What probably happens is that after the "PM: sleep: Suspend async
> > parents after suspending children" , the i2c clients are suspended
> > upfront (because they have no children) and when one of them has
> > suspended, it triggers a parent suspend.  The parent suspend then
> > waits for the other client to complete suspending, but that cannot
> > make progress for some reason.
> >
> > Before that patch, the i2c clients would have suspended only after all
> > of the "sync" devices following them in dpm_list had been suspended
> > (the list is processed in the reverse order during suspend), so it
> > looks like there is a hidden dependency between one of the i2c clients
> > and a "sync" device.
> >
> > If the above supposition is right, flagging the i2c client as "sync"
> > will make the problem go away.
>
> So all the I2C controllers are 'sync' devices ...
>
> $ cat /sys/class/i2c-dev/i2c-*/power/async
> disabled
> disabled
> disabled
> disabled
> disabled
> disabled
> disabled
>
> The I2C clients on the problematic I2C controller are all 'async'
> devices ...
>
> $ cat /sys/class/i2c-dev/i2c-2/device/2-*/power/async
> enabled
> enabled
> enabled
>
> Setting all these to 'disabled' fixes the problem. However, also just
> setting the 'cypd4226' device to 'sync' fixes the problem (the ina3221
> devices seem to be fine being async). The 'cypd4226' device is
> interesting, because this one is a USB Type-C controller and there is a
> circular dependency between the Type-C and USB PHY (see
> arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts).

Circular dependencies are problematic for suspend/resume in general.
I wonder if fw_devlink can resolve this?

> If I make the following change then this does fix it ...
>
> diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c
> b/drivers/usb/typec/ucsi/ucsi_ccg.c
> index f01e4ef6619d..e9a9df1431af 100644
> --- a/drivers/usb/typec/ucsi/ucsi_ccg.c
> +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
> @@ -1483,6 +1483,8 @@ static int ucsi_ccg_probe(struct i2c_client *client)
>
>          i2c_set_clientdata(client, uc);
>
> +       device_disable_async_suspend(uc->dev);
> +
>          pm_runtime_set_active(uc->dev);
>          pm_runtime_enable(uc->dev);
>          pm_runtime_use_autosuspend(uc->dev);
>
> Is this the right fix for this?

At least as a stop-gap, yes.

In order to enable async suspend for a device, one needs to at least
assume with sufficiently high confidence that it will be safe to
reorder it with respect to any other device in the system except for
the devices having known dependencies on the device in question.
Those known dependencies either are parent-child connections or they
need to be represented by device links.

In this particular case, it is painfully clear that the suspend of the
device in question cannot be reordered with respect to at least one
other device where the dependency is not known in the above sense.

Thus the device in question should not be allowed to suspend asynchronously.

Would it be better to represent the dependency in question via a
device link?  Yes, it would, but until that happens, disabling async
suspend is the right thing to do IMV.