| Message ID | 20170215045157.11659-2-j.neuschaefer@gmx.net (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Andy Gross |
| Headers | show |
On Tue 14 Feb 20:51 PST 2017, Jonathan Neusch?fer wrote: > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> > --- > arch/arm/configs/qcom_defconfig | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/arch/arm/configs/qcom_defconfig b/arch/arm/configs/qcom_defconfig > index 4ffdd607205d..97b9e9cd2292 100644 > --- a/arch/arm/configs/qcom_defconfig > +++ b/arch/arm/configs/qcom_defconfig > @@ -190,11 +190,16 @@ CONFIG_MDM_LCC_9615=y > CONFIG_MSM_MMCC_8960=y > CONFIG_MSM_MMCC_8974=y > CONFIG_HWSPINLOCK_QCOM=y > +CONFIG_REMOTEPROC=y > +CONFIG_QCOM_ADSP_PIL=y > +CONFIG_QCOM_Q6V5_PIL=y > +CONFIG_QCOM_WCNSS_PIL=y > CONFIG_QCOM_GSBI=y > CONFIG_QCOM_PM=y > CONFIG_QCOM_SMEM=y > CONFIG_QCOM_SMD=y > CONFIG_QCOM_SMD_RPM=y > +CONFIG_QCOM_SMP2P=y We also need CONFIG_QCOM_SMSM=y here, its currently used to signal state of the ring buffers for WiFi. With the addition of that you have my: Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org> Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 03, 2017 at 05:20:32PM -0800, Bjorn Andersson wrote: > On Tue 14 Feb 20:51 PST 2017, Jonathan Neusch?fer wrote: > > > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> > > --- > > arch/arm/configs/qcom_defconfig | 5 +++++ > > 1 file changed, 5 insertions(+) [...] > > +CONFIG_QCOM_SMP2P=y > > We also need CONFIG_QCOM_SMSM=y here, its currently used to signal state > of the ring buffers for WiFi. FWIW, I enabled CONFIG_QCOM_SMSM on my test system (an Asus Padfone, based on MSM8974; I'm using the Sony Xperia Honami DT because it's close enough), and I think it failed to initialize: [ 0.647743] qcom-smsm smsm: no smsm size info, using defaults [ 0.647775] qcom-smsm smsm: unable to allocate shared state entry I think CONFIG_QCOM_WCNSS_CTRL may be needed too, but I'll leave that for a future patch because I don't understand WCNSS well enough. > With the addition of that you have my: > > Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org> I'll send a v2 of this series with your R-b and A-b tags. Jonathan Neuschäfer
On Sun 05 Mar 05:01 GMT 2017, Jonathan Neusch?fer wrote: > On Fri, Mar 03, 2017 at 05:20:32PM -0800, Bjorn Andersson wrote: > > On Tue 14 Feb 20:51 PST 2017, Jonathan Neusch?fer wrote: > > > > > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> > > > --- > > > arch/arm/configs/qcom_defconfig | 5 +++++ > > > 1 file changed, 5 insertions(+) > [...] > > > +CONFIG_QCOM_SMP2P=y > > > > We also need CONFIG_QCOM_SMSM=y here, its currently used to signal state > > of the ring buffers for WiFi. > > FWIW, I enabled CONFIG_QCOM_SMSM on my test system (an Asus Padfone, > based on MSM8974; I'm using the Sony Xperia Honami DT because it's close > enough), and I think it failed to initialize: > Using Honami should work so far, but please do write a patch adding the Padfone, so that we don't accidentally break your HW at some point. > [ 0.647743] qcom-smsm smsm: no smsm size info, using defaults > [ 0.647775] qcom-smsm smsm: unable to allocate shared state entry > Could you please confirm where in qcom_smem_alloc_global() we're failing? As far as I can tell we should fail with -EEXIST or if the passed "size" parameter is bogus -ENOMEM (but the default number of entries really should be less than the amount of free SMEM space). > I think CONFIG_QCOM_WCNSS_CTRL may be needed too, but I'll leave that > for a future patch because I don't understand WCNSS well enough. > Missed that one, when the WCNSS firmware boots the WCNSS_CTRL driver is probed - it will upload the NV parameter file to the WCNSS "OS" and when that is done it will probe the WiFi and BT drivers. So, you need it as well. > > With the addition of that you have my: > > > > Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > I'll send a v2 of this series with your R-b and A-b tags. > Thanks, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Mar 05, 2017 at 04:49:30PM +0000, Bjorn Andersson wrote: > On Sun 05 Mar 05:01 GMT 2017, Jonathan Neusch?fer wrote: [...] > > FWIW, I enabled CONFIG_QCOM_SMSM on my test system (an Asus Padfone, > > based on MSM8974; I'm using the Sony Xperia Honami DT because it's close > > enough), and I think it failed to initialize: > > > > Using Honami should work so far, but please do write a patch adding the > Padfone, so that we don't accidentally break your HW at some point. I can try how far I get, but unfortunately I don't have hardware documentation or schematics, because I'm a hobbyist. > > [ 0.647743] qcom-smsm smsm: no smsm size info, using defaults > > [ 0.647775] qcom-smsm smsm: unable to allocate shared state entry > > > > Could you please confirm where in qcom_smem_alloc_global() we're > failing? As far as I can tell we should fail with -EEXIST or if the > passed "size" parameter is bogus -ENOMEM (but the default number of > entries really should be less than the amount of free SMEM space). * qcom_smem_get returns -EPROBE_DEFER: void *ptr = ERR_PTR(-EPROBE_DEFER); if (!__smem) return ptr; * smsm_get_size_info prints "no smsm size info, using defaults\n" * qcom_smem_alloc also returns -EPROBE_DEFER early. BTW, I think smsm_get_size_info is using uninitialized memory here: size_t size; /* is uninitialized */ struct { ... } *info; /* qcom_smem_get returns early without setting size */ info = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_SMSM_SIZE_INFO, &size); /* * PTR_ERR(info) is not -ENOENT. * size (still uninitialized) is compared with the size of the local * struct defined above. */ if (PTR_ERR(info) == -ENOENT || size != sizeof(*info)) { ... } > > I think CONFIG_QCOM_WCNSS_CTRL may be needed too, but I'll leave that > > for a future patch because I don't understand WCNSS well enough. > > > > Missed that one, when the WCNSS firmware boots the WCNSS_CTRL driver is > probed - it will upload the NV parameter file to the WCNSS "OS" and when > that is done it will probe the WiFi and BT drivers. > > So, you need it as well. Ok, I'll add it to this patch. Jonathan Neuschäfer
On Tue 07 Mar 03:01 CET 2017, Jonathan Neusch?fer wrote: > On Sun, Mar 05, 2017 at 04:49:30PM +0000, Bjorn Andersson wrote: > > On Sun 05 Mar 05:01 GMT 2017, Jonathan Neusch?fer wrote: > [...] > > > FWIW, I enabled CONFIG_QCOM_SMSM on my test system (an Asus Padfone, > > > based on MSM8974; I'm using the Sony Xperia Honami DT because it's close > > > enough), and I think it failed to initialize: > > > > > > > Using Honami should work so far, but please do write a patch adding the > > Padfone, so that we don't accidentally break your HW at some point. > > I can try how far I get, but unfortunately I don't have hardware > documentation or schematics, because I'm a hobbyist. > If this is something that you will continue to hack on and you think anyone else will be interested in having then please do submit patches for it. > > > [ 0.647743] qcom-smsm smsm: no smsm size info, using defaults > > > [ 0.647775] qcom-smsm smsm: unable to allocate shared state entry > > > > > > > Could you please confirm where in qcom_smem_alloc_global() we're > > failing? As far as I can tell we should fail with -EEXIST or if the > > passed "size" parameter is bogus -ENOMEM (but the default number of > > entries really should be less than the amount of free SMEM space). > > * qcom_smem_get returns -EPROBE_DEFER: > > void *ptr = ERR_PTR(-EPROBE_DEFER); > if (!__smem) > return ptr; > > * smsm_get_size_info prints "no smsm size info, using defaults\n" > * qcom_smem_alloc also returns -EPROBE_DEFER early. > > > BTW, I think smsm_get_size_info is using uninitialized memory here: > > size_t size; /* is uninitialized */ > struct { ... } *info; > > /* qcom_smem_get returns early without setting size */ > info = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_SMSM_SIZE_INFO, &size); > > /* > * PTR_ERR(info) is not -ENOENT. > * size (still uninitialized) is compared with the size of the local > * struct defined above. > */ > if (PTR_ERR(info) == -ENOENT || size != sizeof(*info)) { > ... > } > Thanks for your analysis! As you say the smsm driver is missing handling of probe deferral - which is wrong. Could you please propose a patch checking for EPROBE_DEFER and propagate this error as return value from probe()? (Without an error message) Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 07, 2017 at 12:04:13PM +0100, Bjorn Andersson wrote: > On Tue 07 Mar 03:01 CET 2017, Jonathan Neusch?fer wrote: [...] > If this is something that you will continue to hack on and you think > anyone else will be interested in having then please do submit patches > for it. Ok, I'll give it a try. > > > > [ 0.647743] qcom-smsm smsm: no smsm size info, using defaults > > > > [ 0.647775] qcom-smsm smsm: unable to allocate shared state entry > > > > > > > > > > Could you please confirm where in qcom_smem_alloc_global() we're > > > failing? As far as I can tell we should fail with -EEXIST or if the > > > passed "size" parameter is bogus -ENOMEM (but the default number of > > > entries really should be less than the amount of free SMEM space). > > > > * qcom_smem_get returns -EPROBE_DEFER: > > > > void *ptr = ERR_PTR(-EPROBE_DEFER); > > if (!__smem) > > return ptr; > > > > * smsm_get_size_info prints "no smsm size info, using defaults\n" > > * qcom_smem_alloc also returns -EPROBE_DEFER early. > > > > > > BTW, I think smsm_get_size_info is using uninitialized memory here: > > > > size_t size; /* is uninitialized */ > > struct { ... } *info; > > > > /* qcom_smem_get returns early without setting size */ > > info = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_SMSM_SIZE_INFO, &size); > > > > /* > > * PTR_ERR(info) is not -ENOENT. > > * size (still uninitialized) is compared with the size of the local > > * struct defined above. > > */ > > if (PTR_ERR(info) == -ENOENT || size != sizeof(*info)) { > > ... > > } > > > > Thanks for your analysis! > > As you say the smsm driver is missing handling of probe deferral - which > is wrong. Could you please propose a patch checking for EPROBE_DEFER and > propagate this error as return value from probe()? (Without an error > message) I'm working on it. Regarding the uninitialized memory read in "size != sizeof(*info)": Is qcom_smem_get supposed to store a valid size value in any non-probe-deferral case, or only in non-error cases? Assuming the latter, I think "size != sizeof(*info)" should be changed to "!IS_ERR(info) && size != sizeof(*info)", i.e. only check size if qcom_smem_get returned success. Does that seem reasonable? Jonathan Neuschäfer
diff --git a/arch/arm/configs/qcom_defconfig b/arch/arm/configs/qcom_defconfig index 4ffdd607205d..97b9e9cd2292 100644 --- a/arch/arm/configs/qcom_defconfig +++ b/arch/arm/configs/qcom_defconfig @@ -190,11 +190,16 @@ CONFIG_MDM_LCC_9615=y CONFIG_MSM_MMCC_8960=y CONFIG_MSM_MMCC_8974=y CONFIG_HWSPINLOCK_QCOM=y +CONFIG_REMOTEPROC=y +CONFIG_QCOM_ADSP_PIL=y +CONFIG_QCOM_Q6V5_PIL=y +CONFIG_QCOM_WCNSS_PIL=y CONFIG_QCOM_GSBI=y CONFIG_QCOM_PM=y CONFIG_QCOM_SMEM=y CONFIG_QCOM_SMD=y CONFIG_QCOM_SMD_RPM=y +CONFIG_QCOM_SMP2P=y CONFIG_IIO=y CONFIG_IIO_BUFFER_CB=y CONFIG_IIO_SW_TRIGGER=y
Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> --- arch/arm/configs/qcom_defconfig | 5 +++++ 1 file changed, 5 insertions(+)