pciback_field_is_dup needs a fix

pciback_field_is_dup needs a fix

[Jambunathan K]

Shouldn't pciback_field_is_dup(dev, field->offset) be scoped to include 
the base_offset as well?

Currently with xen-3.0.4 the issue is this:

Adding of  PCI_PM_CTRL (at offset 4)  to the 'config_fields list' gets 
skipped because it ends up being a duplicate of PCI_COMMAND (at offset 4 
as well).  As a result when a PCI device behind a PCI frontend does a 
power up using pci_enable_device() the following message gets flashed on 
the console.

pciback 0000:0b:00.3: Driver tried to write to a read-only configuration 
space field at offset 0x84, size 2. This may be harmless, but if you 
have problems with your device:
1) see permissive attribute in sysfs
2) report problems to the xen-devel mailing list along with details of 
your device obtained from lspci.

Thanks,
Jambunathan K.

Re: pciback_field_is_dup needs a fix

[Jambunathan K]

Here is quick log from frontend debug showing this happening:

pciback 0000:0b:00.3: added config field at offset 0x04
pciback 0000:0b:00.3: added config field at offset 0x3c
pciback 0000:0b:00.3: added config field at offset 0x3d
pciback 0000:0b:00.3: added config field at offset 0x0c
pciback 0000:0b:00.3: added config field at offset 0x0d
pciback 0000:0b:00.3: added config field at offset 0x0f
pciback 0000:0b:00.3: added config field at offset 0x10
pciback 0000:0b:00.3: added config field at offset 0x14
pciback 0000:0b:00.3: added config field at offset 0x18
pciback 0000:0b:00.3: added config field at offset 0x1c
pciback 0000:0b:00.3: added config field at offset 0x20
pciback 0000:0b:00.3: added config field at offset 0x24
pciback 0000:0b:00.3: added config field at offset 0x30

pciback 0000:0b:00.3: Found capability 0x1 at 0x80

pciback 0000:0b:00.3: added config field at offset 0x80
pciback 0000:0b:00.3: added config field at offset 0x82
                                                                   
<------ 0x80 + 4 missing here
pciback 0000:0b:00.3: added config field at offset 0x86
pciback 0000:0b:00.3: added config field at offset 0x87

Thanks,
Jambunathan K.


Jambunathan K wrote:
> Shouldn't pciback_field_is_dup(dev, field->offset) be scoped to 
> include the base_offset as well?
>
> Currently with xen-3.0.4 the issue is this:
>
> Adding of  PCI_PM_CTRL (at offset 4)  to the 'config_fields list' gets 
> skipped because it ends up being a duplicate of PCI_COMMAND (at offset 
> 4 as well).  As a result when a PCI device behind a PCI frontend does 
> a power up using pci_enable_device() the following message gets 
> flashed on the console.
>
> pciback 0000:0b:00.3: Driver tried to write to a read-only 
> configuration space field at offset 0x84, size 2. This may be 
> harmless, but if you have problems with your device:
> 1) see permissive attribute in sysfs
> 2) report problems to the xen-devel mailing list along with details of 
> your device obtained from lspci.
>
> Thanks,
> Jambunathan K.
>
>

Re: pciback_field_is_dup needs a fix

[Chris]

Jambunathan K wrote:
> Shouldn't pciback_field_is_dup(dev, field->offset) be scoped to include
> the base_offset as well?

I think you're right.  Let me see what needs to be changed to accomplish
the intended behavior.

Thanks,
Chris

Re: pciback_field_is_dup needs a fix

[Keir Fraser]

On 8/3/07 19:39, "Jambunathan K" <jambunathan@netxen.com> wrote:

> Shouldn't pciback_field_is_dup(dev, field->offset) be scoped to include
> the base_offset as well?

Absolutely. Thanks for tracking it down!

 -- Keir

[PATCH] [pciback] Re: pciback_field_is_dup needs a fix

[Chris]

[-- Attachment #1: Type: text/plain, Size: 483 bytes --]

This is a follow-up to pciback changes made earlier today that add
several fixes:

- remove superfluous call to pciback_field_is_dup()
- fix a variable type mismatch in pciback_field_is_dup()
- make readability improvements by using the OFFSET macro
- revises quirk data output via the sysfs quirks node so that displayed
offset information includes base_offset.

Thanks to Jambunathan K. for giving such specific bug diagnosis.

Signed-off-by: Chris Bookholt <hap10@tycho.ncsc.mil>

[-- Attachment #2: pciback.patch --]
[-- Type: text/plain, Size: 3525 bytes --]

diff -r 6ff2a1286484 linux-2.6-xen-sparse/drivers/xen/pciback/conf_space.c
--- a/linux-2.6-xen-sparse/drivers/xen/pciback/conf_space.c	Fri Mar 09 16:04:19 2007 +0000
+++ b/linux-2.6-xen-sparse/drivers/xen/pciback/conf_space.c	Fri Mar 09 16:07:38 2007 -0500
@@ -349,16 +349,12 @@ void pciback_config_free_dev(struct pci_
 
 int pciback_config_add_field_offset(struct pci_dev *dev,
 				    struct config_field *field,
-				    unsigned int offset)
+				    unsigned int base_offset)
 {
 	int err = 0;
 	struct pciback_dev_data *dev_data = pci_get_drvdata(dev);
 	struct config_field_entry *cfg_entry;
 	void *tmp;
-
-	/* silently ignore duplicate fields */
-	if (pciback_field_is_dup(dev, field->offset + offset))
-		goto out;
 
 	cfg_entry = kmalloc(sizeof(*cfg_entry), GFP_KERNEL);
 	if (!cfg_entry) {
@@ -368,7 +364,12 @@ int pciback_config_add_field_offset(stru
 
 	cfg_entry->data = NULL;
 	cfg_entry->field = field;
-	cfg_entry->base_offset = offset;
+	cfg_entry->base_offset = base_offset;
+
+	/* silently ignore duplicate fields */
+	err = pciback_field_is_dup(dev,OFFSET(cfg_entry));
+	if (err)
+		goto out;
 
 	if (field->init) {
 		tmp = field->init(dev, OFFSET(cfg_entry));
diff -r 6ff2a1286484 linux-2.6-xen-sparse/drivers/xen/pciback/conf_space_quirks.c
--- a/linux-2.6-xen-sparse/drivers/xen/pciback/conf_space_quirks.c	Fri Mar 09 16:04:19 2007 +0000
+++ b/linux-2.6-xen-sparse/drivers/xen/pciback/conf_space_quirks.c	Fri Mar 09 16:07:38 2007 -0500
@@ -32,16 +32,14 @@ static inline void register_quirk(struct
 	list_add_tail(&quirk->quirks_list, &pciback_quirks);
 }
 
-int pciback_field_is_dup(struct pci_dev *dev, int reg)
+int pciback_field_is_dup(struct pci_dev *dev, unsigned int reg)
 {
 	int ret = 0;
 	struct pciback_dev_data *dev_data = pci_get_drvdata(dev);
-	struct config_field *field;
 	struct config_field_entry *cfg_entry;
 
 	list_for_each_entry(cfg_entry, &dev_data->config_fields, list) {
-		field = cfg_entry->field;
-		if (field->offset == reg) {
+		if ( OFFSET(cfg_entry) == reg) {
 			ret = 1;
 			break;
 		}
diff -r 6ff2a1286484 linux-2.6-xen-sparse/drivers/xen/pciback/conf_space_quirks.h
--- a/linux-2.6-xen-sparse/drivers/xen/pciback/conf_space_quirks.h	Fri Mar 09 16:04:19 2007 +0000
+++ b/linux-2.6-xen-sparse/drivers/xen/pciback/conf_space_quirks.h	Fri Mar 09 16:07:38 2007 -0500
@@ -30,6 +30,6 @@ void pciback_config_field_free(struct co
 
 int pciback_config_quirk_release(struct pci_dev *dev);
 
-int pciback_field_is_dup(struct pci_dev *dev, int reg);
+int pciback_field_is_dup(struct pci_dev *dev, unsigned int reg);
 
 #endif
diff -r 6ff2a1286484 linux-2.6-xen-sparse/drivers/xen/pciback/pci_stub.c
--- a/linux-2.6-xen-sparse/drivers/xen/pciback/pci_stub.c	Fri Mar 09 16:04:19 2007 +0000
+++ b/linux-2.6-xen-sparse/drivers/xen/pciback/pci_stub.c	Fri Mar 09 16:07:38 2007 -0500
@@ -589,10 +589,6 @@ static int pcistub_reg_add(int domain, i
 	}
 	dev = psdev->dev;
 
-	/* check for duplicate field */
-	if (pciback_field_is_dup(dev, reg))
-		goto out;
-
 	field = kzalloc(sizeof(*field), GFP_ATOMIC);
 	if (!field) {
 		err = -ENOMEM;
@@ -728,10 +724,10 @@ static ssize_t pcistub_quirk_show(struct
 			if (count >= PAGE_SIZE)
 				goto out;
 
-			count += scnprintf(buf + count, PAGE_SIZE -
-					   count, "\t\t%08x:%01x:%08x\n",
-					   field->offset, field->size,
-					   field->mask);
+			count += scnprintf(buf + count, PAGE_SIZE - count,
+					   "\t\t%08x:%01x:%08x\n",
+					   cfg_entry->base_offset + field->offset, 
+					   field->size, field->mask);
 		}
 	}
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel