linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] PCI: dwc: Warn only for non-prefetchable memory resource size >4GB
@ 2020-05-13 19:08 Vidya Sagar
  2020-05-13 22:35 ` Bjorn Helgaas
  0 siblings, 1 reply; 21+ messages in thread
From: Vidya Sagar @ 2020-05-13 19:08 UTC (permalink / raw)
  To: jingoohan1, gustavo.pimentel, lorenzo.pieralisi, andrew.murray,
	bhelgaas, thierry.reding, jonathanh
  Cc: linux-pci, linux-kernel, kthota, mmaddireddy, vidyas, sagar.tv

commit 9e73fa02aa009 ("PCI: dwc: Warn if MEM resource size exceeds max for
32-bits") enables warning for MEM resources of size >4GB but prefetchable
 memory resources also come under this category where sizes can go beyond
4GB. Avoid logging a warning for prefetchable memory resources.

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
 drivers/pci/controller/dwc/pcie-designware-host.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 42fbfe2a1b8f..a29396529ea4 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -366,7 +366,8 @@ int dw_pcie_host_init(struct pcie_port *pp)
 			pp->mem = win->res;
 			pp->mem->name = "MEM";
 			mem_size = resource_size(pp->mem);
-			if (upper_32_bits(mem_size))
+			if (upper_32_bits(mem_size) &&
+			    !(win->res->flags & IORESOURCE_PREFETCH))
 				dev_warn(dev, "MEM resource size exceeds max for 32 bits\n");
 			pp->mem_size = mem_size;
 			pp->mem_bus_addr = pp->mem->start - win->offset;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH] PCI: dwc: Warn only for non-prefetchable memory resource size >4GB
  2020-05-13 19:08 [PATCH] PCI: dwc: Warn only for non-prefetchable memory resource size >4GB Vidya Sagar
@ 2020-05-13 22:35 ` Bjorn Helgaas
  2020-05-18 15:54   ` Lorenzo Pieralisi
  0 siblings, 1 reply; 21+ messages in thread
From: Bjorn Helgaas @ 2020-05-13 22:35 UTC (permalink / raw)
  To: Vidya Sagar
  Cc: jingoohan1, gustavo.pimentel, lorenzo.pieralisi, Andrew Murray,
	bhelgaas, thierry.reding, jonathanh, linux-pci, linux-kernel,
	kthota, mmaddireddy, sagar.tv, Alan Mikhak

[+cc Alan; please cc authors of relevant commits,
updated Andrew's email address]

On Thu, May 14, 2020 at 12:38:55AM +0530, Vidya Sagar wrote:
> commit 9e73fa02aa009 ("PCI: dwc: Warn if MEM resource size exceeds max for
> 32-bits") enables warning for MEM resources of size >4GB but prefetchable
>  memory resources also come under this category where sizes can go beyond
> 4GB. Avoid logging a warning for prefetchable memory resources.
> 
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 42fbfe2a1b8f..a29396529ea4 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -366,7 +366,8 @@ int dw_pcie_host_init(struct pcie_port *pp)
>  			pp->mem = win->res;
>  			pp->mem->name = "MEM";
>  			mem_size = resource_size(pp->mem);
> -			if (upper_32_bits(mem_size))
> +			if (upper_32_bits(mem_size) &&
> +			    !(win->res->flags & IORESOURCE_PREFETCH))
>  				dev_warn(dev, "MEM resource size exceeds max for 32 bits\n");
>  			pp->mem_size = mem_size;
>  			pp->mem_bus_addr = pp->mem->start - win->offset;
> -- 
> 2.17.1
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] PCI: dwc: Warn only for non-prefetchable memory resource size >4GB
  2020-05-13 22:35 ` Bjorn Helgaas
@ 2020-05-18 15:54   ` Lorenzo Pieralisi
  2020-05-19 13:55     ` Vidya Sagar
  0 siblings, 1 reply; 21+ messages in thread
From: Lorenzo Pieralisi @ 2020-05-18 15:54 UTC (permalink / raw)
  To: Bjorn Helgaas
  Cc: Vidya Sagar, jingoohan1, gustavo.pimentel, Andrew Murray,
	bhelgaas, thierry.reding, jonathanh, linux-pci, linux-kernel,
	kthota, mmaddireddy, sagar.tv, Alan Mikhak

On Wed, May 13, 2020 at 05:35:08PM -0500, Bjorn Helgaas wrote:
> [+cc Alan; please cc authors of relevant commits,
> updated Andrew's email address]
> 
> On Thu, May 14, 2020 at 12:38:55AM +0530, Vidya Sagar wrote:
> > commit 9e73fa02aa009 ("PCI: dwc: Warn if MEM resource size exceeds max for
> > 32-bits") enables warning for MEM resources of size >4GB but prefetchable
> >  memory resources also come under this category where sizes can go beyond
> > 4GB. Avoid logging a warning for prefetchable memory resources.
> > 
> > Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> > ---
> >  drivers/pci/controller/dwc/pcie-designware-host.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index 42fbfe2a1b8f..a29396529ea4 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -366,7 +366,8 @@ int dw_pcie_host_init(struct pcie_port *pp)
> >  			pp->mem = win->res;
> >  			pp->mem->name = "MEM";
> >  			mem_size = resource_size(pp->mem);
> > -			if (upper_32_bits(mem_size))
> > +			if (upper_32_bits(mem_size) &&
> > +			    !(win->res->flags & IORESOURCE_PREFETCH))
> >  				dev_warn(dev, "MEM resource size exceeds max for 32 bits\n");
> >  			pp->mem_size = mem_size;
> >  			pp->mem_bus_addr = pp->mem->start - win->offset;

That warning was added for a reason - why should not we log legitimate
warnings ? AFAIU having resources larger than 4GB can lead to undefined
behaviour given the current ATU programming API.

Alan ? I want to understand what's the best course of action before
merging these patches.

Lorenzo

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] PCI: dwc: Warn only for non-prefetchable memory resource size >4GB
  2020-05-18 15:54   ` Lorenzo Pieralisi
@ 2020-05-19 13:55     ` Vidya Sagar
  2020-05-19 14:58       ` Lorenzo Pieralisi
  0 siblings, 1 reply; 21+ messages in thread
From: Vidya Sagar @ 2020-05-19 13:55 UTC (permalink / raw)
  To: Lorenzo Pieralisi, Bjorn Helgaas
  Cc: jingoohan1, gustavo.pimentel, Andrew Murray, bhelgaas,
	thierry.reding, jonathanh, linux-pci, linux-kernel, kthota,
	mmaddireddy, sagar.tv, Alan Mikhak



On 18-May-20 9:24 PM, Lorenzo Pieralisi wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Wed, May 13, 2020 at 05:35:08PM -0500, Bjorn Helgaas wrote:
>> [+cc Alan; please cc authors of relevant commits,
>> updated Andrew's email address]
>>
>> On Thu, May 14, 2020 at 12:38:55AM +0530, Vidya Sagar wrote:
>>> commit 9e73fa02aa009 ("PCI: dwc: Warn if MEM resource size exceeds max for
>>> 32-bits") enables warning for MEM resources of size >4GB but prefetchable
>>>   memory resources also come under this category where sizes can go beyond
>>> 4GB. Avoid logging a warning for prefetchable memory resources.
>>>
>>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
>>> ---
>>>   drivers/pci/controller/dwc/pcie-designware-host.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
>>> index 42fbfe2a1b8f..a29396529ea4 100644
>>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>>> @@ -366,7 +366,8 @@ int dw_pcie_host_init(struct pcie_port *pp)
>>>                      pp->mem = win->res;
>>>                      pp->mem->name = "MEM";
>>>                      mem_size = resource_size(pp->mem);
>>> -                   if (upper_32_bits(mem_size))
>>> +                   if (upper_32_bits(mem_size) &&
>>> +                       !(win->res->flags & IORESOURCE_PREFETCH))
>>>                              dev_warn(dev, "MEM resource size exceeds max for 32 bits\n");
>>>                      pp->mem_size = mem_size;
>>>                      pp->mem_bus_addr = pp->mem->start - win->offset;
> 
> That warning was added for a reason - why should not we log legitimate
> warnings ? AFAIU having resources larger than 4GB can lead to undefined
> behaviour given the current ATU programming API.
Yeah. I'm all for a warning if the size is larger than 4GB in case of
non-prefetchable window as one of the ATU outbound translation channels 
is being used, but, we are not employing any ATU outbound translation 
channel for prefetchable window and they can be greater than 4GB in size 
for all right reasons. So, logging a warning for prefetchable region 
doesn't seem correct to me. Please let me know if my understanding is wrong.

- Vidya Sagar
> 
> Alan ? I want to understand what's the best course of action before
> merging these patches.
> 
> Lorenzo
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] PCI: dwc: Warn only for non-prefetchable memory resource size >4GB
  2020-05-19 13:55     ` Vidya Sagar
@ 2020-05-19 14:58       ` Lorenzo Pieralisi
  2020-05-19 17:08         ` Vidya Sagar
  2020-05-19 22:08         ` Gustavo Pimentel
  0 siblings, 2 replies; 21+ messages in thread
From: Lorenzo Pieralisi @ 2020-05-19 14:58 UTC (permalink / raw)
  To: Vidya Sagar
  Cc: Bjorn Helgaas, jingoohan1, gustavo.pimentel, Andrew Murray,
	bhelgaas, thierry.reding, jonathanh, linux-pci, linux-kernel,
	kthota, mmaddireddy, sagar.tv, Alan Mikhak

On Tue, May 19, 2020 at 07:25:02PM +0530, Vidya Sagar wrote:
> 
> 
> On 18-May-20 9:24 PM, Lorenzo Pieralisi wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Wed, May 13, 2020 at 05:35:08PM -0500, Bjorn Helgaas wrote:
> > > [+cc Alan; please cc authors of relevant commits,
> > > updated Andrew's email address]
> > > 
> > > On Thu, May 14, 2020 at 12:38:55AM +0530, Vidya Sagar wrote:
> > > > commit 9e73fa02aa009 ("PCI: dwc: Warn if MEM resource size exceeds max for
> > > > 32-bits") enables warning for MEM resources of size >4GB but prefetchable
> > > >   memory resources also come under this category where sizes can go beyond
> > > > 4GB. Avoid logging a warning for prefetchable memory resources.
> > > > 
> > > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> > > > ---
> > > >   drivers/pci/controller/dwc/pcie-designware-host.c | 3 ++-
> > > >   1 file changed, 2 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > index 42fbfe2a1b8f..a29396529ea4 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > @@ -366,7 +366,8 @@ int dw_pcie_host_init(struct pcie_port *pp)
> > > >                      pp->mem = win->res;
> > > >                      pp->mem->name = "MEM";
> > > >                      mem_size = resource_size(pp->mem);
> > > > -                   if (upper_32_bits(mem_size))
> > > > +                   if (upper_32_bits(mem_size) &&
> > > > +                       !(win->res->flags & IORESOURCE_PREFETCH))
> > > >                              dev_warn(dev, "MEM resource size exceeds max for 32 bits\n");
> > > >                      pp->mem_size = mem_size;
> > > >                      pp->mem_bus_addr = pp->mem->start - win->offset;
> > 
> > That warning was added for a reason - why should not we log legitimate
> > warnings ? AFAIU having resources larger than 4GB can lead to undefined
> > behaviour given the current ATU programming API.
> Yeah. I'm all for a warning if the size is larger than 4GB in case of
> non-prefetchable window as one of the ATU outbound translation
> channels is being used,

Is it true for all DWC host controllers ? Or there may be another
exception whereby we would be forced to disable this warning altogether
?

> but, we are not employing any ATU outbound translation channel for

What does this mean ? "we are not employing any ATU outbound...", is
this the tegra driver ? And what guarantees that this warning is not
legitimate on DWC host controllers that do use the ATU outbound
translation for prefetchable windows ?

Can DWC maintainers chime in and clarify please ?

> prefetchable window and they can be greater than 4GB in size for all
> right reasons. So, logging a warning for prefetchable region doesn't
> seem correct to me. Please let me know if my understanding is wrong.

I think your patch is wrong and it is applied on top of a patch that
is wrong too, so I won't apply yours and it is likely I will revert
Alan's because it seems to solve nothing (and warn spuriously).

It is time for people who maintain DWC please to speak up because I
don't have the HW details required to make a judgment.

Lorenzo

> - Vidya Sagar
> > 
> > Alan ? I want to understand what's the best course of action before
> > merging these patches.
> > 
> > Lorenzo
> > 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] PCI: dwc: Warn only for non-prefetchable memory resource size >4GB
  2020-05-19 14:58       ` Lorenzo Pieralisi
@ 2020-05-19 17:08         ` Vidya Sagar
  2020-05-19 18:20           ` Lorenzo Pieralisi
  2020-05-19 22:08         ` Gustavo Pimentel
  1 sibling, 1 reply; 21+ messages in thread
From: Vidya Sagar @ 2020-05-19 17:08 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: Bjorn Helgaas, jingoohan1, gustavo.pimentel, Andrew Murray,
	bhelgaas, thierry.reding, jonathanh, linux-pci, linux-kernel,
	kthota, mmaddireddy, sagar.tv, Alan Mikhak



On 19-May-20 8:28 PM, Lorenzo Pieralisi wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Tue, May 19, 2020 at 07:25:02PM +0530, Vidya Sagar wrote:
>>
>>
>> On 18-May-20 9:24 PM, Lorenzo Pieralisi wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> On Wed, May 13, 2020 at 05:35:08PM -0500, Bjorn Helgaas wrote:
>>>> [+cc Alan; please cc authors of relevant commits,
>>>> updated Andrew's email address]
>>>>
>>>> On Thu, May 14, 2020 at 12:38:55AM +0530, Vidya Sagar wrote:
>>>>> commit 9e73fa02aa009 ("PCI: dwc: Warn if MEM resource size exceeds max for
>>>>> 32-bits") enables warning for MEM resources of size >4GB but prefetchable
>>>>>    memory resources also come under this category where sizes can go beyond
>>>>> 4GB. Avoid logging a warning for prefetchable memory resources.
>>>>>
>>>>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
>>>>> ---
>>>>>    drivers/pci/controller/dwc/pcie-designware-host.c | 3 ++-
>>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
>>>>> index 42fbfe2a1b8f..a29396529ea4 100644
>>>>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>>>>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>>>>> @@ -366,7 +366,8 @@ int dw_pcie_host_init(struct pcie_port *pp)
>>>>>                       pp->mem = win->res;
>>>>>                       pp->mem->name = "MEM";
>>>>>                       mem_size = resource_size(pp->mem);
>>>>> -                   if (upper_32_bits(mem_size))
>>>>> +                   if (upper_32_bits(mem_size) &&
>>>>> +                       !(win->res->flags & IORESOURCE_PREFETCH))
>>>>>                               dev_warn(dev, "MEM resource size exceeds max for 32 bits\n");
>>>>>                       pp->mem_size = mem_size;
>>>>>                       pp->mem_bus_addr = pp->mem->start - win->offset;
>>>
>>> That warning was added for a reason - why should not we log legitimate
>>> warnings ? AFAIU having resources larger than 4GB can lead to undefined
>>> behaviour given the current ATU programming API.
>> Yeah. I'm all for a warning if the size is larger than 4GB in case of
>> non-prefetchable window as one of the ATU outbound translation
>> channels is being used,
> 
> Is it true for all DWC host controllers ? Or there may be another
> exception whereby we would be forced to disable this warning altogether
> ?I think so. As I see from the code, ATU's
Region-0 is used for config space translation
Region-1 is used for non-prefetchable memory translation
Region-2 is used for I/O translation
So, there is no region reserved for translating prefetchable memory regions.

> 
>> but, we are not employing any ATU outbound translation channel for
> 
> What does this mean ? "we are not employing any ATU outbound...", is
> this the tegra driver ? And what guarantees that this warning is not
> legitimate on DWC host controllers that do use the ATU outbound
> translation for prefetchable windows ?
Not Tegra driver but Tegra HW. Tegra HW doesn't need any ATU outbound 
translation for prefetchable (for that matter any 1-to-1 mapping to 
generate memory transactions on the PCIe bus).
The Warning is still valid for both Tegra and other DWC based 
controllers for non-prefetchable memory translation.

> 
> Can DWC maintainers chime in and clarify please ?
> 
>> prefetchable window and they can be greater than 4GB in size for all
>> right reasons. So, logging a warning for prefetchable region doesn't
>> seem correct to me. Please let me know if my understanding is wrong.
> 
> I think your patch is wrong and it is applied on top of a patch that
> is wrong too, so I won't apply yours and it is likely I will revert
> Alan's because it seems to solve nothing (and warn spuriously).
> 
> It is time for people who maintain DWC please to speak up because I
> don't have the HW details required to make a judgment.
> 
> Lorenzo
> 
>> - Vidya Sagar
>>>
>>> Alan ? I want to understand what's the best course of action before
>>> merging these patches.
>>>
>>> Lorenzo
>>>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] PCI: dwc: Warn only for non-prefetchable memory resource size >4GB
  2020-05-19 17:08         ` Vidya Sagar
@ 2020-05-19 18:20           ` Lorenzo Pieralisi
  0 siblings, 0 replies; 21+ messages in thread
From: Lorenzo Pieralisi @ 2020-05-19 18:20 UTC (permalink / raw)
  To: Vidya Sagar
  Cc: Bjorn Helgaas, jingoohan1, gustavo.pimentel, Andrew Murray,
	bhelgaas, thierry.reding, jonathanh, linux-pci, linux-kernel,
	kthota, mmaddireddy, sagar.tv, Alan Mikhak

On Tue, May 19, 2020 at 10:38:39PM +0530, Vidya Sagar wrote:

[...]

> > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > index 42fbfe2a1b8f..a29396529ea4 100644
> > > > > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > @@ -366,7 +366,8 @@ int dw_pcie_host_init(struct pcie_port *pp)
> > > > > >                       pp->mem = win->res;
> > > > > >                       pp->mem->name = "MEM";
> > > > > >                       mem_size = resource_size(pp->mem);
> > > > > > -                   if (upper_32_bits(mem_size))
> > > > > > +                   if (upper_32_bits(mem_size) &&
> > > > > > +                       !(win->res->flags & IORESOURCE_PREFETCH))
> > > > > >                               dev_warn(dev, "MEM resource size exceeds max for 32 bits\n");
> > > > > >                       pp->mem_size = mem_size;
> > > > > >                       pp->mem_bus_addr = pp->mem->start - win->offset;
> > > > 
> > > > That warning was added for a reason - why should not we log legitimate
> > > > warnings ? AFAIU having resources larger than 4GB can lead to undefined
> > > > behaviour given the current ATU programming API.
> > > Yeah. I'm all for a warning if the size is larger than 4GB in case of
> > > non-prefetchable window as one of the ATU outbound translation
> > > channels is being used,
> > 
> > Is it true for all DWC host controllers ? Or there may be another
> > exception whereby we would be forced to disable this warning altogether
> > ?I think so. As I see from the code, ATU's
> Region-0 is used for config space translation
> Region-1 is used for non-prefetchable memory translation
> Region-2 is used for I/O translation
> So, there is no region reserved for translating prefetchable memory regions.

I am confused. Code in dw_pcie_setup_rc() programs a memory region
into ATU. What defines that memory as non-prefetchable ?

Code in dw_pcie_host_init() retrieves the bridge windows and
there may be multiple memory regions.

How is it determined which one ends up in pp->mem ? DT parsing order ?

I reiterate the point - I need DWC maintainers to chime in to define
how this must work.

I am not conviced that your patch is safe to apply and I need to
understand how HW works.

> > > but, we are not employing any ATU outbound translation channel for
> > 
> > What does this mean ? "we are not employing any ATU outbound...", is
> > this the tegra driver ? And what guarantees that this warning is not
> > legitimate on DWC host controllers that do use the ATU outbound
> > translation for prefetchable windows ?
> Not Tegra driver but Tegra HW. Tegra HW doesn't need any ATU outbound
> translation for prefetchable (for that matter any 1-to-1 mapping to
> generate memory transactions on the PCIe bus).

See above.

Lorenzo

> The Warning is still valid for both Tegra and other DWC based controllers
> for non-prefetchable memory translation.
> 
> > 
> > Can DWC maintainers chime in and clarify please ?
> > 
> > > prefetchable window and they can be greater than 4GB in size for all
> > > right reasons. So, logging a warning for prefetchable region doesn't
> > > seem correct to me. Please let me know if my understanding is wrong.
> > 
> > I think your patch is wrong and it is applied on top of a patch that
> > is wrong too, so I won't apply yours and it is likely I will revert
> > Alan's because it seems to solve nothing (and warn spuriously).
> > 
> > It is time for people who maintain DWC please to speak up because I
> > don't have the HW details required to make a judgment.
> > 
> > Lorenzo
> > 
> > > - Vidya Sagar
> > > > 
> > > > Alan ? I want to understand what's the best course of action before
> > > > merging these patches.
> > > > 
> > > > Lorenzo
> > > > 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* RE: [PATCH] PCI: dwc: Warn only for non-prefetchable memory resource size >4GB
  2020-05-19 14:58       ` Lorenzo Pieralisi
  2020-05-19 17:08         ` Vidya Sagar
@ 2020-05-19 22:08         ` Gustavo Pimentel
  2020-05-20  2:33           ` Alan Mikhak
                             ` (2 more replies)
  1 sibling, 3 replies; 21+ messages in thread
From: Gustavo Pimentel @ 2020-05-19 22:08 UTC (permalink / raw)
  To: Lorenzo Pieralisi, Vidya Sagar
  Cc: Bjorn Helgaas, jingoohan1, Andrew Murray, bhelgaas,
	thierry.reding, jonathanh, linux-pci, linux-kernel, kthota,
	mmaddireddy, sagar.tv, Alan Mikhak

On Tue, May 19, 2020 at 15:58:16, Lorenzo Pieralisi 
<lorenzo.pieralisi@arm.com> wrote:

> On Tue, May 19, 2020 at 07:25:02PM +0530, Vidya Sagar wrote:
> > 
> > 
> > On 18-May-20 9:24 PM, Lorenzo Pieralisi wrote:
> > > External email: Use caution opening links or attachments
> > > 
> > > 
> > > On Wed, May 13, 2020 at 05:35:08PM -0500, Bjorn Helgaas wrote:
> > > > [+cc Alan; please cc authors of relevant commits,
> > > > updated Andrew's email address]
> > > > 
> > > > On Thu, May 14, 2020 at 12:38:55AM +0530, Vidya Sagar wrote:
> > > > > commit 9e73fa02aa009 ("PCI: dwc: Warn if MEM resource size exceeds max for
> > > > > 32-bits") enables warning for MEM resources of size >4GB but prefetchable
> > > > >   memory resources also come under this category where sizes can go beyond
> > > > > 4GB. Avoid logging a warning for prefetchable memory resources.
> > > > > 
> > > > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> > > > > ---
> > > > >   drivers/pci/controller/dwc/pcie-designware-host.c | 3 ++-
> > > > >   1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > index 42fbfe2a1b8f..a29396529ea4 100644
> > > > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > @@ -366,7 +366,8 @@ int dw_pcie_host_init(struct pcie_port *pp)
> > > > >                      pp->mem = win->res;
> > > > >                      pp->mem->name = "MEM";
> > > > >                      mem_size = resource_size(pp->mem);
> > > > > -                   if (upper_32_bits(mem_size))
> > > > > +                   if (upper_32_bits(mem_size) &&
> > > > > +                       !(win->res->flags & IORESOURCE_PREFETCH))
> > > > >                              dev_warn(dev, "MEM resource size exceeds max for 32 bits\n");
> > > > >                      pp->mem_size = mem_size;
> > > > >                      pp->mem_bus_addr = pp->mem->start - win->offset;
> > > 
> > > That warning was added for a reason - why should not we log legitimate
> > > warnings ? AFAIU having resources larger than 4GB can lead to undefined
> > > behaviour given the current ATU programming API.
> > Yeah. I'm all for a warning if the size is larger than 4GB in case of
> > non-prefetchable window as one of the ATU outbound translation
> > channels is being used,
> 
> Is it true for all DWC host controllers ? Or there may be another
> exception whereby we would be forced to disable this warning altogether
> ?
> 
> > but, we are not employing any ATU outbound translation channel for
> 
> What does this mean ? "we are not employing any ATU outbound...", is
> this the tegra driver ? And what guarantees that this warning is not
> legitimate on DWC host controllers that do use the ATU outbound
> translation for prefetchable windows ?
> 
> Can DWC maintainers chime in and clarify please ?

Before this code section, there is the following function call 
pci_parse_request_of_pci_ranges(), which performs a simple validation for 
the IORESOURCE_MEM resource type.
This validation checks if the resource is marked as prefetchable, if so, 
an error message "non-prefetchable memory resource required" is given and 
a return code with the -EINVAL value.

In other words, to reach the code that Vidya is changing, it can be only 
if the resource is a non-prefetchable, any prefetchable resource will be 
blocked by the previous call, if I'm not mistaken.

Having this in mind, Vidya's change will not make the expected result 
aimed by him.

I don't see any problem by having resources larger than 4GB, from what 
I'm seeing in the databook there isn't any restricting related to that as 
long they don't consume the maximum space that is addressable by the 
system (depending on if they are 32-bit or 64-bit system address).

To be honest, I'm not seeing a system that could have this resource 
larger than 4GB, but it might exist some exception that I don't know of, 
that's why I accepted Alan's patch to warn the user that the resource 
exceeds the maximum for the 32 bits so that he can be aware that he 
*might* be consuming the maximum space addressable.

-Gustavo

> 
> > prefetchable window and they can be greater than 4GB in size for all
> > right reasons. So, logging a warning for prefetchable region doesn't
> > seem correct to me. Please let me know if my understanding is wrong.
> 
> I think your patch is wrong and it is applied on top of a patch that
> is wrong too, so I won't apply yours and it is likely I will revert
> Alan's because it seems to solve nothing (and warn spuriously).
> 
> It is time for people who maintain DWC please to speak up because I
> don't have the HW details required to make a judgment.
> 
> Lorenzo
> 
> > - Vidya Sagar
> > > 
> > > Alan ? I want to understand what's the best course of action before
> > > merging these patches.
> > > 
> > > Lorenzo
> > > 



^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: PCI: dwc: Warn only for non-prefetchable memory resource size >4GB
  2020-05-19 22:08         ` Gustavo Pimentel
@ 2020-05-20  2:33           ` Alan Mikhak
  2020-05-22 14:04             ` Lorenzo Pieralisi
  2020-05-20 11:06           ` [PATCH] " Lorenzo Pieralisi
  2020-05-20 11:17           ` Thierry Reding
  2 siblings, 1 reply; 21+ messages in thread
From: Alan Mikhak @ 2020-05-20  2:33 UTC (permalink / raw)
  To: gustavo.pimentel
  Cc: alan.mikhak, amurray, bhelgaas, helgaas, jingoohan1, jonathanh,
	kthota, linux-kernel, linux-pci, lorenzo.pieralisi, mmaddireddy,
	sagar.tv, thierry.reding, vidyas, Alan Mikhak

Hi Lorenzo,

I came across this issue when implementing a Linux NVMe endpoint function
driver under the Linux PCI Endpoint Framework:
https://lwn.net/Articles/804369/

I needed to map up to 128GB of host memory using a single ATU window
from the endpoint side because NVMe PRPs can be scattered all over host
memory. In the process, I came across this 4GB limitation where the
maximum size of memory that can be mapped is limited by what a u32 value
can represent.

I submitted a separate patch to fix an undefined behavior that may also
happen in dw_pcie_prog_outbound_atu_unroll() under some circumstances
when the size of the memory being mapped is greater than what a u32 value
can represent.
https://patchwork.kernel.org/patch/11469701/

The above patch has been accepted. However, the variable pp->mem_size
in dw_pcie_host_init() is still a u32 whereas the value returned by
resource_size() is u64. If the resource size has non-zero upper 32-bits,
those upper 32-bits will be lost when assigning:
 pp->mem_size = resource_size(pp->mem).

Since current callers seem happy with the existing 4GB implementation
and fixing the u32 limit is beyond my available resources and has a long
high-impact tail, a warning seemed to be a good choice to highlight
this issue in case someone else decides to map a MEM region that is
greater than 4GB.

Removing the warning will avoid such discussions. Without this warning,
this limitation will go unnoticed and will only impact whoever has to
deal with it. It cost me time to figure it out when I had an application
that needed a region larger than 4GB. I figured the most I could do about
it is to raise the issue by adding a warning.

Regards,
Alan







^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] PCI: dwc: Warn only for non-prefetchable memory resource size >4GB
  2020-05-19 22:08         ` Gustavo Pimentel
  2020-05-20  2:33           ` Alan Mikhak
@ 2020-05-20 11:06           ` Lorenzo Pieralisi
  2020-05-20 13:16             ` Thierry Reding
  2020-05-20 11:17           ` Thierry Reding
  2 siblings, 1 reply; 21+ messages in thread
From: Lorenzo Pieralisi @ 2020-05-20 11:06 UTC (permalink / raw)
  To: Gustavo Pimentel
  Cc: Vidya Sagar, Bjorn Helgaas, jingoohan1, Andrew Murray, bhelgaas,
	thierry.reding, jonathanh, linux-pci, linux-kernel, kthota,
	mmaddireddy, sagar.tv, Alan Mikhak

On Tue, May 19, 2020 at 10:08:54PM +0000, Gustavo Pimentel wrote:

[...]

> > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > index 42fbfe2a1b8f..a29396529ea4 100644
> > > > > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > @@ -366,7 +366,8 @@ int dw_pcie_host_init(struct pcie_port *pp)
> > > > > >                      pp->mem = win->res;
> > > > > >                      pp->mem->name = "MEM";
> > > > > >                      mem_size = resource_size(pp->mem);
> > > > > > -                   if (upper_32_bits(mem_size))
> > > > > > +                   if (upper_32_bits(mem_size) &&
> > > > > > +                       !(win->res->flags & IORESOURCE_PREFETCH))
> > > > > >                              dev_warn(dev, "MEM resource size exceeds max for 32 bits\n");
> > > > > >                      pp->mem_size = mem_size;
> > > > > >                      pp->mem_bus_addr = pp->mem->start - win->offset;
> > > > 
> > > > That warning was added for a reason - why should not we log legitimate
> > > > warnings ? AFAIU having resources larger than 4GB can lead to undefined
> > > > behaviour given the current ATU programming API.
> > > Yeah. I'm all for a warning if the size is larger than 4GB in case of
> > > non-prefetchable window as one of the ATU outbound translation
> > > channels is being used,
> > 
> > Is it true for all DWC host controllers ? Or there may be another
> > exception whereby we would be forced to disable this warning altogether
> > ?
> > 
> > > but, we are not employing any ATU outbound translation channel for
> > 
> > What does this mean ? "we are not employing any ATU outbound...", is
> > this the tegra driver ? And what guarantees that this warning is not
> > legitimate on DWC host controllers that do use the ATU outbound
> > translation for prefetchable windows ?
> > 
> > Can DWC maintainers chime in and clarify please ?
> 
> Before this code section, there is the following function call 
> pci_parse_request_of_pci_ranges(), which performs a simple validation for 
> the IORESOURCE_MEM resource type.
> This validation checks if the resource is marked as prefetchable, if so, 
> an error message "non-prefetchable memory resource required" is given and 
> a return code with the -EINVAL value.

That code checks if there is *at least* a non-prefetchable resource,
that's all it does.

> In other words, to reach the code that Vidya is changing, it can be only 
> if the resource is a non-prefetchable, any prefetchable resource will be 
> blocked by the previous call, if I'm not mistaken.

I think you are mistaken sorry.

> Having this in mind, Vidya's change will not make the expected result 
> aimed by him.

I think Vidya's patch does what he expects, the question is whether
it is widely applicable to ALL DWC hosts, that's what I want to know.

> I don't see any problem by having resources larger than 4GB, from what 
> I'm seeing in the databook there isn't any restricting related to that as 
> long they don't consume the maximum space that is addressable by the 
> system (depending on if they are 32-bit or 64-bit system address).
> 
> To be honest, I'm not seeing a system that could have this resource 
> larger than 4GB, but it might exist some exception that I don't know of, 
> that's why I accepted Alan's patch to warn the user that the resource 
> exceeds the maximum for the 32 bits so that he can be aware that he 
> *might* be consuming the maximum space addressable.

I think it is most certainly a possibility to have > 4GB prefetchable
address spaces so we ought to fix this for good. I still have to
understand how the DWC host detects the memory region to be programmed
into the ATU given that there is more than one but only 1 ATU memory
region AFAICS.

Lorenzo

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] PCI: dwc: Warn only for non-prefetchable memory resource size >4GB
  2020-05-19 22:08         ` Gustavo Pimentel
  2020-05-20  2:33           ` Alan Mikhak
  2020-05-20 11:06           ` [PATCH] " Lorenzo Pieralisi
@ 2020-05-20 11:17           ` Thierry Reding
  2020-05-20 17:46             ` Vidya Sagar
  2 siblings, 1 reply; 21+ messages in thread
From: Thierry Reding @ 2020-05-20 11:17 UTC (permalink / raw)
  To: Gustavo Pimentel
  Cc: Lorenzo Pieralisi, Vidya Sagar, Bjorn Helgaas, jingoohan1,
	Andrew Murray, bhelgaas, jonathanh, linux-pci, linux-kernel,
	kthota, mmaddireddy, sagar.tv, Alan Mikhak

[-- Attachment #1: Type: text/plain, Size: 6693 bytes --]

On Tue, May 19, 2020 at 10:08:54PM +0000, Gustavo Pimentel wrote:
> On Tue, May 19, 2020 at 15:58:16, Lorenzo Pieralisi 
> <lorenzo.pieralisi@arm.com> wrote:
> 
> > On Tue, May 19, 2020 at 07:25:02PM +0530, Vidya Sagar wrote:
> > > 
> > > 
> > > On 18-May-20 9:24 PM, Lorenzo Pieralisi wrote:
> > > > External email: Use caution opening links or attachments
> > > > 
> > > > 
> > > > On Wed, May 13, 2020 at 05:35:08PM -0500, Bjorn Helgaas wrote:
> > > > > [+cc Alan; please cc authors of relevant commits,
> > > > > updated Andrew's email address]
> > > > > 
> > > > > On Thu, May 14, 2020 at 12:38:55AM +0530, Vidya Sagar wrote:
> > > > > > commit 9e73fa02aa009 ("PCI: dwc: Warn if MEM resource size exceeds max for
> > > > > > 32-bits") enables warning for MEM resources of size >4GB but prefetchable
> > > > > >   memory resources also come under this category where sizes can go beyond
> > > > > > 4GB. Avoid logging a warning for prefetchable memory resources.
> > > > > > 
> > > > > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> > > > > > ---
> > > > > >   drivers/pci/controller/dwc/pcie-designware-host.c | 3 ++-
> > > > > >   1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > index 42fbfe2a1b8f..a29396529ea4 100644
> > > > > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > @@ -366,7 +366,8 @@ int dw_pcie_host_init(struct pcie_port *pp)
> > > > > >                      pp->mem = win->res;
> > > > > >                      pp->mem->name = "MEM";
> > > > > >                      mem_size = resource_size(pp->mem);
> > > > > > -                   if (upper_32_bits(mem_size))
> > > > > > +                   if (upper_32_bits(mem_size) &&
> > > > > > +                       !(win->res->flags & IORESOURCE_PREFETCH))
> > > > > >                              dev_warn(dev, "MEM resource size exceeds max for 32 bits\n");
> > > > > >                      pp->mem_size = mem_size;
> > > > > >                      pp->mem_bus_addr = pp->mem->start - win->offset;
> > > > 
> > > > That warning was added for a reason - why should not we log legitimate
> > > > warnings ? AFAIU having resources larger than 4GB can lead to undefined
> > > > behaviour given the current ATU programming API.
> > > Yeah. I'm all for a warning if the size is larger than 4GB in case of
> > > non-prefetchable window as one of the ATU outbound translation
> > > channels is being used,
> > 
> > Is it true for all DWC host controllers ? Or there may be another
> > exception whereby we would be forced to disable this warning altogether
> > ?
> > 
> > > but, we are not employing any ATU outbound translation channel for
> > 
> > What does this mean ? "we are not employing any ATU outbound...", is
> > this the tegra driver ? And what guarantees that this warning is not
> > legitimate on DWC host controllers that do use the ATU outbound
> > translation for prefetchable windows ?
> > 
> > Can DWC maintainers chime in and clarify please ?
> 
> Before this code section, there is the following function call 
> pci_parse_request_of_pci_ranges(), which performs a simple validation for 
> the IORESOURCE_MEM resource type.
> This validation checks if the resource is marked as prefetchable, if so, 
> an error message "non-prefetchable memory resource required" is given and 
> a return code with the -EINVAL value.

That's not what the code is doing. pci_parse_request_of_pci_range() will
traverse over the whole list of resources that it can find for the given
host controller and checks whether one of the resources defines prefetch
memory (note the res_valid |= ...). The error will only be returned if
no prefetchable memory region was found.

dw_pcie_host_init() will then again traverse the list of resources and
it will typically encounter two resource of type IORESOURCE_MEM, one for
non-prefetchable memory and another for prefetchable memory.

Vidya's patch is to differentiate between these two resources and allow
prefetchable memory regions to exceed sizes of 4 GiB.

That said, I wonder if there isn't a bigger problem at hand here. From
looking at the code it doesn't seem like the DWC driver makes any
distinction between prefetchable and non-prefetchable memory. Or at
least it doesn't allow both to be stored in struct pcie_port.

Am I missing something? Or can anyone explain how we're programming the
apertures for prefetchable vs. non-prefetchable memory? Perhaps this is
what Vidya was referring to when he said: "we are not using an outbound
ATU translation channel for prefetchable memory".

It looks to me like we're also getting partially lucky, or perhaps that
is by design, in that Tegra194 defines PCI regions in the following
order: I/O, prefetchable memory, non-prefetchable memory. That means
that the DWC core code will overwrite prefetchable memory data with that
of non-prefetchable memory and hence the non-prefetchable region ends up
stored in struct pcie_port and is then used to program the ATU outbound
channel.

> In other words, to reach the code that Vidya is changing, it can be only 
> if the resource is a non-prefetchable, any prefetchable resource will be 
> blocked by the previous call, if I'm not mistaken.
> 
> Having this in mind, Vidya's change will not make the expected result 
> aimed by him.

Given the above I think it does. We've also seen this patch eliminate a
warning that we were seeing before, so I think it has the intended
effect.

> I don't see any problem by having resources larger than 4GB, from what 
> I'm seeing in the databook there isn't any restricting related to that as 
> long they don't consume the maximum space that is addressable by the 
> system (depending on if they are 32-bit or 64-bit system address).
> 
> To be honest, I'm not seeing a system that could have this resource 
> larger than 4GB, but it might exist some exception that I don't know of, 
> that's why I accepted Alan's patch to warn the user that the resource 
> exceeds the maximum for the 32 bits so that he can be aware that he 
> *might* be consuming the maximum space addressable.

I think it's pretty common to have this type of prefetchable memory
region when you connect discrete GPUs to PCIe. It's not unusual for
high-end GPUs to have 8 GiB or even more dedicated video memory and
those will typically be mapped to a prefetchable memory region on
the PCI device.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] PCI: dwc: Warn only for non-prefetchable memory resource size >4GB
  2020-05-20 11:06           ` [PATCH] " Lorenzo Pieralisi
@ 2020-05-20 13:16             ` Thierry Reding
  2020-05-20 17:51               ` Vidya Sagar
  0 siblings, 1 reply; 21+ messages in thread
From: Thierry Reding @ 2020-05-20 13:16 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: Gustavo Pimentel, Vidya Sagar, Bjorn Helgaas, jingoohan1,
	Andrew Murray, bhelgaas, jonathanh, linux-pci, linux-kernel,
	kthota, mmaddireddy, sagar.tv, Alan Mikhak

[-- Attachment #1: Type: text/plain, Size: 5016 bytes --]

On Wed, May 20, 2020 at 12:06:40PM +0100, Lorenzo Pieralisi wrote:
> On Tue, May 19, 2020 at 10:08:54PM +0000, Gustavo Pimentel wrote:
> 
> [...]
> 
> > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > > index 42fbfe2a1b8f..a29396529ea4 100644
> > > > > > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > > @@ -366,7 +366,8 @@ int dw_pcie_host_init(struct pcie_port *pp)
> > > > > > >                      pp->mem = win->res;
> > > > > > >                      pp->mem->name = "MEM";
> > > > > > >                      mem_size = resource_size(pp->mem);
> > > > > > > -                   if (upper_32_bits(mem_size))
> > > > > > > +                   if (upper_32_bits(mem_size) &&
> > > > > > > +                       !(win->res->flags & IORESOURCE_PREFETCH))
> > > > > > >                              dev_warn(dev, "MEM resource size exceeds max for 32 bits\n");
> > > > > > >                      pp->mem_size = mem_size;
> > > > > > >                      pp->mem_bus_addr = pp->mem->start - win->offset;
> > > > > 
> > > > > That warning was added for a reason - why should not we log legitimate
> > > > > warnings ? AFAIU having resources larger than 4GB can lead to undefined
> > > > > behaviour given the current ATU programming API.
> > > > Yeah. I'm all for a warning if the size is larger than 4GB in case of
> > > > non-prefetchable window as one of the ATU outbound translation
> > > > channels is being used,
> > > 
> > > Is it true for all DWC host controllers ? Or there may be another
> > > exception whereby we would be forced to disable this warning altogether
> > > ?
> > > 
> > > > but, we are not employing any ATU outbound translation channel for
> > > 
> > > What does this mean ? "we are not employing any ATU outbound...", is
> > > this the tegra driver ? And what guarantees that this warning is not
> > > legitimate on DWC host controllers that do use the ATU outbound
> > > translation for prefetchable windows ?
> > > 
> > > Can DWC maintainers chime in and clarify please ?
> > 
> > Before this code section, there is the following function call 
> > pci_parse_request_of_pci_ranges(), which performs a simple validation for 
> > the IORESOURCE_MEM resource type.
> > This validation checks if the resource is marked as prefetchable, if so, 
> > an error message "non-prefetchable memory resource required" is given and 
> > a return code with the -EINVAL value.
> 
> That code checks if there is *at least* a non-prefetchable resource,
> that's all it does.
> 
> > In other words, to reach the code that Vidya is changing, it can be only 
> > if the resource is a non-prefetchable, any prefetchable resource will be 
> > blocked by the previous call, if I'm not mistaken.
> 
> I think you are mistaken sorry.
> 
> > Having this in mind, Vidya's change will not make the expected result 
> > aimed by him.
> 
> I think Vidya's patch does what he expects, the question is whether
> it is widely applicable to ALL DWC hosts, that's what I want to know.
> 
> > I don't see any problem by having resources larger than 4GB, from what 
> > I'm seeing in the databook there isn't any restricting related to that as 
> > long they don't consume the maximum space that is addressable by the 
> > system (depending on if they are 32-bit or 64-bit system address).
> > 
> > To be honest, I'm not seeing a system that could have this resource 
> > larger than 4GB, but it might exist some exception that I don't know of, 
> > that's why I accepted Alan's patch to warn the user that the resource 
> > exceeds the maximum for the 32 bits so that he can be aware that he 
> > *might* be consuming the maximum space addressable.
> 
> I think it is most certainly a possibility to have > 4GB prefetchable
> address spaces so we ought to fix this for good. I still have to
> understand how the DWC host detects the memory region to be programmed
> into the ATU given that there is more than one but only 1 ATU memory
> region AFAICS.

Probably best to wait for Vidya to confirm since I'm not altogether
familiar with PCI on Tegra194, but looking at the DTS files and the
Tegra194 TRM, the prefetchable memory regions are set to a range in
0x1200000000-0x1fffffffff which is a region of the address map that
is reserved for "PCIe aperture for > 32-bit OS". Part of that is in
use for non-prefetchable memory (and ends up being programmed into
the ATU) whereas a much larger part is used for prefetchable memory
and is not programmed anywhere, as far as I can tell.

But I think given that this is a designated region of the address
map this is probably automatically redirected to the PCIe controller.
What I don't know is if that's something Tegra-specific or whether all
instantiations have something similar set up.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Re: [PATCH] PCI: dwc: Warn only for non-prefetchable memory resource size >4GB
  2020-05-20 11:17           ` Thierry Reding
@ 2020-05-20 17:46             ` Vidya Sagar
  2020-05-20 22:48               ` Rob Herring
  0 siblings, 1 reply; 21+ messages in thread
From: Vidya Sagar @ 2020-05-20 17:46 UTC (permalink / raw)
  To: Thierry Reding, Gustavo Pimentel
  Cc: Lorenzo Pieralisi, Bjorn Helgaas, jingoohan1, Andrew Murray,
	bhelgaas, jonathanh, linux-pci, linux-kernel, kthota,
	mmaddireddy, sagar.tv, Alan Mikhak



On 20-May-20 4:47 PM, Thierry Reding wrote:
> On Tue, May 19, 2020 at 10:08:54PM +0000, Gustavo Pimentel wrote:
>> On Tue, May 19, 2020 at 15:58:16, Lorenzo Pieralisi
>> <lorenzo.pieralisi@arm.com> wrote:
>>
>>> On Tue, May 19, 2020 at 07:25:02PM +0530, Vidya Sagar wrote:
>>>>
>>>>
>>>> On 18-May-20 9:24 PM, Lorenzo Pieralisi wrote:
>>>>> External email: Use caution opening links or attachments
>>>>>
>>>>>
>>>>> On Wed, May 13, 2020 at 05:35:08PM -0500, Bjorn Helgaas wrote:
>>>>>> [+cc Alan; please cc authors of relevant commits,
>>>>>> updated Andrew's email address]
>>>>>>
>>>>>> On Thu, May 14, 2020 at 12:38:55AM +0530, Vidya Sagar wrote:
>>>>>>> commit 9e73fa02aa009 ("PCI: dwc: Warn if MEM resource size exceeds max for
>>>>>>> 32-bits") enables warning for MEM resources of size >4GB but prefetchable
>>>>>>>    memory resources also come under this category where sizes can go beyond
>>>>>>> 4GB. Avoid logging a warning for prefetchable memory resources.
>>>>>>>
>>>>>>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
>>>>>>> ---
>>>>>>>    drivers/pci/controller/dwc/pcie-designware-host.c | 3 ++-
>>>>>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
>>>>>>> index 42fbfe2a1b8f..a29396529ea4 100644
>>>>>>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>>>>>>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>>>>>>> @@ -366,7 +366,8 @@ int dw_pcie_host_init(struct pcie_port *pp)
>>>>>>>                       pp->mem = win->res;
>>>>>>>                       pp->mem->name = "MEM";
>>>>>>>                       mem_size = resource_size(pp->mem);
>>>>>>> -                   if (upper_32_bits(mem_size))
>>>>>>> +                   if (upper_32_bits(mem_size) &&
>>>>>>> +                       !(win->res->flags & IORESOURCE_PREFETCH))
>>>>>>>                               dev_warn(dev, "MEM resource size exceeds max for 32 bits\n");
>>>>>>>                       pp->mem_size = mem_size;
>>>>>>>                       pp->mem_bus_addr = pp->mem->start - win->offset;
>>>>>
>>>>> That warning was added for a reason - why should not we log legitimate
>>>>> warnings ? AFAIU having resources larger than 4GB can lead to undefined
>>>>> behaviour given the current ATU programming API.
>>>> Yeah. I'm all for a warning if the size is larger than 4GB in case of
>>>> non-prefetchable window as one of the ATU outbound translation
>>>> channels is being used,
>>>
>>> Is it true for all DWC host controllers ? Or there may be another
>>> exception whereby we would be forced to disable this warning altogether
>>> ?
>>>
>>>> but, we are not employing any ATU outbound translation channel for
>>>
>>> What does this mean ? "we are not employing any ATU outbound...", is
>>> this the tegra driver ? And what guarantees that this warning is not
>>> legitimate on DWC host controllers that do use the ATU outbound
>>> translation for prefetchable windows ?
>>>
>>> Can DWC maintainers chime in and clarify please ?
>>
>> Before this code section, there is the following function call
>> pci_parse_request_of_pci_ranges(), which performs a simple validation for
>> the IORESOURCE_MEM resource type.
>> This validation checks if the resource is marked as prefetchable, if so,
>> an error message "non-prefetchable memory resource required" is given and
>> a return code with the -EINVAL value.
> 
> That's not what the code is doing. pci_parse_request_of_pci_range() will
> traverse over the whole list of resources that it can find for the given
> host controller and checks whether one of the resources defines prefetch
> memory (note the res_valid |= ...). The error will only be returned if
> no prefetchable memory region was found.
> 
> dw_pcie_host_init() will then again traverse the list of resources and
> it will typically encounter two resource of type IORESOURCE_MEM, one for
> non-prefetchable memory and another for prefetchable memory.
> 
> Vidya's patch is to differentiate between these two resources and allow
> prefetchable memory regions to exceed sizes of 4 GiB.
> 
> That said, I wonder if there isn't a bigger problem at hand here. From
> looking at the code it doesn't seem like the DWC driver makes any
> distinction between prefetchable and non-prefetchable memory. Or at
> least it doesn't allow both to be stored in struct pcie_port.
> 
> Am I missing something? Or can anyone explain how we're programming the
> apertures for prefetchable vs. non-prefetchable memory? Perhaps this is
> what Vidya was referring to when he said: "we are not using an outbound
> ATU translation channel for prefetchable memory".
> 
> It looks to me like we're also getting partially lucky, or perhaps that
> is by design, in that Tegra194 defines PCI regions in the following
> order: I/O, prefetchable memory, non-prefetchable memory. That means
> that the DWC core code will overwrite prefetchable memory data with that
> of non-prefetchable memory and hence the non-prefetchable region ends up
> stored in struct pcie_port and is then used to program the ATU outbound
> channel.
Well,it is by design. I mean, since the code is not differentiating 
between prefetchable and non-prefetchable regions, I ordered the entries 
in 'ranges' property in such a way that 'prefetchable' comes first 
followed by 'non-prefetchable' entry so that ATU region is used for 
generating the translation required for 'non-prefetchable' region (which 
is a non 1-to-1 mapping)

> 
>> In other words, to reach the code that Vidya is changing, it can be only
>> if the resource is a non-prefetchable, any prefetchable resource will be
>> blocked by the previous call, if I'm not mistaken.
>>
>> Having this in mind, Vidya's change will not make the expected result
>> aimed by him.
> 
> Given the above I think it does. We've also seen this patch eliminate a
> warning that we were seeing before, so I think it has the intended
> effect.
> 
>> I don't see any problem by having resources larger than 4GB, from what
>> I'm seeing in the databook there isn't any restricting related to that as
>> long they don't consume the maximum space that is addressable by the
>> system (depending on if they are 32-bit or 64-bit system address).
>>
>> To be honest, I'm not seeing a system that could have this resource
>> larger than 4GB, but it might exist some exception that I don't know of,
>> that's why I accepted Alan's patch to warn the user that the resource
>> exceeds the maximum for the 32 bits so that he can be aware that he
>> *might* be consuming the maximum space addressable.
> 
> I think it's pretty common to have this type of prefetchable memory
> region when you connect discrete GPUs to PCIe. It's not unusual for
> high-end GPUs to have 8 GiB or even more dedicated video memory and
> those will typically be mapped to a prefetchable memory region on
> the PCI device.
> 
> Thierry
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Re: [PATCH] PCI: dwc: Warn only for non-prefetchable memory resource size >4GB
  2020-05-20 13:16             ` Thierry Reding
@ 2020-05-20 17:51               ` Vidya Sagar
  0 siblings, 0 replies; 21+ messages in thread
From: Vidya Sagar @ 2020-05-20 17:51 UTC (permalink / raw)
  To: Thierry Reding, Lorenzo Pieralisi
  Cc: Gustavo Pimentel, Bjorn Helgaas, jingoohan1, Andrew Murray,
	bhelgaas, jonathanh, linux-pci, linux-kernel, kthota,
	mmaddireddy, sagar.tv, Alan Mikhak



On 20-May-20 6:46 PM, Thierry Reding wrote:
> On Wed, May 20, 2020 at 12:06:40PM +0100, Lorenzo Pieralisi wrote:
>> On Tue, May 19, 2020 at 10:08:54PM +0000, Gustavo Pimentel wrote:
>>
>> [...]
>>
>>>>>>>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
>>>>>>>> index 42fbfe2a1b8f..a29396529ea4 100644
>>>>>>>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>>>>>>>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>>>>>>>> @@ -366,7 +366,8 @@ int dw_pcie_host_init(struct pcie_port *pp)
>>>>>>>>                       pp->mem = win->res;
>>>>>>>>                       pp->mem->name = "MEM";
>>>>>>>>                       mem_size = resource_size(pp->mem);
>>>>>>>> -                   if (upper_32_bits(mem_size))
>>>>>>>> +                   if (upper_32_bits(mem_size) &&
>>>>>>>> +                       !(win->res->flags & IORESOURCE_PREFETCH))
>>>>>>>>                               dev_warn(dev, "MEM resource size exceeds max for 32 bits\n");
>>>>>>>>                       pp->mem_size = mem_size;
>>>>>>>>                       pp->mem_bus_addr = pp->mem->start - win->offset;
>>>>>>
>>>>>> That warning was added for a reason - why should not we log legitimate
>>>>>> warnings ? AFAIU having resources larger than 4GB can lead to undefined
>>>>>> behaviour given the current ATU programming API.
>>>>> Yeah. I'm all for a warning if the size is larger than 4GB in case of
>>>>> non-prefetchable window as one of the ATU outbound translation
>>>>> channels is being used,
>>>>
>>>> Is it true for all DWC host controllers ? Or there may be another
>>>> exception whereby we would be forced to disable this warning altogether
>>>> ?
>>>>
>>>>> but, we are not employing any ATU outbound translation channel for
>>>>
>>>> What does this mean ? "we are not employing any ATU outbound...", is
>>>> this the tegra driver ? And what guarantees that this warning is not
>>>> legitimate on DWC host controllers that do use the ATU outbound
>>>> translation for prefetchable windows ?
>>>>
>>>> Can DWC maintainers chime in and clarify please ?
>>>
>>> Before this code section, there is the following function call
>>> pci_parse_request_of_pci_ranges(), which performs a simple validation for
>>> the IORESOURCE_MEM resource type.
>>> This validation checks if the resource is marked as prefetchable, if so,
>>> an error message "non-prefetchable memory resource required" is given and
>>> a return code with the -EINVAL value.
>>
>> That code checks if there is *at least* a non-prefetchable resource,
>> that's all it does.
>>
>>> In other words, to reach the code that Vidya is changing, it can be only
>>> if the resource is a non-prefetchable, any prefetchable resource will be
>>> blocked by the previous call, if I'm not mistaken.
>>
>> I think you are mistaken sorry.
>>
>>> Having this in mind, Vidya's change will not make the expected result
>>> aimed by him.
>>
>> I think Vidya's patch does what he expects, the question is whether
>> it is widely applicable to ALL DWC hosts, that's what I want to know.
>>
>>> I don't see any problem by having resources larger than 4GB, from what
>>> I'm seeing in the databook there isn't any restricting related to that as
>>> long they don't consume the maximum space that is addressable by the
>>> system (depending on if they are 32-bit or 64-bit system address).
>>>
>>> To be honest, I'm not seeing a system that could have this resource
>>> larger than 4GB, but it might exist some exception that I don't know of,
>>> that's why I accepted Alan's patch to warn the user that the resource
>>> exceeds the maximum for the 32 bits so that he can be aware that he
>>> *might* be consuming the maximum space addressable.
>>
>> I think it is most certainly a possibility to have > 4GB prefetchable
>> address spaces so we ought to fix this for good. I still have to
>> understand how the DWC host detects the memory region to be programmed
>> into the ATU given that there is more than one but only 1 ATU memory
>> region AFAICS.
> 
> Probably best to wait for Vidya to confirm since I'm not altogether
> familiar with PCI on Tegra194, but looking at the DTS files and the
> Tegra194 TRM, the prefetchable memory regions are set to a range in
> 0x1200000000-0x1fffffffff which is a region of the address map that
> is reserved for "PCIe aperture for > 32-bit OS". Part of that is in
> use for non-prefetchable memory (and ends up being programmed into
> the ATU) whereas a much larger part is used for prefetchable memory
> and is not programmed anywhere, as far as I can tell.Yes. That is true. In case of Tegra194, for 1-to-1 memory translations, 
there is no need to use any ATU regions at all as the HW is capable of 
generating a memory transaction on the bus on its own for any CPU 
generated reads/writes falling in these apertures and are not captured 
by ATU windows for generating other (config/IO) types of transactions. 
Since a part of 64-bit region is used for mapping non-prefetchable BARs 
of endpoints (which are only 32-bit in size), a translation is required 
from 64-bit CPU/AXI address to 32-bit PCIe bus address and ATU region is 
used precisely for this purpose. Since there is no need for ATU 
translation to do 1-to-1 memory mapping, rest of the 64-bit aperture is 
used for mapping prefetchable BARs. I'm not sure if this HW behavior is 
same across DWC implementations. I took a quick look at the 'ranges' 
property of other DWC implementations and none of them have a region 
marked as prefetchable...! I'm not sure how kernel handles mapping an 
endpoint's prefetchable BAR in this case (does it use the 
'non-prefetchable' mapping of 'ranges' property for 'prefetchable' BARs 
as well??)

> 
> But I think given that this is a designated region of the address
> map this is probably automatically redirected to the PCIe controller.
> What I don't know is if that's something Tegra-specific or whether all
> instantiations have something similar set up.
> 
> Thierry
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Re: [PATCH] PCI: dwc: Warn only for non-prefetchable memory resource size >4GB
  2020-05-20 17:46             ` Vidya Sagar
@ 2020-05-20 22:48               ` Rob Herring
  2020-05-22 12:06                 ` Thierry Reding
  0 siblings, 1 reply; 21+ messages in thread
From: Rob Herring @ 2020-05-20 22:48 UTC (permalink / raw)
  To: Vidya Sagar
  Cc: Thierry Reding, Gustavo Pimentel, Lorenzo Pieralisi,
	Bjorn Helgaas, jingoohan1, Andrew Murray, bhelgaas, jonathanh,
	linux-pci, linux-kernel, kthota, mmaddireddy, sagar.tv,
	Alan Mikhak

On Wed, May 20, 2020 at 11:16:32PM +0530, Vidya Sagar wrote:
> 
> 
> On 20-May-20 4:47 PM, Thierry Reding wrote:
> > On Tue, May 19, 2020 at 10:08:54PM +0000, Gustavo Pimentel wrote:
> > > On Tue, May 19, 2020 at 15:58:16, Lorenzo Pieralisi
> > > <lorenzo.pieralisi@arm.com> wrote:
> > > 
> > > > On Tue, May 19, 2020 at 07:25:02PM +0530, Vidya Sagar wrote:
> > > > > 
> > > > > 
> > > > > On 18-May-20 9:24 PM, Lorenzo Pieralisi wrote:
> > > > > > External email: Use caution opening links or attachments
> > > > > > 
> > > > > > 
> > > > > > On Wed, May 13, 2020 at 05:35:08PM -0500, Bjorn Helgaas wrote:
> > > > > > > [+cc Alan; please cc authors of relevant commits,
> > > > > > > updated Andrew's email address]
> > > > > > > 
> > > > > > > On Thu, May 14, 2020 at 12:38:55AM +0530, Vidya Sagar wrote:
> > > > > > > > commit 9e73fa02aa009 ("PCI: dwc: Warn if MEM resource size exceeds max for
> > > > > > > > 32-bits") enables warning for MEM resources of size >4GB but prefetchable
> > > > > > > >    memory resources also come under this category where sizes can go beyond
> > > > > > > > 4GB. Avoid logging a warning for prefetchable memory resources.
> > > > > > > > 
> > > > > > > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> > > > > > > > ---
> > > > > > > >    drivers/pci/controller/dwc/pcie-designware-host.c | 3 ++-
> > > > > > > >    1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > > > > 
> > > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > > > index 42fbfe2a1b8f..a29396529ea4 100644
> > > > > > > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > > > @@ -366,7 +366,8 @@ int dw_pcie_host_init(struct pcie_port *pp)
> > > > > > > >                       pp->mem = win->res;
> > > > > > > >                       pp->mem->name = "MEM";
> > > > > > > >                       mem_size = resource_size(pp->mem);
> > > > > > > > -                   if (upper_32_bits(mem_size))
> > > > > > > > +                   if (upper_32_bits(mem_size) &&
> > > > > > > > +                       !(win->res->flags & IORESOURCE_PREFETCH))
> > > > > > > >                               dev_warn(dev, "MEM resource size exceeds max for 32 bits\n");
> > > > > > > >                       pp->mem_size = mem_size;
> > > > > > > >                       pp->mem_bus_addr = pp->mem->start - win->offset;
> > > > > > 
> > > > > > That warning was added for a reason - why should not we log legitimate
> > > > > > warnings ? AFAIU having resources larger than 4GB can lead to undefined
> > > > > > behaviour given the current ATU programming API.
> > > > > Yeah. I'm all for a warning if the size is larger than 4GB in case of
> > > > > non-prefetchable window as one of the ATU outbound translation
> > > > > channels is being used,
> > > > 
> > > > Is it true for all DWC host controllers ? Or there may be another
> > > > exception whereby we would be forced to disable this warning altogether
> > > > ?
> > > > 
> > > > > but, we are not employing any ATU outbound translation channel for
> > > > 
> > > > What does this mean ? "we are not employing any ATU outbound...", is
> > > > this the tegra driver ? And what guarantees that this warning is not
> > > > legitimate on DWC host controllers that do use the ATU outbound
> > > > translation for prefetchable windows ?
> > > > 
> > > > Can DWC maintainers chime in and clarify please ?
> > > 
> > > Before this code section, there is the following function call
> > > pci_parse_request_of_pci_ranges(), which performs a simple validation for
> > > the IORESOURCE_MEM resource type.
> > > This validation checks if the resource is marked as prefetchable, if so,
> > > an error message "non-prefetchable memory resource required" is given and
> > > a return code with the -EINVAL value.
> > 
> > That's not what the code is doing. pci_parse_request_of_pci_range() will
> > traverse over the whole list of resources that it can find for the given
> > host controller and checks whether one of the resources defines prefetch
> > memory (note the res_valid |= ...). The error will only be returned if
> > no prefetchable memory region was found.
> > 
> > dw_pcie_host_init() will then again traverse the list of resources and
> > it will typically encounter two resource of type IORESOURCE_MEM, one for
> > non-prefetchable memory and another for prefetchable memory.
> > 
> > Vidya's patch is to differentiate between these two resources and allow
> > prefetchable memory regions to exceed sizes of 4 GiB.
> > 
> > That said, I wonder if there isn't a bigger problem at hand here. From
> > looking at the code it doesn't seem like the DWC driver makes any
> > distinction between prefetchable and non-prefetchable memory. Or at
> > least it doesn't allow both to be stored in struct pcie_port.
> > 
> > Am I missing something? Or can anyone explain how we're programming the
> > apertures for prefetchable vs. non-prefetchable memory? Perhaps this is
> > what Vidya was referring to when he said: "we are not using an outbound
> > ATU translation channel for prefetchable memory".
> > 
> > It looks to me like we're also getting partially lucky, or perhaps that
> > is by design, in that Tegra194 defines PCI regions in the following
> > order: I/O, prefetchable memory, non-prefetchable memory. That means
> > that the DWC core code will overwrite prefetchable memory data with that
> > of non-prefetchable memory and hence the non-prefetchable region ends up
> > stored in struct pcie_port and is then used to program the ATU outbound
> > channel.
> Well,it is by design. I mean, since the code is not differentiating between
> prefetchable and non-prefetchable regions, I ordered the entries in 'ranges'
> property in such a way that 'prefetchable' comes first followed by
> 'non-prefetchable' entry so that ATU region is used for generating the
> translation required for 'non-prefetchable' region (which is a non 1-to-1
> mapping)

You are getting lucky with your 'design'. Relying on order is fragile 
(except of course in the places in DT where order is defined, but ranges 
is not one of them).

Rob

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Re: [PATCH] PCI: dwc: Warn only for non-prefetchable memory resource size >4GB
  2020-05-20 22:48               ` Rob Herring
@ 2020-05-22 12:06                 ` Thierry Reding
  2020-05-22 13:32                   ` Lorenzo Pieralisi
  0 siblings, 1 reply; 21+ messages in thread
From: Thierry Reding @ 2020-05-22 12:06 UTC (permalink / raw)
  To: Rob Herring
  Cc: Vidya Sagar, Gustavo Pimentel, Lorenzo Pieralisi, Bjorn Helgaas,
	jingoohan1, Andrew Murray, bhelgaas, jonathanh, linux-pci,
	linux-kernel, kthota, mmaddireddy, sagar.tv, Alan Mikhak

[-- Attachment #1: Type: text/plain, Size: 6884 bytes --]

On Wed, May 20, 2020 at 04:48:16PM -0600, Rob Herring wrote:
> On Wed, May 20, 2020 at 11:16:32PM +0530, Vidya Sagar wrote:
> > 
> > 
> > On 20-May-20 4:47 PM, Thierry Reding wrote:
> > > On Tue, May 19, 2020 at 10:08:54PM +0000, Gustavo Pimentel wrote:
> > > > On Tue, May 19, 2020 at 15:58:16, Lorenzo Pieralisi
> > > > <lorenzo.pieralisi@arm.com> wrote:
> > > > 
> > > > > On Tue, May 19, 2020 at 07:25:02PM +0530, Vidya Sagar wrote:
> > > > > > 
> > > > > > 
> > > > > > On 18-May-20 9:24 PM, Lorenzo Pieralisi wrote:
> > > > > > > External email: Use caution opening links or attachments
> > > > > > > 
> > > > > > > 
> > > > > > > On Wed, May 13, 2020 at 05:35:08PM -0500, Bjorn Helgaas wrote:
> > > > > > > > [+cc Alan; please cc authors of relevant commits,
> > > > > > > > updated Andrew's email address]
> > > > > > > > 
> > > > > > > > On Thu, May 14, 2020 at 12:38:55AM +0530, Vidya Sagar wrote:
> > > > > > > > > commit 9e73fa02aa009 ("PCI: dwc: Warn if MEM resource size exceeds max for
> > > > > > > > > 32-bits") enables warning for MEM resources of size >4GB but prefetchable
> > > > > > > > >    memory resources also come under this category where sizes can go beyond
> > > > > > > > > 4GB. Avoid logging a warning for prefetchable memory resources.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> > > > > > > > > ---
> > > > > > > > >    drivers/pci/controller/dwc/pcie-designware-host.c | 3 ++-
> > > > > > > > >    1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > > > > > 
> > > > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > > > > index 42fbfe2a1b8f..a29396529ea4 100644
> > > > > > > > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > > > > @@ -366,7 +366,8 @@ int dw_pcie_host_init(struct pcie_port *pp)
> > > > > > > > >                       pp->mem = win->res;
> > > > > > > > >                       pp->mem->name = "MEM";
> > > > > > > > >                       mem_size = resource_size(pp->mem);
> > > > > > > > > -                   if (upper_32_bits(mem_size))
> > > > > > > > > +                   if (upper_32_bits(mem_size) &&
> > > > > > > > > +                       !(win->res->flags & IORESOURCE_PREFETCH))
> > > > > > > > >                               dev_warn(dev, "MEM resource size exceeds max for 32 bits\n");
> > > > > > > > >                       pp->mem_size = mem_size;
> > > > > > > > >                       pp->mem_bus_addr = pp->mem->start - win->offset;
> > > > > > > 
> > > > > > > That warning was added for a reason - why should not we log legitimate
> > > > > > > warnings ? AFAIU having resources larger than 4GB can lead to undefined
> > > > > > > behaviour given the current ATU programming API.
> > > > > > Yeah. I'm all for a warning if the size is larger than 4GB in case of
> > > > > > non-prefetchable window as one of the ATU outbound translation
> > > > > > channels is being used,
> > > > > 
> > > > > Is it true for all DWC host controllers ? Or there may be another
> > > > > exception whereby we would be forced to disable this warning altogether
> > > > > ?
> > > > > 
> > > > > > but, we are not employing any ATU outbound translation channel for
> > > > > 
> > > > > What does this mean ? "we are not employing any ATU outbound...", is
> > > > > this the tegra driver ? And what guarantees that this warning is not
> > > > > legitimate on DWC host controllers that do use the ATU outbound
> > > > > translation for prefetchable windows ?
> > > > > 
> > > > > Can DWC maintainers chime in and clarify please ?
> > > > 
> > > > Before this code section, there is the following function call
> > > > pci_parse_request_of_pci_ranges(), which performs a simple validation for
> > > > the IORESOURCE_MEM resource type.
> > > > This validation checks if the resource is marked as prefetchable, if so,
> > > > an error message "non-prefetchable memory resource required" is given and
> > > > a return code with the -EINVAL value.
> > > 
> > > That's not what the code is doing. pci_parse_request_of_pci_range() will
> > > traverse over the whole list of resources that it can find for the given
> > > host controller and checks whether one of the resources defines prefetch
> > > memory (note the res_valid |= ...). The error will only be returned if
> > > no prefetchable memory region was found.
> > > 
> > > dw_pcie_host_init() will then again traverse the list of resources and
> > > it will typically encounter two resource of type IORESOURCE_MEM, one for
> > > non-prefetchable memory and another for prefetchable memory.
> > > 
> > > Vidya's patch is to differentiate between these two resources and allow
> > > prefetchable memory regions to exceed sizes of 4 GiB.
> > > 
> > > That said, I wonder if there isn't a bigger problem at hand here. From
> > > looking at the code it doesn't seem like the DWC driver makes any
> > > distinction between prefetchable and non-prefetchable memory. Or at
> > > least it doesn't allow both to be stored in struct pcie_port.
> > > 
> > > Am I missing something? Or can anyone explain how we're programming the
> > > apertures for prefetchable vs. non-prefetchable memory? Perhaps this is
> > > what Vidya was referring to when he said: "we are not using an outbound
> > > ATU translation channel for prefetchable memory".
> > > 
> > > It looks to me like we're also getting partially lucky, or perhaps that
> > > is by design, in that Tegra194 defines PCI regions in the following
> > > order: I/O, prefetchable memory, non-prefetchable memory. That means
> > > that the DWC core code will overwrite prefetchable memory data with that
> > > of non-prefetchable memory and hence the non-prefetchable region ends up
> > > stored in struct pcie_port and is then used to program the ATU outbound
> > > channel.
> > Well,it is by design. I mean, since the code is not differentiating between
> > prefetchable and non-prefetchable regions, I ordered the entries in 'ranges'
> > property in such a way that 'prefetchable' comes first followed by
> > 'non-prefetchable' entry so that ATU region is used for generating the
> > translation required for 'non-prefetchable' region (which is a non 1-to-1
> > mapping)
> 
> You are getting lucky with your 'design'. Relying on order is fragile 
> (except of course in the places in DT where order is defined, but ranges 
> is not one of them).

Yeah, I think the DWC core should be improved to differentiate between
the two types of memory resources. There shouldn't be a need to encode
any ordering because the type is already part of the value in the
ranges property.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Re: [PATCH] PCI: dwc: Warn only for non-prefetchable memory resource size >4GB
  2020-05-22 12:06                 ` Thierry Reding
@ 2020-05-22 13:32                   ` Lorenzo Pieralisi
  2020-05-22 14:06                     ` Thierry Reding
  0 siblings, 1 reply; 21+ messages in thread
From: Lorenzo Pieralisi @ 2020-05-22 13:32 UTC (permalink / raw)
  To: Thierry Reding
  Cc: Rob Herring, Vidya Sagar, Gustavo Pimentel, Bjorn Helgaas,
	jingoohan1, Andrew Murray, bhelgaas, jonathanh, linux-pci,
	linux-kernel, kthota, mmaddireddy, sagar.tv, Alan Mikhak

On Fri, May 22, 2020 at 02:06:55PM +0200, Thierry Reding wrote:
> On Wed, May 20, 2020 at 04:48:16PM -0600, Rob Herring wrote:
> > On Wed, May 20, 2020 at 11:16:32PM +0530, Vidya Sagar wrote:
> > > 
> > > 
> > > On 20-May-20 4:47 PM, Thierry Reding wrote:
> > > > On Tue, May 19, 2020 at 10:08:54PM +0000, Gustavo Pimentel wrote:
> > > > > On Tue, May 19, 2020 at 15:58:16, Lorenzo Pieralisi
> > > > > <lorenzo.pieralisi@arm.com> wrote:
> > > > > 
> > > > > > On Tue, May 19, 2020 at 07:25:02PM +0530, Vidya Sagar wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > On 18-May-20 9:24 PM, Lorenzo Pieralisi wrote:
> > > > > > > > External email: Use caution opening links or attachments
> > > > > > > > 
> > > > > > > > 
> > > > > > > > On Wed, May 13, 2020 at 05:35:08PM -0500, Bjorn Helgaas wrote:
> > > > > > > > > [+cc Alan; please cc authors of relevant commits,
> > > > > > > > > updated Andrew's email address]
> > > > > > > > > 
> > > > > > > > > On Thu, May 14, 2020 at 12:38:55AM +0530, Vidya Sagar wrote:
> > > > > > > > > > commit 9e73fa02aa009 ("PCI: dwc: Warn if MEM resource size exceeds max for
> > > > > > > > > > 32-bits") enables warning for MEM resources of size >4GB but prefetchable
> > > > > > > > > >    memory resources also come under this category where sizes can go beyond
> > > > > > > > > > 4GB. Avoid logging a warning for prefetchable memory resources.
> > > > > > > > > > 
> > > > > > > > > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> > > > > > > > > > ---
> > > > > > > > > >    drivers/pci/controller/dwc/pcie-designware-host.c | 3 ++-
> > > > > > > > > >    1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > > > > > > 
> > > > > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > > > > > index 42fbfe2a1b8f..a29396529ea4 100644
> > > > > > > > > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > > > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > > > > > @@ -366,7 +366,8 @@ int dw_pcie_host_init(struct pcie_port *pp)
> > > > > > > > > >                       pp->mem = win->res;
> > > > > > > > > >                       pp->mem->name = "MEM";
> > > > > > > > > >                       mem_size = resource_size(pp->mem);
> > > > > > > > > > -                   if (upper_32_bits(mem_size))
> > > > > > > > > > +                   if (upper_32_bits(mem_size) &&
> > > > > > > > > > +                       !(win->res->flags & IORESOURCE_PREFETCH))
> > > > > > > > > >                               dev_warn(dev, "MEM resource size exceeds max for 32 bits\n");
> > > > > > > > > >                       pp->mem_size = mem_size;
> > > > > > > > > >                       pp->mem_bus_addr = pp->mem->start - win->offset;
> > > > > > > > 
> > > > > > > > That warning was added for a reason - why should not we log legitimate
> > > > > > > > warnings ? AFAIU having resources larger than 4GB can lead to undefined
> > > > > > > > behaviour given the current ATU programming API.
> > > > > > > Yeah. I'm all for a warning if the size is larger than 4GB in case of
> > > > > > > non-prefetchable window as one of the ATU outbound translation
> > > > > > > channels is being used,
> > > > > > 
> > > > > > Is it true for all DWC host controllers ? Or there may be another
> > > > > > exception whereby we would be forced to disable this warning altogether
> > > > > > ?
> > > > > > 
> > > > > > > but, we are not employing any ATU outbound translation channel for
> > > > > > 
> > > > > > What does this mean ? "we are not employing any ATU outbound...", is
> > > > > > this the tegra driver ? And what guarantees that this warning is not
> > > > > > legitimate on DWC host controllers that do use the ATU outbound
> > > > > > translation for prefetchable windows ?
> > > > > > 
> > > > > > Can DWC maintainers chime in and clarify please ?
> > > > > 
> > > > > Before this code section, there is the following function call
> > > > > pci_parse_request_of_pci_ranges(), which performs a simple validation for
> > > > > the IORESOURCE_MEM resource type.
> > > > > This validation checks if the resource is marked as prefetchable, if so,
> > > > > an error message "non-prefetchable memory resource required" is given and
> > > > > a return code with the -EINVAL value.
> > > > 
> > > > That's not what the code is doing. pci_parse_request_of_pci_range() will
> > > > traverse over the whole list of resources that it can find for the given
> > > > host controller and checks whether one of the resources defines prefetch
> > > > memory (note the res_valid |= ...). The error will only be returned if
> > > > no prefetchable memory region was found.
> > > > 
> > > > dw_pcie_host_init() will then again traverse the list of resources and
> > > > it will typically encounter two resource of type IORESOURCE_MEM, one for
> > > > non-prefetchable memory and another for prefetchable memory.
> > > > 
> > > > Vidya's patch is to differentiate between these two resources and allow
> > > > prefetchable memory regions to exceed sizes of 4 GiB.
> > > > 
> > > > That said, I wonder if there isn't a bigger problem at hand here. From
> > > > looking at the code it doesn't seem like the DWC driver makes any
> > > > distinction between prefetchable and non-prefetchable memory. Or at
> > > > least it doesn't allow both to be stored in struct pcie_port.
> > > > 
> > > > Am I missing something? Or can anyone explain how we're programming the
> > > > apertures for prefetchable vs. non-prefetchable memory? Perhaps this is
> > > > what Vidya was referring to when he said: "we are not using an outbound
> > > > ATU translation channel for prefetchable memory".
> > > > 
> > > > It looks to me like we're also getting partially lucky, or perhaps that
> > > > is by design, in that Tegra194 defines PCI regions in the following
> > > > order: I/O, prefetchable memory, non-prefetchable memory. That means
> > > > that the DWC core code will overwrite prefetchable memory data with that
> > > > of non-prefetchable memory and hence the non-prefetchable region ends up
> > > > stored in struct pcie_port and is then used to program the ATU outbound
> > > > channel.
> > > Well,it is by design. I mean, since the code is not differentiating between
> > > prefetchable and non-prefetchable regions, I ordered the entries in 'ranges'
> > > property in such a way that 'prefetchable' comes first followed by
> > > 'non-prefetchable' entry so that ATU region is used for generating the
> > > translation required for 'non-prefetchable' region (which is a non 1-to-1
> > > mapping)
> > 
> > You are getting lucky with your 'design'. Relying on order is fragile 
> > (except of course in the places in DT where order is defined, but ranges 
> > is not one of them).
> 
> Yeah, I think the DWC core should be improved to differentiate between
> the two types of memory resources. There shouldn't be a need to encode
> any ordering because the type is already part of the value in the
> ranges property.

DWC resources handling is broken beyond belief. In practical terms, I
think the best thing I can do is dropping:

9e73fa02aa00 ("PCI: dwc: Warn if MEM resource size exceeds max for 32-bits")

from my pci/dwc branch. However, the ATU programming API must be fixed
and this reliance on DT entries ordering avoided - it is really bad
practice (and it prevents us from reworking kernel code in ways that are
legitimate but would break owing to DT assumptions).

So yes, the DWC host bridge code must be updated asap - this is not
acceptable.

Thanks,
Lorenzo

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: PCI: dwc: Warn only for non-prefetchable memory resource size >4GB
  2020-05-20  2:33           ` Alan Mikhak
@ 2020-05-22 14:04             ` Lorenzo Pieralisi
  0 siblings, 0 replies; 21+ messages in thread
From: Lorenzo Pieralisi @ 2020-05-22 14:04 UTC (permalink / raw)
  To: Alan Mikhak
  Cc: gustavo.pimentel, alan.mikhak, amurray, bhelgaas, helgaas,
	jingoohan1, jonathanh, kthota, linux-kernel, linux-pci,
	mmaddireddy, sagar.tv, thierry.reding, vidyas, Alan Mikhak

On Tue, May 19, 2020 at 07:33:04PM -0700, Alan Mikhak wrote:
> Hi Lorenzo,
> 
> I came across this issue when implementing a Linux NVMe endpoint function
> driver under the Linux PCI Endpoint Framework:
> https://lwn.net/Articles/804369/
> 
> I needed to map up to 128GB of host memory using a single ATU window
> from the endpoint side because NVMe PRPs can be scattered all over host
> memory. In the process, I came across this 4GB limitation where the
> maximum size of memory that can be mapped is limited by what a u32 value
> can represent.
> 
> I submitted a separate patch to fix an undefined behavior that may also
> happen in dw_pcie_prog_outbound_atu_unroll() under some circumstances
> when the size of the memory being mapped is greater than what a u32 value
> can represent.
> https://patchwork.kernel.org/patch/11469701/
> 
> The above patch has been accepted. However, the variable pp->mem_size
> in dw_pcie_host_init() is still a u32 whereas the value returned by
> resource_size() is u64. If the resource size has non-zero upper 32-bits,
> those upper 32-bits will be lost when assigning:
>  pp->mem_size = resource_size(pp->mem).
> 
> Since current callers seem happy with the existing 4GB implementation
> and fixing the u32 limit is beyond my available resources and has a long
> high-impact tail, a warning seemed to be a good choice to highlight
> this issue in case someone else decides to map a MEM region that is
> greater than 4GB.
> 
> Removing the warning will avoid such discussions. Without this warning,
> this limitation will go unnoticed and will only impact whoever has to
> deal with it. It cost me time to figure it out when I had an application
> that needed a region larger than 4GB. I figured the most I could do about
> it is to raise the issue by adding a warning.

You did the right thing (and you helped me unearth some major
deficiencies in current DWC code). Unfortunately I have to drop:

9e73fa02aa00 ("PCI: dwc: Warn if MEM resource size exceeds max for 32-bits")

because it triggers regressions (and it is still not in the mainline,
IMO there would be more if we send it upstream).

I will keep:

e1fc129219a8 ("PCI: dwc: Program outbound ATU upper limit register")

because it is a step in the right direction and makes sense on its own.

Thanks for all the effort you put into this.

Lorenzo

> Regards,
> Alan
> 
> 
> 
> 
> 
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Re: [PATCH] PCI: dwc: Warn only for non-prefetchable memory resource size >4GB
  2020-05-22 13:32                   ` Lorenzo Pieralisi
@ 2020-05-22 14:06                     ` Thierry Reding
  2020-05-23 17:30                       ` Vidya Sagar
  0 siblings, 1 reply; 21+ messages in thread
From: Thierry Reding @ 2020-05-22 14:06 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: Rob Herring, Vidya Sagar, Gustavo Pimentel, Bjorn Helgaas,
	jingoohan1, Andrew Murray, bhelgaas, jonathanh, linux-pci,
	linux-kernel, kthota, mmaddireddy, sagar.tv, Alan Mikhak

[-- Attachment #1: Type: text/plain, Size: 10160 bytes --]

On Fri, May 22, 2020 at 02:32:49PM +0100, Lorenzo Pieralisi wrote:
> On Fri, May 22, 2020 at 02:06:55PM +0200, Thierry Reding wrote:
> > On Wed, May 20, 2020 at 04:48:16PM -0600, Rob Herring wrote:
> > > On Wed, May 20, 2020 at 11:16:32PM +0530, Vidya Sagar wrote:
> > > > 
> > > > 
> > > > On 20-May-20 4:47 PM, Thierry Reding wrote:
> > > > > On Tue, May 19, 2020 at 10:08:54PM +0000, Gustavo Pimentel wrote:
> > > > > > On Tue, May 19, 2020 at 15:58:16, Lorenzo Pieralisi
> > > > > > <lorenzo.pieralisi@arm.com> wrote:
> > > > > > 
> > > > > > > On Tue, May 19, 2020 at 07:25:02PM +0530, Vidya Sagar wrote:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > On 18-May-20 9:24 PM, Lorenzo Pieralisi wrote:
> > > > > > > > > External email: Use caution opening links or attachments
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > On Wed, May 13, 2020 at 05:35:08PM -0500, Bjorn Helgaas wrote:
> > > > > > > > > > [+cc Alan; please cc authors of relevant commits,
> > > > > > > > > > updated Andrew's email address]
> > > > > > > > > > 
> > > > > > > > > > On Thu, May 14, 2020 at 12:38:55AM +0530, Vidya Sagar wrote:
> > > > > > > > > > > commit 9e73fa02aa009 ("PCI: dwc: Warn if MEM resource size exceeds max for
> > > > > > > > > > > 32-bits") enables warning for MEM resources of size >4GB but prefetchable
> > > > > > > > > > >    memory resources also come under this category where sizes can go beyond
> > > > > > > > > > > 4GB. Avoid logging a warning for prefetchable memory resources.
> > > > > > > > > > > 
> > > > > > > > > > > Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> > > > > > > > > > > ---
> > > > > > > > > > >    drivers/pci/controller/dwc/pcie-designware-host.c | 3 ++-
> > > > > > > > > > >    1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > > > > > > > 
> > > > > > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > > > > > > index 42fbfe2a1b8f..a29396529ea4 100644
> > > > > > > > > > > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > > > > > > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > > > > > > > > > > @@ -366,7 +366,8 @@ int dw_pcie_host_init(struct pcie_port *pp)
> > > > > > > > > > >                       pp->mem = win->res;
> > > > > > > > > > >                       pp->mem->name = "MEM";
> > > > > > > > > > >                       mem_size = resource_size(pp->mem);
> > > > > > > > > > > -                   if (upper_32_bits(mem_size))
> > > > > > > > > > > +                   if (upper_32_bits(mem_size) &&
> > > > > > > > > > > +                       !(win->res->flags & IORESOURCE_PREFETCH))
> > > > > > > > > > >                               dev_warn(dev, "MEM resource size exceeds max for 32 bits\n");
> > > > > > > > > > >                       pp->mem_size = mem_size;
> > > > > > > > > > >                       pp->mem_bus_addr = pp->mem->start - win->offset;
> > > > > > > > > 
> > > > > > > > > That warning was added for a reason - why should not we log legitimate
> > > > > > > > > warnings ? AFAIU having resources larger than 4GB can lead to undefined
> > > > > > > > > behaviour given the current ATU programming API.
> > > > > > > > Yeah. I'm all for a warning if the size is larger than 4GB in case of
> > > > > > > > non-prefetchable window as one of the ATU outbound translation
> > > > > > > > channels is being used,
> > > > > > > 
> > > > > > > Is it true for all DWC host controllers ? Or there may be another
> > > > > > > exception whereby we would be forced to disable this warning altogether
> > > > > > > ?
> > > > > > > 
> > > > > > > > but, we are not employing any ATU outbound translation channel for
> > > > > > > 
> > > > > > > What does this mean ? "we are not employing any ATU outbound...", is
> > > > > > > this the tegra driver ? And what guarantees that this warning is not
> > > > > > > legitimate on DWC host controllers that do use the ATU outbound
> > > > > > > translation for prefetchable windows ?
> > > > > > > 
> > > > > > > Can DWC maintainers chime in and clarify please ?
> > > > > > 
> > > > > > Before this code section, there is the following function call
> > > > > > pci_parse_request_of_pci_ranges(), which performs a simple validation for
> > > > > > the IORESOURCE_MEM resource type.
> > > > > > This validation checks if the resource is marked as prefetchable, if so,
> > > > > > an error message "non-prefetchable memory resource required" is given and
> > > > > > a return code with the -EINVAL value.
> > > > > 
> > > > > That's not what the code is doing. pci_parse_request_of_pci_range() will
> > > > > traverse over the whole list of resources that it can find for the given
> > > > > host controller and checks whether one of the resources defines prefetch
> > > > > memory (note the res_valid |= ...). The error will only be returned if
> > > > > no prefetchable memory region was found.
> > > > > 
> > > > > dw_pcie_host_init() will then again traverse the list of resources and
> > > > > it will typically encounter two resource of type IORESOURCE_MEM, one for
> > > > > non-prefetchable memory and another for prefetchable memory.
> > > > > 
> > > > > Vidya's patch is to differentiate between these two resources and allow
> > > > > prefetchable memory regions to exceed sizes of 4 GiB.
> > > > > 
> > > > > That said, I wonder if there isn't a bigger problem at hand here. From
> > > > > looking at the code it doesn't seem like the DWC driver makes any
> > > > > distinction between prefetchable and non-prefetchable memory. Or at
> > > > > least it doesn't allow both to be stored in struct pcie_port.
> > > > > 
> > > > > Am I missing something? Or can anyone explain how we're programming the
> > > > > apertures for prefetchable vs. non-prefetchable memory? Perhaps this is
> > > > > what Vidya was referring to when he said: "we are not using an outbound
> > > > > ATU translation channel for prefetchable memory".
> > > > > 
> > > > > It looks to me like we're also getting partially lucky, or perhaps that
> > > > > is by design, in that Tegra194 defines PCI regions in the following
> > > > > order: I/O, prefetchable memory, non-prefetchable memory. That means
> > > > > that the DWC core code will overwrite prefetchable memory data with that
> > > > > of non-prefetchable memory and hence the non-prefetchable region ends up
> > > > > stored in struct pcie_port and is then used to program the ATU outbound
> > > > > channel.
> > > > Well,it is by design. I mean, since the code is not differentiating between
> > > > prefetchable and non-prefetchable regions, I ordered the entries in 'ranges'
> > > > property in such a way that 'prefetchable' comes first followed by
> > > > 'non-prefetchable' entry so that ATU region is used for generating the
> > > > translation required for 'non-prefetchable' region (which is a non 1-to-1
> > > > mapping)
> > > 
> > > You are getting lucky with your 'design'. Relying on order is fragile 
> > > (except of course in the places in DT where order is defined, but ranges 
> > > is not one of them).
> > 
> > Yeah, I think the DWC core should be improved to differentiate between
> > the two types of memory resources. There shouldn't be a need to encode
> > any ordering because the type is already part of the value in the
> > ranges property.
> 
> DWC resources handling is broken beyond belief. In practical terms, I
> think the best thing I can do is dropping:
> 
> 9e73fa02aa00 ("PCI: dwc: Warn if MEM resource size exceeds max for 32-bits")
> 
> from my pci/dwc branch. However, the ATU programming API must be fixed
> and this reliance on DT entries ordering avoided - it is really bad
> practice (and it prevents us from reworking kernel code in ways that are
> legitimate but would break owing to DT assumptions).
> 
> So yes, the DWC host bridge code must be updated asap - this is not
> acceptable.

Vidya, would you have any spare cycles to look into this a bit since
you're already familiar? I think for starters it would be good to add
a special case to the IORESOURCE_MEM case in dw_pcie_host_init() that
deals with IORESOURCE_PREFETCH set and then store the result in a
separate struct resource in struct pcie_port, something roughly along
the lines of:

	struct pcie_port {
		...
		struct resource *mem;
		struct resource *prefetch;
		...
	};

	...

	int dw_pcie_host_init(struct pcie_port *pp)
	{
		...
		resource_list_for_each_entry(win, &bridge->windows) {
			switch (resource_type(win->res)) {
			...
			case IORESOURCE_MEM:
				if (win->res.flags & IORESOURCE_PREFETCH) {
					pp->prefetch = win->res;
					...
				} else {
					pp->mem = win->res;
					...
				}
				break;
			...
		}
		...
	}

I suppose for the non-prefetchable memory we could leave the warning in
because they can never be larger than 32 bits anyway. Then again, I'm
not sure the check is actually fully correct. My recollection is that
non-prefetchable memory needs to be completely within the 4 GiB range,
rather than just the base and the size. So I think something like the
base starting at 3 GiB and then spanning 2 GiB would be valid according
to the current check, but I don't think it's valid according to the
specification.

The other interesting datapoint to have would be whether the DWC core
always has 1:1 mappings for prefetchable memory. If so, I think it might
be useful to still parse them, even if nothing in the driver is using
them. But I don't know what would be a good way to find out if that's
really the case.

I also saw, like you did, that none of the other, non-Tegra device trees
specify any prefetchable memory for the DWC, so I don't understand how
they would work. Perhaps they just don't support prefetchable memory?

If you don't have the time to do this I could possibly take a stab at it
but there are a few other things I need to look into, so I probably
won't get around to this within the next two or so weeks.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: Re: Re: [PATCH] PCI: dwc: Warn only for non-prefetchable memory resource size >4GB
  2020-05-22 14:06                     ` Thierry Reding
@ 2020-05-23 17:30                       ` Vidya Sagar
  2020-06-02 10:13                         ` Vidya Sagar
  0 siblings, 1 reply; 21+ messages in thread
From: Vidya Sagar @ 2020-05-23 17:30 UTC (permalink / raw)
  To: Thierry Reding, Lorenzo Pieralisi
  Cc: Rob Herring, Gustavo Pimentel, Bjorn Helgaas, jingoohan1,
	Andrew Murray, bhelgaas, jonathanh, linux-pci, linux-kernel,
	kthota, mmaddireddy, sagar.tv, Alan Mikhak



On 22-May-20 7:36 PM, Thierry Reding wrote:
> On Fri, May 22, 2020 at 02:32:49PM +0100, Lorenzo Pieralisi wrote:
>> On Fri, May 22, 2020 at 02:06:55PM +0200, Thierry Reding wrote:
>>> On Wed, May 20, 2020 at 04:48:16PM -0600, Rob Herring wrote:
>>>> On Wed, May 20, 2020 at 11:16:32PM +0530, Vidya Sagar wrote:
>>>>>
>>>>>
>>>>> On 20-May-20 4:47 PM, Thierry Reding wrote:
>>>>>> On Tue, May 19, 2020 at 10:08:54PM +0000, Gustavo Pimentel wrote:
>>>>>>> On Tue, May 19, 2020 at 15:58:16, Lorenzo Pieralisi
>>>>>>> <lorenzo.pieralisi@arm.com> wrote:
>>>>>>>
>>>>>>>> On Tue, May 19, 2020 at 07:25:02PM +0530, Vidya Sagar wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 18-May-20 9:24 PM, Lorenzo Pieralisi wrote:
>>>>>>>>>> External email: Use caution opening links or attachments
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On Wed, May 13, 2020 at 05:35:08PM -0500, Bjorn Helgaas wrote:
>>>>>>>>>>> [+cc Alan; please cc authors of relevant commits,
>>>>>>>>>>> updated Andrew's email address]
>>>>>>>>>>>
>>>>>>>>>>> On Thu, May 14, 2020 at 12:38:55AM +0530, Vidya Sagar wrote:
>>>>>>>>>>>> commit 9e73fa02aa009 ("PCI: dwc: Warn if MEM resource size exceeds max for
>>>>>>>>>>>> 32-bits") enables warning for MEM resources of size >4GB but prefetchable
>>>>>>>>>>>>     memory resources also come under this category where sizes can go beyond
>>>>>>>>>>>> 4GB. Avoid logging a warning for prefetchable memory resources.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
>>>>>>>>>>>> ---
>>>>>>>>>>>>     drivers/pci/controller/dwc/pcie-designware-host.c | 3 ++-
>>>>>>>>>>>>     1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
>>>>>>>>>>>> index 42fbfe2a1b8f..a29396529ea4 100644
>>>>>>>>>>>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>>>>>>>>>>>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>>>>>>>>>>>> @@ -366,7 +366,8 @@ int dw_pcie_host_init(struct pcie_port *pp)
>>>>>>>>>>>>                        pp->mem = win->res;
>>>>>>>>>>>>                        pp->mem->name = "MEM";
>>>>>>>>>>>>                        mem_size = resource_size(pp->mem);
>>>>>>>>>>>> -                   if (upper_32_bits(mem_size))
>>>>>>>>>>>> +                   if (upper_32_bits(mem_size) &&
>>>>>>>>>>>> +                       !(win->res->flags & IORESOURCE_PREFETCH))
>>>>>>>>>>>>                                dev_warn(dev, "MEM resource size exceeds max for 32 bits\n");
>>>>>>>>>>>>                        pp->mem_size = mem_size;
>>>>>>>>>>>>                        pp->mem_bus_addr = pp->mem->start - win->offset;
>>>>>>>>>>
>>>>>>>>>> That warning was added for a reason - why should not we log legitimate
>>>>>>>>>> warnings ? AFAIU having resources larger than 4GB can lead to undefined
>>>>>>>>>> behaviour given the current ATU programming API.
>>>>>>>>> Yeah. I'm all for a warning if the size is larger than 4GB in case of
>>>>>>>>> non-prefetchable window as one of the ATU outbound translation
>>>>>>>>> channels is being used,
>>>>>>>>
>>>>>>>> Is it true for all DWC host controllers ? Or there may be another
>>>>>>>> exception whereby we would be forced to disable this warning altogether
>>>>>>>> ?
>>>>>>>>
>>>>>>>>> but, we are not employing any ATU outbound translation channel for
>>>>>>>>
>>>>>>>> What does this mean ? "we are not employing any ATU outbound...", is
>>>>>>>> this the tegra driver ? And what guarantees that this warning is not
>>>>>>>> legitimate on DWC host controllers that do use the ATU outbound
>>>>>>>> translation for prefetchable windows ?
>>>>>>>>
>>>>>>>> Can DWC maintainers chime in and clarify please ?
>>>>>>>
>>>>>>> Before this code section, there is the following function call
>>>>>>> pci_parse_request_of_pci_ranges(), which performs a simple validation for
>>>>>>> the IORESOURCE_MEM resource type.
>>>>>>> This validation checks if the resource is marked as prefetchable, if so,
>>>>>>> an error message "non-prefetchable memory resource required" is given and
>>>>>>> a return code with the -EINVAL value.
>>>>>>
>>>>>> That's not what the code is doing. pci_parse_request_of_pci_range() will
>>>>>> traverse over the whole list of resources that it can find for the given
>>>>>> host controller and checks whether one of the resources defines prefetch
>>>>>> memory (note the res_valid |= ...). The error will only be returned if
>>>>>> no prefetchable memory region was found.
>>>>>>
>>>>>> dw_pcie_host_init() will then again traverse the list of resources and
>>>>>> it will typically encounter two resource of type IORESOURCE_MEM, one for
>>>>>> non-prefetchable memory and another for prefetchable memory.
>>>>>>
>>>>>> Vidya's patch is to differentiate between these two resources and allow
>>>>>> prefetchable memory regions to exceed sizes of 4 GiB.
>>>>>>
>>>>>> That said, I wonder if there isn't a bigger problem at hand here. From
>>>>>> looking at the code it doesn't seem like the DWC driver makes any
>>>>>> distinction between prefetchable and non-prefetchable memory. Or at
>>>>>> least it doesn't allow both to be stored in struct pcie_port.
>>>>>>
>>>>>> Am I missing something? Or can anyone explain how we're programming the
>>>>>> apertures for prefetchable vs. non-prefetchable memory? Perhaps this is
>>>>>> what Vidya was referring to when he said: "we are not using an outbound
>>>>>> ATU translation channel for prefetchable memory".
>>>>>>
>>>>>> It looks to me like we're also getting partially lucky, or perhaps that
>>>>>> is by design, in that Tegra194 defines PCI regions in the following
>>>>>> order: I/O, prefetchable memory, non-prefetchable memory. That means
>>>>>> that the DWC core code will overwrite prefetchable memory data with that
>>>>>> of non-prefetchable memory and hence the non-prefetchable region ends up
>>>>>> stored in struct pcie_port and is then used to program the ATU outbound
>>>>>> channel.
>>>>> Well,it is by design. I mean, since the code is not differentiating between
>>>>> prefetchable and non-prefetchable regions, I ordered the entries in 'ranges'
>>>>> property in such a way that 'prefetchable' comes first followed by
>>>>> 'non-prefetchable' entry so that ATU region is used for generating the
>>>>> translation required for 'non-prefetchable' region (which is a non 1-to-1
>>>>> mapping)
>>>>
>>>> You are getting lucky with your 'design'. Relying on order is fragile
>>>> (except of course in the places in DT where order is defined, but ranges
>>>> is not one of them).
>>>
>>> Yeah, I think the DWC core should be improved to differentiate between
>>> the two types of memory resources. There shouldn't be a need to encode
>>> any ordering because the type is already part of the value in the
>>> ranges property.
>>
>> DWC resources handling is broken beyond belief. In practical terms, I
>> think the best thing I can do is dropping:
>>
>> 9e73fa02aa00 ("PCI: dwc: Warn if MEM resource size exceeds max for 32-bits")
>>
>> from my pci/dwc branch. However, the ATU programming API must be fixed
>> and this reliance on DT entries ordering avoided - it is really bad
>> practice (and it prevents us from reworking kernel code in ways that are
>> legitimate but would break owing to DT assumptions).
>>
>> So yes, the DWC host bridge code must be updated asap - this is not
>> acceptable.
> 
> Vidya, would you have any spare cycles to look into this a bit since
> you're already familiar? I think for starters it would be good to add
> a special case to the IORESOURCE_MEM case in dw_pcie_host_init() that
> deals with IORESOURCE_PREFETCH set and then store the result in a
> separate struct resource in struct pcie_port, something roughly along
> the lines of:
> 
> 	struct pcie_port {
> 		...
> 		struct resource *mem;
> 		struct resource *prefetch;
> 		...
> 	};
> 
> 	...
> 
> 	int dw_pcie_host_init(struct pcie_port *pp)
> 	{
> 		...
> 		resource_list_for_each_entry(win, &bridge->windows) {
> 			switch (resource_type(win->res)) {
> 			...
> 			case IORESOURCE_MEM:
> 				if (win->res.flags & IORESOURCE_PREFETCH) {
> 					pp->prefetch = win->res;
> 					...
> 				} else {
> 					pp->mem = win->res;
> 					...
> 				}
> 				break;
> 			...
> 		}
> 		...
> 	}
> 
> I suppose for the non-prefetchable memory we could leave the warning in
> because they can never be larger than 32 bits anyway. Then again, I'm
> not sure the check is actually fully correct. My recollection is that
> non-prefetchable memory needs to be completely within the 4 GiB range,
> rather than just the base and the size. So I think something like the
> base starting at 3 GiB and then spanning 2 GiB would be valid according
> to the current check, but I don't think it's valid according to the
> specification.
> 
> The other interesting datapoint to have would be whether the DWC core
> always has 1:1 mappings for prefetchable memory. If so, I think it might
> be useful to still parse them, even if nothing in the driver is using
> them. But I don't know what would be a good way to find out if that's
> really the case.
> 
> I also saw, like you did, that none of the other, non-Tegra device trees
> specify any prefetchable memory for the DWC, so I don't understand how
> they would work. Perhaps they just don't support prefetchable memory?
> 
> If you don't have the time to do this I could possibly take a stab at it
> but there are a few other things I need to look into, so I probably
> won't get around to this within the next two or so weeks.
Sure. I'll try to come up with a patch set to address this at DWC core 
level.

Thanks,
Vidya Sagar
> 
> Thierry
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] PCI: dwc: Warn only for non-prefetchable memory resource size >4GB
  2020-05-23 17:30                       ` Vidya Sagar
@ 2020-06-02 10:13                         ` Vidya Sagar
  0 siblings, 0 replies; 21+ messages in thread
From: Vidya Sagar @ 2020-06-02 10:13 UTC (permalink / raw)
  To: Thierry Reding, Lorenzo Pieralisi
  Cc: Rob Herring, Gustavo Pimentel, Bjorn Helgaas, jingoohan1,
	Andrew Murray, bhelgaas, jonathanh, linux-pci, linux-kernel,
	kthota, mmaddireddy, sagar.tv, Alan Mikhak



On 23-May-20 11:00 PM, Vidya Sagar wrote:
> 
> 
> On 22-May-20 7:36 PM, Thierry Reding wrote:
>> On Fri, May 22, 2020 at 02:32:49PM +0100, Lorenzo Pieralisi wrote:
>>> On Fri, May 22, 2020 at 02:06:55PM +0200, Thierry Reding wrote:
>>>> On Wed, May 20, 2020 at 04:48:16PM -0600, Rob Herring wrote:
>>>>> On Wed, May 20, 2020 at 11:16:32PM +0530, Vidya Sagar wrote:
>>>>>>
>>>>>>
>>>>>> On 20-May-20 4:47 PM, Thierry Reding wrote:
>>>>>>> On Tue, May 19, 2020 at 10:08:54PM +0000, Gustavo Pimentel wrote:
>>>>>>>> On Tue, May 19, 2020 at 15:58:16, Lorenzo Pieralisi
>>>>>>>> <lorenzo.pieralisi@arm.com> wrote:
>>>>>>>>
>>>>>>>>> On Tue, May 19, 2020 at 07:25:02PM +0530, Vidya Sagar wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 18-May-20 9:24 PM, Lorenzo Pieralisi wrote:
>>>>>>>>>>> External email: Use caution opening links or attachments
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On Wed, May 13, 2020 at 05:35:08PM -0500, Bjorn Helgaas wrote:
>>>>>>>>>>>> [+cc Alan; please cc authors of relevant commits,
>>>>>>>>>>>> updated Andrew's email address]
>>>>>>>>>>>>
>>>>>>>>>>>> On Thu, May 14, 2020 at 12:38:55AM +0530, Vidya Sagar wrote:
>>>>>>>>>>>>> commit 9e73fa02aa009 ("PCI: dwc: Warn if MEM resource size 
>>>>>>>>>>>>> exceeds max for
>>>>>>>>>>>>> 32-bits") enables warning for MEM resources of size >4GB 
>>>>>>>>>>>>> but prefetchable
>>>>>>>>>>>>>     memory resources also come under this category where 
>>>>>>>>>>>>> sizes can go beyond
>>>>>>>>>>>>> 4GB. Avoid logging a warning for prefetchable memory 
>>>>>>>>>>>>> resources.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>>     drivers/pci/controller/dwc/pcie-designware-host.c | 3 ++-
>>>>>>>>>>>>>     1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>>>>>>>>>
>>>>>>>>>>>>> diff --git 
>>>>>>>>>>>>> a/drivers/pci/controller/dwc/pcie-designware-host.c 
>>>>>>>>>>>>> b/drivers/pci/controller/dwc/pcie-designware-host.c
>>>>>>>>>>>>> index 42fbfe2a1b8f..a29396529ea4 100644
>>>>>>>>>>>>> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
>>>>>>>>>>>>> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
>>>>>>>>>>>>> @@ -366,7 +366,8 @@ int dw_pcie_host_init(struct pcie_port 
>>>>>>>>>>>>> *pp)
>>>>>>>>>>>>>                        pp->mem = win->res;
>>>>>>>>>>>>>                        pp->mem->name = "MEM";
>>>>>>>>>>>>>                        mem_size = resource_size(pp->mem);
>>>>>>>>>>>>> -                   if (upper_32_bits(mem_size))
>>>>>>>>>>>>> +                   if (upper_32_bits(mem_size) &&
>>>>>>>>>>>>> +                       !(win->res->flags & 
>>>>>>>>>>>>> IORESOURCE_PREFETCH))
>>>>>>>>>>>>>                                dev_warn(dev, "MEM resource 
>>>>>>>>>>>>> size exceeds max for 32 bits\n");
>>>>>>>>>>>>>                        pp->mem_size = mem_size;
>>>>>>>>>>>>>                        pp->mem_bus_addr = pp->mem->start - 
>>>>>>>>>>>>> win->offset;
>>>>>>>>>>>
>>>>>>>>>>> That warning was added for a reason - why should not we log 
>>>>>>>>>>> legitimate
>>>>>>>>>>> warnings ? AFAIU having resources larger than 4GB can lead to 
>>>>>>>>>>> undefined
>>>>>>>>>>> behaviour given the current ATU programming API.
>>>>>>>>>> Yeah. I'm all for a warning if the size is larger than 4GB in 
>>>>>>>>>> case of
>>>>>>>>>> non-prefetchable window as one of the ATU outbound translation
>>>>>>>>>> channels is being used,
>>>>>>>>>
>>>>>>>>> Is it true for all DWC host controllers ? Or there may be another
>>>>>>>>> exception whereby we would be forced to disable this warning 
>>>>>>>>> altogether
>>>>>>>>> ?
>>>>>>>>>
>>>>>>>>>> but, we are not employing any ATU outbound translation channel 
>>>>>>>>>> for
>>>>>>>>>
>>>>>>>>> What does this mean ? "we are not employing any ATU 
>>>>>>>>> outbound...", is
>>>>>>>>> this the tegra driver ? And what guarantees that this warning 
>>>>>>>>> is not
>>>>>>>>> legitimate on DWC host controllers that do use the ATU outbound
>>>>>>>>> translation for prefetchable windows ?
>>>>>>>>>
>>>>>>>>> Can DWC maintainers chime in and clarify please ?
>>>>>>>>
>>>>>>>> Before this code section, there is the following function call
>>>>>>>> pci_parse_request_of_pci_ranges(), which performs a simple 
>>>>>>>> validation for
>>>>>>>> the IORESOURCE_MEM resource type.
>>>>>>>> This validation checks if the resource is marked as 
>>>>>>>> prefetchable, if so,
>>>>>>>> an error message "non-prefetchable memory resource required" is 
>>>>>>>> given and
>>>>>>>> a return code with the -EINVAL value.
>>>>>>>
>>>>>>> That's not what the code is doing. 
>>>>>>> pci_parse_request_of_pci_range() will
>>>>>>> traverse over the whole list of resources that it can find for 
>>>>>>> the given
>>>>>>> host controller and checks whether one of the resources defines 
>>>>>>> prefetch
>>>>>>> memory (note the res_valid |= ...). The error will only be 
>>>>>>> returned if
>>>>>>> no prefetchable memory region was found.
>>>>>>>
>>>>>>> dw_pcie_host_init() will then again traverse the list of 
>>>>>>> resources and
>>>>>>> it will typically encounter two resource of type IORESOURCE_MEM, 
>>>>>>> one for
>>>>>>> non-prefetchable memory and another for prefetchable memory.
>>>>>>>
>>>>>>> Vidya's patch is to differentiate between these two resources and 
>>>>>>> allow
>>>>>>> prefetchable memory regions to exceed sizes of 4 GiB.
>>>>>>>
>>>>>>> That said, I wonder if there isn't a bigger problem at hand here. 
>>>>>>> From
>>>>>>> looking at the code it doesn't seem like the DWC driver makes any
>>>>>>> distinction between prefetchable and non-prefetchable memory. Or at
>>>>>>> least it doesn't allow both to be stored in struct pcie_port.
>>>>>>>
>>>>>>> Am I missing something? Or can anyone explain how we're 
>>>>>>> programming the
>>>>>>> apertures for prefetchable vs. non-prefetchable memory? Perhaps 
>>>>>>> this is
>>>>>>> what Vidya was referring to when he said: "we are not using an 
>>>>>>> outbound
>>>>>>> ATU translation channel for prefetchable memory".
>>>>>>>
>>>>>>> It looks to me like we're also getting partially lucky, or 
>>>>>>> perhaps that
>>>>>>> is by design, in that Tegra194 defines PCI regions in the following
>>>>>>> order: I/O, prefetchable memory, non-prefetchable memory. That means
>>>>>>> that the DWC core code will overwrite prefetchable memory data 
>>>>>>> with that
>>>>>>> of non-prefetchable memory and hence the non-prefetchable region 
>>>>>>> ends up
>>>>>>> stored in struct pcie_port and is then used to program the ATU 
>>>>>>> outbound
>>>>>>> channel.
>>>>>> Well,it is by design. I mean, since the code is not 
>>>>>> differentiating between
>>>>>> prefetchable and non-prefetchable regions, I ordered the entries 
>>>>>> in 'ranges'
>>>>>> property in such a way that 'prefetchable' comes first followed by
>>>>>> 'non-prefetchable' entry so that ATU region is used for generating 
>>>>>> the
>>>>>> translation required for 'non-prefetchable' region (which is a non 
>>>>>> 1-to-1
>>>>>> mapping)
>>>>>
>>>>> You are getting lucky with your 'design'. Relying on order is fragile
>>>>> (except of course in the places in DT where order is defined, but 
>>>>> ranges
>>>>> is not one of them).
>>>>
>>>> Yeah, I think the DWC core should be improved to differentiate between
>>>> the two types of memory resources. There shouldn't be a need to encode
>>>> any ordering because the type is already part of the value in the
>>>> ranges property.
>>>
>>> DWC resources handling is broken beyond belief. In practical terms, I
>>> think the best thing I can do is dropping:
>>>
>>> 9e73fa02aa00 ("PCI: dwc: Warn if MEM resource size exceeds max for 
>>> 32-bits")
>>>
>>> from my pci/dwc branch. However, the ATU programming API must be fixed
>>> and this reliance on DT entries ordering avoided - it is really bad
>>> practice (and it prevents us from reworking kernel code in ways that are
>>> legitimate but would break owing to DT assumptions).
>>>
>>> So yes, the DWC host bridge code must be updated asap - this is not
>>> acceptable.
>>
>> Vidya, would you have any spare cycles to look into this a bit since
>> you're already familiar? I think for starters it would be good to add
>> a special case to the IORESOURCE_MEM case in dw_pcie_host_init() that
>> deals with IORESOURCE_PREFETCH set and then store the result in a
>> separate struct resource in struct pcie_port, something roughly along
>> the lines of:
>>
>>     struct pcie_port {
>>         ...
>>         struct resource *mem;
>>         struct resource *prefetch;
>>         ...
>>     };
>>
>>     ...
>>
>>     int dw_pcie_host_init(struct pcie_port *pp)
>>     {
>>         ...
>>         resource_list_for_each_entry(win, &bridge->windows) {
>>             switch (resource_type(win->res)) {
>>             ...
>>             case IORESOURCE_MEM:
>>                 if (win->res.flags & IORESOURCE_PREFETCH) {
>>                     pp->prefetch = win->res;
>>                     ...
>>                 } else {
>>                     pp->mem = win->res;
>>                     ...
>>                 }
>>                 break;
>>             ...
>>         }
>>         ...
>>     }
>>
>> I suppose for the non-prefetchable memory we could leave the warning in
>> because they can never be larger than 32 bits anyway. Then again, I'm
>> not sure the check is actually fully correct. My recollection is that
>> non-prefetchable memory needs to be completely within the 4 GiB range,
>> rather than just the base and the size. So I think something like the
>> base starting at 3 GiB and then spanning 2 GiB would be valid according
>> to the current check, but I don't think it's valid according to the
>> specification.
>>
>> The other interesting datapoint to have would be whether the DWC core
>> always has 1:1 mappings for prefetchable memory. If so, I think it might
>> be useful to still parse them, even if nothing in the driver is using
>> them. But I don't know what would be a good way to find out if that's
>> really the case.
>>
>> I also saw, like you did, that none of the other, non-Tegra device trees
>> specify any prefetchable memory for the DWC, so I don't understand how
>> they would work. Perhaps they just don't support prefetchable memory?
>>
>> If you don't have the time to do this I could possibly take a stab at it
>> but there are a few other things I need to look into, so I probably
>> won't get around to this within the next two or so weeks.
> Sure. I'll try to come up with a patch set to address this at DWC core 
> level.
New patch set for review is available @ 
http://patchwork.ozlabs.org/project/linux-pci/list/?series=180799

Thanks,
Vidya Sagar
> 
> Thanks,
> Vidya Sagar
>>
>> Thierry
>>

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2020-06-02 10:13 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-13 19:08 [PATCH] PCI: dwc: Warn only for non-prefetchable memory resource size >4GB Vidya Sagar
2020-05-13 22:35 ` Bjorn Helgaas
2020-05-18 15:54   ` Lorenzo Pieralisi
2020-05-19 13:55     ` Vidya Sagar
2020-05-19 14:58       ` Lorenzo Pieralisi
2020-05-19 17:08         ` Vidya Sagar
2020-05-19 18:20           ` Lorenzo Pieralisi
2020-05-19 22:08         ` Gustavo Pimentel
2020-05-20  2:33           ` Alan Mikhak
2020-05-22 14:04             ` Lorenzo Pieralisi
2020-05-20 11:06           ` [PATCH] " Lorenzo Pieralisi
2020-05-20 13:16             ` Thierry Reding
2020-05-20 17:51               ` Vidya Sagar
2020-05-20 11:17           ` Thierry Reding
2020-05-20 17:46             ` Vidya Sagar
2020-05-20 22:48               ` Rob Herring
2020-05-22 12:06                 ` Thierry Reding
2020-05-22 13:32                   ` Lorenzo Pieralisi
2020-05-22 14:06                     ` Thierry Reding
2020-05-23 17:30                       ` Vidya Sagar
2020-06-02 10:13                         ` Vidya Sagar

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).