Re: [PATCH v2 09/16] xen/riscv: introduce register_intc_ops() and intc_hw_ops.

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.05.2025 16:04, Oleksii Kurochko wrote:
> On 5/19/25 3:16 PM, Jan Beulich wrote:
>> On 19.05.2025 11:16, Oleksii Kurochko wrote:
>>> On 5/15/25 10:06 AM, Jan Beulich wrote:
>>>> On 06.05.2025 18:51, Oleksii Kurochko wrote:
>>>>> --- a/xen/arch/riscv/include/asm/intc.h
>>>>> +++ b/xen/arch/riscv/include/asm/intc.h
>>>>> @@ -8,6 +8,8 @@
>>>>>    #ifndef ASM__RISCV__INTERRUPT_CONTOLLER_H
>>>>>    #define ASM__RISCV__INTERRUPT_CONTOLLER_H
>>>>>    
>>>>> +#include <xen/irq.h>
>>>> If you need this include anyway, why ...
>>>>
>>>>> @@ -17,6 +19,26 @@ struct intc_info {
>>>>>        const struct dt_device_node *node;
>>>>>    };
>>>>>    
>>>>> +struct irq_desc;
>>>> ... this "forward" decl for something that's then already fully defined?
>>>> I can't, however, spot why xen/irq.h would be needed for anything ...
>>> forward decl for irq_desc could be really dropped.
>>>
>>> Inclusion of xen/irq.h was added because of hw_irq_controller which is 
>>> defined as:
>>>     typedef const struct hw_interrupt_type hw_irq_controller;
>>>
>>> And I'm not sure how to do forward declaration properly in this case. Just 
>>> use
>>> an explicit definition of hw_irq_controller for host_irq_type member of 
>>> struct
>>> intc_hw_operations seems as not the best one option:
>>>     struct hw_interrupt_type;
>> This isn't needed for the use below.
> 
> Shouldn't I tell the compiler that definition of hw_interrupt_type struct 
> exist
> somewhere else?

The above decl merely introduces the type into (global) scope. The same is
achieved by ...

>>>     struct intc_hw_operations {
>>>         ...
>>>         // const hw_irq_controller *host_irq_type;
>>>         const struct hw_interrupt_type *host_irq_type;

... this. The case where the earlier decl matters is when a type is used
as a function parameter in a prototype. There, if not previously introduced
into global scope, the scope would be limited to that of the function decl
(and hence a type conflict would result when later the same type is
introduced into global scope).

>>>>> --- a/xen/arch/riscv/intc.c
>>>>> +++ b/xen/arch/riscv/intc.c
>>>>> @@ -5,6 +5,15 @@
>>>>>    #include <xen/init.h>
>>>>>    #include <xen/lib.h>
>>>>>    
>>>>> +#include <asm/intc.h>
>>>>> +
>>>>> +static struct __ro_after_init intc_hw_operations *intc_hw_ops;
>>>> Nit: Attributes between type and identifier please. Also shouldn't both
>>>> this and ...
>>>>
>>>>> +void __init register_intc_ops(struct intc_hw_operations *ops)
>>>> ... the parameter here be pointer-to-const?
>>> Then|intc_hw_ops| should also be marked as|const| (with the removal 
>>> of|__ro_after_init|),
>> Why remove the attribute?
> 
> My understanding is that if it is marked as 'const' then it automatically 
> mean that it is read-only
> always before and after init, so '__ro_after_init' is useless.

You need to separate properties of the (pointer type) variable, and what
that pointer points to. __ro_after_init applies to the variable, whereas
the const asked for is to apply to the pointed-to type. (This is more
obvious when you place the attribute as requested. Hence why we want that
particular placement.)

Jan