| Message ID | cover.1712058690.git.mazziesaccount@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Support ROHM BD96801 scalable PMIC | expand |
On 4/2/24 16:07, Matti Vaittinen wrote: > Another "oddity" is that the PMIC has two physical IRQ lines. When I > last wrote this patch in 2021 I had some naming collison in debugfs for > the IRQ domains. Back then I used: > irq_domain_update_bus_token(intb_domain, DOMAIN_BUS_WIRED); > to work-around the issue. Now, when rebasing to v6.9-rc1 the naming > collision was gone and things seemed to work. However, it'd be great if > the IRQ code in MFD driver was reviewed by greater minds :) It appears my statement "things seemed to work" is a bit too optimistic. I am afraid my approach of having two separate IRQ domains for one device (and DT-node) is just somehow fundamentally wrong. It'd be great to learn what's the correct "ideology" here. It appears the naming collision is still there. My config just had the CONFIG_GENERIC_IRQ_DEBUGFS disabled. Enabling it shows the same naming collison: debugfs: File ':ocp:interconnect@48000000:segment@100000:target-module@9c000:i2c@0:pmic@60' in directory 'domains' already present! If I'm not mistaken the debugfs file name is generated from the device-tree node path+name. This is a subtle hint that it is not expected there are more than 1 IRQ-domain / device. I guess this kind of makes sense if we can have more than 1 HWIRQ handled by a single domain (I don't recall having to ever write such domain/IRQ-controller before, but I think it should be possible). I have now 3 new questions =) 1. Should we be able to have more than 1 IRQ domain / device? 2. Should regmap_irq support having more than 1 HWIRQ 3. If answer to 1 is "no" - should we protect against this somehow? (see why below). When CONFIG_GENERIC_IRQ_DEBUGFS is disabled, adding the two IRQ controllers with own IRQ domains (intb and errb here) to a single device is seemingly successful. I see no complaints / errors. Also, most of the IRQs seem to work - but not all. In my case trying to issue: cat /proc/interrupts will oops. Also, looking in the /sys/kernel/irq/ lists folders for all the "intb" and "errb" IRQs - but reading the files contained in these directories will cause an oops for all "errb" interrupts except for the first 16. Finally, if I use the irq_domain_update_bus_token(intb_domain, DOMAIN_BUS_WIRED); to add "-1" at the end of the "intb" - domain name resulting domains: :ocp:interconnect@48000000:segment@100000:target-module@9c000:i2c@0:pmic@60 :ocp:interconnect@48000000:segment@100000:target-module@9c000:i2c@0:pmic@60-1 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). Any tips, hints or thoughts on this? Yours, -- Matti
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?
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
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.
On Fri, Apr 05, 2024 at 12:19:40PM +0300, Matti Vaittinen wrote: > 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. Sure.
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
On Mon, Apr 22, 2024 at 01:52:27PM +0300, Matti Vaittinen wrote: > On 4/5/24 12:19, Matti Vaittinen wrote: > > On 4/4/24 16:15, Matti Vaittinen wrote: > > > > 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 ... > 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 don't understand what the difficulty is here - we're creating multiple interrupt controllers so I'd expect to have to have full definitions of each, and since everything is referenced by name from the root regmap_irq_chip which gets registered it's just a case of supplying different names and all the helpers should be fine? > 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: Right, my suggestion is to register multiple regmap_irq instrances - one per parent - and supply a name that allows all the display/debugfs stuff that currently uses the dev_name() to deduplicate. You'd end up sticking -primary, -secondary or whatever name was supplied onto the names we currently use. > 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 Yes, register mutliple controllers with different names.
On 5/9/24 08:08, Mark Brown wrote: > On Mon, Apr 22, 2024 at 01:52:27PM +0300, Matti Vaittinen wrote: >> On 4/5/24 12:19, Matti Vaittinen wrote: >>> On 4/4/24 16:15, Matti Vaittinen wrote: > >>>>> 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 > > ... > >> 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 don't understand what the difficulty is here - we're creating multiple > interrupt controllers so I'd expect to have to have full definitions of > each, and since everything is referenced by name from the root > regmap_irq_chip which gets registered it's just a case of supplying > different names and all the helpers should be fine? > >> 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: > > Right, my suggestion is to register multiple regmap_irq instrances - one > per parent - and supply a name that allows all the display/debugfs stuff > that currently uses the dev_name() to deduplicate. You'd end up > sticking -primary, -secondary or whatever name was supplied onto the > names we currently use. > >> 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 > > Yes, register mutliple controllers with different names. Thanks for the guidance Mark. The controller name is not a problem. Problem is that I don't see a (proper) way to supply a name for the IRQ domain which gets registered by regmap-IRQ. IRQ domain code picks the name for the domain by the device-tree node. Both of our IRQ controllers would be instantiated from same node => the IRQ domain will get same name => debugfs will conflict. My "solution" was simply dropping the ERRB IRQ from the driver (for now at least). I did send that as a series without 'RFC' - but made a mistake and restarted the versioning from v1. I am currently working with 2 other PMICs, one of them does also provide similar setup of two IRQ lines. Thus, I think being able to provide a name (suffix?) for IRQ domain when registering it instead of just using the name of the DT node is something I should look into. It's just nice to know someone else thinks it is valid approach. Thanks for the input! Yours, -- Matti
On Thu, May 09, 2024 at 10:03:43AM +0300, Matti Vaittinen wrote: > On 5/9/24 08:08, Mark Brown wrote: > > > > > > So if we arrange to supply a name when we register multiple domains > > > > > > things should work fine? > Thanks for the guidance Mark. The controller name is not a problem. Problem > is that I don't see a (proper) way to supply a name for the IRQ domain which > gets registered by regmap-IRQ. IRQ domain code picks the name for the domain > by the device-tree node. Both of our IRQ controllers would be instantiated > from same node => the IRQ domain will get same name => debugfs will > conflict. That's why I'm suggesting to add something - just put a name field in the struct. > My "solution" was simply dropping the ERRB IRQ from the driver (for now at > least). I did send that as a series without 'RFC' - but made a mistake and > restarted the versioning from v1. I am currently working with 2 other PMICs, > one of them does also provide similar setup of two IRQ lines. Thus, I think > being able to provide a name (suffix?) for IRQ domain when registering it > instead of just using the name of the DT node is something I should look > into. It's just nice to know someone else thinks it is valid approach. Yes.