On Wed, Sep 24, 2014 at 11:13:38AM -0600, Jason Gunthorpe wrote: > On Wed, Sep 24, 2014 at 12:06:02PM +0300, Jarkko Sakkinen wrote: > > Added tpm2-sysfs.c that implements sysfs attributes for a TPM2 > > device. > > You need to document in Documentation every new sysfs that is added. > > The existing ones are not documented :(
This came up in Peters reply (you probably saw that). https://www.kernel.org/doc/Documentation/ABI/stable/sysfs-class-tpm > > +++ b/drivers/char/tpm/tpm-interface.c > > @@ -899,6 +899,7 @@ void tpm_remove_hardware(struct device *dev) > > > > tpm_chip_unregister(chip); > > > > + > > /* write it this way to be explicit (chip->dev == dev) */ > > put_device(chip->dev); > > } > > Hunk does not belong in this patch Ack (thanks). > > diff --git a/drivers/char/tpm/tpm2-commands.c > > b/drivers/char/tpm/tpm2-commands.c > > index a21dfd5..6365087 100644 > > +++ b/drivers/char/tpm/tpm2-commands.c > > @@ -195,7 +195,7 @@ ssize_t tpm2_get_tpm_pt(struct device *dev, u32 > > property_id, u32* value, > > cmd.header.in = tpm2_get_tpm_pt_header; > > cmd.params.tpm2_get_tpm_pt_in.cap_id = > > cpu_to_be32(TPM2_CAP_TPM_PROPERTIES); > > - cmd.params.tpm2_get_tpm_pt_in.property_id = property_id; > > + cmd.params.tpm2_get_tpm_pt_in.property_id = cpu_to_be32(property_id); > > cmd.params.tpm2_get_tpm_pt_in.property_cnt = cpu_to_be32(1); > > Hunk does not belong in this patch Ack (thanks). > > +static ssize_t pcrs_show(struct device *dev, struct device_attribute *attr, > > + char *buf) > > +{ > > The pcrs file never conformed to the sysfs rules, if TPM2 is getting a > whole new file set, I wouldn't mind seeing it not include the > non-conformant ones. What do you think? I think that it's better to put extra focus on these sysfs attributes in first patch set because it's user space visible. What's wrong in the current pcrs file? > > +static ssize_t caps_show(struct device *dev, struct device_attribute *attr, > > + char *buf) > > +{ > > Ditto.. The manfacturer number should probably be its own file Maybe here would make sense to have three files: - manufacturer - firmware_1 - firmware_2 More or less following the name of the TPM properties in the specification. > > +static ssize_t durations_show(struct device *dev, struct device_attribute > > *attr, > > + char *buf) > > +{ > > + struct tpm_chip *chip = dev_get_drvdata(dev); > > + > > + if (chip->vendor.duration[TPM_LONG] == 0) > > + return 0; > > + > > + return sprintf(buf, "%d %d %d [%s]\n", > > + jiffies_to_usecs(chip->vendor.duration[TPM_SHORT]), > > + jiffies_to_usecs(chip->vendor.duration[TPM_MEDIUM]), > > + jiffies_to_usecs(chip->vendor.duration[TPM_LONG]), > > + chip->vendor.duration_adjusted > > + ? "adjusted" : "original"); > > +} > > +static DEVICE_ATTR_RO(durations); > > Seem useless since the durations are constant, drop it? > > > +static ssize_t timeouts_show(struct device *dev, struct device_attribute > > *attr, > > + char *buf) > > +{ > > + struct tpm_chip *chip = dev_get_drvdata(dev); > > + > > + return sprintf(buf, "%d %d %d %d [%s]\n", > > + jiffies_to_usecs(chip->vendor.timeout_a), > > + jiffies_to_usecs(chip->vendor.timeout_b), > > + jiffies_to_usecs(chip->vendor.timeout_c), > > + jiffies_to_usecs(chip->vendor.timeout_d), > > + chip->vendor.timeout_adjusted > > + ? "adjusted" : "original"); > > +} > > +static DEVICE_ATTR_RO(timeouts); > > Ditto > > > +static ssize_t version_show(struct device *dev, struct device_attribute > > *attr, > > + char *buf) > > +{ > > + char *str = buf; > > + > > + str += sprintf(str, "2.0\n"); > > + > > + return str - buf; > > +} > > + > > +static DEVICE_ATTR_RO(version); > > + > > + > > +static ssize_t tpm2_show(struct device *dev, struct device_attribute *attr, > > + char *buf) > > +{ > > + struct tpm_chip *chip = dev_get_drvdata(dev); > > + > > + return sprintf(buf, "%d\n", chip->tpm2); > > +} > > +static DEVICE_ATTR_RO(tpm2); > > Two things for the same report? Drop one? > > Also, I think some thought is needed for the char interface - some > kind of ioctl to enter TPM2 mode and EINVAL access until that is done? > > Finally, this is in the wrong place in sysfs, I suspect it should be > attached to the char device node, not the platform device node? We > should at least investigate this... This was forgotten. Should not be here at all. Instead we have the variable 'version' to state specification family and level. I did not fully understand the comment about tpm2 flag. Why driver cannot set it when it initializes the device like with this based on value of the STS3? > > +struct tpm2_permanent { > > + unsigned int owner_auth_set : 1; > > + unsigned int endorsement_auth_set : 1; > > + unsigned int lockout_auth_set : 1; > > + unsigned int reserved1 : 5; > > + unsigned int disable_clear : 1; > > + unsigned int in_lockout : 1; > > + unsigned int tpm_generated_eps : 1; > > + unsigned int reserved2 : 21; > > +}; > > + > > +struct tpm2_startup_clear { > > + unsigned int ph_enable : 1; > > + unsigned int sh_enable : 1; > > + unsigned int eh_enable : 1; > > + unsigned int ph_enable_nv : 1; > > + unsigned int reserved : 27; > > + unsigned int orderly : 1; > > +}; > > This idea is not portable, you cannot use bitfields to index bits in a > word like this. Please use constants defined with BIT(xx) Thanks, I'll change this. > Next posting can you include a github link? Thanks Yup sure. I do everything for v2 in tpm2-v1 by adding fixes on top of these patches. When things look good there I'll create a new branch tpm2-v2 and prepare the next patch set. > Jason /Jarkko -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/