| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20220624002106.GC21400@quicinc.com>
Date: Thu, 23 Jun 2022 17:21:06 -0700
From: Guru Das Srinagesh <quic_gurus@...cinc.com>
To: Aidan MacDonald <aidanmacdonald.0x0@...il.com>
CC: <broonie@...nel.org>, <agross@...nel.org>,
<bjorn.andersson@...aro.org>, <srinivas.kandagatla@...aro.org>,
<bgoswami@...eaurora.org>, <gregkh@...uxfoundation.org>,
<rafael@...nel.org>, <cw00.choi@...sung.com>,
<krzysztof.kozlowski@...aro.org>, <b.zolnierkie@...sung.com>,
<myungjoo.ham@...sung.com>, <michael@...le.cc>,
<linus.walleij@...aro.org>, <brgl@...ev.pl>, <tglx@...utronix.de>,
<maz@...nel.org>, <lee.jones@...aro.org>, <mani@...nel.org>,
<cristian.ciocaltea@...il.com>, <wens@...e.org>,
<tharvey@...eworks.com>, <rjones@...eworks.com>,
<mazziesaccount@...il.com>, <orsonzhai@...il.com>,
<baolin.wang7@...il.com>, <zhang.lyra@...il.com>,
<jernej.skrabec@...il.com>, <samuel@...lland.org>,
<lgirdwood@...il.com>, <perex@...ex.cz>, <tiwai@...e.com>,
<linux-kernel@...r.kernel.org>, <linux-gpio@...r.kernel.org>,
<linux-actions@...ts.infradead.org>,
<linux-arm-msm@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>,
<linux-sunxi@...ts.linux.dev>, <alsa-devel@...a-project.org>
Subject: Re: [PATCH 20/49] regmap-irq: Fix inverted handling of unmask
registers
On Mon, Jun 20, 2022 at 09:06:15PM +0100, Aidan MacDonald wrote:
> To me "unmask" suggests that we write 1s to the register when
> an interrupt is enabled. This also makes sense because it's the
> opposite of what the "mask" register does (write 1s to disable
> an interrupt).
>
> But regmap-irq does the opposite: for a disabled interrupt, it
> writes 1s to "unmask" and 0s to "mask". This is surprising and
> deviates from the usual way mask registers are handled.
Thank you for fixing this.
>
> Additionally, mask_invert didn't interact with unmask registers
> properly -- it caused them to be ignored entirely.
>
> Fix this by making mask and unmask registers orthogonal, using
> the following behavior:
>
> * Mask registers are written with 1s for disabled interrupts.
> * Unmask registers are written with 1s for enabled interrupts.
This is more in line with my understanding of the semantics as well.
>
> This behavior supports both normal or inverted mask registers
> and separate set/clear registers via different combinations of
> mask_base/unmask_base. The mask_invert flag is made redundant,
> since an inverted mask register can be described more directly
> as an unmask register.
>
> To cope with existing drivers that rely on the old "backward"
> behavior, check for the broken_mask_unmask flag and swap the
> roles of mask/unmask registers. This is a compatibility measure
> which can be dropped once the drivers are updated to use the
> new, more consistent behavior.
>
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@...il.com>
> ---
> drivers/base/regmap/regmap-irq.c | 96 +++++++++++++++++---------------
> include/linux/regmap.h | 7 ++-
> 2 files changed, 55 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
> index 875415fc3133..082a2981120c 100644
> --- a/drivers/base/regmap/regmap-irq.c
> +++ b/drivers/base/regmap/regmap-irq.c
> @@ -30,6 +30,9 @@ struct regmap_irq_chip_data {
> int irq;
> int wake_count;
>
> + unsigned int mask_base;
> + unsigned int unmask_base;
> +
> void *status_reg_buf;
> unsigned int *main_status_buf;
> unsigned int *status_buf;
> @@ -95,7 +98,6 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
> struct regmap *map = d->map;
> int i, j, ret;
> u32 reg;
> - u32 unmask_offset;
> u32 val;
>
> if (d->chip->runtime_pm) {
> @@ -124,35 +126,23 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
> * suppress pointless writes.
> */
> for (i = 0; i < d->chip->num_regs; i++) {
> - if (!d->chip->mask_base)
> - continue;
> -
> - reg = sub_irq_reg(d, d->chip->mask_base, i);
> - if (d->chip->mask_invert) {
> + if (d->mask_base) {
> + reg = sub_irq_reg(d, d->mask_base, i);
> ret = regmap_irq_update_mask_bits(d, reg,
> - d->mask_buf_def[i], ~d->mask_buf[i]);
> - } else if (d->chip->unmask_base) {
> - /* set mask with mask_base register */
> + d->mask_buf_def[i], d->mask_buf[i]);
> + if (ret != 0)
> + dev_err(d->map->dev, "Failed to sync masks in %x\n",
> + reg);
> + }
> +
> + if (d->unmask_base) {
> + reg = sub_irq_reg(d, d->unmask_base, i);
> ret = regmap_irq_update_mask_bits(d, reg,
> d->mask_buf_def[i], ~d->mask_buf[i]);
> - if (ret < 0)
> - dev_err(d->map->dev,
> - "Failed to sync unmasks in %x\n",
> + if (ret != 0)
> + dev_err(d->map->dev, "Failed to sync masks in %x\n",
> reg);
> - unmask_offset = d->chip->unmask_base -
> - d->chip->mask_base;
> - /* clear mask with unmask_base register */
> - ret = regmap_irq_update_mask_bits(d,
> - reg + unmask_offset,
> - d->mask_buf_def[i],
> - d->mask_buf[i]);
> - } else {
> - ret = regmap_irq_update_mask_bits(d, reg,
> - d->mask_buf_def[i], d->mask_buf[i]);
> }
> - if (ret != 0)
> - dev_err(d->map->dev, "Failed to sync masks in %x\n",
> - reg);
>
> reg = sub_irq_reg(d, d->chip->wake_base, i);
> if (d->wake_buf) {
> @@ -634,7 +624,6 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
> int i;
> int ret = -ENOMEM;
> u32 reg;
> - u32 unmask_offset;
>
> if (chip->num_regs <= 0)
> return -EINVAL;
> @@ -732,6 +721,24 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
> d->chip = chip;
> d->irq_base = irq_base;
>
> + /*
> + * Swap role of mask_base and unmask_base if mask bits are inverted.
> + *
> + * Historically, chips that specify both mask_base and unmask_base
> + * got inverted mask behavior; this was arguably a bug in regmap-irq
> + * and there was no way to get the normal, non-inverted behavior.
> + * Those chips will set the broken_mask_unmask flag. They don't set
> + * mask_invert so there is no need to worry about interactions with
> + * that flag.
> + */
> + if (chip->mask_invert || chip->broken_mask_unmask) {
I'm not able to comment on whether mask_invert belongs here.
> + d->mask_base = chip->unmask_base;
> + d->unmask_base = chip->mask_base;
> + } else {
> + d->mask_base = chip->mask_base;
> + d->unmask_base = chip->unmask_base;
> + }
> +
> if (chip->irq_reg_stride)
> d->irq_reg_stride = chip->irq_reg_stride;
> else
> @@ -755,28 +762,27 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
> /* Mask all the interrupts by default */
> for (i = 0; i < chip->num_regs; i++) {
> d->mask_buf[i] = d->mask_buf_def[i];
> - if (!chip->mask_base)
> - continue;
>
> - reg = sub_irq_reg(d, d->chip->mask_base, i);
> -
> - if (chip->mask_invert)
> + if (d->mask_base) {
> + reg = sub_irq_reg(d, d->mask_base, i);
> ret = regmap_irq_update_mask_bits(d, reg,
> - d->mask_buf[i], ~d->mask_buf[i]);
> - else if (d->chip->unmask_base) {
> - unmask_offset = d->chip->unmask_base -
> - d->chip->mask_base;
> - ret = regmap_irq_update_mask_bits(d,
> - reg + unmask_offset,
> - d->mask_buf[i],
> - d->mask_buf[i]);
> - } else
> + d->mask_buf_def[i], d->mask_buf[i]);
> + if (ret != 0) {
> + dev_err(map->dev, "Failed to set masks in 0x%x: %d\n",
> + reg, ret);
> + goto err_alloc;
> + }
> + }
> +
> + if (d->unmask_base) {
This makes a lot of sense. unmask_base really needed to be handled separately
and not as an offset from mask_base.
> + reg = sub_irq_reg(d, d->unmask_base, i);
> ret = regmap_irq_update_mask_bits(d, reg,
> - d->mask_buf[i], d->mask_buf[i]);
> - if (ret != 0) {
> - dev_err(map->dev, "Failed to set masks in 0x%x: %d\n",
> - reg, ret);
> - goto err_alloc;
> + d->mask_buf_def[i], ~d->mask_buf[i]);
> + if (ret != 0) {
> + dev_err(map->dev, "Failed to set masks in 0x%x: %d\n",
> + reg, ret);
> + goto err_alloc;
> + }
> }
>
> if (!chip->init_ack_masked)
> diff --git a/include/linux/regmap.h b/include/linux/regmap.h
> index 21a70fd99493..0cf3c4a66946 100644
> --- a/include/linux/regmap.h
> +++ b/include/linux/regmap.h
> @@ -1451,10 +1451,11 @@ struct regmap_irq_sub_irq_map {
> * main_status set.
> *
> * @status_base: Base status register address.
> - * @mask_base: Base mask register address.
> + * @mask_base: Base mask register address. Mask bits are set to 1 when an
> + * interrupt is masked, 0 when unmasked.
> * @mask_writeonly: Base mask register is write only.
> - * @unmask_base: Base unmask register address. for chips who have
> - * separate mask and unmask registers
> + * @unmask_base: Base unmask register address. Unmask bits are set to 1 when
> + * an interrupt is unmasked and 0 when masked.
> * @ack_base: Base ack address. If zero then the chip is clear on read.
> * Using zero value is possible with @use_ack bit.
> * @wake_base: Base address for wake enables. If zero unsupported.
> --
> 2.35.1
>
Powered by blists - more mailing lists