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

["Chen, Jiqian" <Jiqian.Chen@xxxxxxx>]

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;
        }

> 
> Jan

-- 
Best regards,
Jiqian Chen.