Messages in this thread |  | | From | Bartosz Golaszewski <> | Date | Sun, 12 Jul 2020 10:19:09 +0200 | Subject | Re: [PATCH v3 1/3] devres: provide devm_krealloc() |
| |
On Sat, Jul 11, 2020 at 5:08 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Fri, Jul 10, 2020 at 7:17 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > Implement the managed variant of krealloc(). This function works with > > all memory allocated by devm_kmalloc() (or devres functions using it > > implicitly like devm_kmemdup(), devm_kstrdup() etc.). > > > > Managed realloc'ed chunks can be manually released with devm_kfree(). > > ... > > > devm_kfree() > > devm_kmalloc() > > devm_kmalloc_array() > > + devm_krealloc() > > devm_kmemdup() > > devm_kstrdup() > > devm_kvasprintf() > > Order? >
I didn't notice these were ordered alphabetically, I thought it was by functionality.
> ... > > > +void *devm_krealloc(struct device *dev, void *ptr, size_t new_size, gfp_t gfp) > > Do we really need the 'new_' prefix in the parameter? >
Yes, I think this is a good name for a function that's meant to modify the size of a memory chunk.
> > +{ > > + struct devres *old_dr, *new_dr; > > + struct list_head old_head; > > + unsigned long flags; > > + void *ret = NULL; > > + size_t tot_size; > > tot -> total. >
Meh, ok.
> > + > > + if (unlikely(!new_size)) { > > + devm_kfree(dev, ptr); > > + return ZERO_SIZE_PTR; > > + } > > I guess here we need a comment of the possibilities below to have > ZERO_SIZE_PTR as input. >
Better yet: I'll just add a kernel doc for this function.
> > + if (unlikely(ZERO_OR_NULL_PTR(ptr))) > > + return devm_kmalloc(dev, new_size, gfp); > > + > > + if (WARN_ON(is_kernel_rodata((unsigned long)ptr))) > > + /* > > + * We cannot reliably realloc a const string returned by > > + * devm_kstrdup_const(). > > + */ > > + return NULL; > > + > > + if (!check_dr_size(new_size, &tot_size)) > > + return NULL; > > + > > + spin_lock_irqsave(&dev->devres_lock, flags); > > + > > + old_dr = find_dr(dev, devm_kmalloc_release, devm_kmalloc_match, ptr); > > > + if (WARN_ON(!old_dr)) > > Under spin lock? I would rather see spin unlock followed by WARN. >
Yeah, can be done.
> > + /* Memory chunk not managed or managed by a different device. */ > > + goto out; > > + > > + old_head = old_dr->node.entry; > > + > > + new_dr = krealloc(old_dr, tot_size, gfp); > > + if (!new_dr) > > + goto out; > > + > > + if (new_dr != old_dr) > > + list_replace(&old_head, &new_dr->node.entry); > > + > > + ret = new_dr->data; > > + > > +out: > > + spin_unlock_irqrestore(&dev->devres_lock, flags); > > + return ret; > > +} > > -- > With Best Regards, > Andy Shevchenko
Bartosz
|  |