| Message ID | 20170302120242.6342-1-kernel@kempniu.pl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 2, 2017 at 2:02 PM, Michał Kępień <kernel@kempniu.pl> wrote: > Some platform drivers use devm_input_allocate_device() together with > sparse_keymap_setup() in their .probe callbacks. While using the former > simplifies error handling, using the latter necessitates calling > sparse_keymap_free() in the error path and upon module unloading to > avoid leaking the copy of the keymap allocated by sparse_keymap_setup(). > > To help prevent such leaks and enable simpler error handling, make > sparse_keymap_setup() use devm_kcalloc() to allocate memory for the > keymap copy so that it gets automatically freed. > + * Since sparse_keymap_setup() now uses a managed allocation for the > + * keymap copy, use of this function is deprecated. So... > */ > void sparse_keymap_free(struct input_dev *dev) ... add __deprecated then? > { > } > EXPORT_SYMBOL(sparse_keymap_free);
> On Thu, Mar 2, 2017 at 2:02 PM, Michał Kępień <kernel@kempniu.pl> wrote: > > Some platform drivers use devm_input_allocate_device() together with > > sparse_keymap_setup() in their .probe callbacks. While using the former > > simplifies error handling, using the latter necessitates calling > > sparse_keymap_free() in the error path and upon module unloading to > > avoid leaking the copy of the keymap allocated by sparse_keymap_setup(). > > > > To help prevent such leaks and enable simpler error handling, make > > sparse_keymap_setup() use devm_kcalloc() to allocate memory for the > > keymap copy so that it gets automatically freed. > > > + * Since sparse_keymap_setup() now uses a managed allocation for the > > + * keymap copy, use of this function is deprecated. > > So... > > > */ > > void sparse_keymap_free(struct input_dev *dev) > > ... add __deprecated then? Sure, I can do that if Dmitry recognizes this patch as otherwise sane. I have never used that attribute before, so thanks for the tip.
On Mon, Mar 6, 2017 at 9:09 AM, Michał Kępień <kernel@kempniu.pl> wrote: >> On Thu, Mar 2, 2017 at 2:02 PM, Michał Kępień <kernel@kempniu.pl> wrote: >> > + * Since sparse_keymap_setup() now uses a managed allocation for the >> > + * keymap copy, use of this function is deprecated. >> >> So... >> >> > */ >> > void sparse_keymap_free(struct input_dev *dev) >> >> ... add __deprecated then? > > Sure, I can do that if Dmitry recognizes this patch as otherwise sane. > I have never used that attribute before, so thanks for the tip. It will provoke a warning when compiling the code. It makes sense if anyone is going to fix the current users and leave old API for one cycle. So, the question is what we are going to do with an old API.
> On Mon, Mar 6, 2017 at 9:09 AM, Michał Kępień <kernel@kempniu.pl> wrote: > >> On Thu, Mar 2, 2017 at 2:02 PM, Michał Kępień <kernel@kempniu.pl> wrote: > > >> > + * Since sparse_keymap_setup() now uses a managed allocation for the > >> > + * keymap copy, use of this function is deprecated. > >> > >> So... > >> > >> > */ > >> > void sparse_keymap_free(struct input_dev *dev) > >> > >> ... add __deprecated then? > > > > Sure, I can do that if Dmitry recognizes this patch as otherwise sane. > > I have never used that attribute before, so thanks for the tip. > > It will provoke a warning when compiling the code. > It makes sense if anyone is going to fix the current users and leave > old API for one cycle. > > So, the question is what we are going to do with an old API. If this is a latent call to clean it up, then sure, I can take care of it ;) A quick scan shows ca. 30 call sites in the entire tree, fixing which seems to be doable given a few weeks of time. Though obviously I would really like to hear that the patch looks sound before I set out on that journey.
On Thu, Mar 02, 2017 at 01:02:42PM +0100, Michał Kępień wrote: > Some platform drivers use devm_input_allocate_device() together with > sparse_keymap_setup() in their .probe callbacks. While using the former > simplifies error handling, using the latter necessitates calling > sparse_keymap_free() in the error path and upon module unloading to > avoid leaking the copy of the keymap allocated by sparse_keymap_setup(). > > To help prevent such leaks and enable simpler error handling, make > sparse_keymap_setup() use devm_kcalloc() to allocate memory for the > keymap copy so that it gets automatically freed. > > This works for both managed and non-managed input devices, although both > are handled a bit differently. Specifically, the managed keymap copy is > attached to a different struct device: > > - for managed input devices, as all other managed resources are > attached to the device owning the input device, we do the same to > ensure freeing the keymap copy is properly slotted in the devres > stack, > > - for non-managed input devices, the managed keymap copy is attached > to the struct device embedded inside the input device itself, so > that the keymap is freed after the last reference to the input > device is dropped, i.e. after input_unregister_device() is called. > > Note that actions previously taken by sparse_keymap_free(), i.e. taking > the input device's mutex and zeroing its keycode and keycodemax fields, > are now redundant because the managed keymap will always be freed after > the input device is unregistered. > > Signed-off-by: Michał Kępień <kernel@kempniu.pl> > --- > Changes from v1: > > - Do not add a new function. Instead, make sparse_keymap_setup() > always use a managed allocation, which allows making > sparse_keymap_free() a noop and simplifies error handling. > > - Update subject, commit message and comments accordingly. > > drivers/input/sparse-keymap.c | 37 +++++++++---------------------------- > 1 file changed, 9 insertions(+), 28 deletions(-) > > diff --git a/drivers/input/sparse-keymap.c b/drivers/input/sparse-keymap.c > index e7409c45bdd0..fd391c339b4e 100644 > --- a/drivers/input/sparse-keymap.c > +++ b/drivers/input/sparse-keymap.c > @@ -160,12 +160,12 @@ static int sparse_keymap_setkeycode(struct input_dev *dev, > * @keymap: Keymap in form of array of &key_entry structures ending > * with %KE_END type entry > * @setup: Function that can be used to adjust keymap entries > - * depending on device's deeds, may be %NULL > + * depending on device's needs, may be %NULL > * > * The function calculates size and allocates copy of the original > * keymap after which sets up input device event bits appropriately. > - * Before destroying input device allocated keymap should be freed > - * with a call to sparse_keymap_free(). > + * The allocated copy of the keymap is automatically freed when it > + * is no longer needed. > */ > int sparse_keymap_setup(struct input_dev *dev, > const struct key_entry *keymap, > @@ -180,7 +180,8 @@ int sparse_keymap_setup(struct input_dev *dev, > for (e = keymap; e->type != KE_END; e++) > map_size++; > > - map = kcalloc(map_size, sizeof(struct key_entry), GFP_KERNEL); > + map = devm_kcalloc(dev->devres_managed ? dev->dev.parent : &dev->dev, Please always use input device as the owner of the keymap; there is no reason to make distinction between managed and non-managed case. Also maybe do: map = devm_kmemdup(&dev->dev, map_size * sizeof(*map), GFP_KERNEL); There is not a chance of owerflow as we are copying existing valid object, not using arbitrary parameters passed in. Thanks.
On Sat, Mar 04, 2017 at 07:58:51PM +0200, Andy Shevchenko wrote: > On Thu, Mar 2, 2017 at 2:02 PM, Michał Kępień <kernel@kempniu.pl> wrote: > > Some platform drivers use devm_input_allocate_device() together with > > sparse_keymap_setup() in their .probe callbacks. While using the former > > simplifies error handling, using the latter necessitates calling > > sparse_keymap_free() in the error path and upon module unloading to > > avoid leaking the copy of the keymap allocated by sparse_keymap_setup(). > > > > To help prevent such leaks and enable simpler error handling, make > > sparse_keymap_setup() use devm_kcalloc() to allocate memory for the > > keymap copy so that it gets automatically freed. > > > + * Since sparse_keymap_setup() now uses a managed allocation for the > > + * keymap copy, use of this function is deprecated. > > So... > > > */ > > void sparse_keymap_free(struct input_dev *dev) > > ... add __deprecated then? __deprectaed annoys innocent bystanders who just want to compile their kernels; we can simply make sure we will not add new users and clean up old ones in the next release or 2. Thanks.
diff --git a/drivers/input/sparse-keymap.c b/drivers/input/sparse-keymap.c index e7409c45bdd0..fd391c339b4e 100644 --- a/drivers/input/sparse-keymap.c +++ b/drivers/input/sparse-keymap.c @@ -160,12 +160,12 @@ static int sparse_keymap_setkeycode(struct input_dev *dev, * @keymap: Keymap in form of array of &key_entry structures ending * with %KE_END type entry * @setup: Function that can be used to adjust keymap entries - * depending on device's deeds, may be %NULL + * depending on device's needs, may be %NULL * * The function calculates size and allocates copy of the original * keymap after which sets up input device event bits appropriately. - * Before destroying input device allocated keymap should be freed - * with a call to sparse_keymap_free(). + * The allocated copy of the keymap is automatically freed when it + * is no longer needed. */ int sparse_keymap_setup(struct input_dev *dev, const struct key_entry *keymap, @@ -180,7 +180,8 @@ int sparse_keymap_setup(struct input_dev *dev, for (e = keymap; e->type != KE_END; e++) map_size++; - map = kcalloc(map_size, sizeof(struct key_entry), GFP_KERNEL); + map = devm_kcalloc(dev->devres_managed ? dev->dev.parent : &dev->dev, + map_size, sizeof(struct key_entry), GFP_KERNEL); if (!map) return -ENOMEM; @@ -192,7 +193,7 @@ int sparse_keymap_setup(struct input_dev *dev, if (setup) { error = setup(dev, entry); if (error) - goto err_out; + return error; } switch (entry->type) { @@ -221,10 +222,6 @@ int sparse_keymap_setup(struct input_dev *dev, dev->setkeycode = sparse_keymap_setkeycode; return 0; - - err_out: - kfree(map); - return error; } EXPORT_SYMBOL(sparse_keymap_setup); @@ -232,29 +229,13 @@ EXPORT_SYMBOL(sparse_keymap_setup); * sparse_keymap_free - free memory allocated for sparse keymap * @dev: Input device using sparse keymap * - * This function is used to free memory allocated by sparse keymap + * This function used to free memory allocated by sparse keymap * in an input device that was set up by sparse_keymap_setup(). - * NOTE: It is safe to cal this function while input device is - * still registered (however the drivers should care not to try to - * use freed keymap and thus have to shut off interrupts/polling - * before freeing the keymap). + * Since sparse_keymap_setup() now uses a managed allocation for the + * keymap copy, use of this function is deprecated. */ void sparse_keymap_free(struct input_dev *dev) { - unsigned long flags; - - /* - * Take event lock to prevent racing with input_get_keycode() - * and input_set_keycode() if we are called while input device - * is still registered. - */ - spin_lock_irqsave(&dev->event_lock, flags); - - kfree(dev->keycode); - dev->keycode = NULL; - dev->keycodemax = 0; - - spin_unlock_irqrestore(&dev->event_lock, flags); } EXPORT_SYMBOL(sparse_keymap_free);
Some platform drivers use devm_input_allocate_device() together with sparse_keymap_setup() in their .probe callbacks. While using the former simplifies error handling, using the latter necessitates calling sparse_keymap_free() in the error path and upon module unloading to avoid leaking the copy of the keymap allocated by sparse_keymap_setup(). To help prevent such leaks and enable simpler error handling, make sparse_keymap_setup() use devm_kcalloc() to allocate memory for the keymap copy so that it gets automatically freed. This works for both managed and non-managed input devices, although both are handled a bit differently. Specifically, the managed keymap copy is attached to a different struct device: - for managed input devices, as all other managed resources are attached to the device owning the input device, we do the same to ensure freeing the keymap copy is properly slotted in the devres stack, - for non-managed input devices, the managed keymap copy is attached to the struct device embedded inside the input device itself, so that the keymap is freed after the last reference to the input device is dropped, i.e. after input_unregister_device() is called. Note that actions previously taken by sparse_keymap_free(), i.e. taking the input device's mutex and zeroing its keycode and keycodemax fields, are now redundant because the managed keymap will always be freed after the input device is unregistered. Signed-off-by: Michał Kępień <kernel@kempniu.pl> --- Changes from v1: - Do not add a new function. Instead, make sparse_keymap_setup() always use a managed allocation, which allows making sparse_keymap_free() a noop and simplifies error handling. - Update subject, commit message and comments accordingly. drivers/input/sparse-keymap.c | 37 +++++++++---------------------------- 1 file changed, 9 insertions(+), 28 deletions(-)