Re: [Pcihpd-discuss] [patch] pci hotplug: add common acpi functions to core

[Kristen Accardi <kristen.c.accardi@intel.com>]

On Wed, 2006-03-01 at 11:42 +0900, Kenji Kaneshige wrote:
> Hi Kristen,
> 
> Here is one more comment.
> 
> > +void acpi_get_hp_params_from_firmware(struct pci_dev *dev,
> > +		struct hotplug_params *hpp)
> > +{
> > +	acpi_status status = AE_NOT_FOUND;
> > +	struct pci_dev *pdev = dev;
> > +
> > +	/*
> > +	 * _HPP settings apply to all child buses, until another _HPP is
> > +	 * encountered. If we don't find an _HPP for the input pci dev,
> > +	 * look for it in the parent device scope since that would apply to
> > +	 * this pci dev. If we don't find any _HPP, use hardcoded defaults
> > +	 */
> > +	while (pdev && (ACPI_FAILURE(status))) {
> > +		acpi_handle handle = DEVICE_ACPI_HANDLE(&(pdev->dev));
> > +		if (!handle)
> > +			break;
> > +		status = acpi_run_hpp(handle, hpp);
> > +		if (!(pdev->bus->parent))
> > +			break;
> > +		/* Check if a parent object supports _HPP */
> > +		pdev = pdev->bus->parent->self;
> > +	}
> > +}
> > +EXPORT_SYMBOL_GPL(acpi_get_hp_params_from_firmware);
> 
> I think the acpi_get_hp_params_from_firmware() function assumes that
> users set default hpp parameters into *hpp before calling this function.
> I think it is very hard for new users of the function to notice it, so
> I think this assumption should be removed.
> 
> Thanks,
> Kenji Kaneshige
> 

Are you suggesting that we have the defaults set within this function?
I would like to change acpiphp to use the same functions to get hpp
values eventually (in a different patch), but I notice that acpiphp sets
the default cache line size to 8, while shpchp sets the default cache
line size to 16.  So it seems like it would be better to allow drivers
to set the default themselves, unless one of these drivers is doing the
wrong thing and using the wrong default value.