lkml.org 
[lkml]   [2012]   [Mar]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 01/14] edac: rewrite the sysfs code to use struct device
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



\
 
 \ /
  Last update: 2012-03-30 01:21    [W:0.117 / U:0.216 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site