| Message ID | 20171220113538.16099-3-javierm@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
>-----Original Message----- >From: Javier Martinez Canillas [mailto:javierm@redhat.com] >Sent: Wednesday, December 20, 2017 3:36 AM >To: linux-kernel@vger.kernel.org >Cc: James Ettle <james@ettle.org.uk>; Hans de Goede ><hdegoede@redhat.com>; Shaikh, Azhar <azhar.shaikh@intel.com>; Javier >Martinez Canillas <javierm@redhat.com>; Arnd Bergmann <arnd@arndb.de>; >Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>; Peter Huewe ><peterhuewe@gmx.de>; Jason Gunthorpe <jgg@ziepe.ca>; Greg Kroah- >Hartman <gregkh@linuxfoundation.org>; linux-integrity@vger.kernel.org >Subject: [PATCH 2/4] tpm: delete the TPM_TIS_CLK_ENABLE flag > >This flag is only used to warn if CLKRUN_EN wasn't disabled on Braswell >systems, but the only way this can happen is if the code is not correct. > >So it's an unnecessary check that just makes the code harder to read. > Hi Javier, This code was implemented as a suggestion from Jason on the previous patches. https://www.spinics.net/lists/linux-integrity/msg00827.html >Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> >Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> >--- > > drivers/char/tpm/tpm_tis.c | 15 --------------- > drivers/char/tpm/tpm_tis_core.c | 2 -- drivers/char/tpm/tpm_tis_core.h | 1 >- > 3 files changed, 18 deletions(-) > >diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c index >d29add49b033..d0ad9a89b02b 100644 >--- a/drivers/char/tpm/tpm_tis.c >+++ b/drivers/char/tpm/tpm_tis.c >@@ -138,9 +138,6 @@ static int tpm_tcg_read_bytes(struct tpm_tis_data >*data, u32 addr, u16 len, { > struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data); > >- if (is_bsw() && !(data->flags & TPM_TIS_CLK_ENABLE)) >- WARN(1, "CLKRUN not enabled!\n"); >- > while (len--) > *result++ = ioread8(phy->iobase + addr); > >@@ -152,9 +149,6 @@ static int tpm_tcg_write_bytes(struct tpm_tis_data >*data, u32 addr, u16 len, { > struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data); > >- if (is_bsw() && !(data->flags & TPM_TIS_CLK_ENABLE)) >- WARN(1, "CLKRUN not enabled!\n"); >- > while (len--) > iowrite8(*value++, phy->iobase + addr); > >@@ -165,9 +159,6 @@ static int tpm_tcg_read16(struct tpm_tis_data *data, >u32 addr, u16 *result) { > struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data); > >- if (is_bsw() && !(data->flags & TPM_TIS_CLK_ENABLE)) >- WARN(1, "CLKRUN not enabled!\n"); >- > *result = ioread16(phy->iobase + addr); > > return 0; >@@ -177,9 +168,6 @@ static int tpm_tcg_read32(struct tpm_tis_data *data, >u32 addr, u32 *result) { > struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data); > >- if (is_bsw() && !(data->flags & TPM_TIS_CLK_ENABLE)) >- WARN(1, "CLKRUN not enabled!\n"); >- > *result = ioread32(phy->iobase + addr); > > return 0; >@@ -189,9 +177,6 @@ static int tpm_tcg_write32(struct tpm_tis_data *data, >u32 addr, u32 value) { > struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data); > >- if (is_bsw() && !(data->flags & TPM_TIS_CLK_ENABLE)) >- WARN(1, "CLKRUN not enabled!\n"); >- > iowrite32(value, phy->iobase + addr); > > return 0; >diff --git a/drivers/char/tpm/tpm_tis_core.c >b/drivers/char/tpm/tpm_tis_core.c index 3455abbb2035..09da1e1adc40 >100644 >--- a/drivers/char/tpm/tpm_tis_core.c >+++ b/drivers/char/tpm/tpm_tis_core.c >@@ -746,7 +746,6 @@ static void tpm_tis_clkrun_enable(struct tpm_chip >*chip, bool value) > return; > > if (value) { >- data->flags |= TPM_TIS_CLK_ENABLE; > data->clkrun_enabled++; > if (data->clkrun_enabled > 1) > return; >@@ -777,7 +776,6 @@ static void tpm_tis_clkrun_enable(struct tpm_chip >*chip, bool value) > * sure LPC clock is running before sending any TPM >command. > */ > outb(0xCC, 0x80); >- data->flags &= ~TPM_TIS_CLK_ENABLE; > } > } > >diff --git a/drivers/char/tpm/tpm_tis_core.h >b/drivers/char/tpm/tpm_tis_core.h index afc50cde1ba6..d5c6a2e952b3 >100644 >--- a/drivers/char/tpm/tpm_tis_core.h >+++ b/drivers/char/tpm/tpm_tis_core.h >@@ -86,7 +86,6 @@ enum tis_defaults { > > enum tpm_tis_flags { > TPM_TIS_ITPM_WORKAROUND = BIT(0), >- TPM_TIS_CLK_ENABLE = BIT(1), > }; > > struct tpm_tis_data { >-- >2.14.3
On Wed, Dec 20, 2017 at 03:19:19PM +0000, Shaikh, Azhar wrote: > >This flag is only used to warn if CLKRUN_EN wasn't disabled on Braswell > >systems, but the only way this can happen is if the code is not correct. > > > >So it's an unnecessary check that just makes the code harder to read. > > This code was implemented as a suggestion from Jason on the previous patches. > https://www.spinics.net/lists/linux-integrity/msg00827.html The concept was to be like ASSERT_RTNL, maybe it just needs a suitably named static inline to addrress Javier's readability concerns? Jason
On 12/20/2017 07:10 PM, Jason Gunthorpe wrote: > On Wed, Dec 20, 2017 at 03:19:19PM +0000, Shaikh, Azhar wrote: >>> This flag is only used to warn if CLKRUN_EN wasn't disabled on Braswell >>> systems, but the only way this can happen is if the code is not correct. >>> >>> So it's an unnecessary check that just makes the code harder to read. >> >> This code was implemented as a suggestion from Jason on the previous patches. >> https://www.spinics.net/lists/linux-integrity/msg00827.html > > The concept was to be like ASSERT_RTNL, maybe it just needs a suitably > named static inline to addrress Javier's readability concerns? > I really think is not worth it and pollutes all the tpm_tcg_{read,write} functions with those is_bsw() and flags checks. Your example is different since is a core API used by in lot of places in the kernel, but it's not the case here. But I don't have a strong opinion either, it was Jarkko who questioned the value of the flag so I can drop this patch too if you disagree with the change. I'm mostly interested in PATCH 4/4 that's the actual fix. > Jason > Best regards,
On Wed, Dec 20, 2017 at 12:35:36PM +0100, Javier Martinez Canillas wrote: > This flag is only used to warn if CLKRUN_EN wasn't disabled on Braswell > systems, but the only way this can happen is if the code is not correct. > > So it's an unnecessary check that just makes the code harder to read. > > Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> /Jarkko
On Wed, Dec 20, 2017 at 07:26:03PM +0100, Javier Martinez Canillas wrote: > On 12/20/2017 07:10 PM, Jason Gunthorpe wrote: > > On Wed, Dec 20, 2017 at 03:19:19PM +0000, Shaikh, Azhar wrote: > >>> This flag is only used to warn if CLKRUN_EN wasn't disabled on Braswell > >>> systems, but the only way this can happen is if the code is not correct. > >>> > >>> So it's an unnecessary check that just makes the code harder to read. > >> > >> This code was implemented as a suggestion from Jason on the previous patches. > >> https://www.spinics.net/lists/linux-integrity/msg00827.html > > > > The concept was to be like ASSERT_RTNL, maybe it just needs a suitably > > named static inline to addrress Javier's readability concerns? > > > > I really think is not worth it and pollutes all the tpm_tcg_{read,write} > functions with those is_bsw() and flags checks. Your example is different > since is a core API used by in lot of places in the kernel, but it's not > the case here. > > But I don't have a strong opinion either, it was Jarkko who questioned > the value of the flag so I can drop this patch too if you disagree with > the change. I'm mostly interested in PATCH 4/4 that's the actual fix. Not going to fight over this one. I would apply the patch but if there is strong opposition I can reconsider. /Jarkko
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c index d29add49b033..d0ad9a89b02b 100644 --- a/drivers/char/tpm/tpm_tis.c +++ b/drivers/char/tpm/tpm_tis.c @@ -138,9 +138,6 @@ static int tpm_tcg_read_bytes(struct tpm_tis_data *data, u32 addr, u16 len, { struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data); - if (is_bsw() && !(data->flags & TPM_TIS_CLK_ENABLE)) - WARN(1, "CLKRUN not enabled!\n"); - while (len--) *result++ = ioread8(phy->iobase + addr); @@ -152,9 +149,6 @@ static int tpm_tcg_write_bytes(struct tpm_tis_data *data, u32 addr, u16 len, { struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data); - if (is_bsw() && !(data->flags & TPM_TIS_CLK_ENABLE)) - WARN(1, "CLKRUN not enabled!\n"); - while (len--) iowrite8(*value++, phy->iobase + addr); @@ -165,9 +159,6 @@ static int tpm_tcg_read16(struct tpm_tis_data *data, u32 addr, u16 *result) { struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data); - if (is_bsw() && !(data->flags & TPM_TIS_CLK_ENABLE)) - WARN(1, "CLKRUN not enabled!\n"); - *result = ioread16(phy->iobase + addr); return 0; @@ -177,9 +168,6 @@ static int tpm_tcg_read32(struct tpm_tis_data *data, u32 addr, u32 *result) { struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data); - if (is_bsw() && !(data->flags & TPM_TIS_CLK_ENABLE)) - WARN(1, "CLKRUN not enabled!\n"); - *result = ioread32(phy->iobase + addr); return 0; @@ -189,9 +177,6 @@ static int tpm_tcg_write32(struct tpm_tis_data *data, u32 addr, u32 value) { struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data); - if (is_bsw() && !(data->flags & TPM_TIS_CLK_ENABLE)) - WARN(1, "CLKRUN not enabled!\n"); - iowrite32(value, phy->iobase + addr); return 0; diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c index 3455abbb2035..09da1e1adc40 100644 --- a/drivers/char/tpm/tpm_tis_core.c +++ b/drivers/char/tpm/tpm_tis_core.c @@ -746,7 +746,6 @@ static void tpm_tis_clkrun_enable(struct tpm_chip *chip, bool value) return; if (value) { - data->flags |= TPM_TIS_CLK_ENABLE; data->clkrun_enabled++; if (data->clkrun_enabled > 1) return; @@ -777,7 +776,6 @@ static void tpm_tis_clkrun_enable(struct tpm_chip *chip, bool value) * sure LPC clock is running before sending any TPM command. */ outb(0xCC, 0x80); - data->flags &= ~TPM_TIS_CLK_ENABLE; } } diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h index afc50cde1ba6..d5c6a2e952b3 100644 --- a/drivers/char/tpm/tpm_tis_core.h +++ b/drivers/char/tpm/tpm_tis_core.h @@ -86,7 +86,6 @@ enum tis_defaults { enum tpm_tis_flags { TPM_TIS_ITPM_WORKAROUND = BIT(0), - TPM_TIS_CLK_ENABLE = BIT(1), }; struct tpm_tis_data {
This flag is only used to warn if CLKRUN_EN wasn't disabled on Braswell systems, but the only way this can happen is if the code is not correct. So it's an unnecessary check that just makes the code harder to read. Suggested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> --- drivers/char/tpm/tpm_tis.c | 15 --------------- drivers/char/tpm/tpm_tis_core.c | 2 -- drivers/char/tpm/tpm_tis_core.h | 1 - 3 files changed, 18 deletions(-)