[bug report] firmware: arm_scmi: Add notification callbacks-registration
Dan Carpenter
dan.carpenter at oracle.com
Tue Jun 30 13:57:18 EDT 2020
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
More information about the linux-arm-kernel
mailing list