* [PATCH 01/14] edac: rewrite the sysfs code to use struct device
2012-03-29 17:06 [PATCH 00/14] Fix the EDAC API Mauro Carvalho Chehab
@ 2012-03-29 17:06 ` Mauro Carvalho Chehab
2012-03-29 22:03 ` Greg K H
2012-03-29 17:06 ` [PATCH 02/14] mpc85xx_edac: convert sysfs logic " Mauro Carvalho Chehab
` (13 subsequent siblings)
14 siblings, 1 reply; 26+ messages in thread
From: Mauro Carvalho Chehab @ 2012-03-29 17:06 UTC (permalink / raw)
Cc: Mauro Carvalho Chehab, Linux Edac Mailing List,
Linux Kernel Mailing List, Greg K H
The EDAC subsystem uses the old struct sysdev approach,
creating all nodes using the raw sysfs API. This is bad,
as the API is deprecated.
As we'll be changing the EDAC API, let's first port the existing
code to struct device.
There's one side-back on this patch: all device-specific sysfs
nodes won't be created anymore. While it would be possible to
also port the device-specific code, it is easier and nicer to
move the code to the drivers, instead, as the core can get rid
of some complex logic that just emulates what the device_add()
and device_create_file() already does.
The next patches will convert the driver-specific code to use
the device-specific calls. Then, the remaining bits of the old
sysfs API will be removed.
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
drivers/edac/edac_mc.c | 12 +-
drivers/edac/edac_mc_sysfs.c | 1068 +++++++++++++++---------------------------
drivers/edac/edac_module.c | 13 +-
drivers/edac/edac_module.h | 9 +-
include/linux/edac.h | 47 ++-
5 files changed, 432 insertions(+), 717 deletions(-)
diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index 2fa10d8..4a61d9a 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -209,7 +209,6 @@ struct mem_ctl_info *edac_mc_alloc(unsigned edac_index,
unsigned size, tot_dimms, count, pos[EDAC_MAX_LAYERS];
unsigned tot_csrows, tot_cschannels;
int i, j, n, len;
- int err;
int row, chn;
BUG_ON(n_layers > EDAC_MAX_LAYERS);
@@ -369,15 +368,6 @@ struct mem_ctl_info *edac_mc_alloc(unsigned edac_index,
mci->op_state = OP_ALLOC;
INIT_LIST_HEAD(&mci->grp_kobj_list);
- /*
- * Initialize the 'root' kobj for the edac_mc controller
- */
- err = edac_mc_register_sysfs_main_kobj(mci);
- if (err) {
- kfree(mci);
- return NULL;
- }
-
/* at this point, the root kobj is valid, and in order to
* 'free' the object, then the function:
* edac_mc_unregister_sysfs_main_kobj() must be called
@@ -400,7 +390,7 @@ void edac_mc_free(struct mem_ctl_info *mci)
{
debugf1("%s()\n", __func__);
- edac_mc_unregister_sysfs_main_kobj(mci);
+ edac_unregister_sysfs(mci);
/* free the mci instance memory here */
kfree(mci);
diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index dadc03c..83e80cc 100644
--- a/drivers/edac/edac_mc_sysfs.c
+++ b/drivers/edac/edac_mc_sysfs.c
@@ -7,17 +7,20 @@
*
* Written Doug Thompson <norsk5@xmission.com> www.softwarebitmaker.com
*
+ * (c) 2012 - Mauro Carvalho Chehab <mchehab@redhat.com
+ * The entire API were re-written, and ported to use struct device
+ *
*/
#include <linux/ctype.h>
#include <linux/slab.h>
#include <linux/edac.h>
#include <linux/bug.h>
+#include <linux/pm_runtime.h>
#include "edac_core.h"
#include "edac_module.h"
-
/* MC EDAC Controls, setable by module parameter, and sysfs */
static int edac_mc_log_ue = 1;
static int edac_mc_log_ce = 1;
@@ -78,6 +81,8 @@ module_param_call(edac_mc_poll_msec, edac_set_poll_msec, param_get_int,
&edac_mc_poll_msec, 0644);
MODULE_PARM_DESC(edac_mc_poll_msec, "Polling period in milliseconds");
+static struct device mci_pdev;
+
/*
* various constants for Memory Controllers
*/
@@ -125,309 +130,338 @@ static const char *edac_caps[] = {
[EDAC_S16ECD16ED] = "S16ECD16ED"
};
-/* EDAC sysfs CSROW data structures and methods
+/*
+ * EDAC sysfs CSROW data structures and methods
+ */
+
+#define to_csrow(k) container_of(k, struct csrow_info, dev)
+
+/*
+ * We need it to avoid namespace conflicts between the legacy API
+ * and the per-dimm/per-rank one
*/
+#define DEVICE_ATTR_LEGACY(_name, _mode, _show, _store) \
+ struct device_attribute dev_attr_legacy_##_name = __ATTR(_name, _mode, _show, _store)
+
+struct dev_ch_attribute {
+ struct device_attribute attr;
+ int channel;
+};
+
+#define DEVICE_CHANNEL(_name, _mode, _show, _store, _var) \
+ struct dev_ch_attribute dev_attr_legacy_##_name = \
+ { __ATTR(_name, _mode, _show, _store), (_var) }
+
+#define to_channel(k) (container_of(k, struct dev_ch_attribute, attr)->channel)
/* Set of more default csrow<id> attribute show/store functions */
-static ssize_t csrow_ue_mc_show(struct csrow_info *csrow, char *data,
- int private)
+static ssize_t csrow_ue_count_show(struct device *dev,
+ struct device_attribute *mattr, char *data)
{
+ struct csrow_info *csrow = to_csrow(dev);
+
return sprintf(data, "%u\n", csrow->ue_count);
}
-static ssize_t csrow_ce_mc_show(struct csrow_info *csrow, char *data,
- int private)
+static ssize_t csrow_ce_count_show(struct device *dev,
+ struct device_attribute *mattr, char *data)
{
+ struct csrow_info *csrow = to_csrow(dev);
+
return sprintf(data, "%u\n", csrow->ce_count);
}
-static ssize_t csrow_size_show(struct csrow_info *csrow, char *data,
- int private)
+static ssize_t csrow_size_show(struct device *dev,
+ struct device_attribute *mattr, char *data)
{
+ struct csrow_info *csrow = to_csrow(dev);
int i;
u32 nr_pages = 0;
for (i = 0; i < csrow->nr_channels; i++)
nr_pages += csrow->channels[i].dimm->nr_pages;
-
return sprintf(data, "%u\n", PAGES_TO_MiB(nr_pages));
}
-static ssize_t csrow_mem_type_show(struct csrow_info *csrow, char *data,
- int private)
+static ssize_t csrow_mem_type_show(struct device *dev,
+ struct device_attribute *mattr, char *data)
{
+ struct csrow_info *csrow = to_csrow(dev);
+
return sprintf(data, "%s\n", mem_types[csrow->channels[0].dimm->mtype]);
}
-static ssize_t csrow_dev_type_show(struct csrow_info *csrow, char *data,
- int private)
+static ssize_t csrow_dev_type_show(struct device *dev,
+ struct device_attribute *mattr, char *data)
{
+ struct csrow_info *csrow = to_csrow(dev);
+
return sprintf(data, "%s\n", dev_types[csrow->channels[0].dimm->dtype]);
}
-static ssize_t csrow_edac_mode_show(struct csrow_info *csrow, char *data,
- int private)
+static ssize_t csrow_edac_mode_show(struct device *dev,
+ struct device_attribute *mattr,
+ char *data)
{
+ struct csrow_info *csrow = to_csrow(dev);
+
return sprintf(data, "%s\n", edac_caps[csrow->channels[0].dimm->edac_mode]);
}
/* show/store functions for DIMM Label attributes */
-static ssize_t channel_dimm_label_show(struct csrow_info *csrow,
- char *data, int channel)
+static ssize_t channel_dimm_label_show(struct device *dev,
+ struct device_attribute *mattr,
+ char *data)
{
+ struct csrow_info *csrow = to_csrow(dev);
+ unsigned chan = to_channel(mattr);
+ struct rank_info *rank = &csrow->channels[chan];
+
/* if field has not been initialized, there is nothing to send */
- if (!csrow->channels[channel].dimm->label[0])
+ if (!rank->dimm->label[0])
return 0;
return snprintf(data, EDAC_MC_LABEL_LEN, "%s\n",
- csrow->channels[channel].dimm->label);
+ rank->dimm->label);
}
-static ssize_t channel_dimm_label_store(struct csrow_info *csrow,
- const char *data,
- size_t count, int channel)
+static ssize_t channel_dimm_label_store(struct device *dev,
+ struct device_attribute *mattr,
+ const char *data, size_t count)
{
+ struct csrow_info *csrow = to_csrow(dev);
+ unsigned chan = to_channel(mattr);
+ struct rank_info *rank = &csrow->channels[chan];
+
ssize_t max_size = 0;
max_size = min((ssize_t) count, (ssize_t) EDAC_MC_LABEL_LEN - 1);
- strncpy(csrow->channels[channel].dimm->label, data, max_size);
- csrow->channels[channel].dimm->label[max_size] = '\0';
+ strncpy(rank->dimm->label, data, max_size);
+ rank->dimm->label[max_size] = '\0';
return max_size;
}
-/* show function for dynamic chX_ce_mc attribute */
-static ssize_t channel_ce_mc_show(struct csrow_info *csrow,
- char *data, int channel)
+/* show function for dynamic chX_ce_count attribute */
+static ssize_t channel_ce_count_show(struct device *dev,
+ struct device_attribute *mattr, char *data)
{
- return sprintf(data, "%u\n", csrow->channels[channel].ce_count);
+ struct csrow_info *csrow = to_csrow(dev);
+ unsigned chan = to_channel(mattr);
+ struct rank_info *rank = &csrow->channels[chan];
+
+ return sprintf(data, "%u\n", rank->ce_count);
}
-/* csrow specific attribute structure */
-struct csrowdev_attribute {
- struct attribute attr;
- ssize_t(*show) (struct csrow_info *, char *, int);
- ssize_t(*store) (struct csrow_info *, const char *, size_t, int);
- int private;
-};
+/* cwrow<id>/attribute files */
+DEVICE_ATTR_LEGACY(size_mb, S_IRUGO, csrow_size_show, NULL);
+DEVICE_ATTR_LEGACY(dev_type, S_IRUGO, csrow_dev_type_show, NULL);
+DEVICE_ATTR_LEGACY(mem_type, S_IRUGO, csrow_mem_type_show, NULL);
+DEVICE_ATTR_LEGACY(edac_mode, S_IRUGO, csrow_edac_mode_show, NULL);
+DEVICE_ATTR_LEGACY(ue_count, S_IRUGO, csrow_ue_count_show, NULL);
+DEVICE_ATTR_LEGACY(ce_count, S_IRUGO, csrow_ce_count_show, NULL);
-#define to_csrow(k) container_of(k, struct csrow_info, kobj)
-#define to_csrowdev_attr(a) container_of(a, struct csrowdev_attribute, attr)
+/* default attributes of the CSROW<id> object */
+static struct attribute *csrow_attrs[] = {
+ &dev_attr_legacy_dev_type.attr,
+ &dev_attr_legacy_mem_type.attr,
+ &dev_attr_legacy_edac_mode.attr,
+ &dev_attr_legacy_size_mb.attr,
+ &dev_attr_legacy_ue_count.attr,
+ &dev_attr_legacy_ce_count.attr,
+ NULL,
+};
-/* Set of show/store higher level functions for default csrow attributes */
-static ssize_t csrowdev_show(struct kobject *kobj,
- struct attribute *attr, char *buffer)
-{
- struct csrow_info *csrow = to_csrow(kobj);
- struct csrowdev_attribute *csrowdev_attr = to_csrowdev_attr(attr);
+static struct attribute_group csrow_attr_grp = {
+ .attrs = csrow_attrs,
+};
- if (csrowdev_attr->show)
- return csrowdev_attr->show(csrow,
- buffer, csrowdev_attr->private);
- return -EIO;
-}
+static const struct attribute_group *csrow_attr_groups[] = {
+ &csrow_attr_grp,
+ NULL
+};
-static ssize_t csrowdev_store(struct kobject *kobj, struct attribute *attr,
- const char *buffer, size_t count)
+static void csrow_attr_release(struct device *device)
{
- struct csrow_info *csrow = to_csrow(kobj);
- struct csrowdev_attribute *csrowdev_attr = to_csrowdev_attr(attr);
-
- if (csrowdev_attr->store)
- return csrowdev_attr->store(csrow,
- buffer,
- count, csrowdev_attr->private);
- return -EIO;
+ debugf1("Releasing csrow device %s\n", dev_name(device));
}
-static const struct sysfs_ops csrowfs_ops = {
- .show = csrowdev_show,
- .store = csrowdev_store
-};
-
-#define CSROWDEV_ATTR(_name,_mode,_show,_store,_private) \
-static struct csrowdev_attribute attr_##_name = { \
- .attr = {.name = __stringify(_name), .mode = _mode }, \
- .show = _show, \
- .store = _store, \
- .private = _private, \
+static struct device_type csrow_attr_type = {
+ .groups = csrow_attr_groups,
+ .release = csrow_attr_release,
};
-/* default cwrow<id>/attribute files */
-CSROWDEV_ATTR(size_mb, S_IRUGO, csrow_size_show, NULL, 0);
-CSROWDEV_ATTR(dev_type, S_IRUGO, csrow_dev_type_show, NULL, 0);
-CSROWDEV_ATTR(mem_type, S_IRUGO, csrow_mem_type_show, NULL, 0);
-CSROWDEV_ATTR(edac_mode, S_IRUGO, csrow_edac_mode_show, NULL, 0);
-CSROWDEV_ATTR(ue_mc, S_IRUGO, csrow_ue_mc_show, NULL, 0);
-CSROWDEV_ATTR(ce_mc, S_IRUGO, csrow_ce_mc_show, NULL, 0);
+/*
+ * possible dynamic channel DIMM Label attribute files
+ *
+ */
-/* default attributes of the CSROW<id> object */
-static struct csrowdev_attribute *default_csrow_attr[] = {
- &attr_dev_type,
- &attr_mem_type,
- &attr_edac_mode,
- &attr_size_mb,
- &attr_ue_mc,
- &attr_ce_mc,
- NULL,
-};
+#define EDAC_NR_CHANNELS 6
-/* possible dynamic channel DIMM Label attribute files */
-CSROWDEV_ATTR(ch0_dimm_label, S_IRUGO | S_IWUSR,
+DEVICE_CHANNEL(ch0_dimm_label, S_IRUGO | S_IWUSR,
channel_dimm_label_show, channel_dimm_label_store, 0);
-CSROWDEV_ATTR(ch1_dimm_label, S_IRUGO | S_IWUSR,
+DEVICE_CHANNEL(ch1_dimm_label, S_IRUGO | S_IWUSR,
channel_dimm_label_show, channel_dimm_label_store, 1);
-CSROWDEV_ATTR(ch2_dimm_label, S_IRUGO | S_IWUSR,
+DEVICE_CHANNEL(ch2_dimm_label, S_IRUGO | S_IWUSR,
channel_dimm_label_show, channel_dimm_label_store, 2);
-CSROWDEV_ATTR(ch3_dimm_label, S_IRUGO | S_IWUSR,
+DEVICE_CHANNEL(ch3_dimm_label, S_IRUGO | S_IWUSR,
channel_dimm_label_show, channel_dimm_label_store, 3);
-CSROWDEV_ATTR(ch4_dimm_label, S_IRUGO | S_IWUSR,
+DEVICE_CHANNEL(ch4_dimm_label, S_IRUGO | S_IWUSR,
channel_dimm_label_show, channel_dimm_label_store, 4);
-CSROWDEV_ATTR(ch5_dimm_label, S_IRUGO | S_IWUSR,
+DEVICE_CHANNEL(ch5_dimm_label, S_IRUGO | S_IWUSR,
channel_dimm_label_show, channel_dimm_label_store, 5);
/* Total possible dynamic DIMM Label attribute file table */
-static struct csrowdev_attribute *dynamic_csrow_dimm_attr[] = {
- &attr_ch0_dimm_label,
- &attr_ch1_dimm_label,
- &attr_ch2_dimm_label,
- &attr_ch3_dimm_label,
- &attr_ch4_dimm_label,
- &attr_ch5_dimm_label
+static struct device_attribute *dynamic_csrow_dimm_attr[] = {
+ &dev_attr_legacy_ch0_dimm_label.attr,
+ &dev_attr_legacy_ch1_dimm_label.attr,
+ &dev_attr_legacy_ch2_dimm_label.attr,
+ &dev_attr_legacy_ch3_dimm_label.attr,
+ &dev_attr_legacy_ch4_dimm_label.attr,
+ &dev_attr_legacy_ch5_dimm_label.attr
};
-/* possible dynamic channel ce_mc attribute files */
-CSROWDEV_ATTR(ch0_ce_mc, S_IRUGO | S_IWUSR, channel_ce_mc_show, NULL, 0);
-CSROWDEV_ATTR(ch1_ce_mc, S_IRUGO | S_IWUSR, channel_ce_mc_show, NULL, 1);
-CSROWDEV_ATTR(ch2_ce_mc, S_IRUGO | S_IWUSR, channel_ce_mc_show, NULL, 2);
-CSROWDEV_ATTR(ch3_ce_mc, S_IRUGO | S_IWUSR, channel_ce_mc_show, NULL, 3);
-CSROWDEV_ATTR(ch4_ce_mc, S_IRUGO | S_IWUSR, channel_ce_mc_show, NULL, 4);
-CSROWDEV_ATTR(ch5_ce_mc, S_IRUGO | S_IWUSR, channel_ce_mc_show, NULL, 5);
-
-/* Total possible dynamic ce_mc attribute file table */
-static struct csrowdev_attribute *dynamic_csrow_ce_mc_attr[] = {
- &attr_ch0_ce_mc,
- &attr_ch1_ce_mc,
- &attr_ch2_ce_mc,
- &attr_ch3_ce_mc,
- &attr_ch4_ce_mc,
- &attr_ch5_ce_mc
+/* possible dynamic channel ce_count attribute files */
+DEVICE_CHANNEL(ch0_ce_count, S_IRUGO | S_IWUSR,
+ channel_ce_count_show, NULL, 0);
+DEVICE_CHANNEL(ch1_ce_count, S_IRUGO | S_IWUSR,
+ channel_ce_count_show, NULL, 1);
+DEVICE_CHANNEL(ch2_ce_count, S_IRUGO | S_IWUSR,
+ channel_ce_count_show, NULL, 2);
+DEVICE_CHANNEL(ch3_ce_count, S_IRUGO | S_IWUSR,
+ channel_ce_count_show, NULL, 3);
+DEVICE_CHANNEL(ch4_ce_count, S_IRUGO | S_IWUSR,
+ channel_ce_count_show, NULL, 4);
+DEVICE_CHANNEL(ch5_ce_count, S_IRUGO | S_IWUSR,
+ channel_ce_count_show, NULL, 5);
+
+/* Total possible dynamic ce_count attribute file table */
+static struct device_attribute *dynamic_csrow_ce_count_attr[] = {
+ &dev_attr_legacy_ch0_ce_count.attr,
+ &dev_attr_legacy_ch1_ce_count.attr,
+ &dev_attr_legacy_ch2_ce_count.attr,
+ &dev_attr_legacy_ch3_ce_count.attr,
+ &dev_attr_legacy_ch4_ce_count.attr,
+ &dev_attr_legacy_ch5_ce_count.attr
};
-#define EDAC_NR_CHANNELS 6
-
-/* Create dynamic CHANNEL files, indexed by 'chan', under specifed CSROW */
-static int edac_create_channel_files(struct kobject *kobj, int chan)
+/* Create a CSROW object under specifed edac_mc_device */
+static int edac_create_csrow_object(struct mem_ctl_info *mci,
+ struct csrow_info *csrow, int index)
{
- int err = -ENODEV;
+ int err, chan;
- if (chan >= EDAC_NR_CHANNELS)
- return err;
+ if (csrow->nr_channels >= EDAC_NR_CHANNELS)
+ return -ENODEV;
- /* create the DIMM label attribute file */
- err = sysfs_create_file(kobj,
- (struct attribute *)
- dynamic_csrow_dimm_attr[chan]);
-
- if (!err) {
- /* create the CE Count attribute file */
- err = sysfs_create_file(kobj,
- (struct attribute *)
- dynamic_csrow_ce_mc_attr[chan]);
- } else {
- debugf1("%s() dimm labels and ce_mc files created",
- __func__);
- }
+ csrow->dev.type = &csrow_attr_type;
+ csrow->dev.bus = mci_pdev.bus;
+ device_initialize(&csrow->dev);
+ csrow->dev.parent = &mci->dev;
+ dev_set_name(&csrow->dev, "csrow%d", index);
+ dev_set_drvdata(&csrow->dev, csrow);
- return err;
-}
+ debugf0("%s(): creating (virtual) csrow node %s\n", __func__,
+ dev_name(&csrow->dev));
-/* No memory to release for this kobj */
-static void edac_csrow_instance_release(struct kobject *kobj)
-{
- struct mem_ctl_info *mci;
- struct csrow_info *cs;
+ err = device_add(&csrow->dev);
+ if (err < 0)
+ return err;
- debugf1("%s()\n", __func__);
+ for (chan = 0; chan < csrow->nr_channels; chan++) {
+ err = device_create_file(&csrow->dev,
+ dynamic_csrow_dimm_attr[chan]);
+ if (err < 0)
+ goto error;
+ err = device_create_file(&csrow->dev,
+ dynamic_csrow_ce_count_attr[chan]);
+ if (err < 0) {
+ device_remove_file(&csrow->dev,
+ dynamic_csrow_dimm_attr[chan]);
+ goto error;
+ }
+ }
- cs = container_of(kobj, struct csrow_info, kobj);
- mci = cs->mci;
+ return 0;
- kobject_put(&mci->edac_mci_kobj);
-}
+error:
+ for (--chan; chan >= 0; chan--) {
+ device_remove_file(&csrow->dev,
+ dynamic_csrow_dimm_attr[chan]);
+ device_remove_file(&csrow->dev,
+ dynamic_csrow_ce_count_attr[chan]);
+ }
+ put_device(&csrow->dev);
-/* the kobj_type instance for a CSROW */
-static struct kobj_type ktype_csrow = {
- .release = edac_csrow_instance_release,
- .sysfs_ops = &csrowfs_ops,
- .default_attrs = (struct attribute **)default_csrow_attr,
-};
+ return err;
+}
/* Create a CSROW object under specifed edac_mc_device */
-static int edac_create_csrow_object(struct mem_ctl_info *mci,
- struct csrow_info *csrow, int index)
+static int edac_create_csrow_objects(struct mem_ctl_info *mci)
{
- struct kobject *kobj_mci = &mci->edac_mci_kobj;
- struct kobject *kobj;
- int chan;
- int err;
+ int err, i, chan;
+ struct csrow_info *csrow;
- /* generate ..../edac/mc/mc<id>/csrow<index> */
- memset(&csrow->kobj, 0, sizeof(csrow->kobj));
- csrow->mci = mci; /* include container up link */
+ for (i = 0; i < mci->num_csrows; i++) {
+ err = edac_create_csrow_object(mci, &mci->csrows[i], i);
+ if (err < 0)
+ goto error;
+ }
+ return 0;
- /* bump the mci instance's kobject's ref count */
- kobj = kobject_get(&mci->edac_mci_kobj);
- if (!kobj) {
- err = -ENODEV;
- goto err_out;
+error:
+ for (--i; i >= 0; i--) {
+ csrow = &mci->csrows[i];
+ for (chan = csrow->nr_channels - 1; chan >= 0; chan--) {
+ device_remove_file(&csrow->dev,
+ dynamic_csrow_dimm_attr[chan]);
+ device_remove_file(&csrow->dev,
+ dynamic_csrow_ce_count_attr[chan]);
+ }
+ put_device(&mci->csrows[i].dev);
}
- /* Instanstiate the csrow object */
- err = kobject_init_and_add(&csrow->kobj, &ktype_csrow, kobj_mci,
- "csrow%d", index);
- if (err)
- goto err_release_top_kobj;
+ return err;
+}
- /* At this point, to release a csrow kobj, one must
- * call the kobject_put and allow that tear down
- * to work the releasing
- */
+static void edac_delete_csrow_objects(struct mem_ctl_info *mci)
+{
+ int i, chan;
+ struct csrow_info *csrow;
- /* Create the dyanmic attribute files on this csrow,
- * namely, the DIMM labels and the channel ce_mc
- */
- for (chan = 0; chan < csrow->nr_channels; chan++) {
- err = edac_create_channel_files(&csrow->kobj, chan);
- if (err) {
- /* special case the unregister here */
- kobject_put(&csrow->kobj);
- goto err_out;
+ for (i = mci->num_csrows - 1; i >= 0; i--) {
+ csrow = &mci->csrows[i];
+ for (chan = csrow->nr_channels - 1; chan >= 0; chan--) {
+ debugf1("Removing csrow %d channel %d sysfs nodes\n",
+ i, chan);
+ device_remove_file(&csrow->dev,
+ dynamic_csrow_dimm_attr[chan]);
+ device_remove_file(&csrow->dev,
+ dynamic_csrow_ce_count_attr[chan]);
}
+ put_device(&mci->csrows[i].dev);
+ device_del(&mci->csrows[i].dev);
}
- kobject_uevent(&csrow->kobj, KOBJ_ADD);
- return 0;
-
- /* error unwind stack */
-err_release_top_kobj:
- kobject_put(&mci->edac_mci_kobj);
-
-err_out:
- return err;
}
-/* default sysfs methods and data structures for the main MCI kobject */
+/*
+ * Memory controller device
+ */
+
+#define to_mci(k) container_of(k, struct mem_ctl_info, dev)
-static ssize_t mci_reset_counters_store(struct mem_ctl_info *mci,
+static ssize_t mci_reset_counters_store(struct device *dev,
+ struct device_attribute *mattr,
const char *data, size_t count)
{
+ struct mem_ctl_info *mci = to_mci(dev);
int cnt, row, chan, i;
-
mci->ue_mc = 0;
mci->ce_mc = 0;
mci->ue_noinfo_count = 0;
mci->ce_noinfo_count = 0;
+
for (row = 0; row < mci->num_csrows; row++) {
struct csrow_info *ri = &mci->csrows[row];
@@ -441,8 +475,8 @@ static ssize_t mci_reset_counters_store(struct mem_ctl_info *mci,
cnt = 1;
for (i = 0; i < mci->n_layers; i++) {
cnt *= mci->layers[i].size;
- memset(mci->ce_per_layer[i], 0, cnt);
- memset(mci->ue_per_layer[i], 0, cnt);
+ memset(mci->ce_per_layer[i], 0, cnt * sizeof(u32));
+ memset(mci->ue_per_layer[i], 0, cnt * sizeof(u32));
}
mci->start_time = jiffies;
@@ -458,9 +492,11 @@ static ssize_t mci_reset_counters_store(struct mem_ctl_info *mci,
* Negative value still means that an error has occurred while setting
* the scrub rate.
*/
-static ssize_t mci_sdram_scrub_rate_store(struct mem_ctl_info *mci,
+static ssize_t mci_sdram_scrub_rate_store(struct device *dev,
+ struct device_attribute *mattr,
const char *data, size_t count)
{
+ struct mem_ctl_info *mci = to_mci(dev);
unsigned long bandwidth = 0;
int new_bw = 0;
@@ -483,8 +519,11 @@ static ssize_t mci_sdram_scrub_rate_store(struct mem_ctl_info *mci,
/*
* ->get_sdram_scrub_rate() return value semantics same as above.
*/
-static ssize_t mci_sdram_scrub_rate_show(struct mem_ctl_info *mci, char *data)
+static ssize_t mci_sdram_scrub_rate_show(struct device *dev,
+ struct device_attribute *mattr,
+ char *data)
{
+ struct mem_ctl_info *mci = to_mci(dev);
int bandwidth = 0;
if (!mci->get_sdram_scrub_rate)
@@ -500,38 +539,66 @@ static ssize_t mci_sdram_scrub_rate_show(struct mem_ctl_info *mci, char *data)
}
/* default attribute files for the MCI object */
-static ssize_t mci_ue_mc_show(struct mem_ctl_info *mci, char *data)
+static ssize_t mci_ue_count_show(struct device *dev,
+ struct device_attribute *mattr,
+ char *data)
{
+ struct mem_ctl_info *mci = to_mci(dev);
+
return sprintf(data, "%d\n", mci->ue_mc);
}
-static ssize_t mci_ce_mc_show(struct mem_ctl_info *mci, char *data)
+static ssize_t mci_ce_count_show(struct device *dev,
+ struct device_attribute *mattr,
+ char *data)
{
+ struct mem_ctl_info *mci = to_mci(dev);
+
return sprintf(data, "%d\n", mci->ce_mc);
}
-static ssize_t mci_ce_noinfo_show(struct mem_ctl_info *mci, char *data)
+static ssize_t mci_ce_noinfo_show(struct device *dev,
+ struct device_attribute *mattr,
+ char *data)
{
+ struct mem_ctl_info *mci = to_mci(dev);
+
return sprintf(data, "%d\n", mci->ce_noinfo_count);
}
-static ssize_t mci_ue_noinfo_show(struct mem_ctl_info *mci, char *data)
+static ssize_t mci_ue_noinfo_show(struct device *dev,
+ struct device_attribute *mattr,
+ char *data)
{
+ struct mem_ctl_info *mci = to_mci(dev);
+
return sprintf(data, "%d\n", mci->ue_noinfo_count);
}
-static ssize_t mci_seconds_show(struct mem_ctl_info *mci, char *data)
+static ssize_t mci_seconds_show(struct device *dev,
+ struct device_attribute *mattr,
+ char *data)
{
+ struct mem_ctl_info *mci = to_mci(dev);
+
return sprintf(data, "%ld\n", (jiffies - mci->start_time) / HZ);
}
-static ssize_t mci_ctl_name_show(struct mem_ctl_info *mci, char *data)
+static ssize_t mci_ctl_name_show(struct device *dev,
+ struct device_attribute *mattr,
+ char *data)
{
+ struct mem_ctl_info *mci = to_mci(dev);
+
return sprintf(data, "%s\n", mci->ctl_name);
}
-static ssize_t mci_size_mb_show(struct mem_ctl_info *mci, char *data)
+static ssize_t mci_size_mb_show(struct device *dev,
+ struct device_attribute *mattr,
+ char *data)
{
+ struct mem_ctl_info *mci = to_mci(dev);
+
int total_pages, csrow_idx, j;
for (total_pages = csrow_idx = 0; csrow_idx < mci->num_csrows;
@@ -548,360 +615,53 @@ static ssize_t mci_size_mb_show(struct mem_ctl_info *mci, char *data)
return sprintf(data, "%u\n", PAGES_TO_MiB(total_pages));
}
-#define to_mci(k) container_of(k, struct mem_ctl_info, edac_mci_kobj)
-#define to_mcidev_attr(a) container_of(a,struct mcidev_sysfs_attribute,attr)
-
-/* MCI show/store functions for top most object */
-static ssize_t mcidev_show(struct kobject *kobj, struct attribute *attr,
- char *buffer)
-{
- struct mem_ctl_info *mem_ctl_info = to_mci(kobj);
- struct mcidev_sysfs_attribute *mcidev_attr = to_mcidev_attr(attr);
-
- debugf1("%s() mem_ctl_info %p\n", __func__, mem_ctl_info);
-
- if (mcidev_attr->show)
- return mcidev_attr->show(mem_ctl_info, buffer);
-
- return -EIO;
-}
-
-static ssize_t mcidev_store(struct kobject *kobj, struct attribute *attr,
- const char *buffer, size_t count)
-{
- struct mem_ctl_info *mem_ctl_info = to_mci(kobj);
- struct mcidev_sysfs_attribute *mcidev_attr = to_mcidev_attr(attr);
-
- debugf1("%s() mem_ctl_info %p\n", __func__, mem_ctl_info);
-
- if (mcidev_attr->store)
- return mcidev_attr->store(mem_ctl_info, buffer, count);
-
- return -EIO;
-}
-
-/* Intermediate show/store table */
-static const struct sysfs_ops mci_ops = {
- .show = mcidev_show,
- .store = mcidev_store
-};
-
-#define MCIDEV_ATTR(_name,_mode,_show,_store) \
-static struct mcidev_sysfs_attribute mci_attr_##_name = { \
- .attr = {.name = __stringify(_name), .mode = _mode }, \
- .show = _show, \
- .store = _store, \
-};
-
/* default Control file */
-MCIDEV_ATTR(reset_counters, S_IWUSR, NULL, mci_reset_counters_store);
+DEVICE_ATTR(reset_counters, S_IWUSR, NULL, mci_reset_counters_store);
/* default Attribute files */
-MCIDEV_ATTR(mc_name, S_IRUGO, mci_ctl_name_show, NULL);
-MCIDEV_ATTR(size_mb, S_IRUGO, mci_size_mb_show, NULL);
-MCIDEV_ATTR(seconds_since_reset, S_IRUGO, mci_seconds_show, NULL);
-MCIDEV_ATTR(ue_noinfo_count, S_IRUGO, mci_ue_noinfo_show, NULL);
-MCIDEV_ATTR(ce_noinfo_count, S_IRUGO, mci_ce_noinfo_show, NULL);
-MCIDEV_ATTR(ue_mc, S_IRUGO, mci_ue_mc_show, NULL);
-MCIDEV_ATTR(ce_mc, S_IRUGO, mci_ce_mc_show, NULL);
+DEVICE_ATTR(mc_name, S_IRUGO, mci_ctl_name_show, NULL);
+DEVICE_ATTR(size_mb, S_IRUGO, mci_size_mb_show, NULL);
+DEVICE_ATTR(seconds_since_reset, S_IRUGO, mci_seconds_show, NULL);
+DEVICE_ATTR(ue_noinfo_count, S_IRUGO, mci_ue_noinfo_show, NULL);
+DEVICE_ATTR(ce_noinfo_count, S_IRUGO, mci_ce_noinfo_show, NULL);
+DEVICE_ATTR(ue_count, S_IRUGO, mci_ue_count_show, NULL);
+DEVICE_ATTR(ce_count, S_IRUGO, mci_ce_count_show, NULL);
/* memory scrubber attribute file */
-MCIDEV_ATTR(sdram_scrub_rate, S_IRUGO | S_IWUSR, mci_sdram_scrub_rate_show,
+DEVICE_ATTR(sdram_scrub_rate, S_IRUGO | S_IWUSR, mci_sdram_scrub_rate_show,
mci_sdram_scrub_rate_store);
-static struct mcidev_sysfs_attribute *mci_attr[] = {
- &mci_attr_reset_counters,
- &mci_attr_mc_name,
- &mci_attr_size_mb,
- &mci_attr_seconds_since_reset,
- &mci_attr_ue_noinfo_count,
- &mci_attr_ce_noinfo_count,
- &mci_attr_ue_mc,
- &mci_attr_ce_mc,
- &mci_attr_sdram_scrub_rate,
+static struct attribute *mci_attrs[] = {
+ &dev_attr_reset_counters.attr,
+ &dev_attr_mc_name.attr,
+ &dev_attr_size_mb.attr,
+ &dev_attr_seconds_since_reset.attr,
+ &dev_attr_ue_noinfo_count.attr,
+ &dev_attr_ce_noinfo_count.attr,
+ &dev_attr_ue_count.attr,
+ &dev_attr_ce_count.attr,
+ &dev_attr_sdram_scrub_rate.attr,
NULL
};
-
-/*
- * Release of a MC controlling instance
- *
- * each MC control instance has the following resources upon entry:
- * a) a ref count on the top memctl kobj
- * b) a ref count on this module
- *
- * this function must decrement those ref counts and then
- * issue a free on the instance's memory
- */
-static void edac_mci_control_release(struct kobject *kobj)
-{
- struct mem_ctl_info *mci;
-
- mci = to_mci(kobj);
-
- debugf0("%s() mci instance idx=%d releasing\n", __func__, mci->mc_idx);
-
- /* decrement the module ref count */
- module_put(mci->owner);
-}
-
-static struct kobj_type ktype_mci = {
- .release = edac_mci_control_release,
- .sysfs_ops = &mci_ops,
- .default_attrs = (struct attribute **)mci_attr,
-};
-
-/* EDAC memory controller sysfs kset:
- * /sys/devices/system/edac/mc
- */
-static struct kset *mc_kset;
-
-/*
- * edac_mc_register_sysfs_main_kobj
- *
- * setups and registers the main kobject for each mci
- */
-int edac_mc_register_sysfs_main_kobj(struct mem_ctl_info *mci)
-{
- struct kobject *kobj_mci;
- int err;
-
- debugf1("%s()\n", __func__);
-
- kobj_mci = &mci->edac_mci_kobj;
-
- /* Init the mci's kobject */
- memset(kobj_mci, 0, sizeof(*kobj_mci));
-
- /* Record which module 'owns' this control structure
- * and bump the ref count of the module
- */
- mci->owner = THIS_MODULE;
-
- /* bump ref count on this module */
- if (!try_module_get(mci->owner)) {
- err = -ENODEV;
- goto fail_out;
- }
-
- /* this instance become part of the mc_kset */
- kobj_mci->kset = mc_kset;
-
- /* register the mc<id> kobject to the mc_kset */
- err = kobject_init_and_add(kobj_mci, &ktype_mci, NULL,
- "mc%d", mci->mc_idx);
- if (err) {
- debugf1("%s()Failed to register '.../edac/mc%d'\n",
- __func__, mci->mc_idx);
- goto kobj_reg_fail;
- }
- kobject_uevent(kobj_mci, KOBJ_ADD);
-
- /* At this point, to 'free' the control struct,
- * edac_mc_unregister_sysfs_main_kobj() must be used
- */
-
- debugf1("%s() Registered '.../edac/mc%d' kobject\n",
- __func__, mci->mc_idx);
-
- return 0;
-
- /* Error exit stack */
-
-kobj_reg_fail:
- module_put(mci->owner);
-
-fail_out:
- return err;
-}
-
-/*
- * edac_mc_register_sysfs_main_kobj
- *
- * tears down and the main mci kobject from the mc_kset
- */
-void edac_mc_unregister_sysfs_main_kobj(struct mem_ctl_info *mci)
-{
- debugf1("%s()\n", __func__);
-
- /* delete the kobj from the mc_kset */
- kobject_put(&mci->edac_mci_kobj);
-}
-
-#define EDAC_DEVICE_SYMLINK "device"
-
-#define grp_to_mci(k) (container_of(k, struct mcidev_sysfs_group_kobj, kobj)->mci)
-
-/* MCI show/store functions for top most object */
-static ssize_t inst_grp_show(struct kobject *kobj, struct attribute *attr,
- char *buffer)
-{
- struct mem_ctl_info *mem_ctl_info = grp_to_mci(kobj);
- struct mcidev_sysfs_attribute *mcidev_attr = to_mcidev_attr(attr);
-
- debugf1("%s() mem_ctl_info %p\n", __func__, mem_ctl_info);
-
- if (mcidev_attr->show)
- return mcidev_attr->show(mem_ctl_info, buffer);
-
- return -EIO;
-}
-
-static ssize_t inst_grp_store(struct kobject *kobj, struct attribute *attr,
- const char *buffer, size_t count)
-{
- struct mem_ctl_info *mem_ctl_info = grp_to_mci(kobj);
- struct mcidev_sysfs_attribute *mcidev_attr = to_mcidev_attr(attr);
-
- debugf1("%s() mem_ctl_info %p\n", __func__, mem_ctl_info);
-
- if (mcidev_attr->store)
- return mcidev_attr->store(mem_ctl_info, buffer, count);
-
- return -EIO;
-}
-
-/* No memory to release for this kobj */
-static void edac_inst_grp_release(struct kobject *kobj)
-{
- struct mcidev_sysfs_group_kobj *grp;
- struct mem_ctl_info *mci;
-
- debugf1("%s()\n", __func__);
-
- grp = container_of(kobj, struct mcidev_sysfs_group_kobj, kobj);
- mci = grp->mci;
-}
-
-/* Intermediate show/store table */
-static struct sysfs_ops inst_grp_ops = {
- .show = inst_grp_show,
- .store = inst_grp_store
+static struct attribute_group mci_attr_grp = {
+ .attrs = mci_attrs,
};
-/* the kobj_type instance for a instance group */
-static struct kobj_type ktype_inst_grp = {
- .release = edac_inst_grp_release,
- .sysfs_ops = &inst_grp_ops,
+static const struct attribute_group *mci_attr_groups[] = {
+ &mci_attr_grp,
+ NULL
};
-
-/*
- * edac_create_mci_instance_attributes
- * create MC driver specific attributes bellow an specified kobj
- * This routine calls itself recursively, in order to create an entire
- * object tree.
- */
-static int edac_create_mci_instance_attributes(struct mem_ctl_info *mci,
- const struct mcidev_sysfs_attribute *sysfs_attrib,
- struct kobject *kobj)
+static void mci_attr_release(struct device *device)
{
- int err;
-
- debugf4("%s()\n", __func__);
-
- while (sysfs_attrib) {
- debugf4("%s() sysfs_attrib = %p\n",__func__, sysfs_attrib);
- if (sysfs_attrib->grp) {
- struct mcidev_sysfs_group_kobj *grp_kobj;
-
- grp_kobj = kzalloc(sizeof(*grp_kobj), GFP_KERNEL);
- if (!grp_kobj)
- return -ENOMEM;
-
- grp_kobj->grp = sysfs_attrib->grp;
- grp_kobj->mci = mci;
- list_add_tail(&grp_kobj->list, &mci->grp_kobj_list);
-
- debugf0("%s() grp %s, mci %p\n", __func__,
- sysfs_attrib->grp->name, mci);
-
- err = kobject_init_and_add(&grp_kobj->kobj,
- &ktype_inst_grp,
- &mci->edac_mci_kobj,
- sysfs_attrib->grp->name);
- if (err < 0) {
- printk(KERN_ERR "kobject_init_and_add failed: %d\n", err);
- return err;
- }
- err = edac_create_mci_instance_attributes(mci,
- grp_kobj->grp->mcidev_attr,
- &grp_kobj->kobj);
-
- if (err < 0)
- return err;
- } else if (sysfs_attrib->attr.name) {
- debugf4("%s() file %s\n", __func__,
- sysfs_attrib->attr.name);
-
- err = sysfs_create_file(kobj, &sysfs_attrib->attr);
- if (err < 0) {
- printk(KERN_ERR "sysfs_create_file failed: %d\n", err);
- return err;
- }
- } else
- break;
-
- sysfs_attrib++;
- }
-
- return 0;
+ debugf1("Releasing mci device %s\n", dev_name(device));
}
-/*
- * edac_remove_mci_instance_attributes
- * remove MC driver specific attributes at the topmost level
- * directory of this mci instance.
- */
-static void edac_remove_mci_instance_attributes(struct mem_ctl_info *mci,
- const struct mcidev_sysfs_attribute *sysfs_attrib,
- struct kobject *kobj, int count)
-{
- struct mcidev_sysfs_group_kobj *grp_kobj, *tmp;
-
- debugf1("%s()\n", __func__);
-
- /*
- * loop if there are attributes and until we hit a NULL entry
- * Remove first all the attributes
- */
- while (sysfs_attrib) {
- debugf4("%s() sysfs_attrib = %p\n",__func__, sysfs_attrib);
- if (sysfs_attrib->grp) {
- debugf4("%s() seeking for group %s\n",
- __func__, sysfs_attrib->grp->name);
- list_for_each_entry(grp_kobj,
- &mci->grp_kobj_list, list) {
- debugf4("%s() grp_kobj->grp = %p\n",__func__, grp_kobj->grp);
- if (grp_kobj->grp == sysfs_attrib->grp) {
- edac_remove_mci_instance_attributes(mci,
- grp_kobj->grp->mcidev_attr,
- &grp_kobj->kobj, count + 1);
- debugf4("%s() group %s\n", __func__,
- sysfs_attrib->grp->name);
- kobject_put(&grp_kobj->kobj);
- }
- }
- debugf4("%s() end of seeking for group %s\n",
- __func__, sysfs_attrib->grp->name);
- } else if (sysfs_attrib->attr.name) {
- debugf4("%s() file %s\n", __func__,
- sysfs_attrib->attr.name);
- sysfs_remove_file(kobj, &sysfs_attrib->attr);
- } else
- break;
- sysfs_attrib++;
- }
-
- /* Remove the group objects */
- if (count)
- return;
- list_for_each_entry_safe(grp_kobj, tmp,
- &mci->grp_kobj_list, list) {
- list_del(&grp_kobj->list);
- kfree(grp_kobj);
- }
-}
+static struct device_type mci_attr_type = {
+ .groups = mci_attr_groups,
+ .release = mci_attr_release,
+};
/*
@@ -914,81 +674,65 @@ static void edac_remove_mci_instance_attributes(struct mem_ctl_info *mci,
*/
int edac_create_sysfs_mci_device(struct mem_ctl_info *mci)
{
- int i, j;
- int err;
- struct csrow_info *csrow;
- struct kobject *kobj_mci = &mci->edac_mci_kobj;
+ int i, err;
debugf0("%s() idx=%d\n", __func__, mci->mc_idx);
- INIT_LIST_HEAD(&mci->grp_kobj_list);
-
- /* create a symlink for the device */
- err = sysfs_create_link(kobj_mci, &mci->pdev->kobj,
- EDAC_DEVICE_SYMLINK);
- if (err) {
- debugf1("%s() failure to create symlink\n", __func__);
- goto fail0;
- }
+ /* get the /sys/devices/system/edac subsys reference */
- /* If the low level driver desires some attributes,
- * then create them now for the driver.
- */
- if (mci->mc_driver_sysfs_attributes) {
- err = edac_create_mci_instance_attributes(mci,
- mci->mc_driver_sysfs_attributes,
- &mci->edac_mci_kobj);
- if (err) {
- debugf1("%s() failure to create mci attributes\n",
- __func__);
- goto fail0;
- }
- }
+ mci->dev.type = &mci_attr_type;
+ device_initialize(&mci->dev);
- /* Make directories for each CSROW object under the mc<id> kobject
- */
- for (i = 0; i < mci->num_csrows; i++) {
- int n = 0;
+ mci->dev.parent = &mci_pdev;
+ mci->dev.bus = mci_pdev.bus;
+ dev_set_name(&mci->dev, "mc%d", mci->mc_idx);
+ dev_set_drvdata(&mci->dev, mci);
+ pm_runtime_forbid(&mci->dev);
- csrow = &mci->csrows[i];
- for (j = 0; j < csrow->nr_channels; j++) {
- struct dimm_info *dimm = csrow->channels[j].dimm;
- n += dimm->nr_pages;
- }
+ debugf0("%s(): creating device %s\n", __func__,
+ dev_name(&mci->dev));
+ err = device_add(&mci->dev);
+ if (err < 0)
+ return err;
- if (n > 0) {
- err = edac_create_csrow_object(mci, csrow, i);
- if (err) {
- debugf1("%s() failure: create csrow %d obj\n",
- __func__, i);
- goto fail1;
- }
+ /*
+ * Create the dimm/rank devices
+ */
+ for (i = 0; i < mci->tot_dimms; i++) {
+ struct dimm_info *dimm = &mci->dimms[i];
+ /* Only expose populated DIMMs */
+ if (dimm->nr_pages == 0)
+ continue;
+#ifdef CONFIG_EDAC_DEBUG
+ debugf1("%s creating dimm%d, located at ",
+ __func__, i);
+ if (edac_debug_level >= 1) {
+ int lay;
+ for (lay = 0; lay < mci->n_layers; lay++)
+ printk(KERN_CONT "%s %d ",
+ edac_layer_name[mci->layers[lay].type],
+ dimm->location[lay]);
+ printk(KERN_CONT "\n");
}
+#endif
}
+ err = edac_create_csrow_objects(mci);
+ if (err < 0)
+ goto fail;
+
return 0;
-fail1:
+fail:
for (i--; i >= 0; i--) {
- int n = 0;
-
- csrow = &mci->csrows[i];
- for (j = 0; j < csrow->nr_channels; j++) {
- struct dimm_info *dimm = csrow->channels[j].dimm;
- n += dimm->nr_pages;
- }
- if (n > 0)
- kobject_put(&mci->csrows[i].kobj);
+ struct dimm_info *dimm = &mci->dimms[i];
+ if (dimm->nr_pages == 0)
+ continue;
+ put_device(&dimm->dev);
+ device_del(&dimm->dev);
}
-
- /* remove the mci instance's attributes, if any */
- edac_remove_mci_instance_attributes(mci,
- mci->mc_driver_sysfs_attributes, &mci->edac_mci_kobj, 0);
-
- /* remove the symlink */
- sysfs_remove_link(kobj_mci, EDAC_DEVICE_SYMLINK);
-
-fail0:
+ put_device(&mci->dev);
+ device_del(&mci->dev);
return err;
}
@@ -997,100 +741,68 @@ fail0:
*/
void edac_remove_sysfs_mci_device(struct mem_ctl_info *mci)
{
- struct csrow_info *csrow;
- int i, j;
+ int i;
debugf0("%s()\n", __func__);
- /* remove all csrow kobjects */
- debugf4("%s() unregister this mci kobj\n", __func__);
- for (i = 0; i < mci->num_csrows; i++) {
- int n = 0;
-
- csrow = &mci->csrows[i];
- for (j = 0; j < csrow->nr_channels; j++) {
- struct dimm_info *dimm = csrow->channels[j].dimm;
- n += dimm->nr_pages;
- }
- if (n > 0) {
- debugf0("%s() unreg csrow-%d\n", __func__, i);
- kobject_put(&mci->csrows[i].kobj);
- }
- }
+ edac_delete_csrow_objects(mci);
- /* remove this mci instance's attribtes */
- if (mci->mc_driver_sysfs_attributes) {
- debugf4("%s() unregister mci private attributes\n", __func__);
- edac_remove_mci_instance_attributes(mci,
- mci->mc_driver_sysfs_attributes,
- &mci->edac_mci_kobj, 0);
+ for (i = 0; i < mci->tot_dimms; i++) {
+ struct dimm_info *dimm = &mci->dimms[i];
+ if (dimm->nr_pages == 0)
+ continue;
+ debugf0("%s(): removing device %s\n", __func__,
+ dev_name(&dimm->dev));
+ put_device(&dimm->dev);
+ device_del(&dimm->dev);
}
-
- /* remove the symlink */
- debugf4("%s() remove_link\n", __func__);
- sysfs_remove_link(&mci->edac_mci_kobj, EDAC_DEVICE_SYMLINK);
-
- /* unregister this instance's kobject */
- debugf4("%s() remove_mci_instance\n", __func__);
- kobject_put(&mci->edac_mci_kobj);
}
+void edac_unregister_sysfs(struct mem_ctl_info *mci)
+{
+ debugf1("Unregistering device %s\n", dev_name(&mci->dev));
+ put_device(&mci->dev);
+ device_del(&mci->dev);
+}
+static void mc_attr_release(struct device *device)
+{
+ debugf1("Releasing device %s\n", dev_name(device));
+}
-
+static struct device_type mc_attr_type = {
+ .release = mc_attr_release,
+};
/*
- * edac_setup_sysfs_mc_kset(void)
- *
- * Initialize the mc_kset for the 'mc' entry
- * This requires creating the top 'mc' directory with a kset
- * and its controls/attributes.
- *
- * To this 'mc' kset, instance 'mci' will be grouped as children.
- *
- * Return: 0 SUCCESS
- * !0 FAILURE error code
+ * Init/exit code for the module. Basically, creates/removes /sys/class/rc
*/
-int edac_sysfs_setup_mc_kset(void)
+int __init edac_mc_sysfs_init(void)
{
- int err = -EINVAL;
struct bus_type *edac_subsys;
-
- debugf1("%s()\n", __func__);
+ int err;
/* get the /sys/devices/system/edac subsys reference */
edac_subsys = edac_get_sysfs_subsys();
if (edac_subsys == NULL) {
- debugf1("%s() no edac_subsys error=%d\n", __func__, err);
- goto fail_out;
+ debugf1("%s() no edac_subsys\n", __func__);
+ return -EINVAL;
}
- /* Init the MC's kobject */
- mc_kset = kset_create_and_add("mc", NULL, &edac_subsys->dev_root->kobj);
- if (!mc_kset) {
- err = -ENOMEM;
- debugf1("%s() Failed to register '.../edac/mc'\n", __func__);
- goto fail_kset;
- }
+ mci_pdev.bus = edac_subsys;
+ mci_pdev.type = &mc_attr_type;
+ device_initialize(&mci_pdev);
+ dev_set_name(&mci_pdev, "mc");
- debugf1("%s() Registered '.../edac/mc' kobject\n", __func__);
+ err = device_add(&mci_pdev);
+ if (err < 0)
+ return err;
return 0;
-
-fail_kset:
- edac_put_sysfs_subsys();
-
-fail_out:
- return err;
}
-/*
- * edac_sysfs_teardown_mc_kset
- *
- * deconstruct the mc_ket for memory controllers
- */
-void edac_sysfs_teardown_mc_kset(void)
+void __exit edac_mc_sysfs_exit(void)
{
- kset_unregister(mc_kset);
+ put_device(&mci_pdev);
+ device_del(&mci_pdev);
edac_put_sysfs_subsys();
}
-
diff --git a/drivers/edac/edac_module.c b/drivers/edac/edac_module.c
index 5ddaa86..8735a0d 100644
--- a/drivers/edac/edac_module.c
+++ b/drivers/edac/edac_module.c
@@ -90,10 +90,7 @@ static int __init edac_init(void)
*/
edac_pci_clear_parity_errors();
- /*
- * now set up the mc_kset under the edac class object
- */
- err = edac_sysfs_setup_mc_kset();
+ err = edac_mc_sysfs_init();
if (err)
goto error;
@@ -101,15 +98,11 @@ static int __init edac_init(void)
err = edac_workqueue_setup();
if (err) {
edac_printk(KERN_ERR, EDAC_MC, "init WorkQueue failure\n");
- goto workq_fail;
+ goto error;
}
return 0;
- /* Error teardown stack */
-workq_fail:
- edac_sysfs_teardown_mc_kset();
-
error:
return err;
}
@@ -124,7 +117,7 @@ static void __exit edac_exit(void)
/* tear down the various subsystems */
edac_workqueue_teardown();
- edac_sysfs_teardown_mc_kset();
+ edac_mc_sysfs_exit();
}
/*
diff --git a/drivers/edac/edac_module.h b/drivers/edac/edac_module.h
index 0be4b01..2934e5e 100644
--- a/drivers/edac/edac_module.h
+++ b/drivers/edac/edac_module.h
@@ -19,12 +19,12 @@
*
* edac_mc objects
*/
-extern int edac_sysfs_setup_mc_kset(void);
-extern void edac_sysfs_teardown_mc_kset(void);
-extern int edac_mc_register_sysfs_main_kobj(struct mem_ctl_info *mci);
-extern void edac_mc_unregister_sysfs_main_kobj(struct mem_ctl_info *mci);
+ /* on edac_mc_sysfs.c */
+int edac_mc_sysfs_init(void);
+void edac_mc_sysfs_exit(void);
extern int edac_create_sysfs_mci_device(struct mem_ctl_info *mci);
extern void edac_remove_sysfs_mci_device(struct mem_ctl_info *mci);
+void edac_unregister_sysfs(struct mem_ctl_info *mci);
extern int edac_get_log_ue(void);
extern int edac_get_log_ce(void);
extern int edac_get_panic_on_ue(void);
@@ -34,6 +34,7 @@ extern int edac_mc_get_panic_on_ue(void);
extern int edac_get_poll_msec(void);
extern int edac_mc_get_poll_msec(void);
+ /* on edac_device.c */
extern int edac_device_register_sysfs_main_kobj(
struct edac_device_ctl_info *edac_dev);
extern void edac_device_unregister_sysfs_main_kobj(
diff --git a/include/linux/edac.h b/include/linux/edac.h
index 21f37f7..fb1c6e4 100644
--- a/include/linux/edac.h
+++ b/include/linux/edac.h
@@ -422,14 +422,15 @@ struct edac_mc_layer {
__p; \
})
-
-/* FIXME: add the proper per-location error counts */
struct dimm_info {
+ struct device dev;
+
char label[EDAC_MC_LABEL_LEN + 1]; /* DIMM label on motherboard */
/* Memory location data */
unsigned location[EDAC_MAX_LAYERS];
+ struct kobject kobj; /* sysfs kobject for this csrow */
struct mem_ctl_info *mci; /* the parent */
u32 grain; /* granularity of reported error in bytes */
@@ -458,6 +459,8 @@ struct dimm_info {
* patches in this series will fix this issue.
*/
struct rank_info {
+ struct device dev;
+
int chan_idx;
struct csrow_info *csrow;
struct dimm_info *dimm;
@@ -466,6 +469,8 @@ struct rank_info {
};
struct csrow_info {
+ struct device dev;
+
int csrow_idx; /* the chip-select row */
/* Used only by edac_mc_find_csrow_by_page() */
@@ -491,15 +496,6 @@ struct mcidev_sysfs_group {
const struct mcidev_sysfs_attribute *mcidev_attr; /* group attributes */
};
-struct mcidev_sysfs_group_kobj {
- struct list_head list; /* list for all instances within a mc */
-
- struct kobject kobj; /* kobj for the group */
-
- const struct mcidev_sysfs_group *grp; /* group description table */
- struct mem_ctl_info *mci; /* the parent */
-};
-
/* mcidev_sysfs_attribute structure
* used for driver sysfs attributes and in mem_ctl_info
* sysfs top level entries
@@ -510,8 +506,19 @@ struct mcidev_sysfs_attribute {
const struct mcidev_sysfs_group *grp; /* Points to a group of attributes */
/* Ops for show/store values at the attribute - not used on group */
- ssize_t (*show)(struct mem_ctl_info *,char *);
- ssize_t (*store)(struct mem_ctl_info *, const char *,size_t);
+ ssize_t (*show)(struct mem_ctl_info *, char *);
+ ssize_t (*store)(struct mem_ctl_info *, const char *, size_t);
+
+ void *priv;
+};
+
+/*
+ * struct errcount_attribute - used to store the several error counts
+ */
+struct errcount_attribute_data {
+ int n_layers;
+ int pos[EDAC_MAX_LAYERS];
+ int layer0, layer1, layer2;
};
struct edac_hierarchy {
@@ -522,6 +529,8 @@ struct edac_hierarchy {
/* MEMORY controller information structure
*/
struct mem_ctl_info {
+ struct device dev;
+
struct list_head link; /* for global list of mem_ctl_info structs */
struct module *owner; /* Module owner of this control struct */
@@ -566,9 +575,19 @@ struct mem_ctl_info {
struct csrow_info *csrows;
unsigned num_csrows, num_cschannel;
- /* Memory Controller hierarchy */
+ /*
+ * Memory Controller hierarchy
+ *
+ * There are basically two types of memory controller: the ones that
+ * sees memory sticks ("dimms"), and the ones that sees memory ranks.
+ * All old memory controllers enumerate memories per rank, but most
+ * of the recent drivers enumerate memories per DIMM, instead.
+ * When the memory controller is per rank, mem_is_per_rank is true.
+ */
unsigned n_layers;
struct edac_mc_layer *layers;
+ bool mem_is_per_rank;
+
/*
* DIMM info. Will eventually remove the entire csrows_info some day
*/
--
1.7.8
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH 01/14] edac: rewrite the sysfs code to use struct device
2012-03-29 17:06 ` [PATCH 01/14] edac: rewrite the sysfs code to use struct device Mauro Carvalho Chehab
@ 2012-03-29 22:03 ` Greg K H
2012-03-29 23:19 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 26+ messages in thread
From: Greg K H @ 2012-03-29 22:03 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: Linux Edac Mailing List, Linux Kernel Mailing List
On Thu, Mar 29, 2012 at 02:06:48PM -0300, Mauro Carvalho Chehab wrote:
> +static void mci_attr_release(struct device *device)
> {
> - int err;
> -
> - debugf4("%s()\n", __func__);
> -
> - while (sysfs_attrib) {
> - debugf4("%s() sysfs_attrib = %p\n",__func__, sysfs_attrib);
> - if (sysfs_attrib->grp) {
> - struct mcidev_sysfs_group_kobj *grp_kobj;
> -
> - grp_kobj = kzalloc(sizeof(*grp_kobj), GFP_KERNEL);
> - if (!grp_kobj)
> - return -ENOMEM;
> -
> - grp_kobj->grp = sysfs_attrib->grp;
> - grp_kobj->mci = mci;
> - list_add_tail(&grp_kobj->list, &mci->grp_kobj_list);
> -
> - debugf0("%s() grp %s, mci %p\n", __func__,
> - sysfs_attrib->grp->name, mci);
> -
> - err = kobject_init_and_add(&grp_kobj->kobj,
> - &ktype_inst_grp,
> - &mci->edac_mci_kobj,
> - sysfs_attrib->grp->name);
> - if (err < 0) {
> - printk(KERN_ERR "kobject_init_and_add failed: %d\n", err);
> - return err;
> - }
> - err = edac_create_mci_instance_attributes(mci,
> - grp_kobj->grp->mcidev_attr,
> - &grp_kobj->kobj);
> -
> - if (err < 0)
> - return err;
> - } else if (sysfs_attrib->attr.name) {
> - debugf4("%s() file %s\n", __func__,
> - sysfs_attrib->attr.name);
> -
> - err = sysfs_create_file(kobj, &sysfs_attrib->attr);
> - if (err < 0) {
> - printk(KERN_ERR "sysfs_create_file failed: %d\n", err);
> - return err;
> - }
> - } else
> - break;
> -
> - sysfs_attrib++;
> - }
> -
> - return 0;
> + debugf1("Releasing mci device %s\n", dev_name(device));
> }
Sweet, as per the documentation in the Documentation/kobjects.txt file,
I get to publically mock you for thinking you are smarter than the
kernel and this is an acceptable way to "outwhit" the driver core from
spitting errors at you when the kobject is released.
Please fix this up, it's unacceptable.
> +static void mc_attr_release(struct device *device)
> +{
> + debugf1("Releasing device %s\n", dev_name(device));
> +}
Hey, you did it again, not nice :(
You also just leaked memory as well :(
Please fix.
greg k-h
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 01/14] edac: rewrite the sysfs code to use struct device
2012-03-29 22:03 ` Greg K H
@ 2012-03-29 23:19 ` Mauro Carvalho Chehab
2012-03-29 23:40 ` Greg K H
0 siblings, 1 reply; 26+ messages in thread
From: Mauro Carvalho Chehab @ 2012-03-29 23:19 UTC (permalink / raw)
To: Greg K H; +Cc: Linux Edac Mailing List, Linux Kernel Mailing List
Em 29-03-2012 19:03, Greg K H escreveu:
> On Thu, Mar 29, 2012 at 02:06:48PM -0300, Mauro Carvalho Chehab wrote:
>> +static void mci_attr_release(struct device *device)
>> {
>> - int err;
>> -
>> - debugf4("%s()\n", __func__);
>> -
>> - while (sysfs_attrib) {
>> - debugf4("%s() sysfs_attrib = %p\n",__func__, sysfs_attrib);
>> - if (sysfs_attrib->grp) {
>> - struct mcidev_sysfs_group_kobj *grp_kobj;
>> -
>> - grp_kobj = kzalloc(sizeof(*grp_kobj), GFP_KERNEL);
>> - if (!grp_kobj)
>> - return -ENOMEM;
>> -
>> - grp_kobj->grp = sysfs_attrib->grp;
>> - grp_kobj->mci = mci;
>> - list_add_tail(&grp_kobj->list, &mci->grp_kobj_list);
>> -
>> - debugf0("%s() grp %s, mci %p\n", __func__,
>> - sysfs_attrib->grp->name, mci);
>> -
>> - err = kobject_init_and_add(&grp_kobj->kobj,
>> - &ktype_inst_grp,
>> - &mci->edac_mci_kobj,
>> - sysfs_attrib->grp->name);
>> - if (err < 0) {
>> - printk(KERN_ERR "kobject_init_and_add failed: %d\n", err);
>> - return err;
>> - }
>> - err = edac_create_mci_instance_attributes(mci,
>> - grp_kobj->grp->mcidev_attr,
>> - &grp_kobj->kobj);
>> -
>> - if (err < 0)
>> - return err;
>> - } else if (sysfs_attrib->attr.name) {
>> - debugf4("%s() file %s\n", __func__,
>> - sysfs_attrib->attr.name);
>> -
>> - err = sysfs_create_file(kobj, &sysfs_attrib->attr);
>> - if (err < 0) {
>> - printk(KERN_ERR "sysfs_create_file failed: %d\n", err);
>> - return err;
>> - }
>> - } else
>> - break;
>> -
>> - sysfs_attrib++;
>> - }
>> -
>> - return 0;
>> + debugf1("Releasing mci device %s\n", dev_name(device));
>> }
>
> Sweet, as per the documentation in the Documentation/kobjects.txt file,
> I get to publically mock you for thinking you are smarter than the
> kernel and this is an acceptable way to "outwhit" the driver core from
> spitting errors at you when the kobject is released.
There's nothing there to free: all EDAC structures are allocated once
(see edac_mc_alloc() and edac_align_ptr() logic, at drivers/edac/edac_mc.c).
Even the struct device for all csrows/channels/mcu is done on a single alloc
there. Trying to free it earlier would cause a segfault.
I didn't wrote that logic, nor I was tempted to change it: as this subsystem
is focused on memory error detection, having every data structure used there
on a single page helps to minimize the probability of having an error at the
memory used to store the EDAC data.
>
> Please fix this up, it's unacceptable.
>
>> +static void mc_attr_release(struct device *device)
>> +{
>> + debugf1("Releasing device %s\n", dev_name(device));
>> +}
>
> Hey, you did it again, not nice :(
>
> You also just leaked memory as well :(
There's no memory leak. The allocated memory is freed at edac_mc_del_mc()
(also at drivers/edac/edac_mc.c).
>
> Please fix.
>
> greg k-h
> --
> To unsubscribe from this list: send the line "unsubscribe linux-edac" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 01/14] edac: rewrite the sysfs code to use struct device
2012-03-29 23:19 ` Mauro Carvalho Chehab
@ 2012-03-29 23:40 ` Greg K H
2012-03-30 2:13 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 26+ messages in thread
From: Greg K H @ 2012-03-29 23:40 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: Linux Edac Mailing List, Linux Kernel Mailing List
On Thu, Mar 29, 2012 at 08:19:08PM -0300, Mauro Carvalho Chehab wrote:
> > Sweet, as per the documentation in the Documentation/kobjects.txt file,
> > I get to publically mock you for thinking you are smarter than the
> > kernel and this is an acceptable way to "outwhit" the driver core from
> > spitting errors at you when the kobject is released.
>
> There's nothing there to free: all EDAC structures are allocated once
> (see edac_mc_alloc() and edac_align_ptr() logic, at drivers/edac/edac_mc.c).
>
> Even the struct device for all csrows/channels/mcu is done on a single alloc
> there. Trying to free it earlier would cause a segfault.
That's wrong then, these are multiple struct devices, all with their own
reference counts, you can't just treat them all as the same thing, even
if it happens to line up with the module reference count.
> I didn't wrote that logic, nor I was tempted to change it: as this subsystem
> is focused on memory error detection, having every data structure used there
> on a single page helps to minimize the probability of having an error at the
> memory used to store the EDAC data.
Possibly, but again, you have multiple reference counts, you can't just
wave them off as being inconvenient. Please read the documentation for
more details why.
greg k-h
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 01/14] edac: rewrite the sysfs code to use struct device
2012-03-29 23:40 ` Greg K H
@ 2012-03-30 2:13 ` Mauro Carvalho Chehab
2012-03-30 9:11 ` Borislav Petkov
2012-03-30 15:31 ` Greg K H
0 siblings, 2 replies; 26+ messages in thread
From: Mauro Carvalho Chehab @ 2012-03-30 2:13 UTC (permalink / raw)
To: Greg K H; +Cc: Linux Edac Mailing List, Linux Kernel Mailing List
Em 29-03-2012 20:40, Greg K H escreveu:
> On Thu, Mar 29, 2012 at 08:19:08PM -0300, Mauro Carvalho Chehab wrote:
>>> Sweet, as per the documentation in the Documentation/kobjects.txt file,
>>> I get to publically mock you for thinking you are smarter than the
>>> kernel and this is an acceptable way to "outwhit" the driver core from
>>> spitting errors at you when the kobject is released.
>>
>> There's nothing there to free: all EDAC structures are allocated once
>> (see edac_mc_alloc() and edac_align_ptr() logic, at drivers/edac/edac_mc.c).
>>
>> Even the struct device for all csrows/channels/mcu is done on a single alloc
>> there. Trying to free it earlier would cause a segfault.
>
> That's wrong then, these are multiple struct devices, all with their own
> reference counts, you can't just treat them all as the same thing, even
> if it happens to line up with the module reference count.
>
>> I didn't wrote that logic, nor I was tempted to change it: as this subsystem
>> is focused on memory error detection, having every data structure used there
>> on a single page helps to minimize the probability of having an error at the
>> memory used to store the EDAC data.
>
> Possibly, but again, you have multiple reference counts, you can't just
> wave them off as being inconvenient. Please read the documentation for
> more details why.
This is there since the beginning. The current kobj's have this issue. Those
patchsets are not making it better or worse, as the EDAC csrow kobj's are
already there at the current approach: all of them are allocated together
with the mci kobj.
On the other hand, I'm working on this patch series in order to correct a
serious bug at the EDAC API almost all days during the last 2 months, as
nobody ever cared enough to address this serious issue.
However, every time this patch series is submitted, someone come up with a
bright idea to ask me to add more work to the scope, delaying its addition
forever.
While I'm not convinced that moving from a single memory allocation into a
series of k*alloc is a good thing for a subsystem that is there to detect
memory errors (as having everything altogether into a single page can
reduce the chances of errors at the EDAC data), I can work latter on a
patchset to fix this issue for EDAC MC, but I'll do it only after merging
this series, as it is counter-productive to do it otherwise, having to
repeat the same set of tests on 10 machines (and compile the entire series
of patches on 8 different archs/sub-archs).
So, I really want to move this ahead. So, please, first things first: let's
first fix the more serious bug. Then, we can fix the other minor stuff
that aren't so far causing any noticeable harm.
Regards,
Mauro
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 01/14] edac: rewrite the sysfs code to use struct device
2012-03-30 2:13 ` Mauro Carvalho Chehab
@ 2012-03-30 9:11 ` Borislav Petkov
2012-03-30 10:46 ` Mauro Carvalho Chehab
2012-03-30 15:31 ` Greg K H
1 sibling, 1 reply; 26+ messages in thread
From: Borislav Petkov @ 2012-03-30 9:11 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Greg K H, Linux Edac Mailing List, Linux Kernel Mailing List
On Thu, Mar 29, 2012 at 11:13:07PM -0300, Mauro Carvalho Chehab wrote:
> However, every time this patch series is submitted, someone come up with a
> bright idea to ask me to add more work to the scope, delaying its addition
> forever.
>
> While I'm not convinced that moving from a single memory allocation into a
> series of k*alloc is a good thing for a subsystem that is there to detect
> memory errors (as having everything altogether into a single page can
> reduce the chances of errors at the EDAC data), I can work latter on a
> patchset to fix this issue for EDAC MC, but I'll do it only after merging
> this series, as it is counter-productive to do it otherwise, having to
> repeat the same set of tests on 10 machines (and compile the entire series
> of patches on 8 different archs/sub-archs).
>
> So, I really want to move this ahead. So, please, first things first: let's
> first fix the more serious bug. Then, we can fix the other minor stuff
> that aren't so far causing any noticeable harm.
Dude, stop complaining - this is the kernel not some pet project of
yours. You either do things right or you don't do them at all. Others
have to do the same iterations with patches and intergrate maintainer
change requests until everything is done properly.
Btw, this patch is
5 files changed, 432 insertions(+), 717 deletions(-)
It is 1500+ lines and huuuuge! How do you think anyone can review this?
Also, I told already: if you wanna fix one thing, then fix it with a
smaller patchset which easier to review by people instead of throwing at
them humongous patch bombs which are supposed to fix _everything_ and
expecting everyone to understand immediately what you mean. And don't
tell me these huge patches cannot be split, I'm not buying it.
--
Regards/Gruss,
Boris.
Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
GM: Alberto Bozzo
Reg: Dornach, Landkreis Muenchen
HRB Nr. 43632 WEEE Registernr: 129 19551
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 01/14] edac: rewrite the sysfs code to use struct device
2012-03-30 9:11 ` Borislav Petkov
@ 2012-03-30 10:46 ` Mauro Carvalho Chehab
2012-03-30 15:32 ` Greg K H
0 siblings, 1 reply; 26+ messages in thread
From: Mauro Carvalho Chehab @ 2012-03-30 10:46 UTC (permalink / raw)
To: Borislav Petkov
Cc: Greg K H, Linux Edac Mailing List, Linux Kernel Mailing List
Em 30-03-2012 06:11, Borislav Petkov escreveu:
> On Thu, Mar 29, 2012 at 11:13:07PM -0300, Mauro Carvalho Chehab wrote:
>> However, every time this patch series is submitted, someone come up with a
>> bright idea to ask me to add more work to the scope, delaying its addition
>> forever.
>>
>> While I'm not convinced that moving from a single memory allocation into a
>> series of k*alloc is a good thing for a subsystem that is there to detect
>> memory errors (as having everything altogether into a single page can
>> reduce the chances of errors at the EDAC data), I can work latter on a
>> patchset to fix this issue for EDAC MC, but I'll do it only after merging
>> this series, as it is counter-productive to do it otherwise, having to
>> repeat the same set of tests on 10 machines (and compile the entire series
>> of patches on 8 different archs/sub-archs).
>>
>> So, I really want to move this ahead. So, please, first things first: let's
>> first fix the more serious bug. Then, we can fix the other minor stuff
>> that aren't so far causing any noticeable harm.
>
> Dude, stop complaining - this is the kernel not some pet project of
> yours. You either do things right or you don't do them at all. Others
> have to do the same iterations with patches and intergrate maintainer
> change requests until everything is done properly.
>
> Btw, this patch is
>
> 5 files changed, 432 insertions(+), 717 deletions(-)
>
> It is 1500+ lines and huuuuge! How do you think anyone can review this?
If you consider this a big patch, you can imagine how bigger it will be if
it would have there the re-write the edac_mc_alloc() function, in order to break
it into a per-struct-device function, likely patching all drivers/edac/*.c
to use the new way.
As I said: merging the allocation fix on this patch is a very bad idea:
it should be a separate changeset, applied after this one, as the
subsequent changesets simplify the sysfs logic, helping to write a changeset
to fix the kobject issue.
Applying it before would just do a lot of changes on some code that will
be dropped by this series, making harder for busy reviewers to inspect
the changes.
So, as I said, the way to move is to apply this changeset, and then to
go ahead and cleanup the potential problem [1] of having multiple kobj
references for the same memory block.
[1] I never saw any bugzilla complaining about an EDAC failure due to the
usage of multiple kobjects at the same memory block. The reason is probably
because, in practice, once this module is loaded to monitor the memory errors,
this module is never unloaded. Also, module unload/reload works, before
and after this changeset. So, AFAIKT, nobody ever noticed this existing
bug before yesterday.
> Also, I told already: if you wanna fix one thing, then fix it with a
> smaller patchset which easier to review by people instead of throwing at
> them humongous patch bombs which are supposed to fix _everything_ and
> expecting everyone to understand immediately what you mean. And don't
> tell me these huge patches cannot be split, I'm not buying it.
This patch does the absolute minimum stuff to replace kobj by struct device.
Nothing more, nothing less.
I took the care to put all needed driver changes and API changes
on separate patchsets.
The edac_mc_sysfs.c file has just one thing there: the sysfs logic, based
on kobj raw allocation.
Replacing it by struct device means to rewrite the entire code. Period.
Breaking it into smaller pieces would break git bisect, and will make it
even harder for reviewers, as this atomic change unit would be broken
into several patches.
Btw, both Greg and Aris reviewed it yesterday. So, it seems that this is
not as complex as you think.
Mauro.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 01/14] edac: rewrite the sysfs code to use struct device
2012-03-30 10:46 ` Mauro Carvalho Chehab
@ 2012-03-30 15:32 ` Greg K H
0 siblings, 0 replies; 26+ messages in thread
From: Greg K H @ 2012-03-30 15:32 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Borislav Petkov, Linux Edac Mailing List,
Linux Kernel Mailing List
On Fri, Mar 30, 2012 at 07:46:14AM -0300, Mauro Carvalho Chehab wrote:
> Em 30-03-2012 06:11, Borislav Petkov escreveu:
> > On Thu, Mar 29, 2012 at 11:13:07PM -0300, Mauro Carvalho Chehab wrote:
> >> However, every time this patch series is submitted, someone come up with a
> >> bright idea to ask me to add more work to the scope, delaying its addition
> >> forever.
> >>
> >> While I'm not convinced that moving from a single memory allocation into a
> >> series of k*alloc is a good thing for a subsystem that is there to detect
> >> memory errors (as having everything altogether into a single page can
> >> reduce the chances of errors at the EDAC data), I can work latter on a
> >> patchset to fix this issue for EDAC MC, but I'll do it only after merging
> >> this series, as it is counter-productive to do it otherwise, having to
> >> repeat the same set of tests on 10 machines (and compile the entire series
> >> of patches on 8 different archs/sub-archs).
> >>
> >> So, I really want to move this ahead. So, please, first things first: let's
> >> first fix the more serious bug. Then, we can fix the other minor stuff
> >> that aren't so far causing any noticeable harm.
> >
> > Dude, stop complaining - this is the kernel not some pet project of
> > yours. You either do things right or you don't do them at all. Others
> > have to do the same iterations with patches and intergrate maintainer
> > change requests until everything is done properly.
> >
> > Btw, this patch is
> >
> > 5 files changed, 432 insertions(+), 717 deletions(-)
> >
> > It is 1500+ lines and huuuuge! How do you think anyone can review this?
>
> If you consider this a big patch, you can imagine how bigger it will be if
> it would have there the re-write the edac_mc_alloc() function, in order to break
> it into a per-struct-device function, likely patching all drivers/edac/*.c
> to use the new way.
>
> As I said: merging the allocation fix on this patch is a very bad idea:
> it should be a separate changeset, applied after this one, as the
> subsequent changesets simplify the sysfs logic, helping to write a changeset
> to fix the kobject issue.
>
> Applying it before would just do a lot of changes on some code that will
> be dropped by this series, making harder for busy reviewers to inspect
> the changes.
>
> So, as I said, the way to move is to apply this changeset, and then to
> go ahead and cleanup the potential problem [1] of having multiple kobj
> references for the same memory block.
>
> [1] I never saw any bugzilla complaining about an EDAC failure due to the
> usage of multiple kobjects at the same memory block. The reason is probably
> because, in practice, once this module is loaded to monitor the memory errors,
> this module is never unloaded. Also, module unload/reload works, before
> and after this changeset. So, AFAIKT, nobody ever noticed this existing
> bug before yesterday.
>
> > Also, I told already: if you wanna fix one thing, then fix it with a
> > smaller patchset which easier to review by people instead of throwing at
> > them humongous patch bombs which are supposed to fix _everything_ and
> > expecting everyone to understand immediately what you mean. And don't
> > tell me these huge patches cannot be split, I'm not buying it.
>
> This patch does the absolute minimum stuff to replace kobj by struct device.
> Nothing more, nothing less.
>
> I took the care to put all needed driver changes and API changes
> on separate patchsets.
>
> The edac_mc_sysfs.c file has just one thing there: the sysfs logic, based
> on kobj raw allocation.
>
> Replacing it by struct device means to rewrite the entire code. Period.
>
> Breaking it into smaller pieces would break git bisect, and will make it
> even harder for reviewers, as this atomic change unit would be broken
> into several patches.
>
> Btw, both Greg and Aris reviewed it yesterday. So, it seems that this is
> not as complex as you think.
No, it's very complex, I just searched for the most obvious problem that
people do with using 'struct device' and reported on that problem. I
didn't look at anything else.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 01/14] edac: rewrite the sysfs code to use struct device
2012-03-30 2:13 ` Mauro Carvalho Chehab
2012-03-30 9:11 ` Borislav Petkov
@ 2012-03-30 15:31 ` Greg K H
2012-03-30 17:34 ` Mauro Carvalho Chehab
1 sibling, 1 reply; 26+ messages in thread
From: Greg K H @ 2012-03-30 15:31 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: Linux Edac Mailing List, Linux Kernel Mailing List
On Thu, Mar 29, 2012 at 11:13:07PM -0300, Mauro Carvalho Chehab wrote:
> Em 29-03-2012 20:40, Greg K H escreveu:
> > On Thu, Mar 29, 2012 at 08:19:08PM -0300, Mauro Carvalho Chehab wrote:
> >>> Sweet, as per the documentation in the Documentation/kobjects.txt file,
> >>> I get to publically mock you for thinking you are smarter than the
> >>> kernel and this is an acceptable way to "outwhit" the driver core from
> >>> spitting errors at you when the kobject is released.
> >>
> >> There's nothing there to free: all EDAC structures are allocated once
> >> (see edac_mc_alloc() and edac_align_ptr() logic, at drivers/edac/edac_mc.c).
> >>
> >> Even the struct device for all csrows/channels/mcu is done on a single alloc
> >> there. Trying to free it earlier would cause a segfault.
> >
> > That's wrong then, these are multiple struct devices, all with their own
> > reference counts, you can't just treat them all as the same thing, even
> > if it happens to line up with the module reference count.
> >
> >> I didn't wrote that logic, nor I was tempted to change it: as this subsystem
> >> is focused on memory error detection, having every data structure used there
> >> on a single page helps to minimize the probability of having an error at the
> >> memory used to store the EDAC data.
> >
> > Possibly, but again, you have multiple reference counts, you can't just
> > wave them off as being inconvenient. Please read the documentation for
> > more details why.
>
> This is there since the beginning. The current kobj's have this issue. Those
> patchsets are not making it better or worse, as the EDAC csrow kobj's are
> already there at the current approach: all of them are allocated together
> with the mci kobj.
>
> On the other hand, I'm working on this patch series in order to correct a
> serious bug at the EDAC API almost all days during the last 2 months, as
> nobody ever cared enough to address this serious issue.
That's because no one had touched the code. Now that it has an active
developer (i.e. you), it needs to be fixed.
Especially as the code you just added is wrong, and it's documented as
wrong, and you know it is wrong as you are working around the kernel
warnings by providing release functions that do nothing.
So please fix it, it's your code now.
greg k-h
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 01/14] edac: rewrite the sysfs code to use struct device
2012-03-30 15:31 ` Greg K H
@ 2012-03-30 17:34 ` Mauro Carvalho Chehab
2012-03-30 19:24 ` Greg K H
0 siblings, 1 reply; 26+ messages in thread
From: Mauro Carvalho Chehab @ 2012-03-30 17:34 UTC (permalink / raw)
To: Greg K H; +Cc: Linux Edac Mailing List, Linux Kernel Mailing List
Em 30-03-2012 12:31, Greg K H escreveu:
> On Thu, Mar 29, 2012 at 11:13:07PM -0300, Mauro Carvalho Chehab wrote:
>> Em 29-03-2012 20:40, Greg K H escreveu:
>>> On Thu, Mar 29, 2012 at 08:19:08PM -0300, Mauro Carvalho Chehab wrote:
>>>>> Sweet, as per the documentation in the Documentation/kobjects.txt file,
>>>>> I get to publically mock you for thinking you are smarter than the
>>>>> kernel and this is an acceptable way to "outwhit" the driver core from
>>>>> spitting errors at you when the kobject is released.
>>>>
>>>> There's nothing there to free: all EDAC structures are allocated once
>>>> (see edac_mc_alloc() and edac_align_ptr() logic, at drivers/edac/edac_mc.c).
>>>>
>>>> Even the struct device for all csrows/channels/mcu is done on a single alloc
>>>> there. Trying to free it earlier would cause a segfault.
>>>
>>> That's wrong then, these are multiple struct devices, all with their own
>>> reference counts, you can't just treat them all as the same thing, even
>>> if it happens to line up with the module reference count.
>>>
>>>> I didn't wrote that logic, nor I was tempted to change it: as this subsystem
>>>> is focused on memory error detection, having every data structure used there
>>>> on a single page helps to minimize the probability of having an error at the
>>>> memory used to store the EDAC data.
>>>
>>> Possibly, but again, you have multiple reference counts, you can't just
>>> wave them off as being inconvenient. Please read the documentation for
>>> more details why.
>>
>> This is there since the beginning. The current kobj's have this issue. Those
>> patchsets are not making it better or worse, as the EDAC csrow kobj's are
>> already there at the current approach: all of them are allocated together
>> with the mci kobj.
>>
>> On the other hand, I'm working on this patch series in order to correct a
>> serious bug at the EDAC API almost all days during the last 2 months, as
>> nobody ever cared enough to address this serious issue.
>
> That's because no one had touched the code. Now that it has an active
> developer (i.e. you), it needs to be fixed.
>
> Especially as the code you just added is wrong, and it's documented as
> wrong, and you know it is wrong as you are working around the kernel
> warnings by providing release functions that do nothing.
>
> So please fix it, it's your code now.
Please check if this is the right thing to do. It does the EDAC core changes.
The real patch is a way more complex than that, as it required to change the
struct mem_ctrl_info pointers to an array of array, instead of just an array,
with means that all drivers need to be changed. This patch is to be applied at
the end of the patch series.
Regards,
Mauro
[RFC] edac: dynamically allocate one *_info object each time, in order to meet struct device constraints
struct device doen't handle well kobjects that is allocated on
a single k*alloc() call.
PS.: this is just a small part of the patch: all edac drivers should
also change, as the internal API was modified.
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index d4f9336..b1ed014 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -205,14 +205,14 @@ struct mem_ctl_info *edac_mc_alloc(unsigned edac_index,
void *ptr;
struct mem_ctl_info *mci;
struct edac_mc_layer *lay;
- struct csrow_info *csi, *csr;
- struct rank_info *chi, *chp, *chan;
+ struct csrow_info *csr;
+ struct rank_info *chan;
struct dimm_info *dimm;
u32 *ce_per_layer[EDAC_MAX_LAYERS], *ue_per_layer[EDAC_MAX_LAYERS];
void *pvt, *p;
unsigned size, tot_dimms, count, pos[EDAC_MAX_LAYERS];
unsigned tot_csrows, tot_cschannels, tot_errcount = 0;
- int i, j, n, len;
+ int i, j, n, len, off;
int row, chn;
bool per_rank = false;
@@ -243,9 +243,6 @@ struct mem_ctl_info *edac_mc_alloc(unsigned edac_index,
ptr = 0;
mci = edac_align_ptr(&ptr, sizeof(*mci), 1);
lay = edac_align_ptr(&ptr, sizeof(*lay), n_layers);
- csi = edac_align_ptr(&ptr, sizeof(*csi), tot_csrows);
- chi = edac_align_ptr(&ptr, sizeof(*chi), tot_csrows * tot_cschannels);
- dimm = edac_align_ptr(&ptr, sizeof(*dimm), tot_dimms);
count = 1;
for (i = 0; i < n_layers; i++) {
count *= layers[i].size;
@@ -272,9 +269,6 @@ struct mem_ctl_info *edac_mc_alloc(unsigned edac_index,
* rather than an imaginary chunk of memory located at address 0.
*/
lay = (struct edac_mc_layer *)(((char *)mci) + ((unsigned long)lay));
- csi = (struct csrow_info *)(((char *)mci) + ((unsigned long)csi));
- chi = (struct rank_info *)(((char *)mci) + ((unsigned long)chi));
- dimm = (struct dimm_info *)(((char *)mci) + ((unsigned long)dimm));
for (i = 0; i < n_layers; i++) {
mci->ce_per_layer[i] = (u32 *)((char *)mci + ((unsigned long)ce_per_layer[i]));
mci->ue_per_layer[i] = (u32 *)((char *)mci + ((unsigned long)ue_per_layer[i]));
@@ -283,8 +277,6 @@ struct mem_ctl_info *edac_mc_alloc(unsigned edac_index,
/* setup index and various internal pointers */
mci->mc_idx = edac_index;
- mci->csrows = csi;
- mci->dimms = dimm;
mci->tot_dimms = tot_dimms;
mci->pvt_info = pvt;
mci->n_layers = n_layers;
@@ -295,39 +287,55 @@ struct mem_ctl_info *edac_mc_alloc(unsigned edac_index,
mci->mem_is_per_rank = per_rank;
/*
- * Fills the csrow struct
+ * Alocate and fill the csrow/channels structs
*/
+ mci->csrows = kzalloc(sizeof(*mci->csrows) * tot_csrows, GFP_KERNEL);
+ if (!mci->csrows)
+ goto error;
for (row = 0; row < tot_csrows; row++) {
- csr = &csi[row];
+ csr = kzalloc(sizeof(**mci->csrows), GFP_KERNEL);
+ if (!csr)
+ goto error;
+ mci->csrows[row] = csr;
csr->csrow_idx = row;
csr->mci = mci;
csr->nr_channels = tot_cschannels;
- chp = &chi[row * tot_cschannels];
- csr->channels = chp;
+ csr->channels = kzalloc(sizeof(*csr->channels), GFP_KERNEL);
for (chn = 0; chn < tot_cschannels; chn++) {
- chan = &chp[chn];
+ chan = kzalloc(sizeof(**csr->channels), GFP_KERNEL);
+ if (!chan)
+ goto error;
+ csr->channels[chn] = chan;
chan->chan_idx = chn;
chan->csrow = csr;
}
}
/*
- * Fills the dimm struct
+ * Allocate and fill the dimm structs
*/
+ mci->dimms = kzalloc(sizeof(*mci->dimms) * tot_dimms, GFP_KERNEL);
+ if (!mci->dimms)
+ goto error;
+
memset(&pos, 0, sizeof(pos));
row = 0;
chn = 0;
debugf4("%s: initializing %d %s\n", __func__, tot_dimms,
per_rank ? "ranks" : "dimms");
for (i = 0; i < tot_dimms; i++) {
- chan = &csi[row].channels[chn];
- dimm = GET_POS(lay, mci->dimms, n_layers,
- pos[0], pos[1], pos[2]);
+ chan = mci->csrows[row]->channels[chn];
+ off = GET_OFFSET(lay, n_layers, pos[0], pos[1], pos[2]);
+ if (off < 0)
+ goto error;
+
+ dimm = kzalloc(sizeof(**mci->dimms), GFP_KERNEL);
+ mci->dimms[off] = dimm;
dimm->mci = mci;
- debugf2("%s: %d: %s%zd (%d:%d:%d): row %d, chan %d\n", __func__,
- i, per_rank ? "rank" : "dimm", (dimm - mci->dimms),
+ debugf2("%s: %d: %s%i (%d:%d:%d): row %d, chan %d\n", __func__,
+ i, per_rank ? "rank" : "dimm", off,
pos[0], pos[1], pos[2], row, chn);
/*
@@ -393,6 +401,25 @@ struct mem_ctl_info *edac_mc_alloc(unsigned edac_index,
trace_hw_event_init("edac", (unsigned)edac_index);
+error:
+ if (mci->dimms) {
+ for (i = 0; i < tot_dimms; i++)
+ kfree(mci->dimms[i]);
+ kfree(mci->dimms);
+ }
+ if (mci->csrows) {
+ for (chn = 0; chn < tot_cschannels; chn++) {
+ csr = mci->csrows[chn];
+ if (csr) {
+ for (chn = 0; chn < tot_cschannels; chn++)
+ kfree(csr->channels[chn]);
+ kfree(csr);
+ }
+ kfree(mci->csrows[i]);
+ }
+ kfree(mci->csrows);
+ }
+
return mci;
}
EXPORT_SYMBOL_GPL(edac_mc_alloc);
@@ -681,13 +708,12 @@ int edac_mc_add_mc(struct mem_ctl_info *mci)
for (i = 0; i < mci->num_csrows; i++) {
int j;
- edac_mc_dump_csrow(&mci->csrows[i]);
- for (j = 0; j < mci->csrows[i].nr_channels; j++)
- edac_mc_dump_channel(&mci->csrows[i].
- channels[j]);
+ edac_mc_dump_csrow(mci->csrows[i]);
+ for (j = 0; j < mci->csrows[i]->nr_channels; j++)
+ edac_mc_dump_channel(mci->csrows[i]->channels[j]);
}
for (i = 0; i < mci->tot_dimms; i++)
- edac_mc_dump_dimm(&mci->dimms[i]);
+ edac_mc_dump_dimm(mci->dimms[i]);
}
#endif
mutex_lock(&mem_ctls_mutex);
@@ -806,17 +832,17 @@ static void edac_mc_scrub_block(unsigned long page, unsigned long offset,
/* FIXME - should return -1 */
int edac_mc_find_csrow_by_page(struct mem_ctl_info *mci, unsigned long page)
{
- struct csrow_info *csrows = mci->csrows;
+ struct csrow_info **csrows = mci->csrows;
int row, i, j, n;
debugf1("MC%d: %s(): 0x%lx\n", mci->mc_idx, __func__, page);
row = -1;
for (i = 0; i < mci->num_csrows; i++) {
- struct csrow_info *csrow = &csrows[i];
+ struct csrow_info *csrow = csrows[i];
n = 0;
for (j = 0; j < csrow->nr_channels; j++) {
- struct dimm_info *dimm = csrow->channels[j].dimm;
+ struct dimm_info *dimm = csrow->channels[j]->dimm;
n += dimm->nr_pages;
}
if (n == 0)
@@ -969,7 +995,7 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type,
p = label;
*p = '\0';
for (i = 0; i < mci->tot_dimms; i++) {
- struct dimm_info *dimm = &mci->dimms[i];
+ struct dimm_info *dimm = mci->dimms[i];
if (layer0 >= 0 && layer0 != dimm->location[0])
continue;
@@ -1022,13 +1048,13 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type,
strcpy(label, "unknown memory");
if (type == HW_EVENT_ERR_CORRECTED) {
if (row >= 0) {
- mci->csrows[row].ce_count++;
+ mci->csrows[row]->ce_count++;
if (chan >= 0)
- mci->csrows[row].channels[chan].ce_count++;
+ mci->csrows[row]->channels[chan]->ce_count++;
}
} else
if (row >= 0)
- mci->csrows[row].ue_count++;
+ mci->csrows[row]->ue_count++;
}
/* Fill the RAM location data */
diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index 3f2d62f..d9a33d9 100644
--- a/drivers/edac/edac_mc_sysfs.c
+++ b/drivers/edac/edac_mc_sysfs.c
@@ -82,7 +82,7 @@ module_param_call(edac_mc_poll_msec, edac_set_poll_msec, param_get_int,
&edac_mc_poll_msec, 0644);
MODULE_PARM_DESC(edac_mc_poll_msec, "Polling period in milliseconds");
-static struct device mci_pdev;
+static struct device *mci_pdev;
/*
* various constants for Memory Controllers
@@ -181,7 +181,7 @@ static ssize_t csrow_size_show(struct device *dev,
u32 nr_pages = 0;
for (i = 0; i < csrow->nr_channels; i++)
- nr_pages += csrow->channels[i].dimm->nr_pages;
+ nr_pages += csrow->channels[i]->dimm->nr_pages;
return sprintf(data, "%u\n", PAGES_TO_MiB(nr_pages));
}
@@ -190,7 +190,7 @@ static ssize_t csrow_mem_type_show(struct device *dev,
{
struct csrow_info *csrow = to_csrow(dev);
- return sprintf(data, "%s\n", mem_types[csrow->channels[0].dimm->mtype]);
+ return sprintf(data, "%s\n", mem_types[csrow->channels[0]->dimm->mtype]);
}
static ssize_t csrow_dev_type_show(struct device *dev,
@@ -198,7 +198,7 @@ static ssize_t csrow_dev_type_show(struct device *dev,
{
struct csrow_info *csrow = to_csrow(dev);
- return sprintf(data, "%s\n", dev_types[csrow->channels[0].dimm->dtype]);
+ return sprintf(data, "%s\n", dev_types[csrow->channels[0]->dimm->dtype]);
}
static ssize_t csrow_edac_mode_show(struct device *dev,
@@ -207,7 +207,7 @@ static ssize_t csrow_edac_mode_show(struct device *dev,
{
struct csrow_info *csrow = to_csrow(dev);
- return sprintf(data, "%s\n", edac_caps[csrow->channels[0].dimm->edac_mode]);
+ return sprintf(data, "%s\n", edac_caps[csrow->channels[0]->dimm->edac_mode]);
}
/* show/store functions for DIMM Label attributes */
@@ -217,7 +217,7 @@ static ssize_t channel_dimm_label_show(struct device *dev,
{
struct csrow_info *csrow = to_csrow(dev);
unsigned chan = to_channel(mattr);
- struct rank_info *rank = &csrow->channels[chan];
+ struct rank_info *rank = csrow->channels[chan];
/* if field has not been initialized, there is nothing to send */
if (!rank->dimm->label[0])
@@ -233,7 +233,7 @@ static ssize_t channel_dimm_label_store(struct device *dev,
{
struct csrow_info *csrow = to_csrow(dev);
unsigned chan = to_channel(mattr);
- struct rank_info *rank = &csrow->channels[chan];
+ struct rank_info *rank = csrow->channels[chan];
ssize_t max_size = 0;
@@ -250,7 +250,7 @@ static ssize_t channel_ce_count_show(struct device *dev,
{
struct csrow_info *csrow = to_csrow(dev);
unsigned chan = to_channel(mattr);
- struct rank_info *rank = &csrow->channels[chan];
+ struct rank_info *rank = csrow->channels[chan];
return sprintf(data, "%u\n", rank->ce_count);
}
@@ -283,9 +283,12 @@ static const struct attribute_group *csrow_attr_groups[] = {
NULL
};
-static void csrow_attr_release(struct device *device)
+static void csrow_attr_release(struct device *dev)
{
- debugf1("Releasing csrow device %s\n", dev_name(device));
+ struct csrow_info *csrow = container_of(dev, struct csrow_info, dev);
+
+ debugf1("Releasing csrow device %s\n", dev_name(dev));
+ kfree(csrow);
}
static struct device_type csrow_attr_type = {
@@ -352,7 +355,7 @@ static inline int nr_pages_per_csrow(struct csrow_info *csrow)
int chan, nr_pages = 0;
for (chan = 0; chan < csrow->nr_channels; chan++)
- nr_pages += csrow->channels[chan].dimm->nr_pages;
+ nr_pages += csrow->channels[chan]->dimm->nr_pages;
return nr_pages;
}
@@ -382,7 +385,7 @@ static int edac_create_csrow_object(struct mem_ctl_info *mci,
for (chan = 0; chan < csrow->nr_channels; chan++) {
/* Only expose populated DIMMs */
- if (!csrow->channels[chan].dimm->nr_pages)
+ if (!csrow->channels[chan]->dimm->nr_pages)
continue;
err = device_create_file(&csrow->dev,
dynamic_csrow_dimm_attr[chan]);
@@ -418,10 +421,10 @@ static int edac_create_csrow_objects(struct mem_ctl_info *mci)
struct csrow_info *csrow;
for (i = 0; i < mci->num_csrows; i++) {
- csrow = &mci->csrows[i];
+ csrow = mci->csrows[i];
if (!nr_pages_per_csrow(csrow))
continue;
- err = edac_create_csrow_object(mci, &mci->csrows[i], i);
+ err = edac_create_csrow_object(mci, mci->csrows[i], i);
if (err < 0)
goto error;
}
@@ -429,18 +432,18 @@ static int edac_create_csrow_objects(struct mem_ctl_info *mci)
error:
for (--i; i >= 0; i--) {
- csrow = &mci->csrows[i];
+ csrow = mci->csrows[i];
if (!nr_pages_per_csrow(csrow))
continue;
for (chan = csrow->nr_channels - 1; chan >= 0; chan--) {
- if (!csrow->channels[chan].dimm->nr_pages)
+ if (!csrow->channels[chan]->dimm->nr_pages)
continue;
device_remove_file(&csrow->dev,
dynamic_csrow_dimm_attr[chan]);
device_remove_file(&csrow->dev,
dynamic_csrow_ce_count_attr[chan]);
}
- put_device(&mci->csrows[i].dev);
+ put_device(&mci->csrows[i]->dev);
}
return err;
@@ -452,11 +455,11 @@ static void edac_delete_csrow_objects(struct mem_ctl_info *mci)
struct csrow_info *csrow;
for (i = mci->num_csrows - 1; i >= 0; i--) {
- csrow = &mci->csrows[i];
+ csrow = mci->csrows[i];
if (!nr_pages_per_csrow(csrow))
continue;
for (chan = csrow->nr_channels - 1; chan >= 0; chan--) {
- if (!csrow->channels[chan].dimm->nr_pages)
+ if (!csrow->channels[chan]->dimm->nr_pages)
continue;
debugf1("Removing csrow %d channel %d sysfs nodes\n",
i, chan);
@@ -465,8 +468,8 @@ static void edac_delete_csrow_objects(struct mem_ctl_info *mci)
device_remove_file(&csrow->dev,
dynamic_csrow_ce_count_attr[chan]);
}
- put_device(&mci->csrows[i].dev);
- device_del(&mci->csrows[i].dev);
+ put_device(&mci->csrows[i]->dev);
+ device_del(&mci->csrows[i]->dev);
}
}
#endif
@@ -585,9 +588,12 @@ static const struct attribute_group *dimm_attr_groups[] = {
NULL
};
-static void dimm_attr_release(struct device *device)
+static void dimm_attr_release(struct device *dev)
{
- debugf1("Releasing dimm device %s\n", dev_name(device));
+ struct dimm_info *dimm = container_of(dev, struct dimm_info, dev);
+
+ debugf1("Releasing dimm device %s\n", dev_name(dev));
+ kfree(dimm);
}
static struct device_type dimm_attr_type = {
@@ -642,13 +648,13 @@ static ssize_t mci_reset_counters_store(struct device *dev,
for (row = 0; row < mci->num_csrows; row++) {
- struct csrow_info *ri = &mci->csrows[row];
+ struct csrow_info *ri = mci->csrows[row];
ri->ue_count = 0;
ri->ce_count = 0;
for (chan = 0; chan < ri->nr_channels; chan++)
- ri->channels[chan].ce_count = 0;
+ ri->channels[chan]->ce_count = 0;
}
cnt = 1;
@@ -782,10 +788,10 @@ static ssize_t mci_size_mb_show(struct device *dev,
for (total_pages = csrow_idx = 0; csrow_idx < mci->num_csrows;
csrow_idx++) {
- struct csrow_info *csrow = &mci->csrows[csrow_idx];
+ struct csrow_info *csrow = mci->csrows[csrow_idx];
for (j = 0; j < csrow->nr_channels; j++) {
- struct dimm_info *dimm = csrow->channels[j].dimm;
+ struct dimm_info *dimm = csrow->channels[j]->dimm;
total_pages += dimm->nr_pages;
}
@@ -892,9 +898,12 @@ static const struct attribute_group *mci_attr_groups[] = {
NULL
};
-static void mci_attr_release(struct device *device)
+static void mci_attr_release(struct device *dev)
{
- debugf1("Releasing mci device %s\n", dev_name(device));
+ struct mem_ctl_info *mci = container_of(dev, struct mem_ctl_info, dev);
+
+ debugf1("Releasing csrow device %s\n", dev_name(dev));
+ kfree(mci);
}
static struct device_type mci_attr_type = {
@@ -960,7 +969,7 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci)
mci->dev.type = &mci_attr_type;
device_initialize(&mci->dev);
- mci->dev.parent = &mci_pdev;
+ mci->dev.parent = mci_pdev;
mci->dev.bus = &mci->bus;
dev_set_name(&mci->dev, "mc%d", mci->mc_idx);
dev_set_drvdata(&mci->dev, mci);
@@ -989,7 +998,7 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci)
* Create the dimm/rank devices
*/
for (i = 0; i < mci->tot_dimms; i++) {
- struct dimm_info *dimm = &mci->dimms[i];
+ struct dimm_info *dimm = mci->dimms[i];
/* Only expose populated DIMMs */
if (dimm->nr_pages == 0)
continue;
@@ -1026,7 +1035,7 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci)
fail:
for (i--; i >= 0; i--) {
- struct dimm_info *dimm = &mci->dimms[i];
+ struct dimm_info *dimm = mci->dimms[i];
if (dimm->nr_pages == 0)
continue;
put_device(&dimm->dev);
@@ -1056,7 +1065,7 @@ void edac_remove_sysfs_mci_device(struct mem_ctl_info *mci)
#endif
for (i = 0; i < mci->tot_dimms; i++) {
- struct dimm_info *dimm = &mci->dimms[i];
+ struct dimm_info *dimm = mci->dimms[i];
if (dimm->nr_pages == 0)
continue;
debugf0("%s(): removing device %s\n", __func__,
@@ -1075,9 +1084,15 @@ void edac_unregister_sysfs(struct mem_ctl_info *mci)
kfree(mci->bus.name);
}
-static void mc_attr_release(struct device *device)
+static void mc_attr_release(struct device *dev)
{
- debugf1("Releasing device %s\n", dev_name(device));
+ /*
+ * There's no container structure here, as this is just the mci
+ * parent device, used to create the /sys/devices/mc sysfs node.
+ * So, there are no attributes on it.
+ */
+ debugf1("Releasing device %s\n", dev_name(dev));
+ kfree(dev);
}
static struct device_type mc_attr_type = {
@@ -1098,12 +1113,14 @@ int __init edac_mc_sysfs_init(void)
return -EINVAL;
}
- mci_pdev.bus = edac_subsys;
- mci_pdev.type = &mc_attr_type;
- device_initialize(&mci_pdev);
- dev_set_name(&mci_pdev, "mc");
+ mci_pdev = kzalloc(sizeof(*mci_pdev), GFP_KERNEL);
+
+ mci_pdev->bus = edac_subsys;
+ mci_pdev->type = &mc_attr_type;
+ device_initialize(mci_pdev);
+ dev_set_name(mci_pdev, "mc");
- err = device_add(&mci_pdev);
+ err = device_add(mci_pdev);
if (err < 0)
return err;
@@ -1112,7 +1129,7 @@ int __init edac_mc_sysfs_init(void)
void __exit edac_mc_sysfs_exit(void)
{
- put_device(&mci_pdev);
- device_del(&mci_pdev);
+ put_device(mci_pdev);
+ device_del(mci_pdev);
edac_put_sysfs_subsys();
}
diff --git a/include/linux/edac.h b/include/linux/edac.h
index 0e2cd0b4..63dcb9f 100644
--- a/include/linux/edac.h
+++ b/include/linux/edac.h
@@ -409,17 +409,28 @@ struct edac_mc_layer {
* during the memory allocation routine, with would point to the developer
* that he's doing something wrong.
*/
-#define GET_POS(layers, var, nlayers, lay0, lay1, lay2) ({ \
- typeof(var) __p; \
+
+#define GET_OFFSET(layers, nlayers, lay0, lay1, lay2) ({ \
+ int i; \
if ((nlayers) == 1) \
- __p = &var[lay0]; \
+ i = lay0; \
else if ((nlayers) == 2) \
- __p = &var[(lay1) + ((layers[1]).size * (lay0))]; \
+ i = (lay1) + ((layers[1]).size * (lay0)); \
else if ((nlayers) == 3) \
- __p = &var[(lay2) + ((layers[2]).size * ((lay1) + \
- ((layers[1]).size * (lay0))))]; \
+ i = (lay2) + ((layers[2]).size * ((lay1) + \
+ ((layers[1]).size * (lay0)))); \
+ else \
+ i = -EINVAL; \
+ i; \
+})
+
+#define GET_POS(layers, var, nlayers, lay0, lay1, lay2) ({ \
+ typeof(*var) __p; \
+ int i = GET_OFFSET(layers, nlayers, lay0, lay1, lay2); \
+ if (i < 0) \
+ __p = NULL \
else \
- __p = NULL; \
+ __p = (var)[i]; \
__p; \
})
@@ -459,8 +470,6 @@ struct dimm_info {
* patches in this series will fix this issue.
*/
struct rank_info {
- struct device dev;
-
int chan_idx;
struct csrow_info *csrow;
struct dimm_info *dimm;
@@ -486,7 +495,7 @@ struct csrow_info {
/* channel information for this csrow */
u32 nr_channels;
- struct rank_info *channels;
+ struct rank_info **channels;
};
/*
@@ -550,7 +559,7 @@ struct mem_ctl_info {
unsigned long (*ctl_page_to_phys) (struct mem_ctl_info * mci,
unsigned long page);
int mc_idx;
- struct csrow_info *csrows;
+ struct csrow_info **csrows;
unsigned num_csrows, num_cschannel;
/*
@@ -570,7 +579,7 @@ struct mem_ctl_info {
* DIMM info. Will eventually remove the entire csrows_info some day
*/
unsigned tot_dimms;
- struct dimm_info *dimms;
+ struct dimm_info **dimms;
/*
* FIXME - what about controllers on other busses? - IDs must be
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH 01/14] edac: rewrite the sysfs code to use struct device
2012-03-30 17:34 ` Mauro Carvalho Chehab
@ 2012-03-30 19:24 ` Greg K H
0 siblings, 0 replies; 26+ messages in thread
From: Greg K H @ 2012-03-30 19:24 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: Linux Edac Mailing List, Linux Kernel Mailing List
On Fri, Mar 30, 2012 at 02:34:43PM -0300, Mauro Carvalho Chehab wrote:
> Please check if this is the right thing to do. It does the EDAC core changes.
>
> The real patch is a way more complex than that, as it required to change the
> struct mem_ctrl_info pointers to an array of array, instead of just an array,
> with means that all drivers need to be changed. This patch is to be applied at
> the end of the patch series.
At first glance, yes, this does look correct, thanks for doing this.
greg k-h
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 02/14] mpc85xx_edac: convert sysfs logic to use struct device
2012-03-29 17:06 [PATCH 00/14] Fix the EDAC API Mauro Carvalho Chehab
2012-03-29 17:06 ` [PATCH 01/14] edac: rewrite the sysfs code to use struct device Mauro Carvalho Chehab
@ 2012-03-29 17:06 ` Mauro Carvalho Chehab
2012-03-29 17:06 ` [PATCH 03/14] amd64_edac: " Mauro Carvalho Chehab
` (12 subsequent siblings)
14 siblings, 0 replies; 26+ messages in thread
From: Mauro Carvalho Chehab @ 2012-03-29 17:06 UTC (permalink / raw)
Cc: Mauro Carvalho Chehab, Linux Edac Mailing List,
Linux Kernel Mailing List, Greg K H
Now that the EDAC core supports struct device, there's no sense on
having any logic at the EDAC core to simulate it. So, instead of adding
such logic there, change the logic at mpc85xx_edac to use it
compile-tested only.
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
drivers/edac/mpc85xx_edac.c | 93 +++++++++++++++++++++++++-----------------
1 files changed, 55 insertions(+), 38 deletions(-)
diff --git a/drivers/edac/mpc85xx_edac.c b/drivers/edac/mpc85xx_edac.c
index f6fb1e5..bf0d73a 100644
--- a/drivers/edac/mpc85xx_edac.c
+++ b/drivers/edac/mpc85xx_edac.c
@@ -49,34 +49,45 @@ static u32 orig_hid1[2];
/************************ MC SYSFS parts ***********************************/
-static ssize_t mpc85xx_mc_inject_data_hi_show(struct mem_ctl_info *mci,
+#define to_mci(k) container_of(k, struct mem_ctl_info, dev)
+
+static ssize_t mpc85xx_mc_inject_data_hi_show(struct device *dev,
+ struct device_attribute *mattr,
char *data)
{
+ struct mem_ctl_info *mci = to_mci(dev);
struct mpc85xx_mc_pdata *pdata = mci->pvt_info;
return sprintf(data, "0x%08x",
in_be32(pdata->mc_vbase +
MPC85XX_MC_DATA_ERR_INJECT_HI));
}
-static ssize_t mpc85xx_mc_inject_data_lo_show(struct mem_ctl_info *mci,
+static ssize_t mpc85xx_mc_inject_data_lo_show(struct device *dev,
+ struct device_attribute *mattr,
char *data)
{
+ struct mem_ctl_info *mci = to_mci(dev);
struct mpc85xx_mc_pdata *pdata = mci->pvt_info;
return sprintf(data, "0x%08x",
in_be32(pdata->mc_vbase +
MPC85XX_MC_DATA_ERR_INJECT_LO));
}
-static ssize_t mpc85xx_mc_inject_ctrl_show(struct mem_ctl_info *mci, char *data)
+static ssize_t mpc85xx_mc_inject_ctrl_show(struct device *dev,
+ struct device_attribute *mattr,
+ char *data)
{
+ struct mem_ctl_info *mci = to_mci(dev);
struct mpc85xx_mc_pdata *pdata = mci->pvt_info;
return sprintf(data, "0x%08x",
in_be32(pdata->mc_vbase + MPC85XX_MC_ECC_ERR_INJECT));
}
-static ssize_t mpc85xx_mc_inject_data_hi_store(struct mem_ctl_info *mci,
+static ssize_t mpc85xx_mc_inject_data_hi_store(struct device *dev,
+ struct device_attribute *mattr,
const char *data, size_t count)
{
+ struct mem_ctl_info *mci = to_mci(dev);
struct mpc85xx_mc_pdata *pdata = mci->pvt_info;
if (isdigit(*data)) {
out_be32(pdata->mc_vbase + MPC85XX_MC_DATA_ERR_INJECT_HI,
@@ -86,9 +97,11 @@ static ssize_t mpc85xx_mc_inject_data_hi_store(struct mem_ctl_info *mci,
return 0;
}
-static ssize_t mpc85xx_mc_inject_data_lo_store(struct mem_ctl_info *mci,
+static ssize_t mpc85xx_mc_inject_data_lo_store(struct device *dev,
+ struct device_attribute *mattr,
const char *data, size_t count)
{
+ struct mem_ctl_info *mci = to_mci(dev);
struct mpc85xx_mc_pdata *pdata = mci->pvt_info;
if (isdigit(*data)) {
out_be32(pdata->mc_vbase + MPC85XX_MC_DATA_ERR_INJECT_LO,
@@ -98,9 +111,11 @@ static ssize_t mpc85xx_mc_inject_data_lo_store(struct mem_ctl_info *mci,
return 0;
}
-static ssize_t mpc85xx_mc_inject_ctrl_store(struct mem_ctl_info *mci,
- const char *data, size_t count)
+static ssize_t mpc85xx_mc_inject_ctrl_store(struct device *dev,
+ struct device_attribute *mattr,
+ const char *data, size_t count)
{
+ struct mem_ctl_info *mci = to_mci(dev);
struct mpc85xx_mc_pdata *pdata = mci->pvt_info;
if (isdigit(*data)) {
out_be32(pdata->mc_vbase + MPC85XX_MC_ECC_ERR_INJECT,
@@ -110,38 +125,35 @@ static ssize_t mpc85xx_mc_inject_ctrl_store(struct mem_ctl_info *mci,
return 0;
}
-static struct mcidev_sysfs_attribute mpc85xx_mc_sysfs_attributes[] = {
- {
- .attr = {
- .name = "inject_data_hi",
- .mode = (S_IRUGO | S_IWUSR)
- },
- .show = mpc85xx_mc_inject_data_hi_show,
- .store = mpc85xx_mc_inject_data_hi_store},
- {
- .attr = {
- .name = "inject_data_lo",
- .mode = (S_IRUGO | S_IWUSR)
- },
- .show = mpc85xx_mc_inject_data_lo_show,
- .store = mpc85xx_mc_inject_data_lo_store},
- {
- .attr = {
- .name = "inject_ctrl",
- .mode = (S_IRUGO | S_IWUSR)
- },
- .show = mpc85xx_mc_inject_ctrl_show,
- .store = mpc85xx_mc_inject_ctrl_store},
+DEVICE_ATTR(inject_data_hi, S_IRUGO | S_IWUSR,
+ mpc85xx_mc_inject_data_hi_show, mpc85xx_mc_inject_data_hi_store);
+DEVICE_ATTR(inject_data_lo, S_IRUGO | S_IWUSR,
+ mpc85xx_mc_inject_data_lo_show, mpc85xx_mc_inject_data_lo_store);
+DEVICE_ATTR(inject_ctrl, S_IRUGO | S_IWUSR,
+ mpc85xx_mc_inject_ctrl_show, mpc85xx_mc_inject_ctrl_store);
- /* End of list */
- {
- .attr = {.name = NULL}
- }
-};
+static int mpc85xx_create_sysfs_attributes(struct mem_ctl_info *mci)
+{
+ int rc;
+
+ rc = device_create_file(&mci->dev, &dev_attr_inject_data_hi);
+ if (rc < 0)
+ return rc;
+ rc = device_create_file(&mci->dev, &dev_attr_inject_data_lo);
+ if (rc < 0)
+ return rc;
+ rc = device_create_file(&mci->dev, &dev_attr_inject_ctrl);
+ if (rc < 0)
+ return rc;
-static void mpc85xx_set_mc_sysfs_attributes(struct mem_ctl_info *mci)
+ return 0;
+}
+
+static void mpc85xx_remove_sysfs_attributes(struct mem_ctl_info *mci)
{
- mci->mc_driver_sysfs_attributes = mpc85xx_mc_sysfs_attributes;
+ device_remove_file(&mci->dev, &dev_attr_inject_data_hi);
+ device_remove_file(&mci->dev, &dev_attr_inject_data_lo);
+ device_remove_file(&mci->dev, &dev_attr_inject_ctrl);
}
/**************************** PCI Err device ***************************/
@@ -1041,8 +1053,6 @@ static int __devinit mpc85xx_mc_err_probe(struct platform_device *op)
mci->scrub_mode = SCRUB_SW_SRC;
- mpc85xx_set_mc_sysfs_attributes(mci);
-
mpc85xx_init_csrows(mci);
/* store the original error disable bits */
@@ -1058,6 +1068,12 @@ static int __devinit mpc85xx_mc_err_probe(struct platform_device *op)
goto err;
}
+ if (mpc85xx_create_sysfs_attributes(mci)) {
+ edac_mc_del_mc(mci->pdev);
+ debugf3("%s(): failed edac_mc_add_mc()\n", __func__);
+ goto err;
+ }
+
if (edac_op_state == EDAC_OPSTATE_INT) {
out_be32(pdata->mc_vbase + MPC85XX_MC_ERR_INT_EN,
DDR_EIE_MBEE | DDR_EIE_SBEE);
@@ -1117,6 +1133,7 @@ static int mpc85xx_mc_err_remove(struct platform_device *op)
orig_ddr_err_disable);
out_be32(pdata->mc_vbase + MPC85XX_MC_ERR_SBE, orig_ddr_err_sbe);
+ mpc85xx_remove_sysfs_attributes(mci);
edac_mc_del_mc(&op->dev);
edac_mc_free(mci);
return 0;
--
1.7.8
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH 03/14] amd64_edac: convert sysfs logic to use struct device
2012-03-29 17:06 [PATCH 00/14] Fix the EDAC API Mauro Carvalho Chehab
2012-03-29 17:06 ` [PATCH 01/14] edac: rewrite the sysfs code to use struct device Mauro Carvalho Chehab
2012-03-29 17:06 ` [PATCH 02/14] mpc85xx_edac: convert sysfs logic " Mauro Carvalho Chehab
@ 2012-03-29 17:06 ` Mauro Carvalho Chehab
2012-03-29 17:06 ` [PATCH 04/14] i7core_edac: convert it " Mauro Carvalho Chehab
` (11 subsequent siblings)
14 siblings, 0 replies; 26+ messages in thread
From: Mauro Carvalho Chehab @ 2012-03-29 17:06 UTC (permalink / raw)
Cc: Mauro Carvalho Chehab, Linux Edac Mailing List,
Linux Kernel Mailing List, Greg K H
Now that the EDAC core supports struct device, there's no sense
on having any logic at the EDAC core to simulate it. So, instead
of adding such logic there, change the logic at amd64_edac to
use it.
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
drivers/edac/amd64_edac.c | 43 ++++++++------
drivers/edac/amd64_edac.h | 29 +++++++--
drivers/edac/amd64_edac_dbg.c | 89 ++++++++++++++--------------
drivers/edac/amd64_edac_inj.c | 128 +++++++++++++++++++++++-----------------
4 files changed, 167 insertions(+), 122 deletions(-)
diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index 3118924..9851a1a 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -2435,26 +2435,29 @@ static bool ecc_enabled(struct pci_dev *F3, u8 nid)
return true;
}
-struct mcidev_sysfs_attribute sysfs_attrs[ARRAY_SIZE(amd64_dbg_attrs) +
- ARRAY_SIZE(amd64_inj_attrs) +
- 1];
-
-struct mcidev_sysfs_attribute terminator = { .attr = { .name = NULL } };
-
-static void set_mc_sysfs_attrs(struct mem_ctl_info *mci)
+static int set_mc_sysfs_attrs(struct mem_ctl_info *mci)
{
- unsigned int i = 0, j = 0;
+ int rc;
- for (; i < ARRAY_SIZE(amd64_dbg_attrs); i++)
- sysfs_attrs[i] = amd64_dbg_attrs[i];
+ rc = amd64_create_sysfs_dbg_files(mci);
+ if (rc < 0)
+ return rc;
- if (boot_cpu_data.x86 >= 0x10)
- for (j = 0; j < ARRAY_SIZE(amd64_inj_attrs); j++, i++)
- sysfs_attrs[i] = amd64_inj_attrs[j];
+ if (boot_cpu_data.x86 >= 0x10) {
+ rc = amd64_create_sysfs_inject_files(mci);
+ if (rc < 0)
+ return rc;
+ }
+
+ return 0;
+}
- sysfs_attrs[i] = terminator;
+static void del_mc_sysfs_attrs(struct mem_ctl_info *mci)
+{
+ amd64_remove_sysfs_dbg_files(mci);
- mci->mc_driver_sysfs_attributes = sysfs_attrs;
+ if (boot_cpu_data.x86 >= 0x10)
+ amd64_remove_sysfs_inject_files(mci);
}
static void setup_mci_misc_attrs(struct mem_ctl_info *mci,
@@ -2580,13 +2583,15 @@ static int amd64_init_one_instance(struct pci_dev *F2)
if (init_csrows(mci))
mci->edac_cap = EDAC_FLAG_NONE;
- set_mc_sysfs_attrs(mci);
-
ret = -ENODEV;
if (edac_mc_add_mc(mci)) {
debugf1("failed edac_mc_add_mc()\n");
goto err_add_mc;
}
+ if (set_mc_sysfs_attrs(mci)) {
+ debugf1("failed edac_mc_add_mc()\n");
+ goto err_add_sysfs;
+ }
/* register stuff with EDAC MCE */
if (report_gart_errors)
@@ -2600,6 +2605,8 @@ static int amd64_init_one_instance(struct pci_dev *F2)
return 0;
+err_add_sysfs:
+ edac_mc_del_mc(mci->pdev);
err_add_mc:
edac_mc_free(mci);
@@ -2670,6 +2677,8 @@ static void __devexit amd64_remove_one_instance(struct pci_dev *pdev)
struct pci_dev *F3 = node_to_amd_nb(nid)->misc;
struct ecc_settings *s = ecc_stngs[nid];
+ mci = find_mci_by_dev(&pdev->dev);
+ del_mc_sysfs_attrs(mci);
/* Remove from EDAC CORE tracking list */
mci = edac_mc_del_mc(&pdev->dev);
if (!mci)
diff --git a/drivers/edac/amd64_edac.h b/drivers/edac/amd64_edac.h
index 9a666cb..628c2d9 100644
--- a/drivers/edac/amd64_edac.h
+++ b/drivers/edac/amd64_edac.h
@@ -413,20 +413,33 @@ struct ecc_settings {
};
#ifdef CONFIG_EDAC_DEBUG
-#define NUM_DBG_ATTRS 5
+int amd64_create_sysfs_dbg_files(struct mem_ctl_info *mci);
+void amd64_remove_sysfs_dbg_files(struct mem_ctl_info *mci);
+
#else
-#define NUM_DBG_ATTRS 0
+static int amd64_create_sysfs_dbg_files(struct mem_ctl_info *mci)
+{
+ return 0;
+}
+void amd64_remove_sysfs_dbg_files(struct mem_ctl_info *mci)
+{
+}
#endif
#ifdef CONFIG_EDAC_AMD64_ERROR_INJECTION
-#define NUM_INJ_ATTRS 5
+int amd64_create_sysfs_inject_files(struct mem_ctl_info *mci);
+void amd64_remove_sysfs_inject_files(struct mem_ctl_info *mci);
+
#else
-#define NUM_INJ_ATTRS 0
+static int amd64_create_sysfs_inject_files(struct mem_ctl_info *mci)
+{
+ return 0;
+}
+static void amd64_remove_sysfs_inject_files(struct mem_ctl_info *mci)
+{
+}
#endif
-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
* functions and per device encoding/decoding logic.
@@ -460,3 +473,5 @@ int __amd64_write_pci_cfg_dword(struct pci_dev *pdev, int offset,
int amd64_get_dram_hole_info(struct mem_ctl_info *mci, u64 *hole_base,
u64 *hole_offset, u64 *hole_size);
+
+#define to_mci(k) container_of(k, struct mem_ctl_info, dev)
diff --git a/drivers/edac/amd64_edac_dbg.c b/drivers/edac/amd64_edac_dbg.c
index e356228..2c1bbf7 100644
--- a/drivers/edac/amd64_edac_dbg.c
+++ b/drivers/edac/amd64_edac_dbg.c
@@ -1,8 +1,11 @@
#include "amd64_edac.h"
#define EDAC_DCT_ATTR_SHOW(reg) \
-static ssize_t amd64_##reg##_show(struct mem_ctl_info *mci, char *data) \
+static ssize_t amd64_##reg##_show(struct device *dev, \
+ struct device_attribute *mattr, \
+ char *data) \
{ \
+ struct mem_ctl_info *mci = to_mci(dev); \
struct amd64_pvt *pvt = mci->pvt_info; \
return sprintf(data, "0x%016llx\n", (u64)pvt->reg); \
}
@@ -12,8 +15,12 @@ EDAC_DCT_ATTR_SHOW(dbam0);
EDAC_DCT_ATTR_SHOW(top_mem);
EDAC_DCT_ATTR_SHOW(top_mem2);
-static ssize_t amd64_hole_show(struct mem_ctl_info *mci, char *data)
+static ssize_t amd64_hole_show(struct device *dev,
+ struct device_attribute *mattr,
+ char *data)
{
+ struct mem_ctl_info *mci = to_mci(dev);
+
u64 hole_base = 0;
u64 hole_offset = 0;
u64 hole_size = 0;
@@ -27,46 +34,40 @@ static ssize_t amd64_hole_show(struct mem_ctl_info *mci, char *data)
/*
* update NUM_DBG_ATTRS in case you add new members
*/
-struct mcidev_sysfs_attribute amd64_dbg_attrs[] = {
+static DEVICE_ATTR(dhar, S_IRUGO, amd64_dhar_show, NULL);
+static DEVICE_ATTR(dbam, S_IRUGO, amd64_dbam0_show, NULL);
+static DEVICE_ATTR(topmem, S_IRUGO, amd64_top_mem_show, NULL);
+static DEVICE_ATTR(topmem2, S_IRUGO, amd64_top_mem2_show, NULL);
+static DEVICE_ATTR(dram_hole, S_IRUGO, amd64_hole_show, NULL);
+
+int amd64_create_sysfs_dbg_files(struct mem_ctl_info *mci)
+{
+ int rc;
+
+ rc = device_create_file(&mci->dev, &dev_attr_dhar);
+ if (rc < 0)
+ return rc;
+ rc = device_create_file(&mci->dev, &dev_attr_dbam);
+ if (rc < 0)
+ return rc;
+ rc = device_create_file(&mci->dev, &dev_attr_topmem);
+ if (rc < 0)
+ return rc;
+ rc = device_create_file(&mci->dev, &dev_attr_topmem2);
+ if (rc < 0)
+ return rc;
+ rc = device_create_file(&mci->dev, &dev_attr_dram_hole);
+ if (rc < 0)
+ return rc;
- {
- .attr = {
- .name = "dhar",
- .mode = (S_IRUGO)
- },
- .show = amd64_dhar_show,
- .store = NULL,
- },
- {
- .attr = {
- .name = "dbam",
- .mode = (S_IRUGO)
- },
- .show = amd64_dbam0_show,
- .store = NULL,
- },
- {
- .attr = {
- .name = "topmem",
- .mode = (S_IRUGO)
- },
- .show = amd64_top_mem_show,
- .store = NULL,
- },
- {
- .attr = {
- .name = "topmem2",
- .mode = (S_IRUGO)
- },
- .show = amd64_top_mem2_show,
- .store = NULL,
- },
- {
- .attr = {
- .name = "dram_hole",
- .mode = (S_IRUGO)
- },
- .show = amd64_hole_show,
- .store = NULL,
- },
-};
+ return 0;
+}
+
+void amd64_remove_sysfs_dbg_files(struct mem_ctl_info *mci)
+{
+ device_remove_file(&mci->dev, &dev_attr_dhar);
+ device_remove_file(&mci->dev, &dev_attr_dbam);
+ device_remove_file(&mci->dev, &dev_attr_topmem);
+ device_remove_file(&mci->dev, &dev_attr_topmem2);
+ device_remove_file(&mci->dev, &dev_attr_dram_hole);
+}
diff --git a/drivers/edac/amd64_edac_inj.c b/drivers/edac/amd64_edac_inj.c
index 303f10e..ef1ff4e 100644
--- a/drivers/edac/amd64_edac_inj.c
+++ b/drivers/edac/amd64_edac_inj.c
@@ -1,7 +1,10 @@
#include "amd64_edac.h"
-static ssize_t amd64_inject_section_show(struct mem_ctl_info *mci, char *buf)
+static ssize_t amd64_inject_section_show(struct device *dev,
+ struct device_attribute *mattr,
+ char *buf)
{
+ struct mem_ctl_info *mci = to_mci(dev);
struct amd64_pvt *pvt = mci->pvt_info;
return sprintf(buf, "0x%x\n", pvt->injection.section);
}
@@ -12,9 +15,11 @@ static ssize_t amd64_inject_section_show(struct mem_ctl_info *mci, char *buf)
*
* range: 0..3
*/
-static ssize_t amd64_inject_section_store(struct mem_ctl_info *mci,
+static ssize_t amd64_inject_section_store(struct device *dev,
+ struct device_attribute *mattr,
const char *data, size_t count)
{
+ struct mem_ctl_info *mci = to_mci(dev);
struct amd64_pvt *pvt = mci->pvt_info;
unsigned long value;
int ret = 0;
@@ -33,8 +38,11 @@ static ssize_t amd64_inject_section_store(struct mem_ctl_info *mci,
return ret;
}
-static ssize_t amd64_inject_word_show(struct mem_ctl_info *mci, char *buf)
+static ssize_t amd64_inject_word_show(struct device *dev,
+ struct device_attribute *mattr,
+ char *buf)
{
+ struct mem_ctl_info *mci = to_mci(dev);
struct amd64_pvt *pvt = mci->pvt_info;
return sprintf(buf, "0x%x\n", pvt->injection.word);
}
@@ -45,9 +53,11 @@ static ssize_t amd64_inject_word_show(struct mem_ctl_info *mci, char *buf)
*
* range: 0..8
*/
-static ssize_t amd64_inject_word_store(struct mem_ctl_info *mci,
- const char *data, size_t count)
+static ssize_t amd64_inject_word_store(struct device *dev,
+ struct device_attribute *mattr,
+ const char *data, size_t count)
{
+ struct mem_ctl_info *mci = to_mci(dev);
struct amd64_pvt *pvt = mci->pvt_info;
unsigned long value;
int ret = 0;
@@ -66,8 +76,11 @@ static ssize_t amd64_inject_word_store(struct mem_ctl_info *mci,
return ret;
}
-static ssize_t amd64_inject_ecc_vector_show(struct mem_ctl_info *mci, char *buf)
+static ssize_t amd64_inject_ecc_vector_show(struct device *dev,
+ struct device_attribute *mattr,
+ char *buf)
{
+ struct mem_ctl_info *mci = to_mci(dev);
struct amd64_pvt *pvt = mci->pvt_info;
return sprintf(buf, "0x%x\n", pvt->injection.bit_map);
}
@@ -77,9 +90,11 @@ static ssize_t amd64_inject_ecc_vector_show(struct mem_ctl_info *mci, char *buf)
* corresponding bit within the error injection word above. When used during a
* DRAM ECC read, it holds the contents of the of the DRAM ECC bits.
*/
-static ssize_t amd64_inject_ecc_vector_store(struct mem_ctl_info *mci,
- const char *data, size_t count)
+static ssize_t amd64_inject_ecc_vector_store(struct device *dev,
+ struct device_attribute *mattr,
+ const char *data, size_t count)
{
+ struct mem_ctl_info *mci = to_mci(dev);
struct amd64_pvt *pvt = mci->pvt_info;
unsigned long value;
int ret = 0;
@@ -103,9 +118,11 @@ static ssize_t amd64_inject_ecc_vector_store(struct mem_ctl_info *mci,
* Do a DRAM ECC read. Assemble staged values in the pvt area, format into
* fields needed by the injection registers and read the NB Array Data Port.
*/
-static ssize_t amd64_inject_read_store(struct mem_ctl_info *mci,
- const char *data, size_t count)
+static ssize_t amd64_inject_read_store(struct device *dev,
+ struct device_attribute *mattr,
+ const char *data, size_t count)
{
+ struct mem_ctl_info *mci = to_mci(dev);
struct amd64_pvt *pvt = mci->pvt_info;
unsigned long value;
u32 section, word_bits;
@@ -136,9 +153,11 @@ static ssize_t amd64_inject_read_store(struct mem_ctl_info *mci,
* Do a DRAM ECC write. Assemble staged values in the pvt area and format into
* fields needed by the injection registers.
*/
-static ssize_t amd64_inject_write_store(struct mem_ctl_info *mci,
+static ssize_t amd64_inject_write_store(struct device *dev,
+ struct device_attribute *mattr,
const char *data, size_t count)
{
+ struct mem_ctl_info *mci = to_mci(dev);
struct amd64_pvt *pvt = mci->pvt_info;
unsigned long value;
u32 section, word_bits;
@@ -168,46 +187,47 @@ static ssize_t amd64_inject_write_store(struct mem_ctl_info *mci,
/*
* update NUM_INJ_ATTRS in case you add new members
*/
-struct mcidev_sysfs_attribute amd64_inj_attrs[] = {
-
- {
- .attr = {
- .name = "inject_section",
- .mode = (S_IRUGO | S_IWUSR)
- },
- .show = amd64_inject_section_show,
- .store = amd64_inject_section_store,
- },
- {
- .attr = {
- .name = "inject_word",
- .mode = (S_IRUGO | S_IWUSR)
- },
- .show = amd64_inject_word_show,
- .store = amd64_inject_word_store,
- },
- {
- .attr = {
- .name = "inject_ecc_vector",
- .mode = (S_IRUGO | S_IWUSR)
- },
- .show = amd64_inject_ecc_vector_show,
- .store = amd64_inject_ecc_vector_store,
- },
- {
- .attr = {
- .name = "inject_write",
- .mode = (S_IRUGO | S_IWUSR)
- },
- .show = NULL,
- .store = amd64_inject_write_store,
- },
- {
- .attr = {
- .name = "inject_read",
- .mode = (S_IRUGO | S_IWUSR)
- },
- .show = NULL,
- .store = amd64_inject_read_store,
- },
-};
+
+static DEVICE_ATTR(inject_section, S_IRUGO | S_IWUSR,
+ amd64_inject_section_show, amd64_inject_section_store);
+static DEVICE_ATTR(inject_word, S_IRUGO | S_IWUSR,
+ amd64_inject_word_show, amd64_inject_word_store);
+static DEVICE_ATTR(inject_ecc_vector, S_IRUGO | S_IWUSR,
+ amd64_inject_ecc_vector_show, amd64_inject_ecc_vector_store);
+static DEVICE_ATTR(inject_write, S_IRUGO | S_IWUSR,
+ NULL, amd64_inject_write_store);
+static DEVICE_ATTR(inject_read, S_IRUGO | S_IWUSR,
+ NULL, amd64_inject_read_store);
+
+
+int amd64_create_sysfs_inject_files(struct mem_ctl_info *mci)
+{
+ int rc;
+
+ rc = device_create_file(&mci->dev, &dev_attr_inject_section);
+ if (rc < 0)
+ return rc;
+ rc = device_create_file(&mci->dev, &dev_attr_inject_word);
+ if (rc < 0)
+ return rc;
+ rc = device_create_file(&mci->dev, &dev_attr_inject_ecc_vector);
+ if (rc < 0)
+ return rc;
+ rc = device_create_file(&mci->dev, &dev_attr_inject_write);
+ if (rc < 0)
+ return rc;
+ rc = device_create_file(&mci->dev, &dev_attr_inject_read);
+ if (rc < 0)
+ return rc;
+
+ return 0;
+}
+
+void amd64_remove_sysfs_inject_files(struct mem_ctl_info *mci)
+{
+ device_remove_file(&mci->dev, &dev_attr_inject_section);
+ device_remove_file(&mci->dev, &dev_attr_inject_word);
+ device_remove_file(&mci->dev, &dev_attr_inject_ecc_vector);
+ device_remove_file(&mci->dev, &dev_attr_inject_write);
+ device_remove_file(&mci->dev, &dev_attr_inject_read);
+}
--
1.7.8
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH 04/14] i7core_edac: convert it to use struct device
2012-03-29 17:06 [PATCH 00/14] Fix the EDAC API Mauro Carvalho Chehab
` (2 preceding siblings ...)
2012-03-29 17:06 ` [PATCH 03/14] amd64_edac: " Mauro Carvalho Chehab
@ 2012-03-29 17:06 ` Mauro Carvalho Chehab
2012-03-29 17:06 ` [PATCH 05/14] edac: Get rid of the old kobj's from the edac mc code Mauro Carvalho Chehab
` (10 subsequent siblings)
14 siblings, 0 replies; 26+ messages in thread
From: Mauro Carvalho Chehab @ 2012-03-29 17:06 UTC (permalink / raw)
Cc: Mauro Carvalho Chehab, Linux Edac Mailing List,
Linux Kernel Mailing List, Greg K H
Instead of relying on a complex logic inside the edac core to create
a "device tree-like" sysfs struct, just use device_add.
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
drivers/edac/i7core_edac.c | 336 +++++++++++++++++++++++++++-----------------
1 files changed, 208 insertions(+), 128 deletions(-)
diff --git a/drivers/edac/i7core_edac.c b/drivers/edac/i7core_edac.c
index 7f81a04..9fd8ff5 100644
--- a/drivers/edac/i7core_edac.c
+++ b/drivers/edac/i7core_edac.c
@@ -246,6 +246,8 @@ struct i7core_dev {
};
struct i7core_pvt {
+ struct device addrmatch_dev, chancounts_dev;
+
struct pci_dev *pci_noncore;
struct pci_dev *pci_mcr[MAX_MCR_FUNC + 1];
struct pci_dev *pci_ch[NUM_CHANS][MAX_CHAN_FUNC + 1];
@@ -659,6 +661,8 @@ static int get_dimm_config(struct mem_ctl_info *mci)
Error insertion routines
****************************************************************************/
+#define to_mci(k) container_of(k, struct mem_ctl_info, dev)
+
/* The i7core has independent error injection features per channel.
However, to have a simpler code, we don't allow enabling error injection
on more than one channel.
@@ -688,9 +692,11 @@ static int disable_inject(const struct mem_ctl_info *mci)
* bit 0 - refers to the lower 32-byte half cacheline
* bit 1 - refers to the upper 32-byte half cacheline
*/
-static ssize_t i7core_inject_section_store(struct mem_ctl_info *mci,
+static ssize_t i7core_inject_section_store(struct device *dev,
+ struct device_attribute *mattr,
const char *data, size_t count)
{
+ struct mem_ctl_info *mci = to_mci(dev);
struct i7core_pvt *pvt = mci->pvt_info;
unsigned long value;
int rc;
@@ -706,9 +712,11 @@ static ssize_t i7core_inject_section_store(struct mem_ctl_info *mci,
return count;
}
-static ssize_t i7core_inject_section_show(struct mem_ctl_info *mci,
- char *data)
+static ssize_t i7core_inject_section_show(struct device *dev,
+ struct device_attribute *mattr,
+ char *data)
{
+ struct mem_ctl_info *mci = to_mci(dev);
struct i7core_pvt *pvt = mci->pvt_info;
return sprintf(data, "0x%08x\n", pvt->inject.section);
}
@@ -721,10 +729,12 @@ static ssize_t i7core_inject_section_show(struct mem_ctl_info *mci,
* bit 1 - inject ECC error
* bit 2 - inject parity error
*/
-static ssize_t i7core_inject_type_store(struct mem_ctl_info *mci,
+static ssize_t i7core_inject_type_store(struct device *dev,
+ struct device_attribute *mattr,
const char *data, size_t count)
{
- struct i7core_pvt *pvt = mci->pvt_info;
+ struct mem_ctl_info *mci = to_mci(dev);
+struct i7core_pvt *pvt = mci->pvt_info;
unsigned long value;
int rc;
@@ -739,10 +749,13 @@ static ssize_t i7core_inject_type_store(struct mem_ctl_info *mci,
return count;
}
-static ssize_t i7core_inject_type_show(struct mem_ctl_info *mci,
- char *data)
+static ssize_t i7core_inject_type_show(struct device *dev,
+ struct device_attribute *mattr,
+ char *data)
{
+ struct mem_ctl_info *mci = to_mci(dev);
struct i7core_pvt *pvt = mci->pvt_info;
+
return sprintf(data, "0x%08x\n", pvt->inject.type);
}
@@ -756,9 +769,11 @@ static ssize_t i7core_inject_type_show(struct mem_ctl_info *mci,
* 23:16 and 31:24). Flipping bits in two symbol pairs will cause an
* uncorrectable error to be injected.
*/
-static ssize_t i7core_inject_eccmask_store(struct mem_ctl_info *mci,
- const char *data, size_t count)
+static ssize_t i7core_inject_eccmask_store(struct device *dev,
+ struct device_attribute *mattr,
+ const char *data, size_t count)
{
+ struct mem_ctl_info *mci = to_mci(dev);
struct i7core_pvt *pvt = mci->pvt_info;
unsigned long value;
int rc;
@@ -774,10 +789,13 @@ static ssize_t i7core_inject_eccmask_store(struct mem_ctl_info *mci,
return count;
}
-static ssize_t i7core_inject_eccmask_show(struct mem_ctl_info *mci,
- char *data)
+static ssize_t i7core_inject_eccmask_show(struct device *dev,
+ struct device_attribute *mattr,
+ char *data)
{
+ struct mem_ctl_info *mci = to_mci(dev);
struct i7core_pvt *pvt = mci->pvt_info;
+
return sprintf(data, "0x%08x\n", pvt->inject.eccmask);
}
@@ -794,9 +812,11 @@ static ssize_t i7core_inject_eccmask_show(struct mem_ctl_info *mci,
#define DECLARE_ADDR_MATCH(param, limit) \
static ssize_t i7core_inject_store_##param( \
- struct mem_ctl_info *mci, \
- const char *data, size_t count) \
+ struct device *dev, \
+ struct device_attribute *mattr, \
+ const char *data, size_t count) \
{ \
+ struct mem_ctl_info *mci = to_mci(dev); \
struct i7core_pvt *pvt; \
long value; \
int rc; \
@@ -821,9 +841,11 @@ static ssize_t i7core_inject_store_##param( \
} \
\
static ssize_t i7core_inject_show_##param( \
- struct mem_ctl_info *mci, \
- char *data) \
+ struct device *dev, \
+ struct device_attribute *mattr, \
+ char *data) \
{ \
+ struct mem_ctl_info *mci = to_mci(dev); \
struct i7core_pvt *pvt; \
\
pvt = mci->pvt_info; \
@@ -835,14 +857,9 @@ static ssize_t i7core_inject_show_##param( \
}
#define ATTR_ADDR_MATCH(param) \
- { \
- .attr = { \
- .name = #param, \
- .mode = (S_IRUGO | S_IWUSR) \
- }, \
- .show = i7core_inject_show_##param, \
- .store = i7core_inject_store_##param, \
- }
+ static DEVICE_ATTR(param, S_IRUGO | S_IWUSR, \
+ i7core_inject_show_##param, \
+ i7core_inject_store_##param)
DECLARE_ADDR_MATCH(channel, 3);
DECLARE_ADDR_MATCH(dimm, 3);
@@ -851,6 +868,13 @@ DECLARE_ADDR_MATCH(bank, 32);
DECLARE_ADDR_MATCH(page, 0x10000);
DECLARE_ADDR_MATCH(col, 0x4000);
+ATTR_ADDR_MATCH(channel);
+ATTR_ADDR_MATCH(dimm);
+ATTR_ADDR_MATCH(rank);
+ATTR_ADDR_MATCH(bank);
+ATTR_ADDR_MATCH(page);
+ATTR_ADDR_MATCH(col);
+
static int write_and_test(struct pci_dev *dev, const int where, const u32 val)
{
u32 read;
@@ -896,9 +920,11 @@ static int write_and_test(struct pci_dev *dev, const int where, const u32 val)
* is reliable enough to check if the MC is using the
* three channels. However, this is not clear at the datasheet.
*/
-static ssize_t i7core_inject_enable_store(struct mem_ctl_info *mci,
- const char *data, size_t count)
+static ssize_t i7core_inject_enable_store(struct device *dev,
+ struct device_attribute *mattr,
+ const char *data, size_t count)
{
+ struct mem_ctl_info *mci = to_mci(dev);
struct i7core_pvt *pvt = mci->pvt_info;
u32 injectmask;
u64 mask = 0;
@@ -999,9 +1025,11 @@ static ssize_t i7core_inject_enable_store(struct mem_ctl_info *mci,
return count;
}
-static ssize_t i7core_inject_enable_show(struct mem_ctl_info *mci,
- char *data)
+static ssize_t i7core_inject_enable_show(struct device *dev,
+ struct device_attribute *mattr,
+ char *data)
{
+ struct mem_ctl_info *mci = to_mci(dev);
struct i7core_pvt *pvt = mci->pvt_info;
u32 injectmask;
@@ -1021,9 +1049,11 @@ static ssize_t i7core_inject_enable_show(struct mem_ctl_info *mci,
#define DECLARE_COUNTER(param) \
static ssize_t i7core_show_counter_##param( \
- struct mem_ctl_info *mci, \
- char *data) \
+ struct device *dev, \
+ struct device_attribute *mattr, \
+ char *data) \
{ \
+ struct mem_ctl_info *mci = to_mci(dev); \
struct i7core_pvt *pvt = mci->pvt_info; \
\
debugf1("%s() \n", __func__); \
@@ -1034,121 +1064,167 @@ static ssize_t i7core_show_counter_##param( \
}
#define ATTR_COUNTER(param) \
- { \
- .attr = { \
- .name = __stringify(udimm##param), \
- .mode = (S_IRUGO | S_IWUSR) \
- }, \
- .show = i7core_show_counter_##param \
- }
+ static DEVICE_ATTR(udimm##param, S_IRUGO | S_IWUSR, \
+ i7core_show_counter_##param, \
+ NULL)
DECLARE_COUNTER(0);
DECLARE_COUNTER(1);
DECLARE_COUNTER(2);
+ATTR_COUNTER(0);
+ATTR_COUNTER(1);
+ATTR_COUNTER(2);
+
/*
- * Sysfs struct
+ * inject_addrmatch device sysfs struct
*/
-static const struct mcidev_sysfs_attribute i7core_addrmatch_attrs[] = {
- ATTR_ADDR_MATCH(channel),
- ATTR_ADDR_MATCH(dimm),
- ATTR_ADDR_MATCH(rank),
- ATTR_ADDR_MATCH(bank),
- ATTR_ADDR_MATCH(page),
- ATTR_ADDR_MATCH(col),
- { } /* End of list */
+static struct attribute *i7core_addrmatch_attrs[] = {
+ &dev_attr_channel.attr,
+ &dev_attr_dimm.attr,
+ &dev_attr_rank.attr,
+ &dev_attr_bank.attr,
+ &dev_attr_page.attr,
+ &dev_attr_col.attr,
+ NULL
};
-static const struct mcidev_sysfs_group i7core_inject_addrmatch = {
- .name = "inject_addrmatch",
- .mcidev_attr = i7core_addrmatch_attrs,
+static struct attribute_group addrmatch_grp = {
+ .attrs = i7core_addrmatch_attrs,
};
-static const struct mcidev_sysfs_attribute i7core_udimm_counters_attrs[] = {
- ATTR_COUNTER(0),
- ATTR_COUNTER(1),
- ATTR_COUNTER(2),
- { .attr = { .name = NULL } }
+static const struct attribute_group *addrmatch_groups[] = {
+ &addrmatch_grp,
+ NULL
};
-static const struct mcidev_sysfs_group i7core_udimm_counters = {
- .name = "all_channel_counts",
- .mcidev_attr = i7core_udimm_counters_attrs,
+static void addrmatch_release(struct device *device)
+{
+ debugf1("Releasing device %s\n", dev_name(device));
+}
+
+static struct device_type addrmatch_type = {
+ .groups = addrmatch_groups,
+ .release = addrmatch_release,
};
-static const struct mcidev_sysfs_attribute i7core_sysfs_rdimm_attrs[] = {
- {
- .attr = {
- .name = "inject_section",
- .mode = (S_IRUGO | S_IWUSR)
- },
- .show = i7core_inject_section_show,
- .store = i7core_inject_section_store,
- }, {
- .attr = {
- .name = "inject_type",
- .mode = (S_IRUGO | S_IWUSR)
- },
- .show = i7core_inject_type_show,
- .store = i7core_inject_type_store,
- }, {
- .attr = {
- .name = "inject_eccmask",
- .mode = (S_IRUGO | S_IWUSR)
- },
- .show = i7core_inject_eccmask_show,
- .store = i7core_inject_eccmask_store,
- }, {
- .grp = &i7core_inject_addrmatch,
- }, {
- .attr = {
- .name = "inject_enable",
- .mode = (S_IRUGO | S_IWUSR)
- },
- .show = i7core_inject_enable_show,
- .store = i7core_inject_enable_store,
- },
- { } /* End of list */
+/*
+ * all_channel_counts sysfs struct
+ */
+
+static struct attribute *i7core_udimm_counters_attrs[] = {
+ &dev_attr_udimm0.attr,
+ &dev_attr_udimm1.attr,
+ &dev_attr_udimm2.attr,
+ NULL
+};
+
+static struct attribute_group all_channel_counts_grp = {
+ .attrs = i7core_udimm_counters_attrs,
+};
+
+static const struct attribute_group *all_channel_counts_groups[] = {
+ &all_channel_counts_grp,
+ NULL
};
-static const struct mcidev_sysfs_attribute i7core_sysfs_udimm_attrs[] = {
- {
- .attr = {
- .name = "inject_section",
- .mode = (S_IRUGO | S_IWUSR)
- },
- .show = i7core_inject_section_show,
- .store = i7core_inject_section_store,
- }, {
- .attr = {
- .name = "inject_type",
- .mode = (S_IRUGO | S_IWUSR)
- },
- .show = i7core_inject_type_show,
- .store = i7core_inject_type_store,
- }, {
- .attr = {
- .name = "inject_eccmask",
- .mode = (S_IRUGO | S_IWUSR)
- },
- .show = i7core_inject_eccmask_show,
- .store = i7core_inject_eccmask_store,
- }, {
- .grp = &i7core_inject_addrmatch,
- }, {
- .attr = {
- .name = "inject_enable",
- .mode = (S_IRUGO | S_IWUSR)
- },
- .show = i7core_inject_enable_show,
- .store = i7core_inject_enable_store,
- }, {
- .grp = &i7core_udimm_counters,
- },
- { } /* End of list */
+static void all_channel_counts_release(struct device *device)
+{
+ debugf1("Releasing device %s\n", dev_name(device));
+}
+
+static struct device_type all_channel_counts_type = {
+ .groups = all_channel_counts_groups,
+ .release = all_channel_counts_release,
};
+/*
+ * inject sysfs attributes
+ */
+
+static DEVICE_ATTR(inject_section, S_IRUGO | S_IWUSR,
+ i7core_inject_section_show, i7core_inject_section_store);
+
+static DEVICE_ATTR(inject_type, S_IRUGO | S_IWUSR,
+ i7core_inject_type_show, i7core_inject_type_store);
+
+
+static DEVICE_ATTR(inject_eccmask, S_IRUGO | S_IWUSR,
+ i7core_inject_eccmask_show, i7core_inject_eccmask_store);
+
+static DEVICE_ATTR(inject_enable, S_IRUGO | S_IWUSR,
+ i7core_inject_enable_show, i7core_inject_enable_store);
+
+static int i7core_create_sysfs_devices(struct mem_ctl_info *mci)
+{
+ struct i7core_pvt *pvt = mci->pvt_info;
+ int rc;
+
+ rc = device_create_file(&mci->dev, &dev_attr_inject_section);
+ if (rc < 0)
+ return rc;
+ rc = device_create_file(&mci->dev, &dev_attr_inject_type);
+ if (rc < 0)
+ return rc;
+ rc = device_create_file(&mci->dev, &dev_attr_inject_eccmask);
+ if (rc < 0)
+ return rc;
+ rc = device_create_file(&mci->dev, &dev_attr_inject_enable);
+ if (rc < 0)
+ return rc;
+
+ pvt->addrmatch_dev.type = &addrmatch_type;
+ pvt->addrmatch_dev.bus = mci->dev.bus;
+ device_initialize(&pvt->addrmatch_dev);
+ pvt->addrmatch_dev.parent = &mci->dev;
+ dev_set_name(&pvt->addrmatch_dev, "inject_addrmatch");
+ dev_set_drvdata(&pvt->addrmatch_dev, mci);
+
+ debugf1("%s(): creating %s\n", __func__,
+ dev_name(&pvt->addrmatch_dev));
+
+ rc = device_add(&pvt->addrmatch_dev);
+ if (rc < 0)
+ return rc;
+
+ if (!pvt->is_registered) {
+ pvt->chancounts_dev.type = &all_channel_counts_type;
+ pvt->chancounts_dev.bus = mci->dev.bus;
+ device_initialize(&pvt->chancounts_dev);
+ pvt->chancounts_dev.parent = &mci->dev;
+ dev_set_name(&pvt->chancounts_dev, "all_channel_counts");
+ dev_set_drvdata(&pvt->chancounts_dev, mci);
+
+ debugf1("%s(): creating %s\n", __func__,
+ dev_name(&pvt->chancounts_dev));
+
+ rc = device_add(&pvt->chancounts_dev);
+ if (rc < 0)
+ return rc;
+ }
+ return 0;
+}
+
+static void i7core_delete_sysfs_devices(struct mem_ctl_info *mci)
+{
+ struct i7core_pvt *pvt = mci->pvt_info;
+
+ debugf1("\n");
+
+ device_remove_file(&mci->dev, &dev_attr_inject_section);
+ device_remove_file(&mci->dev, &dev_attr_inject_type);
+ device_remove_file(&mci->dev, &dev_attr_inject_eccmask);
+ device_remove_file(&mci->dev, &dev_attr_inject_enable);
+
+ if (!pvt->is_registered) {
+ put_device(&pvt->chancounts_dev);
+ device_del(&pvt->chancounts_dev);
+ }
+ put_device(&pvt->addrmatch_dev);
+ device_del(&pvt->addrmatch_dev);
+}
+
/****************************************************************************
Device initialization routines: put/get, init/exit
****************************************************************************/
@@ -2119,6 +2195,7 @@ static void i7core_unregister_mci(struct i7core_dev *i7core_dev)
i7core_pci_ctl_release(pvt);
/* Remove MC sysfs nodes */
+ i7core_delete_sysfs_devices(mci);
edac_mc_del_mc(mci->pdev);
debugf1("%s: free mci struct\n", mci->ctl_name);
@@ -2177,10 +2254,6 @@ static int i7core_register_mci(struct i7core_dev *i7core_dev)
if (unlikely(rc < 0))
goto fail0;
- if (pvt->is_registered)
- mci->mc_driver_sysfs_attributes = i7core_sysfs_rdimm_attrs;
- else
- mci->mc_driver_sysfs_attributes = i7core_sysfs_udimm_attrs;
/* Get dimm basic config */
get_dimm_config(mci);
@@ -2204,6 +2277,13 @@ static int i7core_register_mci(struct i7core_dev *i7core_dev)
rc = -EINVAL;
goto fail0;
}
+ if (i7core_create_sysfs_devices(mci)) {
+ debugf0("MC: " __FILE__
+ ": %s(): failed to create sysfs nodes\n", __func__);
+ edac_mc_del_mc(mci->pdev);
+ rc = -EINVAL;
+ goto fail0;
+ }
/* Default error mask is any memory */
pvt->inject.channel = 0;
--
1.7.8
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH 05/14] edac: Get rid of the old kobj's from the edac mc code
2012-03-29 17:06 [PATCH 00/14] Fix the EDAC API Mauro Carvalho Chehab
` (3 preceding siblings ...)
2012-03-29 17:06 ` [PATCH 04/14] i7core_edac: convert it " Mauro Carvalho Chehab
@ 2012-03-29 17:06 ` Mauro Carvalho Chehab
2012-03-29 17:06 ` [PATCH 06/14] edac: add a new per-dimm API and make the old per-virtual-rank API obsolete Mauro Carvalho Chehab
` (9 subsequent siblings)
14 siblings, 0 replies; 26+ messages in thread
From: Mauro Carvalho Chehab @ 2012-03-29 17:06 UTC (permalink / raw)
Cc: Mauro Carvalho Chehab, Linux Edac Mailing List,
Linux Kernel Mailing List, Greg K H
Now that al users for the old kobj raw access are gone,
we can get rid of the legacy kobj-based structures and
data.
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
drivers/edac/edac_mc.c | 1 -
drivers/edac/i5000_edac.c | 3 ---
drivers/edac/i82875p_edac.c | 4 ----
include/linux/edac.h | 30 ------------------------------
4 files changed, 0 insertions(+), 38 deletions(-)
diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index 4a61d9a..ced1d95 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -366,7 +366,6 @@ struct mem_ctl_info *edac_mc_alloc(unsigned edac_index,
}
mci->op_state = OP_ALLOC;
- INIT_LIST_HEAD(&mci->grp_kobj_list);
/* at this point, the root kobj is valid, and in order to
* 'free' the object, then the function:
diff --git a/drivers/edac/i5000_edac.c b/drivers/edac/i5000_edac.c
index 0b6ac99..a7efeee 100644
--- a/drivers/edac/i5000_edac.c
+++ b/drivers/edac/i5000_edac.c
@@ -1409,7 +1409,6 @@ static int i5000_probe1(struct pci_dev *pdev, int dev_idx)
if (mci == NULL)
return -ENOMEM;
- kobject_get(&mci->edac_mci_kobj);
debugf0("MC: %s: %s(): mci = %p\n", __FILE__, __func__, mci);
mci->pdev = &pdev->dev; /* record ptr to the generic device */
@@ -1482,7 +1481,6 @@ fail1:
i5000_put_devices(mci);
fail0:
- kobject_put(&mci->edac_mci_kobj);
edac_mc_free(mci);
return -ENODEV;
}
@@ -1528,7 +1526,6 @@ static void __devexit i5000_remove_one(struct pci_dev *pdev)
/* retrieve references to resources, and free those resources */
i5000_put_devices(mci);
- kobject_put(&mci->edac_mci_kobj);
edac_mc_free(mci);
}
diff --git a/drivers/edac/i82875p_edac.c b/drivers/edac/i82875p_edac.c
index e89757e..8b27a64 100644
--- a/drivers/edac/i82875p_edac.c
+++ b/drivers/edac/i82875p_edac.c
@@ -426,9 +426,6 @@ static int i82875p_probe1(struct pci_dev *pdev, int dev_idx)
goto fail0;
}
- /* Keeps mci available after edac_mc_del_mc() till edac_mc_free() */
- kobject_get(&mci->edac_mci_kobj);
-
debugf3("%s(): init mci\n", __func__);
mci->pdev = &pdev->dev;
mci->mtype_cap = MEM_FLAG_DDR;
@@ -471,7 +468,6 @@ static int i82875p_probe1(struct pci_dev *pdev, int dev_idx)
return 0;
fail1:
- kobject_put(&mci->edac_mci_kobj);
edac_mc_free(mci);
fail0:
diff --git a/include/linux/edac.h b/include/linux/edac.h
index fb1c6e4..81ca2ba 100644
--- a/include/linux/edac.h
+++ b/include/linux/edac.h
@@ -430,7 +430,6 @@ struct dimm_info {
/* Memory location data */
unsigned location[EDAC_MAX_LAYERS];
- struct kobject kobj; /* sysfs kobject for this csrow */
struct mem_ctl_info *mci; /* the parent */
u32 grain; /* granularity of reported error in bytes */
@@ -484,34 +483,11 @@ struct csrow_info {
struct mem_ctl_info *mci; /* the parent */
- struct kobject kobj; /* sysfs kobject for this csrow */
-
/* channel information for this csrow */
u32 nr_channels;
struct rank_info *channels;
};
-struct mcidev_sysfs_group {
- const char *name; /* group name */
- const struct mcidev_sysfs_attribute *mcidev_attr; /* group attributes */
-};
-
-/* mcidev_sysfs_attribute structure
- * used for driver sysfs attributes and in mem_ctl_info
- * sysfs top level entries
- */
-struct mcidev_sysfs_attribute {
- /* It should use either attr or grp */
- struct attribute attr;
- const struct mcidev_sysfs_group *grp; /* Points to a group of attributes */
-
- /* Ops for show/store values at the attribute - not used on group */
- ssize_t (*show)(struct mem_ctl_info *, char *);
- ssize_t (*store)(struct mem_ctl_info *, const char *, size_t);
-
- void *priv;
-};
-
/*
* struct errcount_attribute - used to store the several error counts
*/
@@ -615,12 +591,6 @@ struct mem_ctl_info {
struct completion complete;
- /* edac sysfs device control */
- struct kobject edac_mci_kobj;
-
- /* list for all grp instances within a mc */
- struct list_head grp_kobj_list;
-
/* Additional top controller level attributes, but specified
* by the low level driver.
*
--
1.7.8
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH 06/14] edac: add a new per-dimm API and make the old per-virtual-rank API obsolete
2012-03-29 17:06 [PATCH 00/14] Fix the EDAC API Mauro Carvalho Chehab
` (4 preceding siblings ...)
2012-03-29 17:06 ` [PATCH 05/14] edac: Get rid of the old kobj's from the edac mc code Mauro Carvalho Chehab
@ 2012-03-29 17:06 ` Mauro Carvalho Chehab
2012-03-29 17:06 ` [PATCH 07/14] edac: add a sysfs node to report the maximum location for the system Mauro Carvalho Chehab
` (8 subsequent siblings)
14 siblings, 0 replies; 26+ messages in thread
From: Mauro Carvalho Chehab @ 2012-03-29 17:06 UTC (permalink / raw)
Cc: Mauro Carvalho Chehab, Linux Edac Mailing List,
Linux Kernel Mailing List, Greg K H
The old EDAC API is broken. It only works fine for systems manufatured
before 2005 and for AMD 64. The reason is that it forces all memory
controller drivers to discover rank info.
Also, it doesn't allow grouping the several ranks into a DIMM.
So, what almost all modern drivers do is to create a fake virtual-rank
information, and use it to cheat the EDAC core to accept the driver.
While this works if the user has enough time to discover what DIMM slot
corresponds to each "virtual-rank" information, it prevents EDAC usage
for users with less available time. It also makes life hard for vendors
that may want to provide a table with their motherboards to the userspace
tool (edac-utils) as each driver has its own logic for the virtual
mapping.
So, the old API should be removed, in favor of a more flexible API that
allows newer drivers to not lie to the EDAC core.
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
drivers/edac/Kconfig | 8 ++
drivers/edac/edac_mc.c | 47 +++++++++----
drivers/edac/edac_mc_sysfs.c | 165 +++++++++++++++++++++++++++++++++++++++++-
3 files changed, 205 insertions(+), 15 deletions(-)
diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index fdffa1b..3b3f84f 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -31,6 +31,14 @@ if EDAC
comment "Reporting subsystems"
+config EDAC_LEGACY_SYSFS
+ bool "EDAC legacy sysfs"
+ default y
+ help
+ Enable the compatibility sysfs nodes.
+ Use 'Y' if your edac utilities aren't ported to work with the newer
+ structures.
+
config EDAC_DEBUG
bool "Debugging"
help
diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index ced1d95..d4f9336 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -82,6 +82,8 @@ static void edac_mc_dump_csrow(struct csrow_info *csrow)
static void edac_mc_dump_mci(struct mem_ctl_info *mci)
{
+ const char *type = mci->mem_is_per_rank ? "ranks" : "dimms";
+
debugf3("\tmci = %p\n", mci);
debugf3("\tmci->mtype_cap = %lx\n", mci->mtype_cap);
debugf3("\tmci->edac_ctl_cap = %lx\n", mci->edac_ctl_cap);
@@ -89,8 +91,8 @@ static void edac_mc_dump_mci(struct mem_ctl_info *mci)
debugf4("\tmci->edac_check = %p\n", mci->edac_check);
debugf3("\tmci->num_csrows = %d, csrows = %p\n",
mci->num_csrows, mci->csrows);
- debugf3("\tmci->nr_dimms = %d, dimns = %p\n",
- mci->tot_dimms, mci->dimms);
+ debugf3("\tmci->nr_%s = %d, %s = %p\n",
+ type, mci->tot_dimms, type, mci->dimms);
debugf3("\tdev = %p\n", mci->pdev);
debugf3("\tmod_name:ctl_name = %s:%s\n", mci->mod_name, mci->ctl_name);
debugf3("\tpvt_info = %p\n\n", mci->pvt_info);
@@ -170,10 +172,6 @@ void *edac_align_ptr(void **p, unsigned size, int quant)
* @size_pvt: size of private storage needed
*
*
- * FIXME: drivers handle multi-rank memories on different ways: on some
- * drivers, one multi-rank memory is mapped as one DIMM, while, on others,
- * a single multi-rank DIMM would be mapped into several "dimms".
- *
* Non-csrow based drivers (like FB-DIMM and RAMBUS ones) will likely report
* such DIMMS properly, but the CSROWS-based ones will likely do the wrong
* thing, as two chip select values are used for dual-rank memories (and 4, for
@@ -188,6 +186,12 @@ void *edac_align_ptr(void **p, unsigned size, int quant)
*
* Use edac_mc_free() to free mc structures allocated by this function.
*
+ * NOTE: drivers handle multi-rank memories on different ways: on some
+ * drivers, one multi-rank memory is mapped as one entry, while, on others,
+ * a single multi-rank DIMM would be mapped into several entries. Currently,
+ * this function will allocate multiple struct dimm_info on such scenarios,
+ * as grouping the multiple ranks require drivers change.
+ *
* Returns:
* NULL allocation failed
* struct mem_ctl_info pointer
@@ -207,9 +211,10 @@ struct mem_ctl_info *edac_mc_alloc(unsigned edac_index,
u32 *ce_per_layer[EDAC_MAX_LAYERS], *ue_per_layer[EDAC_MAX_LAYERS];
void *pvt, *p;
unsigned size, tot_dimms, count, pos[EDAC_MAX_LAYERS];
- unsigned tot_csrows, tot_cschannels;
+ unsigned tot_csrows, tot_cschannels, tot_errcount = 0;
int i, j, n, len;
int row, chn;
+ bool per_rank = false;
BUG_ON(n_layers > EDAC_MAX_LAYERS);
/*
@@ -225,6 +230,9 @@ struct mem_ctl_info *edac_mc_alloc(unsigned edac_index,
tot_csrows *= layers[i].size;
else
tot_cschannels *= layers[i].size;
+
+ if (layers[i].type == EDAC_MC_LAYER_CHIP_SELECT)
+ per_rank = true;
}
/* Figure out the offsets of the various items from the start of an mc
@@ -241,14 +249,21 @@ struct mem_ctl_info *edac_mc_alloc(unsigned edac_index,
count = 1;
for (i = 0; i < n_layers; i++) {
count *= layers[i].size;
+ debugf4("%s: errcount layer %d size %d\n", __func__, i, count);
ce_per_layer[i] = edac_align_ptr(&ptr, sizeof(u32), count);
ue_per_layer[i] = edac_align_ptr(&ptr, sizeof(u32), count);
+ tot_errcount += 2 * count;
}
+
+ debugf4("%s: allocating %d error counters\n", __func__, tot_errcount);
pvt = edac_align_ptr(&ptr, sz_pvt, 1);
size = ((unsigned long)pvt) + sz_pvt;
- debugf1("%s(): allocating %u bytes for mci data (%d dimms, %d csrows/channels)\n",
- __func__, size, tot_dimms, tot_csrows * tot_cschannels);
+ debugf1("%s(): allocating %u bytes for mci data (%d %s, %d csrows/channels)\n",
+ __func__, size,
+ tot_dimms,
+ per_rank ? "ranks" : "dimms",
+ tot_csrows * tot_cschannels);
mci = kzalloc(size, GFP_KERNEL);
if (mci == NULL)
return NULL;
@@ -277,6 +292,7 @@ struct mem_ctl_info *edac_mc_alloc(unsigned edac_index,
memcpy(mci->layers, layers, sizeof(*lay) * n_layers);
mci->num_csrows = tot_csrows;
mci->num_cschannel = tot_cschannels;
+ mci->mem_is_per_rank = per_rank;
/*
* Fills the csrow struct
@@ -302,15 +318,16 @@ struct mem_ctl_info *edac_mc_alloc(unsigned edac_index,
memset(&pos, 0, sizeof(pos));
row = 0;
chn = 0;
- debugf4("%s: initializing %d dimms\n", __func__, tot_dimms);
+ debugf4("%s: initializing %d %s\n", __func__, tot_dimms,
+ per_rank ? "ranks" : "dimms");
for (i = 0; i < tot_dimms; i++) {
chan = &csi[row].channels[chn];
dimm = GET_POS(lay, mci->dimms, n_layers,
pos[0], pos[1], pos[2]);
dimm->mci = mci;
- debugf2("%s: %d: dimm%zd (%d:%d:%d): row %d, chan %d\n", __func__,
- i, (dimm - mci->dimms),
+ debugf2("%s: %d: %s%zd (%d:%d:%d): row %d, chan %d\n", __func__,
+ i, per_rank ? "rank" : "dimm", (dimm - mci->dimms),
pos[0], pos[1], pos[2], row, chn);
/*
@@ -982,8 +999,10 @@ void edac_mc_handle_error(const enum hw_event_mc_err_type type,
* get csrow/channel of the dimm, in order to allow
* incrementing the compat API counters
*/
- debugf4("%s: dimm csrows (%d,%d)\n",
- __func__, dimm->csrow, dimm->cschannel);
+ debugf4("%s: %s csrows map: (%d,%d)\n",
+ __func__,
+ mci->mem_is_per_rank ? "rank" : "dimm",
+ dimm->csrow, dimm->cschannel);
if (row == -1)
row = dimm->csrow;
else if (row >= 0 && row != dimm->csrow)
diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index 83e80cc..b0cd5d5 100644
--- a/drivers/edac/edac_mc_sysfs.c
+++ b/drivers/edac/edac_mc_sysfs.c
@@ -130,6 +130,7 @@ static const char *edac_caps[] = {
[EDAC_S16ECD16ED] = "S16ECD16ED"
};
+#ifdef CONFIG_EDAC_LEGACY_SYSFS
/*
* EDAC sysfs CSROW data structures and methods
*/
@@ -443,6 +444,159 @@ static void edac_delete_csrow_objects(struct mem_ctl_info *mci)
device_del(&mci->csrows[i].dev);
}
}
+#endif
+
+/*
+ * Per-dimm (or per-rank) devices
+ */
+
+#define to_dimm(k) container_of(k, struct dimm_info, dev)
+
+/* show/store functions for DIMM Label attributes */
+static ssize_t dimmdev_location_show(struct device *dev,
+ struct device_attribute *mattr, char *data)
+{
+ struct dimm_info *dimm = to_dimm(dev);
+ struct mem_ctl_info *mci = dimm->mci;
+ int i;
+ char *p = data;
+
+ for (i = 0; i < mci->n_layers; i++) {
+ p += sprintf(p, "%s %d ",
+ edac_layer_name[mci->layers[i].type],
+ dimm->location[i]);
+ }
+
+ return p - data;
+}
+
+static ssize_t dimmdev_label_show(struct device *dev,
+ struct device_attribute *mattr, char *data)
+{
+ struct dimm_info *dimm = to_dimm(dev);
+
+ /* if field has not been initialized, there is nothing to send */
+ if (!dimm->label[0])
+ return 0;
+
+ return snprintf(data, EDAC_MC_LABEL_LEN, "%s\n", dimm->label);
+}
+
+static ssize_t dimmdev_label_store(struct device *dev,
+ struct device_attribute *mattr,
+ const char *data,
+ size_t count)
+{
+ struct dimm_info *dimm = to_dimm(dev);
+
+ ssize_t max_size = 0;
+
+ max_size = min((ssize_t) count, (ssize_t) EDAC_MC_LABEL_LEN - 1);
+ strncpy(dimm->label, data, max_size);
+ dimm->label[max_size] = '\0';
+
+ return max_size;
+}
+
+static ssize_t dimmdev_size_show(struct device *dev,
+ struct device_attribute *mattr, char *data)
+{
+ struct dimm_info *dimm = to_dimm(dev);
+
+ return sprintf(data, "%u\n", PAGES_TO_MiB(dimm->nr_pages));
+}
+
+static ssize_t dimmdev_mem_type_show(struct device *dev,
+ struct device_attribute *mattr, char *data)
+{
+ struct dimm_info *dimm = to_dimm(dev);
+
+ return sprintf(data, "%s\n", mem_types[dimm->mtype]);
+}
+
+static ssize_t dimmdev_dev_type_show(struct device *dev,
+ struct device_attribute *mattr, char *data)
+{
+ struct dimm_info *dimm = to_dimm(dev);
+
+ return sprintf(data, "%s\n", dev_types[dimm->dtype]);
+}
+
+static ssize_t dimmdev_edac_mode_show(struct device *dev,
+ struct device_attribute *mattr,
+ char *data)
+{
+ struct dimm_info *dimm = to_dimm(dev);
+
+ return sprintf(data, "%s\n", edac_caps[dimm->edac_mode]);
+}
+
+/* dimm/rank attribute files */
+static DEVICE_ATTR(dimm_label, S_IRUGO | S_IWUSR,
+ dimmdev_label_show, dimmdev_label_store);
+static DEVICE_ATTR(dimm_location, S_IRUGO, dimmdev_location_show, NULL);
+static DEVICE_ATTR(size, S_IRUGO, dimmdev_size_show, NULL);
+static DEVICE_ATTR(dimm_mem_type, S_IRUGO, dimmdev_mem_type_show, NULL);
+static DEVICE_ATTR(dimm_dev_type, S_IRUGO, dimmdev_dev_type_show, NULL);
+static DEVICE_ATTR(dimm_edac_mode, S_IRUGO, dimmdev_edac_mode_show, NULL);
+
+/* attributes of the dimm<id>/rank<id> object */
+static struct attribute *dimm_attrs[] = {
+ &dev_attr_dimm_label.attr,
+ &dev_attr_dimm_location.attr,
+ &dev_attr_size.attr,
+ &dev_attr_dimm_mem_type.attr,
+ &dev_attr_dimm_dev_type.attr,
+ &dev_attr_dimm_edac_mode.attr,
+ NULL,
+};
+
+static struct attribute_group dimm_attr_grp = {
+ .attrs = dimm_attrs,
+};
+
+static const struct attribute_group *dimm_attr_groups[] = {
+ &dimm_attr_grp,
+ NULL
+};
+
+static void dimm_attr_release(struct device *device)
+{
+ debugf1("Releasing dimm device %s\n", dev_name(device));
+}
+
+static struct device_type dimm_attr_type = {
+ .groups = dimm_attr_groups,
+ .release = dimm_attr_release,
+};
+
+/* Create a DIMM object under specifed memory controller device */
+static int edac_create_dimm_object(struct mem_ctl_info *mci,
+ struct dimm_info *dimm,
+ int index)
+{
+ int err;
+ dimm->mci = mci;
+
+ dimm->dev.type = &dimm_attr_type;
+ dimm->dev.bus = mci_pdev.bus;
+ device_initialize(&dimm->dev);
+
+ dimm->dev.parent = &mci->dev;
+ if (mci->mem_is_per_rank)
+ dev_set_name(&dimm->dev, "rank%d", index);
+ else
+ dev_set_name(&dimm->dev, "dimm%d", index);
+ dev_set_drvdata(&dimm->dev, dimm);
+ pm_runtime_forbid(&mci->dev);
+
+ err = device_add(&dimm->dev);
+
+ debugf0("%s(): creating rank/dimm device %s\n", __func__,
+ dev_name(&dimm->dev));
+
+ return err;
+}
/*
* Memory controller device
@@ -663,7 +817,6 @@ static struct device_type mci_attr_type = {
.release = mci_attr_release,
};
-
/*
* Create a new Memory Controller kobject instance,
* mc<id> under the 'mc' directory
@@ -715,11 +868,19 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci)
printk(KERN_CONT "\n");
}
#endif
+ err = edac_create_dimm_object(mci, dimm, i);
+ if (err) {
+ debugf1("%s() failure: create dimm %d obj\n",
+ __func__, i);
+ goto fail;
+ }
}
+#ifdef CONFIG_EDAC_LEGACY_SYSFS
err = edac_create_csrow_objects(mci);
if (err < 0)
goto fail;
+#endif
return 0;
@@ -745,7 +906,9 @@ void edac_remove_sysfs_mci_device(struct mem_ctl_info *mci)
debugf0("%s()\n", __func__);
+#ifdef CONFIG_EDAC_LEGACY_SYSFS
edac_delete_csrow_objects(mci);
+#endif
for (i = 0; i < mci->tot_dimms; i++) {
struct dimm_info *dimm = &mci->dimms[i];
--
1.7.8
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH 07/14] edac: add a sysfs node to report the maximum location for the system
2012-03-29 17:06 [PATCH 00/14] Fix the EDAC API Mauro Carvalho Chehab
` (5 preceding siblings ...)
2012-03-29 17:06 ` [PATCH 06/14] edac: add a new per-dimm API and make the old per-virtual-rank API obsolete Mauro Carvalho Chehab
@ 2012-03-29 17:06 ` Mauro Carvalho Chehab
2012-03-29 17:06 ` [PATCH 08/14] edac: Add debufs nodes to allow doing fake error inject Mauro Carvalho Chehab
` (7 subsequent siblings)
14 siblings, 0 replies; 26+ messages in thread
From: Mauro Carvalho Chehab @ 2012-03-29 17:06 UTC (permalink / raw)
Cc: Mauro Carvalho Chehab, Linux Edac Mailing List,
Linux Kernel Mailing List, Greg K H
The userspace tools need to know what's the maximum location on each
system, as it helps to create nice maps showing how the memory was
filled at the system.
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
drivers/edac/edac_mc_sysfs.c | 19 +++++++++++++++++++
1 files changed, 19 insertions(+), 0 deletions(-)
diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index b0cd5d5..bdd81b4 100644
--- a/drivers/edac/edac_mc_sysfs.c
+++ b/drivers/edac/edac_mc_sysfs.c
@@ -769,6 +769,23 @@ static ssize_t mci_size_mb_show(struct device *dev,
return sprintf(data, "%u\n", PAGES_TO_MiB(total_pages));
}
+static ssize_t mci_max_location_show(struct device *dev,
+ struct device_attribute *mattr,
+ char *data)
+{
+ struct mem_ctl_info *mci = to_mci(dev);
+ int i;
+ char *p = data;
+
+ for (i = 0; i < mci->n_layers; i++) {
+ p += sprintf(p, "%s %d ",
+ edac_layer_name[mci->layers[i].type],
+ mci->layers[i].size - 1);
+ }
+
+ return p - data;
+}
+
/* default Control file */
DEVICE_ATTR(reset_counters, S_IWUSR, NULL, mci_reset_counters_store);
@@ -780,6 +797,7 @@ DEVICE_ATTR(ue_noinfo_count, S_IRUGO, mci_ue_noinfo_show, NULL);
DEVICE_ATTR(ce_noinfo_count, S_IRUGO, mci_ce_noinfo_show, NULL);
DEVICE_ATTR(ue_count, S_IRUGO, mci_ue_count_show, NULL);
DEVICE_ATTR(ce_count, S_IRUGO, mci_ce_count_show, NULL);
+DEVICE_ATTR(max_location, S_IRUGO, mci_max_location_show, NULL);
/* memory scrubber attribute file */
DEVICE_ATTR(sdram_scrub_rate, S_IRUGO | S_IWUSR, mci_sdram_scrub_rate_show,
@@ -795,6 +813,7 @@ static struct attribute *mci_attrs[] = {
&dev_attr_ue_count.attr,
&dev_attr_ce_count.attr,
&dev_attr_sdram_scrub_rate.attr,
+ &dev_attr_max_location.attr,
NULL
};
--
1.7.8
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH 08/14] edac: Add debufs nodes to allow doing fake error inject
2012-03-29 17:06 [PATCH 00/14] Fix the EDAC API Mauro Carvalho Chehab
` (6 preceding siblings ...)
2012-03-29 17:06 ` [PATCH 07/14] edac: add a sysfs node to report the maximum location for the system Mauro Carvalho Chehab
@ 2012-03-29 17:06 ` Mauro Carvalho Chehab
2012-03-29 17:06 ` [PATCH 09/14] edac: Create a per-Memory Controller bus Mauro Carvalho Chehab
` (6 subsequent siblings)
14 siblings, 0 replies; 26+ messages in thread
From: Mauro Carvalho Chehab @ 2012-03-29 17:06 UTC (permalink / raw)
Cc: Mauro Carvalho Chehab, Linux Edac Mailing List,
Linux Kernel Mailing List, Greg K H
Sometimes, it is useful to have a mechanism that generates fake
errors, in order to test the EDAC core code, and the userspace
tools.
Provide such mechanism by adding a few debugfs nodes.
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
drivers/edac/edac_mc_sysfs.c | 87 ++++++++++++++++++++++++++++++++++++++++++
include/linux/edac.h | 7 +++
2 files changed, 94 insertions(+), 0 deletions(-)
diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index bdd81b4..4ab2e19 100644
--- a/drivers/edac/edac_mc_sysfs.c
+++ b/drivers/edac/edac_mc_sysfs.c
@@ -17,6 +17,7 @@
#include <linux/edac.h>
#include <linux/bug.h>
#include <linux/pm_runtime.h>
+#include <linux/uaccess.h>
#include "edac_core.h"
#include "edac_module.h"
@@ -786,6 +787,47 @@ static ssize_t mci_max_location_show(struct device *dev,
return p - data;
}
+#ifdef CONFIG_EDAC_DEBUG
+static ssize_t edac_fake_inject_write(struct file *file,
+ const char __user *data,
+ size_t count, loff_t *ppos)
+{
+ struct device *dev = file->private_data;
+ struct mem_ctl_info *mci = to_mci(dev);
+ static enum hw_event_mc_err_type type;
+
+ type = mci->fake_inject_ue ? HW_EVENT_ERR_UNCORRECTED
+ : HW_EVENT_ERR_CORRECTED;
+
+ printk(KERN_DEBUG
+ "Generating a %s fake error to %d.%d.%d to test core handling. NOTE: this won't test the driver-specific decoding logic.\n",
+ (type == HW_EVENT_ERR_UNCORRECTED) ? "UE" : "CE",
+ mci->fake_inject_layer[0],
+ mci->fake_inject_layer[1],
+ mci->fake_inject_layer[2]
+ );
+ edac_mc_handle_error(type, mci, 0, 0, 0,
+ mci->fake_inject_layer[0],
+ mci->fake_inject_layer[1],
+ mci->fake_inject_layer[2],
+ "FAKE ERROR", "for EDAC testing only", NULL);
+
+ return count;
+}
+
+static int debugfs_open(struct inode *inode, struct file *file)
+{
+ file->private_data = inode->i_private;
+ return 0;
+}
+
+static const struct file_operations debug_fake_inject_fops = {
+ .open = debugfs_open,
+ .write = edac_fake_inject_write,
+ .llseek = generic_file_llseek,
+};
+#endif
+
/* default Control file */
DEVICE_ATTR(reset_counters, S_IWUSR, NULL, mci_reset_counters_store);
@@ -836,6 +878,45 @@ static struct device_type mci_attr_type = {
.release = mci_attr_release,
};
+#ifdef CONFIG_EDAC_DEBUG
+int edac_create_debug_nodes(struct mem_ctl_info *mci)
+{
+ struct dentry *d, *parent;
+ char name[80];
+ int i;
+
+ d = debugfs_create_dir(mci->dev.kobj.name, mci->debugfs);
+ if (!d)
+ return -ENOMEM;
+ parent = d;
+
+ for (i = 0; i < mci->n_layers; i++) {
+ sprintf(name, "fake_inject_%s",
+ edac_layer_name[mci->layers[i].type]);
+ d = debugfs_create_u8(name, S_IRUGO | S_IWUSR, parent,
+ &mci->fake_inject_layer[i]);
+ if (!d)
+ goto nomem;
+ }
+
+ d = debugfs_create_bool("fake_inject_ue", S_IRUGO | S_IWUSR, parent,
+ &mci->fake_inject_ue);
+ if (!d)
+ goto nomem;
+
+ d = debugfs_create_file("fake_inject", S_IWUSR, parent,
+ &mci->dev,
+ &debug_fake_inject_fops);
+ if (!d)
+ goto nomem;
+
+ return 0;
+nomem:
+ debugfs_remove(mci->debugfs);
+ return -ENOMEM;
+}
+#endif
+
/*
* Create a new Memory Controller kobject instance,
* mc<id> under the 'mc' directory
@@ -901,6 +982,9 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci)
goto fail;
#endif
+#ifdef CONFIG_EDAC_DEBUG
+ edac_create_debug_nodes(mci);
+#endif
return 0;
fail:
@@ -925,6 +1009,9 @@ void edac_remove_sysfs_mci_device(struct mem_ctl_info *mci)
debugf0("%s()\n", __func__);
+#ifdef CONFIG_EDAC_DEBUG
+ debugfs_remove(mci->debugfs);
+#endif
#ifdef CONFIG_EDAC_LEGACY_SYSFS
edac_delete_csrow_objects(mci);
#endif
diff --git a/include/linux/edac.h b/include/linux/edac.h
index 81ca2ba..3bfdcb9 100644
--- a/include/linux/edac.h
+++ b/include/linux/edac.h
@@ -14,6 +14,7 @@
#include <linux/atomic.h>
#include <linux/device.h>
+#include <linux/debugfs.h>
#define EDAC_OPSTATE_INVAL -1
#define EDAC_OPSTATE_POLL 0
@@ -608,6 +609,12 @@ struct mem_ctl_info {
/* the internal state of this controller instance */
int op_state;
+
+#ifdef CONFIG_EDAC_DEBUG
+ struct dentry *debugfs;
+ u8 fake_inject_layer[EDAC_MAX_LAYERS];
+ u32 fake_inject_ue;
+#endif
};
#endif
--
1.7.8
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH 09/14] edac: Create a per-Memory Controller bus
2012-03-29 17:06 [PATCH 00/14] Fix the EDAC API Mauro Carvalho Chehab
` (7 preceding siblings ...)
2012-03-29 17:06 ` [PATCH 08/14] edac: Add debufs nodes to allow doing fake error inject Mauro Carvalho Chehab
@ 2012-03-29 17:06 ` Mauro Carvalho Chehab
2012-03-29 17:06 ` [PATCH 10/14] edac: Move grain/dtype/edac_type calculus to be out of channel loop Mauro Carvalho Chehab
` (5 subsequent siblings)
14 siblings, 0 replies; 26+ messages in thread
From: Mauro Carvalho Chehab @ 2012-03-29 17:06 UTC (permalink / raw)
Cc: Mauro Carvalho Chehab, Linux Edac Mailing List,
Linux Kernel Mailing List, Greg K H
I'm getting this bug on machines with more than one memory controller:
[ 819.094946] EDAC DEBUG: find_mci_by_dev: find_mci_by_dev()
[ 819.094948] EDAC DEBUG: edac_create_sysfs_mci_device: edac_create_sysfs_mci_device() idx=1
[ 819.094952] EDAC DEBUG: edac_create_sysfs_mci_device: edac_create_sysfs_mci_device(): creating device mc1
[ 819.094967] EDAC DEBUG: edac_create_sysfs_mci_device: edac_create_sysfs_mci_device creating dimm0, located at channel 0 slot 0
[ 819.094984] ------------[ cut here ]------------
[ 819.100142] WARNING: at fs/sysfs/dir.c:481 sysfs_add_one+0xc1/0xf0()
[ 819.107282] Hardware name: S2600CP
[ 819.111078] sysfs: cannot create duplicate filename '/bus/edac/devices/dimm0'
[ 819.119062] Modules linked in: sb_edac(+) edac_core ip6table_filter ip6_tables ebtable_nat ebtables ipt_MASQUERADE iptable_nat nf_nat nf_conntrack_ipv4 nf_defrag_ipv4 xt_state nf_conntrack ipt_REJECT xt_CHECKSUM iptable_mangle iptable_filter ip_tables bridge stp llc sunrpc binfmt_misc dm_mirror dm_region_hash dm_log vhost_net macvtap macvlan tun kvm microcode pcspkr iTCO_wdt iTCO_vendor_support igb i2c_i801 i2c_core sg ioatdma dca sr_mod cdrom sd_mod crc_t10dif ahci libahci isci libsas libata scsi_transport_sas scsi_mod wmi dm_mod [last unloaded: scsi_wait_scan]
[ 819.175748] Pid: 10902, comm: modprobe Not tainted 3.3.0-0.11.el7.v12.2.x86_64 #1
[ 819.184113] Call Trace:
[ 819.186868] [<ffffffff8105adaf>] warn_slowpath_common+0x7f/0xc0
[ 819.193573] [<ffffffff8105aea6>] warn_slowpath_fmt+0x46/0x50
[ 819.200000] [<ffffffff811f53d1>] sysfs_add_one+0xc1/0xf0
[ 819.206025] [<ffffffff811f5cf5>] sysfs_do_create_link+0x135/0x220
[ 819.212944] [<ffffffff811f7023>] ? sysfs_create_group+0x13/0x20
[ 819.219656] [<ffffffff811f5df3>] sysfs_create_link+0x13/0x20
[ 819.226109] [<ffffffff813b04f6>] bus_add_device+0xe6/0x1b0
[ 819.232350] [<ffffffff813ae7cb>] device_add+0x2db/0x460
[ 819.238300] [<ffffffffa0325634>] edac_create_dimm_object+0x84/0xf0 [edac_core]
[ 819.246460] [<ffffffffa0325e18>] edac_create_sysfs_mci_device+0xe8/0x290 [edac_core]
[ 819.255215] [<ffffffffa0322e2a>] edac_mc_add_mc+0x5a/0x2c0 [edac_core]
[ 819.262611] [<ffffffffa03412df>] sbridge_register_mci+0x1bc/0x279 [sb_edac]
[ 819.270493] [<ffffffffa03417a3>] sbridge_probe+0xef/0x175 [sb_edac]
[ 819.277630] [<ffffffff813ba4e8>] ? pm_runtime_enable+0x58/0x90
[ 819.284268] [<ffffffff812f430c>] local_pci_probe+0x5c/0xd0
[ 819.290508] [<ffffffff812f5ba1>] __pci_device_probe+0xf1/0x100
[ 819.297117] [<ffffffff812f5bea>] pci_device_probe+0x3a/0x60
[ 819.303457] [<ffffffff813b1003>] really_probe+0x73/0x270
[ 819.309496] [<ffffffff813b138e>] driver_probe_device+0x4e/0xb0
[ 819.316104] [<ffffffff813b149b>] __driver_attach+0xab/0xb0
[ 819.322337] [<ffffffff813b13f0>] ? driver_probe_device+0xb0/0xb0
[ 819.329151] [<ffffffff813af5d6>] bus_for_each_dev+0x56/0x90
[ 819.335489] [<ffffffff813b0d7e>] driver_attach+0x1e/0x20
[ 819.341534] [<ffffffff813b0980>] bus_add_driver+0x1b0/0x2a0
[ 819.347884] [<ffffffffa0347000>] ? 0xffffffffa0346fff
[ 819.353641] [<ffffffff813b19f6>] driver_register+0x76/0x140
[ 819.359980] [<ffffffff8159f18b>] ? printk+0x51/0x53
[ 819.365524] [<ffffffffa0347000>] ? 0xffffffffa0346fff
[ 819.371291] [<ffffffff812f5896>] __pci_register_driver+0x56/0xd0
[ 819.378096] [<ffffffffa0347054>] sbridge_init+0x54/0x1000 [sb_edac]
[ 819.385231] [<ffffffff8100203f>] do_one_initcall+0x3f/0x170
[ 819.391577] [<ffffffff810bcd2e>] sys_init_module+0xbe/0x230
[ 819.397926] [<ffffffff815bb529>] system_call_fastpath+0x16/0x1b
[ 819.404633] ---[ end trace 1654fdd39556689f ]---
This is happening because the bus is not being properly initialized.
Instead of putting the memory sub-devices inside the memory controller,
it is putting everything under the same directory:
$ tree /sys/bus/edac/
/sys/bus/edac/
├── devices
│ ├── all_channel_counts -> ../../../devices/system/edac/mc/mc0/all_channel_counts
│ ├── csrow0 -> ../../../devices/system/edac/mc/mc0/csrow0
│ ├── csrow1 -> ../../../devices/system/edac/mc/mc0/csrow1
│ ├── csrow2 -> ../../../devices/system/edac/mc/mc0/csrow2
│ ├── dimm0 -> ../../../devices/system/edac/mc/mc0/dimm0
│ ├── dimm1 -> ../../../devices/system/edac/mc/mc0/dimm1
│ ├── dimm3 -> ../../../devices/system/edac/mc/mc0/dimm3
│ ├── dimm6 -> ../../../devices/system/edac/mc/mc0/dimm6
│ ├── inject_addrmatch -> ../../../devices/system/edac/mc/mc0/inject_addrmatch
│ ├── mc -> ../../../devices/system/edac/mc
│ └── mc0 -> ../../../devices/system/edac/mc/mc0
├── drivers
├── drivers_autoprobe
├── drivers_probe
└── uevent
On a multi-memory controller system, the names "csrow%d" and "dimm%d"
should be under "mc%d", and not at the main hierarchy level.
So, we need to create a per-MC bus, in order to have its own namespace.
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
drivers/edac/edac_mc_sysfs.c | 25 +++++++++++++++++++++----
include/linux/edac.h | 1 +
2 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index 4ab2e19..319612c 100644
--- a/drivers/edac/edac_mc_sysfs.c
+++ b/drivers/edac/edac_mc_sysfs.c
@@ -357,7 +357,7 @@ static int edac_create_csrow_object(struct mem_ctl_info *mci,
return -ENODEV;
csrow->dev.type = &csrow_attr_type;
- csrow->dev.bus = mci_pdev.bus;
+ csrow->dev.bus = &mci->bus;
device_initialize(&csrow->dev);
csrow->dev.parent = &mci->dev;
dev_set_name(&csrow->dev, "csrow%d", index);
@@ -580,7 +580,7 @@ static int edac_create_dimm_object(struct mem_ctl_info *mci,
dimm->mci = mci;
dimm->dev.type = &dimm_attr_type;
- dimm->dev.bus = mci_pdev.bus;
+ dimm->dev.bus = &mci->bus;
device_initialize(&dimm->dev);
dimm->dev.parent = &mci->dev;
@@ -937,16 +937,29 @@ int edac_create_sysfs_mci_device(struct mem_ctl_info *mci)
device_initialize(&mci->dev);
mci->dev.parent = &mci_pdev;
- mci->dev.bus = mci_pdev.bus;
+ mci->dev.bus = &mci->bus;
dev_set_name(&mci->dev, "mc%d", mci->mc_idx);
dev_set_drvdata(&mci->dev, mci);
pm_runtime_forbid(&mci->dev);
+ /*
+ * The memory controller needs its own bus, in order to avoid
+ * namespace conflicts at /sys/bus/edac.
+ */
+ debugf0("creating bus %s\n",mci->bus.name);
+ mci->bus.name = kstrdup(dev_name(&mci->dev), GFP_KERNEL);
+ err = bus_register(&mci->bus);
+ if (err < 0)
+ return err;
+
debugf0("%s(): creating device %s\n", __func__,
dev_name(&mci->dev));
err = device_add(&mci->dev);
- if (err < 0)
+ if (err < 0) {
+ bus_unregister(&mci->bus);
+ kfree(mci->bus.name);
return err;
+ }
/*
* Create the dimm/rank devices
@@ -997,6 +1010,8 @@ fail:
}
put_device(&mci->dev);
device_del(&mci->dev);
+ bus_unregister(&mci->bus);
+ kfree(mci->bus.name);
return err;
}
@@ -1032,6 +1047,8 @@ void edac_unregister_sysfs(struct mem_ctl_info *mci)
debugf1("Unregistering device %s\n", dev_name(&mci->dev));
put_device(&mci->dev);
device_del(&mci->dev);
+ bus_unregister(&mci->bus);
+ kfree(mci->bus.name);
}
static void mc_attr_release(struct device *device)
diff --git a/include/linux/edac.h b/include/linux/edac.h
index 3bfdcb9..0e2cd0b4 100644
--- a/include/linux/edac.h
+++ b/include/linux/edac.h
@@ -507,6 +507,7 @@ struct edac_hierarchy {
*/
struct mem_ctl_info {
struct device dev;
+ struct bus_type bus;
struct list_head link; /* for global list of mem_ctl_info structs */
--
1.7.8
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH 10/14] edac: Move grain/dtype/edac_type calculus to be out of channel loop
2012-03-29 17:06 [PATCH 00/14] Fix the EDAC API Mauro Carvalho Chehab
` (8 preceding siblings ...)
2012-03-29 17:06 ` [PATCH 09/14] edac: Create a per-Memory Controller bus Mauro Carvalho Chehab
@ 2012-03-29 17:06 ` Mauro Carvalho Chehab
2012-03-29 17:06 ` [PATCH 11/14] i82975x_edac: Test nr_pages earlier to save a few CPU cycles Mauro Carvalho Chehab
` (4 subsequent siblings)
14 siblings, 0 replies; 26+ messages in thread
From: Mauro Carvalho Chehab @ 2012-03-29 17:06 UTC (permalink / raw)
Cc: Mauro Carvalho Chehab, Linux Edac Mailing List,
Linux Kernel Mailing List
The 3e7bddc changeset (edac: move dimm properties to struct memset_info)
moved the calculus inside a loop. However, at those stuff are common to
all channels, on several drivers, it is better to put the calculus
outside the loop, to optimize the code.
Reported-by: Aristeu Rozanski Filho <arozansk@redhat.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
drivers/edac/cpc925_edac.c | 54 ++++++++++++++++++++++----------------------
drivers/edac/e752x_edac.c | 31 +++++++++++++------------
drivers/edac/e7xxx_edac.c | 32 +++++++++++++------------
3 files changed, 60 insertions(+), 57 deletions(-)
diff --git a/drivers/edac/cpc925_edac.c b/drivers/edac/cpc925_edac.c
index 794cec6..a55abe7 100644
--- a/drivers/edac/cpc925_edac.c
+++ b/drivers/edac/cpc925_edac.c
@@ -330,8 +330,9 @@ static void cpc925_init_csrows(struct mem_ctl_info *mci)
struct cpc925_mc_pdata *pdata = mci->pvt_info;
struct csrow_info *csrow;
struct dimm_info *dimm;
+ enum dev_type dtype;
int index, j;
- u32 mbmr, mbbar, bba;
+ u32 mbmr, mbbar, bba, grain;
unsigned long row_size, nr_pages, last_nr_pages = 0;
get_total_mem(pdata);
@@ -355,37 +356,36 @@ static void cpc925_init_csrows(struct mem_ctl_info *mci)
csrow->last_page = csrow->first_page + nr_pages - 1;
last_nr_pages = csrow->last_page + 1;
+ switch (csrow->nr_channels) {
+ case 1: /* Single channel */
+ grain = 32; /* four-beat burst of 32 bytes */
+ break;
+ case 2: /* Dual channel */
+ default:
+ grain = 64; /* four-beat burst of 64 bytes */
+ break;
+ }
+ switch ((mbmr & MBMR_MODE_MASK) >> MBMR_MODE_SHIFT) {
+ case 6: /* 0110, no way to differentiate X8 VS X16 */
+ case 5: /* 0101 */
+ case 8: /* 1000 */
+ dtype = DEV_X16;
+ break;
+ case 7: /* 0111 */
+ case 9: /* 1001 */
+ dtype = DEV_X8;
+ break;
+ default:
+ dtype = DEV_UNKNOWN;
+ break;
+ }
for (j = 0; j < csrow->nr_channels; j++) {
dimm = csrow->channels[j].dimm;
-
dimm->nr_pages = nr_pages / csrow->nr_channels;
dimm->mtype = MEM_RDDR;
dimm->edac_mode = EDAC_SECDED;
-
- switch (csrow->nr_channels) {
- case 1: /* Single channel */
- dimm->grain = 32; /* four-beat burst of 32 bytes */
- break;
- case 2: /* Dual channel */
- default:
- dimm->grain = 64; /* four-beat burst of 64 bytes */
- break;
- }
-
- switch ((mbmr & MBMR_MODE_MASK) >> MBMR_MODE_SHIFT) {
- case 6: /* 0110, no way to differentiate X8 VS X16 */
- case 5: /* 0101 */
- case 8: /* 1000 */
- dimm->dtype = DEV_X16;
- break;
- case 7: /* 0111 */
- case 9: /* 1001 */
- dimm->dtype = DEV_X8;
- break;
- default:
- dimm->dtype = DEV_UNKNOWN;
- break;
- }
+ dimm->grain = grain;
+ dimm->dtype = dtype;
}
}
}
diff --git a/drivers/edac/e752x_edac.c b/drivers/edac/e752x_edac.c
index 86428be..284526a 100644
--- a/drivers/edac/e752x_edac.c
+++ b/drivers/edac/e752x_edac.c
@@ -1069,6 +1069,7 @@ static void e752x_init_csrows(struct mem_ctl_info *mci, struct pci_dev *pdev,
u16 ddrcsr)
{
struct csrow_info *csrow;
+ enum edac_type edac_mode;
unsigned long last_cumul_size;
int index, mem_dev, drc_chan;
int drc_drbg; /* DRB granularity 0=64mb, 1=128mb */
@@ -1111,6 +1112,20 @@ static void e752x_init_csrows(struct mem_ctl_info *mci, struct pci_dev *pdev,
nr_pages = cumul_size - last_cumul_size;
last_cumul_size = cumul_size;
+ /*
+ * if single channel or x8 devices then SECDED
+ * if dual channel and x4 then S4ECD4ED
+ */
+ if (drc_ddim) {
+ if (drc_chan && mem_dev) {
+ edac_mode = EDAC_S4ECD4ED;
+ mci->edac_cap |= EDAC_FLAG_S4ECD4ED;
+ } else {
+ edac_mode = EDAC_SECDED;
+ mci->edac_cap |= EDAC_FLAG_SECDED;
+ }
+ } else
+ edac_mode = EDAC_NONE;
for (i = 0; i < csrow->nr_channels; i++) {
struct dimm_info *dimm = csrow->channels[i].dimm;
@@ -1119,21 +1134,7 @@ static void e752x_init_csrows(struct mem_ctl_info *mci, struct pci_dev *pdev,
dimm->grain = 1 << 12; /* 4KiB - resolution of CELOG */
dimm->mtype = MEM_RDDR; /* only one type supported */
dimm->dtype = mem_dev ? DEV_X4 : DEV_X8;
-
- /*
- * if single channel or x8 devices then SECDED
- * if dual channel and x4 then S4ECD4ED
- */
- if (drc_ddim) {
- if (drc_chan && mem_dev) {
- dimm->edac_mode = EDAC_S4ECD4ED;
- mci->edac_cap |= EDAC_FLAG_S4ECD4ED;
- } else {
- dimm->edac_mode = EDAC_SECDED;
- mci->edac_cap |= EDAC_FLAG_SECDED;
- }
- } else
- dimm->edac_mode = EDAC_NONE;
+ dimm->edac_mode = edac_mode;
}
}
}
diff --git a/drivers/edac/e7xxx_edac.c b/drivers/edac/e7xxx_edac.c
index 49e0aec..f0a4d52 100644
--- a/drivers/edac/e7xxx_edac.c
+++ b/drivers/edac/e7xxx_edac.c
@@ -362,6 +362,7 @@ static void e7xxx_init_csrows(struct mem_ctl_info *mci, struct pci_dev *pdev,
int drc_chan, drc_drbg, drc_ddim, mem_dev;
struct csrow_info *csrow;
struct dimm_info *dimm;
+ enum edac_type edac_mode;
pci_read_config_dword(pdev, E7XXX_DRA, &dra);
drc_chan = dual_channel_active(drc, dev_idx);
@@ -392,6 +393,21 @@ static void e7xxx_init_csrows(struct mem_ctl_info *mci, struct pci_dev *pdev,
nr_pages = cumul_size - last_cumul_size;
last_cumul_size = cumul_size;
+ /*
+ * if single channel or x8 devices then SECDED
+ * if dual channel and x4 then S4ECD4ED
+ */
+ if (drc_ddim) {
+ if (drc_chan && mem_dev) {
+ edac_mode = EDAC_S4ECD4ED;
+ mci->edac_cap |= EDAC_FLAG_S4ECD4ED;
+ } else {
+ edac_mode = EDAC_SECDED;
+ mci->edac_cap |= EDAC_FLAG_SECDED;
+ }
+ } else
+ edac_mode = EDAC_NONE;
+
for (j = 0; j < drc_chan + 1; j++) {
dimm = csrow->channels[j].dimm;
@@ -399,21 +415,7 @@ static void e7xxx_init_csrows(struct mem_ctl_info *mci, struct pci_dev *pdev,
dimm->grain = 1 << 12; /* 4KiB - resolution of CELOG */
dimm->mtype = MEM_RDDR; /* only one type supported */
dimm->dtype = mem_dev ? DEV_X4 : DEV_X8;
-
- /*
- * if single channel or x8 devices then SECDED
- * if dual channel and x4 then S4ECD4ED
- */
- if (drc_ddim) {
- if (drc_chan && mem_dev) {
- dimm->edac_mode = EDAC_S4ECD4ED;
- mci->edac_cap |= EDAC_FLAG_S4ECD4ED;
- } else {
- dimm->edac_mode = EDAC_SECDED;
- mci->edac_cap |= EDAC_FLAG_SECDED;
- }
- } else
- dimm->edac_mode = EDAC_NONE;
+ dimm->edac_mode = edac_mode;
}
}
}
--
1.7.8
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH 11/14] i82975x_edac: Test nr_pages earlier to save a few CPU cycles
2012-03-29 17:06 [PATCH 00/14] Fix the EDAC API Mauro Carvalho Chehab
` (9 preceding siblings ...)
2012-03-29 17:06 ` [PATCH 10/14] edac: Move grain/dtype/edac_type calculus to be out of channel loop Mauro Carvalho Chehab
@ 2012-03-29 17:06 ` Mauro Carvalho Chehab
2012-03-29 17:06 ` [PATCH 12/14] i5100_edac: Fix a warning when compiled with 32 bits Mauro Carvalho Chehab
` (3 subsequent siblings)
14 siblings, 0 replies; 26+ messages in thread
From: Mauro Carvalho Chehab @ 2012-03-29 17:06 UTC (permalink / raw)
Cc: Mauro Carvalho Chehab, Linux Edac Mailing List,
Linux Kernel Mailing List
Avoid test nr_pages twice, and initializing some data that won't
be used.
Cleanup patch only.
Reported-by: Aristeu Rozanski Filho <arozansk@redhat.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
drivers/edac/i82975x_edac.c | 9 +++------
1 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/edac/i82975x_edac.c b/drivers/edac/i82975x_edac.c
index 84e5fcc..6b0fd31 100644
--- a/drivers/edac/i82975x_edac.c
+++ b/drivers/edac/i82975x_edac.c
@@ -410,6 +410,9 @@ static void i82975x_init_csrows(struct mem_ctl_info *mci,
cumul_size);
nr_pages = cumul_size - last_cumul_size;
+ if (!nr_pages)
+ continue;
+
/*
* Initialise dram labels
* index values:
@@ -420,9 +423,6 @@ static void i82975x_init_csrows(struct mem_ctl_info *mci,
for (chan = 0; chan < csrow->nr_channels; chan++) {
dimm = mci->csrows[index].channels[chan].dimm;
- if (!nr_pages)
- continue;
-
dimm->nr_pages = nr_pages / csrow->nr_channels;
strncpy(csrow->channels[chan].dimm->label,
labels[(index >> 1) + (chan * 2)],
@@ -433,9 +433,6 @@ static void i82975x_init_csrows(struct mem_ctl_info *mci,
dimm->edac_mode = EDAC_SECDED; /* only supported */
}
- if (!nr_pages)
- continue; /* not populated */
-
csrow->first_page = last_cumul_size;
csrow->last_page = cumul_size - 1;
last_cumul_size = cumul_size;
--
1.7.8
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH 12/14] i5100_edac: Fix a warning when compiled with 32 bits
2012-03-29 17:06 [PATCH 00/14] Fix the EDAC API Mauro Carvalho Chehab
` (10 preceding siblings ...)
2012-03-29 17:06 ` [PATCH 11/14] i82975x_edac: Test nr_pages earlier to save a few CPU cycles Mauro Carvalho Chehab
@ 2012-03-29 17:06 ` Mauro Carvalho Chehab
2012-03-29 17:07 ` [PATCH 13/14] i7300_edac: Get rid of some wrongly-solved rebase conflict Mauro Carvalho Chehab
` (2 subsequent siblings)
14 siblings, 0 replies; 26+ messages in thread
From: Mauro Carvalho Chehab @ 2012-03-29 17:06 UTC (permalink / raw)
Cc: Mauro Carvalho Chehab, Linux Edac Mailing List,
Linux Kernel Mailing List
drivers/edac/i5100_edac.c: In function ‘i5100_init_csrows’:
drivers/edac/i5100_edac.c:862:3: warning: format ‘%zd’ expects argument of type ‘signed size_t’, but argument 5 has type ‘long unsigned int’ [-Wformat]
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
drivers/edac/i5100_edac.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/edac/i5100_edac.c b/drivers/edac/i5100_edac.c
index 11aba5e..dd260c8 100644
--- a/drivers/edac/i5100_edac.c
+++ b/drivers/edac/i5100_edac.c
@@ -859,8 +859,8 @@ static void __devinit i5100_init_csrows(struct mem_ctl_info *mci)
i5100_rank_to_slot(mci, chan, rank));
}
- debugf2("dimm channel %d, rank %d, size %zd\n",
- chan, rank, PAGES_TO_MiB(npages));
+ debugf2("dimm channel %d, rank %d, size %ld\n",
+ chan, rank, (long)PAGES_TO_MiB(npages));
}
}
--
1.7.8
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH 13/14] i7300_edac: Get rid of some wrongly-solved rebase conflict
2012-03-29 17:06 [PATCH 00/14] Fix the EDAC API Mauro Carvalho Chehab
` (11 preceding siblings ...)
2012-03-29 17:06 ` [PATCH 12/14] i5100_edac: Fix a warning when compiled with 32 bits Mauro Carvalho Chehab
@ 2012-03-29 17:07 ` Mauro Carvalho Chehab
2012-03-29 17:07 ` [PATCH 14/14] edac: Only expose csrows/channels on legacy API if they're populated Mauro Carvalho Chehab
2012-03-29 20:46 ` [PATCH 00/14] Fix the EDAC API Aristeu Rozanski Filho
14 siblings, 0 replies; 26+ messages in thread
From: Mauro Carvalho Chehab @ 2012-03-29 17:07 UTC (permalink / raw)
Cc: Mauro Carvalho Chehab, Linux Edac Mailing List,
Linux Kernel Mailing List
Drivers should not increment tot_dimms manually. This code were
there for an older approach that got removed. After one of the
several rebases that this code suffered until the final version,
the conflict was wrongly merged, and this code returned.
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
drivers/edac/i7300_edac.c | 3 ---
1 files changed, 0 insertions(+), 3 deletions(-)
diff --git a/drivers/edac/i7300_edac.c b/drivers/edac/i7300_edac.c
index 57f264d..f9a4fa4 100644
--- a/drivers/edac/i7300_edac.c
+++ b/drivers/edac/i7300_edac.c
@@ -803,9 +803,6 @@ static int i7300_init_csrows(struct mem_ctl_info *mci)
mtr = decode_mtr(pvt, slot, ch, branch,
dinfo, dimm);
- mci->tot_dimms++;
- dimm++;
-
/* if no DIMMS on this row, continue */
if (!MTR_DIMMS_PRESENT(mtr))
continue;
--
1.7.8
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH 14/14] edac: Only expose csrows/channels on legacy API if they're populated
2012-03-29 17:06 [PATCH 00/14] Fix the EDAC API Mauro Carvalho Chehab
` (12 preceding siblings ...)
2012-03-29 17:07 ` [PATCH 13/14] i7300_edac: Get rid of some wrongly-solved rebase conflict Mauro Carvalho Chehab
@ 2012-03-29 17:07 ` Mauro Carvalho Chehab
2012-03-29 20:46 ` [PATCH 00/14] Fix the EDAC API Aristeu Rozanski Filho
14 siblings, 0 replies; 26+ messages in thread
From: Mauro Carvalho Chehab @ 2012-03-29 17:07 UTC (permalink / raw)
Cc: Mauro Carvalho Chehab, Linux Edac Mailing List,
Linux Kernel Mailing List, Greg K H
This patch actually fixes a bug with the legacy API, where, at the
same csrow, some channels may have different DIMMs. This can happen
on FB-DIMM/RAMBUS and modern Intel controllers.
This is the case, for example, of Nehalem machines:
$ ./edac-ctl --layout
+-----------------------------------+
| mc0 |
| channel0 | channel1 | channel2 |
-------+-----------------------------------+
slot2: | 0 MB | 0 MB | 0 MB |
slot1: | 1024 MB | 0 MB | 0 MB |
slot0: | 1024 MB | 1024 MB | 1024 MB |
-------+-----------------------------------+
Before this patch, non-filled memories were shown. Now, only what's
filled is there:
grep . /sys/devices/system/edac/mc/mc0/csrow*/ch?*
/sys/devices/system/edac/mc/mc0/csrow0/ch0_ce_count:0
/sys/devices/system/edac/mc/mc0/csrow0/ch0_dimm_label:CPU#0Channel#0_DIMM#0
/sys/devices/system/edac/mc/mc0/csrow0/ch1_ce_count:0
/sys/devices/system/edac/mc/mc0/csrow0/ch1_dimm_label:CPU#0Channel#0_DIMM#1
/sys/devices/system/edac/mc/mc0/csrow1/ch0_ce_count:0
/sys/devices/system/edac/mc/mc0/csrow1/ch0_dimm_label:CPU#0Channel#1_DIMM#0
/sys/devices/system/edac/mc/mc0/csrow2/ch0_ce_count:0
/sys/devices/system/edac/mc/mc0/csrow2/ch0_dimm_label:CPU#0Channel#2_DIMM#0
Thanks-to: Aristeu Rozanski Filho <arozansk@redhat.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
---
drivers/edac/edac_mc_sysfs.c | 24 ++++++++++++++++++++++++
1 files changed, 24 insertions(+), 0 deletions(-)
diff --git a/drivers/edac/edac_mc_sysfs.c b/drivers/edac/edac_mc_sysfs.c
index 319612c..112fff2 100644
--- a/drivers/edac/edac_mc_sysfs.c
+++ b/drivers/edac/edac_mc_sysfs.c
@@ -347,6 +347,16 @@ static struct device_attribute *dynamic_csrow_ce_count_attr[] = {
&dev_attr_legacy_ch5_ce_count.attr
};
+static inline int nr_pages_per_csrow(struct csrow_info *csrow)
+{
+ int chan, nr_pages = 0;
+
+ for (chan = 0; chan < csrow->nr_channels; chan++)
+ nr_pages += csrow->channels[chan].dimm->nr_pages;
+
+ return nr_pages;
+}
+
/* Create a CSROW object under specifed edac_mc_device */
static int edac_create_csrow_object(struct mem_ctl_info *mci,
struct csrow_info *csrow, int index)
@@ -371,6 +381,9 @@ static int edac_create_csrow_object(struct mem_ctl_info *mci,
return err;
for (chan = 0; chan < csrow->nr_channels; chan++) {
+ /* Only expose populated DIMMs */
+ if (!csrow->channels[chan].dimm->nr_pages)
+ continue;
err = device_create_file(&csrow->dev,
dynamic_csrow_dimm_attr[chan]);
if (err < 0)
@@ -405,6 +418,9 @@ static int edac_create_csrow_objects(struct mem_ctl_info *mci)
struct csrow_info *csrow;
for (i = 0; i < mci->num_csrows; i++) {
+ csrow = &mci->csrows[i];
+ if (!nr_pages_per_csrow(csrow))
+ continue;
err = edac_create_csrow_object(mci, &mci->csrows[i], i);
if (err < 0)
goto error;
@@ -414,7 +430,11 @@ static int edac_create_csrow_objects(struct mem_ctl_info *mci)
error:
for (--i; i >= 0; i--) {
csrow = &mci->csrows[i];
+ if (!nr_pages_per_csrow(csrow))
+ continue;
for (chan = csrow->nr_channels - 1; chan >= 0; chan--) {
+ if (!csrow->channels[chan].dimm->nr_pages)
+ continue;
device_remove_file(&csrow->dev,
dynamic_csrow_dimm_attr[chan]);
device_remove_file(&csrow->dev,
@@ -433,7 +453,11 @@ static void edac_delete_csrow_objects(struct mem_ctl_info *mci)
for (i = mci->num_csrows - 1; i >= 0; i--) {
csrow = &mci->csrows[i];
+ if (!nr_pages_per_csrow(csrow))
+ continue;
for (chan = csrow->nr_channels - 1; chan >= 0; chan--) {
+ if (!csrow->channels[chan].dimm->nr_pages)
+ continue;
debugf1("Removing csrow %d channel %d sysfs nodes\n",
i, chan);
device_remove_file(&csrow->dev,
--
1.7.8
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH 00/14] Fix the EDAC API
2012-03-29 17:06 [PATCH 00/14] Fix the EDAC API Mauro Carvalho Chehab
` (13 preceding siblings ...)
2012-03-29 17:07 ` [PATCH 14/14] edac: Only expose csrows/channels on legacy API if they're populated Mauro Carvalho Chehab
@ 2012-03-29 20:46 ` Aristeu Rozanski Filho
14 siblings, 0 replies; 26+ messages in thread
From: Aristeu Rozanski Filho @ 2012-03-29 20:46 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Linux Edac Mailing List, Linux Kernel Mailing List, Greg K H
On Thu, Mar 29, 2012 at 02:06:47PM -0300, Mauro Carvalho Chehab wrote:
> The EDAC API is broken for any memory controller that doesn't use
> a DIMM rank as its primary unit.
>
> That covers RAMBUS and FB-DIMM drivers, where it is impossible to
> track a single rank, as the hank is hidden by a buffer controller
> (AMB - Advanced Memory Buffer, in the case of FB-DIMM).
>
> Also, newer Intel architectures (Nehalem and Sandy Bridge) brings
> advanced memory controllers, where the cachesize can be different
> than 128 bits, and up to 4 channels can be interlaced. The current
> EDAC API doesn't work for those.
>
> So, all drivers that need that do some sort of tricks to lie to the
> EDAC core, in order for the memory to be somehow exposed. There are
> several cases where this is done wrong.
>
> The only way to fix is to create a new ABI capable of exporting what
> the driver actually sees, and not some virtual information, produced
> by the driver just to make the EDAC core happy.
>
> As requested by Greg, the first step is to convert the EDAC MC code
> to use struct device. That means that 3 drivers also need to be
> converted (amd64, i7core and mpc85xx_edac), as they create their own
> ABI's.
>
> Those patches were compile-tested on all architectures.
>
> It was also tested on all types of Memory Controllers with EDAC support
> I was able to find at Red Hat Labs:
> e752x_edac (a Xeon i3100 chipset)
> i3000_edac
> i3200_edac
> i5000_edac
> i5100_edac
> i5400_edac
> i7300_edac
> i7core_edac (Nehalem)
> sb_edac (Sandy Bridge E5)
> amd64_edac
>
> Several of them with multiple memory controllers (the amd64 hardware
> I used is the bigger one, in terms of MC, with 8 memory controllers).
>
> There are 3 intended changes that are out of this series:
>
> - ABI documentation. I'll write the ABI patch as soon as I
> merge this series at -next;
>
> - New API UE/CE error counters. They're needed, but, as the
> discussions weren't finished, let's postpone it. I'll start work on
> it after the merge of this series.
>
> - MCA error trace. Also, there wasn't any agreement yet.
> So, keep this out of this series, until we come to some conclusion.
>
> Regards,
> Mauro
>
> Mauro Carvalho Chehab (14):
> edac: rewrite the sysfs code to use struct device
> mpc85xx_edac: convert sysfs logic to use struct device
> amd64_edac: convert sysfs logic to use struct device
> i7core_edac: convert it to use struct device
> edac: Get rid of the old kobj's from the edac mc code
> edac: add a new per-dimm API and make the old per-virtual-rank API
> obsolete
> edac: add a sysfs node to report the maximum location for the system
> edac: Add debufs nodes to allow doing fake error inject
> edac: Create a per-Memory Controller bus
> edac: Move grain/dtype/edac_type calculus to be out of channel loop
> i82975x_edac: Test nr_pages earlier to save a few CPU cycles
> i5100_edac: Fix a warning when compiled with 32 bits
> i7300_edac: Get rid of some wrongly-solved rebase conflict
> edac: Only expose csrows/channels on legacy API if they're populated
>
> drivers/edac/Kconfig | 8 +
> drivers/edac/amd64_edac.c | 43 +-
> drivers/edac/amd64_edac.h | 29 +-
> drivers/edac/amd64_edac_dbg.c | 89 ++--
> drivers/edac/amd64_edac_inj.c | 128 +++--
> drivers/edac/cpc925_edac.c | 54 +-
> drivers/edac/e752x_edac.c | 31 +-
> drivers/edac/e7xxx_edac.c | 32 +-
> drivers/edac/edac_mc.c | 60 +-
> drivers/edac/edac_mc_sysfs.c | 1322 +++++++++++++++++++++--------------------
> drivers/edac/edac_module.c | 13 +-
> drivers/edac/edac_module.h | 9 +-
> drivers/edac/i5000_edac.c | 3 -
> drivers/edac/i5100_edac.c | 4 +-
> drivers/edac/i7300_edac.c | 3 -
> drivers/edac/i7core_edac.c | 336 +++++++----
> drivers/edac/i82875p_edac.c | 4 -
> drivers/edac/i82975x_edac.c | 9 +-
> drivers/edac/mpc85xx_edac.c | 93 ++--
> include/linux/edac.h | 69 +--
> 20 files changed, 1250 insertions(+), 1089 deletions(-)
Reviewed-by: Aristeu Rozanski <arozansk@redhat.com>
--
Aristeu
^ permalink raw reply [flat|nested] 26+ messages in thread