Hi Joe,
Em 17-04-2012 18:17, Joe Perches escreveu:
On Mon, 2012-04-16 at 17:38 -0300, Mauro Carvalho Chehab wrote:
Kernel kobjects have rigid rules: each container object should be
dynamically allocated, and can't be allocated into a single kmalloc.
EDAC never obeyed this rule: it has a single malloc function that
allocates all needed data into a single kzalloc.
As this is not accepted anymore, change the allocation schema of the
EDAC *_info structs to enforce this Kernel standard.
[]
diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
[]
@@ -2228,9 +2228,9 @@ static int init_csrows(struct mem_ctl_info *mci)
edac_mode = EDAC_NONE;
for (j = 0; j pvt-channel_count; j++) {
-csrow-channels[j].dimm-mtype = mtype;
-csrow-channels[j].dimm-edac_mode = edac_mode;
-csrow-channels[j].dimm-nr_pages = nr_pages;
+csrow-channels[j]-dimm-mtype = mtype;
+csrow-channels[j]-dimm-edac_mode = edac_mode;
+csrow-channels[j]-dimm-nr_pages = nr_pages;
It might be better to use an automatic for
typeof foo dimm = csrow-channels[j]-dimm;
dimm-mtype = mtype;
etc...
[]
@@ -293,39 +286,56 @@ 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);
kcalloc
[]
+csr-channels = kzalloc(sizeof(*csr-channels) * tot_cschannels,
+GFP_KERNEL);
here too
@@ -391,7 +401,31 @@ struct mem_ctl_info *edac_mc_alloc(unsigned edac_index,
trace_hw_event_init(edac, (unsigned)edac_index);
+debugf1(EDAC MCI allocated\n);
return mci;
Agreed with all above. Fixed.
+
+error:
+debugf1(Failed to allocate one or more EDAC MCI structs\n);
Generally, it's not necessary to have specific OOM messages
as allocations without GFP_NOWARN do dump_stack()s
There is one error condition on the error path that might potentially happen
that it is not related directly to the kalloc stuff: if, there GET_POS() tries
to access something out of the allocated range, it bails out. I added this
debug printk due to that, while testing the allocation changes. The check could
potentially be using a BUG_ON() call, but, as failing here, the edac drivers
will not load, I opted to not use BUG_ON, and, instead use the standard error
path for it.
That's said, it seems better to remove this debugf1 from this place, and add
some printk(KERN_ERR, ...) at this specific condition.
--
To unsubscribe from this list: send the line unsubscribe linux-edac in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
I'm folding the enclosed patch on this one, in order to address the comments
on your review.
Thanks!
Mauro
-
diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index 16510a3..08af66c 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -2180,6 +2180,7 @@ static u32 amd64_csrow_nr_pages(struct amd64_pvt *pvt, u8
dct, int csrow_nr)
static int init_csrows(struct mem_ctl_info *mci)
{
struct csrow_info *csrow;
+ struct dimm_info *dimm;
struct amd64_pvt *pvt = mci-pvt_info;
u64 base, mask;
u32 val;
@@ -2228,9 +2229,10 @@ static int init_csrows(struct mem_ctl_info *mci)
edac_mode = EDAC_NONE;
for (j = 0; j pvt-channel_count; j++) {
- csrow-channels[j]-dimm-mtype = mtype;
- csrow-channels[j]-dimm-edac_mode = edac_mode;
- csrow-channels[j]-dimm-nr_pages = nr_pages;
+ dimm = csrow-channels[j]-dimm;
+ dimm-mtype = mtype;
+ dimm-edac_mode = edac_mode;
+ dimm-nr_pages = nr_pages;
}
}
diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c
index fe57684..16f2d3b 100644
--- a/drivers/edac/edac_mc.c
+++ b/drivers/edac/edac_mc.c
@@ -296,7 +296,7 @@ struct mem_ctl_info *edac_mc_alloc(unsigned edac_index,
/*
* Alocate and fill the csrow/channels structs
*/
- mci-csrows = kzalloc(sizeof(*mci-csrows) * tot_csrows, GFP_KERNEL);
+ mci-csrows = kcalloc(sizeof(*mci-csrows), tot_csrows, GFP_KERNEL);
if (!mci-csrows)
goto error;
for (row = 0; row tot_csrows; row++) {
@@ -307,7 +307,7 @@ struct mem_ctl_info *edac_mc_alloc(unsigned edac_index,
csr-csrow_idx = row;
csr-mci = mci;
csr-nr_channels = tot_cschannels;
- csr-channels = kzalloc(sizeof(*csr-channels) * tot_cschannels,
+
