linux-kernel - Re: [PATCH v2] gpiolib: split character device into gpiolib-cdev

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 4 Jun 2020 22:18:04 +0800
From:   Kent Gibson <warthog618@...il.com>
To:     Bartosz Golaszewski <bgolaszewski@...libre.com>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        linux-gpio <linux-gpio@...r.kernel.org>,
        Linus Walleij <linus.walleij@...aro.org>
Subject: Re: [PATCH v2] gpiolib: split character device into gpiolib-cdev

On Thu, Jun 04, 2020 at 02:04:08PM +0200, Bartosz Golaszewski wrote:
> wt., 2 cze 2020 o 16:11 Kent Gibson <warthog618@...il.com> napisał(a):
> >
> > Split the cdev specific functionality out of gpiolib.c and into
> > gpiolib-cdev.c. This improves the readability and maintainability of both
> > the cdev and core gpiolib code.
> >
> > Suggested-by: Bartosz Golaszewski <bgolaszewski@...libre.com>
> > Signed-off-by: Kent Gibson <warthog618@...il.com>
> >
> > ---
> > While this patch is complete and ready for review, I don't expect it to
> > be applied as is. There are a few cdev patches pending merge into
> > gpio/devel that are sure to conflict, and it makes more sense to
> > rebase this on top of them than vice versa. But I thought it would
> > be worthwhile to get it out for review so it can be ready to be rebased
> > when the time is right.
> >
> > Also, this is a naive split. That is, if applied as is, it will lose the
> > line history of the cdev code.  This is not what I intend, and I
> > understand can be avoided by encouraging git to remember the history
> > with a few moves, but I'm unsure how the maintainers would prefer that
> > to be done.
> >
> > Bart,
> >  As this was your idea, I've taken the liberty of adding the Suggested-by.
> >  I hope that is ok.
> >
> > Changes in v2:
> >  - rebased to latest gpio/devel and added base-commit to placate the
> >   build bot.  The comments above still apply, as there are still a
> >   couple of commits in gpio/fixes that will conflict.
> >
> > Kent.
> 
> Thanks for doing this Kent!
> 
> This looks mostly good, see a single comment below.
> 
> Linus: do you think we can get this in for v5.8? Maybe apply this as
> the last patch before your PR?
> 
> >
> >  drivers/gpio/Makefile       |    1 +
> >  drivers/gpio/gpiolib-cdev.c | 1148 +++++++++++++++++++++++++++++++++++
> >  drivers/gpio/gpiolib-cdev.h |   11 +
> >  drivers/gpio/gpiolib.c      | 1112 +--------------------------------
> >  4 files changed, 1164 insertions(+), 1108 deletions(-)
> >  create mode 100644 drivers/gpio/gpiolib-cdev.c
> >  create mode 100644 drivers/gpio/gpiolib-cdev.h
> >
> > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> > index 65bf3940e33c..b5b58b624f37 100644
> > --- a/drivers/gpio/Makefile
> > +++ b/drivers/gpio/Makefile
> > @@ -7,6 +7,7 @@ obj-$(CONFIG_GPIOLIB)           += gpiolib.o
> >  obj-$(CONFIG_GPIOLIB)          += gpiolib-devres.o
> >  obj-$(CONFIG_GPIOLIB)          += gpiolib-legacy.o
> >  obj-$(CONFIG_GPIOLIB)          += gpiolib-devprop.o
> > +obj-$(CONFIG_GPIOLIB)          += gpiolib-cdev.o
> >  obj-$(CONFIG_OF_GPIO)          += gpiolib-of.o
> >  obj-$(CONFIG_GPIO_SYSFS)       += gpiolib-sysfs.o
> >  obj-$(CONFIG_GPIO_ACPI)                += gpiolib-acpi.o
> > diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c
> > new file mode 100644
> > index 000000000000..971470bdc9c9
> > --- /dev/null
> > +++ b/drivers/gpio/gpiolib-cdev.c
> > @@ -0,0 +1,1148 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <linux/bitmap.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/irqreturn.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/gpio.h>
> > +#include <linux/gpio/driver.h>
> > +#include <linux/pinctrl/consumer.h>
> > +#include <linux/cdev.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/compat.h>
> > +#include <linux/anon_inodes.h>
> > +#include <linux/file.h>
> > +#include <linux/kfifo.h>
> > +#include <linux/poll.h>
> > +#include <linux/timekeeping.h>
> > +#include <uapi/linux/gpio.h>
> > +
> > +
> > +#include "gpiolib.h"
> > +
> > +/* Implementation infrastructure for GPIO interfaces.
> > + *
> > + * The GPIO programming interface allows for inlining speed-critical
> > + * get/set operations for common cases, so that access to SOC-integrated
> > + * GPIOs can sometimes cost only an instruction or two per bit.
> > + */
> 
> Is this comment relevant for the character device?
> 

True - that comment should stay in gpiolib, and gpiolib-cdev should get
one of it's own.

Any suggestions on how to maintain line history?
I know you can trick git by moving the original file into two new ones,
then moving one of those back to the old name, but not sure if that is
what you would want to see in a patch.

Cheers,
Kent.