From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755857AbbIWP5P (ORCPT ); Wed, 23 Sep 2015 11:57:15 -0400 Received: from mail-bn1bon0096.outbound.protection.outlook.com ([157.56.111.96]:23466 "EHLO na01-bn1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1755806AbbIWP5J (ORCPT ); Wed, 23 Sep 2015 11:57:09 -0400 Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=David.Daney@caviumnetworks.com; Message-ID: <5602CBCB.4030605@caviumnetworks.com> Date: Wed, 23 Sep 2015 08:56:59 -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: Arnd Bergmann CC: , David Daney , , Bjorn Helgaas , , Will Deacon , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , , Marc Zyngier , "David Daney" Subject: Re: [PATCH v3 3/6] PCI: generic: Quit clobbering our pci_ops. References: <1442965757-12925-1-git-send-email-ddaney.cavm@gmail.com> <1442965757-12925-4-git-send-email-ddaney.cavm@gmail.com> <1729894.341fjYUAKl@wuerfel> In-Reply-To: <1729894.341fjYUAKl@wuerfel> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [64.2.3.194] X-ClientProxiedBy: SN1PR0701CA0023.namprd07.prod.outlook.com (25.162.96.33) To BY1PR0701MB1724.namprd07.prod.outlook.com (25.162.111.143) X-Microsoft-Exchange-Diagnostics: 1;BY1PR0701MB1724;2:P72JDOMH0jCF8/ge1iMGB630VDKUdEz1m0UFPDdtT4rm0dZgpBoyQdZqDJPRhDU9m7u0xOEwd+wt/Ss7ZmU/KWO8CAXvT83rbew42aUcEXmGdHboBoHdFiUJiO3X8Fupuf92gmhDJEp+zgAlitYbimAV/Cmf1HYFnoBogWvwgDg=;3:QUseRv9X9zcPFP6hbQGnCGlkK4iHkrR4+nfRGoWGRBWBfun3ZtaiW8kaTv063eevaklrAYHyGk9g9jsToqk74gDT+lTBHdOe4lIAoy7dow0E3p95wmFBCnlFxpfVIuZ7Z49Ff3fehvaftBkCs2Tanw==;25:be2nYW9ieeSmLsdf58Q6FJxpDzRpaMxTZMtPSQT4yzzsijHI4LXdMyeFQgAsdLztp6kUS4EKgwqIHKmnM2NLSD2AzNGe+xY2dNklZ5nStf7UGddIzt7OyLvQYWhC+PS5iuxZAy3EzG9Lo/x2SJpD2bB6z0ukQslOnzOB+6frAJM83yWquf5Y+jbfKrCfc0WNl//WAyqk3uNXMLfN24s0bzXxAhiJBWiQH4FeFqnHEDOhcF7iAELpCtLl37u3Bb7Z6bdvv6CNj4Ay3ppDqTExXA== X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:BY1PR0701MB1724; X-Microsoft-Exchange-Diagnostics: 1;BY1PR0701MB1724;20:aK2e9y9BmawrlH0GLEyEFwKut5liOOVNyxg5QIS9jHC/dEJRJ5dpNKYHYbKt2LmI6e3LEZipYoWgP3XMA8rNjzJigKb0BpsxOIG4+8gYMwi/H2Etd17Nf2quv/NLeTf4b6ACjaHU1ImuWmkOlhb1Q/TwOZV0IYfwHIwWDGu1DvBOAHQlIlxu1rtcsc5eBxKRICE6TiR6L4bURRMJRR89PHfyl4wm1wbJ1x88ZJsFpms3yPGZrVrjFJoOixIE+DRVMh1zmcq1MwjhPqoDK9bSWESpo7rGUekad2Kcl8ZrAUJnqoWe/mCxABBdS6004Pz0ZaF985MOWdhhLlNeExGV8kQtEHJt+es84ZsbqC8PUU1VVUh25kg8mklbZmU7G7jn077EZEvdV5pi+j2hgxh9KqcVaSp9NUgShwlrigc2vulVWgd5ETN0CVoO7uB/BaXxtXsPQhX5MmdlhfZwbBQOdGYTvFaX6KQG7XTA6qlBBv+CpBwd3g0Cslv43NV/SGX3AxOix8i4cRDg1LnEU386O66P/f9EBJycVVDdcKzNN9cvQ+uivkQW4nM19lg9Tr5h7+1VUcPcRCeVhtWKq295U0pBVsZj7kABKcG5JIXKkgQ= X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004)(2401047)(520078)(8121501046)(5005006)(3002001);SRVR:BY1PR0701MB1724;BCL:0;PCL:0;RULEID:;SRVR:BY1PR0701MB1724; X-Microsoft-Exchange-Diagnostics: 1;BY1PR0701MB1724;4:qADXmkRPN/gDdzRxNLZnV+XEWdiMjfnPtKdKtyucYTPWNTzdmI4qVzieqK8Pu2NB3tE8efdD/7A25z4Mf2OYvEDFXM4Ir07JvX0Cp4uLNYOde+0jpbN7XmXzlyxllp6BoRdlC7xD7wkgxJBbxWFPczh4Eh+jJ2mWRVjZC1GWSjKKLVDeIjMXZAVsj2wXMjhDeRc5vWAMzNEdwaryIE6v8nArNalk4TH3drVukjyRR0fmbLwzkSXsah3NN0GGLWd0EkYX8xww6UqNW64gS6NpywfKvirj2ZcHYJ/w4HLfGxsh/etFsKn+J8UonwzItgtxqMr1fAdegVDk0XHnn31bH/QKXLves/xVkWLneZYn5K8= X-Forefront-PRVS: 07083FF734 X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10009020)(6009001)(199003)(377454003)(189002)(24454002)(479174004)(46102003)(77156002)(76176999)(2950100001)(54356999)(65816999)(92566002)(50986999)(53416004)(59896002)(47776003)(122386002)(87266999)(64126003)(105586002)(69596002)(42186005)(23756003)(19580405001)(68736005)(77096005)(64706001)(66066001)(65956001)(19580395003)(83506001)(110136002)(106356001)(33656002)(80316001)(87976001)(62966003)(189998001)(5001960100002)(5004730100002)(101416001)(5007970100001)(81156007)(4001540100001)(4001350100001)(5001860100001)(5001830100001)(97736004)(40100003)(36756003)(50466002)(65806001)(41533002);DIR:OUT;SFP:1101;SCL:1;SRVR:BY1PR0701MB1724;H:dl.caveonetworks.com;FPR:;SPF:None;PTR:InfoNoRecords;A:1;MX:1;LANG:en; X-Microsoft-Exchange-Diagnostics: =?iso-8859-1?Q?1;BY1PR0701MB1724;23:y6ihewkc07S90Tcv5GeQ8agSJBjBIBAEb/wCj?= =?iso-8859-1?Q?cbxpSmTTx9LsIa0ML1yRxkXEwMOQ+d/YQbxigqprJ0vz9TSwlfAwyaIZNf?= =?iso-8859-1?Q?p9PEl99FZETYD5rNc6CXKvhljOxXGaZgqpVxRZX6uV+GhXc2T0NnruV1T+?= =?iso-8859-1?Q?ruHdFwaMx07TrLLSQzgytEgO8+wU9bWZJKCD12XcNScI2X0bLnmPueTE7L?= =?iso-8859-1?Q?rgN2YnBx1X+CaVXcO/QvOHlselZoYOLqR3DP2Fz7/B1PO5Cv5kdVGzo1FO?= =?iso-8859-1?Q?VvfLv0ZU9nFXSzMSejqr2TfrxHD/0W6x7IO7FmTAMcnl0OfM3WVscq2JIZ?= =?iso-8859-1?Q?hhu+90FuddgphQEDpm1X7oiquo+XJJ4nGfeiTTfyEbMdgxGVaewbt4xdKQ?= =?iso-8859-1?Q?5yED9mViu2yOfkJmSVcrwpj9gK6OcsZqWc+O5cDQ9BULMDeD1bB3BFq5QI?= =?iso-8859-1?Q?nZabold25vm+n+Ep7ryWEFBDrRgaLFQ2f47tPHipAq3VzV92RlmIPniDFe?= =?iso-8859-1?Q?pTx+BxQKoTcqC5gFxZk60olQIceHXeaCCPvAgp5eY8alWXlaVasFwgkbxp?= =?iso-8859-1?Q?dgRgjESb4Mp7FY6CB7vRasW4/LKnvslqiA+Y7GsJxun6eKVbcsqnpJqYAU?= =?iso-8859-1?Q?E5w1O1ZAhUFVAABbOdAafBRRtNp5lwOUmRnV2Yjsn7LqKfnN+K8+ph+E+0?= =?iso-8859-1?Q?c8AtKf/1Le1jent+Y73l0fj5bUoqsEy9Js0ANxRtcG+0sIWYSCJG25CUdc?= =?iso-8859-1?Q?HcM+J9/lTQ/6C+E96it+uhi5Zgt7QHHAYZrXGdkLZeW5vvIFVLUC1yIMUi?= =?iso-8859-1?Q?BzYUZ3UgYcMG3qLqPHwPn+6xLJfrIrICMW4MxargOteGCCNjAjMZf2pzJp?= =?iso-8859-1?Q?/WrQYu3uS45ClnUjl3ju3IXynYtyINnBWQ+e9PL9Raw9uhx2r82fkEBHck?= =?iso-8859-1?Q?jBi0QakiZwY3wf8aGwvqAY+2a2aU7RGNYcGmOdm/ojIaBjHkfhjp7poeXR?= =?iso-8859-1?Q?yCFa0TtF3p09QNw9SPYsvZETdN815HXWtfvxARELBtgKwKTe60nIU+NLJR?= =?iso-8859-1?Q?9tRu0XGPjcrbjVgD0HT+5KJVZo1rgqtpbEeBI8P7hICQRNv6gaXvNBo4N3?= =?iso-8859-1?Q?0ULM2IkeUU4agMHjH7FURflODhtwSCPANDchAYWFOilPettdGngTG6u2js?= =?iso-8859-1?Q?GEv3qmDaugbzscHo4yr4ouOEZQVmcnV6WqE8773onnlMe5wbCLUHpnWLo3?= =?iso-8859-1?Q?eJNv+cMb5Ggh4i15tqV0QIGXPdUGN7WdPqtS6ahSqBpr1EOEnXoEeTXvql?= =?iso-8859-1?Q?U/IM3rZuDiPC3nR2FJfOqK2HbUpxLIiBv0X+Hy4CncBbDXa/ABNGdW+s8K?= =?iso-8859-1?Q?qMGQKEQUEruYKNDZR6jr+vNpuldaBdYtVl45IQWfvIuaoVD5Oa3VoB4ri3?= =?iso-8859-1?Q?gURep4h5nNdBO5lxyyP5jjz8Yeo2NQ3X8Ehvrwx5GvYMhQew6+RO6kseC+?= =?iso-8859-1?B?UT09?= X-Microsoft-Exchange-Diagnostics: 1;BY1PR0701MB1724;5:IuBmULsn+cmEbBDiUt8Z3lJMJmD1OU+xxq320Q5izpEmE6U2JCH/n58iJnwuilvYuprHnK09JMkMnnkJ+qFjlUeGI7Xmlvrc+APo3Fg8w4S73eDk8GOOMYHZD6ykUEOOUzfpEkTBeDg/SggqNSNsMw==;24:2guXIW7neRqS62j5oCUcB3SZL6x/QzcgRSWu/lByFzhg6t01+WSUwdVS7VIaGXMg09XB2L/DUzyMoLuGWHuJ55DqtIL/C8O3yx7FIe8SUss=;20:vGIfp4IG+82cKJ2zXO1OtH1VLLqIa5Hb0dxW2a4/P2eldyBK8L6Y8OyU/xCQRA3xXkci37rEOmCE/DOlU2NUQA== SpamDiagnosticOutput: 1:23 SpamDiagnosticMetadata: NSPM X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Sep 2015 15:57:03.0458 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY1PR0701MB1724 X-Microsoft-Exchange-Diagnostics: 1;BY1PR0701MB1351;2:DeNa1SBkdOJ9HcPPUbuvefbJODmV/jaL6b7F+A0oNdd6jtJTJsXz16rLFScq8HxVMr4jg3CMN3qh0QcClSZ8zeCsYI41JiuipAdggWb+TlCdIH6E6oA/RdCJD0/po2Ud6t2k3BCIUiDz6RhaGkV9YdM3sEE+2ORrvYM981B7VQE=;23:gBC1RZkWcjVfEjCd0fmv67E6U6Wq0icDipMQBwM5bLJCJl+NODF3nTg2LHfvbefVnv1CdRRv3LI9IV+IXJS+o07PT+x/Vh77TaWSAjyTbNKxKP3JghSNCO1EFelMvygZq6BWBVUUnk+f/I2qr3PHjeeolUSssHG2KydrG8y1lQfOL7x2AuSHrWdg7yCqvBqc X-OriginatorOrg: caviumnetworks.com Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 09/23/2015 01:21 AM, Arnd Bergmann wrote: > On Tuesday 22 September 2015 16:49:14 David Daney wrote: >> From: David Daney >> >> The pci-host-generic driver keeps a global struct pci_ops which it >> then patches with the .map_bus method appropriate for the bus device. >> A problem arises when the driver is used for two different types of >> bus devices, the .map_bus method for the last device probed clobbers >> the method for all previous devices. The result, only the last bus >> device probed has the proper .map_bus, and the others fail. >> >> Move the struct pci_ops into the bus specific structure, and >> initialize it when the bus device is probed. Keep a copy of the >> gen_pci_cfg_bus_ops structure, instead of a pointer to a global copy, > > This is a very useful change. > >> to future proof against the addition of bus specific elements to >> struct pci_ops. > > but I don't like this part. We should just not have bus specific > elements in pci_ops. We don't really have that here either, except > that the gen_pci driver had a hack for reusing the same operations > for things that are actually different. > > It's an established practice that anything named '*_operations' is > meant to be constant and ideally defined as 'static const ... *_ops;' > in the driver. We could try to enforce this better by marking > bus->ops as 'const' and changing all the instances of this structure > accordingly. > >> @@ -65,7 +65,11 @@ static void __iomem *gen_pci_map_cfg_bus_cam(struct pci_bus *bus, >> >> static struct gen_pci_cfg_bus_ops gen_pci_cfg_cam_bus_ops = { >> .bus_shift = 16, >> - .map_bus = gen_pci_map_cfg_bus_cam, >> + .ops = { >> + .map_bus = gen_pci_map_cfg_bus_cam, >> + .read = pci_generic_config_read, >> + .write = pci_generic_config_write, >> + } >> }; > > So this is good. We could in theory unify the map_bus functions > like this now: > > static void __iomem *gen_pci_map_cfg_bus(struct pci_bus *bus, > unsigned int devfn, > int where) > { > struct gen_pci *pci = bus->sysdata; > struct gen_pci_cfg_bus_ops *ops; > resource_size_t idx; > > ops = container_of(bus->ops, struct gen_pci_cfg_bus_ops, ops); > idx = bus->number - pci->cfg.bus_range->start; > > return pci->cfg.win[idx] + ((devfn << ops->dev_shift) | where); > } > > Not sure if that improves clarity or not, up to Will. > >> @@ -234,8 +237,7 @@ static int gen_pci_probe(struct platform_device *pdev) >> } >> >> of_id = of_match_node(gen_pci_of_match, np); >> - pci->cfg.ops = of_id->data; >> + pci->cfg.ops = *(struct gen_pci_cfg_bus_ops *)of_id->data; > > This is the part that grabbed my attention, we should not do it like this. > I will consider changing this so that a structure copy is not used, perhaps as you suggest above. David Daney. > Arnd >