Re: [XEN PATCH v15 2/4] x86/irq: allow setting IRQ permissions from GSI instead of pIRQ

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.09.2024 04:38, Chen, Jiqian wrote:
> On 2024/9/12 18:51, Jan Beulich wrote:
>> On 12.09.2024 12:34, Daniel P. Smith wrote:
>>> On 9/11/24 02:58, Jiqian Chen wrote:
>>>> @@ -237,6 +238,34 @@ long arch_do_domctl(
>>>>           break;
>>>>       }
>>>>   
>>>> +    case XEN_DOMCTL_gsi_permission:
>>>> +    {
>>>> +        int irq;
>>>> +        unsigned int gsi = domctl->u.gsi_permission.gsi;
>>>> +        uint32_t flags = domctl->u.gsi_permission.flags;
>>>> +
>>>> +        /* Check only valid bits are set */
>>>> +        ret = -EINVAL;
>>>> +        if ( flags & ~XEN_DOMCTL_GSI_ACTION_MASK )
>>>> +            break;
>>>> +
>>>> +        ret = irq = gsi_2_irq(gsi);
>>>
>>> I was recently informed that a = b = c; form is not MISRA compliant. 
>>> Since you just overwrite ret after the check, why not drop the 
>>> assignment to ret and mae the next check against irq instead.
>>
>> The Misra concern is valid, yet the suggestion doesn't look to be quite
>> matching what is needed. After all if we take ...
>>
>>>> +        if ( ret <= 0 )
>>>> +            break;
>>
>> ... the "break" path, "ret" needs to be set. Plus there's the problem of
>> "ret" being zero when exiting the function indicates success, yet this
>> is an error path (requiring ret < 0). So overall perhaps
>>
>>          irq = gsi_2_irq(gsi);
>>          if ( irq <= 0 )
>>          {
>>              ret = irq ?: -EACCES;
>>              break;
>>          }
>>
>> ?
> 
> Yes, ret needs to be set. And since gsi_2_irq doesn't return 0(if irq is 0, 
> gsi_2_irq returns -EINVAL).
> Maybe below is enough?
>         irq = gsi_2_irq(gsi);
>         if ( irq < 0 )
>         {
>             ret = irq;
>             break;
>         }

My proposal was to cover that elsewhere we exclude IRQ0, and hence
it would be good to be consistent here.

Jan