Messages in this thread Patch in this message | | | From | Aidan MacDonald <> | Subject | Re: [PATCH] regmap-irq: Use the new num_config_regs property in regmap_add_irq_chip_fwnode | Date | Mon, 07 Nov 2022 23:58:01 +0000 |
| |
Hi Yassine,
> Commit faa87ce9196d ("regmap-irq: Introduce config registers for irq > types") added the num_config_regs, then commit 9edd4f5aee84 ("regmap-irq: > Deprecate type registers and virtual registers") suggested to replace > num_type_reg with it. However, regmap_add_irq_chip_fwnode wasn't modified > to use the new property. Later on, commit 255a03bb1bb3 ("ASoC: wcd9335: > Convert irq chip to config regs") removed the old num_type_reg property > from the WCD9335 driver's struct regmap_irq_chip, causing a null pointer > dereference in regmap_irq_set_type when it tried to index d->type_buf as > it was never allocated in regmap_add_irq_chip_fwnode:
Whoops... My bad. Actually, d->type_buf shouldn't be accessed in this configuration, the bug is in regmap_irq_set_type(). The access should be guarded by "if (d->chip->type_in_mask || d->chip->num_type_reg)" to prevent the NULL deref.
> > Use num_config_regs in regmap_add_irq_chip_fwnode instead of num_type_reg, > and fall back to it if num_config_regs isn't defined to maintain backward > compatibility. > > Fixes: faa87ce9196d ("regmap-irq: Introduce config registers for irq types") > Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com> > --- > drivers/base/regmap/regmap-irq.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c > index 4ef9488d05cd..3de89795f584 100644 > --- a/drivers/base/regmap/regmap-irq.c > +++ b/drivers/base/regmap/regmap-irq.c > @@ -722,6 +722,7 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, > int i; > int ret = -ENOMEM; > int num_type_reg; > + int num_regs; > u32 reg; > > if (chip->num_regs <= 0) > @@ -796,14 +797,20 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode, > goto err_alloc; > } > > - num_type_reg = chip->type_in_mask ? chip->num_regs : chip->num_type_reg; > - if (num_type_reg) { > - d->type_buf_def = kcalloc(num_type_reg, > + /* > + * Use num_config_regs if defined, otherwise fall back to num_type_reg > + * to maintain backward compatibility. > + */ > + num_type_reg = chip->num_config_regs ? chip->num_config_regs > + : chip->num_type_reg;
A chip that uses config regs doesn't need type_buf. If you check regmap_irq_sync_unlock(), you'll see the contents of type_buf aren't written to the registers unless num_type_reg > 0.
If type_in_mask is set, type_buf has a completely different meaning. There are no type registers -- instead each trigger has its own IRQ enable bit. The type_buf records which bit (= trigger type) is used for the logical IRQ.
Regards, Aidan
> + num_regs = chip->type_in_mask ? chip->num_regs : num_type_reg; > + if (num_regs) { > + d->type_buf_def = kcalloc(num_regs, > sizeof(*d->type_buf_def), GFP_KERNEL); > if (!d->type_buf_def) > goto err_alloc; > > - d->type_buf = kcalloc(num_type_reg, sizeof(*d->type_buf), > + d->type_buf = kcalloc(num_regs, sizeof(*d->type_buf), > GFP_KERNEL); > if (!d->type_buf) > goto err_alloc;
| |