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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

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@oracle.com> 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@novell.com>
> >> 
> >> ---
> >>  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.