Re: [PATCH v3 3/5] i3c: master: add i3c_for_each_dev helper

[Boris Brezillon]

On Fri, 21 Feb 2020 13:59:11 +0100
Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> wrote:

> On Fri, 21 Feb 2020 12:52:29 +0100
> Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> 
> > On Fri, Feb 21, 2020 at 11:47:22AM +0000, Vitor Soares wrote:  
> > > Hi Greg,
> > > 
> > > From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
> > > Date: Wed, Feb 19, 2020 at 07:35:48
> > >     
> > > > On Wed, Feb 19, 2020 at 01:20:41AM +0100, Vitor Soares wrote:    
> > > > > Introduce i3c_for_each_dev(), an i3c device iterator for use by i3cdev.
> > > > > 
> > > > > Signed-off-by: Vitor Soares <vitor.soares@xxxxxxxxxxxx>
> > > > > ---
> > > > >  drivers/i3c/internals.h |  1 +
> > > > >  drivers/i3c/master.c    | 12 ++++++++++++
> > > > >  2 files changed, 13 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h
> > > > > index bc062e8..a6deedf 100644
> > > > > --- a/drivers/i3c/internals.h
> > > > > +++ b/drivers/i3c/internals.h
> > > > > @@ -24,4 +24,5 @@ int i3c_dev_enable_ibi_locked(struct i3c_dev_desc *dev);
> > > > >  int i3c_dev_request_ibi_locked(struct i3c_dev_desc *dev,
> > > > >  			       const struct i3c_ibi_setup *req);
> > > > >  void i3c_dev_free_ibi_locked(struct i3c_dev_desc *dev);
> > > > > +int i3c_for_each_dev(void *data, int (*fn)(struct device *, void *));
> > > > >  #endif /* I3C_INTERNAL_H */
> > > > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > > > > index 21c4372..8e22da2 100644
> > > > > --- a/drivers/i3c/master.c
> > > > > +++ b/drivers/i3c/master.c
> > > > > @@ -2640,6 +2640,18 @@ void i3c_dev_free_ibi_locked(struct i3c_dev_desc *dev)
> > > > >  	dev->ibi = NULL;
> > > > >  }
> > > > >  
> > > > > +int i3c_for_each_dev(void *data, int (*fn)(struct device *, void *))
> > > > > +{
> > > > > +	int res;
> > > > > +
> > > > > +	mutex_lock(&i3c_core_lock);
> > > > > +	res = bus_for_each_dev(&i3c_bus_type, NULL, data, fn);
> > > > > +	mutex_unlock(&i3c_core_lock);    
> > > > 
> > > > Ick, why the lock?  Are you _sure_ you need that?  The core should
> > > > handle any list locking issues here, right?    
> > > 
> > > I want to make sure that no new devices (eg: Hot-Join capable device) are 
> > > added during this iteration and after this call, each new device will 
> > > release a bus notification.
> > >     
> > > > 
> > > > I don't see bus-specific-locks around other subsystem functions that do
> > > > this (like usb_for_each_dev).    
> > > 
> > > I based in I2C use case.    
> > 
> > Check to see if this is really needed, for some reason I doubt it...  
> 
> Can we please try the spidev approach before fixing those problems. None
> of that would be needed if we declare the i3cdev driver as a regular
> i3c_device_driver and let user space bind devices it wants to expose
> through the sysfs interface. As I said earlier, we even have all the
> pieces we need to automate that using a udev rule, and the resulting
> patchset would be 'less invasive'/simpler for pretty much the same
> result.

So, I went ahead and implemented it the way I suggest. The diffstat is
not representative here (though it's still in favor of this new version)
since I also changed the way we expose/handle SDR transfers. What's
most important IMO is the fact that

* we no longer need to access the internal I3C API
* we no longer need to care about transitions between i3cdev and
  other drivers (the core guarantees that a device is always bound to at
  most one driver)
* the registration/unregistration procedure is simplified

Not all problems have been addressed (we still need to put a limit on
the number of xfers and the max size per transfer we allow, and
probably plenty of other things pointed by Greg, Arnd and others), but
I'd really like to start from there for the next version.

--->8---