linux-kernel - Re: [PATCH v14 7/8] power: supply: Initial support for ROHM BD70528 PMIC charger block

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: <322fa765ddd72972aba931c706657661ca685afa.camel@fi.rohmeurope.com>
Date:   Fri, 3 May 2019 05:29:53 +0000
From:   "Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com>
To:     "sebastian.reichel@...labora.com" <sebastian.reichel@...labora.com>
CC:     "alexandre.belloni@...tlin.com" <alexandre.belloni@...tlin.com>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "mazziesaccount@...il.com" <mazziesaccount@...il.com>,
        "mturquette@...libre.com" <mturquette@...libre.com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
        "linus.walleij@...aro.org" <linus.walleij@...aro.org>,
        "sboyd@...nel.org" <sboyd@...nel.org>,
        "lee.jones@...aro.org" <lee.jones@...aro.org>,
        "a.zummo@...ertech.it" <a.zummo@...ertech.it>,
        "broonie@...nel.org" <broonie@...nel.org>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        "mark.rutland@....com" <mark.rutland@....com>,
        "linux-watchdog@...r.kernel.org" <linux-watchdog@...r.kernel.org>,
        "Mutanen, Mikko" <Mikko.Mutanen@...rohmeurope.com>,
        "linux@...ck-us.net" <linux@...ck-us.net>,
        "lgirdwood@...il.com" <lgirdwood@...il.com>,
        "bgolaszewski@...libre.com" <bgolaszewski@...libre.com>,
        "wim@...ux-watchdog.org" <wim@...ux-watchdog.org>,
        "linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>,
        "linux-rtc@...r.kernel.org" <linux-rtc@...r.kernel.org>,
        "Haikola, Heikki" <Heikki.Haikola@...rohmeurope.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v14 7/8] power: supply: Initial support for ROHM BD70528
 PMIC charger block

Hello Sebastian,

On Thu, 2019-05-02 at 21:50 +0200, Sebastian Reichel wrote:
> Hi,
> 
> On Thu, May 02, 2019 at 12:17:12PM +0300, Matti Vaittinen wrote:
> > ROHM BD70528 PMIC includes battery charger block. Support charger
> > staus queries and doing few basic settings like input current limit
> > and charging current.
> > 
> > Signed-off-by: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>
> > Acked-by: Sebastian Reichel <sebastian.reichel@...labora.com>
> 
> Please only add Acked-by when you receive one, especially when you
> do not implement all requested changes :)

Sorry. This is my bad. I translated your: "Otherwise looks ok to me."
- to an ack. And I was pretty sure you didn't want to staticize the
local array. I should've known better. I should have waited for your Ok
to leaving the static out before adding an ack.

> 
> > +static int bd70528_get_irqs(struct platform_device *pdev,
> > +			    struct bd70528_psy *bdpsy)
> > +{
> > +	int irq, i, ret;
> > +	unsigned int mask;
> > +	const struct irq_name_pair bd70528_chg_irqs[] = {
> > +		{ .n = "bd70528-bat-ov-res", .h =
> > BD_IRQ_HND(BAT_OV_RES) },
> > +		{ .n = "bd70528-bat-ov-det", .h =
> > BD_IRQ_HND(BAT_OV_DET) },
> > +		{ .n = "bd70528-bat-dead", .h = BD_IRQ_HND(DBAT_DET) },
> > +		{ .n = "bd70528-bat-warmed", .h = BD_IRQ_HND(COLD_RES)
> > },
> > +		{ .n = "bd70528-bat-cold", .h = BD_IRQ_HND(COLD_DET) },
> > +		{ .n = "bd70528-bat-cooled", .h = BD_IRQ_HND(HOT_RES)
> > },
> > +		{ .n = "bd70528-bat-hot", .h = BD_IRQ_HND(HOT_DET) },
> > +		{ .n = "bd70528-chg-tshd", .h = BD_IRQ_HND(CHG_TSD) },
> > +		{ .n = "bd70528-bat-removed", .h = BD_IRQ_HND(BAT_RMV)
> > },
> > +		{ .n = "bd70528-bat-detected", .h = BD_IRQ_HND(BAT_DET)
> > },
> > +		{ .n = "bd70528-dcin2-ov-res", .h =
> > BD_IRQ_HND(DCIN2_OV_RES) },
> > +		{ .n = "bd70528-dcin2-ov-det", .h =
> > BD_IRQ_HND(DCIN2_OV_DET) },
> > +		{ .n = "bd70528-dcin2-removed", .h =
> > BD_IRQ_HND(DCIN2_RMV) },
> > +		{ .n = "bd70528-dcin2-detected", .h =
> > BD_IRQ_HND(DCIN2_DET) },
> > +		{ .n = "bd70528-dcin1-removed", .h =
> > BD_IRQ_HND(DCIN1_RMV) },
> > +		{ .n = "bd70528-dcin1-detected", .h =
> > BD_IRQ_HND(DCIN1_DET) },
> > +	};
> 
> Please also make it static. That will move the whole thing to
> read-only (because of const) data section. This improves the
> security and the required cpu time at the same time (no need
> to copy values to the stack).

Ok. I see your point and it sounds reasonable.

> But this can be changed later, so no need to block the whole
> patchset just because of this. If Lee wants to merge this for
> 5.2, that would be fine with me. But please add it directly in
> a new patch revision if the patch does not make it into 5.2.

It would be great to get this in, it has been respinned quite a few
times already... I hope Lee takes the series in when he's back from his
leave - but it is not a problem for me to change this with later patch
(or change this in the series if it still needs to be reworked).


Br,
	Matti Vaittinen