xen/drivers/acpi/tables.c | 809 ++++++++++++++++++++----------------- xen/drivers/char/ns16550.c | 761 +++++++++++++++++----------------- 2 files changed, 813 insertions(+), 757 deletions(-)
Hello, everybody!
As agreed before [1] I am sending a series to show two samples of the
formatting done with clang-format. The clang-format configuration can be
found at [2]. It already has some notes on previous discussions when
Luca presented his version of that configuration and which I used to
start from.
There are two diff files which show what happens in case the same is
applied to the whole xen/drivers directory:
- first one is the result of the "git diff" command, 1.2M [3]
- the second one is for "git diff --ignire-all-space", 600K [4]
This is not to limit the reviewers from seeing a bigger picture and not
only the files in this series.
N.B. xen/drivers uses tabs a lot, so this is no surprise the size
difference is big enough: roughly space conversion is half of the changes.
While reviewing the changes I have collected some of the unexpected things
done by clang-format and some interesting pieces. You can find those
below. For some of those I have filed an issue on clang-format and hope the
community will lead me in resolving those. Of course what I collected is
not the complete list of such changes, so I hope we can discuss missed
ones here.
From this exercise I can definitely tell that clang-format does help a
lot and has potential to be employed as a code formatting tool for Xen.
Of course it cannot be used as is now and will require discussions on the
Xen coding style and possibly submitting patches to clang-format to
satisfy those which cannot be handled by the tool now.
Stay safe,
Oleksandr Andrushchenko
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"
};
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);
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);
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;
5. Long line tables mess, think it can be manually reformated before applying clang-format
==============================================================================
--- a/xen/drivers/acpi/tables/tbfadt.c
+++ b/xen/drivers/acpi/tables/tbfadt.c
@@ -74,12 +74,12 @@ typedef struct acpi_fadt_info {
- ACPI_FADT_OFFSET(pm1a_event_block),
- ACPI_FADT_OFFSET(pm1_event_length), ACPI_FADT_REQUIRED},
+ ACPI_FADT_OFFSET(pm1a_event_block), ACPI_FADT_OFFSET(pm1_event_length),
+ ACPI_FADT_REQUIRED },
--- a/xen/drivers/acpi/utilities/utglobal.c
+++ b/xen/drivers/acpi/utilities/utglobal.c
@@ -97,71 +96,71 @@ struct acpi_bit_register_info acpi_gbl_bit_register_info[ACPI_NUM_BITREG] = {
/* Name Parent Register Register Bit Position Register Bit Mask */
/* ACPI_BITREG_TIMER_STATUS */ { ACPI_REGISTER_PM1_STATUS,
- ACPI_BITPOSITION_TIMER_STATUS,
- ACPI_BITMASK_TIMER_STATUS},
- /* ACPI_BITREG_BUS_MASTER_STATUS */ {ACPI_REGISTER_PM1_STATUS,
- ACPI_BITPOSITION_BUS_MASTER_STATUS,
+ ACPI_BITPOSITION_TIMER_STATUS, ACPI_BITMASK_TIMER_STATUS },
+ /* ACPI_BITREG_BUS_MASTER_STATUS */
+ { ACPI_REGISTER_PM1_STATUS, ACPI_BITPOSITION_BUS_MASTER_STATUS,
6. Macro blocks are formatted
==============================================================================
Grygorii mentioned this and I was sure it is properly handled, but
it is not. I have filed an issue on clang-format [6].
--- a/xen/drivers/char/cadence-uart.c
+++ b/xen/drivers/char/cadence-uart.c
@@ -189,16 +192,14 @@ static int __init cuart_init(struct dt_device_node *dev, const void *data)
DT_DEVICE_START(cuart, "Cadence UART", DEVICE_SERIAL)
- .dt_match = cuart_dt_match,
- .init = cuart_init,
+ .dt_match = cuart_dt_match, .init = cuart_init,
DT_DEVICE_END
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)
+{}
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 },
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)
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 {
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);
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]
[1] https://lists.xen.org/archives/html/xen-devel/2025-02/msg00430.html
[2] https://github.com/andr2000/xen/blob/clang_ml_drivers_v001/xen/.clang-format
[3] https://github.com/andr2000/xen/blob/clang_ml_drivers_v001/xen/drivers.patch
[4] https://github.com/andr2000/xen/blob/clang_ml_drivers_v001/xen/drivers_ignore_all_space.patch
[5] https://github.com/llvm/llvm-project/issues/127383
[6] https://github.com/llvm/llvm-project/issues/127381
[7] https://github.com/llvm/llvm-project/issues/127380
Oleksandr Andrushchenko (2):
code style: Format ns16550 driver
code style: Format ACPI tables
xen/drivers/acpi/tables.c | 809 ++++++++++++++++++++-----------------
xen/drivers/char/ns16550.c | 761 +++++++++++++++++-----------------
2 files changed, 813 insertions(+), 757 deletions(-)
--
2.25.1Hi Oleksandr,
>
> 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);
These kind of issues with line break could be adjusted using the right penalty coefficients in the
clang format config.
>
> 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)
> +{}
I think also this can be mitigated with penalty + a rule saying that empty function can have the brackets
on the same line.
Cheers,
Luca
On 16.02.25 12:21, Oleksandr Andrushchenko wrote:
> Hello, everybody!
>
> As agreed before [1] I am sending a series to show two samples of the
> formatting done with clang-format. The clang-format configuration can be
> found at [2]. It already has some notes on previous discussions when
> Luca presented his version of that configuration and which I used to
> start from.
>
> There are two diff files which show what happens in case the same is
> applied to the whole xen/drivers directory:
> - first one is the result of the "git diff" command, 1.2M [3]
> - the second one is for "git diff --ignire-all-space", 600K [4]
>
> This is not to limit the reviewers from seeing a bigger picture and not
> only the files in this series.
> N.B. xen/drivers uses tabs a lot, so this is no surprise the size
> difference is big enough: roughly space conversion is half of the changes.
>
> While reviewing the changes I have collected some of the unexpected things
> done by clang-format and some interesting pieces. You can find those
> below. For some of those I have filed an issue on clang-format and hope the
> community will lead me in resolving those. Of course what I collected is
> not the complete list of such changes, so I hope we can discuss missed
> ones here.
>
> From this exercise I can definitely tell that clang-format does help a
> lot and has potential to be employed as a code formatting tool for Xen.
> Of course it cannot be used as is now and will require discussions on the
> Xen coding style and possibly submitting patches to clang-format to
> satisfy those which cannot be handled by the tool now.
>
> Stay safe,
> Oleksandr Andrushchenko
>
> 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"
> };
>
> 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);
It doesn't understand properly things like "PRIx64" :(
Most of other items, I've mentioned already,
> Unhappy: it's probably "known" things - identification of complex defines and
> static struct/arrays declarations.
And for them, probably, no magic automation tools exist which will satisfy everybody as,
at least static definitions are usually formatted to achieve better readability
which is always subjective.
The tags /* clang-format off/clang-format on */ might be useful.
[..]
Honestly, It looks a bit strange that Xen community is considering batch automated code formatting,
For example Linux kernel cleanly rejected such approach.
Linux kernel docs "4.1.1. Coding style" section [1].
Another thing, checking the new code and accepting code style violations (if any) on per-patch basis.
[1] https://docs.kernel.org/process/4.Coding.html
BR,
-grygorii
Hello, Grygorii!
On 18.02.25 11:56, Grygorii Strashko wrote:
>
>
> On 16.02.25 12:21, Oleksandr Andrushchenko wrote:
>> Hello, everybody!
>>
>> As agreed before [1] I am sending a series to show two samples of the
>> formatting done with clang-format. The clang-format configuration can be
>> found at [2]. It already has some notes on previous discussions when
>> Luca presented his version of that configuration and which I used to
>> start from.
>>
>> There are two diff files which show what happens in case the same is
>> applied to the whole xen/drivers directory:
>> - first one is the result of the "git diff" command, 1.2M [3]
>> - the second one is for "git diff --ignire-all-space", 600K [4]
>>
>> This is not to limit the reviewers from seeing a bigger picture and not
>> only the files in this series.
>> N.B. xen/drivers uses tabs a lot, so this is no surprise the size
>> difference is big enough: roughly space conversion is half of the changes.
>>
>> While reviewing the changes I have collected some of the unexpected things
>> done by clang-format and some interesting pieces. You can find those
>> below. For some of those I have filed an issue on clang-format and hope the
>> community will lead me in resolving those. Of course what I collected is
>> not the complete list of such changes, so I hope we can discuss missed
>> ones here.
>>
>> From this exercise I can definitely tell that clang-format does help a
>> lot and has potential to be employed as a code formatting tool for Xen.
>> Of course it cannot be used as is now and will require discussions on the
>> Xen coding style and possibly submitting patches to clang-format to
>> satisfy those which cannot be handled by the tool now.
>>
>> Stay safe,
>> Oleksandr Andrushchenko
>>
>> 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"
>> };
>>
>> 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);
>
> It doesn't understand properly things like "PRIx64" :(
>
> Most of other items, I've mentioned already,
>> Unhappy: it's probably "known" things - identification of complex defines and
>> static struct/arrays declarations.
>
> And for them, probably, no magic automation tools exist which will satisfy everybody as,
> at least static definitions are usually formatted to achieve better readability
> which is always subjective.
Strongly agree
>
> The tags /* clang-format off/clang-format on */ might be useful.
>
Yes, I guess we will be forced to use those
> [..]
>
> Honestly, It looks a bit strange that Xen community is considering batch automated code formatting,
> For example Linux kernel cleanly rejected such approach.
> Linux kernel docs "4.1.1. Coding style" section [1].
>
> Another thing, checking the new code and accepting code style violations (if any) on per-patch basis.
The main difference, and it was brought by Jan before [1], the possible existence
of the three different code styles in a single patch: xen, libxl and Linux...
Not sure that such a case can be handled. Of course we may require that no such
patch should exist, but it does depend on the change being made.
Thus, no guarantee
>
> [1] https://docs.kernel.org/process/4.Coding.html
>
> BR,
> -grygorii
[1] https://old-list-archives.xen.org/archives/html/xen-devel/2025-02/msg00464.html
On Tue, Feb 18, 2025 at 11:56:48AM +0200, Grygorii Strashko wrote: > Honestly, It looks a bit strange that Xen community is considering batch automated code formatting, > For example Linux kernel cleanly rejected such approach. > Linux kernel docs "4.1.1. Coding style" section [1]. > > Another thing, checking the new code and accepting code style violations (if any) on per-patch basis. That would indeed be my preference, from what I've seen the clang-format based style could live together with the previous style, there doesn't seem to be any irreconcilable style differences that would prevent new and old code chunks from being adjacent. Thanks, Roger.
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
On Mon, 17 Feb 2025, Jan Beulich wrote:
> 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.
I think the output is acceptable: not necessarily better than before,
but also not significantly worse. To me, the main takeaway is that there
are many unavoidable but unnecessary changes.
On 18.02.2025 03:36, Stefano Stabellini wrote:
> On Mon, 17 Feb 2025, Jan Beulich wrote:
>> 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.
>
> I think the output is acceptable: not necessarily better than before,
> but also not significantly worse.
Hmm, for the change to xen/drivers/acpi/tables.c I wouldn't agree with this
statement. And for xen/drivers/acpi/utilities/utglobal.c remember that this
is code taken from ACPI CA, which we may better not re-format.
> To me, the main takeaway is that there
> are many unavoidable but unnecessary changes.
Interesting. I'd say it slightly differently: The main takeaway is that
there are many avoidable / unnecessary changes.
Jan
Hello, Jan, Stefano!
On 18.02.25 13:34, Jan Beulich wrote:
> On 18.02.2025 03:36, Stefano Stabellini wrote:
>> On Mon, 17 Feb 2025, Jan Beulich wrote:
>>> 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.
>> I think the output is acceptable: not necessarily better than before,
>> but also not significantly worse.
> Hmm, for the change to xen/drivers/acpi/tables.c I wouldn't agree with this
> statement. And for xen/drivers/acpi/utilities/utglobal.c remember that this
> is code taken from ACPI CA, which we may better not re-format.
We can use /* clang-format off */ constructs to protect those lines we
do not want to be touched by clang-format [1]. This is what Grygprii
mentioned in some other e-mail.
>
>> To me, the main takeaway is that there
>> are many unavoidable but unnecessary changes.
> Interesting. I'd say it slightly differently: The main takeaway is that
> there are many avoidable / unnecessary changes.
But at the end of the day it will allow using automatic formatting...
>
> Jan
Thank you,
Oleksandr
[1] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#disabling-formatting-on-a-piece-of-code
On 19.02.2025 13:43, Oleksandr Andrushchenko wrote:
> Hello, Jan, Stefano!
>
> On 18.02.25 13:34, Jan Beulich wrote:
>> On 18.02.2025 03:36, Stefano Stabellini wrote:
>>> On Mon, 17 Feb 2025, Jan Beulich wrote:
>>>> 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.
>>> I think the output is acceptable: not necessarily better than before,
>>> but also not significantly worse.
>> Hmm, for the change to xen/drivers/acpi/tables.c I wouldn't agree with this
>> statement. And for xen/drivers/acpi/utilities/utglobal.c remember that this
>> is code taken from ACPI CA, which we may better not re-format.
> We can use /* clang-format off */ constructs to protect those lines we
> do not want to be touched by clang-format [1]. This is what Grygprii
> mentioned in some other e-mail.
We have fall-through comments. We have SAF comments. Yet another flavor to
keep some external tool happy. If everyone else thinks this is a good idea,
I'm not intending to stand in the way. Yet I don't like this as a workaround.
Instead I think the tool's going too far.
Jan
Hello, Jan!
On 19.02.25 14:49, Jan Beulich wrote:
> On 19.02.2025 13:43, Oleksandr Andrushchenko wrote:
>> Hello, Jan, Stefano!
>>
>> On 18.02.25 13:34, Jan Beulich wrote:
>>> On 18.02.2025 03:36, Stefano Stabellini wrote:
>>>> On Mon, 17 Feb 2025, Jan Beulich wrote:
>>>>> 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.
>>>> I think the output is acceptable: not necessarily better than before,
>>>> but also not significantly worse.
>>> Hmm, for the change to xen/drivers/acpi/tables.c I wouldn't agree with this
>>> statement. And for xen/drivers/acpi/utilities/utglobal.c remember that this
>>> is code taken from ACPI CA, which we may better not re-format.
>> We can use /* clang-format off */ constructs to protect those lines we
>> do not want to be touched by clang-format [1]. This is what Grygprii
>> mentioned in some other e-mail.
> We have fall-through comments. We have SAF comments. Yet another flavor to
> keep some external tool happy. If everyone else thinks this is a good idea,
> I'm not intending to stand in the way. Yet I don't like this as a workaround.
> Instead I think the tool's going too far.
Yes, I do agree. But only if we talk about having an automated
code style check now (which is definitely the goal at some time).
Before that we could still use the tool to take all that good that
it does and manually prepare a set of patches to fix those
code style issues which we like.
>
> Jan
On Wed, 19 Feb 2025, Oleksandr Andrushchenko wrote: > Yes, I do agree. But only if we talk about having an automated > code style check now (which is definitely the goal at some time). > Before that we could still use the tool to take all that good that > it does and manually prepare a set of patches to fix those > code style issues which we like. Let's explore this option a bit more. Let's say that we write down our coding style in plain English by enhancing CODING_STYLE. This newer CODING_STYLE has also a corresponding clang-format configuration. Now, we cannot use clang-format to reformat everything because, as we are discovering with this example, clang-format is overzealous and changes too many things. Instead, we take "inspiration" from clang-format's output but we manually prepare a patch series to apply code style changes to Xen as the coding style today is not uniform across the code base and also not always conforming to CODING_STYLE. At this point, we have already made an improvement to CODING_STYLEe, and made the Xen coding style more uniform across the codebase. But do we also have an automated coding style checker that we can use? Can we use clang-format to check new patches coming in? Or would clang-format flag too many things as coding style errors? If it is flagging too many things as error, so we cannot use it as automated checker, is it still worth going through the exercise? Yes, we make some improvement we haven't reached the goal of having an automated code style checker.
Hello, Stefano! On 20.02.25 03:34, Stefano Stabellini wrote: > On Wed, 19 Feb 2025, Oleksandr Andrushchenko wrote: >> Yes, I do agree. But only if we talk about having an automated >> code style check now (which is definitely the goal at some time). >> Before that we could still use the tool to take all that good that >> it does and manually prepare a set of patches to fix those >> code style issues which we like. > Let's explore this option a bit more. Let's say that we write down our > coding style in plain English by enhancing CODING_STYLE. This newer > CODING_STYLE has also a corresponding clang-format configuration. > > Now, we cannot use clang-format to reformat everything because, as we > are discovering with this example, clang-format is overzealous and > changes too many things. Instead, we take "inspiration" from > clang-format's output but we manually prepare a patch series to apply > code style changes to Xen as the coding style today is not uniform > across the code base and also not always conforming to CODING_STYLE. > > At this point, we have already made an improvement to CODING_STYLEe, and > made the Xen coding style more uniform across the codebase. > > But do we also have an automated coding style checker that we can use? It really depends on what that coding style would look like and if the rules we impose are supported by clang-format and if we ready to change ourselves if they are not. Again, here we are trying to do what we already did, but in the opposite direction: "diff -p" didn't work as expected(?) and we have changed *our* coding style to *fit that tool*. So, are we ready to do the same for another? Funny, but I checked diffutils and they have a test case for that behavior of the "diff -p" that we are trying to avoid. So even if we provide a patch to diffutils we would need a good reasoning why their behavior is wrong and needs fixing. > Can we use clang-format to check new patches coming in? Yes, we can use git-clang-format for that. clang-format is flexible enough in its configuration. So, it can be used for checking patches with different coding styles by providing .clang-format files at any folder level, e.g. we may have something like (just to show an example): - xen/drivers: Linux style .clang-format - xen (rest): Xen style .clang-format - libxl: its own .clang-format - etc. We can also disable formatting for the entire folder if need be by putting a .clang-format with "DisableFormat: true" option in it. clang-format respects the nested configuration files So, to answer your question: I think we can use the tool to check incoming patches. > Or would > clang-format flag too many things as coding style errors? It really depends on what we decide: if we are ready to change our coding style Or if we just want to skip entire folders from formatting, etc. > > If it is flagging too many things as error, so we cannot use it as > automated checker, is it still worth going through the exercise? Yes, we > make some improvement we haven't reached the goal of having an automated > code style checker. My impression from all these conversations is that most of the community can see that there are lots of places where clang-format does the job right. At the same time there are places which we do not like. Some of those we don't like can be discussed though as some feel like "I personally like it" or "don't like", e.g. depend on one's perception. We would need to accept the fact that if an existing code sample does conform at the moment, but still clang-format may re-format that code as well. Just because it will try to improve the code. Or "improve" So, the bottom line would be: yes, this can turn into a series of patches which will improve the coding style and fix the obvious. And then we can see what is left when we try to automatically run .clang-format at that stage and decide. We can also wait for "Year, 2034. Coding style, clang-format-AI. Attempt 21" letter in the future. We can also claim that "the coding style is perfect as it is, handmade and no robots allowed". So, I would love to hear from the community what is the best route here. Thank you, Oleksandr
On 23.02.2025 08:42, Oleksandr Andrushchenko wrote: > On 20.02.25 03:34, Stefano Stabellini wrote: >> On Wed, 19 Feb 2025, Oleksandr Andrushchenko wrote: >>> Yes, I do agree. But only if we talk about having an automated >>> code style check now (which is definitely the goal at some time). >>> Before that we could still use the tool to take all that good that >>> it does and manually prepare a set of patches to fix those >>> code style issues which we like. >> Let's explore this option a bit more. Let's say that we write down our >> coding style in plain English by enhancing CODING_STYLE. This newer >> CODING_STYLE has also a corresponding clang-format configuration. >> >> Now, we cannot use clang-format to reformat everything because, as we >> are discovering with this example, clang-format is overzealous and >> changes too many things. Instead, we take "inspiration" from >> clang-format's output but we manually prepare a patch series to apply >> code style changes to Xen as the coding style today is not uniform >> across the code base and also not always conforming to CODING_STYLE. >> >> At this point, we have already made an improvement to CODING_STYLEe, and >> made the Xen coding style more uniform across the codebase. >> >> But do we also have an automated coding style checker that we can use? > It really depends on what that coding style would look like and > if the rules we impose are supported by clang-format and if we ready > to change ourselves if they are not. > Again, here we are trying to do what we already did, but in the opposite > direction: "diff -p" didn't work as expected(?) and we have changed > *our* coding style to *fit that tool*. So, are we ready to do the same for > another? With a small but relevant difference: "diff" is a tool that people have been using forever. >> Can we use clang-format to check new patches coming in? > Yes, we can use git-clang-format for that. clang-format is flexible enough > in its configuration. So, it can be used for checking patches with different > coding styles by providing .clang-format files at any folder level, e.g. we may > have something like (just to show an example): > - xen/drivers: Linux style .clang-format > - xen (rest): Xen style .clang-format > - libxl: its own .clang-format > - etc. > We can also disable formatting for the entire folder if need be by putting > a .clang-format with "DisableFormat: true" option in it. > clang-format respects the nested configuration files Folder granularity is likely far too coarse. > So, to answer your question: I think we can use the tool to check incoming > patches. I think the question was more aiming at: Can we have the tool check a patch for it to only introduce well-formed code, even if elsewhere in a file being touched there are instances of what the tool would re-format? Jan
Hello, Jan! On 24.02.25 12:55, Jan Beulich wrote: > On 23.02.2025 08:42, Oleksandr Andrushchenko wrote: >> On 20.02.25 03:34, Stefano Stabellini wrote: >>> On Wed, 19 Feb 2025, Oleksandr Andrushchenko wrote: >>>> Yes, I do agree. But only if we talk about having an automated >>>> code style check now (which is definitely the goal at some time). >>>> Before that we could still use the tool to take all that good that >>>> it does and manually prepare a set of patches to fix those >>>> code style issues which we like. >>> Let's explore this option a bit more. Let's say that we write down our >>> coding style in plain English by enhancing CODING_STYLE. This newer >>> CODING_STYLE has also a corresponding clang-format configuration. >>> >>> Now, we cannot use clang-format to reformat everything because, as we >>> are discovering with this example, clang-format is overzealous and >>> changes too many things. Instead, we take "inspiration" from >>> clang-format's output but we manually prepare a patch series to apply >>> code style changes to Xen as the coding style today is not uniform >>> across the code base and also not always conforming to CODING_STYLE. >>> >>> At this point, we have already made an improvement to CODING_STYLEe, and >>> made the Xen coding style more uniform across the codebase. >>> >>> But do we also have an automated coding style checker that we can use? >> It really depends on what that coding style would look like and >> if the rules we impose are supported by clang-format and if we ready >> to change ourselves if they are not. >> Again, here we are trying to do what we already did, but in the opposite >> direction: "diff -p" didn't work as expected(?) and we have changed >> *our* coding style to *fit that tool*. So, are we ready to do the same for >> another? > With a small but relevant difference: "diff" is a tool that people have been > using forever. This is true. It is also true that that respectful tool expects labels to use no indentation at the function scope. > >>> Can we use clang-format to check new patches coming in? >> Yes, we can use git-clang-format for that. clang-format is flexible enough >> in its configuration. So, it can be used for checking patches with different >> coding styles by providing .clang-format files at any folder level, e.g. we may >> have something like (just to show an example): >> - xen/drivers: Linux style .clang-format >> - xen (rest): Xen style .clang-format >> - libxl: its own .clang-format >> - etc. >> We can also disable formatting for the entire folder if need be by putting >> a .clang-format with "DisableFormat: true" option in it. >> clang-format respects the nested configuration files > Folder granularity is likely far too coarse. That was just an example of what we can do. > >> So, to answer your question: I think we can use the tool to check incoming >> patches. > I think the question was more aiming at: Can we have the tool check a patch > for it to only introduce well-formed code, even if elsewhere in a file being > touched there are instances of what the tool would re-format? I need to do some tests, but with git-clang-format and possible some pre-commit hook we can have formatting for the patch only. > > Jan Thank you, Oleksandr
On 16/02/2025 10:21 am, Oleksandr Andrushchenko wrote: > There are two diff files which show what happens in case the same is > applied to the whole xen/drivers directory: > - first one is the result of the "git diff" command, 1.2M [3] > - the second one is for "git diff --ignire-all-space", 600K [4] Please can you format everything, and put it on a branch somewhere, so people can browse. ~Andrew
Hello, Roger! Please find the branch with all the conversions [1]. Unfortunately I cannot provide a branch as seen with diff --ignore-all-space as such a patch will not simply apply. Stay safe, Oleksandr Andrushchenko On 16.02.25 13:58, Andrew Cooper wrote: > On 16/02/2025 10:21 am, Oleksandr Andrushchenko wrote: >> There are two diff files which show what happens in case the same is >> applied to the whole xen/drivers directory: >> - first one is the result of the "git diff" command, 1.2M [3] >> - the second one is for "git diff --ignire-all-space", 600K [4] > Please can you format everything, and put it on a branch somewhere, so > people can browse. > > ~Andrew [1] https://github.com/andr2000/xen/tree/clang_ml_drivers_v002_diff
On 16/02/2025 5:11 pm, Oleksandr Andrushchenko wrote: > Hello, Roger! > > Please find the branch with all the conversions [1]. > Unfortunately I cannot provide a branch as seen with > diff --ignore-all-space as such a patch will not simply apply. > > Stay safe, > Oleksandr Andrushchenko > > On 16.02.25 13:58, Andrew Cooper wrote: >> On 16/02/2025 10:21 am, Oleksandr Andrushchenko wrote: >>> There are two diff files which show what happens in case the same is >>> applied to the whole xen/drivers directory: >>> - first one is the result of the "git diff" command, 1.2M [3] >>> - the second one is for "git diff --ignire-all-space", 600K [4] >> Please can you format everything, and put it on a branch somewhere, so >> people can browse. >> >> ~Andrew > [1] https://github.com/andr2000/xen/tree/clang_ml_drivers_v002_diff That appears to only be drivers/ Please do *everything*. I want to see what this does to files I consider to be pretty clean Xen-style. ~Andrew
Hello, Andrew! On 19.02.25 14:49, Andrew Cooper wrote: > On 16/02/2025 5:11 pm, Oleksandr Andrushchenko wrote: >> Hello, Roger! >> >> Please find the branch with all the conversions [1]. >> Unfortunately I cannot provide a branch as seen with >> diff --ignore-all-space as such a patch will not simply apply. >> >> Stay safe, >> Oleksandr Andrushchenko >> >> On 16.02.25 13:58, Andrew Cooper wrote: >>> On 16/02/2025 10:21 am, Oleksandr Andrushchenko wrote: >>>> There are two diff files which show what happens in case the same is >>>> applied to the whole xen/drivers directory: >>>> - first one is the result of the "git diff" command, 1.2M [3] >>>> - the second one is for "git diff --ignire-all-space", 600K [4] >>> Please can you format everything, and put it on a branch somewhere, so >>> people can browse. >>> >>> ~Andrew >> [1] https://github.com/andr2000/xen/tree/clang_ml_drivers_v002_diff > That appears to only be drivers/ I thought that was the agreement, so we start from the drivers first > > Please do *everything*. I want to see what this does to files I > consider to be pretty clean Xen-style. Sure > > ~Andrew > Thank you, Oleksandr
On 19.02.25 14:51, Oleksandr Andrushchenko wrote: > Hello, Andrew! > > On 19.02.25 14:49, Andrew Cooper wrote: >> On 16/02/2025 5:11 pm, Oleksandr Andrushchenko wrote: >>> Hello, Roger! >>> >>> Please find the branch with all the conversions [1]. >>> Unfortunately I cannot provide a branch as seen with >>> diff --ignore-all-space as such a patch will not simply apply. >>> >>> Stay safe, >>> Oleksandr Andrushchenko >>> >>> On 16.02.25 13:58, Andrew Cooper wrote: >>>> On 16/02/2025 10:21 am, Oleksandr Andrushchenko wrote: >>>>> There are two diff files which show what happens in case the same is >>>>> applied to the whole xen/drivers directory: >>>>> - first one is the result of the "git diff" command, 1.2M [3] >>>>> - the second one is for "git diff --ignire-all-space", 600K [4] >>>> Please can you format everything, and put it on a branch somewhere, so >>>> people can browse. >>>> >>>> ~Andrew >>> [1] https://github.com/andr2000/xen/tree/clang_ml_drivers_v002_diff force pushed to the same branch for all Xen >> That appears to only be drivers/ > I thought that was the agreement, so we start from the drivers first >> >> Please do *everything*. I want to see what this does to files I >> consider to be pretty clean Xen-style. > Sure >> >> ~Andrew >> > Thank you, > Oleksandr
On 19/02/2025 1:19 pm, Oleksandr Andrushchenko wrote: > > > On 19.02.25 14:51, Oleksandr Andrushchenko wrote: >> Hello, Andrew! >> >> On 19.02.25 14:49, Andrew Cooper wrote: >>> On 16/02/2025 5:11 pm, Oleksandr Andrushchenko wrote: >>>> Hello, Roger! >>>> >>>> Please find the branch with all the conversions [1]. >>>> Unfortunately I cannot provide a branch as seen with >>>> diff --ignore-all-space as such a patch will not simply apply. >>>> >>>> Stay safe, >>>> Oleksandr Andrushchenko >>>> >>>> On 16.02.25 13:58, Andrew Cooper wrote: >>>>> On 16/02/2025 10:21 am, Oleksandr Andrushchenko wrote: >>>>>> There are two diff files which show what happens in case the same is >>>>>> applied to the whole xen/drivers directory: >>>>>> - first one is the result of the "git diff" command, 1.2M [3] >>>>>> - the second one is for "git diff --ignire-all-space", 600K [4] >>>>> Please can you format everything, and put it on a branch >>>>> somewhere, so >>>>> people can browse. >>>>> >>>>> ~Andrew >>>> [1] https://github.com/andr2000/xen/tree/clang_ml_drivers_v002_diff > force pushed to the same branch for all Xen Thankyou. ~Andrew
Use .clang-format to format ns16550 driver.
Signed-off-by: Oleksandr Andrushchenko <andr2000@gmail.com>
---
xen/drivers/char/ns16550.c | 761 ++++++++++++++++++-------------------
1 file changed, 378 insertions(+), 383 deletions(-)
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index eaeb0e09d01e..0f605c98b036 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -14,7 +14,7 @@
* abstracted.
*/
#if defined(CONFIG_HAS_PCI) && defined(CONFIG_X86)
-# define NS16550_PCI
+#define NS16550_PCI
#endif
#include <xen/console.h>
@@ -43,12 +43,12 @@
static struct ns16550 {
int baud, clock_hz, data_bits, parity, stop_bits, fifo_size, irq;
- u64 io_base; /* I/O port or memory-mapped I/O address. */
+ u64 io_base; /* I/O port or memory-mapped I/O address. */
u64 io_size;
int reg_shift; /* Bits to shift register offset by */
int reg_width; /* Size of access to use, the registers
* themselves are still bytes */
- char __iomem *remapped_io_base; /* Remapped virtual address of MMIO. */
+ char __iomem *remapped_io_base; /* Remapped virtual address of MMIO. */
/* UART with IRQ line: interrupt-driven I/O. */
struct irqaction irqaction;
u8 lsr_mask;
@@ -63,8 +63,8 @@ static struct ns16550 {
bool dw_usr_bsy;
#ifdef NS16550_PCI
/* PCI card parameters. */
- bool pb_bdf_enable; /* if =1, pb-bdf effective, port behind bridge */
- bool ps_bdf_enable; /* if =1, ps_bdf effective, port on pci card */
+ bool pb_bdf_enable; /* if =1, pb-bdf effective, port behind bridge */
+ bool ps_bdf_enable; /* if =1, ps_bdf effective, port on pci card */
unsigned int pb_bdf[3]; /* pci bridge BDF */
unsigned int ps_bdf[3]; /* pci serial port BDF */
u32 bar;
@@ -80,6 +80,7 @@ static struct ns16550 {
struct ns16550_config {
u16 vendor_id;
u16 dev_id;
+
enum {
param_default, /* Must not be referenced by any table entry. */
param_trumanage,
@@ -227,7 +228,7 @@ static void cf_check __ns16550_poll(const struct cpu_user_regs *regs)
serial_rx_interrupt(port);
}
- if ( ( ns_read_reg(uart, UART_LSR) & uart->lsr_mask ) == uart->lsr_mask )
+ if ( (ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask )
serial_tx_interrupt(port);
out:
@@ -248,8 +249,9 @@ static int cf_check ns16550_tx_ready(struct serial_port *port)
if ( ns16550_ioport_invalid(uart) )
return -EIO;
- return ( (ns_read_reg(uart, UART_LSR) &
- uart->lsr_mask ) == uart->lsr_mask ) ? uart->fifo_size : 0;
+ return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask)
+ ? uart->fifo_size
+ : 0;
}
static void cf_check ns16550_putc(struct serial_port *port, char c)
@@ -263,7 +265,7 @@ static int cf_check ns16550_getc(struct serial_port *port, char *pc)
struct ns16550 *uart = port->uart;
if ( ns16550_ioport_invalid(uart) ||
- !(ns_read_reg(uart, UART_LSR) & UART_LSR_DR) )
+ !(ns_read_reg(uart, UART_LSR) & UART_LSR_DR) )
return 0;
*pc = ns_read_reg(uart, UART_RBR);
@@ -275,9 +277,10 @@ static void pci_serial_early_init(struct ns16550 *uart)
#ifdef NS16550_PCI
if ( uart->bar && uart->io_base >= 0x10000 )
{
- pci_conf_write16(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
- uart->ps_bdf[2]),
- PCI_COMMAND, PCI_COMMAND_MEMORY);
+ pci_conf_write16(
+ PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1], uart->ps_bdf[2]),
+ PCI_COMMAND,
+ PCI_COMMAND_MEMORY);
return;
}
@@ -285,26 +288,26 @@ static void pci_serial_early_init(struct ns16550 *uart)
return;
if ( uart->pb_bdf_enable )
- pci_conf_write16(PCI_SBDF(0, uart->pb_bdf[0], uart->pb_bdf[1],
- uart->pb_bdf[2]),
- PCI_IO_BASE,
- (uart->io_base & 0xF000) |
- ((uart->io_base & 0xF000) >> 8));
-
- pci_conf_write32(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
- uart->ps_bdf[2]),
- PCI_BASE_ADDRESS_0,
- uart->io_base | PCI_BASE_ADDRESS_SPACE_IO);
- pci_conf_write16(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
- uart->ps_bdf[2]),
- PCI_COMMAND, PCI_COMMAND_IO);
+ pci_conf_write16(
+ PCI_SBDF(0, uart->pb_bdf[0], uart->pb_bdf[1], uart->pb_bdf[2]),
+ PCI_IO_BASE,
+ (uart->io_base & 0xF000) | ((uart->io_base & 0xF000) >> 8));
+
+ pci_conf_write32(
+ PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1], uart->ps_bdf[2]),
+ PCI_BASE_ADDRESS_0,
+ uart->io_base | PCI_BASE_ADDRESS_SPACE_IO);
+ pci_conf_write16(
+ PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1], uart->ps_bdf[2]),
+ PCI_COMMAND,
+ PCI_COMMAND_IO);
#endif
}
static void ns16550_setup_preirq(struct ns16550 *uart)
{
unsigned char lcr;
- unsigned int divisor;
+ unsigned int divisor;
uart->intr_works = 0;
@@ -335,14 +338,14 @@ static void ns16550_setup_preirq(struct ns16550 *uart)
else
{
/* Baud rate already set: read it out from the divisor latch. */
- divisor = ns_read_reg(uart, UART_DLL);
+ divisor = ns_read_reg(uart, UART_DLL);
divisor |= ns_read_reg(uart, UART_DLM) << 8;
if ( divisor )
uart->baud = uart->clock_hz / (divisor << 4);
else
- printk(XENLOG_ERR
- "Automatic baud rate determination was requested,"
- " but a baud rate was not set up\n");
+ printk(
+ XENLOG_ERR
+ "Automatic baud rate determination was requested," " but a baud rate was not set up\n");
}
ns_write_reg(uart, UART_LCR, lcr);
@@ -350,8 +353,10 @@ static void ns16550_setup_preirq(struct ns16550 *uart)
ns_write_reg(uart, UART_MCR, UART_MCR_DTR | UART_MCR_RTS);
/* Enable and clear the FIFOs. Set a large trigger threshold. */
- ns_write_reg(uart, UART_FCR,
- UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX | UART_FCR_TRG14);
+ ns_write_reg(uart,
+ UART_FCR,
+ UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX |
+ UART_FCR_TRG14);
}
static void __init cf_check ns16550_init_preirq(struct serial_port *port)
@@ -399,7 +404,8 @@ static void ns16550_setup_postirq(struct ns16550 *uart)
{
/* Master interrupt enable; also keep DTR/RTS asserted. */
ns_write_reg(uart,
- UART_MCR, UART_MCR_OUT2 | UART_MCR_DTR | UART_MCR_RTS);
+ UART_MCR,
+ UART_MCR_OUT2 | UART_MCR_DTR | UART_MCR_RTS);
/* Enable receive interrupts. */
ns_write_reg(uart, UART_IER, UART_IER_ERDAI);
@@ -424,31 +430,37 @@ static void __init cf_check ns16550_init_postirq(struct serial_port *port)
/* Calculate time to fill RX FIFO and/or empty TX FIFO for polling. */
bits = uart->data_bits + uart->stop_bits + !!uart->parity;
- uart->timeout_ms = max_t(
- unsigned int, 1, (bits * uart->fifo_size * 1000) / uart->baud);
+ uart->timeout_ms =
+ max_t(unsigned int, 1, (bits * uart->fifo_size * 1000) / uart->baud);
#ifdef NS16550_PCI
if ( uart->bar || uart->ps_bdf_enable )
{
if ( uart->param && uart->param->mmio &&
- rangeset_add_range(mmio_ro_ranges, PFN_DOWN(uart->io_base),
+ rangeset_add_range(mmio_ro_ranges,
+ PFN_DOWN(uart->io_base),
PFN_UP(uart->io_base + uart->io_size) - 1) )
- printk(XENLOG_INFO "Error while adding MMIO range of device to mmio_ro_ranges\n");
+ printk(
+ XENLOG_INFO
+ "Error while adding MMIO range of device to mmio_ro_ranges\n");
- if ( pci_ro_device(0, uart->ps_bdf[0],
+ if ( pci_ro_device(0,
+ uart->ps_bdf[0],
PCI_DEVFN(uart->ps_bdf[1], uart->ps_bdf[2])) )
- printk(XENLOG_INFO "Could not mark config space of %02x:%02x.%u read-only.\n",
- uart->ps_bdf[0], uart->ps_bdf[1],
+ printk(XENLOG_INFO
+ "Could not mark config space of %02x:%02x.%u read-only.\n",
+ uart->ps_bdf[0],
+ uart->ps_bdf[1],
uart->ps_bdf[2]);
if ( uart->msi )
{
- struct msi_info msi = {
- .sbdf = PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
- uart->ps_bdf[2]),
- .irq = uart->irq,
- .entry_nr = 1
- };
+ struct msi_info msi = { .sbdf = PCI_SBDF(0,
+ uart->ps_bdf[0],
+ uart->ps_bdf[1],
+ uart->ps_bdf[2]),
+ .irq = uart->irq,
+ .entry_nr = 1 };
rc = uart->irq;
@@ -489,7 +501,10 @@ static void __init cf_check ns16550_init_postirq(struct serial_port *port)
if ( rc )
printk(XENLOG_WARNING
"MSI setup failed (%d) for %02x:%02x.%o\n",
- rc, uart->ps_bdf[0], uart->ps_bdf[1], uart->ps_bdf[2]);
+ rc,
+ uart->ps_bdf[0],
+ uart->ps_bdf[1],
+ uart->ps_bdf[2]);
}
}
#endif
@@ -497,8 +512,8 @@ static void __init cf_check ns16550_init_postirq(struct serial_port *port)
if ( uart->irq > 0 )
{
uart->irqaction.handler = ns16550_interrupt;
- uart->irqaction.name = "ns16550";
- uart->irqaction.dev_id = port;
+ uart->irqaction.name = "ns16550";
+ uart->irqaction.dev_id = port;
if ( (rc = setup_irq(uart->irq, 0, &uart->irqaction)) != 0 )
printk("ERROR: Failed to allocate ns16550 IRQ %d\n", uart->irq);
}
@@ -514,8 +529,9 @@ static void cf_check ns16550_suspend(struct serial_port *port)
#ifdef NS16550_PCI
if ( uart->bar )
- uart->cr = pci_conf_read16(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
- uart->ps_bdf[2]), PCI_COMMAND);
+ uart->cr = pci_conf_read16(
+ PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1], uart->ps_bdf[2]),
+ PCI_COMMAND);
#endif
}
@@ -526,19 +542,22 @@ static void _ns16550_resume(struct serial_port *port)
if ( uart->bar )
{
- pci_conf_write32(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
- uart->ps_bdf[2]),
- PCI_BASE_ADDRESS_0 + uart->bar_idx*4, uart->bar);
+ pci_conf_write32(
+ PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1], uart->ps_bdf[2]),
+ PCI_BASE_ADDRESS_0 + uart->bar_idx * 4,
+ uart->bar);
/* If 64 bit BAR, write higher 32 bits to BAR+4 */
if ( uart->bar & PCI_BASE_ADDRESS_MEM_TYPE_64 )
- pci_conf_write32(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
- uart->ps_bdf[2]),
- PCI_BASE_ADDRESS_0 + (uart->bar_idx+1)*4, uart->bar64);
-
- pci_conf_write16(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
- uart->ps_bdf[2]),
- PCI_COMMAND, uart->cr);
+ pci_conf_write32(
+ PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1], uart->ps_bdf[2]),
+ PCI_BASE_ADDRESS_0 + (uart->bar_idx + 1) * 4,
+ uart->bar64);
+
+ pci_conf_write16(
+ PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1], uart->ps_bdf[2]),
+ PCI_COMMAND,
+ uart->cr);
}
#endif
@@ -547,6 +566,7 @@ static void _ns16550_resume(struct serial_port *port)
}
static int delayed_resume_tries;
+
static void cf_check ns16550_delayed_resume(void *data)
{
struct serial_port *port = data;
@@ -634,32 +654,32 @@ static const struct vuart_info *ns16550_vuart_info(struct serial_port *port)
#endif
static struct uart_driver __read_mostly ns16550_driver = {
- .init_preirq = ns16550_init_preirq,
- .init_irq = ns16550_init_irq,
+ .init_preirq = ns16550_init_preirq,
+ .init_irq = ns16550_init_irq,
.init_postirq = ns16550_init_postirq,
- .endboot = ns16550_endboot,
- .suspend = ns16550_suspend,
- .resume = ns16550_resume,
- .tx_ready = ns16550_tx_ready,
- .putc = ns16550_putc,
- .getc = ns16550_getc,
- .irq = ns16550_irq,
- .start_tx = ns16550_start_tx,
- .stop_tx = ns16550_stop_tx,
+ .endboot = ns16550_endboot,
+ .suspend = ns16550_suspend,
+ .resume = ns16550_resume,
+ .tx_ready = ns16550_tx_ready,
+ .putc = ns16550_putc,
+ .getc = ns16550_getc,
+ .irq = ns16550_irq,
+ .start_tx = ns16550_start_tx,
+ .stop_tx = ns16550_stop_tx,
#ifdef CONFIG_ARM
- .vuart_info = ns16550_vuart_info,
+ .vuart_info = ns16550_vuart_info,
#endif
};
static void ns16550_init_common(struct ns16550 *uart)
{
- uart->clock_hz = UART_CLOCK_HZ;
+ uart->clock_hz = UART_CLOCK_HZ;
/* Default is no transmit FIFO. */
uart->fifo_size = 1;
/* Default lsr_mask = UART_LSR_THRE */
- uart->lsr_mask = UART_LSR_THRE;
+ uart->lsr_mask = UART_LSR_THRE;
}
#ifdef CONFIG_X86
@@ -670,13 +690,13 @@ static int __init parse_parity_char(int c)
{
case 'n':
return UART_PARITY_NONE;
- case 'o':
+ case 'o':
return UART_PARITY_ODD;
- case 'e':
+ case 'e':
return UART_PARITY_EVEN;
- case 'm':
+ case 'm':
return UART_PARITY_MARK;
- case 's':
+ case 's':
return UART_PARITY_SPACE;
}
return 0;
@@ -711,7 +731,7 @@ static int __init check_existence(struct ns16550 *uart)
* 16C754B) allow only to modify them if an EFR bit is set.
*/
scratch2 = ns_read_reg(uart, UART_IER) & 0x0f;
- ns_write_reg(uart,UART_IER, 0x0F);
+ ns_write_reg(uart, UART_IER, 0x0F);
scratch3 = ns_read_reg(uart, UART_IER) & 0x0f;
ns_write_reg(uart, UART_IER, scratch);
if ( (scratch2 != 0) || (scratch3 != 0x0F) )
@@ -849,336 +869,293 @@ static const struct ns16550_config_param __initconst uart_param[] = {
},
};
-static const struct ns16550_config __initconst uart_config[] =
-{
+static const struct ns16550_config __initconst uart_config[] = {
/* Broadcom TruManage device */
{
- .vendor_id = PCI_VENDOR_ID_BROADCOM,
- .dev_id = 0x160a,
- .param = param_trumanage,
- },
+ .vendor_id = PCI_VENDOR_ID_BROADCOM,
+ .dev_id = 0x160a,
+ .param = param_trumanage,
+ },
/* OXPCIe952 1 Native UART */
{
- .vendor_id = PCI_VENDOR_ID_OXSEMI,
- .dev_id = 0xc11b,
- .param = param_oxford,
- },
+ .vendor_id = PCI_VENDOR_ID_OXSEMI,
+ .dev_id = 0xc11b,
+ .param = param_oxford,
+ },
/* OXPCIe952 1 Native UART */
{
- .vendor_id = PCI_VENDOR_ID_OXSEMI,
- .dev_id = 0xc11f,
- .param = param_oxford,
- },
+ .vendor_id = PCI_VENDOR_ID_OXSEMI,
+ .dev_id = 0xc11f,
+ .param = param_oxford,
+ },
/* OXPCIe952 1 Native UART */
{
- .vendor_id = PCI_VENDOR_ID_OXSEMI,
- .dev_id = 0xc138,
- .param = param_oxford,
- },
+ .vendor_id = PCI_VENDOR_ID_OXSEMI,
+ .dev_id = 0xc138,
+ .param = param_oxford,
+ },
/* OXPCIe952 2 Native UART */
{
- .vendor_id = PCI_VENDOR_ID_OXSEMI,
- .dev_id = 0xc158,
- .param = param_oxford_2port,
- },
+ .vendor_id = PCI_VENDOR_ID_OXSEMI,
+ .dev_id = 0xc158,
+ .param = param_oxford_2port,
+ },
/* OXPCIe952 1 Native UART */
{
- .vendor_id = PCI_VENDOR_ID_OXSEMI,
- .dev_id = 0xc13d,
- .param = param_oxford,
- },
+ .vendor_id = PCI_VENDOR_ID_OXSEMI,
+ .dev_id = 0xc13d,
+ .param = param_oxford,
+ },
/* OXPCIe952 2 Native UART */
{
- .vendor_id = PCI_VENDOR_ID_OXSEMI,
- .dev_id = 0xc15d,
- .param = param_oxford_2port,
- },
+ .vendor_id = PCI_VENDOR_ID_OXSEMI,
+ .dev_id = 0xc15d,
+ .param = param_oxford_2port,
+ },
/* OXPCIe952 1 Native UART */
{
- .vendor_id = PCI_VENDOR_ID_OXSEMI,
- .dev_id = 0xc40b,
- .param = param_oxford,
- },
+ .vendor_id = PCI_VENDOR_ID_OXSEMI,
+ .dev_id = 0xc40b,
+ .param = param_oxford,
+ },
/* OXPCIe200 1 Native UART */
{
- .vendor_id = PCI_VENDOR_ID_OXSEMI,
- .dev_id = 0xc40f,
- .param = param_oxford,
- },
+ .vendor_id = PCI_VENDOR_ID_OXSEMI,
+ .dev_id = 0xc40f,
+ .param = param_oxford,
+ },
/* OXPCIe200 1 Native UART */
{
- .vendor_id = PCI_VENDOR_ID_OXSEMI,
- .dev_id = 0xc41b,
- .param = param_oxford,
- },
+ .vendor_id = PCI_VENDOR_ID_OXSEMI,
+ .dev_id = 0xc41b,
+ .param = param_oxford,
+ },
/* OXPCIe200 1 Native UART */
{
- .vendor_id = PCI_VENDOR_ID_OXSEMI,
- .dev_id = 0xc41f,
- .param = param_oxford,
- },
+ .vendor_id = PCI_VENDOR_ID_OXSEMI,
+ .dev_id = 0xc41f,
+ .param = param_oxford,
+ },
/* OXPCIe200 1 Native UART */
{
- .vendor_id = PCI_VENDOR_ID_OXSEMI,
- .dev_id = 0xc42b,
- .param = param_oxford,
- },
+ .vendor_id = PCI_VENDOR_ID_OXSEMI,
+ .dev_id = 0xc42b,
+ .param = param_oxford,
+ },
/* OXPCIe200 1 Native UART */
{
- .vendor_id = PCI_VENDOR_ID_OXSEMI,
- .dev_id = 0xc42f,
- .param = param_oxford,
- },
+ .vendor_id = PCI_VENDOR_ID_OXSEMI,
+ .dev_id = 0xc42f,
+ .param = param_oxford,
+ },
/* OXPCIe200 1 Native UART */
{
- .vendor_id = PCI_VENDOR_ID_OXSEMI,
- .dev_id = 0xc43b,
- .param = param_oxford,
- },
+ .vendor_id = PCI_VENDOR_ID_OXSEMI,
+ .dev_id = 0xc43b,
+ .param = param_oxford,
+ },
/* OXPCIe200 1 Native UART */
{
- .vendor_id = PCI_VENDOR_ID_OXSEMI,
- .dev_id = 0xc43f,
- .param = param_oxford,
- },
+ .vendor_id = PCI_VENDOR_ID_OXSEMI,
+ .dev_id = 0xc43f,
+ .param = param_oxford,
+ },
/* OXPCIe200 1 Native UART */
{
- .vendor_id = PCI_VENDOR_ID_OXSEMI,
- .dev_id = 0xc44b,
- .param = param_oxford,
- },
+ .vendor_id = PCI_VENDOR_ID_OXSEMI,
+ .dev_id = 0xc44b,
+ .param = param_oxford,
+ },
/* OXPCIe200 1 Native UART */
{
- .vendor_id = PCI_VENDOR_ID_OXSEMI,
- .dev_id = 0xc44f,
- .param = param_oxford,
- },
+ .vendor_id = PCI_VENDOR_ID_OXSEMI,
+ .dev_id = 0xc44f,
+ .param = param_oxford,
+ },
/* OXPCIe200 1 Native UART */
{
- .vendor_id = PCI_VENDOR_ID_OXSEMI,
- .dev_id = 0xc45b,
- .param = param_oxford,
- },
+ .vendor_id = PCI_VENDOR_ID_OXSEMI,
+ .dev_id = 0xc45b,
+ .param = param_oxford,
+ },
/* OXPCIe200 1 Native UART */
{
- .vendor_id = PCI_VENDOR_ID_OXSEMI,
- .dev_id = 0xc45f,
- .param = param_oxford,
- },
+ .vendor_id = PCI_VENDOR_ID_OXSEMI,
+ .dev_id = 0xc45f,
+ .param = param_oxford,
+ },
/* OXPCIe200 1 Native UART */
{
- .vendor_id = PCI_VENDOR_ID_OXSEMI,
- .dev_id = 0xc46b,
- .param = param_oxford,
- },
+ .vendor_id = PCI_VENDOR_ID_OXSEMI,
+ .dev_id = 0xc46b,
+ .param = param_oxford,
+ },
/* OXPCIe200 1 Native UART */
{
- .vendor_id = PCI_VENDOR_ID_OXSEMI,
- .dev_id = 0xc46f,
- .param = param_oxford,
- },
+ .vendor_id = PCI_VENDOR_ID_OXSEMI,
+ .dev_id = 0xc46f,
+ .param = param_oxford,
+ },
/* OXPCIe200 1 Native UART */
{
- .vendor_id = PCI_VENDOR_ID_OXSEMI,
- .dev_id = 0xc47b,
- .param = param_oxford,
- },
+ .vendor_id = PCI_VENDOR_ID_OXSEMI,
+ .dev_id = 0xc47b,
+ .param = param_oxford,
+ },
/* OXPCIe200 1 Native UART */
{
- .vendor_id = PCI_VENDOR_ID_OXSEMI,
- .dev_id = 0xc47f,
- .param = param_oxford,
- },
+ .vendor_id = PCI_VENDOR_ID_OXSEMI,
+ .dev_id = 0xc47f,
+ .param = param_oxford,
+ },
/* OXPCIe200 1 Native UART */
{
- .vendor_id = PCI_VENDOR_ID_OXSEMI,
- .dev_id = 0xc48b,
- .param = param_oxford,
- },
+ .vendor_id = PCI_VENDOR_ID_OXSEMI,
+ .dev_id = 0xc48b,
+ .param = param_oxford,
+ },
/* OXPCIe200 1 Native UART */
{
- .vendor_id = PCI_VENDOR_ID_OXSEMI,
- .dev_id = 0xc48f,
- .param = param_oxford,
- },
+ .vendor_id = PCI_VENDOR_ID_OXSEMI,
+ .dev_id = 0xc48f,
+ .param = param_oxford,
+ },
/* OXPCIe200 1 Native UART */
{
- .vendor_id = PCI_VENDOR_ID_OXSEMI,
- .dev_id = 0xc49b,
- .param = param_oxford,
- },
+ .vendor_id = PCI_VENDOR_ID_OXSEMI,
+ .dev_id = 0xc49b,
+ .param = param_oxford,
+ },
/* OXPCIe200 1 Native UART */
{
- .vendor_id = PCI_VENDOR_ID_OXSEMI,
- .dev_id = 0xc49f,
- .param = param_oxford,
- },
+ .vendor_id = PCI_VENDOR_ID_OXSEMI,
+ .dev_id = 0xc49f,
+ .param = param_oxford,
+ },
/* OXPCIe200 1 Native UART */
{
- .vendor_id = PCI_VENDOR_ID_OXSEMI,
- .dev_id = 0xc4ab,
- .param = param_oxford,
- },
+ .vendor_id = PCI_VENDOR_ID_OXSEMI,
+ .dev_id = 0xc4ab,
+ .param = param_oxford,
+ },
/* OXPCIe200 1 Native UART */
{
- .vendor_id = PCI_VENDOR_ID_OXSEMI,
- .dev_id = 0xc4af,
- .param = param_oxford,
- },
+ .vendor_id = PCI_VENDOR_ID_OXSEMI,
+ .dev_id = 0xc4af,
+ .param = param_oxford,
+ },
/* OXPCIe200 1 Native UART */
{
- .vendor_id = PCI_VENDOR_ID_OXSEMI,
- .dev_id = 0xc4bb,
- .param = param_oxford,
- },
+ .vendor_id = PCI_VENDOR_ID_OXSEMI,
+ .dev_id = 0xc4bb,
+ .param = param_oxford,
+ },
/* OXPCIe200 1 Native UART */
{
- .vendor_id = PCI_VENDOR_ID_OXSEMI,
- .dev_id = 0xc4bf,
- .param = param_oxford,
- },
+ .vendor_id = PCI_VENDOR_ID_OXSEMI,
+ .dev_id = 0xc4bf,
+ .param = param_oxford,
+ },
/* OXPCIe200 1 Native UART */
{
- .vendor_id = PCI_VENDOR_ID_OXSEMI,
- .dev_id = 0xc4cb,
- .param = param_oxford,
- },
+ .vendor_id = PCI_VENDOR_ID_OXSEMI,
+ .dev_id = 0xc4cb,
+ .param = param_oxford,
+ },
/* OXPCIe200 1 Native UART */
{
- .vendor_id = PCI_VENDOR_ID_OXSEMI,
- .dev_id = 0xc4cf,
- .param = param_oxford,
- },
+ .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
- },
+ { .vendor_id = PCI_VENDOR_ID_PERICOM,
+ .dev_id = 0x7951,
+ .param = param_pericom_1port },
/* Pericom PI7C9X7952 Duo UART */
- {
- .vendor_id = PCI_VENDOR_ID_PERICOM,
- .dev_id = 0x7952,
- .param = param_pericom_2port
- },
+ { .vendor_id = PCI_VENDOR_ID_PERICOM,
+ .dev_id = 0x7952,
+ .param = param_pericom_2port },
/* Pericom PI7C9X7954 Quad UART */
- {
- .vendor_id = PCI_VENDOR_ID_PERICOM,
- .dev_id = 0x7954,
- .param = param_pericom_4port
- },
+ { .vendor_id = PCI_VENDOR_ID_PERICOM,
+ .dev_id = 0x7954,
+ .param = param_pericom_4port },
/* Pericom PI7C9X7958 Octal UART */
- {
- .vendor_id = PCI_VENDOR_ID_PERICOM,
- .dev_id = 0x7958,
- .param = param_pericom_8port
- },
+ { .vendor_id = PCI_VENDOR_ID_PERICOM,
+ .dev_id = 0x7958,
+ .param = param_pericom_8port },
/* Exar Corp. XR17V352 Dual PCIe UART */
- {
- .vendor_id = PCI_VENDOR_ID_EXAR,
- .dev_id = 0x0352,
- .param = param_exar_xr17v352
- },
+ { .vendor_id = PCI_VENDOR_ID_EXAR,
+ .dev_id = 0x0352,
+ .param = param_exar_xr17v352 },
/* Exar Corp. XR17V354 Quad PCIe UART */
- {
- .vendor_id = PCI_VENDOR_ID_EXAR,
- .dev_id = 0x0354,
- .param = param_exar_xr17v354
- },
+ { .vendor_id = PCI_VENDOR_ID_EXAR,
+ .dev_id = 0x0354,
+ .param = param_exar_xr17v354 },
/* Exar Corp. XR17V358 Octal PCIe UART */
- {
- .vendor_id = PCI_VENDOR_ID_EXAR,
- .dev_id = 0x0358,
- .param = param_exar_xr17v358
- },
+ { .vendor_id = PCI_VENDOR_ID_EXAR,
+ .dev_id = 0x0358,
+ .param = param_exar_xr17v358 },
/* Intel Corp. TGL-LP LPSS PCI UART #0 */
- {
- .vendor_id = PCI_VENDOR_ID_INTEL,
- .dev_id = 0xa0a8,
- .param = param_intel_lpss
- },
+ { .vendor_id = PCI_VENDOR_ID_INTEL,
+ .dev_id = 0xa0a8,
+ .param = param_intel_lpss },
/* Intel Corp. TGL-LP LPSS PCI UART #1 */
- {
- .vendor_id = PCI_VENDOR_ID_INTEL,
- .dev_id = 0xa0a9,
- .param = param_intel_lpss
- },
+ { .vendor_id = PCI_VENDOR_ID_INTEL,
+ .dev_id = 0xa0a9,
+ .param = param_intel_lpss },
/* Intel Corp. TGL-LP LPSS PCI UART #2 */
- {
- .vendor_id = PCI_VENDOR_ID_INTEL,
- .dev_id = 0xa0c7,
- .param = param_intel_lpss
- },
+ { .vendor_id = PCI_VENDOR_ID_INTEL,
+ .dev_id = 0xa0c7,
+ .param = param_intel_lpss },
/* Intel Corp. TGL-H LPSS PCI UART #0 */
- {
- .vendor_id = PCI_VENDOR_ID_INTEL,
- .dev_id = 0x43a8,
- .param = param_intel_lpss
- },
+ { .vendor_id = PCI_VENDOR_ID_INTEL,
+ .dev_id = 0x43a8,
+ .param = param_intel_lpss },
/* Intel Corp. TGL-H LPSS PCI UART #1 */
- {
- .vendor_id = PCI_VENDOR_ID_INTEL,
- .dev_id = 0x43a9,
- .param = param_intel_lpss
- },
+ { .vendor_id = PCI_VENDOR_ID_INTEL,
+ .dev_id = 0x43a9,
+ .param = param_intel_lpss },
/* Intel Corp. TGL-H LPSS PCI UART #2 */
- {
- .vendor_id = PCI_VENDOR_ID_INTEL,
- .dev_id = 0x43a7,
- .param = param_intel_lpss
- },
+ { .vendor_id = PCI_VENDOR_ID_INTEL,
+ .dev_id = 0x43a7,
+ .param = param_intel_lpss },
/* Intel Corp. ADL-P LPSS PCI UART #0 */
- {
- .vendor_id = PCI_VENDOR_ID_INTEL,
- .dev_id = 0x51a8,
- .param = param_intel_lpss
- },
+ { .vendor_id = PCI_VENDOR_ID_INTEL,
+ .dev_id = 0x51a8,
+ .param = param_intel_lpss },
/* Intel Corp. ADL-P LPSS PCI UART #1 */
- {
- .vendor_id = PCI_VENDOR_ID_INTEL,
- .dev_id = 0x51a9,
- .param = param_intel_lpss
- },
+ { .vendor_id = PCI_VENDOR_ID_INTEL,
+ .dev_id = 0x51a9,
+ .param = param_intel_lpss },
/* Intel Corp. ADL-P LPSS PCI UART #2 */
- {
- .vendor_id = PCI_VENDOR_ID_INTEL,
- .dev_id = 0x51c7,
- .param = param_intel_lpss
- },
+ { .vendor_id = PCI_VENDOR_ID_INTEL,
+ .dev_id = 0x51c7,
+ .param = param_intel_lpss },
/* Intel Corp. ADL-P LPSS PCI UART #3 */
- {
- .vendor_id = PCI_VENDOR_ID_INTEL,
- .dev_id = 0x51da,
- .param = param_intel_lpss
- },
+ { .vendor_id = PCI_VENDOR_ID_INTEL,
+ .dev_id = 0x51da,
+ .param = param_intel_lpss },
/* Intel Corp. ADL-S LPSS PCI UART #0 */
- {
- .vendor_id = PCI_VENDOR_ID_INTEL,
- .dev_id = 0x7aa8,
- .param = param_intel_lpss
- },
+ { .vendor_id = PCI_VENDOR_ID_INTEL,
+ .dev_id = 0x7aa8,
+ .param = param_intel_lpss },
/* Intel Corp. ADL-S LPSS PCI UART #1 */
- {
- .vendor_id = PCI_VENDOR_ID_INTEL,
- .dev_id = 0x7aa9,
- .param = param_intel_lpss
- },
+ { .vendor_id = PCI_VENDOR_ID_INTEL,
+ .dev_id = 0x7aa9,
+ .param = param_intel_lpss },
/* Intel Corp. ADL-S LPSS PCI UART #2 */
- {
- .vendor_id = PCI_VENDOR_ID_INTEL,
- .dev_id = 0x7afe,
- .param = param_intel_lpss
- },
+ { .vendor_id = PCI_VENDOR_ID_INTEL,
+ .dev_id = 0x7afe,
+ .param = param_intel_lpss },
/* Intel Corp. ADL-S LPSS PCI UART #3 */
- {
- .vendor_id = PCI_VENDOR_ID_INTEL,
- .dev_id = 0x7adc,
- .param = param_intel_lpss
- },
+ { .vendor_id = PCI_VENDOR_ID_INTEL,
+ .dev_id = 0x7adc,
+ .param = param_intel_lpss },
};
-static int __init
-pci_uart_config(struct ns16550 *uart, bool skip_amt, unsigned int idx)
+static int __init pci_uart_config(struct ns16550 *uart, bool skip_amt,
+ unsigned int idx)
{
u64 orig_base = uart->io_base;
unsigned int b, d, f, nextf, i;
@@ -1197,7 +1174,9 @@ pci_uart_config(struct ns16550 *uart, bool skip_amt, unsigned int idx)
nextf = (f || (pci_conf_read16(PCI_SBDF(0, b, d, f),
PCI_HEADER_TYPE) &
- 0x80)) ? f + 1 : 8;
+ 0x80))
+ ? f + 1
+ : 8;
switch ( pci_conf_read16(PCI_SBDF(0, b, d, f),
PCI_CLASS_DEVICE) )
@@ -1250,24 +1229,30 @@ pci_uart_config(struct ns16550 *uart, bool skip_amt, unsigned int idx)
if ( param->mmio && !(bar & PCI_BASE_ADDRESS_SPACE_IO) )
{
pci_conf_write32(PCI_SBDF(0, b, d, f),
- PCI_BASE_ADDRESS_0 + bar_idx*4, ~0u);
+ PCI_BASE_ADDRESS_0 + bar_idx * 4,
+ ~0u);
len = pci_conf_read32(PCI_SBDF(0, b, d, f),
PCI_BASE_ADDRESS_0 + bar_idx * 4);
pci_conf_write32(PCI_SBDF(0, b, d, f),
- PCI_BASE_ADDRESS_0 + bar_idx*4, bar);
+ PCI_BASE_ADDRESS_0 + bar_idx * 4,
+ bar);
/* Handle 64 bit BAR if found */
if ( bar & PCI_BASE_ADDRESS_MEM_TYPE_64 )
{
bar_64 = pci_conf_read32(PCI_SBDF(0, b, d, f),
- PCI_BASE_ADDRESS_0 + (bar_idx + 1) * 4);
+ PCI_BASE_ADDRESS_0 +
+ (bar_idx + 1) * 4);
pci_conf_write32(PCI_SBDF(0, b, d, f),
- PCI_BASE_ADDRESS_0 + (bar_idx+1)*4, ~0u);
+ PCI_BASE_ADDRESS_0 + (bar_idx + 1) * 4,
+ ~0u);
len_64 = pci_conf_read32(PCI_SBDF(0, b, d, f),
- PCI_BASE_ADDRESS_0 + (bar_idx + 1) * 4);
+ PCI_BASE_ADDRESS_0 +
+ (bar_idx + 1) * 4);
pci_conf_write32(PCI_SBDF(0, b, d, f),
- PCI_BASE_ADDRESS_0 + (bar_idx+1)*4, bar_64);
- size = ((u64)~0 << 32) | PCI_BASE_ADDRESS_MEM_MASK;
+ PCI_BASE_ADDRESS_0 + (bar_idx + 1) * 4,
+ bar_64);
+ size = ((u64)~0 << 32) | PCI_BASE_ADDRESS_MEM_MASK;
size &= ((u64)len_64 << 32) | len;
}
else
@@ -1280,11 +1265,13 @@ pci_uart_config(struct ns16550 *uart, bool skip_amt, unsigned int idx)
else if ( !param->mmio && (bar & PCI_BASE_ADDRESS_SPACE_IO) )
{
pci_conf_write32(PCI_SBDF(0, b, d, f),
- PCI_BASE_ADDRESS_0 + bar_idx*4, ~0u);
+ PCI_BASE_ADDRESS_0 + bar_idx * 4,
+ ~0u);
len = pci_conf_read32(PCI_SBDF(0, b, d, f),
PCI_BASE_ADDRESS_0);
pci_conf_write32(PCI_SBDF(0, b, d, f),
- PCI_BASE_ADDRESS_0 + bar_idx*4, bar);
+ PCI_BASE_ADDRESS_0 + bar_idx * 4,
+ bar);
size = len & PCI_BASE_ADDRESS_IO_MASK;
uart->io_base = bar & ~PCI_BASE_ADDRESS_SPACE_IO;
@@ -1301,8 +1288,8 @@ pci_uart_config(struct ns16550 *uart, bool skip_amt, unsigned int idx)
* 8 bytes times (1 << reg_shift).
*/
if ( size < param->first_offset +
- port_idx * param->uart_offset +
- (8 << param->reg_shift) )
+ port_idx * param->uart_offset +
+ (8 << param->reg_shift) )
continue;
uart->param = param;
@@ -1323,12 +1310,12 @@ pci_uart_config(struct ns16550 *uart, bool skip_amt, unsigned int idx)
uart->bar_idx = bar_idx;
uart->bar = bar;
uart->bar64 = bar_64;
- uart->io_size = max(8U << param->reg_shift,
- param->uart_offset);
+ uart->io_size = max(8U << param->reg_shift, param->uart_offset);
uart->irq = pci_conf_read8(PCI_SBDF(0, b, d, f),
- PCI_INTERRUPT_PIN) ?
- pci_conf_read8(PCI_SBDF(0, b, d, f),
- PCI_INTERRUPT_LINE) : 0;
+ PCI_INTERRUPT_PIN)
+ ? pci_conf_read8(PCI_SBDF(0, b, d, f),
+ PCI_INTERRUPT_LINE)
+ : 0;
#ifdef CONFIG_X86
/*
@@ -1422,19 +1409,19 @@ struct serial_param_var {
* com_console_options for serial port com1 and com2.
*/
static const struct serial_param_var __initconst sp_vars[] = {
- {"baud", baud_rate},
- {"clock-hz", clock_hz},
- {"data-bits", data_bits},
- {"io-base", io_base},
- {"irq", irq},
- {"parity", parity},
- {"reg-shift", reg_shift},
- {"reg-width", reg_width},
- {"stop-bits", stop_bits},
+ { "baud", baud_rate },
+ { "clock-hz", clock_hz },
+ { "data-bits", data_bits },
+ { "io-base", io_base },
+ { "irq", irq },
+ { "parity", parity },
+ { "reg-shift", reg_shift },
+ { "reg-width", reg_width },
+ { "stop-bits", stop_bits },
#ifdef CONFIG_HAS_PCI
- {"bridge", bridge_bdf},
- {"dev", device},
- {"port", port_bdf},
+ { "bridge", bridge_bdf },
+ { "dev", device },
+ { "port", port_bdf },
#endif
};
@@ -1476,7 +1463,6 @@ static enum __init serial_param_type get_token(char *token, char **value)
return false; \
} while ( 0 )
-
static bool __init parse_positional(struct ns16550 *uart, char **str)
{
int baud;
@@ -1533,7 +1519,7 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
#ifdef CONFIG_HAS_PCI
if ( strncmp(conf, "pci", 3) == 0 )
{
- if ( pci_uart_config(uart, 1/* skip AMT */, uart - ns16550_com) )
+ if ( pci_uart_config(uart, 1 /* skip AMT */, uart - ns16550_com) )
return true;
conf += 3;
}
@@ -1567,8 +1553,11 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
#ifdef CONFIG_HAS_PCI
if ( *conf == ',' && *++conf != ',' )
{
- conf = parse_pci(conf, NULL, &uart->ps_bdf[0],
- &uart->ps_bdf[1], &uart->ps_bdf[2]);
+ conf = parse_pci(conf,
+ NULL,
+ &uart->ps_bdf[0],
+ &uart->ps_bdf[1],
+ &uart->ps_bdf[2]);
if ( !conf )
PARSE_ERR_RET("Bad port PCI coordinates");
uart->ps_bdf_enable = true;
@@ -1576,8 +1565,11 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
if ( *conf == ',' && *++conf != ',' )
{
- if ( !parse_pci(conf, NULL, &uart->pb_bdf[0],
- &uart->pb_bdf[1], &uart->pb_bdf[2]) )
+ if ( !parse_pci(conf,
+ NULL,
+ &uart->pb_bdf[0],
+ &uart->pb_bdf[1],
+ &uart->pb_bdf[2]) )
PARSE_ERR_RET("Bad bridge PCI coordinates");
uart->pb_bdf_enable = true;
}
@@ -1648,7 +1640,7 @@ static bool __init parse_namevalue_pairs(char *str, struct ns16550 *uart)
case device:
if ( strncmp(param_value, "pci", 3) == 0 )
{
- pci_uart_config(uart, 1/* skip AMT */, uart - ns16550_com);
+ pci_uart_config(uart, 1 /* skip AMT */, uart - ns16550_com);
dev_set = true;
}
else if ( strncmp(param_value, "amt", 3) == 0 )
@@ -1659,15 +1651,21 @@ static bool __init parse_namevalue_pairs(char *str, struct ns16550 *uart)
break;
case port_bdf:
- if ( !parse_pci(param_value, NULL, &uart->ps_bdf[0],
- &uart->ps_bdf[1], &uart->ps_bdf[2]) )
+ if ( !parse_pci(param_value,
+ NULL,
+ &uart->ps_bdf[0],
+ &uart->ps_bdf[1],
+ &uart->ps_bdf[2]) )
PARSE_ERR_RET("Bad port PCI coordinates\n");
uart->ps_bdf_enable = true;
break;
case bridge_bdf:
- if ( !parse_pci(param_value, NULL, &uart->pb_bdf[0],
- &uart->pb_bdf[1], &uart->pb_bdf[2]) )
+ if ( !parse_pci(param_value,
+ NULL,
+ &uart->pb_bdf[0],
+ &uart->pb_bdf[1],
+ &uart->pb_bdf[2]) )
PARSE_ERR_RET("Bad bridge PCI coordinates\n");
uart->pb_bdf_enable = true;
break;
@@ -1681,8 +1679,8 @@ static bool __init parse_namevalue_pairs(char *str, struct ns16550 *uart)
return true;
}
-static void __init ns16550_parse_port_config(
- struct ns16550 *uart, const char *conf)
+static void __init ns16550_parse_port_config(struct ns16550 *uart,
+ const char *conf)
{
char com_console_options[128];
char *str;
@@ -1706,7 +1704,7 @@ static void __init ns16550_parse_port_config(
if ( !parse_namevalue_pairs(str, uart) )
return;
- config_parsed:
+config_parsed:
/* Sanity checks. */
if ( (uart->baud != BAUD_AUTO) &&
((uart->baud < 1200) || (uart->baud > 115200)) )
@@ -1737,15 +1735,15 @@ void __init ns16550_init(int index, struct ns16550_defaults *defaults)
ns16550_init_common(uart);
- uart->baud = (defaults->baud ? :
- console_has((index == 0) ? "com1" : "com2")
- ? BAUD_AUTO : 0);
+ uart->baud = (defaults->baud
+ ?: console_has((index == 0) ? "com1" : "com2") ? BAUD_AUTO
+ : 0);
uart->data_bits = defaults->data_bits;
- uart->parity = parse_parity_char(defaults->parity);
+ uart->parity = parse_parity_char(defaults->parity);
uart->stop_bits = defaults->stop_bits;
- uart->irq = defaults->irq;
- uart->io_base = defaults->io_base;
- uart->io_size = 8;
+ uart->irq = defaults->irq;
+ uart->io_base = defaults->io_base;
+ uart->io_size = 8;
uart->reg_width = 1;
uart->reg_shift = 0;
@@ -1766,9 +1764,9 @@ static int __init ns16550_uart_dt_init(struct dt_device_node *dev,
ns16550_init_common(uart);
- uart->baud = BAUD_AUTO;
+ uart->baud = BAUD_AUTO;
uart->data_bits = 8;
- uart->parity = UART_PARITY_NONE;
+ uart->parity = UART_PARITY_NONE;
uart->stop_bits = 1;
res = dt_device_get_address(dev, 0, &uart->io_base, &uart->io_size);
@@ -1797,7 +1795,7 @@ static int __init ns16550_uart_dt_init(struct dt_device_node *dev,
}
res = platform_get_irq(dev, 0);
- if ( ! res )
+ if ( !res )
return -EINVAL;
uart->irq = res;
@@ -1806,9 +1804,9 @@ static int __init ns16550_uart_dt_init(struct dt_device_node *dev,
#ifdef CONFIG_ARM
uart->vuart.base_addr = uart->io_base;
uart->vuart.size = uart->io_size;
- uart->vuart.data_off = UART_THR <<uart->reg_shift;
- uart->vuart.status_off = UART_LSR<<uart->reg_shift;
- uart->vuart.status = UART_LSR_THRE|UART_LSR_TEMT;
+ uart->vuart.data_off = UART_THR << uart->reg_shift;
+ uart->vuart.status_off = UART_LSR << uart->reg_shift;
+ uart->vuart.status = UART_LSR_THRE | UART_LSR_TEMT;
#endif
/* Register with generic serial driver. */
@@ -1819,8 +1817,7 @@ static int __init ns16550_uart_dt_init(struct dt_device_node *dev,
return 0;
}
-static const struct dt_device_match ns16550_dt_match[] __initconst =
-{
+static const struct dt_device_match ns16550_dt_match[] __initconst = {
DT_MATCH_COMPATIBLE("ns16550"),
DT_MATCH_COMPATIBLE("ns16550a"),
DT_MATCH_COMPATIBLE("snps,dw-apb-uart"),
@@ -1829,8 +1826,7 @@ static const struct dt_device_match ns16550_dt_match[] __initconst =
};
DT_DEVICE_START(ns16550, "NS16550 UART", DEVICE_SERIAL)
- .dt_match = ns16550_dt_match,
- .init = ns16550_uart_dt_init,
+ .dt_match = ns16550_dt_match, .init = ns16550_uart_dt_init,
DT_DEVICE_END
#endif /* HAS_DEVICE_TREE */
@@ -1907,8 +1903,7 @@ static int __init ns16550_acpi_uart_init(const void *data)
}
ACPI_DEVICE_START(ans16550, "NS16550 UART", DEVICE_SERIAL)
- .class_type = ACPI_DBG2_16550_COMPATIBLE,
- .init = ns16550_acpi_uart_init,
+ .class_type = ACPI_DBG2_16550_COMPATIBLE, .init = ns16550_acpi_uart_init,
ACPI_DEVICE_END
#endif /* CONFIG_ACPI && CONFIG_ARM */
--
2.25.1On 16.02.2025 11:21, Oleksandr Andrushchenko wrote:
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -14,7 +14,7 @@
> * abstracted.
> */
> #if defined(CONFIG_HAS_PCI) && defined(CONFIG_X86)
> -# define NS16550_PCI
> +#define NS16550_PCI
> #endif
Hmm. Either form ought to be okay, so the line would want leaving untouched.
> @@ -43,12 +43,12 @@
>
> static struct ns16550 {
> int baud, clock_hz, data_bits, parity, stop_bits, fifo_size, irq;
> - u64 io_base; /* I/O port or memory-mapped I/O address. */
> + u64 io_base; /* I/O port or memory-mapped I/O address. */
> u64 io_size;
> int reg_shift; /* Bits to shift register offset by */
Breaking alignment between comments (also in the immediately following hunk),
while at the same time ...
> int reg_width; /* Size of access to use, the registers
> * themselves are still bytes */
... not taking care of the comment style violation here?
> @@ -63,8 +63,8 @@ static struct ns16550 {
> bool dw_usr_bsy;
> #ifdef NS16550_PCI
> /* PCI card parameters. */
> - bool pb_bdf_enable; /* if =1, pb-bdf effective, port behind bridge */
> - bool ps_bdf_enable; /* if =1, ps_bdf effective, port on pci card */
> + bool pb_bdf_enable; /* if =1, pb-bdf effective, port behind bridge */
> + bool ps_bdf_enable; /* if =1, ps_bdf effective, port on pci card */
(Just leaving this here for context of the earlier comment.)
> @@ -248,8 +249,9 @@ static int cf_check ns16550_tx_ready(struct serial_port *port)
> if ( ns16550_ioport_invalid(uart) )
> return -EIO;
>
> - return ( (ns_read_reg(uart, UART_LSR) &
> - uart->lsr_mask ) == uart->lsr_mask ) ? uart->fifo_size : 0;
> + return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask)
> + ? uart->fifo_size
> + : 0;
Indentation of the ? and : lines is clearly wrong here? What is the tool
doing?
> @@ -275,9 +277,10 @@ static void pci_serial_early_init(struct ns16550 *uart)
> #ifdef NS16550_PCI
> if ( uart->bar && uart->io_base >= 0x10000 )
> {
> - pci_conf_write16(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
> - uart->ps_bdf[2]),
> - PCI_COMMAND, PCI_COMMAND_MEMORY);
> + pci_conf_write16(
> + PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1], uart->ps_bdf[2]),
> + PCI_COMMAND,
> + PCI_COMMAND_MEMORY);
> return;
> }
Hmm, transforming a well-formed block into another well-formed one. No
gain? (Same again further down.)
> @@ -335,14 +338,14 @@ static void ns16550_setup_preirq(struct ns16550 *uart)
> else
> {
> /* Baud rate already set: read it out from the divisor latch. */
> - divisor = ns_read_reg(uart, UART_DLL);
> + divisor = ns_read_reg(uart, UART_DLL);
> divisor |= ns_read_reg(uart, UART_DLM) << 8;
An example where tabulation is being broken. There are quite a bit worse
ones further down.
> @@ -350,8 +353,10 @@ static void ns16550_setup_preirq(struct ns16550 *uart)
> ns_write_reg(uart, UART_MCR, UART_MCR_DTR | UART_MCR_RTS);
>
> /* Enable and clear the FIFOs. Set a large trigger threshold. */
> - ns_write_reg(uart, UART_FCR,
> - UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX | UART_FCR_TRG14);
> + ns_write_reg(uart,
> + UART_FCR,
> + UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX |
> + UART_FCR_TRG14);
What's the underlying indentation rule here? The way it's re-formatted
certainly looks odd to me. What we occasionally do in such cases is add
parentheses:
ns_write_reg(uart,
UART_FCR,
(UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX |
UART_FCR_TRG14));
Also - does the format they apply demand one argument per line? Else
why not
ns_write_reg(uart, UART_FCR,
(UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX |
UART_FCR_TRG14));
Plus what's their criteria to choose between this style of function calls
and the one in context higher up for calls to pci_conf_write16()?
> @@ -424,31 +430,37 @@ static void __init cf_check ns16550_init_postirq(struct serial_port *port)
>
> /* Calculate time to fill RX FIFO and/or empty TX FIFO for polling. */
> bits = uart->data_bits + uart->stop_bits + !!uart->parity;
> - uart->timeout_ms = max_t(
> - unsigned int, 1, (bits * uart->fifo_size * 1000) / uart->baud);
> + uart->timeout_ms =
> + max_t(unsigned int, 1, (bits * uart->fifo_size * 1000) / uart->baud);
Again both forms look conforming to me. I think there's a general issue
with the tool making transformations when none are needed. I won't
further point out any such.
> @@ -1197,7 +1174,9 @@ pci_uart_config(struct ns16550 *uart, bool skip_amt, unsigned int idx)
>
> nextf = (f || (pci_conf_read16(PCI_SBDF(0, b, d, f),
> PCI_HEADER_TYPE) &
> - 0x80)) ? f + 1 : 8;
> + 0x80))
> + ? f + 1
> + : 8;
Again the odd indentation of ? and :.
> @@ -1422,19 +1409,19 @@ struct serial_param_var {
> * com_console_options for serial port com1 and com2.
> */
> static const struct serial_param_var __initconst sp_vars[] = {
> - {"baud", baud_rate},
> - {"clock-hz", clock_hz},
> - {"data-bits", data_bits},
> - {"io-base", io_base},
> - {"irq", irq},
> - {"parity", parity},
> - {"reg-shift", reg_shift},
> - {"reg-width", reg_width},
> - {"stop-bits", stop_bits},
> + { "baud", baud_rate },
> + { "clock-hz", clock_hz },
> + { "data-bits", data_bits },
> + { "io-base", io_base },
> + { "irq", irq },
> + { "parity", parity },
> + { "reg-shift", reg_shift },
> + { "reg-width", reg_width },
> + { "stop-bits", stop_bits },
> #ifdef CONFIG_HAS_PCI
> - {"bridge", bridge_bdf},
> - {"dev", device},
> - {"port", port_bdf},
> + { "bridge", bridge_bdf },
> + { "dev", device },
> + { "port", port_bdf },
> #endif
> };
Interesting - here tabulation is being introduced.
> @@ -1706,7 +1704,7 @@ static void __init ns16550_parse_port_config(
> if ( !parse_namevalue_pairs(str, uart) )
> return;
>
> - config_parsed:
> +config_parsed:
This is a no-go - ./CODING_STYLE specifically says why this isn't appropriate.
All of the remarks aside, there are also a lot of good changes that are being
made.
Jan
Hello, Jan!
On 17.02.25 12:20, Jan Beulich wrote:
> On 16.02.2025 11:21, Oleksandr Andrushchenko wrote:
>> --- a/xen/drivers/char/ns16550.c
>> +++ b/xen/drivers/char/ns16550.c
>> @@ -14,7 +14,7 @@
>> * abstracted.
>> */
>> #if defined(CONFIG_HAS_PCI) && defined(CONFIG_X86)
>> -# define NS16550_PCI
>> +#define NS16550_PCI
>> #endif
> Hmm. Either form ought to be okay, so the line would want leaving untouched.
This is controlled by PPIndentWidth option [1]:
we have it set to "-1" which means "When set to -1 (default) *IndentWidth*
is used also for preprocessor statements."
Unfortunately, I was not able to see a change with PPIndentWidth + IndentWidth
PPIndentWidth: -1
IndentWidth: 4
--- test_define.c 2025-02-19 11:20:30.708096428 +0200
+++ test_define_minus1.c 2025-02-19 11:21:42.313013373 +0200
@@ -1,5 +1,5 @@
#ifdef __linux__
-# define FOO
+#define FOO
#else
-# define BAR
+#define BAR
#endif
PPIndentWidth: 1
IndentWidth: 4
--- test_define.c 2025-02-19 11:20:30.708096428 +0200
+++ test_define_1.c 2025-02-19 11:23:46.986618706 +0200
@@ -1,5 +1,5 @@
#ifdef __linux__
-# define FOO
+#define FOO
#else
-# define BAR
+#define BAR
#endif
I thought it is a bug in the latest clang-format (21), but I was not able to
get the expected result with clang-format 18 as well. I am not sure if
I'm doing anything wrong with .clang-format, but this works one way
for me as ow now.
Takeaway: I was not able to control this, but the result seems to be acceptable
It should be noted in the coding style though
>> @@ -43,12 +43,12 @@
>>
>> static struct ns16550 {
>> int baud, clock_hz, data_bits, parity, stop_bits, fifo_size, irq;
>> - u64 io_base; /* I/O port or memory-mapped I/O address. */
>> + u64 io_base; /* I/O port or memory-mapped I/O address. */
>> u64 io_size;
>> int reg_shift; /* Bits to shift register offset by */
> Breaking alignment between comments (also in the immediately following hunk),
> while at the same time ...
This one...
>> int reg_width; /* Size of access to use, the registers
>> * themselves are still bytes */
> ... not taking care of the comment style violation here?
This is controlled by ReflowComments option [3]:
From the tool point of view the comment is formatted correctly
I didn't find a way to convert such comments into
/*
* Size of access to use, the registers * themselves are still bytes */ if this is what you mean.
>> @@ -63,8 +63,8 @@ static struct ns16550 {
>> bool dw_usr_bsy;
>> #ifdef NS16550_PCI
>> /* PCI card parameters. */
>> - bool pb_bdf_enable; /* if =1, pb-bdf effective, port behind bridge */
>> - bool ps_bdf_enable; /* if =1, ps_bdf effective, port on pci card */
>> + bool pb_bdf_enable; /* if =1, pb-bdf effective, port behind bridge */
>> + bool ps_bdf_enable; /* if =1, ps_bdf effective, port on pci card */
> (Just leaving this here for context of the earlier comment.)
... and this one. It seems that the tool tries to always have a single space before
the comment. There is an option SpacesBeforeTrailingComments [2] which
unfortunately only controls C++ style comments and "...does not affect trailing
block comments (|/*| - comments)..."
So, it seems that this is the rule to consider
>> @@ -248,8 +249,9 @@ static int cf_check ns16550_tx_ready(struct serial_port *port)
>> if ( ns16550_ioport_invalid(uart) )
>> return -EIO;
>>
>> - return ( (ns_read_reg(uart, UART_LSR) &
>> - uart->lsr_mask ) == uart->lsr_mask ) ? uart->fifo_size : 0;
>> + return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask)
>> + ? uart->fifo_size
>> + : 0;
> Indentation of the ? and : lines is clearly wrong here? What is the tool
> doing?
There are number of options that have influence on this formatting:
AllowShortBlocksOnASingleLine [4]
BreakBeforeTernaryOperators [5]
AlignOperands [6]
I was not able to tweak these options to have the previous form.
>> @@ -275,9 +277,10 @@ static void pci_serial_early_init(struct ns16550 *uart)
>> #ifdef NS16550_PCI
>> if ( uart->bar && uart->io_base >= 0x10000 )
>> {
>> - pci_conf_write16(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
>> - uart->ps_bdf[2]),
>> - PCI_COMMAND, PCI_COMMAND_MEMORY);
>> + pci_conf_write16(
>> + PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1], uart->ps_bdf[2]),
>> + PCI_COMMAND,
>> + PCI_COMMAND_MEMORY);
>> return;
>> }
> Hmm, transforming a well-formed block into another well-formed one. No
> gain? (Same again further down.)
No, gain from human point of view
But there is a gain that it is now formatted automatically.
>> @@ -335,14 +338,14 @@ static void ns16550_setup_preirq(struct ns16550 *uart)
>> else
>> {
>> /* Baud rate already set: read it out from the divisor latch. */
>> - divisor = ns_read_reg(uart, UART_DLL);
>> + divisor = ns_read_reg(uart, UART_DLL);
>> divisor |= ns_read_reg(uart, UART_DLM) << 8;
> An example where tabulation is being broken. There are quite a bit worse
> ones further down.
This one will be impossible to implement with clang-format IMO.
Because there are cases where you want this (like above) and you know why:
the assignments look prettier here that way. But this doesn't mean
that in some other places other assignments will look got for you if
formatted the same way.
The question here what is the metric (human perception?) in this case
and how this perception can be be programmed into clang-format
configuration?
>> @@ -350,8 +353,10 @@ static void ns16550_setup_preirq(struct ns16550 *uart)
>> ns_write_reg(uart, UART_MCR, UART_MCR_DTR | UART_MCR_RTS);
>>
>> /* Enable and clear the FIFOs. Set a large trigger threshold. */
>> - ns_write_reg(uart, UART_FCR,
>> - UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX | UART_FCR_TRG14);
>> + ns_write_reg(uart,
>> + UART_FCR,
>> + UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX |
>> + UART_FCR_TRG14);
> What's the underlying indentation rule here? The way it's re-formatted
> certainly looks odd to me. What we occasionally do in such cases is add
> parentheses:
>
> ns_write_reg(uart,
> UART_FCR,
> (UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX |
> UART_FCR_TRG14));
>
> Also - does the format they apply demand one argument per line? Else
> why not
>
> ns_write_reg(uart, UART_FCR,
> (UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX |
> UART_FCR_TRG14));
>
> Plus what's their criteria to choose between this style of function calls
> and the one in context higher up for calls to pci_conf_write16()?
This is controlled with AlignAfterOpenBracket [7]
So this could be:
*AlignAfterOpenBracket: Align*
- ns_write_reg(uart, UART_FCR,
- UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX | UART_FCR_TRG14);
+ ns_write_reg(uart,
+ UART_FCR,
+ UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX |
+ UART_FCR_TRG14);
*AlignAfterOpenBracket: |DontAlign*
- ns_write_reg(uart, UART_FCR,
- UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX | UART_FCR_TRG14);
+ ns_write_reg(uart,
+ UART_FCR,
+ UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX | UART_FCR_TRG14);
|*AlignAfterOpenBracket: |AlwaysBreak*
- ns_write_reg(uart, UART_FCR,
- UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX | UART_FCR_TRG14);
+ ns_write_reg(
+ uart,
+ UART_FCR,
+ UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX | UART_FCR_TRG14);
|*AlignAfterOpenBracket: |BlockIndent*
- ns_write_reg(uart, UART_FCR,
- UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX | UART_FCR_TRG14);
+ ns_write_reg(
+ uart,
+ UART_FCR,
+ UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX | UART_FCR_TRG14
+ );
||
|
>> @@ -424,31 +430,37 @@ static void __init cf_check ns16550_init_postirq(struct serial_port *port)
>>
>> /* Calculate time to fill RX FIFO and/or empty TX FIFO for polling. */
>> bits = uart->data_bits + uart->stop_bits + !!uart->parity;
>> - uart->timeout_ms = max_t(
>> - unsigned int, 1, (bits * uart->fifo_size * 1000) / uart->baud);
>> + uart->timeout_ms =
>> + max_t(unsigned int, 1, (bits * uart->fifo_size * 1000) / uart->baud);
> Again both forms look conforming to me. I think there's a general issue
> with the tool making transformations when none are needed. I won't
> further point out any such.
Again, this is something which a human can decide what is better for
their taste. clang-format won't be able to treat such cases differently
>> @@ -1197,7 +1174,9 @@ pci_uart_config(struct ns16550 *uart, bool skip_amt, unsigned int idx)
>>
>> nextf = (f || (pci_conf_read16(PCI_SBDF(0, b, d, f),
>> PCI_HEADER_TYPE) &
>> - 0x80)) ? f + 1 : 8;
>> + 0x80))
>> + ? f + 1
>> + : 8;
> Again the odd indentation of ? and :.
>
>> @@ -1422,19 +1409,19 @@ struct serial_param_var {
>> * com_console_options for serial port com1 and com2.
>> */
>> static const struct serial_param_var __initconst sp_vars[] = {
>> - {"baud", baud_rate},
>> - {"clock-hz", clock_hz},
>> - {"data-bits", data_bits},
>> - {"io-base", io_base},
>> - {"irq", irq},
>> - {"parity", parity},
>> - {"reg-shift", reg_shift},
>> - {"reg-width", reg_width},
>> - {"stop-bits", stop_bits},
>> + { "baud", baud_rate },
>> + { "clock-hz", clock_hz },
>> + { "data-bits", data_bits },
>> + { "io-base", io_base },
>> + { "irq", irq },
>> + { "parity", parity },
>> + { "reg-shift", reg_shift },
>> + { "reg-width", reg_width },
>> + { "stop-bits", stop_bits },
>> #ifdef CONFIG_HAS_PCI
>> - {"bridge", bridge_bdf},
>> - {"dev", device},
>> - {"port", port_bdf},
>> + { "bridge", bridge_bdf },
>> + { "dev", device },
>> + { "port", port_bdf },
>> #endif
>> };
> Interesting - here tabulation is being introduced.
This is controlled with AlignArrayOfStructures [8] and
we can have it as is if we set it to None
The same was discussed before, so one can refresh that at [9]
>> @@ -1706,7 +1704,7 @@ static void __init ns16550_parse_port_config(
>> if ( !parse_namevalue_pairs(str, uart) )
>> return;
>>
>> - config_parsed:
>> +config_parsed:
> This is a no-go - ./CODING_STYLE specifically says why this isn't appropriate.
Yes, it can't formatted as we wish. This is controlled with IndentGotoLabels [10]
and is a binary option, which leaves no means to disable it as both true and
false will re-format the code
true:false:
intf(){vs.intf(){
if(foo()){if(foo()){
label1:label1:
bar();bar();
}}
label2:label2:
return1;return1;
}}
> All of the remarks aside, there are also a lot of good changes that are being
> made.
Agree and at least applying some of those changes may improve the code
to be more consistent.
> Jan
Thank you,
Oleksandr
[1] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#ppindentwidth
[2] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#spacesbeforetrailingcomments
[3] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#reflowcomments
[4] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#allowshortblocksonasingleline
[5] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#breakbeforeternaryoperators
[6] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#alignoperands
[7] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#alignafteropenbracket
[8] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#alignarrayofstructures
[9] https://lists.xenproject.org/archives/html/xen-devel/2023-11/msg02172.html
[10] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#indentgotolabels
Hello, Jan! On 19.02.25 14:39, Oleksandr Andrushchenko wrote: > Hello, Jan! > > On 17.02.25 12:20, Jan Beulich wrote: >> On 16.02.2025 11:21, Oleksandr Andrushchenko wrote: >>> --- a/xen/drivers/char/ns16550.c >>> +++ b/xen/drivers/char/ns16550.c >>> @@ -14,7 +14,7 @@ >>> * abstracted. >>> */ >>> #if defined(CONFIG_HAS_PCI) && defined(CONFIG_X86) >>> -# define NS16550_PCI >>> +#define NS16550_PCI >>> #endif >> Hmm. Either form ought to be okay, so the line would want leaving untouched. It seems this can actually have 3 forms under IndentPPDirectives control Please see [1]. I would go with BeforeHash personally [1] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#indentppdirectives
On 19.02.2025 15:23, Oleksandr Andrushchenko wrote: > On 19.02.25 14:39, Oleksandr Andrushchenko wrote: >> On 17.02.25 12:20, Jan Beulich wrote: >>> On 16.02.2025 11:21, Oleksandr Andrushchenko wrote: >>>> --- a/xen/drivers/char/ns16550.c >>>> +++ b/xen/drivers/char/ns16550.c >>>> @@ -14,7 +14,7 @@ >>>> * abstracted. >>>> */ >>>> #if defined(CONFIG_HAS_PCI) && defined(CONFIG_X86) >>>> -# define NS16550_PCI >>>> +#define NS16550_PCI >>>> #endif >>> Hmm. Either form ought to be okay, so the line would want leaving untouched. > It seems this can actually have 3 forms under IndentPPDirectives control > Please see [1]. I saw that. None of the three variants matches the original above, i.e. here we're again observing a change just for the sake of making a change. The style used is conforming, after all. Jan > I would go with BeforeHash personally > > [1] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#indentppdirectives > >
On 19.02.2025 13:39, Oleksandr Andrushchenko wrote:
> On 17.02.25 12:20, Jan Beulich wrote:
>> On 16.02.2025 11:21, Oleksandr Andrushchenko wrote:
>>> @@ -43,12 +43,12 @@
>>>
>>> static struct ns16550 {
>>> int baud, clock_hz, data_bits, parity, stop_bits, fifo_size, irq;
>>> - u64 io_base; /* I/O port or memory-mapped I/O address. */
>>> + u64 io_base; /* I/O port or memory-mapped I/O address. */
>>> u64 io_size;
>>> int reg_shift; /* Bits to shift register offset by */
>> Breaking alignment between comments (also in the immediately following hunk),
>> while at the same time ...
> This one...
>>> int reg_width; /* Size of access to use, the registers
>>> * themselves are still bytes */
>> ... not taking care of the comment style violation here?
> This is controlled by ReflowComments option [3]:
>
> From the tool point of view the comment is formatted correctly
> I didn't find a way to convert such comments into
> /*
> * Size of access to use, the registers * themselves are still bytes */ if this is what you mean.
Above you see what I received. I can't really deduce from this what
formatting you suggested. In the case at hand, though, I think it's not
the best idea anyway to put a multi-line comment past a declaration (or
statement).
/*
* Size of access to use, the registers
* themselves are still bytes
*/
int reg_width;
is what I think would be better in such a case (artificially keeping
this to be multi-line, even if it looks as if it might fit on just one
line then).
>>> @@ -248,8 +249,9 @@ static int cf_check ns16550_tx_ready(struct serial_port *port)
>>> if ( ns16550_ioport_invalid(uart) )
>>> return -EIO;
>>>
>>> - return ( (ns_read_reg(uart, UART_LSR) &
>>> - uart->lsr_mask ) == uart->lsr_mask ) ? uart->fifo_size : 0;
>>> + return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask)
>>> + ? uart->fifo_size
>>> + : 0;
>> Indentation of the ? and : lines is clearly wrong here? What is the tool
>> doing?
> There are number of options that have influence on this formatting:
> AllowShortBlocksOnASingleLine [4]
> BreakBeforeTernaryOperators [5]
> AlignOperands [6]
>
> I was not able to tweak these options to have the previous form.
Right, sticking to the original form (with just the stray blanks zapped)
would of course be best. Yet again - the tool is doing more transformations
despite there not being any need. If, however, it does so, then one of my
expectations would be that the ? and : are properly indented:
return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask)
? uart->fifo_size
: 0;
or
return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask)
? uart->fifo_size : 0;
or
return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask
? uart->fifo_size
: 0);
(not going to list more variants which are all okay).
In any event, a fundamental requirement of mine is that such a tool would
only apply adjustments when and where style is actively violated. I.e. in
the case here:
return ((ns_read_reg(uart, UART_LSR) &
uart->lsr_mask) == uart->lsr_mask) ? uart->fifo_size : 0;
That's not overly neat wrapping, but in line with our style. If the other
form was demanded going forward, I'd be curious how you'd verbally
describe the requirement in ./CODING_STYLE.
>>> @@ -275,9 +277,10 @@ static void pci_serial_early_init(struct ns16550 *uart)
>>> #ifdef NS16550_PCI
>>> if ( uart->bar && uart->io_base >= 0x10000 )
>>> {
>>> - pci_conf_write16(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
>>> - uart->ps_bdf[2]),
>>> - PCI_COMMAND, PCI_COMMAND_MEMORY);
>>> + pci_conf_write16(
>>> + PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1], uart->ps_bdf[2]),
>>> + PCI_COMMAND,
>>> + PCI_COMMAND_MEMORY);
>>> return;
>>> }
>> Hmm, transforming a well-formed block into another well-formed one. No
>> gain? (Same again further down.)
> No, gain from human point of view
> But there is a gain that it is now formatted automatically.
See above: I'd first like to see a written, textual description for all these
requirements. After all it needs to be possible for a human to write code
that the tool then wouldn't try to re-arrange. Which in turn requires that
the restrictions / constraints on the layout are spelled out. I'm not looking
forward to pass all my patches through such a tool. I can write style-
conforming code pretty well, with - of course - occasional oversights, right
now. And that in multiple projects all with different styles. I expect to be
in the position to do so also going forward. This, imo, requires that there
be left some room for variations. Which in turn requires that the tool would
leave alone anything that is not in conflict with the written down or defacto
style.
>>> @@ -335,14 +338,14 @@ static void ns16550_setup_preirq(struct ns16550 *uart)
>>> else
>>> {
>>> /* Baud rate already set: read it out from the divisor latch. */
>>> - divisor = ns_read_reg(uart, UART_DLL);
>>> + divisor = ns_read_reg(uart, UART_DLL);
>>> divisor |= ns_read_reg(uart, UART_DLM) << 8;
>> An example where tabulation is being broken. There are quite a bit worse
>> ones further down.
> This one will be impossible to implement with clang-format IMO.
> Because there are cases where you want this (like above) and you know why:
> the assignments look prettier here that way. But this doesn't mean
> that in some other places other assignments will look got for you if
> formatted the same way.
> The question here what is the metric (human perception?) in this case
> and how this perception can be be programmed into clang-format
> configuration?
Which imo is a pretty strong argument against using any auto-formatting. At
least up to the point where AI would end up being smart enough to mimic this
human perception.
>>> @@ -350,8 +353,10 @@ static void ns16550_setup_preirq(struct ns16550 *uart)
>>> ns_write_reg(uart, UART_MCR, UART_MCR_DTR | UART_MCR_RTS);
>>>
>>> /* Enable and clear the FIFOs. Set a large trigger threshold. */
>>> - ns_write_reg(uart, UART_FCR,
>>> - UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX | UART_FCR_TRG14);
>>> + ns_write_reg(uart,
>>> + UART_FCR,
>>> + UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX |
>>> + UART_FCR_TRG14);
>> What's the underlying indentation rule here? The way it's re-formatted
>> certainly looks odd to me. What we occasionally do in such cases is add
>> parentheses:
>>
>> ns_write_reg(uart,
>> UART_FCR,
>> (UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX |
>> UART_FCR_TRG14));
>>
>> Also - does the format they apply demand one argument per line? Else
>> why not
>>
>> ns_write_reg(uart, UART_FCR,
>> (UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX |
>> UART_FCR_TRG14));
>>
>> Plus what's their criteria to choose between this style of function calls
>> and the one in context higher up for calls to pci_conf_write16()?
> This is controlled with AlignAfterOpenBracket [7]
> So this could be:
> *AlignAfterOpenBracket: Align*
>
> - ns_write_reg(uart, UART_FCR,
> - UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX | UART_FCR_TRG14);
> + ns_write_reg(uart,
> + UART_FCR,
> + UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX |
> + UART_FCR_TRG14);
As before indentation is bogus here, ...
> *AlignAfterOpenBracket: |DontAlign*
>
> - ns_write_reg(uart, UART_FCR,
> - UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX | UART_FCR_TRG14);
> + ns_write_reg(uart,
> + UART_FCR,
> + UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX | UART_FCR_TRG14);
... and even more so here, ...
> |*AlignAfterOpenBracket: |AlwaysBreak*
>
> - ns_write_reg(uart, UART_FCR,
> - UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX | UART_FCR_TRG14);
> + ns_write_reg(
> + uart,
> + UART_FCR,
> + UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX | UART_FCR_TRG14);
... while this at least matches one of the forms we might presently use.
Yet again - there was nothing wrong with the original layout.
> |*AlignAfterOpenBracket: |BlockIndent*
>
> - ns_write_reg(uart, UART_FCR,
> - UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX | UART_FCR_TRG14);
> + ns_write_reg(
> + uart,
> + UART_FCR,
> + UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX | UART_FCR_TRG14
> + );
This clearly moves too far away from our present style.
>>> @@ -1706,7 +1704,7 @@ static void __init ns16550_parse_port_config(
>>> if ( !parse_namevalue_pairs(str, uart) )
>>> return;
>>>
>>> - config_parsed:
>>> +config_parsed:
>> This is a no-go - ./CODING_STYLE specifically says why this isn't appropriate.
> Yes, it can't formatted as we wish. This is controlled with IndentGotoLabels [10]
> and is a binary option, which leaves no means to disable it as both true and
> false will re-format the code
>
> true:false:
> intf(){vs.intf(){
> if(foo()){if(foo()){
> label1:label1:
> bar();bar();
> }}
> label2:label2:
> return1;return1;
> }}
If there was some indentation meant to be in that blob, it was all lost,
I'm afraid. Still: The name of the control being IndentGotoLabels and it
being of boolean nature as you say, why would it do anything with an
already-indented label?
Jan
Hello, Jan!
On 19.02.25 15:18, Jan Beulich wrote:
> On 19.02.2025 13:39, Oleksandr Andrushchenko wrote:
>> On 17.02.25 12:20, Jan Beulich wrote:
>>> On 16.02.2025 11:21, Oleksandr Andrushchenko wrote:
>>>> @@ -43,12 +43,12 @@
>>>>
>>>> static struct ns16550 {
>>>> int baud, clock_hz, data_bits, parity, stop_bits, fifo_size, irq;
>>>> - u64 io_base; /* I/O port or memory-mapped I/O address. */
>>>> + u64 io_base; /* I/O port or memory-mapped I/O address. */
>>>> u64 io_size;
>>>> int reg_shift; /* Bits to shift register offset by */
>>> Breaking alignment between comments (also in the immediately following hunk),
>>> while at the same time ...
>> This one...
>>>> int reg_width; /* Size of access to use, the registers
>>>> * themselves are still bytes */
>>> ... not taking care of the comment style violation here?
>> This is controlled by ReflowComments option [3]:
>>
>> From the tool point of view the comment is formatted correctly
>> I didn't find a way to convert such comments into
>> /*
>> * Size of access to use, the registers * themselves are still bytes */ if this is what you mean.
> Above you see what I received. I can't really deduce from this what
> formatting you suggested. In the case at hand, though, I think it's not
> the best idea anyway to put a multi-line comment past a declaration (or
> statement).
Sorry, for the formatting
> /*
> * Size of access to use, the registers
> * themselves are still bytes
> */
This is what I was trying to send
> int reg_width;
>
> is what I think would be better in such a case (artificially keeping
> this to be multi-line, even if it looks as if it might fit on just one
> line then).
>
>>>> @@ -248,8 +249,9 @@ static int cf_check ns16550_tx_ready(struct serial_port *port)
>>>> if ( ns16550_ioport_invalid(uart) )
>>>> return -EIO;
>>>>
>>>> - return ( (ns_read_reg(uart, UART_LSR) &
>>>> - uart->lsr_mask ) == uart->lsr_mask ) ? uart->fifo_size : 0;
>>>> + return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask)
>>>> + ? uart->fifo_size
>>>> + : 0;
>>> Indentation of the ? and : lines is clearly wrong here? What is the tool
>>> doing?
>> There are number of options that have influence on this formatting:
>> AllowShortBlocksOnASingleLine [4]
>> BreakBeforeTernaryOperators [5]
>> AlignOperands [6]
>>
>> I was not able to tweak these options to have the previous form.
> Right, sticking to the original form (with just the stray blanks zapped)
> would of course be best. Yet again - the tool is doing more transformations
> despite there not being any need. If, however, it does so, then one of my
> expectations would be that the ? and : are properly indented:
>
> return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask)
> ? uart->fifo_size
> : 0;
This only differs from what the tool is doing by the fact it applies
the following rule: *IndentWidth: 4*, e.g. it has indented your construct
by 4 spaces, see [1]. Which, IMO, is acceptable change.
> or
>
> return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask)
> ? uart->fifo_size : 0;
>
> or
>
> return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask
> ? uart->fifo_size
> : 0);
>
> (not going to list more variants which are all okay).
>
> In any event, a fundamental requirement of mine is that such a tool would
> only apply adjustments when and where style is actively violated. I.e. in
> the case here:
>
> return ((ns_read_reg(uart, UART_LSR) &
> uart->lsr_mask) == uart->lsr_mask) ? uart->fifo_size : 0;
>
> That's not overly neat wrapping, but in line with our style. If the other
> form was demanded going forward, I'd be curious how you'd verbally
> describe the requirement in ./CODING_STYLE.
I believe this can be stated around the fact that we need to indent,
e.g. apply the same rule as for other constructs already in use
>
>>>> @@ -275,9 +277,10 @@ static void pci_serial_early_init(struct ns16550 *uart)
>>>> #ifdef NS16550_PCI
>>>> if ( uart->bar && uart->io_base >= 0x10000 )
>>>> {
>>>> - pci_conf_write16(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
>>>> - uart->ps_bdf[2]),
>>>> - PCI_COMMAND, PCI_COMMAND_MEMORY);
>>>> + pci_conf_write16(
>>>> + PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1], uart->ps_bdf[2]),
>>>> + PCI_COMMAND,
>>>> + PCI_COMMAND_MEMORY);
>>>> return;
>>>> }
>>> Hmm, transforming a well-formed block into another well-formed one. No
>>> gain? (Same again further down.)
>> No, gain from human point of view
>> But there is a gain that it is now formatted automatically.
> See above: I'd first like to see a written, textual description for all these
> requirements. After all it needs to be possible for a human to write code
> that the tool then wouldn't try to re-arrange. Which in turn requires that
> the restrictions / constraints on the layout are spelled out.
Agree, the existing coding style document will require some extension:
at least clarifications and addition of the rules not described yet.
> I'm not looking
> forward to pass all my patches through such a tool. I can write style-
> conforming code pretty well, with - of course - occasional oversights,
Which the tool will allow not to have for less accurate developers
> right
> now. And that in multiple projects all with different styles. I expect to be
> in the position to do so also going forward. This, imo, requires that there
> be left some room for variations. Which in turn requires that the tool would
> leave alone anything that is not in conflict with the written down or defacto
> style.
Not sure it is possible with any tool at all: it just makes the changes
without distinguishing what can be skipped or not even if it does not
violate the rules. It will always seek to improve or "improve" the
code
>
>>>> @@ -335,14 +338,14 @@ static void ns16550_setup_preirq(struct ns16550 *uart)
>>>> else
>>>> {
>>>> /* Baud rate already set: read it out from the divisor latch. */
>>>> - divisor = ns_read_reg(uart, UART_DLL);
>>>> + divisor = ns_read_reg(uart, UART_DLL);
>>>> divisor |= ns_read_reg(uart, UART_DLM) << 8;
>>> An example where tabulation is being broken. There are quite a bit worse
>>> ones further down.
>> This one will be impossible to implement with clang-format IMO.
>> Because there are cases where you want this (like above) and you know why:
>> the assignments look prettier here that way. But this doesn't mean
>> that in some other places other assignments will look got for you if
>> formatted the same way.
>> The question here what is the metric (human perception?) in this case
>> and how this perception can be be programmed into clang-format
>> configuration?
> Which imo is a pretty strong argument against using any auto-formatting. At
> least up to the point where AI would end up being smart enough to mimic this
> human perception.
Well, you already have the answer: only human or AI? Obviously, this
cannot be done with any of the tools, IMO
>
>>>> @@ -350,8 +353,10 @@ static void ns16550_setup_preirq(struct ns16550 *uart)
>>>> ns_write_reg(uart, UART_MCR, UART_MCR_DTR | UART_MCR_RTS);
>>>>
>>>> /* Enable and clear the FIFOs. Set a large trigger threshold. */
>>>> - ns_write_reg(uart, UART_FCR,
>>>> - UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX | UART_FCR_TRG14);
>>>> + ns_write_reg(uart,
>>>> + UART_FCR,
>>>> + UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX |
>>>> + UART_FCR_TRG14);
>>> What's the underlying indentation rule here? The way it's re-formatted
>>> certainly looks odd to me. What we occasionally do in such cases is add
>>> parentheses:
>>>
>>> ns_write_reg(uart,
>>> UART_FCR,
>>> (UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX |
>>> UART_FCR_TRG14));
>>>
>>> Also - does the format they apply demand one argument per line? Else
>>> why not
>>>
>>> ns_write_reg(uart, UART_FCR,
>>> (UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX |
>>> UART_FCR_TRG14));
>>>
>>> Plus what's their criteria to choose between this style of function calls
>>> and the one in context higher up for calls to pci_conf_write16()?
>> This is controlled with AlignAfterOpenBracket [7]
>> So this could be:
>> *AlignAfterOpenBracket: Align*
>>
>> - ns_write_reg(uart, UART_FCR,
>> - UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX | UART_FCR_TRG14);
>> + ns_write_reg(uart,
>> + UART_FCR,
>> + UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX |
>> + UART_FCR_TRG14);
> As before indentation is bogus here, ...
>
>> *AlignAfterOpenBracket: |DontAlign*
>>
>> - ns_write_reg(uart, UART_FCR,
>> - UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX | UART_FCR_TRG14);
>> + ns_write_reg(uart,
>> + UART_FCR,
>> + UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX | UART_FCR_TRG14);
> ... and even more so here, ...
>
>> |*AlignAfterOpenBracket: |AlwaysBreak*
>>
>> - ns_write_reg(uart, UART_FCR,
>> - UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX | UART_FCR_TRG14);
>> + ns_write_reg(
>> + uart,
>> + UART_FCR,
>> + UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX | UART_FCR_TRG14);
> ... while this at least matches one of the forms we might presently use.
> Yet again - there was nothing wrong with the original layout.
So, we can stick with this option setting: AlignAfterOpenBracket: AlwaysBreak
>
>> |*AlignAfterOpenBracket: |BlockIndent*
>>
>> - ns_write_reg(uart, UART_FCR,
>> - UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX | UART_FCR_TRG14);
>> + ns_write_reg(
>> + uart,
>> + UART_FCR,
>> + UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX | UART_FCR_TRG14
>> + );
> This clearly moves too far away from our present style.
>
>>>> @@ -1706,7 +1704,7 @@ static void __init ns16550_parse_port_config(
>>>> if ( !parse_namevalue_pairs(str, uart) )
>>>> return;
>>>>
>>>> - config_parsed:
>>>> +config_parsed:
>>> This is a no-go - ./CODING_STYLE specifically says why this isn't appropriate.
>> Yes, it can't formatted as we wish. This is controlled with IndentGotoLabels [10]
>> and is a binary option, which leaves no means to disable it as both true and
>> false will re-format the code
>>
>> true:false:
>> intf(){vs.intf(){
>> if(foo()){if(foo()){
>> label1:label1:
>> bar();bar();
>> }}
>> label2:label2:
>> return1;return1;
>> }}
> If there was some indentation meant to be in that blob, it was all lost,
> I'm afraid.
Yes, sorry about that. The sample I was trying to put can be found at [2]
> Still: The name of the control being IndentGotoLabels and it
> being of boolean nature as you say, why would it do anything with an
> already-indented label?
Not really: unfortunately it is not an enum which would have "Leave" option
both true and false values *will* apply formatting, but different
>
> Jan
Thank you,
Oleksandr
[1] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#indentwidth
[2] https://clang.llvm.org/docs/ClangFormatStyleOptions.html#indentgotolabels
On 19.02.2025 14:52, Oleksandr Andrushchenko wrote:
> On 19.02.25 15:18, Jan Beulich wrote:
>> On 19.02.2025 13:39, Oleksandr Andrushchenko wrote:
>>> On 17.02.25 12:20, Jan Beulich wrote:
>>>> On 16.02.2025 11:21, Oleksandr Andrushchenko wrote:
>>>>> @@ -248,8 +249,9 @@ static int cf_check ns16550_tx_ready(struct serial_port *port)
>>>>> if ( ns16550_ioport_invalid(uart) )
>>>>> return -EIO;
>>>>>
>>>>> - return ( (ns_read_reg(uart, UART_LSR) &
>>>>> - uart->lsr_mask ) == uart->lsr_mask ) ? uart->fifo_size : 0;
>>>>> + return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask)
>>>>> + ? uart->fifo_size
>>>>> + : 0;
>>>> Indentation of the ? and : lines is clearly wrong here? What is the tool
>>>> doing?
>>> There are number of options that have influence on this formatting:
>>> AllowShortBlocksOnASingleLine [4]
>>> BreakBeforeTernaryOperators [5]
>>> AlignOperands [6]
>>>
>>> I was not able to tweak these options to have the previous form.
>> Right, sticking to the original form (with just the stray blanks zapped)
>> would of course be best. Yet again - the tool is doing more transformations
>> despite there not being any need. If, however, it does so, then one of my
>> expectations would be that the ? and : are properly indented:
>>
>> return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask)
>> ? uart->fifo_size
>> : 0;
> This only differs from what the tool is doing by the fact it applies
> the following rule: *IndentWidth: 4*, e.g. it has indented your construct
> by 4 spaces, see [1]. Which, IMO, is acceptable change.
I don't view this as acceptable. It falls in the same class then as
ns_write_reg(uart,
UART_FCR,
UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX |
UART_FCR_TRG14);
that I also commented on in my initial reply.
>> or
>>
>> return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask)
>> ? uart->fifo_size : 0;
>>
>> or
>>
>> return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask
>> ? uart->fifo_size
>> : 0);
>>
>> (not going to list more variants which are all okay).
>>
>> In any event, a fundamental requirement of mine is that such a tool would
>> only apply adjustments when and where style is actively violated. I.e. in
>> the case here:
>>
>> return ((ns_read_reg(uart, UART_LSR) &
>> uart->lsr_mask) == uart->lsr_mask) ? uart->fifo_size : 0;
>>
>> That's not overly neat wrapping, but in line with our style. If the other
>> form was demanded going forward, I'd be curious how you'd verbally
>> describe the requirement in ./CODING_STYLE.
> I believe this can be stated around the fact that we need to indent,
> e.g. apply the same rule as for other constructs already in use
Except here the tool didn't merely adjust indentation, but moved tokens
between lines.
>>>>> @@ -275,9 +277,10 @@ static void pci_serial_early_init(struct ns16550 *uart)
>>>>> #ifdef NS16550_PCI
>>>>> if ( uart->bar && uart->io_base >= 0x10000 )
>>>>> {
>>>>> - pci_conf_write16(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
>>>>> - uart->ps_bdf[2]),
>>>>> - PCI_COMMAND, PCI_COMMAND_MEMORY);
>>>>> + pci_conf_write16(
>>>>> + PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1], uart->ps_bdf[2]),
>>>>> + PCI_COMMAND,
>>>>> + PCI_COMMAND_MEMORY);
>>>>> return;
>>>>> }
>>>> Hmm, transforming a well-formed block into another well-formed one. No
>>>> gain? (Same again further down.)
>>> No, gain from human point of view
>>> But there is a gain that it is now formatted automatically.
>> See above: I'd first like to see a written, textual description for all these
>> requirements. After all it needs to be possible for a human to write code
>> that the tool then wouldn't try to re-arrange. Which in turn requires that
>> the restrictions / constraints on the layout are spelled out.
> Agree, the existing coding style document will require some extension:
> at least clarifications and addition of the rules not described yet.
>> I'm not looking
>> forward to pass all my patches through such a tool. I can write style-
>> conforming code pretty well, with - of course - occasional oversights,
> Which the tool will allow not to have for less accurate developers
I fear I don't understand this reply of yours.
>> right
>> now. And that in multiple projects all with different styles. I expect to be
>> in the position to do so also going forward. This, imo, requires that there
>> be left some room for variations. Which in turn requires that the tool would
>> leave alone anything that is not in conflict with the written down or defacto
>> style.
> Not sure it is possible with any tool at all: it just makes the changes
> without distinguishing what can be skipped or not even if it does not
> violate the rules. It will always seek to improve or "improve" the
> code
Which by now I view as the core problem.
>>>>> @@ -1706,7 +1704,7 @@ static void __init ns16550_parse_port_config(
>>>>> if ( !parse_namevalue_pairs(str, uart) )
>>>>> return;
>>>>>
>>>>> - config_parsed:
>>>>> +config_parsed:
>>>> This is a no-go - ./CODING_STYLE specifically says why this isn't appropriate.
>>> Yes, it can't formatted as we wish. This is controlled with IndentGotoLabels [10]
>>> and is a binary option, which leaves no means to disable it as both true and
>>> false will re-format the code
>>>
>>> true:false:
>>> intf(){vs.intf(){
>>> if(foo()){if(foo()){
>>> label1:label1:
>>> bar();bar();
>>> }}
>>> label2:label2:
>>> return1;return1;
>>> }}
>> If there was some indentation meant to be in that blob, it was all lost,
>> I'm afraid.
> Yes, sorry about that. The sample I was trying to put can be found at [2]
Funny, even with the setting "true" label2 there is unindented. We demand that
all labels be indented, even when - contextually - at function scope. (Nor do
we demand that labels be indented according to their - contextual - scope.)
Jan
On 19.02.25 16:05, Jan Beulich wrote:
> On 19.02.2025 14:52, Oleksandr Andrushchenko wrote:
>> On 19.02.25 15:18, Jan Beulich wrote:
>>> On 19.02.2025 13:39, Oleksandr Andrushchenko wrote:
>>>> On 17.02.25 12:20, Jan Beulich wrote:
>>>>> On 16.02.2025 11:21, Oleksandr Andrushchenko wrote:
>>>>>> @@ -248,8 +249,9 @@ static int cf_check ns16550_tx_ready(struct serial_port *port)
>>>>>> if ( ns16550_ioport_invalid(uart) )
>>>>>> return -EIO;
>>>>>>
>>>>>> - return ( (ns_read_reg(uart, UART_LSR) &
>>>>>> - uart->lsr_mask ) == uart->lsr_mask ) ? uart->fifo_size : 0;
>>>>>> + return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask)
>>>>>> + ? uart->fifo_size
>>>>>> + : 0;
>>>>> Indentation of the ? and : lines is clearly wrong here? What is the tool
>>>>> doing?
>>>> There are number of options that have influence on this formatting:
>>>> AllowShortBlocksOnASingleLine [4]
>>>> BreakBeforeTernaryOperators [5]
>>>> AlignOperands [6]
>>>>
>>>> I was not able to tweak these options to have the previous form.
>>> Right, sticking to the original form (with just the stray blanks zapped)
>>> would of course be best. Yet again - the tool is doing more transformations
>>> despite there not being any need. If, however, it does so, then one of my
>>> expectations would be that the ? and : are properly indented:
>>>
>>> return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask)
>>> ? uart->fifo_size
>>> : 0;
>> This only differs from what the tool is doing by the fact it applies
>> the following rule: *IndentWidth: 4*, e.g. it has indented your construct
>> by 4 spaces, see [1]. Which, IMO, is acceptable change.
> I don't view this as acceptable. It falls in the same class then as
>
> ns_write_reg(uart,
> UART_FCR,
> UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX |
> UART_FCR_TRG14);
>
> that I also commented on in my initial reply.
Ok, then how would you have it defined in the coding style as a rule?
Such a diversity in defining indentation? So, will you have a dedicated
rule for the ternary?
>
>>> or
>>>
>>> return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask)
>>> ? uart->fifo_size : 0;
>>>
>>> or
>>>
>>> return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask
>>> ? uart->fifo_size
>>> : 0);
>>>
>>> (not going to list more variants which are all okay).
>>>
>>> In any event, a fundamental requirement of mine is that such a tool would
>>> only apply adjustments when and where style is actively violated. I.e. in
>>> the case here:
>>>
>>> return ((ns_read_reg(uart, UART_LSR) &
>>> uart->lsr_mask) == uart->lsr_mask) ? uart->fifo_size : 0;
>>>
>>> That's not overly neat wrapping, but in line with our style. If the other
>>> form was demanded going forward, I'd be curious how you'd verbally
>>> describe the requirement in ./CODING_STYLE.
>> I believe this can be stated around the fact that we need to indent,
>> e.g. apply the same rule as for other constructs already in use
> Except here the tool didn't merely adjust indentation, but moved tokens
> between lines.
Again, if it moves, but doesn't break the style - then it is going to happen
only once while applying big-scary-patch.
>
>>>>>> @@ -275,9 +277,10 @@ static void pci_serial_early_init(struct ns16550 *uart)
>>>>>> #ifdef NS16550_PCI
>>>>>> if ( uart->bar && uart->io_base >= 0x10000 )
>>>>>> {
>>>>>> - pci_conf_write16(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
>>>>>> - uart->ps_bdf[2]),
>>>>>> - PCI_COMMAND, PCI_COMMAND_MEMORY);
>>>>>> + pci_conf_write16(
>>>>>> + PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1], uart->ps_bdf[2]),
>>>>>> + PCI_COMMAND,
>>>>>> + PCI_COMMAND_MEMORY);
>>>>>> return;
>>>>>> }
>>>>> Hmm, transforming a well-formed block into another well-formed one. No
>>>>> gain? (Same again further down.)
>>>> No, gain from human point of view
>>>> But there is a gain that it is now formatted automatically.
>>> See above: I'd first like to see a written, textual description for all these
>>> requirements. After all it needs to be possible for a human to write code
>>> that the tool then wouldn't try to re-arrange. Which in turn requires that
>>> the restrictions / constraints on the layout are spelled out.
>> Agree, the existing coding style document will require some extension:
>> at least clarifications and addition of the rules not described yet.
>>> I'm not looking
>>> forward to pass all my patches through such a tool. I can write style-
>>> conforming code pretty well, with - of course - occasional oversights,
>> Which the tool will allow not to have for less accurate developers
> I fear I don't understand this reply of yours.
I mean that you can write such a well formatted code without any tool.
But there are others who can't. Then the tool will help others to avoid
code style violations.
>
>>> right
>>> now. And that in multiple projects all with different styles. I expect to be
>>> in the position to do so also going forward. This, imo, requires that there
>>> be left some room for variations. Which in turn requires that the tool would
>>> leave alone anything that is not in conflict with the written down or defacto
>>> style.
>> Not sure it is possible with any tool at all: it just makes the changes
>> without distinguishing what can be skipped or not even if it does not
>> violate the rules. It will always seek to improve or "improve" the
>> code
> Which by now I view as the core problem.
It depends. If this is the change made once, then I don't see it as
a lifetime problem.
>
>>>>>> @@ -1706,7 +1704,7 @@ static void __init ns16550_parse_port_config(
>>>>>> if ( !parse_namevalue_pairs(str, uart) )
>>>>>> return;
>>>>>>
>>>>>> - config_parsed:
>>>>>> +config_parsed:
>>>>> This is a no-go - ./CODING_STYLE specifically says why this isn't appropriate.
>>>> Yes, it can't formatted as we wish. This is controlled with IndentGotoLabels [10]
>>>> and is a binary option, which leaves no means to disable it as both true and
>>>> false will re-format the code
>>>>
>>>> true:false:
>>>> intf(){vs.intf(){
>>>> if(foo()){if(foo()){
>>>> label1:label1:
>>>> bar();bar();
>>>> }}
>>>> label2:label2:
>>>> return1;return1;
>>>> }}
>>> If there was some indentation meant to be in that blob, it was all lost,
>>> I'm afraid.
>> Yes, sorry about that. The sample I was trying to put can be found at [2]
> Funny, even with the setting "true" label2 there is unindented. We demand that
> all labels be indented, even when - contextually - at function scope. (Nor do
> we demand that labels be indented according to their - contextual - scope.)
It seems that other projects (coding styles) see the alignment at function
scope differently. From Xen. If it was not implemented before in clang-format
could mean that either no other project use such an exception or there
are not so many projects use clang-format and didn't obviously face such
an issue
>
> Jan
Thank you,
Oleksandr
On 19.02.2025 16:40, Oleksandr Andrushchenko wrote:
> On 19.02.25 16:05, Jan Beulich wrote:
>> On 19.02.2025 14:52, Oleksandr Andrushchenko wrote:
>>> On 19.02.25 15:18, Jan Beulich wrote:
>>>> On 19.02.2025 13:39, Oleksandr Andrushchenko wrote:
>>>>> On 17.02.25 12:20, Jan Beulich wrote:
>>>>>> On 16.02.2025 11:21, Oleksandr Andrushchenko wrote:
>>>>>>> @@ -248,8 +249,9 @@ static int cf_check ns16550_tx_ready(struct serial_port *port)
>>>>>>> if ( ns16550_ioport_invalid(uart) )
>>>>>>> return -EIO;
>>>>>>>
>>>>>>> - return ( (ns_read_reg(uart, UART_LSR) &
>>>>>>> - uart->lsr_mask ) == uart->lsr_mask ) ? uart->fifo_size : 0;
>>>>>>> + return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask)
>>>>>>> + ? uart->fifo_size
>>>>>>> + : 0;
>>>>>> Indentation of the ? and : lines is clearly wrong here? What is the tool
>>>>>> doing?
>>>>> There are number of options that have influence on this formatting:
>>>>> AllowShortBlocksOnASingleLine [4]
>>>>> BreakBeforeTernaryOperators [5]
>>>>> AlignOperands [6]
>>>>>
>>>>> I was not able to tweak these options to have the previous form.
>>>> Right, sticking to the original form (with just the stray blanks zapped)
>>>> would of course be best. Yet again - the tool is doing more transformations
>>>> despite there not being any need. If, however, it does so, then one of my
>>>> expectations would be that the ? and : are properly indented:
>>>>
>>>> return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask)
>>>> ? uart->fifo_size
>>>> : 0;
>>> This only differs from what the tool is doing by the fact it applies
>>> the following rule: *IndentWidth: 4*, e.g. it has indented your construct
>>> by 4 spaces, see [1]. Which, IMO, is acceptable change.
>> I don't view this as acceptable. It falls in the same class then as
>>
>> ns_write_reg(uart,
>> UART_FCR,
>> UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX |
>> UART_FCR_TRG14);
>>
>> that I also commented on in my initial reply.
> Ok, then how would you have it defined in the coding style as a rule?
> Such a diversity in defining indentation? So, will you have a dedicated
> rule for the ternary?
Well, this feels like you're returning a request I made your way, elsewhere.
Our present, unwritten rule for wrapping lines is to match the earlier
line's indentation (or the start of the expression), plus accounting for any
pending open parentheses, braces, or brackets. Hence why some consider this
form
ns_write_reg(uart,
UART_FCR,
(UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX |
UART_FCR_TRG14));
preferable, as some tools (iirc e.g. Andrew indicated his editor does) then
are capable of inferring the intended indentation from the pending open
parentheses.
>>>> That's not overly neat wrapping, but in line with our style. If the other
>>>> form was demanded going forward, I'd be curious how you'd verbally
>>>> describe the requirement in ./CODING_STYLE.
>>> I believe this can be stated around the fact that we need to indent,
>>> e.g. apply the same rule as for other constructs already in use
>> Except here the tool didn't merely adjust indentation, but moved tokens
>> between lines.
> Again, if it moves, but doesn't break the style - then it is going to happen
> only once while applying big-scary-patch.
As to that patch: To some degree I actually like the idea of following Linux
in generally not allowing style-only patches.
>>>>>>> @@ -275,9 +277,10 @@ static void pci_serial_early_init(struct ns16550 *uart)
>>>>>>> #ifdef NS16550_PCI
>>>>>>> if ( uart->bar && uart->io_base >= 0x10000 )
>>>>>>> {
>>>>>>> - pci_conf_write16(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
>>>>>>> - uart->ps_bdf[2]),
>>>>>>> - PCI_COMMAND, PCI_COMMAND_MEMORY);
>>>>>>> + pci_conf_write16(
>>>>>>> + PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1], uart->ps_bdf[2]),
>>>>>>> + PCI_COMMAND,
>>>>>>> + PCI_COMMAND_MEMORY);
>>>>>>> return;
>>>>>>> }
>>>>>> Hmm, transforming a well-formed block into another well-formed one. No
>>>>>> gain? (Same again further down.)
>>>>> No, gain from human point of view
>>>>> But there is a gain that it is now formatted automatically.
>>>> See above: I'd first like to see a written, textual description for all these
>>>> requirements. After all it needs to be possible for a human to write code
>>>> that the tool then wouldn't try to re-arrange. Which in turn requires that
>>>> the restrictions / constraints on the layout are spelled out.
>>> Agree, the existing coding style document will require some extension:
>>> at least clarifications and addition of the rules not described yet.
>>>> I'm not looking
>>>> forward to pass all my patches through such a tool. I can write style-
>>>> conforming code pretty well, with - of course - occasional oversights,
>>> Which the tool will allow not to have for less accurate developers
>> I fear I don't understand this reply of yours.
> I mean that you can write such a well formatted code without any tool.
> But there are others who can't. Then the tool will help others to avoid
> code style violations.
And it'll screw me up (and possibly others too).
Jan
Hello, Jan!
On 19.02.25 18:01, Jan Beulich wrote:
> On 19.02.2025 16:40, Oleksandr Andrushchenko wrote:
>> On 19.02.25 16:05, Jan Beulich wrote:
>>> On 19.02.2025 14:52, Oleksandr Andrushchenko wrote:
>>>> On 19.02.25 15:18, Jan Beulich wrote:
>>>>> On 19.02.2025 13:39, Oleksandr Andrushchenko wrote:
>>>>>> On 17.02.25 12:20, Jan Beulich wrote:
>>>>>>> On 16.02.2025 11:21, Oleksandr Andrushchenko wrote:
>>>>>>>> @@ -248,8 +249,9 @@ static int cf_check ns16550_tx_ready(struct serial_port *port)
>>>>>>>> if ( ns16550_ioport_invalid(uart) )
>>>>>>>> return -EIO;
>>>>>>>>
>>>>>>>> - return ( (ns_read_reg(uart, UART_LSR) &
>>>>>>>> - uart->lsr_mask ) == uart->lsr_mask ) ? uart->fifo_size : 0;
>>>>>>>> + return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask)
>>>>>>>> + ? uart->fifo_size
>>>>>>>> + : 0;
>>>>>>> Indentation of the ? and : lines is clearly wrong here? What is the tool
>>>>>>> doing?
>>>>>> There are number of options that have influence on this formatting:
>>>>>> AllowShortBlocksOnASingleLine [4]
>>>>>> BreakBeforeTernaryOperators [5]
>>>>>> AlignOperands [6]
>>>>>>
>>>>>> I was not able to tweak these options to have the previous form.
>>>>> Right, sticking to the original form (with just the stray blanks zapped)
>>>>> would of course be best. Yet again - the tool is doing more transformations
>>>>> despite there not being any need. If, however, it does so, then one of my
>>>>> expectations would be that the ? and : are properly indented:
>>>>>
>>>>> return ((ns_read_reg(uart, UART_LSR) & uart->lsr_mask) == uart->lsr_mask)
>>>>> ? uart->fifo_size
>>>>> : 0;
>>>> This only differs from what the tool is doing by the fact it applies
>>>> the following rule: *IndentWidth: 4*, e.g. it has indented your construct
>>>> by 4 spaces, see [1]. Which, IMO, is acceptable change.
>>> I don't view this as acceptable. It falls in the same class then as
>>>
>>> ns_write_reg(uart,
>>> UART_FCR,
>>> UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX |
>>> UART_FCR_TRG14);
>>>
>>> that I also commented on in my initial reply.
>> Ok, then how would you have it defined in the coding style as a rule?
>> Such a diversity in defining indentation? So, will you have a dedicated
>> rule for the ternary?
> Well, this feels like you're returning a request I made your way, elsewhere.
> Our present, unwritten rule for wrapping lines is to match the earlier
> line's indentation (or the start of the expression), plus accounting for any
> pending open parentheses, braces, or brackets. Hence why some consider this
> form
>
> ns_write_reg(uart,
> UART_FCR,
> (UART_FCR_ENABLE | UART_FCR_CLRX | UART_FCR_CLTX |
> UART_FCR_TRG14));
>
> preferable, as some tools (iirc e.g. Andrew indicated his editor does) then
> are capable of inferring the intended indentation from the pending open
> parentheses.
I do understand that the tool needs to do the job and be able to fit
any coding style exists and not vice versa. But this is only in an
ideal world which doesn't exist yet: those tools are also developed by
an open source community and they also have some limited bandwidth.
I mean that bot Xen and some magic tool might need to co-exist and
accept each other. Or just decide not to use any.
>>>>> That's not overly neat wrapping, but in line with our style. If the other
>>>>> form was demanded going forward, I'd be curious how you'd verbally
>>>>> describe the requirement in ./CODING_STYLE.
>>>> I believe this can be stated around the fact that we need to indent,
>>>> e.g. apply the same rule as for other constructs already in use
>>> Except here the tool didn't merely adjust indentation, but moved tokens
>>> between lines.
>> Again, if it moves, but doesn't break the style - then it is going to happen
>> only once while applying big-scary-patch.
> As to that patch: To some degree I actually like the idea of following Linux
> in generally not allowing style-only patches.
Well, yes. I can suggest that if we decide to provide a series of
style-only patches that we commit those with a fake authorship,
e.g. "Author: clang-format@xenproject.org"
>
>>>>>>>> @@ -275,9 +277,10 @@ static void pci_serial_early_init(struct ns16550 *uart)
>>>>>>>> #ifdef NS16550_PCI
>>>>>>>> if ( uart->bar && uart->io_base >= 0x10000 )
>>>>>>>> {
>>>>>>>> - pci_conf_write16(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
>>>>>>>> - uart->ps_bdf[2]),
>>>>>>>> - PCI_COMMAND, PCI_COMMAND_MEMORY);
>>>>>>>> + pci_conf_write16(
>>>>>>>> + PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1], uart->ps_bdf[2]),
>>>>>>>> + PCI_COMMAND,
>>>>>>>> + PCI_COMMAND_MEMORY);
>>>>>>>> return;
>>>>>>>> }
>>>>>>> Hmm, transforming a well-formed block into another well-formed one. No
>>>>>>> gain? (Same again further down.)
>>>>>> No, gain from human point of view
>>>>>> But there is a gain that it is now formatted automatically.
>>>>> See above: I'd first like to see a written, textual description for all these
>>>>> requirements. After all it needs to be possible for a human to write code
>>>>> that the tool then wouldn't try to re-arrange. Which in turn requires that
>>>>> the restrictions / constraints on the layout are spelled out.
>>>> Agree, the existing coding style document will require some extension:
>>>> at least clarifications and addition of the rules not described yet.
>>>>> I'm not looking
>>>>> forward to pass all my patches through such a tool. I can write style-
>>>>> conforming code pretty well, with - of course - occasional oversights,
>>>> Which the tool will allow not to have for less accurate developers
>>> I fear I don't understand this reply of yours.
>> I mean that you can write such a well formatted code without any tool.
>> But there are others who can't. Then the tool will help others to avoid
>> code style violations.
> And it'll screw me up (and possibly others too).
>
> Jan
Thank you,
Oleksandr
Use .clang-format to format ACPI tables.
Signed-off-by: Oleksandr Andrushchenko <andr2000@gmail.com>
---
xen/drivers/acpi/tables.c | 809 ++++++++++++++++++++------------------
1 file changed, 435 insertions(+), 374 deletions(-)
diff --git a/xen/drivers/acpi/tables.c b/xen/drivers/acpi/tables.c
index 20aed8929b86..cf9085ac2fc7 100644
--- a/xen/drivers/acpi/tables.c
+++ b/xen/drivers/acpi/tables.c
@@ -38,362 +38,419 @@
bool __initdata opt_acpi_verbose;
-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",
+ "level" };
static int acpi_apic_instance __initdata;
void __init acpi_table_print_madt_entry(struct acpi_subtable_header *header)
{
- if (!header)
- return;
-
- switch (header->type) {
-
- case ACPI_MADT_TYPE_LOCAL_APIC:
- if (opt_acpi_verbose)
- {
- const struct acpi_madt_local_apic *p =
- container_of(header, const struct acpi_madt_local_apic, header);
-
- printk(KERN_INFO PREFIX
- "LAPIC (acpi_id[0x%02x] lapic_id[0x%02x] %s)\n",
- p->processor_id, p->id,
- (p->lapic_flags & ACPI_MADT_ENABLED) ? "enabled" : "disabled");
- }
- break;
-
- case ACPI_MADT_TYPE_LOCAL_X2APIC:
- if (opt_acpi_verbose)
- {
- const struct acpi_madt_local_x2apic *p =
- container_of(header, const struct acpi_madt_local_x2apic, header);
-
- printk(KERN_INFO PREFIX
- "X2APIC (apic_id[0x%02x] uid[0x%02x] %s)\n",
- p->local_apic_id, p->uid,
- (p->lapic_flags & ACPI_MADT_ENABLED) ?
- "enabled" : "disabled");
- }
- break;
-
- case ACPI_MADT_TYPE_IO_APIC:
- {
- const struct acpi_madt_io_apic *p =
- container_of(header, const struct acpi_madt_io_apic, header);
-
- printk(KERN_INFO PREFIX
- "IOAPIC (id[0x%02x] address[0x%08x] gsi_base[%d])\n",
- p->id, p->address, p->global_irq_base);
- }
- break;
-
- case ACPI_MADT_TYPE_INTERRUPT_OVERRIDE:
- {
- const struct acpi_madt_interrupt_override *p =
- container_of(header, const struct acpi_madt_interrupt_override, header);
-
- printk(KERN_INFO PREFIX
- "INT_SRC_OVR (bus %d bus_irq %d global_irq %d %s %s)\n",
- p->bus, p->source_irq, p->global_irq,
- mps_inti_flags_polarity[p->inti_flags & ACPI_MADT_POLARITY_MASK],
- mps_inti_flags_trigger[(p->inti_flags & ACPI_MADT_TRIGGER_MASK) >> 2]);
- if (p->inti_flags &
- ~(ACPI_MADT_POLARITY_MASK | ACPI_MADT_TRIGGER_MASK))
- printk(KERN_INFO PREFIX
- "INT_SRC_OVR unexpected reserved flags: %#x\n",
- p->inti_flags &
- ~(ACPI_MADT_POLARITY_MASK | ACPI_MADT_TRIGGER_MASK));
-
- }
- break;
-
- case ACPI_MADT_TYPE_NMI_SOURCE:
- {
- const struct acpi_madt_nmi_source *p =
- container_of(header, const struct acpi_madt_nmi_source, header);
-
- printk(KERN_INFO PREFIX
- "NMI_SRC (%s %s global_irq %d)\n",
- mps_inti_flags_polarity[p->inti_flags & ACPI_MADT_POLARITY_MASK],
- mps_inti_flags_trigger[(p->inti_flags & ACPI_MADT_TRIGGER_MASK) >> 2],
- p->global_irq);
- }
- break;
-
- case ACPI_MADT_TYPE_LOCAL_APIC_NMI:
- if (opt_acpi_verbose)
- {
- const struct acpi_madt_local_apic_nmi *p =
- container_of(header, const struct acpi_madt_local_apic_nmi, header);
-
- printk(KERN_INFO PREFIX
- "LAPIC_NMI (acpi_id[0x%02x] %s %s lint[%#x])\n",
- p->processor_id,
- mps_inti_flags_polarity[p->inti_flags & ACPI_MADT_POLARITY_MASK ],
- mps_inti_flags_trigger[(p->inti_flags & ACPI_MADT_TRIGGER_MASK) >> 2],
- p->lint);
- }
- break;
-
- case ACPI_MADT_TYPE_LOCAL_X2APIC_NMI:
- if (opt_acpi_verbose)
- {
- const struct acpi_madt_local_x2apic_nmi *p =
- container_of(header, const struct acpi_madt_local_x2apic_nmi, header);
- unsigned int polarity = MASK_EXTR(p->inti_flags, ACPI_MADT_POLARITY_MASK);
- unsigned int trigger = MASK_EXTR(p->inti_flags, ACPI_MADT_TRIGGER_MASK);
-
- printk(KERN_INFO PREFIX
- "X2APIC_NMI (uid[0x%02x] %s %s lint[%#x])\n",
- p->uid,
- mps_inti_flags_polarity[polarity],
- mps_inti_flags_trigger[trigger],
- p->lint);
- }
- break;
-
- case ACPI_MADT_TYPE_LOCAL_APIC_OVERRIDE:
- {
- const struct acpi_madt_local_apic_override *p =
- container_of(header, const struct acpi_madt_local_apic_override, header);
-
- printk(KERN_INFO PREFIX
- "LAPIC_ADDR_OVR (address[%p])\n",
- (void *)(unsigned long)p->address);
- }
- break;
-
- case ACPI_MADT_TYPE_IO_SAPIC:
- {
- const struct acpi_madt_io_sapic *p =
- container_of(header, const struct acpi_madt_io_sapic, header);
-
- printk(KERN_INFO PREFIX
- "IOSAPIC (id[%#x] address[%p] gsi_base[%d])\n",
- p->id, (void *)(unsigned long)p->address,
- p->global_irq_base);
- }
- break;
-
- case ACPI_MADT_TYPE_LOCAL_SAPIC:
- if (opt_acpi_verbose)
- {
- const struct acpi_madt_local_sapic *p =
- container_of(header, const struct acpi_madt_local_sapic, header);
-
- printk(KERN_INFO PREFIX
- "LSAPIC (acpi_id[0x%02x] lsapic_id[0x%02x] lsapic_eid[0x%02x] %s)\n",
- p->processor_id, p->id, p->eid,
- (p->lapic_flags & ACPI_MADT_ENABLED) ? "enabled" : "disabled");
- }
- break;
-
- case ACPI_MADT_TYPE_INTERRUPT_SOURCE:
- {
- const struct acpi_madt_interrupt_source *p =
- container_of(header, const struct acpi_madt_interrupt_source, header);
-
- printk(KERN_INFO PREFIX
- "PLAT_INT_SRC (%s %s type[%#x] id[0x%04x] eid[%#x] iosapic_vector[%#x] global_irq[%#x]\n",
- mps_inti_flags_polarity[p->inti_flags & ACPI_MADT_POLARITY_MASK],
- mps_inti_flags_trigger[(p->inti_flags & ACPI_MADT_TRIGGER_MASK) >> 2],
- p->type, p->id, p->eid, p->io_sapic_vector,
- p->global_irq);
- }
- break;
-
- case ACPI_MADT_TYPE_GENERIC_INTERRUPT:
- {
- struct acpi_madt_generic_interrupt *p =
- container_of(header, struct acpi_madt_generic_interrupt, header);
-
- printk(KERN_DEBUG PREFIX
- "GICC (acpi_id[0x%04x] address[0x%"PRIx64"] MPIDR[0x%"PRIx64"] %s)\n",
- p->uid, p->base_address,
- p->arm_mpidr,
- (p->flags & ACPI_MADT_ENABLED) ? "enabled" : "disabled");
-
- }
- break;
-
- case ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR:
- {
- struct acpi_madt_generic_distributor *p =
- container_of(header, struct acpi_madt_generic_distributor, 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);
- }
- break;
-
- default:
- printk(KERN_WARNING PREFIX
- "Found unsupported MADT entry (type = %#x)\n",
- header->type);
- break;
- }
+ if ( !header )
+ return;
+
+ switch ( header->type )
+ {
+ case ACPI_MADT_TYPE_LOCAL_APIC:
+ if ( opt_acpi_verbose )
+ {
+ const struct acpi_madt_local_apic *p =
+ container_of(header, const struct acpi_madt_local_apic, header);
+
+ printk(KERN_INFO PREFIX
+ "LAPIC (acpi_id[0x%02x] lapic_id[0x%02x] %s)\n",
+ p->processor_id,
+ p->id,
+ (p->lapic_flags & ACPI_MADT_ENABLED) ? "enabled"
+ : "disabled");
+ }
+ break;
+
+ case ACPI_MADT_TYPE_LOCAL_X2APIC:
+ if ( opt_acpi_verbose )
+ {
+ const struct acpi_madt_local_x2apic *p =
+ container_of(header,
+ const struct acpi_madt_local_x2apic,
+ header);
+
+ printk(KERN_INFO PREFIX "X2APIC (apic_id[0x%02x] uid[0x%02x] %s)\n",
+ p->local_apic_id,
+ p->uid,
+ (p->lapic_flags & ACPI_MADT_ENABLED) ? "enabled"
+ : "disabled");
+ }
+ break;
+
+ case ACPI_MADT_TYPE_IO_APIC:
+ {
+ const struct acpi_madt_io_apic *p =
+ container_of(header, const struct acpi_madt_io_apic, header);
+
+ printk(KERN_INFO PREFIX
+ "IOAPIC (id[0x%02x] address[0x%08x] gsi_base[%d])\n",
+ p->id,
+ p->address,
+ p->global_irq_base);
+ }
+ break;
+
+ case ACPI_MADT_TYPE_INTERRUPT_OVERRIDE:
+ {
+ const struct acpi_madt_interrupt_override *p =
+ container_of(header,
+ const struct acpi_madt_interrupt_override,
+ header);
+
+ printk(KERN_INFO PREFIX
+ "INT_SRC_OVR (bus %d bus_irq %d global_irq %d %s %s)\n",
+ p->bus,
+ p->source_irq,
+ p->global_irq,
+ mps_inti_flags_polarity[p->inti_flags & ACPI_MADT_POLARITY_MASK],
+ mps_inti_flags_trigger[(p->inti_flags & ACPI_MADT_TRIGGER_MASK) >>
+ 2]);
+ if ( p->inti_flags &
+ ~(ACPI_MADT_POLARITY_MASK | ACPI_MADT_TRIGGER_MASK) )
+ printk(KERN_INFO PREFIX
+ "INT_SRC_OVR unexpected reserved flags: %#x\n",
+ p->inti_flags &
+ ~(ACPI_MADT_POLARITY_MASK | ACPI_MADT_TRIGGER_MASK));
+ }
+ break;
+
+ case ACPI_MADT_TYPE_NMI_SOURCE:
+ {
+ const struct acpi_madt_nmi_source *p =
+ container_of(header, const struct acpi_madt_nmi_source, header);
+
+ printk(KERN_INFO PREFIX "NMI_SRC (%s %s global_irq %d)\n",
+ mps_inti_flags_polarity[p->inti_flags & ACPI_MADT_POLARITY_MASK],
+ mps_inti_flags_trigger[(p->inti_flags & ACPI_MADT_TRIGGER_MASK) >>
+ 2],
+ p->global_irq);
+ }
+ break;
+
+ case ACPI_MADT_TYPE_LOCAL_APIC_NMI:
+ if ( opt_acpi_verbose )
+ {
+ const struct acpi_madt_local_apic_nmi *p =
+ container_of(header,
+ const struct acpi_madt_local_apic_nmi,
+ header);
+
+ printk(
+ KERN_INFO PREFIX
+ "LAPIC_NMI (acpi_id[0x%02x] %s %s lint[%#x])\n",
+ p->processor_id,
+ mps_inti_flags_polarity[p->inti_flags & ACPI_MADT_POLARITY_MASK],
+ mps_inti_flags_trigger
+ [(p->inti_flags & ACPI_MADT_TRIGGER_MASK) >> 2],
+ p->lint);
+ }
+ break;
+
+ case ACPI_MADT_TYPE_LOCAL_X2APIC_NMI:
+ if ( opt_acpi_verbose )
+ {
+ const struct acpi_madt_local_x2apic_nmi *p =
+ container_of(header,
+ const struct acpi_madt_local_x2apic_nmi,
+ header);
+ unsigned int polarity = MASK_EXTR(p->inti_flags,
+ ACPI_MADT_POLARITY_MASK);
+ unsigned int trigger = MASK_EXTR(p->inti_flags,
+ ACPI_MADT_TRIGGER_MASK);
+
+ printk(KERN_INFO PREFIX
+ "X2APIC_NMI (uid[0x%02x] %s %s lint[%#x])\n",
+ p->uid,
+ mps_inti_flags_polarity[polarity],
+ mps_inti_flags_trigger[trigger],
+ p->lint);
+ }
+ break;
+
+ case ACPI_MADT_TYPE_LOCAL_APIC_OVERRIDE:
+ {
+ const struct acpi_madt_local_apic_override *p =
+ container_of(header,
+ const struct acpi_madt_local_apic_override,
+ header);
+
+ printk(KERN_INFO PREFIX "LAPIC_ADDR_OVR (address[%p])\n",
+ (void *)(unsigned long)p->address);
+ }
+ break;
+
+ case ACPI_MADT_TYPE_IO_SAPIC:
+ {
+ const struct acpi_madt_io_sapic *p =
+ container_of(header, const struct acpi_madt_io_sapic, header);
+
+ printk(KERN_INFO PREFIX "IOSAPIC (id[%#x] address[%p] gsi_base[%d])\n",
+ p->id,
+ (void *)(unsigned long)p->address,
+ p->global_irq_base);
+ }
+ break;
+
+ case ACPI_MADT_TYPE_LOCAL_SAPIC:
+ if ( opt_acpi_verbose )
+ {
+ const struct acpi_madt_local_sapic *p =
+ container_of(header,
+ const struct acpi_madt_local_sapic,
+ header);
+
+ printk(
+ KERN_INFO PREFIX
+ "LSAPIC (acpi_id[0x%02x] lsapic_id[0x%02x] lsapic_eid[0x%02x] %s)\n",
+ p->processor_id,
+ p->id,
+ p->eid,
+ (p->lapic_flags & ACPI_MADT_ENABLED) ? "enabled" : "disabled");
+ }
+ break;
+
+ case ACPI_MADT_TYPE_INTERRUPT_SOURCE:
+ {
+ const struct acpi_madt_interrupt_source *p =
+ container_of(header,
+ const struct acpi_madt_interrupt_source,
+ header);
+
+ printk(
+ KERN_INFO PREFIX
+ "PLAT_INT_SRC (%s %s type[%#x] id[0x%04x] eid[%#x] iosapic_vector[%#x] global_irq[%#x]\n",
+ mps_inti_flags_polarity[p->inti_flags & ACPI_MADT_POLARITY_MASK],
+ mps_inti_flags_trigger[(p->inti_flags & ACPI_MADT_TRIGGER_MASK) >>
+ 2],
+ p->type,
+ p->id,
+ p->eid,
+ p->io_sapic_vector,
+ p->global_irq);
+ }
+ break;
+
+ case ACPI_MADT_TYPE_GENERIC_INTERRUPT:
+ {
+ struct acpi_madt_generic_interrupt *p =
+ container_of(header, struct acpi_madt_generic_interrupt, header);
+
+ printk(KERN_DEBUG PREFIX "GICC (acpi_id[0x%04x] address[0x%" PRIx64
+ "] MPIDR[0x%" PRIx64 "] %s)\n",
+ p->uid,
+ p->base_address,
+ p->arm_mpidr,
+ (p->flags & ACPI_MADT_ENABLED) ? "enabled" : "disabled");
+ }
+ break;
+
+ case ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR:
+ {
+ struct acpi_madt_generic_distributor *p =
+ container_of(header, struct acpi_madt_generic_distributor, 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);
+ }
+ break;
+
+ default:
+ printk(KERN_WARNING PREFIX
+ "Found unsupported MADT entry (type = %#x)\n",
+ header->type);
+ break;
+ }
}
-static struct acpi_subtable_header * __init
+static struct acpi_subtable_header *__init
acpi_get_entry(const char *id, unsigned long table_size,
- const struct acpi_table_header *table_header,
- enum acpi_madt_type entry_id, unsigned int entry_index)
+ const struct acpi_table_header *table_header,
+ enum acpi_madt_type entry_id, unsigned int entry_index)
{
- struct acpi_subtable_header *entry;
- int count = 0;
- unsigned long table_end;
-
- if (!table_size)
- return NULL;
-
- if (!table_header) {
- printk(KERN_WARNING PREFIX "%4.4s not present\n", id);
- return NULL;
- }
-
- table_end = (unsigned long)table_header + table_header->length;
-
- /* Parse all entries looking for a match. */
- entry = (void *)table_header + table_size;
-
- while ((unsigned long)(entry + 1) < table_end) {
- if (entry->length < sizeof(*entry)) {
- printk(KERN_ERR PREFIX "[%4.4s:%#x] Invalid length\n",
- id, entry_id);
- return NULL;
- }
-
- if (entry->type == entry_id) {
- if (count == entry_index)
- return entry;
- count++;
- }
-
- entry = (void *)entry + entry->length;
- }
-
- return NULL;
+ struct acpi_subtable_header *entry;
+ int count = 0;
+ unsigned long table_end;
+
+ if ( !table_size )
+ return NULL;
+
+ if ( !table_header )
+ {
+ printk(KERN_WARNING PREFIX "%4.4s not present\n", id);
+ return NULL;
+ }
+
+ table_end = (unsigned long)table_header + table_header->length;
+
+ /* Parse all entries looking for a match. */
+ entry = (void *)table_header + table_size;
+
+ while ( (unsigned long)(entry + 1) < table_end )
+ {
+ if ( entry->length < sizeof(*entry) )
+ {
+ printk(KERN_ERR PREFIX "[%4.4s:%#x] Invalid length\n",
+ id,
+ entry_id);
+ return NULL;
+ }
+
+ if ( entry->type == entry_id )
+ {
+ if ( count == entry_index )
+ return entry;
+ count++;
+ }
+
+ entry = (void *)entry + entry->length;
+ }
+
+ return NULL;
}
-struct acpi_subtable_header * __init
+struct acpi_subtable_header *__init
acpi_table_get_entry_madt(enum acpi_madt_type entry_id,
- unsigned int entry_index)
+ unsigned int entry_index)
{
- struct acpi_table_header *table_header;
- acpi_status status;
-
- status = acpi_get_table(ACPI_SIG_MADT, acpi_apic_instance,
- &table_header);
- if (ACPI_FAILURE(status)) {
- printk(KERN_WARNING PREFIX "%4.4s not present\n",
- ACPI_SIG_MADT);
- return NULL;
- }
-
- return acpi_get_entry(ACPI_SIG_MADT, sizeof(struct acpi_table_madt),
- table_header, entry_id, entry_index);
+ struct acpi_table_header *table_header;
+ acpi_status status;
+
+ status = acpi_get_table(ACPI_SIG_MADT, acpi_apic_instance, &table_header);
+ if ( ACPI_FAILURE(status) )
+ {
+ printk(KERN_WARNING PREFIX "%4.4s not present\n", ACPI_SIG_MADT);
+ return NULL;
+ }
+
+ return acpi_get_entry(ACPI_SIG_MADT,
+ sizeof(struct acpi_table_madt),
+ table_header,
+ entry_id,
+ entry_index);
}
-int __init
-acpi_parse_entries(const char *id, unsigned long table_size,
- acpi_table_entry_handler handler,
- struct acpi_table_header *table_header,
- int entry_id, unsigned int max_entries)
+int __init acpi_parse_entries(const char *id, unsigned long table_size,
+ acpi_table_entry_handler handler,
+ struct acpi_table_header *table_header,
+ int entry_id, unsigned int max_entries)
{
- struct acpi_subtable_header *entry;
- int count = 0;
- unsigned long table_end;
-
- if (acpi_disabled)
- return -ENODEV;
-
- if (!id || !handler)
- return -EINVAL;
-
- if (!table_size)
- return -EINVAL;
-
- if (!table_header) {
- printk(KERN_WARNING PREFIX "%4.4s not present\n", id);
- return -ENODEV;
- }
-
- table_end = (unsigned long)table_header + table_header->length;
-
- /* Parse all entries looking for a match. */
-
- entry = (struct acpi_subtable_header *)
- ((unsigned long)table_header + table_size);
-
- while (((unsigned long)entry) + sizeof(struct acpi_subtable_header) <
- table_end) {
- if (entry->length < sizeof(*entry)) {
- printk(KERN_ERR PREFIX "[%4.4s:%#x] Invalid length\n",
- id, entry_id);
- return -ENODATA;
- }
-
- if (entry->type == entry_id
- && (!max_entries || count < max_entries)) {
- if (handler(entry, table_end))
- return -EINVAL;
-
- count++;
- }
-
- entry = (struct acpi_subtable_header *)
- ((unsigned long)entry + entry->length);
- }
-
- if (max_entries && count > max_entries) {
- printk(KERN_WARNING PREFIX "[%4.4s:%#x] ignored %i entries of "
- "%i found\n", id, entry_id, count - max_entries, count);
- }
-
- return count;
+ struct acpi_subtable_header *entry;
+ int count = 0;
+ unsigned long table_end;
+
+ if ( acpi_disabled )
+ return -ENODEV;
+
+ if ( !id || !handler )
+ return -EINVAL;
+
+ if ( !table_size )
+ return -EINVAL;
+
+ if ( !table_header )
+ {
+ printk(KERN_WARNING PREFIX "%4.4s not present\n", id);
+ return -ENODEV;
+ }
+
+ table_end = (unsigned long)table_header + table_header->length;
+
+ /* Parse all entries looking for a match. */
+
+ entry = (struct acpi_subtable_header *)((unsigned long)table_header +
+ table_size);
+
+ while ( ((unsigned long)entry) + sizeof(struct acpi_subtable_header) <
+ table_end )
+ {
+ if ( entry->length < sizeof(*entry) )
+ {
+ printk(KERN_ERR PREFIX "[%4.4s:%#x] Invalid length\n",
+ id,
+ entry_id);
+ return -ENODATA;
+ }
+
+ if ( entry->type == entry_id && (!max_entries || count < max_entries) )
+ {
+ if ( handler(entry, table_end) )
+ return -EINVAL;
+
+ count++;
+ }
+
+ entry = (struct acpi_subtable_header *)((unsigned long)entry +
+ entry->length);
+ }
+
+ if ( max_entries && count > max_entries )
+ {
+ printk(KERN_WARNING PREFIX
+ "[%4.4s:%#x] ignored %i entries of " "%i found\n",
+ id,
+ entry_id,
+ count - max_entries,
+ count);
+ }
+
+ return count;
}
-int __init
-acpi_table_parse_entries(const char *id,
- unsigned long table_size,
- int entry_id,
- acpi_table_entry_handler handler,
- unsigned int max_entries)
+int __init acpi_table_parse_entries(const char *id, unsigned long table_size,
+ int entry_id,
+ acpi_table_entry_handler handler,
+ unsigned int max_entries)
{
- struct acpi_table_header *table_header = NULL;
- u32 instance = 0;
-
- if (acpi_disabled)
- return -ENODEV;
-
- if (!id || !handler)
- return -EINVAL;
-
- if (!strncmp(id, ACPI_SIG_MADT, 4))
- instance = acpi_apic_instance;
-
- acpi_get_table(id, instance, &table_header);
- if (!table_header) {
- printk(KERN_WARNING PREFIX "%4.4s not present\n", id);
- return -ENODEV;
- }
-
- return acpi_parse_entries(id, table_size, handler, table_header,
- entry_id, max_entries);
+ struct acpi_table_header *table_header = NULL;
+ u32 instance = 0;
+
+ if ( acpi_disabled )
+ return -ENODEV;
+
+ if ( !id || !handler )
+ return -EINVAL;
+
+ if ( !strncmp(id, ACPI_SIG_MADT, 4) )
+ instance = acpi_apic_instance;
+
+ acpi_get_table(id, instance, &table_header);
+ if ( !table_header )
+ {
+ printk(KERN_WARNING PREFIX "%4.4s not present\n", id);
+ return -ENODEV;
+ }
+
+ return acpi_parse_entries(id,
+ table_size,
+ handler,
+ table_header,
+ entry_id,
+ max_entries);
}
-int __init
-acpi_table_parse_madt(enum acpi_madt_type id,
- acpi_table_entry_handler handler, unsigned int max_entries)
+int __init acpi_table_parse_madt(enum acpi_madt_type id,
+ acpi_table_entry_handler handler,
+ unsigned int max_entries)
{
- return acpi_table_parse_entries(ACPI_SIG_MADT,
- sizeof(struct acpi_table_madt), id,
- handler, max_entries);
+ return acpi_table_parse_entries(ACPI_SIG_MADT,
+ sizeof(struct acpi_table_madt),
+ id,
+ handler,
+ max_entries);
}
/**
@@ -407,23 +464,25 @@ acpi_table_parse_madt(enum acpi_madt_type id,
*/
int __init acpi_table_parse(const char *id, acpi_table_handler handler)
{
- struct acpi_table_header *table = NULL;
+ struct acpi_table_header *table = NULL;
- if (acpi_disabled)
- return -ENODEV;
+ if ( acpi_disabled )
+ return -ENODEV;
- if (!handler)
- return -EINVAL;
+ if ( !handler )
+ return -EINVAL;
- if (strncmp(id, ACPI_SIG_MADT, 4) == 0)
- acpi_get_table(id, acpi_apic_instance, &table);
- else
- acpi_get_table(id, 0, &table);
+ if ( strncmp(id, ACPI_SIG_MADT, 4) == 0 )
+ acpi_get_table(id, acpi_apic_instance, &table);
+ else
+ acpi_get_table(id, 0, &table);
- if (table) {
- return handler(table);
- } else
- return -ENODEV;
+ if ( table )
+ {
+ return handler(table);
+ }
+ else
+ return -ENODEV;
}
/*
@@ -433,22 +492,23 @@ int __init acpi_table_parse(const char *id, acpi_table_handler handler)
*/
static void __init check_multiple_madt(void)
{
- struct acpi_table_header *table = NULL;
-
- acpi_get_table(ACPI_SIG_MADT, 2, &table);
- if (table) {
- printk(KERN_WARNING PREFIX
- "BIOS bug: multiple APIC/MADT found,"
- " using %d\n", acpi_apic_instance);
- printk(KERN_WARNING PREFIX
- "If \"acpi_apic_instance=%d\" works better, "
- "notify linux-acpi@vger.kernel.org\n",
- acpi_apic_instance ? 0 : 2);
-
- } else
- acpi_apic_instance = 0;
-
- return;
+ struct acpi_table_header *table = NULL;
+
+ acpi_get_table(ACPI_SIG_MADT, 2, &table);
+ if ( table )
+ {
+ printk(KERN_WARNING PREFIX
+ "BIOS bug: multiple APIC/MADT found," " using %d\n",
+ acpi_apic_instance);
+ printk(
+ KERN_WARNING PREFIX
+ "If \"acpi_apic_instance=%d\" works better, " "notify linux-acpi@vger.kernel.org\n",
+ acpi_apic_instance ? 0 : 2);
+ }
+ else
+ acpi_apic_instance = 0;
+
+ return;
}
/*
@@ -462,25 +522,26 @@ static void __init check_multiple_madt(void)
int __init acpi_table_init(void)
{
- acpi_status status;
+ acpi_status status;
- status = acpi_initialize_tables(NULL, ACPI_MAX_TABLES, 0);
- if (ACPI_FAILURE(status))
- return -EINVAL;
+ status = acpi_initialize_tables(NULL, ACPI_MAX_TABLES, 0);
+ if ( ACPI_FAILURE(status) )
+ return -EINVAL;
- check_multiple_madt();
- return 0;
+ check_multiple_madt();
+ return 0;
}
static int __init cf_check acpi_parse_apic_instance(const char *str)
{
- const char *q;
+ const char *q;
- acpi_apic_instance = simple_strtoul(str, &q, 0);
+ acpi_apic_instance = simple_strtoul(str, &q, 0);
- printk(KERN_NOTICE PREFIX "Shall use APIC/MADT table %d\n",
- acpi_apic_instance);
+ printk(KERN_NOTICE PREFIX "Shall use APIC/MADT table %d\n",
+ acpi_apic_instance);
- return *q ? -EINVAL : 0;
+ return *q ? -EINVAL : 0;
}
+
custom_param("acpi_apic_instance", acpi_parse_apic_instance);
--
2.25.1© 2016 - 2025 Red Hat, Inc.