linux-kernel - Re: [PATCH] soc: qcom: Add SoC info 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]

Message-ID: <2349770.MVMXr64OoI@wuerfel>
Date:   Fri, 21 Oct 2016 12:33:40 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Imran Khan <kimran@...eaurora.org>
Cc:     andy.gross@...aro.org, David Brown <david.brown@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        "open list:ARM/QUALCOMM SUPPORT" <linux-arm-msm@...r.kernel.org>,
        "open list:ARM/QUALCOMM SUPPORT" <linux-soc@...r.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] soc: qcom: Add SoC info driver

On Thursday, October 20, 2016 7:36:22 PM CEST Imran Khan wrote:
> +#include <linux/soc/qcom/socinfo.h>
> +#include <linux/soc/qcom/smem.h>
> +
> +#include <asm/system_misc.h>

I don't see anything here that needs asm/system_misc.h

> +const char *hw_platform[] = {
> +	[HW_PLATFORM_UNKNOWN] = "Unknown",
> +	[HW_PLATFORM_SURF] = "Surf",
> +	[HW_PLATFORM_FFA] = "FFA",
> +	[HW_PLATFORM_FLUID] = "Fluid",
> +};
> +
> +const char *qrd_hw_platform_subtype[] = {
> +	[PLATFORM_SUBTYPE_QRD] = "QRD",
> +	[PLATFORM_SUBTYPE_SKUAA] = "SKUAA",
> +};
> +
> +const char *hw_platform_subtype[] = {
> +	[PLATFORM_SUBTYPE_UNKNOWN] = "Unknown",
> +};


Make these all static

> +
> +/* Used to parse shared memory. Must match the modem. */
> +struct socinfo_v0_1 {
> +	uint32_t format;
> +	uint32_t id;
> +	uint32_t version;

s/uint32_t/u32/

> +
> +uint32_t socinfo_get_id(void)
> +{
> +	return (socinfo) ? socinfo->v0_1.id : 0;
> +}
> +EXPORT_SYMBOL_GPL(socinfo_get_id);
> +
> +uint32_t socinfo_get_version(void)
> +{
> +	return (socinfo) ? socinfo->v0_1.version : 0;
> +}
> +EXPORT_SYMBOL_GPL(socinfo_get_version);
> +
> +char *socinfo_get_build_id(void)
> +{
> +	return (socinfo) ? socinfo->v0_1.build_id : NULL;
> +}
> +EXPORT_SYMBOL_GPL(socinfo_get_build_id);

Please remove all the exports and mark the functions static,
we don't want any drivers poking vendor-specific interfaces
for this.

> +/* socinfo: sysfs functions */

This seems overly verbose, having both raw and human-readable
IDs is generally not necessary, pick one of the two. If you
need any fields that we don't already support in soc_device,
let's talk about adding them to the generic structure.


> +static ssize_t
> +qcom_get_image_version(struct device *dev,
> +			struct device_attribute *attr,
> +			char *buf)
> +{
> +	char *string_address;
> +
> +	string_address = socinfo_get_image_version_base_address(dev);
> +	if (IS_ERR_OR_NULL(string_address)) {

IS_ERR_OR_NULL() indicates that your interface is not well-designed.
Please return either NULL on all failures, or return an error pointer
on all failures, not both.

> +/* Platform Subtype String is being deprecated. Use Platform
> + * Subtype ID instead.
> + */

If it's deprecated, don't add it to the kernel!


> diff --git a/include/linux/soc/qcom/socinfo.h b/include/linux/soc/qcom/socinfo.h
> new file mode 100644
> index 0000000..17ca50a
> --- /dev/null
> +++ b/include/linux/soc/qcom/socinfo.h

Merge the header file into the driver.

	Arnd