Re: [Xen-devel] [PATCH v2] Re-enable LTR/OBFF when device is owned by pciback

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.05.12 at 09:49, "Hao, Xudong" <xudong.hao@xxxxxxxxx> wrote:
> When PCIE device which has LTR/OBFF capabilities is owned by pciback, 
> LTR/OBFF feature may be disabled. This patch re-enable LTR and OBFF, so that 
> guest with device assigned can be benefitted.
> 
> Changes from v1:
> - put the variable definition at the start of function
> - add error log report
> 
> Signed-off-by: Xudong Hao <xudong.hao@xxxxxxxxx>

I continue to disagree to doing this always, unconditionally, with
hard-coded settings. If these features require explicit enabling,
there likely is something that drivers unaware of them could have
problems with - otherwise I would think these features would be
always enabled when a device supports them, and no options
would exist to control certain parameters.

Plus, if the patch is nevertheless going to be acceptable, ...

> --- a/drivers/xen/xen-pciback/pci_stub.c
> +++ b/drivers/xen/xen-pciback/pci_stub.c
> @@ -313,6 +313,10 @@ static int __devinit pcistub_init_device(struct pci_dev 
> *dev)
>       struct xen_pcibk_dev_data *dev_data;
>       int err = 0;
>  
> +     /* set default value */
> +     unsigned long type = PCI_EXP_OBFF_SIGNAL_ALWAYS;
> +     int snoop_lat_ns = 1024, nosnoop_lat_ns = 1024;
> +
>       dev_dbg(&dev->dev, "initializing...\n");
>  
>       /* The PCI backend is not intended to be a module (or to work with
> @@ -369,6 +373,28 @@ static int __devinit pcistub_init_device(struct pci_dev 
> *dev)
>       dev_dbg(&dev->dev, "reset device\n");
>       xen_pcibk_reset_device(dev);
>  
> +     /* Enable LTR and OBFF before do device assignment */
> +     /* LTR(Latency tolerance reporting) allows devices to send
> +      * messages to the root complex indicating their latency tolerance
> +      * for snooped & unsnooped memory transactions.
> +      */
> +     err = pci_enable_ltr(dev);
> +     if (err)
> +             dev_err(&dev->dev, "Counld not enalbe LTR for device!\n");

... dev_err() here and below is certainly too severe, particularly for
the -ENOTSUPP cases. And the spelling would need fixing, especially
with it being broken exactly the same way a second time below.

> +
> +     err = pci_set_ltr(dev, snoop_lat_ns, nosnoop_lat_ns);

What's the point of calling this function when pci_enable_ltr() failed?

Jan

> +     if (err)
> +             dev_err(&dev->dev, "Set LTR latency values failed.\n");
> +
> +     /* OBFF (optimized buffer flush/fill), where supported, can help
> +      * improve energy efficiency by giving devices information about
> +      * when interrupts and other activity will have a reduced power
> +      * impact.
> +      */
> +     err = pci_enable_obff(dev, type);
> +     if (err)
> +             dev_err(&dev->dev, "Counld not enalbe OBFF for device!\n");
> +
>       dev->dev_flags |= PCI_DEV_FLAGS_ASSIGNED;
>       return 0;
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx 
> http://lists.xen.org/xen-devel 




_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel