| 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: <88f890ef-b16e-a558-3c2f-cfeba107b578@synopsys.com>
Date: Mon, 9 Jan 2017 17:19:47 +0000
From: Ramiro Oliveira <Ramiro.Oliveira@...opsys.com>
To: Philipp Zabel <p.zabel@...gutronix.de>,
Ramiro Oliveira <Ramiro.Oliveira@...opsys.com>
CC: <linux-kernel@...r.kernel.org>,
<laurent.pinchart@...asonboard.com>, <CARLOS.PALMINHA@...opsys.com>
Subject: Re: [PATCH v2 2/2] reset: make optional functions really optional
Hi Philipp
On 1/9/2017 10:45 AM, Philipp Zabel wrote:
> Hi Ramiro,
>
> Am Dienstag, den 27.12.2016, 12:37 +0000 schrieb Ramiro Oliveira:
>> The optional functions weren't really optional so this patch makes them
>> really optional.
>
> Please add a bit of detail to the description. Since this changes the
> API, you should mention that the reset_control_get_optional variants now
> return NULL instead of an error if there is no matching reset phandle in
> the device tree and that the reset_control_* functions accept NULL rstc
> pointers.
>
Would you be ok with something like this:
"The *_get_optional_* functions weren't really optional so this patch makes them
really optional.
These *_get_optional_* functions will now return NULL instead of an error if no
matching reset phandle is found in the DT, and all the reset_control_* functions
now accept NULL rstc pointers
>> Signed-off-by: Ramiro Oliveira <Ramiro.Oliveira@...opsys.com>
>> ---
>> drivers/reset/core.c | 35 +++++++++++++++++++++++++++++------
>> include/linux/reset.h | 45 ++++++++++++++++++++++++++-------------------
>> 2 files changed, 55 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/reset/core.c b/drivers/reset/core.c
>> index 70023997d031..f933e9dfebc5 100644
>> --- a/drivers/reset/core.c
>> +++ b/drivers/reset/core.c
>> @@ -135,9 +135,15 @@ EXPORT_SYMBOL_GPL(devm_reset_controller_register);
>> * @rstc: reset controller
>> *
>> * Calling this on a shared reset controller is an error.
>> + *
>> + * If rstc is NULL it is an optional reset and the function will just
>> + * return 0.
>> */
>> int reset_control_reset(struct reset_control *rstc)
>> {
>> + if (!rstc)
>> + return 0;
>> +
>> if (WARN_ON(rstc->shared))
>> return -EINVAL;
>>
>> @@ -158,9 +164,15 @@ EXPORT_SYMBOL_GPL(reset_control_reset);
>> *
>> * For shared reset controls a driver cannot expect the hw's registers and
>> * internal state to be reset, but must be prepared for this to happen.
>> + *
>> + * If rstc is NULL it is an optional reset and the function will just
>> + * return 0.
>> */
>> int reset_control_assert(struct reset_control *rstc)
>> {
>> + if (!rstc)
>> + return 0;
>> +
>> if (!rstc->rcdev->ops->assert)
>> return -ENOTSUPP;
>>
>> @@ -181,9 +193,15 @@ EXPORT_SYMBOL_GPL(reset_control_assert);
>> * @rstc: reset controller
>> *
>> * After calling this function, the reset is guaranteed to be deasserted.
>> + *
>> + * If rstc is NULL it is an optional reset and the function will just
>> + * return 0.
>> */
>> int reset_control_deassert(struct reset_control *rstc)
>> {
>> + if (!rstc)
>> + return 0;
>> +
>> if (!rstc->rcdev->ops->deassert)
>> return -ENOTSUPP;
>>
>> @@ -199,11 +217,14 @@ EXPORT_SYMBOL_GPL(reset_control_deassert);
>> /**
>> * reset_control_status - returns a negative errno if not supported, a
>> * positive value if the reset line is asserted, or zero if the reset
>> - * line is not asserted.
>> + * line is not asserted or if the desc is NULL (optional reset).
>> * @rstc: reset controller
>> */
>> int reset_control_status(struct reset_control *rstc)
>> {
>> + if (!rstc)
>> + return 0;
>> +
>> if (rstc->rcdev->ops->status)
>> return rstc->rcdev->ops->status(rstc->rcdev, rstc->id);
>>
>> @@ -258,7 +279,8 @@ static void __reset_control_put(struct reset_control *rstc)
>> }
>>
>> struct reset_control *__of_reset_control_get(struct device_node *node,
>> - const char *id, int index, bool shared)
>> + const char *id, int index, bool shared,
>> + bool optional)
>> {
>> struct reset_control *rstc;
>> struct reset_controller_dev *r, *rcdev;
>> @@ -273,13 +295,13 @@ struct reset_control *__of_reset_control_get(struct device_node *node,
>> index = of_property_match_string(node,
>> "reset-names", id);
>> if (index < 0)
>> - return ERR_PTR(-ENOENT);
>> + return optional ? NULL : ERR_PTR(-ENOENT);
>
> of_property_match_string can return -EINVAL, -ENODATA, or -EILSEQ.
> I think -EILSEQ should still be returned.
>
I'll make the function return NULL, -ENOENT, or -EILSEQ.
>> }
>>
>> ret = of_parse_phandle_with_args(node, "resets", "#reset-cells",
>> index, &args);
>> if (ret)
>> - return ERR_PTR(ret);
>> + return optional ? NULL : ERR_PTR(ret);
>
> of_parse_phandle_with_args can return -ENOENT or -EINVAL.
> I think -EINVAL should still be returned.
>
Same as above. I'll make the function return NULL, -ENOENT, or -EINVAL.
>>
>> mutex_lock(&reset_list_mutex);
>> rcdev = NULL;
>> @@ -338,7 +360,8 @@ static void devm_reset_control_release(struct device *dev, void *res)
>> }
>>
>> struct reset_control *__devm_reset_control_get(struct device *dev,
>> - const char *id, int index, bool shared)
>> + const char *id, int index, bool shared,
>> + bool optional)
>> {
>> struct reset_control **ptr, *rstc;
>>
>> @@ -348,7 +371,7 @@ struct reset_control *__devm_reset_control_get(struct device *dev,
>> return ERR_PTR(-ENOMEM);
>>
>> rstc = __of_reset_control_get(dev ? dev->of_node : NULL,
>> - id, index, shared);
>> + id, index, shared, optional);
>> if (!IS_ERR(rstc)) {
>> *ptr = rstc;
>> devres_add(dev, ptr);
>> diff --git a/include/linux/reset.h b/include/linux/reset.h
>> index ec1d1fd28f5f..e4cc7146b5ed 100644
>> --- a/include/linux/reset.h
>> +++ b/include/linux/reset.h
>> @@ -13,10 +13,12 @@ int reset_control_deassert(struct reset_control *rstc);
>> int reset_control_status(struct reset_control *rstc);
>>
>> struct reset_control *__of_reset_control_get(struct device_node *node,
>> - const char *id, int index, bool shared);
>> + const char *id, int index, bool shared,
>> + bool optional);
>> void reset_control_put(struct reset_control *rstc);
>> struct reset_control *__devm_reset_control_get(struct device *dev,
>> - const char *id, int index, bool shared);
>> + const char *id, int index, bool shared,
>> + bool optional);
>>
>> int __must_check device_reset(struct device *dev);
>>
>> @@ -69,14 +71,15 @@ static inline int device_reset_optional(struct device *dev)
>>
>> static inline struct reset_control *__of_reset_control_get(
>> struct device_node *node,
>> - const char *id, int index, bool shared)
>> + const char *id, int index, bool shared,
>> + bool optional)
>> {
>> return ERR_PTR(-ENOTSUPP);
>> }
>>
>> static inline struct reset_control *__devm_reset_control_get(
>> - struct device *dev,
>> - const char *id, int index, bool shared)
>> + struct device *dev, const char *id,
>> + int index, bool shared, bool optional)
>> {
>> return ERR_PTR(-ENOTSUPP);
>> }
>> @@ -104,7 +107,8 @@ __must_check reset_control_get_exclusive(struct device *dev, const char *id)
>> #ifndef CONFIG_RESET_CONTROLLER
>> WARN_ON(1);
>> #endif
>> - return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, 0);
>> + return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, false,
>> + false);
>> }
>>
>> /**
>> @@ -132,19 +136,22 @@ __must_check reset_control_get_exclusive(struct device *dev, const char *id)
>> static inline struct reset_control *reset_control_get_shared(
>> struct device *dev, const char *id)
>> {
>> - return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, true);
>> + return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, true,
>> + false);
>> }
>>
>> static inline struct reset_control *reset_control_get_optional_exclusive(
>> struct device *dev, const char *id)
>> {
>> - return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, false);
>> + return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, false,
>> + true);
>> }
>>
>> static inline struct reset_control *reset_control_get_optional_shared(
>> struct device *dev, const char *id)
>> {
>> - return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, true);
>> + return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, true,
>> + true);
>> }
>>
>> /**
>> @@ -160,7 +167,7 @@ static inline struct reset_control *reset_control_get_optional_shared(
>> static inline struct reset_control *of_reset_control_get_exclusive(
>> struct device_node *node, const char *id)
>> {
>> - return __of_reset_control_get(node, id, 0, 0);
>> + return __of_reset_control_get(node, id, 0, false, false);
>> }
>>
>> /**
>> @@ -185,7 +192,7 @@ static inline struct reset_control *of_reset_control_get_exclusive(
>> static inline struct reset_control *of_reset_control_get_shared(
>> struct device_node *node, const char *id)
>> {
>> - return __of_reset_control_get(node, id, 0, true);
>> + return __of_reset_control_get(node, id, 0, true, false);
>> }
>>
>> /**
>> @@ -202,7 +209,7 @@ static inline struct reset_control *of_reset_control_get_shared(
>> static inline struct reset_control *of_reset_control_get_exclusive_by_index(
>> struct device_node *node, int index)
>> {
>> - return __of_reset_control_get(node, NULL, index, false);
>> + return __of_reset_control_get(node, NULL, index, false, false);
>> }
>>
>> /**
>> @@ -230,7 +237,7 @@ static inline struct reset_control *of_reset_control_get_exclusive_by_index(
>> static inline struct reset_control *of_reset_control_get_shared_by_index(
>> struct device_node *node, int index)
>> {
>> - return __of_reset_control_get(node, NULL, index, true);
>> + return __of_reset_control_get(node, NULL, index, true, false);
>> }
>>
>> /**
>> @@ -252,7 +259,7 @@ __must_check devm_reset_control_get_exclusive(struct device *dev,
>> #ifndef CONFIG_RESET_CONTROLLER
>> WARN_ON(1);
>> #endif
>> - return __devm_reset_control_get(dev, id, 0, false);
>> + return __devm_reset_control_get(dev, id, 0, false, false);
>> }
>>
>> /**
>> @@ -267,19 +274,19 @@ __must_check devm_reset_control_get_exclusive(struct device *dev,
>> static inline struct reset_control *devm_reset_control_get_shared(
>> struct device *dev, const char *id)
>> {
>> - return __devm_reset_control_get(dev, id, 0, true);
>> + return __devm_reset_control_get(dev, id, 0, true, false);
>> }
>>
>> static inline struct reset_control *devm_reset_control_get_optional_exclusive(
>> struct device *dev, const char *id)
>> {
>> - return __devm_reset_control_get(dev, id, 0, false);
>> + return __devm_reset_control_get(dev, id, 0, false, true);
>> }
>>
>> static inline struct reset_control *devm_reset_control_get_optional_shared(
>> struct device *dev, const char *id)
>> {
>> - return __devm_reset_control_get(dev, id, 0, true);
>> + return __devm_reset_control_get(dev, id, 0, true, true);
>> }
>>
>> /**
>> @@ -297,7 +304,7 @@ static inline struct reset_control *devm_reset_control_get_optional_shared(
>> static inline struct reset_control *
>> devm_reset_control_get_exclusive_by_index(struct device *dev, int index)
>> {
>> - return __devm_reset_control_get(dev, NULL, index, false);
>> + return __devm_reset_control_get(dev, NULL, index, false, false);
>> }
>>
>> /**
>> @@ -313,7 +320,7 @@ devm_reset_control_get_exclusive_by_index(struct device *dev, int index)
>> static inline struct reset_control *
>> devm_reset_control_get_shared_by_index(struct device *dev, int index)
>> {
>> - return __devm_reset_control_get(dev, NULL, index, true);
>> + return __devm_reset_control_get(dev, NULL, index, true, false);
>> }
>>
>> /*
>
> regards
> Philipp
>
BRs,
Ramiro
Powered by blists - more mailing lists