headers
Andrew Morton | 1 Jul 2008 01:57

Re: [patch 5/5] x86 PCI: use dev_printk when possible

On Fri, 13 Jun 2008 10:52:14 -0600
Bjorn Helgaas <bjorn.helgaas <at> hp.com> wrote:

> I converted DBG() to dev_dbg().  This DBG() is from arch/x86/pci/pci.h
> and requires source-code modification to enable, so dev_dbg() seems
> roughly equivalent.
> 
> The printks in arch/x86/pci/irq.c were a little hairy, with lines
> printed in several pieces.  I straightened it out a little, but
> another set of eyes would be good.
> 
> Signed-off-by: Bjorn Helgaas <bjorn.helgaas <at> hp.com>
> 
> ---
>  arch/x86/pci/fixup.c |    3 +
>  arch/x86/pci/i386.c  |   26 ++++++--------
>  arch/x86/pci/irq.c   |   91 ++++++++++++++++++++++-----------------------------
>  arch/x86/pci/numa.c  |    5 +-

Got a bunch of rejects in arch/x86/pci/irq.c due to pending whitespace
fixes.  I fixed a couple of them but then I got bored, so the
conversion in there is only partial.

Please prefer to raise patches against linux-next if poss, especially
during late -rc's.
Permalink | Reply | 0 comments |
headers
Benjamin Li | 1 Jul 2008 02:59
Favicon

Re: [PATCH] PCI: Set VPD size conservatively

Hi Matt and Ben,

What happens when the VPD information is corrupted?  I have an NIC which
functions but has a badly formed VPD (When iterating through the VPD
tags, you will never reach an end tag)  And because this patch will scan
through the entire VPD address space, it will generate an out-of-range
access.  Would you have any suggestions on how we could correct this?

Thanks again.

-Ben

On Mon, 2008-06-30 at 17:52 +0100, Ben Hutchings wrote:
> The PCI 2.2 VPD address range is 32 KB but few PCI devices actually
> map this much storage.  It is not clear whether hardware or software
> is responsible for avoiding out-of-range access, and some devices are
> known to lock up if we read beyond the valid size.  Therefore we
> follow the VPD resource chain to find the valid size.
> 
> Signed-off-by: Ben Hutchings <bhutchings <at> solarflare.com>
> ---
>  drivers/pci/access.c    |   55 ++++++++++++++++++++++++++++++++++++++--------
>  drivers/pci/pci-sysfs.c |    2 +-
>  drivers/pci/pci.h       |    4 ++-
>  3 files changed, 49 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> index ec8f700..8df2db2 100644
> --- a/drivers/pci/access.c
> +++ b/drivers/pci/access.c
(Continue reading)

Permalink | Reply | 2 comments |
headers
Matthew Wilcox | 1 Jul 2008 03:45

Re: [PATCH] PCI: Set VPD size conservatively

On Mon, Jun 30, 2008 at 05:59:55PM -0700, Benjamin Li wrote:
> What happens when the VPD information is corrupted?  I have an NIC which
> functions but has a badly formed VPD (When iterating through the VPD
> tags, you will never reach an end tag)  And because this patch will scan
> through the entire VPD address space, it will generate an out-of-range
> access.  Would you have any suggestions on how we could correct this?

You raise a good point.  I think the call to pci_vpd_set_real_size()
needs to be moved later after a quirk has had time to run.  Then a quirk
can adjust the size for that board.

I think this combines the best of both patches.  Each stage successively
limits the range of data.  First, the generic code sets the maximum to
32k (as per spec).  Then the quirks can lower the size.  Then the generic
code gets to run again and try to determine if the actual size is smaller.

If we do it this way, a device with broken VPD (and I've seen a few
myself) won't bring the system down, as long as it has a quirk.

--

-- 
Intel are signing my paycheques ... these opinions are still mine
"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."
Permalink | Reply | 1 comment |
headers
Ben Hutchings | 1 Jul 2008 18:18

[PATCH] PCI: Restrict VPD read permission to root

Some PCI devices will lock up if we attempt to read from VPD addresses
beyond some device-dependent limit.  Until we can identify these
devices and adjust the file size accordingly, only let root read VPD
through sysfs.

Signed-off-by: Ben Hutchings <bhutchings <at> solarflare.com>
---
Jesse,

Please either apply this or revert 94e6108803469a37ee1e3c92dafdd1d59298602f
for 2.6.26.

Ben.

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 6f3c744..1f855f0 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
 <at>  <at>  -738,7 +738,7  <at>  <at>  int __must_check pci_create_sysfs_dev_files (struct pci_dev *pdev)
 			pdev->vpd->attr = attr;
 			attr->size = pdev->vpd->ops->get_size(pdev);
 			attr->attr.name = "vpd";
-			attr->attr.mode = S_IRUGO | S_IWUSR;
+			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);

--

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
(Continue reading)

Permalink | Reply | 14 comments |
headers
Jesse Barnes | 1 Jul 2008 19:05
Favicon

Re: [PATCH] PCI: Restrict VPD read permission to root

On Tuesday, July 01, 2008 9:18 am Ben Hutchings wrote:
> Some PCI devices will lock up if we attempt to read from VPD addresses
> beyond some device-dependent limit.  Until we can identify these
> devices and adjust the file size accordingly, only let root read VPD
> through sysfs.
>
> Signed-off-by: Ben Hutchings <bhutchings <at> solarflare.com>

Thanks Ben, applied and pushed to Linus, hopefully it's not too late.

Jesse
Permalink | Reply | 13 comments |
headers
Jesse Barnes | 2 Jul 2008 03:57
Favicon

Re: [patch 5/5] x86 PCI: use dev_printk when possible

On Monday, June 30, 2008 4:57 pm Andrew Morton wrote:
> On Fri, 13 Jun 2008 10:52:14 -0600
>
> Bjorn Helgaas <bjorn.helgaas <at> hp.com> wrote:
> > I converted DBG() to dev_dbg().  This DBG() is from arch/x86/pci/pci.h
> > and requires source-code modification to enable, so dev_dbg() seems
> > roughly equivalent.
> >
> > The printks in arch/x86/pci/irq.c were a little hairy, with lines
> > printed in several pieces.  I straightened it out a little, but
> > another set of eyes would be good.
> >
> > Signed-off-by: Bjorn Helgaas <bjorn.helgaas <at> hp.com>
> >
> > ---
> >  arch/x86/pci/fixup.c |    3 +
> >  arch/x86/pci/i386.c  |   26 ++++++--------
> >  arch/x86/pci/irq.c   |   91
> > ++++++++++++++++++++++----------------------------- arch/x86/pci/numa.c 
> > |    5 +-
>
> Got a bunch of rejects in arch/x86/pci/irq.c due to pending whitespace
> fixes.  I fixed a couple of them but then I got bored, so the
> conversion in there is only partial.
>
> Please prefer to raise patches against linux-next if poss, especially
> during late -rc's.

Yeah, the x86 tree(s) move too fast for me to keep up with.  The change seems 
worthwhile though, maybe Ingo or one of the x86 team could just search &
(Continue reading)

Permalink | Reply | 0 comments |
headers
Jesse Barnes | 2 Jul 2008 04:04
Favicon

Re: [PATCH] PCI: Set VPD size conservatively

On Monday, June 30, 2008 6:45 pm Matthew Wilcox wrote:
> On Mon, Jun 30, 2008 at 05:59:55PM -0700, Benjamin Li wrote:
> > What happens when the VPD information is corrupted?  I have an NIC which
> > functions but has a badly formed VPD (When iterating through the VPD
> > tags, you will never reach an end tag)  And because this patch will scan
> > through the entire VPD address space, it will generate an out-of-range
> > access.  Would you have any suggestions on how we could correct this?
>
> You raise a good point.  I think the call to pci_vpd_set_real_size()
> needs to be moved later after a quirk has had time to run.  Then a quirk
> can adjust the size for that board.
>
> I think this combines the best of both patches.  Each stage successively
> limits the range of data.  First, the generic code sets the maximum to
> 32k (as per spec).  Then the quirks can lower the size.  Then the generic
> code gets to run again and try to determine if the actual size is smaller.
>
> If we do it this way, a device with broken VPD (and I've seen a few
> myself) won't bring the system down, as long as it has a quirk.

Sounds like a reasonable thing to do, though it might still be a good idea to 
keep the VPD data root only in sysfs by default.  Perms can always be 
adjusted later by distro scripts etc, right?

Jesse
Permalink | Reply | 0 comments |
headers
Benjamin Li | 2 Jul 2008 04:37
Favicon

Re: [PATCH] PCI: Restrict VPD read permission to root

Hi Jesse, Matt, and Ben,

The following is a patch which is needed to prevent Broadcom 5706, 5708 and 5709 rev A. nics from hanging with
the current PCI VPD code.  Switching the permissions to be read by root still does not solve the case when any
app with root privileges tries to read the VPD data from these nics.  It will still hang the device.  Please
let us know what you think.  Thanks again.

-Ben
Permalink | Reply | 12 comments |
headers
Benjamin Li | 2 Jul 2008 04:37
Favicon

[PATCH] [PATCH] PCI: Limit VPD read/write lengths for Broadcom 5706, 5708, 5709 rev. A nics

From: root <root <at> localhost.localdomain>

For Broadcom 5706, 5708, 5709 rev. A nics, any read beyond the
VPD end tag will hang the device.  This problem was initially
observed when a vpd entry was created in sysfs
('/sys/bus/pci/devices/≤id>/vpd').   A read to this sysfs entry
will dump 32k of data.  Reading a full 32k will cause an access
beyond the VPD end tag causing the device to hang.  Once the device
is hung, the bnx2 driver will not be able to reset the device.
We believe that it is legal to read beyond the end tag and
therefore the solution is to limit the read/write length.

A majority of this patch is from Matthew Wilcox who gave code for
reworking the PCI vpd size information.  A PCI quirk added for the
Broadcom NIC's to limit the read/write's.

Signed-off-by: Benjamin Li <benli <at> broadcom.com>
---
 drivers/pci/access.c    |   14 ++++----------
 drivers/pci/pci-sysfs.c |    2 +-
 drivers/pci/pci.h       |    2 +-
 drivers/pci/quirks.c    |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 52 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index ec8f700..39bb96b 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
 <at>  <at>  -178,8 +178,7  <at>  <at>  static int pci_vpd_pci22_read(struct pci_dev *dev, int pos, int size,
 	int ret;
(Continue reading)

Permalink | Reply | 1 comment |
headers
Matthew Wilcox | 2 Jul 2008 04:45

Re: [PATCH] [PATCH] PCI: Limit VPD read/write lengths for Broadcom 5706, 5708, 5709 rev. A nics

On Tue, Jul 01, 2008 at 07:37:11PM -0700, Benjamin Li wrote:
> +static void __devinit quirk_brcm_570x_limit_vpd(struct pci_dev *dev)
> +{
> +	u8 rev;
> +
> +	pci_read_config_byte(dev, PCI_REVISION_ID, &rev);

Do we have pdev->revision filled in at this point?

> +	/*  Only disable the VPD capability for 5706, 5708, and 5709 rev. A */
> +	if ((dev->device == PCI_DEVICE_ID_NX2_5706) ||
> +	    (dev->device == PCI_DEVICE_ID_NX2_5708) ||
> +	    ((dev->device == PCI_DEVICE_ID_NX2_5709) &&
> +	     (rev & 0xf0) == 0x0)) {
> +		if(dev->vpd)
> +			dev->vpd->len = 0x80;
> +	}
> +}

--

-- 
Intel are signing my paycheques ... these opinions are still mine
"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."
Permalink | Reply | 0 comments |

Gmane