diff mbox series

[v4,4/6] tpm: tpm_tis_spi: Export functionality to other drivers

Message ID 20190812223622.73297-5-swboyd@chromium.org (mailing list archive)
State New, archived
Headers show
Series tpm: Add driver for cr50 | expand

Commit Message

Stephen Boyd Aug. 12, 2019, 10:36 p.m. UTC 
Export a new function, tpm_tis_spi_init(), and the associated
read/write/transfer APIs so that we can create variant drivers based on
the core functionality of this TPM SPI driver. Variant drivers can wrap
the tpm_tis_spi_phy struct with their own struct and override the
behavior of tpm_tis_spi_transfer() by supplying their own flow control
and pre-transfer hooks. This shares the most code between the core
driver and any variants that want to override certain behavior without
cluttering the core driver.

Cc: Andrey Pronin <apronin@chromium.org>
Cc: Duncan Laurie <dlaurie@chromium.org>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Guenter Roeck <groeck@chromium.org>
Cc: Alexander Steffen <Alexander.Steffen@infineon.com>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/char/tpm/tpm_tis_spi.c | 52 ++++++++++++++++------------------
 drivers/char/tpm/tpm_tis_spi.h | 37 ++++++++++++++++++++++++
 2 files changed, 62 insertions(+), 27 deletions(-)
 create mode 100644 drivers/char/tpm/tpm_tis_spi.h

Comments

Jarkko Sakkinen Aug. 19, 2019, 4:40 p.m. UTC | #1 
On Mon, Aug 12, 2019 at 03:36:20PM -0700, Stephen Boyd wrote:
> Export a new function, tpm_tis_spi_init(), and the associated
> read/write/transfer APIs so that we can create variant drivers based on
> the core functionality of this TPM SPI driver. Variant drivers can wrap
> the tpm_tis_spi_phy struct with their own struct and override the
> behavior of tpm_tis_spi_transfer() by supplying their own flow control
> and pre-transfer hooks. This shares the most code between the core
> driver and any variants that want to override certain behavior without
> cluttering the core driver.

I think this is adding way too much complexity for the purpose. We
definitely do want this three layer architecture here.

Instead there should be a single tpm_tis_spi driver that dynamically
either TCG or CR50. I rather take some extra bytes in the LKM than
the added complexity.

/Jarkko
Stephen Boyd Aug. 19, 2019, 5:10 p.m. UTC | #2 
Quoting Jarkko Sakkinen (2019-08-19 09:40:05)
> On Mon, Aug 12, 2019 at 03:36:20PM -0700, Stephen Boyd wrote:
> > Export a new function, tpm_tis_spi_init(), and the associated
> > read/write/transfer APIs so that we can create variant drivers based on
> > the core functionality of this TPM SPI driver. Variant drivers can wrap
> > the tpm_tis_spi_phy struct with their own struct and override the
> > behavior of tpm_tis_spi_transfer() by supplying their own flow control
> > and pre-transfer hooks. This shares the most code between the core
> > driver and any variants that want to override certain behavior without
> > cluttering the core driver.
> 
> I think this is adding way too much complexity for the purpose. We
> definitely do want this three layer architecture here.
> 
> Instead there should be a single tpm_tis_spi driver that dynamically
> either TCG or CR50. I rather take some extra bytes in the LKM than
> the added complexity.
> 

Ok. I had that patch originally[1]. Do you want me to resend that patch
and start review over from there?

[1] https://lkml.kernel.org/r/5d2f955d.1c69fb81.35877.7018@mx.google.com
Jarkko Sakkinen Aug. 21, 2019, 5:58 p.m. UTC | #3 
On Mon, Aug 19, 2019 at 10:10:08AM -0700, Stephen Boyd wrote:
> Quoting Jarkko Sakkinen (2019-08-19 09:40:05)
> > On Mon, Aug 12, 2019 at 03:36:20PM -0700, Stephen Boyd wrote:
> > > Export a new function, tpm_tis_spi_init(), and the associated
> > > read/write/transfer APIs so that we can create variant drivers based on
> > > the core functionality of this TPM SPI driver. Variant drivers can wrap
> > > the tpm_tis_spi_phy struct with their own struct and override the
> > > behavior of tpm_tis_spi_transfer() by supplying their own flow control
> > > and pre-transfer hooks. This shares the most code between the core
> > > driver and any variants that want to override certain behavior without
> > > cluttering the core driver.
> > 
> > I think this is adding way too much complexity for the purpose. We
> > definitely do want this three layer architecture here.
> > 
> > Instead there should be a single tpm_tis_spi driver that dynamically
> > either TCG or CR50. I rather take some extra bytes in the LKM than
> > the added complexity.
> > 
> 
> Ok. I had that patch originally[1]. Do you want me to resend that patch
> and start review over from there?
> 
> [1] https://lkml.kernel.org/r/5d2f955d.1c69fb81.35877.7018@mx.google.com

What if:

1. You mostly use this solution but have it as a separate source module
   only.
2. Use TPM_IS_CR50 only once to bind the callbacks.

/Jarkko
Stephen Boyd Aug. 22, 2019, 5:29 p.m. UTC | #4 
Quoting Jarkko Sakkinen (2019-08-21 10:58:46)
> On Mon, Aug 19, 2019 at 10:10:08AM -0700, Stephen Boyd wrote:
> > Quoting Jarkko Sakkinen (2019-08-19 09:40:05)
> > > 
> > > Instead there should be a single tpm_tis_spi driver that dynamically
> > > either TCG or CR50. I rather take some extra bytes in the LKM than
> > > the added complexity.
> > > 
> > 
> > Ok. I had that patch originally[1]. Do you want me to resend that patch
> > and start review over from there?
> > 
> > [1] https://lkml.kernel.org/r/5d2f955d.1c69fb81.35877.7018@mx.google.com
> 
> What if:
> 
> 1. You mostly use this solution but have it as a separate source module
>    only.
> 2. Use TPM_IS_CR50 only once to bind the callbacks.
> 

Ok I think I understand. Take the callback approach from these patches
and combine that with the TPM_IS_CR50 changes I made in [1]. I'll try it
out and resend.
diff mbox series

Patch

diff --git a/drivers/char/tpm/tpm_tis_spi.c b/drivers/char/tpm/tpm_tis_spi.c
index 93f49b1941f0..fd3fb4f9f506 100644
--- a/drivers/char/tpm/tpm_tis_spi.c
+++ b/drivers/char/tpm/tpm_tis_spi.c
@@ -36,23 +36,10 @@ 
 #include <linux/tpm.h>
 #include "tpm.h"
 #include "tpm_tis_core.h"
+#include "tpm_tis_spi.h"
 
 #define MAX_SPI_FRAMESIZE 64
 
-struct tpm_tis_spi_phy {
-	struct tpm_tis_data priv;
-	struct spi_device *spi_device;
-	int (*flow_control)(struct tpm_tis_spi_phy *phy,
-			    struct spi_transfer *xfer);
-	void (*pre_transfer)(struct tpm_tis_spi_phy *phy);
-	u8 *iobuf;
-};
-
-static inline struct tpm_tis_spi_phy *to_tpm_tis_spi_phy(struct tpm_tis_data *data)
-{
-	return container_of(data, struct tpm_tis_spi_phy, priv);
-}
-
 static int tpm_tis_spi_flow_control(struct tpm_tis_spi_phy *phy,
 				    struct spi_transfer *spi_xfer)
 {
@@ -81,7 +68,7 @@  static int tpm_tis_spi_flow_control(struct tpm_tis_spi_phy *phy,
 	return 0;
 }
 
-static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
+int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
 				u8 *in, const u8 *out)
 {
 	struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
@@ -148,9 +135,10 @@  static int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
 	spi_bus_unlock(phy->spi_device->master);
 	return ret;
 }
+EXPORT_SYMBOL_GPL(tpm_tis_spi_transfer);
 
-static int tpm_tis_spi_read_bytes(struct tpm_tis_data *data, u32 addr,
-				  u16 len, u8 *result)
+static int tpm_tis_spi_read_bytes(struct tpm_tis_data *data, u32 addr, u16 len,
+				  u8 *result)
 {
 	return tpm_tis_spi_transfer(data, addr, len, result, NULL);
 }
@@ -161,7 +149,7 @@  static int tpm_tis_spi_write_bytes(struct tpm_tis_data *data, u32 addr,
 	return tpm_tis_spi_transfer(data, addr, len, NULL, value);
 }
 
-static int tpm_tis_spi_read16(struct tpm_tis_data *data, u32 addr, u16 *result)
+int tpm_tis_spi_read16(struct tpm_tis_data *data, u32 addr, u16 *result)
 {
 	__le16 result_le;
 	int rc;
@@ -173,8 +161,9 @@  static int tpm_tis_spi_read16(struct tpm_tis_data *data, u32 addr, u16 *result)
 
 	return rc;
 }
+EXPORT_SYMBOL_GPL(tpm_tis_spi_read16);
 
-static int tpm_tis_spi_read32(struct tpm_tis_data *data, u32 addr, u32 *result)
+int tpm_tis_spi_read32(struct tpm_tis_data *data, u32 addr, u32 *result)
 {
 	__le32 result_le;
 	int rc;
@@ -186,8 +175,9 @@  static int tpm_tis_spi_read32(struct tpm_tis_data *data, u32 addr, u32 *result)
 
 	return rc;
 }
+EXPORT_SYMBOL_GPL(tpm_tis_spi_read32);
 
-static int tpm_tis_spi_write32(struct tpm_tis_data *data, u32 addr, u32 value)
+int tpm_tis_spi_write32(struct tpm_tis_data *data, u32 addr, u32 value)
 {
 	__le32 value_le;
 	int rc;
@@ -198,6 +188,20 @@  static int tpm_tis_spi_write32(struct tpm_tis_data *data, u32 addr, u32 value)
 
 	return rc;
 }
+EXPORT_SYMBOL_GPL(tpm_tis_spi_write32);
+
+int tpm_tis_spi_init(struct spi_device *spi, struct tpm_tis_spi_phy *phy,
+		     int irq, const struct tpm_tis_phy_ops *phy_ops)
+{
+	phy->iobuf = devm_kmalloc(&spi->dev, MAX_SPI_FRAMESIZE, GFP_KERNEL);
+	if (!phy->iobuf)
+		return -ENOMEM;
+
+	phy->spi_device = spi;
+
+	return tpm_tis_core_init(&spi->dev, &phy->priv, irq, phy_ops, NULL);
+}
+EXPORT_SYMBOL_GPL(tpm_tis_spi_init);
 
 static const struct tpm_tis_phy_ops tpm_spi_phy_ops = {
 	.read_bytes = tpm_tis_spi_read_bytes,
@@ -217,11 +221,6 @@  static int tpm_tis_spi_probe(struct spi_device *dev)
 	if (!phy)
 		return -ENOMEM;
 
-	phy->spi_device = dev;
-
-	phy->iobuf = devm_kmalloc(&dev->dev, MAX_SPI_FRAMESIZE, GFP_KERNEL);
-	if (!phy->iobuf)
-		return -ENOMEM;
 	phy->flow_control = tpm_tis_spi_flow_control;
 
 	/* If the SPI device has an IRQ then use that */
@@ -230,8 +229,7 @@  static int tpm_tis_spi_probe(struct spi_device *dev)
 	else
 		irq = -1;
 
-	return tpm_tis_core_init(&dev->dev, &phy->priv, irq, &tpm_spi_phy_ops,
-				 NULL);
+	return tpm_tis_spi_init(dev, phy, irq, &tpm_spi_phy_ops);
 }
 
 static SIMPLE_DEV_PM_OPS(tpm_tis_pm, tpm_pm_suspend, tpm_tis_resume);
diff --git a/drivers/char/tpm/tpm_tis_spi.h b/drivers/char/tpm/tpm_tis_spi.h
new file mode 100644
index 000000000000..48be5130794a
--- /dev/null
+++ b/drivers/char/tpm/tpm_tis_spi.h
@@ -0,0 +1,37 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2015 Infineon Technologies AG
+ * Copyright (C) 2016 STMicroelectronics SAS
+ */
+
+#ifndef TPM_TIS_SPI_H
+#define TPM_TIS_SPI_H
+
+#include "tpm_tis_core.h"
+
+struct tpm_tis_spi_phy {
+	struct tpm_tis_data priv;
+	struct spi_device *spi_device;
+	int (*flow_control)(struct tpm_tis_spi_phy *phy,
+			     struct spi_transfer *xfer);
+	void (*pre_transfer)(struct tpm_tis_spi_phy *phy);
+
+	u8 *iobuf;
+};
+
+static inline struct tpm_tis_spi_phy *to_tpm_tis_spi_phy(struct tpm_tis_data *data)
+{
+	return container_of(data, struct tpm_tis_spi_phy, priv);
+}
+
+extern int tpm_tis_spi_init(struct spi_device *spi, struct tpm_tis_spi_phy *phy,
+			    int irq, const struct tpm_tis_phy_ops *phy_ops);
+
+extern int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
+				u8 *in, const u8 *out);
+
+extern int tpm_tis_spi_read16(struct tpm_tis_data *data, u32 addr, u16 *result);
+extern int tpm_tis_spi_read32(struct tpm_tis_data *data, u32 addr, u32 *result);
+extern int tpm_tis_spi_write32(struct tpm_tis_data *data, u32 addr, u32 value);
+
+#endif