| Message ID | 1470416816-53336-1-git-send-email-lina.iyer@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Fri, Aug 05, 2016 at 11:06:56AM -0600, Lina Iyer wrote: > ENOSYS is for syscalls that are not supported. ENODEV is a better return > value when the PM Domains are not available. > > Signed-off-by: Lina Iyer <lina.iyer@linaro.org> I have no opinion on this, but would like to point out that other parts in the PM core use -ENOSYS as well (e.g. see drivers/base/power/core.c). For consistency, if pm_domain.h is changed, the rest should probably be changed as well. It should also be kept in mind that functions might check the return code and react specifically to the currently used -ENOSYS. (I've written code like that just yesterday, for the (unmerged) thunderbolt runtime pm.) Thanks, Lukas > --- > include/linux/pm_domain.h | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h > index 688dc57..8a63406 100644 > --- a/include/linux/pm_domain.h > +++ b/include/linux/pm_domain.h > @@ -151,7 +151,7 @@ extern struct dev_power_governor pm_domain_always_on_gov; > > static inline struct generic_pm_domain_data *dev_gpd_data(struct device *dev) > { > - return ERR_PTR(-ENOSYS); > + return ERR_PTR(-ENODEV); > } > static inline struct generic_pm_domain *pm_genpd_lookup_dev(struct device *dev) > { > @@ -161,27 +161,27 @@ static inline int __pm_genpd_add_device(struct generic_pm_domain *genpd, > struct device *dev, > struct gpd_timing_data *td) > { > - return -ENOSYS; > + return -ENODEV; > } > static inline int pm_genpd_remove_device(struct generic_pm_domain *genpd, > struct device *dev) > { > - return -ENOSYS; > + return -ENODEV; > } > static inline int pm_genpd_add_subdomain(struct generic_pm_domain *genpd, > struct generic_pm_domain *new_sd) > { > - return -ENOSYS; > + return -ENODEV; > } > static inline int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd, > struct generic_pm_domain *target) > { > - return -ENOSYS; > + return -ENODEV; > } > static inline int pm_genpd_init(struct generic_pm_domain *genpd, > struct dev_power_governor *gov, bool is_off) > { > - return -ENOSYS; > + return -ENODEV; > } > static inline int pm_genpd_of_parse_power_states( > struct generic_pm_domain *genpd) > -- > 2.7.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 5, 2016 at 9:43 PM, Lukas Wunner <lukas@wunner.de> wrote: > On Fri, Aug 05, 2016 at 11:06:56AM -0600, Lina Iyer wrote: >> ENOSYS is for syscalls that are not supported. ENODEV is a better return >> value when the PM Domains are not available. >> >> Signed-off-by: Lina Iyer <lina.iyer@linaro.org> > > I have no opinion on this, but would like to point out that other parts > in the PM core use -ENOSYS as well (e.g. see drivers/base/power/core.c). > For consistency, if pm_domain.h is changed, the rest should probably be > changed as well. I agree here. Changing one piece only is likely to result in confusion. Also I'm not sure if ENODEV is a good replacement at all. ENOTSUPP was suggested instead last time a similar patch was discussed. > It should also be kept in mind that functions might > check the return code and react specifically to the currently used -ENOSYS. > (I've written code like that just yesterday, for the (unmerged) thunderbolt > runtime pm.) But I wouldn't write code doing something like this. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Aug 06, 2016 at 12:17:58AM +0200, Rafael J. Wysocki wrote: > On Fri, Aug 5, 2016 at 9:43 PM, Lukas Wunner <lukas@wunner.de> wrote: > > On Fri, Aug 05, 2016 at 11:06:56AM -0600, Lina Iyer wrote: > >> ENOSYS is for syscalls that are not supported. ENODEV is a better return > >> value when the PM Domains are not available. > >> > >> Signed-off-by: Lina Iyer <lina.iyer@linaro.org> > > > > I have no opinion on this, but would like to point out that other parts > > in the PM core use -ENOSYS as well (e.g. see drivers/base/power/core.c). I meant drivers/base/power/runtime.c and include/linux/pm_runtime.h, apologies. > > For consistency, if pm_domain.h is changed, the rest should probably be > > changed as well. > > I agree here. Changing one piece only is likely to result in confusion. > > Also I'm not sure if ENODEV is a good replacement at all. ENOTSUPP > was suggested instead last time a similar patch was discussed. > > > It should also be kept in mind that functions might > > check the return code and react specifically to the currently used -ENOSYS. > > (I've written code like that just yesterday, for the (unmerged) thunderbolt > > runtime pm.) > > But I wouldn't write code doing something like this. Understood, I just removed it from my tree. :-) Thanks, Lukas -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Rafael, On Sat, Aug 6, 2016 at 12:17 AM, Rafael J. Wysocki <rafael@kernel.org> wrote: > On Fri, Aug 5, 2016 at 9:43 PM, Lukas Wunner <lukas@wunner.de> wrote: >> On Fri, Aug 05, 2016 at 11:06:56AM -0600, Lina Iyer wrote: >>> ENOSYS is for syscalls that are not supported. ENODEV is a better return >>> value when the PM Domains are not available. >>> >>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org> >> >> I have no opinion on this, but would like to point out that other parts >> in the PM core use -ENOSYS as well (e.g. see drivers/base/power/core.c). >> For consistency, if pm_domain.h is changed, the rest should probably be >> changed as well. ENOSYS has been used to indicate "functionality not supported" all over the place... > I agree here. Changing one piece only is likely to result in confusion. > > Also I'm not sure if ENODEV is a good replacement at all. ENOTSUPP > was suggested instead last time a similar patch was discussed. > >> It should also be kept in mind that functions might >> check the return code and react specifically to the currently used -ENOSYS. >> (I've written code like that just yesterday, for the (unmerged) thunderbolt >> runtime pm.) > > But I wouldn't write code doing something like this. (in the general case, not limited to PM) What to do instead, if you know a dummy function returns -ENOSYS and you need to react on that? In these cases the exact error code is a contract between caller and callee. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday, August 08, 2016 11:11:11 AM Geert Uytterhoeven wrote: > Hi Rafael, > > On Sat, Aug 6, 2016 at 12:17 AM, Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Fri, Aug 5, 2016 at 9:43 PM, Lukas Wunner <lukas@wunner.de> wrote: > >> On Fri, Aug 05, 2016 at 11:06:56AM -0600, Lina Iyer wrote: > >>> ENOSYS is for syscalls that are not supported. ENODEV is a better return > >>> value when the PM Domains are not available. > >>> > >>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org> > >> > >> I have no opinion on this, but would like to point out that other parts > >> in the PM core use -ENOSYS as well (e.g. see drivers/base/power/core.c). > >> For consistency, if pm_domain.h is changed, the rest should probably be > >> changed as well. > > ENOSYS has been used to indicate "functionality not supported" all over the > place... > > > I agree here. Changing one piece only is likely to result in confusion. > > > > Also I'm not sure if ENODEV is a good replacement at all. ENOTSUPP > > was suggested instead last time a similar patch was discussed. > > > >> It should also be kept in mind that functions might > >> check the return code and react specifically to the currently used -ENOSYS. > >> (I've written code like that just yesterday, for the (unmerged) thunderbolt > >> runtime pm.) > > > > But I wouldn't write code doing something like this. > > (in the general case, not limited to PM) What to do instead, if you know a > dummy function returns -ENOSYS and you need to react on that? Use IS_ENABLED()? That at least will make the intention clear to an occasional reader of the code. > In these cases the exact error code is a contract between caller and > callee. That's fair enough. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pm" 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/include/linux/pm_domain.h b/include/linux/pm_domain.h index 688dc57..8a63406 100644 --- a/include/linux/pm_domain.h +++ b/include/linux/pm_domain.h @@ -151,7 +151,7 @@ extern struct dev_power_governor pm_domain_always_on_gov; static inline struct generic_pm_domain_data *dev_gpd_data(struct device *dev) { - return ERR_PTR(-ENOSYS); + return ERR_PTR(-ENODEV); } static inline struct generic_pm_domain *pm_genpd_lookup_dev(struct device *dev) { @@ -161,27 +161,27 @@ static inline int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev, struct gpd_timing_data *td) { - return -ENOSYS; + return -ENODEV; } static inline int pm_genpd_remove_device(struct generic_pm_domain *genpd, struct device *dev) { - return -ENOSYS; + return -ENODEV; } static inline int pm_genpd_add_subdomain(struct generic_pm_domain *genpd, struct generic_pm_domain *new_sd) { - return -ENOSYS; + return -ENODEV; } static inline int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd, struct generic_pm_domain *target) { - return -ENOSYS; + return -ENODEV; } static inline int pm_genpd_init(struct generic_pm_domain *genpd, struct dev_power_governor *gov, bool is_off) { - return -ENOSYS; + return -ENODEV; } static inline int pm_genpd_of_parse_power_states( struct generic_pm_domain *genpd)
ENOSYS is for syscalls that are not supported. ENODEV is a better return value when the PM Domains are not available. Signed-off-by: Lina Iyer <lina.iyer@linaro.org> --- include/linux/pm_domain.h | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)