Message-ID: <CAKGbVbucXy+5Sn9U55DY69Lw9bQ+emmN1G4L8DQcUC1wdFSP_Q@mail.gmail.com>
Date: Fri, 26 Jul 2024 14:07:00 +0800
From: Qiang Yu <yuq825@...il.com>
To: Dragan Simic <dsimic@...jaro.org>
Cc: Maxime Ripard <mripard@...nel.org>, dri-devel@...ts.freedesktop.org, 
	lima@...ts.freedesktop.org, maarten.lankhorst@...ux.intel.com, 
	tzimmermann@...e.de, airlied@...il.com, daniel@...ll.ch, 
	linux-kernel@...r.kernel.org, Philip Muller <philm@...jaro.org>, 
	Oliver Smith <ollieparanoid@...tmarketos.org>, Daniel Smith <danct12@...root.org>, 
	stable@...r.kernel.org
Subject: Re: [PATCH] drm/lima: Mark simple_ondemand governor as softdep

Yeah, I agree weakdep is a better choice here. It solves the confusion
of softdep
which the depend module is optional.

But I prefer using weakdep directly instead of creating an aliasing of
it which has
no actual difference.


On Thu, Jul 25, 2024 at 4:21 PM Dragan Simic <dsimic@...jaro.org> wrote:
>
> Hello Qiang,
>
> On 2024-06-26 08:49, Dragan Simic wrote:
> > On 2024-06-26 03:11, Qiang Yu wrote:
> >> On Wed, Jun 26, 2024 at 2:15 AM Dragan Simic <dsimic@...jaro.org>
> >> wrote:
> >>> Just checking, any further thoughts about this patch?
> >>>
> >> I'm OK with this as a temp workaround because it's simple and do no
> >> harm
> >> even it's not perfect. If no other better suggestion for short term,
> >> I'll submit
> >> this at weekend.
> >
> > Thanks.  Just as you described it, it's far from perfect, but it's
> > still
> > fine until there's a better solution, such as harddeps.  I'll continue
> > my
> > research about the possibility for adding harddeps, which would
> > hopefully
> > replace quite a few instances of the softdep (ab)use.
>
> Another option has become available for expressing additional module
> dependencies, weakdeps. [1][2]  Long story short, weakdeps are similar
> to softdeps, in the sense of telling the initial ramdisk utilities to
> include additional kernel modules, but weakdeps result in no module
> loading being performed by userspace.
>
> Maybe "weak" isn't the best possible word choice (arguably, "soft" also
> wasn't the best word choice), but weakdeps should be a better choice for
> use with Lima and governor_simpleondemand, because weakdeps provide the
> required information to the utilities used to generate initial ramdisk,
> while the actual module loading is left to the kernel.
>
> The recent addition of weakdeps renders the previously mentioned
> harddeps
> obsolete, because weakdeps actually do what we need.  Obviously, "weak"
> doesn't go along very well with the actual nature of the dependency
> between
> Lima and governor_simpleondemand, but it's pretty much just the somewhat
> unfortunate word choice.
>
> The support for weakdeps has been already added to the kmod [3][4] and
> Dracut [5] userspace utilities.  I'll hopefully add support for weakdeps
> to mkinitcpio [6] rather soon.
>
> Maybe we could actually add MODULE_HARDDEP() as some kind of syntactic
> sugar, which would currently be an alias for MODULE_WEAKDEP(), so the
> actual hard module dependencies could be expressed properly, and
> possibly
> handled differently in the future, with no need to go back and track all
> such instances of hard module dependencies.
>
> With all this in mind, here's what I'm going to do:
>
> 1) Submit a patch that adds MODULE_HARDDEP() as syntactic sugar
> 2) Implement support for weakdeps in Arch Linux's mkinitcpio [6]
> 3) Depending on what kind of feedback the MODULE_HARDDEP() patch
> receives,
>     I'll submit follow-up patches for Lima and Panfrost, which will swap
>     uses of MODULE_SOFTDEP() with MODULE_HARDDEP() or MODULE_WEAKDEP()
>
> Looking forward to your thoughts.
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/include/linux/module.h?id=61842868de13aa7fd7391c626e889f4d6f1450bf
> [2]
> https://lore.kernel.org/linux-kernel/20240724102349.430078-1-jtornosm@redhat.com/T/#u
> [3]
> https://github.com/kmod-project/kmod/commit/05828b4a6e9327a63ef94df544a042b5e9ce4fe7
> [4]
> https://github.com/kmod-project/kmod/commit/d06712b51404061eef92cb275b8303814fca86ec
> [5]
> https://github.com/dracut-ng/dracut-ng/commit/8517a6be5e20f4a6d87e55fce35ee3e29e2a1150
> [6] https://gitlab.archlinux.org/archlinux/mkinitcpio/mkinitcpio
>
>
> >>> On 2024-06-18 21:22, Dragan Simic wrote:
> >>> > On 2024-06-18 12:33, Dragan Simic wrote:
> >>> >> On 2024-06-18 10:13, Maxime Ripard wrote:
> >>> >>> On Tue, Jun 18, 2024 at 04:01:26PM GMT, Qiang Yu wrote:
> >>> >>>> On Tue, Jun 18, 2024 at 12:33 PM Qiang Yu <yuq825@...il.com> wrote:
> >>> >>>> >
> >>> >>>> > I see the problem that initramfs need to build a module dependency chain,
> >>> >>>> > but lima does not call any symbol from simpleondemand governor module.
> >>> >>>> > softdep module seems to be optional while our dependency is hard one,
> >>> >>>> > can we just add MODULE_INFO(depends, _depends), or create a new
> >>> >>>> > macro called MODULE_DEPENDS()?
> >>> >>
> >>> >> I had the same thoughts, because softdeps are for optional module
> >>> >> dependencies, while in this case it's a hard dependency.  Though,
> >>> >> I went with adding a softdep, simply because I saw no better option
> >>> >> available.
> >>> >>
> >>> >>>> This doesn't work on my side because depmod generates modules.dep
> >>> >>>> by symbol lookup instead of modinfo section. So softdep may be our
> >>> >>>> only
> >>> >>>> choice to add module dependency manually. I can accept the softdep
> >>> >>>> first, then make PM optional later.
> >>> >>
> >>> >> I also thought about making devfreq optional in the Lima driver,
> >>> >> which would make this additional softdep much more appropriate.
> >>> >> Though, I'm not really sure that's a good approach, because not
> >>> >> having working devfreq for Lima might actually cause issues on
> >>> >> some devices, such as increased power consumption.
> >>> >>
> >>> >> In other words, it might be better to have Lima probing fail if
> >>> >> devfreq can't be initialized, rather than having probing succeed
> >>> >> with no working devfreq.  Basically, failed probing is obvious,
> >>> >> while a warning in the kernel log about no devfreq might easily
> >>> >> be overlooked, causing regressions on some devices.
> >>> >>
> >>> >>> It's still super fragile, and depends on the user not changing the
> >>> >>> policy. It should be solved in some other, more robust way.
> >>> >>
> >>> >> I see, but I'm not really sure how to make it more robust?  In
> >>> >> the end, some user can blacklist the simple_ondemand governor
> >>> >> module, and we can't do much about it.
> >>> >>
> >>> >> Introducing harddeps alongside softdeps would make sense from
> >>> >> the design standpoint, but the amount of required changes wouldn't
> >>> >> be trivial at all, on various levels.
> >>> >
> >>> > After further investigation, it seems that the softdeps have
> >>> > already seen a fair amount of abuse for what they actually aren't
> >>> > intended, i.e. resolving hard dependencies.  For example, have
> >>> > a look at the commit d5178578bcd4 (btrfs: directly call into
> >>> > crypto framework for checksumming) [1] and the lines containing
> >>> > MODULE_SOFTDEP() at the very end of fs/btrfs/super.c. [2]
> >>> >
> >>> > If a filesystem driver can rely on the abuse of softdeps, which
> >>> > admittedly are a bit fragile, I think we can follow the same
> >>> > approach, at least for now.
> >>> >
> >>> > With all that in mind, I think that accepting this patch, as well
> >>> > as the related Panfrost patch, [3] should be warranted.  I'd keep
> >>> > investigating the possibility of introducing harddeps in form
> >>> > of MODULE_HARDDEP() and the related support in kmod project,
> >>> > similar to the already existing softdep support, [4] but that
> >>> > will inevitably take a lot of time, both for implementing it
> >>> > and for reaching various Linux distributions, which is another
> >>> > reason why accepting these patches seems reasonable.
> >>> >
> >>> > [1]
> >>> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d5178578bcd4
> >>> > [2]
> >>> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/btrfs/super.c#n2593
> >>> > [3]
> >>> > https://lore.kernel.org/dri-devel/4e1e00422a14db4e2a80870afb704405da16fd1b.1718655077.git.dsimic@manjaro.org/
> >>> > [4]
> >>> > https://git.kernel.org/pub/scm/utils/kernel/kmod/kmod.git/commit/?id=49d8e0b59052999de577ab732b719cfbeb89504d

Powered by blists - more mailing lists