lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 29 Jan 2009 16:19:09 -0800 From: Matt Helsley <matthltc@...ibm.com> To: Rajiv Andrade <srajiv@...ux.vnet.ibm.com> Cc: linux-kernel@...r.kernel.org, akpm@...ux-foundation.org, dave@...ux.vnet.ibm.com, jmorris@...ei.org, zohar@...ux.vnet.ibm.com Subject: Re: [PATCH 1/2] TPM: sysfs functions consolidation On Thu, 2009-01-29 at 21:01 -0200, Rajiv Andrade wrote: > According to Dave Hansen's comments on the tpm_show_*, some of these functions > present a pattern when allocating data[] memory space and also when setting its > content. A new function was created so that this pattern could be consolidated. > Also, replaced the data[] command vectors and its indexes by meaningful structures > as pointed out by Matt Helsley too. > > Signed-off-by: Rajiv Andrade <srajiv@...ux.vnet.ibm.com> > --- > drivers/char/tpm/tpm.c | 410 +++++++++++++++++------------------------------- > drivers/char/tpm/tpm.h | 117 ++++++++++++++ > 2 files changed, 258 insertions(+), 269 deletions(-) > > diff --git a/drivers/char/tpm/tpm.c b/drivers/char/tpm/tpm.c > index 9c47dc4..58ea16f 100644 > --- a/drivers/char/tpm/tpm.c > +++ b/drivers/char/tpm/tpm.c <snip> > - rc = transmit_cmd(chip, data, sizeof(data), > + rc = transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE, > "attempting to determine the timeouts"); > if (rc) > goto duration; > > - if (be32_to_cpu(*((__be32 *) (data + TPM_GET_CAP_RET_SIZE_IDX))) > + if (be32_to_cpu(tpm_cmd.header.out.length) > != 4 * sizeof(u32)) > goto duration; > > + timeout_cap = &tpm_cmd.params.getcap_out.cap.timeout; > /* Don't overwrite default if value is 0 */ > - timeout = > - be32_to_cpu(*((__be32 *) (data + TPM_GET_CAP_RET_UINT32_1_IDX))); > + timeout = be32_to_cpu(timeout_cap->a); > if (timeout) > chip->vendor.timeout_a = usecs_to_jiffies(timeout); > - timeout = > - be32_to_cpu(*((__be32 *) (data + TPM_GET_CAP_RET_UINT32_2_IDX))); > + timeout = be32_to_cpu(timeout_cap->b); > if (timeout) > chip->vendor.timeout_b = usecs_to_jiffies(timeout); > - timeout = > - be32_to_cpu(*((__be32 *) (data + TPM_GET_CAP_RET_UINT32_3_IDX))); > + timeout = be32_to_cpu(timeout_cap->c); > if (timeout) > chip->vendor.timeout_c = usecs_to_jiffies(timeout); > - timeout = > - be32_to_cpu(*((__be32 *) (data + TPM_GET_CAP_RET_UINT32_4_IDX))); > + timeout = be32_to_cpu(timeout_cap->d); > if (timeout) > chip->vendor.timeout_d = usecs_to_jiffies(timeout); Are jiffies really the appropriate units of time for the needs of this driver? I could easily be wrong but I thought most drivers were discouraged from using jiffies since HZ is configurable... > > duration: > - memcpy(data, tpm_cap, sizeof(tpm_cap)); > - data[TPM_CAP_IDX] = TPM_CAP_PROP; > - data[TPM_CAP_SUBCAP_IDX] = TPM_CAP_PROP_TIS_DURATION; > + tpm_cmd.header.in = tpm_getcap_header; > + tpm_cmd.params.getcap_in.cap = TPM_CAP_PROP; > + tpm_cmd.params.getcap_in.subcap_size = cpu_to_be32(4); > + tpm_cmd.params.getcap_in.subcap = TPM_CAP_PROP_TIS_DURATION; > > - rc = transmit_cmd(chip, data, sizeof(data), > + rc = transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE, > "attempting to determine the durations"); > if (rc) > return; > > - if (be32_to_cpu(*((__be32 *) (data + TPM_GET_CAP_RET_SIZE_IDX))) > + if (be32_to_cpu(tpm_cmd.header.out.return_code) > != 3 * sizeof(u32)) > return; > - > + timeout_cap = &tpm_cmd.params.getcap_out.cap.timeout; > chip->vendor.duration[TPM_SHORT] = > - usecs_to_jiffies(be32_to_cpu > - (*((__be32 *) (data + > - TPM_GET_CAP_RET_UINT32_1_IDX)))); > + usecs_to_jiffies(be32_to_cpu(timeout_cap->a)); > /* The Broadcom BCM0102 chipset in a Dell Latitude D820 gets the above > * value wrong and apparently reports msecs rather than usecs. So we > * fix up the resulting too-small TPM_SHORT value to make things work. > @@ -565,13 +578,9 @@ duration: > chip->vendor.duration[TPM_SHORT] = HZ; > > chip->vendor.duration[TPM_MEDIUM] = > - usecs_to_jiffies(be32_to_cpu > - (*((__be32 *) (data + > - TPM_GET_CAP_RET_UINT32_2_IDX)))); > + usecs_to_jiffies(be32_to_cpu(timeout_cap->b)); > chip->vendor.duration[TPM_LONG] = > - usecs_to_jiffies(be32_to_cpu > - (*((__be32 *) (data + > - TPM_GET_CAP_RET_UINT32_3_IDX)))); > + usecs_to_jiffies(be32_to_cpu(timeout_cap->c)); OK, so it looks like these timeouts are short, medium, and long-duration timeouts and those correspond to "a", "b", and "c". What's "d"? Also this suggests slightly-better names for these fields. If you can think of short names suggesting why these separate, varying-length timeouts are needed that could be even better. <snip> > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h > index 8e30df4..867987d 100644 > --- a/drivers/char/tpm/tpm.h > +++ b/drivers/char/tpm/tpm.h <snip> > +struct timeout_t { > + __be32 a; > + __be32 b; > + __be32 c; > + __be32 d; > +}__attribute__((packed)); As I pointed out above I think these could use better names. I also noticed that there are timeout_a, timeout_b, etc. fields of another struct (somewhere under "chips" if I recall..). Perhaps similar naming -- maybe even this struct -- should be (re)used? <snip> Cheers, -Matt Helsley -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists