| Message ID | 20170308082217.5466-1-kernel@kempniu.pl (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Wed, Mar 08, 2017 at 09:22:17AM +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_kmemdup() to create the keymap copy so > that it gets automatically freed. > > This works for both managed and non-managed input devices as the keymap > is freed after the last reference to the input device is dropped. > > 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> OK, I think this looks good. Do platform folks want an immutable branch off 4.10 with this change so we can start cleaning sparse_keymap_free() users in this cycle? > --- > Changes from v2: > > - Always use the input device as the owner of the managed keymap copy, > no matter whether that input device is itself managed or > non-managed. > > - Use devm_kmemdup() instead of devm_kcalloc() to avoid a separate > call to memcpy(). > > - Update commit message to reflect the above changes. > > 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 | 39 +++++++++------------------------------ > 1 file changed, 9 insertions(+), 30 deletions(-) > > diff --git a/drivers/input/sparse-keymap.c b/drivers/input/sparse-keymap.c > index e7409c45bdd0..12a3ad83296d 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,19 +180,18 @@ 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_kmemdup(&dev->dev, keymap, map_size * sizeof(*map), > + GFP_KERNEL); > if (!map) > return -ENOMEM; > > - memcpy(map, keymap, map_size * sizeof(struct key_entry)); > - > for (i = 0; i < map_size; i++) { > entry = &map[i]; > > if (setup) { > error = setup(dev, entry); > if (error) > - goto err_out; > + return error; > } > > switch (entry->type) { > @@ -221,10 +220,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 +227,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); > > -- > 2.12.0 >
On Wed, Mar 8, 2017 at 8:12 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > On Wed, Mar 08, 2017 at 09:22:17AM +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_kmemdup() to create the keymap copy so >> that it gets automatically freed. >> >> This works for both managed and non-managed input devices as the keymap >> is freed after the last reference to the input device is dropped. >> >> 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. > OK, I think this looks good. Do platform folks want an immutable branch > off 4.10 with this change so we can start cleaning sparse_keymap_free() > users in this cycle? If there PDx86 related patches are anticipated this cycle, definitely we need an immutable branch (perhaps based on v4.11-rc1).
On Wed, Mar 08, 2017 at 10:50:16PM +0200, Andy Shevchenko wrote: > On Wed, Mar 8, 2017 at 8:12 PM, Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > On Wed, Mar 08, 2017 at 09:22:17AM +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_kmemdup() to create the keymap copy so > >> that it gets automatically freed. > >> > >> This works for both managed and non-managed input devices as the keymap > >> is freed after the last reference to the input device is dropped. > >> > >> 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. > > > OK, I think this looks good. Do platform folks want an immutable branch > > off 4.10 with this change so we can start cleaning sparse_keymap_free() > > users in this cycle? > > If there PDx86 related patches are anticipated this cycle, definitely > we need an immutable branch (perhaps based on v4.11-rc1). OK, I'll make one (based on 4.10 final - the patch should apply cleanly there and there is no point of forcing anyone's workflow to use kernel in the beginning of stabilization cycle). As to whether there are PDx86 patches - I hope Michał will supply them ;) I'll take care of the 2 instances in drivers/input/misc. Thanks.
On Wed, Mar 8, 2017 at 11:05 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > On Wed, Mar 08, 2017 at 10:50:16PM +0200, Andy Shevchenko wrote: >> On Wed, Mar 8, 2017 at 8:12 PM, Dmitry Torokhov >> <dmitry.torokhov@gmail.com> wrote: >> > On Wed, Mar 08, 2017 at 09:22:17AM +0100, Michał Kępień wrote: >> If there PDx86 related patches are anticipated this cycle, definitely >> we need an immutable branch (perhaps based on v4.11-rc1). > > OK, I'll make one (based on 4.10 final - the patch should apply cleanly > there and there is no point of forcing anyone's workflow to use kernel > in the beginning of stabilization cycle). Fine by me. > As to whether there are PDx86 patches - I hope Michał will supply them > ;) Michał, whenever you send the first one based on this change, please mention that it has dependency, I will pull the immutable branch at that time. Thanks.
> On Wed, Mar 8, 2017 at 11:05 PM, Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > On Wed, Mar 08, 2017 at 10:50:16PM +0200, Andy Shevchenko wrote: > >> On Wed, Mar 8, 2017 at 8:12 PM, Dmitry Torokhov > >> <dmitry.torokhov@gmail.com> wrote: > >> > On Wed, Mar 08, 2017 at 09:22:17AM +0100, Michał Kępień wrote: > > >> If there PDx86 related patches are anticipated this cycle, definitely > >> we need an immutable branch (perhaps based on v4.11-rc1). > > > > OK, I'll make one (based on 4.10 final - the patch should apply cleanly > > there and there is no point of forcing anyone's workflow to use kernel > > in the beginning of stabilization cycle). > > Fine by me. > > > As to whether there are PDx86 patches - I hope Michał will supply them > > ;) > > Michał, whenever you send the first one based on this change, please > mention that it has dependency, I will pull the immutable branch at > that time. Noted. I will prepare the patches and submit them within the next few days.
On Thu, Mar 09, 2017 at 08:58:54AM +0100, Michał Kępień wrote: > > On Wed, Mar 8, 2017 at 11:05 PM, Dmitry Torokhov > > <dmitry.torokhov@gmail.com> wrote: > > > On Wed, Mar 08, 2017 at 10:50:16PM +0200, Andy Shevchenko wrote: > > >> On Wed, Mar 8, 2017 at 8:12 PM, Dmitry Torokhov > > >> <dmitry.torokhov@gmail.com> wrote: > > >> > On Wed, Mar 08, 2017 at 09:22:17AM +0100, Michał Kępień wrote: > > > > >> If there PDx86 related patches are anticipated this cycle, definitely > > >> we need an immutable branch (perhaps based on v4.11-rc1). > > > > > > OK, I'll make one (based on 4.10 final - the patch should apply cleanly > > > there and there is no point of forcing anyone's workflow to use kernel > > > in the beginning of stabilization cycle). > > > > Fine by me. > > > > > As to whether there are PDx86 patches - I hope Michał will supply them > > > ;) > > > > Michał, whenever you send the first one based on this change, please > > mention that it has dependency, I will pull the immutable branch at > > that time. > > Noted. I will prepare the patches and submit them within the next few > days. OK, the branch with this single commit applied to 4.10 is git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git ib/4.10-sparse-keymap-managed Thanks.
diff --git a/drivers/input/sparse-keymap.c b/drivers/input/sparse-keymap.c index e7409c45bdd0..12a3ad83296d 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,19 +180,18 @@ 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_kmemdup(&dev->dev, keymap, map_size * sizeof(*map), + GFP_KERNEL); if (!map) return -ENOMEM; - memcpy(map, keymap, map_size * sizeof(struct key_entry)); - for (i = 0; i < map_size; i++) { entry = &map[i]; if (setup) { error = setup(dev, entry); if (error) - goto err_out; + return error; } switch (entry->type) { @@ -221,10 +220,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 +227,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_kmemdup() to create the keymap copy so that it gets automatically freed. This works for both managed and non-managed input devices as the keymap is freed after the last reference to the input device is dropped. 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 v2: - Always use the input device as the owner of the managed keymap copy, no matter whether that input device is itself managed or non-managed. - Use devm_kmemdup() instead of devm_kcalloc() to avoid a separate call to memcpy(). - Update commit message to reflect the above changes. 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 | 39 +++++++++------------------------------ 1 file changed, 9 insertions(+), 30 deletions(-)