linux-kernel - Re: [PATCH v4 3/4] nvmem: qfprom: Add fuse blowing support

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:   Tue, 7 Jul 2020 07:52:38 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Andy Gross <agross@...nel.org>
Cc:     mturney@...eaurora.org, Jeffrey Hugo <jhugo@...eaurora.org>,
        Rajendra Nayak <rnayak@...eaurora.org>, dhavalp@...eaurora.org,
        Sai Prakash Ranjan <saiprakash.ranjan@...eaurora.org>,
        sparate@...eaurora.org,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        mkurumel@...eaurora.org, Ravi Kumar Bokka <rbokka@...eaurora.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 3/4] nvmem: qfprom: Add fuse blowing support

Hi,

On Mon, Jun 22, 2020 at 7:49 AM Douglas Anderson <dianders@...omium.org> wrote:
>
> From: Ravi Kumar Bokka <rbokka@...eaurora.org>
>
> This patch adds support for blowing fuses to the qfprom driver if the
> required properties are defined in the device tree.
>
> Signed-off-by: Ravi Kumar Bokka <rbokka@...eaurora.org>
> Signed-off-by: Douglas Anderson <dianders@...omium.org>
> ---
>
> Changes in v4:
> - Only get clock/regulator if all address ranges are provided.
> - Don't use optional version of clk_get now.
> - Clock name is "core", not "sec".
> - Cleaned up error message if couldn't get clock.
> - Fixed up minor version mask.
> - Use GENMASK to generate masks.
>
> Changes in v3:
> - Don't provide "reset" value for things; just save/restore.
> - Use the major/minor version read from 0x6000.
> - Reading should still read "corrected", not "raw".
> - Added a sysfs knob to allow you to read "raw" instead of "corrected"
> - Simplified the SoC data structure.
> - No need for quite so many levels of abstraction for clocks/regulator.
> - Don't set regulator voltage.  Rely on device tree to make sure it's right.
> - Properly undo things in the case of failure.
> - Don't just keep enabling the regulator over and over again.
> - Enable / disable the clock each time
> - Polling every 100 us but timing out in 10 us didn't make sense; swap.
> - No reason for 100 us to be SoC specific.
> - No need for reg-names.
> - We shouldn't be creating two separate nvmem devices.
>
>  drivers/nvmem/qfprom.c | 314 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 303 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/nvmem/qfprom.c b/drivers/nvmem/qfprom.c
> index 8a91717600be..0a8576f2d4c6 100644
> --- a/drivers/nvmem/qfprom.c
> +++ b/drivers/nvmem/qfprom.c
> @@ -3,57 +3,349 @@
>   * Copyright (C) 2015 Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
>   */
>
> +#include <linux/clk.h>
>  #include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/mod_devicetable.h>
> -#include <linux/io.h>
>  #include <linux/nvmem-provider.h>
>  #include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +
> +/* Blow timer clock frequency in Mhz */
> +#define QFPROM_BLOW_TIMER_OFFSET 0x03c
> +
> +/* Amount of time required to hold charge to blow fuse in micro-seconds */
> +#define QFPROM_FUSE_BLOW_POLL_US       10
> +#define QFPROM_FUSE_BLOW_TIMEOUT_US    100

A quick follow up found that, in some cases, a timeout of 100 us
wasn't enough.  Changing the above to:

poll - 100 us
timeout - 1000 us

...fixed the problems.  I'm happy to:

* Spin the whole series to change those 2 numbers.

* Have those numbers changed by a maintainer when patches are applied.

* Sit tight and wait for additional feedback before taking action.

Given that I've resolved previous feedback, I've been assuming that
the series looks fine and we're sitting waiting for Rob Herring's
blessings on the bindings before landing.  Is that correct?

-Doug