From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S935346AbdJQL3f (ORCPT ); Tue, 17 Oct 2017 07:29:35 -0400 Received: from mail-bn3nam01on0053.outbound.protection.outlook.com ([104.47.33.53]:1645 "EHLO NAM01-BN3-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S932552AbdJQL3a (ORCPT ); Tue, 17 Oct 2017 07:29:30 -0400 Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=Vadim.Lomovtsev@cavium.com; Date: Tue, 17 Oct 2017 04:29:24 -0700 From: Vadim Lomovtsev To: Bjorn Helgaas Cc: bhelgaas@google.com, linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org, Wilson.Snyder@cavium.com, alex.williamson@redhat.com, David Daney , Manish Jaggi , vadim.lomovtsev@cavium.com Subject: Re: [PATCH v6] PCI: quirks: update Cavium ThunderX ACS quirk implementation Message-ID: <20171017112924.GA2354@localhost.localdomain> References: <1506344920-24016-1-git-send-email-Vadim.Lomovtsev@caviumnetworks.com> <1506536439-29318-1-git-send-email-Vadim.Lomovtsev@caviumnetworks.com> <20171016212320.GI25517@bhelgaas-glaptop.roam.corp.google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171016212320.GI25517@bhelgaas-glaptop.roam.corp.google.com> User-Agent: Mutt/1.6.1 (2016-04-27) X-Originating-IP: [50.233.148.156] X-ClientProxiedBy: CY4PR1701CA0001.namprd17.prod.outlook.com (10.171.208.11) To CY4PR07MB2997.namprd07.prod.outlook.com (10.172.116.11) X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: f7a9fcb3-acc4-4bc4-7f11-08d51552539b X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:(22001)(2017030254152)(2017052603199)(201703131423075)(201703031133081)(201702281549075);SRVR:CY4PR07MB2997; X-Microsoft-Exchange-Diagnostics: 1;CY4PR07MB2997;3:nA33lVtWXLQSShL5tDWBpeelOEYPYew/PESbHl9IF+mLPdEeAD6ckufGLI16pYJbIj3NijBu62wZ8IQjWSlKc/A+z7KOSJEla4ZftO8Yiz61S5GNL6G4sW145sKpLCbO8QVpa+wUEobyM86Z5/QN0nUnSezWQoEyCtO1uAY6Wf8SvNEc0UUUUVrW1W8xQUxBU0nDEtLTcPNEKd0O6NTabDaS12NCfnkNQE4ubiRXRLmm71fSdjygrjbagm6A2lRq;25:w99yYOAgfcYHIuppo5wsutE9EnCV04ilv+uhuuwscSCKviHjAhUqptzPvtRchYGgerC5kCaDMKkjeO++jy5BAPRaOBf+C5e7E+0zZijs9nAgNd2Hebib1INSxMdMIb6LNllazsQKK4Wm5O/EOmFcZC+20Kjx3GX3wkvQy5xh83POE2p59Z70zw8n/Kx4KbUdvrRuCUphVH1HCbKZw4NhK3TD3Vz4iqMAStq1iHymAFwUzaByRQEqQOH7k40TyvcLMBsRPUy8/PGmGmqqoQsfLyKQNLTkrQX+y2e8mOgleokyCTpotm8x3u/StTbeZHYjYwAN9vF8WxMcvLIh2R83KWoB1QRwRTkcx4n1/PWbfyE=;31:ah/AaPLeG11ahM9WT10+3ic2angofNa0V15c4N9rzaXzDB12nyWgK8dQmuvVJifYAZ+jjUuWWnzx0cRr6iaxb3sIxlUznqnIoDfE0zFF0lp65vyhk9XX7PNE/nJJj9tKlb9Rd//gyo4yXWXC7+GunGohI9fWXhXeIJn/247nWodFGtW0Rw22Ue9ErF0gD3h91a+mW9t1YlGjdi5gRSORA5huilLdT5MNcpmM7ejmCWw= X-MS-TrafficTypeDiagnostic: CY4PR07MB2997: X-Microsoft-Exchange-Diagnostics: 1;CY4PR07MB2997;20:xpg9zL3/cpX+pHJo1iaIeAz3Edy6aH16ETdEbr0ecgz9vPrMEvm4KCEV36fwQVD6vl2qeLNQ5oQI4qmEEz6PTyBJW7nKPQ732ioVgsQldYVh8QbH2VETCx3Y6r1xAcwG05HtorBtF0S6zA2bLI9a7qD2Wc90xkKenT+dU9EqKWcwQ/m8iATKhmA3/JNRvETqIzzNCVJQzdbkf+akYEvHgQu/WX8m93/c2ASxdyxoyOHeALLF4tGLeixXmymjUiwhNpHffNeJC/F3EUYDetHwDs/Q2VipGEsH3RB2DJbsOBdwjncSYFfHekQ/h8T/Az1AD6ZhO8N4MoX3Wd38/99YvkaQ4tlLgq9lhFXzo4tVAvnBI0bEILuBMjflKdpWh40+jnS5Cfk4KR6l/7NXqnScbEJlxZARt2dJdOdS3uX6Ks4YZXW5zAYwnljXvX2N3TCvRu84+hrfFo56YFeI99wMj4lmbAVVbb7gALv7jkEzctvMBpALRBvfTPJpGPNmgIj7DzMcyPKEBLQpG6gIgKLE7QLcwIJwT3nm1IQuhdAKpmrRcxTGYRE346JG8Hlu26QOcnfJD7JYcPPFsg+jReooxFJQ5boi+bdaKw9n6QJnZe0=;4:h4jQKSIxe5SPH5BVlTP8H/Wz+MCKyStS4EsFbFD82bXWznwlOhcTXkmItzl9xo12BeaUjiISsTamrqQoGvCNrD7TprdLNslAjlpbZrYpibJn3QWka6fgLgn2zHYmrDdRgiTTnoc9mYgUBF+lQ1hGDFUYKp4RAcA2HOmgQj4r284pX61cTQAmpi/OMvelwSTPmZtDNpFXoZyjs1tFlAuF6rHh500mbP+7gYWUPenA8a2k3vyJIqXJTImwyzhkhiVM X-Exchange-Antispam-Report-Test: UriScan:; X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(100000700101)(100105000095)(100000701101)(100105300095)(100000702101)(100105100095)(6040450)(2401047)(5005006)(8121501046)(10201501046)(3002001)(100000703101)(100105400095)(93006095)(6041248)(20161123560025)(20161123564025)(20161123555025)(20161123558100)(20161123562025)(201703131423075)(201702281528075)(201703061421075)(201703061406153)(6072148)(201708071742011)(100000704101)(100105200095)(100000705101)(100105500095);SRVR:CY4PR07MB2997;BCL:0;PCL:0;RULEID:(100000800101)(100110000095)(100000801101)(100110300095)(100000802101)(100110100095)(100000803101)(100110400095)(100000804101)(100110200095)(100000805101)(100110500095);SRVR:CY4PR07MB2997; X-Forefront-PRVS: 04631F8F77 X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10009020)(6069001)(6009001)(346002)(376002)(189002)(54534003)(24454002)(199003)(316002)(54906003)(8936002)(81156014)(5660300001)(16526018)(81166006)(1076002)(305945005)(6246003)(33656002)(478600001)(58126008)(6116002)(7736002)(107886003)(106356001)(8676002)(105586002)(4326008)(23726003)(72206003)(3846002)(53936002)(16586007)(47776003)(66066001)(25786009)(15650500001)(54356999)(6506006)(229853002)(76176999)(50986999)(2950100002)(55016002)(6916009)(9686003)(42882006)(50466002)(101416001)(97736004)(575784001)(83506001)(68736007)(61506002)(6666003)(189998001)(2906002)(18370500001);DIR:OUT;SFP:1101;SCL:1;SRVR:CY4PR07MB2997;H:localhost.localdomain;FPR:;SPF:None;PTR:InfoNoRecords;MX:1;A:1;LANG:en; X-Microsoft-Exchange-Diagnostics: =?us-ascii?Q?1;CY4PR07MB2997;23:u34wa1u/kvud/nsQFPKgCQ7Vo9adVfg30OyMjoIDy?= =?us-ascii?Q?jwEq/KGeoOWztOl3Sg8V5Vz50w/gw8dUvYc6OdkRfH+nanAUDcXfRBLyz5PI?= =?us-ascii?Q?r2kC3s5HzZ4Y/qr4SNS9ypL0UbfwTSCKfTh79k/H3aaR0pD7CT3cIQmNf/ED?= =?us-ascii?Q?tGCcBjRu05BhtFKwH+TCBpsAp2cYGsgZENPZgtW92ZaZlDYwC8xTmRh1Vi6B?= =?us-ascii?Q?GgdxxH1S9zVVa/DLfgNjXfoBFva1lf17DCNtAChJp7UZ7c0luVs6aT0Pomlo?= =?us-ascii?Q?oaZnhpEz+dSvhH6Ow08HZxhiz4y4qsNJCtR6sawmMR6INmnc6/pm7micx8CJ?= =?us-ascii?Q?dBnFudLOPtggu9rtzgsodLpvwBBUuqSjw9GLwB+K9TnUIkMCiybz6avEQbN2?= =?us-ascii?Q?aMc245Fn0mXclwEGktWXdvOyXfpOzmLXRtgeS65qxQmt8gyTXUsXAJ19g8xM?= =?us-ascii?Q?Y0yPGxTICG60eByWpZF3mSIR6wph0MKS9SqKJBSnIzO9uA2I9+MUy6b8T2RG?= =?us-ascii?Q?poB6hYVguQ7ZC7JbeGZMO+ARFXvt2itMoJ4/JtRypwE35BgIBUHNmkS2iP9/?= =?us-ascii?Q?m1EGVusmeky1ag2dNxDUzjnwyoiieirYzAMUPLljbxVNOnT8D6H2M9XEdPZ0?= =?us-ascii?Q?bmVNteunv/B1trvLP91eKEbrT9DZ5Vg7C2FafPTXNRClDUYNr8O6GpT+89cV?= =?us-ascii?Q?qqjvwyCMwqesTphPAFj51tkQzV525pgaiaMzWOyKKvPkVZWd8TBMb1tsdA9V?= =?us-ascii?Q?pfgHU7/HA8cZydJPqfK1wjaOD/0gw/f4y8evCeskrVXdwCyjoEJ5pI2ozEfR?= =?us-ascii?Q?wbHYm/KnZrGDaLXH7UR272J11NaTYR5xUuBkTO1RBeUiWSEnHG1M++qHgA9p?= =?us-ascii?Q?90sB3EqOGs9+Z/h9hT8Vr5GTbHpAI0U7MVXeEBjEyu3Pl/il9fEIyDhDiKsf?= =?us-ascii?Q?2AzrwuL1fVfx+DiOEMgNc2pkyOZ0h3zoHFH5FTffYTAkVt3jgiVyvpWjCYKY?= =?us-ascii?Q?C/PVeLe6ByaY5Beor7/vsEZrQwka/qkQ96J+15pEPZtY63wiaCCYwWvLDtoX?= =?us-ascii?Q?rDqFVaJs7rw37AHiAmvcYG3z+iEtRRVTFt49At4bjd4Pwrakh+PON5F9aOr2?= =?us-ascii?Q?MthnGaARW6eY8BW7R7M65HUWTfKhK6bN5rGQiAhaQESW2s2JiStLT15nrNT/?= =?us-ascii?Q?4UDN/EV8sM0uUFuIPwE4tKOb7/vZjomtBGLDC89z2LqK4CujohKFxaaac2Cd?= =?us-ascii?Q?2z1N/Db3WoFQk7Psdbb5w0TgqBMcuqULXsM3LDy0wKwGlk3uBCnC3GAicvNG?= =?us-ascii?Q?r9PRdA1a4slVn69IchHTPUZfPyyPjY4FdgbmRHNi8qG?= X-Microsoft-Exchange-Diagnostics: 1;CY4PR07MB2997;6:+5xov++e2JYP/Rm6iAOj+FWutBcZGuEugIj8El/wzEaA1iBfJd4ivJQHR7BGB+osBcoNqwFxfMqTD76We0XD+TGWdR875FSoHZoaKEodix2l+1O/yS4GUviUV5/8WQ0LfD8Hw1uBxWebr6ZDeMivyPNmXlqHk5RDSbdq87FOjZh8dKtN2O3aKrLxYdt7VwS/yTeMsmsgZ/+kQean2enqlNALB+7QWqomsYbcyxkqNAR6WoANTVd7CV8euY4etYiNegWdgoElx1McovO96Ev+4yvtBfxQPNOnHxVmkh8AKuPAaXWEJakmyeWw4O7VZ6a3xqWMnwAfnUNusKsPwaz/uA==;5:BC8XagWNirmtL09EsCexob1IDHLFevPw2LqPkZSgthXNAE99TA076gcdHk3KSmSOmfoJ0DlWC2+TKWoxmlmLCBu0KtBElqB1wGWt727RTsQGF+qkeCTJH+LB1Rrj6evU9bDDLSXwuttO9pZhYlf8hw==;24:HLraUumDLihM1sZJcrtzBWwowxUszI4qghtwA1yVACmtYlj4QUmNo8278ZMoIpTyXyKd0rJd8mSUgQWmVYGj0VWfsJSynpLRJw3HDsEIQ1E=;7:jdRJn46M8k0lLKNoW0h6sjRBu6NZJNu+Q78BcY+TacSucqne0kXUf7FOiT+pSeIC8BIfMFfzn/reflko2B6IB2/Au/uctPu34tfz/HJg9iR4rJKk9ln/3nf7Tcw9dDrnqahW34dB8K/V1ptHW8ffSokDctEU6/7ebZ77YEFPh+68gcH3X5gNb0iAFThC8eIL9SESb/106CQfCqwyG5AMmKFzRpCbdPzIhmqhsd4x1pg= SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: caviumnetworks.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Oct 2017 11:29:27.3244 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 711e4ccf-2e9b-4bcf-a551-4094005b6194 X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR07MB2997 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Oct 16, 2017 at 04:23:20PM -0500, Bjorn Helgaas wrote: Hi Bjorn, > [+cc David, Manish] > > Please use a subject line that tells more about what's going on. > "Update quirk" doesn't really convey any useful information. > Something like "Apply Cavium ThunderX ACS quirk only to Root Ports". > > On Wed, Sep 27, 2017 at 11:20:39AM -0700, Vadim Lomovtsev wrote: > > This commit makes Cavium PCI ACS quirk applicable only to Cavium > > ThunderX (CN8XXX) family PCIE Root Ports which has limited PCI capabilities > > in terms of no ACS support advertisement. However, the RTL internally > > implements similar protection as if ACS had completion/request redirection, > > upstream forwarding and validation features enabled. > > > > Current quirk implementation doesn't take into account PCIERCs which > > also needs to be quirked. So the pci device id check mask is updated > > and check of device ID moved into separate function. > > s/PCIE/PCIe/ above > > s/PCIERCs/PCIe Root Ports/ (I assume, since usually a Root Complex > itself doesn't appear as a pci_dev) > > > Signed-off-by: Vadim Lomovtsev > > --- > > v5 -> v6: comment typo fix: change 0xa00 to 0xa000 > > > > drivers/pci/quirks.c | 29 +++++++++++++++++++++-------- > > 1 file changed, 21 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > > index a4d3361..ed6c76d 100644 > > --- a/drivers/pci/quirks.c > > +++ b/drivers/pci/quirks.c > > @@ -4211,20 +4211,33 @@ static int pci_quirk_amd_sb_acs(struct pci_dev *dev, u16 acs_flags) > > #endif > > } > > > > -static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags) > > +/* > > + * The Cavium downstream ports doesn't advertise their ACS capability registers. > > + * However, the RTL internally implements similar protection as if > > + * ACS had completion redirection, forwarding and validation features enabled. > > + * So by this flags we're asserting that the hardware implements and > > + * enables equivalent ACS functionality for these flags. > > + */ > > +#define CAVIUM_CN8XXX_ACS_FLAGS (PCI_ACS_RR | PCI_ACS_CR | PCI_ACS_SV | PCI_ACS_UF) > > You are changing the set of flags, which isn't mentioned in the changelog. > I think the best thing would be to have two patches: one that changes the > set of flags and a second that changes the set of affected devices. > > This set of flags was used twice in the original patch, so a #define made > sense. But now you only use it once, so there's no benefit in adding the > #define, and adding it makes the change in the set of flags harder to see. > > > +static __inline__ bool pci_quirk_cavium_acs_match(struct pci_dev *dev) > > No need to use "__inline__" here. This isn't performance critical, > and the compiler will probably inline it anyway because there's only > one use. > > > { > > /* > > - * Cavium devices matching this quirk do not perform peer-to-peer > > - * with other functions, allowing masking out these bits as if they > > - * were unimplemented in the ACS capability. > > + * Effectively selects all downstream ports for whole ThunderX 1 family > > + * by 0xa000 mask (which represents 8 SoCs), while the lower bits of device ID > > + * are used to indicate which subdevice is used within the SoC. > > Please wrap your comments so they fit in 80 columns. > > > */ > > - acs_flags &= ~(PCI_ACS_SV | PCI_ACS_TB | PCI_ACS_RR | > > - PCI_ACS_CR | PCI_ACS_UF | PCI_ACS_DT); > > + return (pci_is_pcie(dev) && > > + (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT) && > > + ((dev->device & 0xf800) == 0xa000)); > > I'm just a little confused by the 0xa000 mask you refer to in the > comment, because the mask in the code is 0xf800. Sorry for confusion, will correct it to 0xf800 for comment. > > Previously the quirk applied to all Cavium device IDs in the range > 0xa000-0xa0ff. Now it applies to device IDs in the range > 0xa000-0xa7ff, but only if they are PCIe Root Ports. Right? Correct. Since this quirk is necessary for Root Ports only. Thanks for reply, I'll re-work this one accordingly to your comments and re-send v7. WBR, Vadim > > > +} > > > > - if (!((dev->device >= 0xa000) && (dev->device <= 0xa0ff))) > > +static int pci_quirk_cavium_acs(struct pci_dev *dev, u16 acs_flags) > > +{ > > + if (!pci_quirk_cavium_acs_match(dev)) > > return -ENOTTY; > > > > - return acs_flags ? 0 : 1; > > + return acs_flags & ~(CAVIUM_CN8XXX_ACS_FLAGS) ? 0 : 1; > > } > > > > static int pci_quirk_xgene_acs(struct pci_dev *dev, u16 acs_flags) > > -- > > 2.4.11 > >