On Wed, Sep 19, 2018 at 04:46:27PM +0300, Jarkko Sakkinen wrote: > On Tue, Sep 18, 2018 at 12:34:40PM +0300, Tomas Winkler wrote: > > 1. TPM2_CC_LAST has moved from 182 to 193 > > 2. Convert tpm2_ordinal_duration from an array into a switch statement, > > as there are not so many commands that require special duration > > relative to a number of commands. > > > > Signed-off-by: Tomas Winkler <[email protected]> > > --- > > V2-V3: Rebase. > > drivers/char/tpm/tpm.h | 41 +++++---- > > drivers/char/tpm/tpm2-cmd.c | 196 > > +++++++++++++++----------------------------- > > 2 files changed, 93 insertions(+), 144 deletions(-) > > > > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h > > index f20dc8ece348..0f08518b525d 100644 > > --- a/drivers/char/tpm/tpm.h > > +++ b/drivers/char/tpm/tpm.h > > @@ -134,22 +134,31 @@ enum tpm2_algorithms { > > }; > > > > enum tpm2_command_codes { > > - TPM2_CC_FIRST = 0x011F, > > - TPM2_CC_CREATE_PRIMARY = 0x0131, > > - TPM2_CC_SELF_TEST = 0x0143, > > - TPM2_CC_STARTUP = 0x0144, > > - TPM2_CC_SHUTDOWN = 0x0145, > > - TPM2_CC_CREATE = 0x0153, > > - TPM2_CC_LOAD = 0x0157, > > - TPM2_CC_UNSEAL = 0x015E, > > - TPM2_CC_CONTEXT_LOAD = 0x0161, > > - TPM2_CC_CONTEXT_SAVE = 0x0162, > > - TPM2_CC_FLUSH_CONTEXT = 0x0165, > > - TPM2_CC_GET_CAPABILITY = 0x017A, > > - TPM2_CC_GET_RANDOM = 0x017B, > > - TPM2_CC_PCR_READ = 0x017E, > > - TPM2_CC_PCR_EXTEND = 0x0182, > > - TPM2_CC_LAST = 0x018F, > > + TPM2_CC_FIRST = 0x011F, > > + TPM2_CC_HIERARCHY_CONTROL = 0x0121, > > + TPM2_CC_HIERARCHY_CHANGE_AUTH = 0x0129, > > + TPM2_CC_CREATE_PRIMARY = 0x0131, > > + TPM2_CC_SEQUENCE_COMPLETE = 0x013E, > > + TPM2_CC_SELF_TEST = 0x0143, > > + TPM2_CC_STARTUP = 0x0144, > > + TPM2_CC_SHUTDOWN = 0x0145, > > + TPM2_CC_NV_READ = 0x014E, > > + TPM2_CC_CREATE = 0x0153, > > + TPM2_CC_LOAD = 0x0157, > > + TPM2_CC_SEQUENCE_UPDATE = 0x015C, > > + TPM2_CC_UNSEAL = 0x015E, > > + TPM2_CC_CONTEXT_LOAD = 0x0161, > > + TPM2_CC_CONTEXT_SAVE = 0x0162, > > + TPM2_CC_FLUSH_CONTEXT = 0x0165, > > + TPM2_CC_VERIFY_SIGNATURE = 0x0177, > > + TPM2_CC_GET_CAPABILITY = 0x017A, > > + TPM2_CC_GET_RANDOM = 0x017B, > > + TPM2_CC_PCR_READ = 0x017E, > > + TPM2_CC_PCR_EXTEND = 0x0182, > > + TPM2_CC_EVENT_SEQUENCE_COMPLETE = 0x0185, > > + TPM2_CC_HASH_SEQUENCE_START = 0x0186, > > + TPM2_CC_CREATE_LOADED = 0x0191, > > + TPM2_CC_LAST = 0x0193, /* Spec 1.36 */ > > }; > > > > enum tpm2_permanent_handles { > > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c > > index 3acf4fd4e5a5..9c3c5c0628d9 100644 > > --- a/drivers/char/tpm/tpm2-cmd.c > > +++ b/drivers/char/tpm/tpm2-cmd.c > > @@ -41,128 +41,73 @@ static struct tpm2_hash tpm2_hash_map[] = { > > }; > > > > /* > > - * Array with one entry per ordinal defining the maximum amount > > + * tpm2_ordinal_duration returns the maximum amount > > tpm2_ordinal_duration should have parentheses when in a sentence > for clarity. > > Add something this to tpm2_calc_ordinal_duration() instead: > > /* tpm2_calc_ordinal_duration - return the maximum amount of time for command > duration > * @ordinal: the command code > * > * Return the maximum amount of time for the command duration. The values are > * taken from the PC Client Profile (PTP) specification. The values can > * are defined in &enum tpm_duration. > */ > > > * of time the chip could take to return the result. The values > > + * of the MEDIUM, and LONG durations are taken from the > > + * PC Client Profile (PTP) specification (750, 2000 msec) > > + * > > * LONG_LONG is for commands that generates keys which empirically > > * takes longer time on some systems. > > This tail should really be part of the kdoc for enum tpm_duration > (including the existing comment about LONG_LONG). > > > */ > > -static const u8 tpm2_ordinal_duration[TPM2_CC_LAST - TPM2_CC_FIRST + 1] = { > > - TPM_UNDEFINED, /* 11F */ > > - TPM_UNDEFINED, /* 120 */ > > - TPM_LONG, /* 121 */ > > - TPM_UNDEFINED, /* 122 */ > > - TPM_UNDEFINED, /* 123 */ > > - TPM_UNDEFINED, /* 124 */ > > - TPM_UNDEFINED, /* 125 */ > > - TPM_UNDEFINED, /* 126 */ > > - TPM_UNDEFINED, /* 127 */ > > - TPM_UNDEFINED, /* 128 */ > > - TPM_LONG, /* 129 */ > > - TPM_UNDEFINED, /* 12a */ > > - TPM_UNDEFINED, /* 12b */ > > - TPM_UNDEFINED, /* 12c */ > > - TPM_UNDEFINED, /* 12d */ > > - TPM_UNDEFINED, /* 12e */ > > - TPM_UNDEFINED, /* 12f */ > > - TPM_UNDEFINED, /* 130 */ > > - TPM_LONG_LONG, /* 131 */ > > - TPM_UNDEFINED, /* 132 */ > > - TPM_UNDEFINED, /* 133 */ > > - TPM_UNDEFINED, /* 134 */ > > - TPM_UNDEFINED, /* 135 */ > > - TPM_UNDEFINED, /* 136 */ > > - TPM_UNDEFINED, /* 137 */ > > - TPM_UNDEFINED, /* 138 */ > > - TPM_UNDEFINED, /* 139 */ > > - TPM_UNDEFINED, /* 13a */ > > - TPM_UNDEFINED, /* 13b */ > > - TPM_UNDEFINED, /* 13c */ > > - TPM_UNDEFINED, /* 13d */ > > - TPM_MEDIUM, /* 13e */ > > - TPM_UNDEFINED, /* 13f */ > > - TPM_UNDEFINED, /* 140 */ > > - TPM_UNDEFINED, /* 141 */ > > - TPM_UNDEFINED, /* 142 */ > > - TPM_LONG, /* 143 */ > > - TPM_MEDIUM, /* 144 */ > > - TPM_UNDEFINED, /* 145 */ > > - TPM_UNDEFINED, /* 146 */ > > - TPM_UNDEFINED, /* 147 */ > > - TPM_UNDEFINED, /* 148 */ > > - TPM_UNDEFINED, /* 149 */ > > - TPM_UNDEFINED, /* 14a */ > > - TPM_UNDEFINED, /* 14b */ > > - TPM_UNDEFINED, /* 14c */ > > - TPM_UNDEFINED, /* 14d */ > > - TPM_LONG, /* 14e */ > > - TPM_UNDEFINED, /* 14f */ > > - TPM_UNDEFINED, /* 150 */ > > - TPM_UNDEFINED, /* 151 */ > > - TPM_UNDEFINED, /* 152 */ > > - TPM_LONG_LONG, /* 153 */ > > - TPM_UNDEFINED, /* 154 */ > > - TPM_UNDEFINED, /* 155 */ > > - TPM_UNDEFINED, /* 156 */ > > - TPM_UNDEFINED, /* 157 */ > > - TPM_UNDEFINED, /* 158 */ > > - TPM_UNDEFINED, /* 159 */ > > - TPM_UNDEFINED, /* 15a */ > > - TPM_UNDEFINED, /* 15b */ > > - TPM_MEDIUM, /* 15c */ > > - TPM_UNDEFINED, /* 15d */ > > - TPM_UNDEFINED, /* 15e */ > > - TPM_UNDEFINED, /* 15f */ > > - TPM_UNDEFINED, /* 160 */ > > - TPM_UNDEFINED, /* 161 */ > > - TPM_UNDEFINED, /* 162 */ > > - TPM_UNDEFINED, /* 163 */ > > - TPM_UNDEFINED, /* 164 */ > > - TPM_UNDEFINED, /* 165 */ > > - TPM_UNDEFINED, /* 166 */ > > - TPM_UNDEFINED, /* 167 */ > > - TPM_UNDEFINED, /* 168 */ > > - TPM_UNDEFINED, /* 169 */ > > - TPM_UNDEFINED, /* 16a */ > > - TPM_UNDEFINED, /* 16b */ > > - TPM_UNDEFINED, /* 16c */ > > - TPM_UNDEFINED, /* 16d */ > > - TPM_UNDEFINED, /* 16e */ > > - TPM_UNDEFINED, /* 16f */ > > - TPM_UNDEFINED, /* 170 */ > > - TPM_UNDEFINED, /* 171 */ > > - TPM_UNDEFINED, /* 172 */ > > - TPM_UNDEFINED, /* 173 */ > > - TPM_UNDEFINED, /* 174 */ > > - TPM_UNDEFINED, /* 175 */ > > - TPM_UNDEFINED, /* 176 */ > > - TPM_LONG, /* 177 */ > > - TPM_UNDEFINED, /* 178 */ > > - TPM_UNDEFINED, /* 179 */ > > - TPM_MEDIUM, /* 17a */ > > - TPM_LONG, /* 17b */ > > - TPM_UNDEFINED, /* 17c */ > > - TPM_UNDEFINED, /* 17d */ > > - TPM_UNDEFINED, /* 17e */ > > - TPM_UNDEFINED, /* 17f */ > > - TPM_UNDEFINED, /* 180 */ > > - TPM_UNDEFINED, /* 181 */ > > - TPM_MEDIUM, /* 182 */ > > - TPM_UNDEFINED, /* 183 */ > > - TPM_UNDEFINED, /* 184 */ > > - TPM_MEDIUM, /* 185 */ > > - TPM_MEDIUM, /* 186 */ > > - TPM_UNDEFINED, /* 187 */ > > - TPM_UNDEFINED, /* 188 */ > > - TPM_UNDEFINED, /* 189 */ > > - TPM_UNDEFINED, /* 18a */ > > - TPM_UNDEFINED, /* 18b */ > > - TPM_UNDEFINED, /* 18c */ > > - TPM_UNDEFINED, /* 18d */ > > - TPM_UNDEFINED, /* 18e */ > > - TPM_UNDEFINED /* 18f */ > > -}; > > +static u8 tpm2_ordinal_duration(u32 ordinal) > > This naming scheme becomes confusing. I would suggest to name this > as __tpm2_calc_ordinal_duration(u32 ordinal). > > > +{ > > + switch (ordinal) { > > + /* Startup */ > > Please remove these comments. They add only extra weight to maintain > the code. > > > + case TPM2_CC_STARTUP: /* 144 */ > > + return TPM_MEDIUM; > > + > > + /* Selftest */ > > + case TPM2_CC_SELF_TEST: /* 143 */ > > + return TPM_LONG; > > + > > + /* Random Number Generator */ > > + case TPM2_CC_GET_RANDOM: /* 17B */ > > + return TPM_LONG; > > + > > + /* Hash/HMAC/Event Sequences */ > > + case TPM2_CC_SEQUENCE_UPDATE: /* 15C */ > > + return TPM_MEDIUM; > > + case TPM2_CC_SEQUENCE_COMPLETE: /* 13E */ > > + return TPM_MEDIUM; > > + case TPM2_CC_EVENT_SEQUENCE_COMPLETE: /* 185 */ > > + return TPM_MEDIUM; > > + case TPM2_CC_HASH_SEQUENCE_START: /* 186 */ > > + return TPM_MEDIUM; > > + > > + /* Signature Verification */ > > + case TPM2_CC_VERIFY_SIGNATURE: /* 177 */ > > + return TPM_LONG; > > + > > + /* Integrity Collection (PCR) */ > > + case TPM2_CC_PCR_EXTEND: /* 182 */ > > + return TPM_MEDIUM; > > + > > + /* Hierarchy Commands */ > > + case TPM2_CC_HIERARCHY_CONTROL: /* 121 */ > > + return TPM_LONG; > > + case TPM2_CC_HIERARCHY_CHANGE_AUTH: /* 129 */ > > + return TPM_LONG; > > + > > + /* Capability Commands */ > > + case TPM2_CC_GET_CAPABILITY: /* 17A */ > > + return TPM_MEDIUM; > > + > > + /* Non-volatile Storage */ > > + case TPM2_CC_NV_READ: /* 14E */ > > + return TPM_LONG; > > + > > + /* Key generation (not in PTP) */ > > + case TPM2_CC_CREATE_PRIMARY: /* 131 */ > > + return TPM_LONG_LONG; > > + case TPM2_CC_CREATE: /* 153 */ > > + return TPM_LONG_LONG; > > + case TPM2_CC_CREATE_LOADED: /* 191 */ > > + return TPM_LONG_LONG; > > + > > + default: > > + return TPM_UNDEFINED; > > + } > > +} > > > > struct tpm2_pcr_read_out { > > __be32 update_cnt; > > @@ -758,19 +703,14 @@ void tpm2_shutdown(struct tpm_chip *chip, u16 > > shutdown_type) > > */ > > unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *chip, u32 > > ordinal) > > { > > - int index = TPM_UNDEFINED; > > - int duration = 0; > > + unsigned int index; > > > > - if (ordinal >= TPM2_CC_FIRST && ordinal <= TPM2_CC_LAST) > > - index = tpm2_ordinal_duration[ordinal - TPM2_CC_FIRST]; > > + index = tpm2_ordinal_duration(ordinal); > > > > if (index != TPM_UNDEFINED) > > - duration = chip->duration[index]; > > - > > - if (duration <= 0) > > - duration = msecs_to_jiffies(TPM2_DURATION_DEFAULT); > > - > > - return duration; > > + return chip->duration[index]; > > + else > > + return msecs_to_jiffies(TPM2_DURATION_DEFAULT); > > } > > EXPORT_SYMBOL_GPL(tpm2_calc_ordinal_duration); > > > > -- > > 2.14.4 > > > > /Jarkko
Forgot to add this: Tested-by: Jarkko Sakkinen <[email protected]> /Jarkko
