linux-kernel - Re: [PATCH] driver core: fw_devlink: Don't warn in fw_devlink_dev_sync

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: <CAJZ5v0g_QHQzYTN4Sra_gGQXmRDxUPEjDMyOMB4+PxHcD6vfsQ@mail.gmail.com>
Date: Fri, 26 Sep 2025 19:08:16 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Ulf Hansson <ulf.hansson@...aro.org>
Cc: Saravana Kannan <saravanak@...gle.com>, Diederik de Haas <didi.debian@...ow.org>, 
	"Rafael J . Wysocki" <rafael@...nel.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, linux-pm@...r.kernel.org, 
	Geert Uytterhoeven <geert@...ux-m68k.org>, 
	Nicolas Frattaroli <nicolas.frattaroli@...labora.com>, Heiko Stuebner <heiko@...ech.de>, 
	Sebastian Reichel <sebastian.reichel@...labora.com>, Sebin Francis <sebin.francis@...com>, 
	Tomi Valkeinen <tomi.valkeinen@...asonboard.com>, Jon Hunter <jonathanh@...dia.com>, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] driver core: fw_devlink: Don't warn in fw_devlink_dev_sync_state()

On Fri, Sep 26, 2025 at 2:05 PM Ulf Hansson <ulf.hansson@...aro.org> wrote:
>
> On Thu, 25 Sept 2025 at 23:49, Saravana Kannan <saravanak@...gle.com> wrote:
> >
> > On Thu, Sep 25, 2025 at 10:52 AM Diederik de Haas <didi.debian@...ow.org> wrote:
> > >
> > > On Thu Sep 25, 2025 at 4:26 PM CEST, Ulf Hansson wrote:
> > > > On Thu, 25 Sept 2025 at 15:59, Diederik de Haas <didi.debian@...ow.org> wrote:
> > > >> On Thu Sep 25, 2025 at 1:59 PM CEST, Ulf Hansson wrote:
> > > >> > Due to the wider deployment of the ->sync_state() support, for PM domains
> > > >> > for example, we are receiving reports about the messages that are being
> > > >> > logged in fw_devlink_dev_sync_state(). In particular as they are at the
> > > >> > warning level, which doesn't seem correct.
> > > >> >
> > > >> > Even if it certainly is useful to know that the ->sync_state() condition
> > > >> > could not be met, there may be nothing wrong with it. For example, a driver
> > > >> > may be built as module and are still waiting to be initialized/probed.
> > > >>
> > > >> "there may be nothing wrong with it" doesn't sound very convincing.
> > > >> So there *can* be something wrong with it, so warning sounds
> > > >> appropriate? If there is (certainly) something wrong with it, I expect
> > > >> an error.
> > > >
> > > > Sorry if I was too vague. See more below.
> > > >
> > > >> FWIW: most of my drivers/modules are built as modules.
> > > >> I do seem to run into 'problems' more then average because of that, but
> > > >> to me it just signals there is something wrong ... which should be
> > > >> fixed. Not silenced.
> > > >
> > > > Well, why is it wrong to have drivers being built as modules? They
> >
> > IIUC/Remember Kconfig correctly, FW_DEVLINK_SYNC_STATE_TIMEOUT should
> > be off by default? (if I don't give any default, what ends up
> > happening?)
>
> As "FW_DEVLINK_SYNC_STATE_STRICT" is default, the "sync_state()
> pending due..." print gets printed for a lot of consumer devices that
> have not been probed yet.
>
> >
> > If we can assume, timeout won't happen by default, then the only
> > people seeing this warning are people setting the flag or setting the
> > command line param. So I'm okay with making it an info.
>
> I think you got this wrong. It's the default behaviour that triggers
> lots of prints.
>
> >
> > But the "sync_state() pending due to" is for the default behavior. I'm
> > assuming without the sync_state() supported added to power domains,
> > the ones getting this warning would have been powered off? But after
> > the sync_state() support, it's not powered off anymore. That can cause
> > increased power usage. Seems like something worth warning about.
>
> Yes, I certainly agree, but...
>
> To me, it kind of sounds like we should change to use
> FW_DEVLINK_SYNC_STATE_TIMEOUT as the default behaviour. In that case,
> I don't think there is a need to change the log-level for
> "sync_state() pending due to..".
>
> >
> > Also, another thing I want to understand is, for all the reports you
> > are getting for "sync_state() pending due to", is it actually correct
> > to have them pending? Or can fw_devlink do better in those cases and
> > turn them off? It could be a driver fix/fw_devlink fix depending on
> > the issue.
>
> There are some cases that we potentially could fix, in particular for
> when a genpd-provider, provides multiple power-domains per device-node
> (#power-domain-cells = <1>;). But let's discuss that separately.
>
> >
> > TL;DR:
> > 1. For timeout messages, since timeout isn't default behavior, I'm
> > okay making it an info. But
> > 2. For pending sync state messages, I think it should remain as a warning.
>
> See above.
>
> This means drivers being built as modules will trigger a lot of
> warnings, in the default behavior. Is this really what we want?

No, at least I don't.

Printing a warn level message about anything that is not a clear
functional issue is seriously problematic IMV.

As for whether or not the messages should be info level or debug
level, it also depends on the amount of noise generated by them.  IMV,
generating a lot of noise at the info level is not really useful and
may cause some actually useful information to be missed.