| Message ID | 1354887168-3073-1-git-send-email-shawn.guo@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Shawn On Fri, 7 Dec 2012, Shawn Guo wrote: > Use devm_* managed functions, so that slot-gpio users do not have to > call mmc_gpio_free_ro/cd to free up resources requested in > mmc_gpio_request_ro/cd. > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> Thanks for the patch, but I'm not sure this is a good idea. Firstly, using devm_* allocation functions means, that normally you don't have to free these resources explicitly any more. So, actually you would have to remove free_irq() and gpio_free() calls completely from the API instead of replacing them with devm_* analogs. Secondly, I do use devm_kzalloc() in the API because I really want that allocated memory to just be allocated once and stay there until the device is freed. Whereas with with card detection and write-protect pins - usually you're right, you would only allocate those once during the lifetime of your host device. But - are we sure there cannot be exceptions? What if someone decides to switch between CD-IRQ and CD-polling at runtime? Or do something equally weird with their GPIOs... At least I personally don't think I have sufficient knowledge of all possible configurations to make such a decision. Think about pinctrl etc. What if at runtime the CD pin has to be released by mmc to be used by some other device? Anyway, I'm fine either way, but I'm just not sure how reasonable this change is. Thanks Guennadi > --- > drivers/mmc/core/slot-gpio.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c > index 16a1c0b..8b55748 100644 > --- a/drivers/mmc/core/slot-gpio.c > +++ b/drivers/mmc/core/slot-gpio.c > @@ -106,7 +106,8 @@ int mmc_gpio_request_ro(struct mmc_host *host, unsigned int gpio) > > ctx = host->slot.handler_priv; > > - ret = gpio_request_one(gpio, GPIOF_DIR_IN, ctx->ro_label); > + ret = devm_gpio_request_one(&host->class_dev, gpio, GPIOF_DIR_IN, > + ctx->ro_label); > if (ret < 0) > return ret; > > @@ -128,7 +129,8 @@ int mmc_gpio_request_cd(struct mmc_host *host, unsigned int gpio) > > ctx = host->slot.handler_priv; > > - ret = gpio_request_one(gpio, GPIOF_DIR_IN, ctx->cd_label); > + ret = devm_gpio_request_one(&host->class_dev, gpio, GPIOF_DIR_IN, > + ctx->cd_label); > if (ret < 0) > /* > * don't bother freeing memory. It might still get used by other > @@ -146,7 +148,8 @@ int mmc_gpio_request_cd(struct mmc_host *host, unsigned int gpio) > irq = -EINVAL; > > if (irq >= 0) { > - ret = request_threaded_irq(irq, NULL, mmc_gpio_cd_irqt, > + ret = devm_request_threaded_irq(&host->class_dev, irq, > + NULL, mmc_gpio_cd_irqt, > IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > ctx->cd_label, host); > if (ret < 0) > @@ -175,7 +178,7 @@ void mmc_gpio_free_ro(struct mmc_host *host) > gpio = ctx->ro_gpio; > ctx->ro_gpio = -EINVAL; > > - gpio_free(gpio); > + devm_gpio_free(&host->class_dev, gpio); > } > EXPORT_SYMBOL(mmc_gpio_free_ro); > > @@ -188,13 +191,13 @@ void mmc_gpio_free_cd(struct mmc_host *host) > return; > > if (host->slot.cd_irq >= 0) { > - free_irq(host->slot.cd_irq, host); > + devm_free_irq(&host->class_dev, host->slot.cd_irq, host); > host->slot.cd_irq = -EINVAL; > } > > gpio = ctx->cd_gpio; > ctx->cd_gpio = -EINVAL; > > - gpio_free(gpio); > + devm_gpio_free(&host->class_dev, gpio); > } > EXPORT_SYMBOL(mmc_gpio_free_cd); > -- > 1.7.9.5 > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Dec 07, 2012 at 02:49:43PM +0100, Guennadi Liakhovetski wrote: > Hi Shawn > > On Fri, 7 Dec 2012, Shawn Guo wrote: > > > Use devm_* managed functions, so that slot-gpio users do not have to > > call mmc_gpio_free_ro/cd to free up resources requested in > > mmc_gpio_request_ro/cd. > > > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > > Thanks for the patch, but I'm not sure this is a good idea. Firstly, using > devm_* allocation functions means, that normally you don't have to free > these resources explicitly any more. So, actually you would have to remove > free_irq() and gpio_free() calls completely from the API instead of > replacing them with devm_* analogs. With the changes, most of the slot-gpio users will only need to call mmc_gpio_request_* functions at probe time, nothing else. That said, they will not call mmc_gpio_free_* functions at all. I patched mmc_gpio_free_* functions replacing free_irq() and gpio_free() with devm_* versions to 1) ease the migration of exiting users calling mmc_gpio_free_*; 2) provide a mean for users to manually free resources for whatever reasons. > Secondly, I do use devm_kzalloc() in > the API because I really want that allocated memory to just be allocated > once and stay there until the device is freed. Whereas with with card > detection and write-protect pins - usually you're right, you would only > allocate those once during the lifetime of your host device. But - are we > sure there cannot be exceptions? What if someone decides to switch between > CD-IRQ and CD-polling at runtime? Or do something equally weird with their > GPIOs... At least I personally don't think I have sufficient knowledge of > all possible configurations to make such a decision. Think about pinctrl > etc. What if at runtime the CD pin has to be released by mmc to be used > by some other device? mmc_gpio_request_* and mmc_gpio_free_* pair are still there anyway. Users can manually call them in any way they want. > Anyway, I'm fine either way, but I'm just not sure > how reasonable this change is. My intension is to save slot-gpio users from checking GPIO CD/WP cases and calling mmc_gpio_request_* functions, so that we can have a cleaner .probe error path and .remove function. Shawn -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Shawn, Guennadi, On Fri, Dec 07 2012, Shawn Guo wrote: >> > Use devm_* managed functions, so that slot-gpio users do not have to >> > call mmc_gpio_free_ro/cd to free up resources requested in >> > mmc_gpio_request_ro/cd. >> > >> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> >> >> Thanks for the patch, but I'm not sure this is a good idea. Firstly, using >> devm_* allocation functions means, that normally you don't have to free >> these resources explicitly any more. So, actually you would have to remove >> free_irq() and gpio_free() calls completely from the API instead of >> replacing them with devm_* analogs. > > With the changes, most of the slot-gpio users will only need to call > mmc_gpio_request_* functions at probe time, nothing else. That said, > they will not call mmc_gpio_free_* functions at all. I patched > mmc_gpio_free_* functions replacing free_irq() and gpio_free() with > devm_* versions to 1) ease the migration of exiting users calling > mmc_gpio_free_*; 2) provide a mean for users to manually free resources > for whatever reasons. I'd like to find a way to merge this -- maybe we can compromise by adding a comment or some documentation that explains that you need to be careful (and use _request_* and _free_*) to avoid allocating more than once if you're doing something odd like runtime switching between CD methods? Thanks! - Chris.
On Mon, 10 Dec 2012, Chris Ball wrote: > Hi Shawn, Guennadi, > > On Fri, Dec 07 2012, Shawn Guo wrote: > >> > Use devm_* managed functions, so that slot-gpio users do not have to > >> > call mmc_gpio_free_ro/cd to free up resources requested in > >> > mmc_gpio_request_ro/cd. > >> > > >> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > >> > >> Thanks for the patch, but I'm not sure this is a good idea. Firstly, using > >> devm_* allocation functions means, that normally you don't have to free > >> these resources explicitly any more. So, actually you would have to remove > >> free_irq() and gpio_free() calls completely from the API instead of > >> replacing them with devm_* analogs. > > > > With the changes, most of the slot-gpio users will only need to call > > mmc_gpio_request_* functions at probe time, nothing else. That said, > > they will not call mmc_gpio_free_* functions at all. I patched > > mmc_gpio_free_* functions replacing free_irq() and gpio_free() with > > devm_* versions to 1) ease the migration of exiting users calling > > mmc_gpio_free_*; 2) provide a mean for users to manually free resources > > for whatever reasons. > > I'd like to find a way to merge this -- maybe we can compromise by > adding a comment or some documentation that explains that you need to be > careful (and use _request_* and _free_*) to avoid allocating more than > once if you're doing something odd like runtime switching between CD > methods? Ok, so, you agree, that the normal case is, when these GPIO functions are requested only once during probing and are released during driver unbinding, and that it's worth optimisine for this case. In principle I agree, and so far all 3 users of this API do that. So, maybe it would be good to extend this patch series with a patch (or 3 patches, if per-driver is preferred), removing mmc_gpio_free_*() calls from those drivers. I think, however, there is one more thing to consider here: the slot-gpio API contains a GPIO CD IRQ handler, which after these changes will only be freed after the calls to mmc_remove_host() and mmc_free_host(). This should be safe, because mmc_remove_host() sets the .rescan_disable flag. After the recent patch the GPIO CD ISR also calls host->ops->card_event(), so, hosts muct make sure, that this function is safe to call even after the driver's .remove() method has completed. With that in mind, I think, we're fine with this patch. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 11, 2012 at 08:32:14AM +0100, Guennadi Liakhovetski wrote: > Ok, so, you agree, that the normal case is, when these GPIO functions are > requested only once during probing and are released during driver > unbinding, and that it's worth optimisine for this case. In principle I > agree, and so far all 3 users of this API do that. So, maybe it would be > good to extend this patch series with a patch (or 3 patches, if per-driver > is preferred), removing mmc_gpio_free_*() calls from those drivers. Ok, will do (with single patch approach first). > I > think, however, there is one more thing to consider here: the slot-gpio > API contains a GPIO CD IRQ handler, which after these changes will only be > freed after the calls to mmc_remove_host() and mmc_free_host(). This > should be safe, because mmc_remove_host() sets the .rescan_disable flag. > After the recent patch the GPIO CD ISR also calls host->ops->card_event(), > so, hosts muct make sure, that this function is safe to call even after > the driver's .remove() method has completed. With that in mind, I think, > we're fine with this patch. > Thanks for the point. I think we should be fine as client drivers' .remove() hook will generally call mmc_remove_host() and mmc_free_host() in there. Shawn -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c index 16a1c0b..8b55748 100644 --- a/drivers/mmc/core/slot-gpio.c +++ b/drivers/mmc/core/slot-gpio.c @@ -106,7 +106,8 @@ int mmc_gpio_request_ro(struct mmc_host *host, unsigned int gpio) ctx = host->slot.handler_priv; - ret = gpio_request_one(gpio, GPIOF_DIR_IN, ctx->ro_label); + ret = devm_gpio_request_one(&host->class_dev, gpio, GPIOF_DIR_IN, + ctx->ro_label); if (ret < 0) return ret; @@ -128,7 +129,8 @@ int mmc_gpio_request_cd(struct mmc_host *host, unsigned int gpio) ctx = host->slot.handler_priv; - ret = gpio_request_one(gpio, GPIOF_DIR_IN, ctx->cd_label); + ret = devm_gpio_request_one(&host->class_dev, gpio, GPIOF_DIR_IN, + ctx->cd_label); if (ret < 0) /* * don't bother freeing memory. It might still get used by other @@ -146,7 +148,8 @@ int mmc_gpio_request_cd(struct mmc_host *host, unsigned int gpio) irq = -EINVAL; if (irq >= 0) { - ret = request_threaded_irq(irq, NULL, mmc_gpio_cd_irqt, + ret = devm_request_threaded_irq(&host->class_dev, irq, + NULL, mmc_gpio_cd_irqt, IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING | IRQF_ONESHOT, ctx->cd_label, host); if (ret < 0) @@ -175,7 +178,7 @@ void mmc_gpio_free_ro(struct mmc_host *host) gpio = ctx->ro_gpio; ctx->ro_gpio = -EINVAL; - gpio_free(gpio); + devm_gpio_free(&host->class_dev, gpio); } EXPORT_SYMBOL(mmc_gpio_free_ro); @@ -188,13 +191,13 @@ void mmc_gpio_free_cd(struct mmc_host *host) return; if (host->slot.cd_irq >= 0) { - free_irq(host->slot.cd_irq, host); + devm_free_irq(&host->class_dev, host->slot.cd_irq, host); host->slot.cd_irq = -EINVAL; } gpio = ctx->cd_gpio; ctx->cd_gpio = -EINVAL; - gpio_free(gpio); + devm_gpio_free(&host->class_dev, gpio); } EXPORT_SYMBOL(mmc_gpio_free_cd);
Use devm_* managed functions, so that slot-gpio users do not have to call mmc_gpio_free_ro/cd to free up resources requested in mmc_gpio_request_ro/cd. Signed-off-by: Shawn Guo <shawn.guo@linaro.org> --- drivers/mmc/core/slot-gpio.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)