Re: [PATCH 01/24] PCI: Add iobusn_resource

[Jesse Barnes]

On Mon, 6 Feb 2012 10:55:14 -0800
Yinghai Lu <yinghai@xxxxxxxxxx> wrote:

> On Mon, Feb 6, 2012 at 10:48 AM, Bjorn Helgaas <bhelgaas@xxxxxxxxxx> wrote:
> >> +struct resource iobusn_resource = {
> >> +    .name  = "PCI busn",
> >> + Â Â Â .start Â= 0,
> >> +    .end  Â= 0xffffff,
> >> + Â Â Â .flags Â= IORESOURCE_BUS,
> >> +};
> >
> > I'm not sure this should be global. Âiomem_resource and
> > ioport_resource *are* really global, because they refer to processor
> > address space that is the same for everybody. ÂBut PCI bus numbers are
> > specific to PCI. ÂSome machines don't have PCI at all, and there are
> > different bus architectures to which this doesn't apply.
> 
> that does not hurt them.

Yes but it's superfluous and confusing if you're porting to a new arch
or looking at changes in generic code that may affect you.

> > The 0-0xffffff range is misleading because it includes both the domain
> > and the bus number, and it's meaningless to allocate ranges that cross
> > domain boundaries. ÂFor example, [bus 0x0000f0-0x000120] includes bus
> > numbers from domain 0000 and domain 0001, which doesn't make any sense
> > because a bus can only be in one domain.
> 
> allocation code will make sure it will be cross the boundary for domain.

But that means everyone reading it will do a double take, have to dig
into the implementation, and only then say "ah yeah ok it looks
correct" rather than it being obvious from the fact that the resource
is tracked on a per-domain basis.

> > I think it would make more sense to keep this bus number resource in a
> > per-host bridge structure. ÂThen we wouldn't need to include the
> > domain number at all because the host bridge determines the domain.
> 
> not sure. insert the all busn_res of all peer root buses into one
> global iobusn_resource
> looks more simple.

In what sense?  Simpler in the sense of your current implementation,
but not simpler at all to someone just reading the code...

-- 
Jesse Barnes, Intel Open Source Technology Center

Attachment: signature.asc
Description: PGP signature