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] [thread-next>] [day] [month] [year] [list]
Message-ID: <ba54014c-429b-44ed-a887-e25a4bf033c0@roeck-us.net>
Date: Mon, 12 Aug 2024 14:26:00 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Javier Carrasco <javier.carrasco.cruz@...il.com>,
 Jean Delvare <jdelvare@...e.com>
Cc: linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] hwmon: chipcap2: disable sensor if request ready irq
 fails

On 8/12/24 13:48, Javier Carrasco wrote:
> On 12/08/2024 22:08, Guenter Roeck wrote:
>> On 8/12/24 12:59, Javier Carrasco wrote:
>>> On 12/08/2024 18:49, Guenter Roeck wrote:
>>>> On 8/12/24 08:43, Javier Carrasco wrote:
>>>>> This check is carried out after getting the regulator, and the device
>>>>> can be disabled if an error occurs.
>>>>>
>>>>
>>>> I do not see a possible path for a call to cc2_enable() at this point,
>>>> meaning the regulator won't ever be enabled. Please provide a better
>>>> explanation why this patch would be necessary.
>>>>
>>>> Guenter
>>>>
>>>
>>> Hi Guenter,
>>>
>>> this patch enforces the state where the dedicated regulator is disabled,
>>> no matter what the history of the regulator was. If a previous
>>> regulator_disable() failed, it would still be desirable that the
>>> regulator gets disabled the next time the driver is probed (i.e. a new
>>> attempt to disable it on failure).
>>> cc2_disable() checks first if the regulator is enabled to avoid any
>>> imbalance.
>>>
>>
>> That is very theoretic. Sorry, I am not going to accept this patch.
>>
>> Guenter
>>
> 
> I get your point, but given that this device requires a dedicated
> regulator, I believe it makes sense that it tries to disable it whenever
> possible if it's not going to be used. I think that makes more sense
> that just returning an error value without even making sure that de
> regulator was disabled, doesn't it?
> 

No, it doesn't make any sense whatsoever. What are you planning to do,
clutter the kernel with code to disable regulators if instantiating a device
fails for whatever reason and it turns out that a regulator which should
not have been enabled to start with turns out to be enabled anyway ?

> Of course this is not a killer feature, and I don't want to make you
> waste much time with it. But I think the dedicated regulator should be
> shut down in all error paths, whatever status it had before.
> 

I strongly disagree. This can only mess up the kernel all over the place.
Maybe you can convince other maintainers to accept such code, but please
refrain from doing that in my scope of responsibility. If the regulator
subsystem has the habit of leaving regulators enabled even after they
have been released, that problem should be fixed in the regulator subsystem
and not be worked around in individual drivers.

Guenter


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ