| Message ID | da55b552-08ce-6bbe-c70b-eda6f53727f0@xs4all.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: i2c: adp1653: don't reuse the same node pointer | expand |
Hi Hans, Thanks for the patch. On Fri, Jun 09, 2023 at 11:43:14AM +0200, Hans Verkuil wrote: > The child device_node pointer was used for two different childs. > This confused smatch, causing this warning: > > drivers/media/i2c/adp1653.c:444 adp1653_of_init() warn: missing unwind goto? > > Use two different pointers, one for each child node, and add separate > goto labels for each node as well. This also improves error logging > since it will now state for which node the property was missing. It would have been better to fix smatch. :-( I guess changing the driver isn't wrong either still. > > Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > --- > diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c > index 98ca417b8004..04ec465aaa94 100644 > --- a/drivers/media/i2c/adp1653.c > +++ b/drivers/media/i2c/adp1653.c > @@ -411,43 +411,43 @@ static int adp1653_of_init(struct i2c_client *client, > struct device_node *node) > { > struct adp1653_platform_data *pd; > - struct device_node *child; > + struct device_node *node_flash, *node_indicator; > > pd = devm_kzalloc(&client->dev, sizeof(*pd), GFP_KERNEL); > if (!pd) > return -ENOMEM; > flash->platform_data = pd; > > - child = of_get_child_by_name(node, "flash"); > - if (!child) > + node_flash = of_get_child_by_name(node, "flash"); > + if (!node_flash) > return -EINVAL; > > - if (of_property_read_u32(child, "flash-timeout-us", > + if (of_property_read_u32(node_flash, "flash-timeout-us", > &pd->max_flash_timeout)) > - goto err; > + goto err_flash; > > - if (of_property_read_u32(child, "flash-max-microamp", > + if (of_property_read_u32(node_flash, "flash-max-microamp", > &pd->max_flash_intensity)) > - goto err; > + goto err_flash; > > pd->max_flash_intensity /= 1000; > > - if (of_property_read_u32(child, "led-max-microamp", > + if (of_property_read_u32(node_flash, "led-max-microamp", > &pd->max_torch_intensity)) > - goto err; > + goto err_flash; > > pd->max_torch_intensity /= 1000; > - of_node_put(child); > + of_node_put(node_flash); How about moving this to where the other node is put, and initialise the nodes to NULL and so continue having a single error label? > > - child = of_get_child_by_name(node, "indicator"); > - if (!child) > + node_indicator = of_get_child_by_name(node, "indicator"); > + if (!node_indicator) > return -EINVAL; > > - if (of_property_read_u32(child, "led-max-microamp", > + if (of_property_read_u32(node_indicator, "led-max-microamp", > &pd->max_indicator_intensity)) > - goto err; > + goto err_indicator; > > - of_node_put(child); > + of_node_put(node_indicator); > > pd->enable_gpio = devm_gpiod_get(&client->dev, "enable", GPIOD_OUT_LOW); > if (IS_ERR(pd->enable_gpio)) { > @@ -456,9 +456,13 @@ static int adp1653_of_init(struct i2c_client *client, > } > > return 0; > -err: > - dev_err(&client->dev, "Required property not found\n"); > - of_node_put(child); > +err_flash: > + dev_err(&client->dev, "Required flash property not found\n"); > + of_node_put(node_flash); > + return -EINVAL; > +err_indicator: > + dev_err(&client->dev, "Required indicator property not found\n"); > + of_node_put(node_indicator); > return -EINVAL; > } >
On 09/06/2023 11:47, Sakari Ailus wrote: > Hi Hans, > > Thanks for the patch. > > On Fri, Jun 09, 2023 at 11:43:14AM +0200, Hans Verkuil wrote: >> The child device_node pointer was used for two different childs. >> This confused smatch, causing this warning: >> >> drivers/media/i2c/adp1653.c:444 adp1653_of_init() warn: missing unwind goto? >> >> Use two different pointers, one for each child node, and add separate >> goto labels for each node as well. This also improves error logging >> since it will now state for which node the property was missing. > > It would have been better to fix smatch. :-( It was fixed for certain corner cases of this issue, but that didn't cover this particular driver. It's just not quite smart enough to follow the logic here since it uses the same pointer for two different device nodes. Regards, Hans > > I guess changing the driver isn't wrong either still. > >> >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> >> --- >> diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c >> index 98ca417b8004..04ec465aaa94 100644 >> --- a/drivers/media/i2c/adp1653.c >> +++ b/drivers/media/i2c/adp1653.c >> @@ -411,43 +411,43 @@ static int adp1653_of_init(struct i2c_client *client, >> struct device_node *node) >> { >> struct adp1653_platform_data *pd; >> - struct device_node *child; >> + struct device_node *node_flash, *node_indicator; >> >> pd = devm_kzalloc(&client->dev, sizeof(*pd), GFP_KERNEL); >> if (!pd) >> return -ENOMEM; >> flash->platform_data = pd; >> >> - child = of_get_child_by_name(node, "flash"); >> - if (!child) >> + node_flash = of_get_child_by_name(node, "flash"); >> + if (!node_flash) >> return -EINVAL; >> >> - if (of_property_read_u32(child, "flash-timeout-us", >> + if (of_property_read_u32(node_flash, "flash-timeout-us", >> &pd->max_flash_timeout)) >> - goto err; >> + goto err_flash; >> >> - if (of_property_read_u32(child, "flash-max-microamp", >> + if (of_property_read_u32(node_flash, "flash-max-microamp", >> &pd->max_flash_intensity)) >> - goto err; >> + goto err_flash; >> >> pd->max_flash_intensity /= 1000; >> >> - if (of_property_read_u32(child, "led-max-microamp", >> + if (of_property_read_u32(node_flash, "led-max-microamp", >> &pd->max_torch_intensity)) >> - goto err; >> + goto err_flash; >> >> pd->max_torch_intensity /= 1000; >> - of_node_put(child); >> + of_node_put(node_flash); > > How about moving this to where the other node is put, and initialise the > nodes to NULL and so continue having a single error label? > >> >> - child = of_get_child_by_name(node, "indicator"); >> - if (!child) >> + node_indicator = of_get_child_by_name(node, "indicator"); >> + if (!node_indicator) >> return -EINVAL; >> >> - if (of_property_read_u32(child, "led-max-microamp", >> + if (of_property_read_u32(node_indicator, "led-max-microamp", >> &pd->max_indicator_intensity)) >> - goto err; >> + goto err_indicator; >> >> - of_node_put(child); >> + of_node_put(node_indicator); >> >> pd->enable_gpio = devm_gpiod_get(&client->dev, "enable", GPIOD_OUT_LOW); >> if (IS_ERR(pd->enable_gpio)) { >> @@ -456,9 +456,13 @@ static int adp1653_of_init(struct i2c_client *client, >> } >> >> return 0; >> -err: >> - dev_err(&client->dev, "Required property not found\n"); >> - of_node_put(child); >> +err_flash: >> + dev_err(&client->dev, "Required flash property not found\n"); >> + of_node_put(node_flash); >> + return -EINVAL; >> +err_indicator: >> + dev_err(&client->dev, "Required indicator property not found\n"); >> + of_node_put(node_indicator); >> return -EINVAL; >> } >> >
diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c index 98ca417b8004..04ec465aaa94 100644 --- a/drivers/media/i2c/adp1653.c +++ b/drivers/media/i2c/adp1653.c @@ -411,43 +411,43 @@ static int adp1653_of_init(struct i2c_client *client, struct device_node *node) { struct adp1653_platform_data *pd; - struct device_node *child; + struct device_node *node_flash, *node_indicator; pd = devm_kzalloc(&client->dev, sizeof(*pd), GFP_KERNEL); if (!pd) return -ENOMEM; flash->platform_data = pd; - child = of_get_child_by_name(node, "flash"); - if (!child) + node_flash = of_get_child_by_name(node, "flash"); + if (!node_flash) return -EINVAL; - if (of_property_read_u32(child, "flash-timeout-us", + if (of_property_read_u32(node_flash, "flash-timeout-us", &pd->max_flash_timeout)) - goto err; + goto err_flash; - if (of_property_read_u32(child, "flash-max-microamp", + if (of_property_read_u32(node_flash, "flash-max-microamp", &pd->max_flash_intensity)) - goto err; + goto err_flash; pd->max_flash_intensity /= 1000; - if (of_property_read_u32(child, "led-max-microamp", + if (of_property_read_u32(node_flash, "led-max-microamp", &pd->max_torch_intensity)) - goto err; + goto err_flash; pd->max_torch_intensity /= 1000; - of_node_put(child); + of_node_put(node_flash); - child = of_get_child_by_name(node, "indicator"); - if (!child) + node_indicator = of_get_child_by_name(node, "indicator"); + if (!node_indicator) return -EINVAL; - if (of_property_read_u32(child, "led-max-microamp", + if (of_property_read_u32(node_indicator, "led-max-microamp", &pd->max_indicator_intensity)) - goto err; + goto err_indicator; - of_node_put(child); + of_node_put(node_indicator); pd->enable_gpio = devm_gpiod_get(&client->dev, "enable", GPIOD_OUT_LOW); if (IS_ERR(pd->enable_gpio)) { @@ -456,9 +456,13 @@ static int adp1653_of_init(struct i2c_client *client, } return 0; -err: - dev_err(&client->dev, "Required property not found\n"); - of_node_put(child); +err_flash: + dev_err(&client->dev, "Required flash property not found\n"); + of_node_put(node_flash); + return -EINVAL; +err_indicator: + dev_err(&client->dev, "Required indicator property not found\n"); + of_node_put(node_indicator); return -EINVAL; }
The child device_node pointer was used for two different childs. This confused smatch, causing this warning: drivers/media/i2c/adp1653.c:444 adp1653_of_init() warn: missing unwind goto? Use two different pointers, one for each child node, and add separate goto labels for each node as well. This also improves error logging since it will now state for which node the property was missing. Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> ---