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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

It might be that something like this is already done elsewhere in the tree,
so not really an issue imo if a 2nd instance appeared.

> It seems like the best one option is to do the following:
>    typedef const struct hw_interrupt_type hw_irq_controller; in asm/intc.h.
> I will follow it then.

Misra may dislike this.

>>> --- 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?

> otherwise a compilation error will occur (something like/"assignment discards 
> 'const' qualifier"/).
> 
> Additionally,|__ro_after_init| should be replaced with|const| for|aplic_ops| 
> in future
> patches.

Same question here then.

Jan