[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2] xen/pciif: Clarify what values go in op->err and op->result.
>>> On 12.06.15 at 22:57, <konrad.wilk@xxxxxxxxxx> wrote: > The earlier comment says that errno values go in op->err. > However all implementations (NetBSD, Linux) of the most > common operations use XEN_PCI_ERR_* instead of -EXX values. > > The exception is the xen-pciback in Linux (upstream & XenClassic) > code when doing XEN_PCI_OP_enable_msix can stash the -EXX in op->result > and in op->err, but they are also the only ones implementing this > operation. > > Here is how it works right now with the XEN_PCI_OP: From here on, other than said above, you appear to talk about frontend behavior. This should be made explicit. > - XEN_PCI_OP_conf_read and XEN_PCI_OP_conf_write > it expects 'err' to contain XEN_PCI_ERR* values. And it converts them > as it sees fit to -Exx. > Note that NetBSD only implements XEN_PCI_OP_conf_write and > XEN_PCI_OP_conf_read. > > - For XEN_PCI_OP_enable_msi if 'err' has any value it will convert > all of them to -EINVAL (Linux). > > - For XEN_PCI_OP_disable_msix and XEN_PCI_OP_disable_msi it just > reports the value (printk) and discards the 'err'. > > - The XEN_PCI_OP_enable_msix differs on the frontend (classic Linux > vs upstream). > In Linux classic, if 'err' has any value it will convert all of them > to '-EINVAL'. > In Linux upstream it will convert the 'err' to uint32_t and pass it > back up (to 'pci_enable_msi_range'). However due to the casting > errors it ends up being 0xffffffffa (or such) and is useless. > > Which means that it really does not matter what (-EXX or XEN_PCI_ERR_*) > or where (op->err or op->result) the backend stashes it as the frontend > screws it up or ignores it. > > Which means this patch will not break existing implementations and mandating > op->err to use XEN_PCI_ERR_* and stick in op->result -EXX if the > opcode wants it is the step in the right direction. Albeit you realize that passing -E... values here is bogus anyway. If anything, this should be -XEN_E..., so that their values don't vary by OS. > --- a/xen/include/public/io/pciif.h > +++ b/xen/include/public/io/pciif.h > @@ -71,7 +71,7 @@ struct xen_pci_op { > /* IN: what action to perform: XEN_PCI_OP_* */ > uint32_t cmd; > > - /* OUT: will contain an error number (if any) from errno.h */ > + /* OUT: will contain an XEN_PCI_ERR_* value. */ > int32_t err; > > /* IN: which device to touch */ > @@ -83,7 +83,9 @@ struct xen_pci_op { > int32_t offset; > int32_t size; > > - /* IN/OUT: Contains the result after a READ or the value to WRITE */ > + /* IN/OUT: Contains the result after a READ or the value to WRITE. > + * If the err does not have XEN_PCI_ERR_success, depending on > + * XEN_PCI_OP_* might have the errno value. */ > uint32_t value; The comment (apart from being badly formatted) is still too vague to be of any use to the reader. Plus I think references to other fields in the structure should either quote the field name or add "field" after the name. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |