Message-ID: <20161107193514.GC25787@tuxbot>
Date:   Mon, 7 Nov 2016 11:35:14 -0800
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     Imran Khan <kimran@...eaurora.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 v3] soc: qcom: Add SoC info driver

On Mon 07 Nov 06:35 PST 2016, Imran Khan wrote:

> On 11/3/2016 12:00 AM, Bjorn Andersson wrote:
> Thanks Bjorn for your review comments. I have taken into account
> most of these in the subsequent patch set. For a few of the comments
> I want to discuss further, so please see the same inline and 
> let me know your feedback about the same.
> 
> > On Wed 02 Nov 03:06 PDT 2016, Imran Khan wrote:
[..]
> >> +#define SMEM_IMAGE_VERSION_NAME_SIZE 75
> >> +#define SMEM_IMAGE_VERSION_VARIANT_SIZE 20
> >> +#define SMEM_IMAGE_VERSION_VARIANT_OFFSET 75
> >> +#define SMEM_IMAGE_VERSION_OEM_SIZE 32
> >> +#define SMEM_IMAGE_VERSION_OEM_OFFSET 96
> > 
> > Please throw these into a:
> > 
> > struct image_version {
> > 	char name[75];
> > 	char variant[20];
> > 	char pad;
> > 	char oem[32];
> > };
> > 
> > This allows you to describe the version SMEM item just as an array of
> > SMEM_IMAGE_VERSION_BLOCKS_COUNT image_version objects - i.e. the
> > compiler will take care of doing the math for you.
> >
> 
> Okay done. Although I have kept the macros to describe
> length of various members of image_version object.
>  

That sounds wise.

[..]
> >> +static void socinfo_populate_sysfs_files(struct device *qcom_soc_device)
> >> +{
> >> +	device_create_file(qcom_soc_device, &qcom_soc_attr_vendor);
> > 
> > If you're keeping vendor it should be added to the generic
> > soc_device_attribute struct.
> >
> 
> Actually along with vendor, I also intend to keep serial number 
> and foundry_id in the generic soc_device_attribute structure 
> but as this part is still under discussion, so far I have kept
> these fields separate. Once these fileds gets accepted as 
> part of generic soc_device_attribute, I can modify this driver.
> I hope this approach is okay, if you have some alternative suggestion
> please let me know.
>  

Worst case we end up with a couple of warnings about the sysfs
collisions and we have to introduce the trivial patch that drop these
entries.

So I'm fine with your plan.

[..]

> >> +static void socinfo_populate(struct soc_device_attribute *soc_dev_attr)
> >> +{
> >> +	u32 soc_version = socinfo_get_version();
> >> +
> >> +	soc_dev_attr->soc_id   = kasprintf(GFP_KERNEL, "%d", socinfo_get_id());
> > 
> > I believe soc_id is supposed to be a human readable name; e.g. "MSM8996"
> > not "246".
> > 
> 
> I am not sure about this. I see other vendors also exposing soc_id as numeric value
> and machine is perhaps used for a human readable name. Please let me if I 
> am getting something wrong here.
> 

I'm slightly confused to what these various properties are supposed to
contain, according to Documentation/ABI/testing/sysfs-devices-soc soc_id
should contain the SoC serial number, while most implementations does
like you and put something telling which SoC it is.

246 is however not a useful number, as everyone reading it - be it human
or computer - will have to carry the translation table to figure out
what it actually says.

> >> +	soc_dev_attr->family  =  "Snapdragon";

I think family should be e.g. "MSM8996" and then machine should be e.g.
"MSM8996AU".

> >> +	soc_dev_attr->machine  = socinfo_get_id_string();
> > 
> > I think you're supposed to read the /model string from DT and put in
> > "machine".
> > 
> 
> Yes. We can read the machine string from DT also, but here we want to use
> the relevant information latched into SMEM as munch as possible so I went
> for reading this from socinfo. Please let me know if you see see some problem
> with this approach.
> 

That's ok with me.

> >> +	soc_dev_attr->revision = kasprintf(GFP_KERNEL, "%u.%u",
> >> +			SOCINFO_VERSION_MAJOR(soc_version),
> >> +			SOCINFO_VERSION_MINOR(soc_version));
> > 
> > "revision" is supposed to state the revision of the SoC, not of the
> > socinfo data.
> > 
> 
> Yes. It is indeed stating the revision of SoC. I guess the name of the
> macro is misleading here. Have corrected that in subsequent patch.
> 

I'm sorry, you're right.

> > 
> > Also, skip the indentation of the assignments in this section (it's not
> > consistent anyways).
> > 
> >> +	return;
> > 
> > No need to "return" here.
> >
> 
> Okay done.
>  
> >> +
> >> +}
> >> +
> >> +static int socinfo_init_sysfs(void)
> >> +{
> >> +	struct device *qcom_soc_device;
> >> +	struct soc_device *soc_dev;
> >> +	struct soc_device_attribute *soc_dev_attr;
> >> +
> >> +	soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
> >> +	if (!soc_dev_attr) {
> >> +		pr_err("Soc Device alloc failed!\n");
> > 
> > No need to print error message on kzalloc failures.
> > 
> 
> Okay done.
> 
> >> +		return -ENOMEM;
> >> +	}
> >> +
> >> +	socinfo_populate(soc_dev_attr);
> > 
> > Just inline socinfo_populate() here.
> > 
> >> +	soc_dev = soc_device_register(soc_dev_attr);
> >> +	if (IS_ERR(soc_dev)) {
> >> +		kfree(soc_dev_attr);
> >> +		pr_err("Soc device register failed\n");
> > 
> > I believe soc_device_register() will print something useful in the
> > various error paths, so you shouldn't need to add another print here.
> > 
> 
> Okay done.
> 
> >> +		return PTR_ERR(soc_dev);
> >> +	}
> >> +
> >> +	qcom_soc_device = soc_device_to_device(soc_dev);
> >> +	socinfo_populate_sysfs_files(qcom_soc_device);
> >> +	return 0;
> >> +}
> >> +#else
> <snip>
> >> +static void socinfo_print(void)
> >> +{
> > 
> > Does this function serve a purpose?
> > 
> > We already have an overwhelming amount of information (noise) being
> > thrown at us during boot.
> > 
> 
> As such this funtion does not serve any overly important purpose. It just prints
> socinfo related stuff in one line in boot logs which sometimes helps in identification
> of the platform while doing a debugging from dumps. 
> So I intend to keep it but if you see some issues please let me know, I can try to
> modify it.
> 

I presumed this was used for post mortem purposes. But how useful is
this? Doesn't the dump file come with enough information to identify the
platform you collected the dump on?

And when we enter download mode the SMEM items should be intact, so the
raw information should be available already - verbatim.


The kernel log is for human consumption, first and foremost, so if
you're printing anything it must make sense to humans.

> >> +	u32 f_maj = SOCINFO_VERSION_MAJOR(socinfo_format);
> >> +	u32 f_min = SOCINFO_VERSION_MINOR(socinfo_format);
> >> +	u32 v_maj = SOCINFO_VERSION_MAJOR(socinfo->v0_1.version);
> >> +	u32 v_min = SOCINFO_VERSION_MINOR(socinfo->v0_1.version);
> >> +
> >> +	switch (socinfo_format) {
> >> +	case SOCINFO_VERSION(0, 1):
> >> +		pr_info("v%u.%u, id=%u, ver=%u.%u\n",
> >> +			f_maj, f_min, socinfo->v0_1.id, v_maj, v_min);

This is neither nicely formatted (info logs should not be based on
__func__) nor useful to the average developer.

The reason why I'm objecting is that booting a CAF kernel gives me pages
and pages of cryptic debug messages, warning and error messages. But due
to the amount of logs no-one will actually read through the log and
correct things - and when you're actually looking for an error the
scroll-back buffer is too short.

So I object to this, based on the broken window theory.

[..]
> Thanks and Regards,

Thank you Imran, looking forward to the next version.

Regards,
Bjorn

Powered by blists - more mailing lists