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

Kristen Accardi
2006-03-02T23:32:54Z

On Thu, 2006-03-02 at 12:18 +0900, Kenji Kaneshige wrote:
> Kristen Accardi wrote:
> > 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.
> >
>
> The other options are:
>
> (a) Leave the code as it is, and put the comments to indicate users
> how to use this function. That is, users have to set defaults
> before calling this function, otherwise the parameters returned
> from the function would be undefined.
>
> (b) Change the return value to let users know whether _HPP parameters
> were successfully parsed.
>
> I'd prefer (b).
> I'm attaching the sample patch for (b), though I've not tested it at all.
> This patch is against 2.6.16-rc5-mm1 with the following patches applied
>
> - shpchp: cleanup bus speed handling
> - acpiphp: Scan slots under the nested P2P bridge
> - Your set of patches
>
> Thanks,
> Kenji Kaneshige
>

Kenji,
Thanks for the feedback, I will implement (b) as part of my next patch
which also incorporates all the other feedback I've gotten - coming
hopefully tomorrow or so.

Kristen