Received-SPF: pass (google.com: domain of linux-kernel+bounces-132725-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <f1e3d31d-8c24-4cdc-ae26-747f383a937b@gmail.com>
Date: Fri, 5 Apr 2024 12:19:40 +0300
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC PATCH 0/6] Support ROHM BD96801 scalable PMIC
Content-Language: en-US, en-GB
From: Matti Vaittinen <mazziesaccount@gmail.com>
To: Mark Brown <broonie@kernel.org>
Cc: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>,
 Lee Jones <lee@kernel.org>, Rob Herring <robh@kernel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
 Conor Dooley <conor+dt@kernel.org>, Liam Girdwood <lgirdwood@gmail.com>,
 Wim Van Sebroeck <wim@linux-watchdog.org>, Guenter Roeck
 <linux@roeck-us.net>, devicetree@vger.kernel.org,
 linux-kernel@vger.kernel.org, linux-watchdog@vger.kernel.org
References: <cover.1712058690.git.mazziesaccount@gmail.com>
 <f7d454ac-6ecb-4431-a1de-c9b5d1240969@gmail.com>
 <eb03ec33-0627-4986-be04-8e35da390d6b@sirena.org.uk>
 <b6279be8-cf7d-4608-b556-3c01587f0d43@gmail.com>
In-Reply-To: <b6279be8-cf7d-4608-b556-3c01587f0d43@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

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! ~~