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

Hi Casey, Quentin,

On 7/3/2026 6:29 PM, Casey Connolly wrote:
> 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-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.

I've reworked the reset -edl support to follow the approach from the 
kernel reference you pointed .

Series is here: 
https://lore.kernel.org/u-boot/20260811-b4-reset-edl-rebootmode-v1-0-2e15adc156a3@oss.qualcomm.com/

Let me know if this addresses your concerns.

Regards,

Balaji

>
> Thanks,
> // Casey
>
>>
>> Thanks,
>> Quentin
>