Re: [PATCH v1 1/2] driver core: Add fw_devlink.sync_state command line param

[Doug Anderson]

Hi,

On Fri, Mar 3, 2023 at 4:53 PM Saravana Kannan <saravanak@xxxxxxxxxx> wrote:
>
> > IMO better would be to say something like when sync_state=strict that
> > you'll just leave resources in a high power state
>
> But this statement is not true either. Just because a device driver
> has a sync_state() doesn't mean the device was left in a powered on
> state by the bootloader.

Though I guess it's theoretically possible that a device using
sync_state will leave resources in a _lower_ power state until
sync_state is reached, I'm skeptical if that actually happens. Can you
point to any examples? The sync state docs
"sysfs-devices-state_synced" actually document that the common case is
when the bootloader left a resource enabled and we won't disable the
resource until sync_state is reached. That's almost certainly a higher
power state.

I would also point to one of the users of sync_state: the interconnect
framework. Take a look at commit b1d681d8d324 ("interconnect: Add sync
state support"). You can see that in icc_node_add() if we can't read
the bandwidth at bootup we end up at the max (INT_MAX). That's exactly
the case we actually hit for Qualcomm. It's not that we just avoid
touching the resources until sync state is reached--we actually max it
out.

In general, something feels a bit awkward here in defining this as
"however the bootloader left it". That concept makes sense for things
where we need to manage a handoff from the bootloader for the kernel,
but it's not the answer for all things. The bootloader's job is to
boot the system and get out of the way, not to init all resources. It
only inits resources that it cares about. That means if the bootloader
displays a splash screen then it might init resources for the display.
if it doesn't display a splash screen it might not. The kernel needs
to handle either case.

In general, the problems being solved with sync_state seem to require
resources to be left on and in high power until sync state is reached.
Today, you define that as "the state the bootloader left it in".
...but if the bootloader didn't leave it in a high power state then
you'd need to change this definition.

If you truly want to couch the verbiage, I guess I'd be OK with saying
"when sync_state=strict that you'll _LIKELY_ leave resources in a high
power state if sync_state is never reached"


> > While I don't object to this being a kernel command line flag, the
> > default should also be a Kconfig option. The kernel command line is
> > not a great place for general configuration. As we jam too much stuff
> > in the kernel command line it gets unwieldy quickly. IMO:
> >
> > * Kconfig: the right place for stuff for config options that a person
> > building the kernel might want to tweak.
> >
> > * Kernel command line: the right place for a user of a pre-built
> > kernel to tweak; also (sometimes) the right place for the bootloader
> > to pass info to the kernel; also a good place for debug options that a
> > kernel engineer might want to tweak w/out rebuilding the kernel.
> >
> > In this case it makes sense for the person building the kernel to
> > choose a default that makes sense for the hardware that their kernel
> > is targetting. It can also make sense for a user of a pre-built kernel
> > to tweak this if their hardware isn't working correctly. Thus it makes
> > sense for Kconfig to choose the default and the kernel command line to
> > override.
>
> I don't mind adding a Kconfig to select the default behavior, but
> maybe as a separate patch in the future so if there's any debate about
> that, you'll at least get this option.

I don't mind it being a separate patch, but it should be part of the
initial series.


> > Specifically, I think this warning message gets printed out after
> > we've given up waiting for devices to show up. At this point
> > -EPROBE_DEFER becomes an error that we won't retry.
>
> This is not true. We will always retry on an -EPROBE_DEFER, even after timeout.

OK, so I think this is the main point of contention here, so let's get
to the bottom of it first and then we can address anything else.

I guess I'm trying to figure out what "deferred_probe_timeout" is
supposed to be about. From reading
driver_deferred_probe_check_state(), I see that the idea is that once
the timeout expires then we'll start returning -ETIMEDOUT when we used
to return -EPROBE_DEFER. I guess I mispoke then. You're correct that
-EPROBE_DEFER will still be retried. That being said, things that used
to be retired (because they returned -EPROBE_DEFER) will now become
permanent/non-retired errors (because they return -ETIMEDOUT).

My point is that if we ever actually hit that case (where we return
-ETIMEDOUT instead of -EPROBE_DEFER) we really enter a state where
it's not going to be great to load any more drivers. Once a driver
failed to probe (because it got back an -ETIMEDOUT instead of
-EPROBE_DEFER) then the user needs to manually unbind/rebind the
device to retry. That's not a good state.

So the above is the crux of my argument that once
"deferred_probe_timeout" fires that the system really isn't in good
shape to load more drivers.

So looking more carefully, I think I can understand where you're
coming from. Specifically I note that very few subsystems have "opted
in" to the deferred_probe_timeout on ToT. I can also see that recently
you made the effort to delete driver_deferred_probe_check_state(),
though those were reverted. That means that, as it stands, devices
will _probably_ not end up with the problem I describe above (unless
they depend on a subsystem that has opted-in). ...and, if your plans
come to fruition, then eventually we'll never hit it.

Where does that leave us? I guess I will step back on my assertion
that when the timeout fires that drivers can't load anymore. Certainly
the state that ToT Linux is in is confusing. "deferred_probe_timeout"
is still documented (in kernel-parameters.txt) to cause us to "give
up" waiting for dependencies. ...and it still causes a few subsystems
to give up. ...but I guess it mostly works.


> > I would perhaps also make it sound a little scarier since,
>
> I definitely don't want to make it sound scarier and get everyone to
> enable the timeout by default without actually knowing if it has a
> power impact on their system.
>
> > IMO, this
> > is a problem that really shouldn't be "shipped" if this is an embedded
> > kernel. Maybe something like:
>
> This is how it's shipped on all Android devices in the past 2 years.
> So it's not a global problem like you make it to be.

You're saying devices _shipped_ but booted up where devices never
reached sync_state? ...and that's not a power consumption problem???
I'm not saying that the sync_state concept couldn't ship, I'm saying
that if this printout shows up in boot logs that it's highly likely
there's a problem that needs to be fixed and that's causing extra
power consumption. That's why I want the printout to sound scarier.

-Doug