Re: [PATCH v4 1/4] cmd: bind: Add unbind command with driver filter

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Tom,

trini@konsulko.com wrote on Fri, 4 Aug 2023 14:51:07 -0400:

> On Fri, Aug 04, 2023 at 08:01:56PM +0200, Miquel Raynal wrote:
> > Hi Tom,
> > 
> > trini@konsulko.com wrote on Fri, 4 Aug 2023 13:20:29 -0400:
> >   
> > > On Fri, Aug 04, 2023 at 07:01:46PM +0200, Miquel Raynal wrote:  
> > > > Hi Tom,
> > > >     
> > > > > > > >>> Well, what's needed / is it possible to get to the point where we don't
> > > > > > > >>> _need_ to call bind/unbind for each of these cases? Is there something
> > > > > > > >>> we're supposed to be setting in the DT that we aren't?        
> > > > > > > >>
> > > > > > > >> You do need to unbind the ethernet before using fastboot, always, with the 'unbind ethernet 0', you dont need the peripheral unbind/rebind part, so it should behave like before.        
> > > > > > > > 
> > > > > > > > And for my own understanding, why don't we need to bind a fastboot
> > > > > > > > gadget?        
> > > > > > > 
> > > > > > > The fastboot command does that internally .      
> > > > > > 
> > > > > > This is not visible with dm tree and I did not find how to bind the
> > > > > > fastboot gadget manually.
> > > > > > 
> > > > > > This makes the CLI clearly uneven and the internal code badly different
> > > > > > between gadgets/commands. Why can't we have them both
> > > > > > autoloaded/unloaded like before? I think I missed something which
> > > > > > explains the rationale behind this series.      
> > > > > 
> > > > > They aren't both auto-loaded currently. We have a legacy call,
> > > > > usb_ether_init(), in a few cases, so that gadget mode ethernet starts.
> > > > > But this leads to the ref counting problems you encountered and
> > > > > re-posted the rejected series for.    
> > > > 
> > > > Ok, thanks for the additional details.
> > > > 
> > > > I don't understand why fastboot autoloads the correct gadget driver if
> > > > there is none bound, while all network commands just fail to do that if
> > > > we don't make the usb_ether_init() call manually.    
> > > 
> > > Because "fastboot" or "dfu" are both being told (as part of their call)
> > > "find usb gadget controller number X".  That's doing the bind.  Calling
> > > usb_ether_init just takes the first controller and that's that (and so
> > > could be / is wrong depending on the platform).  
> > 
> > This makes total sense, thanks for pointing it out.
> >   
> > > > I also don't understand why I need to unbind the ethernet gadget but I
> > > > cannot bind the fastboot gadget.    
> > > 
> > > You can't bind fastboot while ethernet is still configured.  
> > 
> > I guess "before this series", the ethernet would not be kept
> > configured, because I could use both fastboot and ethernet without
> > caring about which driver had to be bound. And that's maybe what lead to
> > the bug reports also. So you want to get rid of this. Do I get the
> > situation right now?  
> 
> We're getting rid of this path because it leads to failures, yes.
> 
> > >  It's in
> > > use.  And we aren't a full blown operating system with the logic to have
> > > multiple end points and devices configured and exposed.  I mean heck, we
> > > don't keep the gadget interface up between network calls so on the host
> > > side you need to re-configure the interface (or have something that's
> > > bringing it up there again each time).  Which is why DFU or fastboot or
> > > other "treat USB like USB" options tend to be more popular than "treat
> > > USB like ethernet" work flows.  
> > 
> > Yes of course.
> >   
> > > > My underlying question is: can we have a single approach for all
> > > > drivers, or is it too complex today? Could it be possible, when we
> > > > perform these autoloads, to look up the registered gadget and
> > > > potentially unbind the one already in place before?    
> > > 
> > > I would invite you to look in to this, yes.  
> > 
> > Sounds reasonably complex now, with the reasoning you made above.
> >   
> > >  No one objects to enhancing
> > > the code, but the "functionality" you see on am33xx is as you've also
> > > seen very broken in other ways, which is why it's not used virtually
> > > anywhere else and instead the "bind" command is.  For example, if you
> > > wanted to do this workflow on the new beagle devices, that's DWC3 and
> > > where the "bind" command came from :)  
> > 
> > Again to be very clear, while I felt misunderstood at the beginning and
> > did not accept Marek's series because it was replacing a data abort
> > with a non-functional setup, I now get a better understanding of the
> > problem (I believe) and, as said before, I am always in favor of
> > writing better code, easier to maintain, clearer to review. I am in
> > favor of such change. I just want the user life to be eased when this
> > happens if we break the CLI. So if you think the right approach is to
> > get rid of the usb_ether_init() call, alright, let's drop it off. But
> > we should not let the users in the dark by providing proper doc or
> > error messages which should compensate the extra step now required.  
> 
> I think it would be good if you posted a patch to update
> doc/board/ti/am335x_evm.rst to explain how to use the various gadget
> device cases.

Good idea, I've tried something.

Thanks,
Miquèl