Message ID | 20200630162547.40824-1-cristian.marussi@arm.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | firmware: arm_scmi: fix notifications macros argument reuse | expand |
Hi Cristian,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on next-20200630]
[cannot apply to linux/master soc/for-next linus/master v5.8-rc3 v5.8-rc2 v5.8-rc1 v5.8-rc3]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Cristian-Marussi/firmware-arm_scmi-fix-notifications-macros-argument-reuse/20200701-002818
base: f2b92b14533e646e434523abdbafddb727c23898
compiler: gcc-9 (Debian 9.3.0-14) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
cppcheck warnings: (new ones prefixed by >>)
drivers/firmware/arm_scmi/notify.c:660:9: warning: Identical condition 'ret', second condition is always false [identicalConditionAfterEarlyExit]
return ret;
^
drivers/firmware/arm_scmi/notify.c:654:6: note: first condition
if (ret)
^
drivers/firmware/arm_scmi/notify.c:660:9: note: second condition
return ret;
^
>> drivers/firmware/arm_scmi/notify.c:1125:9: warning: Variable 'r' is reassigned a value before the old one has been used. [redundantAssignment]
r = REVT_NOTIFY_ENABLE(r_evt,
^
drivers/firmware/arm_scmi/notify.c:1125:7: note: Variable 'r' is reassigned a value before the old one has been used.
r = REVT_NOTIFY_ENABLE(r_evt,
^
drivers/firmware/arm_scmi/notify.c:1125:9: note: Variable 'r' is reassigned a value before the old one has been used.
r = REVT_NOTIFY_ENABLE(r_evt,
^
vim +/r +1125 drivers/firmware/arm_scmi/notify.c
7a57069e9daf95a Cristian Marussi 2020-06-19 1090
5b352c5379300ec Cristian Marussi 2020-06-19 1091 /**
5b352c5379300ec Cristian Marussi 2020-06-19 1092 * __scmi_enable_evt() - Enable/disable events generation
5b352c5379300ec Cristian Marussi 2020-06-19 1093 * @r_evt: The registered event to act upon
5b352c5379300ec Cristian Marussi 2020-06-19 1094 * @src_id: The src_id to act upon
5b352c5379300ec Cristian Marussi 2020-06-19 1095 * @enable: The action to perform: true->Enable, false->Disable
5b352c5379300ec Cristian Marussi 2020-06-19 1096 *
5b352c5379300ec Cristian Marussi 2020-06-19 1097 * Takes care of proper refcounting while performing enable/disable: handles
5b352c5379300ec Cristian Marussi 2020-06-19 1098 * the special case of ALL sources requests by itself.
5b352c5379300ec Cristian Marussi 2020-06-19 1099 *
5b352c5379300ec Cristian Marussi 2020-06-19 1100 * Return: True when the required action has been successfully executed
5b352c5379300ec Cristian Marussi 2020-06-19 1101 */
5b352c5379300ec Cristian Marussi 2020-06-19 1102 static inline bool __scmi_enable_evt(struct scmi_registered_event *r_evt,
5b352c5379300ec Cristian Marussi 2020-06-19 1103 u32 src_id, bool enable)
5b352c5379300ec Cristian Marussi 2020-06-19 1104 {
5b352c5379300ec Cristian Marussi 2020-06-19 1105 int ret = 0;
5b352c5379300ec Cristian Marussi 2020-06-19 1106 u32 num_sources;
5b352c5379300ec Cristian Marussi 2020-06-19 1107 refcount_t *sid;
5b352c5379300ec Cristian Marussi 2020-06-19 1108
5b352c5379300ec Cristian Marussi 2020-06-19 1109 if (src_id == SRC_ID_MASK) {
5b352c5379300ec Cristian Marussi 2020-06-19 1110 src_id = 0;
5b352c5379300ec Cristian Marussi 2020-06-19 1111 num_sources = r_evt->num_sources;
5b352c5379300ec Cristian Marussi 2020-06-19 1112 } else if (src_id < r_evt->num_sources) {
5b352c5379300ec Cristian Marussi 2020-06-19 1113 num_sources = 1;
5b352c5379300ec Cristian Marussi 2020-06-19 1114 } else {
5b352c5379300ec Cristian Marussi 2020-06-19 1115 return ret;
5b352c5379300ec Cristian Marussi 2020-06-19 1116 }
5b352c5379300ec Cristian Marussi 2020-06-19 1117
5b352c5379300ec Cristian Marussi 2020-06-19 1118 mutex_lock(&r_evt->sources_mtx);
5b352c5379300ec Cristian Marussi 2020-06-19 1119 if (enable) {
5b352c5379300ec Cristian Marussi 2020-06-19 1120 for (; num_sources; src_id++, num_sources--) {
5b352c5379300ec Cristian Marussi 2020-06-19 1121 bool r;
5b352c5379300ec Cristian Marussi 2020-06-19 1122
5b352c5379300ec Cristian Marussi 2020-06-19 1123 sid = &r_evt->sources[src_id];
5b352c5379300ec Cristian Marussi 2020-06-19 1124 if (refcount_read(sid) == 0) {
5b352c5379300ec Cristian Marussi 2020-06-19 @1125 r = REVT_NOTIFY_ENABLE(r_evt,
5b352c5379300ec Cristian Marussi 2020-06-19 1126 r_evt->evt->id, src_id);
5b352c5379300ec Cristian Marussi 2020-06-19 1127 if (r)
5b352c5379300ec Cristian Marussi 2020-06-19 1128 refcount_set(sid, 1);
5b352c5379300ec Cristian Marussi 2020-06-19 1129 } else {
5b352c5379300ec Cristian Marussi 2020-06-19 1130 refcount_inc(sid);
5b352c5379300ec Cristian Marussi 2020-06-19 1131 r = true;
5b352c5379300ec Cristian Marussi 2020-06-19 1132 }
5b352c5379300ec Cristian Marussi 2020-06-19 1133 ret += r;
5b352c5379300ec Cristian Marussi 2020-06-19 1134 }
5b352c5379300ec Cristian Marussi 2020-06-19 1135 } else {
5b352c5379300ec Cristian Marussi 2020-06-19 1136 for (; num_sources; src_id++, num_sources--) {
5b352c5379300ec Cristian Marussi 2020-06-19 1137 sid = &r_evt->sources[src_id];
5b352c5379300ec Cristian Marussi 2020-06-19 1138 if (refcount_dec_and_test(sid))
5b352c5379300ec Cristian Marussi 2020-06-19 1139 REVT_NOTIFY_DISABLE(r_evt,
5b352c5379300ec Cristian Marussi 2020-06-19 1140 r_evt->evt->id, src_id);
5b352c5379300ec Cristian Marussi 2020-06-19 1141 }
5b352c5379300ec Cristian Marussi 2020-06-19 1142 ret = 1;
5b352c5379300ec Cristian Marussi 2020-06-19 1143 }
5b352c5379300ec Cristian Marussi 2020-06-19 1144 mutex_unlock(&r_evt->sources_mtx);
5b352c5379300ec Cristian Marussi 2020-06-19 1145
5b352c5379300ec Cristian Marussi 2020-06-19 1146 return ret;
5b352c5379300ec Cristian Marussi 2020-06-19 1147 }
5b352c5379300ec Cristian Marussi 2020-06-19 1148
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi On Sat, Jul 04, 2020 at 07:01:21AM +0800, kernel test robot wrote: > Hi Cristian, > > Thank you for the patch! Perhaps something to improve: > > [auto build test WARNING on next-20200630] > [cannot apply to linux/master soc/for-next linus/master v5.8-rc3 v5.8-rc2 v5.8-rc1 v5.8-rc3] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use as documented in > https://git-scm.com/docs/git-format-patch] > > url: https://github.com/0day-ci/linux/commits/Cristian-Marussi/firmware-arm_scmi-fix-notifications-macros-argument-reuse/20200701-002818 > base: f2b92b14533e646e434523abdbafddb727c23898 > compiler: gcc-9 (Debian 9.3.0-14) 9.3.0 > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot <lkp@intel.com> > > This patch was afterwards squashed into the SCMI Notifications series V11: https://lore.kernel.org/linux-arm-kernel/20200701155348.52864-1-cristian.marussi@arm.com/ and requeued on next, and, as I can see all of the surrounding/offending code referenced in this report has been modified in V11 so I don' expect the reported issues to be anymore valid...but let's wait to see the next CI run results on next- to be sure about that :D (a local SMATCH/sparse run returned me no issues on V11) Thanks Cristian > cppcheck warnings: (new ones prefixed by >>) > > drivers/firmware/arm_scmi/notify.c:660:9: warning: Identical condition 'ret', second condition is always false [identicalConditionAfterEarlyExit] > return ret; > ^ > drivers/firmware/arm_scmi/notify.c:654:6: note: first condition > if (ret) > ^ > drivers/firmware/arm_scmi/notify.c:660:9: note: second condition > return ret; > ^ > >> drivers/firmware/arm_scmi/notify.c:1125:9: warning: Variable 'r' is reassigned a value before the old one has been used. [redundantAssignment] > r = REVT_NOTIFY_ENABLE(r_evt, > ^ > drivers/firmware/arm_scmi/notify.c:1125:7: note: Variable 'r' is reassigned a value before the old one has been used. > r = REVT_NOTIFY_ENABLE(r_evt, > ^ > drivers/firmware/arm_scmi/notify.c:1125:9: note: Variable 'r' is reassigned a value before the old one has been used. > r = REVT_NOTIFY_ENABLE(r_evt, > ^ > > vim +/r +1125 drivers/firmware/arm_scmi/notify.c > > 7a57069e9daf95a Cristian Marussi 2020-06-19 1090 > 5b352c5379300ec Cristian Marussi 2020-06-19 1091 /** > 5b352c5379300ec Cristian Marussi 2020-06-19 1092 * __scmi_enable_evt() - Enable/disable events generation > 5b352c5379300ec Cristian Marussi 2020-06-19 1093 * @r_evt: The registered event to act upon > 5b352c5379300ec Cristian Marussi 2020-06-19 1094 * @src_id: The src_id to act upon > 5b352c5379300ec Cristian Marussi 2020-06-19 1095 * @enable: The action to perform: true->Enable, false->Disable > 5b352c5379300ec Cristian Marussi 2020-06-19 1096 * > 5b352c5379300ec Cristian Marussi 2020-06-19 1097 * Takes care of proper refcounting while performing enable/disable: handles > 5b352c5379300ec Cristian Marussi 2020-06-19 1098 * the special case of ALL sources requests by itself. > 5b352c5379300ec Cristian Marussi 2020-06-19 1099 * > 5b352c5379300ec Cristian Marussi 2020-06-19 1100 * Return: True when the required action has been successfully executed > 5b352c5379300ec Cristian Marussi 2020-06-19 1101 */ > 5b352c5379300ec Cristian Marussi 2020-06-19 1102 static inline bool __scmi_enable_evt(struct scmi_registered_event *r_evt, > 5b352c5379300ec Cristian Marussi 2020-06-19 1103 u32 src_id, bool enable) > 5b352c5379300ec Cristian Marussi 2020-06-19 1104 { > 5b352c5379300ec Cristian Marussi 2020-06-19 1105 int ret = 0; > 5b352c5379300ec Cristian Marussi 2020-06-19 1106 u32 num_sources; > 5b352c5379300ec Cristian Marussi 2020-06-19 1107 refcount_t *sid; > 5b352c5379300ec Cristian Marussi 2020-06-19 1108 > 5b352c5379300ec Cristian Marussi 2020-06-19 1109 if (src_id == SRC_ID_MASK) { > 5b352c5379300ec Cristian Marussi 2020-06-19 1110 src_id = 0; > 5b352c5379300ec Cristian Marussi 2020-06-19 1111 num_sources = r_evt->num_sources; > 5b352c5379300ec Cristian Marussi 2020-06-19 1112 } else if (src_id < r_evt->num_sources) { > 5b352c5379300ec Cristian Marussi 2020-06-19 1113 num_sources = 1; > 5b352c5379300ec Cristian Marussi 2020-06-19 1114 } else { > 5b352c5379300ec Cristian Marussi 2020-06-19 1115 return ret; > 5b352c5379300ec Cristian Marussi 2020-06-19 1116 } > 5b352c5379300ec Cristian Marussi 2020-06-19 1117 > 5b352c5379300ec Cristian Marussi 2020-06-19 1118 mutex_lock(&r_evt->sources_mtx); > 5b352c5379300ec Cristian Marussi 2020-06-19 1119 if (enable) { > 5b352c5379300ec Cristian Marussi 2020-06-19 1120 for (; num_sources; src_id++, num_sources--) { > 5b352c5379300ec Cristian Marussi 2020-06-19 1121 bool r; > 5b352c5379300ec Cristian Marussi 2020-06-19 1122 > 5b352c5379300ec Cristian Marussi 2020-06-19 1123 sid = &r_evt->sources[src_id]; > 5b352c5379300ec Cristian Marussi 2020-06-19 1124 if (refcount_read(sid) == 0) { > 5b352c5379300ec Cristian Marussi 2020-06-19 @1125 r = REVT_NOTIFY_ENABLE(r_evt, > 5b352c5379300ec Cristian Marussi 2020-06-19 1126 r_evt->evt->id, src_id); > 5b352c5379300ec Cristian Marussi 2020-06-19 1127 if (r) > 5b352c5379300ec Cristian Marussi 2020-06-19 1128 refcount_set(sid, 1); > 5b352c5379300ec Cristian Marussi 2020-06-19 1129 } else { > 5b352c5379300ec Cristian Marussi 2020-06-19 1130 refcount_inc(sid); > 5b352c5379300ec Cristian Marussi 2020-06-19 1131 r = true; > 5b352c5379300ec Cristian Marussi 2020-06-19 1132 } > 5b352c5379300ec Cristian Marussi 2020-06-19 1133 ret += r; > 5b352c5379300ec Cristian Marussi 2020-06-19 1134 } > 5b352c5379300ec Cristian Marussi 2020-06-19 1135 } else { > 5b352c5379300ec Cristian Marussi 2020-06-19 1136 for (; num_sources; src_id++, num_sources--) { > 5b352c5379300ec Cristian Marussi 2020-06-19 1137 sid = &r_evt->sources[src_id]; > 5b352c5379300ec Cristian Marussi 2020-06-19 1138 if (refcount_dec_and_test(sid)) > 5b352c5379300ec Cristian Marussi 2020-06-19 1139 REVT_NOTIFY_DISABLE(r_evt, > 5b352c5379300ec Cristian Marussi 2020-06-19 1140 r_evt->evt->id, src_id); > 5b352c5379300ec Cristian Marussi 2020-06-19 1141 } > 5b352c5379300ec Cristian Marussi 2020-06-19 1142 ret = 1; > 5b352c5379300ec Cristian Marussi 2020-06-19 1143 } > 5b352c5379300ec Cristian Marussi 2020-06-19 1144 mutex_unlock(&r_evt->sources_mtx); > 5b352c5379300ec Cristian Marussi 2020-06-19 1145 > 5b352c5379300ec Cristian Marussi 2020-06-19 1146 return ret; > 5b352c5379300ec Cristian Marussi 2020-06-19 1147 } > 5b352c5379300ec Cristian Marussi 2020-06-19 1148 > > --- > 0-DAY CI Kernel Test Service, Intel Corporation > https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff --git a/drivers/firmware/arm_scmi/notify.c b/drivers/firmware/arm_scmi/notify.c index b5b449f70605..bf8b3430c801 100644 --- a/drivers/firmware/arm_scmi/notify.c +++ b/drivers/firmware/arm_scmi/notify.c @@ -122,10 +122,13 @@ */ #define KEY_FIND(__ht, __obj, __k) \ ({ \ - hash_for_each_possible((__ht), (__obj), hash, (__k)) \ - if (likely((__obj)->key == (__k))) \ + typeof(__k) k_ = __k; \ + typeof(__obj) obj_; \ + \ + hash_for_each_possible((__ht), obj_, hash, k_) \ + if (likely(obj_->key == k_)) \ break; \ - __obj; \ + __obj = obj_; \ }) #define KEY_XTRACT_PROTO_ID(key) FIELD_GET(PROTO_ID_MASK, (key)) @@ -140,19 +143,22 @@ */ #define SCMI_GET_PROTO(__ni, __pid) \ ({ \ + typeof(__ni) ni_ = __ni; \ struct scmi_registered_events_desc *__pd = NULL; \ \ - if ((__ni)) \ - __pd = READ_ONCE((__ni)->registered_protocols[(__pid)]);\ + if (ni_) \ + __pd = READ_ONCE(ni_->registered_protocols[(__pid)]); \ __pd; \ }) #define SCMI_GET_REVT_FROM_PD(__pd, __eid) \ ({ \ + typeof(__pd) pd_ = __pd; \ + typeof(__eid) eid_ = __eid; \ struct scmi_registered_event *__revt = NULL; \ \ - if ((__pd) && (__eid) < (__pd)->num_events) \ - __revt = READ_ONCE((__pd)->registered_events[(__eid)]); \ + if (pd_ && eid_ < pd_->num_events) \ + __revt = READ_ONCE(pd_->registered_events[eid_]); \ __revt; \ }) @@ -167,15 +173,25 @@ }) /* A couple of utility macros to limit cruft when calling protocols' helpers */ -#define REVT_NOTIFY_ENABLE(revt, eid, sid) \ - ((revt)->proto->ops->set_notify_enabled((revt)->proto->ni->handle, \ - (eid), (sid), true)) -#define REVT_NOTIFY_DISABLE(revt, eid, sid) \ - ((revt)->proto->ops->set_notify_enabled((revt)->proto->ni->handle, \ - (eid), (sid), false)) -#define REVT_FILL_REPORT(revt, ...) \ - ((revt)->proto->ops->fill_custom_report((revt)->proto->ni->handle, \ - __VA_ARGS__)) +#define REVT_NOTIFY_SET_STATUS(revt, eid, sid, state) \ +({ \ + typeof(revt) r = revt; \ + r->proto->ops->set_notify_enabled(r->proto->ni->handle, \ + (eid), (sid), (state)); \ +}) + +#define REVT_NOTIFY_ENABLE(revt, eid, sid) \ + REVT_NOTIFY_SET_STATUS((revt), (eid), (sid), true) + +#define REVT_NOTIFY_DISABLE(revt, eid, sid) \ + REVT_NOTIFY_SET_STATUS((revt), (eid), (sid), false) + +#define REVT_FILL_REPORT(revt, ...) \ +({ \ + typeof(revt) r = revt; \ + r->proto->ops->fill_custom_report(r->proto->ni->handle, \ + __VA_ARGS__); \ +}) #define SCMI_PENDING_HASH_SZ 4 #define SCMI_REGISTERED_HASH_SZ 6
Checkpatch --strict reports some possible side-effects related to argument reuse in some SCMI Notification Core macros: these are indeed false flags in the context of actual macros invocations. Nevertheless cleanup fixing all the warnings. No functional change. Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> --- drivers/firmware/arm_scmi/notify.c | 48 ++++++++++++++++++++---------- 1 file changed, 32 insertions(+), 16 deletions(-)