On Sun, Oct 16, 2011 at 5:28 AM, Marek Vasut marek.vasut@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.vasut@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.vasut@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.vasut@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