| Message ID | c27c1c50-2d98-1796-f0e5-8fbae9f50045@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/vPCI: MSI/MSI-X related adjustments | expand |
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@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.
--- 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)); } else vpci_msix_arch_mask_entry(entry, pdev, entry->masked);
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. Signed-off-by: Jan Beulich <jbeulich@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.