Message ID | 20231212-strncpy-drivers-iio-proximity-sx9324-c-v3-1-b8ae12fc8a5d@google.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | [v3] iio: sx9324: avoid copying property strings | expand |
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 --- 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; }
On Tue, Dec 12, 2023 at 12:42:52AM +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. > > For the case where a format string is used, let's extract the parsing > logic out into sx9324_parse_phase_prop(). We no longer need to create > copies or allocate new memory. > > sx9324_parse_phase_prop() will simply return the default def value if it > fails. > > This also cleans up some deprecated strncpy() uses [1]. > > Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] > Link: https://github.com/KSPP/linux/issues/90 > Cc: linux-hardening@vger.kernel.org > Signed-off-by: Justin Stitt <justinstitt@google.com> Yeah, I like this approach. Totally removes the string copying and creates a helper that does the "heavy lifting". :) Reviewed-by: Kees Cook <keescook@chromium.org>
Quoting Justin Stitt (2023-12-11 16:42:52) > diff --git a/drivers/iio/proximity/sx9324.c b/drivers/iio/proximity/sx9324.c > index 438f9c9aba6e..e3bc30b57b19 100644 > --- a/drivers/iio/proximity/sx9324.c > +++ b/drivers/iio/proximity/sx9324.c > @@ -873,6 +873,32 @@ static int sx9324_init_compensation(struct iio_dev *indio_dev) > 20000, 2000000); > } > > +static u32 sx9324_parse_phase_prop(struct device *dev, return u8? because that's the type of struct sx_common_reg_default::def. > + struct sx_common_reg_default *reg_def, > + int idx, const char *prop) > +{ > + unsigned int pin_defs[SX9324_NUM_PINS]; > + int count, ret, pin; > + u8 default_def; > + u32 raw = 0; > + > + default_def = sx9324_default_regs[idx].def; Do we need to do this? Isn't this reg_def->def? > + > + count = device_property_count_u32(dev, prop); > + if (count != ARRAY_SIZE(pin_defs)) > + return default_def; return reg_def->def? > + ret = device_property_read_u32_array(dev, prop, pin_defs, > + ARRAY_SIZE(pin_defs)); > + if (ret) > + return default_def; return reg_def->def? > + > + for (pin = 0; pin < SX9324_NUM_PINS; pin++) > + raw |= (pin_defs[pin] << (2 * pin)) & > + SX9324_REG_AFE_PH0_PIN_MASK(pin); > + > + return raw; > +} > + > static const struct sx_common_reg_default * > sx9324_get_default_reg(struct device *dev, int idx, > struct sx_common_reg_default *reg_def) > @@ -884,35 +910,30 @@ sx9324_get_default_reg(struct device *dev, int idx, > #define SX9324_PIN_DEF "semtech,ph0-pin" > #define SX9324_RESOLUTION_DEF "semtech,ph01-resolution" > #define SX9324_PROXRAW_DEF "semtech,ph01-proxraw-strength" Can you send a followup to remove these defines? > - unsigned int pin_defs[SX9324_NUM_PINS]; > - char prop[] = SX9324_PROXRAW_DEF; > + const char *prop = SX9324_PROXRAW_DEF; This can be left unassigned now, right? > u32 start = 0, raw = 0, pos = 0; > - int ret, count, ph, pin; > const char *res; > + int ret;
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; > } > > >
On Sun, 2023-12-17 at 13:24 +0000, Jonathan Cameron wrote: > 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? random, no specific need, though I prefer not reusing identifiers with different types in separate local scopes. > 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. maybe, give it a try and see what you think.
diff --git a/drivers/iio/proximity/sx9324.c b/drivers/iio/proximity/sx9324.c index 438f9c9aba6e..e3bc30b57b19 100644 --- a/drivers/iio/proximity/sx9324.c +++ b/drivers/iio/proximity/sx9324.c @@ -873,6 +873,32 @@ static int sx9324_init_compensation(struct iio_dev *indio_dev) 20000, 2000000); } +static u32 sx9324_parse_phase_prop(struct device *dev, + struct sx_common_reg_default *reg_def, + int idx, const char *prop) +{ + unsigned int pin_defs[SX9324_NUM_PINS]; + int count, ret, pin; + u8 default_def; + u32 raw = 0; + + default_def = sx9324_default_regs[idx].def; + + count = device_property_count_u32(dev, prop); + if (count != ARRAY_SIZE(pin_defs)) + return default_def; + ret = device_property_read_u32_array(dev, prop, pin_defs, + ARRAY_SIZE(pin_defs)); + if (ret) + return default_def; + + for (pin = 0; pin < SX9324_NUM_PINS; pin++) + raw |= (pin_defs[pin] << (2 * pin)) & + SX9324_REG_AFE_PH0_PIN_MASK(pin); + + return raw; +} + static const struct sx_common_reg_default * sx9324_get_default_reg(struct device *dev, int idx, struct sx_common_reg_default *reg_def) @@ -884,35 +910,30 @@ sx9324_get_default_reg(struct device *dev, int idx, #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; + const char *prop = SX9324_PROXRAW_DEF; u32 start = 0, raw = 0, pos = 0; - int ret, count, ph, pin; const char *res; + int ret; memcpy(reg_def, &sx9324_default_regs[idx], sizeof(*reg_def)); sx_common_get_raw_register_config(dev, reg_def); switch (reg_def->reg) { case SX9324_REG_AFE_PH0: + reg_def->def = sx9324_parse_phase_prop(dev, reg_def, idx, + "semtech,ph0-pin"); + break; case SX9324_REG_AFE_PH1: + reg_def->def = sx9324_parse_phase_prop(dev, reg_def, idx, + "semtech,ph1-pin"); + break; case SX9324_REG_AFE_PH2: + reg_def->def = sx9324_parse_phase_prop(dev, reg_def, idx, + "semtech,ph2-pin"); + break; case SX9324_REG_AFE_PH3: - ph = reg_def->reg - SX9324_REG_AFE_PH0; - snprintf(prop, ARRAY_SIZE(prop), "semtech,ph%d-pin", ph); - - count = device_property_count_u32(dev, prop); - if (count != ARRAY_SIZE(pin_defs)) - break; - ret = device_property_read_u32_array(dev, prop, pin_defs, - ARRAY_SIZE(pin_defs)); - if (ret) - break; - - for (pin = 0; pin < SX9324_NUM_PINS; pin++) - raw |= (pin_defs[pin] << (2 * pin)) & - SX9324_REG_AFE_PH0_PIN_MASK(pin); - reg_def->def = raw; + reg_def->def = sx9324_parse_phase_prop(dev, reg_def, idx, + "semtech,ph3-pin"); break; case SX9324_REG_AFE_CTRL0: ret = device_property_read_string(dev, @@ -937,11 +958,9 @@ sx9324_get_default_reg(struct device *dev, int idx, case SX9324_REG_AFE_CTRL4: case SX9324_REG_AFE_CTRL7: if (reg_def->reg == SX9324_REG_AFE_CTRL4) - strncpy(prop, "semtech,ph01-resolution", - ARRAY_SIZE(prop)); + prop = "semtech,ph01-resolution"; else - strncpy(prop, "semtech,ph23-resolution", - ARRAY_SIZE(prop)); + prop = "semtech,ph23-resolution"; ret = device_property_read_u32(dev, prop, &raw); if (ret) @@ -1012,11 +1031,9 @@ sx9324_get_default_reg(struct device *dev, int idx, case SX9324_REG_PROX_CTRL0: case SX9324_REG_PROX_CTRL1: if (reg_def->reg == SX9324_REG_PROX_CTRL0) - strncpy(prop, "semtech,ph01-proxraw-strength", - ARRAY_SIZE(prop)); + prop = "semtech,ph01-proxraw-strength"; else - strncpy(prop, "semtech,ph23-proxraw-strength", - ARRAY_SIZE(prop)); + prop = "semtech,ph23-proxraw-strength"; ret = device_property_read_u32(dev, prop, &raw); if (ret) break;
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. For the case where a format string is used, let's extract the parsing logic out into sx9324_parse_phase_prop(). We no longer need to create copies or allocate new memory. sx9324_parse_phase_prop() will simply return the default def value if it fails. This also cleans up some deprecated strncpy() uses [1]. Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings [1] Link: https://github.com/KSPP/linux/issues/90 Cc: linux-hardening@vger.kernel.org Signed-off-by: Justin Stitt <justinstitt@google.com> --- Changes in v3: - extract logic into sx9324_parse_phase_prop() and use string literals (thanks Stephen) - rebase onto mainline bee0e7762ad2c602 - Link to v2: https://lore.kernel.org/r/20231026-strncpy-drivers-iio-proximity-sx9324-c-v2-1-cee6e5db700c@google.com Changes in v2: - make prop a const char* and do simple assignments (thanks Jonathan) - rebase onto 3a568e3a961ba330 - Link to v1: https://lore.kernel.org/r/20230921-strncpy-drivers-iio-proximity-sx9324-c-v1-1-4e8d28fd1e7c@google.com --- --- drivers/iio/proximity/sx9324.c | 69 ++++++++++++++++++++++++++---------------- 1 file changed, 43 insertions(+), 26 deletions(-) --- base-commit: bee0e7762ad2c6025b9f5245c040fcc36ef2bde8 change-id: 20230921-strncpy-drivers-iio-proximity-sx9324-c-8c3437676039 Best regards, -- Justin Stitt <justinstitt@google.com>