media: Fix locking issues in rc core
Subsystems (8)
Sean Young
maintainerv5 cover media: Fix locking issues in rc core
Fix issues found by Sashiko and claude, and other cleanups.
Changes since v4:
- Split keymap locking into two commits: one for krealloc rewrite and one for the locking
- Fixed meson-ir-tx: Validate carrier and duty_cycle to actually fix divide by zero
- Improved commit messages
- imon calls usb_driver_claim_interface() before using it
- Ensure lirc_unregister() is always called with registered = false else any polling fds never get triggered
Changes since v3:
- serial_ir: Fix race condition where timer can be re-armed This introduced a new race condition; reworked
- rc: After rc_unregister_device() timers can be re-armed Sashiko suggested a simpler implementation with timer_shutdown_sync()
- Add meson-ir-tx: Validate carrier and duty_cycle
- Add ir_toy: Validate the maximum tx carrier
Changes since v2:
- Cosmetic changes in: Add missing locking for keymap
- Patch 7 to 17 are new issues found
Changes since v1:
- Fix issue where input device was still leaked
- Add: Fix race between bpf(BPG_PROG_ATTACH) and device unregister
- Add: Fix inconsistent locking of keylock
Sean Young (20):
media: rc: Ensure registered is cleared in error path
media: rc: Ensure that rc_unregister_device() does not free input
device
media: rc: Fix ABBA deadlock by making locks more fine grained
media: rc: Replace open coded krealloc() for keymap
media: rc: Add missing locking for keymap
media: rc: Fix race between bpf(BPG_PROG_ATTACH) and device unregister
media: rc: mce_kbd: Fix inconsistent locking of keylock
media: ene_ir: Ensure teardown is done in the correct order
media: rc: Use binary search for adding or updating a scancode
media: rc: imon: Bind both interfaces via usb_driver_claim_interface()
media: ir_toy: Remove unused struct field
media: nuvoton-cir: Remove unused struct field
media: ite-cir: Removed unused struct field
media: fintek-cir: Remove unused fields
media: mceusb: Remove unused field
media: serial_ir: Fix race condition where timer can be re-armed
media: rc: After rc_unregister_device() timers can be re-armed
media: rc: Validate carrier range in LIRC_SET_REC_CARRIER ioctl
media: ir_toy: Validate the maximum tx carrier
media: meson-ir-tx: Prevent divide by zero in
meson_irtx_prepare_pulse()
drivers/media/rc/ene_ir.c | 4 +- drivers/media/rc/fintek-cir.c | 13 -- drivers/media/rc/fintek-cir.h | 22 --- drivers/media/rc/imon.c | 164 ++++++++++------------- drivers/media/rc/ir-mce_kbd-decoder.c | 5 +- drivers/media/rc/ir_toy.c | 4 +- drivers/media/rc/ite-cir.c | 1 - drivers/media/rc/ite-cir.h | 1 - drivers/media/rc/lirc_dev.c | 2 + drivers/media/rc/mceusb.c | 1 - drivers/media/rc/meson-ir-tx.c | 3 + drivers/media/rc/nuvoton-cir.h | 3 - drivers/media/rc/rc-ir-raw.c | 39 +++--- drivers/media/rc/rc-loopback.c | 5 - drivers/media/rc/rc-main.c | 186 ++++++++++++++++---------- drivers/media/rc/serial_ir.c | 2 +- include/media/rc-map.h | 2 - 17 files changed, 222 insertions(+), 235 deletions(-)
Signature · 1 line
2.55.0
Sean Young
maintainerPATCH v5 1/20 media: rc: Ensure registered is cleared in error path
If rc_register_device() fails, ensure that registered is not set to true. If lirc_register() succeeded, then userspace could have an open file descriptor open. This leads to a use-after-free.
lirc_unregister() must be called with registered = false, else there is a possibility that an fd is open and polling, which would never return POLLERR unless registered is false.
Note that registered must be changed under dev->lock, many functions read registered under lock (e.g. lirc_ioctl).
--- drivers/media/rc/rc-main.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)
drivers/media/rc/rc-main.c +12 -4
@@ -1934,7 +1934,8 @@ int rc_register_device(struct rc_dev *dev) goto out_raw; } - dev->registered = true; +scoped_guard (mutex, &dev->lock) + dev->registered = true; rc =device_add (&dev->dev);if (rc)
@@ -1954,7 +1955,7 @@ int rc_register_device(struct rc_dev *dev)if (dev->allowed_protocols !=RC_PROTO_BIT_CEC ) { rc =lirc_register (dev);if (rc <0 ) - goto out_dev; + goto out_lirc; }if (dev->driver_type !=RC_DRIVER_IR_RAW_TX ) {
@@ -1977,11 +1978,18 @@ int rc_register_device(struct rc_dev *dev) out_rx:rc_free_rx_device (dev); out_lirc: -if (dev->allowed_protocols !=RC_PROTO_BIT_CEC ) +scoped_guard (mutex, &dev->lock) + dev->registered = false; + +if (device_is_registered (&dev->lirc_dev))lirc_unregister (dev); -out_dev:device_del (&dev->dev); +/* registered already cleared above */ + goto out_free_table; out_rx_free: +scoped_guard (mutex, &dev->lock) + dev->registered = false; +out_free_table:ir_free_table (&dev->rc_map); out_raw:ida_free (&rc_ida, minor);
Signature · 1 line
2.55.0
Sean Young
maintainerPATCH v5 2/20 media: rc: Ensure that rc_unregister_device() does not free input device
In reply to Sean YoungDuring or after rc_unregister_device(), IR may still be reported which results in a input event being reported. This could result in a null pointer deref in rc_keydown() or a use-after-free of the input device if the pointer was read before it is set to NULL.
--- drivers/media/rc/rc-main.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/media/rc/rc-main.c +4 -4
@@ -1753,7 +1753,7 @@ void rc_free_device(struct rc_dev *dev)if (!dev)return ; -input_free_device (dev->input_dev); +input_put_device (dev->input_dev);put_device (&dev->dev);
@@ -1865,6 +1865,8 @@ static int rc_setup_rx_device(struct rc_dev *dev)if (rc)return rc; +input_get_device (dev->input_dev); +/* * Default delay of 250ms is too short for some protocols, especially * since the timeout is currently set to 250ms. Increase it to 500ms,
@@ -1891,10 +1893,8 @@ static void rc_free_rx_device(struct rc_dev *dev)if (!dev)return ; -if (dev->input_dev) { +if (dev->input_dev)input_unregister_device (dev->input_dev); - dev->input_dev =NULL ; - }ir_free_table (&dev->rc_map); }
Signature · 1 line
2.55.0
Sean Young
maintainerPATCH v5 3/20 media: rc: Fix ABBA deadlock by making locks more fine grained
In reply to Sean YoungThe ir_raw_event_unregister() function takes the ir_raw_handler_lock first and then the rc_dev->lock. Other functions like change_protocol do this in the reverse order.
This means that an ir decoder module unload and writing to the protocol sysfs file can cause a deadlock.
ir_raw_handler_lock protects the client list, amongst other things. Split this out into a separate lock so we can ensure the locking order is always the same.
--- drivers/media/rc/rc-ir-raw.c | 39 ++++++++++++++++++++++-------------- 1 file changed, 24 insertions(+), 15 deletions(-)
drivers/media/rc/rc-ir-raw.c +24 -15
@@ -10,7 +10,8 @@#include <linux/sched.h> #include "rc-core-priv.h" -/* Used to keep track of IR raw clients, protected by ir_raw_handler_lock */ +/* Used to keep track of IR raw clients, protected by ir_raw_client_lock */ +static DEFINE_MUTEX (ir_raw_client_lock);static LIST_HEAD (ir_raw_client_list);/* Used to handle IR raw handler extensions */
@@ -273,13 +274,6 @@ static int change_protocol(struct rc_dev *dev, u64 *rc_proto)return 0 ; } -static void ir_raw_disable_protocols (struct rc_dev *dev,u64 protocols) -{ -mutex_lock (&dev->lock); - dev->enabled_protocols &= ~protocols; -mutex_unlock (&dev->lock); -} -/** * ir_raw_gen_manchester() - Encode data with Manchester (bi-phase) modulation. * @ev: Pointer to pointer to next free event. *@ev is incremented for
@@ -615,15 +609,18 @@ int ir_raw_event_register(struct rc_dev *dev) {struct task_struct *thread; +/* Holding dev->lock could result in a dead-lock */ +lockdep_assert_not_held (&dev->lock); + thread =kthread_run (ir_raw_event_thread, dev->raw,"rc%u" , dev->minor);if (IS_ERR (thread))return PTR_ERR (thread); dev->raw->thread = thread; -mutex_lock (&ir_raw_handler_lock); +mutex_lock (&ir_raw_client_lock);list_add_tail (&dev->raw->list, &ir_raw_client_list); -mutex_unlock (&ir_raw_handler_lock); +mutex_unlock (&ir_raw_client_lock);return 0 ; }
@@ -656,16 +653,19 @@ void ir_raw_event_unregister(struct rc_dev *dev)kthread_stop (dev->raw->thread);timer_delete_sync (&dev->raw->edge_handle); -mutex_lock (&ir_raw_handler_lock); +mutex_lock (&ir_raw_client_lock);list_del (&dev->raw->list); + +mutex_lock (&ir_raw_handler_lock);list_for_each_entry (handler, &ir_raw_handler_list, list)if (handler->raw_unregister && (handler->protocols & dev->enabled_protocols)) handler->raw_unregister (dev);lirc_bpf_free (dev); -mutex_unlock (&ir_raw_handler_lock); + +mutex_unlock (&ir_raw_client_lock); }/*
@@ -688,15 +688,24 @@ void ir_raw_handler_unregister(struct ir_raw_handler *ir_raw_handler)struct ir_raw_event_ctrl *raw;u64 protocols = ir_raw_handler->protocols; +mutex_lock (&ir_raw_client_lock); +mutex_lock (&ir_raw_handler_lock);list_del (&ir_raw_handler->list); +atomic64_andnot (protocols, &available_protocols); +mutex_unlock (&ir_raw_handler_lock); +list_for_each_entry (raw, &ir_raw_client_list, list) { +mutex_lock (&raw->dev->lock); +mutex_lock (&ir_raw_handler_lock);if (ir_raw_handler->raw_unregister && (raw->dev->enabled_protocols & protocols)) ir_raw_handler->raw_unregister (raw->dev); -ir_raw_disable_protocols (raw->dev, protocols); + raw->dev->enabled_protocols &= ~protocols; +mutex_unlock (&ir_raw_handler_lock); +mutex_unlock (&raw->dev->lock); } -atomic64_andnot (protocols, &available_protocols); -mutex_unlock (&ir_raw_handler_lock); + +mutex_unlock (&ir_raw_client_lock); }EXPORT_SYMBOL (ir_raw_handler_unregister);
Signature · 1 line
2.55.0
Sean Young
maintainerPATCH v5 4/20 media: rc: Replace open coded krealloc() for keymap
In reply to Sean YoungReplace some ugly code with open coded krealloc() and remove superfluous member of struct rc_map.
--- drivers/media/rc/rc-main.c | 45 +++++++++++++++++++------------------- include/media/rc-map.h | 2 -- 2 files changed, 22 insertions(+), 25 deletions(-)
drivers/media/rc/rc-main.c +22 -23
@@ -17,9 +17,8 @@#include <linux/module.h> #include "rc-core-priv.h" -/* Sizes are in bytes, 256 bytes allows for 32 entries on x64 */ -#define IR_TAB_MIN_SIZE 256 -#define IR_TAB_MAX_SIZE 8192 +#define IR_TAB_MIN_ENTRIES 32 +#define IR_TAB_MAX_ENTRIES 1024 static const struct { const char *name;
@@ -214,21 +213,23 @@ static int scancode_to_u64(const struct input_keymap_entry *ke, u64 *scancode)static int ir_create_table (struct rc_dev *dev,struct rc_map *rc_map,const char *name,u64 rc_proto,size_t size) { +unsigned int alloc; rc_map->name =kstrdup (name,GFP_KERNEL );if (!rc_map->name)return -ENOMEM ; + alloc =roundup_pow_of_two (size); rc_map->rc_proto = rc_proto; - rc_map->alloc =roundup_pow_of_two (size *sizeof (struct rc_map_table )); - rc_map->size = rc_map->alloc /sizeof (struct rc_map_table ); - rc_map->scan =kmalloc (rc_map->alloc,GFP_KERNEL ); + rc_map->len =0 ; + rc_map->size = alloc; + rc_map->scan = kmalloc_objs(struct rc_map_table, alloc, GFP_KERNEL);if (!rc_map->scan) {kfree (rc_map->name); rc_map->name =NULL ;return -ENOMEM ; } -dev_dbg (&dev->dev,"Allocated space for %u keycode entries (%u bytes)\n" , - rc_map->size, rc_map->alloc); +dev_dbg (&dev->dev,"Allocated space for %u keycode entries (%zu bytes)\n" , + alloc, alloc *sizeof (struct rc_map_table ));return 0 ; }
@@ -262,38 +263,36 @@ static void ir_free_table(struct rc_map *rc_map)static int ir_resize_table (struct rc_dev *dev,struct rc_map *rc_map,gfp_t gfp_flags) { -unsigned int oldalloc = rc_map->alloc; -unsigned int newalloc = oldalloc; -struct rc_map_table *oldscan = rc_map->scan; +unsigned int newsize = rc_map->size;struct rc_map_table *newscan;if (rc_map->size == rc_map->len) {/* All entries in use -> grow keytable */ -if (rc_map->alloc >=IR_TAB_MAX_SIZE ) +if (newsize >=IR_TAB_MAX_ENTRIES )return -ENOMEM ; - newalloc *=2 ; -dev_dbg (&dev->dev,"Growing table to %u bytes\n" , newalloc); + newsize *=2 ; + +dev_dbg (&dev->dev,"Growing table to %u entries\n" , newsize); } -if ((rc_map->len *3 < rc_map->size) && (oldalloc >IR_TAB_MIN_SIZE )) { +if (rc_map->len *3 < rc_map->size && rc_map->size >IR_TAB_MIN_ENTRIES ) {/* Less than 1/3 of entries in use -> shrink keytable */ - newalloc /=2 ; -dev_dbg (&dev->dev,"Shrinking table to %u bytes\n" , newalloc); + newsize /=2 ; +dev_dbg (&dev->dev,"Shrinking table to %u entries\n" , newsize); } -if (newalloc == oldalloc) +if (newsize == rc_map->size)return 0 ; - newscan =kmalloc (newalloc, gfp_flags); + newscan =krealloc_array (rc_map->scan, newsize, +sizeof (struct rc_map_table ), gfp_flags);if (!newscan)return -ENOMEM ; -memcpy (newscan, rc_map->scan, rc_map->len *sizeof (struct rc_map_table )); rc_map->scan = newscan; - rc_map->alloc = newalloc; - rc_map->size = rc_map->alloc /sizeof (struct rc_map_table ); -kfree (oldscan); + rc_map->size = newsize; +return 0 ; }
include/media/rc-map.h +0 -2
@@ -148,7 +148,6 @@ struct rc_map_table {* @scan: pointer to struct &rc_map_table * @size: Max number of entries * @len: Number of entries that are in use -* @alloc: size of \*scan, in bytes * @rc_proto: type of the remote controller protocol, as defined at * enum &rc_proto * @name: name of the key map table
@@ -158,7 +157,6 @@ struct rc_map {struct rc_map_table *scan;unsigned int size;unsigned int len; -unsigned int alloc;enum rc_proto rc_proto;const char *name;spinlock_t lock;
Signature · 1 line
2.55.0
Re: [PATCH v5 04/20] media: rc: Replace open coded krealloc() for keymap
Quoted 14 lines · Sean Young
Replace some ugly code with open coded
krealloc()and remove superfluous member of structrc_map.Signed-off-by Sean Young <[email protected]>--- drivers/media/rc/rc-main.c | 45 +++++++++++++++++++------------------- include/media/rc-map.h | 2 -- 2 files changed, 22 insertions(+), 25 deletions(-)drivers/media/rc/rc-main.c +0
-0
drivers/media/rc/rc-main.c line 21
Quoted 8 lines
drivers/media/rc/rc-main.c +2
-3@@ -17,9 +17,8 @@#include <linux/module.h> #include "rc-core-priv.h" -/* Sizes are in bytes, 256 bytes allows for 32 entries on x64 */ -#define IR_TAB_MIN_SIZE 256 -#define IR_TAB_MAX_SIZE 8192 +#define IR_TAB_MIN_ENTRIES 32 +#define IR_TAB_MAX_ENTRIES 1024
This is better...
Quoted 8 lines
drivers/media/rc/rc-main.c +1
-0static const struct { const char *name;@@ -214,21 +213,23 @@ static int scancode_to_u64(const struct input_keymap_entry *ke, u64 *scancode)static int ir_create_table (struct rc_dev *dev,struct rc_map *rc_map,const char *name,u64 rc_proto,size_t size) { +unsigned int alloc;
drivers/media/rc/rc-main.c static int scancode_to_u64(const struct input_keymap_entry *ke, u64 *scancode) · line 223
Quoted 10 lines
drivers/media/rc/rc-main.c +3
-3rc_map->name =kstrdup (name,GFP_KERNEL );if (!rc_map->name)return -ENOMEM ; + alloc =roundup_pow_of_two (size); rc_map->rc_proto = rc_proto; - rc_map->alloc =roundup_pow_of_two (size *sizeof (struct rc_map_table )); - rc_map->size = rc_map->alloc /sizeof (struct rc_map_table ); - rc_map->scan =kmalloc (rc_map->alloc,GFP_KERNEL ); + rc_map->len =0 ; + rc_map->size = alloc;
...but this still says 'size'...
Quoted 29 lines
drivers/media/rc/rc-main.c +5
-6+ rc_map->scan = kmalloc_objs(struct rc_map_table, alloc, GFP_KERNEL);if (!rc_map->scan) {kfree (rc_map->name); rc_map->name =NULL ;return -ENOMEM ; } -dev_dbg (&dev->dev,"Allocated space for %u keycode entries (%u bytes)\n" , - rc_map->size, rc_map->alloc); +dev_dbg (&dev->dev,"Allocated space for %u keycode entries (%zu bytes)\n" , + alloc, alloc *sizeof (struct rc_map_table ));return 0 ; }@@ -262,38 +263,36 @@ static void ir_free_table(struct rc_map *rc_map)static int ir_resize_table (struct rc_dev *dev,struct rc_map *rc_map,gfp_t gfp_flags) { -unsigned int oldalloc = rc_map->alloc; -unsigned int newalloc = oldalloc; -struct rc_map_table *oldscan = rc_map->scan; +unsigned int newsize = rc_map->size;struct rc_map_table *newscan;if (rc_map->size == rc_map->len) {/* All entries in use -> grow keytable */ -if (rc_map->alloc >=IR_TAB_MAX_SIZE ) +if (newsize >=IR_TAB_MAX_ENTRIES )return -ENOMEM ;
drivers/media/rc/rc-main.c static void ir_free_table(struct rc_map *rc_map) · line 279
Quoted 10 lines
drivers/media/rc/rc-main.c +4
-3- newalloc *=2 ; -dev_dbg (&dev->dev,"Growing table to %u bytes\n" , newalloc); + newsize *=2 ; + +dev_dbg (&dev->dev,"Growing table to %u entries\n" , newsize); } -if ((rc_map->len *3 < rc_map->size) && (oldalloc >IR_TAB_MIN_SIZE )) { +if (rc_map->len *3 < rc_map->size && rc_map->size >IR_TAB_MIN_ENTRIES ) {
...which is especially confusing here where rc_map->size and IR_TAB_MIN_ENTRIES are mixed.
This is not important for this series, so you can go ahead with it.
But personally I would prefer to rename rc_map->size to rc_map->entries and ditto for newsize to newentries. That way it is clear that these variables deal with the number of entries and not the size in bytes.
I mention it only because when I reviewed v4 it confused me.
Regards,
Hans
Quoted 47 lines
drivers/media/rc/rc-main.c +7
-8/* Less than 1/3 of entries in use -> shrink keytable */ - newalloc /=2 ; -dev_dbg (&dev->dev,"Shrinking table to %u bytes\n" , newalloc); + newsize /=2 ; +dev_dbg (&dev->dev,"Shrinking table to %u entries\n" , newsize); } -if (newalloc == oldalloc) +if (newsize == rc_map->size)return 0 ; - newscan =kmalloc (newalloc, gfp_flags); + newscan =krealloc_array (rc_map->scan, newsize, +sizeof (struct rc_map_table ), gfp_flags);if (!newscan)return -ENOMEM ; -memcpy (newscan, rc_map->scan, rc_map->len *sizeof (struct rc_map_table )); rc_map->scan = newscan; - rc_map->alloc = newalloc; - rc_map->size = rc_map->alloc /sizeof (struct rc_map_table ); -kfree (oldscan); + rc_map->size = newsize; +return 0 ; }include/media/rc-map.h +0
-2@@ -148,7 +148,6 @@ struct rc_map_table {* @scan: pointer to struct &rc_map_table * @size: Max number of entries * @len: Number of entries that are in use -* @alloc: size of \*scan, in bytes * @rc_proto: type of the remote controller protocol, as defined at * enum &rc_proto * @name: name of the key map table @@ -158,7 +157,6 @@ struct rc_map {struct rc_map_table *scan;unsigned int size;unsigned int len; -unsigned int alloc;enum rc_proto rc_proto;const char *name;spinlock_t lock;
Sean Young
maintainerRe: [PATCH v5 04/20] media: rc: Replace open coded krealloc() for keymap
Quoted 38 lines · Hans Verkuil
Quoted 14 lines · Sean Young
Replace some ugly code with open coded
krealloc()and remove superfluous member of structrc_map.Signed-off-by Sean Young <[email protected]>--- drivers/media/rc/rc-main.c | 45 +++++++++++++++++++------------------- include/media/rc-map.h | 2 -- 2 files changed, 22 insertions(+), 25 deletions(-)drivers/media/rc/rc-main.c +0
-0drivers/media/rc/rc-main.c line 21
Quoted 8 lines
drivers/media/rc/rc-main.c +2
-3@@ -17,9 +17,8 @@#include <linux/module.h> #include "rc-core-priv.h" -/* Sizes are in bytes, 256 bytes allows for 32 entries on x64 */ -#define IR_TAB_MIN_SIZE 256 -#define IR_TAB_MAX_SIZE 8192 +#define IR_TAB_MIN_ENTRIES 32 +#define IR_TAB_MAX_ENTRIES 1024This is better...
Quoted 12 lines
drivers/media/rc/rc-main.c +2
-0static const struct { const char *name;@@ -214,21 +213,23 @@ static int scancode_to_u64(const struct input_keymap_entry *ke, u64 *scancode)static int ir_create_table (struct rc_dev *dev,struct rc_map *rc_map,const char *name,u64 rc_proto,size_t size) { +unsigned int alloc; rc_map->name =kstrdup (name,GFP_KERNEL );if (!rc_map->name)return -ENOMEM ; + alloc =roundup_pow_of_two (size);
Quoted 8 lines
drivers/media/rc/rc-main.c static int scancode_to_u64(const struct input_keymap_entry *ke, u64 *scancode) · line 223
Quoted 6 lines
drivers/media/rc/rc-main.c +2
-3rc_map->rc_proto = rc_proto; - rc_map->alloc =roundup_pow_of_two (size *sizeof (struct rc_map_table )); - rc_map->size = rc_map->alloc /sizeof (struct rc_map_table ); - rc_map->scan =kmalloc (rc_map->alloc,GFP_KERNEL ); + rc_map->len =0 ; + rc_map->size = alloc;...but this still says 'size'...
struct rc_map_table has a len and size member. I agree that size is not a good name. How about capacity or cap]? I am not sure that entries is much clearer that entries tbh, because what's the difference between len and entries?
Quoted 43 lines
Quoted 29 lines
drivers/media/rc/rc-main.c +5
-6+ rc_map->scan = kmalloc_objs(struct rc_map_table, alloc, GFP_KERNEL);if (!rc_map->scan) {kfree (rc_map->name); rc_map->name =NULL ;return -ENOMEM ; } -dev_dbg (&dev->dev,"Allocated space for %u keycode entries (%u bytes)\n" , - rc_map->size, rc_map->alloc); +dev_dbg (&dev->dev,"Allocated space for %u keycode entries (%zu bytes)\n" , + alloc, alloc *sizeof (struct rc_map_table ));return 0 ; }@@ -262,38 +263,36 @@ static void ir_free_table(struct rc_map *rc_map)static int ir_resize_table (struct rc_dev *dev,struct rc_map *rc_map,gfp_t gfp_flags) { -unsigned int oldalloc = rc_map->alloc; -unsigned int newalloc = oldalloc; -struct rc_map_table *oldscan = rc_map->scan; +unsigned int newsize = rc_map->size;struct rc_map_table *newscan;if (rc_map->size == rc_map->len) {/* All entries in use -> grow keytable */ -if (rc_map->alloc >=IR_TAB_MAX_SIZE ) +if (newsize >=IR_TAB_MAX_ENTRIES )return -ENOMEM ;drivers/media/rc/rc-main.c static void ir_free_table(struct rc_map *rc_map) · line 279
Quoted 10 lines
drivers/media/rc/rc-main.c +4
-3- newalloc *=2 ; -dev_dbg (&dev->dev,"Growing table to %u bytes\n" , newalloc); + newsize *=2 ; + +dev_dbg (&dev->dev,"Growing table to %u entries\n" , newsize); } -if ((rc_map->len *3 < rc_map->size) && (oldalloc >IR_TAB_MIN_SIZE )) { +if (rc_map->len *3 < rc_map->size && rc_map->size >IR_TAB_MIN_ENTRIES ) {...which is especially confusing here where
rc_map->sizeandIR_TAB_MIN_ENTRIESare mixed.
Quoted 5 lines
This is not important for this series, so you can go ahead with it.
But personally I would prefer to rename
rc_map->sizetorc_map->entriesand ditto for newsize to newentries. That way it is clear that these variables deal with the number of entries and not the size in bytes.
How about newcapicity or newcap?
Quoted 1 line
I mention it only because when I reviewed v4 it confused me.
Thanks,
Sean
Sean Young
maintainerRe: [PATCH v5 04/20] media: rc: Replace open coded krealloc() for keymap
Quoted 48 lines · Sean Young
Quoted 38 lines · Hans Verkuil
Quoted 14 lines · Sean Young
Replace some ugly code with open coded
krealloc()and remove superfluous member of structrc_map.Signed-off-by Sean Young <[email protected]>--- drivers/media/rc/rc-main.c | 45 +++++++++++++++++++------------------- include/media/rc-map.h | 2 -- 2 files changed, 22 insertions(+), 25 deletions(-)drivers/media/rc/rc-main.c +0
-0drivers/media/rc/rc-main.c line 21
Quoted 8 lines
drivers/media/rc/rc-main.c +2
-3@@ -17,9 +17,8 @@#include <linux/module.h> #include "rc-core-priv.h" -/* Sizes are in bytes, 256 bytes allows for 32 entries on x64 */ -#define IR_TAB_MIN_SIZE 256 -#define IR_TAB_MAX_SIZE 8192 +#define IR_TAB_MIN_ENTRIES 32 +#define IR_TAB_MAX_ENTRIES 1024This is better...
Quoted 12 lines
drivers/media/rc/rc-main.c +2
-0static const struct { const char *name;@@ -214,21 +213,23 @@ static int scancode_to_u64(const struct input_keymap_entry *ke, u64 *scancode)static int ir_create_table (struct rc_dev *dev,struct rc_map *rc_map,const char *name,u64 rc_proto,size_t size) { +unsigned int alloc; rc_map->name =kstrdup (name,GFP_KERNEL );if (!rc_map->name)return -ENOMEM ; + alloc =roundup_pow_of_two (size);Quoted 8 lines
drivers/media/rc/rc-main.c static int scancode_to_u64(const struct input_keymap_entry *ke, u64 *scancode) · line 223
Quoted 6 lines
drivers/media/rc/rc-main.c +2
-3rc_map->rc_proto = rc_proto; - rc_map->alloc =roundup_pow_of_two (size *sizeof (struct rc_map_table )); - rc_map->size = rc_map->alloc /sizeof (struct rc_map_table ); - rc_map->scan =kmalloc (rc_map->alloc,GFP_KERNEL ); + rc_map->len =0 ; + rc_map->size = alloc;...but this still says 'size'...
Quoted 4 lines
struct
rc_map_tablehas a len and size member. I agree that size is not a good name. How about capacity or cap]? I am not sure that entries is much clearer that entries tbh, because what's the difference between len and entries?
Turns out this is a bit of a mess. All the keymaps in drivers/media/rc/keymaps/ set the size member for the number of entries they have - which really should be len. So, renaming this member of struct rc_map_table will mean patching all the keymap entries. Nothing too complex but maybe this is something for the next patch series.
It would be useful to hear what you think of I'm proposing though:
1. Rename size to cap in struct rc_map_table (so it will have a len and cap)
2. Rename newsize to newcap in ir_resize_table() and related functions
3. In every keymap, replace:
.size = ARRAY_SIZE(empty),
With
.len = ARRAY_SIZE(empty),
This will be part of next series for rc-core, probably for the next release
cycle.
Thanks,
Sean
Quoted 57 lines
Quoted 44 lines
Quoted 30 lines
drivers/media/rc/rc-main.c +5
-6+ rc_map->scan = kmalloc_objs(struct rc_map_table, alloc, GFP_KERNEL);if (!rc_map->scan) {kfree (rc_map->name); rc_map->name =NULL ;return -ENOMEM ; } -dev_dbg (&dev->dev,"Allocated space for %u keycode entries (%u bytes)\n" , - rc_map->size, rc_map->alloc); +dev_dbg (&dev->dev,"Allocated space for %u keycode entries (%zu bytes)\n" , + alloc, alloc *sizeof (struct rc_map_table ));return 0 ; }@@ -262,38 +263,36 @@ static void ir_free_table(struct rc_map *rc_map)static int ir_resize_table (struct rc_dev *dev,struct rc_map *rc_map,gfp_t gfp_flags) { -unsigned int oldalloc = rc_map->alloc; -unsigned int newalloc = oldalloc; -struct rc_map_table *oldscan = rc_map->scan; +unsigned int newsize = rc_map->size;struct rc_map_table *newscan;if (rc_map->size == rc_map->len) {/* All entries in use -> grow keytable */ -if (rc_map->alloc >=IR_TAB_MAX_SIZE ) +if (newsize >=IR_TAB_MAX_ENTRIES )return -ENOMEM ;drivers/media/rc/rc-main.c static void ir_free_table(struct rc_map *rc_map) · line 279
Quoted 10 lines
drivers/media/rc/rc-main.c +4
-3- newalloc *=2 ; -dev_dbg (&dev->dev,"Growing table to %u bytes\n" , newalloc); + newsize *=2 ; + +dev_dbg (&dev->dev,"Growing table to %u entries\n" , newsize); } -if ((rc_map->len *3 < rc_map->size) && (oldalloc >IR_TAB_MIN_SIZE )) { +if (rc_map->len *3 < rc_map->size && rc_map->size >IR_TAB_MIN_ENTRIES ) {...which is especially confusing here where
rc_map->sizeandIR_TAB_MIN_ENTRIESare mixed.Quoted 5 lines
This is not important for this series, so you can go ahead with it.
But personally I would prefer to rename
rc_map->sizetorc_map->entriesand ditto for newsize to newentries. That way it is clear that these variables deal with the number of entries and not the size in bytes.How about newcapicity or newcap?
Quoted 1 line
I mention it only because when I reviewed v4 it confused me.
Thanks,
Sean
Hans Verkuil
Acked-byRe: [PATCH v5 04/20] media: rc: Replace open coded krealloc() for keymap
Quoted 60 lines · Sean Young
Quoted 48 lines · Sean Young
Quoted 38 lines · Hans Verkuil
Quoted 14 lines · Sean Young
Replace some ugly code with open coded
krealloc()and remove superfluous member of structrc_map.Signed-off-by Sean Young <[email protected]>--- drivers/media/rc/rc-main.c | 45 +++++++++++++++++++------------------- include/media/rc-map.h | 2 -- 2 files changed, 22 insertions(+), 25 deletions(-)drivers/media/rc/rc-main.c +0
-0drivers/media/rc/rc-main.c line 21
Quoted 8 lines
drivers/media/rc/rc-main.c +2
-3@@ -17,9 +17,8 @@#include <linux/module.h> #include "rc-core-priv.h" -/* Sizes are in bytes, 256 bytes allows for 32 entries on x64 */ -#define IR_TAB_MIN_SIZE 256 -#define IR_TAB_MAX_SIZE 8192 +#define IR_TAB_MIN_ENTRIES 32 +#define IR_TAB_MAX_ENTRIES 1024This is better...
Quoted 12 lines
drivers/media/rc/rc-main.c +2
-0static const struct { const char *name;@@ -214,21 +213,23 @@ static int scancode_to_u64(const struct input_keymap_entry *ke, u64 *scancode)static int ir_create_table (struct rc_dev *dev,struct rc_map *rc_map,const char *name,u64 rc_proto,size_t size) { +unsigned int alloc; rc_map->name =kstrdup (name,GFP_KERNEL );if (!rc_map->name)return -ENOMEM ; + alloc =roundup_pow_of_two (size);Quoted 8 lines
drivers/media/rc/rc-main.c static int scancode_to_u64(const struct input_keymap_entry *ke, u64 *scancode) · line 223
Quoted 6 lines
drivers/media/rc/rc-main.c +2
-3rc_map->rc_proto = rc_proto; - rc_map->alloc =roundup_pow_of_two (size *sizeof (struct rc_map_table )); - rc_map->size = rc_map->alloc /sizeof (struct rc_map_table ); - rc_map->scan =kmalloc (rc_map->alloc,GFP_KERNEL ); + rc_map->len =0 ; + rc_map->size = alloc;...but this still says 'size'...
Quoted 4 lines
struct
rc_map_tablehas a len and size member. I agree that size is not a good name. How about capacity or cap]? I am not sure that entries is much clearer that entries tbh, because what's the difference between len and entries?Turns out this is a bit of a mess. All the keymaps in
drivers/media/rc/keymaps/set the size member for the number of entries they have - which really should be len. So, renaming this member of structrc_map_tablewill mean patching all the keymap entries. Nothing too complex but maybe this is something for the next patch series.
Quoted 8 lines
It would be useful to hear what you think of I'm proposing though:
1. Rename size to cap in struct
rc_map_table(so it will have a len and cap)
2. Rename newsize to newcap inir_resize_table()and related functions
3. In every keymap, replace:
.size =ARRAY_SIZE(empty),
With
.len =ARRAY_SIZE(empty),
The problem with 'size' and 'len' is that it is not obvious what they refer to:
bytes? Number of elements in an array? Most commonly size and len refer to number of bytes.
For the number of array elements I would typically use 'elems' or possibly 'entries'. Or sometimes nr_of_'something'.
Quoted 3 lines
This will be part of next series for rc-core, probably for the next release
cycle.
Of course, that doesn't belong in this series.
In any case, for the series:
Regards,
Hans
Quoted 63 lines
Thanks,
SeanQuoted 57 lines
Quoted 44 lines
Quoted 30 lines
drivers/media/rc/rc-main.c +5
-6+ rc_map->scan = kmalloc_objs(struct rc_map_table, alloc, GFP_KERNEL);if (!rc_map->scan) {kfree (rc_map->name); rc_map->name =NULL ;return -ENOMEM ; } -dev_dbg (&dev->dev,"Allocated space for %u keycode entries (%u bytes)\n" , - rc_map->size, rc_map->alloc); +dev_dbg (&dev->dev,"Allocated space for %u keycode entries (%zu bytes)\n" , + alloc, alloc *sizeof (struct rc_map_table ));return 0 ; }@@ -262,38 +263,36 @@ static void ir_free_table(struct rc_map *rc_map)static int ir_resize_table (struct rc_dev *dev,struct rc_map *rc_map,gfp_t gfp_flags) { -unsigned int oldalloc = rc_map->alloc; -unsigned int newalloc = oldalloc; -struct rc_map_table *oldscan = rc_map->scan; +unsigned int newsize = rc_map->size;struct rc_map_table *newscan;if (rc_map->size == rc_map->len) {/* All entries in use -> grow keytable */ -if (rc_map->alloc >=IR_TAB_MAX_SIZE ) +if (newsize >=IR_TAB_MAX_ENTRIES )return -ENOMEM ;drivers/media/rc/rc-main.c static void ir_free_table(struct rc_map *rc_map) · line 279
Quoted 10 lines
drivers/media/rc/rc-main.c +4
-3- newalloc *=2 ; -dev_dbg (&dev->dev,"Growing table to %u bytes\n" , newalloc); + newsize *=2 ; + +dev_dbg (&dev->dev,"Growing table to %u entries\n" , newsize); } -if ((rc_map->len *3 < rc_map->size) && (oldalloc >IR_TAB_MIN_SIZE )) { +if (rc_map->len *3 < rc_map->size && rc_map->size >IR_TAB_MIN_ENTRIES ) {...which is especially confusing here where
rc_map->sizeandIR_TAB_MIN_ENTRIESare mixed.Quoted 5 lines
This is not important for this series, so you can go ahead with it.
But personally I would prefer to rename
rc_map->sizetorc_map->entriesand ditto for newsize to newentries. That way it is clear that these variables deal with the number of entries and not the size in bytes.How about newcapicity or newcap?
Quoted 1 line
I mention it only because when I reviewed v4 it confused me.
Thanks,
Sean
Sean Young
maintainerPATCH v5 5/20 media: rc: Add missing locking for keymap
In reply to Sean YoungWhen the rc_map for an rc_dev gets updated, locking is required but is missing in places. A concurrent scancode lookup and a call to rc_{register,unregistered}_device() could result in a use-after-free; this could happen if IR is decoded during those function calls.
Add lockdep assertions where locks are required.
ir_free_table() should only be called for non-TX devices else we will use a spinlock without iniitializing it.
--- drivers/media/rc/rc-main.c | 86 +++++++++++++++++++++++++------------- 1 file changed, 57 insertions(+), 29 deletions(-)
drivers/media/rc/rc-main.c +57 -29
@@ -104,7 +104,6 @@ static struct rc_map_list *seek_rc_map(const char *name)struct rc_map *rc_map_get (const char *name) { -struct rc_map_list *map; map =seek_rc_map (name);
@@ -201,7 +200,7 @@ static int scancode_to_u64(const struct input_keymap_entry *ke, u64 *scancode)* ir_create_table() - initializes a scancode table * @dev: the rc_dev device * @rc_map: the rc_map to initialize -* @name: name to assign to the table +* @map_name: name to assign to the table * @rc_proto: ir type to assign to the new table * @size: initial size of the table *
@@ -211,23 +210,31 @@ static int scancode_to_u64(const struct input_keymap_entry *ke, u64 *scancode)* return: zero on success or a negative error code */ static int ir_create_table (struct rc_dev *dev,struct rc_map *rc_map, -const char *name,u64 rc_proto,size_t size) +const char *map_name,u64 rc_proto,size_t size) { +struct rc_map_table *scan;unsigned int alloc; - rc_map->name =kstrdup (name,GFP_KERNEL ); -if (!rc_map->name) +char *name; + + name =kstrdup (map_name,GFP_KERNEL ); +if (!name)return -ENOMEM ; + alloc =roundup_pow_of_two (size); - rc_map->rc_proto = rc_proto; - rc_map->len =0 ; - rc_map->size = alloc; - rc_map->scan = kmalloc_objs(struct rc_map_table, alloc, GFP_KERNEL); -if (!rc_map->scan) { -kfree (rc_map->name); - rc_map->name =NULL ; + scan = kmalloc_objs(struct rc_map_table, alloc, GFP_KERNEL); +if (!scan) { +kfree (name);return -ENOMEM ; } + scoped_guard(spinlock_irqsave, &rc_map->lock) { + rc_map->name = name; + rc_map->scan = scan; + rc_map->rc_proto = rc_proto; + rc_map->len =0 ; + rc_map->size = alloc; + } +dev_dbg (&dev->dev,"Allocated space for %u keycode entries (%zu bytes)\n" , alloc, alloc *sizeof (struct rc_map_table ));return 0 ;
@@ -237,16 +244,26 @@ static int ir_create_table(struct rc_dev *dev, struct rc_map *rc_map,* ir_free_table() - frees memory allocated by a scancode table * @rc_map: the table whose mappings need to be freed * -* This routine will free memory alloctaed for key mappings used by given +* This routine will free memory allocated for key mappings used by given * scancode table. */ static void ir_free_table (struct rc_map *rc_map) { - rc_map->size =0 ; -kfree (rc_map->name); - rc_map->name =NULL ; -kfree (rc_map->scan); - rc_map->scan =NULL ; +struct rc_map_table *scan; +const char *name; + + scoped_guard(spinlock_irqsave, &rc_map->lock) { + name = rc_map->name; + scan = rc_map->scan; + + rc_map->size =0 ; + rc_map->len =0 ; + rc_map->name =NULL ; + rc_map->scan =NULL ; + } + +kfree (name); +kfree (scan); }/**
@@ -266,6 +283,8 @@ static int ir_resize_table(struct rc_dev *dev, struct rc_map *rc_map,unsigned int newsize = rc_map->size;struct rc_map_table *newscan; +lockdep_assert_held (&rc_map->lock); +if (rc_map->size == rc_map->len) {/* All entries in use -> grow keytable */ if (newsize >=IR_TAB_MAX_ENTRIES )
@@ -317,6 +336,8 @@ static unsigned int ir_update_mapping(struct rc_dev *dev,int old_keycode = rc_map->scan[index].keycode;int i; +lockdep_assert_held (&rc_map->lock); +/* Did the user wish to remove the mapping? */ if (new_keycode ==KEY_RESERVED || new_keycode ==KEY_UNKNOWN ) { dev_dbg(&dev->dev, "#%d: Deleting scan 0x%04llx\n",
@@ -372,6 +393,8 @@ static unsigned int ir_establish_scancode(struct rc_dev *dev, {unsigned int i; +lockdep_assert_held (&rc_map->lock); +/* * Unfortunately, some hardware-based IR decoders don't provide * all bits for the complete IR code. In general, they provide only
@@ -396,7 +419,7 @@ static unsigned int ir_establish_scancode(struct rc_dev *dev,/* No previous mapping found, we might need to grow the table */ if (rc_map->size == rc_map->len) {if (!resize ||ir_resize_table (dev, rc_map,GFP_ATOMIC )) -return -1U ; +return UINT_MAX ; }/* i is the proper index to insert our new keycode */
@@ -478,16 +501,18 @@ static int ir_setkeytable(struct rc_dev *dev, const struct rc_map *from)if (rc)return rc; -for (i =0 ; i < from->size; i++) { - index =ir_establish_scancode (dev, rc_map, - from->scan[i].scancode, false); -if (index >= rc_map->len) { - rc = -ENOMEM ; -break ; - } + scoped_guard(spinlock_irqsave, &rc_map->lock) { +for (i =0 ; i < from->size; i++) { + index =ir_establish_scancode (dev, rc_map, + from->scan[i].scancode, false); +if (index >= rc_map->len) { + rc = -ENOMEM ; +break ; + } -ir_update_mapping (dev, rc_map, index, - from->scan[i].keycode); +ir_update_mapping (dev, rc_map, index, + from->scan[i].keycode); + } }if (rc)
@@ -523,6 +548,8 @@ static unsigned int ir_lookup_by_scancode(const struct rc_map *rc_map, {struct rc_map_table *res; +lockdep_assert_held (&rc_map->lock); + res =bsearch (&scancode, rc_map->scan, rc_map->len,sizeof (struct rc_map_table ), rc_map_cmp);if (!res)
@@ -1989,7 +2016,8 @@ int rc_register_device(struct rc_dev *dev)scoped_guard (mutex, &dev->lock) dev->registered = false; out_free_table: -ir_free_table (&dev->rc_map); +if (dev->driver_type !=RC_DRIVER_IR_RAW_TX ) +ir_free_table (&dev->rc_map); out_raw:ida_free (&rc_ida, minor);return rc;
Signature · 1 line
2.55.0
Sean Young
maintainerPATCH v5 6/20 media: rc: Fix race between bpf(BPG_PROG_ATTACH) and device unregister
In reply to Sean YoungMark the device as unregistered before tearing down raw IR/bpf state, so that racing bpf(BPF_PROG_{ATTACH,DETACH,QUERY}) calls cannot slip in between ir_raw_event_unregister() completing and dev->registered being cleared.
--- drivers/media/rc/rc-main.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/media/rc/rc-main.c +6 -6
@@ -2056,18 +2056,18 @@ void rc_unregister_device(struct rc_dev *dev)if (!dev)return ; +mutex_lock (&dev->lock); + dev->registered = false; +if (dev->users && dev->close) + dev->close (dev); +mutex_unlock (&dev->lock); +if (dev->driver_type ==RC_DRIVER_IR_RAW )ir_raw_event_unregister (dev);timer_delete_sync (&dev->timer_keyup);timer_delete_sync (&dev->timer_repeat); -mutex_lock (&dev->lock); -if (dev->users && dev->close) - dev->close (dev); - dev->registered = false; -mutex_unlock (&dev->lock); -rc_free_rx_device (dev);/*
Signature · 1 line
2.55.0
Sean Young
maintainerPATCH v5 7/20 media: rc: mce_kbd: Fix inconsistent locking of keylock
In reply to Sean Youngdata->keylock is taken with spin_lock() in ir_mce_kbd_decode(), which runs in the raw IR decode kthread with local interrupts enabled, but with spin_lock_irqsave() in mce_kbd_rx_timeout(), the rx_timeout timer callback, which runs in softirq context.
If the decode kthread is holding keylock via spin_lock() when a timer interrupt fires on the same CPU and the softirq runs mce_kbd_rx_timeout(), the softirq spins forever waiting for a lock that only the now-preempted kthread can release, while the kthread cannot run again until the softirq gives up the CPU. This deadlocks the CPU.
--- drivers/media/rc/ir-mce_kbd-decoder.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/media/rc/ir-mce_kbd-decoder.c +3 -2
@@ -219,6 +219,7 @@ static int ir_mce_kbd_decode(struct rc_dev *dev, struct ir_raw_event ev)struct mce_kbd_dec *data = &dev->raw->mce_kbd;u32 scancode;unsigned long delay; +unsigned long flags;struct lirc_scancode lsc = {};if (!is_timing_event (ev)) {
@@ -319,7 +320,7 @@ static int ir_mce_kbd_decode(struct rc_dev *dev, struct ir_raw_event ev) scancode = data->body &0xffffff ;dev_dbg (&dev->dev,"keyboard data 0x%08x\n" , data->body); -spin_lock (&data->keylock); +spin_lock_irqsave (&data->keylock, flags);if (scancode) { delay =usecs_to_jiffies (dev->timeout) +msecs_to_jiffies (100 );
@@ -329,7 +330,7 @@ static int ir_mce_kbd_decode(struct rc_dev *dev, struct ir_raw_event ev) }/* Pass data to keyboard buffer parser */ ir_mce_kbd_process_keyboard_data (dev, scancode); -spin_unlock (&data->keylock); +spin_unlock_irqrestore (&data->keylock, flags); lsc.rc_proto =RC_PROTO_MCIR2_KBD ;break ;case MCIR2_MOUSE_NBITS :
Signature · 1 line
2.55.0
Sean Young
maintainerPATCH v5 8/20 media: ene_ir: Ensure teardown is done in the correct order
In reply to Sean YoungThe timer can be re-armed from the irq, so ensure that the timers are deleted after the irq and that we only free the rc device once we are sure that the irq has been silenced.
--- drivers/media/rc/ene_ir.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/media/rc/ene_ir.c +2 -2
@@ -1103,15 +1103,15 @@ static void ene_remove(struct pnp_dev *pnp_dev)unsigned long flags;rc_unregister_device (dev->rdev); -timer_delete_sync (&dev->tx_sim_timer);spin_lock_irqsave (&dev->hw_lock, flags);ene_rx_disable (dev);ene_rx_restore_hw_buffer (dev);spin_unlock_irqrestore (&dev->hw_lock, flags); -rc_free_device (dev->rdev);free_irq (dev->irq, dev); +timer_delete_sync (&dev->tx_sim_timer);release_region (dev->hw_io,ENE_IO_SIZE ); +rc_free_device (dev->rdev);kfree (dev); }
Signature · 1 line
2.55.0
Sean Young
maintainerPATCH v5 9/20 media: rc: Use binary search for adding or updating a scancode
In reply to Sean YoungWe can have up to 1024 scancodes entries which are always sorted, so make this a little faster.
We need to either get an scancode entry, or find the insertion point in the sorted array if no existing entry was found. The bsearch() function cannot do this as it runs NULL is no matching entry is found. Therefore, we provide an open coded binary search.
--- drivers/media/rc/rc-main.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-)
drivers/media/rc/rc-main.c +13 -7
@@ -391,7 +391,7 @@ static unsigned int ir_establish_scancode(struct rc_dev *dev,struct rc_map *rc_map, u64 scancode, bool resize) { - unsigned int i; + unsigned int i, lo, hi;lockdep_assert_held (&rc_map->lock);
@@ -406,15 +406,21 @@ static unsigned int ir_establish_scancode(struct rc_dev *dev,if (dev->scancode_mask) scancode &= dev->scancode_mask; -/* First check if we already have a mapping for this ir command */ -for (i =0 ; i < rc_map->len; i++) { +/* +* Binary search for an existing mapping for this ir command. +*/ + lo =0 ; + hi = rc_map->len; +while (lo < hi) { + i = lo + (hi - lo) /2 ;if (rc_map->scan[i].scancode == scancode)return i; - -/* Keytable is sorted from lowest to highest scancode */ -if (rc_map->scan[i].scancode >= scancode) -break ; +if (rc_map->scan[i].scancode < scancode) + lo = i +1 ; +else + hi = i; } + i = lo;/* No previous mapping found, we might need to grow the table */ if (rc_map->size == rc_map->len) {
Signature · 1 line
2.55.0
Sean Young
maintainerPATCH v5 10/20 media: rc: imon: Bind both interfaces via usb_driver_claim_interface()
In reply to Sean YoungiMON devices expose two USB interfaces: interface 0 (IR, and the display/knob on older devices) and interface 1 (touchscreen/ display on newer devices).
The driver currently pieces the interface together and this has to deal with various race conditions. Restrict the id_table to interface 0 only (USB_DEVICE_INTERFACE_NUMBER) so imon_probe() is only ever entered once per device and then we find the second interface with usb_driver_claim_interface(). This makes the driver much simpler.
--- drivers/media/rc/imon.c | 164 +++++++++++++++++----------------------- 1 file changed, 70 insertions(+), 94 deletions(-)
drivers/media/rc/imon.c +70 -94
@@ -96,7 +96,6 @@ struct imon_context {bool dev_present_intf1;/* USB device presence, interface 1 */ struct mutex lock;/* to lock this object */ -wait_queue_head_t remove_ok;/* For unexpected USB disconnects */ struct usb_endpoint_descriptor *rx_endpoint_intf0;struct usb_endpoint_descriptor *rx_endpoint_intf1;
@@ -378,7 +377,7 @@ static const struct usb_device_id imon_usb_id_table[] = {* SoundGraph iMON PAD (IR & LCD) * SoundGraph iMON Knob (IR only) */ - { USB_DEVICE(0x15c2, 0xffdc), + { USB_DEVICE_INTERFACE_NUMBER(0x15c2, 0xffdc, 0), .driver_info = (unsigned long)&imon_default_table },/*
@@ -387,61 +386,61 @@ static const struct usb_device_id imon_usb_id_table[] = {* Need user input to fill in details on unknown devices. */ /* SoundGraph iMON OEM Touch LCD (IR & 7" VGA LCD) */ - { USB_DEVICE(0x15c2, 0x0034), + { USB_DEVICE_INTERFACE_NUMBER(0x15c2, 0x0034, 0), .driver_info = (unsigned long)&imon_DH102 }, /* SoundGraph iMON OEM Touch LCD (IR & 4.3" VGA LCD) */ - { USB_DEVICE(0x15c2, 0x0035), + { USB_DEVICE_INTERFACE_NUMBER(0x15c2, 0x0035, 0), .driver_info = (unsigned long)&imon_default_table}, /* SoundGraph iMON OEM VFD (IR & VFD) */ - { USB_DEVICE(0x15c2, 0x0036), + { USB_DEVICE_INTERFACE_NUMBER(0x15c2, 0x0036, 0), .driver_info = (unsigned long)&imon_OEM_VFD }, /* device specifics unknown */ - { USB_DEVICE(0x15c2, 0x0037), + { USB_DEVICE_INTERFACE_NUMBER(0x15c2, 0x0037, 0), .driver_info = (unsigned long)&imon_default_table}, /* SoundGraph iMON OEM LCD (IR & LCD) */ - { USB_DEVICE(0x15c2, 0x0038), + { USB_DEVICE_INTERFACE_NUMBER(0x15c2, 0x0038, 0), .driver_info = (unsigned long)&imon_default_table}, /* SoundGraph iMON UltraBay (IR & LCD) */ - { USB_DEVICE(0x15c2, 0x0039), + { USB_DEVICE_INTERFACE_NUMBER(0x15c2, 0x0039, 0), .driver_info = (unsigned long)&imon_default_table}, /* device specifics unknown */ - { USB_DEVICE(0x15c2, 0x003a), + { USB_DEVICE_INTERFACE_NUMBER(0x15c2, 0x003a, 0), .driver_info = (unsigned long)&imon_default_table}, /* device specifics unknown */ - { USB_DEVICE(0x15c2, 0x003b), + { USB_DEVICE_INTERFACE_NUMBER(0x15c2, 0x003b, 0), .driver_info = (unsigned long)&imon_default_table}, /* SoundGraph iMON OEM Inside (IR only) */ - { USB_DEVICE(0x15c2, 0x003c), + { USB_DEVICE_INTERFACE_NUMBER(0x15c2, 0x003c, 0), .driver_info = (unsigned long)&imon_default_table}, /* device specifics unknown */ - { USB_DEVICE(0x15c2, 0x003d), + { USB_DEVICE_INTERFACE_NUMBER(0x15c2, 0x003d, 0), .driver_info = (unsigned long)&imon_default_table}, /* device specifics unknown */ - { USB_DEVICE(0x15c2, 0x003e), + { USB_DEVICE_INTERFACE_NUMBER(0x15c2, 0x003e, 0), .driver_info = (unsigned long)&imon_default_table}, /* device specifics unknown */ - { USB_DEVICE(0x15c2, 0x003f), + { USB_DEVICE_INTERFACE_NUMBER(0x15c2, 0x003f, 0), .driver_info = (unsigned long)&imon_default_table}, /* device specifics unknown */ - { USB_DEVICE(0x15c2, 0x0040), + { USB_DEVICE_INTERFACE_NUMBER(0x15c2, 0x0040, 0), .driver_info = (unsigned long)&imon_default_table}, /* SoundGraph iMON MINI (IR only) */ - { USB_DEVICE(0x15c2, 0x0041), + { USB_DEVICE_INTERFACE_NUMBER(0x15c2, 0x0041, 0), .driver_info = (unsigned long)&imon_default_table}, /* Antec Veris Multimedia Station EZ External (IR only) */ - { USB_DEVICE(0x15c2, 0x0042), + { USB_DEVICE_INTERFACE_NUMBER(0x15c2, 0x0042, 0), .driver_info = (unsigned long)&imon_default_table}, /* Antec Veris Multimedia Station Basic Internal (IR only) */ - { USB_DEVICE(0x15c2, 0x0043), + { USB_DEVICE_INTERFACE_NUMBER(0x15c2, 0x0043, 0), .driver_info = (unsigned long)&imon_default_table}, /* Antec Veris Multimedia Station Elite (IR & VFD) */ - { USB_DEVICE(0x15c2, 0x0044), + { USB_DEVICE_INTERFACE_NUMBER(0x15c2, 0x0044, 0), .driver_info = (unsigned long)&imon_default_table}, /* Antec Veris Multimedia Station Premiere (IR & LCD) */ - { USB_DEVICE(0x15c2, 0x0045), + { USB_DEVICE_INTERFACE_NUMBER(0x15c2, 0x0045, 0), .driver_info = (unsigned long)&imon_default_table}, /* device specifics unknown */ - { USB_DEVICE(0x15c2, 0x0046), + { USB_DEVICE_INTERFACE_NUMBER(0x15c2, 0x0046, 0), .driver_info = (unsigned long)&imon_default_table}, {} };
@@ -2328,15 +2327,20 @@ static struct imon_context *imon_init_intf1(struct usb_interface *intf,struct usb_host_interface *iface_desc;int ret = -ENOMEM ; + ret =usb_driver_claim_interface (&imon_driver, intf, ictx); +if (ret) { +dev_err (ictx->dev,"failed to claim second interface (%d)\n" , ret); +return NULL ; + } + rx_urb =usb_alloc_urb (0 ,GFP_KERNEL );if (!rx_urb) goto rx_urb_alloc_failed;mutex_lock (&ictx->lock); -if (ictx->display_type ==IMON_DISPLAY_TYPE_VGA ) { +if (ictx->display_type ==IMON_DISPLAY_TYPE_VGA )timer_setup (&ictx->ttimer, imon_touch_display_timeout,0 ); - } ictx->usbdev_intf1 =interface_to_usbdev (intf); ictx->rx_urb_intf1 = rx_urb;
@@ -2363,13 +2367,14 @@ static struct imon_context *imon_init_intf1(struct usb_interface *intf, ret =usb_submit_urb (ictx->rx_urb_intf1,GFP_KERNEL );if (ret) { -pr_err ("usb_submit_urb failed for intf1 (%d)\n" , ret); +dev_err (ictx->dev,"usb_submit_urb failed for intf1 (%d)\n" , ret); goto urb_submit_failed; } ictx->dev_present_intf1 = true;mutex_unlock (&ictx->lock); +return ictx; urb_submit_failed:
@@ -2377,11 +2382,14 @@ static struct imon_context *imon_init_intf1(struct usb_interface *intf,input_unregister_device (ictx->touch); touch_setup_failed: find_endpoint_failed: +if (ictx->display_type ==IMON_DISPLAY_TYPE_VGA ) +timer_delete_sync (&ictx->ttimer); ictx->usbdev_intf1 =NULL ;mutex_unlock (&ictx->lock);usb_free_urb (rx_urb); ictx->rx_urb_intf1 =NULL ; rx_urb_alloc_failed: +usb_driver_release_interface (&imon_driver, intf);dev_err (ictx->dev,"unable to initialize intf1, err %d\n" , ret);return NULL ;
@@ -2416,90 +2424,50 @@ static void imon_init_display(struct imon_context *ictx,static int imon_probe (struct usb_interface *interface,const struct usb_device_id *id) { -struct usb_device *usbdev =NULL ; -struct usb_host_interface *iface_desc =NULL ; -struct usb_interface *first_if; +struct usb_device *usbdev; +struct usb_interface *second_if;struct device *dev = &interface->dev; -int ifnum, sysfs_err; -int ret =0 ; -struct imon_context *ictx =NULL ; +int sysfs_err; +struct imon_context *ictx;u16 vendor, product; - usbdev =interface_to_usbdev (interface); - iface_desc = interface->cur_altsetting; - ifnum = iface_desc->desc.bInterfaceNumber; - vendor =le16_to_cpu (usbdev->descriptor.idVendor); - product =le16_to_cpu (usbdev->descriptor.idProduct); +if (interface->cur_altsetting->desc.bInterfaceNumber !=0 ) +return -ENODEV ; -dev_dbg (dev,"%s: found iMON device (%04x:%04x, intf%d)\n" , - __func__, vendor, product, ifnum); + usbdev =interface_to_usbdev (interface); + vendor =le16_to_cpu (usbdev->descriptor.idVendor); + product =le16_to_cpu (usbdev->descriptor.idProduct); - first_if =usb_ifnum_to_if (usbdev,0 ); -if (!first_if) { - ret = -ENODEV ; - goto fail; - } +dev_dbg (dev,"found iMON device (%04x:%04x)\n" , vendor, product); -if (first_if->dev.driver != interface->dev.driver) { -dev_err (&interface->dev,"inconsistent driver matching\n" ); - ret = -EINVAL ; - goto fail; - } + ictx =imon_init_intf0 (interface, id); +if (!ictx) +return -ENODEV ; -if (ifnum ==0 ) { - ictx =imon_init_intf0 (interface, id); -if (!ictx) { -pr_err ("failed to initialize context!\n" ); - ret = -ENODEV ; - goto fail; - } -refcount_set (&ictx->users,1 ); +refcount_set (&ictx->users,1 ); - }else { -/* this is the secondary interface on the device */ -struct imon_context *first_if_ctx =usb_get_intfdata (first_if); - -/* fail early if first intf failed to register */ -if (!first_if_ctx) { - ret = -ENODEV ; - goto fail; - } +usb_set_intfdata (interface, ictx); - ictx =imon_init_intf1 (interface, first_if_ctx); -if (!ictx) { -pr_err ("failed to attach to context!\n" ); - ret = -ENODEV ; - goto fail; - } +/* Newer devices export a second interface for display/touchscreen */ + second_if =usb_ifnum_to_if (usbdev,1 ); +if (second_if &&imon_init_intf1 (second_if, ictx))refcount_inc (&ictx->users); +if (product ==0xffdc && ictx->rf_device) { + sysfs_err =sysfs_create_group (&interface->dev.kobj, + &imon_rf_attr_group); +if (sysfs_err) +pr_err ("Could not create RF sysfs entries(%d)\n" , + sysfs_err); } -usb_set_intfdata (interface, ictx); +if (ictx->display_supported) +imon_init_display (ictx, interface); -if (ifnum ==0 ) { -if (product ==0xffdc && ictx->rf_device) { - sysfs_err =sysfs_create_group (&interface->dev.kobj, - &imon_rf_attr_group); -if (sysfs_err) -pr_err ("Could not create RF sysfs entries(%d)\n" , - sysfs_err); - } - -if (ictx->display_supported) -imon_init_display (ictx, interface); - } - -dev_info (dev,"iMON device (%04x:%04x, intf%d) on usb<%d:%d> initialized\n" , - vendor, product, ifnum, - usbdev->bus->busnum, usbdev->devnum); +dev_info (dev,"iMON device (%04x:%04x) on usb<%d:%d> initialized\n" , + vendor, product, usbdev->bus->busnum, usbdev->devnum);return 0 ; - -fail: -dev_err (dev,"unable to register, err %d\n" , ret); - -return ret; }/*
@@ -2507,11 +2475,15 @@ static int imon_probe(struct usb_interface *interface,*/ static void imon_disconnect (struct usb_interface *interface) { +struct usb_device *usbdev =interface_to_usbdev (interface); +struct usb_interface *other_if;struct imon_context *ictx;struct device *dev;int ifnum; ictx =usb_get_intfdata (interface); +if (!ictx) +return ;mutex_lock (&ictx->lock); ictx->disconnected = true;
@@ -2547,6 +2519,7 @@ static void imon_disconnect(struct usb_interface *interface)else if (ictx->display_type ==IMON_DISPLAY_TYPE_VFD )usb_deregister_dev (interface, &imon_vfd_class); } + other_if =usb_ifnum_to_if (usbdev,1 ); }else { ictx->dev_present_intf1 = false;usb_kill_urb (ictx->rx_urb_intf1);
@@ -2554,13 +2527,16 @@ static void imon_disconnect(struct usb_interface *interface)timer_delete_sync (&ictx->ttimer);input_unregister_device (ictx->touch); } + other_if =usb_ifnum_to_if (usbdev,0 ); } +if (other_if) +usb_driver_release_interface (&imon_driver, other_if); +if (refcount_dec_and_test (&ictx->users))free_imon_context (ictx); -dev_dbg (dev,"%s: iMON device (intf%d) disconnected\n" , - __func__, ifnum); +dev_dbg (dev,"iMON device (intf%d) disconnected\n" , ifnum); }static int imon_suspend (struct usb_interface *intf,pm_message_t message)
Signature · 1 line
2.55.0
Sean Young
maintainerPATCH v5 11/20 media: ir_toy: Remove unused struct field
In reply to Sean YoungNever assigned or read.
--- drivers/media/rc/ir_toy.c | 2 -- 1 file changed, 2 deletions(-)
drivers/media/rc/ir_toy.c +0 -2
@@ -55,7 +55,6 @@ enum state {struct irtoy {struct device *dev; -struct usb_device *usbdev;struct rc_dev *rc;struct urb *urb_in, *urb_out;
@@ -441,7 +440,6 @@ static int irtoy_probe(struct usb_interface *intf, irtoy_out_callback, irtoy); irtoy->dev = &intf->dev; - irtoy->usbdev = usbdev; irtoy->rc = rc; irtoy->urb_out = urb; irtoy->pulse = true;
Signature · 1 line
2.55.0
Sean Young
maintainerPATCH v5 12/20 media: nuvoton-cir: Remove unused struct field
In reply to Sean YoungNever assigned or read.
--- drivers/media/rc/nuvoton-cir.h | 3 --- 1 file changed, 3 deletions(-)
drivers/media/rc/nuvoton-cir.h +0 -3
@@ -77,9 +77,6 @@ struct nvt_dev {/* hardware id */ u8 chip_major;u8 chip_minor; - -/* carrier period = 1 / frequency */ -u32 carrier; };/* buffer packet constants */
Signature · 1 line
2.55.0
Sean Young
maintainerPATCH v5 13/20 media: ite-cir: Removed unused struct field
In reply to Sean YoungNever assigned or read.
--- drivers/media/rc/ite-cir.c | 1 - drivers/media/rc/ite-cir.h | 1 - 2 files changed, 2 deletions(-)
drivers/media/rc/ite-cir.c +0 -1
@@ -1359,7 +1359,6 @@ static int ite_probe(struct pnp_dev *pdev, const struct pnp_device_id/* set driver data into the pnp device */ pnp_set_drvdata (pdev, itdev); - itdev->pdev = pdev;/* initialize waitqueues for transmission */ init_waitqueue_head (&itdev->tx_queue);
drivers/media/rc/ite-cir.h +0 -1
@@ -76,7 +76,6 @@ struct ite_dev_params {/* ITE CIR device structure */ struct ite_dev { -struct pnp_dev *pdev;struct rc_dev *rdev;/* sync data */
Signature · 1 line
2.55.0
Sean Young
maintainerPATCH v5 14/20 media: fintek-cir: Remove unused fields
In reply to Sean YoungSome of these fields are vestigal from transmit, which never worked.
--- drivers/media/rc/fintek-cir.c | 13 ------------- drivers/media/rc/fintek-cir.h | 22 ---------------------- 2 files changed, 35 deletions(-)
drivers/media/rc/fintek-cir.c +0 -13
@@ -140,17 +140,6 @@ static int fintek_hw_detect(struct fintek_dev *fintek) ir_class =fintek_cir_reg_read (fintek,CIR_CR_CLASS );fit_dbg ("ir_class reg: 0x%02x" , ir_class); -switch (ir_class) { -case CLASS_RX_2TX : -case CLASS_RX_1TX : - fintek->hw_tx_capable = true; -break ; -case CLASS_RX_ONLY : -default : - fintek->hw_tx_capable = false; -break ; - } - chip_major =fintek_cr_read (fintek,GCR_CHIP_ID_HI ); chip_minor =fintek_cr_read (fintek,GCR_CHIP_ID_LO ); chip = chip_major <<8 | chip_minor;
@@ -169,7 +158,6 @@ static int fintek_hw_detect(struct fintek_dev *fintek)spin_lock_irqsave (&fintek->fintek_lock, flags); fintek->chip_major = chip_major; fintek->chip_minor = chip_minor; - fintek->chip_vendor = vendor;/* * Newer reviews of this chipset uses port 8 instead of 5
@@ -496,7 +484,6 @@ static int fintek_probe(struct pnp_dev *pdev, const struct pnp_device_id *dev_idspin_lock_init (&fintek->fintek_lock);pnp_set_drvdata (pdev, fintek); - fintek->pdev = pdev; ret =fintek_hw_detect (fintek);if (ret)
drivers/media/rc/fintek-cir.h +0 -22
@@ -40,11 +40,9 @@ static int debug; KBUILD_MODNAME ": " text "\n" , ## __VA_ARGS__) -#define TX_BUF_LEN 256#define RX_BUF_LEN 32struct fintek_dev { -struct pnp_dev *pdev;struct rc_dev *rdev;spinlock_t fintek_lock;
@@ -53,14 +51,6 @@ struct fintek_dev {u8 buf[RX_BUF_LEN ];unsigned int pkts; -struct { -spinlock_t lock; -u8 buf[TX_BUF_LEN ]; -unsigned int buf_count; -unsigned int cur_buf_num; -wait_queue_head_t queue; - } tx; -/* Config register index/data port pair */ u32 cr_ip;u32 cr_dp;
@@ -73,17 +63,8 @@ struct fintek_dev {/* hardware id */ u8 chip_major;u8 chip_minor; -u16 chip_vendor;u8 logical_dev_cir; -/* hardware features */ -bool hw_learning_capable; -bool hw_tx_capable; - -/* rx settings */ -bool learning_enabled; -bool carrier_detect_enabled; -enum {CMD_HEADER =0 ,SUBCMD ,
@@ -92,9 +73,6 @@ struct fintek_dev { } parser_state;u8 cmd, rem; - -/* carrier period = 1 / frequency */ -u32 carrier; };/* buffer packet constants, largely identical to mceusb.c */
Signature · 1 line
2.55.0
Sean Young
maintainerPATCH v5 15/20 media: mceusb: Remove unused field
In reply to Sean YoungNever assigned or read.
--- drivers/media/rc/mceusb.c | 1 - 1 file changed, 1 deletion(-)
drivers/media/rc/mceusb.c +0 -1
@@ -484,7 +484,6 @@ struct mceusb_dev {u8 cmd, rem;/* Remaining IR data bytes in packet */ struct { -u32 connected:1 ;u32 tx_mask_normal:1 ;u32 microsoft_gen1:1 ;u32 no_tx:1 ;
Signature · 1 line
2.55.0
Sean Young
maintainerPATCH v5 16/20 media: serial_ir: Fix race condition where timer can be re-armed
In reply to Sean YoungEnsure the timer cannot be re-armed.
--- drivers/media/rc/serial_ir.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/media/rc/serial_ir.c +1 -1
@@ -798,7 +798,7 @@ static int __init serial_ir_init_module(void) static void __exit serial_ir_exit_module(void) { -timer_delete_sync (&serial_ir.timeout_timer); +timer_shutdown_sync (&serial_ir.timeout_timer);serial_ir_exit (); }
Signature · 1 line
2.55.0
Sean Young
maintainerPATCH v5 17/20 media: rc: After rc_unregister_device() timers can be re-armed
In reply to Sean YoungIf rc_keydown() is called after rc_register_device(), then the timers can be re-armed. Delete them again for a final time.
--- drivers/media/rc/rc-main.c | 5 +++++ 1 file changed, 5 insertions(+)
drivers/media/rc/rc-main.c +5 -0
@@ -1785,6 +1785,11 @@ void rc_free_device(struct rc_dev *dev)if (!dev)return ; +if (dev->input_dev) { +timer_delete_sync (&dev->timer_keyup); +timer_delete_sync (&dev->timer_repeat); + } +input_put_device (dev->input_dev);put_device (&dev->dev);
Signature · 1 line
2.55.0
Sean Young
maintainerPATCH v5 18/20 media: rc: Validate carrier range in LIRC_SET_REC_CARRIER ioctl
In reply to Sean YoungEnsure that the low value of the carrier range is smaller than the high value. This fixes an underflow in ite_set_rx_carrier_range().
--- drivers/media/rc/lirc_dev.c | 2 ++ drivers/media/rc/rc-loopback.c | 5 ----- 2 files changed, 2 insertions(+), 5 deletions(-)
drivers/media/rc/lirc_dev.c +2 -0
@@ -492,6 +492,8 @@ static long lirc_ioctl(struct file *file, unsigned int cmd, unsigned long arg) ret = -ENOTTY ; else if (val <= 0) ret = -EINVAL ; + else if (fh->carrier_low && fh->carrier_low > val) + ret = -EINVAL ; else ret = dev->s_rx_carrier_range (dev, fh->carrier_low, val);
drivers/media/rc/rc-loopback.c +0 -5
@@ -74,11 +74,6 @@ static int loop_set_rx_carrier_range(struct rc_dev *dev, u32 min, u32 max) {struct loopback_dev *lodev = dev->priv; -if (min <1 || min > max) { -dev_dbg (&dev->dev,"invalid rx carrier range %u to %u\n" , min, max); -return -EINVAL ; - } -dev_dbg (&dev->dev,"setting rx carrier range %u to %u\n" , min, max); lodev->rxcarriermin = min; lodev->rxcarriermax = max;
Signature · 1 line
2.55.0
Sean Young
maintainerPATCH v5 19/20 media: ir_toy: Validate the maximum tx carrier
In reply to Sean YoungReject any carrier the hardware cannot generate. This also fixes a divide by zero if a carrier of U32_MAX/16 is set.
--- drivers/media/rc/ir_toy.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/media/rc/ir_toy.c +1 -1
@@ -372,7 +372,7 @@ static int irtoy_tx_carrier(struct rc_dev *rc, uint32_t carrier)u8 buf[3 ];int err; -if (carrier <11800 ) +if (carrier <11800 || carrier >3000000 )return -EINVAL ; buf[0 ] =0x06 ;
Signature · 1 line
2.55.0
Sean Young
maintainerPATCH v5 20/20 media: meson-ir-tx: Prevent divide by zero in meson_irtx_prepare_pulse()
In reply to Sean YoungIf an excesively high carrier is set, meson_irtx_prepare_pulse() will cause a divide by zero if tb_us is 0. Prevent such a carrier from being set.
--- drivers/media/rc/meson-ir-tx.c | 3 +++ 1 file changed, 3 insertions(+)
drivers/media/rc/meson-ir-tx.c +3 -0
@@ -219,6 +219,9 @@ static int meson_irtx_set_carrier(struct rc_dev *rc, u32 carrier)if (carrier ==0 )return -EINVAL ; +if (!DIV_ROUND_CLOSEST (USEC_PER_SEC , carrier)) +return -EINVAL ; + ir->carrier = carrier;meson_irtx_set_mod (ir);
Signature · 1 line
2.55.0