| 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: <ba95a3fec9c726e2cd61665ac5d7f89d16232d60.camel@pengutronix.de>
Date: Wed, 23 Mar 2022 16:29:41 +0100
From: Philipp Zabel <p.zabel@...gutronix.de>
To: Clément Léger <clement.leger@...tlin.com>,
Rob Herring <robh+dt@...nel.org>,
Frank Rowand <frowand.list@...il.com>
Cc: Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
Alexandre Belloni <alexandre.belloni@...tlin.com>,
Allan Nielsen <allan.nielsen@...rochip.com>,
linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH 2/2] reset: add support for fwnode
On Mi, 2022-03-23 at 10:50 +0100, Clément Léger wrote:
[...]
> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> index 61e688882643..f014da03b7c1 100644
> --- a/drivers/reset/core.c
> +++ b/drivers/reset/core.c
> @@ -4,6 +4,7 @@
> *
> * Copyright 2013 Philipp Zabel, Pengutronix
> */
> +#include <linux/acpi.h>
> #include <linux/atomic.h>
> #include <linux/device.h>
> #include <linux/err.h>
> @@ -70,26 +71,49 @@ static const char *rcdev_name(struct
> reset_controller_dev *rcdev)
> if (rcdev->of_node)
> return rcdev->of_node->full_name;
Could the above be removed, since reset_controller_register() set
rcdev->fwnode to of_fwnode_handle(rcdev->of_node) earlier?
>
> + if (rcdev->fwnode)
> + return fwnode_get_name(rcdev->fwnode);
> +
> return NULL;
> }
>
[...]
>
> /**
> @@ -98,9 +122,21 @@ static int of_reset_simple_xlate(struct reset_controller_dev *rcdev,
> */
> int reset_controller_register(struct reset_controller_dev *rcdev)
> {
> - if (!rcdev->of_xlate) {
> - rcdev->of_reset_n_cells = 1;
> - rcdev->of_xlate = of_reset_simple_xlate;
> + if (!rcdev->fwnode) {
> + rcdev->fwnode = of_fwnode_handle(rcdev->of_node);
> + } else {
> + if (is_acpi_node(rcdev->fwnode))
> + return -EINVAL;
> + }
> +
> + if (rcdev->of_xlate) {
> + rcdev->fwnode_xlate = fwnode_of_reset_xlate;
It should be documented that .fwnode_xlate/.fwnode_reset_n_cells are
ignored if .of_xlate is set.
> + rcdev->fwnode_reset_n_cells = rcdev->of_reset_n_cells;
> + }
> +
> + if (!rcdev->fwnode_xlate) {
> + rcdev->fwnode_reset_n_cells = 1;
> + rcdev->fwnode_xlate = fwnode_reset_simple_xlate;
> }
>
> INIT_LIST_HEAD(&rcdev->reset_control_head);
> @@ -810,29 +846,28 @@ static void __reset_control_put_internal(struct
> reset_control *rstc)
> }
>
> struct reset_control *
> -__of_reset_control_get(struct device_node *node, const char *id, int index,
> - bool shared, bool optional, bool acquired)
> +__fwnode_reset_control_get(struct fwnode_handle *fwnode, const char *id,
> + int index, bool shared, bool optional, bool acquired)
> {
> struct reset_control *rstc;
> struct reset_controller_dev *r, *rcdev;
> - struct of_phandle_args args;
> + struct fwnode_reference_args args;
> int rstc_id;
> int ret;
>
> - if (!node)
> + if (!fwnode || is_acpi_node(fwnode))
> return ERR_PTR(-EINVAL);
>
> if (id) {
> - index = of_property_match_string(node,
> - "reset-names", id);
> + index = fwnode_property_match_string(fwnode, "reset-names", id);
> if (index == -EILSEQ)
> return ERR_PTR(index);
I don't think this is good enough any more. At least -ENOMEM is added
as a possible error return code by this change.
[...]
> @@ -945,6 +989,9 @@ struct reset_control *__reset_control_get(struct device *dev, const char *id,
> if (dev->of_node)
> return __of_reset_control_get(dev->of_node, id, index, shared,
> optional, acquired);
Could the above be removed, given that __of_reset_control_get() just
wraps __fwnode_reset_control_get(), which is called right below:
> + if (dev_fwnode(dev))
> + return __fwnode_reset_control_get(dev_fwnode(dev), id, index,
> + shared, optional, acquired);
>
> return __reset_control_get_from_lookup(dev, id, shared, optional,
> acquired);
> diff --git a/include/linux/reset-controller.h b/include/linux/reset-
> controller.h
> index 0fa4f60e1186..292552003d11 100644
> --- a/include/linux/reset-controller.h
> +++ b/include/linux/reset-controller.h
> @@ -24,7 +24,9 @@ struct reset_control_ops {
>
> struct module;
> struct device_node;
> +struct fwnode_handle;
> struct of_phandle_args;
> +struct fwnode_reference_args;
>
> /**
> * struct reset_control_lookup - represents a single lookup entry
> @@ -60,10 +62,16 @@ struct reset_control_lookup {
> * @reset_control_head: head of internal list of requested reset controls
> * @dev: corresponding driver model device struct
> * @of_node: corresponding device tree node as phandle target
> + * @fwnode: corresponding firmware node as reference target
> * @of_reset_n_cells: number of cells in reset line specifiers
> * @of_xlate: translation function to translate from specifier as found in the
> * device tree to id as given to the reset control ops, defaults
> - * to :c:func:`of_reset_simple_xlate`.
> + * to :c:func:`fwnode_of_reset_xlate`.
> + * @fwnode_reset_n_cells: number of cells in reset line reference specifiers
> + * @fwnode_xlate: translation function to translate from reference specifier as
> + * found in the firmware node description to id as given to the
> + * reset control ops, defaults to
> + * :c:func:`fwnode_reset_simple_xlate`.
This should mention that .fwnode_xlate is ignored/overwritten when
.of_xlate is set.
regards
Philipp
Powered by blists - more mailing lists