| Message ID | 1617881896-3164-6-git-send-email-yangyicong@hisilicon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Simplify codes with devm_add_action_or_reset | expand |
On Thu, Apr 8, 2021 at 2:41 PM Yicong Yang <yangyicong@hisilicon.com> wrote: > > Use devm_add_action_or_reset() instead of devres_alloc() and > devres_add(), which works the same. This will simplify the > code. There is no functional changes. > > Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> > --- > drivers/iio/industrialio-core.c | 43 +++++++++++++++-------------------------- > 1 file changed, 16 insertions(+), 27 deletions(-) > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c > index 7db761a..2dfbed3 100644 > --- a/drivers/iio/industrialio-core.c > +++ b/drivers/iio/industrialio-core.c > @@ -1627,9 +1627,9 @@ void iio_device_free(struct iio_dev *dev) > } > EXPORT_SYMBOL(iio_device_free); > > -static void devm_iio_device_release(struct device *dev, void *res) > +static void devm_iio_device_release(void *iio_dev) > { > - iio_device_free(*(struct iio_dev **)res); > + iio_device_free(iio_dev); > } > > /** > @@ -1645,20 +1645,17 @@ static void devm_iio_device_release(struct device *dev, void *res) > */ > struct iio_dev *devm_iio_device_alloc(struct device *parent, int sizeof_priv) > { > - struct iio_dev **ptr, *iio_dev; > - > - ptr = devres_alloc(devm_iio_device_release, sizeof(*ptr), > - GFP_KERNEL); > - if (!ptr) > - return NULL; > + struct iio_dev *iio_dev; > + int ret; > > iio_dev = iio_device_alloc(parent, sizeof_priv); > - if (iio_dev) { > - *ptr = iio_dev; > - devres_add(parent, ptr); > - } else { > - devres_free(ptr); > - } > + if (!iio_dev) > + return iio_dev; This is correct. But the preference is usually: if (!iio_dev) return NULL; > + > + ret = devm_add_action_or_reset(parent, devm_iio_device_release, > + iio_dev); > + if (ret) > + return ERR_PTR(ret); > > return iio_dev; > } > @@ -1889,29 +1886,21 @@ void iio_device_unregister(struct iio_dev *indio_dev) > } > EXPORT_SYMBOL(iio_device_unregister); > > -static void devm_iio_device_unreg(struct device *dev, void *res) > +static void devm_iio_device_unreg(void *indio_dev) > { > - iio_device_unregister(*(struct iio_dev **)res); > + iio_device_unregister(indio_dev); > } > > int __devm_iio_device_register(struct device *dev, struct iio_dev *indio_dev, > struct module *this_mod) > { > - struct iio_dev **ptr; > int ret; > > - ptr = devres_alloc(devm_iio_device_unreg, sizeof(*ptr), GFP_KERNEL); > - if (!ptr) > - return -ENOMEM; > - > - *ptr = indio_dev; > ret = __iio_device_register(indio_dev, this_mod); > - if (!ret) > - devres_add(dev, ptr); > - else > - devres_free(ptr); > + if (ret) > + return ret; > > - return ret; > + return devm_add_action_or_reset(dev, devm_iio_device_unreg, indio_dev); > } > EXPORT_SYMBOL_GPL(__devm_iio_device_register); > > -- > 2.8.1 >
On 2021/4/8 21:09, Alexandru Ardelean wrote: > On Thu, Apr 8, 2021 at 2:41 PM Yicong Yang <yangyicong@hisilicon.com> wrote: >> >> Use devm_add_action_or_reset() instead of devres_alloc() and >> devres_add(), which works the same. This will simplify the >> code. There is no functional changes. >> >> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> >> --- >> drivers/iio/industrialio-core.c | 43 +++++++++++++++-------------------------- >> 1 file changed, 16 insertions(+), 27 deletions(-) >> >> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c >> index 7db761a..2dfbed3 100644 >> --- a/drivers/iio/industrialio-core.c >> +++ b/drivers/iio/industrialio-core.c >> @@ -1627,9 +1627,9 @@ void iio_device_free(struct iio_dev *dev) >> } >> EXPORT_SYMBOL(iio_device_free); >> >> -static void devm_iio_device_release(struct device *dev, void *res) >> +static void devm_iio_device_release(void *iio_dev) >> { >> - iio_device_free(*(struct iio_dev **)res); >> + iio_device_free(iio_dev); >> } >> >> /** >> @@ -1645,20 +1645,17 @@ static void devm_iio_device_release(struct device *dev, void *res) >> */ >> struct iio_dev *devm_iio_device_alloc(struct device *parent, int sizeof_priv) >> { >> - struct iio_dev **ptr, *iio_dev; >> - >> - ptr = devres_alloc(devm_iio_device_release, sizeof(*ptr), >> - GFP_KERNEL); >> - if (!ptr) >> - return NULL; >> + struct iio_dev *iio_dev; >> + int ret; >> >> iio_dev = iio_device_alloc(parent, sizeof_priv); >> - if (iio_dev) { >> - *ptr = iio_dev; >> - devres_add(parent, ptr); >> - } else { >> - devres_free(ptr); >> - } >> + if (!iio_dev) >> + return iio_dev; > > This is correct. > But the preference is usually: > if (!iio_dev) > return NULL; > since it returned iio_dev when failure before, so i just keep as is >> + >> + ret = devm_add_action_or_reset(parent, devm_iio_device_release, >> + iio_dev); >> + if (ret) >> + return ERR_PTR(ret); >> >> return iio_dev; >> } >> @@ -1889,29 +1886,21 @@ void iio_device_unregister(struct iio_dev *indio_dev) >> } >> EXPORT_SYMBOL(iio_device_unregister); >> >> -static void devm_iio_device_unreg(struct device *dev, void *res) >> +static void devm_iio_device_unreg(void *indio_dev) >> { >> - iio_device_unregister(*(struct iio_dev **)res); >> + iio_device_unregister(indio_dev); >> } >> >> int __devm_iio_device_register(struct device *dev, struct iio_dev *indio_dev, >> struct module *this_mod) >> { >> - struct iio_dev **ptr; >> int ret; >> >> - ptr = devres_alloc(devm_iio_device_unreg, sizeof(*ptr), GFP_KERNEL); >> - if (!ptr) >> - return -ENOMEM; >> - >> - *ptr = indio_dev; >> ret = __iio_device_register(indio_dev, this_mod); >> - if (!ret) >> - devres_add(dev, ptr); >> - else >> - devres_free(ptr); >> + if (ret) >> + return ret; >> >> - return ret; >> + return devm_add_action_or_reset(dev, devm_iio_device_unreg, indio_dev); >> } >> EXPORT_SYMBOL_GPL(__devm_iio_device_register); >> >> -- >> 2.8.1 >> > > . >
On Fri, Apr 9, 2021 at 10:22 AM Yicong Yang <yangyicong@hisilicon.com> wrote: > On 2021/4/8 21:09, Alexandru Ardelean wrote: > > On Thu, Apr 8, 2021 at 2:41 PM Yicong Yang <yangyicong@hisilicon.com> wrote: ... > >> - if (!ptr) > >> - return NULL; > >> + if (!iio_dev) > >> + return iio_dev; > > > > This is correct. > > But the preference is usually: > > if (!iio_dev) > > return NULL; > > > > since it returned iio_dev when failure before, so i just keep as is Actually Alexandru has a good point. Compare the pieces I left above.
On 2021/4/9 17:19, Andy Shevchenko wrote: > On Fri, Apr 9, 2021 at 10:22 AM Yicong Yang <yangyicong@hisilicon.com> wrote: >> On 2021/4/8 21:09, Alexandru Ardelean wrote: >>> On Thu, Apr 8, 2021 at 2:41 PM Yicong Yang <yangyicong@hisilicon.com> wrote: > > ... > >>>> - if (!ptr) >>>> - return NULL; > >>>> + if (!iio_dev) >>>> + return iio_dev; >>> >>> This is correct. >>> But the preference is usually: >>> if (!iio_dev) >>> return NULL; >>> >> >> since it returned iio_dev when failure before, so i just keep as is > > Actually Alexandru has a good point. Compare the pieces I left above. > a little different from my perspective when I did this. previously: - ptr = devres_alloc(devm_iio_device_release, sizeof(*ptr), - GFP_KERNEL); - if (!ptr) - return NULL; if devres_alloc() failure we return NULL. iio_dev = iio_device_alloc(parent, sizeof_priv); - if (iio_dev) { - *ptr = iio_dev; - devres_add(parent, ptr); - } else { - devres_free(ptr); - } return iio_dev; if iio_device_alloc() failed, we return what it returns. now we remove devres_alloc() and stay iio_device_alloc(), so I just keep to return iio_dev. but actually iio_device_alloc() will return NULL at failure. :) so return NULL here is definitely correct. i'll change to that if it's recommended. thanks, Yicong
On Fri, Apr 9, 2021 at 12:42 PM Yicong Yang <yangyicong@hisilicon.com> wrote: > On 2021/4/9 17:19, Andy Shevchenko wrote: > > On Fri, Apr 9, 2021 at 10:22 AM Yicong Yang <yangyicong@hisilicon.com> wrote: > >> On 2021/4/8 21:09, Alexandru Ardelean wrote: > >>> On Thu, Apr 8, 2021 at 2:41 PM Yicong Yang <yangyicong@hisilicon.com> wrote: ... > > Actually Alexandru has a good point. Compare the pieces I left above. > > > > a little different from my perspective when I did this. > > previously: > > - ptr = devres_alloc(devm_iio_device_release, sizeof(*ptr), > - GFP_KERNEL); > - if (!ptr) > - return NULL; > > if devres_alloc() failure we return NULL. > > > iio_dev = iio_device_alloc(parent, sizeof_priv); > - if (iio_dev) { > - *ptr = iio_dev; > - devres_add(parent, ptr); > - } else { > - devres_free(ptr); > - } > > return iio_dev; > > if iio_device_alloc() failed, we return what it returns. > > now we remove devres_alloc() and stay iio_device_alloc(), so I just > keep to return iio_dev. > > but actually iio_device_alloc() will return NULL at failure. :) > so return NULL here is definitely correct. i'll change to that if > it's recommended. Confusing here (in your case) is that: if (!FOO) return BAR; ... return BAR; I.e. twice returning BAR for different occasions. This makes additional cognitive work and decrease readability / maintainability of the code. I.o.w., yes, it's preferred.
On 2021/4/9 17:55, Andy Shevchenko wrote: > On Fri, Apr 9, 2021 at 12:42 PM Yicong Yang <yangyicong@hisilicon.com> wrote: >> On 2021/4/9 17:19, Andy Shevchenko wrote: >>> On Fri, Apr 9, 2021 at 10:22 AM Yicong Yang <yangyicong@hisilicon.com> wrote: >>>> On 2021/4/8 21:09, Alexandru Ardelean wrote: >>>>> On Thu, Apr 8, 2021 at 2:41 PM Yicong Yang <yangyicong@hisilicon.com> wrote: > > ... > >>> Actually Alexandru has a good point. Compare the pieces I left above. >>> >> >> a little different from my perspective when I did this. >> >> previously: >> >> - ptr = devres_alloc(devm_iio_device_release, sizeof(*ptr), >> - GFP_KERNEL); >> - if (!ptr) >> - return NULL; >> >> if devres_alloc() failure we return NULL. >> >> >> iio_dev = iio_device_alloc(parent, sizeof_priv); >> - if (iio_dev) { >> - *ptr = iio_dev; >> - devres_add(parent, ptr); >> - } else { >> - devres_free(ptr); >> - } >> >> return iio_dev; >> >> if iio_device_alloc() failed, we return what it returns. >> >> now we remove devres_alloc() and stay iio_device_alloc(), so I just >> keep to return iio_dev. >> >> but actually iio_device_alloc() will return NULL at failure. :) >> so return NULL here is definitely correct. i'll change to that if >> it's recommended. > > Confusing here (in your case) is that: > > if (!FOO) > return BAR; > > ... > > return BAR; > > I.e. twice returning BAR for different occasions. This makes > additional cognitive work and decrease readability / maintainability > of the code. > > I.o.w., yes, it's preferred. > got it. will return NULL and will check if other patches in this series met the same condition. thanks.
On Fri, 9 Apr 2021 21:05:49 +0800 Yicong Yang <yangyicong@hisilicon.com> wrote: > On 2021/4/9 17:55, Andy Shevchenko wrote: > > On Fri, Apr 9, 2021 at 12:42 PM Yicong Yang <yangyicong@hisilicon.com> wrote: > >> On 2021/4/9 17:19, Andy Shevchenko wrote: > >>> On Fri, Apr 9, 2021 at 10:22 AM Yicong Yang <yangyicong@hisilicon.com> wrote: > >>>> On 2021/4/8 21:09, Alexandru Ardelean wrote: > >>>>> On Thu, Apr 8, 2021 at 2:41 PM Yicong Yang <yangyicong@hisilicon.com> wrote: > > > > ... > > > >>> Actually Alexandru has a good point. Compare the pieces I left above. > >>> > >> > >> a little different from my perspective when I did this. > >> > >> previously: > >> > >> - ptr = devres_alloc(devm_iio_device_release, sizeof(*ptr), > >> - GFP_KERNEL); > >> - if (!ptr) > >> - return NULL; > >> > >> if devres_alloc() failure we return NULL. > >> > >> > >> iio_dev = iio_device_alloc(parent, sizeof_priv); > >> - if (iio_dev) { > >> - *ptr = iio_dev; > >> - devres_add(parent, ptr); > >> - } else { > >> - devres_free(ptr); > >> - } > >> > >> return iio_dev; > >> > >> if iio_device_alloc() failed, we return what it returns. > >> > >> now we remove devres_alloc() and stay iio_device_alloc(), so I just > >> keep to return iio_dev. > >> > >> but actually iio_device_alloc() will return NULL at failure. :) > >> so return NULL here is definitely correct. i'll change to that if > >> it's recommended. > > > > Confusing here (in your case) is that: > > > > if (!FOO) > > return BAR; > > > > ... > > > > return BAR; > > > > I.e. twice returning BAR for different occasions. This makes > > additional cognitive work and decrease readability / maintainability > > of the code. > > > > I.o.w., yes, it's preferred. > > > > got it. will return NULL and will check if other patches in this series > met the same condition. > > thanks. > Rather than go around a gain, I fixed this up as discussed above and applied to the togreg branch of iio.git and pushed out as testing. Thanks, Jonathan
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c index 7db761a..2dfbed3 100644 --- a/drivers/iio/industrialio-core.c +++ b/drivers/iio/industrialio-core.c @@ -1627,9 +1627,9 @@ void iio_device_free(struct iio_dev *dev) } EXPORT_SYMBOL(iio_device_free); -static void devm_iio_device_release(struct device *dev, void *res) +static void devm_iio_device_release(void *iio_dev) { - iio_device_free(*(struct iio_dev **)res); + iio_device_free(iio_dev); } /** @@ -1645,20 +1645,17 @@ static void devm_iio_device_release(struct device *dev, void *res) */ struct iio_dev *devm_iio_device_alloc(struct device *parent, int sizeof_priv) { - struct iio_dev **ptr, *iio_dev; - - ptr = devres_alloc(devm_iio_device_release, sizeof(*ptr), - GFP_KERNEL); - if (!ptr) - return NULL; + struct iio_dev *iio_dev; + int ret; iio_dev = iio_device_alloc(parent, sizeof_priv); - if (iio_dev) { - *ptr = iio_dev; - devres_add(parent, ptr); - } else { - devres_free(ptr); - } + if (!iio_dev) + return iio_dev; + + ret = devm_add_action_or_reset(parent, devm_iio_device_release, + iio_dev); + if (ret) + return ERR_PTR(ret); return iio_dev; } @@ -1889,29 +1886,21 @@ void iio_device_unregister(struct iio_dev *indio_dev) } EXPORT_SYMBOL(iio_device_unregister); -static void devm_iio_device_unreg(struct device *dev, void *res) +static void devm_iio_device_unreg(void *indio_dev) { - iio_device_unregister(*(struct iio_dev **)res); + iio_device_unregister(indio_dev); } int __devm_iio_device_register(struct device *dev, struct iio_dev *indio_dev, struct module *this_mod) { - struct iio_dev **ptr; int ret; - ptr = devres_alloc(devm_iio_device_unreg, sizeof(*ptr), GFP_KERNEL); - if (!ptr) - return -ENOMEM; - - *ptr = indio_dev; ret = __iio_device_register(indio_dev, this_mod); - if (!ret) - devres_add(dev, ptr); - else - devres_free(ptr); + if (ret) + return ret; - return ret; + return devm_add_action_or_reset(dev, devm_iio_device_unreg, indio_dev); } EXPORT_SYMBOL_GPL(__devm_iio_device_register);
Use devm_add_action_or_reset() instead of devres_alloc() and devres_add(), which works the same. This will simplify the code. There is no functional changes. Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> --- drivers/iio/industrialio-core.c | 43 +++++++++++++++-------------------------- 1 file changed, 16 insertions(+), 27 deletions(-)