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.

quoted

quoted

quoted

[    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