linux-kernel - Re: [PATCH 1/3] spi: tegra210-quad: use device_reset_optional() instead of device

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <5doq6itaz6uicvqcn37q2dkaxyzy3etz5qgv6wlsyd5troqlag@yqs6ltjp3gsz>
Date: Wed, 19 Mar 2025 19:26:52 +0100
From: Thierry Reding <thierry.reding@...il.com>
To: Breno Leitao <leitao@...ian.org>
Cc: Arnd Bergmann <arnd@...db.de>, jonathanh@...dia.com, 
	skomatineni@...dia.com, Mark Brown <broonie@...ian.org>, 
	Laxman Dewangan <ldewangan@...dia.com>, linux-tegra@...r.kernel.org, linux-spi@...r.kernel.org, 
	linux-kernel@...r.kernel.org, rmikey@...a.com, kernel-team@...a.com, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, noodles@...th.li, Jarkko Sakkinen <jarkko@...nel.org>, 
	Peter Huewe <peterhuewe@....de>
Subject: Re: [PATCH 1/3] spi: tegra210-quad: use device_reset_optional()
 instead of device_reset()

On Wed, Mar 19, 2025 at 03:09:57AM -0700, Breno Leitao wrote:
> Hello Arnd, Thierry, Jonathan, Sowjanya,
> 
> On Tue, Mar 18, 2025 at 09:07:28PM +0100, Arnd Bergmann wrote:
> > On Tue, Mar 18, 2025, at 20:13, Mark Brown wrote:
> > > On Tue, Mar 18, 2025 at 08:00:05PM +0100, Arnd Bergmann wrote:
> > >
> > >> That does sound like the easiest answer: if the spi controller driver
> > >> knows that it needs a reset but there is no reset controller, shutting
> > >> itself down and removing its child devices seems like the least
> > >> offensive action.
> > >
> > > In that case it's probably more just refuse to probe in the first case
> > > without the reset controller.  Given that the device isn't working at
> > > all it seems like the hardware description is broken anyway...
> > 
> > Right, I see now that it's doing a rather silly
> > 
> >        if (device_reset(tqspi->dev) < 0)
> >                dev_warn_once(tqspi->dev, "device reset failed\n");
> > 
> > after which it just continues instead of propagating returning
> > the error from the probe function. 
> 
> This would be another option, and I would be happy to update this patch
> with this suggestion.
> 
> This patch was attempting to address the issue the other way around,
> where I was expecting that the reset methods are optional, thus
> marking the device_reset() function as optional.
> 
> It appears that on certain UEFI machine types, the ACPI firmware doesn't
> implement the _RST methods, and device_reset() will *always* fail. It's
> unclear whether this is due to a broken ACPI table or if it was
> intentionally designed this way.
> 
> Tagging the driver maintainer (Thierry, Jonathan, Sowjanya) who might
> have a better understanding of the design in such cases.

Can you specify what device this is and what software you've been
running (including firmware, L4T release, etc.)? I can try to find out
if this is a known issue, or if it's even intended to be this way.

> > This is also broken when
> > the reset controller driver has not been loaded yet and it
> > should do an -EPROBE_DEFER.
> > 
> > In case of a broken ACPI table, this would simply fail the
> > probe() with an error, which seems like a sensible behavior.
> 
> Do we agree that the device reset methods MUST always exist (on both DT
> and UEFI hosts)?
> 
> Anyway, from my naive view, we should:
> 
> 1) Mark as required, and fail the probe,  if this device_reset() must
>    have available methods. (Arnd's suggestion)
> 
> 2) Mark device_reset as optional if device reset is optional (as the
>    current situation suggest).  
> 
>    a) If the requirements are different for DT and UEFI, then should we 
>       create a "device_reset_optional_on_acpi_but_not_DT()" helper to
>       handle such cases(!?)

I'm not very familiar with the ACPI side of things, but my recollection
is that essentially ACPI talks to BPMP in the background, much the same
way that we do using the BPMP driver if booted with a DT. I wouldn't
expect there to be any functional differences, so the lack of _RST for
this controller seems strange.

Again, if you can provide a bit more information about the set up, I can
try to find out more.

Thanks,
Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)