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 <jbeul...@suse.com>
> ---
> 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.
