Re: [PATCH v2 1/3] HID: nintendo: Add preliminary Switch 2 controller driver

[Vicki Pfau <vi@endrift.com>]

On 4/11/26 8:29 AM, Silvan Jegen wrote:
> Vicki Pfau <vi@endrift.com> wrote:
>> On 4/10/26 12:44, Silvan Jegen wrote:
>>> Vicki Pfau <vi@endrift.com> wrote:
>>>> Replies inline
>>>>
>>>> On 4/8/26 12:51, Silvan Jegen wrote:
>>>>> Heyhey!
>>>>>
>>>>> Vicki Pfau <vi@endrift.com> wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Replies inline
>>>>>>
>>>>>> On 4/2/26 12:09 PM, Silvan Jegen wrote:
>>>>>>> Hi
>>>>>>>
>>>>>>> Thanks for the patch!
>>>>>>>
>>>>>>> Just some comments and questions inline below.
>>>>>>>
>>>>>>> Vicki Pfau <vi@endrift.com> wrote:
>>>>>>>>
>>>>>>>> [...]
>>>>>>>>
>>>>>>>> +
>>>>>>>> +static int switch2_set_report_format(struct switch2_controller *ns2, enum switch2_report_id fmt)
>>>>>>>> +{
>>>>>>>> +	__le32 format_id = __cpu_to_le32(fmt);
>>>>>>>> +
>>>>>>>> +	if (!ns2->cfg)
>>>>>>>> +		return -ENOTCONN;
>>>>>>>> +	return ns2->cfg->send_command(NS2_CMD_INIT, NS2_SUBCMD_INIT_SELECT_REPORT,
>>>>>>>> +		&format_id, sizeof(format_id),
>>>>>>>> +		ns2->cfg);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static int switch2_init_controller(struct switch2_controller *ns2)
>>>>>>>
>>>>>>> This is now a recursive call while in v1 it wasn't. I think I preferred
>>>>>>> the non-recursive version as there was one place where init_step
>>>>>>> state was changed while now I am not sure where it happens (and whether
>>>>>>> there is a code path where we end up in an infinite recursion)
>>>>>>>
>>>>>>> What is the advantage of the recursive version compared to the
>>>>>>> non-recursive one?
>>>>>    >
>>>>>>
>>>>>> The old version incremented the step regardless of whether or not it
>>>>>> could confirm it had happened. Since the confirmation is now handled
>>>>>> with an external step, calling into switch2_init_step_done, the loop
>>>>>> condition would become somewhat complicated.
>>>>>> I replaced it with explicit tail calls since that make the
>>>>>> control flow simplier, and it is always matched with a call to
>>>>>> switch2_init_step_done to ensure that the state is always advanced. As
>>>>>
>>>>>   From what I can tell switch2_init_step_done currently only advances
>>>>> the state if the current state is the expected one. This seems fine,
>>>>> but it also means that if the state is not the expected one, the
>>>>> state is not advanced and the recursive call continues anyway (in the
>>>>> NS2_INIT_READ_USER_SECONDARY_CALIB case, for example). I assume this
>>>>> should never happen but if we end up in this case for some reason we
>>>>> will recurse forever.
>>>>
>>>> That's correct, and the only way it would happen forever is if there's a
>>>> bug. The same would be true in a loop version if it doesn't advance the
>>>> state properly either, fwiw, which happened during development of this
>>>> version. Regardless, I can reduce the chance of introducing such a bug
>>>> by passing ns2->init_step instead of a constant, so I'll make that
>>>> change in v4.
>>>>
>>>>>
>>>>> The same case also potentially calls switch2_read_flash and it isn't
>>>>> clear to me if this means that the initialisation is done (as there
>>>>> is no switch2_init_step_done call and we are not in the FINISH state
>>>>> either). There is also the possibility of switch2_read_flash calling
>>>>> switch2_init_controller again, which one then has to check ... (note
>>>>> that this is not the case here though)
>>>>
>>>> switch2_handle_flash_read will advance the state once it's verified that
>>>> the read actually happened. If the step failed for whatever reason, this
>>>> same codepath will retry the specific read, as the caller
>>>> (switch2_receive_command) will always call into switch2_init_controller
>>>> if setup isn't done. This is how the retry logic works.
>>>
>>> Ah, so the call chain looks something like the below?
>>>
>>> switch2_read_flash->
>>> switch2_usb_send_cmd->
>>> switch2_usb_message_in_work (?)->
>>> switch2_receive_command->
>>> switch2_handle_flash_read->
>>> switch2_init_step_done
>>
>> Yes, and then switch2_init_controller is called again at the end of 
>> switch2_receive_command
>>>
>>>>
>>>>>
>>>>> To me it seems like it would be clearer to do a `ns2->init_step++` and
>>>>> then `continue` to make the progress of the state more visible and to
>>>>> do an explicit `break` when we are supposed to stop the initialisation.
>>>>>
>>>>
>>>> The problem with the loop approach, in my opinion is due to the fact
>>>> that the loop is the *exception*, not the rule. The loop idiom makes it
>>>> look like a loop is expected. Further the ns2->init_step++ in the
>>>> previous version means that the verification does not occur, so in the
>>>> case of any sort of failure it'll plow ahead anyway instead of retrying.
>>>> The point of this approach is to avoid that.
>>>
>>> In my mind a while-loop like you mentioned it above would make the state
>>> changes more obvious (since they could all be done in the loop body), while
>>> still allowing for retries. Something like the below, perhaps (untested).
>>>
>>> while (ns2->init_step < NS2_INIT_DONE) {
>>> 	switch (ns2->init_step) {
>>> 		...
>>> 		case NS2_INIT_READ_FACTORY_TRIGGER_CALIB:
>>> 			if (ns2->ctlr_type != NS2_CTLR_TYPE_GC) {
>>> 				ns->init_step++
>>> 				continue;
>>> 			}
>>>
>>> 			ret = switch2_read_flash(ns2, NS2_FLASH_ADDR_FACTORY_TRIGGER_CALIB,
>>> 			        NS2_FLASH_SIZE_FACTORY_TRIGGER_CALIB);		
>>> 			if (ret) {
>>> 				// if it makes sense to retry here
>>> 				continue;
>>> 			}
>>>
>>> 			ns->init_step++
>>> 			break;
>>>
>>> 		case ...
>>> 	}
>>> }
>>>
>>
>> This can't be done because the process is fundamentally asynchronous. 
>> We'd have to block on waiting for a reply to the USB packet, which is 
>> not a good idea. This is why switch2_receive_command calls into 
>> switch2_init_controller at the end: it's resuming where it left off. 
> 
> Ah, that wasn't clear to me either.
> 
> I assume having the code wait for the reply is not allowed because
> otherwise it would stall the whole bootup process (or will there be some
> sort of dedicated Kthread for this)?

I'm not sure there is a synchronous USB API at all, but if there is it should really only be used when absolutely necessary. If you can get away without blocking, do.

> 
> Are you aware of any documentation where I can read up on how the probing
> of USB HID devices work in the Linux Kernel?

This has nothing to do with HID, actually. The Switch 2 controllers have a proprietary side-channel interface that we need to poke at here to get it to do anything. It's called cfg in the code and over USB it uses a bulk interface, which is unusual. Presumably because the side-channel doesn't care about latency, whereas the HID interface does. Something similar happens over bluetooth where they're accessed through disparate attributes. The latter isn't handled here yet, though.

> 
> Thanks for the help!
> 
>> Each of those returns is a step that interacts with the hardware, and we 
>> need to wait for the hardware to reply.
>>
>> There is a potential weird interaction here whereby if we get an 
>> unprompted command and/or reply from the controller it will retry a step 
>> before it gets a reply for it, but in practice this doesn't happen. The 
>> controllers, as far as we know, only reply and never initiate any 
>> commands. Furthermore, all of these steps are also idempotent, so it's 
>> not a big deal of they get repeated erroneously.
> 
> Could there be another reply incoming while the driver is still processing
> the previous one? I assume at least at probing time that shouldn't be
> the case. I wouldn't expect an USB HID device to send unsolicited replies
> in general ...

Replies only come in one at a time. Unless we send multiple messages without waiting for a reply first, which this code is specifically designed *not* to do, I don't think it will happen in practice.

> 
> Cheers,
> Silvan