On 31.07.2023 15:35, Nicola Vetrini wrote:
> Rule 5.3 has the following headline:
> "An identifier declared in an inner scope shall not hide an
> identifier declared in an outer scope"
>
> Local variables have been suitably renamed to address some violations
> of this rule:
> - s/cmp/c/ because it shadows the union declared at line 87.
> - s/nodes/numa_nodes/ shadows the static variable declared at line 18.
> - s/ctrl/controller/ because the homonymous function parameter is later
> read.
> - s/baud/baud_rate/ to avoid shadowing the enum constant defined
> at line 1391.
>
> No functional change.
>
> Signed-off-by: Nicola Vetrini <nicola.vetr...@bugseng.com>
> ---
> xen/common/compat/memory.c | 6 +++---
> xen/common/numa.c | 36 ++++++++++++++++++------------------
> xen/drivers/char/ehci-dbgp.c | 4 ++--
> xen/drivers/char/ns16550.c | 4 ++--
> 4 files changed, 25 insertions(+), 25 deletions(-)
This is an odd mix of files touched in a single patch. How about splitting
into two, one for common/ and one for drivers/?
> --- a/xen/common/compat/memory.c
> +++ b/xen/common/compat/memory.c
> @@ -321,12 +321,12 @@ int compat_memory_op(unsigned int cmd,
> XEN_GUEST_HANDLE_PARAM(void) compat)
>
> case XENMEM_remove_from_physmap:
> {
> - struct compat_remove_from_physmap cmp;
> + struct compat_remove_from_physmap c;
The intention of the outer scope cmp is to avoid such inner scope
ones then consuming extra stack space. This wants making part of the
union there.
> --- a/xen/common/numa.c
> +++ b/xen/common/numa.c
> @@ -382,7 +382,7 @@ static bool __init numa_process_nodes(paddr_t start,
> paddr_t end)
> * 0 if memnodmap[] too small (or shift too small)
> * -1 if node overlap or lost ram (shift too big)
> */
> -static int __init populate_memnodemap(const struct node *nodes,
> +static int __init populate_memnodemap(const struct node *numa_nodes,
> unsigned int numnodes, unsigned int
> shift,
> const nodeid_t *nodeids)
> {
> @@ -393,8 +393,8 @@ static int __init populate_memnodemap(const struct node
> *nodes,
>
> for ( i = 0; i < numnodes; i++ )
> {
> - unsigned long spdx = paddr_to_pdx(nodes[i].start);
> - unsigned long epdx = paddr_to_pdx(nodes[i].end - 1);
> + unsigned long spdx = paddr_to_pdx(numa_nodes[i].start);
> + unsigned long epdx = paddr_to_pdx(numa_nodes[i].end - 1);
>
> if ( spdx > epdx )
> continue;
> @@ -440,7 +440,7 @@ static int __init allocate_cachealigned_memnodemap(void)
> * The LSB of all start addresses in the node map is the value of the
> * maximum possible shift.
> */
> -static unsigned int __init extract_lsb_from_nodes(const struct node *nodes,
> +static unsigned int __init extract_lsb_from_nodes(const struct node
> *numa_nodes,
> nodeid_t numnodes,
> const nodeid_t *nodeids)
> {
> @@ -449,8 +449,8 @@ static unsigned int __init extract_lsb_from_nodes(const
> struct node *nodes,
>
> for ( i = 0; i < numnodes; i++ )
> {
> - unsigned long spdx = paddr_to_pdx(nodes[i].start);
> - unsigned long epdx = paddr_to_pdx(nodes[i].end - 1) + 1;
> + unsigned long spdx = paddr_to_pdx(numa_nodes[i].start);
> + unsigned long epdx = paddr_to_pdx(numa_nodes[i].end - 1) + 1;
>
> if ( spdx >= epdx )
> continue;
> @@ -475,10 +475,10 @@ static unsigned int __init extract_lsb_from_nodes(const
> struct node *nodes,
> return i;
> }
>
> -int __init compute_hash_shift(const struct node *nodes,
> +int __init compute_hash_shift(const struct node *numa_nodes,
> unsigned int numnodes, const nodeid_t *nodeids)
> {
> - unsigned int shift = extract_lsb_from_nodes(nodes, numnodes, nodeids);
> + unsigned int shift = extract_lsb_from_nodes(numa_nodes, numnodes,
> nodeids);
>
> if ( memnodemapsize <= ARRAY_SIZE(_memnodemap) )
> memnodemap = _memnodemap;
> @@ -487,7 +487,7 @@ int __init compute_hash_shift(const struct node *nodes,
>
> printk(KERN_DEBUG "NUMA: Using %u for the hash shift\n", shift);
>
> - if ( populate_memnodemap(nodes, numnodes, shift, nodeids) != 1 )
> + if ( populate_memnodemap(numa_nodes, numnodes, shift, nodeids) != 1 )
> {
> printk(KERN_INFO "Your memory is not aligned you need to "
> "rebuild your hypervisor with a bigger NODEMAPSIZE "
> @@ -541,7 +541,7 @@ static int __init numa_emulation(unsigned long start_pfn,
> {
> int ret;
> unsigned int i;
> - struct node nodes[MAX_NUMNODES];
> + struct node numa_nodes[MAX_NUMNODES];
> uint64_t sz = pfn_to_paddr(end_pfn - start_pfn) / numa_fake;
>
> /* Kludge needed for the hash function */
> @@ -556,22 +556,22 @@ static int __init numa_emulation(unsigned long
> start_pfn,
> sz = x;
> }
>
> - memset(&nodes, 0, sizeof(nodes));
> + memset(&numa_nodes, 0, sizeof(numa_nodes));
> for ( i = 0; i < numa_fake; i++ )
> {
> - nodes[i].start = pfn_to_paddr(start_pfn) + i * sz;
> + numa_nodes[i].start = pfn_to_paddr(start_pfn) + i * sz;
>
> if ( i == numa_fake - 1 )
> - sz = pfn_to_paddr(end_pfn) - nodes[i].start;
> + sz = pfn_to_paddr(end_pfn) - numa_nodes[i].start;
>
> - nodes[i].end = nodes[i].start + sz;
> + numa_nodes[i].end = numa_nodes[i].start + sz;
> printk(KERN_INFO "Faking node %u at %"PRIx64"-%"PRIx64"
> (%"PRIu64"MB)\n",
> - i, nodes[i].start, nodes[i].end,
> - (nodes[i].end - nodes[i].start) >> 20);
> + i, numa_nodes[i].start, numa_nodes[i].end,
> + (numa_nodes[i].end - numa_nodes[i].start) >> 20);
> node_set_online(i);
> }
>
> - ret = compute_hash_shift(nodes, numa_fake, NULL);
> + ret = compute_hash_shift(numa_nodes, numa_fake, NULL);
> if ( ret < 0 )
> {
> printk(KERN_ERR "No NUMA hash function found. Emulation
> disabled.\n");
> @@ -580,7 +580,7 @@ static int __init numa_emulation(unsigned long start_pfn,
> memnode_shift = ret;
>
> for_each_online_node ( i )
> - setup_node_bootmem(i, nodes[i].start, nodes[i].end);
> + setup_node_bootmem(i, numa_nodes[i].start, numa_nodes[i].end);
>
> numa_init_array();
>
I think renaming the file-scope variable this way would be more logical
and less risky (the way you do it it's easy to miss one place without
the build breaking).
> --- a/xen/drivers/char/ehci-dbgp.c
> +++ b/xen/drivers/char/ehci-dbgp.c
> @@ -424,9 +424,9 @@ static void dbgp_issue_command(struct ehci_dbgp *dbgp,
> u32 ctrl,
> * checks to see if ACPI or some other initialization also
> * reset the EHCI debug port.
> */
> - u32 ctrl = readl(&dbgp->ehci_debug->control);
> + u32 controller = readl(&dbgp->ehci_debug->control);
Why "controller" when the field read is named "control"? Perhaps
easiest would be to drop the variablöe altogether: It's used exactly
once, ...
> - if ( ctrl & DBGP_ENABLED )
> + if ( controller & DBGP_ENABLED )
... here.
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -1473,7 +1473,7 @@ static enum __init serial_param_type get_token(char
> *token, char **value)
>
> static bool __init parse_positional(struct ns16550 *uart, char **str)
> {
> - int baud;
> + int baud_rate;
> const char *conf;
> char *name_val_pos;
>
> @@ -1504,7 +1504,7 @@ static bool __init parse_positional(struct ns16550
> *uart, char **str)
> uart->baud = BAUD_AUTO;
> conf += 4;
> }
> - else if ( (baud = simple_strtoul(conf, &conf, 10)) != 0 )
> + else if ( (baud_rate = simple_strtoul(conf, &conf, 10)) != 0 )
> uart->baud = baud;
So along the lines of the earlier remark on common/numa.c: Here you're
actively introducing a bug, by not also renaming the further use of the
variable. Please reconsider the name change.
Jan
