| Message ID | 20181211200746.9557-2-jcrouse@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fixes for failed LLCC probe | expand |
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@codeaurora.org> > --- > 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? Regards, Andy
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@codeaurora.org> > > --- > > 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
On Tue, Feb 12, 2019 at 03:35:40PM -0700, Jordan Crouse wrote: <snip> > > > 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. OK, if you don't mind I can just change it to the above without you sending in a new patch. Andy
On Thu, Feb 14, 2019 at 10:16:30AM -0600, Andy Gross wrote: > On Tue, Feb 12, 2019 at 03:35:40PM -0700, Jordan Crouse wrote: > > <snip> > > > > > 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. > > OK, if you don't mind I can just change it to the above without you sending in a > new patch. No objections. Jordan > Andy
On Thu, Feb 14, 2019 at 03:05:31PM -0700, Jordan Crouse wrote:
> No objections.
On second thought, I left it as is. Sorry for the noise.
Andy
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; 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)) + return ERR_CAST(drv_data); + cfg = drv_data->cfg; sz = drv_data->cfg_size; @@ -108,6 +111,9 @@ static int llcc_update_act_ctrl(u32 sid, u32 slice_status; int ret; + if (IS_ERR(drv_data)) + return PTR_ERR(drv_data); + act_ctrl_reg = LLCC_TRP_ACT_CTRLn(sid); status_reg = LLCC_TRP_STATUSn(sid); @@ -143,6 +149,9 @@ int llcc_slice_activate(struct llcc_slice_desc *desc) int ret; u32 act_ctrl_val; + If (IS_ERR(drv_data)) + return PTR_ERR(drv_data); + if (IS_ERR_OR_NULL(desc)) return -EINVAL; @@ -180,6 +189,9 @@ int llcc_slice_deactivate(struct llcc_slice_desc *desc) u32 act_ctrl_val; int ret; + If (IS_ERR(drv_data)) + return PTR_ERR(drv_data); + if (IS_ERR_OR_NULL(desc)) return -EINVAL; @@ -289,6 +301,14 @@ static int qcom_llcc_cfg_program(struct platform_device *pdev) return ret; } +int qcom_llcc_remove(struct platform_device *pdev) +{ + /* Set the global pointer to a error code to avoid referencing it */ + drv_data = ERR_PTR(-ENODEV); + return 0; +} +EXPORT_SYMBOL_GPL(qcom_llcc_remove); + int qcom_llcc_probe(struct platform_device *pdev, const struct llcc_slice_config *llcc_cfg, u32 sz) { @@ -300,35 +320,45 @@ int qcom_llcc_probe(struct platform_device *pdev, struct platform_device *llcc_edac; drv_data = devm_kzalloc(dev, sizeof(*drv_data), GFP_KERNEL); - if (!drv_data) - return -ENOMEM; + if (!drv_data) { + ret = -ENOMEM; + goto err; + } llcc_banks_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "llcc_base"); llcc_banks_base = devm_ioremap_resource(&pdev->dev, llcc_banks_res); - if (IS_ERR(llcc_banks_base)) - return PTR_ERR(llcc_banks_base); + if (IS_ERR(llcc_banks_base)) { + ret = PTR_ERR(llcc_banks_base); + goto err; + } drv_data->regmap = devm_regmap_init_mmio(dev, llcc_banks_base, &llcc_regmap_config); - if (IS_ERR(drv_data->regmap)) - return PTR_ERR(drv_data->regmap); + if (IS_ERR(drv_data->regmap)) { + ret = PTR_ERR(drv_data->regmap); + goto err; + } llcc_bcast_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "llcc_broadcast_base"); llcc_bcast_base = devm_ioremap_resource(&pdev->dev, llcc_bcast_res); - if (IS_ERR(llcc_bcast_base)) - return PTR_ERR(llcc_bcast_base); + if (IS_ERR(llcc_bcast_base)) { + ret = PTR_ERR(llcc_bcast_base); + goto err; + } drv_data->bcast_regmap = devm_regmap_init_mmio(dev, llcc_bcast_base, &llcc_regmap_config); - if (IS_ERR(drv_data->bcast_regmap)) - return PTR_ERR(drv_data->bcast_regmap); + if (IS_ERR(drv_data->bcast_regmap)) { + ret = PTR_ERR(drv_data->bcast_regmap); + goto err; + } ret = regmap_read(drv_data->regmap, LLCC_COMMON_STATUS0, &num_banks); if (ret) - return ret; + goto err; num_banks &= LLCC_LB_CNT_MASK; num_banks >>= LLCC_LB_CNT_SHIFT; @@ -340,8 +370,10 @@ int qcom_llcc_probe(struct platform_device *pdev, drv_data->offsets = devm_kcalloc(dev, num_banks, sizeof(u32), GFP_KERNEL); - if (!drv_data->offsets) - return -ENOMEM; + if (!drv_data->offsets) { + ret = -ENOMEM; + goto err; + } for (i = 0; i < num_banks; i++) drv_data->offsets[i] = i * BANK_OFFSET_STRIDE; @@ -349,8 +381,10 @@ int qcom_llcc_probe(struct platform_device *pdev, drv_data->bitmap = devm_kcalloc(dev, BITS_TO_LONGS(drv_data->max_slices), sizeof(unsigned long), GFP_KERNEL); - if (!drv_data->bitmap) - return -ENOMEM; + if (!drv_data->bitmap) { + ret = -ENOMEM; + goto err; + } drv_data->cfg = llcc_cfg; drv_data->cfg_size = sz; @@ -359,7 +393,7 @@ int qcom_llcc_probe(struct platform_device *pdev, ret = qcom_llcc_cfg_program(pdev); if (ret) - return ret; + goto err; drv_data->ecc_irq = platform_get_irq(pdev, 0); if (drv_data->ecc_irq >= 0) { @@ -370,6 +404,9 @@ int qcom_llcc_probe(struct platform_device *pdev, dev_err(dev, "Failed to register llcc edac driver\n"); } + return 0; +err: + drv_data = ERR_PTR(-ENODEV); return ret; } EXPORT_SYMBOL_GPL(qcom_llcc_probe); diff --git a/include/linux/soc/qcom/llcc-qcom.h b/include/linux/soc/qcom/llcc-qcom.h index 69c285b1c990..eb71a50b8afc 100644 --- a/include/linux/soc/qcom/llcc-qcom.h +++ b/include/linux/soc/qcom/llcc-qcom.h @@ -162,6 +162,12 @@ int llcc_slice_deactivate(struct llcc_slice_desc *desc); */ int qcom_llcc_probe(struct platform_device *pdev, const struct llcc_slice_config *table, u32 sz); + +/** + * qcom_llcc_remove - remove the sct table + * @pdev: Platform device pointer + */ +int qcom_llcc_remove(struct platform_device *pdev); #else static inline struct llcc_slice_desc *llcc_slice_getd(u32 uid) {
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@codeaurora.org> --- 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(-)