From: Dan Carpenter <dan.carpenter@oracle.com>
To: cristian.marussi@arm.com
Cc: linux-arm-kernel@lists.infradead.org
Subject: [bug report] firmware: arm_scmi: Add notification callbacks-registration
Date: Tue, 30 Jun 2020 20:57:18 +0300 [thread overview]
Message-ID: <20200630175718.GB34614@mwanda> (raw)
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
next reply other threads:[~2020-06-30 18:01 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-30 17:57 Dan Carpenter [this message]
2020-07-01 12:29 ` [bug report] firmware: arm_scmi: Add notification callbacks-registration Cristian Marussi
2020-07-02 11:17 ` Dan Carpenter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200630175718.GB34614@mwanda \
--to=dan.carpenter@oracle.com \
--cc=cristian.marussi@arm.com \
--cc=linux-arm-kernel@lists.infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.