On Wed, Sep 26, 2018 at 9:51 PM Jordan Crouse <jcrouse@xxxxxxxxxxxxxx> wrote:
>
> 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.
Sure, makes sense then to embed these checks in llcc-slice only.
Reviewed-by: Vivek Gautam <vivek.gautam@xxxxxxxxxxxxxx>
Thanks
Vivek
>
> 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
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
