* [bug report] mfd: lp87565: Handle optional reset pin
@ 2021-11-17 11:17 Dan Carpenter
2021-11-18 8:38 ` Lee Jones
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2021-11-17 11:17 UTC (permalink / raw)
To: luca; +Cc: Lee Jones, kernel-janitors
Hello Luca Ceresoli,
The patch 50e4d7a2a667: "mfd: lp87565: Handle optional reset pin"
from Feb 26, 2021, leads to the following Smatch static checker
warning:
drivers/mfd/lp87565.c:76 lp87565_probe()
warn: 'lp87565->reset_gpio' could be an error pointer
drivers/mfd/lp87565.c
46 static int lp87565_probe(struct i2c_client *client,
47 const struct i2c_device_id *ids)
48 {
49 struct lp87565 *lp87565;
50 const struct of_device_id *of_id;
51 int ret;
52 unsigned int otpid;
53
54 lp87565 = devm_kzalloc(&client->dev, sizeof(*lp87565), GFP_KERNEL);
55 if (!lp87565)
56 return -ENOMEM;
57
58 lp87565->dev = &client->dev;
59
60 lp87565->regmap = devm_regmap_init_i2c(client, &lp87565_regmap_config);
61 if (IS_ERR(lp87565->regmap)) {
62 ret = PTR_ERR(lp87565->regmap);
63 dev_err(lp87565->dev,
64 "Failed to initialize register map: %d\n", ret);
65 return ret;
66 }
67
68 lp87565->reset_gpio = devm_gpiod_get_optional(lp87565->dev, "reset",
69 GPIOD_OUT_LOW);
70 if (IS_ERR(lp87565->reset_gpio)) {
71 ret = PTR_ERR(lp87565->reset_gpio);
72 if (ret == -EPROBE_DEFER)
73 return ret;
Only "ret = -EPROBE_DEFER" is handled. Other error pointer will lead to
a crash.
74 }
75
--> 76 if (lp87565->reset_gpio) {
77 gpiod_set_value_cansleep(lp87565->reset_gpio, 1);
78 /* The minimum assertion time is undocumented, just guess */
79 usleep_range(2000, 4000);
80
81 gpiod_set_value_cansleep(lp87565->reset_gpio, 0);
82 /* Min 1.2 ms before first I2C transaction */
83 usleep_range(1500, 3000);
84 }
85
86 ret = regmap_read(lp87565->regmap, LP87565_REG_OTP_REV, &otpid);
87 if (ret) {
88 dev_err(lp87565->dev, "Failed to read OTP ID\n");
89 return ret;
90 }
91
92 lp87565->rev = otpid & LP87565_OTP_REV_OTP_ID;
93
94 of_id = of_match_device(of_lp87565_match_table, &client->dev);
95 if (of_id)
96 lp87565->dev_type = (enum lp87565_device_type)of_id->data;
97
98 i2c_set_clientdata(client, lp87565);
99
100 return devm_mfd_add_devices(lp87565->dev, PLATFORM_DEVID_AUTO,
101 lp87565_cells, ARRAY_SIZE(lp87565_cells),
102 NULL, 0, NULL);
103 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] mfd: lp87565: Handle optional reset pin
2021-11-17 11:17 [bug report] mfd: lp87565: Handle optional reset pin Dan Carpenter
@ 2021-11-18 8:38 ` Lee Jones
2021-11-18 8:47 ` Dan Carpenter
0 siblings, 1 reply; 7+ messages in thread
From: Lee Jones @ 2021-11-18 8:38 UTC (permalink / raw)
To: Dan Carpenter; +Cc: luca, kernel-janitors
On Wed, 17 Nov 2021, Dan Carpenter wrote:
> Hello Luca Ceresoli,
>
> The patch 50e4d7a2a667: "mfd: lp87565: Handle optional reset pin"
> from Feb 26, 2021, leads to the following Smatch static checker
> warning:
>
> drivers/mfd/lp87565.c:76 lp87565_probe()
> warn: 'lp87565->reset_gpio' could be an error pointer
>
> drivers/mfd/lp87565.c
> 46 static int lp87565_probe(struct i2c_client *client,
> 47 const struct i2c_device_id *ids)
> 48 {
> 49 struct lp87565 *lp87565;
> 50 const struct of_device_id *of_id;
> 51 int ret;
> 52 unsigned int otpid;
> 53
> 54 lp87565 = devm_kzalloc(&client->dev, sizeof(*lp87565), GFP_KERNEL);
> 55 if (!lp87565)
> 56 return -ENOMEM;
> 57
> 58 lp87565->dev = &client->dev;
> 59
> 60 lp87565->regmap = devm_regmap_init_i2c(client, &lp87565_regmap_config);
> 61 if (IS_ERR(lp87565->regmap)) {
> 62 ret = PTR_ERR(lp87565->regmap);
> 63 dev_err(lp87565->dev,
> 64 "Failed to initialize register map: %d\n", ret);
> 65 return ret;
> 66 }
> 67
> 68 lp87565->reset_gpio = devm_gpiod_get_optional(lp87565->dev, "reset",
> 69 GPIOD_OUT_LOW);
> 70 if (IS_ERR(lp87565->reset_gpio)) {
> 71 ret = PTR_ERR(lp87565->reset_gpio);
> 72 if (ret == -EPROBE_DEFER)
> 73 return ret;
>
> Only "ret = -EPROBE_DEFER" is handled. Other error pointer will lead to
> a crash.
>
> 74 }
> 75
> --> 76 if (lp87565->reset_gpio) {
I guess this should be:
if (lp87565->reset_gpio >= 0)
If 0 is valid, or '>' if it's not.
> 77 gpiod_set_value_cansleep(lp87565->reset_gpio, 1);
> 78 /* The minimum assertion time is undocumented, just guess */
> 79 usleep_range(2000, 4000);
> 80
> 81 gpiod_set_value_cansleep(lp87565->reset_gpio, 0);
> 82 /* Min 1.2 ms before first I2C transaction */
> 83 usleep_range(1500, 3000);
> 84 }
> 85
> 86 ret = regmap_read(lp87565->regmap, LP87565_REG_OTP_REV, &otpid);
> 87 if (ret) {
> 88 dev_err(lp87565->dev, "Failed to read OTP ID\n");
> 89 return ret;
> 90 }
> 91
> 92 lp87565->rev = otpid & LP87565_OTP_REV_OTP_ID;
> 93
> 94 of_id = of_match_device(of_lp87565_match_table, &client->dev);
> 95 if (of_id)
> 96 lp87565->dev_type = (enum lp87565_device_type)of_id->data;
> 97
> 98 i2c_set_clientdata(client, lp87565);
> 99
> 100 return devm_mfd_add_devices(lp87565->dev, PLATFORM_DEVID_AUTO,
> 101 lp87565_cells, ARRAY_SIZE(lp87565_cells),
> 102 NULL, 0, NULL);
> 103 }
>
> regards,
> dan carpenter
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] mfd: lp87565: Handle optional reset pin
2021-11-18 8:38 ` Lee Jones
@ 2021-11-18 8:47 ` Dan Carpenter
2021-11-18 9:00 ` Lee Jones
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2021-11-18 8:47 UTC (permalink / raw)
To: Lee Jones; +Cc: luca, kernel-janitors
On Thu, Nov 18, 2021 at 08:38:12AM +0000, Lee Jones wrote:
> On Wed, 17 Nov 2021, Dan Carpenter wrote:
>
> > Hello Luca Ceresoli,
> >
> > The patch 50e4d7a2a667: "mfd: lp87565: Handle optional reset pin"
> > from Feb 26, 2021, leads to the following Smatch static checker
> > warning:
> >
> > drivers/mfd/lp87565.c:76 lp87565_probe()
> > warn: 'lp87565->reset_gpio' could be an error pointer
> >
> > drivers/mfd/lp87565.c
> > 46 static int lp87565_probe(struct i2c_client *client,
> > 47 const struct i2c_device_id *ids)
> > 48 {
> > 49 struct lp87565 *lp87565;
> > 50 const struct of_device_id *of_id;
> > 51 int ret;
> > 52 unsigned int otpid;
> > 53
> > 54 lp87565 = devm_kzalloc(&client->dev, sizeof(*lp87565), GFP_KERNEL);
> > 55 if (!lp87565)
> > 56 return -ENOMEM;
> > 57
> > 58 lp87565->dev = &client->dev;
> > 59
> > 60 lp87565->regmap = devm_regmap_init_i2c(client, &lp87565_regmap_config);
> > 61 if (IS_ERR(lp87565->regmap)) {
> > 62 ret = PTR_ERR(lp87565->regmap);
> > 63 dev_err(lp87565->dev,
> > 64 "Failed to initialize register map: %d\n", ret);
> > 65 return ret;
> > 66 }
> > 67
> > 68 lp87565->reset_gpio = devm_gpiod_get_optional(lp87565->dev, "reset",
> > 69 GPIOD_OUT_LOW);
> > 70 if (IS_ERR(lp87565->reset_gpio)) {
> > 71 ret = PTR_ERR(lp87565->reset_gpio);
> > 72 if (ret == -EPROBE_DEFER)
> > 73 return ret;
> >
> > Only "ret = -EPROBE_DEFER" is handled. Other error pointer will lead to
> > a crash.
> >
> > 74 }
> > 75
> > --> 76 if (lp87565->reset_gpio) {
>
> I guess this should be:
>
> if (lp87565->reset_gpio >= 0)
>
> If 0 is valid, or '>' if it's not.
>
lp87565->reset_gpio is a pointer so that's not going to work.
There are two options. The first is to just "return ret;" for all
errors. The second option is to say something like:
if (IS_ERR(lp87565->reset_gpio)) {
ret = dev_err_probe(lp87565->dev, PTR_ERR(lp87565->reset_gpio),
"no GPIO error for ->reset_gpio");
if (ret == -EPROBE_DEFER)
return;
lp87565->reset_gpio = NULL;
}
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] mfd: lp87565: Handle optional reset pin
2021-11-18 8:47 ` Dan Carpenter
@ 2021-11-18 9:00 ` Lee Jones
2021-11-18 9:22 ` Luca Ceresoli
0 siblings, 1 reply; 7+ messages in thread
From: Lee Jones @ 2021-11-18 9:00 UTC (permalink / raw)
To: Dan Carpenter; +Cc: luca, kernel-janitors
On Thu, 18 Nov 2021, Dan Carpenter wrote:
> On Thu, Nov 18, 2021 at 08:38:12AM +0000, Lee Jones wrote:
> > On Wed, 17 Nov 2021, Dan Carpenter wrote:
> >
> > > Hello Luca Ceresoli,
> > >
> > > The patch 50e4d7a2a667: "mfd: lp87565: Handle optional reset pin"
> > > from Feb 26, 2021, leads to the following Smatch static checker
> > > warning:
> > >
> > > drivers/mfd/lp87565.c:76 lp87565_probe()
> > > warn: 'lp87565->reset_gpio' could be an error pointer
> > >
> > > drivers/mfd/lp87565.c
> > > 46 static int lp87565_probe(struct i2c_client *client,
> > > 47 const struct i2c_device_id *ids)
> > > 48 {
> > > 49 struct lp87565 *lp87565;
> > > 50 const struct of_device_id *of_id;
> > > 51 int ret;
> > > 52 unsigned int otpid;
> > > 53
> > > 54 lp87565 = devm_kzalloc(&client->dev, sizeof(*lp87565), GFP_KERNEL);
> > > 55 if (!lp87565)
> > > 56 return -ENOMEM;
> > > 57
> > > 58 lp87565->dev = &client->dev;
> > > 59
> > > 60 lp87565->regmap = devm_regmap_init_i2c(client, &lp87565_regmap_config);
> > > 61 if (IS_ERR(lp87565->regmap)) {
> > > 62 ret = PTR_ERR(lp87565->regmap);
> > > 63 dev_err(lp87565->dev,
> > > 64 "Failed to initialize register map: %d\n", ret);
> > > 65 return ret;
> > > 66 }
> > > 67
> > > 68 lp87565->reset_gpio = devm_gpiod_get_optional(lp87565->dev, "reset",
> > > 69 GPIOD_OUT_LOW);
> > > 70 if (IS_ERR(lp87565->reset_gpio)) {
> > > 71 ret = PTR_ERR(lp87565->reset_gpio);
> > > 72 if (ret == -EPROBE_DEFER)
> > > 73 return ret;
> > >
> > > Only "ret = -EPROBE_DEFER" is handled. Other error pointer will lead to
> > > a crash.
> > >
> > > 74 }
> > > 75
> > > --> 76 if (lp87565->reset_gpio) {
> >
> > I guess this should be:
> >
> > if (lp87565->reset_gpio >= 0)
> >
> > If 0 is valid, or '>' if it's not.
> >
>
> lp87565->reset_gpio is a pointer so that's not going to work.
Ah, I assumed this was a GPIO #. I missed the gpioD part.
> There are two options. The first is to just "return ret;" for all
> errors.
No, don't do that. This GPIO is optional.
> The second option is to say something like:
>
> if (IS_ERR(lp87565->reset_gpio)) {
> ret = dev_err_probe(lp87565->dev, PTR_ERR(lp87565->reset_gpio),
> "no GPIO error for ->reset_gpio");
> if (ret == -EPROBE_DEFER)
> return;
> lp87565->reset_gpio = NULL;
> }
Works for me.
Or:
if (!IS_ERR(lp87565->reset_gpio))
Or stick it in the else.
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] mfd: lp87565: Handle optional reset pin
2021-11-18 9:00 ` Lee Jones
@ 2021-11-18 9:22 ` Luca Ceresoli
2021-11-18 10:40 ` Dan Carpenter
0 siblings, 1 reply; 7+ messages in thread
From: Luca Ceresoli @ 2021-11-18 9:22 UTC (permalink / raw)
To: Lee Jones, Dan Carpenter; +Cc: kernel-janitors
Hi,
On 18/11/21 10:00, Lee Jones wrote:
> On Thu, 18 Nov 2021, Dan Carpenter wrote:
>
>> On Thu, Nov 18, 2021 at 08:38:12AM +0000, Lee Jones wrote:
>>> On Wed, 17 Nov 2021, Dan Carpenter wrote:
>>>
>>>> Hello Luca Ceresoli,
>>>>
>>>> The patch 50e4d7a2a667: "mfd: lp87565: Handle optional reset pin"
>>>> from Feb 26, 2021, leads to the following Smatch static checker
>>>> warning:
>>>>
>>>> drivers/mfd/lp87565.c:76 lp87565_probe()
>>>> warn: 'lp87565->reset_gpio' could be an error pointer
>>>>
>>>> drivers/mfd/lp87565.c
>>>> 46 static int lp87565_probe(struct i2c_client *client,
>>>> 47 const struct i2c_device_id *ids)
>>>> 48 {
>>>> 49 struct lp87565 *lp87565;
>>>> 50 const struct of_device_id *of_id;
>>>> 51 int ret;
>>>> 52 unsigned int otpid;
>>>> 53
>>>> 54 lp87565 = devm_kzalloc(&client->dev, sizeof(*lp87565), GFP_KERNEL);
>>>> 55 if (!lp87565)
>>>> 56 return -ENOMEM;
>>>> 57
>>>> 58 lp87565->dev = &client->dev;
>>>> 59
>>>> 60 lp87565->regmap = devm_regmap_init_i2c(client, &lp87565_regmap_config);
>>>> 61 if (IS_ERR(lp87565->regmap)) {
>>>> 62 ret = PTR_ERR(lp87565->regmap);
>>>> 63 dev_err(lp87565->dev,
>>>> 64 "Failed to initialize register map: %d\n", ret);
>>>> 65 return ret;
>>>> 66 }
>>>> 67
>>>> 68 lp87565->reset_gpio = devm_gpiod_get_optional(lp87565->dev, "reset",
>>>> 69 GPIOD_OUT_LOW);
>>>> 70 if (IS_ERR(lp87565->reset_gpio)) {
>>>> 71 ret = PTR_ERR(lp87565->reset_gpio);
>>>> 72 if (ret == -EPROBE_DEFER)
>>>> 73 return ret;
>>>>
>>>> Only "ret = -EPROBE_DEFER" is handled. Other error pointer will lead to
>>>> a crash.
>>>>
>>>> 74 }
>>>> 75
>>>> --> 76 if (lp87565->reset_gpio) {
>>>
>>> I guess this should be:
>>>
>>> if (lp87565->reset_gpio >= 0)
>>>
>>> If 0 is valid, or '>' if it's not.
>>>
>>
>> lp87565->reset_gpio is a pointer so that's not going to work.
>
> Ah, I assumed this was a GPIO #. I missed the gpioD part.
>
>> There are two options. The first is to just "return ret;" for all
>> errors.
>
> No, don't do that. This GPIO is optional.
>
>> The second option is to say something like:
>>
>> if (IS_ERR(lp87565->reset_gpio)) {
>> ret = dev_err_probe(lp87565->dev, PTR_ERR(lp87565->reset_gpio),
>> "no GPIO error for ->reset_gpio");
>> if (ret == -EPROBE_DEFER)
>> return;
>> lp87565->reset_gpio = NULL;
>> }
>
> Works for me.
>
> Or:
>
> if (!IS_ERR(lp87565->reset_gpio))
>
> Or stick it in the else.
>
As the author of the code to blame, I wrote this patch, but just needed
a little time to test it before sending:
lp87565->reset_gpio = devm_gpiod_get_optional(lp87565->dev, "reset",
GPIOD_OUT_LOW);
if (IS_ERR(lp87565->reset_gpio))
return dev_err_probe(lp87565->dev, PTR_ERR(lp87565->reset_gpio),
"Failed getting reset GPIO");
if (lp87565->reset_gpio) {
...
I prefer to exit on any error as it would be either -EPROBE_DEFER of a
_real_ error (e.g. GPIO already in use). If there's no GPIO specified,
then devm_gpiod_get_optional() returns NULL and libgpio ignores NULL
pointers gracefully.
Would that work?
--
Luca
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] mfd: lp87565: Handle optional reset pin
2021-11-18 9:22 ` Luca Ceresoli
@ 2021-11-18 10:40 ` Dan Carpenter
2021-11-18 12:45 ` Luca Ceresoli
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2021-11-18 10:40 UTC (permalink / raw)
To: Luca Ceresoli; +Cc: Lee Jones, kernel-janitors
On Thu, Nov 18, 2021 at 10:22:52AM +0100, Luca Ceresoli wrote:
>
> As the author of the code to blame, I wrote this patch, but just needed
> a little time to test it before sending:
>
> lp87565->reset_gpio = devm_gpiod_get_optional(lp87565->dev, "reset",
> GPIOD_OUT_LOW);
> if (IS_ERR(lp87565->reset_gpio))
> return dev_err_probe(lp87565->dev, PTR_ERR(lp87565->reset_gpio),
> "Failed getting reset GPIO");
>
> if (lp87565->reset_gpio) {
> ...
>
> I prefer to exit on any error as it would be either -EPROBE_DEFER of a
> _real_ error (e.g. GPIO already in use). If there's no GPIO specified,
> then devm_gpiod_get_optional() returns NULL and libgpio ignores NULL
> pointers gracefully.
>
> Would that work?
I generally prefer that as well, because to me optional means it's up to
the user not that it's up to the kernel. But it depends on if the
system can boot without it etc...
I guess in this case we know that no one was relying on the old behavior
because that would have crashed so returning errors is safe.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] mfd: lp87565: Handle optional reset pin
2021-11-18 10:40 ` Dan Carpenter
@ 2021-11-18 12:45 ` Luca Ceresoli
0 siblings, 0 replies; 7+ messages in thread
From: Luca Ceresoli @ 2021-11-18 12:45 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Lee Jones, kernel-janitors
Hi,
On 18/11/21 11:40, Dan Carpenter wrote:
> On Thu, Nov 18, 2021 at 10:22:52AM +0100, Luca Ceresoli wrote:
>>
>> As the author of the code to blame, I wrote this patch, but just needed
>> a little time to test it before sending:
>>
>> lp87565->reset_gpio = devm_gpiod_get_optional(lp87565->dev, "reset",
>> GPIOD_OUT_LOW);
>> if (IS_ERR(lp87565->reset_gpio))
>> return dev_err_probe(lp87565->dev, PTR_ERR(lp87565->reset_gpio),
>> "Failed getting reset GPIO");
>>
>> if (lp87565->reset_gpio) {
>> ...
>>
>> I prefer to exit on any error as it would be either -EPROBE_DEFER of a
>> _real_ error (e.g. GPIO already in use). If there's no GPIO specified,
>> then devm_gpiod_get_optional() returns NULL and libgpio ignores NULL
>> pointers gracefully.
>>
>> Would that work?
>
> I generally prefer that as well, because to me optional means it's up to
> the user not that it's up to the kernel. But it depends on if the
> system can boot without it etc...
>
> I guess in this case we know that no one was relying on the old behavior
> because that would have crashed so returning errors is safe.
I sent a patch using this approach. I hope it's OK, otherwise I can send
a v2.
Last but not least: thanks Dan for finding and reporting!
--
Luca
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-11-18 12:46 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-17 11:17 [bug report] mfd: lp87565: Handle optional reset pin Dan Carpenter
2021-11-18 8:38 ` Lee Jones
2021-11-18 8:47 ` Dan Carpenter
2021-11-18 9:00 ` Lee Jones
2021-11-18 9:22 ` Luca Ceresoli
2021-11-18 10:40 ` Dan Carpenter
2021-11-18 12:45 ` Luca Ceresoli
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.