Messages in this thread | | | Subject | Re: Re: [PATCH] PCI: dwc: Warn only for non-prefetchable memory resource size >4GB | From | Vidya Sagar <> | Date | Wed, 20 May 2020 23:16:32 +0530 |
| |
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 >
| |