Date:   Mon, 12 Dec 2016 20:44:48 +0530
From:   Imran Khan <kimran@...eaurora.org>
To:     Bjorn Andersson <bjorn.andersson@...aro.org>
Cc:     andy.gross@...aro.org, lee.jones@...aro.org,
        David Brown <david.brown@...aro.org>,
        open list <linux-kernel@...r.kernel.org>,
        "open list:ARM/QUALCOMM SUPPORT" <linux-arm-msm@...r.kernel.org>,
        "open list:ARM/QUALCOMM SUPPORT" <linux-soc@...r.kernel.org>
Subject: Re: [PATCH v5] soc: qcom: Add SoC info driver

On 12/5/2016 11:35 PM, Bjorn Andersson wrote:
> On Wed 16 Nov 05:22 PST 2016, Imran Khan wrote:
> 
<snip>
>>  
>> diff --git a/drivers/soc/qcom/socinfo.c b/drivers/soc/qcom/socinfo.c
>> new file mode 100644
>> index 0000000..eca46df
>> --- /dev/null
>> +++ b/drivers/soc/qcom/socinfo.c
>> @@ -0,0 +1,989 @@
>> +/*
>> + * Copyright (c) 2009-2016, The Linux Foundation. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + */
>> +
>> +#include <linux/export.h>
>> +#include <linux/module.h>
>> +#include <linux/err.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/sys_soc.h>
>> +#include <linux/slab.h>
>> +#include <linux/stat.h>
>> +#include <linux/string.h>
>> +#include <linux/types.h>
>> +#include <linux/random.h>
>> +#include <linux/soc/qcom/smem.h>
>> +
>> +#define PMIC_MODEL_UNKNOWN 0
>> +#define HW_PLATFORM_QRD 11
>> +#define PLATFORM_SUBTYPE_QRD_INVALID 6
>> +#define PLATFORM_SUBTYPE_INVALID 4
>> +/*
>> + * SOC version type with major number in the upper 16 bits and minor
>> + * number in the lower 16 bits.  For example:
>> + *   1.0 -> 0x00010000
>> + *   2.3 -> 0x00020003
>> + */
>> +#define SOC_VERSION_MAJOR(ver) (((ver) & 0xffff0000) >> 16)
>> +#define SOC_VERSION_MINOR(ver) ((ver) & 0x0000ffff)
>> +#define SOCINFO_FORMAT(x)  (x)
>> +#define SOCINFO_VERSION_MAJOR	SOC_VERSION_MAJOR
>> +#define SOCINFO_VERSION_MINOR	SOC_VERSION_MINOR
>> +
>> +#define SMEM_SOCINFO_BUILD_ID_LENGTH 32
>> +#define SMEM_IMAGE_VERSION_BLOCKS_COUNT 32
>> +#define SMEM_IMAGE_VERSION_SIZE 4096
>> +#define SMEM_IMAGE_VERSION_NAME_SIZE 75
>> +#define SMEM_IMAGE_VERSION_VARIANT_SIZE 20
>> +#define SMEM_IMAGE_VERSION_OEM_SIZE 32
>> +#define SMEM_IMAGE_VERSION_PARTITION_APPS 10
> 
> Please indent the values evenly, making this slightly easier to read.
> 
Done.

>> +
>> +/*
>> + * Shared memory identifiers, used to acquire handles to respective memory
>> + * region.
>> + */
>> +#define SMEM_IMAGE_VERSION_TABLE	469
>> +
>> +/*
>> + * Shared memory identifiers, used to acquire handles to respective memory
>> + * region.
>> + */
> 
> Duplicating this comment doesn't add value, please just drop this second
> copy and replace the above comment with something like "SMEM item ids"
> 
Done.
>> +#define SMEM_HW_SW_BUILD_ID		137
>> +
>> +/*
>> + * SMEM Image table indices
>> + */
<snip>
> 
> Rather than having this based on two huge macros take a look at
> DEVICE_INT_ATTR() and struct dev_ext_attribute in
> include/linux/device.h. It looks like if you just put the index in a
> struct and use container_of to reach that you can replace these with
> direct functions.
> 
> Also, it's perfectly fine to always specify a store operation and use
> 0444 vs 0644 to control it's availability. So you don't need two sets if
> you just expose the mode in your macro.
>

If I understood this correct, the main idea here is to use dev_ext_attribute
and avoid wrapper macros. Have tried to implement this suggestion in the 
subsequent patch set. Please let me know if it looks okay or can be improved
further. 
 
>> +
>> +/*
>> + * Qcom SoC types
>> + */
>> +enum qcom_cpu {
>> +	MSM_CPU_UNKNOWN = 0,
>> +	MSM_CPU_8960,
>> +	MSM_CPU_8960AB,
>> +	MSM_CPU_8064,
>> +	MSM_CPU_8974,
>> +	MSM_CPU_8974PRO_AA,
>> +	MSM_CPU_8974PRO_AB,
>> +	MSM_CPU_8974PRO_AC,
>> +	MSM_CPU_8916,
>> +	MSM_CPU_8084,
>> +	MSM_CPU_8996
>> +};
> 
> These are now "unused" (per below comment).
> 

Done. As these are no more needed.
>> +
>> +struct qcom_soc_info {
>> +	enum qcom_cpu generic_soc_type;
> 
> This enum is no longer used, seems like you can turn cpu_of_id into a
> simple char *[] array with with soc_id_string indexed by id.
> 
True. Have implemented cpu_of_id as per this suggestion.

>> +	char *soc_id_string;
>> +};
>> +
<snip>
>> +
>> +static struct smem_image_version *smem_image_version;
>> +static bool smem_img_tbl_avlbl = true;
> 
> You don't need to keep track of this separately, just test if
> smem_image_version is NULL or not (reset it to NULL if qcom_smem_get()
> fails).
> 
Done.

>> +static bool hw_subtype_valid = true;
>> +static u32 hw_subtype;
> 
> Don't keep track of this in a global variable, it's perfectly fine to
> figure it out in qcom_get_platform_subtype()
> 
Done.
>> +
>> +
>> +/* max socinfo format version supported */
>> +#define MAX_SOCINFO_FORMAT SOCINFO_FORMAT(12)
> 
> It's fine to drop SOCINFO_FORMAT and just put 12 here.
> 
Done.
>> +
>> +static struct qcom_soc_info cpu_of_id[] = {
>> +
>> +	[0] = {MSM_CPU_UNKNOWN, "Unknown CPU"},
>> +
<snip>
>> +static u32 socinfo_format;
>> +
>> +static u32 socinfo_get_id(void);
>> +
> 
> Merge socinfo_init_sysfs() and qcom_socinfo_init() into one function and
> you don't need to declare this here.
>
Done.
 
>> +/* socinfo: sysfs functions */
>> +
>> +static char *socinfo_get_id_string(void)
>> +{
>> +	return (socinfo) ? cpu_of_id[socinfo->v0_1.id].soc_id_string : NULL;
> 
> socinfo can't be NULL anymore.
> 
True. This checking is redundant indeed. In the subsequent patch set I
have removed redundant checking of socinfo being NULL or not.

>> +}
>> +
>> +static char *socinfo_get_image_version_base_address(struct device *dev)
> 
> Inline this into qcom_socinfo_init() as described below.
> 
Done.

>> +{
>> +	size_t size, size_in;
>> +	void *ptr;
>> +
>> +	ptr = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_IMAGE_VERSION_TABLE,
>> +			&size);
>> +	if (IS_ERR(ptr))
>> +		return ptr;
>> +
>> +	size_in = SMEM_IMAGE_VERSION_SIZE;
>> +	if (size_in != size) {
>> +		dev_err(dev, "Wrong size for smem item\n");
>> +		return ERR_PTR(-EINVAL);
>> +	}
>> +
>> +	return ptr;
>> +}
>> +
>> +
>> +static u32 socinfo_get_version(void)
>> +{
>> +	return (socinfo) ? socinfo->v0_1.version : 0;
> 
> socinfo can't be NULL, so you can just inline "socinfo->v0_1.version" in
> the one place calling this.
> 
Done.

>> +}
>> +
>> +static char *socinfo_get_build_id(void)
>> +{
>> +	return (socinfo) ? socinfo->v0_1.build_id : NULL;
> 
> socinfo can't be NULL, and this is only called from one place, so inline
> "socinfo->v0_1.build_id" there.
> 
Done.
>> +}
>> +
>> +static u32 socinfo_get_raw_version(void)
>> +{
>> +	return socinfo ?
> 
> socinfo can't be NULL.
> 
>> +		(socinfo_format >= SOCINFO_FORMAT(2) ?
> 
As mentioned above, have taken care of this.

> qcom_soc_attr_raw_version will not be created unless socinfo_format >=
> 2, so this can't be called if socinfo_format < 2, so you don't need this
> extra check.
> 
>> +			socinfo->v0_2.raw_version : 0)
>> +		: 0;
> 
> And as this is then simplified to "return socinfo->v0_2.raw_version;" I
> think you should just inline it in the one place calling this function.
> 
Done. Have removed redundant checking of socinfo format version from 
this and other functions and also have made these functions as much inline
as possible.

>> +}
>> +
>> +static u32 socinfo_get_platform_type(void)
>> +{
>> +	return socinfo ?
>> +		(socinfo_format >= SOCINFO_FORMAT(3) ?
>> +			socinfo->v0_3.hw_platform : 0)
>> +		: 0;
>> +}
<snip>

>> +
>> +static ssize_t
>> +qcom_get_raw_version(struct device *dev,
>> +		     struct device_attribute *attr,
>> +		     char *buf)
> 
> This is the "show" function of an attr, so please drop the "get" and add
> "_show".
> 
Done. Have renamed attr show and store functions properly.
>> +{
>> +	return sprintf(buf, "%u\n",
>> +		socinfo_get_raw_version());
> 
> And please inline as much as you can of these, there's little point in
> having a helper function to wrap a single line accessing the element in
> the socinfo struct.
> 

Done. Have tried this but please let me know if you see any further scope
in this regard.

>> +}
>> +
>> +static ssize_t
>> +qcom_get_build_id(struct device *dev,
>> +		   struct device_attribute *attr,
>> +		   char *buf)
>> +{
>> +	return scnprintf(buf, SMEM_SOCINFO_BUILD_ID_LENGTH, "%s\n",
>> +				socinfo_get_build_id());
>> +}
>> +
<snip>
>> +
>> +static ssize_t
>> +qcom_get_pmic_die_revision(struct device *dev,
>> +			       struct device_attribute *attr,
>> +			       char *buf)
>> +{
>> +	return sprintf(buf, "%u\n", socinfo_get_pmic_die_revision());
>> +}
>> +
>> +static ssize_t
>> +qcom_get_image_version(int index, char *buf)
>> +{
> 
> Just inline these into the version show() and store() functions.  And
> whenever you copy to the sysfs buffer, it's of size PAGE_SIZE.
> 
Done.
>> +	return scnprintf(buf, SMEM_IMAGE_VERSION_NAME_SIZE, "%s\n",
>> +			smem_image_version[index].name);
>> +}
>> +
<snip>
>> +static ssize_t
>> +qcom_set_image_crm_version(int index, const char *buf)
>> +{
>> +	return strlcpy(smem_image_version[index].oem, buf,
>> +			SMEM_IMAGE_VERSION_OEM_SIZE);
>> +}
>> +
>> +static struct device_attribute qcom_soc_attr_raw_version =
>> +	__ATTR(raw_version, 0444, qcom_get_raw_version,  NULL);
> 
> Rather than writing all these out here you can put them in an array of:
> 
> struct qcom_socinfo_attr {
> 	struct device_attribute attr;
> 	int min_version;
> };
> 
> #define QCOM_SOCINFO_ATTR(_name, _show, _min_version) \
> 	{ __ATTR(_name, 0444, _show, NULL), _min_version) }
> 
> static const struct qcom_socinfo_attr qcom_socinfo_attrs[] = {
> 	QCOM_SOCINFO_ATTR(raw_version, qcom_get_raw_version, 2),
> };
> 
> And then socinfo_populate_sysfs_files() will be a matter of looping over
> this array and comparing min_version and registering the attr.
>
Done. This makes the implementation really neat and concise. Thanks for
this suggestion.
 
>> +
>> +static struct device_attribute qcom_soc_attr_vendor =
>> +	__ATTR(vendor, 0444, qcom_get_vendor,  NULL);
>> +
>> +static struct device_attribute qcom_soc_attr_build_id =
>> +	__ATTR(build_id, 0444, qcom_get_build_id, NULL);
<snip>
>> +	/*
>> +	 * Expose SMEM_IMAGE_TABLE to sysfs only when we have IMAGE_TABLE
>> +	 * available in SMEM. As IMAGE_TABLE and SOCINFO are two separate
>> +	 * items within SMEM, we expose the remaining soc information(i.e
>> +	 * only the SOCINFO item available in SMEM) to sysfs even in the
>> +	 * absence of an IMAGE_TABLE.
>> +	 */
>> +	if (smem_img_tbl_avlbl) {
>> +		for (img_idx = 0; img_idx < SMEM_IMAGE_VERSION_BLOCKS_COUNT;
>> +				img_idx++) {
>> +			if (smem_image_table[img_idx]) {
>> +				err = sysfs_create_group(&qcom_soc_device->kobj,
>> +					smem_image_table[img_idx]);
>> +				if (err)
>> +					break;
> 
> Should we just ignore this error?
> 
Yeah. I think we can and should ignore this error. Failing to create sysfs entry
for one item in image table should not deprive other SMEM image items.
>> +			}
>> +		}
>> +	}
>> +	err = device_create_file(qcom_soc_device, &qcom_soc_attr_vendor);
>> +	if (!err) {
> 
> Flip this conditional around and handle the error here, it saves you one
> indentation level for the rest of this function.
> 
Done. No more needed in new implementation.

>> +		switch (socinfo_format) {
>> +		case SOCINFO_FORMAT(12):
>> +			err = device_create_file(qcom_soc_device,
>> +					&qcom_soc_attr_chip_family);
>> +			if (!err)
>> +				err = device_create_file(qcom_soc_device,
>> +					&qcom_soc_attr_raw_device_family);
>> +			if (!err)
>> +				err = device_create_file(qcom_soc_device,
>> +					&qcom_soc_attr_raw_device_number);
<snip>
>> +			return 0;
>> +		default:
>> +			dev_err(qcom_soc_device, "Unknown socinfo format: v0.%u\n",
>> +					SOCINFO_VERSION_MINOR(socinfo_format));
>> +			return -EINVAL;
>> +		}
>> +	} else {
>> +		dev_err(qcom_soc_device, "Could not create sysfs entry\n");
>> +		return err;
>> +	}
>> +}
>> +
>> +static int socinfo_init_sysfs(struct device *dev)
> 
> This device * is specifically the smem device, so if you name it
> "smem_dev"...
> 
Done.
>> +{
>> +	struct device *qcom_soc_device;
> 
> ...then you can name this "dev" to shorten things.
>
Done.
 
>> +	struct soc_device *soc_dev;
>> +	struct soc_device_attribute *soc_dev_attr;
> 
> "attr" would be long enough.
>
Done.
 
>> +	u32 soc_version;
>> +
>> +	soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
>> +	if (!soc_dev_attr)
>> +		return -ENOMEM;
>> +
>> +	soc_version = socinfo_get_version();
>> +
>> +	soc_dev_attr->soc_id = kasprintf(GFP_KERNEL, "%d", socinfo_get_id());
>> +	soc_dev_attr->family = "Snapdragon";
>> +	soc_dev_attr->machine = socinfo_get_id_string();
> 
> Single use of socinfo_get_id_string() so you should just go:
> 
> unsigned int id = socinfo->v0_1.id;
> 
> and then use that for your soc_id print and use soc_id_string[id] here.
> 
Done.
>> +	soc_dev_attr->revision = kasprintf(GFP_KERNEL, "%u.%u",
>> +				SOC_VERSION_MAJOR(soc_version),
>> +				SOC_VERSION_MINOR(soc_version));
>> +
>> +	soc_dev = soc_device_register(soc_dev_attr);
>> +	if (IS_ERR(soc_dev)) {
>> +		kfree(soc_dev_attr);
>> +		return PTR_ERR(soc_dev);
>> +	}
>> +
>> +	qcom_soc_device = soc_device_to_device(soc_dev);
>> +	socinfo_populate_sysfs_files(qcom_soc_device);
>> +	return 0;
>> +}
>> +
>> +static u32 socinfo_get_id(void)
>> +{
>> +	return (socinfo) ? socinfo->v0_1.id : 0;
> 
> As we no longer expose the socinfo functions to external callers this
> will only ever be called if socinfo_init() succeeded, so no need to
> check for socinfo being NULL.
> 
Done.
>> +}
>> +
>> +static int socinfo_select_format(struct device *dev)
>> +{
>> +	u32 f_maj = SOCINFO_VERSION_MAJOR(socinfo->v0_1.format);
>> +	u32 f_min = SOCINFO_VERSION_MINOR(socinfo->v0_1.format);
>> +
>> +	if (f_maj != 0) {
>> +		dev_err(dev, "Unsupported format v%u.%u.\n",
>> +			f_maj, f_min);
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (socinfo->v0_1.format > MAX_SOCINFO_FORMAT) {
>> +		dev_warn(dev, "Unsupported format v%u.%u. Falling back to v%u.%u.\n",
>> +			f_maj, f_min, SOCINFO_VERSION_MAJOR(MAX_SOCINFO_FORMAT),
>> +			SOCINFO_VERSION_MINOR(MAX_SOCINFO_FORMAT));
>> +		socinfo_format = MAX_SOCINFO_FORMAT;
> 
> Don't fall back, just print an error and fail.
> 
Done.
>> +	} else {
>> +		socinfo_format = socinfo->v0_1.format;
>> +	}
>> +	return 0;
>> +}
>> +
>> +int qcom_socinfo_init(struct platform_device *pdev)
> 
> As smem doesn't care if we're failing here you can just have the return
> type void.
> 
Done.
>> +{
>> +	size_t size;
>> +
>> +	socinfo = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_HW_SW_BUILD_ID,
>> +			&size);
>> +	if (IS_ERR(socinfo) || (socinfo_select_format(&pdev->dev) < 0)) {
>> +		dev_warn(&pdev->dev,
>> +			"Coudn't find soc information; Unable to setup socinfo.\n");
>> +		return -ENOMEM;
> 
> "Couldn't find socinfo" and return -ENOENT if IS_ERR(socinfo).
> 
Done
>> +	}
> 
> And then just move the rest of socinfo_select_format() here:
> 
> 	if (socinfo->v0.1.format > MAX_SOCINFO_FORMAT) {
> 		dev_err(&pdev->dev, "Invalid socinfo version %d.%d\n",
> 			MAJOR(), MINOR());
> 		return -EINVAL;
> 	}
> 
Done
>> +
>> +	if (!socinfo_get_id()) {
>> +		dev_err(&pdev->dev, "socinfo: Unknown SoC ID!\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	WARN(socinfo_get_id() >= ARRAY_SIZE(cpu_of_id),
>> +		"New IDs added! ID => CPU mapping needs an update.\n");
>> +
>> +	smem_image_version = (struct smem_image_version *)
>> +			socinfo_get_image_version_base_address(&pdev->dev);
> 
> You could make socinfo_get_image_version_base_address() return void *
> and you don't have to do the type cast. But you can make this more
> compact by just inlining the qcom_smem_get() here directly.
> 
> 	smem_image_version = qcom_smem_get(QCOM_SMEM_HOST_ANY,
> 					   SMEM_IMAGE_VERSION_TABLE,
> 					   &size);
> 	if (IS_ERR(smem_image_version) || size != SMEM_IMAGE_VERSION_SIZE) {
> 		...
> 	}
> 

Done
>> +	if (IS_ERR(smem_image_version)) {
>> +		dev_warn(&pdev->dev, "Unable to get address for image version table.\n");
> 
> "Image version table missing" should keep you < 80 characters.
> 
Done.
>> +		smem_img_tbl_avlbl = false;
>> +	}
>> +
>> +	hw_subtype = socinfo_get_platform_subtype();
>> +	if (socinfo_get_platform_type() == HW_PLATFORM_QRD) {
>> +		if (hw_subtype >= PLATFORM_SUBTYPE_QRD_INVALID) {
>> +			dev_err(&pdev->dev, "Invalid hardware platform sub type for qrd found\n");
>> +			hw_subtype_valid = false;
>> +		}
>> +	} else {
>> +		if (hw_subtype >= PLATFORM_SUBTYPE_INVALID) {
>> +			dev_err(&pdev->dev, "Invalid hardware platform subtype\n");
>> +			hw_subtype_valid = false;
>> +		}
>> +	}
> 
> This will only matter if format >= 6 and hw_subtype is outside the array
> of strings, I think you should just be optimistic about this not being
> an issue and then in all cases where you dereference a lookup table
> check that you're within bounds, or fail with -EINVAL; to userspace.
> 
True. Have taken care of this. No more tracking this with a global variable.
>> +
>> +	socinfo_init_sysfs(&pdev->dev);
> 
> socinfo_init_sysfs() is just the second half of this function, so inline
> it here.
> 
Done. now this is inline.
>> +
>> +	/* Feed the soc specific unique data into entropy pool */
>> +	add_device_randomness(socinfo, size);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(qcom_socinfo_init);
>> +
> 
> No empty lines at the end of the file, please.
> 
Done.
> Regards,
> Bjorn
> 
Thanks Bjorn for review comments.
Regards,
Imran

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a\nmember of the Code Aurora Forum, hosted by The Linux Foundation

Powered by blists - more mailing lists