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

["Hao, Xudong" <xudong.hao@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Friday, May 04, 2012 4:07 PM
> To: Hao, Xudong
> Cc: xen-devel; konrad.wilk@xxxxxxxxxx
> Subject: Re: [Xen-devel] [PATCH v2] Re-enable LTR/OBFF when device is owned
> by pciback
> 
> >>> 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.
> 
I agree the driver control it, but it's difficult to let guest driver do it 
till now(because of PCIe emulation by qemu issue).

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

dev_alert should be ok. Thanks for the careful review of typo "enable", I'll 
modify it in next version.

> > +
> > +   err = pci_set_ltr(dev, snoop_lat_ns, nosnoop_lat_ns);
> 
> What's the point of calling this function when pci_enable_ltr() failed?
> 

Oh, thanks the point.
It's unnecessary to call this function when pci_enable_ltr() failed indeed, 
I'll insert this calling in pci_enable_ltr() passing logical in next version.

> 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