Re: [RFC 2/5] i3c: Add core I3C infrastructure

[Greg Kroah-Hartman]

On Tue, Aug 01, 2017 at 12:48:01PM +0200, Boris Brezillon wrote:
> > > +static DEFINE_MUTEX(i3c_core_lock);
> > > +
> > > +void i3c_bus_lock(struct i3c_bus *bus, bool exclusive)
> > > +{
> > > +	if (exclusive)
> > > +		down_write(&bus->lock);
> > > +	else
> > > +		down_read(&bus->lock);
> > > +}  
> > 
> > The "exclusive" flag is odd, and messy, and hard to understand, don't
> > you agree?
> 
> I could create 2 functions, one for the exclusive lock and the other
> one for the shared lock.

Or you could just use a simple mutex until you determine you really
would do better with a rw lock :)

> > And have you measured the difference in using a rw lock over
> > a normal mutex and found it to be faster?  If not, just use a normal
> > mutex, it's simpler and almost always better in the end.
> 
> I did not measure the difference, but using a standard lock means
> serializing all I3C accesses going through a given master in the core.

Which you are doing with a rw lock anyway, right?

> Note that this lock is not here to guarantee that calls to
> master->ops->xxx() are serialized (this part is left to the master
> driver), but to ensure that when a bus management operation is in
> progress (like changing the address of a device on the bus), no one can
> send I3C frames. If we don't do that, one could queue an I3C transfer,
> and by the time this transfer reach the bus, the I3C device this
> message was targeting may have a different address.

That sounds really odd.  locks should protect data, not bus access,
right?

> Unless you see a good reason to not use a R/W lock, I'd like to keep it
> this way because master IPs are likely to implement advanced queuing
> mechanism (allows one to queue new transfers even if the master is
> already busy processing other requests), and serializing things at the
> framework level will just prevent us from using this kind of
> optimization.

Unless you can prove otherwise, using a rw lock is almost always worse
than just a mutex.  And you shouldn't have a lock for bus transactions,
that's just going to be a total mess.  You could have a lock for a
single device access, but that still seems really strange, is the i3c
spec that bad?

> > > +static ssize_t hdrcap_show(struct device *dev,
> > > +			   struct device_attribute *da,
> > > +			   char *buf)
> > > +{
> > > +	struct i3c_device *i3cdev = dev_to_i3cdev(dev);
> > > +	struct i3c_bus *bus = i3c_device_get_bus(i3cdev);
> > > +	unsigned long caps = i3cdev->info.hdr_cap;
> > > +	ssize_t offset = 0, ret;
> > > +	int mode;
> > > +
> > > +	i3c_bus_lock(bus, false);
> > > +	for_each_set_bit(mode, &caps, 8) {
> > > +		if (mode >= ARRAY_SIZE(hdrcap_strings))
> > > +			break;
> > > +
> > > +		if (!hdrcap_strings[mode])
> > > +			continue;
> > > +
> > > +		ret = sprintf(buf + offset, "%s\n", hdrcap_strings[mode]);  
> > 
> > Multiple lines in a single sysfs file?  No.
> 
> Okay. Would that be okay with a different separator (like a comma)?

No, sysfs files are "one value per file", given you don't have any
documentation saying what this file is supposed to be showing, I can't
really judge the proper way for you to present it to userspace :)

> > > +static const struct attribute_group *i3c_device_groups[] = {
> > > +	&i3c_device_group,
> > > +	NULL,
> > > +};  
> > 
> > ATTRIBUTE_GROUPS()?
> 
> My initial plan was to have a common set of attributes that apply to
> both devices and masters, and then add specific attributes in each of
> them when they only apply to a specific device type.

That's fine, but you do know that attributes can be enabled/disabled at
device creation time with the return value of the callback is_visable(),
right?  Why not just use that here, simplifying a lot of logic?

> Just out of curiosity, what's the preferred solution when you need to
> do something like that? Should we just use ATTRIBUTE_GROUPS() and
> duplicate the entries that apply to both device types?

is_visable()?

> > No release type?  Oh that's bad bad bad and implies you have never
> > removed a device from your system as the kernel would have complained
> > loudly at you.
> 
> You got me, never tried to remove a device :-). Note that these
> ->release() hooks will just be dummy ones, because right now, device
> resources are freed at bus destruction time.

You better not have a "dummy" release hook, do that and as per the
kernel documentation, I get to make fun of you in public for doing that
:(

> Also, I see that dev->release() is called instead of
> dev->type->release() if it's not NULL [1]. what's the preferred solution
> here? Set dev->release or type->release()?

It depends on how your bus is managed, who controls the creation of the
resources, free it in the same place you create it.

thanks,

greg k-h