* Re: [PATCH RFC] tpm_crb: Fix AMD Zen on-chip fTPM detection
[not found] ` <20170616182951.398757-1-manuel.lauss-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2017-06-16 19:25 ` Jason Gunthorpe
[not found] ` <CAOLZvyEeiRQ8LfCmq1p70xv9worYfTXDPTgEZrhFcqUev1+h+Q@mail.gmail.com>
2017-06-19 0:15 ` Jarkko Sakkinen
1 sibling, 1 reply; 5+ messages in thread
From: Jason Gunthorpe @ 2017-06-16 19:25 UTC (permalink / raw)
To: Manuel Lauss; +Cc: tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f
On Fri, Jun 16, 2017 at 08:29:51PM +0200, Manuel Lauss wrote:
> This RFC patch fixes 2 issues which prevent the fTPM device from being initialized
> by the tpm_crb driver:
>
> 1) use devm_ioremap() instead of devm_ioremap_resource() to fix the following error
> due to it not allowing overlapping resources:
>
> tpm_crb MSFT0101:00: can't request region for resource [mem 0xdd84f000-0xdd84ffff]
> tpm_crb: probe of MSFT0101:00 failed with error -16
No, we can't do this, it breaks other situations that rely on
request_resource.
We already put a work around for a very similar problem on a different
system, do you have commit?
commit b4e2eb0651ac3180a942d378b040c5cc045113ee
Author: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
Date: Tue Feb 21 14:14:24 2017 -0700
tpm crb: Work around BIOS's that report the wrong ACPI region size
> 2) The fTPM uses different buffer addresses for command and response, so the
> priv->cmd_size member never gets initialized. Move this initialization
> to right after succesfully mapping the cmd buffer.
Yes for this though, can you prepare a seperate patch and include a
proper Fixes line refering to aa77ea0e43dc5bb0c1dcc9bad76afaa7faca8cab
?
Jason
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH RFC] tpm_crb: Fix AMD Zen on-chip fTPM detection
[not found] ` <CAOLZvyEeiRQ8LfCmq1p70xv9worYfTXDPTgEZrhFcqUev1+h+Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2017-06-16 19:57 ` Jason Gunthorpe
[not found] ` <CAOLZvyFUS+Foj0ZycrZS=yWp+=x1mi8yL1-3Bq+mhcav9iesYA@mail.gmail.com>
0 siblings, 1 reply; 5+ messages in thread
From: Jason Gunthorpe @ 2017-06-16 19:57 UTC (permalink / raw)
To: Manuel Lauss; +Cc: tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f
On Fri, Jun 16, 2017 at 09:41:38PM +0200, Manuel Lauss wrote:
> On Fri, Jun 16, 2017 at 9:25 PM, Jason Gunthorpe
> <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:
> > On Fri, Jun 16, 2017 at 08:29:51PM +0200, Manuel Lauss wrote:
> >> This RFC patch fixes 2 issues which prevent the fTPM device from being initialized
> >> by the tpm_crb driver:
> >>
> >> 1) use devm_ioremap() instead of devm_ioremap_resource() to fix the following error
> >> due to it not allowing overlapping resources:
> >>
> >> tpm_crb MSFT0101:00: can't request region for resource [mem 0xdd84f000-0xdd84ffff]
> >> tpm_crb: probe of MSFT0101:00 failed with error -16
> >
> > No, we can't do this, it breaks other situations that rely on
> > request_resource.
> >
> > We already put a work around for a very similar problem on a different
> > system, do you have commit?
> >
> > commit b4e2eb0651ac3180a942d378b040c5cc045113ee
> > Author: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> > Date: Tue Feb 21 14:14:24 2017 -0700
> >
> > tpm crb: Work around BIOS's that report the wrong ACPI region size
> >
>
> Yes, that was actually the third problem I encountered on 4.11.5, but this
> patch does not fix point 1) above.
>
> /proc/iomem looks like this before the probe attempt:
> dd759000-dd868fff : ACPI Non-volatile Storage
> dd84b000-dd84bfff : MSFT0101:00
> dd84f000-dd84ffff : MSFT0101:00
>
> I have no idea yet why devm_request_mem_region() fails here. Is it because
> the ACPI NVS parent is already marked busy by the previous mapping
> of b000-bfff?
Hum. I wonder what does
static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
struct acpi_table_tpm2 *buf)
{
ret = acpi_dev_get_resources(device, &resources, crb_check_resource,
&io_res);
return in io_res for this arrangment? I'm guessing it isn't
dd759000-dd868fff ?
The issue might be that crb_check_resource/iomem assumes that there is
a single contiguous io region, while this device seems to have two of
them..
If so then the required patch is to iomap all of the io regsions that
crb_check_resource finds, and search all of them in
crb_map_res. Currently it only does the first one.
Jason
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH RFC] tpm_crb: Fix AMD Zen on-chip fTPM detection
[not found] ` <CAOLZvyFUS+Foj0ZycrZS=yWp+=x1mi8yL1-3Bq+mhcav9iesYA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2017-06-16 20:27 ` Jason Gunthorpe
[not found] ` <CAOLZvyH2rG3RpqPfiMOf68yHC91DdO8UEHXYh4FCb0ZqKJf1dw@mail.gmail.com>
0 siblings, 1 reply; 5+ messages in thread
From: Jason Gunthorpe @ 2017-06-16 20:27 UTC (permalink / raw)
To: Manuel Lauss; +Cc: tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f
On Fri, Jun 16, 2017 at 10:08:41PM +0200, Manuel Lauss wrote:
> On Fri, Jun 16, 2017 at 9:57 PM, Jason Gunthorpe
> <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:
> > On Fri, Jun 16, 2017 at 09:41:38PM +0200, Manuel Lauss wrote:
> >> On Fri, Jun 16, 2017 at 9:25 PM, Jason Gunthorpe
> >> <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:
> >> > On Fri, Jun 16, 2017 at 08:29:51PM +0200, Manuel Lauss wrote:
> >> >> This RFC patch fixes 2 issues which prevent the fTPM device from being initialized
> >> >> by the tpm_crb driver:
> >> >>
> >> >> 1) use devm_ioremap() instead of devm_ioremap_resource() to fix the following error
> >> >> due to it not allowing overlapping resources:
> >> >>
> >> >> tpm_crb MSFT0101:00: can't request region for resource [mem 0xdd84f000-0xdd84ffff]
> >> >> tpm_crb: probe of MSFT0101:00 failed with error -16
> >> >
> >> > No, we can't do this, it breaks other situations that rely on
> >> > request_resource.
> >> >
> >> > We already put a work around for a very similar problem on a different
> >> > system, do you have commit?
> >> >
> >> > commit b4e2eb0651ac3180a942d378b040c5cc045113ee
> >> > Author: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> >> > Date: Tue Feb 21 14:14:24 2017 -0700
> >> >
> >> > tpm crb: Work around BIOS's that report the wrong ACPI region size
> >> >
> >>
> >> Yes, that was actually the third problem I encountered on 4.11.5, but this
> >> patch does not fix point 1) above.
> >>
> >> /proc/iomem looks like this before the probe attempt:
> >> dd759000-dd868fff : ACPI Non-volatile Storage
> >> dd84b000-dd84bfff : MSFT0101:00
> >> dd84f000-dd84ffff : MSFT0101:00
> >>
> >> I have no idea yet why devm_request_mem_region() fails here. Is it because
> >> the ACPI NVS parent is already marked busy by the previous mapping
> >> of b000-bfff?
> >
> > Hum. I wonder what does
> >
> > static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
> > struct acpi_table_tpm2 *buf)
> > {
> > ret = acpi_dev_get_resources(device, &resources, crb_check_resource,
> > &io_res);
> >
> > return in io_res for this arrangment? I'm guessing it isn't
> > dd759000-dd868fff ?
>
> It returns dd84b000-dd84bfff. mapping that fails already.
Erm, okay, that is what guessed.
What do you mean 'mapping that fails already' ? The report you gave
above shows the failure is on the other region:
tpm_crb MSFT0101:00: can't request region for resource [mem 0xdd84f000-0xdd84ffff]
Which supports my guess that the problem here is the multiple regions
and the fix is to map them all, as I described.
Jason
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH RFC] tpm_crb: Fix AMD Zen on-chip fTPM detection
[not found] ` <CAOLZvyH2rG3RpqPfiMOf68yHC91DdO8UEHXYh4FCb0ZqKJf1dw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2017-06-16 20:41 ` Jason Gunthorpe
0 siblings, 0 replies; 5+ messages in thread
From: Jason Gunthorpe @ 2017-06-16 20:41 UTC (permalink / raw)
To: Manuel Lauss; +Cc: tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f
On Fri, Jun 16, 2017 at 10:35:11PM +0200, Manuel Lauss wrote:
> > What do you mean 'mapping that fails already' ? The report you gave
> > above shows the failure is on the other region:
> >
> > tpm_crb MSFT0101:00: can't request region for resource [mem 0xdd84f000-0xdd84ffff]
>
> the address seems to change every boot (it's one of the two MSFT0101 devices)
> but it's always the first call to devm_ioremap_resource() in tpm_crb.c
> line 454 that fails:
>
> priv->iobase = devm_ioremap_resource(dev, &io_res);
I have no idea why that would fail, based on your proc file it should
not, you need to figure out what is going on..
Jason
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH RFC] tpm_crb: Fix AMD Zen on-chip fTPM detection
[not found] ` <20170616182951.398757-1-manuel.lauss-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2017-06-16 19:25 ` [PATCH RFC] tpm_crb: Fix AMD Zen on-chip fTPM detection Jason Gunthorpe
@ 2017-06-19 0:15 ` Jarkko Sakkinen
1 sibling, 0 replies; 5+ messages in thread
From: Jarkko Sakkinen @ 2017-06-19 0:15 UTC (permalink / raw)
To: Manuel Lauss, Peter Huewe, Marcel Selhorst, Jason Gunthorpe,
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f
I'll look into this after it has been properly split.
/Jarkko
On Fri, 2017-06-16 at 20:29 +0200, Manuel Lauss wrote:
> This RFC patch fixes 2 issues which prevent the fTPM device from being initialized
> by the tpm_crb driver:
>
> 1) use devm_ioremap() instead of devm_ioremap_resource() to fix the following error
> due to it not allowing overlapping resources:
>
> tpm_crb MSFT0101:00: can't request region for resource [mem 0xdd84f000-0xdd84ffff]
> tpm_crb: probe of MSFT0101:00 failed with error -16
>
> 2) The fTPM uses different buffer addresses for command and response, so the
> priv->cmd_size member never gets initialized. Move this initialization
> to right after succesfully mapping the cmd buffer.
>
> With these 2 issues fixed, the AMD Zen fTPM is detected correctly.
>
> ---
> RFC, should probably be split into 2 parts.
>
> drivers/char/tpm/tpm_crb.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index b917b9d5f710..796274811f02 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -400,7 +400,7 @@ static void __iomem *crb_map_res(struct device *dev, struct crb_priv *priv,
> return (void __iomem *) ERR_PTR(-EINVAL);
>
> if (!resource_contains(io_res, &new_res))
> - return devm_ioremap_resource(dev, &new_res);
> + return devm_ioremap(dev, new_res.start, resource_size(&new_res));
>
> return priv->iobase + (new_res.start - io_res->start);
> }
> @@ -451,7 +451,7 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
> return -EINVAL;
> }
>
> - priv->iobase = devm_ioremap_resource(dev, &io_res);
> + priv->iobase = devm_ioremap(dev, io_res.start, resource_size(&io_res));
> if (IS_ERR(priv->iobase))
> return PTR_ERR(priv->iobase);
>
> @@ -494,6 +494,7 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
> ret = PTR_ERR(priv->cmd);
> goto out;
> }
> + priv->cmd_size = cmd_size;
>
> memcpy_fromio(&rsp_pa, &priv->regs_t->ctrl_rsp_pa, 8);
> rsp_pa = le64_to_cpu(rsp_pa);
> @@ -515,8 +516,6 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
> goto out;
> }
>
> - priv->cmd_size = cmd_size;
> -
> priv->rsp = priv->cmd;
>
> out:
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-06-19 0:15 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20170616182951.398757-1-manuel.lauss@gmail.com>
[not found] ` <20170616182951.398757-1-manuel.lauss-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2017-06-16 19:25 ` [PATCH RFC] tpm_crb: Fix AMD Zen on-chip fTPM detection Jason Gunthorpe
[not found] ` <CAOLZvyEeiRQ8LfCmq1p70xv9worYfTXDPTgEZrhFcqUev1+h+Q@mail.gmail.com>
[not found] ` <CAOLZvyEeiRQ8LfCmq1p70xv9worYfTXDPTgEZrhFcqUev1+h+Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-06-16 19:57 ` Jason Gunthorpe
[not found] ` <CAOLZvyFUS+Foj0ZycrZS=yWp+=x1mi8yL1-3Bq+mhcav9iesYA@mail.gmail.com>
[not found] ` <CAOLZvyFUS+Foj0ZycrZS=yWp+=x1mi8yL1-3Bq+mhcav9iesYA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-06-16 20:27 ` Jason Gunthorpe
[not found] ` <CAOLZvyH2rG3RpqPfiMOf68yHC91DdO8UEHXYh4FCb0ZqKJf1dw@mail.gmail.com>
[not found] ` <CAOLZvyH2rG3RpqPfiMOf68yHC91DdO8UEHXYh4FCb0ZqKJf1dw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-06-16 20:41 ` Jason Gunthorpe
2017-06-19 0:15 ` Jarkko Sakkinen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).