| 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: <3238562932094718b902ee0a2e3a3c8e@SFHDAG7NODE2.st.com>
Date: Wed, 31 Jul 2019 09:22:17 +0000
From: Loic PALLARDY <loic.pallardy@...com>
To: Fabien DESSENNE <fabien.dessenne@...com>,
Ohad Ben-Cohen <ohad@...ery.com>,
Bjorn Andersson <bjorn.andersson@...aro.org>,
Rob Herring <robh+dt@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Maxime Coquelin <mcoquelin.stm32@...il.com>,
Alexandre TORGUE <alexandre.torgue@...com>,
Jonathan Corbet <corbet@....net>,
"linux-remoteproc@...r.kernel.org" <linux-remoteproc@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-stm32@...md-mailman.stormreply.com"
<linux-stm32@...md-mailman.stormreply.com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>
CC: Fabien DESSENNE <fabien.dessenne@...com>,
Benjamin GAIGNARD <benjamin.gaignard@...com>
Subject: RE: [PATCH 2/6] hwspinlock: allow sharing of hwspinlocks
> -----Original Message-----
> From: linux-remoteproc-owner@...r.kernel.org <linux-remoteproc-
> owner@...r.kernel.org> On Behalf Of Fabien Dessenne
> Sent: mercredi 13 mars 2019 16:51
> To: Ohad Ben-Cohen <ohad@...ery.com>; Bjorn Andersson
> <bjorn.andersson@...aro.org>; Rob Herring <robh+dt@...nel.org>; Mark
> Rutland <mark.rutland@....com>; Maxime Coquelin
> <mcoquelin.stm32@...il.com>; Alexandre TORGUE
> <alexandre.torgue@...com>; Jonathan Corbet <corbet@....net>; linux-
> remoteproc@...r.kernel.org; devicetree@...r.kernel.org; linux-
> kernel@...r.kernel.org; linux-stm32@...md-mailman.stormreply.com;
> linux-arm-kernel@...ts.infradead.org; linux-doc@...r.kernel.org
> Cc: Fabien DESSENNE <fabien.dessenne@...com>; Benjamin GAIGNARD
> <benjamin.gaignard@...com>
> Subject: [PATCH 2/6] hwspinlock: allow sharing of hwspinlocks
>
> The current implementation does not allow different devices to use a
> common hwspinlock. Offer the possibility to use the same hwspinlock by
> several users.
> If a device registers to the framework with #hwlock-cells = 2, then
> the second parameter of the 'hwlocks' DeviceTree property defines
> whether an hwlock is requested for an exclusive or a shared usage.
> If a device registers with #hwlock-cells = 1, then all the hwlocks are
> for an exclusive usage.
>
> Signed-off-by: Fabien Dessenne <fabien.dessenne@...com>
Looks good for me.
Acked-by: Loic Pallardy <loic.pallardy@...com>
Regards,
Loic
> ---
> Documentation/hwspinlock.txt | 10 ++--
> drivers/hwspinlock/hwspinlock_core.c | 82
> +++++++++++++++++++++++++-------
> drivers/hwspinlock/hwspinlock_internal.h | 2 +
> 3 files changed, 73 insertions(+), 21 deletions(-)
>
> diff --git a/Documentation/hwspinlock.txt b/Documentation/hwspinlock.txt
> index ed640a2..e6ce2dd 100644
> --- a/Documentation/hwspinlock.txt
> +++ b/Documentation/hwspinlock.txt
> @@ -54,9 +54,11 @@ Should be called from a process context (might sleep).
> struct hwspinlock *hwspin_lock_request_specific(unsigned int id);
>
> Assign a specific hwspinlock id and return its address, or NULL
> -if that hwspinlock is already in use. Usually board code will
> -be calling this function in order to reserve specific hwspinlock
> -ids for predefined purposes.
> +if that hwspinlock is already in use and not shared. If that specific
> +hwspinlock is declared as shared, it can be requested and used by
> +several users.
> +Usually board code will be calling this function in order to reserve
> +specific hwspinlock ids for predefined purposes.
>
> Should be called from a process context (might sleep).
>
> @@ -368,11 +370,13 @@ of which represents a single hardware lock::
> * struct hwspinlock - this struct represents a single hwspinlock
> instance
> * @bank: the hwspinlock_device structure which owns this lock
> * @lock: initialized and used by hwspinlock core
> + * @refcount: number of users (when shared)
> * @priv: private data, owned by the underlying platform-specific
> hwspinlock drv
> */
> struct hwspinlock {
> struct hwspinlock_device *bank;
> spinlock_t lock;
> + unsigned int refcount;
> void *priv;
> };
>
> diff --git a/drivers/hwspinlock/hwspinlock_core.c
> b/drivers/hwspinlock/hwspinlock_core.c
> index 2bad40d..53afdeb 100644
> --- a/drivers/hwspinlock/hwspinlock_core.c
> +++ b/drivers/hwspinlock/hwspinlock_core.c
> @@ -25,6 +25,8 @@
>
> /* radix tree tags */
> #define HWSPINLOCK_UNUSED (0) /* tags an hwspinlock as unused
> */
> +#define HWSPINLOCK_EXCLUSIVE (1) /* tags an hwspinlock as exclusive
> */
> +#define HWSPINLOCK_SHARED (2) /* tags an hwspinlock as shared */
>
> /*
> * A radix tree is used to maintain the available hwspinlock instances.
> @@ -291,7 +293,7 @@ EXPORT_SYMBOL_GPL(__hwspin_unlock);
> * @hwlock_spec: hwlock specifier as found in the device tree
> *
> * This is a simple translation function, suitable for hwspinlock platform
> - * drivers that only has a lock specifier length of 1.
> + * drivers that only has a lock specifier length of 1 or 2.
> *
> * Returns a relative index of the lock within a specified bank on success,
> * or -EINVAL on invalid specifier cell count.
> @@ -299,7 +301,8 @@ EXPORT_SYMBOL_GPL(__hwspin_unlock);
> static inline int
> of_hwspin_lock_simple_xlate(const struct of_phandle_args *hwlock_spec)
> {
> - if (WARN_ON(hwlock_spec->args_count != 1))
> + if (WARN_ON(hwlock_spec->args_count != 1 &&
> + hwlock_spec->args_count != 2))
> return -EINVAL;
>
> return hwlock_spec->args[0];
> @@ -322,11 +325,12 @@ of_hwspin_lock_simple_xlate(const struct
> of_phandle_args *hwlock_spec)
> int of_hwspin_lock_get_id(struct device_node *np, int index)
> {
> struct of_phandle_args args;
> - struct hwspinlock *hwlock;
> + struct hwspinlock *hwlock, *tmp;
> struct radix_tree_iter iter;
> void **slot;
> int id;
> int ret;
> + unsigned int tag;
>
> ret = of_parse_phandle_with_args(np, "hwlocks", "#hwlock-cells",
> index,
> &args);
> @@ -361,6 +365,37 @@ int of_hwspin_lock_get_id(struct device_node *np,
> int index)
> }
> id += hwlock->bank->base_id;
>
> + /* Set the EXCLUSIVE / SHARED tag */
> + if (args.args_count == 2 && args.args[1]) {
> + /* Tag SHARED unless already tagged EXCLUSIVE */
> + if (radix_tree_tag_get(&hwspinlock_tree, id,
> + HWSPINLOCK_EXCLUSIVE)) {
> + ret = -EINVAL;
> + goto out;
> + }
> + tag = HWSPINLOCK_SHARED;
> + } else {
> + /* Tag EXCLUSIVE unless already tagged SHARED */
> + if (radix_tree_tag_get(&hwspinlock_tree, id,
> + HWSPINLOCK_SHARED)) {
> + ret = -EINVAL;
> + goto out;
> + }
> + tag = HWSPINLOCK_EXCLUSIVE;
> + }
> +
> + /* mark this hwspinlock */
> + hwlock = radix_tree_lookup(&hwspinlock_tree, id);
> + if (!hwlock) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + tmp = radix_tree_tag_set(&hwspinlock_tree, id, tag);
> +
> + /* self-sanity check which should never fail */
> + WARN_ON(tmp != hwlock);
> +
> out:
> of_node_put(args.np);
> return ret ? ret : id;
> @@ -483,6 +518,7 @@ int hwspin_lock_register(struct hwspinlock_device
> *bank, struct device *dev,
>
> spin_lock_init(&hwlock->lock);
> hwlock->bank = bank;
> + hwlock->refcount = 0;
>
> ret = hwspin_lock_register_single(hwlock, base_id + i);
> if (ret)
> @@ -625,7 +661,7 @@ static int __hwspin_lock_request(struct hwspinlock
> *hwlock)
> {
> struct device *dev = hwlock->bank->dev;
> struct hwspinlock *tmp;
> - int ret;
> + int ret, id;
>
> /* prevent underlying implementation from being removed */
> if (!try_module_get(dev->driver->owner)) {
> @@ -642,13 +678,18 @@ static int __hwspin_lock_request(struct hwspinlock
> *hwlock)
> return ret;
> }
>
> + /* update shareable refcount */
> + id = hwlock_to_id(hwlock);
> + if (radix_tree_tag_get(&hwspinlock_tree, id, HWSPINLOCK_SHARED)
> &&
> + hwlock->refcount++)
> + goto out;
> +
> /* mark hwspinlock as used, should not fail */
> - tmp = radix_tree_tag_clear(&hwspinlock_tree,
> hwlock_to_id(hwlock),
> -
> HWSPINLOCK_UNUSED);
> + tmp = radix_tree_tag_clear(&hwspinlock_tree, id,
> HWSPINLOCK_UNUSED);
>
> /* self-sanity check that should never fail */
> WARN_ON(tmp != hwlock);
> -
> +out:
> return ret;
> }
>
> @@ -742,9 +783,9 @@ struct hwspinlock
> *hwspin_lock_request_specific(unsigned int id)
> /* sanity check (this shouldn't happen) */
> WARN_ON(hwlock_to_id(hwlock) != id);
>
> - /* make sure this hwspinlock is unused */
> - ret = radix_tree_tag_get(&hwspinlock_tree, id,
> HWSPINLOCK_UNUSED);
> - if (ret == 0) {
> + /* make sure this hwspinlock is unused or shareable */
> + if (!radix_tree_tag_get(&hwspinlock_tree, id,
> HWSPINLOCK_SHARED) &&
> + !radix_tree_tag_get(&hwspinlock_tree, id,
> HWSPINLOCK_UNUSED)) {
> pr_warn("hwspinlock %u is already in use\n", id);
> hwlock = NULL;
> goto out;
> @@ -777,7 +818,7 @@ int hwspin_lock_free(struct hwspinlock *hwlock)
> {
> struct device *dev;
> struct hwspinlock *tmp;
> - int ret;
> + int ret, id;
>
> if (!hwlock) {
> pr_err("invalid hwlock\n");
> @@ -788,30 +829,35 @@ int hwspin_lock_free(struct hwspinlock *hwlock)
> mutex_lock(&hwspinlock_tree_lock);
>
> /* make sure the hwspinlock is used */
> - ret = radix_tree_tag_get(&hwspinlock_tree, hwlock_to_id(hwlock),
> -
> HWSPINLOCK_UNUSED);
> + id = hwlock_to_id(hwlock);
> + ret = radix_tree_tag_get(&hwspinlock_tree, id,
> HWSPINLOCK_UNUSED);
> if (ret == 1) {
> dev_err(dev, "%s: hwlock is already free\n", __func__);
> dump_stack();
> ret = -EINVAL;
> - goto out;
> + goto unlock;
> }
>
> /* notify the underlying device that power is not needed */
> ret = pm_runtime_put(dev);
> if (ret < 0)
> - goto out;
> + goto unlock;
> +
> + /* update shareable refcount */
> + if (radix_tree_tag_get(&hwspinlock_tree, id, HWSPINLOCK_SHARED)
> &&
> + --hwlock->refcount)
> + goto put;
>
> /* mark this hwspinlock as available */
> - tmp = radix_tree_tag_set(&hwspinlock_tree, hwlock_to_id(hwlock),
> -
> HWSPINLOCK_UNUSED);
> + tmp = radix_tree_tag_set(&hwspinlock_tree, id,
> HWSPINLOCK_UNUSED);
>
> /* sanity check (this shouldn't happen) */
> WARN_ON(tmp != hwlock);
>
> +put:
> module_put(dev->driver->owner);
>
> -out:
> +unlock:
> mutex_unlock(&hwspinlock_tree_lock);
> return ret;
> }
> diff --git a/drivers/hwspinlock/hwspinlock_internal.h
> b/drivers/hwspinlock/hwspinlock_internal.h
> index 9eb6bd0..c808e11 100644
> --- a/drivers/hwspinlock/hwspinlock_internal.h
> +++ b/drivers/hwspinlock/hwspinlock_internal.h
> @@ -35,11 +35,13 @@ struct hwspinlock_ops {
> * struct hwspinlock - this struct represents a single hwspinlock instance
> * @bank: the hwspinlock_device structure which owns this lock
> * @lock: initialized and used by hwspinlock core
> + * @refcount: number of users (when shared)
> * @priv: private data, owned by the underlying platform-specific hwspinlock
> drv
> */
> struct hwspinlock {
> struct hwspinlock_device *bank;
> spinlock_t lock;
> + unsigned int refcount;
> void *priv;
> };
>
> --
> 2.7.4
Powered by blists - more mailing lists