Re: [PATCH 1/6] EDAC PCI device to DEVICE cleanup

[Doug Thompson <norsk5@yahoo.com>]

--- Andrew Morton <akpm@osdl.org> wrote:

> Doug Thompson <norsk5@yahoo.com> wrote:
> >
> > From: Doug Thompson <norsk5@xmission.com>
> > 
> > Change MC drivers from using CVS revision strings for their version number,
> > Now each driver has its own local string.
> > 
> > Remove some PCI dependencies from the core EDAC module.  
> > Made the code 'struct device' centric instead of 'struct pci_dev'
> > Most of the code changes here are from a patch by Dave Jiang.
> > It may be best to eventually move the PCI-specific code into a separate source
> file.
> > 
> > ...
> >
> > --- linux-2.6.17-rc6.orig/drivers/edac/edac_mc.h	2006-06-12 18:17:17.000000000
> -0600
> > +++ linux-2.6.17-rc6/drivers/edac/edac_mc.h	2006-06-12 18:17:29.000000000 -0600
> > @@ -88,6 +88,12 @@
> >  #define PCI_VEND_DEV(vend, dev) PCI_VENDOR_ID_ ## vend, \
> >  	PCI_DEVICE_ID_ ## vend ## _ ## dev
> >  
> > +#if defined(CONFIG_X86) && defined(CONFIG_PCI)
> > +#define dev_name(dev) pci_name(to_pci_dev(dev))
> > +#else
> > +#define dev_name(dev) to_platform_device(dev)->name
> > +#endif
> 
> This looks fishy.  pci_name() should work OK on non-x86?

Many of these mods, including this one, came from a non-x86 (PPC I think) developer
to make it work for him. Since I don't have a PPC test machine, I had/have to assume
he knew what he needed. It might work on non-x86, but I don't have a way to test.


> 
> > +static void do_pci_parity_check(void)
> > +{
> > +	unsigned long flags;
> > +	int before_count;
> > +
> > +	debugf3("%s()\n", __func__);
> > +
> > +	if (!check_pci_parity)
> > +		return;
> > +
> > +	before_count = atomic_read(&pci_parity_count);
> > +
> > +	/* scan all PCI devices looking for a Parity Error on devices and
> > +	 * bridges
> > +	 */
> > +	local_irq_save(flags);
> > +	edac_pci_dev_parity_iterator(edac_pci_dev_parity_test);
> > +	local_irq_restore(flags);
> > +
> > +	/* Only if operator has selected panic on PCI Error */
> > +	if (panic_on_pci_parity) {
> > +		/* If the count is different 'after' from 'before' */
> > +		if (before_count != atomic_read(&pci_parity_count))
> > +			panic("EDAC: PCI Parity Error");
> > +	}
> > +}
> 
> What is the local_irq_save() attempting to do in there?  It won't provide
> any locking-style coverage on SMP..

The PCI parity scanning and ECC memory harvesting are performed by a single kthread.
No multi-threads are performing these operation. Thus we only have to worry about
the single thread issues.

The iterator will examine all the PCI devices' parity in the system during a scan.
Since each device under examination will have its reference count bumped, it won't
be removed from the list, until the examination passes the device. I had to assume
that the developer was protecting the list during the scan from possible hotplug
events, which could be wrong on my part, as the reference count should do that.

More comments are needed to explain the single harvester model I suppose.  A future
patch will remove the the kthread and utilize a "work queue" model, but that has no
bearing on this issue.


doug t

> 
>