| Message ID | 1496870610-29462-1-git-send-email-azhar.shaikh@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----Original Message----- > From: Alan Cox [mailto:gnomes@lxorguk.ukuu.org.uk] > Sent: Wednesday, June 7, 2017 2:45 PM > To: Shaikh, Azhar <azhar.shaikh@intel.com> > Cc: jarkko.sakkinen@linux.intel.com; jgunthorpe@obsidianresearch.com; > tpmdd-devel@lists.sourceforge.net; linux-kernel@vger.kernel.org; linux- > security-module@vger.kernel.org > Subject: Re: [PATCH v3] tpm: Enable CLKRUN protocol for Braswell systems > > > +++ b/drivers/char/tpm/tpm_tis.c > > @@ -89,13 +89,89 @@ static inline int is_itpm(struct acpi_device *dev) > > } #endif > > > > +#ifdef CONFIG_X86 > > +static inline bool is_bsw(void) > > +{ > > + return ((boot_cpu_data.x86_model == > INTEL_FAM6_ATOM_AIRMONT) ? 1 : > > +0); } #else static inline bool is_bsw(void) { > > + return false; > > +} > > +#endif > > This isn't the only bit that is x86 specific > > > + > > +#define INTEL_LEGACY_BLK_BASE_ADDR 0xFED08000 > > +#define ILB_REMAP_SIZE 0x100 > > +#define LPC_CNTRL_REG_OFFSET 0x84 > > +#define LPC_CLKRUN_EN (1 << 2) > > + > > +void __iomem *ilb_base_addr; > > + > > +/** > > + * disable_lpc_clk_run() - clear LPC CLKRUN_EN i.e. clocks will be > > +free running */ static void disable_lpc_clk_run(void) { > > + u32 clkrun_val; > > + > > + if (!is_bsw()) > > + return; > > + > > + clkrun_val = ioread32(ilb_base_addr + LPC_CNTRL_REG_OFFSET); > > + > > + /* Disable LPC CLKRUN# */ > > + clkrun_val &= ~LPC_CLKRUN_EN; > > + iowrite32(clkrun_val, ilb_base_addr + LPC_CNTRL_REG_OFFSET); > > + > > + /* > > + * Write any random value on port 0x80 which is on LPC, to make > > + * sure LPC clock is running before sending any TPM command. > > + */ > > + outb(0x80, 0xCC); > > + > > + /* Make sure the above write is completed */ > > + wmb(); > > Why the wmb(). It doesn't do what the comment says! Also this code is x86 > specific > > Memory barrier to enforce the order so that the outb() is completed, which ensures that the LPC clocks are running before sending any TPM command. > > +} > > + > > +/** > > + * enable_lpc_clk_run() - set LPC CLKRUN_EN i.e. clocks can be turned > > +off */ static void enable_lpc_clk_run(void) { > > + u32 clkrun_val; > > + > > + if (!is_bsw()) > > + return; > > + > > + clkrun_val = ioread32(ilb_base_addr + LPC_CNTRL_REG_OFFSET); > > + > > + /* Enable LPC CLKRUN# */ > > + clkrun_val |= LPC_CLKRUN_EN; > > + iowrite32(clkrun_val, ilb_base_addr + LPC_CNTRL_REG_OFFSET); > > + > > + /* > > + * Write any random value on port 0x80 which is on LPC, to make > > + * sure LPC clock is running before sending any TPM command. > > + */ > > + outb(0x80, 0xCC); > > + > > + /* Make sure the above write is completed */ > > + wmb(); > > +} > > Same > > > + > > static int tpm_tcg_read_bytes(struct tpm_tis_data *data, u32 addr, u16 > len, > > u8 *result) > > { > > struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data); > > > > + disable_lpc_clk_run(); > > + > > while (len--) > > *result++ = ioread8(phy->iobase + addr); > > + > > + enable_lpc_clk_run(); > > + > > return 0; > > } > > So what you actually want to do is fold all the errata crap into an x86 specific > chunk and just define disable/enable_lpc_clk_run() as null functions on > everything else. > Ok, will do. > I'd pick better names too - if other platforms need a hook here it won't I > imagine be about LPC. Possibly you want names like > > platform_begin_tpm_xfer(data); > platform_end_tpm_xfer(data); > How about these? Since most of the functions in this driver begin with 'tpm_' disable_lpc_clk_run() - > tpm_start_xfer() enable_lpc_clk_run() -> tpm_end_xfer() > > > > @@ -104,8 +180,13 @@ 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); > > > > + disable_lpc_clk_run(); > > + > > while (len--) > > iowrite8(*value++, phy->iobase + addr); > > + > > + enable_lpc_clk_run(); > > + > > return 0; > > } > > > > @@ -113,7 +194,12 @@ 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); > > > > + disable_lpc_clk_run(); > > + > > *result = ioread16(phy->iobase + addr); > > + > > + enable_lpc_clk_run(); > > + > > return 0; > > } > > > > @@ -121,7 +207,12 @@ 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); > > > > + disable_lpc_clk_run(); > > + > > *result = ioread32(phy->iobase + addr); > > + > > + enable_lpc_clk_run(); > > + > > return 0; > > } > > > > @@ -129,7 +220,12 @@ 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); > > > > + disable_lpc_clk_run(); > > + > > iowrite32(value, phy->iobase + addr); > > + > > + enable_lpc_clk_run(); > > + > > return 0; > > } > > > > @@ -191,6 +287,10 @@ static int tpm_tis_pnp_init(struct pnp_dev > *pnp_dev, > > acpi_dev_handle = ACPI_HANDLE(&pnp_dev->dev); > > } > > > > + if (is_bsw()) > > + ilb_base_addr = ioremap(INTEL_LEGACY_BLK_BASE_ADDR, > > + ILB_REMAP_SIZE); > > + > > This suggests to me that the bsw stuff wants to wrap the standard methods > because it's weird and ugly having random magic hardware globals in what > should be standard code. > > > return tpm_tis_init(&pnp_dev->dev, &tpm_info, acpi_dev_handle); > } > > > > @@ -214,6 +314,9 @@ static void tpm_tis_pnp_remove(struct pnp_dev > > *dev) > > > > tpm_chip_unregister(chip); > > tpm_tis_remove(chip); > > + > > + if (is_bsw()) > > + iounmap(ilb_base_addr); > > } > > > > static struct pnp_driver tis_pnp_driver = { ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On Thu, Jun 08, 2017 at 01:11:43AM +0000, Shaikh, Azhar wrote: > > > > -----Original Message----- > > From: Alan Cox [mailto:gnomes@lxorguk.ukuu.org.uk] > > Sent: Wednesday, June 7, 2017 2:45 PM > > To: Shaikh, Azhar <azhar.shaikh@intel.com> > > Cc: jarkko.sakkinen@linux.intel.com; jgunthorpe@obsidianresearch.com; > > tpmdd-devel@lists.sourceforge.net; linux-kernel@vger.kernel.org; linux- > > security-module@vger.kernel.org > > Subject: Re: [PATCH v3] tpm: Enable CLKRUN protocol for Braswell systems > > > > > +++ b/drivers/char/tpm/tpm_tis.c > > > @@ -89,13 +89,89 @@ static inline int is_itpm(struct acpi_device *dev) > > > } #endif > > > > > > +#ifdef CONFIG_X86 > > > +static inline bool is_bsw(void) > > > +{ > > > + return ((boot_cpu_data.x86_model == > > INTEL_FAM6_ATOM_AIRMONT) ? 1 : > > > +0); } #else static inline bool is_bsw(void) { > > > + return false; > > > +} > > > +#endif > > > > This isn't the only bit that is x86 specific > > > > > + > > > +#define INTEL_LEGACY_BLK_BASE_ADDR 0xFED08000 > > > +#define ILB_REMAP_SIZE 0x100 > > > +#define LPC_CNTRL_REG_OFFSET 0x84 > > > +#define LPC_CLKRUN_EN (1 << 2) > > > + > > > +void __iomem *ilb_base_addr; > > > + > > > +/** > > > + * disable_lpc_clk_run() - clear LPC CLKRUN_EN i.e. clocks will be > > > +free running */ static void disable_lpc_clk_run(void) { > > > + u32 clkrun_val; > > > + > > > + if (!is_bsw()) > > > + return; > > > + > > > + clkrun_val = ioread32(ilb_base_addr + LPC_CNTRL_REG_OFFSET); > > > + > > > + /* Disable LPC CLKRUN# */ > > > + clkrun_val &= ~LPC_CLKRUN_EN; > > > + iowrite32(clkrun_val, ilb_base_addr + LPC_CNTRL_REG_OFFSET); > > > + > > > + /* > > > + * Write any random value on port 0x80 which is on LPC, to make > > > + * sure LPC clock is running before sending any TPM command. > > > + */ > > > + outb(0x80, 0xCC); > > > + > > > + /* Make sure the above write is completed */ > > > + wmb(); > > > > Why the wmb(). It doesn't do what the comment says! Also this code is x86 > > specific > > > > > > Memory barrier to enforce the order so that the outb() is completed, which ensures that the LPC clocks are running before sending any TPM command. > > > > +} > > > + > > > +/** > > > + * enable_lpc_clk_run() - set LPC CLKRUN_EN i.e. clocks can be turned > > > +off */ static void enable_lpc_clk_run(void) { > > > + u32 clkrun_val; > > > + > > > + if (!is_bsw()) > > > + return; > > > + > > > + clkrun_val = ioread32(ilb_base_addr + LPC_CNTRL_REG_OFFSET); > > > + > > > + /* Enable LPC CLKRUN# */ > > > + clkrun_val |= LPC_CLKRUN_EN; > > > + iowrite32(clkrun_val, ilb_base_addr + LPC_CNTRL_REG_OFFSET); > > > + > > > + /* > > > + * Write any random value on port 0x80 which is on LPC, to make > > > + * sure LPC clock is running before sending any TPM command. > > > + */ > > > + outb(0x80, 0xCC); > > > + > > > + /* Make sure the above write is completed */ > > > + wmb(); > > > +} > > > > Same > > > > > + > > > static int tpm_tcg_read_bytes(struct tpm_tis_data *data, u32 addr, u16 > > len, > > > u8 *result) > > > { > > > struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data); > > > > > > + disable_lpc_clk_run(); > > > + > > > while (len--) > > > *result++ = ioread8(phy->iobase + addr); > > > + > > > + enable_lpc_clk_run(); > > > + > > > return 0; > > > } > > > > So what you actually want to do is fold all the errata crap into an x86 specific > > chunk and just define disable/enable_lpc_clk_run() as null functions on > > everything else. > > > > Ok, will do. > > > I'd pick better names too - if other platforms need a hook here it won't I > > imagine be about LPC. Possibly you want names like > > > > platform_begin_tpm_xfer(data); > > platform_end_tpm_xfer(data); > > > > How about these? Since most of the functions in this driver begin with 'tpm_' > disable_lpc_clk_run() - > tpm_start_xfer() > enable_lpc_clk_run() -> tpm_end_xfer() tpm_platform_begin_xfer() would be the best alternative as it highlights platform quirk better. /Jarkko ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On Thu, Jun 08, 2017 at 07:22:59PM +0100, Alan Cox wrote: > > > > + outb(0x80, 0xCC); > > > > + > > > > + /* Make sure the above write is completed */ > > > > + wmb(); > > > > > > Why the wmb(). It doesn't do what the comment says! Also this code is x86 > > > specific > > > > > > > > > > Memory barrier to enforce the order so that the outb() is > > completed, which ensures that the LPC clocks are running before > > sending any TPM command. > > wmb() doesn't do that. It merely ensures that the write has been posted > to the fabric. If as I suspect your LPC bus implements outb() as a > non-posted write you don't need the wmb(). I think the point here is to bootstrap the sleeping LPC bus clock before a TPM command is issued, presumably because the auto-wakeup circuit is busted or something. For that purpose all that should be required is strong ordering of the outb relative to the other TPM commands at the LPC interface FIFO. I also think the wmb is not needed because outb is already defined to be strongly in order with respect to writel/readl ? Jason ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> -----Original Message----- > From: Alan Cox [mailto:gnomes@lxorguk.ukuu.org.uk] > Sent: Thursday, June 8, 2017 11:23 AM > To: Shaikh, Azhar <azhar.shaikh@intel.com> > Cc: jarkko.sakkinen@linux.intel.com; jgunthorpe@obsidianresearch.com; > tpmdd-devel@lists.sourceforge.net; linux-kernel@vger.kernel.org; linux- > security-module@vger.kernel.org > Subject: Re: [PATCH v3] tpm: Enable CLKRUN protocol for Braswell systems > > > > > + outb(0x80, 0xCC); > > > > + > > > > + /* Make sure the above write is completed */ > > > > + wmb(); > > > > > > Why the wmb(). It doesn't do what the comment says! Also this code > > > is x86 specific > > > > > > > > > > Memory barrier to enforce the order so that the outb() is completed, > which ensures that the LPC clocks are running before sending any TPM > command. > > wmb() doesn't do that. It merely ensures that the write has been posted to > the fabric. If as I suspect your LPC bus implements outb() as a non-posted > write you don't need the wmb(). If it doesn't then you need to issue > whatever access is needed to the fabric to ensure the post completed (eg for > PCI if you do an MMIO write you must do an MMIO read from the same > devfn). > > Secondly outb(0x80, 0xCC) doesn't write 0xCC to port 0x80. It writes 0x80 to > port 0xCC ! > Oops my bad! I got that reversed. Will change it. > Alan ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> -----Original Message----- > From: Alan Cox [mailto:gnomes@lxorguk.ukuu.org.uk] > Sent: Thursday, June 8, 2017 11:50 AM > To: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > Cc: Shaikh, Azhar <azhar.shaikh@intel.com>; > jarkko.sakkinen@linux.intel.com; tpmdd-devel@lists.sourceforge.net; linux- > kernel@vger.kernel.org; linux-security-module@vger.kernel.org > Subject: Re: [PATCH v3] tpm: Enable CLKRUN protocol for Braswell systems > > > For that purpose all that should be required is strong ordering of the > > outb relative to the other TPM commands at the LPC interface FIFO. I > > also think the wmb is not needed because outb is already defined to be > > strongly in order with respect to writel/readl ? > > That's my assumption but given this is all some kind of 'it's broken' > fixup I thought best to ask. Assuming there is nothing else magical going on > then yes it should be deleted. > As Jason mentioned, outb is already define to be strongly ordered, then wmb is not needed. I will delete it. > Alan ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
On Thu, Jun 08, 2017 at 12:39:20PM -0600, Jason Gunthorpe wrote: > On Thu, Jun 08, 2017 at 07:22:59PM +0100, Alan Cox wrote: > > > > > + outb(0x80, 0xCC); > > > > > + > > > > > + /* Make sure the above write is completed */ > > > > > + wmb(); > > > > > > > > Why the wmb(). It doesn't do what the comment says! Also this code is x86 > > > > specific > > > > > > > > > > > > > > Memory barrier to enforce the order so that the outb() is > > > completed, which ensures that the LPC clocks are running before > > > sending any TPM command. > > > > wmb() doesn't do that. It merely ensures that the write has been posted > > to the fabric. If as I suspect your LPC bus implements outb() as a > > non-posted write you don't need the wmb(). > > I think the point here is to bootstrap the sleeping LPC bus clock > before a TPM command is issued, presumably because the auto-wakeup circuit > is busted or something. > > For that purpose all that should be required is strong ordering of the > outb relative to the other TPM commands at the LPC interface FIFO. I > also think the wmb is not needed because outb is already defined to be > strongly in order with respect to writel/readl ? > > Jason writel AFAIK guarantees by itself strong RW ordering. /Jarkko ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h index 4b4c8dee3096..6b769fbc4407 100644 --- a/drivers/char/tpm/tpm.h +++ b/drivers/char/tpm/tpm.h @@ -36,6 +36,10 @@ #include <linux/highmem.h> #include <crypto/hash_info.h> +#ifdef CONFIG_X86 +#include <asm/intel-family.h> +#endif + enum tpm_const { TPM_MINOR = 224, /* officially assigned */ TPM_BUFSIZE = 4096, diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c index c7e1384f1b08..6e32b4c7c70d 100644 --- a/drivers/char/tpm/tpm_tis.c +++ b/drivers/char/tpm/tpm_tis.c @@ -89,13 +89,89 @@ static inline int is_itpm(struct acpi_device *dev) } #endif +#ifdef CONFIG_X86 +static inline bool is_bsw(void) +{ + return ((boot_cpu_data.x86_model == INTEL_FAM6_ATOM_AIRMONT) ? 1 : 0); +} +#else +static inline bool is_bsw(void) +{ + return false; +} +#endif + +#define INTEL_LEGACY_BLK_BASE_ADDR 0xFED08000 +#define ILB_REMAP_SIZE 0x100 +#define LPC_CNTRL_REG_OFFSET 0x84 +#define LPC_CLKRUN_EN (1 << 2) + +void __iomem *ilb_base_addr; + +/** + * disable_lpc_clk_run() - clear LPC CLKRUN_EN i.e. clocks will be free running + */ +static void disable_lpc_clk_run(void) +{ + u32 clkrun_val; + + if (!is_bsw()) + return; + + clkrun_val = ioread32(ilb_base_addr + LPC_CNTRL_REG_OFFSET); + + /* Disable LPC CLKRUN# */ + clkrun_val &= ~LPC_CLKRUN_EN; + iowrite32(clkrun_val, ilb_base_addr + LPC_CNTRL_REG_OFFSET); + + /* + * Write any random value on port 0x80 which is on LPC, to make + * sure LPC clock is running before sending any TPM command. + */ + outb(0x80, 0xCC); + + /* Make sure the above write is completed */ + wmb(); +} + +/** + * enable_lpc_clk_run() - set LPC CLKRUN_EN i.e. clocks can be turned off + */ +static void enable_lpc_clk_run(void) +{ + u32 clkrun_val; + + if (!is_bsw()) + return; + + clkrun_val = ioread32(ilb_base_addr + LPC_CNTRL_REG_OFFSET); + + /* Enable LPC CLKRUN# */ + clkrun_val |= LPC_CLKRUN_EN; + iowrite32(clkrun_val, ilb_base_addr + LPC_CNTRL_REG_OFFSET); + + /* + * Write any random value on port 0x80 which is on LPC, to make + * sure LPC clock is running before sending any TPM command. + */ + outb(0x80, 0xCC); + + /* Make sure the above write is completed */ + wmb(); +} + static int tpm_tcg_read_bytes(struct tpm_tis_data *data, u32 addr, u16 len, u8 *result) { struct tpm_tis_tcg_phy *phy = to_tpm_tis_tcg_phy(data); + disable_lpc_clk_run(); + while (len--) *result++ = ioread8(phy->iobase + addr); + + enable_lpc_clk_run(); + return 0; } @@ -104,8 +180,13 @@ 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); + disable_lpc_clk_run(); + while (len--) iowrite8(*value++, phy->iobase + addr); + + enable_lpc_clk_run(); + return 0; } @@ -113,7 +194,12 @@ 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); + disable_lpc_clk_run(); + *result = ioread16(phy->iobase + addr); + + enable_lpc_clk_run(); + return 0; } @@ -121,7 +207,12 @@ 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); + disable_lpc_clk_run(); + *result = ioread32(phy->iobase + addr); + + enable_lpc_clk_run(); + return 0; } @@ -129,7 +220,12 @@ 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); + disable_lpc_clk_run(); + iowrite32(value, phy->iobase + addr); + + enable_lpc_clk_run(); + return 0; } @@ -191,6 +287,10 @@ static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev, acpi_dev_handle = ACPI_HANDLE(&pnp_dev->dev); } + if (is_bsw()) + ilb_base_addr = ioremap(INTEL_LEGACY_BLK_BASE_ADDR, + ILB_REMAP_SIZE); + return tpm_tis_init(&pnp_dev->dev, &tpm_info, acpi_dev_handle); } @@ -214,6 +314,9 @@ static void tpm_tis_pnp_remove(struct pnp_dev *dev) tpm_chip_unregister(chip); tpm_tis_remove(chip); + + if (is_bsw()) + iounmap(ilb_base_addr); } static struct pnp_driver tis_pnp_driver = {
To overcome a hardware limitation on Intel Braswell systems, disable CLKRUN protocol during TPM transactions and re-enable once the transaction is completed. Signed-off-by: Azhar Shaikh <azhar.shaikh@intel.com> --- Changes from v1: - Add CONFIG_X86 around disable_lpc_clk_run () and enable_lpc_clk_run() to avoid - build breakage on architectures which do not implement kmap_atomic_pfn() Changes from v2: - Use ioremap()/iounmap() instead of kmap_atomic_pfn()/kunmap_atomic() - Move is_bsw() and all macros from tpm.h to tpm_tis.c file. - Add the is_bsw() check in disable_lpc_clk_run() and enable_lpc_clk_run() - instead of adding it in each read/write API. drivers/char/tpm/tpm.h | 4 ++ drivers/char/tpm/tpm_tis.c | 103 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+)