linux-kernel - Re: [PATCH 3/3] phy: amlogic: meson8b-usb2: fix shared reset control use

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: <b7989575-e7f0-a4c4-9f00-7500b432571e@baylibre.com>
Date:   Tue, 17 Nov 2020 10:13:55 +0100
From:   Amjad Ouled-Ameur <aouledameur@...libre.com>
To:     Martin Blumenstingl <martin.blumenstingl@...glemail.com>
Cc:     Kevin Hilman <khilman@...libre.com>,
        Felipe Balbi <balbi@...nel.org>, linux-usb@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Philipp Zabel <p.zabel@...gutronix.de>,
        linux-amlogic@...ts.infradead.org,
        Jerome Brunet <jbrunet@...libre.com>
Subject: Re: [PATCH 3/3] phy: amlogic: meson8b-usb2: fix shared reset control
 use

Hi Martin,

Thank you for the review !

On 14/11/2020 20:11, Martin Blumenstingl wrote:

> Hi Amjad,
>
> On Fri, Nov 13, 2020 at 1:07 AM Amjad Ouled-Ameur
> <aouledameur@...libre.com> wrote:
> [...]
>>          ret = clk_prepare_enable(priv->clk_usb);
>>          if (ret) {
>>                  dev_err(&phy->dev, "Failed to enable USB DDR clock\n");
>> +               reset_control_rearm(priv->reset);
> this should come after reset_control_rearm so we're cleaning up in
> reverse order of initializing things
> (in this case it probably makes no difference since
> reset_control_rearm is not touching any registers, but I'd still have
> it in the correct order to not confuse future developers)

Agreed, it works in this current order since the two lines do not
interfere with each other, but it is cleaner to do it in the reverse
order of initialization. Will fix it in next change.

>>                  clk_disable_unprepare(priv->clk_usb_general);
>>                  return ret;
>>          }
>> @@ -197,6 +199,7 @@ static int phy_meson8b_usb2_power_on(struct phy *phy)
>>                          regmap_read(priv->regmap, REG_ADP_BC, &reg);
>>                          if (reg & REG_ADP_BC_ACA_PIN_FLOAT) {
>>                                  dev_warn(&phy->dev, "USB ID detect failed!\n");
>> +                               reset_control_rearm(priv->reset);
> same here, reset_control_rearm should be after clk_disable_unprepare

Ditto, will fix it in next change.

>
>>                                  clk_disable_unprepare(priv->clk_usb);
>>                                  clk_disable_unprepare(priv->clk_usb_general);
>>                                  return -EINVAL;
>> @@ -216,6 +219,7 @@ static int phy_meson8b_usb2_power_off(struct phy *phy)
>>                                     REG_DBG_UART_SET_IDDQ,
>>                                     REG_DBG_UART_SET_IDDQ);
>>
>> +       reset_control_rearm(priv->reset);
> same here, reset_control_rearm should be after clk_disable_unprepare

Ditto, will fix it in next change.

>
>>          clk_disable_unprepare(priv->clk_usb);
>>          clk_disable_unprepare(priv->clk_usb_general);
>
> Best regards,
> Martin
>

Sincerely,
Amjad