Message-ID: <20090507215815.GB28770@elte.hu>
Date:	Thu, 7 May 2009 23:58:15 +0200
From:	Ingo Molnar <mingo@...e.hu>
To:	Borislav Petkov <borislav.petkov@....com>
Cc:	akpm@...ux-foundation.org, greg@...ah.com, norsk5@...oo.com,
	tglx@...utronix.de, hpa@...or.com, mchehab@...hat.com,
	aris@...hat.com, edt@....ca, linux-kernel@...r.kernel.org,
	Doug Thompson <dougthompson@...ssion.com>
Subject: Re: [PATCH 21/21] amd64_edac: add module registration routines


* Borislav Petkov <borislav.petkov@....com> wrote:

> +config EDAC_AMD64_OPTERON
> +	tristate "AMD64 (Opteron, Athlon64) K8, F10h, F11h"
> +	depends on EDAC_MM_EDAC && X86 && PCI && NUMA

Please dont add too many depends conditions - that just reduces the 
testing (and the utility) of the code.

Especially the dependency on NUMA seems harmful - non-NUMA kernels 
are running on Opterons just fine. Especially when people have 
interleaved memory and dont fancy a NUMA scheduler they might opt to 
disable CONFIG_NUMA. EDAC support should really be orthogonal to 
that.

The CONFIG_PCI dependency is understandable :)

i'd also add a:

	default m

As when EDAC is enabled people (and distros) generally want to 
enable most of the sub-drivers without having to go through them.

> +	  Recent Opterons (Family 10h and later) provide for Memory Error
> +	  Injection into the ECC detection circuits. The amd64_edac module
> +	  allows the operator/user to inject Uncorrectable and Correctable
> +	  errors into DRAM.
> +
> +	  When enabled, in each of the respective memory controller directories
> +	  (/sys/devices/system/edac/mc/mcX), there are 3 input files:
> +
> +	  - z_inject_section (0..3, 16-byte section of 64-byte cacheline),
> +	  - z_inject_word (0..8, 16-bit word of 16-byte section),
> +	  - z_inject_bit_map (hex bitmap vector: mask bits of 16 bit word to
> +	    error-out)
> +
> +	  In addition, there are two control files, z_inject_read and
> +	  z_inject_write, which trigger the Read and Write errors respectively.

I think the file names should follow existing EDAC driver practices, 
and be named according to the existing inject_data_* pattern?

There's nothing more annoying to users (and tools) than inconsistent 
driver namespaces.

> +ifeq ($(CONFIG_EDAC_DEBUG), y)
> +	amd64_edac_mod-y += amd64_edac_dbg.o
> +endif

there's a shortcut for this:

	amd64_edac_mod-$(CONFIG_EDAC_DEBUG) += amd64_edac_dbg.o

> +ifeq ($(CONFIG_EDAC_AMD64_OPTERON_ERROR_INJECTION), y)
> +	amd64_edac_mod-y += amd64_edac_inj.o
> +endif

ditto.

> +
> +obj-$(CONFIG_EDAC_AMD64_OPTERON)	+= amd64_edac_mod.o
> +
>  obj-$(CONFIG_EDAC_PASEMI)		+= pasemi_edac.o
>  obj-$(CONFIG_EDAC_MPC85XX)		+= mpc85xx_edac.o
>  obj-$(CONFIG_EDAC_MV64X60)		+= mv64x60_edac.o
> diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> index af6eafd..c99cb55 100644
> --- a/drivers/edac/amd64_edac.c
> +++ b/drivers/edac/amd64_edac.c
> @@ -3272,3 +3272,488 @@ static int amd64_check_ecc_enabled(struct amd64_pvt *pvt)
>  	return rc;
>  }
>  
> +struct mcidev_sysfs_attribute sysfs_attrs[
> +#ifdef CONFIG_EDAC_DEBUG
> +ARRAY_SIZE(amd64_dbg_attrs) +
> +#endif
> +#ifdef CONFIG_EDAC_AMD64_OPTERON_ERROR_INJECTION
> +ARRAY_SIZE(amd64_inj_attrs) +
> +#endif
> +1];

There might be a cleaner way to express this by making the 
amd64_dbg_attrs[] array zero-sized in the !CONFIG_EDAC_DEBUG case - 
ditto for amd64_inj_attrs[].

(but if that results in awkwardness elsewhere then i guess we have 
to live with this)


> +static void amd64_set_mc_sysfs_attributes(struct mem_ctl_info *mci)
> +{
> +	struct mcidev_sysfs_attribute terminator = { .attr = { .name = NULL } };
> +	unsigned int i = 0, uninitialized_var(j);

i'd suggest a plain j=0. There's no performance argument here, and 
if j _ever_ in the future becomes truly unused, we have a GCC 
warning supressed and might face some serious bug without 
thecompiler helping us.

> +
> +#ifdef CONFIG_EDAC_DEBUG
> +	for (; i < ARRAY_SIZE(amd64_dbg_attrs); i++)
> +		sysfs_attrs[i] = amd64_dbg_attrs[i];
> +#endif

In fact, my suggestion above to make amd64_dbg_attrs[] defined but 
zero-size would eliminate this ifdef. So i'd suggest doing it.

> +
> +#ifdef CONFIG_EDAC_AMD64_OPTERON_ERROR_INJECTION
> +	for (j = 0; j < ARRAY_SIZE(amd64_inj_attrs); j++, i++)
> +		sysfs_attrs[i] = amd64_inj_attrs[j];
> +#endif

ditto.

> +
> +	sysfs_attrs[i] = terminator;
> +
> +	mci->mc_driver_sysfs_attributes = sysfs_attrs;
> +}
> +
> +/*
> + * amd64_setup_mci_misc_attributes
> + *
> + *	initialize various attributes of the mci structure
> + */
> +static void amd64_setup_mci_misc_attributes(struct mem_ctl_info *mci)
> +{
> +	struct amd64_pvt *pvt = mci->pvt_info;
> +
> +	/* Initialize various states */
> +	mci->mtype_cap = MEM_FLAG_DDR2 | MEM_FLAG_RDDR2;
> +	mci->edac_ctl_cap = EDAC_FLAG_NONE;
> +	mci->edac_cap = EDAC_FLAG_NONE;

Please write such 3-line or larger structure assignments as a 
vertically spaces construct:

	mci->mtype_cap		= MEM_FLAG_DDR2 | MEM_FLAG_RDDR2;
	mci->edac_ctl_cap	= EDAC_FLAG_NONE;
	mci->edac_cap		= EDAC_FLAG_NONE;

Note how visible the flags have become. (This form also tell us that 
the only non-standard initialization here is for the mtype_cap 
field. Such things help review - and can pinpoint bugs.)

> +	/* Exam the capabilities of the northbridge in order to reflect them
> +	 * in the presentation via sysfs attributes, etc
> +	 */

please use the customary comment style:

  /*
   * Comment .....
   * ...... goes here:
   */

also described in Documentation/CodingStyle.

This is a continuing observation for a number of other cases where 
you introduce half-winged comments.

> +	if (pvt->nbcap & K8_NBCAP_SECDED)
> +		mci->edac_ctl_cap |= EDAC_FLAG_SECDED;
> +
> +	if (pvt->nbcap & K8_NBCAP_CHIPKILL)
> +		mci->edac_ctl_cap |= EDAC_FLAG_S4ECD4ED;
> +
> +	/* What type of SECDED is there? */
> +	mci->edac_cap = amd64_determine_edac_cap(pvt);
> +
> +	/* Misc attributes to set */
> +	mci->mod_name = EDAC_MOD_STR;
> +	mci->mod_ver = EDAC_AMD64_VERSION;
> +	mci->ctl_name = get_amd_family_name(pvt->mc_type_index);
> +	mci->dev_name = pci_name(pvt->dram_f2_ctl);
> +	mci->ctl_page_to_phys = NULL;

vertical spacing probably helps here too.

> +
> +	/* IMPORTANT: Set the polling 'check' function in this module */
> +	mci->edac_check = amd64_check;
> +
> +	/* memory scrubber interface */
> +	mci->set_sdram_scrub_rate = amd64_set_scrub_rate;
> +	mci->get_sdram_scrub_rate = amd64_get_scrub_rate;
> +}
> +
> +/*
> + * amd64_probe_one_instance
> + *
> + *    probe function to determine if there is a DRAM Controller device is
> + *    present and to construct data tables for it.
> + *
> + *    Due to a hardware feature on Family 10H cpus, the Enable Extended
> + *    Configuration Space feature MUST be enabled on ALL Processors prior to
> + *    actually reading from the ECS registers. Since the loading of the module
> + *    can occur on any 'core', and cores don't 'see' all the other processors
> + *    ECS data when the others are NOT enabled. Our solution is to first
> + *    enable ECS access in this routine on all processors, gather some data in
> + *    a amd64_pvt structure and later come back in a 'finishup_setup' function
> + *    to perform that final initialization.
> + *
> + *    See also amd64_init_2nd_stage().
> + */

Nice comment!

> +static int amd64_probe_one_instance(struct pci_dev *dram_f2_ctl,
> +					int mc_type_index)
> +{
> +	struct amd64_pvt *pvt;
> +	int err;
> +	int rc;
> +
> +	pvt = kzalloc(sizeof(struct amd64_pvt), GFP_KERNEL);
> +	if (pvt == NULL) {
> +		rc = -ENOMEM;
> +		goto exit_now;
> +	}

The customary pattern is:

	ret = -ENOMEM;
	pvt = kzalloc(sizeof(struct amd64_pvt), GFP_KERNEL);
	if (!pvt)
		goto err;

Note the four differences:

 - use 'ret' instead of 'rc' for integer returns
 - pre-initialize 'ret', not in the exception branch
 - !pvt instead of 'pvt == NULL'
 - use 'err*' naming for error labels


> +	pvt->mc_node_id = get_mc_node_id_from_pdev(dram_f2_ctl);
> +
> +	debugf0("=========== %s(Instance= %d) ===========\n",
> +		__func__, pvt->mc_node_id);

please use something like trace_printk() instead. That will allow 
high-performance tracing of this code, should the need arise. Also 
'debugf0' says nothing - what does it mean?

> +
> +	pvt->dram_f2_ctl = dram_f2_ctl;
> +	pvt->ext_model = boot_cpu_data.x86_model >> 4;
> +	pvt->mc_type_index = mc_type_index;
> +	pvt->ops = get_amd_family_ops(mc_type_index);
> +	pvt->old_mcgctl = 0;

vertical spacing please.

Also, i'd suggest s/get_amd_family_ops/family_ops. This method is 
local to the driver so saying that it's 'amd' is superfluous.

> +
> +	/*
> +	 * We have the dram_f2_ctl device as an argument, now go reserved its
> +	 * sibling devices from the PCI system.
> +	 */
> +	err = amd64_reserve_mc_sibling_devices(pvt, mc_type_index);
> +	if (err) {
> +		rc = -ENODEV;
> +		goto exit_now;
> +	}

same allocation-error error pattern observation as above. It appears 
you use it in other places as well - so please fix it everywhere.

Bug: in the error case there's now a memory leak of 'pvt'.

> +
> +	rc = amd64_check_ecc_enabled(pvt);
> +	if (rc)
> +		goto exit_release_devices;

Bug: in the error case there's now a memory leak of 'pvt'.

> +
> +	/*
> +	 * Key operation here: setup of HW prior to performing ops on it. Some
> +	 * setup is required to access ECS data. After this is performed, then
> +	 * the 'teardown' function must be called upon error and normal exit
> +	 * paths.
> +	 */
> +	if (boot_cpu_data.x86 > 0xf)
> +		amd64_setup(pvt);

You really want to write >= 0x10 here - it's the same code but tells 
us the real story, that it's Fam10 and later. (And there's a 
symbolic constant for Fam10.)

> +
> +	/*
> +	 * Save the pointer to the private data for use in 2nd initialization
> +	 * stage
> +	 */
> +	pvt_lookup[pvt->mc_node_id] = pvt;
> +
> +	debugf0("%s(): init 1st stage done pvt-%d\n", __func__,
> +		pvt->mc_node_id);
> +	return 0;
> +
> +
> +exit_release_devices:
> +	pci_dev_put(pvt->addr_f1_ctl);
> +	pci_dev_put(pvt->misc_f3_ctl);

this teardown is correct but a bit unclean: it open-codes the 
reverse direction of:

	err = amd64_reserve_mc_sibling_devices(pvt, mc_type_index);

It would be better to add a amd64_free_mc_sibling_devices(pvt) 
method.

> +
> +exit_now:
> +	return rc;
> +}
> +
> +/*
> + * amd64_init_2nd_stage
> + *
> + *	this is the "finishing" up initialization code
> + *	Needs to be performed after all MCs' Hardware have been
> + *	"prep'ed" for accessing extended config space.
> + */
> +static int amd64_init_2nd_stage(struct amd64_pvt *pvt_temp)
> +{
> +	int node_id = pvt_temp->mc_node_id;
> +	struct mem_ctl_info *mci;
> +	struct amd64_pvt *pvt;
> +	int rc;
> +	int err;
> +
> +	debugf0("%s()\n", __func__);
> +
> +	amd64_read_mc_registers(pvt_temp);
> +
> +	/* Check hardware to see if this module can support HW at this time */
> +	if (pvt_temp->ops->probe_valid_hardware) {
> +		err = pvt_temp->ops->probe_valid_hardware(pvt_temp);
> +		if (err) {
> +			rc = -ENODEV;
> +			goto exit_failure;
> +		}

error handling pattern.

> +	}
> +
> +	/*
> +	 * We need to determine how many memory channels there are. Then use
> +	 * that information for calculating the size of the dynamic instance
> +	 * tables in the 'mci' structure
> +	 */
> +	pvt_temp->channel_count = pvt_temp->ops->early_channel_count(pvt_temp);
> +
> +	mci = edac_mc_alloc(sizeof(*pvt_temp),
> +				CHIPSELECT_COUNT,
> +				pvt_temp->channel_count,
> +				node_id);
> +	if (mci == NULL) {
> +		rc = -ENOMEM;
> +		goto exit_failure;
> +	}

ditto.

> +
> +	/*
> +	 * transfer the info from the interium pvt area to the private area of
> +	 * the MC instance structure
> +	 */
> +	pvt = mci->pvt_info;
> +	*pvt = *pvt_temp;

hm, such copying can be dangerous. If pvt_temp ever grows something 
like a list_head, then the copy can corrupt the list. Could this be 
avoided?

> +
> +	mci->dev = &pvt_temp->dram_f2_ctl->dev;
> +	amd64_setup_mci_misc_attributes(mci);
> +
> +	if (amd64_init_csrows(mci)) {
> +		debugf1("Setting mci->edac_cap to EDAC_FLAG_NONE because\n");
> +		debugf1("   amd64_init_csrows() returned NO csrows found\n");
> +		mci->edac_cap = EDAC_FLAG_NONE;
> +	}
> +
> +	amd64_enable_ecc_error_reporting(mci);
> +	amd64_set_mc_sysfs_attributes(mci);
> +
> +	if (edac_mc_add_mc(mci)) {
> +		debugf1("%s(): failed edac_mc_add_mc()\n", __func__);
> +		rc = -ENODEV;
> +		goto exit_add_mc_failure;
> +	}
> +
> +	debugf0("%s(): init 2nd stage done mci%d\n", __func__,
> +		pvt->mc_node_id);
> +
> +	mci_lookup[node_id] = mci;
> +
> +	kfree(pvt_lookup[pvt->mc_node_id]);
> +	pvt_lookup[node_id] = NULL;
> +	return 0;
> +
> +exit_add_mc_failure:
> +	edac_mc_free(mci);
> +
> +exit_failure:
> +	debugf0("%s() failure init 2nd stage: rc=%d\n", __func__, rc);
> +
> +	amd64_restore_ecc_error_reporting(pvt);
> +
> +	if (boot_cpu_data.x86 > 0xf)
> +		amd64_teardown(pvt);
> +
> +	pci_dev_put(pvt->addr_f1_ctl);
> +	pci_dev_put(pvt->misc_f3_ctl);

this too is a teardown open-coding assymetry.

> +
> +	kfree(pvt_lookup[pvt->mc_node_id]);
> +	pvt_lookup[node_id] = NULL;
> +
> +	return rc;
> +}
> +
> +
> +/*
> + * amd64_init_one_instance
> + *
> + *	initialize just one device
> + *
> + *	returns:
> + *		 count (>= 0), or
> + *		negative on error
> + */
> +static int __devinit amd64_init_one_instance(struct pci_dev *pdev,
> +				 const struct pci_device_id *mc_type)
> +{
> +	int rc;
> +
> +	debugf0("%s(MC node=%d,mc_type='%s')\n",
> +		__func__,
> +		get_mc_node_id_from_pdev(pdev),
> +		get_amd_family_name(mc_type->driver_data));
> +
> +	/* wake up and enable device */
> +	rc = pci_enable_device(pdev);
> +	if (rc < 0)
> +		rc = -EIO;
> +	else
> +		rc = amd64_probe_one_instance(pdev, mc_type->driver_data);
> +
> +	if (rc < 0)
> +		debugf0("%s() rc=%d\n", __func__, rc);

there's way too many debugf*() calls in the whole code, often 
obscuring the real structure. I think we'll be far better off with 
having just a few key ones.

> +
> +	return rc;
> +}
> +
> +/*
> + * amd64_remove_one_instance
> + *
> + *	remove just one device instance upon driver unloading
> + */
> +static void __devexit amd64_remove_one_instance(struct pci_dev *pdev)
> +{
> +	struct mem_ctl_info *mci;
> +	struct amd64_pvt *pvt;
> +
> +	debugf0("%s()\n", __func__);
> +
> +	/* Remove from EDAC CORE tracking list */
> +	mci = edac_mc_del_mc(&pdev->dev);
> +	if (mci == NULL)
> +		return;
> +
> +	pvt = mci->pvt_info;
> +
> +	amd64_restore_ecc_error_reporting(pvt);
> +
> +	if (boot_cpu_data.x86 > 0xf)
> +		amd64_teardown(pvt);
> +
> +	pci_dev_put(pvt->addr_f1_ctl);
> +	pci_dev_put(pvt->misc_f3_ctl);
> +
> +	mci_lookup[pvt->mc_node_id] = NULL;
> +
> +	/* Free the EDAC CORE resources */
> +	edac_mc_free(mci);
> +}
> +
> +/*
> + * The 'pci_device_id' table.
> + *
> + *	This table is part of the interface for loading drivers for PCI
> + *	devices. The PCI core identifies what devices are on a system
> + *	during boot, and then inquiry this table to see if this driver
> + *	is for a given device found.
> + *
> + *	The PCI helpper functions walk this table and call the
> + *	'.probe' function of the 'pci_driver' table, for each
> + *	instance in this table
> + */
> +static const struct pci_device_id amd64_pci_table[] __devinitdata = {
> +	{
> +		/* Rev F and prior */
> +		.vendor = PCI_VENDOR_ID_AMD,
> +		.device = PCI_DEVICE_ID_AMD_K8_NB_MEMCTL,
> +		.subvendor = PCI_ANY_ID,
> +		.subdevice = PCI_ANY_ID,
> +		.class = 0,
> +		.class_mask = 0,
> +		.driver_data = K8_CPUS

these initializers benefit from vertical spacing too:

		.vendor		= PCI_VENDOR_ID_AMD,
		.device		= PCI_DEVICE_ID_AMD_K8_NB_MEMCTL,
		.subvendor	= PCI_ANY_ID,
		.subdevice	= PCI_ANY_ID,
		.class		= 0,
		.class_mask	= 0,
		.driver_data	= K8_CPUS

It is elegant and readable at a glance.

> +	},
> +	{
> +		/* Family 10h */
> +		.vendor = PCI_VENDOR_ID_AMD,
> +		.device = PCI_DEVICE_ID_AMD_10H_NB_DRAM,
> +		.subvendor = PCI_ANY_ID,
> +		.subdevice = PCI_ANY_ID,
> +		.class = 0,
> +		.class_mask = 0,
> +		.driver_data = F10_CPUS
> +	},
> +	{
> +		/* Family 11h */
> +		.vendor = PCI_VENDOR_ID_AMD,
> +		.device = PCI_DEVICE_ID_AMD_11H_NB_DRAM,
> +		.subvendor = PCI_ANY_ID,
> +		.subdevice = PCI_ANY_ID,
> +		.class = 0,
> +		.class_mask = 0,
> +		.driver_data = F11_CPUS
> +	},
> +	{0, }
> +};
> +MODULE_DEVICE_TABLE(pci, amd64_pci_table);
> +
> +/*
> + * The 'pci_driver' structure to define the name, probe and removal
> + * functions
> + */
> +static struct pci_driver amd64_pci_driver = {
> +	.name = EDAC_MOD_STR,
> +	.probe = amd64_init_one_instance,
> +	.remove = __devexit_p(amd64_remove_one_instance),
> +	.id_table = amd64_pci_table,

ditto.

> +};
> +
> +
> +/*
> + * amd64_setup_pci_device
> + *
> + *	setup the PCI Device Driver for monitoring PCI errors
> + */
> +static void amd64_setup_pci_device(void)
> +{
> +	struct mem_ctl_info *mci;
> +	struct amd64_pvt *pvt;
> +
> +	if (!amd64_ctl_pci) {

this branch goes on:

> +
> +		mci = mci_lookup[0];
> +		if (mci) {
> +			debugf1("%s(): Registering ONE PCI control\n",
> +				__func__);

and on:

> +
> +			pvt = mci->pvt_info;
> +			amd64_ctl_pci = edac_pci_create_generic_ctl(
> +						&pvt->dram_f2_ctl->dev,
> +						EDAC_MOD_STR);
> +			if (!amd64_ctl_pci) {

and on:

> +				printk(KERN_WARNING
> +					"%s(): Unable to create PCI control\n",
> +					__func__);

And here you have too many tabs (five of them), that break up that 
printk in an ugly way.

The better solution is to realize that the branch could be inverted 
and a full level of indentation won by doing:

	if (amd64_ctl_pci)
		return;

And winning another level of indentation later on by doing:

	if (mci) {
		debugf1("%s(): ONE PCI control already registered\n",
				__func__);
		return;
	}

Btw., please change all instances of:

   printk(KERN_WARNING,       => pr_warning(
   printk(KERN_INFO,          => pr_info(

etc.

> +				printk(KERN_WARNING
> +					"%s(): PCI error report via EDAC "
> +					"not setup\n",
> +					__func__);
> +			}
> +		} else {
> +			debugf1("%s(): ONE PCI control already registered\n",
> +				__func__);
> +		}
> +	}
> +}
> +
> +static int __init amd64_edac_init(void)
> +{
> +	int err;
> +	int node;
> +
> +	edac_printk(KERN_INFO, EDAC_MOD_STR, EDAC_AMD64_VERSION "\n");
> +
> +	opstate_init();
> +
> +	debugf0("%s() ******************  ENTRY  **********************\n",
> +		__func__);


if you want a debug facility, please shorten it to something like:

	dbg(" **** ENTRY ****\n")

and make the printing of __func__ implicit in the dbg() method.  
This will shorten these things quite a bit. But please get rid of 
most of them - there's no excuse for having such things in the 
kernel beyond the first 1-2 days of the development of this 
function.

> +	/* Attempt to register drivers for instances
> +	 * DUE to the failure of some 'cores' to access Extended Config Space
> +	 * prior to all memory controllers having their ECS register enabled,
> +	 * the initialization has been created into 2 stages. Here we
> +	 * call for the 1st stage. After all have been enabled, then we
> +	 * do the 2nd stage to finishup setup.
> +	 */
> +	err = pci_register_driver(&amd64_pci_driver);
> +
> +	/* At this point, the array 'pvt_lookup[]' contains pointers to
> +	 * allocated struct amd64_pvt control structures. These will be used
> +	 * in the 2nd stage init function to finish initialization of
> +	 * the MC instances.
> +	 */

after using good comment style for a while, bad comment style shows 
up again.

> +
> +	/* if no error occurred on first pass init, then do 2nd pass init */
> +	if (!err) {

Again, by doing:

	if (err)
		goto err;

You can save an identation level below, and avoid line-wrap 
artifacts. 

> +		for_each_online_node(node) {
> +			if (!pvt_lookup[node])
> +				continue;
> +
> +			/* If any failure then need to clean up */
> +			err = amd64_init_2nd_stage(pvt_lookup[node]);
> +			if (err) {
> +				debugf0("%s() 'finish_setup' stage failed\n",
> +					__func__);
> +
> +				/* undo prior instances' registrations
> +				 * and leave as failed
> +				 */
> +				pci_unregister_driver(&amd64_pci_driver);
> +				goto error_exit;
> +			}
> +		}
> +		amd64_setup_pci_device();
> +	}
> +
> +error_exit:
> +	return err;
> +}
> +
> +static void __exit amd64_edac_exit(void)
> +{
> +	if (amd64_ctl_pci)
> +		edac_pci_release_generic_ctl(amd64_ctl_pci);
> +
> +	pci_unregister_driver(&amd64_pci_driver);
> +}
> +
> +module_init(amd64_edac_init);
> +module_exit(amd64_edac_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("SoftwareBitMaker: Doug Thompson, "
> +		"Dave Peterson, Thayne Harbaugh");
> +MODULE_DESCRIPTION("MC support for AMD64 memory controllers - "
> +		EDAC_AMD64_VERSION);
> +
> +module_param(edac_op_state, int, 0444);
> +MODULE_PARM_DESC(edac_op_state, "EDAC Error Reporting state: 0=Poll,1=NMI");
> diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
> index d435ace..a3603f7 100644
> --- a/drivers/edac/amd64_edac.h
> +++ b/drivers/edac/amd64_edac.h
> @@ -891,6 +891,11 @@ extern const char *ii_msgs[4];
>  extern const char *ext_msgs[32];
>  extern const char *htlink_msgs[8];
>  
> +#define NUM_DBG_ATTRS  9
> +#define NUM_INJ_ATTRS  5
> +extern struct mcidev_sysfs_attribute amd64_dbg_attrs[NUM_DBG_ATTRS],
> +				     amd64_inj_attrs[NUM_INJ_ATTRS];
> +
>  /*
>   * Each of the PCI Device IDs types have their own set of hardware
>   * accessor function and per device encoding/decoding logic.
> diff --git a/drivers/edac/amd64_edac_dbg.c b/drivers/edac/amd64_edac_dbg.c
> index 3741af9..f538129 100644
> --- a/drivers/edac/amd64_edac_dbg.c
> +++ b/drivers/edac/amd64_edac_dbg.c
> @@ -211,6 +211,9 @@ static ssize_t amd64_hole_show(struct mem_ctl_info *mci, char *data)
>  						 hole_size);
>  }
>  
> +/*
> + * update NUM_DBG_ATTRS in case you add new members
> + */
>  struct mcidev_sysfs_attribute amd64_dbg_attrs[] = {
>  
>  	{
> diff --git a/drivers/edac/amd64_edac_inj.c b/drivers/edac/amd64_edac_inj.c
> index 7f978cb..8706954 100644
> --- a/drivers/edac/amd64_edac_inj.c
> +++ b/drivers/edac/amd64_edac_inj.c
> @@ -155,6 +155,9 @@ static ssize_t amd64_inject_write_store(struct mem_ctl_info *mci,
>  	return 0;
>  }
>  
> +/*
> + * update NUM_INJ_ATTRS in case you add new members
> + */
>  struct mcidev_sysfs_attribute amd64_inj_attrs[] = {
>  
>  	{

Looks like these fixlets dont really belong in this patch and should 
either be backmerged or put into a separate patch?

	Ingo
--
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