linux-kernel - Re: [PATCH v3] PM: sleep: Don't wait for SYNC_STATE

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: <CAGETcx_bzjiLcQSXhCGM1gg2Xy=fgD2FPAdt3ZHDJmY357WnHg@mail.gmail.com>
Date: Fri, 12 Sep 2025 11:29:27 -0700
From: Saravana Kannan <saravanak@...gle.com>
To: Pin-yen Lin <treapking@...omium.org>
Cc: "Rafael J . Wysocki" <rafael@...nel.org>, Len Brown <lenb@...nel.org>, Pavel Machek <pavel@...nel.org>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Danilo Krummrich <dakr@...nel.org>, 
	linux-kernel@...r.kernel.org, Chen-Yu Tsai <wenst@...omium.org>, 
	Hsin-Te Yuan <yuanhsinte@...omium.org>, linux-pm@...r.kernel.org
Subject: Re: [PATCH v3] PM: sleep: Don't wait for SYNC_STATE_ONLY device links

On Fri, Sep 12, 2025 at 7:48 AM Pin-yen Lin <treapking@...omium.org> wrote:
>
> Device links with DL_FLAG_SYNC_STATE_ONLY should not affect suspend
> and resume, and functions like device_reorder_to_tail() and
> device_link_add() doesn't try to reorder the consumers with such flag.
>
> However, dpm_wait_for_consumers() and dpm_wait_for_suppliers() doesn't
> check this flag before triggering dpm_wait, leading to potential hang
> during suspend/resume.
>
> This can be reproduced on MT8186 Corsola Chromebook with devicetree like:
>
> usb-a-connector {
>         compatible = "usb-a-connector";
>         port {
>                 usb_a_con: endpoint {
>                         remote-endpoint = <&usb_hs>;
>                 };
>         };
> };
>
> usb_host {
>         compatible = "mediatek,mt8186-xhci", "mediatek,mtk-xhci";
>         port {
>                 usb_hs: endpoint {
>                         remote-endpoint = <&usb_a_con>;
>                 };
>         };
> };

We've had SYNC_STATE_ONLY devlink for years in the kernel. Also,
SYNC_STATE_ONLY device links get deleted once the consumer device
probes. You also can't suspend a device before it probes.

Why are you hitting this issue only now? Something doesn't sound
right. I'm asking this because I'm wondering if some other recent
change broke any of my statements/design decisions above.

Not to fix this, but to also ensure better suspend/resume ordering in
the future, you might want to add a "post-init-suppliers" to the real
supplier node here and point it to the real consumer here. This will
also help break the cycle here. Because even with this fix, all it
does is avoid enforcing any ordering between these two devices. It is
just working based on the order in DT, which you shouldn't count on
for correctness.

Thanks,
Saravana

>
> In this case, the two nodes form a cycle and a SYNC_STATE_ONLY devlink
> between usb_host (supplier) and usb-a-connector (consumer) is created.
>
> Export device_link_flag_is_sync_state_only() and use it to check this in
> dpm_wait_for_consumers() and dpm_wait_for_suppliers() to fix this.
>
> Fixes: 05ef983e0d65a ("driver core: Add device link support for SYNC_STATE_ONLY flag")
> Signed-off-by: Pin-yen Lin <treapking@...omium.org>
> ---
>
> Changes in v3:
> - Squash to one patch and fix the export approach
>
> Changes in v2:
> - Update commit message
> - Use device_link_flag_is_sync_state_only()
>
>  drivers/base/base.h       | 1 +
>  drivers/base/core.c       | 2 +-
>  drivers/base/power/main.c | 6 ++++--
>  3 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/base.h b/drivers/base/base.h
> index 123031a757d9..80415b140ce7 100644
> --- a/drivers/base/base.h
> +++ b/drivers/base/base.h
> @@ -248,6 +248,7 @@ void device_links_driver_cleanup(struct device *dev);
>  void device_links_no_driver(struct device *dev);
>  bool device_links_busy(struct device *dev);
>  void device_links_unbind_consumers(struct device *dev);
> +bool device_link_flag_is_sync_state_only(u32 flags);
>  void fw_devlink_drivers_done(void);
>  void fw_devlink_probing_done(void);
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index d22d6b23e758..741aa0571fc7 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -287,7 +287,7 @@ static bool device_is_ancestor(struct device *dev, struct device *target)
>  #define DL_MARKER_FLAGS                (DL_FLAG_INFERRED | \
>                                  DL_FLAG_CYCLE | \
>                                  DL_FLAG_MANAGED)
> -static inline bool device_link_flag_is_sync_state_only(u32 flags)
> +inline bool device_link_flag_is_sync_state_only(u32 flags)
>  {
>         return (flags & ~DL_MARKER_FLAGS) == DL_FLAG_SYNC_STATE_ONLY;
>  }
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index 2ea6e05e6ec9..73a1916170ae 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -282,7 +282,8 @@ static void dpm_wait_for_suppliers(struct device *dev, bool async)
>          * walking.
>          */
>         list_for_each_entry_rcu_locked(link, &dev->links.suppliers, c_node)
> -               if (READ_ONCE(link->status) != DL_STATE_DORMANT)
> +               if (READ_ONCE(link->status) != DL_STATE_DORMANT &&
> +                   !device_link_flag_is_sync_state_only(link->flags))
>                         dpm_wait(link->supplier, async);
>
>         device_links_read_unlock(idx);
> @@ -339,7 +340,8 @@ static void dpm_wait_for_consumers(struct device *dev, bool async)
>          * unregistration).
>          */
>         list_for_each_entry_rcu_locked(link, &dev->links.consumers, s_node)
> -               if (READ_ONCE(link->status) != DL_STATE_DORMANT)
> +               if (READ_ONCE(link->status) != DL_STATE_DORMANT &&
> +                   !device_link_flag_is_sync_state_only(link->flags))
>                         dpm_wait(link->consumer, async);
>
>         device_links_read_unlock(idx);
> --
> 2.51.0.384.g4c02a37b29-goog
>