On Tue, Feb 12, 2019 at 03:34:35PM -0600, Andy Gross wrote:
> On Tue, Dec 11, 2018 at 01:07:45PM -0700, Jordan Crouse wrote:
> > Currently the data structure for llc-slice is devm allocated and
> > stored as a global but never cleared if the probe function fails.
> > This is a problem because devm managed memory gets freed on probe
> > failure the API functions could access the pointer after it has been
> > freed.
> >
> > Initialize the drv_data pointer to an error and reset it to an error
> > on probe failure or device destroy and add protection to the API
> > functions to make sure the memory doesn't get accessed.
> >
> > Signed-off-by: Jordan Crouse <jcrouse@xxxxxxxxxxxxxx>
> > ---
> > drivers/soc/qcom/llcc-sdm845.c | 6 +++
> > drivers/soc/qcom/llcc-slice.c | 71 +++++++++++++++++++++++-------
> > include/linux/soc/qcom/llcc-qcom.h | 6 +++
> > 3 files changed, 66 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/soc/qcom/llcc-sdm845.c b/drivers/soc/qcom/llcc-sdm845.c
> > index 2e1e4f0a5db8..86600d97c36d 100644
> > --- a/drivers/soc/qcom/llcc-sdm845.c
> > +++ b/drivers/soc/qcom/llcc-sdm845.c
> > @@ -71,6 +71,11 @@ static struct llcc_slice_config sdm845_data[] = {
> > SCT_ENTRY(LLCC_AUDHW, 22, 1024, 1, 1, 0xffc, 0x2, 0, 0, 1, 1, 0),
> > };
> >
> > +static int sdm845_qcom_llcc_remove(struct platform_device *pdev)
> > +{
> > + return qcom_llcc_remove(pdev);
> > +}
> > +
> > static int sdm845_qcom_llcc_probe(struct platform_device *pdev)
> > {
> > return qcom_llcc_probe(pdev, sdm845_data, ARRAY_SIZE(sdm845_data));
> > @@ -87,6 +92,7 @@ static struct platform_driver sdm845_qcom_llcc_driver = {
> > .of_match_table = sdm845_qcom_llcc_of_match,
> > },
> > .probe = sdm845_qcom_llcc_probe,
> > + .remove = sdm845_qcom_llcc_remove,
> > };
> > module_platform_driver(sdm845_qcom_llcc_driver);
> >
> > diff --git a/drivers/soc/qcom/llcc-slice.c b/drivers/soc/qcom/llcc-slice.c
> > index 80667f7be52c..8390bc006a31 100644
> > --- a/drivers/soc/qcom/llcc-slice.c
> > +++ b/drivers/soc/qcom/llcc-slice.c
> > @@ -46,7 +46,7 @@
> >
> > #define BANK_OFFSET_STRIDE 0x80000
> >
> > -static struct llcc_drv_data *drv_data;
> > +static struct llcc_drv_data *drv_data = (void *) -EPROBE_DEFER;
>
> I'd rather this be left null
>
> >
> > static const struct regmap_config llcc_regmap_config = {
> > .reg_bits = 32,
> > @@ -68,6 +68,9 @@ struct llcc_slice_desc *llcc_slice_getd(u32 uid)
> > struct llcc_slice_desc *desc;
> > u32 sz, count;
> >
> > + if (IS_ERR(drv_data))
>
> IS_ERR_OR_NULL would catch the null from above. Are you using the EPROBE_DEFER
> to probe defer somewhere else?
Yeah, the intent was to automatically return -EPROBE_DEFER until the sub-system
came along and initialized it so that the leaf driver could tell the difference
between an non existent LLCC and just one that hasn't been set up yet. Its a
bit contrived. I'm okay with leaving it as NULL and then letting the leaf driver
decide if it wants to try again later.
Jordan
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
