Re: [PATCH 3/5] vPCI/MSI-X: fold clearing of entry->updated

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Dec 07, 2020 at 11:37:51AM +0100, Jan Beulich wrote:
> Both call sites clear the flag after a successfull call to
> update_entry(). This can be simplified by moving the clearing into the
> function, onto its success path.

The point of returning a value was to set the updated field, as there
was no failure log message printed by the callers.

> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> As a result it becomes clear that the return value of the function is of
> no interest to either of the callers. I'm not sure whether ditching it
> is the right thing to do, or whether this rather hints at some problem.

I think you should make the function void as part of this change,
there's a log message printed by update_entry in the failure case
which IMO should be enough.

There's not much else callers can do AFAICT.

> --- a/xen/drivers/vpci/msix.c
> +++ b/xen/drivers/vpci/msix.c
> @@ -64,6 +64,8 @@ static int update_entry(struct vpci_msix
>          return rc;
>      }
>  
> +    entry->updated = false;
> +
>      return 0;
>  }
>  
> @@ -92,13 +94,8 @@ static void control_write(const struct p
>      if ( new_enabled && !new_masked && (!msix->enabled || msix->masked) )
>      {
>          for ( i = 0; i < msix->max_entries; i++ )
> -        {
> -            if ( msix->entries[i].masked || !msix->entries[i].updated ||
> -                 update_entry(&msix->entries[i], pdev, i) )
> -                continue;
> -
> -            msix->entries[i].updated = false;
> -        }
> +            if ( !msix->entries[i].masked && msix->entries[i].updated )
> +                update_entry(&msix->entries[i], pdev, i);
>      }
>      else if ( !new_enabled && msix->enabled )
>      {
> @@ -365,10 +362,7 @@ static int msix_write(struct vcpu *v, un
>               * data fields Xen needs to disable and enable the entry in order
>               * to pick up the changes.
>               */
> -            if ( update_entry(entry, pdev, vmsix_entry_nr(msix, entry)) )
> -                break;
> -
> -            entry->updated = false;
> +            update_entry(entry, pdev, vmsix_entry_nr(msix, entry));
>          }

You can also drop this braces now if you feel like.

Thanks, Roger.