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

["Jan Beulich" <JBeulich@xxxxxxxxxx>]

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

Jan

> +     }
> +     return NOTIFY_OK;
>  }
>  
> -struct notifier_block device_nb = {
> +static struct notifier_block device_nb = {
>       .notifier_call = xen_pci_notifier,
>  };
>  




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