From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752012AbdHARvj (ORCPT ); Tue, 1 Aug 2017 13:51:39 -0400 Received: from mail.linuxfoundation.org ([140.211.169.12]:39578 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751815AbdHARvg (ORCPT ); Tue, 1 Aug 2017 13:51:36 -0400 Date: Tue, 1 Aug 2017 10:51:33 -0700 From: Greg Kroah-Hartman To: Boris Brezillon Cc: Wolfram Sang , linux-i2c@vger.kernel.org, Jonathan Corbet , linux-doc@vger.kernel.org, Arnd Bergmann , Przemyslaw Sroka , Arkadiusz Golec , Alan Douglas , Bartosz Folta , Damian Kos , Alicja Jurasik-Urbaniak , Jan Kotas , Cyprian Wronka , Alexandre Belloni , Thomas Petazzoni , Nishanth Menon , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , devicetree@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [RFC 2/5] i3c: Add core I3C infrastructure Message-ID: <20170801175133.GA30520@kroah.com> References: <1501518290-5723-1-git-send-email-boris.brezillon@free-electrons.com> <1501518290-5723-3-git-send-email-boris.brezillon@free-electrons.com> <20170801014021.GA20004@kroah.com> <20170801124801.38bbc431@bbrezillon> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170801124801.38bbc431@bbrezillon> User-Agent: Mutt/1.8.3 (2017-05-23) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Greg Kroah-Hartman Subject: Re: [RFC 2/5] i3c: Add core I3C infrastructure Date: Tue, 1 Aug 2017 10:51:33 -0700 Message-ID: <20170801175133.GA30520@kroah.com> References: <1501518290-5723-1-git-send-email-boris.brezillon@free-electrons.com> <1501518290-5723-3-git-send-email-boris.brezillon@free-electrons.com> <20170801014021.GA20004@kroah.com> <20170801124801.38bbc431@bbrezillon> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <20170801124801.38bbc431@bbrezillon> Sender: linux-doc-owner@vger.kernel.org To: Boris Brezillon Cc: Wolfram Sang , linux-i2c@vger.kernel.org, Jonathan Corbet , linux-doc@vger.kernel.org, Arnd Bergmann , Przemyslaw Sroka , Arkadiusz Golec , Alan Douglas , Bartosz Folta , Damian Kos , Alicja Jurasik-Urbaniak , Jan Kotas , Cyprian Wronka , Alexandre Belloni , Thomas Petazzoni , Nishanth Menon , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala List-Id: devicetree@vger.kernel.org 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