Re: [PATCH RFC] tpm_crb: Fix AMD Zen on-chip fTPM detection

Re: [PATCH RFC] tpm_crb: Fix AMD Zen on-chip fTPM detection

[Jason Gunthorpe]

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

Re: [PATCH RFC] tpm_crb: Fix AMD Zen on-chip fTPM detection

[Jason Gunthorpe]

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

Re: [PATCH RFC] tpm_crb: Fix AMD Zen on-chip fTPM detection

[Jason Gunthorpe]

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

Re: [PATCH RFC] tpm_crb: Fix AMD Zen on-chip fTPM detection

[Jason Gunthorpe]

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

Re: [PATCH RFC] tpm_crb: Fix AMD Zen on-chip fTPM detection

[Jarkko Sakkinen]

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