* [bug report] firmware: arm_scmi: Add notification callbacks-registration
@ 2020-06-30 17:57 Dan Carpenter
2020-07-01 12:29 ` Cristian Marussi
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2020-06-30 17:57 UTC (permalink / raw)
To: cristian.marussi; +Cc: linux-arm-kernel
Hello Cristian Marussi,
The patch 5b352c537930: "firmware: arm_scmi: Add notification
callbacks-registration" from Jun 19, 2020, leads to the following
static checker warning:
drivers/firmware/arm_scmi/notify.c:1267 scmi_register_notifier()
warn: passing zero to 'PTR_ERR'
drivers/firmware/arm_scmi/notify.c
1248 static int scmi_register_notifier(const struct scmi_handle *handle,
1249 u8 proto_id, u8 evt_id, u32 *src_id,
1250 struct notifier_block *nb)
1251 {
1252 int ret = 0;
1253 u32 evt_key;
1254 struct scmi_event_handler *hndl;
1255 struct scmi_notify_instance *ni;
1256
1257 /* Ensure notify_priv is updated */
1258 smp_rmb();
1259 if (unlikely(!handle->notify_priv))
1260 return -ENODEV;
1261 ni = handle->notify_priv;
1262
1263 evt_key = MAKE_HASH_KEY(proto_id, evt_id,
1264 src_id ? *src_id : SRC_ID_MASK);
1265 hndl = scmi_get_or_create_handler(ni, evt_key);
1266 if (IS_ERR_OR_NULL(hndl))
^^^^^^^^^^^^^^^^^^^^
There are a lot of wrong uses of IS_ERR_OR_NULL() in this driver.
When a function returns both NULL and error pointers, then NULL is a
special kind of success. For example, we could have code which does
"p = get_feature();" and maybe there is an allocation failure, then
get_feature() should return an error pointer and we return that to the
user.
But if the get_feature() is optional and it has been deliberately
disabled by the user with CONFIG_FOO=n then that is *not* an error. But
we are still not able to return a valid pointer to the feature so it
returns NULL. The driver should continue to operate without the
optional feature.
1267 return PTR_ERR(hndl);
In this situation the scmi_get_or_create_handler() never returns error
pointers. It's not an optional feature. It returns NULL on failure.
So that means we return PTR_ERR(NULL) which is zero which means success.
The code should instead a negative error code instead:
if (!hndl)
return -EINVAL;
All the uses of IS_ERR_OR_NULL() that I saw were wrong but this is the
only one that caused a user visible bug.
1268
1269 blocking_notifier_chain_register(&hndl->chain, nb);
1270
1271 /* Enable events for not pending handlers */
1272 if (likely(!IS_HNDL_PENDING(hndl))) {
^^^^^^
The likely/unlikely() annotations should only be used where it afects
benchmarking. They should all be removed from this driver.
1273 if (!scmi_event_handler_enable_events(hndl)) {
I really don't like that half these functions follow normal kernel error
style and half return true/false on failure. Normally we would want
boolean functions to be clear from the name like access_ok() which
clearly returns true/false. But I didn't see that it causes any bugs
yet.
One thing I did notice is that scmi_sensors_init() return non-zero
positive values on success because it's returning the result from
idr_alloc(). This should trigger a warning message, I believe.
1274 scmi_put_handler(ni, hndl);
1275 ret = -EINVAL;
1276 }
1277 }
1278
1279 return ret;
1280 }
regards,
dan carpenter
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] firmware: arm_scmi: Add notification callbacks-registration
2020-06-30 17:57 [bug report] firmware: arm_scmi: Add notification callbacks-registration Dan Carpenter
@ 2020-07-01 12:29 ` Cristian Marussi
2020-07-02 11:17 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Cristian Marussi @ 2020-07-01 12:29 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-arm-kernel, sudeep.holla
On Tue, Jun 30, 2020 at 08:57:18PM +0300, Dan Carpenter wrote:
> Hello Cristian Marussi,
Hi Dan Carpenter
thanks for the feedback first of all.
>
> The patch 5b352c537930: "firmware: arm_scmi: Add notification
> callbacks-registration" from Jun 19, 2020, leads to the following
> static checker warning:
>
> drivers/firmware/arm_scmi/notify.c:1267 scmi_register_notifier()
> warn: passing zero to 'PTR_ERR'
>
> drivers/firmware/arm_scmi/notify.c
> 1248 static int scmi_register_notifier(const struct scmi_handle *handle,
> 1249 u8 proto_id, u8 evt_id, u32 *src_id,
> 1250 struct notifier_block *nb)
> 1251 {
> 1252 int ret = 0;
> 1253 u32 evt_key;
> 1254 struct scmi_event_handler *hndl;
> 1255 struct scmi_notify_instance *ni;
> 1256
> 1257 /* Ensure notify_priv is updated */
> 1258 smp_rmb();
> 1259 if (unlikely(!handle->notify_priv))
> 1260 return -ENODEV;
> 1261 ni = handle->notify_priv;
> 1262
> 1263 evt_key = MAKE_HASH_KEY(proto_id, evt_id,
> 1264 src_id ? *src_id : SRC_ID_MASK);
> 1265 hndl = scmi_get_or_create_handler(ni, evt_key);
> 1266 if (IS_ERR_OR_NULL(hndl))
> ^^^^^^^^^^^^^^^^^^^^
> There are a lot of wrong uses of IS_ERR_OR_NULL() in this driver.
>
> When a function returns both NULL and error pointers, then NULL is a
> special kind of success. For example, we could have code which does
> "p = get_feature();" and maybe there is an allocation failure, then
> get_feature() should return an error pointer and we return that to the
> user.
>
> But if the get_feature() is optional and it has been deliberately
> disabled by the user with CONFIG_FOO=n then that is *not* an error. But
> we are still not able to return a valid pointer to the feature so it
> returns NULL. The driver should continue to operate without the
> optional feature.
>
> 1267 return PTR_ERR(hndl);
>
> In this situation the scmi_get_or_create_handler() never returns error
> pointers. It's not an optional feature. It returns NULL on failure.
> So that means we return PTR_ERR(NULL) which is zero which means success.
> The code should instead a negative error code instead:
>
> if (!hndl)
> return -EINVAL;
>
> All the uses of IS_ERR_OR_NULL() that I saw were wrong but this is the
> only one that caused a user visible bug.
I did not know about this intended usage, I'm removing all these unneded
IS_ERR_OR_NULL() usages with plain NULL checks.
>
> 1268
> 1269 blocking_notifier_chain_register(&hndl->chain, nb);
> 1270
> 1271 /* Enable events for not pending handlers */
> 1272 if (likely(!IS_HNDL_PENDING(hndl))) {
> ^^^^^^
> The likely/unlikely() annotations should only be used where it afects
> benchmarking. They should all be removed from this driver.
I'm removing all the likely/unlikely from this series.
>
> 1273 if (!scmi_event_handler_enable_events(hndl)) {
>
> I really don't like that half these functions follow normal kernel error
> style and half return true/false on failure. Normally we would want
> boolean functions to be clear from the name like access_ok() which
> clearly returns true/false. But I didn't see that it causes any bugs
> yet.
Ok I'll convert all the functions to classical 0-Success style dumping
the booleans. I cannot see in fact any suitable func_ok() style function.
>
> One thing I did notice is that scmi_sensors_init() return non-zero
> positive values on success because it's returning the result from
> idr_alloc(). This should trigger a warning message, I believe.
>
> 1274 scmi_put_handler(ni, hndl);
> 1275 ret = -EINVAL;
> 1276 }
> 1277 }
> 1278
> 1279 return ret;
> 1280 }
>
The scmi_sensors_init() retval is not part of this series so I'll make
a note of it but do not change it right now, also because I'm also reviewing
more generally the whole SCMI initialization core, aiming at full modularization
of the protocols, so that this part is already changed/gone in another
unpublished series.
Thanks
Cristian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] firmware: arm_scmi: Add notification callbacks-registration
2020-07-01 12:29 ` Cristian Marussi
@ 2020-07-02 11:17 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2020-07-02 11:17 UTC (permalink / raw)
To: Cristian Marussi; +Cc: linux-arm-kernel, sudeep.holla
On Wed, Jul 01, 2020 at 01:29:24PM +0100, Cristian Marussi wrote:
> > 1273 if (!scmi_event_handler_enable_events(hndl)) {
> >
> > I really don't like that half these functions follow normal kernel error
> > style and half return true/false on failure. Normally we would want
> > boolean functions to be clear from the name like access_ok() which
> > clearly returns true/false. But I didn't see that it causes any bugs
> > yet.
>
> Ok I'll convert all the functions to classical 0-Success style dumping
> the booleans. I cannot see in fact any suitable func_ok() style function.
You are a hero. Thanks!
regards,
dan carpenter
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-07-02 11:20 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-30 17:57 [bug report] firmware: arm_scmi: Add notification callbacks-registration Dan Carpenter
2020-07-01 12:29 ` Cristian Marussi
2020-07-02 11:17 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).