linux-kernel - Re: [PATCH v2] usb: typec: qcom: check regulator enable status before disabling it

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: <0b6b864c-3e41-4b41-ac35-9ab8edb156a0@quicinc.com>
Date:   Fri, 25 Aug 2023 18:03:02 +0800
From:   hui liu <quic_huliu@...cinc.com>
To:     Heikki Krogerus <heikki.krogerus@...ux.intel.com>
CC:     Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
        Guenter Roeck <linux@...ck-us.net>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>,
        <linux-arm-msm@...r.kernel.org>, <linux-usb@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <quic_fenglinw@...cinc.com>,
        <subbaram@...cinc.com>
Subject: Re: [PATCH v2] usb: typec: qcom: check regulator enable status before
 disabling it

Hi Heikki,

I will let Bryan to comment, I am using the driver to support the pdphy 
in SMB2352 and there is no external regulator required, so I am just 
using a dummy regulator device and I saw this unbalanced regulator 
disabling warnings, so my intention for this change is just fixing the 
warning message. However, I am fine with whatever suggestion you have, 
since the logic is straightforward, and I can make the changes once you 
have the agreement.

Thanks,
Hui

On 8/24/2023 10:49 PM, Heikki Krogerus wrote:
> On Thu, Aug 24, 2023 at 05:12:14PM +0300, Heikki Krogerus wrote:
>> On Thu, Aug 24, 2023 at 10:32:03AM +0800, Hui Liu via B4 Relay wrote:
>>> From: Hui Liu <quic_huliu@...cinc.com>
>>>
>>> Check regulator enable status before disabling it to avoid
>>> unbalanced regulator disable warnings.
>>>
>>> Reviewed-by: Guenter Roeck <linux@...ck-us.net>
>>> Fixes: a4422ff22142 ("usb: typec: qcom: Add Qualcomm PMIC Type-C driver")
>>> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
>>> Signed-off-by: Hui Liu <quic_huliu@...cinc.com>
>>> ---
>>> Changes in v2:
>>> - Add Fixes tag
>>> - Link to v1: https://lore.kernel.org/r/20230823-qcom-tcpc-v1-1-fa81a09ca056@quicinc.com
>>> ---
>>>   drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_pdphy.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_pdphy.c b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_pdphy.c
>>> index bb0b8479d80f..ca616b17b5b6 100644
>>> --- a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_pdphy.c
>>> +++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_pdphy.c
>>> @@ -422,7 +422,8 @@ static int qcom_pmic_typec_pdphy_disable(struct pmic_typec_pdphy *pmic_typec_pdp
>>>   	ret = regmap_write(pmic_typec_pdphy->regmap,
>>>   			   pmic_typec_pdphy->base + USB_PDPHY_EN_CONTROL_REG, 0);
>>>   
>>> -	regulator_disable(pmic_typec_pdphy->vdd_pdphy);
>>> +	if (regulator_is_enabled(pmic_typec_pdphy->vdd_pdphy))
>>> +		regulator_disable(pmic_typec_pdphy->vdd_pdphy);
>>
>> Would it be an option to just enable the regulator in
>> qcom_pmic_typec_pdphy_start() and disable it in
>> qcom_pmic_typec_pdphy_stop()?
>>
>> Now the whole thing looks weird. That regulator is in practice
>> only disabled and then enabled in one and the same place -
>> pmic_typec_pdphy_reset(). It's not touched anywhere else. That makes
>> the above condition confusing to me. I may be missing something.
>>
>> At least more explanation is needed.
> 
> I took a closer look at these drivers, and I think I understand the
> code path now. This driver is made with an assumption that the
> regulator is "on" when the driver is probed, but in your case it's
> actually "off".
> 
> So there is something wrong here, but I don't know where the root
> cause is. If the regulator is really "on" when this driver is probed,
> then there should be another user for it somewhere (no?). In that case
> the driver can't just switch off the regulator like it does now - this
> part I think really has to be fixed (or explained).
> 
> The problem with your fix is that it will leave the regulator always
> on when the driver is removed, which it really can't do, not at least
> if the regulator was off by default.
> 
> I would propose this:
> 
> diff --git a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_pdphy.c b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_pdphy.c
> index bb0b8479d80f..bbe40634e821 100644
> --- a/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_pdphy.c
> +++ b/drivers/usb/typec/tcpm/qcom/qcom_pmic_typec_pdphy.c
> @@ -449,6 +449,10 @@ int qcom_pmic_typec_pdphy_start(struct pmic_typec_pdphy *pmic_typec_pdphy,
>   
>          pmic_typec_pdphy->tcpm_port = tcpm_port;
>   
> +       ret = regulator_enable(pmic_typec_pdphy->vdd_pdphy);
> +       if (ret)
> +               return ret;
> +
>          ret = pmic_typec_pdphy_reset(pmic_typec_pdphy);
>          if (ret)
>                  return ret;
> @@ -467,6 +471,7 @@ void qcom_pmic_typec_pdphy_stop(struct pmic_typec_pdphy *pmic_typec_pdphy)
>                  disable_irq(pmic_typec_pdphy->irq_data[i].irq);
>   
>          qcom_pmic_typec_pdphy_reset_on(pmic_typec_pdphy);
> +       regulator_disable(pmic_typec_pdphy->vdd_pdphy);
>   }
>   
>   struct pmic_typec_pdphy *qcom_pmic_typec_pdphy_alloc(struct device *dev)
> 
> 
> The problem with it is that the regulator is not going to be disabled
> if there really is another user for it when the component is expected
> to be reset. But as said above, if there really is an other user, then
> this driver simply can't just turn off the regulator.
> 
> thanks,
>