| 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
| ||
|
Message-ID: <20171213094205.GB13194@kroah.com>
Date: Wed, 13 Dec 2017 10:42:05 +0100
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Viresh Kumar <viresh.kumar@...aro.org>
Cc: Vincent Guittot <vincent.guittot@...aro.org>,
Stephen Boyd <sboyd@...eaurora.org>,
Rajendra Nayak <rnayak@...eaurora.org>,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
robdclark@...il.com, s.hauer@...gutronix.de,
l.stach@...gutronix.de, shawnguo@...nel.org, fabio.estevam@....com,
nm@...com, xuwei5@...ilicon.com, robh+dt@...nel.org
Subject: Re: [PATCH V4 03/12] drivers: Add boot constraints core
On Sun, Oct 29, 2017 at 07:18:51PM +0530, Viresh Kumar wrote:
> Some devices are powered ON by the bootloader before the bootloader
> handovers control to Linux. It maybe important for those devices to keep
> working until the time a Linux device driver probes the device and
> reconfigure its resources.
>
> A typical example of that can be the LCD controller, which is used by
> the bootloaders to show image(s) while the platform is booting into
> Linux. The LCD controller can be using some resources, like clk,
> regulators, PM domain, etc, that are shared between several devices.
> These shared resources should be configured to satisfy need of all the
> users. If another device's (X) driver gets probed before the LCD
> controller driver in this case, then it may end up reconfiguring these
> resources to ranges satisfying the current users (only device X) and
> that can make the LCD screen unstable.
>
> This patch introduces the concept of boot-constraints, which will be set
> by the bootloaders and the kernel will satisfy them until the time
> driver for such a device is probed (successfully or unsuccessfully).
>
> The list of boot constraint types is empty for now, and will be
> incrementally updated by later patches.
>
> Only two routines are exposed by the boot constraints core for now:
>
> - dev_boot_constraint_add(): This shall be called by parts of the kernel
> (before the device is probed) to set the constraints.
>
> - dev_boot_constraints_remove(): This is called only by the driver core
> after a device is probed successfully or unsuccessfully. Special
> handling is done here for deferred probing.
>
> Tested-by: Rajendra Nayak <rnayak@...eaurora.org>
> Signed-off-by: Viresh Kumar <viresh.kumar@...aro.org>
Minor nits:
> ---
> drivers/Kconfig | 2 +
> drivers/Makefile | 1 +
> drivers/base/dd.c | 20 ++--
> drivers/boot_constraints/Kconfig | 9 ++
> drivers/boot_constraints/Makefile | 3 +
> drivers/boot_constraints/core.c | 199 ++++++++++++++++++++++++++++++++++++++
> drivers/boot_constraints/core.h | 33 +++++++
> include/linux/boot_constraint.h | 46 +++++++++
> 8 files changed, 306 insertions(+), 7 deletions(-)
> create mode 100644 drivers/boot_constraints/Kconfig
> create mode 100644 drivers/boot_constraints/Makefile
> create mode 100644 drivers/boot_constraints/core.c
> create mode 100644 drivers/boot_constraints/core.h
> create mode 100644 include/linux/boot_constraint.h
>
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index 505c676fa9c7..e595ffad2214 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -4,6 +4,8 @@ source "drivers/amba/Kconfig"
>
> source "drivers/base/Kconfig"
>
> +source "drivers/boot_constraints/Kconfig"
> +
> source "drivers/bus/Kconfig"
>
> source "drivers/connector/Kconfig"
> diff --git a/drivers/Makefile b/drivers/Makefile
> index d90fdc413648..29d03466cb2a 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -72,6 +72,7 @@ obj-$(CONFIG_FB_INTEL) += video/fbdev/intelfb/
> obj-$(CONFIG_PARPORT) += parport/
> obj-$(CONFIG_NVM) += lightnvm/
> obj-y += base/ block/ misc/ mfd/ nfc/
> +obj-$(CONFIG_DEV_BOOT_CONSTRAINTS) += boot_constraints/
> obj-$(CONFIG_LIBNVDIMM) += nvdimm/
> obj-$(CONFIG_DAX) += dax/
> obj-$(CONFIG_DMA_SHARED_BUFFER) += dma-buf/
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index ad44b40fe284..4eec27fe2b2b 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -17,6 +17,7 @@
> * This file is released under the GPLv2
> */
Can you rebase this patch, I think it will have conflicts or fuzz here.
>
> +#include <linux/boot_constraint.h>
> #include <linux/device.h>
> #include <linux/delay.h>
> #include <linux/dma-mapping.h>
> @@ -409,15 +410,20 @@ static int really_probe(struct device *dev, struct device_driver *drv)
> */
> devices_kset_move_last(dev);
>
> - if (dev->bus->probe) {
> + if (dev->bus->probe)
> ret = dev->bus->probe(dev);
> - if (ret)
> - goto probe_failed;
> - } else if (drv->probe) {
> + else if (drv->probe)
> ret = drv->probe(dev);
> - if (ret)
> - goto probe_failed;
> - }
> +
> + /*
> + * Remove boot constraints for both successful and unsuccessful probe(),
> + * except for the case where EPROBE_DEFER is returned by probe().
> + */
> + if (ret != -EPROBE_DEFER)
> + dev_boot_constraints_remove(dev);
This feels odd, but ok, I trust you :)
> +
> + if (ret)
> + goto probe_failed;
>
> if (test_remove) {
> test_remove = false;
> diff --git a/drivers/boot_constraints/Kconfig b/drivers/boot_constraints/Kconfig
> new file mode 100644
> index 000000000000..77831af1c6fb
> --- /dev/null
> +++ b/drivers/boot_constraints/Kconfig
> @@ -0,0 +1,9 @@
> +config DEV_BOOT_CONSTRAINTS
> + bool "Boot constraints for devices"
> + help
> + This enables boot constraints detection for devices. These constraints
> + are (normally) set by the Bootloader and must be satisfied by the
> + kernel until the relevant device driver is probed. Once the driver is
> + probed, the constraint is dropped.
> +
> + If unsure, say N.
> diff --git a/drivers/boot_constraints/Makefile b/drivers/boot_constraints/Makefile
> new file mode 100644
> index 000000000000..0f2680177974
> --- /dev/null
> +++ b/drivers/boot_constraints/Makefile
> @@ -0,0 +1,3 @@
> +# Makefile for device boot constraints
> +
> +obj-y := core.o
> diff --git a/drivers/boot_constraints/core.c b/drivers/boot_constraints/core.c
> new file mode 100644
> index 000000000000..366a05d6d9ba
> --- /dev/null
> +++ b/drivers/boot_constraints/core.c
> @@ -0,0 +1,199 @@
> +/*
> + * This takes care of boot time device constraints, normally set by the
> + * Bootloader.
> + *
> + * Copyright (C) 2017 Linaro.
> + * Viresh Kumar <viresh.kumar@...aro.org>
> + *
> + * This file is released under the GPLv2.
Care to update this patch with the new SPDX format for licenses?
> + */
> +
> +#define pr_fmt(fmt) "Boot Constraints: " fmt
You don't have any pr_* calls, so this isn't needed :)
> +struct constraint {
> + struct constraint_dev *cdev;
> + struct list_head node;
> + enum dev_boot_constraint_type type;
> + void (*free_resources)(void *data);
> + void *free_resources_data;
> +
> + int (*add)(struct constraint *constraint, void *data);
> + void (*remove)(struct constraint *constraint);
> + void *private;
> +};
> +
> +/* Forward declarations of constraint specific callbacks */
> +#endif /* _CORE_H */
What is this comment at the end of the file for?
> diff --git a/include/linux/boot_constraint.h b/include/linux/boot_constraint.h
> new file mode 100644
> index 000000000000..2b816bf74144
> --- /dev/null
> +++ b/include/linux/boot_constraint.h
> @@ -0,0 +1,46 @@
> +/*
> + * Boot constraints header.
> + *
> + * Copyright (C) 2017 Linaro.
> + * Viresh Kumar <viresh.kumar@...aro.org>
> + *
> + * This file is released under the GPLv2
> + */
> +#ifndef _LINUX_BOOT_CONSTRAINT_H
> +#define _LINUX_BOOT_CONSTRAINT_H
> +
> +#include <linux/err.h>
> +#include <linux/types.h>
> +
> +struct device;
> +
> +enum dev_boot_constraint_type {
> + DEV_BOOT_CONSTRAINT_NONE,
> +};
> +
> +struct dev_boot_constraint {
> + enum dev_boot_constraint_type type;
> + void *data;
> +};
> +
> +struct dev_boot_constraint_info {
> + struct dev_boot_constraint constraint;
> +
> + /* This will be called just before the constraint is removed */
> + void (*free_resources)(void *data);
> + void *free_resources_data;
> +};
> +
> +#ifdef CONFIG_DEV_BOOT_CONSTRAINTS
> +int dev_boot_constraint_add(struct device *dev,
> + struct dev_boot_constraint_info *info);
> +void dev_boot_constraints_remove(struct device *dev);
> +#else
> +static inline
> +int dev_boot_constraint_add(struct device *dev,
> + struct dev_boot_constraint_info *info)
> +{ return -EINVAL; }
Why return an error? Shouldn't this just "succeed" if the option is not
built in? What will you do with it if it fails because of this?
thanks,
greg k-h
Powered by blists - more mailing lists