linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: John Stultz <john.stultz@linaro.org>
To: Rob Clark <robdclark@gmail.com>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>,
	Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
	Vinod Koul <vinod.koul@linaro.org>,
	Srini Kandagatla <srinivas.kandagatla@linaro.org>,
	Amit Pundir <amit.pundir@linaro.org>,
	YongQin Liu <yongqin.liu@linaro.org>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>
Subject: Re: Regression: arm64: dts: sdm845-db845c: make firmware filenames follow linux-firmware
Date: Wed, 5 May 2021 14:14:10 -0700	[thread overview]
Message-ID: <CALAqxLXn6wFBAxRkThxWg5RvTuFEX80kHPt8BVja1CpAB-qzGA@mail.gmail.com> (raw)
In-Reply-To: <CAF6AEGvZMe5BTBtu+=koAeDQF9AKDa0CTEWJTSV8+CW2jsh7fA@mail.gmail.com>

On Wed, May 5, 2021 at 1:01 PM Rob Clark <robdclark@gmail.com> wrote:
> On Wed, May 5, 2021 at 12:35 PM John Stultz <john.stultz@linaro.org> wrote:
> >
> > On Wed, May 5, 2021 at 8:04 AM Rob Clark <robdclark@gmail.com> wrote:
> > > On Tue, May 4, 2021 at 11:37 PM John Stultz <john.stultz@linaro.org> wrote:
> > > > Hey Dmitry, Bjorn,
> > > >   I wanted to raise a regression I caught in the merge window on db845c.
> > > >
> > > > I was seeing troubles with audio and while there are a few other
> > > > pending fixes needed, they did not seem to work for me. So I spent
> > > > some time bisecting things down and found the problematic commit was
> > > > 7443ff06da45 ("arm64: dts: sdm845-db845c: make firmware filenames
> > > > follow linux-firmware").
> > > >
> > > > It seems for systems using the old firmware filenames, this will break
> > > > dependent devices on adsp_pas and cdsp_pas nodes.
> > > >
> > > > Now, obviously updating the firmware files in userland should resolve
> > > > this, but it adds the complexity that we can't just replace the
> > > > firmware files because older LTS kernels will look for the old names,
> > > > while newer kernels will look for the new names. We can add both files
> > > > to the system images, but then there is some confusion on which
> > > > version of the firmware files are being used where.
> > > >
> > > > So yes, we should align with linux-firmware file names, but I think
> > > > more care is needed for this sort of thing as it has the potential to
> > > > break folks, and this isn't the first time around we've had similar
> > > > firmware name changes break us.
> > > >
> > > > So I'm working on fixing this by including both filenames in userland,
> > > > so we probably don't need a revert here, but *please* maybe take more
> > > > care on this sort of change.
> > > >
> > >
> > > It is rather more difficult than you think, because if you try the
> > > wrong path you could end up waiting with a timeout.. we have
> > > shenanigans to work around that for gpu fw in drm/msm to avoid this
> > > sort of regression with people using downstream firmware trees.  I'd
> > > like to rip that out at some point, but I suppose doing so would be
> > > problematic for folks doing upstream kernel on android devices.
> > >
> > > Maybe there is some way to add support to simultaneously
> > > request_firmware for two different paths at the same time.. not sure
> > > how that would work from the PoV of the usermode helper path.
> > >
> > > It really is a lot of pain to deal with downstream firmware layout..
> >
> > Yeah, but on other platforms, the kernel has to meet and deal with the
> > hardware, firmware and userland as it exists in the world.
> > It would be quite a thing if an upstream kernel change required a new
> > BIOS update which then made the system incompatible with the most
> > recent LTS.
>
> Does downstream exist? :-P

The converse is if upstream has no devices on which it boots, is it relevant?
Both statements are extreme and silly, I think. :)

> We first ran into this issue with GPU firmware when it was originally
> pushed to linux-firmware.  Upstream linux-firmware wanted it moved to
> qcom subdirectory.  At this point, now upstream and downstream
> firmware are incompatible in their directory layout.  You can't really
> expect the kernel to support the downstream layout but not the
> upstream layout.  If the upstream kernel has to choose, it will go
> with the upstream linux-firmware layout.

So my apologies, I worry I've come off a bit hectoring, and my
admiration and appreciation for your work hasn't come through.  I'm
very sympathetic to the difficulties trying to balance supporting the
crazy things that get shipped and creating a maintainable approach for
the future, *and* why the latter is rightly prioritized.

And while the gpu firmware case was one previous example of similar
trouble, we've had a number of other casual changes to DTS node names
break us before (due to sysfs paths then shifting and userland then
not monitoring the right sysfs files) on numerous boards (not just
db845c), so I'm sorry if my general frustration seemed a bit too
pointed.

> Seriously though, the extra 60sec timeout delays is not an option.  I
> don't really see any good option other than somehow teaching
> request_firmware how to look for two (or multiple) paths at the same
> time.  I'm not sure (a) if that is possible (ie. without breaking
> compatibility with usermode helper), and (b) how others would feel
> about adding more complexity to the firmware loader to deal with
> downstream firmware trees.  If others are open to that idea, I'm all
> for it.
>
> And tbf, the BIOS update example is a *bit* of a stretch, since that
> is often not something owners of the hw can do.  In this case, it is
> just a matter of putting some symlinks in the downstream /lib/firmware
> to make it work with both cases.
>

I think in this case, it's just the existing convention that previous
kernels used was that firmware on the db845c used the .mdt extension.
To me, changing the db845c dts to use .mbn breaking convention wasn't
the best choice.

Instead, we could have left it as mdt in the db845c dts, and made it
so when updating linux-firmware files, the .mbn files would need to be
copied (or symlinked) to .mdt (which has to be done now anyway) to
match existing convention, and it would have had less negative impact.

For new boards and dts files, go with the new convention! But we
should avoid casually breaking existing ones on existing boards.
Again, it's done and we have a fix, so no worries now, but just things
to consider going forward.

thanks
-john

  parent reply	other threads:[~2021-05-05 21:14 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-05  6:20 John Stultz
2021-05-05 15:08 ` Rob Clark
2021-05-05 19:35   ` John Stultz
2021-05-05 20:05     ` Rob Clark
2021-05-05 20:07       ` Dmitry Baryshkov
2021-05-05 21:14       ` John Stultz [this message]
2021-05-05 21:02     ` Bjorn Andersson
2021-05-05 18:43 ` Dmitry Baryshkov
2021-05-05 19:06 ` Bjorn Andersson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CALAqxLXn6wFBAxRkThxWg5RvTuFEX80kHPt8BVja1CpAB-qzGA@mail.gmail.com \
    --to=john.stultz@linaro.org \
    --cc=amit.pundir@linaro.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=dmitry.baryshkov@linaro.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=robdclark@gmail.com \
    --cc=srinivas.kandagatla@linaro.org \
    --cc=vinod.koul@linaro.org \
    --cc=yongqin.liu@linaro.org \
    --subject='Re: Regression: arm64: dts: sdm845-db845c: make firmware filenames follow linux-firmware' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).