linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Regression: arm64: dts: sdm845-db845c: make firmware filenames follow linux-firmware
@ 2021-05-05  6:20 John Stultz
  2021-05-05 15:08 ` Rob Clark
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: John Stultz @ 2021-05-05  6:20 UTC (permalink / raw)
  To: Bjorn Andersson, Dmitry Baryshkov
  Cc: Vinod Koul, Srini Kandagatla, Amit Pundir, YongQin Liu, linux-arm-msm

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.

thanks
-john

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Regression: arm64: dts: sdm845-db845c: make firmware filenames follow linux-firmware
  2021-05-05  6:20 Regression: arm64: dts: sdm845-db845c: make firmware filenames follow linux-firmware John Stultz
@ 2021-05-05 15:08 ` Rob Clark
  2021-05-05 19:35   ` John Stultz
  2021-05-05 18:43 ` Dmitry Baryshkov
  2021-05-05 19:06 ` Bjorn Andersson
  2 siblings, 1 reply; 9+ messages in thread
From: Rob Clark @ 2021-05-05 15:08 UTC (permalink / raw)
  To: John Stultz
  Cc: Bjorn Andersson, Dmitry Baryshkov, Vinod Koul, Srini Kandagatla,
	Amit Pundir, YongQin Liu, linux-arm-msm

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..

BR,
-R

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Regression: arm64: dts: sdm845-db845c: make firmware filenames follow linux-firmware
  2021-05-05  6:20 Regression: arm64: dts: sdm845-db845c: make firmware filenames follow linux-firmware John Stultz
  2021-05-05 15:08 ` Rob Clark
@ 2021-05-05 18:43 ` Dmitry Baryshkov
  2021-05-05 19:06 ` Bjorn Andersson
  2 siblings, 0 replies; 9+ messages in thread
From: Dmitry Baryshkov @ 2021-05-05 18:43 UTC (permalink / raw)
  To: John Stultz
  Cc: Bjorn Andersson, Vinod Koul, Srini Kandagatla, Amit Pundir,
	YongQin Liu, linux-arm-msm

Hi

On Wed, 5 May 2021 at 09:20, 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.

Point taken.


-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Regression: arm64: dts: sdm845-db845c: make firmware filenames follow linux-firmware
  2021-05-05  6:20 Regression: arm64: dts: sdm845-db845c: make firmware filenames follow linux-firmware John Stultz
  2021-05-05 15:08 ` Rob Clark
  2021-05-05 18:43 ` Dmitry Baryshkov
@ 2021-05-05 19:06 ` Bjorn Andersson
  2 siblings, 0 replies; 9+ messages in thread
From: Bjorn Andersson @ 2021-05-05 19:06 UTC (permalink / raw)
  To: John Stultz
  Cc: Dmitry Baryshkov, Vinod Koul, Srini Kandagatla, Amit Pundir,
	YongQin Liu, linux-arm-msm

On Wed 05 May 01:20 CDT 2021, John Stultz 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.
> 

Due to the workings of the userspace firmware loader fallback I
unfortunately don't see any reasonable way to deal with this.
Introducing a fallback mechanism would suffer from an unavoidable 60
second delay waiting for the first choice to timeout, or if we used the
non-userspace-assisted method we'd probably give up too early.

> So I'm working on fixing this by including both filenames in userland,

The kernel will detect in runtime if you pass it a squashed or split
firmware package, the suffix has no significance. So if you have the
need to go back and forth just make sure you have a symlink that points
the .mbn to the .mdt (or vice versa).

> so we probably don't need a revert here, but *please* maybe take more
> care on this sort of change.
> 

Yes, I should have paid more attention when we merged the original
firmware-name to avoid this issue. Sorry for not getting this right from
the beginning.

Regards,
Bjorn

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Regression: arm64: dts: sdm845-db845c: make firmware filenames follow linux-firmware
  2021-05-05 15:08 ` Rob Clark
@ 2021-05-05 19:35   ` John Stultz
  2021-05-05 20:05     ` Rob Clark
  2021-05-05 21:02     ` Bjorn Andersson
  0 siblings, 2 replies; 9+ messages in thread
From: John Stultz @ 2021-05-05 19:35 UTC (permalink / raw)
  To: Rob Clark
  Cc: Bjorn Andersson, Dmitry Baryshkov, Vinod Koul, Srini Kandagatla,
	Amit Pundir, YongQin Liu, linux-arm-msm

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.

And yes, I'm very sympathetic that it's a pain (In the timekeeping
code, there's a ton of messy duplicative code required to keep
timespec alongside of ktime_t representations of time so we can be
fast no matter which interface is used). :)

And again, I can work around this one.  It's just that this isn't the
first time, so I want to nudge folks to have a broader view and
consider these issues a little more when making changes (not just in
the kernel, but in why the names in linux-firmware are different then
what's on devices in the field, etc).

thanks
-john

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Regression: arm64: dts: sdm845-db845c: make firmware filenames follow linux-firmware
  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
  2021-05-05 21:02     ` Bjorn Andersson
  1 sibling, 2 replies; 9+ messages in thread
From: Rob Clark @ 2021-05-05 20:05 UTC (permalink / raw)
  To: John Stultz
  Cc: Bjorn Andersson, Dmitry Baryshkov, Vinod Koul, Srini Kandagatla,
	Amit Pundir, YongQin Liu, linux-arm-msm

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

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.

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.

BR,
-R

> And yes, I'm very sympathetic that it's a pain (In the timekeeping
> code, there's a ton of messy duplicative code required to keep
> timespec alongside of ktime_t representations of time so we can be
> fast no matter which interface is used). :)
>
> And again, I can work around this one.  It's just that this isn't the
> first time, so I want to nudge folks to have a broader view and
> consider these issues a little more when making changes (not just in
> the kernel, but in why the names in linux-firmware are different then
> what's on devices in the field, etc).
>
> thanks
> -john

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Regression: arm64: dts: sdm845-db845c: make firmware filenames follow linux-firmware
  2021-05-05 20:05     ` Rob Clark
@ 2021-05-05 20:07       ` Dmitry Baryshkov
  2021-05-05 21:14       ` John Stultz
  1 sibling, 0 replies; 9+ messages in thread
From: Dmitry Baryshkov @ 2021-05-05 20:07 UTC (permalink / raw)
  To: Rob Clark
  Cc: John Stultz, Bjorn Andersson, Vinod Koul, Srini Kandagatla,
	Amit Pundir, YongQin Liu, linux-arm-msm

On Wed, 5 May 2021 at 23:01, 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
>
> 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.
>
> 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.

In fact we had this in place for a while for RB5 (qrb5165, sm8250),
when there was no official firmware release.


-- 
With best wishes
Dmitry

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Regression: arm64: dts: sdm845-db845c: make firmware filenames follow linux-firmware
  2021-05-05 19:35   ` John Stultz
  2021-05-05 20:05     ` Rob Clark
@ 2021-05-05 21:02     ` Bjorn Andersson
  1 sibling, 0 replies; 9+ messages in thread
From: Bjorn Andersson @ 2021-05-05 21:02 UTC (permalink / raw)
  To: John Stultz
  Cc: Rob Clark, Dmitry Baryshkov, Vinod Koul, Srini Kandagatla,
	Amit Pundir, YongQin Liu, linux-arm-msm

On Wed 05 May 14:35 CDT 2021, John Stultz 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.

The whole problem is that the kernel _didn't_ work with the firmware
that was published to the world, we used the wrong firmware name and
because you and apparently the LT releases doesn't follow linux-firmware
this was not noticed until now.

> 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.
> 

Certainly so, but even if we didn't have to deal with OEM-signed,
per-board firmware files we'd have to hard code the file names in the
drivers - and would have run into this problem anyways.

> And yes, I'm very sympathetic that it's a pain (In the timekeeping
> code, there's a ton of messy duplicative code required to keep
> timespec alongside of ktime_t representations of time so we can be
> fast no matter which interface is used). :)
> 
> And again, I can work around this one.  It's just that this isn't the
> first time, so I want to nudge folks to have a broader view and
> consider these issues a little more when making changes (not just in
> the kernel, but in why the names in linux-firmware are different then
> what's on devices in the field, etc).
> 

Right, you certainly have suffered from this in the past, because we
didn't have a strategy for handling more than a single Qualcomm device
with any given instance of /lib/firmware.

Now we have a plan, and I believe db845c was the first board to
implement this plan and I got it wrong.  Going forward I hope to avoid
making this mistake again.

Regards,
Bjorn

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Regression: arm64: dts: sdm845-db845c: make firmware filenames follow linux-firmware
  2021-05-05 20:05     ` Rob Clark
  2021-05-05 20:07       ` Dmitry Baryshkov
@ 2021-05-05 21:14       ` John Stultz
  1 sibling, 0 replies; 9+ messages in thread
From: John Stultz @ 2021-05-05 21:14 UTC (permalink / raw)
  To: Rob Clark
  Cc: Bjorn Andersson, Dmitry Baryshkov, Vinod Koul, Srini Kandagatla,
	Amit Pundir, YongQin Liu, linux-arm-msm

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-05-05 21:14 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-05  6:20 Regression: arm64: dts: sdm845-db845c: make firmware filenames follow linux-firmware 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
2021-05-05 21:02     ` Bjorn Andersson
2021-05-05 18:43 ` Dmitry Baryshkov
2021-05-05 19:06 ` Bjorn Andersson

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).