Re: [PATCH RFC v2 0/2] Fix corner cases with disabling CLKRUN in tpm_tis

Re: [PATCH RFC v2 0/2] Fix corner cases with disabling CLKRUN in tpm_tis

[Jarkko Sakkinen]

On Tue, Nov 07, 2017 at 01:32:06PM -0800, Azhar Shaikh wrote:
> Changes from v1:
> - Patch 1: "tpm: Keep CLKRUN enabled throughout the duration of transmit_cmd()"
>   - Add NULL checks before calling clk_toggle callback
>   - Use IS_ENABLED instead of ifdef in tpm_tis_clkrun_toggle()
>   - Do not call tpm_platform_begin_xfer() and tpm_platform_end_xfer()
>     from tpm_tis_clkrun_toggle(). Make them static again.
> 
> - Patch 2: "tpm_tis: Move ilb_base_addr to tpm_tis_tcg_phy"
>   - This is a new patch in this series as per suggestion from Jason.
>   - Is the current implementation ok or I should move the code in tpm_tis_pnp_remove()
>     and tpm_tis_plat_remove() inside tpm_tis_remove(). That way all the unmapping
>     can be done in one place, instead of 3 different places now. Also the unmapping
>     in tpm_tis_init() can be moved to tpm_tis_remove(), since in case of error
>     tpm_tis_core_init() calls tpm_tis_remove(). Kindly suggest.
> 
> 
> Azhar Shaikh (2):
>   tpm: Keep CLKRUN enabled throughout the duration of transmit_cmd()
>   tpm_tis: Move ilb_base_addr to tpm_tis_tcg_phy
> 
>  drivers/char/tpm/tpm-interface.c |   6 +++
>  drivers/char/tpm/tpm_tis.c       | 102 ++++++++++++++++++++++++---------------
>  drivers/char/tpm/tpm_tis_core.c  |  21 ++++++++
>  drivers/char/tpm/tpm_tis_core.h  |   1 +
>  include/linux/tpm.h              |   1 +
>  5 files changed, 93 insertions(+), 38 deletions(-)
> 
> -- 
> 1.9.1
> 

Please include my email (@linux.intel.com) to the TO-field for all TPM
patches (and the cover letter).

I will eventually catch these as I go through the ML but sometimes there
is more latency to do that when I have a busy period. In a less busy
period there is of course less latency.

As this is a shared list with IMA and EVM I have to check every message
whether it is a TPM patch. Thus, in a shared list including maintainers
is even more important. You can find maintainers for every subsystem
from MAINTAINERS file in the root of the Linux GIT tree.

Right now I have a very busy period as I'm upstream the SGX driver. That
is why it took a week to even spot this (just did).

I'll try to find time this week to properly review your changes but
cannot promise it will be tomorrow because, well, I just saw the patch
set.

With a quick oversight I do not see anything that would shock me but I
still have to look into it with time and care.

/Jarkko

RE: [PATCH RFC v2 0/2] Fix corner cases with disabling CLKRUN in tpm_tis

[Shaikh, Azhar]

>-----Original Message-----
>From: linux-integrity-owner@vger.kernel.org [mailto:linux-integrity-
>owner@vger.kernel.org] On Behalf Of Jarkko Sakkinen
>Sent: Tuesday, November 14, 2017 1:39 PM
>To: Shaikh, Azhar <azhar.shaikh@intel.com>
>Cc: linux-integrity@vger.kernel.org; Sakkinen, Jarkko
><jarkko.sakkinen@intel.com>
>Subject: Re: [PATCH RFC v2 0/2] Fix corner cases with disabling CLKRUN in
>tpm_tis
>
>On Tue, Nov 07, 2017 at 01:32:06PM -0800, Azhar Shaikh wrote:
>> Changes from v1:
>> - Patch 1: "tpm: Keep CLKRUN enabled throughout the duration of
>transmit_cmd()"
>>   - Add NULL checks before calling clk_toggle callback
>>   - Use IS_ENABLED instead of ifdef in tpm_tis_clkrun_toggle()
>>   - Do not call tpm_platform_begin_xfer() and tpm_platform_end_xfer()
>>     from tpm_tis_clkrun_toggle(). Make them static again.
>>
>> - Patch 2: "tpm_tis: Move ilb_base_addr to tpm_tis_tcg_phy"
>>   - This is a new patch in this series as per suggestion from Jason.
>>   - Is the current implementation ok or I should move the code in
>tpm_tis_pnp_remove()
>>     and tpm_tis_plat_remove() inside tpm_tis_remove(). That way all the
>unmapping
>>     can be done in one place, instead of 3 different places now. Also the
>unmapping
>>     in tpm_tis_init() can be moved to tpm_tis_remove(), since in case of error
>>     tpm_tis_core_init() calls tpm_tis_remove(). Kindly suggest.
>>
>>
>> Azhar Shaikh (2):
>>   tpm: Keep CLKRUN enabled throughout the duration of transmit_cmd()
>>   tpm_tis: Move ilb_base_addr to tpm_tis_tcg_phy
>>
>>  drivers/char/tpm/tpm-interface.c |   6 +++
>>  drivers/char/tpm/tpm_tis.c       | 102 ++++++++++++++++++++++++---------
>------
>>  drivers/char/tpm/tpm_tis_core.c  |  21 ++++++++
>>  drivers/char/tpm/tpm_tis_core.h  |   1 +
>>  include/linux/tpm.h              |   1 +
>>  5 files changed, 93 insertions(+), 38 deletions(-)
>>
>> --
>> 1.9.1
>>
>
>Please include my email (@linux.intel.com) to the TO-field for all TPM
>patches (and the cover letter).
>

Sorry about this. Going forward will include @linux.intel.com and other TPM maintainers to the TO-field.

>I will eventually catch these as I go through the ML but sometimes there
>is more latency to do that when I have a busy period. In a less busy
>period there is of course less latency.
>
>As this is a shared list with IMA and EVM I have to check every message
>whether it is a TPM patch. Thus, in a shared list including maintainers
>is even more important. You can find maintainers for every subsystem
>from MAINTAINERS file in the root of the Linux GIT tree.
>
>Right now I have a very busy period as I'm upstream the SGX driver. That
>is why it took a week to even spot this (just did).
>
>I'll try to find time this week to properly review your changes but
>cannot promise it will be tomorrow because, well, I just saw the patch
>set.
>

Sure, I understand. Thank you!

>With a quick oversight I do not see anything that would shock me but I
>still have to look into it with time and care.
>
>/Jarkko

Re: [PATCH RFC v2 2/2] tpm_tis: Move ilb_base_addr to tpm_tis_tcg_phy

[Jason Gunthorpe]

On Tue, Nov 07, 2017 at 01:32:08PM -0800, Azhar Shaikh wrote:

> +#ifdef CONFIG_X86
> +	if (is_bsw())
> +		iounmap(phy->ilb_base_addr);
> +#endif

This whole thing would be much better if is_bsw was just

bool is_bsw(void)
{ 
   if (!IS_ENABLED(CONFIG_X86))
       return false;
   [..]
}

Then drop every single one of these #ifdef CONFIG_X86

Jason

Re: [PATCH RFC v2 0/2] Fix corner cases with disabling CLKRUN in tpm_tis

[Jarkko Sakkinen]

On Tue, Nov 14, 2017 at 11:59:45PM +0200, Shaikh, Azhar wrote:
> Sorry about this. Going forward will include @linux.intel.com and
> other TPM maintainers to the TO-field.

NP, didn't really affect me :-) But if you want less latency to your
review, you should do this.

/Jarkko

Re: [PATCH RFC v2 2/2] tpm_tis: Move ilb_base_addr to tpm_tis_tcg_phy

[Jarkko Sakkinen]

On Tue, Nov 14, 2017 at 04:28:12PM -0700, Jason Gunthorpe wrote:
> On Tue, Nov 07, 2017 at 01:32:08PM -0800, Azhar Shaikh wrote:
> 
> > +#ifdef CONFIG_X86
> > +	if (is_bsw())
> > +		iounmap(phy->ilb_base_addr);
> > +#endif
> 
> This whole thing would be much better if is_bsw was just
> 
> bool is_bsw(void)
> { 
>    if (!IS_ENABLED(CONFIG_X86))
>        return false;
>    [..]
> }
> 
> Then drop every single one of these #ifdef CONFIG_X86
> 
> Jason

+1

/Jarkko

RE: [PATCH RFC v2 2/2] tpm_tis: Move ilb_base_addr to tpm_tis_tcg_phy

[Shaikh, Azhar]

>-----Original Message-----
>From: Sakkinen, Jarkko
>Sent: Wednesday, November 15, 2017 12:14 AM
>To: Jason Gunthorpe <jgg@ziepe.ca>
>Cc: Shaikh, Azhar <azhar.shaikh@intel.com>; linux-integrity@vger.kernel.org
>Subject: Re: [PATCH RFC v2 2/2] tpm_tis: Move ilb_base_addr to
>tpm_tis_tcg_phy
>
>On Tue, Nov 14, 2017 at 04:28:12PM -0700, Jason Gunthorpe wrote:
>> On Tue, Nov 07, 2017 at 01:32:08PM -0800, Azhar Shaikh wrote:
>>
>> > +#ifdef CONFIG_X86
>> > +	if (is_bsw())
>> > +		iounmap(phy->ilb_base_addr);
>> > +#endif
>>
>> This whole thing would be much better if is_bsw was just
>>
>> bool is_bsw(void)
>> {
>>    if (!IS_ENABLED(CONFIG_X86))
>>        return false;
>>    [..]
>> }
>>
>> Then drop every single one of these #ifdef CONFIG_X86
>>
>> Jason
>
>+1
>

Thank you for the suggestion.
Will incorporate this in next patch series.

>/Jarkko

RE: [PATCH RFC v2 2/2] tpm_tis: Move ilb_base_addr to tpm_tis_tcg_phy

[Shaikh, Azhar]

>-----Original Message-----
>From: Shaikh, Azhar
>Sent: Wednesday, November 15, 2017 9:59 AM
>To: Sakkinen, Jarkko <jarkko.sakkinen@intel.com>; Jason Gunthorpe
><jgg@ziepe.ca>
>Cc: linux-integrity@vger.kernel.org
>Subject: RE: [PATCH RFC v2 2/2] tpm_tis: Move ilb_base_addr to
>tpm_tis_tcg_phy
>
>
>
>>-----Original Message-----
>>From: Sakkinen, Jarkko
>>Sent: Wednesday, November 15, 2017 12:14 AM
>>To: Jason Gunthorpe <jgg@ziepe.ca>
>>Cc: Shaikh, Azhar <azhar.shaikh@intel.com>;
>>linux-integrity@vger.kernel.org
>>Subject: Re: [PATCH RFC v2 2/2] tpm_tis: Move ilb_base_addr to
>>tpm_tis_tcg_phy
>>
>>On Tue, Nov 14, 2017 at 04:28:12PM -0700, Jason Gunthorpe wrote:
>>> On Tue, Nov 07, 2017 at 01:32:08PM -0800, Azhar Shaikh wrote:
>>>
>>> > +#ifdef CONFIG_X86
>>> > +	if (is_bsw())
>>> > +		iounmap(phy->ilb_base_addr);
>>> > +#endif
>>>
>>> This whole thing would be much better if is_bsw was just
>>>
>>> bool is_bsw(void)
>>> {
>>>    if (!IS_ENABLED(CONFIG_X86))
>>>        return false;
>>>    [..]
>>> }
>>>
>>> Then drop every single one of these #ifdef CONFIG_X86
>>>
>>> Jason
>>
>>+1
>>
>
>Thank you for the suggestion.
>Will incorporate this in next patch series.
>

If I implement is_bsw() as below and move it(is_bsw()) outside the #ifdef CONFIG_X86, I will still get compilation errors for non-x86 platforms, since INTEL_FAM6_ATOM_AIRMONT will be undefined. 

bool is_bsw(void)
{
    if (!IS_ENABLED(CONFIG_X86))
        return false;
return ((boot_cpu_data.x86_model == INTEL_FAM6_ATOM_AIRMONT) ? 1 : 0);
}

I think I will have to keep is_bsw() implementation unchanged?

>>/Jarkko

Re: [PATCH RFC v2 2/2] tpm_tis: Move ilb_base_addr to tpm_tis_tcg_phy

[Jason Gunthorpe]

On Wed, Nov 15, 2017 at 07:36:47PM +0000, Shaikh, Azhar wrote:

> If I implement is_bsw() as below and move it(is_bsw()) outside the #ifdef CONFIG_X86, I will still get compilation errors for non-x86 platforms, since INTEL_FAM6_ATOM_AIRMONT will be undefined. 
> 
> bool is_bsw(void)
> {
>     if (!IS_ENABLED(CONFIG_X86))
>         return false;
> return ((boot_cpu_data.x86_model == INTEL_FAM6_ATOM_AIRMONT) ? 1 : 0);
> }
> 
> I think I will have to keep is_bsw() implementation unchanged?

No, still move the #ifdef to is_bsw, just like this instead:

bool is_bsw(void)
{
#ifdef CONFIG_X86
     return ((boot_cpu_data.x86_model == INTEL_FAM6_ATOM_AIRMONT) ? 1
     : 0);
#else
     return false;
#endif
}

Jason

RE: [PATCH RFC v2 2/2] tpm_tis: Move ilb_base_addr to tpm_tis_tcg_phy

[Shaikh, Azhar]

>-----Original Message-----
>From: Jason Gunthorpe [mailto:jgg@ziepe.ca]
>Sent: Wednesday, November 15, 2017 11:40 AM
>To: Shaikh, Azhar <azhar.shaikh@intel.com>
>Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>; linux-
>integrity@vger.kernel.org
>Subject: Re: [PATCH RFC v2 2/2] tpm_tis: Move ilb_base_addr to
>tpm_tis_tcg_phy
>
>On Wed, Nov 15, 2017 at 07:36:47PM +0000, Shaikh, Azhar wrote:
>
>> If I implement is_bsw() as below and move it(is_bsw()) outside the #ifdef
>CONFIG_X86, I will still get compilation errors for non-x86 platforms, since
>INTEL_FAM6_ATOM_AIRMONT will be undefined.
>>
>> bool is_bsw(void)
>> {
>>     if (!IS_ENABLED(CONFIG_X86))
>>         return false;
>> return ((boot_cpu_data.x86_model == INTEL_FAM6_ATOM_AIRMONT) ? 1
>: 0);
>> }
>>
>> I think I will have to keep is_bsw() implementation unchanged?
>
>No, still move the #ifdef to is_bsw, just like this instead:
>
>bool is_bsw(void)
>{
>#ifdef CONFIG_X86
>     return ((boot_cpu_data.x86_model == INTEL_FAM6_ATOM_AIRMONT) ? 1
>     : 0);
>#else
>     return false;
>#endif
>}
>


Ok, sure.
Should have thought myself about this... :(

>Jason