| Message ID | 1472852886-7640-1-git-send-email-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 ------------------------------------------------------------------------------
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 ------------------------------------------------------------------------------
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 ------------------------------------------------------------------------------
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 ------------------------------------------------------------------------------
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 ------------------------------------------------------------------------------
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 ------------------------------------------------------------------------------
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 ------------------------------------------------------------------------------
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 ------------------------------------------------------------------------------
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);
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(-)