Received-SPF: pass (google.com: domain of linux-kernel+bounces-2608-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Sun, 17 Dec 2023 13:24:47 +0000
From: Jonathan Cameron <jic23@kernel.org>
To: Joe Perches <joe@perches.com>
Cc: Justin Stitt <justinstitt@google.com>, Lars-Peter Clausen
 <lars@metafoo.de>, Stephen Boyd <swboyd@chromium.org>,
 linux-iio@vger.kernel.org, linux-kernel@vger.kernel.org,
 linux-hardening@vger.kernel.org
Subject: Re: [PATCH v3] iio: sx9324: avoid copying property strings
Message-ID: <20231217132447.269072df@jic23-huawei>
In-Reply-To: <f03d372a282712dee8412e47aff9bb54f181efd9.camel@perches.com>
References: <20231212-strncpy-drivers-iio-proximity-sx9324-c-v3-1-b8ae12fc8a5d@google.com>
	<f03d372a282712dee8412e47aff9bb54f181efd9.camel@perches.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Mon, 11 Dec 2023 22:30:12 -0800
Joe Perches <joe@perches.com> wrote:

> On Tue, 2023-12-12 at 00:42 +0000, Justin Stitt wrote:
> > We're doing some needless string copies when trying to assign the proper
> > `prop` string. We can make `prop` a const char* and simply assign to
> > string literals.  
> 
> trivia:
> 
> I would have updated it like this moving the
> various declarations into the case blocks
> where they are used and removing a few unused
> #defines

I'd definitely like to see those defines gone.
Arguably an unrelated change as I guess they are left from a previous refactor
of this code.

Why prop to type renaming?  It's getting passed into calls where the parameter
is propname so I'd understand renaming to that, but type just seems a bit random
to me.  I do wonder if we are better off having some long lines and getting rid
of the property naming local variables completely by just duplicating
the device_property_read_u32() call and passing them in directly.

Moving declarations more locally is a nice to have but I'll leave that up to Justin.

Anyhow, both solutions look much better than the original so I'm fine either way
(subject to responses to Stephen's review)

> 
> ---
>  drivers/iio/proximity/sx9324.c | 69 +++++++++++++++++++++++++-----------------
>  1 file changed, 41 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/iio/proximity/sx9324.c b/drivers/iio/proximity/sx9324.c
> index ac2ed2da21ccc..c50c1108a69cc 100644
> --- a/drivers/iio/proximity/sx9324.c
> +++ b/drivers/iio/proximity/sx9324.c
> @@ -877,17 +877,8 @@ static const struct sx_common_reg_default *
>  sx9324_get_default_reg(struct device *dev, int idx,
>  		       struct sx_common_reg_default *reg_def)
>  {
> -	static const char * const sx9324_rints[] = { "lowest", "low", "high",
> -		"highest" };
> -	static const char * const sx9324_csidle[] = { "hi-z", "hi-z", "gnd",
> -		"vdd" };
> -#define SX9324_PIN_DEF "semtech,ph0-pin"
> -#define SX9324_RESOLUTION_DEF "semtech,ph01-resolution"
> -#define SX9324_PROXRAW_DEF "semtech,ph01-proxraw-strength"
> -	unsigned int pin_defs[SX9324_NUM_PINS];
> -	char prop[] = SX9324_PROXRAW_DEF;
> -	u32 start = 0, raw = 0, pos = 0;
> -	int ret, count, ph, pin;
> +	u32 raw = 0;
> +	int ret;
>  
>  	memcpy(reg_def, &sx9324_default_regs[idx], sizeof(*reg_def));
>  
> @@ -896,7 +887,13 @@ sx9324_get_default_reg(struct device *dev, int idx,
>  	case SX9324_REG_AFE_PH0:
>  	case SX9324_REG_AFE_PH1:
>  	case SX9324_REG_AFE_PH2:
> -	case SX9324_REG_AFE_PH3:
> +	case SX9324_REG_AFE_PH3: {
> +		unsigned int pin_defs[SX9324_NUM_PINS];
> +		int count;
> +		int pin;
> +		int ph;
> +		char prop[32];
> +
>  		ph = reg_def->reg - SX9324_REG_AFE_PH0;
>  		snprintf(prop, ARRAY_SIZE(prop), "semtech,ph%d-pin", ph);
>  
> @@ -913,7 +910,15 @@ sx9324_get_default_reg(struct device *dev, int idx,
>  			       SX9324_REG_AFE_PH0_PIN_MASK(pin);
>  		reg_def->def = raw;
>  		break;
> -	case SX9324_REG_AFE_CTRL0:
> +	}
> +	case SX9324_REG_AFE_CTRL0: {
> +		static const char * const sx9324_csidle[] = {
> +			"hi-z", "hi-z", "gnd", "vdd"
> +		};
> +		static const char * const sx9324_rints[] = {
> +			"lowest", "low", "high", "highest"
> +		};
> +
>  		ret = device_property_match_property_string(dev, "semtech,cs-idle-sleep",
>  							    sx9324_csidle,
>  							    ARRAY_SIZE(sx9324_csidle));
> @@ -930,16 +935,17 @@ sx9324_get_default_reg(struct device *dev, int idx,
>  			reg_def->def |= ret << SX9324_REG_AFE_CTRL0_RINT_SHIFT;
>  		}
>  		break;
> +	}
>  	case SX9324_REG_AFE_CTRL4:
> -	case SX9324_REG_AFE_CTRL7:
> +	case SX9324_REG_AFE_CTRL7: {
> +		const char *type;
> +
>  		if (reg_def->reg == SX9324_REG_AFE_CTRL4)
> -			strncpy(prop, "semtech,ph01-resolution",
> -				ARRAY_SIZE(prop));
> +			type = "semtech,ph01-resolution";
>  		else
> -			strncpy(prop, "semtech,ph23-resolution",
> -				ARRAY_SIZE(prop));
> +			type = "semtech,ph23-resolution";
>  
> -		ret = device_property_read_u32(dev, prop, &raw);
> +		ret = device_property_read_u32(dev, type, &raw);
>  		if (ret)
>  			break;
>  
> @@ -949,6 +955,7 @@ sx9324_get_default_reg(struct device *dev, int idx,
>  		reg_def->def |= FIELD_PREP(SX9324_REG_AFE_CTRL4_RESOLUTION_MASK,
>  					   raw);
>  		break;
> +	}
>  	case SX9324_REG_AFE_CTRL8:
>  		ret = device_property_read_u32(dev,
>  				"semtech,input-precharge-resistor-ohms",
> @@ -982,17 +989,21 @@ sx9324_get_default_reg(struct device *dev, int idx,
>  					   6 + raw * (raw + 3) / 2);
>  		break;
>  
> -	case SX9324_REG_ADV_CTRL5:
> +	case SX9324_REG_ADV_CTRL5: {
> +		u32 start = 0;
> +
>  		ret = device_property_read_u32(dev, "semtech,startup-sensor",
>  					       &start);
>  		if (ret)
>  			break;
> -
>  		reg_def->def &= ~SX9324_REG_ADV_CTRL5_STARTUPSENS_MASK;
>  		reg_def->def |= FIELD_PREP(SX9324_REG_ADV_CTRL5_STARTUPSENS_MASK,
>  					   start);
>  		break;
> -	case SX9324_REG_PROX_CTRL4:
> +	}
> +	case SX9324_REG_PROX_CTRL4: {
> +		u32 pos = 0;
> +
>  		ret = device_property_read_u32(dev, "semtech,avg-pos-strength",
>  					       &pos);
>  		if (ret)
> @@ -1005,15 +1016,16 @@ sx9324_get_default_reg(struct device *dev, int idx,
>  		reg_def->def |= FIELD_PREP(SX9324_REG_PROX_CTRL4_AVGPOSFILT_MASK,
>  					   raw);
>  		break;
> +	}
>  	case SX9324_REG_PROX_CTRL0:
> -	case SX9324_REG_PROX_CTRL1:
> +	case SX9324_REG_PROX_CTRL1: {
> +		const char *type;
> +
>  		if (reg_def->reg == SX9324_REG_PROX_CTRL0)
> -			strncpy(prop, "semtech,ph01-proxraw-strength",
> -				ARRAY_SIZE(prop));
> +			type = "semtech,ph01-proxraw-strength";
>  		else
> -			strncpy(prop, "semtech,ph23-proxraw-strength",
> -				ARRAY_SIZE(prop));
> -		ret = device_property_read_u32(dev, prop, &raw);
> +			type = "semtech,ph23-proxraw-strength";
> +		ret = device_property_read_u32(dev, type, &raw);
>  		if (ret)
>  			break;
>  
> @@ -1022,6 +1034,7 @@ sx9324_get_default_reg(struct device *dev, int idx,
>  					   raw);
>  		break;
>  	}
> +	}
>  	return reg_def;
>  }
>  
> 
>