RE: [PATCH] PCI: dwc: Warn only for non-prefetchable memory resource size >4GB

[Gustavo Pimentel]

On Tue, May 19, 2020 at 15:58:16, Lorenzo Pieralisi 
<lorenzo.pieralisi@xxxxxxx> 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@xxxxxxxxxx>
> > > > > ---
> > > > >   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
> > >