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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.06.19 at 11:22, <roger.pau@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel