On Thu, Jun 08, 2017 at 01:11:43AM +0000, Shaikh, Azhar wrote:

quoted

-----Original Message-----
From: Alan Cox [mailto:gnomes at lxorguk.ukuu.org.uk]
Sent: Wednesday, June 7, 2017 2:45 PM
To: Shaikh, Azhar <redacted>
Cc: jarkko.sakkinen at linux.intel.com; jgunthorpe at obsidianresearch.com;
tpmdd-devel at lists.sourceforge.net; linux-kernel at vger.kernel.org; linux-
security-module at vger.kernel.org
Subject: Re: [PATCH v3] tpm: Enable CLKRUN protocol for Braswell systems

quoted

+++ 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 :

quoted

+0); } #else static inline bool is_bsw(void) {
+	return false;
+}
+#endif

This isn't the only bit that is x86 specific

quoted

+
+#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.

quoted

quoted

+}
+
+/**
+ * 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

quoted

+
 static int tpm_tcg_read_bytes(struct tpm_tis_data *data, u32 addr, u16

len,

quoted

 			      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.

quoted

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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html