Re: [Xen-devel] [PATCH v3 13/13] print: introduce a format specifier for pci_sbdf_t

From: "Jan Beulich" <JBeulich@suse.com>
To: "Roger Pau Monne" <roger.pau@citrix.com>
Cc: Kevin Tian <kevin.tian@intel.com>,
	Stefano Stabellini <sstabellini@kernel.org>, WeiLiu <wl@xen.org>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	George Dunlap <George.Dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <Ian.Jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>,
	Julien Grall <julien.grall@arm.com>,
	Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>,
	xen-devel <xen-devel@lists.xenproject.org>,
	Brian Woods <brian.woods@amd.com>
Subject: Re: [Xen-devel] [PATCH v3 13/13] print: introduce a format specifier for pci_sbdf_t
Date: Mon, 17 Jun 2019 09:10:47 -0600	[thread overview]
Message-ID: <5D07AD770200007800238D9E@prv1-mh.provo.novell.com> (raw)
In-Reply-To: <20190607092232.83179-14-roger.pau@citrix.com>

>>> On 07.06.19 at 11:22, <roger.pau@citrix.com> wrote:
> The new format specifier is '%pp', and prints a pci_sbdf_t using the
> seg:bus:dev.func format. Replace all SBDFs printed using
> '%04x:%02x:%02x.%u' to use the new format specifier.

Well, I continue to not be happy about the use of %p (rather than the
proposed %o) here. I don't suppose you've actually tried, and it didn't
work out?

> --- a/xen/common/vsprintf.c
> +++ b/xen/common/vsprintf.c
> @@ -394,6 +394,20 @@ static char *print_vcpu(char *str, const char *end, const struct vcpu *v)
>      return number(str + 1, end, v->vcpu_id, 10, -1, -1, 0);
>  }
>  
> +static char *print_pci_addr(char *str, const char *end, const pci_sbdf_t *sbdf)
> +{
> +    str = number(str, end, sbdf->seg, 16, 4, -1, ZEROPAD);
> +    if ( str < end )
> +        *str = ':';
> +    str = number(str + 1, end, sbdf->bus, 16, 2, -1, ZEROPAD);
> +    if ( str < end )
> +        *str = ':';
> +    str = number(str + 1, end, sbdf->dev, 16, 2, -1, ZEROPAD);
> +    if ( str < end )
> +        *str = '.';
> +    return number(str + 1, end, sbdf->fn, 8, -1, -1, 0);

Nit: Blank line above here please.

> +}
> +
>  static char *pointer(char *str, const char *end, const char **fmt_ptr,
>                       const void *arg, int field_width, int precision,
>                       int flags)
> @@ -476,6 +490,10 @@ static char *pointer(char *str, const char *end, const char **fmt_ptr,
>          }
>      }
>  
> +    case 'p': /* PCI SBDF. */

Another nit: The full stop would imo better be omitted here. I notice this
is inconsistent at present in this function, but generally I think it's better
to not have it when a comment is not really a sentence. The style
guidelines explicitly permit omitting it iirc.

> +        ++*fmt_ptr;
> +        return print_pci_addr(str, end, arg);
> +
>      case 's': /* Symbol name with offset and size (iff offset != 0) */
>      case 'S': /* Symbol name unconditionally with offset and size */
>      {

In principle I'd expect this to not even build on Arm, as pci_sbdf_t should
not be defined. I guess you did build test it and it worked, but I think the
additions should still go in #ifdef CONFIG_HAS_PCI.

Also I'd like to point out that again review and in particular collection of
acks might be easier if this got split up a little. But I'm not going to insist.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel