Linux-IIO Archive on lore.kernel.org
 help / color / Atom feed
From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Bartosz Golaszewski <brgl@bgdev.pl>
Cc: Jonathan Cameron <jic23@kernel.org>,
	Hartmut Knaack <knaack.h@gmx.de>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Peter Meerwald-Stadler <pmeerw@pmeerw.net>,
	Michal Simek <michal.simek@xilinx.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Guenter Roeck <linux@roeck-us.net>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	linux-iio <linux-iio@vger.kernel.org>,
	linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Bartosz Golaszewski <bgolaszewski@baylibre.com>
Subject: Re: [PATCH v6 1/3] devres: provide devm_krealloc()
Date: Sun, 2 Aug 2020 13:42:00 +0300
Message-ID: <CAHp75Vfm_vUKZOGkNp+0uTe0b=vk8yDyjs7XPdw_1GRauTBx4g@mail.gmail.com> (raw)
In-Reply-To: <20200802083458.24323-2-brgl@bgdev.pl>

On Sun, Aug 2, 2020 at 11:37 AM 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().

Some thoughts below. You may ignore nit-picks, of course :-)

...

> + * Managed krealloc(). Resizes the memory chunk allocated with devm_kmalloc().
> + * Behaves similarly to regular krealloc(): if @ptr is NULL or ZERO_SIZE_PTR,

> + * it's the equivalent of devm_kmalloc(). If new_size is zero, it frees the

equivalent for

> + * previously allocated memory and returns ZERO_SIZE_PTR. This function doesn't
> + * change the order in which the release callback for the re-alloc'ed devres
> + * will be called (except when falling back to devm_kmalloc() or when freeing
> + * resources when new_size is zero). The contents of the memory are preserved
> + * up to the lesser of new and old sizes.

Might deserve to say about pointers to RO, but see below.

...

> +       if (WARN_ON(is_kernel_rodata((unsigned long)ptr)))
> +               /*
> +                * We cannot reliably realloc a const string returned by
> +                * devm_kstrdup_const().
> +                */
> +               return NULL;

I was thinking about this bit... Shouldn't we rather issue a simple
dev_warn() and return the existing pointer?
For example in some cases we might want to have resources coming
either from heap or from constant. Then, if at some circumstances we
would like to extend that memory (only for non-constant cases) we
would need to manage this ourselves. Otherwise we may simply call
krealloc().
It seems that devm_kstrdup_const returns an initial pointer. Getting
NULL is kinda inconvenient (and actually dev_warn() might also be
quite a noise, however I would give a message to the user, because
it's something worth checking).

...

> +       spin_lock_irqsave(&dev->devres_lock, flags);
> +       old_dr = find_dr(dev, devm_kmalloc_release, devm_kmalloc_match, ptr);
> +       spin_unlock_irqrestore(&dev->devres_lock, flags);

> +       if (!old_dr) {

I would have this under spin lock b/c of below.

> +               WARN(1, "Memory chunk not managed or managed by a different device.");
> +               return NULL;
> +       }

> +       old_head = old_dr->node.entry;

This would be still better to be under spin lock.

> +       new_dr = krealloc(old_dr, total_size, gfp);
> +       if (!new_dr)
> +               return NULL;

And perhaps spin lock taken already here.

> +       if (new_dr != old_dr) {
> +               spin_lock_irqsave(&dev->devres_lock, flags);
> +               list_replace(&old_head, &new_dr->node.entry);
> +               spin_unlock_irqrestore(&dev->devres_lock, flags);
> +       }

Yes, I understand that covering more code under spin lock does not fix
any potential race, but at least it minimizes scope of the code that
is not under it to see exactly what is problematic.

I probably will think more about a better approach to avoid potential races.

-- 
With Best Regards,
Andy Shevchenko

  reply index

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-02  8:34 [PATCH v6 0/3] devres: provide and use devm_krealloc() Bartosz Golaszewski
2020-08-02  8:34 ` [PATCH v6 1/3] devres: provide devm_krealloc() Bartosz Golaszewski
2020-08-02 10:42   ` Andy Shevchenko [this message]
2020-08-03 19:31     ` Bartosz Golaszewski
2020-08-02  8:34 ` [PATCH v6 2/3] hwmon: pmbus: use more devres helpers Bartosz Golaszewski
2020-08-02  8:34 ` [PATCH v6 3/3] iio: adc: xilinx-xadc: use devm_krealloc() Bartosz Golaszewski

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAHp75Vfm_vUKZOGkNp+0uTe0b=vk8yDyjs7XPdw_1GRauTBx4g@mail.gmail.com' \
    --to=andy.shevchenko@gmail.com \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=bgolaszewski@baylibre.com \
    --cc=brgl@bgdev.pl \
    --cc=gregkh@linuxfoundation.org \
    --cc=jic23@kernel.org \
    --cc=knaack.h@gmx.de \
    --cc=lars@metafoo.de \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=michal.simek@xilinx.com \
    --cc=pmeerw@pmeerw.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-IIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iio/0 linux-iio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iio linux-iio/ https://lore.kernel.org/linux-iio \
		linux-iio@vger.kernel.org
	public-inbox-index linux-iio

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-iio


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git