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

["Silvan Jegen" <s.jegen@gmail.com>]

Vicki Pfau <vi@endrift.com> wrote:
> 
> 
> 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.

Makes sense!


> > 
> > 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

I have seen this part of the code, but have assumed that it's actually
part of the regular HID spec ...


> 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.

That's what I would have expected as well. Thanks for clearing that up!

Cheers,
Silvan