[Skiboot] [PATCH] tpm_i2c_nuvoton: check TPM vendor id register during probe
Klaus Heinrich Kiwi
klaus at linux.vnet.ibm.com
Fri Mar 6 22:22:56 AEDT 2020
On 3/5/2020 11:39 PM, Oliver O'Halloran wrote:
> On Fri, Mar 6, 2020 at 8:17 AM Klaus Heinrich Kiwi
> <klaus at linux.vnet.ibm.com> wrote:
>>
>>
>>
>> On 3/5/2020 3:54 PM, Eric Richter wrote:
>>> + /* ensure there's really the TPM we expect at that address */
>>> + if (tpm_i2c_request_send(tpm_device, SMBUS_READ, TPM_VID_DID,
>>> + 1, &vendor, sizeof(vendor)) ||
>>> + (vendor & TPM_VENDOR_ID_MASK) != TPM_650_VENDOR_ID) {
>> if tpm_i2c_request_send() fails (negative error code), you skip this
>> block and still tries to tpm_register_chip()?
>
> Any non-zero value is considered true so it'll go down the error path
> if the I2C transaction fails. I'm not 100% sure I understand what
> you're saying here though...
I misread the logic here, which I guess reinforces the point you are
also making in your review..
>>> + prlog(PR_ERR, "NUVOTON: i2c device inaccessible, or "
>>> + "expected vendor id mismatch\n");
>>> + if (vendor) {
>>> + prlog(PR_ERR, "NUVOTON: expected 0x%X, "
>>> + "got 0x%X\n", TPM_650_VENDOR_ID, vendor);
>>> + }
>>> + free(tpm_device);
>>> + continue;
>>> + }
>
>>> if (tpm_register_chip(node, tpm_device,
>>> &tpm_i2c_nuvoton_driver)) {
>>> free(tpm_device);
>>>
>>
>> Perhaps registering it would then fail? Since tpm_register_chip() sounds
>> both like an action as well as a predicate, perhaps be more explicit
>> about what we are testing? (I know this is outside the proposed patch
>> but still...)
>
> It doesn't fail, that's the problem. Right now we just keep trucking
> along until skiboot tries to write a measurement to the TPM at which
> point we notice it's not there.
Took a look at the function and it can only return 0 or -1, kinda
reinforcing that this should be a predicate (but again, not this patch -
perhaps I could propose something sometime)
--
Klaus Heinrich Kiwi <klaus at linux.vnet.ibm.com>
More information about the Skiboot
mailing list