Re: [tpmdd-devel] [PATCH RFC 3/3] tpm_crb: request and relinquish locality 0

[Jarkko Sakkinen]

On Sun, Oct 09, 2016 at 09:43:59AM +0000, Winkler, Tomas wrote:
> 
> > >
> > > >
> > > > Request and relinquish locality for the driver use in order to be a better
> > citizen
> > > > in a multi locality environment like with TXT as it uses locality 2.
> > >
> > > >
> > > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@xxxxxxxxxxxxxxx>
> > > > ---
> > > >  drivers/char/tpm/tpm_crb.c | 36
> > > > ++++++++++++++++++++++++------------
> > > >  1 file changed, 24 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> > > > index
> > > > ffd3a6c..9e07cf3 100644
> > > > --- a/drivers/char/tpm/tpm_crb.c
> > > > +++ b/drivers/char/tpm/tpm_crb.c
> > > > @@ -34,6 +34,15 @@ enum crb_defaults {
> > > >  	CRB_ACPI_START_INDEX = 1,
> > > >  };
> > > >
> > > > +enum crb_loc_ctrl {
> > > > +	CRB_LOC_CTRL_REQUEST_ACCESS	= BIT(0),
> > > > +	CRB_LOC_CTRL_RELINQUISH		= BIT(1),
> > > > +};
> > > > +
> > > > +enum crb_loc_state {
> > > > +	CRB_LOC_STATE_LOC_ASSIGNED	= BIT(1),
> > > > +};
> > > > +
> > > >  enum crb_ctrl_req {
> > > >  	CRB_CTRL_REQ_CMD_READY	= BIT(0),
> > > >  	CRB_CTRL_REQ_GO_IDLE	= BIT(1),
> > > > @@ -98,12 +107,8 @@ struct crb_priv {
> > > >   * @dev:  crb device
> > > >   * @priv: crb private data
> > > >   *
> > > > - * Write CRB_CTRL_REQ_GO_IDLE to TPM_CRB_CTRL_REQ
> > > > - * The device should respond within TIMEOUT_C by clearing the bit.
> > > > - * Anyhow, we do not wait here as a consequent CMD_READY request
> > > > - * will be handled correctly even if idle was not completed.
> > > > - *
> > > > - * The function does nothing for devices with ACPI-start method.
> > > > + * Put device to the idle state and relinquish locality. The
> > > > + function does
> > > > + * nothing for devices with the ACPI-start method.
> > > >   *
> > > >   * Return: 0 always
> > > >   */
> > > > @@ -112,6 +117,7 @@ static int __maybe_unused crb_go_idle(struct
> > > > device *dev, struct crb_priv *priv)
> > > >  	if (priv->flags & CRB_FL_ACPI_START)
> > > >  		return 0;
> > > >
> > > > +	iowrite32(CRB_LOC_CTRL_RELINQUISH, &priv->regs->loc_ctrl);
> > >
> > >
> > > Please don't mix different functionalities in one function
> > 
> > ??
> > 
> > > Also those functions are called from runtime pm, this has nothing to
> > > do with the power management
> > 
> > It all depends on granularity. If you want to make an argument, could you
> > propose a better granularity? Do you think it'd be better to do it for each
> > transmission?
> 
> Not sure, I don't believe we closed the design here with all the
> parties, you are jumping ahead. 

I also added RFC tag.

I have had this done for a while but it has had a dependency for runtime
PM so I decided to make it available.

> > You are saying that this is all bad without saying really backing up your
> > statements by any means.
> 
> You are right,  I assumed it's pretty obvious, I'm taking this to my
> attention not do it again.
> 
> So my point is if you have a function which is named go_idle  it
> probably does go idle flow and no other things like relinquish
> functionality that's for code readability and style.  Also you lost

When you go to idle you certain tasks before going to sleep. If the
granularity matches, relinquishing locality is one of those tasks.

> degree  of freedom even now you may want perform each of these
> operation for each tpm request, that might not be true in general, we
> would like to witch to runtime auto suspend it won't work anymore, for
> example. Also as you pointed right now we are not clear on the
> granularity of the locality access. 

That's not true. You don't loose any freedoms. You can start with
something simple like once per transmission or once per idle/ready.

If that does not scale, then it must be adjusted. I think this is
really business as usual...

> Thanks
> Tomas

/Jarkko