Message ID | 1635860673-12146-1-git-send-email-pillair@codeaurora.org |
---|---|
Headers | show |
Series | Add support for sc7280 WPSS PIL loading | expand |
Quoting Rakesh Pillai (2021-11-02 06:44:33) > diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c b/drivers/remoteproc/qcom_q6v5_adsp.c > index 098362e6..e2e8d33 100644 > --- a/drivers/remoteproc/qcom_q6v5_adsp.c > +++ b/drivers/remoteproc/qcom_q6v5_adsp.c > @@ -435,12 +571,22 @@ static int adsp_probe(struct platform_device *pdev) > if (!desc) > return -EINVAL; > > + firmware_name = desc->firmware_name; > + ret = of_property_read_string(pdev->dev.of_node, "firmware-name", Is this documented in the binding? If not, please add it. > + &firmware_name); > + if (ret < 0 && ret != -EINVAL) { > + dev_err(&pdev->dev, "unable to read firmware-name\n"); > + return ret; > + } > + > rproc = rproc_alloc(&pdev->dev, pdev->name, &adsp_ops, > - desc->firmware_name, sizeof(*adsp)); > + firmware_name, sizeof(*adsp)); > if (!rproc) {
> -----Original Message----- > From: Stephen Boyd <swboyd@chromium.org> > Sent: Tuesday, November 16, 2021 5:13 AM > To: Rakesh Pillai <pillair@codeaurora.org>; agross@kernel.org; > bjorn.andersson@linaro.org; mathieu.poirier@linaro.org; ohad@wizery.com; > p.zabel@pengutronix.de; robh+dt@kernel.org > Cc: linux-arm-msm@vger.kernel.org; linux-remoteproc@vger.kernel.org; > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > sibis@codeaurora.org; mpubbise@codeaurora.org; kuabhs@chromium.org > Subject: Re: [PATCH v8 3/3] remoteproc: qcom: q6v5_wpss: Add support for > sc7280 WPSS > > Quoting Rakesh Pillai (2021-11-02 06:44:33) > > diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c > b/drivers/remoteproc/qcom_q6v5_adsp.c > > index 098362e6..e2e8d33 100644 > > --- a/drivers/remoteproc/qcom_q6v5_adsp.c > > +++ b/drivers/remoteproc/qcom_q6v5_adsp.c > > @@ -435,12 +571,22 @@ static int adsp_probe(struct platform_device > *pdev) > > if (!desc) > > return -EINVAL; > > > > + firmware_name = desc->firmware_name; > > + ret = of_property_read_string(pdev->dev.of_node, "firmware- > name", > > Is this documented in the binding? If not, please add it. Hi Stephen, "firmware-name" is already documented in the bindings. Thanks, Rakesh Pillai.
Quoting Rakesh Pillai (2021-11-02 06:44:33) > @@ -457,7 +608,13 @@ static int adsp_probe(struct platform_device *pdev) > if (ret) > goto free_rproc; > > - pm_runtime_enable(adsp->dev); > + ret = qcom_rproc_pds_attach(adsp->dev, adsp->proxy_pds, > + desc->proxy_pd_names); > + if (ret < 0) { > + dev_err(&pdev->dev, "Failed to attach proxy power domains\n"); > + goto free_rproc; > + } > + adsp->proxy_pd_count = ret; Can we check this against the define so that we don't have more than the fixed number of power domains and try to access elements beyond the length of the array?
Quoting pillair@codeaurora.org (2021-11-16 09:49:05) > > > > Is this documented in the binding? If not, please add it. > > Hi Stephen, > "firmware-name" is already documented in the bindings. > Ok I see it now. Thanks!
> -----Original Message----- > From: Stephen Boyd <swboyd@chromium.org> > Sent: Wednesday, November 17, 2021 4:25 AM > To: Rakesh Pillai <pillair@codeaurora.org>; agross@kernel.org; > bjorn.andersson@linaro.org; mathieu.poirier@linaro.org; ohad@wizery.com; > p.zabel@pengutronix.de; robh+dt@kernel.org > Cc: linux-arm-msm@vger.kernel.org; linux-remoteproc@vger.kernel.org; > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > sibis@codeaurora.org; mpubbise@codeaurora.org; kuabhs@chromium.org > Subject: Re: [PATCH v8 3/3] remoteproc: qcom: q6v5_wpss: Add support for > sc7280 WPSS > > Quoting Rakesh Pillai (2021-11-02 06:44:33) > > @@ -457,7 +608,13 @@ static int adsp_probe(struct platform_device > *pdev) > > if (ret) > > goto free_rproc; > > > > - pm_runtime_enable(adsp->dev); > > + ret = qcom_rproc_pds_attach(adsp->dev, adsp->proxy_pds, > > + desc->proxy_pd_names); > > + if (ret < 0) { > > + dev_err(&pdev->dev, "Failed to attach proxy power domains\n"); > > + goto free_rproc; > > + } > > + adsp->proxy_pd_count = ret; > > Can we check this against the define so that we don't have more than the > fixed number of power domains and try to access elements beyond the > length of the array? The number of entries populated in the "proxy_pds" array depends on the "desc->proxy_pd_names", which is statically initialized for each remoteproc. Hence there will not be any out of bound access for this array. Thanks, Rakesh Pillai.
Quoting Rakesh Pillai (2021-11-16 22:31:51) > > > > -----Original Message----- > > From: Stephen Boyd <swboyd@chromium.org> > > Sent: Wednesday, November 17, 2021 4:25 AM > > To: Rakesh Pillai <pillair@codeaurora.org>; agross@kernel.org; > > bjorn.andersson@linaro.org; mathieu.poirier@linaro.org; ohad@wizery.com; > > p.zabel@pengutronix.de; robh+dt@kernel.org > > Cc: linux-arm-msm@vger.kernel.org; linux-remoteproc@vger.kernel.org; > > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; > > sibis@codeaurora.org; mpubbise@codeaurora.org; kuabhs@chromium.org > > Subject: Re: [PATCH v8 3/3] remoteproc: qcom: q6v5_wpss: Add support for > > sc7280 WPSS > > > > Quoting Rakesh Pillai (2021-11-02 06:44:33) > > > @@ -457,7 +608,13 @@ static int adsp_probe(struct platform_device > > *pdev) > > > if (ret) > > > goto free_rproc; > > > > > > - pm_runtime_enable(adsp->dev); > > > + ret = qcom_rproc_pds_attach(adsp->dev, adsp->proxy_pds, > > > + desc->proxy_pd_names); > > > + if (ret < 0) { > > > + dev_err(&pdev->dev, "Failed to attach proxy power domains\n"); > > > + goto free_rproc; > > > + } > > > + adsp->proxy_pd_count = ret; > > > > Can we check this against the define so that we don't have more than the > > fixed number of power domains and try to access elements beyond the > > length of the array? > > The number of entries populated in the "proxy_pds" array depends on the "desc->proxy_pd_names", which is statically > initialized for each remoteproc. Hence there will not be any out of bound access for this array. > Sure nothing is wrong today but it's a potential problem in the future if someone adds more elements to proxy_pd_names than proxy_pds can hold. Please prevent that from happening by writing stricter code.