| Message ID | YyP8GaYqdwVUNzPe@nixie71 |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Additional fixes for Azoteq IQS7222A/B/C | expand |
On Thu, Sep 15, 2022 at 23:31, Jeff LaBundy <jeff@labundy.com> wrote: > Nonzero return values of several calls to fwnode_property_read_u32() > are silently ignored, leaving no way to know the properties were not > applied in the event of an error. > > Solve this problem by evaluating fwnode_property_read_u32()'s return > value, and reporting an error for any nonzero return value not equal > to -EINVAL which indicates the property was absent altogether. > > Fixes: e505edaedcb9 ("Input: add support for Azoteq IQS7222A/B/C") > Signed-off-by: Jeff LaBundy <jeff@labundy.com> > --- > Changes in v2: > - Used -EINVAL returned by fwnode_property_read_u32() to indicate an absent > optional property as opposed to calling fwnode_property_present() > - Updated commit message > > drivers/input/misc/iqs7222.c | 43 +++++++++++++++++++++++++++++------- > 1 file changed, 35 insertions(+), 8 deletions(-) > > diff --git a/drivers/input/misc/iqs7222.c b/drivers/input/misc/iqs7222.c > index d39b3fdfb849..36c3b24e99a3 100644 > --- a/drivers/input/misc/iqs7222.c > +++ b/drivers/input/misc/iqs7222.c > @@ -1820,8 +1820,9 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, > chan_setup[0] |= IQS7222_CHAN_SETUP_0_REF_MODE_FOLLOW; > chan_setup[4] = val * 42 + 1048; > > - if (!fwnode_property_read_u32(chan_node, "azoteq,ref-weight", > - &val)) { > + error = fwnode_property_read_u32(chan_node, "azoteq,ref-weight", > + &val); > + if (!error) { > if (val > U16_MAX) { > dev_err(&client->dev, > "Invalid %s reference weight: %u\n", > @@ -1830,6 +1831,11 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, > } > > chan_setup[5] = val; > + } else if (error != -EINVAL) { > + dev_err(&client->dev, > + "Failed to read %s reference weight: %d\n", > + fwnode_get_name(chan_node), error); > + return error; > } > > /* > @@ -1902,9 +1908,10 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, > if (!event_node) > continue; > > - if (!fwnode_property_read_u32(event_node, > - "azoteq,timeout-press-ms", > - &val)) { > + error = fwnode_property_read_u32(event_node, > + "azoteq,timeout-press-ms", > + &val); > + if (!error) { > /* > * The IQS7222B employs a global pair of press timeout > * registers as opposed to channel-specific registers. > @@ -1924,6 +1931,11 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, > > *setup &= ~(U8_MAX << i * 8); > *setup |= (val / 500 << i * 8); > + } else if (error != -EINVAL) { > + dev_err(&client->dev, > + "Failed to read %s press timeout: %d\n", > + fwnode_get_name(event_node), error); Shouldn't we call fwnode_handle_put(event_node); here? It's what we do in the error path just above (line 2029) With that added, feel free to include: Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> > + return error; > } > > error = iqs7222_parse_event(iqs7222, event_node, chan_index, > @@ -2028,7 +2040,8 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222, > if (fwnode_property_present(sldr_node, "azoteq,use-prox")) > sldr_setup[4 + reg_offset] -= 2; > > - if (!fwnode_property_read_u32(sldr_node, "azoteq,slider-size", &val)) { > + error = fwnode_property_read_u32(sldr_node, "azoteq,slider-size", &val); > + if (!error) { > if (!val || val > dev_desc->sldr_res) { > dev_err(&client->dev, "Invalid %s size: %u\n", > fwnode_get_name(sldr_node), val); > @@ -2042,9 +2055,14 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222, > sldr_setup[2] |= (val / 16 << > IQS7222_SLDR_SETUP_2_RES_SHIFT); > } > + } else if (error != -EINVAL) { > + dev_err(&client->dev, "Failed to read %s size: %d\n", > + fwnode_get_name(sldr_node), error); > + return error; > } > > - if (!fwnode_property_read_u32(sldr_node, "azoteq,top-speed", &val)) { > + error = fwnode_property_read_u32(sldr_node, "azoteq,top-speed", &val); > + if (!error) { > if (val > (reg_offset ? U16_MAX : U8_MAX * 4)) { > dev_err(&client->dev, "Invalid %s top speed: %u\n", > fwnode_get_name(sldr_node), val); > @@ -2057,9 +2075,14 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222, > sldr_setup[2] &= ~IQS7222_SLDR_SETUP_2_TOP_SPEED_MASK; > sldr_setup[2] |= (val / 4); > } > + } else if (error != -EINVAL) { > + dev_err(&client->dev, "Failed to read %s top speed: %d\n", > + fwnode_get_name(sldr_node), error); > + return error; > } > > - if (!fwnode_property_read_u32(sldr_node, "linux,axis", &val)) { > + error = fwnode_property_read_u32(sldr_node, "linux,axis", &val); > + if (!error) { > u16 sldr_max = sldr_setup[3] - 1; > > if (!reg_offset) { > @@ -2073,6 +2096,10 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222, > > input_set_abs_params(iqs7222->keypad, val, 0, sldr_max, 0, 0); > iqs7222->sl_axis[sldr_index] = val; > + } else if (error != -EINVAL) { > + dev_err(&client->dev, "Failed to read %s axis: %d\n", > + fwnode_get_name(sldr_node), error); > + return error; > } > > if (dev_desc->wheel_enable) { > -- > 2.34.1
Hi Mattijs, On Fri, Oct 07, 2022 at 03:12:17PM +0200, Mattijs Korpershoek wrote: > On Thu, Sep 15, 2022 at 23:31, Jeff LaBundy <jeff@labundy.com> wrote: > > > Nonzero return values of several calls to fwnode_property_read_u32() > > are silently ignored, leaving no way to know the properties were not > > applied in the event of an error. > > > > Solve this problem by evaluating fwnode_property_read_u32()'s return > > value, and reporting an error for any nonzero return value not equal > > to -EINVAL which indicates the property was absent altogether. > > > > Fixes: e505edaedcb9 ("Input: add support for Azoteq IQS7222A/B/C") > > Signed-off-by: Jeff LaBundy <jeff@labundy.com> > > --- > > Changes in v2: > > - Used -EINVAL returned by fwnode_property_read_u32() to indicate an absent > > optional property as opposed to calling fwnode_property_present() > > - Updated commit message > > > > drivers/input/misc/iqs7222.c | 43 +++++++++++++++++++++++++++++------- > > 1 file changed, 35 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/input/misc/iqs7222.c b/drivers/input/misc/iqs7222.c > > index d39b3fdfb849..36c3b24e99a3 100644 > > --- a/drivers/input/misc/iqs7222.c > > +++ b/drivers/input/misc/iqs7222.c > > @@ -1820,8 +1820,9 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, > > chan_setup[0] |= IQS7222_CHAN_SETUP_0_REF_MODE_FOLLOW; > > chan_setup[4] = val * 42 + 1048; > > > > - if (!fwnode_property_read_u32(chan_node, "azoteq,ref-weight", > > - &val)) { > > + error = fwnode_property_read_u32(chan_node, "azoteq,ref-weight", > > + &val); > > + if (!error) { > > if (val > U16_MAX) { > > dev_err(&client->dev, > > "Invalid %s reference weight: %u\n", > > @@ -1830,6 +1831,11 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, > > } > > > > chan_setup[5] = val; > > + } else if (error != -EINVAL) { > > + dev_err(&client->dev, > > + "Failed to read %s reference weight: %d\n", > > + fwnode_get_name(chan_node), error); > > + return error; > > } > > > > /* > > @@ -1902,9 +1908,10 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, > > if (!event_node) > > continue; > > > > - if (!fwnode_property_read_u32(event_node, > > - "azoteq,timeout-press-ms", > > - &val)) { > > + error = fwnode_property_read_u32(event_node, > > + "azoteq,timeout-press-ms", > > + &val); > > + if (!error) { > > /* > > * The IQS7222B employs a global pair of press timeout > > * registers as opposed to channel-specific registers. > > @@ -1924,6 +1931,11 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, > > > > *setup &= ~(U8_MAX << i * 8); > > *setup |= (val / 500 << i * 8); > > + } else if (error != -EINVAL) { > > + dev_err(&client->dev, > > + "Failed to read %s press timeout: %d\n", > > + fwnode_get_name(event_node), error); > > Shouldn't we call fwnode_handle_put(event_node); here? > It's what we do in the error path just above (line 2029) Thank you for your review and for finding this; it's a great catch. I will send out a v3 after the merge window with this fixed. > > With that added, feel free to include: > > Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> > > > + return error; > > } > > > > error = iqs7222_parse_event(iqs7222, event_node, chan_index, > > @@ -2028,7 +2040,8 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222, > > if (fwnode_property_present(sldr_node, "azoteq,use-prox")) > > sldr_setup[4 + reg_offset] -= 2; > > > > - if (!fwnode_property_read_u32(sldr_node, "azoteq,slider-size", &val)) { > > + error = fwnode_property_read_u32(sldr_node, "azoteq,slider-size", &val); > > + if (!error) { > > if (!val || val > dev_desc->sldr_res) { > > dev_err(&client->dev, "Invalid %s size: %u\n", > > fwnode_get_name(sldr_node), val); > > @@ -2042,9 +2055,14 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222, > > sldr_setup[2] |= (val / 16 << > > IQS7222_SLDR_SETUP_2_RES_SHIFT); > > } > > + } else if (error != -EINVAL) { > > + dev_err(&client->dev, "Failed to read %s size: %d\n", > > + fwnode_get_name(sldr_node), error); > > + return error; > > } > > > > - if (!fwnode_property_read_u32(sldr_node, "azoteq,top-speed", &val)) { > > + error = fwnode_property_read_u32(sldr_node, "azoteq,top-speed", &val); > > + if (!error) { > > if (val > (reg_offset ? U16_MAX : U8_MAX * 4)) { > > dev_err(&client->dev, "Invalid %s top speed: %u\n", > > fwnode_get_name(sldr_node), val); > > @@ -2057,9 +2075,14 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222, > > sldr_setup[2] &= ~IQS7222_SLDR_SETUP_2_TOP_SPEED_MASK; > > sldr_setup[2] |= (val / 4); > > } > > + } else if (error != -EINVAL) { > > + dev_err(&client->dev, "Failed to read %s top speed: %d\n", > > + fwnode_get_name(sldr_node), error); > > + return error; > > } > > > > - if (!fwnode_property_read_u32(sldr_node, "linux,axis", &val)) { > > + error = fwnode_property_read_u32(sldr_node, "linux,axis", &val); > > + if (!error) { > > u16 sldr_max = sldr_setup[3] - 1; > > > > if (!reg_offset) { > > @@ -2073,6 +2096,10 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222, > > > > input_set_abs_params(iqs7222->keypad, val, 0, sldr_max, 0, 0); > > iqs7222->sl_axis[sldr_index] = val; > > + } else if (error != -EINVAL) { > > + dev_err(&client->dev, "Failed to read %s axis: %d\n", > > + fwnode_get_name(sldr_node), error); > > + return error; > > } > > > > if (dev_desc->wheel_enable) { > > -- > > 2.34.1 Kind regards, Jeff LaBundy
diff --git a/drivers/input/misc/iqs7222.c b/drivers/input/misc/iqs7222.c index d39b3fdfb849..36c3b24e99a3 100644 --- a/drivers/input/misc/iqs7222.c +++ b/drivers/input/misc/iqs7222.c @@ -1820,8 +1820,9 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, chan_setup[0] |= IQS7222_CHAN_SETUP_0_REF_MODE_FOLLOW; chan_setup[4] = val * 42 + 1048; - if (!fwnode_property_read_u32(chan_node, "azoteq,ref-weight", - &val)) { + error = fwnode_property_read_u32(chan_node, "azoteq,ref-weight", + &val); + if (!error) { if (val > U16_MAX) { dev_err(&client->dev, "Invalid %s reference weight: %u\n", @@ -1830,6 +1831,11 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, } chan_setup[5] = val; + } else if (error != -EINVAL) { + dev_err(&client->dev, + "Failed to read %s reference weight: %d\n", + fwnode_get_name(chan_node), error); + return error; } /* @@ -1902,9 +1908,10 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, if (!event_node) continue; - if (!fwnode_property_read_u32(event_node, - "azoteq,timeout-press-ms", - &val)) { + error = fwnode_property_read_u32(event_node, + "azoteq,timeout-press-ms", + &val); + if (!error) { /* * The IQS7222B employs a global pair of press timeout * registers as opposed to channel-specific registers. @@ -1924,6 +1931,11 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, *setup &= ~(U8_MAX << i * 8); *setup |= (val / 500 << i * 8); + } else if (error != -EINVAL) { + dev_err(&client->dev, + "Failed to read %s press timeout: %d\n", + fwnode_get_name(event_node), error); + return error; } error = iqs7222_parse_event(iqs7222, event_node, chan_index, @@ -2028,7 +2040,8 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222, if (fwnode_property_present(sldr_node, "azoteq,use-prox")) sldr_setup[4 + reg_offset] -= 2; - if (!fwnode_property_read_u32(sldr_node, "azoteq,slider-size", &val)) { + error = fwnode_property_read_u32(sldr_node, "azoteq,slider-size", &val); + if (!error) { if (!val || val > dev_desc->sldr_res) { dev_err(&client->dev, "Invalid %s size: %u\n", fwnode_get_name(sldr_node), val); @@ -2042,9 +2055,14 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222, sldr_setup[2] |= (val / 16 << IQS7222_SLDR_SETUP_2_RES_SHIFT); } + } else if (error != -EINVAL) { + dev_err(&client->dev, "Failed to read %s size: %d\n", + fwnode_get_name(sldr_node), error); + return error; } - if (!fwnode_property_read_u32(sldr_node, "azoteq,top-speed", &val)) { + error = fwnode_property_read_u32(sldr_node, "azoteq,top-speed", &val); + if (!error) { if (val > (reg_offset ? U16_MAX : U8_MAX * 4)) { dev_err(&client->dev, "Invalid %s top speed: %u\n", fwnode_get_name(sldr_node), val); @@ -2057,9 +2075,14 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222, sldr_setup[2] &= ~IQS7222_SLDR_SETUP_2_TOP_SPEED_MASK; sldr_setup[2] |= (val / 4); } + } else if (error != -EINVAL) { + dev_err(&client->dev, "Failed to read %s top speed: %d\n", + fwnode_get_name(sldr_node), error); + return error; } - if (!fwnode_property_read_u32(sldr_node, "linux,axis", &val)) { + error = fwnode_property_read_u32(sldr_node, "linux,axis", &val); + if (!error) { u16 sldr_max = sldr_setup[3] - 1; if (!reg_offset) { @@ -2073,6 +2096,10 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222, input_set_abs_params(iqs7222->keypad, val, 0, sldr_max, 0, 0); iqs7222->sl_axis[sldr_index] = val; + } else if (error != -EINVAL) { + dev_err(&client->dev, "Failed to read %s axis: %d\n", + fwnode_get_name(sldr_node), error); + return error; } if (dev_desc->wheel_enable) {
Nonzero return values of several calls to fwnode_property_read_u32() are silently ignored, leaving no way to know the properties were not applied in the event of an error. Solve this problem by evaluating fwnode_property_read_u32()'s return value, and reporting an error for any nonzero return value not equal to -EINVAL which indicates the property was absent altogether. Fixes: e505edaedcb9 ("Input: add support for Azoteq IQS7222A/B/C") Signed-off-by: Jeff LaBundy <jeff@labundy.com> --- Changes in v2: - Used -EINVAL returned by fwnode_property_read_u32() to indicate an absent optional property as opposed to calling fwnode_property_present() - Updated commit message drivers/input/misc/iqs7222.c | 43 +++++++++++++++++++++++++++++------- 1 file changed, 35 insertions(+), 8 deletions(-)