From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753253AbbIPRjl (ORCPT ); Wed, 16 Sep 2015 13:39:41 -0400 Received: from mail-bl2on0060.outbound.protection.outlook.com ([65.55.169.60]:3232 "EHLO na01-bl2-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753083AbbIPRjg (ORCPT ); Wed, 16 Sep 2015 13:39:36 -0400 Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=David.Daney@caviumnetworks.com; Message-ID: <55F9A94B.4070702@caviumnetworks.com> Date: Wed, 16 Sep 2015 10:39:23 -0700 From: David Daney User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130625 Thunderbird/17.0.7 MIME-Version: 1.0 To: Will Deacon CC: Lorenzo Pieralisi , David Daney , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Rob Herring , Frank Rowand , "grant.likely@linaro.org" , Bjorn Helgaas , "linux-pci@vger.kernel.org" , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , "linux-arm-kernel@lists.infradead.org" , David Daney Subject: Re: [PATCH 4/6] PCI: generic: Correct, and avoid overflow, in bus_max calculation. References: <1442013719-5001-1-git-send-email-ddaney.cavm@gmail.com> <1442013719-5001-5-git-send-email-ddaney.cavm@gmail.com> <20150915174934.GL31157@arm.com> <55F85D4E.9020604@caviumnetworks.com> <20150915183500.GR31157@arm.com> <55F86764.4060502@caviumnetworks.com> <20150916104152.GC28771@arm.com> <20150916112852.GC17510@red-moon> <20150916172935.GT28771@arm.com> In-Reply-To: <20150916172935.GT28771@arm.com> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [64.2.3.194] X-ClientProxiedBy: BY2PR07CA073.namprd07.prod.outlook.com (10.141.251.48) To BY1PR0701MB1722.namprd07.prod.outlook.com (25.162.111.141) X-Microsoft-Exchange-Diagnostics: 1;BY1PR0701MB1722;2:YJB6dUljX3YQUbcE+D6kBopBiPIs7nbT0c4VJjHP+HOPuaMhovUeTicqUjC8wxNunLu8ngcnNpj9bcNUbe0vZKm97E6naeSB3+8I9PVakMC2UD9nV53yZvZf/1gInumIgrt9vmdLXdM8mu4wysVWBx5cjc55GYK4g9fo7KcrK40=;3:wNnzPsf/TdvgBX8jlXkQb7C26s7HXvA2Ppea3Tm+7FxBkvZmF3AdlGE7vRbzhDdf3SXgmKGWYPr6Syv+NRHsE1ec2W+ktC9bk+5g/eUau1KGy3nP8KKfq2BJyOQhLFTDoyughrow/wbxOo6DPtZr3w==;25:+/IYAdsFuysEmMnWpFJVZQoT+8WVXHkZLDC5aaloEUqDcsPpegTtCOSAba3W4FRXdYINN8qLjqXyR+wVi/15eOij/Ck2iQ3A80O5FSnHRgHaSJDfyrux63Cy/sb/gxof3oIXRimA+IuJa782WEnDLZS8lrz7Wg+qMoZdSlp24yufUW7Qz/NYCyF+PCmKpFQgDEuo9yzGAoGFsITdgFJnW7ws+8HxFJ1Oe78lXRfbRPSyJxlCtrmn9bTAdvx6+Yc9 X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:BY1PR0701MB1722; X-Microsoft-Exchange-Diagnostics: 1;BY1PR0701MB1722;20:iMJ0QpXwcueZuuuBz0ML4fGOIJd91EBfIOhB7baAZTfy1cb8brl8oS9Zt3Dx+WHRVyZ0g0Slx9ONtQsiVl3f39Jx5JahcfkQ2uXdqL7T6rn81ZdXy9AKN4cp06SUEMjsMnIZl1iWwzFcOm8rfbIDGnOWyeiP+SRS0NuB3tdHRh/GNVn2lluElBwN+DA6P0qtcbiaSkbA9G4OZ1wV1beFtHlcTNOiUwWR0sHnhLdDJdcRQwvO36IhXBb/RWRkSjDyoYZ5qlztUy3yha0EXsWoP4y+e/lUEqkymcEtgtmVXXw9wFzrcbz6lK3nqtUTJgRQOmYthqNuAjIcgvwD58rd+VRptiVu/oJQAKg8Zn+WFQbf9oj4x/SNTJR01gPRu86ko5Hf2SXxdxw2vSwEC3R9Y5zgOEEMweNBXKaKbPnSYyhQrKU5VInqoBiASNuhV8AnMZgvivb1L4Iouj/7t9zEjI2X6WpVX6jCQtMQnhJP2w4Y+tO6jcRMFbc8ENY8TxqGj83PnWmiQVI9U08jkOPZYoo+pfXj24SgrXJmACNRZ2zNcKpwZWo+x1n4Igf3f8Sg9VPkw2NcZaM14b5qqqDUWdUl/Ov5ifSCYPR3tQGDuyY=;4:7wMmFeanBFa/bklrdWJA4PJledmQUDwCuS6jEH8F0JzFhFMYxg5/AIDFoNy5y6kqRnRGUjoi7Sx5V11YOnx4e7vda0l7nhnbFem/Grl4U+ylaaT0YID0oW93SuvlbjvyfN7DtII3sfDs0eHyDkiYahGPF5Pzhv8DazvzyG1RF1mYcNV1n8cDUZIXonE1WR8lAZczVC1n6mcSl38BsqwZLH72B6OJYtr9yKwAmTz73Mn4eoqJXdiHkszN+rBttI9GCEo7Fj937DbDC9SwSGvgaXk1Yl7zEspHOeazmmwY5hSxSRx3QgM1dsz7l1kIwjrTEv5t7kj2aS+qK/CTkaK8lg== X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004)(520078)(8121501046)(5005006)(3002001);SRVR:BY1PR0701MB1722;BCL:0;PCL:0;RULEID:;SRVR:BY1PR0701MB1722; X-Forefront-PRVS: 07013D7479 X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10009020)(6009001)(479174004)(199003)(24454002)(377454003)(189002)(51444003)(2950100001)(5007970100001)(59896002)(33656002)(5004730100002)(46102003)(77096005)(53416004)(62966003)(5001860100001)(77156002)(23756003)(42186005)(5001830100001)(83506001)(92566002)(68736005)(93886004)(4001540100001)(97736004)(87976001)(81156007)(50986999)(40100003)(101416001)(69596002)(76176999)(5001920100001)(66066001)(4001350100001)(65806001)(19580395003)(105586002)(65956001)(47776003)(87266999)(54356999)(65816999)(50466002)(122386002)(64706001)(64126003)(36756003)(80316001)(106356001)(5001960100002)(110136002)(189998001)(41533002);DIR:OUT;SFP:1101;SCL:1;SRVR:BY1PR0701MB1722;H:dl.caveonetworks.com;FPR:;SPF:None;PTR:InfoNoRecords;A:1;MX:1;LANG:en; X-Microsoft-Exchange-Diagnostics: =?iso-8859-1?Q?1;BY1PR0701MB1722;23:L49YcSMVdpbrMkQUaQpiZFBmjFGum7nCnknSo?= =?iso-8859-1?Q?LPwLAvLigqpzrphhWLg7pXaTqQYM+E+WiXdb1oiVzb/EYh3Y6wCJgwkvNL?= =?iso-8859-1?Q?09oOLRWMUDG+azVCaoSwMUK/CdtW5M1uKOmvQSg4laqaZ0ZrNQhnVplmG2?= =?iso-8859-1?Q?/jko3dV255i256K0ahPVuXvCzzcyz+uCeNaYLDEBiHKYmYQ90OCgQeAx+h?= =?iso-8859-1?Q?klSn4oYNOOxKtITnLdZgOuyXZeBzJhimcu41BtZKPmc40kUdvj4aswjbdp?= =?iso-8859-1?Q?UFhuE/Bd2ISrlX6iBS9ftBUU10F1voJkhx4x2jUovHcChLhRe9mjeCAz2F?= =?iso-8859-1?Q?MobwdvPHaqsvtaQgSS+pWFqgBd401hl/RhBzLeoqu2xxc3s45LLEIxbLYB?= =?iso-8859-1?Q?KS6EeNMh/ZTwHPTKqRTj3CBCt62CUSquNR44wVQTOh3pRy47R9fhGYrCuw?= =?iso-8859-1?Q?ia1sXCQovu1MPvHhnkfDTX07DMLRvUH/NXE4HPQFedyJgXP6TSYu9ukqM3?= =?iso-8859-1?Q?bxuK4Yvlf799nI7NdY8vAm9eZ3rgfJRwJmltw4syQAaZRj+OUuWkqX1kCx?= =?iso-8859-1?Q?5joTChPiybqVQ8yd7W4PDLn8lQAhx+eE+7NHfBMRwTFJc+lyBkBdbAiDyb?= =?iso-8859-1?Q?trpTdZiEeUsFZsV1EojkXblc3vF4czQKBRlrFsHIbTYFWvfh7fsRRUwX3V?= =?iso-8859-1?Q?gdxr9zYqXVjimwHAxRhuI5Xk1dt+JIgGpxjjeWPdjl7UGETx1kIWa7eMQP?= =?iso-8859-1?Q?ypWzWVosKuYOQW35Lst6P+2lIxKEqxYCmfa5duYf+sHmeb1c1MK7fwDSsN?= =?iso-8859-1?Q?7QrApDhcRRN2kIVqCLoNSJpcnD/t6AVXeyfm+Ny4SYgcXqNGkToDnJfkXp?= =?iso-8859-1?Q?g8Wz5vmonekCwuWF1efaQO/WTLFm+KsuoGkwH3vCAxDYL9CD2TTAtGno2F?= =?iso-8859-1?Q?Iy1NzUfnuxjUO9G4HEozqnbEq+nJKf9s14oXwuNaoiVU95NCPnB3VTdlIw?= =?iso-8859-1?Q?ijvK2dYaloevEKgfwAPXcT91uB3cx6iTcsBBo2xhSg3sk5+Dce63AxcjIt?= =?iso-8859-1?Q?pD31RXu6SAAgkwcc2il+MbyTqOpmAGo/UuMn7t/xLCu1lSWf3YI6OPQH7v?= =?iso-8859-1?Q?R4nFH5Pw4sY1lJnrHJJwQSa+sjwukJY6PtdNn9BVZ7xSHXDTFEtOTElZEm?= =?iso-8859-1?Q?T2tuijZWn/NkAWi/FabAlgoyug8lI9+6flRn8kYeDd1wNobKvLyDws55pC?= =?iso-8859-1?Q?k34eIPRZp2lN007OsO4jpuFclufgxVd1HjqlRs6EJDgeooPXcQbo0YTQMI?= =?iso-8859-1?Q?pgZmabSE1TyCZJYyeX23Fd6grnxgmoZhxP8scB5GJbLdavYRJE6lhM9GBK?= =?iso-8859-1?Q?ac9k7H8X5eZpKV59uwb4CWki45Uef/xk6eKnZd26MwJw2/7qpsz59BE449?= =?iso-8859-1?Q?JS0tmdANqG0bV7yam7Gzsn+aX0uIIfzhutbZNHBBtOTzt+60Nrf3jz4xCi?= =?iso-8859-1?Q?LMLAv8pRyDLQSbzaqvNqwB//xqXmmyovbVfxlLrCyrv?= X-Microsoft-Exchange-Diagnostics: 1;BY1PR0701MB1722;5:p66ZRi+8sCEx1ayX92igbR+xI1n5MblNgAz1Hcf4ujCZ5pEX0MdW2PXC82icFD7f+XOvBVE++z0KAXGS8s25mXikx1LLzNa7QnX+tc00wP/thwx93NwqolYkcUqKt4BEnhHoGPpOJiuCadl7c2Pmxw==;24:mml8HqTMwMAg3JKWmX6hyHf4fl/ZOISF38D1cBOiWjaShNKiYgKaIbJETTJ5Jr3FN0wRM+2J2KntZXxpjdlcyJpQc1HhILe3xKloYChk8W8=;20:3Yiz/pzs2LW4q2BURKyMo+Dmm4E8AngXn/TSe1fwkuVnppIHKwlxqpe0iBMDcywFBPcSp+MIVr9saTH7Ua1UnA== SpamDiagnosticOutput: 1:23 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: caviumnetworks.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Sep 2015 17:39:27.3754 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY1PR0701MB1722 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 09/16/2015 10:29 AM, Will Deacon wrote: > Hi Lorenzo, > > On Wed, Sep 16, 2015 at 12:28:52PM +0100, Lorenzo Pieralisi wrote: >> On Wed, Sep 16, 2015 at 11:41:53AM +0100, Will Deacon wrote: >>>> Here is the current code: >>>> >>>>>> bus_range = pci->cfg.bus_range; >>>>>> for (busn = bus_range->start; busn <= bus_range->end; ++busn) { >>>>>> u32 idx = busn - bus_range->start; >>>> >>>> The index is offset by the bus range start... >>>> >>>>>> u32 sz = 1 << pci->cfg.ops.bus_shift; >>>>>> >>>>>> pci->cfg.win[idx] = devm_ioremap(dev, >>>>>> pci->cfg.res.start + busn * sz, >>>>>> sz); >>>> >>>> But, the offset into the "reg" property is the raw bus number. >>>> >>>> >>>>>> if (!pci->cfg.win[idx]) >>>>>> return -ENOMEM; >>>>>> } >>>> >>>> >>>> I hope that makes it more clear. >>> >>> Got it. So we should be using idx instead of busn in the devm_ioremap >>> call above. >> >> I think that's not what's specified in the PCI firmware specification, >> at least for the MMCFG regions. For MMCFG regions (quoting the specs) >> the "base address of the memory mapped configuration space always >> corresponds to bus number 0 (regardless of the start bus number decoded >> by the host bridge)..." >> >> For the x86 implementation have a look at: >> >> arch/x86/pci/mmconfig_64.c mcfg_ioremap() >> >> static void __iomem *mcfg_ioremap(struct pci_mmcfg_region *cfg) >> { >> void __iomem *addr; >> u64 start, size; >> int num_buses; >> >> start = cfg->address + PCI_MMCFG_BUS_OFFSET(cfg->start_bus); >> num_buses = cfg->end_bus - cfg->start_bus + 1; >> size = PCI_MMCFG_BUS_OFFSET(num_buses); >> addr = ioremap_nocache(start, size); >> if (addr) >> addr -= PCI_MMCFG_BUS_OFFSET(cfg->start_bus); >> return addr; >> } >> >> The MCFG config accessors add back the PCI_MMCFG_BUS_OFFSET(cfg->start_bus) >> to the virtual address so that the proper virtual address is used when >> issuing the config cycles, that's my understanding. > > Ok. I think that whether the config space mapping or the config accessors > do the fixup should remain an implementation detail, but the resource > identifying config space should be dealt with consistently. > > So that means the reg property should describe everything from bus 0, > but then we only map the region corresponding to the bus-range. > >> So IMO we have to define what "reg" represents for ECAM in DT, we can't >> leave this open to interpretation (and I think makng MCFG and DT config >> work the same way would be ideal). I will update the Documentation/devicetree/bindings/pci/host-generic-pci.txt to reflect this interpretation. > > If we define reg to cover the whole config space from bus 0 onwards, > then I think the driver should work as-is today. It's slightly odd, in > that there may be a prefix of config space that maps to god-knows-where, > but it's consistent with ACPI and doesn't require us to change the driver. > > David? I agree with this approach for two reasons: 1) My interpretation of relevant specifications agrees with Lorenzo's 2) It is less work for me, as this is how my firmware is currently configured. This patch 4/6 is still necessary, as the bus_max calculation is broken for non-zero start_bus. I anticipate sending a new version of the patch set later today (PDT). I will add any Acked-by/Reviewed-by that I receive to the new set. David Daney