|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH V10 3/8] Introduce XenHostPCIDevice to access a pci device on the host.
> +static int xen_host_pci_sysfs_path(const XenHostPCIDevice *d,
> + const char *name, char *buf, ssize_t size)
> +{
> + int rc;
> +
> + rc = snprintf(buf, size, "/sys/bus/pci/devices/%04x:%02x:%02x.%x/%s",
The format is actually " %04x:%02x:%02x.%d"
> + d->domain, d->bus, d->dev, d->func, name);
> +
> + if (rc >= size || rc < 0) {
> + /* The ouput is truncated or an other error is encountered */
> + return -1;
-ENODEV
> + }
> + return 0;
> +}
> +
> +static int xen_host_pci_get_resource(XenHostPCIDevice *d)
> +{
> + int i, rc, fd;
> + char path[PATH_MAX];
> + char buf[512];
You should have a #define for the 512.
> + unsigned long long start, end, flags, size;
> + char *endptr, *s;
> +
> + if (xen_host_pci_sysfs_path(d, "resource", path, sizeof (path))) {
> + return -1;
-ENODEV
> + }
> + fd = open(path, O_RDONLY);
> + if (fd == -1) {
> + XEN_HOST_PCI_LOG("Error: Can't open %s: %s\n", path,
> strerror(errno));
> + return -errno;
> + }
> +
> + do {
> + rc = read(fd, &buf, sizeof (buf));
> + if (rc < 0 && errno != EINTR) {
> + rc = -errno;
So you are using the errnor types, so you should use them throughout
this function.
> + goto out;
> + }
> + } while (rc < 0);
> + buf[rc] = 0;
> + rc = 0;
> +
> + s = buf;
> + for (i = 0; i < PCI_NUM_REGIONS; i++) {
> + start = strtoll(s, &endptr, 16);
> + if (*endptr != ' ' || s == endptr) {
> + break;
> + }
> + s = endptr + 1;
> + end = strtoll(s, &endptr, 16);
> + if (*endptr != ' ' || s == endptr) {
> + break;
> + }
> + s = endptr + 1;
> + flags = strtoll(s, &endptr, 16);
> + if (*endptr != '\n' || s == endptr) {
> + break;
> + }
> + s = endptr + 1;
> +
> + if (start) {
> + size = end - start + 1;
> + } else {
> + size = 0;
> + }
> +
> + if (i < PCI_ROM_SLOT) {
> + d->io_regions[i].base_addr = start;
> + d->io_regions[i].size = size;
> + d->io_regions[i].flags = flags;
> + } else {
> + d->rom.base_addr = start;
> + d->rom.size = size;
> + d->rom.flags = flags;
> + }
> + }
> + if (i != PCI_NUM_REGIONS) {
> + rc = -1;
-ENODEV
> + }
> +
> +out:
> + close(fd);
> + return rc;
> +}
> +
> +static int xen_host_pci_get_value(XenHostPCIDevice *d, const char *name,
> + unsigned int *pvalue, int base)
> +{
> + char path[PATH_MAX];
> + char buf[42];
You should use a #define
> + int fd, rc;
> + unsigned long value;
> + char *endptr;
> +
> + if (xen_host_pci_sysfs_path(d, name, path, sizeof (path))) {
> + return -1;
-ENODEV
> + }
> + fd = open(path, O_RDONLY);
> + if (fd == -1) {
> + XEN_HOST_PCI_LOG("Error: Can't open %s: %s\n", path,
> strerror(errno));
> + return -errno;
> + }
> + do {
> + rc = read(fd, &buf, sizeof (buf) - 1);
Here you have sizeof(buf) - 1, but in the function xen_host_pci_get_resource(..)
you didn't do that. Any particular reason?
> + if (rc < 0 && errno != EINTR) {
> + rc = -errno;
> + goto out;
> + }
> + } while (rc < 0);
> + buf[rc] = 0;
> + value = strtol(buf, &endptr, base);
> + if (endptr == buf || *endptr != '\n') {
> + rc = -1;
??? -Exx something I think
> + } else if ((value == LONG_MIN || value == LONG_MAX) && errno == ERANGE) {
> + rc = -errno;
> + } else {
> + rc = 0;
> + *pvalue = value;
> + }
> +out:
> + close(fd);
> + return rc;
> +}
> +
> +static inline int xen_host_pci_get_hex_value(XenHostPCIDevice *d,
> + const char *name,
> + unsigned int *pvalue)
> +{
> + return xen_host_pci_get_value(d, name, pvalue, 16);
> +}
> +
> +static inline int xen_host_pci_get_dec_value(XenHostPCIDevice *d,
> + const char *name,
> + unsigned int *pvalue)
> +{
> + return xen_host_pci_get_value(d, name, pvalue, 10);
> +}
> +
> +static bool xen_host_pci_dev_is_virtfn(XenHostPCIDevice *d)
> +{
> + char path[PATH_MAX];
> + struct stat buf;
> +
> + if (xen_host_pci_sysfs_path(d, "physfn", path, sizeof (path))) {
> + return false;
> + }
> + return !stat(path, &buf);
> +}
> +
> +static int xen_host_pci_config_open(XenHostPCIDevice *d)
> +{
> + char path[PATH_MAX];
> +
> + if (xen_host_pci_sysfs_path(d, "config", path, sizeof (path))) {
> + return -1;
-ENODEV
> + }
> + d->config_fd = open(path, O_RDWR);
> + if (d->config_fd < 0) {
> + return -errno;
> + }
> + return 0;
> +}
> +
> +static int xen_host_pci_config_read(XenHostPCIDevice *d,
> + int pos, void *buf, int len)
> +{
> + int rc;
> +
> + do {
> + rc = pread(d->config_fd, buf, len, pos);
> + } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
> + if (rc != len) {
> + return -errno;
> + }
> + return 0;
> +}
> +
> +static int xen_host_pci_config_write(XenHostPCIDevice *d,
> + int pos, const void *buf, int len)
> +{
> + int rc;
> +
> + do {
> + rc = pwrite(d->config_fd, buf, len, pos);
> + } while (rc < 0 && (errno == EINTR || errno == EAGAIN));
> + if (rc != len) {
> + return -errno;
> + }
> + return 0;
> +}
> +
> +int xen_host_pci_get_byte(XenHostPCIDevice *d, int pos, uint8_t *p)
> +{
> + uint8_t buf;
> + int rc = xen_host_pci_config_read(d, pos, &buf, 1);
> + if (!rc) {
> + *p = buf;
> + }
> + return rc;
> +}
> +
> +int xen_host_pci_get_word(XenHostPCIDevice *d, int pos, uint16_t *p)
> +{
> + uint16_t buf;
> + int rc = xen_host_pci_config_read(d, pos, &buf, 2);
> + if (!rc) {
> + *p = le16_to_cpu(buf);
> + }
> + return rc;
> +}
> +
> +int xen_host_pci_get_long(XenHostPCIDevice *d, int pos, uint32_t *p)
> +{
> + uint32_t buf;
> + int rc = xen_host_pci_config_read(d, pos, &buf, 4);
> + if (!rc) {
> + *p = le32_to_cpu(buf);
> + }
> + return rc;
> +}
> +
> +int xen_host_pci_get_block(XenHostPCIDevice *d, int pos, uint8_t *buf, int
> len)
> +{
> + return xen_host_pci_config_read(d, pos, buf, len);
> +}
> +
> +int xen_host_pci_set_byte(XenHostPCIDevice *d, int pos, uint8_t data)
> +{
> + return xen_host_pci_config_write(d, pos, &data, 1);
> +}
> +
> +int xen_host_pci_set_word(XenHostPCIDevice *d, int pos, uint16_t data)
> +{
> + data = cpu_to_le16(data);
> + return xen_host_pci_config_write(d, pos, &data, 2);
> +}
> +
> +int xen_host_pci_set_long(XenHostPCIDevice *d, int pos, uint32_t data)
> +{
> + data = cpu_to_le32(data);
> + return xen_host_pci_config_write(d, pos, &data, 4);
> +}
> +
> +int xen_host_pci_set_block(XenHostPCIDevice *d, int pos, uint8_t *buf, int
> len)
> +{
> + return xen_host_pci_config_write(d, pos, buf, len);
> +}
> +
> +int xen_host_pci_find_ext_cap_offset(XenHostPCIDevice *d, uint32_t cap)
> +{
> + uint32_t header = 0;
> + int max_cap = XEN_HOST_PCI_MAX_EXT_CAP;
> + int pos = PCI_CONFIG_SPACE_SIZE;
> +
> + do {
> + if (xen_host_pci_get_long(d, pos, &header)) {
> + break;
> + }
> + /*
> + * If we have no capabilities, this is indicated by cap ID,
> + * cap version and next pointer all being 0.
> + */
> + if (header == 0) {
> + break;
> + }
> +
> + if (PCI_EXT_CAP_ID(header) == cap) {
> + return pos;
> + }
> +
> + pos = PCI_EXT_CAP_NEXT(header);
> + if (pos < PCI_CONFIG_SPACE_SIZE) {
> + break;
> + }
> +
> + max_cap--;
> + } while (max_cap > 0);
> +
> + return 0;
If we fail in this function (which can happen
[https://bugzilla.redhat.com/show_bug.cgi?id=767742])
shouldn't this function return something else besides 0?
Besides those comments, the patch looks good to me.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |