linux-kernel - Re: [PATCH 2/4] ath10k: snoc: fix unabalanced regulator error handling

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: <CAD=FV=U0uK25mL9_d1nVB7uBV+vR9YTqGm1iTQgzdNuZO5O5_A@mail.gmail.com>
Date:   Thu, 18 Oct 2018 10:54:39 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Brian Norris <briannorris@...omium.org>
Cc:     kvalo@....qualcomm.com, ath10k@...ts.infradead.org,
        linux-wireless@...r.kernel.org,
        Govind Singh <govinds@...eaurora.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/4] ath10k: snoc: fix unabalanced regulator error handling

Hi,

On Fri, Oct 12, 2018 at 5:55 PM Brian Norris <briannorris@...omium.org> wrote:
> +       if (vreg_info->settle_delay)
> +               udelay(vreg_info->settle_delay);

Not new to your patch, but this seems like a duplication of what the
regulator framework is doing for you.  There are plenty of regulator
properties describing lots of different types delays and that would be
the place to put it.  Doing so makes it automatically easy for boards
to specify a different delay if they have different ramp
characteristics (like someone put a bigger capacitor on the line or
somesuch).

At the moment it would be easy for someone to submit a patch to kill
the settle delay in this driver this since the entire vreg_cfg table
has 0 for the settle delay.

> +static int __ath10k_snoc_vreg_off(struct ath10k *ar,
> +                                 struct ath10k_vreg_info *vreg_info)
> +{
> +       int ret;
> +
> +       ath10k_dbg(ar, ATH10K_DBG_SNOC, "snoc regulator %s being disabled\n",
> +                  vreg_info->name);
> +
> +       ret = regulator_disable(vreg_info->reg);
> +       if (ret)
> +               ath10k_err(ar, "failed to disable regulator %s\n",
> +                          vreg_info->name);
> +
> +       ret = regulator_set_load(vreg_info->reg, 0);
> +       if (ret < 0)
> +               ath10k_err(ar, "failed to set load %s\n", vreg_info->name);
> +
> +       ret = regulator_set_voltage(vreg_info->reg, 0, vreg_info->max_v);
> +       if (ret)
> +               ath10k_err(ar, "failed to set voltage %s\n", vreg_info->name);

Not new to your patch, but ick, forcing someone to manually set the
load / voltage of a regulator that they've turned off is silly.  It's
only list to try to send a patch to remedy this situation.  Let me try
to put that higher on my plate.

...just like with the clock patch I suspect that some of this is
duplicating the "regulator_bulk" APIs.  ...though I guess maybe we
can't use those too easily until we can avoid setting voltage and
current so much...  In any case, your patch overall looks good and an
improvement.

Reviewed-by: Douglas Anderson <dianders@...omium.org>