linux-kernel - Re: [PATCH v9 2/9] PCI: Using PCI configuration space header type instead of class type to assign resource

Open Source and information security mailing list archives

Message-ID: <1539825912.11281.8.camel@mhfsdcap03>
Date:   Thu, 18 Oct 2018 09:25:12 +0800
From:   Honghui Zhang <honghui.zhang@...iatek.com>
To:     Bjorn Helgaas <helgaas@...nel.org>
CC:     Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        <youlin.pei@...iatek.com>, <devicetree@...r.kernel.org>,
        <ulf.hansson@...aro.org>, <ryder.lee@...iatek.com>,
        <marc.zyngier@....com>, <linux-pci@...r.kernel.org>,
        <jianjun.wang@...iatek.com>, <linux-kernel@...r.kernel.org>,
        <yt.shen@...iatek.com>, <matthias.bgg@...il.com>,
        <linux-mediatek@...ts.infradead.org>, <bhelgaas@...gle.com>,
        <yingjoe.chen@...iatek.com>, <eddie.huang@...iatek.com>,
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v9 2/9] PCI: Using PCI configuration space header type
 instead of class type to assign resource

On Wed, 2018-10-17 at 08:22 -0500, Bjorn Helgaas wrote:
> On Tue, Oct 16, 2018 at 03:53:55PM +0100, Lorenzo Pieralisi wrote:
> > On Tue, Oct 16, 2018 at 06:44:43PM +0800, honghui.zhang@...iatek.com wrote:
> > > From: Honghui Zhang <honghui.zhang@...iatek.com>
> > > 
> > > The PCI configuration space header type defines the layout of the rest
> > > of the header (PCI r3.0 sec 6.1, PCIe r4.0 sec 7.5.1.1.9) while the
> > > resource assignment is based on the configuration space layout instead
> > > of its class type. Using configuration space header type instead of
> > > class type for the resource assignment.
> > > 
> > > Suggested-by: Bjorn Helgaas <bhelgaas@...gle.com>
> > > Signed-off-by: Honghui Zhang <honghui.zhang@...iatek.com>
> > > ---
> > >  drivers/pci/pci.c       |  3 +--
> > >  drivers/pci/probe.c     |  3 ---
> > >  drivers/pci/setup-bus.c | 20 ++++++++++----------
> > >  3 files changed, 11 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index 29ff961..7d379ca 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -5908,8 +5908,7 @@ void pci_reassigndev_resource_alignment(struct pci_dev *dev)
> > >  	 * to enable the kernel to reassign new resource
> > >  	 * window later on.
> > >  	 */
> > > -	if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE &&
> > > -	    (dev->class >> 8) == PCI_CLASS_BRIDGE_PCI) {
> > > +	if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
> > >  		for (i = PCI_BRIDGE_RESOURCES; i < PCI_NUM_RESOURCES; i++) {
> > >  			r = &dev->resource[i];
> > >  			if (!(r->flags & IORESOURCE_MEM))
> > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > index ec78400..29a35c1 100644
> > > --- a/drivers/pci/probe.c
> > > +++ b/drivers/pci/probe.c
> > > @@ -1695,9 +1695,6 @@ int pci_setup_device(struct pci_dev *dev)
> > >  		break;
> > >  
> > >  	case PCI_HEADER_TYPE_BRIDGE:		    /* bridge header */
> > > -		if (class != PCI_CLASS_BRIDGE_PCI)
> > > -			goto bad;
> > > -
> > >  		/*
> > >  		 * The PCI-to-PCI bridge spec requires that subtractive
> > >  		 * decoding (i.e. transparent) bridge must have programming
> > > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> > > index 79b1824..69f90f4 100644
> > > --- a/drivers/pci/setup-bus.c
> > > +++ b/drivers/pci/setup-bus.c
> > > @@ -182,7 +182,7 @@ static void __dev_sort_resources(struct pci_dev *dev,
> > >  	u16 class = dev->class >> 8;
> > >  
> > >  	/* Don't touch classless devices or host bridges or ioapics.  */
> > > -	if (class == PCI_CLASS_NOT_DEFINED || class == PCI_CLASS_BRIDGE_HOST)
> > > +	if (class == PCI_CLASS_NOT_DEFINED)
> > 
> > I think this check has been there since the first initial git commit,
> > whether that's _really_ needed or not in the current kernel it is very
> > hard to say.
> > 
> > I am not that sure it is safe to remove it, especially given that we are at
> > -rc8 and close to a release, it would be good if this patch could sit in
> > next to give it some exposure to testing before merging it upstream.
> 

I'm not sure why the first version has take care of
PCI_CLASS_BRIDGE_HOST separately, I have no idea whether there's some
host bridge device that has their resource fixed.

I agree that this patch should be hold for the moment since I have it
tested only in Mediatek's platform.

Thanks

> Yes, you're right; I think I think this is a little too risky at this
> point.  I'll pull this patch out and queue it up for the next cycle
> (v4.21).

Thanks, I will follow up this patch.

> 
> For v4.20, I think you should resurrect the class code patch [1].  That
> should be enough to make things work in v4.20, even without this hdr_type
> patch.  It will also improve the lspci output, because I think it uses the
> class code to look up the generic description, e.g., in this output:
> 
>   00:1c.0 PCI bridge: Intel Corporation Sunrise Point-LP PCI Express Root Port (rev f1)
> 
> I think the "PCI bridge" part is based on the class code.
> 
> Bjorn
> 
> [1] https://lore.kernel.org/linux-pci/1539590940-13355-3-git-send-email-honghui.zhang@mediatek.com
> 
> > >  		return;
> > >  
> > >  	/* Don't touch ioapic devices already enabled by firmware */
> > > @@ -1221,12 +1221,12 @@ void __pci_bus_size_bridges(struct pci_bus *bus, struct list_head *realloc_head)
> > >  		if (!b)
> > >  			continue;
> > >  
> > > -		switch (dev->class >> 8) {
> > > -		case PCI_CLASS_BRIDGE_CARDBUS:
> > > +		switch (dev->hdr_type) {
> > > +		case PCI_HEADER_TYPE_CARDBUS:
> > >  			pci_bus_size_cardbus(b, realloc_head);
> > >  			break;
> > >  
> > > -		case PCI_CLASS_BRIDGE_PCI:
> > > +		case PCI_HEADER_TYPE_BRIDGE:
> > >  		default:
> > >  			__pci_bus_size_bridges(b, realloc_head);
> > >  			break;
> > > @@ -1237,12 +1237,12 @@ void __pci_bus_size_bridges(struct pci_bus *bus, struct list_head *realloc_head)
> > >  	if (pci_is_root_bus(bus))
> > >  		return;
> > >  
> > > -	switch (bus->self->class >> 8) {
> > > -	case PCI_CLASS_BRIDGE_CARDBUS:
> > > +	switch (bus->self->hdr_type) {
> > > +	case PCI_HEADER_TYPE_CARDBUS:
> > >  		/* don't size cardbuses yet. */
> > >  		break;
> > >  
> > > -	case PCI_CLASS_BRIDGE_PCI:
> > > +	case PCI_HEADER_TYPE_BRIDGE:
> > >  		pci_bridge_check_ranges(bus);
> > >  		if (bus->self->is_hotplug_bridge) {
> > >  			additional_io_size  = pci_hotplug_io_size;
> > > @@ -1391,13 +1391,13 @@ void __pci_bus_assign_resources(const struct pci_bus *bus,
> > >  
> > >  		__pci_bus_assign_resources(b, realloc_head, fail_head);
> > >  
> > > -		switch (dev->class >> 8) {
> > > -		case PCI_CLASS_BRIDGE_PCI:
> > > +		switch (dev->hdr_type) {
> > > +		case PCI_HEADER_TYPE_BRIDGE:
> > >  			if (!pci_is_enabled(dev))
> > >  				pci_setup_bridge(b);
> > >  			break;
> > >  
> > > -		case PCI_CLASS_BRIDGE_CARDBUS:
> > > +		case PCI_HEADER_TYPE_CARDBUS:
> > >  			pci_setup_cardbus(b);
> > >  			break;
> > >  
> > > -- 
> > > 2.6.4
> > > 
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@...ts.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives