Hi Quentin,
On 7/3/26 14:35, Quentin Schulz wrote:
Hi Casey,
Thanks for the prompt answer!
On 7/3/26 2:27 PM, Casey Connolly wrote:
Hi Quentin,
On 7/2/26 20:27, Quentin Schulz wrote:
The args support for the sysreset uclass contains a logic bug. The first
sysreset device implementing the request_arg callback will consume the
args, not support the specified arg and thus return -EPROTONOSUPPORT
which will stop the iteration over all sysreset devices.
This is an issue if one has multiple sysreset devices and each with
support for different (valid) args. If a sysreset device implements a
-dummy argument and another -foo and one calls reset -dummy on the
U-Boot CLI, it'll depend on which sysreset device will be attempted
first. If it is the one implementing -foo, it'll return it doesn't
support the argument with -EPROTONOSUPPORT in which case the device
implementing -dummy will never be attempted and instead we'll do a cold
reset which is very likely not what's expected.
I suggest to either add an additional return code for the case where the
sysreset device knows it isn't supposed to handle this argument because
it isn't among the ones it supports, and thus we try other sysreset
devices until we find one that says it tried and either is in progress
or couldn't realize the request, or no sysreset device supports this
argument and we end up doing a cold reset (but should we even try to do
a cold reset if the type of reset requested by the user isn't
supported?). Or just don't try piggy-backing on the reset command and
have a new command instead?
Maybe I misunderstood something or misread the code, so please correct
me if I'm wrong.
We've tried bringing up this issue multiple times over the last
month[1][2][3] but it seems like we're left on read. This is the last
resort to get attention on this patches, hopefully we don't have to get
through a revert a few days before the release though there is only two
working days left before the release. Sending a revert as requested by
Tom[4].
I'm so sorry I missed this feedback. That really isn't great.
It happens, I'm glad you see it now :)
I'm with you that this is a pretty fundamental issue with the
implementation here and I think the best course of action is to revert
it and come up with a more suitable approach that isn't so open to
trivial logic bugs like this, likely with some deeper refactoring of
sysreset/reboot-mode code in U-Boot.
In hindsight I think I should have pushed back harder on this
implementation, this is something I'll be more cautious of in the future.
Just to be clear I wasn't trying to shame anyone here, contributor,
reviewer or maintainer. Mistakes happen.
Acked-by: Casey Connolly <[email protected]>
I don't have a chance today to test this today as I'm OOO but if it
doesn't break other platforms then it ought to be fine on Qualcomm
too, maybe Neil can give it a spin on 8650.
Tom: feel free to pick this up if you're happy with it.
Varada sent a series trying to fix the logic, c.f. https://
lore.kernel.org/u-boot/[email protected]/T/
#mf098752c30cd6a6c8e7700f61cd54cbc8f307d6d. But if you feel like we
should start from a clean slate again instead then we can revert this
now and take the time to implement this differently for next release. I
think your feedback on what to do next is important here. But I
appreciate you're OOO so we'll do without if you don't have time :)
heh I just saw that after hitting send...
I think the original series could probably have used some more
discussion before it got picked up, and the overall approach feels less
than ideal particularly when the reboot-mode mechanism already exists
for exactly this purpose.
imo eeping this implementation will only result in further issues down
the road particularly as other vendors adopt reboot2 support, and
frankly doing argument parsing in drivers just feels deeply wrong to me.
So I'd say let's revert it and go back to the drawing board with
something cleaner and more scalable. The exact same functionality has
gone through a lot of iterations on the Linux mailing list and it seems
like they're coming up with something quite sensible in the end with a
reboot-mode subnode on the psci DT node. With those DT changes in place
it seems foolish to a) have this be qcom specific and b) hardcode these
magic values in the driver.
https://lore.kernel.org/linux-pm/20260514-arm-psci-system_reset2-vendor-reboots-v22-4-28a5bde07...@oss.qualcomm.com/
I would be strongly in favour of something like the above, the "reset"
command in U-Boot could then have a way to retrieve all the possible
modes registered with the reboot-mode framework.
Thanks,
// Casey
Thanks,
Quentin
