| 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
| ||
|
Date: Mon, 15 Jun 2009 09:47:38 +0800
From: "Zhang, Yanmin" <yanmin_zhang@...ux.intel.com>
To: Andrew Patterson <andrew.patterson@...com>
Cc: linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
Jesse Barnes <jbarnes@...tuousgeek.org>
Subject: Re: [PATCH V4: 3/3] pci: Provide Multiple Error Received support
on AER
On Fri, 2009-06-12 at 22:16 +0000, Andrew Patterson wrote:
> On Fri, 2009-06-12 at 11:08 +0800, Zhang, Yanmin wrote:
> > When a root port receive the same errors more than once before kernel
> > process them, the Multiple Error Messages Received flags are set by
> > hardware. Because root port could only save one kind of correctable
> > error source id and another uncorrectable error source id at the same
> > time, so the second message sender id is lost if the 2 messages are
> > sent from 2 different devices. Below patch searches all devices under
> > the root port when multiple messages are received.
> >
> > Signed-off-by: Zhang Yanmin <yanmin.zhang@...ux.intel.com>
> >
> > ---
> >
> > diff -Nraup linux-2.6_next_aernoid/drivers/pci/pcie/aer/aerdrv_core.c linux-2.6_next_aermultierror/drivers/pci/pcie/aer/aerdrv_core.c
> > --- linux-2.6_next_aernoid/drivers/pci/pcie/aer/aerdrv_core.c 2009-06-12 05:39:24.000000000 +0800
> > +++ linux-2.6_next_aermultierror/drivers/pci/pcie/aer/aerdrv_core.c 2009-06-12 05:45:15.000000000 +0800
> > @@ -145,13 +145,22 @@ static void set_downstream_devices_error
> > pci_walk_bus(dev->subordinate, set_device_error_reporting, &enable);
> > }
> >
> > +static int add_error_device(struct aer_err_info *e_info, struct pci_dev *dev)
> > +{
> > + if (e_info->error_dev_num < AER_MAX_MULTI_ERR_DEVICES) {
> > + e_info->dev[e_info->error_dev_num ++] = dev;
>
> checkpatch reports:
> ERROR: space prohibited before that '++' (ctx:WxB)
> #46: FILE: drivers/pci/pcie/aer/aerdrv_core.c:151:
> + e_info->dev[e_info->error_dev_num ++] = dev;
Thanks. I will change it and use checkpatch to check it.
>
> Personally I would prefer:
> e_info->dev[e_info->error_dev_num] = dev;
> e_info->error_dev_num++;
>
> > + return 1;
> > + } else
> > + return 0;
> > +}
> > +
>
>
> This function is now doing more than just comparing device ID's.
> Perhaps you could rename it or put call add_error_device after
> compare_device_id in find_device_iter?
I will move the call add_error_device to find_device_iter.
> > static int compare_device_id(struct pci_dev *dev, struct aer_err_info *e_info)
> > {
> > if (e_info->id == ((dev->bus->number << 8) | dev->devfn)) {
> > /*
> > * Device ID match
> > */
> > - e_info->dev = dev;
> > + add_error_device(e_info, dev);
> > return 1;
> > }
> >
> > @@ -166,20 +175,38 @@ static int find_device_iter(struct pci_d
> > u32 status;
> > u32 mask;
> > u16 reg16;
> > + int result;
> > struct aer_err_info *e_info = (struct aer_err_info *)data;
> >
> > /*
> > * When bus id is equal to 0, it might be a bad id
> > * reported by root port.
> > */
> > - if (!nosourceid && (PCI_BUS(e_info->id) != 0))
> > - return compare_device_id(dev, e_info);
> > + if (!nosourceid && (PCI_BUS(e_info->id) != 0)) {
> > + result = compare_device_id(dev, e_info);
> > + /*
> > + * If there is no multiple error, we stop
> > + * or continue based on the id comparing.
> > + */
> > + if (!(e_info->flags & AER_MULTI_ERROR_VALID_FLAG))
> > + return result;
> > +
> > + /*
> > + * If there are multiple errors and id does match,
> > + * We need continue to search other devices under
> > + * the root port. Return 0 means that.
> > + */
> > + if (result)
> > + return 0;
> > + }
> >
> > /*
> > - * Next is to check when bus id is equal to 0 or
> > - * nosourceid==y. Some ports might lose the bus
> > - * id of error source id. We check AER status
> > - * registers to find the initial reporter.
> > + * When either
> > + * 1) nosourceid==y;
> > + * 2) bus id is equal to 0. Some ports might lose the bus
> > + * id of error source id;
> > + * 3) There are multiple errors and prior id comparing fails;
> > + * We check AER status registers to find the initial reporter.
> > */
> > if (atomic_read(&dev->enable_cnt) == 0)
> > return 0;
> > @@ -208,8 +235,8 @@ static int find_device_iter(struct pci_d
> > pos + PCI_ERR_COR_MASK,
> > &mask);
> > if (status & ERR_CORRECTABLE_ERROR_MASK & ~mask) {
> > - e_info->dev = dev;
> > - return 1;
> > + add_error_device(e_info, dev);
> > + goto added;
> > }
> > } else {
> > pci_read_config_dword(dev,
> > @@ -219,12 +246,18 @@ static int find_device_iter(struct pci_d
> > pos + PCI_ERR_UNCOR_MASK,
> > &mask);
> > if (status & ERR_UNCORRECTABLE_ERROR_MASK & ~mask) {
> > - e_info->dev = dev;
> > - return 1;
> > + add_error_device(e_info, dev);
> > + goto added;
> > }
> > }
> >
> > return 0;
> > +
> > +added:
> > + if (e_info->flags & AER_MULTI_ERROR_VALID_FLAG) {
> > + return 0;
> > + } else
> > + return 1;
>
> checkpatch reports:
> WARNING: braces {} are not necessary for any arm of this statement
> #133: FILE: drivers/pci/pcie/aer/aerdrv_core.c:257:
I will change it.
> + if (e_info->flags & AER_MULTI_ERROR_VALID_FLAG) {
> [...]
> + } else
> [...]
>
>
> > }
> >
> > /**
> > @@ -705,6 +738,30 @@ static int get_device_error_info(struct
> > return AER_SUCCESS;
> > }
> >
> > +static inline void aer_process_err_devices(struct pcie_device *p_device,
> > + struct aer_err_info *e_info)
> > +{
> > + int i;
> > +
> > + if (e_info->dev[0] == NULL) {
> Minor not. Can we use
Yes. We can.
> if (!e_info->dev[0]) {
>
> > + printk(KERN_DEBUG "%s->can't find device of ID%04x\n",
> > + __func__, e_info->id);
>
> I suspect we don't want to embed the function name here, and use
> dev_printk.
Ok.
>
> > + }
> > +
> > + for (i = 0; i < e_info->error_dev_num; i ++) {
> checkpatch reports:
> ERROR: space prohibited before that '++' (ctx:WxB)
> #154: FILE: drivers/pci/pcie/aer/aerdrv_core.c:751:
> + for (i = 0; i < e_info->error_dev_num; i +
I will change it.
>
>
> > + if (e_info->dev[i] == NULL)
> again if (!e_info->dev[i])
Will do.
>
> You could also put this check in the for loop.
I planed to, but one guy helped me test it within a guest OS on XEN and
reported a weired oops of guest OS. She said useing e_info->error_dev_num
could avoid the oops.
>
>
> > + break;
> > +
> > + if (get_device_error_info(e_info->dev[i], e_info) ==
> > + AER_SUCCESS) {
> > + aer_print_error(e_info->dev[i], e_info);
> > + handle_error_source(p_device,
> > + e_info->dev[i],
> > + e_info);
> > + }
> > + }
> > +}
> > +
> > /**
> > * aer_isr_one_error - consume an error detected by root port
> > * @p_device: pointer to error root port service device
> > @@ -747,18 +804,7 @@ static void aer_isr_one_error(struct pci
> > e_info->flags |= AER_MULTI_ERROR_VALID_FLAG;
> >
> > find_source_device(p_device->port, e_info);
> > - if (e_info->dev == NULL) {
> > - printk(KERN_DEBUG "%s->can't find device of ID%04x\n",
> > - __func__, e_info->id);
> > - continue;
> > - }
> > - if (get_device_error_info(e_info->dev, e_info) ==
> > - AER_SUCCESS) {
> > - aer_print_error(e_info->dev, e_info);
> > - handle_error_source(p_device,
> > - e_info->dev,
> > - e_info);
> > - }
> > + aer_process_err_devices(p_device, e_info);
> > }
> >
> > kfree(e_info);
> > diff -Nraup linux-2.6_next_aernoid/drivers/pci/pcie/aer/aerdrv.h linux-2.6_next_aermultierror/drivers/pci/pcie/aer/aerdrv.h
> > --- linux-2.6_next_aernoid/drivers/pci/pcie/aer/aerdrv.h 2009-06-12 05:39:24.000000000 +0800
> > +++ linux-2.6_next_aermultierror/drivers/pci/pcie/aer/aerdrv.h 2009-06-12 05:45:15.000000000 +0800
> > @@ -57,8 +57,10 @@ struct header_log_regs {
> > unsigned int dw3;
> > };
> >
> > +#define AER_MAX_MULTI_ERR_DEVICES 5
> Is this number arbitrary or in the spec somewhere?
It's arbitrary and not spec. The startpoint is it's very rare that there are more
than 5 devices under the same root port reporting errors at the same time. It's hard
to say number 5 is the best. I just don't want the array is big.
>
>
> > struct aer_err_info {
> > - struct pci_dev *dev;
> > + struct pci_dev *dev[AER_MAX_MULTI_ERR_DEVICES];
> > + int error_dev_num;
> > u16 id;
> > int severity; /* 0:NONFATAL | 1:FATAL | 2:COR */
> > int flags;
> >
--
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/
Powered by blists - more mailing lists