Re: [EDAC ABI v13 24/25] edac: change the mem allocation scheme tomake Documentation/kobject.txt happy
From: Mauro Carvalho Chehab
Date: Thu Apr 19 2012 - 09:15:44 EST
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 majordomo@xxxxxxxxxxxxxxx
> 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,
+ csr->channels = kcalloc(sizeof(*csr->channels), tot_cschannels,
GFP_KERNEL);
for (chn = 0; chn < tot_cschannels; chn++) {
@@ -323,7 +323,7 @@ struct mem_ctl_info *edac_mc_alloc(unsigned edac_index,
/*
* Allocate and fill the dimm structs
*/
- mci->dimms = kzalloc(sizeof(*mci->dimms) * tot_dimms, GFP_KERNEL);
+ mci->dimms = kcalloc(sizeof(*mci->dimms), tot_dimms, GFP_KERNEL);
if (!mci->dimms)
goto error;
@@ -335,8 +335,10 @@ struct mem_ctl_info *edac_mc_alloc(unsigned edac_index,
for (i = 0; i < tot_dimms; i++) {
chan = mci->csrows[row]->channels[chn];
off = GET_OFFSET(lay, n_layers, pos[0], pos[1], pos[2]);
- if (off < 0 || off >= tot_dimms)
+ if (off < 0 || off >= tot_dimms) {
+ edac_mc_printk(mci, KERN_ERR, "EDAC core bug: GET_OFFSET is trying to do an illegal data access\n");
goto error;
+ }
dimm = kzalloc(sizeof(**mci->dimms), GFP_KERNEL);
mci->dimms[off] = dimm;
@@ -409,11 +411,9 @@ 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;
error:
- debugf1("Failed to allocate one or more EDAC MCI structs\n");
if (mci->dimms) {
for (i = 0; i < tot_dimms; i++)
kfree(mci->dimms[i]);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/