Re: [PATCH 2/3] x86/msi: remove return value from msi_set_mask_bit()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.11.2022 17:59, David Vrabel wrote:
> The return value was only used for WARN()s or BUG()s so it has no
> functional purpose. Simplify the code by removing it.
> 
> The meaning of the return value and the purpose of the various WARNs()
> and BUGs() is rather unclear. The only failure path (where an MSI-X
> vector needs to be masked but the MSI-X table is not accessible) has a
> useful warning message.

No, you're removing an important 2nd such path - the default case in the
switch() statement. Getting there would mean another bug elsewhere, which
we don't want to leave undetected for yet longer (and hence yet harder to
debug once finally some misbehavior surfaces). That default case may
warrant the addition of ASSERT_UNREACHABLE() according to the respective
description in ./CODING_STYLE, but I don't see the removal of the
"return" as acceptable (also for another reason as explained below).

The idea of the WARN() / BUG_ON() is to
- not leave failed unmasking unrecorded,
- not continue after failure to mask an entry.
This combines with the functions where the constructs are not having
ways to properly propagate the errors to their callers.

> @@ -361,11 +361,6 @@ static bool msi_set_mask_bit(struct irq_desc *desc, bool 
> host, bool guest)
>  
>              if ( likely(control & PCI_MSIX_FLAGS_ENABLE) )
>                  break;
> -
> -            entry->msi_attrib.host_masked = host;
> -            entry->msi_attrib.guest_masked = guest;
> -
> -            flag = true;
>          }
>          else if ( flag && !(control & PCI_MSIX_FLAGS_MASKALL) )
>          {
> @@ -385,14 +380,10 @@ static bool msi_set_mask_bit(struct irq_desc *desc, 
> bool host, bool guest)
>              control |= PCI_MSIX_FLAGS_MASKALL;
>          pci_conf_write16(pdev->sbdf,
>                           msix_control_reg(entry->msi_attrib.pos), control);
> -        return flag;

Both this and ...

> -    default:
> -        return 0;

... this have previously prevented to make it ...

> +        break;
>      }
>      entry->msi_attrib.host_masked = host;
>      entry->msi_attrib.guest_masked = guest;

... here. This is a change in behavior which is neither obviously benign
nor properly justified in the description, yet clearly going beyond what
the title says the patch is about.

Jan