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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
