| Message ID | 20181025163811.17316-4-jcrouse@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fixes for failed LLCC probe | expand |
Hi, On Thu, Oct 25, 2018 at 9:38 AM Jordan Crouse <jcrouse@codeaurora.org> wrote: > > The real llcc_slide_getd() function returns ERR_PTR() encoded errors > so the stub function should too. > > Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org> > --- > include/linux/soc/qcom/llcc-qcom.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Douglas Anderson <dianders@chromium.org>
Quoting Jordan Crouse (2018-10-25 09:38:11) > The real llcc_slide_getd() function returns ERR_PTR() encoded errors > so the stub function should too. > > Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org> > --- > include/linux/soc/qcom/llcc-qcom.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/linux/soc/qcom/llcc-qcom.h b/include/linux/soc/qcom/llcc-qcom.h > index eb71a50b8afc..e9806d548834 100644 > --- a/include/linux/soc/qcom/llcc-qcom.h > +++ b/include/linux/soc/qcom/llcc-qcom.h > @@ -171,7 +171,7 @@ int qcom_llcc_remove(struct platform_device *pdev); > #else > static inline struct llcc_slice_desc *llcc_slice_getd(u32 uid) > { > - return NULL; > + return ERR_PTR(-ENODEV); Do you want it to be an error if your driver uses this API and doesn't get the pointer it was requesting? Typically if a framework isn't compiled in, and it isn't essential to the operation of the device, it makes sense to NOP out the API by returning NULL instead of an error. Then drivers only check for error pointers and treat the NULL pointer as a cookie meaning "do nothing".
On Fri, Nov 02, 2018 at 09:50:31AM -0700, Stephen Boyd wrote: > Quoting Jordan Crouse (2018-10-25 09:38:11) > > The real llcc_slide_getd() function returns ERR_PTR() encoded errors > > so the stub function should too. > > > > Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org> > > --- > > include/linux/soc/qcom/llcc-qcom.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/include/linux/soc/qcom/llcc-qcom.h b/include/linux/soc/qcom/llcc-qcom.h > > index eb71a50b8afc..e9806d548834 100644 > > --- a/include/linux/soc/qcom/llcc-qcom.h > > +++ b/include/linux/soc/qcom/llcc-qcom.h > > @@ -171,7 +171,7 @@ int qcom_llcc_remove(struct platform_device *pdev); > > #else > > static inline struct llcc_slice_desc *llcc_slice_getd(u32 uid) > > { > > - return NULL; > > + return ERR_PTR(-ENODEV); > > Do you want it to be an error if your driver uses this API and doesn't > get the pointer it was requesting? Typically if a framework isn't > compiled in, and it isn't essential to the operation of the device, it > makes sense to NOP out the API by returning NULL instead of an error. > Then drivers only check for error pointers and treat the NULL pointer as > a cookie meaning "do nothing". In my case, llcc is optional so if we get an error we just shrug and move on. We also have some local stuff to do for llcc so we would have to use an IF_ERR_OR_NULL instead of an IF_ERR() but I suppose that isn't terrible. Mostly was looking for consistency. I hate having code that does if (IS_ERR_OR_NULL(ptr)) pr_err("I got an error %d\n", IS_ERR(ptr) ? PTR_ERR(ptr) : -ESOMETHING); And since the regular code uses -ENODEV when the slice doesn't exist (which is the same as the whole thing not existing from the perspective of the driver) I figured that we could use -ENODEV as the universal sign of no soup for you. But I'm not angry about it - I can happily use IS_ERR_OR_NULL() if we need to. Jordan
Quoting Jordan Crouse (2018-11-02 10:07:22) > On Fri, Nov 02, 2018 at 09:50:31AM -0700, Stephen Boyd wrote: > > Quoting Jordan Crouse (2018-10-25 09:38:11) > > > The real llcc_slide_getd() function returns ERR_PTR() encoded errors > > > so the stub function should too. > > > > > > Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org> > > > --- > > > include/linux/soc/qcom/llcc-qcom.h | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/include/linux/soc/qcom/llcc-qcom.h b/include/linux/soc/qcom/llcc-qcom.h > > > index eb71a50b8afc..e9806d548834 100644 > > > --- a/include/linux/soc/qcom/llcc-qcom.h > > > +++ b/include/linux/soc/qcom/llcc-qcom.h > > > @@ -171,7 +171,7 @@ int qcom_llcc_remove(struct platform_device *pdev); > > > #else > > > static inline struct llcc_slice_desc *llcc_slice_getd(u32 uid) > > > { > > > - return NULL; > > > + return ERR_PTR(-ENODEV); > > > > Do you want it to be an error if your driver uses this API and doesn't > > get the pointer it was requesting? Typically if a framework isn't > > compiled in, and it isn't essential to the operation of the device, it > > makes sense to NOP out the API by returning NULL instead of an error. > > Then drivers only check for error pointers and treat the NULL pointer as > > a cookie meaning "do nothing". > > In my case, llcc is optional so if we get an error we just shrug and move > on. We also have some local stuff to do for llcc so we would have to use an > IF_ERR_OR_NULL instead of an IF_ERR() but I suppose that isn't terrible. > > Mostly was looking for consistency. I hate having code that does > > if (IS_ERR_OR_NULL(ptr)) > pr_err("I got an error %d\n", IS_ERR(ptr) ? PTR_ERR(ptr) : -ESOMETHING); Definitely, that looks awful. I'm not suggesting using IS_ERR_OR_NULL() here. > > And since the regular code uses -ENODEV when the slice doesn't exist (which is > the same as the whole thing not existing from the perspective of the driver) > I figured that we could use -ENODEV as the universal sign of no soup for you. > > But I'm not angry about it - I can happily use IS_ERR_OR_NULL() if we need to. > It may be that the llcc code needs to get an 'optional' API that lets you get the slice and not care if it isn't specified in DT. Then when the API isn't compiled in it's not an error, and when it's not specified in DT it isn't an error either, it's just a NULL pointer. You may want to if (IS_ENABLED()) the local things done in your driver based on the ifdef for LLCC itself too, or perhaps make it only do something if the cache size is non-zero, but the slice grabbing part could be generically done in driver probe and if that returned NULL you could make your local code skip whatever it does with the pointer. So then errors are blindly treated as probe errors and NULL pointers mean it's not there in DT or not compile in.
Hi, On Thu, Oct 25, 2018 at 9:38 AM Jordan Crouse <jcrouse@codeaurora.org> wrote: > > The real llcc_slide_getd() function returns ERR_PTR() encoded errors > so the stub function should too. > > Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org> > --- > include/linux/soc/qcom/llcc-qcom.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) What's the plan with this patch series? Andy: I think patches #1 and #2 are all set to go. They just seem like nice cleanups and have no comments on them. We're going to land them in the Chrome OS 4.19 tree so it'd be convenient to keep things synced up if you were able to land them too. Stephen / Jordan: can you come to a conclusion about what you want to do about patch #3? Land it as-is? Drop it? Spin it? ...I guess to summarize the argument (correct me if I'm wrong): Stephen would prefer it so that if the LLCC API returns an error that clients _shouldn't_ just shrug it off. They should treat it as an important error. ...but if LLCC API returns NULL then that's something that can be ignored. ...so if you meant to use LLCC and you're getting back -ENODEV then you should yell about it, but if you compile out LLCC they you won't hit your IS_ERR() and you'll be fine. -Doug
On Wed, Nov 28, 2018 at 08:56:01AM -0800, Doug Anderson wrote: > Hi, > > On Thu, Oct 25, 2018 at 9:38 AM Jordan Crouse <jcrouse@codeaurora.org> wrote: > > > > The real llcc_slide_getd() function returns ERR_PTR() encoded errors > > so the stub function should too. > > > > Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org> > > --- > > include/linux/soc/qcom/llcc-qcom.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > What's the plan with this patch series? > > Andy: I think patches #1 and #2 are all set to go. They just seem > like nice cleanups and have no comments on them. We're going to land > them in the Chrome OS 4.19 tree so it'd be convenient to keep things > synced up if you were able to land them too. > > > Stephen / Jordan: can you come to a conclusion about what you want to > do about patch #3? Land it as-is? Drop it? Spin it? > > ...I guess to summarize the argument (correct me if I'm wrong): > > Stephen would prefer it so that if the LLCC API returns an error that > clients _shouldn't_ just shrug it off. They should treat it as an > important error. ...but if LLCC API returns NULL then that's > something that can be ignored. ...so if you meant to use LLCC and > you're getting back -ENODEV then you should yell about it, but if you > compile out LLCC they you won't hit your IS_ERR() and you'll be fine. I still lean on the side of consistency with error codes but I'm not militant about it. My current version of the GPU patch handles NULL so we can drop this. Jordan
diff --git a/include/linux/soc/qcom/llcc-qcom.h b/include/linux/soc/qcom/llcc-qcom.h index eb71a50b8afc..e9806d548834 100644 --- a/include/linux/soc/qcom/llcc-qcom.h +++ b/include/linux/soc/qcom/llcc-qcom.h @@ -171,7 +171,7 @@ int qcom_llcc_remove(struct platform_device *pdev); #else static inline struct llcc_slice_desc *llcc_slice_getd(u32 uid) { - return NULL; + return ERR_PTR(-ENODEV); } static inline void llcc_slice_putd(struct llcc_slice_desc *desc)
The real llcc_slide_getd() function returns ERR_PTR() encoded errors so the stub function should too. Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org> --- include/linux/soc/qcom/llcc-qcom.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)