| 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
| ||
|
Message-ID: <9f332148-57ef-4716-8866-36c702a9aeb6@oss.qualcomm.com>
Date: Wed, 4 Jun 2025 17:40:13 +0800
From: Fenglin Wu <fenglin.wu@....qualcomm.com>
To: Krzysztof Kozlowski <krzk@...nel.org>, Sebastian Reichel
<sre@...nel.org>,
Bjorn Andersson <andersson@...nel.org>,
Konrad Dybcio <konradybcio@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley
<conor+dt@...nel.org>,
Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Subbaraman Narayanamurthy <subbaraman.narayanamurthy@....qualcomm.com>,
David Collins <david.collins@....qualcomm.com>,
linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-msm@...r.kernel.org, kernel@....qualcomm.com,
devicetree@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [PATCH v2 6/8] dt-bindings: soc: qcom: pmic-glink: Move X1E80100
out of fallbacks
On 6/3/2025 5:34 PM, Krzysztof Kozlowski wrote:
> On 03/06/2025 09:41, Fenglin Wu wrote:
>> On 6/3/2025 3:06 PM, Krzysztof Kozlowski wrote:
>>> On 03/06/2025 08:59, Fenglin Wu wrote:
>>>> On 6/3/2025 2:47 PM, Krzysztof Kozlowski wrote:
>>>>> On 03/06/2025 08:42, Fenglin Wu wrote:
>>>>>> On 6/2/2025 3:40 PM, Krzysztof Kozlowski wrote:
>>>>>>> On 30/05/2025 09:35, Fenglin Wu via B4 Relay wrote:
>>>>>>>> From: Fenglin Wu <fenglin.wu@....qualcomm.com>
>>>>>>>>
>>>>>>>> Move X1E80100 out of the fallbacks of SM8550 in pmic-glink support.
>>>>>>> Why?
>>>>>>>
>>>>>>> Do not describe what you do here, it's obvious. We see it from the diff.
>>>>>>>
>>>>>>>
>>>>>>> Best regards,
>>>>>>> Krzysztof
>>>>>> Previously, in qcom_battmgr driver, x1e80100 was specified with a match
>>>>>> data the same as sc8280xp, also sm8550 was treated a fallback of sm8350
>>>>>> without the need of a match data.
>>>>>>
>>>>>> In ucsi_glink driver, sm8550 had a match data and x1e80100 was treated
>>>>>> as a fallback of sm8550. There was no issues to make x1e80100 as a
>>>>>> fallback of sm8550 from both qcom_battmgr and ucsi_glink driver perspective.
>>>>>>
>>>>>> In patch [5/8] in this series, in qcom_battmgr driver, it added charge
>>>>>> control functionality for sm8550 and x1e80100 differently hence
>>>>>> different match data was specified for them, and it makes x1e80100 ad
>>>>>> sm8550 incompatible and they need to be treated differently.
>>>>> So you break ABI and that's your problem to fix. You cannot make devices
>>>>> incompatible without good justification.
>>>> I would say x1e80100 and sm8550 are different and incompatible from a
>>>> battery management firmware support perspective. The x1e80100 follows
>>>> the sc8280xp as a compute platform, whereas the sm8550 follows the
>>>> sm8350 as a mobile platform.
>>> Not correct arguments for compatibility.
>>>
>>>> The difference between them was initially ignored because the sm8550
>>>> could use everything that the sm8350 has, and no match data needed to be
>>>> specified for it. However, now the sm8550 has new features that the
>>>> sm8350 doesn't have, requiring us to treat it differently, thus the
>>>> incompatibility was acknowledged.
>>> So they are perfectly compatible.
>>>
>>> I really do not understand what we are discussing here. Explain in
>>> simple terms of DT spec: what is incompatible that SW cannot use one
>>> interface to handle the other?
>> 1. x1e80100 was a fallback of sc8280xp, it used "sc8280xp_bat_psy_desc"
>
> No, that's not true. Read the binding again:
>
> - qcom,x1e80100-pmic-glink
> - const: qcom,sm8550-pmic-glink
>
> No fallback to sc8280xp.
>
>
>> when registering the power supply device.
>>
>> 2. sm8550 was a fallback of sm8350, and they all used
>
> Also not true. The remaining fallback is not sm8350.
>
>
>> "sm8350_bat_psy_desc" when registering the power supply device.
>>
>> 3. x1e80100 and sm8550 they are incompatible as they are using different
>> data structure of "xxx_bat_psy_desc" and other “psy_desc" too, such as,
>> ac/usb/wls.
> Look at the driver and bindings now - they are compatible. It looks like
> you made it incompatible and now you claim the "they are incompatible".
> No, you did it. Look at the driver.
>
>
>
>> 4. For charge control functionality, it's only supported in the battery
>> management firmware in x1e80100 and sm8550 platforms. And the change in
>> battmgr driver (patch [5/8]) adds the support by using 2 additional
>> power supply properties, which eventually need to be added in the
>> "properties" data member of "xxx_bat_psy_desc" when registering power
>> supply devices. Hence, "x1e80100_bat_psy_desc" and "sm8550_bat_psy_desc"
>> are created and used separately when registering power supply device
>> according to the "variant" value defined in the match data.
>>
>> The main code change is in [5/8], I am pasting a snippet which might
>> help to explain this a little bit:
>>
>> - if (battmgr->variant == QCOM_BATTMGR_SC8280XP) {
>> - battmgr->bat_psy = devm_power_supply_register(dev,
>> &sc8280xp_bat_psy_desc, &psy_cfg);
>> + if (battmgr->variant == QCOM_BATTMGR_SC8280XP ||
>> battmgr->variant == QCOM_BATTMGR_X1E80100) {
>> + if (battmgr->variant == QCOM_BATTMGR_X1E80100)
>> + psy_desc = &x1e80100_bat_psy_desc;
>> + else
>> + psy_desc = &sc8280xp_bat_psy_desc;
>> +
>> + battmgr->bat_psy = devm_power_supply_register(dev,
>> psy_desc, &psy_cfg);
>> if (IS_ERR(battmgr->bat_psy))
>> return dev_err_probe(dev,
>> PTR_ERR(battmgr->bat_psy),
>
> This explains nothing to me. I think you did not get my questions at all
> and just want to push whatever you have in drivers.
>
> Such ping pongs are just tiring, so go back to my previous email, read
> it carefully and try harder to understand what compatibility means.
>
>
> NAK, you are affecting the users and ABI with justification "I make it
> now incompatible, so it is incompatible".
>
> Best regards,
> Krzysztof
Thanks for the explanation with patience. I misunderstood the fallback
behavior.
I was worried about if the compatible string matching would work
correctly if both the device node and the driver declared multiple
identical compatible strings.
I understand now and even if the device node and the driver have defined
multiple identical compatible strings, the best match which is the most
specific compatible string will be found.
So in the example below, for X1E80100-CRD, the battmgr driver will
always match to "qcom,x1e80100-pmic-glink" which is the most specific
compatible string defined at the beginning of the device node compatible
string, and the compatibility has not been broken.
In qcom_battmgr driver:
static const struct of_device_id qcom_battmgr_of_variants[] = {
...
{ .compatible = "qcom,x1e80100-pmic-glink", .data = (void
*)QCOM_BATTMGR_X1E80100 },
{ .compatible = "qcom,sm8550-pmic-glink", .data = (void
*)QCOM_BATTMGR_SM8550 },
...
};
In x1-crd.dtsi:
pmic-glink {
compatible = "qcom,x1e80100-pmic-glink",
"qcom,sm8550-pmic-glink",
"qcom,pmic-glink";
...
}
Let me know if my understanding is correct. I will drop patch
[6/8],[7/8],[8/8] in next version.
Thanks
Powered by blists - more mailing lists