Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755533AbdLVA4q (ORCPT ); Thu, 21 Dec 2017 19:56:46 -0500 Received: from Mailgw01.mediatek.com ([1.203.163.78]:34093 "EHLO mailgw01.mediatek.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1752385AbdLVA4o (ORCPT ); Thu, 21 Dec 2017 19:56:44 -0500 X-UUID: ae2e0dfe4f1347908dcf84c84c746438-20171222 Message-ID: <1513904194.25872.23.camel@mhfsdcap03> Subject: Re: [PATCH v2 2/2] PCI: mediatek: Fixup class type for MT7622 From: Honghui Zhang To: Bjorn Helgaas CC: Yong Wu , , , , , , , , , , , , , , , , , Date: Fri, 22 Dec 2017 08:56:34 +0800 In-Reply-To: <20171222002712.GE30595@bhelgaas-glaptop.roam.corp.google.com> References: <1513822277-18329-1-git-send-email-honghui.zhang@mediatek.com> <1513822277-18329-3-git-send-email-honghui.zhang@mediatek.com> <1513838476.23174.3.camel@mhfsdcap03> <1513839672.25872.13.camel@mhfsdcap03> <20171222002712.GE30595@bhelgaas-glaptop.roam.corp.google.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.2.3-0ubuntu6 Content-Transfer-Encoding: 7bit MIME-Version: 1.0 X-MTK: N Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3959 Lines: 97 On Thu, 2017-12-21 at 18:27 -0600, Bjorn Helgaas wrote: > On Thu, Dec 21, 2017 at 03:01:12PM +0800, Honghui Zhang wrote: > > On Thu, 2017-12-21 at 14:41 +0800, Yong Wu wrote: > > > On Thu, 2017-12-21 at 10:11 +0800, honghui.zhang@mediatek.com wrote: > > > > From: Honghui Zhang > > > > > > > > The host bridge of MT7622 has hardware code the class code to an > > > > arbitrary, meaningless value, fix that. > > > > > > > > Signed-off-by: Honghui Zhang > > > > --- > > > > drivers/pci/host/pcie-mediatek.c | 12 ++++++++++++ > > > > 1 file changed, 12 insertions(+) > > > > > > > > diff --git a/drivers/pci/host/pcie-mediatek.c b/drivers/pci/host/pcie-mediatek.c > > > > index 3248771..ae8d367 100644 > > > > --- a/drivers/pci/host/pcie-mediatek.c > > > > +++ b/drivers/pci/host/pcie-mediatek.c > > > > @@ -1174,3 +1174,15 @@ static struct platform_driver mtk_pcie_driver = { > > > > }, > > > > }; > > > > builtin_platform_driver(mtk_pcie_driver); > > > > + > > > > +/* The host bridge of MT7622 advertises the wrong device class. */ > > > > +static void mtk_fixup_class(struct pci_dev *dev) > > > > +{ > > > > + dev->class = PCI_CLASS_BRIDGE_PCI << 8; > > > > +} > > > > + > > > > +/* > > > > + * The HW default value of vendor id and device id for mt7622 are 0x0e8d, > > > > + * 0x3258, which are arbitrary, meaningless values. > > They may be arbitrary but they are certainly not meaningless. > > > > What's the right vendor id and device id? is it possible to fix them > > > too? > > > > Vendor ID is managed by PCI-SIG, you may get the assigned vendor ID > > from: > > https://pci-ids.ucw.cz/read/PC?restrict= > > It's true that Vendor IDs are managed by the PCI-SIG. The link above > is not managed by the PCI-SIG and is not the official list of assigned > Vendor IDs. > > > The vendor ID for Mediatek Corp. should be 14c3. > > Device ID is something like vendor-defined. > > Those values are in the configuration space and are read-only defined by > > spec, it's been stored at the pci_dev, we may change the vendor and > > device values in pci_dev, but I don't think it's necessary to change > > that. > > BTW, Does anyone really cares about the vendor ID and device ID except > > the device's driver? > > Yes. This is a fundamental misunderstanding of how Vendor and Device > IDs work. The *driver* really doesn't care about the IDs. The PCI > *core* cares about them because it uses the IDs to select the > appropriate driver to bind to the device. > Thanks, I was wrong about this, I had not seen that the Vendor IDs may be assigned to another company in the future. I should try another way to fix this. > > > > +DECLARE_PCI_FIXUP_EARLY(0x0e8d, 0x3258, mtk_fixup_class); > > This is absolutely not OK. You cannot take over Vendor ID 0x0e8d > unless it has been assigned to you by the PCI-SIG. > > To my knowledge, 0x0e8d has not been assigned to any company yet, but > the PCI-SIG could assign it to some new company X tomorrow. Company X > may then build a device with Device ID 0x3258. Now we cannot tell the > difference between the Company X device that is correctly designed and > this Mediatek device that is broken. > > This quirk would improperly overwrite dev->class for the Company X > device, and we would not be able to bind the correct driver to either > device. > I will try another way to fix this, thanks very much for your explain. > I am amazed that somebody could build a device that claims to be a PCI > device and get the Vendor ID wrong. That should be literally the > first thing the hardware designer does. If you have hardware in the > field that uses the wrong Vendor ID, it is definitely not > PCI-compliant. There's an internal control register that control the Vendor ID and device ID values, our designer leave the default value un-touched. I will set these values in that way for the next version of fix. Thanks. > > Bjorn