Re: [PATCH] tpm: tpm_crb: relinquish locality on error path.
On Wed, May 09, 2018 at 09:49:49PM +, Winkler, Tomas wrote: > > > > On Sat, May 05, 2018 at 09:07:21PM +, 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
Re: [PATCH] tpm: tpm_crb: relinquish locality on error path.
On Wed, May 09, 2018 at 09:49:49PM +, Winkler, Tomas wrote: > > > > On Sat, May 05, 2018 at 09:07:21PM +, 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
RE: [PATCH] tpm: tpm_crb: relinquish locality on error path.
> > On Sat, May 05, 2018 at 09:07:21PM +, 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
RE: [PATCH] tpm: tpm_crb: relinquish locality on error path.
> > On Sat, May 05, 2018 at 09:07:21PM +, 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
Re: [PATCH] tpm: tpm_crb: relinquish locality on error path.
On Sat, May 05, 2018 at 09:07:21PM +, 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
Re: [PATCH] tpm: tpm_crb: relinquish locality on error path.
On Sat, May 05, 2018 at 09:07:21PM +, 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
RE: [PATCH] tpm: tpm_crb: relinquish locality on error path.
> > On Thu, May 03, 2018 at 06:42:26AM +, 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 +, Winkler, Tomas wrote: > > > > > > > > > > On Tue, 2018-04-10 at 09:00 +, 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.wink...@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, _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 > > > > > > > > > > > > > > > > > > > > &
RE: [PATCH] tpm: tpm_crb: relinquish locality on error path.
> > On Thu, May 03, 2018 at 06:42:26AM +, 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 +, Winkler, Tomas wrote: > > > > > > > > > > On Tue, 2018-04-10 at 09:00 +, 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 > > > > > > > > > > > > > > > > > > > > > > > > > > --- > > > > > > > > > > > > > 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, _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 > > > > > > > > > > > > > > > > > > > > > > > > @@ > > > > >
Re: [PATCH] tpm: tpm_crb: relinquish locality on error path.
On Thu, May 03, 2018 at 06:42:26AM +, Winkler, Tomas wrote: > > > > > > > > > > > > On Tue, Apr 24, 2018 at 08:04:01PM +, Winkler, Tomas wrote: > > > > > > Subject: Re: [PATCH] tpm: tpm_crb: relinquish locality on error > > > > > > path. > > > > > > > > > > > > On Fri, Apr 20, 2018 at 01:19:12PM +, Winkler, Tomas wrote: > > > > > > > > > On Tue, 2018-04-10 at 09:00 +, 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.wink...@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, _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); > > > > > > > > > > > &g
Re: [PATCH] tpm: tpm_crb: relinquish locality on error path.
On Thu, May 03, 2018 at 06:42:26AM +, Winkler, Tomas wrote: > > > > > > > > > > > > On Tue, Apr 24, 2018 at 08:04:01PM +, Winkler, Tomas wrote: > > > > > > Subject: Re: [PATCH] tpm: tpm_crb: relinquish locality on error > > > > > > path. > > > > > > > > > > > > On Fri, Apr 20, 2018 at 01:19:12PM +, Winkler, Tomas wrote: > > > > > > > > > On Tue, 2018-04-10 at 09:00 +, 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 > > > > > > > > > > > > > > > > > > > > > > > > --- > > > > > > > > > > > > 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, _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) > > > &
RE: [PATCH] tpm: tpm_crb: relinquish locality on error path.
> > > > > > > > On Tue, Apr 24, 2018 at 08:04:01PM +, Winkler, Tomas wrote: > > > > > Subject: Re: [PATCH] tpm: tpm_crb: relinquish locality on error path. > > > > > > > > > > On Fri, Apr 20, 2018 at 01:19:12PM +, Winkler, Tomas wrote: > > > > > > > > On Tue, 2018-04-10 at 09:00 +, 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.wink...@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, _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(>regs_t- > >ctrl_cmd_pa_high); > > > > > > > > > > > pa_low = > > > > > > > > > &g
RE: [PATCH] tpm: tpm_crb: relinquish locality on error path.
> > > > > > > > On Tue, Apr 24, 2018 at 08:04:01PM +, Winkler, Tomas wrote: > > > > > Subject: Re: [PATCH] tpm: tpm_crb: relinquish locality on error path. > > > > > > > > > > On Fri, Apr 20, 2018 at 01:19:12PM +, Winkler, Tomas wrote: > > > > > > > > On Tue, 2018-04-10 at 09:00 +, 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 > > > > > > > > > > > > > > > > > > > > > > --- > > > > > > > > > > > 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, _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(>regs_t- > >ctrl_cmd_pa_high); > > > > > > > > > > > pa_low = > > > > > > > > > > > ioread32(>regs_t->c
Re: [PATCH] tpm: tpm_crb: relinquish locality on error path.
On Wed, May 02, 2018 at 01:35:47PM +, Winkler, Tomas wrote: > > > > > > On Tue, Apr 24, 2018 at 08:04:01PM +, Winkler, Tomas wrote: > > > > Subject: Re: [PATCH] tpm: tpm_crb: relinquish locality on error path. > > > > > > > > On Fri, Apr 20, 2018 at 01:19:12PM +, Winkler, Tomas wrote: > > > > > > > On Tue, 2018-04-10 at 09:00 +, 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.wink...@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, _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(>regs_t->ctrl_cmd_pa_high); > > > > > > > > > > pa_low = ioread32(>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); >
Re: [PATCH] tpm: tpm_crb: relinquish locality on error path.
On Wed, May 02, 2018 at 01:35:47PM +, Winkler, Tomas wrote: > > > > > > On Tue, Apr 24, 2018 at 08:04:01PM +, Winkler, Tomas wrote: > > > > Subject: Re: [PATCH] tpm: tpm_crb: relinquish locality on error path. > > > > > > > > On Fri, Apr 20, 2018 at 01:19:12PM +, Winkler, Tomas wrote: > > > > > > > On Tue, 2018-04-10 at 09:00 +, 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 > > > > > > > > > > --- > > > > > > > > > > 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, _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(>regs_t->ctrl_cmd_pa_high); > > > > > > > > > > pa_low = ioread32(>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); > > > >
RE: [PATCH] tpm: tpm_crb: relinquish locality on error path.
> > On Tue, Apr 24, 2018 at 08:04:01PM +, Winkler, Tomas wrote: > > > Subject: Re: [PATCH] tpm: tpm_crb: relinquish locality on error path. > > > > > > On Fri, Apr 20, 2018 at 01:19:12PM +, Winkler, Tomas wrote: > > > > > > On Tue, 2018-04-10 at 09:00 +, 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.wink...@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, _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(>regs_t->ctrl_cmd_pa_high); > > > > > > > > > pa_low = ioread32(>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
RE: [PATCH] tpm: tpm_crb: relinquish locality on error path.
> > On Tue, Apr 24, 2018 at 08:04:01PM +, Winkler, Tomas wrote: > > > Subject: Re: [PATCH] tpm: tpm_crb: relinquish locality on error path. > > > > > > On Fri, Apr 20, 2018 at 01:19:12PM +, Winkler, Tomas wrote: > > > > > > On Tue, 2018-04-10 at 09:00 +, 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 > > > > > > > > > --- > > > > > > > > > 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, _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(>regs_t->ctrl_cmd_pa_high); > > > > > > > > > pa_low = ioread32(>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
Re: [PATCH] tpm: tpm_crb: relinquish locality on error path.
On Tue, Apr 24, 2018 at 08:04:01PM +, Winkler, Tomas wrote: > > Subject: Re: [PATCH] tpm: tpm_crb: relinquish locality on error path. > > > > On Fri, Apr 20, 2018 at 01:19:12PM +, Winkler, Tomas wrote: > > > > > On Tue, 2018-04-10 at 09:00 +, 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.wink...@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, _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(>regs_t->ctrl_cmd_pa_high); > > > > > > > > pa_low = ioread32(>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
Re: [PATCH] tpm: tpm_crb: relinquish locality on error path.
On Tue, Apr 24, 2018 at 08:04:01PM +, Winkler, Tomas wrote: > > Subject: Re: [PATCH] tpm: tpm_crb: relinquish locality on error path. > > > > On Fri, Apr 20, 2018 at 01:19:12PM +, Winkler, Tomas wrote: > > > > > On Tue, 2018-04-10 at 09:00 +, 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 > > > > > > > > --- > > > > > > > > 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, _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(>regs_t->ctrl_cmd_pa_high); > > > > > > > > pa_low = ioread32(>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
RE: [PATCH] tpm: tpm_crb: relinquish locality on error path.
> On Tue, Apr 24, 2018 at 07:03:28PM +0300, Jarkko Sakkinen wrote: > > On Fri, Apr 20, 2018 at 01:19:12PM +, Winkler, Tomas wrote: > > > > > On Tue, 2018-04-10 at 09:00 +, 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> > > > > > > > --- > > > > > > > > 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, _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(>regs_t->ctrl_cmd_pa_high); > > > > > > > > pa_low = ioread32(>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 > Thanks Tomas
RE: [PATCH] tpm: tpm_crb: relinquish locality on error path.
> On Tue, Apr 24, 2018 at 07:03:28PM +0300, Jarkko Sakkinen wrote: > > On Fri, Apr 20, 2018 at 01:19:12PM +, Winkler, Tomas wrote: > > > > > On Tue, 2018-04-10 at 09:00 +, 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 > > > > > > > > --- > > > > > > > > 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, _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(>regs_t->ctrl_cmd_pa_high); > > > > > > > > pa_low = ioread32(>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 > Thanks Tomas
RE: [PATCH] tpm: tpm_crb: relinquish locality on error path.
> Subject: Re: [PATCH] tpm: tpm_crb: relinquish locality on error path. > > On Fri, Apr 20, 2018 at 01:19:12PM +, Winkler, Tomas wrote: > > > > On Tue, 2018-04-10 at 09:00 +, 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.wink...@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, _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(>regs_t->ctrl_cmd_pa_high); > > > > > > > pa_low = ioread32(>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
RE: [PATCH] tpm: tpm_crb: relinquish locality on error path.
> Subject: Re: [PATCH] tpm: tpm_crb: relinquish locality on error path. > > On Fri, Apr 20, 2018 at 01:19:12PM +, Winkler, Tomas wrote: > > > > On Tue, 2018-04-10 at 09:00 +, 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 > > > > > > > --- > > > > > > > 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, _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(>regs_t->ctrl_cmd_pa_high); > > > > > > > pa_low = ioread32(>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
Re: [PATCH] tpm: tpm_crb: relinquish locality on error path.
On Tue, Apr 24, 2018 at 07:03:28PM +0300, Jarkko Sakkinen wrote: > On Fri, Apr 20, 2018 at 01:19:12PM +, Winkler, Tomas wrote: > > > > On Tue, 2018-04-10 at 09:00 +, 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> > > > > > > --- > > > > > > > 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, _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(>regs_t->ctrl_cmd_pa_high); > > > > > > > pa_low = ioread32(>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
Re: [PATCH] tpm: tpm_crb: relinquish locality on error path.
On Tue, Apr 24, 2018 at 07:03:28PM +0300, Jarkko Sakkinen wrote: > On Fri, Apr 20, 2018 at 01:19:12PM +, Winkler, Tomas wrote: > > > > On Tue, 2018-04-10 at 09:00 +, 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 > > > > > > > --- > > > > > > > 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, _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(>regs_t->ctrl_cmd_pa_high); > > > > > > > pa_low = ioread32(>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
Re: [PATCH] tpm: tpm_crb: relinquish locality on error path.
On Fri, Apr 20, 2018 at 01:19:12PM +, Winkler, Tomas wrote: > > > On Tue, 2018-04-10 at 09:00 +, 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> > > > > > --- > > > > > > 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, _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(>regs_t->ctrl_cmd_pa_high); > > > > > > pa_low = ioread32(>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
Re: [PATCH] tpm: tpm_crb: relinquish locality on error path.
On Fri, Apr 20, 2018 at 01:19:12PM +, Winkler, Tomas wrote: > > > On Tue, 2018-04-10 at 09:00 +, 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 > > > > > > --- > > > > > > 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, _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(>regs_t->ctrl_cmd_pa_high); > > > > > > pa_low = ioread32(>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
RE: [PATCH] tpm: tpm_crb: relinquish locality on error path.
> > On Tue, 2018-04-10 at 09:00 +, 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> > > > > --- > > > > > 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, _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(>regs_t->ctrl_cmd_pa_high); > > > > > pa_low = ioread32(>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
RE: [PATCH] tpm: tpm_crb: relinquish locality on error path.
> > On Tue, 2018-04-10 at 09:00 +, 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 > > > > > --- > > > > > 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, _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(>regs_t->ctrl_cmd_pa_high); > > > > > pa_low = ioread32(>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
RE: [PATCH] tpm: tpm_crb: relinquish locality on error path.
> > On Tue, 2018-04-10 at 09:00 +, 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> > > > --- > > > > 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, _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(>regs_t->ctrl_cmd_pa_high); > > > > pa_low = ioread32(>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#centra > > 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 complexitity ? I see it as a standard way of undoing on exit. Tomas
RE: [PATCH] tpm: tpm_crb: relinquish locality on error path.
> > On Tue, 2018-04-10 at 09:00 +, 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 > > > > --- > > > > 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, _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(>regs_t->ctrl_cmd_pa_high); > > > > pa_low = ioread32(>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#centra > > 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 complexitity ? I see it as a standard way of undoing on exit. Tomas
Re: [PATCH] tpm: tpm_crb: relinquish locality on error path.
On Tue, 2018-04-10 at 09:00 +, 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> > > --- > > > 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, _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(>regs_t->ctrl_cmd_pa_high); > > > pa_low = ioread32(>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#centralized-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. /Jarkko
Re: [PATCH] tpm: tpm_crb: relinquish locality on error path.
On Tue, 2018-04-10 at 09:00 +, 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 > > > --- > > > 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, _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(>regs_t->ctrl_cmd_pa_high); > > > pa_low = ioread32(>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#centralized-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. /Jarkko
RE: [PATCH] tpm: tpm_crb: relinquish locality on error path.
> > 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> > --- > > 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, _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(>regs_t->ctrl_cmd_pa_high); > > pa_low = ioread32(>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#centralized-exiting-of-functions Thanks Tomas > > /Jarkko
RE: [PATCH] tpm: tpm_crb: relinquish locality on error path.
> > 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 > > --- > > 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, _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(>regs_t->ctrl_cmd_pa_high); > > pa_low = ioread32(>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#centralized-exiting-of-functions Thanks Tomas > > /Jarkko
Re: [PATCH] tpm: tpm_crb: relinquish locality on error path.
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> --- > 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, _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(>regs_t->ctrl_cmd_pa_high); > pa_low = ioread32(>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. /Jarkko
Re: [PATCH] tpm: tpm_crb: relinquish locality on error path.
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 > --- > 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, _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(>regs_t->ctrl_cmd_pa_high); > pa_low = ioread32(>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. /Jarkko
[PATCH] tpm: tpm_crb: relinquish locality on error path.
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--- 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, _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(>regs_t->ctrl_cmd_pa_high); pa_low = ioread32(>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; -- 2.14.3
[PATCH] tpm: tpm_crb: relinquish locality on error path.
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 --- 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, _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(>regs_t->ctrl_cmd_pa_high); pa_low = ioread32(>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; -- 2.14.3