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; i3; 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 [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 1/6 v3] PCI: export some functions and macros
On Saturday, September 27, 2008 1:27 am Zhao, Yu wrote:
Export some functions and move some macros from c file to header file.
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 9c71858..f99160d 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -696,7 +696,7 @@ static struct bin_attribute pci_config_attr = {
.name = config,
.mode = S_IRUGO | S_IWUSR,
},
- .size = 256,
+ .size = PCI_CFG_SPACE_SIZE,
.read = pci_read_config,
.write = pci_write_config,
I just pushed Yanmin's cleanups here, can you separate out the rest of your
config space size changes and push them separately?
extern int pci_uevent(struct device *dev, struct kobj_uevent_env *env);
@@ -144,3 +150,17 @@ struct pci_slot_attribute {
};
#define to_pci_slot_attr(s) container_of(s, struct pci_slot_attribute,
attr)
+enum pci_bar_type {
+ pci_bar_unknown,/* Standard PCI BAR probe */
+ pci_bar_io, /* An io port BAR */
+ pci_bar_mem32, /* A 32-bit memory BAR */
+ pci_bar_mem64, /* A 64-bit memory BAR */
+ pci_bar_rom,/* A ROM BAR */
+};
+
+extern int pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
+ struct resource *res, unsigned int reg);
+extern struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
+ struct pci_dev *bridge, int busnr);
+
+#endif /* DRIVERS_PCI_H */
See Matthew's comments here; the pci_read_base changes should be part of a
separate patch too.
a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 3abbfad..6c78cf8 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -299,7 +299,7 @@ static void pbus_size_io(struct pci_bus *bus)
if (r-parent || !(r-flags IORESOURCE_IO))
continue;
- r_size = r-end - r-start + 1;
+ r_size = resource_size(r);
if (r_size 0x400)
/* Might be re-aligned for ISA */
@@ -350,7 +350,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned
long mask, unsigned long
if (r-parent || (r-flags mask) != type)
continue;
- r_size = r-end - r-start + 1;
+ r_size = resource_size(r);
/* For bridges size != alignment */
align = resource_alignment(r);
order = __ffs(align) - 20;
diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index 1a5fc83..56e4042 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -133,7 +133,7 @@ int pci_assign_resource(struct pci_dev *dev, int resno)
resource_size_t size, min, align;
int ret;
- size = res-end - res-start + 1;
+ size = resource_size(res);
min = (res-flags IORESOURCE_IO) ? PCIBIOS_MIN_IO :
PCIBIOS_MIN_MEM;
align = resource_alignment(res);
These resource_size changes seem like good cleanups by themselves, can you
separate them out into a separate patch?
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 98dc624..cc78be6 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -456,8 +456,8 @@ struct pci_driver {
/**
* PCI_VDEVICE - macro used to describe a specific pci device in short
form - * @vend: the vendor name
- * @dev: the 16 bit PCI Device ID
+ * @vendor: the vendor name
+ * @device: the 16 bit PCI Device ID
*
* This macro is used to create a struct pci_device_id that matches a
* specific PCI device. The subvendor, and subdevice fields will be set
Another good standalone cleanup, please submit separately.
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
Re: [PATCH 1/6 v3] PCI: export some functions and macros
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.
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?
-/*
- * 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.
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.
- 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?
- for(i=0; i3; i++)
- child-resource[i] = dev-resource[PCI_BRIDGE_RESOURCES+i];
-
Er, this is rather important. Why can you just delete it?
--
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 kvm in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
