| Message ID | 20210115210107.3087548-1-saravanak@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] gpiolib: Bind gpio_device to a driver to enable fw_devlink=on by default | expand |
Hi Saravana,
I love your patch! Yet something to improve:
[auto build test ERROR on gpio/for-next]
[also build test ERROR on v5.11-rc3 next-20210115]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Saravana-Kannan/gpiolib-Bind-gpio_device-to-a-driver-to-enable-fw_devlink-on-by-default/20210116-050450
base: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git for-next
config: arm64-randconfig-r023-20210115 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 5b42fd8dd4e7e29125a09a41a33af7c9cb57d144)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm64 cross compiling tool for clang build
# apt-get install binutils-aarch64-linux-gnu
# https://github.com/0day-ci/linux/commit/8c370ef8f557575cb23e5e288c861d9447db0b3e
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Saravana-Kannan/gpiolib-Bind-gpio_device-to-a-driver-to-enable-fw_devlink-on-by-default/20210116-050450
git checkout 8c370ef8f557575cb23e5e288c861d9447db0b3e
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> drivers/gpio/gpiolib.c:614:69: error: too few arguments to function call, expected 3, have 2
if (!fwnode_dev && of_find_property(gdev->dev.of_node, "compatible")) {
~~~~~~~~~~~~~~~~ ^
include/linux/compiler.h:56:47: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^~~~
include/linux/compiler.h:58:52: note: expanded from macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
^~~~
include/linux/of.h:304:25: note: 'of_find_property' declared here
extern struct property *of_find_property(const struct device_node *np,
^
>> drivers/gpio/gpiolib.c:614:69: error: too few arguments to function call, expected 3, have 2
if (!fwnode_dev && of_find_property(gdev->dev.of_node, "compatible")) {
~~~~~~~~~~~~~~~~ ^
include/linux/compiler.h:56:47: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^~~~
include/linux/compiler.h:58:61: note: expanded from macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
^~~~
include/linux/of.h:304:25: note: 'of_find_property' declared here
extern struct property *of_find_property(const struct device_node *np,
^
>> drivers/gpio/gpiolib.c:614:69: error: too few arguments to function call, expected 3, have 2
if (!fwnode_dev && of_find_property(gdev->dev.of_node, "compatible")) {
~~~~~~~~~~~~~~~~ ^
include/linux/compiler.h:56:47: note: expanded from macro 'if'
#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
^~~~
include/linux/compiler.h:58:86: note: expanded from macro '__trace_if_var'
#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
^~~~
include/linux/compiler.h:69:3: note: expanded from macro '__trace_if_value'
(cond) ? \
^~~~
include/linux/of.h:304:25: note: 'of_find_property' declared here
extern struct property *of_find_property(const struct device_node *np,
^
>> drivers/gpio/gpiolib.c:615:51: error: expected ')'
chip_warn(gc, "Create a real device for %pOF\n" of_node);
^
drivers/gpio/gpiolib.c:615:3: note: to match this '('
chip_warn(gc, "Create a real device for %pOF\n" of_node);
^
drivers/gpio/gpiolib.h:182:2: note: expanded from macro 'chip_warn'
dev_warn(&gc->gpiodev->dev, "(%s): " fmt, gc->label, ##__VA_ARGS__)
^
include/linux/dev_printk.h:114:11: note: expanded from macro 'dev_warn'
_dev_warn(dev, dev_fmt(fmt), ##__VA_ARGS__)
^
4 errors generated.
vim +614 drivers/gpio/gpiolib.c
567
568 int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data,
569 struct lock_class_key *lock_key,
570 struct lock_class_key *request_key)
571 {
572 unsigned long flags;
573 int ret = 0;
574 unsigned i;
575 int base = gc->base;
576 struct gpio_device *gdev;
577 struct device_node *of_node;
578 struct fwnode_handle *fwnode;
579 struct device *fwnode_dev;
580
581 /*
582 * First: allocate and populate the internal stat container, and
583 * set up the struct device.
584 */
585 gdev = kzalloc(sizeof(*gdev), GFP_KERNEL);
586 if (!gdev)
587 return -ENOMEM;
588 gdev->dev.bus = &gpio_bus_type;
589 gdev->chip = gc;
590 gc->gpiodev = gdev;
591 if (gc->parent) {
592 gdev->dev.parent = gc->parent;
593 gdev->dev.of_node = gc->parent->of_node;
594 }
595
596 #ifdef CONFIG_OF_GPIO
597 /* If the gpiochip has an assigned OF node this takes precedence */
598 if (gc->of_node)
599 gdev->dev.of_node = gc->of_node;
600 else
601 gc->of_node = gdev->dev.of_node;
602
603 of_node = gdev->dev.of_node;
604 fwnode = of_fwnode_handle(of_node);
605 fwnode_dev = get_dev_from_fwnode(fwnode);
606
607 /*
608 * If your driver hits this warning, it's because you are directly
609 * parsing a device tree node with "compatible" property and
610 * initializing it instead of using the standard DT + device driver
611 * model of creating a struct device and then initializing it in the
612 * probe function. Please refactor your driver.
613 */
> 614 if (!fwnode_dev && of_find_property(gdev->dev.of_node, "compatible")) {
> 615 chip_warn(gc, "Create a real device for %pOF\n" of_node);
616 gdev->dev.fwnode = fwnode;
617 }
618 #endif
619
620 gdev->id = ida_alloc(&gpio_ida, GFP_KERNEL);
621 if (gdev->id < 0) {
622 ret = gdev->id;
623 goto err_free_gdev;
624 }
625 dev_set_name(&gdev->dev, GPIOCHIP_NAME "%d", gdev->id);
626 device_initialize(&gdev->dev);
627 dev_set_drvdata(&gdev->dev, gdev);
628 if (gc->parent && gc->parent->driver)
629 gdev->owner = gc->parent->driver->owner;
630 else if (gc->owner)
631 /* TODO: remove chip->owner */
632 gdev->owner = gc->owner;
633 else
634 gdev->owner = THIS_MODULE;
635
636 gdev->descs = kcalloc(gc->ngpio, sizeof(gdev->descs[0]), GFP_KERNEL);
637 if (!gdev->descs) {
638 ret = -ENOMEM;
639 goto err_free_ida;
640 }
641
642 if (gc->ngpio == 0) {
643 chip_err(gc, "tried to insert a GPIO chip with zero lines\n");
644 ret = -EINVAL;
645 goto err_free_descs;
646 }
647
648 if (gc->ngpio > FASTPATH_NGPIO)
649 chip_warn(gc, "line cnt %u is greater than fast path cnt %u\n",
650 gc->ngpio, FASTPATH_NGPIO);
651
652 gdev->label = kstrdup_const(gc->label ?: "unknown", GFP_KERNEL);
653 if (!gdev->label) {
654 ret = -ENOMEM;
655 goto err_free_descs;
656 }
657
658 gdev->ngpio = gc->ngpio;
659 gdev->data = data;
660
661 spin_lock_irqsave(&gpio_lock, flags);
662
663 /*
664 * TODO: this allocates a Linux GPIO number base in the global
665 * GPIO numberspace for this chip. In the long run we want to
666 * get *rid* of this numberspace and use only descriptors, but
667 * it may be a pipe dream. It will not happen before we get rid
668 * of the sysfs interface anyways.
669 */
670 if (base < 0) {
671 base = gpiochip_find_base(gc->ngpio);
672 if (base < 0) {
673 ret = base;
674 spin_unlock_irqrestore(&gpio_lock, flags);
675 goto err_free_label;
676 }
677 /*
678 * TODO: it should not be necessary to reflect the assigned
679 * base outside of the GPIO subsystem. Go over drivers and
680 * see if anyone makes use of this, else drop this and assign
681 * a poison instead.
682 */
683 gc->base = base;
684 }
685 gdev->base = base;
686
687 ret = gpiodev_add_to_list(gdev);
688 if (ret) {
689 spin_unlock_irqrestore(&gpio_lock, flags);
690 goto err_free_label;
691 }
692
693 for (i = 0; i < gc->ngpio; i++)
694 gdev->descs[i].gdev = gdev;
695
696 spin_unlock_irqrestore(&gpio_lock, flags);
697
698 BLOCKING_INIT_NOTIFIER_HEAD(&gdev->notifier);
699
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Fri, Jan 15, 2021 at 11:03 PM Saravana Kannan <saravanak@google.com> wrote: > > There are multiple instances of GPIO devictree nodes of the form: What? Device Tree (or device tree). > foo { > compatible = "acme,foo"; > ... > > gpio0: gpio0@xxxxxxxx { > compatible = "acme,bar"; > ... > gpio-controller; > }; > > gpio1: gpio1@xxxxxxxx { > compatible = "acme,bar"; > ... > gpio-controller; > }; > > ... > } > > bazz { > my-gpios = <&gpio0 ...>; > } > > Case 1: The driver for "foo" populates struct device for these gpio* > nodes and then probes them using a driver that binds with "acme,bar". > This lines up with how DT nodes with the "compatible" property are > generally converted to struct devices and then registered with driver > core to probe them. This also allows the gpio* devices to hook into all > the driver core capabilities like runtime PM, probe deferral, > suspend/resume ordering, device links, etc. > > Case 2: The driver for "foo" doesn't populate its child device nodes > with "compatible" property and instead just loops through its child > nodes and directly registers the GPIOs with gpiolib without ever > populating a struct device or binding a driver to it. > > Drivers that follow the case 2 cause problems with fw_devlink=on. This follow case > is because fw_devlink will prevent bazz from probing until there's a > struct device that has gpio0 as its fwnode (because bazz lists gpio0 as > a GPIO supplier). Once the struct device is available, fw_devlink will > create a device link between with gpio0 as the supplier and bazz as the > consumer. After this point, the device link will prevent bazz from > probing until its supplier (the gpio0 device) has bound to a driver. > Once the supplier is bound to a driver, the probe of bazz is triggered > automatically. > > Finding and refactoring all the instances of drivers that follow case 2 > will cause a lot of code churn and it not something that can be done in it is not > one shot. Examples of such instances are [1] [2]. > > This patch works around this problem and avoids all the code churn by > simply creating a stub driver to bind to the gpio_device. Since the > gpio_device already points to the GPIO device tree node, this allows all > the consumers to continue probing when the driver follows case 2. > > If/when all the old drivers are refactored, we can revert this patch. > > [1] - https://lore.kernel.org/lkml/20201014191235.7f71fcb4@xhacker.debian/ > [2] - https://lore.kernel.org/lkml/e28e1f38d87c12a3c714a6573beba6e1@kernel.org/ Link: tags? ... > + of_node = gdev->dev.of_node; This seems unused (see below). > + fwnode = of_fwnode_handle(of_node); I don't get this. Are you telling that dev_fwnode(&gdev->dev) is not the same? > + fwnode_dev = get_dev_from_fwnode(fwnode); > + > + /* > + * If your driver hits this warning, it's because you are directly > + * parsing a device tree node with "compatible" property and > + * initializing it instead of using the standard DT + device driver > + * model of creating a struct device and then initializing it in the > + * probe function. Please refactor your driver. > + */ > + if (!fwnode_dev && of_find_property(gdev->dev.of_node, "compatible")) { fwnode_property_present() ? > + chip_warn(gc, "Create a real device for %pOF\n" of_node); %pfw ? > + gdev->dev.fwnode = fwnode; Why not dev_fwnode()? > + } > #endif ... > +static struct device_driver gpio_drv = { > + .name = "gpio_drv", Can it have a better name, please? > + .bus = &gpio_bus_type, > + .probe = gpio_drv_probe, > +};
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index b02cc2abd3b6..36f0af42e203 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -574,6 +574,9 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, unsigned i; int base = gc->base; struct gpio_device *gdev; + struct device_node *of_node; + struct fwnode_handle *fwnode; + struct device *fwnode_dev; /* * First: allocate and populate the internal stat container, and @@ -596,6 +599,22 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, gdev->dev.of_node = gc->of_node; else gc->of_node = gdev->dev.of_node; + + of_node = gdev->dev.of_node; + fwnode = of_fwnode_handle(of_node); + fwnode_dev = get_dev_from_fwnode(fwnode); + + /* + * If your driver hits this warning, it's because you are directly + * parsing a device tree node with "compatible" property and + * initializing it instead of using the standard DT + device driver + * model of creating a struct device and then initializing it in the + * probe function. Please refactor your driver. + */ + if (!fwnode_dev && of_find_property(gdev->dev.of_node, "compatible")) { + chip_warn(gc, "Create a real device for %pOF\n" of_node); + gdev->dev.fwnode = fwnode; + } #endif gdev->id = ida_alloc(&gpio_ida, GFP_KERNEL); @@ -4202,6 +4221,17 @@ void gpiod_put_array(struct gpio_descs *descs) } EXPORT_SYMBOL_GPL(gpiod_put_array); +static int gpio_drv_probe(struct device *dev) +{ + return 0; +} + +static struct device_driver gpio_drv = { + .name = "gpio_drv", + .bus = &gpio_bus_type, + .probe = gpio_drv_probe, +}; + static int __init gpiolib_dev_init(void) { int ret; @@ -4213,9 +4243,16 @@ static int __init gpiolib_dev_init(void) return ret; } + if (driver_register(&gpio_drv) < 0) { + pr_err("gpiolib: could not register GPIO stub driver\n"); + bus_unregister(&gpio_bus_type); + return ret; + } + ret = alloc_chrdev_region(&gpio_devt, 0, GPIO_DEV_MAX, GPIOCHIP_NAME); if (ret < 0) { pr_err("gpiolib: failed to allocate char dev region\n"); + driver_unregister(&gpio_drv); bus_unregister(&gpio_bus_type); return ret; }
There are multiple instances of GPIO devictree nodes of the form: foo { compatible = "acme,foo"; ... gpio0: gpio0@xxxxxxxx { compatible = "acme,bar"; ... gpio-controller; }; gpio1: gpio1@xxxxxxxx { compatible = "acme,bar"; ... gpio-controller; }; ... } bazz { my-gpios = <&gpio0 ...>; } Case 1: The driver for "foo" populates struct device for these gpio* nodes and then probes them using a driver that binds with "acme,bar". This lines up with how DT nodes with the "compatible" property are generally converted to struct devices and then registered with driver core to probe them. This also allows the gpio* devices to hook into all the driver core capabilities like runtime PM, probe deferral, suspend/resume ordering, device links, etc. Case 2: The driver for "foo" doesn't populate its child device nodes with "compatible" property and instead just loops through its child nodes and directly registers the GPIOs with gpiolib without ever populating a struct device or binding a driver to it. Drivers that follow the case 2 cause problems with fw_devlink=on. This is because fw_devlink will prevent bazz from probing until there's a struct device that has gpio0 as its fwnode (because bazz lists gpio0 as a GPIO supplier). Once the struct device is available, fw_devlink will create a device link between with gpio0 as the supplier and bazz as the consumer. After this point, the device link will prevent bazz from probing until its supplier (the gpio0 device) has bound to a driver. Once the supplier is bound to a driver, the probe of bazz is triggered automatically. Finding and refactoring all the instances of drivers that follow case 2 will cause a lot of code churn and it not something that can be done in one shot. Examples of such instances are [1] [2]. This patch works around this problem and avoids all the code churn by simply creating a stub driver to bind to the gpio_device. Since the gpio_device already points to the GPIO device tree node, this allows all the consumers to continue probing when the driver follows case 2. If/when all the old drivers are refactored, we can revert this patch. [1] - https://lore.kernel.org/lkml/20201014191235.7f71fcb4@xhacker.debian/ [2] - https://lore.kernel.org/lkml/e28e1f38d87c12a3c714a6573beba6e1@kernel.org/ Cc: Marc Zyngier <maz@kernel.org> Cc: Jisheng Zhang <Jisheng.Zhang@synaptics.com> Cc: Kever Yang <kever.yang@rock-chips.com> Fixes: e590474768f1 ("driver core: Set fw_devlink=on by default") Signed-off-by: Saravana Kannan <saravanak@google.com> --- drivers/gpio/gpiolib.c | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)