linux-kernel - Re: [PATCH v6 1/6] nvmem: add driver for JZ4780 efuse

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: <B30BD902-CF5D-421A-9BD5-5FA038895712@goldelico.com>
Date:   Thu, 27 Feb 2020 16:26:23 +0100
From:   "H. Nikolaus Schaller" <hns@...delico.com>
To:     Paul Cercueil <paul@...pouillou.net>
Cc:     PrasannaKumar Muralidharan <prasannatsmkumar@...il.com>,
        Andreas Kemnade <andreas@...nade.info>,
        Mathieu Malaterre <malat@...ian.org>,
        Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Ralf Baechle <ralf@...ux-mips.org>,
        Paul Burton <paulburton@...nel.org>,
        Mauro Carvalho Chehab <mchehab+samsung@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Kees Cook <keescook@...omium.org>,
        Andi Kleen <ak@...ux.intel.com>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        linux-mips@...r.kernel.org, letux-kernel@...nphoenux.org,
        kernel@...a-handheld.com
Subject: Re: [PATCH v6 1/6] nvmem: add driver for JZ4780 efuse

Hi Paul,

> Am 27.02.2020 um 15:47 schrieb Paul Cercueil <paul@...pouillou.net>:
> 
> Hi Nikolaus,
> 
> 
> Le mer., févr. 26, 2020 at 12:15, H. Nikolaus Schaller <hns@...delico.com> a écrit :
>> From: PrasannaKumar Muralidharan <prasannatsmkumar@...il.com>
>> This patch brings support for the JZ4780 efuse. Currently it only exposes
>> a read only access to the entire 8K bits efuse memory and nvmem cells.
>> To fetch for example the MAC address:
>> dd if=/sys/devices/platform/134100d0.efuse/jz4780-efuse0/nvmem bs=1 skip=34 count=6 status=none | xxd
>> Tested-by: Mathieu Malaterre <malat@...ian.org>
>> Signed-off-by: PrasannaKumar Muralidharan <prasannatsmkumar@...il.com>
>> Signed-off-by: Mathieu Malaterre <malat@...ian.org>
>> Signed-off-by: H. Nikolaus Schaller <hns@...delico.com>
>> Signed-off-by: Paul Cercueil <paul@...pouillou.net>
>> ---
>> drivers/nvmem/Kconfig        |  12 ++
>> drivers/nvmem/Makefile       |   2 +
>> drivers/nvmem/jz4780-efuse.c | 239 +++++++++++++++++++++++++++++++++++
>> 3 files changed, 253 insertions(+)
>> create mode 100644 drivers/nvmem/jz4780-efuse.c
>> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
>> index 35efab1ba8d9..d7b7f6d688e7 100644
>> --- a/drivers/nvmem/Kconfig
>> +++ b/drivers/nvmem/Kconfig
>> @@ -55,6 +55,18 @@ config NVMEM_IMX_OCOTP_SCU


>> +	clk_rate = clk_get_rate(efuse->clk);
>> +
>> +	efuse->dev = dev;
>> +
>> +	/*
>> +	 * rd_adj and rd_strobe are 4 bit values
>> +	 * conditions:
>> +	 *   bus clk_period * (rd_adj + 1) > 6.5ns
>> +	 *   bus clk_period * (rd_adj + 5 + rd_strobe) > 35ns
>> +	 *   i.e. rd_adj >= 6.5ns / clk_period
>> +	 *   i.e. rd_strobe >= 35 ns / clk_period - 5 - rd_adj + 1
>> +	 * constants:
>> +	 *   1 / 6.5ns == 153846154 Hz
>> +	 *   1 / 35ns == 28571429 Hz
>> +	 */
>> +
>> +	rd_adj = clk_rate / 153846154;
>> +	rd_strobe = clk_rate / 28571429 - 5 - rd_adj + 1;
>> +
>> +	if (rd_adj > BIT(4) - 1 || rd_strobe > BIT(4) - 1) {
> 
> Just use 0xF or GENMASK(3, 0) instead of BIT(4) - 1

Or would rd_adj >= BIT(4) be better since this is not used as a mask?
This would also correspond to that the register is 4 bits wide.

> 
>> +		dev_err(&pdev->dev, "Cannot set clock configuration\n");
>> +		return -EINVAL;
> 
> You need to clk_disable_unprepare() before you return now, since the clock has been enabled.

Ah, ok! Forgot about that.

> I'd suggest registering a cleanup function (with devm_add_action_or_reset) after clk_prepare_enable() is called, then you don't need extra handling in the rest of the probe, and the .remove function (and platform_set_drvdata) can be dropped too.

Ok, this will indeed be simpler that the remove function if we take the error path into account.

Noted for v7.

> 
> The rest looks pretty good.
> 
> Cheers,
> -Paul

BR,
Nikolaus