Messages in this thread |  | | | Date | Wed, 18 Apr 2012 16:06:40 +0200 | | From | Borislav Petkov <> | | Subject | Re: [EDAC PATCH v13 5/7] edac: rewrite edac_align_ptr() |
| |
On Mon, Apr 16, 2012 at 05:12:11PM -0300, Mauro Carvalho Chehab wrote: > The edac_align_ptr() function is used to prepare data for a single > memory allocation kzalloc() call. It counts how many bytes are needed > by some data structure. > > Using it as-is is not that trivial, as the quantity of memory elements > reserved is not there, but, instead, it is on a next call. > > In order to avoid mistakes when using it, move the number of allocated > elements into it, making easier to use it. > > Reviewed-by: Aristeu Rozanski <arozansk@redhat.com>
AFAICT, this is a new patch so Aristeu cannot have reviewed it too. In such case, you can't simply keep the Reviewed-by tagging. Unless he really did that and I missed his mail with the tag somehow...?
> Cc: Doug Thompson <norsk5@yahoo.com> > Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com> > --- > drivers/edac/edac_device.c | 27 +++++++++++---------------- > drivers/edac/edac_mc.c | 19 +++++++++++++------ > drivers/edac/edac_module.h | 2 +- > drivers/edac/edac_pci.c | 7 ++++--- > 4 files changed, 29 insertions(+), 26 deletions(-) > > diff --git a/drivers/edac/edac_device.c b/drivers/edac/edac_device.c > index 4b15459..cb397d9 100644 > --- a/drivers/edac/edac_device.c > +++ b/drivers/edac/edac_device.c > @@ -79,7 +79,7 @@ struct edac_device_ctl_info *edac_device_alloc_ctl_info( > unsigned total_size; > unsigned count; > unsigned instance, block, attr; > - void *pvt; > + void *pvt, *p; > int err; > > debugf4("%s() instances=%d blocks=%d\n", > @@ -92,35 +92,30 @@ struct edac_device_ctl_info *edac_device_alloc_ctl_info( > * to be at least as stringent as what the compiler would > * provide if we could simply hardcode everything into a single struct. > */ > - dev_ctl = (struct edac_device_ctl_info *)NULL; > + p = NULL; > + dev_ctl = edac_align_ptr(&p, sizeof(*dev_ctl), 1); > > /* Calc the 'end' offset past end of ONE ctl_info structure > * which will become the start of the 'instance' array > */ > - dev_inst = edac_align_ptr(&dev_ctl[1], sizeof(*dev_inst)); > + dev_inst = edac_align_ptr(&p, sizeof(*dev_inst), nr_instances); > > /* Calc the 'end' offset past the instance array within the ctl_info > * which will become the start of the block array > */ > - dev_blk = edac_align_ptr(&dev_inst[nr_instances], sizeof(*dev_blk)); > + count = nr_instances * nr_blocks; > + dev_blk = edac_align_ptr(&p, sizeof(*dev_blk), count); > > /* Calc the 'end' offset past the dev_blk array > * which will become the start of the attrib array, if any. > */ > - count = nr_instances * nr_blocks; > - dev_attrib = edac_align_ptr(&dev_blk[count], sizeof(*dev_attrib)); > - > - /* Check for case of when an attribute array is specified */ > - if (nr_attrib > 0) { > - /* calc how many nr_attrib we need */ > + /* calc how many nr_attrib we need */ > + if (nr_attrib > 0) > count *= nr_attrib; > + dev_attrib = edac_align_ptr(&p, sizeof(*dev_attrib), count); > > - /* Calc the 'end' offset past the attributes array */ > - pvt = edac_align_ptr(&dev_attrib[count], sz_private); > - } else { > - /* no attribute array specificed */ > - pvt = edac_align_ptr(dev_attrib, sz_private); > - } > + /* Calc the 'end' offset past the attributes array */ > + pvt = edac_align_ptr(&p, sz_private, 1); > > /* 'pvt' now points to where the private data area is. > * At this point 'pvt' (like dev_inst,dev_blk and dev_attrib) > diff --git a/drivers/edac/edac_mc.c b/drivers/edac/edac_mc.c > index ffedae9..98de5d1 100644 > --- a/drivers/edac/edac_mc.c > +++ b/drivers/edac/edac_mc.c > @@ -108,9 +108,12 @@ EXPORT_SYMBOL_GPL(edac_mem_types); > * If 'size' is a constant, the compiler will optimize this whole function > * down to either a no-op or the addition of a constant to the value of 'ptr'. > */ > -void *edac_align_ptr(void *ptr, unsigned size) > +void *edac_align_ptr(void **p, unsigned size, int quant)
Oh, no, pls write it out as 'quantity'. 'quant' only means nothing... ok, it does but it does not fit in this here context:
From The Collaborative International Dictionary of English v.0.48 [gcide]:
Quant \Quant\, n. A punting pole with a broad flange near the end to prevent it from sinking into the mud; a setting pole. [1913 Webster]
:-)
> { > unsigned align, r; > + void *ptr = *p; > + > + *p += size * quant; > > /* Here we assume that the alignment of a "long long" is the most > * stringent alignment that the compiler will ever provide by default. > @@ -132,6 +135,8 @@ void *edac_align_ptr(void *ptr, unsigned size) > if (r == 0) > return (char *)ptr; > > + *p += align - r; > +
Why increment *p here too - we're returning ptr below? Or are we keeping the alignment in the original pointer too? Why can't we pass the aligned pointer from the previous pass? I.e., do
p = NULL; dev_ctl = edac_align_ptr(&p, sizeof(*dev_ctl), 1);
and then do
dev_inst = edac_align_ptr(&dev_ctl, sizeof(*dev_inst), nr_instances);
In any case, this is not trivial so the function needs a bunch of comments.
> return (void *)(((unsigned long)ptr) + align - r); > } > > @@ -154,6 +159,7 @@ void *edac_align_ptr(void *ptr, unsigned size) > struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows, > unsigned nr_chans, int edac_index) > { > + void *ptr; > struct mem_ctl_info *mci; > struct csrow_info *csi, *csrow; > struct rank_info *chi, *chp, *chan; > @@ -168,11 +174,12 @@ struct mem_ctl_info *edac_mc_alloc(unsigned sz_pvt, unsigned nr_csrows, > * stringent as what the compiler would provide if we could simply > * hardcode everything into a single struct. > */ > - mci = (struct mem_ctl_info *)0; > - csi = edac_align_ptr(&mci[1], sizeof(*csi)); > - chi = edac_align_ptr(&csi[nr_csrows], sizeof(*chi)); > - dimm = edac_align_ptr(&chi[nr_chans * nr_csrows], sizeof(*dimm)); > - pvt = edac_align_ptr(&dimm[nr_chans * nr_csrows], sz_pvt); > + ptr = 0;
Declare it above like this:
void *ptr = NULL;
> + mci = edac_align_ptr(&ptr, sizeof(*mci), 1); > + csi = edac_align_ptr(&ptr, sizeof(*csi), nr_csrows); > + chi = edac_align_ptr(&ptr, sizeof(*chi), nr_csrows * nr_chans); > + dimm = edac_align_ptr(ptr, sizeof(*dimm), nr_csrows * nr_chans); > + pvt = edac_align_ptr(&ptr, sz_pvt, 1); > size = ((unsigned long)pvt) + sz_pvt; > > mci = kzalloc(size, GFP_KERNEL); > diff --git a/drivers/edac/edac_module.h b/drivers/edac/edac_module.h > index 00f81b4..0be4b01 100644 > --- a/drivers/edac/edac_module.h > +++ b/drivers/edac/edac_module.h > @@ -50,7 +50,7 @@ extern void edac_device_reset_delay_period(struct edac_device_ctl_info > *edac_dev, unsigned long value); > extern void edac_mc_reset_delay_period(int value); > > -extern void *edac_align_ptr(void *ptr, unsigned size); > +extern void *edac_align_ptr(void **p, unsigned size, int quant); > > /* > * EDAC PCI functions > diff --git a/drivers/edac/edac_pci.c b/drivers/edac/edac_pci.c > index 63af1c5..9016560 100644 > --- a/drivers/edac/edac_pci.c > +++ b/drivers/edac/edac_pci.c > @@ -42,13 +42,14 @@ struct edac_pci_ctl_info *edac_pci_alloc_ctl_info(unsigned int sz_pvt, > const char *edac_pci_name) > { > struct edac_pci_ctl_info *pci; > - void *pvt; > + void *p, *pvt; > unsigned int size; > > debugf1("%s()\n", __func__); > > - pci = (struct edac_pci_ctl_info *)0; > - pvt = edac_align_ptr(&pci[1], sz_pvt); > + p = 0;
ditto.
> + pci = edac_align_ptr(&p, sizeof(*pci), 1); > + pvt = edac_align_ptr(&p, 1, sz_pvt); > size = ((unsigned long)pvt) + sz_pvt; > > /* Alloc the needed control struct memory */ > -- > 1.7.8 > > -- > 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 >
-- 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
|  |