Re: Regression with commit f9cde5f in 2.6.30-gitX

[Thomas Gleixner]

On Wed, 24 Jun 2009, Jaswinder Singh Rajput wrote:
> On Wed, 2009-06-24 at 16:51 +0200, Thomas Gleixner wrote:
> > On Wed, 24 Jun 2009, Jaswinder Singh Rajput wrote:
> > > Reported-by: Larry Finger <Larry.Finger@xxxxxxxxxxxx>
> > > Signed-off-by: Jaswinder Singh Rajput <jaswinderrajput@xxxxxxxxx>
> > > ---
> > >  arch/x86/pci/acpi.c |   16 ++++++++++------
> > >  1 files changed, 10 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/arch/x86/pci/acpi.c b/arch/x86/pci/acpi.c
> > > index 16c3fda..0bc015f 100644
> > > --- a/arch/x86/pci/acpi.c
> > > +++ b/arch/x86/pci/acpi.c
> > > @@ -69,6 +69,9 @@ setup_resource(struct acpi_resource *acpi_res, void *data)
> > >  	struct resource *root;
> > >  	int max_root_bus_resources = PCI_BUS_NUM_RESOURCES;
> > >  
> > > +	if (info->res_num >= PCI_BUS_NUM_RESOURCES)
> > > +		return AE_OK;
> > > +
> 
> We need to test this otherwise we will get another oops.

Right, because you would store beyond the end of the array.
 
> > >  	status = resource_to_addr(acpi_res, &addr);
> > >  	if (!ACPI_SUCCESS(status))
> > >  		return AE_OK;
> > > @@ -94,17 +97,18 @@ setup_resource(struct acpi_resource *acpi_res, void *data)
> > >  	if (bus_has_transparent_bridge(info->bus))
> > >  		max_root_bus_resources -= 3;
> > >  	if (info->res_num >= max_root_bus_resources) {
> > > -		printk(KERN_WARNING "PCI: Failed to allocate 0x%lx-0x%lx "
> > > -			"from %s for %s due to _CRS returning more than "
> > > -			"%d resource descriptors\n", (unsigned long) res->start,
> > > -			(unsigned long) res->end, root->name, info->name,
> > > -			max_root_bus_resources);
> > > +		pr_warning("PCI: Failed to allocate 0x%lx-0x%lx "
> > > +			   "from %s for %s due to _CRS returning more than "
> > > +			   "%d resource descriptors\n",
> > > +			   (unsigned long)res->start, (unsigned long)res->end,
> > > +			   root->name, info->name, max_root_bus_resources);
> > 
> > Can you please avoid mixing cleanup patches with bug fixes ? I almost
> > did not see the line below.
> > 
> > > +		info->bus->resource[info->res_num] = res;
> > 
> 
> We need to set the resource even it is transparent bridge, otherwise pci
> will fail and system will not able to boot.

The fact that the system does boot with that patch is not making the
patch more correct. It fixes the boot problem at hand, but it does not
fix the root cause of the problem.

You stick the resource into the resource array, but it does not call
insert_resource(). Also you print a warning while the system seems to
be happy to use the device.

This is just inconsistent and the patch simply papers over the root
cause of the problem.

The reservation of the last 3 resources in the transparent case is
more likely the real reason as it reduces the number of available
slots in bus->resource[].

Can you please increase PCI_BUS_NUM_RESOURCES by at least 3 and test
with the one liner patch which removes the increment applied.

Also please provide the output of lspci on 2.6.30 (which does not have
f9cde5f applied).

Thanks,

	tglx

--
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/