Re: [PATCH v3 2/3] xen/ppc: Implement early serial printk on pseries

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.07.2023 20:32, Shawn Anastasio wrote:
> On 7/17/23 11:17 AM, Jan Beulich wrote:
>> On 06.07.2023 21:04, Shawn Anastasio wrote:
>>> --- /dev/null
>>> +++ b/xen/arch/ppc/boot-of.c
>>> @@ -0,0 +1,100 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>>> +/*
>>> + * Copyright IBM Corp. 2005, 2006, 2007
>>
>> Judging from the years the file was taken from somewhere. Is the license
>> there permitting "2.0 or later"? (For files [partly] taken from somewhere,
>> a clarification as to the originals' licenses would be helpful to have in
>> the description, or maybe in the post-commit-message area.)
> 
> The original license of the file that this was derived from
> (xen/arch/powerpc/boot_of.c from Xen 3.2) is GPL v2.0 or later.
> 
> In any case where I'm deriving code from existing files, I'm always
> using the original license of the derived code. Should I still clarify
> this in the header comment?

I think it would be good to mention explicitly, as 2.0-only is the
common expectation.

>>> +/* OF entrypoint*/
>>> +static unsigned long __initdata of_vec;
>>> +
>>> +/* OF device handles*/
>>> +static int __initdata bof_chosen;
>>> +static int __initdata of_out;
>>> +
>>> +static int __init of_call(const char *service, uint32_t nargs, uint32_t 
>>> nrets,
>>> +                   int32_t rets[], ...)
>>
>> Nit: Indentation.
> 
> Will fix.
> 
>>> +{
>>> +    int rc;
>>> +    va_list args;
>>> +    int i;
>>
>> unsigned int?
> 
> I might as well change it to uint32_t to be in line with nargs.

Please don't. See ./CODING_STYLE for when it is okay to use fixed-
width types.

>>> +    va_start(args, rets);
>>> +
>>> +    for ( i = 0; i < nargs; i++ )
>>> +        s.ofs_args[i] = cpu_to_be32(va_arg(args, uint32_t));
>>> +
>>> +    va_end(args);
>>> +
>>> +    rc = enter_of(&s, of_vec);
>>> +
>>> +    /* copy all return values to the output rets array */
>>> +    for ( i = 0; i < nrets; i++ )
>>> +        rets[i] = be32_to_cpu(s.ofs_args[i + nargs]);
>>> +
>>> +    return rc;
>>> +}
>>> +
>>> +static int __init of_finddevice(const char *devspec)
>>> +{
>>> +    int rets[1] = { OF_FAILURE };
>>
>> Hmm, of_call() uses int32_t. Again below several times.
> 
> Good catch. I'll switch all of these to int32_t for consistency.

Here, for example, it is (because of being used to interface with
firmware).

>>> --- /dev/null
>>> +++ b/xen/arch/ppc/early_printk.c
>>> @@ -0,0 +1,28 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +#include <xen/init.h>
>>> +#include <asm/boot.h>
>>> +
>>> +static void (*putchar_func)(char);
>>
>> __initdata? (Connected to the question of building into .init.o.)
> 
> Since I'm going to change this to build to .init.o, would this
> automatically be put into the correct .init section? Would it still be
> preferable style-wise to mark it as __initdata?

No, it would complain that .data or .bss is non-empty. Automatic
conversion occurs only for things you can't control at the source
level, e.g. string literals.

Jan