[PATCH 1/2] firmware: arm_scmi: Remove unused local variables

[PATCH 1/2] firmware: arm_scmi: Remove unused local variables

[Cristian Marussi]

While using SCMI iterators helpers a few local automatic variables are
defined but then used only as input for sizeof operators.

cppcheck is fooled to complain about this with:

drivers/firmware/arm_scmi/sensors.c:341:48: warning: Variable 'msg' is not assigned a value. [unassignedVariable]
 struct scmi_msg_sensor_list_update_intervals *msg;

Even though this is an innocuos warning, since the uninitialized variable
is at the end never used in the reported cases, fix these occurences all
over SCMI stack to avoid keeping unneeded objects on the stack.

Cc: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/clock.c   |  5 ++---
 drivers/firmware/arm_scmi/perf.c    |  4 ++--
 drivers/firmware/arm_scmi/sensors.c | 12 ++++++------
 drivers/firmware/arm_scmi/voltage.c |  4 ++--
 4 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index 4d36a9a133d1..1a718faa4192 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -266,9 +266,7 @@ scmi_clock_describe_rates_get(const struct scmi_protocol_handle *ph, u32 clk_id,
 			      struct scmi_clock_info *clk)
 {
 	int ret;
-
 	void *iter;
-	struct scmi_msg_clock_describe_rates *msg;
 	struct scmi_iterator_ops ops = {
 		.prepare_message = iter_clk_describe_prepare_message,
 		.update_state = iter_clk_describe_update_state,
@@ -281,7 +279,8 @@ scmi_clock_describe_rates_get(const struct scmi_protocol_handle *ph, u32 clk_id,
 
 	iter = ph->hops->iter_response_init(ph, &ops, SCMI_MAX_NUM_RATES,
 					    CLOCK_DESCRIBE_RATES,
-					    sizeof(*msg), &cpriv);
+					    sizeof(struct scmi_msg_clock_describe_rates),
+					    &cpriv);
 	if (IS_ERR(iter))
 		return PTR_ERR(iter);
 
diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index 8f4051aca220..c1f701623058 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -332,7 +332,6 @@ scmi_perf_describe_levels_get(const struct scmi_protocol_handle *ph, u32 domain,
 {
 	int ret;
 	void *iter;
-	struct scmi_msg_perf_describe_levels *msg;
 	struct scmi_iterator_ops ops = {
 		.prepare_message = iter_perf_levels_prepare_message,
 		.update_state = iter_perf_levels_update_state,
@@ -345,7 +344,8 @@ scmi_perf_describe_levels_get(const struct scmi_protocol_handle *ph, u32 domain,
 
 	iter = ph->hops->iter_response_init(ph, &ops, MAX_OPPS,
 					    PERF_DESCRIBE_LEVELS,
-					    sizeof(*msg), &ppriv);
+					    sizeof(struct scmi_msg_perf_describe_levels),
+					    &ppriv);
 	if (IS_ERR(iter))
 		return PTR_ERR(iter);
 
diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
index 21e0ce89b153..75b9d716508e 100644
--- a/drivers/firmware/arm_scmi/sensors.c
+++ b/drivers/firmware/arm_scmi/sensors.c
@@ -338,7 +338,6 @@ static int scmi_sensor_update_intervals(const struct scmi_protocol_handle *ph,
 					struct scmi_sensor_info *s)
 {
 	void *iter;
-	struct scmi_msg_sensor_list_update_intervals *msg;
 	struct scmi_iterator_ops ops = {
 		.prepare_message = iter_intervals_prepare_message,
 		.update_state = iter_intervals_update_state,
@@ -351,7 +350,8 @@ static int scmi_sensor_update_intervals(const struct scmi_protocol_handle *ph,
 
 	iter = ph->hops->iter_response_init(ph, &ops, s->intervals.count,
 					    SENSOR_LIST_UPDATE_INTERVALS,
-					    sizeof(*msg), &upriv);
+					    sizeof(struct scmi_msg_sensor_list_update_intervals),
+					    &upriv);
 	if (IS_ERR(iter))
 		return PTR_ERR(iter);
 
@@ -459,7 +459,6 @@ scmi_sensor_axis_extended_names_get(const struct scmi_protocol_handle *ph,
 				    struct scmi_sensor_info *s)
 {
 	void *iter;
-	struct scmi_msg_sensor_axis_description_get *msg;
 	struct scmi_iterator_ops ops = {
 		.prepare_message = iter_axes_desc_prepare_message,
 		.update_state = iter_axes_extended_name_update_state,
@@ -468,7 +467,8 @@ scmi_sensor_axis_extended_names_get(const struct scmi_protocol_handle *ph,
 
 	iter = ph->hops->iter_response_init(ph, &ops, s->num_axis,
 					    SENSOR_AXIS_NAME_GET,
-					    sizeof(*msg), s);
+					    sizeof(struct scmi_msg_sensor_axis_description_get),
+					    s);
 	if (IS_ERR(iter))
 		return PTR_ERR(iter);
 
@@ -481,7 +481,6 @@ static int scmi_sensor_axis_description(const struct scmi_protocol_handle *ph,
 {
 	int ret;
 	void *iter;
-	struct scmi_msg_sensor_axis_description_get *msg;
 	struct scmi_iterator_ops ops = {
 		.prepare_message = iter_axes_desc_prepare_message,
 		.update_state = iter_axes_desc_update_state,
@@ -495,7 +494,8 @@ static int scmi_sensor_axis_description(const struct scmi_protocol_handle *ph,
 
 	iter = ph->hops->iter_response_init(ph, &ops, s->num_axis,
 					    SENSOR_AXIS_DESCRIPTION_GET,
-					    sizeof(*msg), s);
+					    sizeof(struct scmi_msg_sensor_axis_description_get),
+					    s);
 	if (IS_ERR(iter))
 		return PTR_ERR(iter);
 
diff --git a/drivers/firmware/arm_scmi/voltage.c b/drivers/firmware/arm_scmi/voltage.c
index 9d195d8719ab..97df6d3dd131 100644
--- a/drivers/firmware/arm_scmi/voltage.c
+++ b/drivers/firmware/arm_scmi/voltage.c
@@ -180,7 +180,6 @@ static int scmi_voltage_levels_get(const struct scmi_protocol_handle *ph,
 {
 	int ret;
 	void *iter;
-	struct scmi_msg_cmd_describe_levels *msg;
 	struct scmi_iterator_ops ops = {
 		.prepare_message = iter_volt_levels_prepare_message,
 		.update_state = iter_volt_levels_update_state,
@@ -193,7 +192,8 @@ static int scmi_voltage_levels_get(const struct scmi_protocol_handle *ph,
 
 	iter = ph->hops->iter_response_init(ph, &ops, v->num_levels,
 					    VOLTAGE_DESCRIBE_LEVELS,
-					    sizeof(*msg), &vpriv);
+					    sizeof(struct scmi_msg_cmd_describe_levels),
+					    &vpriv);
 	if (IS_ERR(iter))
 		return PTR_ERR(iter);
 
-- 
2.32.0

[PATCH 2/2] firmware: arm_scmi: Fix pointers arithmetic in Base protocol

[Cristian Marussi]

Fix a possible undefined behaviour involving pointer arithmetic in Base
protocol scmi_base_implementation_list_get().

cppcheck complains with:

drivers/firmware/arm_scmi/base.c:190:19: warning: 't->rx.buf' is of type 'void *'. When using void pointers in calculations, the behaviour is undefined. [arithOperationsOnVoidPointer]
 list = t->rx.buf + sizeof(*num_ret);

Fixes: b6f20ff8bd94 ("firmware: arm_scmi: add common infrastructure and support for base protocol")
Cc: Sudeep Holla <sudeep.holla@arm.com>
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/base.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c
index 20fba7370f4e..6d6214d9e68c 100644
--- a/drivers/firmware/arm_scmi/base.c
+++ b/drivers/firmware/arm_scmi/base.c
@@ -187,7 +187,7 @@ scmi_base_implementation_list_get(const struct scmi_protocol_handle *ph,
 
 	num_skip = t->tx.buf;
 	num_ret = t->rx.buf;
-	list = t->rx.buf + sizeof(*num_ret);
+	list = ((u8 *)t->rx.buf) + sizeof(*num_ret);
 
 	do {
 		size_t real_list_sz;
-- 
2.32.0

Re: [PATCH 2/2] firmware: arm_scmi: Fix pointers arithmetic in Base protocol

[Robin Murphy]

On 2022-05-30 12:52, Cristian Marussi wrote:
> Fix a possible undefined behaviour involving pointer arithmetic in Base
> protocol scmi_base_implementation_list_get().
> 
> cppcheck complains with:
> 
> drivers/firmware/arm_scmi/base.c:190:19: warning: 't->rx.buf' is of type 'void *'. When using void pointers in calculations, the behaviour is undefined. [arithOperationsOnVoidPointer]
>   list = t->rx.buf + sizeof(*num_ret);

Except we use GNU C, where it is well-defined[1]. We use void pointer 
arithmetic *all over* Linux, so there really isn't any valid argument 
that it could be problematic. If this was a common SCMI library intended 
to be portable then the patch would seem more reasonable, but in 
Linux-specific driver code it's just pointless churn.

Cheers,
Robin.

[1] https://gcc.gnu.org/onlinedocs/gcc/Pointer-Arith.html

> Fixes: b6f20ff8bd94 ("firmware: arm_scmi: add common infrastructure and support for base protocol")
> Cc: Sudeep Holla <sudeep.holla@arm.com>
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
>   drivers/firmware/arm_scmi/base.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c
> index 20fba7370f4e..6d6214d9e68c 100644
> --- a/drivers/firmware/arm_scmi/base.c
> +++ b/drivers/firmware/arm_scmi/base.c
> @@ -187,7 +187,7 @@ scmi_base_implementation_list_get(const struct scmi_protocol_handle *ph,
>   
>   	num_skip = t->tx.buf;
>   	num_ret = t->rx.buf;
> -	list = t->rx.buf + sizeof(*num_ret);
> +	list = ((u8 *)t->rx.buf) + sizeof(*num_ret);
>   
>   	do {
>   		size_t real_list_sz;

Re: [PATCH 2/2] firmware: arm_scmi: Fix pointers arithmetic in Base protocol

[Cristian Marussi]

On Tue, May 31, 2022 at 12:02:53PM +0100, Robin Murphy wrote:
> On 2022-05-30 12:52, Cristian Marussi wrote:
> > Fix a possible undefined behaviour involving pointer arithmetic in Base
> > protocol scmi_base_implementation_list_get().
> > 
> > cppcheck complains with:
> > 
> > drivers/firmware/arm_scmi/base.c:190:19: warning: 't->rx.buf' is of type 'void *'. When using void pointers in calculations, the behaviour is undefined. [arithOperationsOnVoidPointer]
> >   list = t->rx.buf + sizeof(*num_ret);
> 
> Except we use GNU C, where it is well-defined[1]. We use void pointer
> arithmetic *all over* Linux, so there really isn't any valid argument that
> it could be problematic. If this was a common SCMI library intended to be
> portable then the patch would seem more reasonable, but in Linux-specific
> driver code it's just pointless churn.
> 

Hi Robin,

thanks for the correction, I'll drop this.

Thanks,
Cristian

> Cheers,
> Robin.
> 
> [1] https://gcc.gnu.org/onlinedocs/gcc/Pointer-Arith.html
> 
> > Fixes: b6f20ff8bd94 ("firmware: arm_scmi: add common infrastructure and support for base protocol")
> > Cc: Sudeep Holla <sudeep.holla@arm.com>
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > ---
> >   drivers/firmware/arm_scmi/base.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c
> > index 20fba7370f4e..6d6214d9e68c 100644
> > --- a/drivers/firmware/arm_scmi/base.c
> > +++ b/drivers/firmware/arm_scmi/base.c
> > @@ -187,7 +187,7 @@ scmi_base_implementation_list_get(const struct scmi_protocol_handle *ph,
> >   	num_skip = t->tx.buf;
> >   	num_ret = t->rx.buf;
> > -	list = t->rx.buf + sizeof(*num_ret);
> > +	list = ((u8 *)t->rx.buf) + sizeof(*num_ret);
> >   	do {
> >   		size_t real_list_sz;

Re: [PATCH 1/2] firmware: arm_scmi: Remove unused local variables

[Sudeep Holla]

On Mon, 30 May 2022 12:52:36 +0100, Cristian Marussi wrote:
> While using SCMI iterators helpers a few local automatic variables are
> defined but then used only as input for sizeof operators.
> 
> cppcheck is fooled to complain about this with:
> 
> drivers/firmware/arm_scmi/sensors.c:341:48: warning: Variable 'msg' is not assigned a value. [unassignedVariable]
>  struct scmi_msg_sensor_list_update_intervals *msg;
> 
> [...]

Applied to sudeep.holla/linux (for-next/scmi), thanks!

[1/2] firmware: arm_scmi: Remove unused local variables
      https://git.kernel.org/sudeep.holla/c/d0c94bef70

--
Regards,
Sudeep