Messages in this thread Patch in this message | | | Date | Fri, 5 Apr 2024 12:19:40 +0300 | Subject | Re: [RFC PATCH 0/6] Support ROHM BD96801 scalable PMIC | From | Matti Vaittinen <> |
| |
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! ~~
| |