On Thu, Apr 04, 2024 at 10:26:34AM +0300, Matti Vaittinen wrote:

> 1. Should we be able to have more than 1 IRQ domain / device?
> 2. Should regmap_irq support having more than 1 HWIRQ

I would expect each parent interrupt to show up as a separate remap_irq.

> then it seems that reading the IRQ information from the /proc/interrupts
> works as expected. Here I am making a wild guess that the name of the domain
> is used as a key for some data-lookups, and having two domains with a same
> name will either overwrite something or cause wrong domain data to be
> fetched. (This is just guessing for now).

So if we arrange to supply a name when we register multiple domains
things should work fine?

Attachments:

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

2024-04-04 13:16:10

by Matti Vaittinen

[permalink] [raw]

Subject: Re: [RFC PATCH 0/6] Support ROHM BD96801 scalable PMIC

Hi Mark,

On 4/4/24 15:09, Mark Brown wrote:
> On Thu, Apr 04, 2024 at 10:26:34AM +0300, Matti Vaittinen wrote:
>
>> 1. Should we be able to have more than 1 IRQ domain / device?
>> 2. Should regmap_irq support having more than 1 HWIRQ
>
> I would expect each parent interrupt to show up as a separate remap_irq.
>
>> then it seems that reading the IRQ information from the /proc/interrupts
>> works as expected. Here I am making a wild guess that the name of the domain
>> is used as a key for some data-lookups, and having two domains with a same
>> name will either overwrite something or cause wrong domain data to be
>> fetched. (This is just guessing for now).
>
> So if we arrange to supply a name when we register multiple domains
> things should work fine?

Thanks for taking the time to look at my questions :)
I have been debugging this thing whole day today, without getting too
far :) It seems there is something beyond the name collision though.

After I tried adding '-1' to the end of the other domain name to avoid
the debugfs name collision I managed to do couple of successful runs -
after which I reported here that problem seems to be just the naming.
Soon after sending that mail I hit the oops again even though the naming
was fixed.

Further debugging shows that the desc->action->name for the last 28
'errb' IRQs get corrupted. This might point more to the IRQ requester
side - so I need to further study the BD96801 driver side as well as the
regulator_irq_helper. I'm having the creeping feeling that at the end of
the day I need to find the guilty one from the mirror :)

But yes, creating 2 regmap-IRQ controllers for one device seems to
generate naming conflict in the debugfs - so unless I'm mistaken, with
the current regmap-IRQ we can't have more than 1 regmap-IRQ entity for a
single device.

Just please give me some more time to see if I find the cause of the
corruption and I hope I can write more concrete description. For now it
was enough for me to hear having more than 1 IRQ domain / device is not
something on the "DON'T DO THIS" -list.

Yours,
-- Matti

--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

2024-04-05 09:27:16

by Matti Vaittinen

[permalink] [raw]

Subject: Re: [RFC PATCH 0/6] Support ROHM BD96801 scalable PMIC

On 4/4/24 16:15, Matti Vaittinen wrote:
> Hi Mark,
>
> On 4/4/24 15:09, Mark Brown wrote:
>> On Thu, Apr 04, 2024 at 10:26:34AM +0300, Matti Vaittinen wrote:
>>
>>> 1. Should we be able to have more than 1 IRQ domain / device?
>>> 2. Should regmap_irq support having more than 1 HWIRQ
>>
>> I would expect each parent interrupt to show up as a separate remap_irq.
>>
>>> then it seems that reading the IRQ information from the /proc/interrupts
>>> works as expected. Here I am making a wild guess that the name of the
>>> domain
>>> is used as a key for some data-lookups, and having two domains with a
>>> same
>>> name will either overwrite something or cause wrong domain data to be
>>> fetched. (This is just guessing for now).

This was wrong guessing.

>> So if we arrange to supply a name when we register multiple domains
>> things should work fine?

After my latest findings, yes, I think so. How to do this correctly is
beyond me though. The __irq_domain_create() seems to me that the name is
meant to be the dt-node name when the controller is backed by a real
dt-node. Naming of the irq_domain_alloc_named_fwnode() sounds to me like
it is only intended to be used when there is no real fwnode. All
suggestions appreciated. Using the:
irq_domain_update_bus_token(intb_domain, DOMAIN_BUS_WIRED);
feels like a dirty hack, and won't scale if there is more HWIRQs.

> Thanks for taking the time to look at my questions :)
> I have been debugging this thing whole day today, without getting too
> far :) It seems there is something beyond the name collision though.
>
> After I tried adding '-1' to the end of the other domain name to avoid
> the debugfs name collision I managed to do couple of successful runs -
> after which I reported here that problem seems to be just the naming.
> Soon after sending that mail I hit the oops again even though the naming
> was fixed.
>
> Further debugging shows that the desc->action->name for the last 28
> 'errb' IRQs get corrupted. This might point more to the IRQ requester
> side - so I need to further study the BD96801 driver side as well as the
> regulator_irq_helper. I'm having the creeping feeling that at the end of
> the day I need to find the guilty one from the mirror :)

I was not wrong on this one. The regulator_irq_helper() duplicates
memory for the data given in const struct regulator_irq_desc *d - but
it does not duplicate the irq name pointed from the given
regulator_irq_desc. Nor does the request_threaded_irq(). I passed some
of the IRQ names from the stack in the BD96801 driver ... a bug I
should've caught earlier.

Well, good thing is that now I can fix the regulator_irq_helper() to do:

--- a/drivers/regulator/irq_helpers.c
+++ b/drivers/regulator/irq_helpers.c
@@ -352,6 +352,9 @@ void *regulator_irq_helper(struct device *dev,

h->irq = irq;
h->desc = *d;
+ h->desc.name = devm_kstrdup(dev, d->name, GFP_KERNEL);
+ if (!h->desc.name)
+ return ERR_PTR(-ENOMEM);

ret = init_rdev_state(dev, h, rdev, common_errs, per_rdev_errs,
rdev_amount);

I'll send a patch if this sounds like a correct thing to do.

--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~

2024-04-05 21:28:11

Hi dee Ho peeps,

On 4/5/24 12:19, Matti Vaittinen wrote:
> On 4/4/24 16:15, Matti Vaittinen wrote:
>> Hi Mark,
>>
>> On 4/4/24 15:09, Mark Brown wrote:
>>> On Thu, Apr 04, 2024 at 10:26:34AM +0300, Matti Vaittinen wrote:
>>>
>>>> 1. Should we be able to have more than 1 IRQ domain / device?
>>>> 2. Should regmap_irq support having more than 1 HWIRQ
>>>
>>> I would expect each parent interrupt to show up as a separate remap_irq.

..
>>> So if we arrange to supply a name when we register multiple domains
>>> things should work fine?
>
> After my latest findings, yes, I think so. How to do this correctly is
> beyond me though. The __irq_domain_create() seems to me that the name is
> meant to be the dt-node name when the controller is backed by a real
> dt-node. Naming of the irq_domain_alloc_named_fwnode() sounds to me like
> it is only intended to be used when there is no real fwnode. All
> suggestions appreciated. Using the:
> irq_domain_update_bus_token(intb_domain, DOMAIN_BUS_WIRED);
> feels like a dirty hack, and won't scale if there is more HWIRQs.

I tried taking a look at this again.

If we wanted to support multiple HWIRQs / regmap-IRQ controller, it
would require us to duplicate almost everything in the struct
regmap_irq_chip for every new parent IRQ. The status/mask register
information, IRQ type, etc. Naturally, it would require also duplicating
lot of the data contained in the struct regmap_irq_chip_data. I am not
sure if this could be done so the change is not reflected in the
existing IRQ data initialization macros etc. Furthermore, some API
changes would be required like changes to regmap_irq_get_domain().

I am a bit afraid this change, if implemented in regmap-IRQ, would be
very intrusive and potentially impact large amount of callers. But more
importantly, looking the amount of data that should be duplicated per
new HWIRQ makes me think that an IRQ controller is really a product of a
parent IRQ, not product of the device. Hence, assuming there is only one
IRQ controller instance / device does not feel any more correct than
assuming there is an IRQ controller instance / parent IRQ. Same thinking
applies to IRQ domains.

Thus, forcing the regmap-IRQ to support multiple parents instead of
having own regmap-IRQ instance / parent IRQ feels like fitting square
item to a round hole. I am sure fixing all the bugs I caused would give
donate a lot of EXP-points though :rolleyes:

Question is, should I still try?

Another option I see, is trying to think if irq-domain name could be
changed. (This is what the RFC v3 does, [ab]using the
irq_domain_update_bus_token()). I was a bit put off by the idea of
'instantiating' multiple domains (or regmap-IRQ controllers) from a
single node, but more I think of this, more I lean towards it. Besides,
this is not something completely odd, I think MFD devices do this
anyways. (Instantiate multiple [sub]devices from single DT-node). I
would love to get an opinion from someone who knows the 'fundamentals'
of the IRQ domains, and possibly some pointer for the right approach.

Finally we might also consider adding own sub-node in DT for each parent
IRQ - but this feels very wrong to me.

All in all, I am having very hard time trying to think how to proceed.
The last option for me is to skip support for the ERRB IRQ from the
BD96801 driver, which would leave this problem to the next person
working with a device providing multiple physical IRQs.

Yours,
-- Matti

--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~