[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 June 15, 2015 5:45:09 AM EDT, Jan Beulich <JBeulich@xxxxxxxx> wrote: >>>> 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. Yes, thank you. > >> - 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. Ah, hadn't realized we have made an public -XEN_Exx defines, will use that. > >> --- 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. OK, will be more explicit. > >Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |