| Message ID | 1409137049-15848-1-git-send-email-pramod.gurav@smartplayin.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Wed, Aug 27, 2014 at 3:57 AM, Pramod Gurav <pramod.gurav@smartplayin.com> wrote: > This patches adds a call to gpiochip_remove_pin_ranges when > gpiochip_irqchip_add fails to release memory allocated for pin_ranges. > > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c > @@ -845,6 +845,7 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl) > IRQ_TYPE_NONE); > if (ret) { > dev_err(pctrl->dev, "Failed to add irqchip to gpiochip\n"); > + gpiochip_remove_pin_ranges(chip); > return -ENOSYS; > } Good catch, I guess this was lost in the introduction of gpiochip_irqchip... Rather than just releasing the pin_ranges of the gpio_chip you should probably add a gpiochip_remove() both here and in the case of gpiochip_add_pin_range() failing. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 28 August 2014 02:54 AM, Bjorn Andersson wrote: > On Wed, Aug 27, 2014 at 3:57 AM, Pramod Gurav > <pramod.gurav@smartplayin.com> wrote: >> This patches adds a call to gpiochip_remove_pin_ranges when >> gpiochip_irqchip_add fails to release memory allocated for pin_ranges. >> >> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c >> @@ -845,6 +845,7 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl) >> IRQ_TYPE_NONE); >> if (ret) { >> dev_err(pctrl->dev, "Failed to add irqchip to gpiochip\n"); >> + gpiochip_remove_pin_ranges(chip); >> return -ENOSYS; >> } > > Good catch, I guess this was lost in the introduction of gpiochip_irqchip... > > > Rather than just releasing the pin_ranges of the gpio_chip you should > probably add a gpiochip_remove() both here and in the case of > gpiochip_add_pin_range() failing. Thanks for review. But if I see implementation of gpiochip_remove() it does: gpiochip_irqchip_remove(chip); gpiochip_remove_pin_ranges(chip); of_gpiochip_remove(chip); In above failure case only gpiochip_add() and gpiochip_add_pin_range() have been successful hence I thought that would cause any problem to add gpiochip_remove(). If that is not a problem I think we can call gpiochip_remove() in fail case of gpiochip_add_pin_range() as well. Do I make sense? > > Regards, > Bjorn > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu 28 Aug 00:13 PDT 2014, Pramod Gurav wrote: > On Thursday 28 August 2014 02:54 AM, Bjorn Andersson wrote: > > On Wed, Aug 27, 2014 at 3:57 AM, Pramod Gurav > > <pramod.gurav@smartplayin.com> wrote: > >> This patches adds a call to gpiochip_remove_pin_ranges when > >> gpiochip_irqchip_add fails to release memory allocated for pin_ranges. > >> > >> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c > >> @@ -845,6 +845,7 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl) > >> IRQ_TYPE_NONE); > >> if (ret) { > >> dev_err(pctrl->dev, "Failed to add irqchip to gpiochip\n"); > >> + gpiochip_remove_pin_ranges(chip); > >> return -ENOSYS; > >> } > > > > Good catch, I guess this was lost in the introduction of gpiochip_irqchip... > > > > > > Rather than just releasing the pin_ranges of the gpio_chip you should > > probably add a gpiochip_remove() both here and in the case of > > gpiochip_add_pin_range() failing. > > Thanks for review. But if I see implementation of gpiochip_remove() it does: > gpiochip_irqchip_remove(chip); > gpiochip_remove_pin_ranges(chip); > of_gpiochip_remove(chip); > > In above failure case only gpiochip_add() and gpiochip_add_pin_range() > have been successful hence I thought that would cause any problem to add > gpiochip_remove(). If that is not a problem I think we can call > gpiochip_remove() in fail case of gpiochip_add_pin_range() as well. > Do I make sense? > As soon as gpiochip_add() have returned successfully we will have a live gpio_chip, upon returning unsuccessfully from probe devres will free the pctrl node and the gpio core will continue to operate on freed memory. Therefor we need to call gpio_remove() in the ccase of both gpiochip_add_pin_range() and gpiochip_irqchip_add() failing. The gpio_remove() does as you say remove those additional items, but handles the case where they are not yet "allocated". I hope this answers your conserns. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 29-08-2014 09:19 AM, Bjorn Andersson wrote: > On Thu 28 Aug 00:13 PDT 2014, Pramod Gurav wrote: > >> On Thursday 28 August 2014 02:54 AM, Bjorn Andersson wrote: >>> On Wed, Aug 27, 2014 at 3:57 AM, Pramod Gurav >>> <pramod.gurav@smartplayin.com> wrote: >>>> This patches adds a call to gpiochip_remove_pin_ranges when >>>> gpiochip_irqchip_add fails to release memory allocated for pin_ranges. >>>> >>>> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c >>>> @@ -845,6 +845,7 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl) >>>> IRQ_TYPE_NONE); >>>> if (ret) { >>>> dev_err(pctrl->dev, "Failed to add irqchip to gpiochip\n"); >>>> + gpiochip_remove_pin_ranges(chip); >>>> return -ENOSYS; >>>> } >>> >>> Good catch, I guess this was lost in the introduction of gpiochip_irqchip... >>> >>> >>> Rather than just releasing the pin_ranges of the gpio_chip you should >>> probably add a gpiochip_remove() both here and in the case of >>> gpiochip_add_pin_range() failing. >> >> Thanks for review. But if I see implementation of gpiochip_remove() it does: >> gpiochip_irqchip_remove(chip); >> gpiochip_remove_pin_ranges(chip); >> of_gpiochip_remove(chip); >> >> In above failure case only gpiochip_add() and gpiochip_add_pin_range() >> have been successful hence I thought that would cause any problem to add >> gpiochip_remove(). If that is not a problem I think we can call >> gpiochip_remove() in fail case of gpiochip_add_pin_range() as well. >> Do I make sense? >> > > As soon as gpiochip_add() have returned successfully we will have a live > gpio_chip, upon returning unsuccessfully from probe devres will free the pctrl > node and the gpio core will continue to operate on freed memory. > > Therefor we need to call gpio_remove() in the ccase of both > gpiochip_add_pin_range() and gpiochip_irqchip_add() failing. > > The gpio_remove() does as you say remove those additional items, but handles > the case where they are not yet "allocated". > > I hope this answers your conserns. That really does. Thanks for your time. Will resend the patch with handling failure cases of both gpiochip_add_pin_range() and gpiochip_irqchip_add(). > > Regards, > Bjorn > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 27, 2014 at 12:57 PM, Pramod Gurav <pramod.gurav@smartplayin.com> wrote: > This patches adds a call to gpiochip_remove_pin_ranges when > gpiochip_irqchip_add fails to release memory allocated for pin_ranges. > > CC: Ivan T. Ivanov <iivanov@mm-sol.com> > CC: Bjorn Andersson <bjorn.andersson@sonymobile.com> > CC: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Pramod Gurav <pramod.gurav@smartplayin.com> Dropping this one as the gpiochip_remove() patch fixes the problem. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" 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/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c index f4e4f8f..aa34b5a 100644 --- a/drivers/pinctrl/qcom/pinctrl-msm.c +++ b/drivers/pinctrl/qcom/pinctrl-msm.c @@ -845,6 +845,7 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl) IRQ_TYPE_NONE); if (ret) { dev_err(pctrl->dev, "Failed to add irqchip to gpiochip\n"); + gpiochip_remove_pin_ranges(chip); return -ENOSYS; }
This patches adds a call to gpiochip_remove_pin_ranges when gpiochip_irqchip_add fails to release memory allocated for pin_ranges. CC: Ivan T. Ivanov <iivanov@mm-sol.com> CC: Bjorn Andersson <bjorn.andersson@sonymobile.com> CC: Linus Walleij <linus.walleij@linaro.org> Signed-off-by: Pramod Gurav <pramod.gurav@smartplayin.com> --- drivers/pinctrl/qcom/pinctrl-msm.c | 1 + 1 file changed, 1 insertion(+)