On Mon, Jun 20, 2022 at 10:08 PM Aidan MacDonald
<[email protected]> wrote:
>
> This flag is necessary to prepare for fixing the behavior of unmask
> registers. Existing chips that set mask_base and unmask_base must
> set broken_mask_unmask=1 to declare that they expect the mask bits

Boolean should take true/false.

> will be inverted in both registers, contrary to the usual behavior
> of mask registers.

> diff --git a/include/linux/regmap.h b/include/linux/regmap.h
> index ee2567a0465c..21a70fd99493 100644
> --- a/include/linux/regmap.h
> +++ b/include/linux/regmap.h
> @@ -1523,6 +1523,7 @@ struct regmap_irq_chip {
> bool clear_on_unmask:1;
> bool not_fixed_stride:1;
> bool status_invert:1;
> + bool broken_mask_unmask:1;

Looking at the given context, I would group it with clean_on_unmask above.

The above is weird enough on its own. Can you prepare a precursor
patch that either drops the bit fields of booleans or moves them to
unsigned int?

Note, bit fields in C are beasts when it goes to concurrent access. It
would be nice to ensure these are not the cases of a such.

--
With Best Regards,
Andy Shevchenko

2022-06-21 17:18:26

On Tue, Jun 21, 2022 at 10:04:59PM +0100, Aidan MacDonald wrote:

> Thanks for the advice. After reading this and some of Andy's comments I
> think I understand how to organize this better.

> I'll send a trimmed down series including only regmap changes, and
> instead of removing things right away I'll just mark them deprecated.
> Then the driver patches can go by subsystem (one series per subsystem?)
> and once everything is merged the deprecated stuff in regmap-irq can be
> removed at a later date.

That sounds like it should help a lot, thanks.

Attachments:

(No filename) (567.00 B)
signature.asc (499.00 B)
Download all attachments

2022-06-23 21:01:30

by Aidan MacDonald

[permalink] [raw]

Subject: Re: [PATCH 15/49] regmap-irq: Change the behavior of mask_writeonly

Andy Shevchenko <[email protected]> writes:

> On Tuesday, June 21, 2022, Aidan MacDonald <[email protected]> wrote:
>> Andy Shevchenko <[email protected]> writes:
>>
>> > On Mon, Jun 20, 2022 at 10:08 PM Aidan MacDonald
>> > <[email protected]> wrote:
>> >>
>> >> No drivers currently use mask_writeonly, and in its current form
>> >> it seems a bit misleading. When set, mask registers will be
>> >> updated with regmap_write_bits() instead of regmap_update_bits(),
>> >> but regmap_write_bits() still does a read-modify-write under the
>> >> hood. It's not a write-only operation.
>> >>
>> >> Performing a simple regmap_write() is probably more useful, since
>> >> it can be used for chips that have separate set & clear registers
>> >> for controlling mask bits. Such registers are normally volatile
>> >> and read as 0, so avoiding a register read minimizes bus traffic.
>> >
>> > Reading your explanations and the code, I would rather think about
>> > fixing the regmap_write_bits() to be writeonly op.
>>
>> That's impossible without special hardware support.
>>
>> > Otherwise it's unclear what's the difference between
>> > regmap_write_bits() vs. regmap_update_bits().
>>
>> This was not obvious to me either. They're the same except in how they
>> issue the low-level write op -- regmap_update_bits() will only do the
>> write if the new value differs from the current one. regmap_write_bits()
>> will always do a write, even if the new value is the same.
>
> Okay, it makes a lot of sense for W1C type of bits in the register.
> Also, “reading” might imply to restore last value from cache, no?

Maybe there needs to be some explanation of what the typical use case is
and why you'd choose write_bits() over update_bits(), because the more I
think about it the less clear it is. You're right that the read could be
served from a cache. But I'm not sure if a cache would be safe if even
one bit in the register is volatile, and I can't really see a use case
for write_bits() that doesn't involve volatile behavior of some sort.

In any event, I'm just going to drop this patch and the related driver
patches in favor of removing mask_writeonly entirely, since it looks
like it was never used, and after thinking about it I'm not sure what
I did helps much. If some driver needs write_bits() for mask registers
down the road it's not a big deal to add this back.

>> I think the problem is lack of documentation. I only figured this out
>> by reading the implementation.
>>
>> >> if (d->chip->mask_writeonly)
>> >> - return regmap_write_bits(d->map, reg, mask, val);
>> >> + return regmap_write(d->map, reg, val & mask);
>> >> else
>> >> return regmap_update_bits(d->map, reg, mask, val);

2022-06-23 23:41:21

by Guru Das Srinagesh

[permalink] [raw]

Subject: Re: [PATCH 02/49] regmap-irq: Fix offset/index mismatch in read_sub_irq_data()

On Mon, Jun 20, 2022 at 09:05:57PM +0100, Aidan MacDonald wrote:
> We need to divide the sub-irq status register offset by register
> stride to get an index for the status buffer to avoid an out of
> bounds write when the register stride is greater than 1.
>
> Fixes: a2d21848d921 ("regmap: regmap-irq: Add main status register support")
> Signed-off-by: Aidan MacDonald <[email protected]>

Reviewed-by: Guru Das Srinagesh <[email protected]>

2022-06-24 00:17:17

by Guru Das Srinagesh

[permalink] [raw]

Subject: Re: [PATCH 18/49] mfd: qcom-pm8008: Add broken_mask_unmask irq chip flag

On Tue, Jun 21, 2022 at 11:35:09AM +0200, Andy Shevchenko wrote:
> On Mon, Jun 20, 2022 at 10:08 PM Aidan MacDonald
> <[email protected]> wrote:
> >
> > The qcom-pm8008 appears to use "1 to enable" convention for
> > enabling interrupts, with separate set and clear registers.
> > It's relying on masks and unmasks being inverted from their
>
> It relies
>
> > intuitive meaning, so it needs the broken_mask_unmask flag.
>
> How has it worked until now?

It is as Aidan rightly pointed out. When I was writing the pm8008 driver, I
found that the mask and unmask terminology used in the framework was inverted
when it came to the hardware, so I had to make do and swap them.

It works because in regmap_irq_sync_unlock(), the same mask is used to update
mask_reg and unmask_reg, except that it is inverted for updating the unmask
register. So, by just swapping which register gets updated with the plain mask
and which one gets updated with the inverted mask, I could use the framework to
accomplish the setting and clearing of the correct registers.

2022-06-24 00:45:25

by Guru Das Srinagesh

[permalink] [raw]

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 <[email protected]>
> ---
> 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
>