linux-kernel - Re: [PATCH v1 8/9] regulator: keembay: Add regulator for Keem Bay SoC

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:   Thu, 14 Jan 2021 17:14:34 +0000
From:   Mark Brown <broonie@...nel.org>
To:     Muhammad Husaini Zulkifli <muhammad.husaini.zulkifli@...el.com>
Cc:     ulf.hansson@...aro.org, lgirdwood@...il.com, robh+dt@...nel.org,
        devicetree@...r.kernel.org, adrian.hunter@...el.com,
        michal.simek@...inx.com, linux-mmc@...r.kernel.org,
        linux-kernel@...r.kernel.org, andriy.shevchenko@...el.com,
        Rashmi.A@...el.com, mahesh.r.vaidya@...el.com,
        Sudeep Holla <sudeep.holla@....com>
Subject: Re: [PATCH v1 8/9] regulator: keembay: Add regulator for Keem Bay SoC

On Thu, Jan 14, 2021 at 11:26:59PM +0800, Muhammad Husaini Zulkifli wrote:

> Keem Bay SD regulator driver module is added to encapsulate ARM
> Secure Monitor Call Calling Convention (SMCCC) during set voltage
> operations to control I/O Rail supplied voltage levels which communicate
> with Trusted Firmware.

Adding in Sudeep again for the SMCCC bits.  I just checked and am I
right in thinking this might already be shipping in production?

> @@ -0,0 +1,112 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Intel Keem Bay SD Regulator
> + *
> + * Copyright (C) 2020, Intel Corporation
> + * Author: Muhammad Husaini Zulkifli <Muhammad.Husaini.Zulkifli@...el.com>
> + */

Please make the entire comment a C++ comment to improve legibility.

> +static int keembay_regulator_set_voltage(struct regulator_dev *dev,
> +					int min_uV, int max_uV,
> +					unsigned *selector)
> +{
> +	int tmp_volt;
> +
> +	if (min_uV == KEEMBAY_IOV_1_8V_uV && max_uV == KEEMBAY_IOV_1_8V_uV)
> +		tmp_volt = KEEMBAY_SET_1V8_IO_RAIL;
> +	else
> +		tmp_volt = KEEMBAY_SET_3V3_IO_RAIL;
> +
> +	return keembay_set_io_rail_supplied_voltage(tmp_volt);
> +}

This has serious problems with input validation and is broken for most
valid input.  A set_voltage() function should set the voltage to the
lowest valid voltage within the range specified in the arguments and
return an error if it is not possible to set a voltage within the range
specified by the arguments.  This function will set 3.3V for any input
range other than exactly 1.8V so for example if the caller validly sets
a range of 1.7V-1.9V then 3.3V will be selected instead of 1.8V, or if
the user sets 1.0-1.1V then it will set 3.3V instead of returning an
error.

> +static int keembay_regulator_get_voltage(struct regulator_dev *dev)
> +{
> +	int ret;
> +
> +	ret = keembay_get_io_rail_supplied_voltage();
> +
> +	return ret ? KEEMBAY_IOV_1_8V_uV : KEEMBAY_IOV_3_3V_uV;
> +}

This ignores any errors or out of bounds values returned by the called
function, and please write normal conditional statements rather than
using the ternery operator to improve legibility.

> +/*
> + * Using subsys_initcall to ensure that Keem Bay regulator platform driver
> + * is initialized before device driver try to utilize it.
> + */
> +subsys_initcall(keembay_regulator_init);

There is no need to do this, probe deferral will ensure that
dependencies will be resolved.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)