| Message ID | 20220908131548.48120-3-jeff@labundy.com |
|---|---|
| State | New |
| Headers | show |
| Series | Additional fixes for Azoteq IQS7222A/B/C | expand |
On Thu, Sep 08, 2022 at 08:15:39AM -0500, Jeff LaBundy wrote: > Nonzero return values of several calls to fwnode_property_read_u32() > are silenty ignored, leaving no way to know that the properties were > not applied in the event of an error. > > To solve this problem, follow the design pattern used throughout the > rest of the driver by first checking if the property is present, and > then evaluating the return value of fwnode_property_read_u32(). > > Fixes: e505edaedcb9 ("Input: add support for Azoteq IQS7222A/B/C") > Signed-off-by: Jeff LaBundy <jeff@labundy.com> > --- > drivers/input/misc/iqs7222.c | 65 ++++++++++++++++++++++++++++++------ > 1 file changed, 55 insertions(+), 10 deletions(-) > > diff --git a/drivers/input/misc/iqs7222.c b/drivers/input/misc/iqs7222.c > index 04c1050d845c..fdade24caa8a 100644 > --- a/drivers/input/misc/iqs7222.c > +++ b/drivers/input/misc/iqs7222.c > @@ -1819,8 +1819,17 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, int chan_index) > 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)) { > + if (fwnode_property_present(chan_node, "azoteq,ref-weight")) { > + error = fwnode_property_read_u32(chan_node, > + "azoteq,ref-weight", > + &val); fwnode_property_read_u32() returns EINVAL if property is missing, so maybe have: error = fwnode_property_read_u32(chan_node, "azoteq,ref-weight", &val); if (!error) { ... } else { if (error != -EINVAL) { dev_err(&client->dev, "Failed to read %s reference weight: %d\n", fwnode_get_name(chan_node), error); goto put_chan_node; } } to avoid double calls into property handling code?
Hi Dmitry, On Thu, Sep 08, 2022 at 02:21:13PM -0700, Dmitry Torokhov wrote: > On Thu, Sep 08, 2022 at 08:15:39AM -0500, Jeff LaBundy wrote: > > Nonzero return values of several calls to fwnode_property_read_u32() > > are silenty ignored, leaving no way to know that the properties were > > not applied in the event of an error. > > > > To solve this problem, follow the design pattern used throughout the > > rest of the driver by first checking if the property is present, and > > then evaluating the return value of fwnode_property_read_u32(). > > > > Fixes: e505edaedcb9 ("Input: add support for Azoteq IQS7222A/B/C") > > Signed-off-by: Jeff LaBundy <jeff@labundy.com> > > --- > > drivers/input/misc/iqs7222.c | 65 ++++++++++++++++++++++++++++++------ > > 1 file changed, 55 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/input/misc/iqs7222.c b/drivers/input/misc/iqs7222.c > > index 04c1050d845c..fdade24caa8a 100644 > > --- a/drivers/input/misc/iqs7222.c > > +++ b/drivers/input/misc/iqs7222.c > > @@ -1819,8 +1819,17 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, int chan_index) > > 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)) { > > + if (fwnode_property_present(chan_node, "azoteq,ref-weight")) { > > + error = fwnode_property_read_u32(chan_node, > > + "azoteq,ref-weight", > > + &val); > > fwnode_property_read_u32() returns EINVAL if property is missing, so > maybe have: > > error = fwnode_property_read_u32(chan_node, > "azoteq,ref-weight", &val); > if (!error) { > ... > } else { > if (error != -EINVAL) { > dev_err(&client->dev, > "Failed to read %s reference weight: %d\n", > fwnode_get_name(chan_node), error); > goto put_chan_node; > } > } > > to avoid double calls into property handling code? That's a better idea; I can fold this into the helper functions proposed for the previous patch too. > > -- > Dmitry Kind regards, Jeff LaBundy
On Thu, Sep 08, 2022 at 09:08:09PM -0500, Jeff LaBundy wrote: > Hi Dmitry, > > On Thu, Sep 08, 2022 at 02:21:13PM -0700, Dmitry Torokhov wrote: > > On Thu, Sep 08, 2022 at 08:15:39AM -0500, Jeff LaBundy wrote: > > > Nonzero return values of several calls to fwnode_property_read_u32() > > > are silenty ignored, leaving no way to know that the properties were > > > not applied in the event of an error. > > > > > > To solve this problem, follow the design pattern used throughout the > > > rest of the driver by first checking if the property is present, and > > > then evaluating the return value of fwnode_property_read_u32(). > > > > > > Fixes: e505edaedcb9 ("Input: add support for Azoteq IQS7222A/B/C") > > > Signed-off-by: Jeff LaBundy <jeff@labundy.com> > > > --- > > > drivers/input/misc/iqs7222.c | 65 ++++++++++++++++++++++++++++++------ > > > 1 file changed, 55 insertions(+), 10 deletions(-) > > > > > > diff --git a/drivers/input/misc/iqs7222.c b/drivers/input/misc/iqs7222.c > > > index 04c1050d845c..fdade24caa8a 100644 > > > --- a/drivers/input/misc/iqs7222.c > > > +++ b/drivers/input/misc/iqs7222.c > > > @@ -1819,8 +1819,17 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, int chan_index) > > > 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)) { > > > + if (fwnode_property_present(chan_node, "azoteq,ref-weight")) { > > > + error = fwnode_property_read_u32(chan_node, > > > + "azoteq,ref-weight", > > > + &val); > > > > fwnode_property_read_u32() returns EINVAL if property is missing, so > > maybe have: > > > > error = fwnode_property_read_u32(chan_node, > > "azoteq,ref-weight", &val); > > if (!error) { > > ... > > } else { > > if (error != -EINVAL) { > > dev_err(&client->dev, > > "Failed to read %s reference weight: %d\n", > > fwnode_get_name(chan_node), error); > > goto put_chan_node; > > } > > } > > > > to avoid double calls into property handling code? > > That's a better idea; I can fold this into the helper functions proposed > for the previous patch too. We might be talking about different helpers. I had in mind: static int __iqs7222_parse_cycle(...) { ... } static int iqs7222_parse_cycle(...) { ... int retval = 0; error = iqs7222_parse_props(iqs7222, &cycle_node, cycle_index, IQS7222_REG_GRP_CYCLE, IQS7222_REG_KEY_NONE); if (error) return error; if (cycle_node) { retval = __iqs7222_parse_cycle(...); fwnode_put(cycle_node); } return retval; } so that we drop the node from one place. Thanks.
Hi Dmitry, On Thu, Sep 08, 2022 at 09:42:47PM -0700, Dmitry Torokhov wrote: > On Thu, Sep 08, 2022 at 09:08:09PM -0500, Jeff LaBundy wrote: > > Hi Dmitry, > > > > On Thu, Sep 08, 2022 at 02:21:13PM -0700, Dmitry Torokhov wrote: > > > On Thu, Sep 08, 2022 at 08:15:39AM -0500, Jeff LaBundy wrote: > > > > Nonzero return values of several calls to fwnode_property_read_u32() > > > > are silenty ignored, leaving no way to know that the properties were > > > > not applied in the event of an error. > > > > > > > > To solve this problem, follow the design pattern used throughout the > > > > rest of the driver by first checking if the property is present, and > > > > then evaluating the return value of fwnode_property_read_u32(). > > > > > > > > Fixes: e505edaedcb9 ("Input: add support for Azoteq IQS7222A/B/C") > > > > Signed-off-by: Jeff LaBundy <jeff@labundy.com> > > > > --- > > > > drivers/input/misc/iqs7222.c | 65 ++++++++++++++++++++++++++++++------ > > > > 1 file changed, 55 insertions(+), 10 deletions(-) > > > > > > > > diff --git a/drivers/input/misc/iqs7222.c b/drivers/input/misc/iqs7222.c > > > > index 04c1050d845c..fdade24caa8a 100644 > > > > --- a/drivers/input/misc/iqs7222.c > > > > +++ b/drivers/input/misc/iqs7222.c > > > > @@ -1819,8 +1819,17 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, int chan_index) > > > > 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)) { > > > > + if (fwnode_property_present(chan_node, "azoteq,ref-weight")) { > > > > + error = fwnode_property_read_u32(chan_node, > > > > + "azoteq,ref-weight", > > > > + &val); > > > > > > fwnode_property_read_u32() returns EINVAL if property is missing, so > > > maybe have: > > > > > > error = fwnode_property_read_u32(chan_node, > > > "azoteq,ref-weight", &val); > > > if (!error) { > > > ... > > > } else { > > > if (error != -EINVAL) { > > > dev_err(&client->dev, > > > "Failed to read %s reference weight: %d\n", > > > fwnode_get_name(chan_node), error); > > > goto put_chan_node; > > > } > > > } > > > > > > to avoid double calls into property handling code? > > > > That's a better idea; I can fold this into the helper functions proposed > > for the previous patch too. > > We might be talking about different helpers. I had in mind: > > static int __iqs7222_parse_cycle(...) > { > ... > } > > static int iqs7222_parse_cycle(...) > { > ... > int retval = 0; > > error = iqs7222_parse_props(iqs7222, &cycle_node, cycle_index, > IQS7222_REG_GRP_CYCLE, > IQS7222_REG_KEY_NONE); > if (error) > return error; > > if (cycle_node) { > retval = __iqs7222_parse_cycle(...); > fwnode_put(cycle_node); > } > > > return retval; > } > > so that we drop the node from one place. Right, originally I had imagined a wrapper around fwnode_property_read_u32() that calls fwnode_handle_put() in the error path. However, your proposal is even better. Thanks for the fruitful discussion; I'll clean all of this up for v2. > > Thanks. > > -- > Dmitry Kind regards, Jeff LaBundy
diff --git a/drivers/input/misc/iqs7222.c b/drivers/input/misc/iqs7222.c index 04c1050d845c..fdade24caa8a 100644 --- a/drivers/input/misc/iqs7222.c +++ b/drivers/input/misc/iqs7222.c @@ -1819,8 +1819,17 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, int chan_index) 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)) { + if (fwnode_property_present(chan_node, "azoteq,ref-weight")) { + error = fwnode_property_read_u32(chan_node, + "azoteq,ref-weight", + &val); + if (error) { + dev_err(&client->dev, + "Failed to read %s reference weight: %d\n", + fwnode_get_name(chan_node), error); + goto put_chan_node; + } + if (val > U16_MAX) { dev_err(&client->dev, "Invalid %s reference weight: %u\n", @@ -1921,9 +1930,8 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, int chan_index) goto put_chan_node; } - if (!fwnode_property_read_u32(event_node, - "azoteq,timeout-press-ms", - &val)) { + if (fwnode_property_present(event_node, + "azoteq,timeout-press-ms")) { /* * The IQS7222B employs a global pair of press timeout * registers as opposed to channel-specific registers. @@ -1933,6 +1941,17 @@ static int iqs7222_parse_chan(struct iqs7222_private *iqs7222, int chan_index) &iqs7222->btn_setup[chan_index][2] : &sys_setup[9]; + error = fwnode_property_read_u32(event_node, + "azoteq,timeout-press-ms", + &val); + if (error) { + dev_err(&client->dev, + "Failed to read %s press timeout: %d\n", + fwnode_get_name(chan_node), error); + fwnode_handle_put(event_node); + goto put_chan_node; + } + if (val > U8_MAX * 500) { dev_err(&client->dev, "Invalid %s press timeout: %u\n", @@ -2098,7 +2117,15 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222, int sldr_index) 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)) { + if (fwnode_property_present(sldr_node, "azoteq,slider-size")) { + error = fwnode_property_read_u32(sldr_node, + "azoteq,slider-size", &val); + if (error) { + dev_err(&client->dev, "Failed to read %s size: %d\n", + fwnode_get_name(sldr_node), error); + goto put_sldr_node; + } + if (!val || val > dev_desc->sldr_res) { dev_err(&client->dev, "Invalid %s size: %u\n", fwnode_get_name(sldr_node), val); @@ -2115,7 +2142,16 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222, int sldr_index) } } - if (!fwnode_property_read_u32(sldr_node, "azoteq,top-speed", &val)) { + if (fwnode_property_present(sldr_node, "azoteq,top-speed")) { + error = fwnode_property_read_u32(sldr_node, + "azoteq,top-speed", &val); + if (error) { + dev_err(&client->dev, + "Failed to read %s top speed: %d\n", + fwnode_get_name(sldr_node), error); + goto put_sldr_node; + } + 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); @@ -2131,10 +2167,19 @@ static int iqs7222_parse_sldr(struct iqs7222_private *iqs7222, int sldr_index) } } - if (!fwnode_property_read_u32(sldr_node, "linux,axis", &val)) { - u16 sldr_max = sldr_setup[3] - 1; + if (fwnode_property_present(sldr_node, "linux,axis")) { + u16 sldr_max; + + error = fwnode_property_read_u32(sldr_node, "linux,axis", &val); + if (error) { + dev_err(&client->dev, "Failed to read %s axis: %d\n", + fwnode_get_name(sldr_node), error); + goto put_sldr_node; + } - if (!reg_offset) { + if (reg_offset) { + sldr_max = sldr_setup[3] - 1; + } else { sldr_max = sldr_setup[2]; sldr_max &= IQS7222_SLDR_SETUP_2_RES_MASK;
Nonzero return values of several calls to fwnode_property_read_u32() are silenty ignored, leaving no way to know that the properties were not applied in the event of an error. To solve this problem, follow the design pattern used throughout the rest of the driver by first checking if the property is present, and then evaluating the return value of fwnode_property_read_u32(). Fixes: e505edaedcb9 ("Input: add support for Azoteq IQS7222A/B/C") Signed-off-by: Jeff LaBundy <jeff@labundy.com> --- drivers/input/misc/iqs7222.c | 65 ++++++++++++++++++++++++++++++------ 1 file changed, 55 insertions(+), 10 deletions(-)