>-----Original Message----- >From: Jason Gunthorpe [mailto:jgg@xxxxxxxx] >Sent: Monday, November 20, 2017 11:38 AM >To: Shaikh, Azhar <azhar.shaikh@xxxxxxxxx> >Cc: jarkko.sakkinen@xxxxxxxxxxxxxxx; peterhuewe@xxxxxx; linux- >integrity@xxxxxxxxxxxxxxx >Subject: Re: [PATCH RFC v3 1/2] tpm: Keep CLKRUN enabled throughout the >duration of transmit_cmd() > >On Mon, Nov 20, 2017 at 07:34:07PM +0000, Shaikh, Azhar wrote: > >> >However, why not have check_locality, release_locality, wait_startup >> >use clk_toggle instead? That seems better to me?? >> > >> >> I think, better to have the clk disable/enable at one place instead of >> adding it all locations where TPM access is done. The clk_toggle is >> kind of a special case where, for the complete duration of the TPM >> command processing, the CLKRUN protocol was supposed to be disabled. >> For rest of the places we can disable and re-enable the clkrun as soon >> as possible. > >> Also since it is part of the read/write APIs we won't miss any other >> location in the code where TPM access is done. > >But I wonder if you will hit the same bug that motivated this change in the >other places? > I guess not, since, I have tested this patch on 6 devices which completed 10000 suspend/resume cycles successfully. This is also one of the reason to keep tpm_platform_begin_xfer and tpm_platform_end_xfer functions. >It seems clearer to me to have a strong guard of the clock across the entire >high level action, since this hardware is so broken. > So, I should remove the tpm_platform_begin_xfer and tpm_platform_end_xfer functions and have that code in clk_toggle(clk_enable). And then for having high level action, I will have to add +if (chip->ops->clk_toggle != NULL) + chip->ops->clk_toggle(chip, true); .. .. <snip> + if (chip->ops->clk_toggle != NULL) + chip->ops->clk_toggle(chip, false); In -> tpm_tis_core_init() -> tpm_tis_plat_remove() -> tpm_tis_reenable_interrupts() -> tpm_tis_update_timeouts() -> tpm_transmit_cmd() [ Already implemented in this patch ] >You could add a debug to ensure that read/write is never without clk_enable >being held. > Sorry, but I didn't get this, adding a debug part. >Jason
