resource sanity fails on ioremap

resource sanity fails on ioremap

[Jarkko Sakkinen]

Hi

I've been investigating this bug:

https://bugzilla.kernel.org/show_bug.cgi?id=111511

In all the machines that I've tried tpm_crb this does not occur on 4.4
kernel and still the ioremapped buffers do interleave. It looks as if
something that is not tpm_crb added that memory area to iomem_resource
because 4.4 driver uses ioremap_nocache() and does not add anything to
the resource tree.

/Jarkko

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140

Re: resource sanity fails on ioremap

[Jason Gunthorpe]

On Wed, Feb 03, 2016 at 03:37:01PM -0800, Jarkko Sakkinen wrote:
> Hi
> 
> I've been investigating this bug:
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=111511
> 
> In all the machines that I've tried tpm_crb this does not occur on 4.4
> kernel and still the ioremapped buffers do interleave. It looks as if
> something that is not tpm_crb added that memory area to iomem_resource
> because 4.4 driver uses ioremap_nocache() and does not add anything to
> the resource tree.

The error says the BIOS set the ACPI resource for MSFT0101:00 to
0xfed40000-0xfed4087f and then proceeded to say that the region
0xfed40040-0xfed4103f is one of the additional TPM resources.

Is this right?

        priv->cca = crb_access(dev, priv, buf->control_address, 0x1000);

Where did 0x1000 come from?

Why not sizeof(crb_control_area) ?

Jason

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140

Re: resource sanity fails on ioremap

[Jarkko Sakkinen]

On Thu, Feb 04, 2016 at 04:49:47PM -0700, Jason Gunthorpe wrote:
> On Wed, Feb 03, 2016 at 03:37:01PM -0800, Jarkko Sakkinen wrote:
> > Hi
> > 
> > I've been investigating this bug:
> > 
> > https://bugzilla.kernel.org/show_bug.cgi?id=111511
> > 
> > In all the machines that I've tried tpm_crb this does not occur on 4.4
> > kernel and still the ioremapped buffers do interleave. It looks as if
> > something that is not tpm_crb added that memory area to iomem_resource
> > because 4.4 driver uses ioremap_nocache() and does not add anything to
> > the resource tree.
> 
> The error says the BIOS set the ACPI resource for MSFT0101:00 to
> 0xfed40000-0xfed4087f and then proceeded to say that the region
> 0xfed40040-0xfed4103f is one of the additional TPM resources.
> 
> Is this right?
> 
>         priv->cca = crb_access(dev, priv, buf->control_address, 0x1000);
> 
> Where did 0x1000 come from?
> 
> Why not sizeof(crb_control_area) ?

4.4 does not use devm_ioremap_resource(). That is why this is
confusing. If that was the kernel from my master branch, then this
would make sense.

> Jason

/Jarkko

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140

Re: resource sanity fails on ioremap

[Jason Gunthorpe]

On Thu, Feb 04, 2016 at 08:05:00PM -0800, Jarkko Sakkinen wrote:

> 4.4 does not use devm_ioremap_resource(). That is why this is
> confusing. If that was the kernel from my master branch, then this
> would make sense.

It makes sense, the MSFT resource is created by the kernel when it
parses the ACPI tables.

devm_ioremap_resource creates a child resource of the above owned by
the driver.

Eg, a PCI example:

$ cat /proc/iomem
    d0000000-d0003fff : 0000:06:00.0
          d0000000-d0003fff : r8169

0000:06:00.0 is created by parsing the BAR, it always exists,
equivalent to the MSFT resource

r8169 is created when the driver attaches to it, and serves to lock
the memory range from other drivers.

tpm_crb works the same.

ioremap complains if a mapping crosses any resource boundary, not
just driver created resources.

Jason

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140

Re: resource sanity fails on ioremap

[Jarkko Sakkinen]

On Thu, Feb 04, 2016 at 10:08:50PM -0700, Jason Gunthorpe wrote:
> On Thu, Feb 04, 2016 at 08:05:00PM -0800, Jarkko Sakkinen wrote:
> 
> > 4.4 does not use devm_ioremap_resource(). That is why this is
> > confusing. If that was the kernel from my master branch, then this
> > would make sense.
> 
> It makes sense, the MSFT resource is created by the kernel when it
> parses the ACPI tables.
> 
> devm_ioremap_resource creates a child resource of the above owned by
> the driver.
> 
> Eg, a PCI example:
> 
> $ cat /proc/iomem
>     d0000000-d0003fff : 0000:06:00.0
>           d0000000-d0003fff : r8169
> 
> 0000:06:00.0 is created by parsing the BAR, it always exists,
> equivalent to the MSFT resource
> 
> r8169 is created when the driver attaches to it, and serves to lock
> the memory range from other drivers.
> 
> tpm_crb works the same.
> 
> ioremap complains if a mapping crosses any resource boundary, not
> just driver created resources.

Right, thanks for the explanation and I'm able to reproduce it on my own
work laptop now:

[    0.708272] resource sanity check: requesting [mem
0xfed10000-0xfed15fff], which spans more than pnp 00:01 [mem
0xfed10000-0xfed13fff]
[    0.708302]  [<ffffffff8108548a>] ? iomem_map_sanity_check+0xba/0xd0

Luckily we have this fixed (in two commits) in 4.5.

> Jason

/Jarkko

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140

Re: resource sanity fails on ioremap

[Jason Gunthorpe]

On Fri, Feb 05, 2016 at 08:47:04AM -0800, Jarkko Sakkinen wrote:

> Right, thanks for the explanation and I'm able to reproduce it on my own
> work laptop now:
> 
> [    0.708272] resource sanity check: requesting [mem
> 0xfed10000-0xfed15fff], which spans more than pnp 00:01 [mem
> 0xfed10000-0xfed13fff]
> [    0.708302]  [<ffffffff8108548a>] ? iomem_map_sanity_check+0xba/0xd0
> 
> Luckily we have this fixed (in two commits) in 4.5.

We do? I don't recall seeing a commit that changes the 0x1000

Jason

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140

Re: resource sanity fails on ioremap

[Jarkko Sakkinen]

On Fri, Feb 05, 2016 at 10:42:58AM -0700, Jason Gunthorpe wrote:
> On Fri, Feb 05, 2016 at 08:47:04AM -0800, Jarkko Sakkinen wrote:
> 
> > Right, thanks for the explanation and I'm able to reproduce it on my own
> > work laptop now:
> > 
> > [    0.708272] resource sanity check: requesting [mem
> > 0xfed10000-0xfed15fff], which spans more than pnp 00:01 [mem
> > 0xfed10000-0xfed13fff]
> > [    0.708302]  [<ffffffff8108548a>] ? iomem_map_sanity_check+0xba/0xd0
> > 
> > Luckily we have this fixed (in two commits) in 4.5.
> 
> We do? I don't recall seeing a commit that changes the 0x1000

Ignore our fixes do not do anything to this. And it does not even occur
my laptop (that was a different case). Should think before pressing.
Apologies. I'll push a fix as soon to master.

> Jason

/Jarkko

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140

Re: resource sanity fails on ioremap

[Jarkko Sakkinen]

On Fri, Feb 05, 2016 at 11:28:57AM -0800, Jarkko Sakkinen wrote:
> On Fri, Feb 05, 2016 at 10:42:58AM -0700, Jason Gunthorpe wrote:
> > On Fri, Feb 05, 2016 at 08:47:04AM -0800, Jarkko Sakkinen wrote:
> > 
> > > Right, thanks for the explanation and I'm able to reproduce it on my own
> > > work laptop now:
> > > 
> > > [    0.708272] resource sanity check: requesting [mem
> > > 0xfed10000-0xfed15fff], which spans more than pnp 00:01 [mem
> > > 0xfed10000-0xfed13fff]
> > > [    0.708302]  [<ffffffff8108548a>] ? iomem_map_sanity_check+0xba/0xd0
> > > 
> > > Luckily we have this fixed (in two commits) in 4.5.
> > 
> > We do? I don't recall seeing a commit that changes the 0x1000
> 
> Ignore our fixes do not do anything to this. And it does not even occur
> my laptop (that was a different case). Should think before pressing.
> Apologies. I'll push a fix as soon to master.

Actually this does make things right, doesn't it?

https://github.com/jsakkine/linux-tpmdd/commit/1bd047be37d95bf65a219f4931215f71878ac060

struct resource new_res = {
	.start	= start,
	.end	= start + size - 1,
	.flags	= IORESOURCE_MEM,
};

/Jarkko

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140

Re: resource sanity fails on ioremap

[Jarkko Sakkinen]

On Wed, Feb 10, 2016 at 02:22:27PM +0200, Jarkko Sakkinen wrote:
> On Fri, Feb 05, 2016 at 11:28:57AM -0800, Jarkko Sakkinen wrote:
> > On Fri, Feb 05, 2016 at 10:42:58AM -0700, Jason Gunthorpe wrote:
> > > On Fri, Feb 05, 2016 at 08:47:04AM -0800, Jarkko Sakkinen wrote:
> > > 
> > > > Right, thanks for the explanation and I'm able to reproduce it on my own
> > > > work laptop now:
> > > > 
> > > > [    0.708272] resource sanity check: requesting [mem
> > > > 0xfed10000-0xfed15fff], which spans more than pnp 00:01 [mem
> > > > 0xfed10000-0xfed13fff]
> > > > [    0.708302]  [<ffffffff8108548a>] ? iomem_map_sanity_check+0xba/0xd0
> > > > 
> > > > Luckily we have this fixed (in two commits) in 4.5.
> > > 
> > > We do? I don't recall seeing a commit that changes the 0x1000
> > 
> > Ignore our fixes do not do anything to this. And it does not even occur
> > my laptop (that was a different case). Should think before pressing.
> > Apologies. I'll push a fix as soon to master.
> [SNIP did not make sense] 

Right, this was confusing because it requests something that is not
multiple of than page size :)

So would the right thing to do change it request size of the control
area structure (in the call site of crb_map_res)?

/Jarkko

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140

Re: resource sanity fails on ioremap

[Jason Gunthorpe]

On Wed, Feb 10, 2016 at 02:42:16PM +0200, Jarkko Sakkinen wrote:

> So would the right thing to do change it request size of the control
> area structure (in the call site of crb_map_res)?

I'm guessing so, where did the 0x1000 fixed size come from? If it is
just made up then surely use sizeof() instead

Jason

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140

Re: resource sanity fails on ioremap

[Jarkko Sakkinen]

On Wed, Feb 10, 2016 at 09:02:38AM -0700, Jason Gunthorpe wrote:
> On Wed, Feb 10, 2016 at 02:42:16PM +0200, Jarkko Sakkinen wrote:
> 
> > So would the right thing to do change it request size of the control
> > area structure (in the call site of crb_map_res)?
> 
> I'm guessing so, where did the 0x1000 fixed size come from? If it is
> just made up then surely use sizeof() instead

To be honest it came from my false understanding that ioremap() should
map in page granularity. I've only used it before doing tpm_crb in that
granularity.

> Jason

/Jarkko

------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140