[PATCH 0/8] xen: Remove unused-but-set variables

[PATCH 0/8] xen: Remove unused-but-set variables

[Michal Orzel]

Fix all the gcc findings related to unused-but-set variables when performing
Xen compilation using target allyesconfig on arm32 and arm64. This is done by
temporarily removing flag -Wno-unused-but-set-variable set in Config.mk.

This series acts as a prerequisite to get rid of this flag one day.

Michal Orzel (8):
  xen/arm: bootfdt.c: Remove unused-but-set variable
  efi/boot.c: Remove unused-but-set variable
  gnttab: Remove unused-but-set variable
  xen/arm: smmu.c: Remove unused-but-set variable
  xen/sched: Remove unused-but-set variable
  platforms/xgene: Make use of dt_device_get_address return value
  platforms/omap: Remove unused-but-set variable
  drivers/exynos4210: Remove unused-but-set variable

 xen/arch/arm/bootfdt.c               |  3 ---
 xen/arch/arm/platforms/omap5.c       |  3 +--
 xen/arch/arm/platforms/xgene-storm.c |  2 +-
 xen/common/efi/boot.c                |  4 ++--
 xen/common/grant_table.c             |  6 ++----
 xen/common/sched/core.c              |  3 +--
 xen/drivers/char/exynos4210-uart.c   | 10 ++++++----
 xen/drivers/passthrough/arm/smmu.c   |  3 +--
 8 files changed, 14 insertions(+), 20 deletions(-)

-- 
2.25.1

[PATCH 1/8] xen/arm: bootfdt.c: Remove unused-but-set variable

[Michal Orzel]

Function device_tree_node_compatible defines and sets a variable
mlen but does not make use of it. Remove this variable.

Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
 xen/arch/arm/bootfdt.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
index e318ef9603..29671c8df0 100644
--- a/xen/arch/arm/bootfdt.c
+++ b/xen/arch/arm/bootfdt.c
@@ -36,11 +36,8 @@ static bool __init device_tree_node_compatible(const void *fdt, int node,
                                                const char *match)
 {
     int len, l;
-    int mlen;
     const void *prop;
 
-    mlen = strlen(match);
-
     prop = fdt_getprop(fdt, node, "compatible", &len);
     if ( prop == NULL )
         return false;
-- 
2.25.1

[PATCH 2/8] efi/boot.c: Remove unused-but-set variable

[Michal Orzel]

Function efi_start defines and sets a variable size but does not
make use of it. Remove this variable.

Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
 xen/common/efi/boot.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index ac1b235372..a25e1d29f1 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -1226,9 +1226,9 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
     {
         EFI_FILE_HANDLE dir_handle;
         EFI_HANDLE gop_handle;
-        UINTN depth, cols, rows, size;
+        UINTN depth, cols, rows;
 
-        size = cols = rows = depth = 0;
+        cols = rows = depth = 0;
 
         if ( StdOut->QueryMode(StdOut, StdOut->Mode->Mode,
                                &cols, &rows) == EFI_SUCCESS )
-- 
2.25.1

[PATCH 3/8] gnttab: Remove unused-but-set variable

[Michal Orzel]

Function unmap_common_complete defines and sets a variable ld that is
later on passed to a macro gnttab_host_mapping_get_page_type. On arm
this macro does not make use of any arguments causing a compiler to
warn about unused-but-set variable (when -Wunused-but-set-variable is
enabled). Fix this by removing ld and directly passing current->domain
to gnttab_host_mapping_get_page_type.

Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
 xen/common/grant_table.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index febbe12eab..71b1107999 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1556,7 +1556,7 @@ unmap_common(
 static void
 unmap_common_complete(struct gnttab_unmap_common *op)
 {
-    struct domain *ld, *rd = op->rd;
+    struct domain *rd = op->rd;
     struct grant_table *rgt;
     struct active_grant_entry *act;
     grant_entry_header_t *sha;
@@ -1569,8 +1569,6 @@ unmap_common_complete(struct gnttab_unmap_common *op)
         return;
     }
 
-    ld = current->domain;
-
     rcu_lock_domain(rd);
     rgt = rd->grant_table;
 
@@ -1608,7 +1606,7 @@ unmap_common_complete(struct gnttab_unmap_common *op)
         if ( pg )
         {
             if ( gnttab_host_mapping_get_page_type(op->done & GNTMAP_readonly,
-                                                   ld, rd) )
+                                                   current->domain, rd) )
                 put_page_type(pg);
             put_page(pg);
         }
-- 
2.25.1

[PATCH 4/8] xen/arm: smmu.c: Remove unused-but-set variable

[Michal Orzel]

Function arm_smmu_init_context_bank defines and sets a variable
gr0_base but does not make use of it. Remove this variable.

Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
 xen/drivers/passthrough/arm/smmu.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 5cacb2dd99..c21c4f3ac0 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -1086,10 +1086,9 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain)
 	bool stage1;
 	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
 	struct arm_smmu_device *smmu = smmu_domain->smmu;
-	void __iomem *cb_base, *gr0_base, *gr1_base;
+	void __iomem *cb_base, *gr1_base;
 	paddr_t p2maddr;
 
-	gr0_base = ARM_SMMU_GR0(smmu);
 	gr1_base = ARM_SMMU_GR1(smmu);
 	stage1 = cfg->cbar != CBAR_TYPE_S2_TRANS;
 	cb_base = ARM_SMMU_CB_BASE(smmu) + ARM_SMMU_CB(smmu, cfg->cbndx);
-- 
2.25.1

[PATCH 5/8] xen/sched: Remove unused-but-set variable

[Michal Orzel]

Function schedule_cpu_add defines and sets a variable old_unit but
does not make use of it. Remove this variable.

Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
 xen/common/sched/core.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
index 19ab678181..8a8c25bbda 100644
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -3104,7 +3104,7 @@ int schedule_cpu_add(unsigned int cpu, struct cpupool *c)
     {
         const cpumask_t *mask;
         unsigned int cpu_iter, idx = 0;
-        struct sched_unit *old_unit, *master_unit;
+        struct sched_unit *master_unit;
         struct sched_resource *sr_old;
 
         /*
@@ -3128,7 +3128,6 @@ int schedule_cpu_add(unsigned int cpu, struct cpupool *c)
             if ( cpu == cpu_iter )
                 continue;
 
-            old_unit = idle_vcpu[cpu_iter]->sched_unit;
             sr_old = get_sched_res(cpu_iter);
             kill_timer(&sr_old->s_timer);
             idle_vcpu[cpu_iter]->sched_unit = master_unit;
-- 
2.25.1

[PATCH 6/8] platforms/xgene: Make use of dt_device_get_address return value

[Michal Orzel]

Currently function xgene_check_pirq_eoi assignes a return value of
dt_device_get_address to a variable res but does not make use of it.
Fix it by making use of res in a condition checking the result of a
call to dt_device_get_address instead of checking the address
stored in dbase.

Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
 xen/arch/arm/platforms/xgene-storm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
index fced4d7c2c..befd0c3c2d 100644
--- a/xen/arch/arm/platforms/xgene-storm.c
+++ b/xen/arch/arm/platforms/xgene-storm.c
@@ -51,7 +51,7 @@ static void __init xgene_check_pirq_eoi(void)
         panic("%s: Can not find interrupt controller node\n", __func__);
 
     res = dt_device_get_address(node, 0, &dbase, NULL);
-    if ( !dbase )
+    if ( res )
         panic("%s: Cannot find a valid address for the distributor\n", __func__);
 
     /*
-- 
2.25.1

[PATCH 7/8] platforms/omap: Remove unused-but-set variable

[Michal Orzel]

Function omap5_init_time defines and sets a variable den but does not
make use of it. Remove this variable.

Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
 xen/arch/arm/platforms/omap5.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/xen/arch/arm/platforms/omap5.c b/xen/arch/arm/platforms/omap5.c
index aee24e4d28..5cf424a23e 100644
--- a/xen/arch/arm/platforms/omap5.c
+++ b/xen/arch/arm/platforms/omap5.c
@@ -48,7 +48,7 @@ static int omap5_init_time(void)
     void __iomem *ckgen_prm_base;
     void __iomem *rt_ct_base;
     unsigned int sys_clksel;
-    unsigned int num, den, frac1, frac2;
+    unsigned int num, frac1, frac2;
 
     ckgen_prm_base = ioremap_nocache(OMAP5_CKGEN_PRM_BASE, 0x20);
     if ( !ckgen_prm_base )
@@ -78,7 +78,6 @@ static int omap5_init_time(void)
     }
 
     frac2 = readl(rt_ct_base + INCREMENTER_DENUMERATOR_RELOAD_OFFSET);
-    den = frac2 & ~NUMERATOR_DENUMERATOR_MASK;
     if ( num_den[sys_clksel][1] != num )
     {
         frac2 &= NUMERATOR_DENUMERATOR_MASK;
-- 
2.25.1

[PATCH 8/8] drivers/exynos4210: Remove unused-but-set variable

[Michal Orzel]

Function exynos4210_uart_init_preirq defines and sets a variable
divisor but does not make use of it. Remove the definition and comment
out the assignment as this function already has some TODOs.

Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
Commenting out a code is a bad practise as well as using TODOs.
However the only alternative would be to get rid of divisor variable
and TODO comments. I'm open for solutions.
---
 xen/drivers/char/exynos4210-uart.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/xen/drivers/char/exynos4210-uart.c b/xen/drivers/char/exynos4210-uart.c
index fa7dbc0391..43aaf02e18 100644
--- a/xen/drivers/char/exynos4210-uart.c
+++ b/xen/drivers/char/exynos4210-uart.c
@@ -101,7 +101,6 @@ static void exynos4210_uart_interrupt(int irq, void *data, struct cpu_user_regs
 static void __init exynos4210_uart_init_preirq(struct serial_port *port)
 {
     struct exynos4210_uart *uart = port->uart;
-    unsigned int divisor;
     uint32_t ulcon;
 
     /* reset, TX/RX disables */
@@ -113,9 +112,12 @@ static void __init exynos4210_uart_init_preirq(struct serial_port *port)
     /* Line control and baud-rate generator. */
     if ( uart->baud != BAUD_AUTO )
     {
-        /* Baud rate specified: program it into the divisor latch. */
-        divisor = ((uart->clock_hz) / (uart->baud)) - 1;
-        /* FIXME: will use a hacked divisor, assuming the src clock and bauds */
+        /*
+         * TODO: should be updated
+         * Baud rate specified: program it into the divisor latch.
+         * divisor = ((uart->clock_hz) / (uart->baud)) - 1;
+         * FIXME: will use a hacked divisor, assuming the src clock and bauds.
+         */
         exynos4210_write(uart, UFRACVAL, 53);
         exynos4210_write(uart, UBRDIV, 4);
     }
-- 
2.25.1

Re: [PATCH 2/8] efi/boot.c: Remove unused-but-set variable

[Jan Beulich]

On 27.04.2022 11:49, Michal Orzel wrote:
> Function efi_start defines and sets a variable size but does not
> make use of it. Remove this variable.
> 
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH 3/8] gnttab: Remove unused-but-set variable

[Jan Beulich]

On 27.04.2022 11:49, Michal Orzel wrote:
> Function unmap_common_complete defines and sets a variable ld that is
> later on passed to a macro gnttab_host_mapping_get_page_type. On arm
> this macro does not make use of any arguments causing a compiler to
> warn about unused-but-set variable (when -Wunused-but-set-variable is
> enabled). Fix this by removing ld and directly passing current->domain
> to gnttab_host_mapping_get_page_type.

I think we want to retain the ld / rd notation. Therefore I think it's
rather the Arm macro which wants adjusting to not leave this argument
unused.

Jan

Re: [PATCH 5/8] xen/sched: Remove unused-but-set variable

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 279 bytes --]

On 27.04.22 11:49, Michal Orzel wrote:
> Function schedule_cpu_add defines and sets a variable old_unit but
> does not make use of it. Remove this variable.
> 
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

Re: [PATCH 3/8] gnttab: Remove unused-but-set variable

[Michal Orzel]

Hi Jan,

On 27.04.2022 11:59, Jan Beulich wrote:
> On 27.04.2022 11:49, Michal Orzel wrote:
>> Function unmap_common_complete defines and sets a variable ld that is
>> later on passed to a macro gnttab_host_mapping_get_page_type. On arm
>> this macro does not make use of any arguments causing a compiler to
>> warn about unused-but-set variable (when -Wunused-but-set-variable is
>> enabled). Fix this by removing ld and directly passing current->domain
>> to gnttab_host_mapping_get_page_type.
> 
> I think we want to retain the ld / rd notation. Therefore I think it's
> rather the Arm macro which wants adjusting to not leave this argument
> unused.
> 
I would agree provided that the ld variable was used in more than one place.
As it is not, it does not seem very beneficial to keep a variable that is used
just in one place and stores the macro value.

When it comes to gnttab_host_mapping_get_page_type macro, on Arm it is defined as (0)
so modifying it seems to be a quite big overhead.

> Jan
> 

Cheers,
Michal

Re: [PATCH 3/8] gnttab: Remove unused-but-set variable

[Andrew Cooper]

On 27/04/2022 12:06, Michal Orzel wrote:
> Hi Jan,
>
> On 27.04.2022 11:59, Jan Beulich wrote:
>> On 27.04.2022 11:49, Michal Orzel wrote:
>>> Function unmap_common_complete defines and sets a variable ld that is
>>> later on passed to a macro gnttab_host_mapping_get_page_type. On arm
>>> this macro does not make use of any arguments causing a compiler to
>>> warn about unused-but-set variable (when -Wunused-but-set-variable is
>>> enabled). Fix this by removing ld and directly passing current->domain
>>> to gnttab_host_mapping_get_page_type.
>> I think we want to retain the ld / rd notation. Therefore I think it's
>> rather the Arm macro which wants adjusting to not leave this argument
>> unused.
>>
> I would agree provided that the ld variable was used in more than one place.
> As it is not, it does not seem very beneficial to keep a variable that is used
> just in one place and stores the macro value.
>
> When it comes to gnttab_host_mapping_get_page_type macro, on Arm it is defined as (0)
> so modifying it seems to be a quite big overhead.

diff --git a/xen/arch/arm/include/asm/grant_table.h
b/xen/arch/arm/include/asm/grant_table.h
index d31a4d6805d6..9f68c2a37eb6 100644
--- a/xen/arch/arm/include/asm/grant_table.h
+++ b/xen/arch/arm/include/asm/grant_table.h
@@ -31,10 +31,10 @@ static inline void gnttab_mark_dirty(struct domain
*d, mfn_t mfn)
 
 int create_grant_host_mapping(unsigned long gpaddr, mfn_t mfn,
                               unsigned int flags, unsigned int
cache_flags);
-#define gnttab_host_mapping_get_page_type(ro, ld, rd) (0)
+#define gnttab_host_mapping_get_page_type(ro, ld, rd) (ro, ld, rd, 0)
 int replace_grant_host_mapping(unsigned long gpaddr, mfn_t mfn,
                                unsigned long new_gpaddr, unsigned int
flags);
-#define gnttab_release_host_mappings(domain) 1
+#define gnttab_release_host_mappings(domain) (domain, 1)
 
 /*
  * The region used by Xen on the memory will never be mapped in DOM0

It's about parameter evaluation, not about adding extra code when compiled.

~Andrew

Re: [PATCH 3/8] gnttab: Remove unused-but-set variable

[Michal Orzel]

Hi Andrew,

On 27.04.2022 14:33, Andrew Cooper wrote:
> On 27/04/2022 12:06, Michal Orzel wrote:
>> Hi Jan,
>>
>> On 27.04.2022 11:59, Jan Beulich wrote:
>>> On 27.04.2022 11:49, Michal Orzel wrote:
>>>> Function unmap_common_complete defines and sets a variable ld that is
>>>> later on passed to a macro gnttab_host_mapping_get_page_type. On arm
>>>> this macro does not make use of any arguments causing a compiler to
>>>> warn about unused-but-set variable (when -Wunused-but-set-variable is
>>>> enabled). Fix this by removing ld and directly passing current->domain
>>>> to gnttab_host_mapping_get_page_type.
>>> I think we want to retain the ld / rd notation. Therefore I think it's
>>> rather the Arm macro which wants adjusting to not leave this argument
>>> unused.
>>>
>> I would agree provided that the ld variable was used in more than one place.
>> As it is not, it does not seem very beneficial to keep a variable that is used
>> just in one place and stores the macro value.
>>
>> When it comes to gnttab_host_mapping_get_page_type macro, on Arm it is defined as (0)
>> so modifying it seems to be a quite big overhead.
> 
> diff --git a/xen/arch/arm/include/asm/grant_table.h
> b/xen/arch/arm/include/asm/grant_table.h
> index d31a4d6805d6..9f68c2a37eb6 100644
> --- a/xen/arch/arm/include/asm/grant_table.h
> +++ b/xen/arch/arm/include/asm/grant_table.h
> @@ -31,10 +31,10 @@ static inline void gnttab_mark_dirty(struct domain
> *d, mfn_t mfn)
>  
>  int create_grant_host_mapping(unsigned long gpaddr, mfn_t mfn,
>                                unsigned int flags, unsigned int
> cache_flags);
> -#define gnttab_host_mapping_get_page_type(ro, ld, rd) (0)
> +#define gnttab_host_mapping_get_page_type(ro, ld, rd) (ro, ld, rd, 0)
>  int replace_grant_host_mapping(unsigned long gpaddr, mfn_t mfn,
>                                 unsigned long new_gpaddr, unsigned int
> flags);
> -#define gnttab_release_host_mappings(domain) 1
> +#define gnttab_release_host_mappings(domain) (domain, 1)
>  
>  /*
>   * The region used by Xen on the memory will never be mapped in DOM0
> 
> It's about parameter evaluation, not about adding extra code when compiled.
> 
You're right, thanks. I will do it your way in v2.

> ~Andrew

Cheers,
Michal

Re: [PATCH 1/8] xen/arm: bootfdt.c: Remove unused-but-set variable

[Julien Grall]

Hi Michal,

On 27/04/2022 10:49, Michal Orzel wrote:
> Function device_tree_node_compatible defines and sets a variable
> mlen but does not make use of it. Remove this variable.
> 
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>

Reviewed-by: Julien Grall <jgrall@amazon.com>

> ---
>   xen/arch/arm/bootfdt.c | 3 ---
>   1 file changed, 3 deletions(-)
> 
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index e318ef9603..29671c8df0 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -36,11 +36,8 @@ static bool __init device_tree_node_compatible(const void *fdt, int node,
>                                                  const char *match)
>   {
>       int len, l;
> -    int mlen;
>       const void *prop;
>   
> -    mlen = strlen(match);
> -
>       prop = fdt_getprop(fdt, node, "compatible", &len);
>       if ( prop == NULL )
>           return false;

Cheers,

-- 
Julien Grall

Re: [PATCH 4/8] xen/arm: smmu.c: Remove unused-but-set variable

[Julien Grall]

Hi Michal,

On 27/04/2022 10:49, Michal Orzel wrote:
> Function arm_smmu_init_context_bank defines and sets a variable
> gr0_base but does not make use of it. Remove this variable.
> 
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH 6/8] platforms/xgene: Make use of dt_device_get_address return value

[Julien Grall]

Hi,

On 27/04/2022 10:49, Michal Orzel wrote:
> Currently function xgene_check_pirq_eoi assignes a return value of
Typo: s/assignes/assigns/ also I think s/a return/the return/

> dt_device_get_address to a variable res but does not make use of it.
> Fix it by making use of res in a condition checking the result of a

Typo: s/a condition/the condition/ I think.

> call to dt_device_get_address instead of checking the address
> stored in dbase.
> 
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>

Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,

> ---
>   xen/arch/arm/platforms/xgene-storm.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c
> index fced4d7c2c..befd0c3c2d 100644
> --- a/xen/arch/arm/platforms/xgene-storm.c
> +++ b/xen/arch/arm/platforms/xgene-storm.c
> @@ -51,7 +51,7 @@ static void __init xgene_check_pirq_eoi(void)
>           panic("%s: Can not find interrupt controller node\n", __func__);
>   
>       res = dt_device_get_address(node, 0, &dbase, NULL);
> -    if ( !dbase )
> +    if ( res )
>           panic("%s: Cannot find a valid address for the distributor\n", __func__);
>   
>       /*

-- 
Julien Grall

Re: [PATCH 7/8] platforms/omap: Remove unused-but-set variable

[Julien Grall]

Hi,

On 27/04/2022 10:49, Michal Orzel wrote:
> Function omap5_init_time defines and sets a variable den but does not

s/a/the/ I think

> make use of it. Remove this variable.
> 
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>

Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH 8/8] drivers/exynos4210: Remove unused-but-set variable

[Julien Grall]

Hi,

On 27/04/2022 10:49, Michal Orzel wrote:
> Function exynos4210_uart_init_preirq defines and sets a variable
> divisor but does not make use of it. Remove the definition and comment
> out the assignment as this function already has some TODOs.
> 
> Signed-off-by: Michal Orzel <michal.orzel@arm.com>
> ---
> Commenting out a code is a bad practise as well as using TODOs.

I disagree, having TODOs in the code is useful to track issues that are 
not critical or necessary to update the support state.

> However the only alternative would be to get rid of divisor variable
> and TODO comments. I'm open for solutions.

I am not overly happy with commented code, but I prefer it over removing 
the TODOs comment as you wouldn't address them and the issues are not fixed.

So for this patch:

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH 5/8] xen/sched: Remove unused-but-set variable

[Dario Faggioli]

[-- Attachment #1: Type: text/plain, Size: 668 bytes --]

On Wed, 2022-04-27 at 12:06 +0200, Juergen Gross wrote:
> On 27.04.22 11:49, Michal Orzel wrote:
> > Function schedule_cpu_add defines and sets a variable old_unit but
> > does not make use of it. Remove this variable.
> > 
> > Signed-off-by: Michal Orzel <michal.orzel@arm.com>
> 
> Reviewed-by: Juergen Gross <jgross@suse.com>
>
Acked-by: Dario Faggioli <dfaggioli@suse.com>

Regards
-- 
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 8/8] drivers/exynos4210: Remove unused-but-set variable

[Stefano Stabellini]

On Wed, 27 Apr 2022, Julien Grall wrote:
> On 27/04/2022 10:49, Michal Orzel wrote:
> > Function exynos4210_uart_init_preirq defines and sets a variable
> > divisor but does not make use of it. Remove the definition and comment
> > out the assignment as this function already has some TODOs.
> > 
> > Signed-off-by: Michal Orzel <michal.orzel@arm.com>
> > ---
> > Commenting out a code is a bad practise as well as using TODOs.
> 
> I disagree, having TODOs in the code is useful to track issues that are not
> critical or necessary to update the support state.
> 
> > However the only alternative would be to get rid of divisor variable
> > and TODO comments. I'm open for solutions.
> 
> I am not overly happy with commented code, but I prefer it over removing the
> TODOs comment as you wouldn't address them and the issues are not fixed.
> 
> So for this patch:
> 
> Acked-by: Julien Grall <jgrall@amazon.com>

I share the same opinion. I committed all patches except for patch #3
that requires further discussions/changes.

Re: [PATCH 3/8] gnttab: Remove unused-but-set variable

[Michal Orzel]

Hi Andrew, Jan

On 27.04.2022 14:33, Andrew Cooper wrote:
> On 27/04/2022 12:06, Michal Orzel wrote:
>> Hi Jan,
>>
>> On 27.04.2022 11:59, Jan Beulich wrote:
>>> On 27.04.2022 11:49, Michal Orzel wrote:
>>>> Function unmap_common_complete defines and sets a variable ld that is
>>>> later on passed to a macro gnttab_host_mapping_get_page_type. On arm
>>>> this macro does not make use of any arguments causing a compiler to
>>>> warn about unused-but-set variable (when -Wunused-but-set-variable is
>>>> enabled). Fix this by removing ld and directly passing current->domain
>>>> to gnttab_host_mapping_get_page_type.
>>> I think we want to retain the ld / rd notation. Therefore I think it's
>>> rather the Arm macro which wants adjusting to not leave this argument
>>> unused.
>>>
>> I would agree provided that the ld variable was used in more than one place.
>> As it is not, it does not seem very beneficial to keep a variable that is used
>> just in one place and stores the macro value.
>>
>> When it comes to gnttab_host_mapping_get_page_type macro, on Arm it is defined as (0)
>> so modifying it seems to be a quite big overhead.
> 
> diff --git a/xen/arch/arm/include/asm/grant_table.h
> b/xen/arch/arm/include/asm/grant_table.h
> index d31a4d6805d6..9f68c2a37eb6 100644
> --- a/xen/arch/arm/include/asm/grant_table.h
> +++ b/xen/arch/arm/include/asm/grant_table.h
> @@ -31,10 +31,10 @@ static inline void gnttab_mark_dirty(struct domain
> *d, mfn_t mfn)
>  
>  int create_grant_host_mapping(unsigned long gpaddr, mfn_t mfn,
>                                unsigned int flags, unsigned int
> cache_flags);
> -#define gnttab_host_mapping_get_page_type(ro, ld, rd) (0)
> +#define gnttab_host_mapping_get_page_type(ro, ld, rd) (ro, ld, rd, 0)
>  int replace_grant_host_mapping(unsigned long gpaddr, mfn_t mfn,
>                                 unsigned long new_gpaddr, unsigned int
> flags);
> -#define gnttab_release_host_mappings(domain) 1
> +#define gnttab_release_host_mappings(domain) (domain, 1)
>  
>  /*
>   * The region used by Xen on the memory will never be mapped in DOM0
> 
> It's about parameter evaluation, not about adding extra code when compiled.
> 

Unfortunately, solution presented by Andrew does not work.
We will get the following error due to -Werror=unused-value:
"left-hand operand of comma expression has no effect"

If we want to keep this variable, how about using unused attribute?

struct domain *__maybe_unused ld

Cheers,
Michal

Re: [PATCH 3/8] gnttab: Remove unused-but-set variable

[Jan Beulich]

On 28.04.2022 09:16, Michal Orzel wrote:
> Hi Andrew, Jan
> 
> On 27.04.2022 14:33, Andrew Cooper wrote:
>> On 27/04/2022 12:06, Michal Orzel wrote:
>>> Hi Jan,
>>>
>>> On 27.04.2022 11:59, Jan Beulich wrote:
>>>> On 27.04.2022 11:49, Michal Orzel wrote:
>>>>> Function unmap_common_complete defines and sets a variable ld that is
>>>>> later on passed to a macro gnttab_host_mapping_get_page_type. On arm
>>>>> this macro does not make use of any arguments causing a compiler to
>>>>> warn about unused-but-set variable (when -Wunused-but-set-variable is
>>>>> enabled). Fix this by removing ld and directly passing current->domain
>>>>> to gnttab_host_mapping_get_page_type.
>>>> I think we want to retain the ld / rd notation. Therefore I think it's
>>>> rather the Arm macro which wants adjusting to not leave this argument
>>>> unused.
>>>>
>>> I would agree provided that the ld variable was used in more than one place.
>>> As it is not, it does not seem very beneficial to keep a variable that is used
>>> just in one place and stores the macro value.
>>>
>>> When it comes to gnttab_host_mapping_get_page_type macro, on Arm it is defined as (0)
>>> so modifying it seems to be a quite big overhead.
>>
>> diff --git a/xen/arch/arm/include/asm/grant_table.h
>> b/xen/arch/arm/include/asm/grant_table.h
>> index d31a4d6805d6..9f68c2a37eb6 100644
>> --- a/xen/arch/arm/include/asm/grant_table.h
>> +++ b/xen/arch/arm/include/asm/grant_table.h
>> @@ -31,10 +31,10 @@ static inline void gnttab_mark_dirty(struct domain
>> *d, mfn_t mfn)
>>  
>>  int create_grant_host_mapping(unsigned long gpaddr, mfn_t mfn,
>>                                unsigned int flags, unsigned int
>> cache_flags);
>> -#define gnttab_host_mapping_get_page_type(ro, ld, rd) (0)
>> +#define gnttab_host_mapping_get_page_type(ro, ld, rd) (ro, ld, rd, 0)
>>  int replace_grant_host_mapping(unsigned long gpaddr, mfn_t mfn,
>>                                 unsigned long new_gpaddr, unsigned int
>> flags);
>> -#define gnttab_release_host_mappings(domain) 1
>> +#define gnttab_release_host_mappings(domain) (domain, 1)
>>  
>>  /*
>>   * The region used by Xen on the memory will never be mapped in DOM0
>>
>> It's about parameter evaluation, not about adding extra code when compiled.
>>
> 
> Unfortunately, solution presented by Andrew does not work.
> We will get the following error due to -Werror=unused-value:
> "left-hand operand of comma expression has no effect"

Perhaps

#define gnttab_host_mapping_get_page_type(ro, ld, rd) \
    ((void)(ro), (void)(ld), (void)(rd), 0)

?

Jan

Re: [PATCH 3/8] gnttab: Remove unused-but-set variable

[Michal Orzel]

On 28.04.2022 09:27, Jan Beulich wrote:
>>> diff --git a/xen/arch/arm/include/asm/grant_table.h
>>> b/xen/arch/arm/include/asm/grant_table.h
>>> index d31a4d6805d6..9f68c2a37eb6 100644
>>> --- a/xen/arch/arm/include/asm/grant_table.h
>>> +++ b/xen/arch/arm/include/asm/grant_table.h
>>> @@ -31,10 +31,10 @@ static inline void gnttab_mark_dirty(struct domain
>>> *d, mfn_t mfn)
>>>  
>>>  int create_grant_host_mapping(unsigned long gpaddr, mfn_t mfn,
>>>                                unsigned int flags, unsigned int
>>> cache_flags);
>>> -#define gnttab_host_mapping_get_page_type(ro, ld, rd) (0)
>>> +#define gnttab_host_mapping_get_page_type(ro, ld, rd) (ro, ld, rd, 0)
>>>  int replace_grant_host_mapping(unsigned long gpaddr, mfn_t mfn,
>>>                                 unsigned long new_gpaddr, unsigned int
>>> flags);
>>> -#define gnttab_release_host_mappings(domain) 1
>>> +#define gnttab_release_host_mappings(domain) (domain, 1)
>>>  
>>>  /*
>>>   * The region used by Xen on the memory will never be mapped in DOM0
>>>
>>> It's about parameter evaluation, not about adding extra code when compiled.
>>>
>>
>> Unfortunately, solution presented by Andrew does not work.
>> We will get the following error due to -Werror=unused-value:
>> "left-hand operand of comma expression has no effect"
> 
> Perhaps
> 
> #define gnttab_host_mapping_get_page_type(ro, ld, rd) \
>     ((void)(ro), (void)(ld), (void)(rd), 0)
> 
> ?
I already tried that and it won't work producing the following:
"error: void value not ignored as it ought to be"

Michal

Re: [PATCH 3/8] gnttab: Remove unused-but-set variable

[Michal Orzel]

On 28.04.2022 09:32, Michal Orzel wrote:
> 
> 
> On 28.04.2022 09:27, Jan Beulich wrote:
>>>> diff --git a/xen/arch/arm/include/asm/grant_table.h
>>>> b/xen/arch/arm/include/asm/grant_table.h
>>>> index d31a4d6805d6..9f68c2a37eb6 100644
>>>> --- a/xen/arch/arm/include/asm/grant_table.h
>>>> +++ b/xen/arch/arm/include/asm/grant_table.h
>>>> @@ -31,10 +31,10 @@ static inline void gnttab_mark_dirty(struct domain
>>>> *d, mfn_t mfn)
>>>>  
>>>>  int create_grant_host_mapping(unsigned long gpaddr, mfn_t mfn,
>>>>                                unsigned int flags, unsigned int
>>>> cache_flags);
>>>> -#define gnttab_host_mapping_get_page_type(ro, ld, rd) (0)
>>>> +#define gnttab_host_mapping_get_page_type(ro, ld, rd) (ro, ld, rd, 0)
>>>>  int replace_grant_host_mapping(unsigned long gpaddr, mfn_t mfn,
>>>>                                 unsigned long new_gpaddr, unsigned int
>>>> flags);
>>>> -#define gnttab_release_host_mappings(domain) 1
>>>> +#define gnttab_release_host_mappings(domain) (domain, 1)
>>>>  
>>>>  /*
>>>>   * The region used by Xen on the memory will never be mapped in DOM0
>>>>
>>>> It's about parameter evaluation, not about adding extra code when compiled.
>>>>
>>>
>>> Unfortunately, solution presented by Andrew does not work.
>>> We will get the following error due to -Werror=unused-value:
>>> "left-hand operand of comma expression has no effect"
>>
>> Perhaps
>>
>> #define gnttab_host_mapping_get_page_type(ro, ld, rd) \
>>     ((void)(ro), (void)(ld), (void)(rd), 0)
>>
>> ?
> I already tried that and it won't work producing the following:
> "error: void value not ignored as it ought to be"
> 
> Michal

Sorry about that but I was wrong. Your solution does work.
I did not enclose the params in parentheses.

Michal