Re: [PATCH 0/2] code style exercise: Drivers folder samples

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.02.2025 11:21, Oleksandr Andrushchenko wrote:
> 1. Const string arrays reformatting
> In case the length of items change we might need to introduce a bigger
> change wrt new formatting of unaffected lines
> ==============================================================================
> 
> --- a/xen/drivers/acpi/tables.c
> +++ b/xen/drivers/acpi/tables.c
> @@ -38,10 +38,10 @@
> -static const char *__initdata
> -mps_inti_flags_polarity[] = { "dfl", "high", "res", "low" };
> -static const char *__initdata
> -mps_inti_flags_trigger[] = { "dfl", "edge", "res", "level" };
> +static const char *__initdata mps_inti_flags_polarity[] = { "dfl", "high",
> +                                                            "res", "low" };
> +static const char *__initdata mps_inti_flags_trigger[] = { "dfl", "edge", 
> "res",
> 
> --- a/xen/drivers/acpi/utilities/utglobal.c
> +++ b/xen/drivers/acpi/utilities/utglobal.c
>  static const char *const acpi_gbl_region_types[ACPI_NUM_PREDEFINED_REGIONS] 
> = {
> -     "SystemMemory",
> -     "SystemIO",
> -     "PCI_Config",
> -     "EmbeddedControl",
> -     "SMBus",
> -     "CMOS",
> -     "PCIBARTarget",
> -     "DataTable"
> +    "SystemMemory", "SystemIO", "PCI_Config",   "EmbeddedControl",
> +    "SMBus",        "CMOS",     "PCIBARTarget", "DataTable"
>  };

Why in the world would a tool need to touch anything like the two examples
above? My take is that the code is worse readability-wise afterwards.

> 2. Long strings in ptintk violations
> I filed an issue on printk format strings [5]
> ==============================================================================
> @@ -225,9 +231,9 @@ void __init acpi_table_print_madt_entry(struct 
> acpi_subtable_header *header)
>          printk(KERN_DEBUG PREFIX
> -                            "GIC Distributor (gic_id[0x%04x] 
> address[0x%"PRIx64"] gsi_base[%d])\n",
> -                            p->gic_id, p->base_address,
> -                            p->global_irq_base);
> +               "GIC Distributor (gic_id[0x%04x] address[0x%" PRIx64
> +               "] gsi_base[%d])\n",
> +               p->gic_id, p->base_address, p->global_irq_base);
> 
> @@ -629,12 +628,14 @@ acpi_parse_one_rmrr(struct acpi_dmar_header *header)
> -           printk(XENLOG_ERR VTDPREFIX
> -                  "Overlapping RMRRs [%"PRIx64",%"PRIx64"] and 
> [%"PRIx64",%"PRIx64"]\n",
> -                  rmrru->base_address, rmrru->end_address,
> -                  base_addr, end_addr);
> +            printk(XENLOG_ERR VTDPREFIX "Overlapping RMRRs [%" PRIx64
> +                                        ",%" PRIx64 "] and [%" PRIx64
> +                                        ",%" PRIx64 "]\n",
> +                   rmrru->base_address, rmrru->end_address, base_addr,
> +                   end_addr);

This yet more definitely needs avoiding. Seeing that e.g. Linux also
makes line length exceptions for format string literals, I would seem
pretty likely that there already is a control to leave such alone.

> 3. String concatenation after clang - needs manual work to fix
> ==============================================================================
> --- a/xen/drivers/acpi/apei/apei-base.c
> +++ b/xen/drivers/acpi/apei/apei-base.c
> @@ -171,16 +169,19 @@ int __apei_exec_run(struct apei_exec_context *ctx, u8 
> action,
>                  printk(KERN_WARNING
> -                               "Invalid action table, unknown instruction "
> -                               "type: %d\n", entry->instruction);
> +                       "Invalid action table, unknown instruction " "type: 
> %d\n",
> +                       entry->instruction);

Urgh. Why would it not join together the two literals?

> 4. Looks a bit weird, but correct
> ==============================================================================
> --- a/xen/drivers/acpi/apei/apei-io.c
> +++ b/xen/drivers/acpi/apei/apei-io.c
> @@ -80,13 +78,15 @@ static void __iomem *__init apei_range_map(paddr_t paddr, 
> unsigned long size)
> -       pg = ((((paddr + size -1) & PAGE_MASK)
> -                - (paddr & PAGE_MASK)) >> PAGE_SHIFT) + 1;
> +    pg = ((((paddr + size - 1) & PAGE_MASK) - (paddr & PAGE_MASK)) >>
> +          PAGE_SHIFT) +
> +         1;

It trying to squash as much on the 1st line as it can, does it really put
the lone "1;" at a separate line?

> 7. Parentheses for empty functions
> ==============================================================================
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -1311,7 +1307,9 @@ void panic(const char *fmt, ...)
> -static void cf_check suspend_steal_fn(const char *str, size_t nr) { }
> +static void cf_check suspend_steal_fn(const char *str, size_t nr)
> +{}

While not the end of the world, an option for keeping the braces on the
same line surely would be worthwhile for them to support in the tool?

> 8. Const struct reformatting is weird and inconsistent
> ==============================================================================
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -1050,135 +1055,93 @@ static const struct ns16550_config __initconst 
> uart_config[] =
>       .param = param_oxford,
>       },
>      /* Pericom PI7C9X7951 Uno UART */
> -    {
> -        .vendor_id = PCI_VENDOR_ID_PERICOM,
> +    { .vendor_id = PCI_VENDOR_ID_PERICOM,
>       .dev_id = 0x7951,
> -        .param = param_pericom_1port
> -    },
> +     .param = param_pericom_1port },

A matter of some control that needs flipping? Readability again suffers
here quite a bit, imo.

> 9. Define is longer than 80 chars
> ==============================================================================
> --- a/xen/drivers/cpufreq/cpufreq_ondemand.c
> +++ b/xen/drivers/cpufreq/cpufreq_ondemand.c
> @@ -27,10 +27,8 @@
> -#define MIN_STAT_SAMPLING_RATE                  \
> -    (MIN_SAMPLING_MILLISECS * MILLISECS(1))
> -#define MIN_SAMPLING_RATE                       \
> -    (def_sampling_rate / MIN_SAMPLING_RATE_RATIO)
> +#define MIN_STAT_SAMPLING_RATE               (MIN_SAMPLING_MILLISECS * 
> MILLISECS(1))
> +#define MIN_SAMPLING_RATE                    (def_sampling_rate / 
> MIN_SAMPLING_RATE_RATIO)

Oops. Transformed code violating a fundamental rule?

> 10. Union memebers require an empty line between them
> ==============================================================================
> --- a/xen/drivers/passthrough/amd/iommu-defs.h
> +++ b/xen/drivers/passthrough/amd/iommu-defs.h
> @@ -289,6 +289,7 @@ struct amd_iommu_dte {
>  
>  union amd_iommu_control {
>      uint64_t raw;
> +
>      struct {

This might be acceptable. It's a little wasteful for small unions, but
may be quite helpful for larger ones.

> 11. Multiline string alignment for at the first string, not for the function
> opening bracket. Depends on the macro before the string?
> ==============================================================================
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -748,18 +757,18 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>              printk(XENLOG_WARNING "SR-IOV device %pp has its virtual"
> -                   " functions already enabled (%04x)\n", &pdev->sbdf, ctrl);
> +                                  " functions already enabled (%04x)\n",
> +                   &pdev->sbdf, ctrl);

This kind of line wrapping wants manual adjustment up front then, imo:

            printk(XENLOG_WARNING
                   "SR-IOV device %pp has its virtual functions already enabled 
(%04x)\n",
                   &pdev->sbdf, ctrl);

Unless of course the tool can be convinced to do the transformations this
way in the first place.

> 11. const struct initializers are inconsistent
> I have filed a bug on clang-format for that [7]
> ==============================================================================
> 
> [snip]
> static const struct ns16550_config __initconst uart_config[] = {
> [snip]
>     /* OXPCIe200 1 Native UART  */
>     {
>      .vendor_id = PCI_VENDOR_ID_OXSEMI,
>      .dev_id = 0xc4cf,
>      .param = param_oxford,
>      },
>     /* Pericom PI7C9X7951 Uno UART */
>     { .vendor_id = PCI_VENDOR_ID_PERICOM,
>      .dev_id = 0x7951,
>      .param = param_pericom_1port },
> [snip]

Odd; related to point 8 I think.

Jan