[PATCH] tpm: move struct tpm_class_ops to drivers/char/tpm/tpm.h

[PATCH] tpm: move struct tpm_class_ops to drivers/char/tpm/tpm.h

[Jarkko Sakkinen]

The struct tpm_class_ops is not used outside the TPM driver. Thus,
it can be safely move to drivers/char/tpm/tpm.h.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm.h | 13 +++++++++++++
 include/linux/tpm.h    | 14 --------------
 2 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 3e952fb..e1d277d 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -138,6 +138,19 @@ enum tpm2_startup_types {
 
 #define TPM_PPI_VERSION_LEN		3
 
+struct tpm_class_ops {
+	unsigned int flags;
+	const u8 req_complete_mask;
+	const u8 req_complete_val;
+	bool (*req_canceled)(struct tpm_chip *chip, u8 status);
+	int (*recv) (struct tpm_chip *chip, u8 *buf, size_t len);
+	int (*send) (struct tpm_chip *chip, u8 *buf, size_t len);
+	void (*cancel) (struct tpm_chip *chip);
+	u8 (*status) (struct tpm_chip *chip);
+	bool (*update_timeouts)(struct tpm_chip *chip,
+				unsigned long *timeout_cap);
+};
+
 enum tpm_chip_flags {
 	TPM_CHIP_FLAG_REGISTERED	= BIT(0),
 	TPM_CHIP_FLAG_TPM2		= BIT(1),
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index da158f0..76ba592 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -37,20 +37,6 @@ enum TPM_OPS_FLAGS {
 	TPM_OPS_AUTO_STARTUP = BIT(0),
 };
 
-struct tpm_class_ops {
-	unsigned int flags;
-	const u8 req_complete_mask;
-	const u8 req_complete_val;
-	bool (*req_canceled)(struct tpm_chip *chip, u8 status);
-	int (*recv) (struct tpm_chip *chip, u8 *buf, size_t len);
-	int (*send) (struct tpm_chip *chip, u8 *buf, size_t len);
-	void (*cancel) (struct tpm_chip *chip);
-	u8 (*status) (struct tpm_chip *chip);
-	bool (*update_timeouts)(struct tpm_chip *chip,
-				unsigned long *timeout_cap);
-
-};
-
 #if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE)
 
 extern int tpm_is_tpm2(u32 chip_num);
-- 
2.7.4

Re: [PATCH] tpm: move struct tpm_class_ops to drivers/char/tpm/tpm.h

[Jason Gunthorpe]

On Sat, Sep 03, 2016 at 12:48:03AM +0300, Jarkko Sakkinen wrote:
> The struct tpm_class_ops is not used outside the TPM driver. Thus,
> it can be safely move to drivers/char/tpm/tpm.h.

No, this is the wrong direction.

The goal is to make things more like other subsystems, so we should be
moving struct tpm_chip into the public header, and that requires ops
to be in the public header.

This is why I put ops here in the first place.

Jason

Re: [PATCH] tpm: move struct tpm_class_ops to drivers/char/tpm/tpm.h

[Jarkko Sakkinen]

On Fri, Sep 02, 2016 at 04:11:22PM -0600, Jason Gunthorpe wrote:
> On Sat, Sep 03, 2016 at 12:48:03AM +0300, Jarkko Sakkinen wrote:
> > The struct tpm_class_ops is not used outside the TPM driver. Thus,
> > it can be safely move to drivers/char/tpm/tpm.h.
> 
> No, this is the wrong direction.
> 
> The goal is to make things more like other subsystems, so we should be
> moving struct tpm_chip into the public header, and that requires ops
> to be in the public header.
> 
> This is why I put ops here in the first place.

I'm OK with it as long as you explain why this is necessary. I see no
use for them outside the TPM subsystem.

> Jason

/Jarkko

Re: [PATCH] tpm: move struct tpm_class_ops to drivers/char/tpm/tpm.h

[Jason Gunthorpe]

On Sat, Sep 03, 2016 at 01:35:22AM +0300, Jarkko Sakkinen wrote:
> On Fri, Sep 02, 2016 at 04:11:22PM -0600, Jason Gunthorpe wrote:
> > On Sat, Sep 03, 2016 at 12:48:03AM +0300, Jarkko Sakkinen wrote:
> > > The struct tpm_class_ops is not used outside the TPM driver. Thus,
> > > it can be safely move to drivers/char/tpm/tpm.h.
> > 
> > No, this is the wrong direction.
> > 
> > The goal is to make things more like other subsystems, so we should be
> > moving struct tpm_chip into the public header, and that requires ops
> > to be in the public header.
> > 
> > This is why I put ops here in the first place.
> 
> I'm OK with it as long as you explain why this is necessary. I see no
> use for them outside the TPM subsystem.

That is because the users out side the subsystem are Doing it Wrong.

eg this:

 extern int tpm_is_tpm2(u32 chip_num);

Should be:

 extern int tpm_is_tpm2(struct tpm_chip *chip);

And same for all other examples.

The 'chip_num' thing is bonkers.

Jason

------------------------------------------------------------------------------

Re: [PATCH] tpm: move struct tpm_class_ops to drivers/char/tpm/tpm.h

[Jarkko Sakkinen]

On Fri, Sep 02, 2016 at 04:45:31PM -0600, Jason Gunthorpe wrote:
> On Sat, Sep 03, 2016 at 01:35:22AM +0300, Jarkko Sakkinen wrote:
> > On Fri, Sep 02, 2016 at 04:11:22PM -0600, Jason Gunthorpe wrote:
> > > On Sat, Sep 03, 2016 at 12:48:03AM +0300, Jarkko Sakkinen wrote:
> > > > The struct tpm_class_ops is not used outside the TPM driver. Thus,
> > > > it can be safely move to drivers/char/tpm/tpm.h.
> > > 
> > > No, this is the wrong direction.
> > > 
> > > The goal is to make things more like other subsystems, so we should be
> > > moving struct tpm_chip into the public header, and that requires ops
> > > to be in the public header.
> > > 
> > > This is why I put ops here in the first place.
> > 
> > I'm OK with it as long as you explain why this is necessary. I see no
> > use for them outside the TPM subsystem.
> 
> That is because the users out side the subsystem are Doing it Wrong.
> 
> eg this:
> 
>  extern int tpm_is_tpm2(u32 chip_num);
> 
> Should be:
> 
>  extern int tpm_is_tpm2(struct tpm_chip *chip);
> 
> And same for all other examples.
> 
> The 'chip_num' thing is bonkers.

OK, how would one get the chip instance?

/Jarkko

------------------------------------------------------------------------------

Re: [PATCH] tpm: move struct tpm_class_ops to drivers/char/tpm/tpm.h

[Jarkko Sakkinen]

On Sat, Sep 03, 2016 at 09:22:21AM +0300, Jarkko Sakkinen wrote:
> On Fri, Sep 02, 2016 at 04:45:31PM -0600, Jason Gunthorpe wrote:
> > On Sat, Sep 03, 2016 at 01:35:22AM +0300, Jarkko Sakkinen wrote:
> > > On Fri, Sep 02, 2016 at 04:11:22PM -0600, Jason Gunthorpe wrote:
> > > > On Sat, Sep 03, 2016 at 12:48:03AM +0300, Jarkko Sakkinen wrote:
> > > > > The struct tpm_class_ops is not used outside the TPM driver. Thus,
> > > > > it can be safely move to drivers/char/tpm/tpm.h.
> > > > 
> > > > No, this is the wrong direction.
> > > > 
> > > > The goal is to make things more like other subsystems, so we should be
> > > > moving struct tpm_chip into the public header, and that requires ops
> > > > to be in the public header.
> > > > 
> > > > This is why I put ops here in the first place.
> > > 
> > > I'm OK with it as long as you explain why this is necessary. I see no
> > > use for them outside the TPM subsystem.
> > 
> > That is because the users out side the subsystem are Doing it Wrong.
> > 
> > eg this:
> > 
> >  extern int tpm_is_tpm2(u32 chip_num);
> > 
> > Should be:
> > 
> >  extern int tpm_is_tpm2(struct tpm_chip *chip);
> > 
> > And same for all other examples.
> > 
> > The 'chip_num' thing is bonkers.
> 
> OK, how would one get the chip instance?

This still doesn't explain why moving the structures inside the driver
would be wrong. Even if outside callers would use a pointer the
structure could be opaque.

/Jarkko

Re: [PATCH] tpm: move struct tpm_class_ops to drivers/char/tpm/tpm.h

[Jason Gunthorpe]

On Sat, Sep 03, 2016 at 09:26:05AM +0300, Jarkko Sakkinen wrote:
> > 
> > OK, how would one get the chip instance?

Most subsystems have a get function that returns a kref'd pointer. For
TPM all we really need today is a 'get_default_tpm_for_ns' kind of
function.

> This still doesn't explain why moving the structures inside the driver
> would be wrong. Even if outside callers would use a pointer the
> structure could be opaque.

For instance, if we did a get function then the 'put' function would
be an inline around dev_put and that needs to see inside the chip.

This is a well trodden pattern in the kernel, there is no reason to do
something different for tpm.

Jason

------------------------------------------------------------------------------

Re: [PATCH] tpm: move struct tpm_class_ops to drivers/char/tpm/tpm.h

[Jarkko Sakkinen]

On Sun, Sep 04, 2016 at 02:14:06PM -0600, Jason Gunthorpe wrote:
> On Sat, Sep 03, 2016 at 09:26:05AM +0300, Jarkko Sakkinen wrote:
> > > 
> > > OK, how would one get the chip instance?
> 
> Most subsystems have a get function that returns a kref'd pointer. For
> TPM all we really need today is a 'get_default_tpm_for_ns' kind of
> function.

Sorry, but I have no idea what you are talking about.

This does not imply that these structure definitions need to be in
include/linux/tpm.h since you are talking something that does not exist.

> > This still doesn't explain why moving the structures inside the driver
> > would be wrong. Even if outside callers would use a pointer the
> > structure could be opaque.
> 
> For instance, if we did a get function then the 'put' function would
> be an inline around dev_put and that needs to see inside the chip.

I do not see any get/put functionality in include/linux/tpm.h.

> This is a well trodden pattern in the kernel, there is no reason to do
> something different for tpm.

/Jarkko

Re: [PATCH] tpm: move struct tpm_class_ops to drivers/char/tpm/tpm.h

[Jason Gunthorpe]

On Mon, Sep 05, 2016 at 08:44:34AM +0300, Jarkko Sakkinen wrote:
> On Sun, Sep 04, 2016 at 02:14:06PM -0600, Jason Gunthorpe wrote:
> > On Sat, Sep 03, 2016 at 09:26:05AM +0300, Jarkko Sakkinen wrote:
> > > > 
> > > > OK, how would one get the chip instance?
> > 
> > Most subsystems have a get function that returns a kref'd pointer. For
> > TPM all we really need today is a 'get_default_tpm_for_ns' kind of
> > function.
> 
> Sorry, but I have no idea what you are talking about.

Go look at how rtc or net work.

> This does not imply that these structure definitions need to be in
> include/linux/tpm.h since you are talking something that does not exist.

Well, I've been slowly pushing tpm to be more like a standard subystem
for a long time - not all the parts are there yet. A standard
subsystem will have a get/put scheme for tpm_chip.

So, yes, it does not exist, but you should be planning for it to exist
someday..

Jason