Re: [PATCH v3 05/11] PCI: beef up pci_do_scan_bus()

[Kenji Kaneshige]

Alex Chiang wrote:
> Hello Kenji-san,
>
> * Kenji Kaneshige <kaneshige.kenji@xxxxxxxxxxxxxx>:
> > Alex Chiang wrote:
> > > I hadn't gotten around to verifying/fixing existing callers of
> > > pci_do_scan_bus yet. I was focusing on the core first.
> > >
> > > There aren't too many callers, but unfortunately, I don't have
> > > any hardware that actually uses the existing drivers.
> > >
> > > I seem to recall that your machines support shpchp. Would you
> > > mind testing this patch and telling me if your machine still
> > > behaves properly?
> > I have machines that support shpchp. But unfortunately I don't
> > have any adapter card that contains bridge, which is needed to
> > test your change.
>
> You're right.
>
> The more I think about it though, the more I think that even
> without the below patch to clean up the callers of
> pci_do_scan_bus, we should be ok, because:
>
> 	- all the old code (which I removed below) existed
> 	  because the old PCI core would refuse to scan PCI buses
> 	  that had already been discovered
>
> 	- that meant that it would never descend past a known
> 	  bridge to try and find new child bridges
>
> 	- that meant that hotplug drivers had to manually
> 	  discover new bridges and add them, essentially
> 	  duplicating functionality in pci_scan_bridge
>
> This patch series allows the PCI core to scan existing bridges
> and descend down into the children every time, looking for new
> bridges and devices, so all the code in shpchp, cpcihp, and other
> callers of pci_do_scan_bus shouldn't be necessary anymore.
>
> Also, if we do add new bridges once manually in shpchp, and then
> call the new pci_do_scan_bus again, we will _not_ add devices
> twice because the core should check each bridge and device for
> struct pci_dev.is_added.
>
> So anyway, I think that cleaning up the callers of
> pci_do_scan_bus is a good idea, but multiple calls to the
> interface definitely should not result in problems. If they do,
> then that's a bug in my patch series.

I'm sorry, but I didn't have enough time to try your patch on
my environment. So I'm still just looking at the code.

I looked at shpchp_configure_device() from the view point of
bridge hot-add. I think it is broken regardless of your change
because it calls pci_bus_add_devices() (through pci_do_scan_bus)
before assigning resources. So I think it must be changed
regardless of your change. But it's a little difficult for me
because I don't have any test environment as I mentioned before.

But I'm still worrying about your change against pci_do_scan_bus().
Without your change, pci_do_scan_bus() scans child buses and add
devices without assigning resources. I guess that it means existing
callers of pci_do_scan_bus() have some mechanism to assign resource
by theirselves and they don't expect pci_do_scan_bus() assigns
resources.

By the way, I have one question about rescan. Please suppose that
we enable the bridge(B) and its children using rescan interface
in the picture below.

                  |
-------------------------------------- parent bus
        |                  |
    bridge(A)          bridge(B)
    (working)        (Not working)
        |                  |
  -------------      -------------
   |         |        |         |
  dev       dev      dev       dev
(working) (working)   (Not working)

In this case, your rescan mechanism calls pci_do_scan_bus() for
parent bus, and pci_do_scan_bus() calls pci_bus_assign_resources()
for parent bus. My question is, does pci_bus_assign_resources() do
nothing against bridge(A) that is currently working? I guess 
pci_bus_assign_resources() would update some registers of bridge(A)
and it would breaks currently working devices.

Thanks,
Kenji Kaneshige



> > > I looked at shpchp_configure_device and I think that simply
> > > scanning the device's parent bus should work.
> > Ok, I'll try it.
>
> I set up a git tree to make it easier to test:
>
> 	git://git.kernel.org/pub/scm/linux/kernel/git/achiang/pci-hotplug.git
>
> The 'test-20090313' branch contains all the latest fixes to
> enable this patch series. It does not contain the patch below, so
> you will have to apply it by hand.
>
> Thanks for testing!
>
> /ac
>
> > > Thanks.
> > >
> > > /ac
> > >
> > > diff --git a/drivers/pci/hotplug/shpchp_pci.c b/drivers/pci/hotplug/shpchp_pci.c
> > > index aa315e5..7e8457b 100644
> > > --- a/drivers/pci/hotplug/shpchp_pci.c
> > > +++ b/drivers/pci/hotplug/shpchp_pci.c
> > > @@ -110,11 +110,7 @@ int __ref shpchp_configure_device(struct slot *p_slot)
> > >  		return -EINVAL;
> > >  	}
> > >  
> > > -	num = pci_scan_slot(parent, PCI_DEVFN(p_slot->device, 0));
> > > -	if (num == 0) {
> > > -		ctrl_err(ctrl, "No new device found\n");
> > > -		return -ENODEV;
> > > -	}
> > > +	pci_do_scan_bus(parent);
> > >  
> > >  	for (fn = 0; fn < 8; fn++) {
> > >  		dev = pci_get_slot(parent, PCI_DEVFN(p_slot->device, fn));
> > > @@ -126,40 +122,10 @@ int __ref shpchp_configure_device(struct slot *p_slot)
> > >  			pci_dev_put(dev);
> > >  			continue;
> > >  		}
> > > -		if ((dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) ||
> > > -				(dev->hdr_type == PCI_HEADER_TYPE_CARDBUS)) {
> > > -			/* Find an unused bus number for the new bridge */
> > > -			struct pci_bus *child;
> > > -			unsigned char busnr, start = parent->secondary;
> > > -			unsigned char end = parent->subordinate;
> > > -			for (busnr = start; busnr <= end; busnr++) {
> > > -				if (!pci_find_bus(pci_domain_nr(parent),
> > > -							busnr))
> > > -					break;
> > > -			}
> > > -			if (busnr > end) {
> > > -				ctrl_err(ctrl,
> > > -					 "No free bus for hot-added bridge\n");
> > > -				pci_dev_put(dev);
> > > -				continue;
> > > -			}
> > > -			child = pci_add_new_bus(parent, dev, busnr);
> > > -			if (!child) {
> > > -				ctrl_err(ctrl, "Cannot add new bus for %s\n",
> > > -					 pci_name(dev));
> > > -				pci_dev_put(dev);
> > > -				continue;
> > > -			}
> > > -			child->subordinate = pci_do_scan_bus(child);
> > > -			pci_bus_size_bridges(child);
> > > -		}
> > >  		program_fw_provided_values(dev);
> > >  		pci_dev_put(dev);
> > >  	}
> > >  
> > > -	pci_bus_assign_resources(parent);
> > > -	pci_bus_add_devices(parent);
> > > -	pci_enable_bridges(parent);
> > >  	return 0;
> > >  }
> > >  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/