From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753793AbdDCQFE (ORCPT ); Mon, 3 Apr 2017 12:05:04 -0400 Received: from metis.ext.4.pengutronix.de ([92.198.50.35]:45615 "EHLO metis.ext.4.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752760AbdDCQFD (ORCPT ); Mon, 3 Apr 2017 12:05:03 -0400 Message-ID: <1491235498.2378.106.camel@pengutronix.de> Subject: Re: [PATCH v1] reset: Make optional stuff optional for all users From: Philipp Zabel To: Andy Shevchenko Cc: "mika.westerberg@linux.intel.com" , "linux-kernel@vger.kernel.org" , "Ramiro.Oliveira@synopsys.com" Date: Mon, 03 Apr 2017 18:04:58 +0200 In-Reply-To: <1491233004.708.112.camel@linux.intel.com> References: <20170403122638.88263-1-andriy.shevchenko@linux.intel.com> <1491229671.2378.73.camel@pengutronix.de> <1491229885.708.106.camel@linux.intel.com> <1491230017.708.108.camel@intel.com> <1491232187.2378.90.camel@pengutronix.de> <1491233004.708.112.camel@linux.intel.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.12.9-1+b1 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-SA-Exim-Connect-IP: 2001:67c:670:100:3ad5:47ff:feaf:1a17 X-SA-Exim-Mail-From: p.zabel@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-kernel@vger.kernel.org Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 2017-04-03 at 18:23 +0300, Andy Shevchenko wrote: > On Mon, 2017-04-03 at 17:09 +0200, Philipp Zabel wrote: > > On Mon, 2017-04-03 at 14:33 +0000, Shevchenko, Andriy wrote: > > > On Mon, 2017-04-03 at 17:31 +0300, Andy Shevchenko wrote: > > > > On Mon, 2017-04-03 at 16:27 +0200, Philipp Zabel wrote: > > > > > > > > > > > int rstc_id; > > > > > > int ret; > > > > > > > > > > > > - if (!node) > > > > > > - return ERR_PTR(-EINVAL); > > > > > > - > > > > > > > > > > This should be > > > > > > > > > > if (!node) > > > > > return optional ? NULL : ERR_PTR(-EINVAL); > > > > > > > > > > instead. Can you confirm this works for Intel boards with DW > > > > > UART? I > > > > > can > > > > > fix it up when applying if you agree. > > > > > > > > I don't think it worth to change. I specifically checked all of_* > > > > calls > > > > in that function and they cope pretty nice with node == NULL. > > > > __of_reset_control_get called with id != NULL calls > > of_property_match_string first, which then returns -EINVAL if > > node == NULL, which makes __of_reset_control_get return NULL if > > optional > > or -ENOENT otherwise, even though the correct return value would be > > -EINVAL in the DT case. > > Error handling mess as usual. :-) > > > > > __of_reset_control_get called with id == NULL calls > > of_parse_phandle_with_args first, which calls > > __of_parse_phandle_with_args, which returns an undefined value if > > np == NULL, as far as I can tell: > > of_for_each_phandle first calls of_phandle_iterator_init, which, when > > called with np == NULL clears the iterator structure returns -ENOENT. > > The return value is ignored in the of_for_each_phandle macro, and > > of_phandle_iterator_next is then called and returns -ENOENT because > > it->cur == NULL, ending the loop without ever assigning a value to rc. > > __of_parse_phandle_with_args then returns the uninitialized value. > > It returns -ENOENT. Error value is kept in function variable rc. Thanks for the correction. That's still the wrong error code IMHO, but it certainly makes your patch unproblematic. > > The point being, instead of having to regularly forage through a > > number > > of of_ API functions to make sure my expectations are still met, I'd > > prefer to keep the check in place. > > I would not insist, I already shared my view on this. Appreciated. With a correct understanding of the current of_* error return paths it makes a lot more sense. > I really don't like ping-ponging of the code. Perhaps you would fix it > once for the best now? How about: ----------8<---------- >>From 75205aeaf52cef5c62710630cceb871468175c12 Mon Sep 17 00:00:00 2001 From: Philipp Zabel Date: Fri, 5 Feb 2016 13:41:39 +0100 Subject: [PATCH] reset: add exported __reset_control_get, return NULL if optional Rename the internal __reset_control_get/put functions to __reset_control_get/put_internal and add an exported __reset_control_get equivalent to __of_reset_control_get that takes a struct device parameter. This avoids the confusing call to __of_reset_control_get in the non-DT case and fixes the devm_reset_control_get_optional function to return NULL if RESET_CONTROLLER is enabled but dev->of_node == NULL. Fixes: bb475230b8e5 ("reset: make optional functions really optional") Reported-by: Andy Shevchenko Cc: Andy Shevchenko Cc: Ramiro Oliveira Signed-off-by: Philipp Zabel --- drivers/reset/core.c | 22 ++++++++++++++++------ include/linux/reset.h | 22 ++++++++++++++-------- 2 files changed, 30 insertions(+), 14 deletions(-) diff --git a/drivers/reset/core.c b/drivers/reset/core.c index f1e5e65388bb5..d227168c8971a 100644 --- a/drivers/reset/core.c +++ b/drivers/reset/core.c @@ -275,7 +275,7 @@ int reset_control_status(struct reset_control *rstc) } EXPORT_SYMBOL_GPL(reset_control_status); -static struct reset_control *__reset_control_get( +static struct reset_control *__reset_control_get_internal( struct reset_controller_dev *rcdev, unsigned int index, bool shared) { @@ -308,7 +308,7 @@ static struct reset_control *__reset_control_get( return rstc; } -static void __reset_control_put(struct reset_control *rstc) +static void __reset_control_put_internal(struct reset_control *rstc) { lockdep_assert_held(&reset_list_mutex); @@ -377,7 +377,7 @@ struct reset_control *__of_reset_control_get(struct device_node *node, } /* reset_list_mutex also protects the rcdev's reset_control list */ - rstc = __reset_control_get(rcdev, rstc_id, shared); + rstc = __reset_control_get_internal(rcdev, rstc_id, shared); mutex_unlock(&reset_list_mutex); @@ -385,6 +385,17 @@ struct reset_control *__of_reset_control_get(struct device_node *node, } EXPORT_SYMBOL_GPL(__of_reset_control_get); +struct reset_control *__reset_control_get(struct device *dev, const char *id, + int index, bool shared, bool optional) +{ + if (dev->of_node) + return __of_reset_control_get(dev->of_node, id, index, shared, + optional); + else + return optional ? NULL : ERR_PTR(-EINVAL); +} +EXPORT_SYMBOL_GPL(__reset_control_get); + /** * reset_control_put - free the reset controller * @rstc: reset controller @@ -396,7 +407,7 @@ void reset_control_put(struct reset_control *rstc) return; mutex_lock(&reset_list_mutex); - __reset_control_put(rstc); + __reset_control_put_internal(rstc); mutex_unlock(&reset_list_mutex); } EXPORT_SYMBOL_GPL(reset_control_put); @@ -417,8 +428,7 @@ struct reset_control *__devm_reset_control_get(struct device *dev, if (!ptr) return ERR_PTR(-ENOMEM); - rstc = __of_reset_control_get(dev ? dev->of_node : NULL, - id, index, shared, optional); + rstc = __reset_control_get(dev, 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 96fb139bdd08f..13d8681210d54 100644 --- a/include/linux/reset.h +++ b/include/linux/reset.h @@ -15,6 +15,9 @@ 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, bool optional); +struct reset_control *__reset_control_get(struct device *dev, 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, @@ -72,6 +75,13 @@ static inline struct reset_control *__of_reset_control_get( return optional ? NULL : ERR_PTR(-ENOTSUPP); } +static inline struct reset_control *__reset_control_get( + struct device *dev, const char *id, + int index, bool shared, bool optional) +{ + return optional ? NULL : ERR_PTR(-ENOTSUPP); +} + static inline struct reset_control *__devm_reset_control_get( struct device *dev, const char *id, int index, bool shared, bool optional) @@ -102,8 +112,7 @@ __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, false, - false); + return __reset_control_get(dev, id, 0, false, false); } /** @@ -131,22 +140,19 @@ __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, - false); + return __reset_control_get(dev, 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, - true); + return __reset_control_get(dev, 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, - true); + return __reset_control_get(dev, id, 0, true, true); } /** -- 2.11.0 ---------->8---------- That should make both the __reset_control_get stub and implementation return NULL if optional && (dev->of_node == NULL), and stop calling __of_reset_control_get in the non-DT case, while maintaining the -EINVAL error return value if of_reset_control_get* is called with node == NULL. regards Philipp