linux-kernel - Re: [PATCH] regulator: ti-abb: don't use devm_platform_ioremap_resource

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <b5decdec-f33a-4997-a607-e31290fcd83c@smile.fr>
Date: Tue, 23 Jan 2024 10:48:50 +0100
From: Romain Naour <romain.naour@...le.fr>
To: Mark Brown <broonie@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-omap@...r.kernel.org,
 tony@...mide.com, lgirdwood@...il.com, Romain Naour <romain.naour@....com>
Subject: Re: [PATCH] regulator: ti-abb: don't use
 devm_platform_ioremap_resource_byname for shared interrupt register

Hello,

Le 22/01/2024 à 18:30, Mark Brown a écrit :
> On Mon, Jan 22, 2024 at 06:04:42PM +0100, Romain Naour wrote:
> 
>> We can't use devm_platform_ioremap_resource_byname() to remap the
>> interrupt register that can be shared between
>> regulator-abb-{ivahd,dspeve,gpu} drivers instance.
> 
> ...
> 
>> The commit b36c6b1887ff (regulator: ti-abb: Make use of the helper
>> function devm_ioremap related) overlooked the following comment
>> explaining why devm_ioremap() is used in this case:
> 
>> /*
>>  * We may have shared interrupt register offsets which are
>>  * write-1-to-clear between domains ensuring exclusivity.
>>  */
> 
> I have to say that I wouldn't infer from that comment that there is any
> reason why _byname() won't work - one would generally expect that a
> get_resource_by_name() followed by an ioremap() of that resource would
> be equivalent to the combined helper.  Based on the commit log here I
> frankly have no idea what the issue is.  You should also add something
> to the code which makes it clear what the issue is so the same
> conversion isn't performed again, assuming that the fix isn't in the
> helper.

I'm agree with you about the existing comment that is not really crystal clear.

The combined helper introduce a call to devm_request_mem_region() that create a
new busy resource region on PRM_IRQSTATUS_MPU register (0x4ae06010). The first
devm_request_mem_region() call succeed for regulator-abb-ivahd but fail for the
two other regulator-abb-dspeve and regulator-abb-gpu.

Here is the iomem content without this patch:
# cat /proc/iomem | grep -i 4ae06
4ae06010-4ae06013 : 4ae07e34.regulator-abb-ivahd int-address
4ae06014-4ae06017 : 4ae07ddc.regulator-abb-mpu int-address

regulator-abb-dspeve and regulator-abb-gpu are missing due to
devm_request_mem_region() failure (EBUSY)

I don't know how to fix this issue keeping
devm_platform_ioremap_resource_byname() when the same address is used several
time... suggestion welcome.

> 
>>
>> Fixes:
> 
> You're missing the commit here.
> 
>> This partially reverts commit b36c6b1887ffc6b58b556120bfbd511880515247.
> 
> Please include human readable descriptions of things like commits and
> issues being discussed in e-mail in your mails, this makes them much
> easier for humans to read especially when they have no internet access.
> I do frequently catch up on my mail on flights or while otherwise
> travelling so this is even more pressing for me than just being about
> making things a bit easier to read.

I added such human description above in the commit log but forgot to update this
one, sorry.

Thank you for the review.

Best regards,
Romain