linux-kernel - RE: [PATCH V3 1/2] soc: imx: Add SCU SoC info driver 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]

Message-ID: <DB3PR0402MB3916C57857BC1064FD4295B5F50A0@DB3PR0402MB3916.eurprd04.prod.outlook.com>
Date:   Thu, 16 May 2019 11:09:29 +0000
From:   Anson Huang <anson.huang@....com>
To:     Leonard Crestez <leonard.crestez@....com>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>
CC:     "catalin.marinas@....com" <catalin.marinas@....com>,
        "will.deacon@....com" <will.deacon@....com>,
        "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>,
        "festevam@...il.com" <festevam@...il.com>,
        "agross@...nel.org" <agross@...nel.org>,
        "maxime.ripard@...tlin.com" <maxime.ripard@...tlin.com>,
        "olof@...om.net" <olof@...om.net>,
        "horms+renesas@...ge.net.au" <horms+renesas@...ge.net.au>,
        "jagan@...rulasolutions.com" <jagan@...rulasolutions.com>,
        "bjorn.andersson@...aro.org" <bjorn.andersson@...aro.org>,
        "marc.w.gonzalez@...e.fr" <marc.w.gonzalez@...e.fr>,
        "dinguyen@...nel.org" <dinguyen@...nel.org>,
        "enric.balletbo@...labora.com" <enric.balletbo@...labora.com>,
        "l.stach@...gutronix.de" <l.stach@...gutronix.de>,
        Aisheng Dong <aisheng.dong@....com>,
        Abel Vesa <abel.vesa@....com>,
        "robh@...nel.org" <robh@...nel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        dl-linux-imx <linux-imx@....com>
Subject: RE: [PATCH V3 1/2] soc: imx: Add SCU SoC info driver support

Hi, Leonard

> -----Original Message-----
> From: Leonard Crestez
> Sent: Thursday, May 16, 2019 6:07 PM
> To: Anson Huang <anson.huang@....com>; shawnguo@...nel.org
> Cc: catalin.marinas@....com; will.deacon@....com;
> s.hauer@...gutronix.de; kernel@...gutronix.de; festevam@...il.com;
> agross@...nel.org; maxime.ripard@...tlin.com; olof@...om.net;
> horms+renesas@...ge.net.au; jagan@...rulasolutions.com;
> bjorn.andersson@...aro.org; marc.w.gonzalez@...e.fr;
> dinguyen@...nel.org; enric.balletbo@...labora.com;
> l.stach@...gutronix.de; Aisheng Dong <aisheng.dong@....com>; Abel Vesa
> <abel.vesa@....com>; robh@...nel.org; linux-arm-
> kernel@...ts.infradead.org; linux-kernel@...r.kernel.org; dl-linux-imx
> <linux-imx@....com>
> Subject: Re: [PATCH V3 1/2] soc: imx: Add SCU SoC info driver support
> 
> On 16.05.2019 06:24, Anson Huang wrote:
> > Add i.MX SCU SoC info driver to support i.MX8QXP SoC, introduce driver
> > dependency into Kconfig as CONFIG_IMX_SCU must be selected to support
> > i.MX SCU SoC driver, also need to use platform driver model to make
> > sure IMX_SCU driver is probed before i.MX SCU SoC driver.
> 
> > +#define imx_scu_revision(soc_rev) \
> > +	soc_rev ? \
> > +	kasprintf(GFP_KERNEL, "%d.%d", (soc_rev >> 4) & 0xf,  soc_rev &
> 0xf) : \
> > +	"unknown"
> 
> > +	id = of_match_node(imx_scu_soc_match, pdev->dev.of_node);
> > +	data = id->data;
> > +	if (data) {
> > +		soc_dev_attr->soc_id = data->name;
> > +		if (data->soc_revision)
> > +			soc_rev = data->soc_revision();
> > +	}
> > +
> > +	soc_dev_attr->revision = imx_scu_revision(soc_rev);
> > +	if (!soc_dev_attr->revision)
> > +		return -ENODEV;
> 
> The imx_scu_revision macro returns either kasprintf or "unknown", never
> NULL. So it's not clear what this return -ENODEV does exactly.

The kasprintf could return NULL though.

> 
> It makes more sense to return -ENODEV if get_soc_revision fails, so maybe
> check "soc_rev != 0" instead?
> 
> If you really want to check the kasprintf result then you should return -
> ENOMEM for it. It would be clearer if you dropped the imx_scu_revision
> revision macro and open-coded instead.

This makes more sense, I think maybe we can remove the imx_scu_revision macro,
just use below code instead, and return -ENOMEM if kasprintf returns NULL.

113         soc_dev_attr->revision = soc_rev ? kasprintf(GFP_KERNEL,
114                                                      "%d.%d",
115                                                      (soc_rev >> 4) & 0xf,
116                                                      soc_rev & 0xf) : "unknown";
117         if (!soc_dev_attr->revision)
118                 return -ENOMEM;

BTW, the soc-imx8.c looks like also having same issue, do you think we should fix it
as well?

Anson

> 
> --
> Regards,
> Leonard