[bug report] firmware: arm_scmi: Add notification callbacks-registration
Cristian Marussi
cristian.marussi at arm.com
Wed Jul 1 08:29:24 EDT 2020
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
More information about the linux-arm-kernel
mailing list