lkml.org 
[lkml]   [2008]   [Oct]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH 2/6 v3] PCI: add new general functions
Date
On Saturday, September 27, 2008 1:27 am Zhao, Yu wrote:
> Centralize capability related functions into several new functions and put
> PCI resource definitions into an enum.

> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index f99160d..f2feebc 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c

The sysfs changes look fine, they should be submitted separately.

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 259eaff..400d3b3 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -356,25 +356,10 @@ pci_find_parent_resource(const struct pci_dev *dev,
> struct resource *res) static void
> pci_restore_bars(struct pci_dev *dev)
> {
> - int i, numres;
> -
> - switch (dev->hdr_type) {
> - case PCI_HEADER_TYPE_NORMAL:
> - numres = 6;
> - break;
> - case PCI_HEADER_TYPE_BRIDGE:
> - numres = 2;
> - break;
> - case PCI_HEADER_TYPE_CARDBUS:
> - numres = 1;
> - break;
> - default:
> - /* Should never get here, but just in case... */
> - return;
> - }
> + int i;
>
> - for (i = 0; i < numres; i ++)
> - pci_update_resource(dev, &dev->resource[i], i);
> + for (i = 0; i < PCI_BRIDGE_RESOURCES; i++)
> + pci_update_resource(dev, i);
> }

This confused me for a minute until I saw that the new pci_update_resource
ignores invalid BAR numbers. Not sure if that's clearer than the current
code...

> +/**
> + * pci_resource_bar - get position of the BAR associated with a resource
> + * @dev: the PCI device
> + * @resno: the resource number
> + * @type: the BAR type to be filled in
> + *
> + * Returns BAR position in config space, or 0 if the BAR is invalid.
> + */
> +int pci_resource_bar(struct pci_dev *dev, int resno, enum pci_bar_type
> *type) +{
> + if (resno < PCI_ROM_RESOURCE) {
> + *type = pci_bar_unknown;
> + return PCI_BASE_ADDRESS_0 + 4 * resno;
> + } else if (resno == PCI_ROM_RESOURCE) {
> + *type = pci_bar_rom;
> + return dev->rom_base_reg;
> + }
> +
> + dev_err(&dev->dev, "BAR: invalid resource #%d\n", resno);
> + return 0;
> +}

It looks like this will spew an error even under normal circumstances since
pci_restore_bars gets called at resume time, right? You could make this into
a debug message or just get rid of it. Also now that I look at this, I don't
think it'll provide equivalent functionality to the old restore_bars code,
won't a cardbus bridge end up getting pci_update_resource called on invalid
BARs?

> +static void pci_init_capabilities(struct pci_dev *dev)
> +{
> + /* MSI/MSI-X list */
> + pci_msi_init_pci_dev(dev);
> +
> + /* Power Management */
> + pci_pm_init(dev);
> +
> + /* Vital Product Data */
> + pci_vpd_pci22_init(dev);
> +}
> +

These capabilities changes look good, care to separate them out?

Let's see if we can whittle down this patchset by extracting and applying all
the various cleanups; that should make the core bits easier to review.

Thanks,
--
Jesse Barnes, Intel Open Source Technology Center


\
 
 \ /
  Last update: 2008-10-02 18:25    [W:2.245 / U:0.280 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site