Hi Linus,
This is the big GPIO pull-request for this merge window. We've got two new
drivers, new features for existing ones (like edge detection support in
gpio-ich) and a lot of minor tweaks and improvements all over the place (but
not in the core gpiolib code this time). We also have much appreciated
documentation fixes and extensions. The details are in the signed tag.
You'll notice that we have a bunch of configfs commits in our tree not acked by
the configfs maintainers. These commits implement the concept of committable
items in configfs - something that was well defined in the documentation for
years but has remained unimplemented. Despite the first submission of these
patches back in November 2020[1] and repeated pings & resending, configfs
maintainers have remained unresponsive. After reviewing these on the GPIO
mailing list, we decided to pick them up ourselves and send them your way
together with the first user: the new GPIO simulator.
Which brings us to one of the new drivers which is a new testing module based
on configfs & sysfs (as opposed to the old one using module parameters and
debugfs) which allows to dynamically create simulated chips from user-space.
It's meant to eventually completely replace gpio-mockup.
The other new driver is the one supporting the Otto GPIO controller from
Realtek.
Other than configfs changes, there's nothing really controversial in there.
Please pull!
Bartosz
[1] https://www.lkml.org/lkml/2020/11/25/514
The following changes since commit 0d02ec6b3136c73c09e7859f0d0e4e2c4c07b49b:
Linux 5.12-rc4 (2021-03-21 14:56:43 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git tags/gpio-updates-for-v5.13
for you to fetch changes up to edc510855d963b5687b05a5b39a72bd35fc4c4ba:
gpio: sim: Fix dereference of free'd pointer config (2021-04-27 14:59:05 +0200)
----------------------------------------------------------------
gpio updates for v5.13
- new driver for the Realtek Otto GPIO controller
- ACPI support for gpio-mpc8xxx
- edge event support for gpio-sch (+ Kconfig fixes)
- Kconfig improvements in gpio-ich
- fixes to older issues in gpio-mockup
- ACPI quirk for ignoring EC wakeups on Dell Venue 10 Pro 5055
- improve the GPIO aggregator code by using more generic interfaces instead of
reimplementing them in the driver
- implement configfs committable items
- implement a new GPIO testing module based on configfs & sysfs together with
its test-suite with the intention of eventually removing the old gpio-mockup
- convert the DT bindings for gpio-74x164 to yaml
- documentation improvements
- a slew of other minor fixes and improvements to GPIO drivers
----------------------------------------------------------------
Alexander Dahl (2):
docs: kernel-parameters: Move gpio-mockup for alphabetic order
docs: kernel-parameters: Add gpio_mockup_named_lines
Andy Shevchenko (14):
lib/cmdline: Export next_arg() for being used in modules
gpio: aggregator: Replace custom get_arg() with a generic next_arg()
irqdomain: Introduce irq_domain_create_simple() API
gpiolib: Unify the checks on fwnode type
gpiolib: Move of_node operations to gpiolib-of and correct fwnode use
gpiolib: Introduce acpi_gpio_dev_init() and call it from core
gpiolib: Reuse device's fwnode to create IRQ domain
gpiolib: Fold conditionals into a simple ternary operator
gpio: mockup: Drop duplicate NULL check in gpio_mockup_unregister_pdevs()
gpio: mockup: Adjust documentation to the code
gpio: sch: Hook into ACPI GPE handler to catch GPIO edge events
gpio: sch: Drop MFD_CORE selection
gpio: ich: Switch to be dependent on LPC_ICH
gpio: sim: Initialize attribute allocated on the heap
Barney Goette (1):
gpio: 104-dio-48e: Fix coding style issues
Bartosz Golaszewski (15):
configfs: increase the item name length
configfs: use (1UL << bit) for internal flags
configfs: implement committable items
samples: configfs: add a committable group
lib: bitmap: remove the 'extern' keyword from function declarations
lib: bitmap: order includes alphabetically
lib: bitmap: provide devm_bitmap_alloc() and devm_bitmap_zalloc()
gpio: sim: new testing module
selftests: gpio: provide a helper for reading chip info
selftests: gpio: add a helper for reading GPIO line names
selftests: gpio: add test cases for gpio-sim
gpio: sim: actually use the OF module table
Merge tag 'intel-gpio-v5.13-1' of gitolite.kernel.org:pub/scm/linux/kernel/git/andy/linux-gpio-intel into gpio/for-next
Merge tag 'intel-gpio-v5.13-2' of gitolite.kernel.org:pub/scm/linux/kernel/git/andy/linux-gpio-intel into gpio/for-next
gpio: sim: allocate IDA numbers earlier
Colin Ian King (1):
gpio: sim: Fix dereference of free'd pointer config
Geert Uytterhoeven (1):
dt-bindings: gpio: fairchild,74hc595: Convert to json-schema
Hans de Goede (1):
gpiolib: acpi: Add quirk to ignore EC wakeups on Dell Venue 10 Pro 5055
Jan Kiszka (1):
gpio: sch: Add edge event support
Jiapeng Chong (2):
gpio: it87: remove unused code
gpio: mxs: remove useless function
Johan Jonker (1):
dt-bindings: gpio: add YAML description for rockchip,gpio-bank
Jonathan Neuschäfer (1):
docs: driver-api: gpio: consumer: Mark another line of code as such
Linus Walleij (1):
gpio: Mention GPIO MUX in docs
Ran Wang (1):
gpio: mpc8xxx: Add ACPI support
Randy Dunlap (3):
tools: gpio-utils: fix various kernel-doc warnings
gpiolib: some edits of kernel docs for clarity
gpio: sch: depends on LPC_SCH
Sander Vanheule (2):
dt-bindings: gpio: Binding for Realtek Otto GPIO
gpio: Add Realtek Otto GPIO support
Tian Tao (1):
gpio: omap: Use device_get_match_data() helper
Documentation/admin-guide/gpio/gpio-mockup.rst | 11 +-
Documentation/admin-guide/gpio/gpio-sim.rst | 72 ++
Documentation/admin-guide/kernel-parameters.txt | 10 +-
Documentation/core-api/irq/irq-domain.rst | 22 +-
.../bindings/gpio/fairchild,74hc595.yaml | 77 ++
.../devicetree/bindings/gpio/gpio-74x164.txt | 27 -
.../bindings/gpio/realtek,otto-gpio.yaml | 78 ++
.../bindings/gpio/rockchip,gpio-bank.yaml | 82 ++
.../bindings/pinctrl/rockchip,pinctrl.txt | 58 +-
Documentation/driver-api/gpio/consumer.rst | 2 +-
Documentation/driver-api/gpio/drivers-on-gpio.rst | 6 +
Documentation/filesystems/configfs.rst | 6 +-
drivers/gpio/Kconfig | 32 +-
drivers/gpio/Makefile | 2 +
drivers/gpio/gpio-104-dio-48e.c | 50 +-
drivers/gpio/gpio-aggregator.c | 39 +-
drivers/gpio/gpio-ich.c | 2 -
drivers/gpio/gpio-it87.c | 8 -
drivers/gpio/gpio-mockup.c | 9 +-
drivers/gpio/gpio-mpc8xxx.c | 47 +-
drivers/gpio/gpio-mxs.c | 5 -
drivers/gpio/gpio-omap.c | 5 +-
drivers/gpio/gpio-realtek-otto.c | 325 ++++++++
drivers/gpio/gpio-sch.c | 198 ++++-
drivers/gpio/gpio-sim.c | 877 +++++++++++++++++++++
drivers/gpio/gpiolib-acpi.c | 21 +
drivers/gpio/gpiolib-acpi.h | 4 +
drivers/gpio/gpiolib-of.c | 6 +-
drivers/gpio/gpiolib.c | 62 +-
fs/configfs/configfs_internal.h | 22 +-
fs/configfs/dir.c | 245 +++++-
include/linux/bitmap.h | 127 +--
include/linux/configfs.h | 3 +-
include/linux/gpio/driver.h | 12 +-
include/linux/irqdomain.h | 19 +-
kernel/irq/irqdomain.c | 20 +-
lib/bitmap.c | 42 +-
lib/cmdline.c | 1 +
samples/configfs/configfs_sample.c | 153 ++++
tools/gpio/gpio-utils.c | 18 +-
tools/testing/selftests/gpio/.gitignore | 2 +
tools/testing/selftests/gpio/Makefile | 4 +-
tools/testing/selftests/gpio/config | 1 +
tools/testing/selftests/gpio/gpio-chip-info.c | 57 ++
tools/testing/selftests/gpio/gpio-line-name.c | 55 ++
tools/testing/selftests/gpio/gpio-sim.sh | 229 ++++++
46 files changed, 2781 insertions(+), 372 deletions(-)
create mode 100644 Documentation/admin-guide/gpio/gpio-sim.rst
create mode 100644 Documentation/devicetree/bindings/gpio/fairchild,74hc595.yaml
delete mode 100644 Documentation/devicetree/bindings/gpio/gpio-74x164.txt
create mode 100644 Documentation/devicetree/bindings/gpio/realtek,otto-gpio.yaml
create mode 100644 Documentation/devicetree/bindings/gpio/rockchip,gpio-bank.yaml
create mode 100644 drivers/gpio/gpio-realtek-otto.c
create mode 100644 drivers/gpio/gpio-sim.c
create mode 100644 tools/testing/selftests/gpio/gpio-chip-info.c
create mode 100644 tools/testing/selftests/gpio/gpio-line-name.c
create mode 100755 tools/testing/selftests/gpio/gpio-sim.sh
Al,
would you mind taking a look at this part:
On Sun, May 2, 2021 at 12:32 PM Bartosz Golaszewski <[email protected]> wrote:
>
> You'll notice that we have a bunch of configfs commits in our tree not acked by
> the configfs maintainers. These commits implement the concept of committable
> items in configfs - something that was well defined in the documentation for
> years but has remained unimplemented. Despite the first submission of these
> patches back in November 2020[1] and repeated pings & resending, configfs
> maintainers have remained unresponsive. After reviewing these on the GPIO
> mailing list, we decided to pick them up ourselves and send them your way
> together with the first user: the new GPIO simulator.
It doesn't look huge to me, and I don't care all that deeply about
configfs, and honestly, I'm not seeing huge amounts of actual
development there, with recent commits all being about cleanup of vfs
changes (eg things like the new idmapping changes etc).
That said, I really don't want to pull that with some core sanity checking.
So Al, do you see anything horrendous in how that configfs thing uses
a rename to do kind of an "atomic swap" of configfs state?
Linus
On Mon, May 03, 2021 at 11:03:57AM -0700, Linus Torvalds wrote:
> Al,
> would you mind taking a look at this part:
>
> On Sun, May 2, 2021 at 12:32 PM Bartosz Golaszewski <[email protected]> wrote:
> >
> > You'll notice that we have a bunch of configfs commits in our tree not acked by
> > the configfs maintainers. These commits implement the concept of committable
> > items in configfs - something that was well defined in the documentation for
> > years but has remained unimplemented. Despite the first submission of these
> > patches back in November 2020[1] and repeated pings & resending, configfs
> > maintainers have remained unresponsive. After reviewing these on the GPIO
> > mailing list, we decided to pick them up ourselves and send them your way
> > together with the first user: the new GPIO simulator.
>
> It doesn't look huge to me, and I don't care all that deeply about
> configfs, and honestly, I'm not seeing huge amounts of actual
> development there, with recent commits all being about cleanup of vfs
> changes (eg things like the new idmapping changes etc).
>
> That said, I really don't want to pull that with some core sanity checking.
>
> So Al, do you see anything horrendous in how that configfs thing uses
> a rename to do kind of an "atomic swap" of configfs state?
Give me a few hours; configfs is playing silly buggers with a lot of
structures when creating/tearing down subtrees, and I'd actually
expect more trouble with configfs data structures than with VFS ones.
I'll take a look.
On Mon, May 3, 2021 at 11:28 AM Al Viro <[email protected]> wrote:
>
> Give me a few hours; configfs is playing silly buggers with a lot of
> structures when creating/tearing down subtrees, and I'd actually
> expect more trouble with configfs data structures than with VFS ones.
>
> I'll take a look.
Thanks, appreciated,
Linus
On Mon, May 03, 2021 at 06:28:38PM +0000, Al Viro wrote:
> > So Al, do you see anything horrendous in how that configfs thing uses
> > a rename to do kind of an "atomic swap" of configfs state?
>
> Give me a few hours; configfs is playing silly buggers with a lot of
> structures when creating/tearing down subtrees, and I'd actually
> expect more trouble with configfs data structures than with VFS ones.
>
> I'll take a look.
FWIW, one obviously bogus thing is this:
+ spin_lock(&configfs_dirent_lock);
+ new_dentry->d_fsdata = sd;
+ list_move(&sd->s_sibling, &new_parent_sd->s_children);
+ item->ci_parent = new_parent_item;
+ d_move(old_dentry, new_dentry);
+ spin_unlock(&configfs_dirent_lock);
on successful ->rename(). sd here comes from
+ sd = old_dentry->d_fsdata;
Now, take a look at configfs_d_iput(). ->d_fsdata contributes
to refcount of sd, and I don't see anything here that would grab the
reference.
Incidentally, if your code critically depends upon some field
being first in such-and-such structure, you should either get rid of
the dependency or at least bother to document that.
That
+ /*
+ * Free memory allocated for the pending and live directories
+ * of committable groups.
+ */
+ if (sd->s_type & (CONFIGFS_GROUP_PENDING | CONFIGFS_GROUP_LIVE))
+ kfree(sd->s_element);
+
is asking for trouble down the road.
I dislike (for the lack of adequate printable terms) the way configfs
deals with subtree creation and, especially, removal. It's kept attached
to dentry tree (all the way to the root) as we build it and, in case we
fail halfway through, as we are trying to take it apart.
There is convoluted code trying to prevent breakage in such cases,
but it's complex, brittle and I don't remember how critical the lack of
renames had been in its analysis. I can try to redo that, but that would
take some time - IIRC, the last time I did it, it took several days
of work (including arseloads of grepping through configfs users and
doing RTFS in those)
IMO we should attach the subtree we'd built only when it's
fully set up. I can dig out the notes (from 2 years ago) on how to massage
the damn thing in that direction, but again, it'll take a day or two
to verify that analysis still applies. OTOH, that would simplify the code
considerably, so the next time we want to change something it wouldn't
be so unpleasant.
On 5/4/21, Al Viro <[email protected]> wrote:
> On Mon, May 03, 2021 at 06:28:38PM +0000, Al Viro wrote:
>
>> > So Al, do you see anything horrendous in how that configfs thing uses
>> > a rename to do kind of an "atomic swap" of configfs state?
>>
>> Give me a few hours; configfs is playing silly buggers with a lot of
>> structures when creating/tearing down subtrees, and I'd actually
>> expect more trouble with configfs data structures than with VFS ones.
>>
>> I'll take a look.
>
Hi Al and thanks for the comments!
> FWIW, one obviously bogus thing is this:
>
> + spin_lock(&configfs_dirent_lock);
> + new_dentry->d_fsdata = sd;
> + list_move(&sd->s_sibling, &new_parent_sd->s_children);
> + item->ci_parent = new_parent_item;
> + d_move(old_dentry, new_dentry);
> + spin_unlock(&configfs_dirent_lock);
> on successful ->rename(). sd here comes from
> + sd = old_dentry->d_fsdata;
>
> Now, take a look at configfs_d_iput(). ->d_fsdata contributes
> to refcount of sd, and I don't see anything here that would grab the
> reference.
>
> Incidentally, if your code critically depends upon some field
> being first in such-and-such structure, you should either get rid of
> the dependency or at least bother to document that.
> That
> + /*
> + * Free memory allocated for the pending and live
> directories
> + * of committable groups.
> + */
> + if (sd->s_type & (CONFIGFS_GROUP_PENDING |
> CONFIGFS_GROUP_LIVE))
> + kfree(sd->s_element);
> +
> is asking for trouble down the road.
>
I'm not sure if this is a hard NAK for these changes or if you
consider this something that can be ironed out post v5.13-rc1?
> I dislike (for the lack of adequate printable terms) the way configfs
> deals with subtree creation and, especially, removal. It's kept attached
> to dentry tree (all the way to the root) as we build it and, in case we
> fail halfway through, as we are trying to take it apart.
>
> There is convoluted code trying to prevent breakage in such cases,
> but it's complex, brittle and I don't remember how critical the lack of
> renames had been in its analysis. I can try to redo that, but that would
> take some time - IIRC, the last time I did it, it took several days
> of work (including arseloads of grepping through configfs users and
> doing RTFS in those)
>
> IMO we should attach the subtree we'd built only when it's
> fully set up. I can dig out the notes (from 2 years ago) on how to massage
> the damn thing in that direction, but again, it'll take a day or two
> to verify that analysis still applies. OTOH, that would simplify the code
> considerably, so the next time we want to change something it wouldn't
> be so unpleasant.
>
This seems to address fundamental issues with configfs. I probably
don't have enough deep understanding of the VFS to even try to take on
this. My question again is: should this block the committable items
from getting merged or is this a plan for future improvement?
Can we proceed with merging it to see if it causes any regressions
later in the release cycle?
IMO this isn't a case where we could corrupt someone's files if we
make a mistake but I also acknowledge that I'm biased because I'm the
one who wants this functionality to improve our user-space tests.
Best Regards
Bartosz Golaszewski
On Tue, May 04, 2021 at 04:17:02PM +0200, Bartosz Golaszewski wrote:
> > Incidentally, if your code critically depends upon some field
> > being first in such-and-such structure, you should either get rid of
> > the dependency or at least bother to document that.
> > That
> > + /*
> > + * Free memory allocated for the pending and live
> > directories
> > + * of committable groups.
> > + */
> > + if (sd->s_type & (CONFIGFS_GROUP_PENDING |
> > CONFIGFS_GROUP_LIVE))
> > + kfree(sd->s_element);
> > +
> > is asking for trouble down the road.
> >
>
> I'm not sure if this is a hard NAK for these changes or if you
> consider this something that can be ironed out post v5.13-rc1?
Rename implementation is simply bogus. You are, for some reason, attaching
stuff to *destination*, which won't be seen by anyone not currently using
it. It's the old_dentry that will be seen from that point on - you are
moving it to new location by that d_move(). So I rather wonder how much
had that thing been tested. And I'm pretty much certain that you are
mishandling the refcounts on configfs-internal objects, with everything
that entails in terms of UAF and leaks.
FWIW, I'm not happy about the userland API of that thing (what is supposed
to happen if you create, move to live, then create another with the same
name and try to move it to live or original back from live?), but
Documentation/filesystems/configfs.rst is too sparse on such details.
So I would like to see the specifics on that as well. _Before_ signing
up on anything, including "we can fix it up after merge".
On Tue, May 4, 2021 at 8:35 PM Al Viro <[email protected]> wrote:
> On Tue, May 04, 2021 at 04:17:02PM +0200, Bartosz Golaszewski wrote:
...
> So I would like to see the specifics on that as well. _Before_ signing
> up on anything, including "we can fix it up after merge".
Bart, I think we need to split your PR to the patches w/o configfs et
al and submit to Linus to avoid missing a merge window. This stuff can
be queued later as an additional part in case being ready.
--
With Best Regards,
Andy Shevchenko
On Tue, May 4, 2021 at 7:34 PM Al Viro <[email protected]> wrote:
>
> On Tue, May 04, 2021 at 04:17:02PM +0200, Bartosz Golaszewski wrote:
> > > Incidentally, if your code critically depends upon some field
> > > being first in such-and-such structure, you should either get rid of
> > > the dependency or at least bother to document that.
> > > That
> > > + /*
> > > + * Free memory allocated for the pending and live
> > > directories
> > > + * of committable groups.
> > > + */
> > > + if (sd->s_type & (CONFIGFS_GROUP_PENDING |
> > > CONFIGFS_GROUP_LIVE))
> > > + kfree(sd->s_element);
> > > +
> > > is asking for trouble down the road.
> > >
> >
> > I'm not sure if this is a hard NAK for these changes or if you
> > consider this something that can be ironed out post v5.13-rc1?
>
> Rename implementation is simply bogus. You are, for some reason, attaching
> stuff to *destination*, which won't be seen by anyone not currently using
> it. It's the old_dentry that will be seen from that point on - you are
> moving it to new location by that d_move(). So I rather wonder how much
> had that thing been tested. And I'm pretty much certain that you are
> mishandling the refcounts on configfs-internal objects, with everything
> that entails in terms of UAF and leaks.
>
The interface's stability in user-space has been tested a lot with the
test-suite for libgpiod[1] but I didn't look for leaks indeed.
> FWIW, I'm not happy about the userland API of that thing (what is supposed
> to happen if you create, move to live, then create another with the same
> name and try to move it to live or original back from live?), but
> Documentation/filesystems/configfs.rst is too sparse on such details.
> So I would like to see the specifics on that as well. _Before_ signing
> up on anything, including "we can fix it up after merge".
Understood. I've sent out a new PR without these changes. I'll start
another thread asking for your help on the correct approach and maybe
some better ideas for the user interface.
Thanks,
Bartosz
[1] https://patchwork.ozlabs.org/project/linux-gpio/patch/[email protected]/
Hi Al!
This is a follow-up to your review of the configfs rename operation
for committable items. I'd like to start with a disclaimer: I have a
very limited knowledge of the linux VFS and am more of a driver &
platform guy. That means I may be asking silly questions so please be
patient with me.
You've mentioned two sets of problems: one with my rename
implementation and one with the general architecture of configfs.
While I can try to take on the latter as a side-project, I'll
definitely need guidance in the form of your old notes you mentioned.
For the former: I'd like to ask for advice on how to approach it. My
goal is to get an acceptable implementation of committable items in
for v5.14 and then potentially try to improve the overall configfs
design later.
> FWIW, I'm not happy about the userland API of that thing (what is supposed
> to happen if you create, move to live, then create another with the same
> name and try to move it to live or original back from live?), but
> Documentation/filesystems/configfs.rst is too sparse on such details.
> So I would like to see the specifics on that as well. _Before_ signing
> up on anything, including "we can fix it up after merge".
Do you not like just the details like this (i.e. fixing this use-case
and documenting the behavior would be fine) or do you have an entirely
different idea for committable items? I'm open for other designs as
admittedly the hard-coded "live" and "pending" directories aren't very
elegant.
Regarding the repeating names: are there any helpers I could use for
finding the sibling (live for pending and vice versa) and then
inspecting its children for a repeating name? Any caveats to watch out
for?
> Rename implementation is simply bogus. You are, for some reason, attaching
> stuff to *destination*, which won't be seen by anyone not currently using
> it. It's the old_dentry that will be seen from that point on - you are
> moving it to new location by that d_move(). So I rather wonder how much
> had that thing been tested. And I'm pretty much certain that you are
> mishandling the refcounts on configfs-internal objects, with everything
> that entails in terms of UAF and leaks.
I admit I don't really understand the meaning of "won't be seen by
anyone not currently using it". Should this cause some visible
problems in user-space or is it just related to VFS' accounting? Could
you point me to any existing example of the rename() callback that
would be useful in my case? The documentation does not really explain
the goal of rename wrt both the vfs and underlying fs' structures.
I assume d_move() should not be here at all (no other rename()
callback seems to call it) but does moving of the configfs sub-tree to
the live/pending sibling make sense?
Is the fact that configfs has its own tree with children and siblings
in parallel to the VFS a problem? To my inexperienced eye this looks
redundant if the VFS already keeps track of this. Is this one of the
things that should be reworked?
Best regards,
Bartosz Golaszewski