LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: "Winkler, Tomas" <tomas.winkler@intel.com>
To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Cc: "'Jason Gunthorpe'" <jgg@ziepe.ca>,
	"Usyskin, Alexander" <alexander.usyskin@intel.com>,
	"'linux-integrity@vger.kernel.org'"
	<linux-integrity@vger.kernel.org>,
	"'linux-security-module@vger.kernel.org'"
	<linux-security-module@vger.kernel.org>,
	"'linux-kernel@vger.kernel.org'" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH] tpm: tpm_crb: relinquish locality on error path.
Date: Thu, 3 May 2018 06:42:26 +0000	[thread overview]
Message-ID: <5B8DA87D05A7694D9FA63FD143655C1B9427B9A6@hasmsx108.ger.corp.intel.com> (raw)
In-Reply-To: <20180503063508.GA8702@linux.intel.com>


> >
> > >
> > > On Tue, Apr 24, 2018 at 08:04:01PM +0000, Winkler, Tomas wrote:
> > > > > Subject: Re: [PATCH] tpm: tpm_crb: relinquish locality on error path.
> > > > >
> > > > > On Fri, Apr 20, 2018 at 01:19:12PM +0000, Winkler, Tomas wrote:
> > > > > > > > On Tue, 2018-04-10 at 09:00 +0000, Winkler, Tomas wrote:
> > > > > > > > > >
> > > > > > > > > > On Sat, 2018-04-07 at 19:12 +0300, Tomas Winkler wrote:
> > > > > > > > > > > In crb_map_io() function, __crb_request_locality()
> > > > > > > > > > > is called prior to crb_cmd_ready(), but if one of
> > > > > > > > > > > the consecutive function fails the flow bails out
> > > > > > > > > > > instead of trying to relinquish
> > > > > locality.
> > > > > > > > > > > This patch adds goto jump to
> > > > > > > > > > > __crb_relinquish_locality() on the error path.
> > > > > > > > > > >
> > > > > > > > > > > Fixes: 888d867df441 (tpm: cmd_ready command can be
> > > > > > > > > > > issued only after granting
> > > > > > > > > > > locality)
> > > > > > > > > > > Signed-off-by: Tomas Winkler
> > > > > > > > > > > <tomas.winkler@intel.com>
> > > > > > > > > > > ---
> > > > > > > > > > >  drivers/char/tpm/tpm_crb.c | 10 +++++++---
> > > > > > > > > > >  1 file changed, 7 insertions(+), 3 deletions(-)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/drivers/char/tpm/tpm_crb.c
> > > > > > > > > > > b/drivers/char/tpm/tpm_crb.c index
> > > > > > > > > > > 7f78482cd157..34fbc6cb097b
> > > > > > > > > > > 100644
> > > > > > > > > > > --- a/drivers/char/tpm/tpm_crb.c
> > > > > > > > > > > +++ b/drivers/char/tpm/tpm_crb.c
> > > > > > > > > > > @@ -511,8 +511,10 @@ static int crb_map_io(struct
> > > > > > > > > > > acpi_device *device, struct crb_priv *priv,
> > > > > > > > > > >
> > > > > > > > > > >  	priv->regs_t = crb_map_res(dev, priv, &io_res,
> > > > > > > > > > > buf-
> > > > > > > > > > > >control_address,
> > > > > > > > > > >  				   sizeof(struct crb_regs_tail));
> > > > > > > > > > > -	if (IS_ERR(priv->regs_t))
> > > > > > > > > > > -		return PTR_ERR(priv->regs_t);
> > > > > > > > > > > +	if (IS_ERR(priv->regs_t)) {
> > > > > > > > > > > +		ret = PTR_ERR(priv->regs_t);
> > > > > > > > > > > +		goto out_relinquish_locality;
> > > > > > > > > > > +	}
> > > > > > > > > > >
> > > > > > > > > > >  	/*
> > > > > > > > > > >  	 * PTT HW bug w/a: wake up the device to access
> @@
> > > > > > > > > > > -520,7
> > > > > > > > > > > +522,7
> > > > > > > > > >
> > > > > > > > > > @@
> > > > > > > > > > > static int crb_map_io(struct acpi_device *device,
> > > > > > > > > > > struct crb_priv *priv,
> > > > > > > > > > >  	 */
> > > > > > > > > > >  	ret = crb_cmd_ready(dev, priv);
> > > > > > > > > > >  	if (ret)
> > > > > > > > > > > -		return ret;
> > > > > > > > > > > +		goto out_relinquish_locality;
> > > > > > > > > > >
> > > > > > > > > > >  	pa_high = ioread32(&priv->regs_t-
> >ctrl_cmd_pa_high);
> > > > > > > > > > >  	pa_low  =
> > > > > > > > > > > ioread32(&priv->regs_t->ctrl_cmd_pa_low);
> > > > > > > > > > > @@ -565,6 +567,8 @@ static int crb_map_io(struct
> > > > > > > > > > > acpi_device *device, struct crb_priv *priv,
> > > > > > > > > > >
> > > > > > > > > > >  	crb_go_idle(dev, priv);
> > > > > > > > > > >
> > > > > > > > > > > +out_relinquish_locality:
> > > > > > > > > > > +
> > > > > > > > > > >  	__crb_relinquish_locality(dev, priv, 0);
> > > > > > > > > > >
> > > > > > > > > > >  	return ret;
> > > > > > > > > >
> > > > > > > > > > Thanks, please just call it before returning in the error path.
> > > > > > > > >
> > > > > > > > > Can you please elaborate why, isn't the centralized
> > > > > > > > > exiting of functions preferred kernel coding style?
> > > > > > > > > https://www.kernel.org/doc/html/v4.11/process/coding-sty
> > > > > > > > > le.h
> > > > > > > > > tml#
> > > > > > > > > cent
> > > > > > > > > ra
> > > > > > > > > lized-ex
> > > > > > > > > iting-of-functions
> > > > > > > >
> > > > > > > > You exit only from one location (not multiple) and not
> > > > > > > > from a nested context. Here you just add more complexity by
> doing this.
> > > > > > >
> > > > > > > Where is the complexity ? I see it as a standard way of
> > > > > > > undoing on
> > > exit.
> > > > > > > Tomas
> > > > > >
> > > > > > Jarkko, can you please respond.
> > > > > > Thanks
> > > > > > Tomas
> > > > >
> > > > > I was away for Mon-Wed last week and did not work on TPM for Thu-
> Fri.
> > > > >
> > > > > My earlier comment was incorrect as there are two locations to
> > > > > exit (not sure how I managed to overlook the patch that way).
> > > > >
> > > > > Thus, I have only two very  minor requets:
> > > > >
> > > > > * Remove the extra newline (the last line addition in the patch).
> > > > Okay
> > > > > * Use just label named out as we have only one exception handler.
> > > > Cannot do that, as the bail out is prior to cmd_ready request so
> > > > there is no
> > > need for go_idle which is under out label.
> > > > >
> > > > > I'll move on to testing, and if it it passes, I can do those updates
> myself.
> > > > Thanks,  I prefer to resend myself.
> > > >
> > > > Tomas
> > >
> > > Add my tested-by as it is cosmectic change, thanks.
> >
> >
> > What change exactly? I had impression you've accepted the patch as is?
> > Thanks
> > Tomas
> 
> Hmm... maybe there is some misunderstandig but I thought you were going
> to make the updates above yourself and send a revised patch.


Probably,  if I remember there were to mails going in asynchronous, I've received your tested-by, the second I've sent this answer,
so I thought you came to the conclusion that there is nothing to change in the patch yourself.
Frankly I've checked the patch and there are no even extra new lines in my version unless it has scrambled on the way.

Thanks
Tomas

  reply	other threads:[~2018-05-03  6:42 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-07 16:12 Tomas Winkler
2018-04-09 11:13 ` Jarkko Sakkinen
2018-04-10  9:00   ` Winkler, Tomas
2018-04-10 12:33     ` Jarkko Sakkinen
2018-04-10 20:25       ` Winkler, Tomas
2018-04-20 13:19         ` Winkler, Tomas
2018-04-24 16:03           ` Jarkko Sakkinen
2018-04-24 17:48             ` Jarkko Sakkinen
2018-04-24 20:10               ` Winkler, Tomas
2018-04-24 20:04             ` Winkler, Tomas
2018-05-02 13:32               ` Jarkko Sakkinen
2018-05-02 13:35                 ` Winkler, Tomas
2018-05-03  6:35                   ` Jarkko Sakkinen
2018-05-03  6:42                     ` Winkler, Tomas [this message]
2018-05-04  8:16                       ` Jarkko Sakkinen
2018-05-05 21:07                         ` Winkler, Tomas
2018-05-09 12:26                           ` Jarkko Sakkinen
2018-05-09 21:49                             ` Winkler, Tomas
2018-05-11 10:18                               ` Jarkko Sakkinen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5B8DA87D05A7694D9FA63FD143655C1B9427B9A6@hasmsx108.ger.corp.intel.com \
    --to=tomas.winkler@intel.com \
    --cc=alexander.usyskin@intel.com \
    --cc=jarkko.sakkinen@linux.intel.com \
    --cc=jgg@ziepe.ca \
    --cc=linux-integrity@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --subject='RE: [PATCH] tpm: tpm_crb: relinquish locality on error path.' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).