linux-kernel - RE: [PATCH 2/6 v3] PCI: add new general functions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:	Wed, 8 Oct 2008 11:25:00 +0800
From:	"Zhao, Yu" <yu.zhao@...el.com>
To:	Jesse Barnes <jbarnes@...tuousgeek.org>
CC:	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	Randy Dunlap <randy.dunlap@...cle.com>,
	Grant Grundler <grundler@...isc-linux.org>,
	Alex Chiang <achiang@...com>, Matthew Wilcox <matthew@....cx>,
	Roland Dreier <rdreier@...co.com>, Greg KH <greg@...ah.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"kvm@...r.kernel.org" <kvm@...r.kernel.org>,
	"virtualization@...ts.linux-foundation.org" 
	<virtualization@...ts.linux-foundation.org>
Subject: 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 majordomo@...r.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/