From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751693AbbIOSqL (ORCPT ); Tue, 15 Sep 2015 14:46:11 -0400 Received: from mail-bn1bon0077.outbound.protection.outlook.com ([157.56.111.77]:18861 "EHLO na01-bn1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751074AbbIOSqH (ORCPT ); Tue, 15 Sep 2015 14:46:07 -0400 Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=David.Daney@caviumnetworks.com; Message-ID: <55F86764.4060502@caviumnetworks.com> Date: Tue, 15 Sep 2015 11:45:56 -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: 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 , Lorenzo Pieralisi 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> In-Reply-To: <20150915183500.GR31157@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: SN1PR07CA0011.namprd07.prod.outlook.com (25.162.170.149) To CY1PR0701MB1726.namprd07.prod.outlook.com (25.163.21.140) X-Microsoft-Exchange-Diagnostics: 1;CY1PR0701MB1726;2:+3tZs7RHhUunxgi0PJE5IujUtVmCk4OWhIzecjRDadX1aoKWUP/DCcLbv2EjHHGjkePkmwimTnSHmWZh0sRfYGno36FAxgg5VwYUyd/DuwcgGX8NDhalAf2e7uBpcm1k2IT8LIEwzT7zYlxys7Zm4nB/05ikPdWkESRhm9TOevo=;3:OWeA+6dmgLErx84vGn3Bii8sU1j6T+//r3Gn0qzOObzHxN/+7woYpgFy5rBtzWVHSOYvcx1nlsN0opiGbF76yUYyM+ENK84CxpPJD3U2dS2k2zydBbD0ubMmXqECZ06dR1/hv9Wr2lyN1lOAI+YbgA==;25:+tNqkpPQ7MGzNuQJKrKTBIrfQMhVsiOq2qcXiUBSpekiblVLjtRSdVE8t6I4LfdxuO3Ca5IazZb8IuL6XFRzMqMhPDBIDXaiQnOFcVeezFj0YBFLoWvMzx1UN8Lt5iht0oMwWIEd4g8SZlcNP/474YK/ZebnrZYRFIUyiyvZkGJ1eYOOFzwMj4mf/CDD+tUBLkVm3M6FRz+Uxh44x3kDygp22hqtxvdkaIXrkdiFywrLVC8kWynL+0FKpOR7/sEI X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:CY1PR0701MB1726; X-Microsoft-Exchange-Diagnostics: 1;CY1PR0701MB1726;20:7Vc0Zr4yLv4sCf938niATjvDgRgXlK/Nq1yoURX/5e2m8JmHifTZw5iMrx00mHVEuDhMBWI/TaL1oT1/N3rZiE5j693SEDCaXcCK0L3gV86UNjm2VzXXTPXuVX4Os0U0tS5hjSwq9VzjHCb2tdGE3m/BwtWohr60zOVOILLGUtBkO3ASqITqJ8quK4gUU20FnZpDxOMyQWQzY+Izjbl/3HKPM9TBKqqCW/hjlBH77ptFNThEcVTo4PVGlZssZ5R7djYfjQtSkQTrawxmdLiHzIU+tpprwlOfErUZUVlPEyob3/9W3b9gF74/0mQBcGRWV5OAZy6ZgMnBRLH5nsxwBHjAYGz34dpxelYQzPZjHo3nEzANRdfWnE8ZhvT2FDjpJpq4ql6s2lW71NpnJvzMa1H1j4nJxyLZ/z7PuHJD/KUkasWi8ltJwC/Ruor9HWvVEkDyoLebguBlsEP+5dpmWlYadmoDdoUmeR046eG0ys9rT87tWjpU+Fx4dNqDShHRT6bhIOqNDMdEEhtpUlI3U6URtN3/hfm0HqTRcbbMPTV5GEAxAaXLMwcsISgM4vTiFFitgyYH7fHVx8HF2xmHdvHzDiG/9twx6sv67d6JrGg= X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004)(5005006)(520075)(8121501046)(520078)(3002001);SRVR:CY1PR0701MB1726;BCL:0;PCL:0;RULEID:;SRVR:CY1PR0701MB1726; X-Microsoft-Exchange-Diagnostics: 1;CY1PR0701MB1726;4:kFIl6HB2Bu2c0GN6cBYUI1phc+cKO5RjNSVLrie31NQRlEHEn5wXlszZj2ebTy1zUzGKDGzkN8iPTOjYztBWOwNqW9GmuurQ/mjgJ5WyHg3WdAsuczKc/BfixuWrnCbngvdMPF+7RNMTtBtkVzjK43NTbqk7kN3wLrxFxLVQ+HZ5naNb7PRAxbebWwmceGoHpMnADJTdgrDL4/biQdJpEvuwmxb6uYLM3mn2yEh0Dvr6pUySnx66yNsKvDOJMSrKDWPVDpGQepSF/jN1cIZGAhNFG9pbg303ZPh7woQ1fgTHY2qcvydkGH6CjgthFuzZwbEO6M82Zu73dtymQLc7DlDgn7SwPIFFjmL+jw6K40o= X-Forefront-PRVS: 070092A9D3 X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10009020)(6009001)(199003)(479174004)(377454003)(189002)(24454002)(69596002)(83506001)(4001540100001)(50466002)(4001350100001)(2950100001)(64126003)(106356001)(87976001)(53416004)(87266999)(23756003)(19580395003)(101416001)(54356999)(50986999)(46102003)(42186005)(65816999)(105586002)(65956001)(64706001)(59896002)(47776003)(33656002)(5001860100001)(76176999)(93886004)(80316001)(189998001)(5007970100001)(36756003)(81156007)(68736005)(66066001)(110136002)(65806001)(77096005)(5001960100002)(62966003)(40100003)(122386002)(77156002)(97736004)(92566002)(5004730100002)(5001830100001)(41533002);DIR:OUT;SFP:1101;SCL:1;SRVR:CY1PR0701MB1726;H:dl.caveonetworks.com;FPR:;SPF:None;PTR:InfoNoRecords;A:1;MX:1;LANG:en; X-Microsoft-Exchange-Diagnostics: =?iso-8859-1?Q?1;CY1PR0701MB1726;23:KG96h/xyR8L3OnirEhNcMtctrLj3b8k5Qa4Bl?= =?iso-8859-1?Q?AByzX6DJsLavGecbm9HYq7SuW4PLuychn+8vKp3dIZu74Ao+LK6KwohW95?= =?iso-8859-1?Q?ICE5nSFva6Kl6cUQbFw/S5T1vwCRJIILtWrch4HGLuOh4Ke0ynjJkbH9gf?= =?iso-8859-1?Q?5nDCisAs5iZsJmP7J/LISqpZYwZ0YomUycZyg6MaxORIfqEWgQ3EqUeeXc?= =?iso-8859-1?Q?VpDQlK5Cyfktbp5VQT1rymkQ5nC0L8c7R+g+hBsIP0Z+Mf6Hb5eeGy9Hs0?= =?iso-8859-1?Q?L3q4LqUwWQHdXb74WJUIUtNoeqbKsDKC9sk6VX8OLAjuGsMHmclY8nDyVw?= =?iso-8859-1?Q?89z6+lWcoX7Kji1VCvqQUPtyap7OkQJojbtrcVYw6NaKsOPtmAXiRCCZWc?= =?iso-8859-1?Q?MOY7X1xPKTP9nOiQSRLnhvFPeAZwK0kMdXiChuCefBAkxIAq6968pdacOM?= =?iso-8859-1?Q?+F80xjiZ9ljpb4Kyej+8Li9x4IzqlycsnDVZVMls8H9llTkhWrNkghdZDt?= =?iso-8859-1?Q?peUxJcHWvKlKsaBboj5xqinD0Ga/lxOTs4F0w3uGVOui7Ss6pdlq1a2oLr?= =?iso-8859-1?Q?VDmn1vX1Pb0XlTp0M+7ZqPgqJUCRJiSYjOhSQPNw3yJJurCS5bjnqIMqmO?= =?iso-8859-1?Q?lZVL1LJbkUl/UdyozCUB7Gpjykpq5R0NcEuEfNLfd7dPNHkd7MZJgBTiMk?= =?iso-8859-1?Q?hvhF3sh0nPZ2aAKUYX37NHtMhlAPCzqgZh7uwOqynmzDMQp4aXbLVEGsTZ?= =?iso-8859-1?Q?PFV5qd/yuDT18lqwHV/2MGesEANKVnhzKXkiUWR/IjblUgD9UPin5Vi4EZ?= =?iso-8859-1?Q?RGrXz83/5z1gpKGIcRyxKylFlysKMfWWT1CUybfQ5Io7U3djTZlV/VDqT3?= =?iso-8859-1?Q?jQISknP/yzA8A5ATbJzIqruqYa2Qr5QYQ4l0jJ/L8E0yemKz7V9JGPeHF3?= =?iso-8859-1?Q?OyJ5LHk0zkp9aysjFU5i/FOq+viLZrDMiZE/r70pBiMFoEIkRkMkn9BETq?= =?iso-8859-1?Q?VGNUCxGgKdHqmWG4OGU8XTQdMrhR3Xp27Dlyz1VuNwnKTMpSS3/aFlHEy1?= =?iso-8859-1?Q?T5l5PjcZYQSptl8PRHRpXCjcRNZf3slxq8NLXvDW2o0OR70577ngs1aSeW?= =?iso-8859-1?Q?copTr/XeP+ZEFZXYxUZozqYFrB/OtbyLiLE9nsp7gi73cfKS0OZdpq/jdO?= =?iso-8859-1?Q?cpktBfq+Ybe/Tw0bgH9edkgX3iNWkbITlluHtHv2RXCLH71+qrLAfE5XZs?= =?iso-8859-1?Q?LcQYum5MY7a9Rcwo9s1OgyGGSqtOUq3XBw6dZO7nYAd5xAjrBwTH4vrXMv?= =?iso-8859-1?Q?a1D1B+3LCtN5Te5pqUDTp6coLOwol/uwyh+ItAqRd5zzFaKWxrWFdmwJ12?= =?iso-8859-1?Q?fhxBe2+VpMTYXdm+uhSSGFzBBHYjBWs2AmamF2xLQn3mJGXN77umqEu1lM?= =?iso-8859-1?Q?ECq2NlnyD2ngn9JOMi9N84kvMwt47J3yZ9qqojUxtaCEScF6qNTNfEbz8v?= =?iso-8859-1?B?dz09?= X-Microsoft-Exchange-Diagnostics: 1;CY1PR0701MB1726;5:c1xd44epgZ9Bgmw+tl0+yW/YEf/IXjogw4Ja1ZuXgjX5ZeI3st2H+YA9z9OpvANMQJd3enLPpv7L4phqe9WudeSnBK/rO9JPCw1yR81N/J2khL73RW3+D6S/Z45uQuKcKsJMFtVEqjJEns66iYHpXw==;24:u9ADpyfO5b0/qJiK7FrbalSoP5ZdXglhcdW0XgrIiR7KMxIY6pmQB93PR00WYcFhD8HfeTtcgOMIqnJFJ6v/akt2se4YiRRLcneDUz8Tu50=;20:KELNCg3TZWOuRWgD0V3ivuPMDoMsYEb/Dt9aUesJnEuzP2QF6dq2GPGut2sL5u125kJ3ba95k46HVpt0lb2qtw== SpamDiagnosticOutput: 1:23 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: caviumnetworks.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Sep 2015 18:46:01.2505 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY1PR0701MB1726 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 09/15/2015 11:35 AM, Will Deacon wrote: > On Tue, Sep 15, 2015 at 07:02:54PM +0100, David Daney wrote: >> On 09/15/2015 10:49 AM, Will Deacon wrote: >>> On Sat, Sep 12, 2015 at 12:21:57AM +0100, David Daney wrote: >>>> /* Limit the bus-range to fit within reg */ >>>> - bus_max = pci->cfg.bus_range->start + >>>> - (resource_size(&pci->cfg.res) >> pci->cfg.ops.bus_shift) - 1; >>>> + bus_max = (resource_size(&pci->cfg.res) >> pci->cfg.ops.bus_shift) - 1; >>>> + if (bus_max > 255) >>>> + bus_max = 255; >>>> pci->cfg.bus_range->end = min_t(resource_size_t, >>>> pci->cfg.bus_range->end, bus_max); >>> >>> Hmm, this is changing the meaning of the bus-range property in the >>> device-tree, which really needs to match what IEEE Std 1275-1994 requires. >> >> I doesn't change the bus-range. > > Not directly, but pci->cfg.bus_range is a resource populated from the > "bus-range" property in the device-tree, so it's changing how the driver > uses that property. > >>> My understanding was that the bus-range could be used to offset the config >>> space, which is why it's subtracted from the bus number in >>> gen_pci_map_cfg_bus_[e]cam. >> >> There is an inconsistency in the current code. The calculation of the >> cfg.win[?] pointers is done such that the beginning of the config space >> specified in the "reg" property corresponds to bus 0. > > I don't follow you here. The mapping functions explicitly subtract the > start of the bus range when calculating the window offset: > > resource_size_t idx = bus->number - pci->cfg.bus_range->start; > > so if I have bus-range = <128 255>; then bus 128 lives at the start of > the configuration space described by the reg property, not bus 0. > > Sorry if I'm being thick; I just can't see the inconsistency. > 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. >> The calculation that I am changing, was done such that the beginning of >> the config space specified in the "reg" property corresponds to the >> first bus of the "bus-range" >> >> Which is correct? I assumed that the config space specified in the >> "reg" property corresponds to bus 0. Based on this assumption, I made >> the bus_max calculation match. >> >> Due to hardware peculiarities, our bus-range starts at a non-zero bus >> number. So, something has to be done to make all the code agree on a >> single interpretation of the meaning "reg" property. > > I think you're the first to exercise this code, so it's definitely worth > us fixing whatever's going wrong. > >>> Also, why is your config space so large that >>> we end up overflowing bus_max? >> >> It isn't. The part of the patch that changes the type from u8 to int >> was just to add some sanity. The code was easily susceptible to >> overflow failures, it seemed best to change to int. > > Can we drop this part for now if it's not actually needed? > > Will >