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