On Mon, 2009-06-15 at 04:01 +0000, Andrew Patterson wrote:
> On Mon, 2009-06-15 at 09:47 +0800, Zhang, Yanmin wrote:
> > 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@xxxxxxxxxxxxxxx>
> > > >
> > > > + 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.
>
> I think something like:
>
> for (i = 0; i < e_info->error_dev_num && e_info->dev[i]; i++)
>
> is functionally equivalent.
I changed it.
>
> >
> > >
> > >
> > > > + 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.
>
> I suspected so.
>
> > 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.
>
> Agreed.
>
> > It's hard
> > to say number 5 is the best. I just don't want the array is big.
>
> I don't have a problem with that decision. But you might add a comment
> saying so, e.g.,
>
> #define AER_MAX_MULTI_ERR_DEVICES 5 /* Not likely to have more */
Added.
Thanks Andrew.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
