linux-kernel - Re: [PATCH v6] rtc: isl12026: Add driver.

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] [day] [month] [year] [list]

Date:   Wed, 28 Feb 2018 16:17:01 +0100
From:   Alexandre Belloni <alexandre.belloni@...e-electrons.com>
To:     David Daney <david.daney@...ium.com>
Cc:     Alessandro Zummo <a.zummo@...ertech.it>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>, linux-rtc@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        Andy Shevchenko <andy.shevchenko@...il.com>
Subject: Re: [PATCH v6] rtc: isl12026: Add driver.

Hi,

That's mostly good

On 22/02/2018 at 12:04:32 -0800, David Daney wrote:
> +	priv->rtc->ops = &isl12026_rtc_ops;
> +	priv->rtc->nvram_old_abi = false;

This allocation is not necessary and I would refer not having t so when
the ABI goes away, it is not necessary to change this driver.

> +	ret = rtc_register_device(priv->rtc);
> +	if (ret)
> +		return ret;
> +
> +	memset(&nvm_cfg, 0, sizeof(nvm_cfg));
> +	nvm_cfg.name = "eeprom";

You probably need something more descriptive, usually the rtc model name
else, it is difficult (but not impossible) to find where this nvmem is
actually located when using /sys/bus/nvmem/devices
isl12026- would be a good choice.

> +	nvm_cfg.read_only = false;
> +	nvm_cfg.root_only = true;

any reason to have it root only?

> +	nvm_cfg.base_dev = &client->dev;
> +	nvm_cfg.priv = priv;
> +	nvm_cfg.stride = 1;
> +	nvm_cfg.word_size = 1;
> +	nvm_cfg.size = 512;
> +	nvm_cfg.reg_read = isl12026_nvm_read;
> +	nvm_cfg.reg_write = isl12026_nvm_write;
> +
> +	return rtc_nvmem_register(priv->rtc, &nvm_cfg);

The probe function must not fail after rtc_register_device has been
called so you must return 0 here.

It is not currently possible to call rtc_nvmem_register before
rtc_register_device unless we move the nvmem to the parent device:

diff --git a/drivers/rtc/nvmem.c b/drivers/rtc/nvmem.c
index 3a02357eb783..17ec4c8d0fad 100644
--- a/drivers/rtc/nvmem.c
+++ b/drivers/rtc/nvmem.c
@@ -91,7 +91,7 @@ int rtc_nvmem_register(struct rtc_device *rtc,
        if (!nvmem_config)
                return -ENODEV;

-       nvmem_config->dev = &rtc->dev;
+       nvmem_config->dev = rtc->dev.parent;
        nvmem_config->owner = rtc->owner;
        rtc->nvmem = nvmem_register(nvmem_config);
        if (IS_ERR_OR_NULL(rtc->nvmem))

I'll submit this patch for this cycle before there are many users of the
interface (converted drivers are still exposing the old ABI).

If you prefer, you can depend on it.

-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com