| 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
| ||
|
Message-ID: <20090514175205.GB8575@aftab>
Date: Thu, 14 May 2009 19:52:05 +0200
From: Borislav Petkov <borislav.petkov@....com>
To: Ingo Molnar <mingo@...e.hu>
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
Hi Ingo,
thanks for so thoroughly reviewing the driver, the newest patchset will
be arriving shortly after testing is done.
On Thu, May 07, 2009 at 11:58:15PM +0200, Ingo Molnar wrote:
>
> * 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.
I needed the NUMA dependency for for_each_online_node() in order to init
an EDAC instance per northbridge on a multi-socket system. Converted the
code to num_k8_northbridges <x86/kernel/k8.c>, which should be ok for
now.
> 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.
done.
> > + 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.
Agreed. Done.
@Doug: edac-utils doesn't use those yet, right?
[..]
> > +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.
done.
> > +
> > +#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.
Yep, zero sized array is much more elegant, thanks.
[..]
> > +
> > + 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.)
fixed.
> > + /* 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.
done.
> > + 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.
done.
[..]
> > +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
done.
> > + 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?
Yeah, those debugf0X are not that elegant but since they're not
driver-specific but pertain to the EDAC core, we should postpone fixing
those for now.
> > +
> > + 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.
done and...
> 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.
done.
> > +
> > + /*
> > + * 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'.
yep, you bet. Thanks for catching that one. Fixed.
> > +
> > + /*
> > + * 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.)
symbolic constant? E.g., <arch/x86/kernel/cpu/amd.c> tests F10h with bare
numbers:
...
if (c->x86 == 0x10 || c->x86 == 0x11)
...
Maybe I'm missing something, hmm?
> > +
> > + /*
> > + * 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.
done.
> > +
> > +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.
done.
> > + }
> > +
> > + /*
> > + * 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.
>
done.
> > +
> > + /*
> > + * 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?
Well, this design is not really kernel-y. From the top of my head, we
could bypass the EDAC core' mc_alloc and use the void *pvt_info the way
it is done in the zillion other drivers - pointer to private data and
alloc that amd64_pvt dynamically in the driver, thus alleviating the
copy...
done.
> > +
> > + 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.
done
> > +
> > + 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.
Cleaning up... done. I was able to remove some of the dbg calls which I
thought are not needed. I'd suggest we have a second stab at that after
development settles, as we might still need the occasional dbg call here
and there.
> > +
> > + 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.
done
> > + },
> > + {
> > + /* 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.
done.
> > +};
> > +
> > +
> > +/*
> > + * 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;
> }
done and...
> Btw., please change all instances of:
>
> printk(KERN_WARNING, => pr_warning(
> printk(KERN_INFO, => pr_info(
>
> etc.
done.
> > + 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.
Even better, this dbg call got killed instead.
[..]
> > +
> > + /* 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.
Yep, thanks. Looks much more readable now.
> > + 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?
Done.
--
Regards/Gruss,
Boris.
Operating | Advanced Micro Devices GmbH
System | Karl-Hammerschmidt-Str. 34, 85609 Dornach b. München, Germany
Research | Geschäftsführer: Thomas M. McCoy, Giuliano Meroni
Center | Sitz: Dornach, Gemeinde Aschheim, Landkreis München
(OSRC) | Registergericht München, HRB Nr. 43632
--
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