From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: Wolfram Sang <wsa@the-dreams.de>,
linux-i2c@vger.kernel.org, Jonathan Corbet <corbet@lwn.net>,
linux-doc@vger.kernel.org, Arnd Bergmann <arnd@arndb.de>,
Przemyslaw Sroka <psroka@cadence.com>,
Arkadiusz Golec <agolec@cadence.com>,
Alan Douglas <adouglas@cadence.com>,
Bartosz Folta <bfolta@cadence.com>, Damian Kos <dkos@cadence.com>,
Alicja Jurasik-Urbaniak <alicja@cadence.com>,
Jan Kotas <jank@cadence.com>,
Cyprian Wronka <cwronka@cadence.com>,
Alexandre Belloni <alexandre.belloni@free-electrons.com>,
Thomas Petazzoni <thomas.petazzoni@free-electrons.com>,
Nishanth Menon <nm@ti.com>, Rob Herring <robh+dt@kernel.org>,
Pawel Moll <pawel.moll@arm.com>,
Mark Rutland <mark.rutland@arm.com>,
Ian Campbell <ijc+devicetree@hellion.org.uk>,
Kumar Gala <galak@co>
Subject: Re: [RFC 2/5] i3c: Add core I3C infrastructure
Date: Tue, 1 Aug 2017 10:51:33 -0700 [thread overview]
Message-ID: <20170801175133.GA30520@kroah.com> (raw)
In-Reply-To: <20170801124801.38bbc431@bbrezillon>
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
next prev parent reply other threads:[~2017-08-01 17:51 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-07-31 16:24 [RFC 0/5] Add I3C subsystem Boris Brezillon
2017-07-31 16:24 ` [RFC 1/5] i2c: Export of_i2c_get_board_info() Boris Brezillon
2017-07-31 16:24 ` [RFC 2/5] i3c: Add core I3C infrastructure Boris Brezillon
2017-07-31 19:17 ` Wolfram Sang
2017-07-31 20:46 ` Boris Brezillon
2017-07-31 20:16 ` Arnd Bergmann
[not found] ` <CAK8P3a06GoMdKdn=3Cq0FUwYnjGX0oG+FQLjxfiasVDpbonWRw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-07-31 21:15 ` Boris Brezillon
2017-07-31 21:32 ` Peter Rosin
2017-07-31 21:42 ` Wolfram Sang
2017-08-01 16:47 ` Andrew F. Davis
2017-08-01 17:27 ` Wolfram Sang
2017-08-01 21:47 ` Boris Brezillon
2017-08-02 10:21 ` Wolfram Sang
2017-08-01 12:00 ` Arnd Bergmann
2017-08-01 12:29 ` Boris Brezillon
2017-08-01 13:11 ` Arnd Bergmann
2017-08-01 13:34 ` Boris Brezillon
2017-08-01 13:58 ` Boris Brezillon
2017-08-01 14:22 ` Arnd Bergmann
2017-08-01 15:14 ` Boris Brezillon
2017-08-01 20:16 ` Arnd Bergmann
2017-08-01 14:12 ` Wolfram Sang
2017-08-01 14:48 ` Boris Brezillon
2017-08-01 15:01 ` Wolfram Sang
2017-08-01 15:20 ` Boris Brezillon
2017-08-03 8:03 ` Boris Brezillon
2017-08-16 21:03 ` Geert Uytterhoeven
2017-08-17 7:48 ` Boris Brezillon
2017-08-01 1:40 ` Greg Kroah-Hartman
[not found] ` <20170801014021.GA20004-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
2017-08-01 10:48 ` Boris Brezillon
2017-08-01 17:51 ` Greg Kroah-Hartman [this message]
2017-08-01 21:30 ` Boris Brezillon
2017-08-02 0:54 ` Greg Kroah-Hartman
2017-08-02 2:13 ` Greg Kroah-Hartman
[not found] ` <20170802021327.GB23033-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
2017-12-13 16:20 ` Boris Brezillon
2017-12-13 16:51 ` Greg Kroah-Hartman
2017-08-17 9:03 ` Linus Walleij
2017-08-17 9:28 ` Boris Brezillon
[not found] ` <1501518290-5723-1-git-send-email-boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
2017-07-31 16:24 ` [RFC 3/5] dt-bindings: i3c: Document core bindings Boris Brezillon
2017-08-09 23:43 ` Rob Herring
2017-08-10 8:49 ` Boris Brezillon
2017-07-31 16:24 ` [RFC 4/5] i3c: master: Add driver for Cadence IP Boris Brezillon
2017-07-31 16:24 ` [RFC 5/5] dt-bindings: i3c: Document Cadence I3C master bindings Boris Brezillon
2017-07-31 19:17 ` [RFC 0/5] Add I3C subsystem Wolfram Sang
2017-07-31 20:40 ` Boris Brezillon
2017-07-31 20:47 ` Wolfram Sang
2017-12-12 19:58 ` Boris Brezillon
2017-12-12 22:01 ` Wolfram Sang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170801175133.GA30520@kroah.com \
--to=gregkh@linuxfoundation.org \
--cc=adouglas@cadence.com \
--cc=agolec@cadence.com \
--cc=alexandre.belloni@free-electrons.com \
--cc=alicja@cadence.com \
--cc=arnd@arndb.de \
--cc=bfolta@cadence.com \
--cc=boris.brezillon@free-electrons.com \
--cc=corbet@lwn.net \
--cc=cwronka@cadence.com \
--cc=dkos@cadence.com \
--cc=galak@co \
--cc=ijc+devicetree@hellion.org.uk \
--cc=jank@cadence.com \
--cc=linux-doc@vger.kernel.org \
--cc=linux-i2c@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=nm@ti.com \
--cc=pawel.moll@arm.com \
--cc=psroka@cadence.com \
--cc=robh+dt@kernel.org \
--cc=thomas.petazzoni@free-electrons.com \
--cc=wsa@the-dreams.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).