[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.