[PATCH 0/6] drivers: sysreset: revert support for args

[Casey Connolly]

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 <casey.connolly at linaro.org>
>>
>> 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/d7e003b3-e693-4579-8e3a-b09ce9f78d10 at cherry.de/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-28a5bde07483@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