On Sun, Oct 16, 2011 at 5:28 AM, Marek Vasut <marek.va...@gmail.com> wrote: > On Sunday, October 16, 2011 05:45:40 AM Vadim Bendebury wrote: >> On Sat, Oct 15, 2011 at 8:31 PM, Marek Vasut <marek.va...@gmail.com> wrote: >> > On Sunday, October 16, 2011 03:04:33 AM Vadim Bendebury wrote: >> >> On Sat, Oct 15, 2011 at 2:09 PM, Marek Vasut <marek.va...@gmail.com> >> >> wrote: >> >> > On Saturday, October 15, 2011 08:47:39 PM Vadim Bendebury wrote: >> >> >> Dear Marek Vasut, >> >> >> >> >> >> thank you for your comments, please see below: >> >> >> >> >> >> On Sat, Oct 15, 2011 at 11:08 AM, Marek Vasut <marek.va...@gmail.com> >> > >> > wrote: >> >> >> > On Saturday, October 15, 2011 05:38:50 AM Vadim Bendebury wrote: >> >> >> >> TPM (Trusted Platform Module) is an integrated circuit and >> >> >> >> software platform that provides computer manufacturers with the >> >> >> >> core components of a subsystem used to assure authenticity, >> >> >> >> integrity and confidentiality. >> >> >> > >> >> >> > [...] >> >> >> > >> >> >> > Quick points: >> >> >> > * The license >> >> >> >> >> >> Please suggest the appropriate file header text. >> >> > >> >> > Uh ... you should know the license !!! >> >> >> >> removed the BSD part >> > >> > Are you sure you're not relicensing code you don't own ? I'm just >> > curious, what's the origin of the code ? I'd prefer to avoid legal crap. >> >> I am sure. > > Would you mind answering my second question please ? >
I wrote this from scratch. >> >> >> [..] >> >> >> >> >> >> + >> >> >> >> +struct lpc_tpm { >> >> >> >> + struct tpm_locality locality[TPM_TOTAL_LOCALITIES]; >> >> >> >> +}; >> >> >> > >> >> >> > Do you need such envelope ? >> >> >> >> >> >> I think I do - this accurately describes the structure of the chip. >> >> > >> >> > There's just one member ... it's weird? >> >> >> >> I think it is appropriate in this case to encapsulate the entire chip >> >> description in a structure. Among other things makes it easier to pass >> >> a pointer to the chip description around. >> > >> > can't you pass the locality array ? >> >> no, because it would not be clear how big the array is. > > TPM_TOTAL_LOCALITIES big ? > I believe it is clearer when this information is included in a structure describing the chip (as opposed to the array size being a separate #define) >> >> >> [..] >> >> >> >> >> > Dot missing at the end. >> >> >> >> >> >> ok. >> >> > >> >> > Please fix globally. >> >> >> >> done >> >> >> >> >> >> +#define TPM_DRIVER_ERR (-1) >> >> >> >> + >> >> >> >> + /* 1 second is plenty for anything TPM does.*/ >> >> >> >> +#define MAX_DELAY_US (1000 * 1000) >> >> >> >> + >> >> >> >> +/* Retrieve burst count value out of the status register >> >> >> >> contents. */ +#define BURST_COUNT(status) ((u16)(((status) >> >> >> >> >> TIS_STS_BURST_COUNT_SHIFT) & \ + >> >> >> >> TIS_STS_BURST_COUNT_MASK)) >> >> >> > >> >> >> > Do you need the cast ? >> >> >> >> >> >> I think it demonstrates the intentional truncation of the value, it >> >> >> gets assigned to u16 values down the road, prevents compiler warnings >> >> >> about assigning incompatible values in some cases. >> >> > >> >> > Make it an inline function then, this will do the typechecking for >> >> > you. >> >> >> >> I am not sure what is wrong with a short macro in this case - is this >> >> against the coding style? >> > >> > It doesn't do typechecking. >> >> but the code around it does, doesn't it? >> >> Sorry, as I said, I am new here: how does this work on this project - >> does the submitter have to agree to all reviewer's comments? Can I ask >> somebody else to confirm that using a macro in this case in >> inappropriate? >> converted BURST_COUNT into a function. >> >> >> >> + >> >> >> >> +/* >> >> >> >> + * Structures defined below allow creating descriptions of TPM >> >> >> >> vendor/device + * ID information for run time discovery. The only >> >> >> >> device the system knows + * about at this time is Infineon slb9635 >> >> >> >> + */ >> >> >> >> +struct device_name { >> >> >> >> + u16 dev_id; >> >> >> >> + const char * const dev_name; >> >> >> >> +}; >> >> >> >> + >> >> >> >> +struct vendor_name { >> >> >> >> + u16 vendor_id; >> >> >> >> + const char *vendor_name; >> >> >> >> + const struct device_name *dev_names; >> >> >> >> +}; >> >> >> >> + >> >> >> >> +static const struct device_name infineon_devices[] = { >> >> >> >> + {0xb, "SLB9635 TT 1.2"}, >> >> >> >> + {0} >> >> >> >> +}; >> >> >> >> + >> >> >> >> +static const struct vendor_name vendor_names[] = { >> >> >> >> + {0x15d1, "Infineon", infineon_devices}, >> >> >> >> +}; >> >> >> >> + >> >> >> >> +/* >> >> >> >> + * Cached vendor/device ID pair to indicate that the device has >> >> >> >> been already + * discovered >> >> >> >> + */ >> >> >> >> +static u32 vendor_dev_id; >> >> >> >> + >> >> >> >> +/* TPM access going through macros to make tracing easier. */ >> >> >> >> +#define tpm_read(ptr) ({ \ >> >> >> >> + u32 __ret; \ >> >> >> >> + __ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \ >> >> >> >> + debug(PREFIX "Read reg 0x%x returns 0x%x\n", \ >> >> >> >> + (u32)ptr - (u32)lpc_tpm_dev, __ret); \ >> >> >> >> + __ret; }) >> >> >> >> + >> >> >> > >> >> >> > Make this inline function >> >> >> > >> >> >> >> +#define tpm_write(value, ptr) ({ \ >> >> >> >> + u32 __v = value; \ >> >> >> >> + debug(PREFIX "Write reg 0x%x with 0x%x\n", \ >> >> >> >> + (u32)ptr - (u32)lpc_tpm_dev, __v); \ >> >> >> >> + if (sizeof(*ptr) == 1) \ >> >> >> >> + writeb(__v, ptr); \ >> >> >> >> + else \ >> >> >> >> + writel(__v, ptr); }) >> >> >> >> + >> >> >> > >> >> >> > DTTO >> >> >> >> >> >> Are you sure these will work as inline functions? >> >> > >> >> > Why not ? Also, why do you introduce the __v ? >> >> >> >> macro vs function: need to be able to tell the pointed object size at >> >> run time. >> > >> > This seems wrong like hell. >> >> You are entitled to your opinion, but you should not be suggesting to >> change this code to inline functions, because it would break it. > > Then write it so it won't break please. > it is not broken as of now and IMO this is a good case for using macros. There is at least one other custodian who supports this, and Wolfgang Denk does not seem to mind. >> >> >> __v is needed to avoid side effects when invoking the macro. >> > >> > Side effects ? What side effects ? >> >> https://www.securecoding.cert.org/confluence/display/seccode/PRE31-C.+Avoid >> +side-effects+in+arguments+to+unsafe+macros > > I still don't see it. You use the variable in printf() and writeX(), neither > of > which change the variable ... so where's the sideeffect ? > The side effect comes from the calling site. When data[count++] is used as a macro argument, if there is no intermediate variable defined in the macro declaration, macro expansion inserts data[count++] in the code several times (as many times as the parameter is used in the macro declaration), and in this particular case gets executed twice, resulting in `count' advancing by 2 and wrong `data' values used. > Cheers > cheers, /vb _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot