From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bjorn Helgaas Subject: Re: [PATCH] pci: fix incorrect value returned from pcie_get_speed_cap Date: Tue, 27 Nov 2018 14:40:49 -0600 Message-ID: <20181127204049.GA150956@google.com> References: <20181120004704.GA191199@google.com> <20181126224051.GB212532@google.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Content-Disposition: inline In-Reply-To: List-Id: Discussion list for AMD gfx List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org Sender: "amd-gfx" To: Mikulas Patocka Cc: Tal Gilboa , linux-pci-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Alex Deucher , amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org, Tariq Toukan T24gVHVlLCBOb3YgMjcsIDIwMTggYXQgMTE6NDk6NDNBTSAtMDUwMCwgTWlrdWxhcyBQYXRvY2th IHdyb3RlOgo+IE9uIE1vbiwgMjYgTm92IDIwMTgsIEJqb3JuIEhlbGdhYXMgd3JvdGU6Cj4gPiBP biBNb24sIE5vdiAxOSwgMjAxOCBhdCAwNjo0NzowNFBNIC0wNjAwLCBCam9ybiBIZWxnYWFzIHdy b3RlOgo+ID4gPiBPbiBUdWUsIE9jdCAzMCwgMjAxOCBhdCAxMjozNjowOFBNIC0wNDAwLCBNaWt1 bGFzIFBhdG9ja2Egd3JvdGU6Cj4gPiA+ID4gVGhlIG1hY3JvcyBQQ0lfRVhQX0xOS0NBUF9TTFNf KkdCIGFyZSB2YWx1ZXMsIG5vdCBiaXQgbWFza3MuIFdlIG11c3QgbWFzawo+ID4gPiA+IHRoZSBy ZWdpc3RlciBhbmQgY29tcGFyZSBpdCBhZ2FpbnN0IHRoZW0uCj4gPiA+ID4gCj4gPiA+ID4gVGhp cyBwYXRjaCBmaXhlcyBlcnJvcnMgImFtZGdwdTogW3Bvd2VycGxheV0gZmFpbGVkIHRvIHNlbmQg bWVzc2FnZSAyNjEKPiA+ID4gPiByZXQgaXMgMCIgZXJyb3JzIHdoZW4gUENJZS12MyBjYXJkIGlz IHBsdWdnZWQgaW50byBQQ0llLXYxIHNsb3QsIGJlY2F1c2UKPiA+ID4gPiB0aGUgc2xvdCBpcyBi ZWluZyBpbmNvcnJlY3RseSByZXBvcnRlZCBhcyBQQ0llLXYzIGNhcGFibGUuCj4gPiA+ID4gCj4g PiA+ID4gU2lnbmVkLW9mZi1ieTogTWlrdWxhcyBQYXRvY2thIDxtcGF0b2NrYUByZWRoYXQuY29t Pgo+ID4gPiA+IEZpeGVzOiA2Y2Y1N2JlMGY3OGUgKCJQQ0k6IEFkZCBwY2llX2dldF9zcGVlZF9j YXAoKSB0byBmaW5kIG1heCBzdXBwb3J0ZWQgbGluayBzcGVlZCIpCj4gPiA+ID4gQ2M6IHN0YWJs ZUB2Z2VyLmtlcm5lbC5vcmcJIyB2NC4xNysKPiA+ID4gPiAKPiA+ID4gPiAtLS0KPiA+ID4gPiAg ZHJpdmVycy9wY2kvcGNpLmMgfCAgICA4ICsrKystLS0tCj4gPiA+ID4gIDEgZmlsZSBjaGFuZ2Vk LCA0IGluc2VydGlvbnMoKyksIDQgZGVsZXRpb25zKC0pCj4gPiA+ID4gCj4gPiA+ID4gSW5kZXg6 IGxpbnV4LTQuMTkvZHJpdmVycy9wY2kvcGNpLmMKPiA+ID4gPiA9PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Cj4gPiA+ID4g LS0tIGxpbnV4LTQuMTkub3JpZy9kcml2ZXJzL3BjaS9wY2kuYwkyMDE4LTEwLTMwIDE2OjU4OjU4 LjAwMDAwMDAwMCArMDEwMAo+ID4gPiA+ICsrKyBsaW51eC00LjE5L2RyaXZlcnMvcGNpL3BjaS5j CTIwMTgtMTAtMzAgMTY6NTg6NTguMDAwMDAwMDAwICswMTAwCj4gPiA+ID4gQEAgLTU0OTIsMTMg KzU0OTIsMTMgQEAgZW51bSBwY2lfYnVzX3NwZWVkIHBjaWVfZ2V0X3NwZWVkX2NhcChzdAo+ID4g PiA+ICAKPiA+ID4gPiAgCXBjaWVfY2FwYWJpbGl0eV9yZWFkX2R3b3JkKGRldiwgUENJX0VYUF9M TktDQVAsICZsbmtjYXApOwo+ID4gPiA+ICAJaWYgKGxua2NhcCkgewo+ID4gPiA+IC0JCWlmIChs bmtjYXAgJiBQQ0lfRVhQX0xOS0NBUF9TTFNfMTZfMEdCKQo+ID4gPiA+ICsJCWlmICgobG5rY2Fw ICYgUENJX0VYUF9MTktDQVBfU0xTKSA9PSBQQ0lfRVhQX0xOS0NBUF9TTFNfMTZfMEdCKQo+ID4g PiA+ICAJCQlyZXR1cm4gUENJRV9TUEVFRF8xNl8wR1Q7Cj4gPiA+ID4gLQkJZWxzZSBpZiAobG5r Y2FwICYgUENJX0VYUF9MTktDQVBfU0xTXzhfMEdCKQo+ID4gPiA+ICsJCWVsc2UgaWYgKChsbmtj YXAgJiBQQ0lfRVhQX0xOS0NBUF9TTFMpID09IFBDSV9FWFBfTE5LQ0FQX1NMU184XzBHQikKPiA+ ID4gPiAgCQkJcmV0dXJuIFBDSUVfU1BFRURfOF8wR1Q7Cj4gPiA+ID4gLQkJZWxzZSBpZiAobG5r Y2FwICYgUENJX0VYUF9MTktDQVBfU0xTXzVfMEdCKQo+ID4gPiA+ICsJCWVsc2UgaWYgKChsbmtj YXAgJiBQQ0lfRVhQX0xOS0NBUF9TTFMpID09UENJX0VYUF9MTktDQVBfU0xTXzVfMEdCKQo+ID4g PiA+ICAJCQlyZXR1cm4gUENJRV9TUEVFRF81XzBHVDsKPiA+ID4gPiAtCQllbHNlIGlmIChsbmtj YXAgJiBQQ0lfRVhQX0xOS0NBUF9TTFNfMl81R0IpCj4gPiA+ID4gKwkJZWxzZSBpZiAoKGxua2Nh cCAmIFBDSV9FWFBfTE5LQ0FQX1NMUykgPT0gUENJX0VYUF9MTktDQVBfU0xTXzJfNUdCKQo+ID4g PiA+ICAJCQlyZXR1cm4gUENJRV9TUEVFRF8yXzVHVDsKPiA+ID4gPiAgCX0KPiA+IAo+ID4gPiBX ZSBhbHNvIG5lZWQgc2ltaWxhciBmaXhlcyBpbiBwY2lfc2V0X2J1c19zcGVlZCgpLCBwY2llX3Nw ZWVkcygpCj4gPiA+IChoZmkxKSwgY29iYWx0X3BjaWVfc3RhdHVzX3Nob3coKSwgaGJhX2lvY3Rs X2NhbGxiYWNrKCksCj4gPiA+IHFsYTI0eHhfcGNpX2luZm9fc3RyKCksIGFuZCBtYXliZSBhIGNv dXBsZSBvdGhlciBwbGFjZXMuCj4gPiAKPiA+IERvZXMgYW55Ym9keSB3YW50IHRvIHZvbHVudGVl ciB0byBmaXggdGhlIHBsYWNlcyBhYm92ZSBhcyB3ZWxsPyAgSQo+ID4gZm91bmQgdGhlbSBieSBn cmVwcGluZyBmb3IgUENJX0VYUF9MTktDQVAsIGFuZCB0aGV5J3JlIGFsbCBicm9rZW4gaW4KPiA+ IHdheXMgc2ltaWxhciB0byBwY2llX2dldF9zcGVlZF9jYXAoKS4gIFBvc3NpYmx5IHNvbWUgb2Yg dGhlc2UgcGxhY2VzCj4gPiBjb3VsZCB1c2UgcGNpZV9nZXRfc3BlZWRfY2FwKCkgZGlyZWN0bHku Cj4gCj4gVGhleSBhcmUgbm90IGJyb2tlbiwgdGhleSBhcmUgbWFza2luZyB0aGUgdmFsdWUgd2l0 aCBQQ0lfRVhQX0xOS0NBUF9TTFMgLSAKPiB0aGF0IGlzIGNvcnJlY3QuCgpZb3UncmUgcmlnaHQs ICJicm9rZW4iIGlzIGFuIG92ZXJzdGF0ZW1lbnQgZXhjZXB0IGluIHRlcm1zIG9mCm1haW50YWlu YW5jZTogdGhleSBkbyBoYXBwZW4gdG8gd29yayBjb3JyZWN0bHksIGJ1dCB0aGVyZSdzIG5vIHJl YXNvbgp0byByZWltcGxlbWVudCB0aGUgc2FtZSBmdW5jdGlvbmFsaXR5IHNldmVyYWwgcGxhY2Vz IGluIHNsaWdodGx5CmRpZmZlcmVudCB3YXlzLgoKcGNpZV9nZXRfc3BlZWRfY2FwKCkgbG9va3Mg YXQgTE5LQ0FQMiwgdGhlbiBMTktDQVAuICBUaGUgb3RoZXIgcGxhY2VzCmxvb2sgb25seSBhdCBM TktDQVAuICBTaW5jZSB0aGVzZSBhcmUgYWxsIGZpbmRpbmcgb25seSB0aGUgbWF4IGxpbmsKc3Bl ZWQgKHdoaWNoIGNhbiBiZSBkZXRlcm1pbmVkIHVzaW5nIG9ubHkgTEtOQ0FQKSwgbm90IHRoZSBz ZXQgb2YKc3VwcG9ydGVkIHNwZWVkcyAod2hpY2ggcmVxdWlyZXMgYm90aCBMTktDQVAyIGFuZCBM TktDQVApLCB0aGV5IHNob3VsZAphbGwgZG8gaXQgdGhlIHNhbWUgd2F5LgoKPiBwY2lfc2V0X2J1 c19zcGVlZDoKPiAgICAgICAgICAgICAgICAgcGNpZV9jYXBhYmlsaXR5X3JlYWRfZHdvcmQoYnJp ZGdlLCBQQ0lfRVhQX0xOS0NBUCwgJmxpbmtjYXApOwo+ICAgICAgICAgICAgICAgICBidXMtPm1h eF9idXNfc3BlZWQgPSBwY2llX2xpbmtfc3BlZWRbbGlua2NhcCAmIFBDSV9FWFBfTE5LQ0FQX1NM U107Cj4gCj4gcGNpZV9zcGVlZHM6Cj4gCWlmICgobGlua2NhcCAmIFBDSV9FWFBfTE5LQ0FQX1NM UykgIT0gUENJX0VYUF9MTktDQVBfU0xTXzhfMEdCKQo+IAo+IGNvYmFsdF9wY2llX3N0YXR1c19z aG93Ogo+IAlqdXN0IHByaW50cyB0aGUgdmFsdWVzIHdpdGhvdXQgZG9pbmcgYW55dGhpbmcgd2l0 aCB0aGVtCj4gCj4gaGJhX2lvY3RsX2NhbGxiYWNrOgo+IAlnYWktPnBjaS5saW5rX3NwZWVkX21h eCA9ICh1OCkoY2FwcyAmIFBDSV9FWFBfTE5LQ0FQX1NMUyk7Cj4gCj4gCWdhaS0+cGNpLmxpbmtf d2lkdGhfbWF4ID0gKHU4KSgoY2FwcyAmIFBDSV9FWFBfTE5LQ0FQX01MVykgPj4gNCk7Cj4gCj4g cWxhMjR4eF9wY2lfaW5mb19zdHI6Cj4gCWxzcGVlZCA9IGxzdGF0ICYgUENJX0VYUF9MTktDQVBf U0xTOwo+IAlsd2lkdGggPSAobHN0YXQgJiBQQ0lfRVhQX0xOS0NBUF9NTFcpID4+IDQ7Cj4gCj4g TWlrdWxhcwpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwph bWQtZ2Z4IG1haWxpbmcgbGlzdAphbWQtZ2Z4QGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpodHRwczov L2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2FtZC1nZngK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.1 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS, USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0FB83C43441 for ; Tue, 27 Nov 2018 20:40:54 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id C5EF821104 for ; Tue, 27 Nov 2018 20:40:53 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="U5TFA9ZM" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C5EF821104 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-pci-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726166AbeK1Hj7 (ORCPT ); Wed, 28 Nov 2018 02:39:59 -0500 Received: from mail.kernel.org ([198.145.29.99]:36134 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726068AbeK1Hj7 (ORCPT ); Wed, 28 Nov 2018 02:39:59 -0500 Received: from localhost (unknown [69.71.4.100]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 845AD2086B; Tue, 27 Nov 2018 20:40:51 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1543351251; bh=jmeDNkNrjz9LGcqC7JU1JeCGQux+y8u3unPekQKJOOI=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=U5TFA9ZMdrbOHcLg4dqNjG1LlZE4cKTY0HAswk3VQtzw1dRhgNgZQbviuKKApI2i2 Xm2m2hYCLW0RYGxGkYjLXsFm4w7qG5sslR6XT/6fTAECvULW1BqZAEYO4DYpGhSUcn vQ7oVk+1aFgz9fxcamBOgWBqtPoDp653n7TfPULQ= Date: Tue, 27 Nov 2018 14:40:49 -0600 From: Bjorn Helgaas To: Mikulas Patocka Cc: linux-pci@vger.kernel.org, Tal Gilboa , Tariq Toukan , Alex Deucher , amd-gfx@lists.freedesktop.org Subject: Re: [PATCH] pci: fix incorrect value returned from pcie_get_speed_cap Message-ID: <20181127204049.GA150956@google.com> References: <20181120004704.GA191199@google.com> <20181126224051.GB212532@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Sender: linux-pci-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org On Tue, Nov 27, 2018 at 11:49:43AM -0500, Mikulas Patocka wrote: > On Mon, 26 Nov 2018, Bjorn Helgaas wrote: > > On Mon, Nov 19, 2018 at 06:47:04PM -0600, Bjorn Helgaas wrote: > > > On Tue, Oct 30, 2018 at 12:36:08PM -0400, Mikulas Patocka wrote: > > > > The macros PCI_EXP_LNKCAP_SLS_*GB are values, not bit masks. We must mask > > > > the register and compare it against them. > > > > > > > > This patch fixes errors "amdgpu: [powerplay] failed to send message 261 > > > > ret is 0" errors when PCIe-v3 card is plugged into PCIe-v1 slot, because > > > > the slot is being incorrectly reported as PCIe-v3 capable. > > > > > > > > Signed-off-by: Mikulas Patocka > > > > Fixes: 6cf57be0f78e ("PCI: Add pcie_get_speed_cap() to find max supported link speed") > > > > Cc: stable@vger.kernel.org # v4.17+ > > > > > > > > --- > > > > drivers/pci/pci.c | 8 ++++---- > > > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > > > > > Index: linux-4.19/drivers/pci/pci.c > > > > =================================================================== > > > > --- linux-4.19.orig/drivers/pci/pci.c 2018-10-30 16:58:58.000000000 +0100 > > > > +++ linux-4.19/drivers/pci/pci.c 2018-10-30 16:58:58.000000000 +0100 > > > > @@ -5492,13 +5492,13 @@ enum pci_bus_speed pcie_get_speed_cap(st > > > > > > > > pcie_capability_read_dword(dev, PCI_EXP_LNKCAP, &lnkcap); > > > > if (lnkcap) { > > > > - if (lnkcap & PCI_EXP_LNKCAP_SLS_16_0GB) > > > > + if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_16_0GB) > > > > return PCIE_SPEED_16_0GT; > > > > - else if (lnkcap & PCI_EXP_LNKCAP_SLS_8_0GB) > > > > + else if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_8_0GB) > > > > return PCIE_SPEED_8_0GT; > > > > - else if (lnkcap & PCI_EXP_LNKCAP_SLS_5_0GB) > > > > + else if ((lnkcap & PCI_EXP_LNKCAP_SLS) ==PCI_EXP_LNKCAP_SLS_5_0GB) > > > > return PCIE_SPEED_5_0GT; > > > > - else if (lnkcap & PCI_EXP_LNKCAP_SLS_2_5GB) > > > > + else if ((lnkcap & PCI_EXP_LNKCAP_SLS) == PCI_EXP_LNKCAP_SLS_2_5GB) > > > > return PCIE_SPEED_2_5GT; > > > > } > > > > > We also need similar fixes in pci_set_bus_speed(), pcie_speeds() > > > (hfi1), cobalt_pcie_status_show(), hba_ioctl_callback(), > > > qla24xx_pci_info_str(), and maybe a couple other places. > > > > Does anybody want to volunteer to fix the places above as well? I > > found them by grepping for PCI_EXP_LNKCAP, and they're all broken in > > ways similar to pcie_get_speed_cap(). Possibly some of these places > > could use pcie_get_speed_cap() directly. > > They are not broken, they are masking the value with PCI_EXP_LNKCAP_SLS - > that is correct. You're right, "broken" is an overstatement except in terms of maintainance: they do happen to work correctly, but there's no reason to reimplement the same functionality several places in slightly different ways. pcie_get_speed_cap() looks at LNKCAP2, then LNKCAP. The other places look only at LNKCAP. Since these are all finding only the max link speed (which can be determined using only LKNCAP), not the set of supported speeds (which requires both LNKCAP2 and LNKCAP), they should all do it the same way. > pci_set_bus_speed: > pcie_capability_read_dword(bridge, PCI_EXP_LNKCAP, &linkcap); > bus->max_bus_speed = pcie_link_speed[linkcap & PCI_EXP_LNKCAP_SLS]; > > pcie_speeds: > if ((linkcap & PCI_EXP_LNKCAP_SLS) != PCI_EXP_LNKCAP_SLS_8_0GB) > > cobalt_pcie_status_show: > just prints the values without doing anything with them > > hba_ioctl_callback: > gai->pci.link_speed_max = (u8)(caps & PCI_EXP_LNKCAP_SLS); > > gai->pci.link_width_max = (u8)((caps & PCI_EXP_LNKCAP_MLW) >> 4); > > qla24xx_pci_info_str: > lspeed = lstat & PCI_EXP_LNKCAP_SLS; > lwidth = (lstat & PCI_EXP_LNKCAP_MLW) >> 4; > > Mikulas