Re: [dpdk-dev] [PATCH 1/4] [PATCH 1/4] igb_uio: Add interrupt_mode sysfs entry for igb_uio devices

[Stephen Hemminger <stephen@networkplumber.org>]

On Thu,  5 Jun 2014 15:39:16 +0100
Alan Carew <alan.carew@intel.com> wrote:

> This patch adds an "interrupt_mode" sysfs entry for igb_uio devices,
> allowing userspace eal_pci to track which interrupt mode has been enabled in kernel space.
> The sysfs entry can be inspected via /sys/bus/pci/devices/<D:B:D:F/interrupt_mode
> 
> Signed-off-by: Alan Carew <alan.carew@intel.com>
> ---
>  .../common/include/rte_pci_dev_feature_defs.h      |   85 ++++++++++++++++++++
>  .../common/include/rte_pci_dev_features.h          |   70 ++++++++++++++++
>  lib/librte_eal/linuxapp/igb_uio/igb_uio.c          |   48 +++++++++---
>  3 files changed, 193 insertions(+), 10 deletions(-)
>  create mode 100755 lib/librte_eal/common/include/rte_pci_dev_feature_defs.h
>  create mode 100755 lib/librte_eal/common/include/rte_pci_dev_features.h
> 
> diff --git a/lib/librte_eal/common/include/rte_pci_dev_feature_defs.h b/lib/librte_eal/common/include/rte_pci_dev_feature_defs.h
> new file mode 100755
> index 0000000..d23ed7d
> --- /dev/null
> +++ b/lib/librte_eal/common/include/rte_pci_dev_feature_defs.h
> @@ -0,0 +1,85 @@
> +/*-
> + * This file is provided under a dual BSD/GPLv2 license.  When using or
> + *   redistributing this file, you may do so under either license.
> + * 
> + *   GPL LICENSE SUMMARY
> + * 
> + *   Copyright(c) 2010-2014 Intel Corporation. All rights reserved.
> + * 
> + *   This program is free software; you can redistribute it and/or modify
> + *   it under the terms of version 2 of the GNU General Public License as
> + *   published by the Free Software Foundation.
> + * 
> + *   This program is distributed in the hope that it will be useful, but
> + *   WITHOUT ANY WARRANTY; without even the implied warranty of
> + *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *   General Public License for more details.
> + * 
> + *   You should have received a copy of the GNU General Public License
> + *   along with this program; if not, write to the Free Software
> + *   Foundation, Inc., 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
> + *   The full GNU General Public License is included in this distribution
> + *   in the file called LICENSE.GPL.
> + * 
> + *   Contact Information:
> + *   Intel Corporation
> + * 
> + *   BSD LICENSE
> + * 
> + *   Copyright(c) 2010-2014 Intel Corporation. All rights reserved.
> + *   All rights reserved.
> + * 
> + *   Redistribution and use in source and binary forms, with or without
> + *   modification, are permitted provided that the following conditions
> + *   are met:
> + * 
> + *     * Redistributions of source code must retain the above copyright
> + *       notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above copyright
> + *       notice, this list of conditions and the following disclaimer in
> + *       the documentation and/or other materials provided with the
> + *       distribution.
> + *     * Neither the name of Intel Corporation nor the names of its
> + *       contributors may be used to endorse or promote products derived
> + *       from this software without specific prior written permission.
> + * 
> + *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + * 
> + */
> +
> +#ifndef _RTE_PCI_DEV_DEFS_H_
> +#define _RTE_PCI_DEV_DEFS_H_
> +
> +#define RTE_PCI_DEV_FEATURE_INTR_MODE  "interrupt_mode"
> +
> +#define INTR_NAME_LEN 10
> +
> +#define IGBUIO_NONE_INTR_NAME "none"
> +#define IGBUIO_LEGACY_INTR_NAME "legacy"
> +#define IGBUIO_MSI_INTR_NAME "msi"
> +#define IGBUIO_MSIX_INTR_NAME "msix"
> +
> +
> +#define INTR_MODE(id, mode_name) \
> +        .mode = (id), .name = (mode_name)
> +
> +/* interrupt mode */
> +enum igbuio_intr_mode {
> +	IGBUIO_NONE_INTR_MODE = 0,
> +	IGBUIO_LEGACY_INTR_MODE,
> +	IGBUIO_MSI_INTR_MODE,
> +	IGBUIO_MSIX_INTR_MODE,
> +	IGBUIO_INTR_MODE_MAX
> +};
> +
> +#endif /* _RTE_PCI_DEV_DEFS_H_ */
> diff --git a/lib/librte_eal/common/include/rte_pci_dev_features.h b/lib/librte_eal/common/include/rte_pci_dev_features.h
> new file mode 100755
> index 0000000..a45c056
> --- /dev/null
> +++ b/lib/librte_eal/common/include/rte_pci_dev_features.h
> @@ -0,0 +1,70 @@
> +/*-
> + * This file is provided under a dual BSD/GPLv2 license.  When using or
> + *   redistributing this file, you may do so under either license.
> + * 
> + *   GPL LICENSE SUMMARY
> + * 
> + *   Copyright(c) 2010-2014 Intel Corporation. All rights reserved.
> + * 
> + *   This program is free software; you can redistribute it and/or modify
> + *   it under the terms of version 2 of the GNU General Public License as
> + *   published by the Free Software Foundation.
> + * 
> + *   This program is distributed in the hope that it will be useful, but
> + *   WITHOUT ANY WARRANTY; without even the implied warranty of
> + *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *   General Public License for more details.
> + * 
> + *   You should have received a copy of the GNU General Public License
> + *   along with this program; if not, write to the Free Software
> + *   Foundation, Inc., 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
> + *   The full GNU General Public License is included in this distribution
> + *   in the file called LICENSE.GPL.
> + * 
> + *   Contact Information:
> + *   Intel Corporation
> + * 
> + *   BSD LICENSE
> + * 
> + *   Copyright(c) 2010-2014 Intel Corporation. All rights reserved.
> + *   All rights reserved.
> + * 
> + *   Redistribution and use in source and binary forms, with or without
> + *   modification, are permitted provided that the following conditions
> + *   are met:
> + * 
> + *     * Redistributions of source code must retain the above copyright
> + *       notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above copyright
> + *       notice, this list of conditions and the following disclaimer in
> + *       the documentation and/or other materials provided with the
> + *       distribution.
> + *     * Neither the name of Intel Corporation nor the names of its
> + *       contributors may be used to endorse or promote products derived
> + *       from this software without specific prior written permission.
> + * 
> + *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + * 
> + */
> +
> +#ifndef RTE_PCI_DEV_INTR_MODE
> +#define RTE_PCI_DEV_INTR_MODE(id, mode_name)
> +#endif
> +
> +RTE_PCI_DEV_INTR_MODE(IGBUIO_NONE_INTR_MODE, IGBUIO_NONE_INTR_NAME)
> +RTE_PCI_DEV_INTR_MODE(IGBUIO_LEGACY_INTR_MODE, IGBUIO_LEGACY_INTR_NAME)
> +RTE_PCI_DEV_INTR_MODE(IGBUIO_MSI_INTR_MODE, IGBUIO_MSI_INTR_NAME)
> +RTE_PCI_DEV_INTR_MODE(IGBUIO_MSIX_INTR_MODE, IGBUIO_MSIX_INTR_NAME)
> +
> +#undef RTE_PCI_DEV_INTR_MODE
> +
> diff --git a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> index 09c40bf..dd1124b 100644
> --- a/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> +++ b/lib/librte_eal/linuxapp/igb_uio/igb_uio.c
> @@ -34,6 +34,8 @@
>  #include <xen/xen.h>
>  #endif
>  
> +#include <rte_pci_dev_feature_defs.h>
> +
>  /**
>   * MSI-X related macros, copy from linux/pci_regs.h in kernel 2.6.39,
>   * but none of them in kernel 2.6.35.
> @@ -49,12 +51,15 @@
>  
>  #define IGBUIO_NUM_MSI_VECTORS 1
>  
> -/* interrupt mode */
> -enum igbuio_intr_mode {
> -	IGBUIO_LEGACY_INTR_MODE = 0,
> -	IGBUIO_MSI_INTR_MODE,
> -	IGBUIO_MSIX_INTR_MODE,
> -	IGBUIO_INTR_MODE_MAX
> +struct rte_pci_dev_intr_mode {
> +	enum igbuio_intr_mode mode;
> +	const char *name;
> +};
> +
> +/* Table of interrupt modes */
> +static struct rte_pci_dev_intr_mode interrupt_modes[] = {
> +#define RTE_PCI_DEV_INTR_MODE(id, mode_name) {INTR_MODE(id, mode_name)},
> +#include <rte_pci_dev_features.h>
>  };
>  
>  /**
> @@ -150,8 +155,32 @@ store_max_vfs(struct device *dev, struct device_attribute *attr,
>  }
>  
>  static DEVICE_ATTR(max_vfs, S_IRUGO | S_IWUSR, show_max_vfs, store_max_vfs);
> +
> +static ssize_t
> +show_interrupt_mode(struct device *dev, struct device_attribute *attr,
> +	     char *buf)
> +{
> +	unsigned i, num_modes;
> +	enum igbuio_intr_mode mode;
> +	struct uio_info *info;
> +	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
> +
> +	info = pci_get_drvdata(pdev);
> +	mode = ((struct rte_uio_pci_dev *)info->priv)->mode;
> +	num_modes = sizeof(interrupt_modes)/sizeof(interrupt_modes[0]);
> +	
> +	for (i = 0; i < num_modes; i++) {
> +		if (mode == interrupt_modes[i].mode)
> +			return snprintf(buf, INTR_NAME_LEN, "%s\n", interrupt_modes[i].name );
> +	}
> +	return snprintf(buf, INTR_NAME_LEN, "unknown\n");
> +}
> +
> +static DEVICE_ATTR(interrupt_mode, S_IRUGO, show_interrupt_mode, NULL);
> +
>  static struct attribute *dev_attrs[] = {
>  	&dev_attr_max_vfs.attr,
> +	&dev_attr_interrupt_mode.attr,
>          NULL,
>  };
>  
> @@ -520,7 +549,7 @@ igbuio_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  #endif
>  	udev->info.priv = udev;
>  	udev->pdev = dev;
> -	udev->mode = 0; /* set the default value for interrupt mode */
> +	udev->mode = IGBUIO_LEGACY_INTR_MODE; /* set the default value for interrupt mode */
>  	spin_lock_init(&udev->lock);
>  
>  	/* check if it need to try msix first */
> @@ -610,11 +639,10 @@ igbuio_config_intr_mode(char *intr_str)
>  		printk(KERN_INFO "Use MSIX interrupt by default\n");
>  		return 0;
>  	}
> -
> -	if (!strcmp(intr_str, "msix")) {
> +	if (!strcmp(intr_str, IGBUIO_MSIX_INTR_NAME)) {
>  		igbuio_intr_mode_preferred = IGBUIO_MSIX_INTR_MODE;
>  		printk(KERN_INFO "Use MSIX interrupt\n");
> -	} else if (!strcmp(intr_str, "legacy")) {
> +	} else if (!strcmp(intr_str, IGBUIO_LEGACY_INTR_NAME)) {
>  		igbuio_intr_mode_preferred = IGBUIO_LEGACY_INTR_MODE;
>  		printk(KERN_INFO "Use legacy interrupt\n");
>  	} else {

This assumes that all devices have same interrupt mode.
The whole intr_mode_preferred logic in this driver is borked.