Re: [PATCH] tpm: Add sysfs interface to show TPM family version

[Jarkko Sakkinen]

On Mon, Mar 13, 2017 at 12:54:24PM +0000, Li, Meng wrote:
> 
> 
> > -----Original Message-----
> > From: Jarkko Sakkinen [mailto:jarkko.sakkinen@xxxxxxxxxxxxxxx]
> > Sent: Monday, March 13, 2017 7:54 PM
> > To: Li, Meng
> > Cc: linux-kernel@xxxxxxxxxxxxxxx; peterhuewe@xxxxxx;
> > tpmdd@xxxxxxxxxxxx; jgunthorpe@xxxxxxxxxxxxxxxxxxxx; tpmdd-
> > devel@xxxxxxxxxxxxxxxxxxxxx
> > Subject: Re: [PATCH] tpm: Add sysfs interface to show TPM family version
> > 
> > On Mon, Mar 13, 2017 at 05:20:17PM +0800, Meng.Li@xxxxxxxxxxxxx wrote:
> > > From: Limeng <Meng.Li@xxxxxxxxxxxxx>
> > >
> > > So far, there is not a sysfs interface for user space code to check
> > > the TPM family version(TPM1.x or TPM2). So, add a file named
> > > description in /sys/class/tpm/tpmX/ to show it.
> > >
> > > Signed-off-by: Meng Li <Meng.Li@xxxxxxxxxxxxx>
> > > ---
> > 
> > Is this the first or which version of the patch is this? Version number and
> > changelog are missing :/
> 
> Hi Jarkko,
> 
> This is the second version of this patch. The first one is reviewed by Peter who give out some good advices.
> 
> It is my first time to submit patch to upstream(main line),  and I am not very clear with the submitting rule.
> So, could you please give me a template to record the version and changing log?
> In my previous experience, I will attach a patch-0000 to record version and describe the background of this patch.
> But how to show the changing log when submit patch into mainline, I am not very  clear.
> 
> Regards,
> Limeng

Maybe this will help for the moment:

https://kernelnewbies.org/FirstKernelPatch#head-5c81b3c517a1d0bbc24f92594cb734e155fcbbcb

In the long run you probably should check this too:

https://www.kernel.org/doc/html/latest/process/submitting-patches.html

You probably should document in the commit message why this new sysfs
attribute is needed. Saying that it is needed to detect the TPM version
does not give any insight of any actual workload. User space facing
things are something where I rather not apply something unless it is
absolutely essetial because they have to be maintained forever.

In addition this looks odd:

if (chip->flags & TPM_CHIP_FLAG_TPM2)
	ret = sprintf(buf, "TPM 2.0");
else
	ret = sprintf(buf, "TPM 1.x");

I do not undertand for what specific purpose is the string "TPM " before
the family number is needed. And I'm not sure whether the attribute
should have both major and minor number, or should they be separate
attributes. For this the commit message does not give any insight on the
design choice.

And please remove the dangling store function...

/Jarkko