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/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)?

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

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

Cheers,
Silvan