diff mbox

drivers: Remove unused devm_*_put functions

Message ID 20131221104920.GA3626@rashika
State Rejected
Headers show

Commit Message

Rashika Kheria Dec. 21, 2013, 10:49 a.m. UTC 
Remove unused devm_*_put functions because none of them are
used anywhere in the kernel and devm does automatic cleanup
of resources and requires no manual cleanup.

Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
Reviewed-by: Josh Triplett <josh@joshtriplett.org>
---
 drivers/gpio/devres.c         |   25 -------------------------
 drivers/phy/phy-core.c        |   17 -----------------
 drivers/pwm/core.c            |   15 ---------------
 include/linux/gpio/consumer.h |    9 ---------
 include/linux/phy/phy.h       |    5 -----
 include/linux/pwm.h           |    5 -----
 6 files changed, 76 deletions(-)

Comments

Linus Walleij Jan. 2, 2014, 1:07 p.m. UTC | #1 
On Sat, Dec 21, 2013 at 11:49 AM, Rashika Kheria
<rashika.kheria@gmail.com> wrote:

> Remove unused devm_*_put functions because none of them are
> used anywhere in the kernel and devm does automatic cleanup
> of resources and requires no manual cleanup.
>
> Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
> Reviewed-by: Josh Triplett <josh@joshtriplett.org>

>  drivers/gpio/devres.c         |   25 -------------------------
>  include/linux/gpio/consumer.h |    9 ---------

Can you please split these two hunks on the patch into a separate
patch I can apply to the GPIO tree?


> -static int devm_gpiod_match(struct device *dev, void *res, void *data)
> -{
> -       struct gpio_desc **this = res, **gpio = data;
> -
> -       return *this == *gpio;
> -}
(...)
> -/**
> - * devm_gpiod_put - Resource-managed gpiod_put()
> - * @desc:      GPIO descriptor to dispose of
> - *
> - * Dispose of a GPIO descriptor obtained with devm_gpiod_get() or
> - * devm_gpiod_get_index(). Normally this function will not be called as the GPIO
> - * will be disposed of by the resource management code.
> - */
> -void devm_gpiod_put(struct device *dev, struct gpio_desc *desc)
> -{
> -       WARN_ON(devres_release(dev, devm_gpiod_release, devm_gpiod_match,
> -               &desc));
> -}
> -EXPORT_SYMBOL(devm_gpiod_put);

Alexandre what do you think about this? Can we think of a scenario
where explicit garbage collection is going to be needed or should we
remove this for now?

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexandre Courbot Jan. 3, 2014, 11:07 a.m. UTC | #2 
On Thu, Jan 2, 2014 at 10:07 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Sat, Dec 21, 2013 at 11:49 AM, Rashika Kheria
> <rashika.kheria@gmail.com> wrote:
>> -/**
>> - * devm_gpiod_put - Resource-managed gpiod_put()
>> - * @desc:      GPIO descriptor to dispose of
>> - *
>> - * Dispose of a GPIO descriptor obtained with devm_gpiod_get() or
>> - * devm_gpiod_get_index(). Normally this function will not be called as the GPIO
>> - * will be disposed of by the resource management code.
>> - */
>> -void devm_gpiod_put(struct device *dev, struct gpio_desc *desc)
>> -{
>> -       WARN_ON(devres_release(dev, devm_gpiod_release, devm_gpiod_match,
>> -               &desc));
>> -}
>> -EXPORT_SYMBOL(devm_gpiod_put);
>
> Alexandre what do you think about this? Can we think of a scenario
> where explicit garbage collection is going to be needed or should we
> remove this for now?

Sorry for the delayed reply, I quickly saw the patch and then the
holidays got in the way. :)

I think all these functions should be kept. It is true that they are
seldomly used, and that the purpose of devm is to garbage-collect
resources upon driver removal, but they might (actually, probably
will) become needed by someone at some point in the future. One
example I can think of is two drivers that collaborate to share the
same GPIO line. If they acquire the GPIO through devm_gpiod_get() they
will need devm_gpiod_put() to release it so the other driver can
acquire it.

On a more general note, devm_clk_put(), devm_regulator_put(),
devm_pinctrl_put() and probably others devm_*_put() functions are
actively used in the kernel, to  support the idea that a devm removal
function makes sense. That not all the subsystem-provided functions
are used by mainline drivers does not necessary mean they should be
removed, especially if they serve a purpose. We should keep our APIs
consistent and future-proof, not to mention out-of-tree drivers that
may use them.

So this patch is a nack as far as I'm concerned, not only the GPIO
part, but the whole of it.
--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Josh Triplett Jan. 3, 2014, 8:22 p.m. UTC | #3 
On Fri, Jan 03, 2014 at 08:07:22PM +0900, Alexandre Courbot wrote:
> On Thu, Jan 2, 2014 at 10:07 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Sat, Dec 21, 2013 at 11:49 AM, Rashika Kheria
> > <rashika.kheria@gmail.com> wrote:
> >> -/**
> >> - * devm_gpiod_put - Resource-managed gpiod_put()
> >> - * @desc:      GPIO descriptor to dispose of
> >> - *
> >> - * Dispose of a GPIO descriptor obtained with devm_gpiod_get() or
> >> - * devm_gpiod_get_index(). Normally this function will not be called as the GPIO
> >> - * will be disposed of by the resource management code.
> >> - */
> >> -void devm_gpiod_put(struct device *dev, struct gpio_desc *desc)
> >> -{
> >> -       WARN_ON(devres_release(dev, devm_gpiod_release, devm_gpiod_match,
> >> -               &desc));
> >> -}
> >> -EXPORT_SYMBOL(devm_gpiod_put);
> >
> > Alexandre what do you think about this? Can we think of a scenario
> > where explicit garbage collection is going to be needed or should we
> > remove this for now?
> 
> Sorry for the delayed reply, I quickly saw the patch and then the
> holidays got in the way. :)
> 
> I think all these functions should be kept. It is true that they are
> seldomly used, and that the purpose of devm is to garbage-collect
> resources upon driver removal, but they might (actually, probably
> will) become needed by someone at some point in the future. One
> example I can think of is two drivers that collaborate to share the
> same GPIO line. If they acquire the GPIO through devm_gpiod_get() they
> will need devm_gpiod_put() to release it so the other driver can
> acquire it.
> 
> On a more general note, devm_clk_put(), devm_regulator_put(),
> devm_pinctrl_put() and probably others devm_*_put() functions are
> actively used in the kernel, to  support the idea that a devm removal
> function makes sense. That not all the subsystem-provided functions
> are used by mainline drivers does not necessary mean they should be
> removed, especially if they serve a purpose. We should keep our APIs
> consistent and future-proof, not to mention out-of-tree drivers that
> may use them.
> 
> So this patch is a nack as far as I'm concerned, not only the GPIO
> part, but the whole of it.

As far as I can tell, the few calls to the devm_*_put functions I can
see look like unnecessary calls as part of driver/device
uninitialization, and could be removed either directly or with minor
reworking.  That there are only 1-2 calls to each in the entire kernel
is also quite telling.

In any case, it's disappointing to have a pile of unused functions in
the kernel on the theory that they *might* be needed; it's not like it'd
be hard to retrieve them from git if they're ever needed.

However, if you're insistent on keeping them, it'd be easy enough to
provide patches to include an appropriate header with prototypes for
them instead, which would also eliminate the warning this patch series
set out to eliminate.

- Josh Triplett
--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexandre Courbot Jan. 6, 2014, 7:48 a.m. UTC | #4 
On Sat, Jan 4, 2014 at 5:22 AM, Josh Triplett <josh@joshtriplett.org> wrote:
> On Fri, Jan 03, 2014 at 08:07:22PM +0900, Alexandre Courbot wrote:
>> On Thu, Jan 2, 2014 at 10:07 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> > On Sat, Dec 21, 2013 at 11:49 AM, Rashika Kheria
>> > <rashika.kheria@gmail.com> wrote:
>> >> -/**
>> >> - * devm_gpiod_put - Resource-managed gpiod_put()
>> >> - * @desc:      GPIO descriptor to dispose of
>> >> - *
>> >> - * Dispose of a GPIO descriptor obtained with devm_gpiod_get() or
>> >> - * devm_gpiod_get_index(). Normally this function will not be called as the GPIO
>> >> - * will be disposed of by the resource management code.
>> >> - */
>> >> -void devm_gpiod_put(struct device *dev, struct gpio_desc *desc)
>> >> -{
>> >> -       WARN_ON(devres_release(dev, devm_gpiod_release, devm_gpiod_match,
>> >> -               &desc));
>> >> -}
>> >> -EXPORT_SYMBOL(devm_gpiod_put);
>> >
>> > Alexandre what do you think about this? Can we think of a scenario
>> > where explicit garbage collection is going to be needed or should we
>> > remove this for now?
>>
>> Sorry for the delayed reply, I quickly saw the patch and then the
>> holidays got in the way. :)
>>
>> I think all these functions should be kept. It is true that they are
>> seldomly used, and that the purpose of devm is to garbage-collect
>> resources upon driver removal, but they might (actually, probably
>> will) become needed by someone at some point in the future. One
>> example I can think of is two drivers that collaborate to share the
>> same GPIO line. If they acquire the GPIO through devm_gpiod_get() they
>> will need devm_gpiod_put() to release it so the other driver can
>> acquire it.
>>
>> On a more general note, devm_clk_put(), devm_regulator_put(),
>> devm_pinctrl_put() and probably others devm_*_put() functions are
>> actively used in the kernel, to  support the idea that a devm removal
>> function makes sense. That not all the subsystem-provided functions
>> are used by mainline drivers does not necessary mean they should be
>> removed, especially if they serve a purpose. We should keep our APIs
>> consistent and future-proof, not to mention out-of-tree drivers that
>> may use them.
>>
>> So this patch is a nack as far as I'm concerned, not only the GPIO
>> part, but the whole of it.
>
> As far as I can tell, the few calls to the devm_*_put functions I can
> see look like unnecessary calls as part of driver/device
> uninitialization, and could be removed either directly or with minor
> reworking.  That there are only 1-2 calls to each in the entire kernel
> is also quite telling.
>
> In any case, it's disappointing to have a pile of unused functions in
> the kernel on the theory that they *might* be needed; it's not like it'd
> be hard to retrieve them from git if they're ever needed.

My motivation is mainly API consistency ; and these functions are
small enough to not worry too much about them IMHO. If we *really*
want to get rid of them when they are unneeded, how about defining
them as static inline in the corresponding header file? Since they are
short and seldomly used (when used at all), this may be an acceptable
compromise.

> However, if you're insistent on keeping them, it'd be easy enough to
> provide patches to include an appropriate header with prototypes for
> them instead, which would also eliminate the warning this patch series
> set out to eliminate.

The patch did not mention any warning, unless I missed something.
Would be nice to explicitly mention it in the commit message.

Thanks,
Alex.
--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Josh Triplett Jan. 6, 2014, 8:17 a.m. UTC | #5 
On Mon, Jan 06, 2014 at 04:48:39PM +0900, Alexandre Courbot wrote:
> On Sat, Jan 4, 2014 at 5:22 AM, Josh Triplett <josh@joshtriplett.org> wrote:
> > On Fri, Jan 03, 2014 at 08:07:22PM +0900, Alexandre Courbot wrote:
> >> On Thu, Jan 2, 2014 at 10:07 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> >> > On Sat, Dec 21, 2013 at 11:49 AM, Rashika Kheria
> >> > <rashika.kheria@gmail.com> wrote:
> >> >> -/**
> >> >> - * devm_gpiod_put - Resource-managed gpiod_put()
> >> >> - * @desc:      GPIO descriptor to dispose of
> >> >> - *
> >> >> - * Dispose of a GPIO descriptor obtained with devm_gpiod_get() or
> >> >> - * devm_gpiod_get_index(). Normally this function will not be called as the GPIO
> >> >> - * will be disposed of by the resource management code.
> >> >> - */
> >> >> -void devm_gpiod_put(struct device *dev, struct gpio_desc *desc)
> >> >> -{
> >> >> -       WARN_ON(devres_release(dev, devm_gpiod_release, devm_gpiod_match,
> >> >> -               &desc));
> >> >> -}
> >> >> -EXPORT_SYMBOL(devm_gpiod_put);
> >> >
> >> > Alexandre what do you think about this? Can we think of a scenario
> >> > where explicit garbage collection is going to be needed or should we
> >> > remove this for now?
> >>
> >> Sorry for the delayed reply, I quickly saw the patch and then the
> >> holidays got in the way. :)
> >>
> >> I think all these functions should be kept. It is true that they are
> >> seldomly used, and that the purpose of devm is to garbage-collect
> >> resources upon driver removal, but they might (actually, probably
> >> will) become needed by someone at some point in the future. One
> >> example I can think of is two drivers that collaborate to share the
> >> same GPIO line. If they acquire the GPIO through devm_gpiod_get() they
> >> will need devm_gpiod_put() to release it so the other driver can
> >> acquire it.
> >>
> >> On a more general note, devm_clk_put(), devm_regulator_put(),
> >> devm_pinctrl_put() and probably others devm_*_put() functions are
> >> actively used in the kernel, to  support the idea that a devm removal
> >> function makes sense. That not all the subsystem-provided functions
> >> are used by mainline drivers does not necessary mean they should be
> >> removed, especially if they serve a purpose. We should keep our APIs
> >> consistent and future-proof, not to mention out-of-tree drivers that
> >> may use them.
> >>
> >> So this patch is a nack as far as I'm concerned, not only the GPIO
> >> part, but the whole of it.
> >
> > As far as I can tell, the few calls to the devm_*_put functions I can
> > see look like unnecessary calls as part of driver/device
> > uninitialization, and could be removed either directly or with minor
> > reworking.  That there are only 1-2 calls to each in the entire kernel
> > is also quite telling.
> >
> > In any case, it's disappointing to have a pile of unused functions in
> > the kernel on the theory that they *might* be needed; it's not like it'd
> > be hard to retrieve them from git if they're ever needed.
> 
> My motivation is mainly API consistency ; and these functions are
> small enough to not worry too much about them IMHO. If we *really*
> want to get rid of them when they are unneeded, how about defining
> them as static inline in the corresponding header file? Since they are
> short and seldomly used (when used at all), this may be an acceptable
> compromise.

That seems quite reasonable.

> > However, if you're insistent on keeping them, it'd be easy enough to
> > provide patches to include an appropriate header with prototypes for
> > them instead, which would also eliminate the warning this patch series
> > set out to eliminate.
> 
> The patch did not mention any warning, unless I missed something.
> Would be nice to explicitly mention it in the commit message.

This patch and the others like it are seeking to eliminate
-Wmissing-prototypes warnings from GCC and -Wdecl warnings from Sparse,
caused by having functions that are neither static nor prototyped in
advance.

- Josh Triplett
--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/gpio/devres.c b/drivers/gpio/devres.c
index 307464f..07ecca3 100644
--- a/drivers/gpio/devres.c
+++ b/drivers/gpio/devres.c
@@ -28,13 +28,6 @@  static void devm_gpiod_release(struct device *dev, void *res)
 	gpiod_put(*desc);
 }
 
-static int devm_gpiod_match(struct device *dev, void *res, void *data)
-{
-	struct gpio_desc **this = res, **gpio = data;
-
-	return *this == *gpio;
-}
-
 /**
  * devm_gpiod_get - Resource-managed gpiod_get()
  * @dev:	GPIO consumer
@@ -86,24 +79,6 @@  struct gpio_desc *__must_check devm_gpiod_get_index(struct device *dev,
 }
 EXPORT_SYMBOL(devm_gpiod_get_index);
 
-/**
- * devm_gpiod_put - Resource-managed gpiod_put()
- * @desc:	GPIO descriptor to dispose of
- *
- * Dispose of a GPIO descriptor obtained with devm_gpiod_get() or
- * devm_gpiod_get_index(). Normally this function will not be called as the GPIO
- * will be disposed of by the resource management code.
- */
-void devm_gpiod_put(struct device *dev, struct gpio_desc *desc)
-{
-	WARN_ON(devres_release(dev, devm_gpiod_release, devm_gpiod_match,
-		&desc));
-}
-EXPORT_SYMBOL(devm_gpiod_put);
-
-
-
-
 static void devm_gpio_release(struct device *dev, void *res)
 {
 	unsigned *gpio = res;
diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index 03cf8fb..913643f 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -299,23 +299,6 @@  void phy_put(struct phy *phy)
 EXPORT_SYMBOL_GPL(phy_put);
 
 /**
- * devm_phy_put() - release the PHY
- * @dev: device that wants to release this phy
- * @phy: the phy returned by devm_phy_get()
- *
- * destroys the devres associated with this phy and invokes phy_put
- * to release the phy.
- */
-void devm_phy_put(struct device *dev, struct phy *phy)
-{
-	int r;
-
-	r = devres_destroy(dev, devm_phy_release, devm_phy_match, phy);
-	dev_WARN_ONCE(dev, r, "couldn't find PHY resource\n");
-}
-EXPORT_SYMBOL_GPL(devm_phy_put);
-
-/**
  * of_phy_simple_xlate() - returns the phy instance from phy provider
  * @dev: the PHY provider device
  * @args: of_phandle_args (not used here)
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 2ca9504..c503b88e 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -771,21 +771,6 @@  static int devm_pwm_match(struct device *dev, void *res, void *data)
 }
 
 /**
- * devm_pwm_put() - resource managed pwm_put()
- * @dev: device for PWM consumer
- * @pwm: PWM device
- *
- * Release a PWM previously allocated using devm_pwm_get(). Calling this
- * function is usually not needed because devm-allocated resources are
- * automatically released on driver detach.
- */
-void devm_pwm_put(struct device *dev, struct pwm_device *pwm)
-{
-	WARN_ON(devres_release(dev, devm_pwm_release, devm_pwm_match, pwm));
-}
-EXPORT_SYMBOL_GPL(devm_pwm_put);
-
-/**
   * pwm_can_sleep() - report whether PWM access will sleep
   * @pwm: PWM device
   *
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index 4d34dbb..70862f7 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -31,7 +31,6 @@  struct gpio_desc *__must_check devm_gpiod_get(struct device *dev,
 struct gpio_desc *__must_check devm_gpiod_get_index(struct device *dev,
 						    const char *con_id,
 						    unsigned int idx);
-void devm_gpiod_put(struct device *dev, struct gpio_desc *desc);
 
 int gpiod_get_direction(const struct gpio_desc *desc);
 int gpiod_direction_input(struct gpio_desc *desc);
@@ -94,14 +93,6 @@  struct gpio_desc *__must_check devm_gpiod_get_index(struct device *dev,
 {
 	return ERR_PTR(-ENOSYS);
 }
-static inline void devm_gpiod_put(struct device *dev, struct gpio_desc *desc)
-{
-	might_sleep();
-
-	/* GPIO can never have been requested */
-	WARN_ON(1);
-}
-
 
 static inline int gpiod_get_direction(const struct gpio_desc *desc)
 {
diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
index 6d72269..129bac7 100644
--- a/include/linux/phy/phy.h
+++ b/include/linux/phy/phy.h
@@ -130,7 +130,6 @@  int phy_power_off(struct phy *phy);
 struct phy *phy_get(struct device *dev, const char *string);
 struct phy *devm_phy_get(struct device *dev, const char *string);
 void phy_put(struct phy *phy);
-void devm_phy_put(struct device *dev, struct phy *phy);
 struct phy *of_phy_simple_xlate(struct device *dev,
 	struct of_phandle_args *args);
 struct phy *phy_create(struct device *dev, const struct phy_ops *ops,
@@ -213,10 +212,6 @@  static inline void phy_put(struct phy *phy)
 {
 }
 
-static inline void devm_phy_put(struct device *dev, struct phy *phy)
-{
-}
-
 static inline struct phy *of_phy_simple_xlate(struct device *dev,
 	struct of_phandle_args *args)
 {
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index f0feafd..10ec30a 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -198,7 +198,6 @@  void pwm_put(struct pwm_device *pwm);
 struct pwm_device *devm_pwm_get(struct device *dev, const char *con_id);
 struct pwm_device *devm_of_pwm_get(struct device *dev, struct device_node *np,
 				   const char *con_id);
-void devm_pwm_put(struct device *dev, struct pwm_device *pwm);
 
 bool pwm_can_sleep(struct pwm_device *pwm);
 #else
@@ -258,10 +257,6 @@  static inline struct pwm_device *devm_of_pwm_get(struct device *dev,
 	return ERR_PTR(-ENODEV);
 }
 
-static inline void devm_pwm_put(struct device *dev, struct pwm_device *pwm)
-{
-}
-
 static inline bool pwm_can_sleep(struct pwm_device *pwm)
 {
 	return false;