Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] firmware: arm_scmi: fix notifications macros argument reuse
@ 2020-06-30 16:25 Cristian Marussi
  2020-07-03 23:01 ` kernel test robot
  0 siblings, 1 reply; 3+ messages in thread
From: Cristian Marussi @ 2020-06-30 16:25 UTC (permalink / raw)
  To: linux-kernel, linux-arm-kernel; +Cc: cristian.marussi, sudeep.holla

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(-)

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
-- 
2.17.1


_______________________________________________
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: [PATCH] firmware: arm_scmi: fix notifications macros argument reuse
  2020-06-30 16:25 [PATCH] firmware: arm_scmi: fix notifications macros argument reuse Cristian Marussi
@ 2020-07-03 23:01 ` kernel test robot
  2020-07-06 13:20   ` Cristian Marussi
  0 siblings, 1 reply; 3+ messages in thread
From: kernel test robot @ 2020-07-03 23:01 UTC (permalink / raw)
  To: Cristian Marussi, linux-kernel, linux-arm-kernel
  Cc: kbuild-all, cristian.marussi, sudeep.holla

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

_______________________________________________
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: [PATCH] firmware: arm_scmi: fix notifications macros argument reuse
  2020-07-03 23:01 ` kernel test robot
@ 2020-07-06 13:20   ` Cristian Marussi
  0 siblings, 0 replies; 3+ messages in thread
From: Cristian Marussi @ 2020-07-06 13:20 UTC (permalink / raw)
  To: kernel test robot
  Cc: kbuild-all, linux-kernel, linux-arm-kernel, sudeep.holla

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

_______________________________________________
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, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-30 16:25 [PATCH] firmware: arm_scmi: fix notifications macros argument reuse Cristian Marussi
2020-07-03 23:01 ` kernel test robot
2020-07-06 13:20   ` Cristian Marussi

Linux-ARM-Kernel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/0 linux-arm-kernel/git/0.git
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/1 linux-arm-kernel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-kernel linux-arm-kernel/ https://lore.kernel.org/linux-arm-kernel \
		linux-arm-kernel@lists.infradead.org
	public-inbox-index linux-arm-kernel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-arm-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git