linux-kernel - Re: [PATCH v12 1/2] cpufreq: Add Kryo CPU scaling driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 24 May 2018 15:01:15 +0100
From:   Sudeep Holla <sudeep.holla@....com>
To:     ilialin@...eaurora.org, vireshk@...nel.org, nm@...com,
        sboyd@...nel.org, robh@...nel.org, mark.rutland@....com,
        rjw@...ysocki.net
Cc:     Sudeep Holla <sudeep.holla@....com>, linux-pm@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v12 1/2] cpufreq: Add Kryo CPU scaling driver

On 24/05/18 14:03, ilialin@...eaurora.org wrote:
> 
> 

[...]

>>> +
>>> +	ret = PTR_ERR_OR_ZERO(platform_device_register_simple(
>>> +		"qcom-cpufreq-kryo", -1, NULL, 0));
>>
>>
>> You simply can't do this unconditionally here. This will blow up on platforms
>> where this driver is not supposed to work. The probe will be called on non-
>> QCOM or non-Kryo QCOM platforms and I reckon it will crash trying to
>> execute something in qcom_smem_get.
> 
> What do you mean by 'unconditionally'?

Why should you even add/register a device "qcom-cpufreq-kryo" on other
platforms. Drivers can get registered, but only devices that are present
or required by the platform need to be registered.

> The driver depends on the smem and nvmem drivers, which depend on ARCH_QCOM:
>  +	depends on QCOM_QFPROM
>  +	depends on QCOM_SMEM
> 

Sure, but we have something called single image for all ARM64 platforms.
May be QCOM still used to tweeking config to build binary for your
particular mobile platform but the distro kernel need single binary to
work on all platforms. We have moved far away from platform specific
builds long back now IIRC.

> And if SMEM read in the probe returns something other than Kryo, it will exit.
> 

Check what this driver does ?

	msm_id = qcom_smem_get(QCOM_SMEM_HOST_ANY, MSM_ID_SMEM, &len);
	msm_id++;
	switch ((enum _msm_id)*msm_id)

I think it *will and should* crash here ? You need to check the return
value for sure. But since qcom_smem_get return -EPROBE_DEFER, we keep
retrying even on non QCOM platforms which is something I would like to
avoid.

Therefore that's not the main concern. Why do I have to see
"qcom-cpufreq-kryo" device registered on my non QCOM platform ?

-- 
Regards,
Sudeep