On Wed, Sep 26, 2018 at 05:16:50PM +0530, Vivek Gautam wrote:
> Hi Jordan
>
> On Tue, Sep 25, 2018 at 2:56 AM Jordan Crouse <jcrouse@xxxxxxxxxxxxxx> wrote:
> >
> > llcc_slice_getd can return a ERR_PTR code on failure. Add a IS_ERR_OR_NULL
> > check to subsequent API calls that use struct llcc_slice_desc to guard
> > against faults and to let the leaf drivers get away with safely using a
> > ERR_PTR() encoded "pointer" in the aftermath of a llcc_slice_getd error.
>
> To me calling llcc_slice_putd(), llcc_slice_{de}activate(), and others
> with a error pointer sounds like we are on the wrong path anyways.
> The users can check for the error pointer return value and assign
> llcc_slice_desc to NULL, e.g., something that GPU is trying to do [1].
> So we can simply check for NULL value of desc in these APIs.
>
> [1] https://patchwork.freedesktop.org/patch/212400/
The GPU does set it to null, but it doens't check for NULL before calling
the API functions. I'm just taking it a step further and asserting that
the leaf driver shouldn't even need to bother resetting it to NULL if we
don't otherwise care about the status of the feature.
For us, the llcc is either there or it isn't and we don't have any way to
affect the outcome from the GPU driver. Of course we can check the return
value from the getd and set the pointer back to NULL but since this isn't
a mandatory feature for us it makes just as much sense to call the
activate/deactivate/putd functions as we normally do without worrying about it.
As for checking NULL - since getd returns a ERR_PTR encoded error code you
really need to check for it. Even if you assert that the leaf driver should
be checking the error code and resetting the pointer to NULL you have to admit
that at some point somebody will try to use the return value from
llc_slice_getd() without checking it or resetting it to NULL. It makes sense
to check for the same format that the allocating function returns.
Jordan
> >
> > Signed-off-by: Jordan Crouse <jcrouse@xxxxxxxxxxxxxx>
> > ---
> > drivers/soc/qcom/llcc-slice.c | 15 ++++++++++++++-
> > 1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/soc/qcom/llcc-slice.c b/drivers/soc/qcom/llcc-slice.c
> > index 192ca761b2cb..de1a35cd20ed 100644
> > --- a/drivers/soc/qcom/llcc-slice.c
> > +++ b/drivers/soc/qcom/llcc-slice.c
> > @@ -95,7 +95,8 @@ EXPORT_SYMBOL_GPL(llcc_slice_getd);
> > */
> > void llcc_slice_putd(struct llcc_slice_desc *desc)
> > {
> > - kfree(desc);
> > + if (!IS_ERR_OR_NULL(desc))
> > + kfree(desc);
>
> you will not need this check at all when desc = NULL.
>
> > }
> > EXPORT_SYMBOL_GPL(llcc_slice_putd);
> >
> > @@ -142,6 +143,9 @@ int llcc_slice_activate(struct llcc_slice_desc *desc)
> > int ret;
> > u32 act_ctrl_val;
> >
> > + if (IS_ERR_OR_NULL(desc))
>
> This can be simply replaced with NULL check.
> if (!desc)
>
> and same in the below hunks.
>
> Best regards
> Vivek
> > + return -EINVAL;
> > +
> > mutex_lock(&drv_data->lock);
> > if (test_bit(desc->slice_id, drv_data->bitmap)) {
> > mutex_unlock(&drv_data->lock);
> > @@ -176,6 +180,9 @@ int llcc_slice_deactivate(struct llcc_slice_desc *desc)
> > u32 act_ctrl_val;
> > int ret;
> >
> > + if (IS_ERR_OR_NULL(desc))
> > + return -EINVAL;
> > +
> > mutex_lock(&drv_data->lock);
> > if (!test_bit(desc->slice_id, drv_data->bitmap)) {
> > mutex_unlock(&drv_data->lock);
> > @@ -203,6 +210,9 @@ EXPORT_SYMBOL_GPL(llcc_slice_deactivate);
> > */
> > int llcc_get_slice_id(struct llcc_slice_desc *desc)
> > {
> > + if (IS_ERR_OR_NULL(desc))
> > + return -EINVAL;
> > +
> > return desc->slice_id;
> > }
> > EXPORT_SYMBOL_GPL(llcc_get_slice_id);
> > @@ -213,6 +223,9 @@ EXPORT_SYMBOL_GPL(llcc_get_slice_id);
> > */
> > size_t llcc_get_slice_size(struct llcc_slice_desc *desc)
> > {
> > + if (IS_ERR_OR_NULL(desc))
> > + return 0;
> > +
> > return desc->slice_size;
> > }
> > EXPORT_SYMBOL_GPL(llcc_get_slice_size);
> > --
> > 2.18.0
> >
>
>
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
