linux-kernel - RE: [PATCH 1/6 v3] PCI: export some functions and macros

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 10:23:18 +0800
From:	"Zhao, Yu" <yu.zhao@...el.com>
To:	Matthew Wilcox <matthew@....cx>
CC:	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	Jesse Barnes <jbarnes@...tuousgeek.org>,
	Randy Dunlap <randy.dunlap@...cle.com>,
	Grant Grundler <grundler@...isc-linux.org>,
	Alex Chiang <achiang@...com>,
	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 1/6 v3] PCI: export some functions and macros

On Saturday, September 27, 2008 8:59 PM, Matthew Wilcox wrote:
>On Sat, Sep 27, 2008 at 04:27:44PM +0800, Zhao, Yu wrote:
>> Export some functions and move some macros from c file to header file.
>
>That's absolutely not everything this patch does.  You need to split
>this into smaller pieces and explain what you're doing and why for each
>of them.

Sure, I'll split it.

>
>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index d807cd7..596efa6 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -1,3 +1,9 @@
>> +#ifndef DRIVERS_PCI_H
>> +#define DRIVERS_PCI_H
>
>Do we really need header guards on this file?

Maybe it's not necessary, but we use guards in almost all private headers. So I added this to make this file look not so different.

>
>> -/*
>> - * If the type is not unknown, we assume that the lowest bit is 'enable'.
>> - * Returns 1 if the BAR was 64-bit and 0 if it was 32-bit.
>> +/**
>> + * pci_read_base - read a PCI BAR
>> + * @dev: the PCI device
>> + * @type: type of the BAR
>> + * @res: resource buffer to be filled in
>> + * @pos: BAR position in the config space
>> + *
>> + * Returns 1 if the BAR is 64-bit, or 0 if 32-bit.
>>   */
>> -static int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
>> +int pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
>
>The original intent here was to have a pci_read_base() that called
>__pci_read_base() and then did things like translate physical BAR
>addresses to virtual ones.  That patch is in the archives somewhere.
>We ended up not including that patch because my user found out he could
>get the address he wanted from elsewhere.  I'm not sure we want to
>remove the __ at this point.

I've studied your patch that adds wrapper of __pci_read_base. If you are going to push it again, I'm ok with keeping the name unchanged.

>
>The eventual goal is to fix up the BARs at this point, but there's
>several architectures that will break if we do this now.  It's on my
>long-term todo list.
>
>>                         struct resource *res, unsigned int pos)
>>  {
>>         u32 l, sz, mask;
>>
>> -       mask = type ? ~PCI_ROM_ADDRESS_ENABLE : ~0;
>> +       mask = (type == pci_bar_rom) ? ~PCI_ROM_ADDRESS_ENABLE : ~0;
>
>What's going on here?  Why are you adding pci_bar_rom?  For the rom we
>use pci_bar_mem32.  Take a look at, for example, the MCHBAR in the 965
>spec (313053.pdf).  That's something that uses the pci_bar_mem64 type
>and definitely wants to use the PCI_ROM_ADDRESS_ENABLE mask.

Thanks for pointing this out. I wonder how the PC_ROM_ADDRESS_ENABLE mask is set for those non-standard BARs like MCHBAR after the probe stage -- I don't think pci_update_resource will take care of them. So how about adding BAR type checking again pci_bar_mem32 in pci_update_resource so we can set the bit there?

>
>>
>> -       if (type == pci_bar_unknown) {
>> +       if (type == pci_bar_rom) {
>> +               res->flags |= (l & IORESOURCE_ROM_ENABLE);
>> +               l &= PCI_ROM_ADDRESS_MASK;
>> +               mask = (u32)PCI_ROM_ADDRESS_MASK;
>> +       } else {
>
>This looks wrong too.
>
>>         if (rom) {
>> @@ -344,7 +340,7 @@ static void pci_read_bases(struct pci_dev *dev, unsigned
>int howmany, int rom)
>>                 res->flags = IORESOURCE_MEM | IORESOURCE_PREFETCH |
>>                                 IORESOURCE_READONLY | IORESOURCE_CACHEABLE
>|
>>                                 IORESOURCE_SIZEALIGN;
>> -               __pci_read_base(dev, pci_bar_mem32, res, rom);
>> +               pci_read_base(dev, pci_bar_mem32, res, rom);
>>         }
>
>And you don't even change the type here ... have you tested this code on
>a system which has a ROM?

Oh, you caught it.

>
>>
>> -       for(i=0; i<3; i++)
>> -               child->resource[i] =
>&dev->resource[PCI_BRIDGE_RESOURCES+i];
>> -
>
>Er, this is rather important.  Why can you just delete it?

I guess pci_alloc_child_bus has done this so we don't have to do it again.

>
>--
>Matthew Wilcox                          Intel Open Source Technology Centre
>"Bill, look, we understand that you're interested in selling us this
>operating system, but compare it to ours.  We can't possibly take such
>a retrograde step."
>--
>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/