RE: [PATCH 2/6 v3] PCI: add new general functions
On Friday, October 03, 2008 12:22 AM, Jesse Barnes wrote:
>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.
Will do.
>
>> 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...
When device has its own specific BARs, we have to add more 'case' statement in
this function and may mass this function up. Simply ignoring the unused
resources in pci_update_resource looks concise to me. Anyway, I can use keep
the old structure if you feel the change brought more confusion than concision.
>
>> +/**
>> + * 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
It won't print the message unless there is something wrong with the system.
pci_update_resource is only called when the resource # is less than
PCI_BRIDGE_RESOURCES and it will ignore unused resource. So when
pci_resource_bar gets involved, all resource # shouldn't big than
PCI_ROM_RESOURCE (PCI_BRIDGE_RESOURCE = PCI_ROM_RESOURCE + 1)
>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?
The cardbus uses 1 BAR resource plus 4 (max) bridge resources. The
pci_update_resource is only called when restoring the BAR resource. It won't be
called to update the bridge resources for the reason I mentioned above.
>
>> +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?
Will do.
>
>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 for the careful reviewing and the comments. I'll send the updated
version soon according to all the comments I've got.
>
>Thanks,
>--
>Jesse Barnes, Intel Open Source Technology Center
>--
>To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>the body of a message to [EMAIL PROTECTED]
>More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/6 v3] PCI: add new general functions
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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/6 v3] PCI: add new general functions
Centralize capability related functions into several new functions and put PCI
resource definitions into an enum.
Cc: Jesse Barnes <[EMAIL PROTECTED]>
Cc: Randy Dunlap <[EMAIL PROTECTED]>
Cc: Grant Grundler <[EMAIL PROTECTED]>
Cc: Alex Chiang <[EMAIL PROTECTED]>
Cc: Matthew Wilcox <[EMAIL PROTECTED]>
Cc: Roland Dreier <[EMAIL PROTECTED]>
Cc: Greg KH <[EMAIL PROTECTED]>
Signed-off-by: Yu Zhao <[EMAIL PROTECTED]>
---
drivers/pci/pci-sysfs.c | 119 +++
drivers/pci/pci.c | 68 ---
drivers/pci/pci.h |3 +
drivers/pci/probe.c | 29 ---
drivers/pci/proc.c |7 ++-
drivers/pci/setup-bus.c |4 +-
drivers/pci/setup-res.c | 27 +--
include/linux/pci.h | 39 ++--
8 files changed, 186 insertions(+), 110 deletions(-)
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
@@ -100,11 +100,11 @@ resource_show(struct device * dev, struct
device_attribute *attr, char * buf)
struct pci_dev * pci_dev = to_pci_dev(dev);
char * str = buf;
int i;
- int max = 7;
+ int max;
resource_size_t start, end;
- if (pci_dev->subordinate)
- max = DEVICE_COUNT_RESOURCE;
+ max = pci_dev->subordinate ? DEVICE_COUNT_RESOURCE :
+ PCI_BRIDGE_RESOURCES;
for (i = 0; i < max; i++) {
struct resource *res = &pci_dev->resource[i];
@@ -716,10 +716,40 @@ int __attribute__ ((weak))
pcibios_add_platform_entries(struct pci_dev *dev)
return 0;
}
+static int pci_create_capabilities_sysfs(struct pci_dev *dev)
+{
+ int retval;
+ struct bin_attribute *attr;
+
+ /* If the device has VPD, try to expose it in sysfs. */
+ if (dev->vpd) {
+ attr = kzalloc(sizeof(*attr), GFP_ATOMIC);
+ if (!attr)
+ return -ENOMEM;
+
+ attr->size = dev->vpd->len;
+ attr->attr.name = "vpd";
+ attr->attr.mode = S_IRUSR | S_IWUSR;
+ attr->read = pci_read_vpd;
+ attr->write = pci_write_vpd;
+ retval = sysfs_create_bin_file(&dev->dev.kobj, attr);
+ if (retval) {
+ kfree(dev->vpd->attr);
+ return retval;
+ }
+ dev->vpd->attr = attr;
+ }
+
+ /* Active State Power Management */
+ pcie_aspm_create_sysfs_dev_files(dev);
+
+ return 0;
+}
+
int __must_check pci_create_sysfs_dev_files (struct pci_dev *pdev)
{
- struct bin_attribute *attr = NULL;
int retval;
+ struct bin_attribute *attr;
if (!sysfs_initialized)
return -EACCES;
@@ -731,69 +761,50 @@ int __must_check pci_create_sysfs_dev_files (struct
pci_dev *pdev)
if (retval)
goto err;
- /* If the device has VPD, try to expose it in sysfs. */
- if (pdev->vpd) {
- attr = kzalloc(sizeof(*attr), GFP_ATOMIC);
- if (attr) {
- pdev->vpd->attr = attr;
- attr->size = pdev->vpd->len;
- attr->attr.name = "vpd";
- attr->attr.mode = S_IRUSR | S_IWUSR;
- attr->read = pci_read_vpd;
- attr->write = pci_write_vpd;
- retval = sysfs_create_bin_file(&pdev->dev.kobj, attr);
- if (retval)
- goto err_vpd;
- } else {
- retval = -ENOMEM;
- goto err_config_file;
- }
- }
-
retval = pci_create_resource_files(pdev);
if (retval)
- goto err_vpd_file;
+ goto err_config_file;
/* If the device has a ROM, try to expose it in sysfs. */
if (pci_resource_len(pdev, PCI_ROM_RESOURCE) ||
(pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_SHADOW)) {
attr = kzalloc(sizeof(*attr), GFP_ATOMIC);
- if (attr) {
- pdev->rom_attr = attr;
- attr->size = pci_resource_len(pdev, PCI_ROM_RESOURCE);
- attr->attr.name = "rom";
- attr->attr.mode = S_IRUSR;
- attr->read = pci_read_rom;
- attr->write = pci_write_rom;
- retval = sysfs_create_bin_file(&pdev->dev.kobj, attr);
- if (retval)
- goto err_rom;
- } else {
+ if (!attr) {
retval = -ENOMEM;
goto err_resource_files;
}
+ attr->size = pci_resource_len(pdev, PCI_ROM_RESOURCE);
+ attr->attr.name = "rom";
