On 11/8/2016 1:05 AM, Bjorn Andersson wrote:
> On Mon 07 Nov 06:35 PST 2016, Imran Khan wrote:
>
>
>
> [..]
>
>>>> +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.
>
Yeah. I agree on this point. I was just following the lead of other SoCs here.
Just worried if having a string here breaks the convention. At least having
a numeric number is more in line with the documentation which expects a
serial number. May be here by serial number the documentation means numeric
id itself. Can someone please provide some feedback?
>>>> + soc_dev_attr->family = "Snapdragon";
>
> I think family should be e.g. "MSM8996" and then machine should be e.g.
> "MSM8996AU".
>
I think here family should be Snapdragon.The following site also mentions
the SoCs as Snapdragon family of processors.
https://www.qualcomm.com/products/snapdragon/processors/comparison
Could you please confirm if it's okay?
<snip>
>>
>> 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.
>
> [..]
Okay. I will remove this function in next patch set.
>> Thanks and Regards,
>
> Thank you Imran, looking forward to the next version.
>
> Regards,
> Bjorn
>
Thanks Bjorn, Looking forward for your answers about the queries, so that
I can float the next patch set.
Best Regards,
Imran
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a\nmember of the Code Aurora Forum, hosted by The Linux Foundation
--
To unsubscribe from this list: send the line "unsubscribe linux-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
