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
|
Home