| Message ID | 20101007095751.GA7098@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, 2010-10-07 at 11:57 +0200, Michael S. Tsirkin wrote: > On Wed, Oct 06, 2010 at 04:05:38PM -0600, Alex Williamson wrote: > > On Wed, 2010-10-06 at 23:44 +0200, Michael S. Tsirkin wrote: > > > On Wed, Oct 06, 2010 at 11:24:24AM -0600, Alex Williamson wrote: > > > > You could always keep the functions as separate wrapper callers of the > > > > common function so you only need to keep true = unset, false = set > > > > straight in one place. Thanks, > > > > > > > > > Just to show why it does not work, I did exactly this: as you see the > > > code is shorter but the true/false magic gets spread: it was in 2 > > > places, (set/unset) now it is in 4 places and it is within the loop, in > > > code that is more complex. > > > > You seem to have missed the wrapper function. I'm simply suggesting > > something like this: > > Good idea. I tweaked the code a bit more and came up with this: > I think I will keep this as an incremental patch just to > keep diffs small. > > msix: factor out mask notifier code > > Move some common code handling msix mask notifiers to a function. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > diff --git a/hw/msix.c b/hw/msix.c > index 3d4dd61..5f66d20 100644 > --- a/hw/msix.c > +++ b/hw/msix.c > @@ -583,40 +583,26 @@ void msix_unuse_all_vectors(PCIDevice *dev) > msix_free_irq_entries(dev); > } > > -static int msix_set_mask_notifier_for_vector(PCIDevice *dev, unsigned vector) > +/* Invoke the notifier if vector entry is used and unmasked. */ > +static int msix_notify_if_unmasked(PCIDevice *dev, unsigned vector, int masked) > { > - int r = 0; > - if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[vector]) > - return 0; > - > assert(dev->msix_mask_notifier); > - > - /* Unmask the new notifier unless vector is masked. */ > - if (!msix_is_masked(dev, vector)) { > - r = dev->msix_mask_notifier(dev, vector, false); > - if (r < 0) { > - return r; > - } > + if (!dev->msix_entry_used[vector] || msix_is_masked(dev, vector)) { > + return 0; > } > - return r; > + return dev->msix_mask_notifier(dev, vector, masked); > } > > -static int msix_unset_mask_notifier_for_vector(PCIDevice *dev, unsigned vector) > +static int msix_set_mask_notifier_for_vector(PCIDevice *dev, unsigned vector) > { > - int r = 0; > - if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[vector]) > - return 0; > - > - assert(dev->msix_mask_notifier); > + /* Notifier has been set. Invoke it on unmasked vectors. */ > + return msix_notify_if_unmasked(dev, vector, 0); > +} Looks fine except for the extra white space here and copied below. Thanks, Alex > - /* Mask the old notifier unless it is already masked. */ > - if (!msix_is_masked(dev, vector)) { > - r = dev->msix_mask_notifier(dev, vector, true); > - if (r < 0) { > - return r; > - } > - } > - return r; > +static int msix_unset_mask_notifier_for_vector(PCIDevice *dev, unsigned vector) > +{ > + /* Notifier will be unset. Invoke it to mask unmasked entries. */ > + return msix_notify_if_unmasked(dev, vector, 1); > } > > int msix_set_mask_notifier(PCIDevice *dev, msix_mask_notifier_func f)
> Looks fine except for the extra white space here and copied below. > Thanks, > > Alex OK, I fixed this up and queued the patch with your ack. Thanks for the review!
diff --git a/hw/msix.c b/hw/msix.c index 3d4dd61..5f66d20 100644 --- a/hw/msix.c +++ b/hw/msix.c @@ -583,40 +583,26 @@ void msix_unuse_all_vectors(PCIDevice *dev) msix_free_irq_entries(dev); } -static int msix_set_mask_notifier_for_vector(PCIDevice *dev, unsigned vector) +/* Invoke the notifier if vector entry is used and unmasked. */ +static int msix_notify_if_unmasked(PCIDevice *dev, unsigned vector, int masked) { - int r = 0; - if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[vector]) - return 0; - assert(dev->msix_mask_notifier); - - /* Unmask the new notifier unless vector is masked. */ - if (!msix_is_masked(dev, vector)) { - r = dev->msix_mask_notifier(dev, vector, false); - if (r < 0) { - return r; - } + if (!dev->msix_entry_used[vector] || msix_is_masked(dev, vector)) { + return 0; } - return r; + return dev->msix_mask_notifier(dev, vector, masked); } -static int msix_unset_mask_notifier_for_vector(PCIDevice *dev, unsigned vector) +static int msix_set_mask_notifier_for_vector(PCIDevice *dev, unsigned vector) { - int r = 0; - if (vector >= dev->msix_entries_nr || !dev->msix_entry_used[vector]) - return 0; - - assert(dev->msix_mask_notifier); + /* Notifier has been set. Invoke it on unmasked vectors. */ + return msix_notify_if_unmasked(dev, vector, 0); +} - /* Mask the old notifier unless it is already masked. */ - if (!msix_is_masked(dev, vector)) { - r = dev->msix_mask_notifier(dev, vector, true); - if (r < 0) { - return r; - } - } - return r; +static int msix_unset_mask_notifier_for_vector(PCIDevice *dev, unsigned vector) +{ + /* Notifier will be unset. Invoke it to mask unmasked entries. */ + return msix_notify_if_unmasked(dev, vector, 1); } int msix_set_mask_notifier(PCIDevice *dev, msix_mask_notifier_func f)