On 26/03/2019 13.41, Sakari Ailus wrote:
> Add support for %pfw conversion specifier (with "f" and "P" modifiers) to
> support printing full path of the node, including its name ("f") and only
> the node's name ("P") in the printk family of functions. The two flags
> have equivalent functionality to existing %pOF with the same two modifiers
> ("f" and "P") on OF based systems. The ability to do the same on ACPI
> based systems is added by this patch.
>
>
> +static noinline_for_stack
> +char *fwnode_string(char *buf, char *end, struct fwnode_handle *fwnode,
> + struct printf_spec spec, const char *fmt)
> +{
> + const char * const modifiers = "fP";
> + struct printf_spec str_spec = spec;
> + char *buf_start = buf;
> + bool pass;
> +
> + str_spec.field_width = -1;
> +
> + if ((unsigned long)fwnode < PAGE_SIZE)
> + return string(buf, end, "(null)", spec);
> +
> + /* simple case without anything any more format specifiers */
> + fmt++;
> + if (fmt[0] == '\0' || strcspn(fmt, modifiers) > 0)
> + fmt = "f";
> +
> + for (pass = false; strspn(fmt, modifiers); fmt++, pass = true) {
> + if (pass) {
> + if (buf < end)
> + *buf = ':';
> + buf++;
> + }
> +
> + switch (*fmt) {
> + case 'f': /* full_name */
> + buf = fwnode_gen_full_name(fwnode, buf, end);
> + break;
> + case 'P': /* name */
> + buf = string(buf, end, fwnode_get_name(fwnode),
> + str_spec);
> + break;
> + default:
> + break;
> + }
> + }
This seems awfully complicated. Why would anyone ever pass more than one
of 'f' and 'P'? Why not just
switch(*fmt) {
case 'P':
...
case 'f':
default:
...
}
which avoids the loop and the strcspn. Or, drop the default: case and
don't have logic at all for falling back to 'f' if neither is present.
> + return widen_string(buf, buf - buf_start, end, spec);
> +}
> +
> /*
> * Show a '%p' thing. A kernel extension is that the '%p' is followed
> * by an extra set of alphanumeric characters that are extended format
> @@ -1936,6 +1978,10 @@ char *device_node_string(char *buf, char *end, struct
> device_node *dn,
> * F device node flags
> * c major compatible string
> * C full compatible string
> + * - 'fw[fP]' For a firmware node (struct fwnode_handle) pointer
> + * Without an option prints the full name of the node
> + * f full name
> + * P node name, including a possible unit address
> * - 'x' For printing the address. Equivalent to "%lx".
> *
> * ** When making changes please also update:
> @@ -2060,6 +2106,16 @@ char *pointer(const char *fmt, char *buf, char *end,
> void *ptr,
> return device_node_string(buf, end, ptr, spec, fmt + 1);
> }
> break;
> + case 'f':
> + switch (fmt[1]) {
> + case 'w':
> + return fwnode_string(buf, end, ptr, spec, fmt + 1);
Why not pass fmt+2; we know that fmt+1 points at a 'w'. Just to avoid
doing the fmt++ inside fwnode_string().
Rasmus
