Message ID | 20201021221253.GA73207@mattapan.m5p.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | xen/acpi: Don't fail if SPCR table is absent | expand |
On Wed, 21 Oct 2020, Elliott Mitchell wrote: > Absence of a SPCR table likely means the console is a framebuffer. In > such case acpi_iomem_deny_access() should NOT fail. > > Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > xen/arch/arm/acpi/domain_build.c | 19 ++++++++++--------- > 1 file changed, 10 insertions(+), 9 deletions(-) > > diff --git a/xen/arch/arm/acpi/domain_build.c b/xen/arch/arm/acpi/domain_build.c > index 1b1cfabb00..bbdc90f92c 100644 > --- a/xen/arch/arm/acpi/domain_build.c > +++ b/xen/arch/arm/acpi/domain_build.c > @@ -42,17 +42,18 @@ static int __init acpi_iomem_deny_access(struct domain *d) > status = acpi_get_table(ACPI_SIG_SPCR, 0, > (struct acpi_table_header **)&spcr); > > - if ( ACPI_FAILURE(status) ) > + if ( ACPI_SUCCESS(status) ) > { > - printk("Failed to get SPCR table\n"); > - return -EINVAL; > + mfn = spcr->serial_port.address >> PAGE_SHIFT; > + /* Deny MMIO access for UART */ > + rc = iomem_deny_access(d, mfn, mfn + 1); > + if ( rc ) > + return rc; > + } > + else > + { > + printk("Failed to get SPCR table, Xen console may be unavailable\n"); > } > - > - mfn = spcr->serial_port.address >> PAGE_SHIFT; > - /* Deny MMIO access for UART */ > - rc = iomem_deny_access(d, mfn, mfn + 1); > - if ( rc ) > - return rc; > > /* Deny MMIO access for GIC regions */ > return gic_iomem_deny_access(d);
On 22.10.2020 00:12, Elliott Mitchell wrote: > --- a/xen/arch/arm/acpi/domain_build.c > +++ b/xen/arch/arm/acpi/domain_build.c > @@ -42,17 +42,18 @@ static int __init acpi_iomem_deny_access(struct domain *d) > status = acpi_get_table(ACPI_SIG_SPCR, 0, > (struct acpi_table_header **)&spcr); > > - if ( ACPI_FAILURE(status) ) > + if ( ACPI_SUCCESS(status) ) > { > - printk("Failed to get SPCR table\n"); > - return -EINVAL; > + mfn = spcr->serial_port.address >> PAGE_SHIFT; > + /* Deny MMIO access for UART */ > + rc = iomem_deny_access(d, mfn, mfn + 1); > + if ( rc ) > + return rc; > + } > + else > + { > + printk("Failed to get SPCR table, Xen console may be unavailable\n"); > } Nit: While I see you've got Stefano's R-b already, I Xen we typically omit the braces here. Jan
On Thu, Oct 22, 2020 at 09:42:17AM +0200, Jan Beulich wrote: > On 22.10.2020 00:12, Elliott Mitchell wrote: > > --- a/xen/arch/arm/acpi/domain_build.c > > +++ b/xen/arch/arm/acpi/domain_build.c > > @@ -42,17 +42,18 @@ static int __init acpi_iomem_deny_access(struct domain *d) > > status = acpi_get_table(ACPI_SIG_SPCR, 0, > > (struct acpi_table_header **)&spcr); > > > > - if ( ACPI_FAILURE(status) ) > > + if ( ACPI_SUCCESS(status) ) > > { > > - printk("Failed to get SPCR table\n"); > > - return -EINVAL; > > + mfn = spcr->serial_port.address >> PAGE_SHIFT; > > + /* Deny MMIO access for UART */ > > + rc = iomem_deny_access(d, mfn, mfn + 1); > > + if ( rc ) > > + return rc; > > + } > > + else > > + { > > + printk("Failed to get SPCR table, Xen console may be unavailable\n"); > > } > > Nit: While I see you've got Stefano's R-b already, I Xen we typically > omit the braces here. Personally, I prefer that myself, but was unsure of the preference here. I've seen multiple projects which *really* dislike using having brackets for some clauses, but not others (ie they want either all clauses with or all clauses without; instead of only if required). I sent what I thought was the more often used format. (I also like tabs, and dislike having so many spaces; alas my preferences are apparently uncommon)
Hi Elliott, On 22/10/2020 18:13, Elliott Mitchell wrote: > On Thu, Oct 22, 2020 at 09:42:17AM +0200, Jan Beulich wrote: >> On 22.10.2020 00:12, Elliott Mitchell wrote: >>> --- a/xen/arch/arm/acpi/domain_build.c >>> +++ b/xen/arch/arm/acpi/domain_build.c >>> @@ -42,17 +42,18 @@ static int __init acpi_iomem_deny_access(struct domain *d) >>> status = acpi_get_table(ACPI_SIG_SPCR, 0, >>> (struct acpi_table_header **)&spcr); >>> >>> - if ( ACPI_FAILURE(status) ) >>> + if ( ACPI_SUCCESS(status) ) >>> { >>> - printk("Failed to get SPCR table\n"); >>> - return -EINVAL; >>> + mfn = spcr->serial_port.address >> PAGE_SHIFT; >>> + /* Deny MMIO access for UART */ >>> + rc = iomem_deny_access(d, mfn, mfn + 1); >>> + if ( rc ) >>> + return rc; >>> + } >>> + else >>> + { >>> + printk("Failed to get SPCR table, Xen console may be unavailable\n"); >>> } >> >> Nit: While I see you've got Stefano's R-b already, I Xen we typically >> omit the braces here. > > Personally, I prefer that myself, but was unsure of the preference here. I don't think we are very consistent here... I would not add them myself, but I don't particularly mind them (I know some editors will add them automatically). I will keep them while committing. For the patch: Acked-by: Julien Grall <jgrall@amazon.com> > I've seen multiple projects which *really* dislike using having brackets > for some clauses, but not others (ie they want either all clauses with or > all clauses without; instead of only if required). > > I sent what I thought was the more often used format. (I also like tabs, > and dislike having so many spaces; alas my preferences are apparently > uncommon) We have a few files in Xen using tabs (yes, we like mixing coding style!). Cheers,
Hi, Thank you for the patch. FIY I tweak a bit the commit title before committing. The title is now: "xen/arm: acpi: Don't fail it SPCR table is absent". Cheers, On 21/10/2020 23:12, Elliott Mitchell wrote: > Absence of a SPCR table likely means the console is a framebuffer. In > such case acpi_iomem_deny_access() should NOT fail. > > Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com> > --- > xen/arch/arm/acpi/domain_build.c | 19 ++++++++++--------- > 1 file changed, 10 insertions(+), 9 deletions(-) > > diff --git a/xen/arch/arm/acpi/domain_build.c b/xen/arch/arm/acpi/domain_build.c > index 1b1cfabb00..bbdc90f92c 100644 > --- a/xen/arch/arm/acpi/domain_build.c > +++ b/xen/arch/arm/acpi/domain_build.c > @@ -42,17 +42,18 @@ static int __init acpi_iomem_deny_access(struct domain *d) > status = acpi_get_table(ACPI_SIG_SPCR, 0, > (struct acpi_table_header **)&spcr); > > - if ( ACPI_FAILURE(status) ) > + if ( ACPI_SUCCESS(status) ) > { > - printk("Failed to get SPCR table\n"); > - return -EINVAL; > + mfn = spcr->serial_port.address >> PAGE_SHIFT; > + /* Deny MMIO access for UART */ > + rc = iomem_deny_access(d, mfn, mfn + 1); > + if ( rc ) > + return rc; > + } > + else > + { > + printk("Failed to get SPCR table, Xen console may be unavailable\n"); > } > - > - mfn = spcr->serial_port.address >> PAGE_SHIFT; > - /* Deny MMIO access for UART */ > - rc = iomem_deny_access(d, mfn, mfn + 1); > - if ( rc ) > - return rc; > > /* Deny MMIO access for GIC regions */ > return gic_iomem_deny_access(d); >
On Thu, Oct 22, 2020 at 07:38:26PM +0100, Julien Grall wrote: > I don't think we are very consistent here... I would not add them > myself, but I don't particularly mind them (I know some editors will add > them automatically). > > I will keep them while committing. For the patch: I would tend to remove them on commit since I dislike them. Just as stated, I was unsure. On default settings, clang-format will object to: if(thing) { foo } else bar Or if(thing) foo else { bar } I *like* those formats, but was under the impression most people did not. The indentation is the more visually obvious indicator, just the compiler actually uses the brackets. As such I *like* the misleading indentation warnings as those seemed to have a fairly high true-positive rate. On Thu, Oct 22, 2020 at 07:44:26PM +0100, Julien Grall wrote: > Thank you for the patch. FIY I tweak a bit the commit title before > committing. > > The title is now: "xen/arm: acpi: Don't fail it SPCR table is absent". Perhaps "xen/arm: acpi: Don't fail on absent SPCR table"? What you're suggesting doesn't read well to me.
On 22.10.2020 19:13, Elliott Mitchell wrote: > On Thu, Oct 22, 2020 at 09:42:17AM +0200, Jan Beulich wrote: >> On 22.10.2020 00:12, Elliott Mitchell wrote: >>> --- a/xen/arch/arm/acpi/domain_build.c >>> +++ b/xen/arch/arm/acpi/domain_build.c >>> @@ -42,17 +42,18 @@ static int __init acpi_iomem_deny_access(struct domain *d) >>> status = acpi_get_table(ACPI_SIG_SPCR, 0, >>> (struct acpi_table_header **)&spcr); >>> >>> - if ( ACPI_FAILURE(status) ) >>> + if ( ACPI_SUCCESS(status) ) >>> { >>> - printk("Failed to get SPCR table\n"); >>> - return -EINVAL; >>> + mfn = spcr->serial_port.address >> PAGE_SHIFT; >>> + /* Deny MMIO access for UART */ >>> + rc = iomem_deny_access(d, mfn, mfn + 1); >>> + if ( rc ) >>> + return rc; >>> + } >>> + else >>> + { >>> + printk("Failed to get SPCR table, Xen console may be unavailable\n"); >>> } >> >> Nit: While I see you've got Stefano's R-b already, I Xen we typically >> omit the braces here. > > Personally, I prefer that myself, but was unsure of the preference here. > I've seen multiple projects which *really* dislike using having brackets > for some clauses, but not others (ie they want either all clauses with or > all clauses without; instead of only if required). > > I sent what I thought was the more often used format. (I also like tabs, > and dislike having so many spaces; alas my preferences are apparently > uncommon) "More often used" doesn't matter when there's an explicit statement about this in ./CODING_STYLE: "Braces should be omitted for blocks with a single statement." Yes, there are projects requiring all if/else-if belonging together to consistently use or not use braces. But there's explicitly no such statement in our doc. (Along these lines, dislike of spaces [and favoring tabs] also doesn't matter, as again the doc is explicit about it.) Jan
On 22.10.2020 21:18, Elliott Mitchell wrote: > On Thu, Oct 22, 2020 at 07:44:26PM +0100, Julien Grall wrote: >> Thank you for the patch. FIY I tweak a bit the commit title before >> committing. >> >> The title is now: "xen/arm: acpi: Don't fail it SPCR table is absent". > > Perhaps "xen/arm: acpi: Don't fail on absent SPCR table"? > > What you're suggesting doesn't read well to me. Perhaps Julien meant "if" instead of "it". i.e. a simple typo? Jan
Hi Elliott, On 22/10/2020 20:18, Elliott Mitchell wrote: > On Thu, Oct 22, 2020 at 07:38:26PM +0100, Julien Grall wrote: > On Thu, Oct 22, 2020 at 07:44:26PM +0100, Julien Grall wrote: >> Thank you for the patch. FIY I tweak a bit the commit title before >> committing. >> >> The title is now: "xen/arm: acpi: Don't fail it SPCR table is absent". > > Perhaps "xen/arm: acpi: Don't fail on absent SPCR table"? > > What you're suggesting doesn't read well to me. Sorry, I made a typo when writing the title in the e-mail. Here a direct copy from the commit: "xen/arm: acpi: Don't fail if SPCR table is absent" This is pretty much your original title with "arm: " added to clarify the subsystem modified. Cheers,
diff --git a/xen/arch/arm/acpi/domain_build.c b/xen/arch/arm/acpi/domain_build.c index 1b1cfabb00..bbdc90f92c 100644 --- a/xen/arch/arm/acpi/domain_build.c +++ b/xen/arch/arm/acpi/domain_build.c @@ -42,17 +42,18 @@ static int __init acpi_iomem_deny_access(struct domain *d) status = acpi_get_table(ACPI_SIG_SPCR, 0, (struct acpi_table_header **)&spcr); - if ( ACPI_FAILURE(status) ) + if ( ACPI_SUCCESS(status) ) { - printk("Failed to get SPCR table\n"); - return -EINVAL; + mfn = spcr->serial_port.address >> PAGE_SHIFT; + /* Deny MMIO access for UART */ + rc = iomem_deny_access(d, mfn, mfn + 1); + if ( rc ) + return rc; + } + else + { + printk("Failed to get SPCR table, Xen console may be unavailable\n"); } - - mfn = spcr->serial_port.address >> PAGE_SHIFT; - /* Deny MMIO access for UART */ - rc = iomem_deny_access(d, mfn, mfn + 1); - if ( rc ) - return rc; /* Deny MMIO access for GIC regions */ return gic_iomem_deny_access(d);
Absence of a SPCR table likely means the console is a framebuffer. In such case acpi_iomem_deny_access() should NOT fail. Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com> --- xen/arch/arm/acpi/domain_build.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)