* [PATCH] tpm: tpm_crb: relinquish locality on error path.
@ 2018-04-24 16:03 ` Jarkko Sakkinen
0 siblings, 0 replies; 38+ messages in thread
From: Jarkko Sakkinen @ 2018-04-24 16:03 UTC (permalink / raw)
To: linux-security-module
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-style.html#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).
* Use just label named out as we have only one exception handler.
I'll move on to testing, and if it it passes, I can do those updates
myself.
/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] tpm: tpm_crb: relinquish locality on error path.
2018-04-24 16:03 ` Jarkko Sakkinen
@ 2018-04-24 17:48 ` Jarkko Sakkinen
-1 siblings, 0 replies; 38+ messages in thread
From: Jarkko Sakkinen @ 2018-04-24 17:48 UTC (permalink / raw)
To: Winkler, Tomas
Cc: 'Jason Gunthorpe',
Usyskin, Alexander, 'linux-integrity@vger.kernel.org',
'linux-security-module@vger.kernel.org',
'linux-kernel@vger.kernel.org'
On Tue, Apr 24, 2018 at 07:03:28PM +0300, Jarkko Sakkinen wrote:
> 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-style.html#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).
> * Use just label named out as we have only one exception handler.
>
> I'll move on to testing, and if it it passes, I can do those updates
> myself.
>
> /Jarkko
Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
/Jarkko
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH] tpm: tpm_crb: relinquish locality on error path.
@ 2018-04-24 17:48 ` Jarkko Sakkinen
0 siblings, 0 replies; 38+ messages in thread
From: Jarkko Sakkinen @ 2018-04-24 17:48 UTC (permalink / raw)
To: linux-security-module
On Tue, Apr 24, 2018 at 07:03:28PM +0300, Jarkko Sakkinen wrote:
> 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-style.html#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).
> * Use just label named out as we have only one exception handler.
>
> I'll move on to testing, and if it it passes, I can do those updates
> myself.
>
> /Jarkko
Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 38+ messages in thread
* RE: [PATCH] tpm: tpm_crb: relinquish locality on error path.
2018-04-24 17:48 ` Jarkko Sakkinen
@ 2018-04-24 20:10 ` Winkler, Tomas
-1 siblings, 0 replies; 38+ messages in thread
From: Winkler, Tomas @ 2018-04-24 20:10 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: 'Jason Gunthorpe',
Usyskin, Alexander, 'linux-integrity@vger.kernel.org',
'linux-security-module@vger.kernel.org',
'linux-kernel@vger.kernel.org'
> On Tue, Apr 24, 2018 at 07:03:28PM +0300, Jarkko Sakkinen wrote:
> > 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-style.htm
> > > > > > l#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).
> > * Use just label named out as we have only one exception handler.
> >
> > I'll move on to testing, and if it it passes, I can do those updates
> > myself.
> >
> > /Jarkko
>
> Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>
Thanks
Tomas
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH] tpm: tpm_crb: relinquish locality on error path.
@ 2018-04-24 20:10 ` Winkler, Tomas
0 siblings, 0 replies; 38+ messages in thread
From: Winkler, Tomas @ 2018-04-24 20:10 UTC (permalink / raw)
To: linux-security-module
> On Tue, Apr 24, 2018 at 07:03:28PM +0300, Jarkko Sakkinen wrote:
> > 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-style.htm
> > > > > > l#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).
> > * Use just label named out as we have only one exception handler.
> >
> > I'll move on to testing, and if it it passes, I can do those updates
> > myself.
> >
> > /Jarkko
>
> Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>
Thanks
Tomas
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 38+ messages in thread
* RE: [PATCH] tpm: tpm_crb: relinquish locality on error path.
2018-04-24 16:03 ` Jarkko Sakkinen
@ 2018-04-24 20:04 ` Winkler, Tomas
-1 siblings, 0 replies; 38+ messages in thread
From: Winkler, Tomas @ 2018-04-24 20:04 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: 'Jason Gunthorpe',
Usyskin, Alexander, 'linux-integrity@vger.kernel.org',
'linux-security-module@vger.kernel.org',
'linux-kernel@vger.kernel.org'
> 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-style.html#
> > > > > 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
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH] tpm: tpm_crb: relinquish locality on error path.
@ 2018-04-24 20:04 ` Winkler, Tomas
0 siblings, 0 replies; 38+ messages in thread
From: Winkler, Tomas @ 2018-04-24 20:04 UTC (permalink / raw)
To: linux-security-module
> 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-style.html#
> > > > > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] tpm: tpm_crb: relinquish locality on error path.
2018-04-24 20:04 ` Winkler, Tomas
@ 2018-05-02 13:32 ` Jarkko Sakkinen
-1 siblings, 0 replies; 38+ messages in thread
From: Jarkko Sakkinen @ 2018-05-02 13:32 UTC (permalink / raw)
To: Winkler, Tomas
Cc: 'Jason Gunthorpe',
Usyskin, Alexander, 'linux-integrity@vger.kernel.org',
'linux-security-module@vger.kernel.org',
'linux-kernel@vger.kernel.org'
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-style.html#
> > > > > > 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.
/Jarkko
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH] tpm: tpm_crb: relinquish locality on error path.
@ 2018-05-02 13:32 ` Jarkko Sakkinen
0 siblings, 0 replies; 38+ messages in thread
From: Jarkko Sakkinen @ 2018-05-02 13:32 UTC (permalink / raw)
To: linux-security-module
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-style.html#
> > > > > > 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.
/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 38+ messages in thread
* RE: [PATCH] tpm: tpm_crb: relinquish locality on error path.
2018-05-02 13:32 ` Jarkko Sakkinen
@ 2018-05-02 13:35 ` Winkler, Tomas
-1 siblings, 0 replies; 38+ messages in thread
From: Winkler, Tomas @ 2018-05-02 13:35 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: 'Jason Gunthorpe',
Usyskin, Alexander, 'linux-integrity@vger.kernel.org',
'linux-security-module@vger.kernel.org',
'linux-kernel@vger.kernel.org'
>
> 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-style.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
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH] tpm: tpm_crb: relinquish locality on error path.
@ 2018-05-02 13:35 ` Winkler, Tomas
0 siblings, 0 replies; 38+ messages in thread
From: Winkler, Tomas @ 2018-05-02 13:35 UTC (permalink / raw)
To: linux-security-module
>
> 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-style.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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] tpm: tpm_crb: relinquish locality on error path.
2018-05-02 13:35 ` Winkler, Tomas
@ 2018-05-03 6:35 ` Jarkko Sakkinen
-1 siblings, 0 replies; 38+ messages in thread
From: Jarkko Sakkinen @ 2018-05-03 6:35 UTC (permalink / raw)
To: Winkler, Tomas
Cc: 'Jason Gunthorpe',
Usyskin, Alexander, 'linux-integrity@vger.kernel.org',
'linux-security-module@vger.kernel.org',
'linux-kernel@vger.kernel.org'
On Wed, May 02, 2018 at 01:35:47PM +0000, Winkler, Tomas wrote:
>
>
> >
> > 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-style.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.
/Jarkko
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH] tpm: tpm_crb: relinquish locality on error path.
@ 2018-05-03 6:35 ` Jarkko Sakkinen
0 siblings, 0 replies; 38+ messages in thread
From: Jarkko Sakkinen @ 2018-05-03 6:35 UTC (permalink / raw)
To: linux-security-module
On Wed, May 02, 2018 at 01:35:47PM +0000, Winkler, Tomas wrote:
>
>
> >
> > 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-style.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.
/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 38+ messages in thread
* RE: [PATCH] tpm: tpm_crb: relinquish locality on error path.
2018-05-03 6:35 ` Jarkko Sakkinen
@ 2018-05-03 6:42 ` Winkler, Tomas
-1 siblings, 0 replies; 38+ messages in thread
From: Winkler, Tomas @ 2018-05-03 6:42 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: 'Jason Gunthorpe',
Usyskin, Alexander, 'linux-integrity@vger.kernel.org',
'linux-security-module@vger.kernel.org',
'linux-kernel@vger.kernel.org'
> >
> > >
> > > 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
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH] tpm: tpm_crb: relinquish locality on error path.
@ 2018-05-03 6:42 ` Winkler, Tomas
0 siblings, 0 replies; 38+ messages in thread
From: Winkler, Tomas @ 2018-05-03 6:42 UTC (permalink / raw)
To: linux-security-module
> >
> > >
> > > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] tpm: tpm_crb: relinquish locality on error path.
2018-05-03 6:42 ` Winkler, Tomas
@ 2018-05-04 8:16 ` Jarkko Sakkinen
-1 siblings, 0 replies; 38+ messages in thread
From: Jarkko Sakkinen @ 2018-05-04 8:16 UTC (permalink / raw)
To: Winkler, Tomas
Cc: 'Jason Gunthorpe',
Usyskin, Alexander, 'linux-integrity@vger.kernel.org',
'linux-security-module@vger.kernel.org',
'linux-kernel@vger.kernel.org'
On Thu, May 03, 2018 at 06:42:26AM +0000, Winkler, Tomas wrote:
>
> > >
> > > >
> > > > 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
The only thing was the label name. That is why I asked if I can
change it by myself instead of you having to send a follow up
patch. Did you send a new one? Did not find it from patchwork.
/Jarkko
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH] tpm: tpm_crb: relinquish locality on error path.
@ 2018-05-04 8:16 ` Jarkko Sakkinen
0 siblings, 0 replies; 38+ messages in thread
From: Jarkko Sakkinen @ 2018-05-04 8:16 UTC (permalink / raw)
To: linux-security-module
On Thu, May 03, 2018 at 06:42:26AM +0000, Winkler, Tomas wrote:
>
> > >
> > > >
> > > > 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
The only thing was the label name. That is why I asked if I can
change it by myself instead of you having to send a follow up
patch. Did you send a new one? Did not find it from patchwork.
/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 38+ messages in thread
* RE: [PATCH] tpm: tpm_crb: relinquish locality on error path.
2018-05-04 8:16 ` Jarkko Sakkinen
@ 2018-05-05 21:07 ` Winkler, Tomas
-1 siblings, 0 replies; 38+ messages in thread
From: Winkler, Tomas @ 2018-05-05 21:07 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: 'Jason Gunthorpe',
Usyskin, Alexander, 'linux-integrity@vger.kernel.org',
'linux-security-module@vger.kernel.org',
'linux-kernel@vger.kernel.org'
>
> On Thu, May 03, 2018 at 06:42:26AM +0000, Winkler, Tomas wrote:
> >
> > > >
> > > > >
> > > > > 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
>
> The only thing was the label name. That is why I asked if I can change it by
> myself instead of you having to send a follow up patch. Did you send a new
> one? Did not find it from patchwork.
There is already one 'out' label in that function, what would you prefer for this new label name?
Thanks
Tomas
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH] tpm: tpm_crb: relinquish locality on error path.
@ 2018-05-05 21:07 ` Winkler, Tomas
0 siblings, 0 replies; 38+ messages in thread
From: Winkler, Tomas @ 2018-05-05 21:07 UTC (permalink / raw)
To: linux-security-module
>
> On Thu, May 03, 2018 at 06:42:26AM +0000, Winkler, Tomas wrote:
> >
> > > >
> > > > >
> > > > > 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
>
> The only thing was the label name. That is why I asked if I can change it by
> myself instead of you having to send a follow up patch. Did you send a new
> one? Did not find it from patchwork.
There is already one 'out' label in that function, what would you prefer for this new label name?
Thanks
Tomas
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] tpm: tpm_crb: relinquish locality on error path.
2018-05-05 21:07 ` Winkler, Tomas
@ 2018-05-09 12:26 ` Jarkko Sakkinen
-1 siblings, 0 replies; 38+ messages in thread
From: Jarkko Sakkinen @ 2018-05-09 12:26 UTC (permalink / raw)
To: Winkler, Tomas
Cc: 'Jason Gunthorpe',
Usyskin, Alexander, 'linux-integrity@vger.kernel.org',
'linux-security-module@vger.kernel.org',
'linux-kernel@vger.kernel.org'
On Sat, May 05, 2018 at 09:07:21PM +0000, Winkler, Tomas wrote:
> There is already one 'out' label in that function, what would you prefer for this new label name?
> Thanks
> Tomas
Aah, of course. My bad that I did not notice it but please just point
me if I miss something obvious like that :-) I'll apply the patch
today..
/Jarkko
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH] tpm: tpm_crb: relinquish locality on error path.
@ 2018-05-09 12:26 ` Jarkko Sakkinen
0 siblings, 0 replies; 38+ messages in thread
From: Jarkko Sakkinen @ 2018-05-09 12:26 UTC (permalink / raw)
To: linux-security-module
On Sat, May 05, 2018 at 09:07:21PM +0000, Winkler, Tomas wrote:
> There is already one 'out' label in that function, what would you prefer for this new label name?
> Thanks
> Tomas
Aah, of course. My bad that I did not notice it but please just point
me if I miss something obvious like that :-) I'll apply the patch
today..
/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 38+ messages in thread
* RE: [PATCH] tpm: tpm_crb: relinquish locality on error path.
2018-05-09 12:26 ` Jarkko Sakkinen
@ 2018-05-09 21:49 ` Winkler, Tomas
-1 siblings, 0 replies; 38+ messages in thread
From: Winkler, Tomas @ 2018-05-09 21:49 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: 'Jason Gunthorpe',
Usyskin, Alexander, 'linux-integrity@vger.kernel.org',
'linux-security-module@vger.kernel.org',
'linux-kernel@vger.kernel.org'
>
> On Sat, May 05, 2018 at 09:07:21PM +0000, Winkler, Tomas wrote:
> > There is already one 'out' label in that function, what would you prefer for
> this new label name?
> > Thanks
> > Tomas
>
> Aah, of course. My bad that I did not notice it but please just point me if I
> miss something obvious like that :-) I'll apply the patch today..
>
Thanks
Tomas
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH] tpm: tpm_crb: relinquish locality on error path.
@ 2018-05-09 21:49 ` Winkler, Tomas
0 siblings, 0 replies; 38+ messages in thread
From: Winkler, Tomas @ 2018-05-09 21:49 UTC (permalink / raw)
To: linux-security-module
>
> On Sat, May 05, 2018 at 09:07:21PM +0000, Winkler, Tomas wrote:
> > There is already one 'out' label in that function, what would you prefer for
> this new label name?
> > Thanks
> > Tomas
>
> Aah, of course. My bad that I did not notice it but please just point me if I
> miss something obvious like that :-) I'll apply the patch today..
>
Thanks
Tomas
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH] tpm: tpm_crb: relinquish locality on error path.
2018-05-09 21:49 ` Winkler, Tomas
@ 2018-05-11 10:18 ` Jarkko Sakkinen
-1 siblings, 0 replies; 38+ messages in thread
From: Jarkko Sakkinen @ 2018-05-11 10:18 UTC (permalink / raw)
To: Winkler, Tomas
Cc: 'Jason Gunthorpe',
Usyskin, Alexander, 'linux-integrity@vger.kernel.org',
'linux-security-module@vger.kernel.org',
'linux-kernel@vger.kernel.org'
On Wed, May 09, 2018 at 09:49:49PM +0000, Winkler, Tomas wrote:
> >
> > On Sat, May 05, 2018 at 09:07:21PM +0000, Winkler, Tomas wrote:
> > > There is already one 'out' label in that function, what would you prefer for
> > this new label name?
> > > Thanks
> > > Tomas
> >
> > Aah, of course. My bad that I did not notice it but please just point me if I
> > miss something obvious like that :-) I'll apply the patch today..
> >
> Thanks
> Tomas
It's also in next now!
/Jarkko
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH] tpm: tpm_crb: relinquish locality on error path.
@ 2018-05-11 10:18 ` Jarkko Sakkinen
0 siblings, 0 replies; 38+ messages in thread
From: Jarkko Sakkinen @ 2018-05-11 10:18 UTC (permalink / raw)
To: linux-security-module
On Wed, May 09, 2018 at 09:49:49PM +0000, Winkler, Tomas wrote:
> >
> > On Sat, May 05, 2018 at 09:07:21PM +0000, Winkler, Tomas wrote:
> > > There is already one 'out' label in that function, what would you prefer for
> > this new label name?
> > > Thanks
> > > Tomas
> >
> > Aah, of course. My bad that I did not notice it but please just point me if I
> > miss something obvious like that :-) I'll apply the patch today..
> >
> Thanks
> Tomas
It's also in next now!
/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 38+ messages in thread