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]
Date:   Thu, 9 Nov 2023 10:52:13 +0100
From:   Javier Carrasco <javier.carrasco.cruz@...il.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Jean Delvare <jdelvare@...e.com>,
        Guenter Roeck <linux@...ck-us.net>,
        Jonathan Corbet <corbet@....net>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>
Cc:     Rob Herring <robh@...nel.org>, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-hwmon@...r.kernel.org,
        linux-doc@...r.kernel.org
Subject: Re: [PATCH 3/4] hwmon: Add support for Amphenol ChipCap 2

On 09.11.23 10:35, Krzysztof Kozlowski wrote:
> On 09/11/2023 09:59, Javier Carrasco wrote:
>>
>>
>> On 09.11.23 09:40, Krzysztof Kozlowski wrote:
>>> On 08/11/2023 17:35, Javier Carrasco wrote:
>>>>>> +
>>>>>> +	data->regulator = devm_regulator_get_optional(dev, "vdd");
>>>>>> +	if (!IS_ERR(data->regulator)) {
>>>>>> +		ret = cc2_retrive_alarm_config(data);
>>>>>> +		if (ret)
>>>>>> +			goto cleanup;
>>>>>> +	} else {
>>>>>> +		/* No access to EEPROM without regulator: no alarm control */
>>>>>
>>>>> Test your code with deferred probe. Are you sure you handle it
>>>>> correctly? To me, it looks like you handle deferred probe the same as
>>>>> any error.
>>>>>
>>>> The -EPROBE_DEFER is propagated to the probe function and it is the
>>>> returned value. I clarified the error path in v2 so no error messages
>>>
>>> Really?
>>>
>>> I see:
>>> if (!IS_ERR(data->regulator)) {
>>> 	// so you do not go here
>>> } else {
>>> 	goto dev_register;
>>> }
>>> dev_register is not error path. So how do you return EPROBE_DEFER?
>>>
>>> Which line of code does it?
>>>
>> EPROBE_DEFER is returned if the command window was missed, which is
> 
> How "command window was missed" is related to the place I commented?
> 
it is right below the comment you added and hence the misunderstanding.
But focusing on the line where your comment is, there is no probe
deferring in that case. This is why I asked if you were talking about
devm_regulator_get_optional() failing, which is not covered by the
deferring mechanism in the current form.

I have never experienced the case where the regulator was still not
available, but I suppose there is no reason why that should never happen.
The regulator is not mandatory and there is no reason to retry if it is
not defined. But in case it is defined and not available, the deferring
would make sense. I could consider that case as well.
>> checked in cc2_retrieve_alarm_config() (there is a typo I just corrected
>> -> cc2_retrive_alarm_config() in the current version). It could then
>> happen where you added a comment, but not because
>> devm_regulator_get_optional() failed.
>>
>> Are you expecting a probe deferring if devm_regulator_get_optional()
>> fails as well? Like if the regulator is still not ready when the
>> function is called.
> 
> We talk only about this place. Not others.
> 
> 
> Best regards,
> Krzysztof
> 
Best regards,
Javier Carrasco

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ