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

[Thierry Reding]

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@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.
> > > 
> > > 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: signature.asc
Description: PGP signature