On Tue, Aug 01, 2017 at 12:48:01PM +0200, Boris Brezillon wrote:

quoted

quoted

+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 :)

quoted

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?

quoted

quoted

+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 :)

quoted

quoted

+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()?

quoted

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