Re: [Xen-devel] [PATCH V10 3/8] Introduce XenHostPCIDevice to access a pci device on the host.

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Wed, Mar 28, 2012 at 19:52, Konrad Rzeszutek Wilk
<konrad.wilk@xxxxxxxxxx> wrote:
>> +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

I still does not understand why return ENODEV (No such device) when an
error occure. Is it the better way to say that a device does not
behave like it should, or that a "module" using a device encounter any
kind of error that prevent it to use the device ?


>> + Â Â}
>> + Â Â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?

Nop, it's just a mistake.

>> + Â Â Â Â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;
>> +}

[...]

>> +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?

-1 should be good enough, as it's not an offset.

> Besides those comments, the patch looks good to me.

Thanks,

-- 
Anthony PERARD

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