Re: [Xen-devel] [PATCH] xen/pci: make bus notifier handler return sane values

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Tue, Aug 23, 2011 at 09:19:04AM +0100, Jan Beulich wrote:
> >>> On 22.08.11 at 18:20, Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> 
> >>> wrote:
> > On Wed, Aug 17, 2011 at 09:32:32AM +0100, Jan Beulich wrote:
> >> Notifier functions are expected to return NOTIFY_* codes, not -E...
> >> ones. In particular, since the respective hypercalls failing is not
> >> fatal to the operation of the Dom0 kernel, it must be avoided to
> >> return negative values here as those would make it appear as if
> >> NOTIFY_STOP_MASK wa set, suppressing further notification calls to
> >> other interested parties (which is also why we don't want to use
> >> notifier_from_errno() here).
> >> 
> >> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxxxx>
> >> 
> >> ---
> >>  drivers/xen/pci.c |   11 +++++------
> >>  1 file changed, 5 insertions(+), 6 deletions(-)
> >> 
> >> --- 3.1-rc2/drivers/xen/pci.c
> >> +++ 3.1-rc2-xen-pci-bus-notifier/drivers/xen/pci.c
> >> @@ -86,23 +86,22 @@ static int xen_pci_notifier(struct notif
> >>                        unsigned long action, void *data)
> >>  {
> >>    struct device *dev = data;
> >> -  int r = 0;
> >>  
> >>    switch (action) {
> >>    case BUS_NOTIFY_ADD_DEVICE:
> >> -          r = xen_add_device(dev);
> >> +          xen_add_device(dev);
> >>            break;
> >>    case BUS_NOTIFY_DEL_DEVICE:
> >> -          r = xen_remove_device(dev);
> >> +          xen_remove_device(dev);
> >>            break;
> >>    default:
> >> -          break;
> >> +          return NOTIFY_DONE;
> >>    }
> >>  
> >> -  return r;
> >> +  return NOTIFY_OK;
> >>  }
> >>  
> >> -struct notifier_block device_nb = {
> >> +static struct notifier_block device_nb = {
> >>    .notifier_call = xen_pci_notifier,
> >>  };
> >>  
> >> 
> > 
> > Somehow I lost the email thread.. but what if we did this:
> > 
> > diff --git a/drivers/xen/pci.c b/drivers/xen/pci.c
> > index cef4baf..11e231c 100644
> > --- a/drivers/xen/pci.c
> > +++ b/drivers/xen/pci.c
> > @@ -96,13 +96,19 @@ static int xen_pci_notifier(struct notifier_block *nb,
> >             r = xen_remove_device(dev);
> >             break;
> >     default:
> > -           break;
> > +           return NOTIFY_DONE;
> >     }
> > -
> > -   return r;
> > +   if (r) {
> > +           dev_err(dev, "Failed to %s - passthrough or MSI/MSI-X might 
> > fail!\n",
> > +                   action == BUS_NOTIFY_ADD_DEVICE ? "add" :
> > +                   (action == BUS_NOTIFY_DEL_DEVICE ? "delete" : 
> > "unknown"));
> 
> This part is fine.
> 
> > +           if (action == BUS_NOTIFY_ADD_DEVICE)
> > +                   pci_no_msi();
> 
> But this I'm not sure about: First of all, you're suggesting to disable MSI
> altogether. That's very unlikely to be necessary.
> 
> Requiring the PCI device to be known to Xen to be able to use MSI is
> an implementation detail of the hypervisor. And in fact I had a patch
> to remove the call to pci_get_pdev() from map_domain_pirq() (since
> setup_msi_irq() doesn't really consume this parameter). This got made
> impossible to do (at least for the moment) with George's
> 23753:2e0cf9428554 (and later on the device is getting looked up by
> __pci_enable_msi{,x}() anyway, but even that is an implementation
> choice, not a hard requirement afaict).
> 
> I would view it as quite viable an option to have PHYSDEVOP_map_pirq
> implicitly create the device in the MAP_PIRQ_TYPE_MSI case. And hence
> I wouldn't want to disable MSI for the device here.

Ok. let me drop that section and just go with the warning.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel