WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

[Xen-devel] Re: [PATCH] xen/pciback: miscellaneous adjustments

To: Jan Beulich <JBeulich@xxxxxxxx>
Subject: [Xen-devel] Re: [PATCH] xen/pciback: miscellaneous adjustments
From: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
Date: Wed, 21 Sep 2011 16:20:54 -0400
Cc: "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>
Delivery-date: Wed, 21 Sep 2011 13:22:02 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <4E778D100200007800056B76@xxxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <4E778D100200007800056B76@xxxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mutt/1.5.21 (2010-09-15)
On Mon, Sep 19, 2011 at 05:42:24PM +0100, Jan Beulich wrote:
> This is a minor bugfix and a set of small cleanups; as it is not clear
> whether this needs splitting into pieces (and if so, at what
> granularity), I'm submitting this as a single combined patch as a
> first try (I'd hope it doesn't need to be one patch per description
> line below):
> - add a missing return statement to an error path in
>   kill_domain_by_device()
> - use pci_is_enabled() rather than raw atomic_read()
> - use resource_size() rather than open coding it

I got a patch for that from 

commit b1d39d366a412f634fe50c1db6cfc751829157d3
Author: Thomas Meyer <thomas@xxxxxxxx>
Date:   Sat Aug 6 11:05:35 2011 +0200

    xen/pciback: use resource_size()
    
     Use resource_size function on resource object
     instead of explicit computation.
    
     The semantic patch that makes this output is available
     in scripts/coccinelle/api/resource_size.cocci.
    
     More information about semantic patching is available at
     http://coccinelle.lip6.fr/
    
    Signed-off-by: Thomas Meyer <thomas@xxxxxxxx>
    Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>

diff --git a/drivers/xen/xen-pciback/conf_space_header.c 
b/drivers/xen/xen-pciback/conf_space_header.c
index da3cbdf..f14b30f 100644
--- a/drivers/xen/xen-pciback/conf_space_header.c
+++ b/drivers/xen/xen-pciback/conf_space_header.c
@@ -187,7 +187,7 @@ static inline void read_dev_bar(struct pci_dev *dev,
 
        bar_info->val = res[pos].start |
                        (res[pos].flags & PCI_REGION_FLAG_MASK);
-       bar_info->len_val = res[pos].end - res[pos].start + 1;
+       bar_info->len_val = resource_size(&res[pos]);
 }
 
 static void *bar_init(struct pci_dev *dev, int offset)

so will use that instead, and drop that section from your patch.


> - remove a bogus attempt to zero-terminate an already zero-terminated
>   string
> - #define DRV_NAME once uniformly in the shared local header
> - make DRIVER_ATTR() variables static
> - eliminate a pointless use of list_for_each_entry_safe()
> - add MODULE_ALIAS()
> - a little bit of constification
> - adjust a few messages
> - remove stray semicolons from inline function definitions

Otherwsie they look good to me.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> ---
>  drivers/xen/xen-pciback/conf_space.c        |    1 
>  drivers/xen/xen-pciback/conf_space_header.c |    5 +---
>  drivers/xen/xen-pciback/conf_space_quirks.c |    3 --
>  drivers/xen/xen-pciback/passthrough.c       |    2 -
>  drivers/xen/xen-pciback/pci_stub.c          |   31 
> +++++++++++-----------------
>  drivers/xen/xen-pciback/pciback.h           |   30 
> +++++++++++++++++----------
>  drivers/xen/xen-pciback/pciback_ops.c       |    1 
>  drivers/xen/xen-pciback/vpci.c              |    9 +++-----
>  drivers/xen/xen-pciback/xenbus.c            |    5 +---
>  9 files changed, 42 insertions(+), 45 deletions(-)
> 
> --- 3.1-rc6/drivers/xen/xen-pciback/conf_space.c
> +++ 3.1-rc6-xen-pciback-misc/drivers/xen/xen-pciback/conf_space.c
> @@ -15,7 +15,6 @@
>  #include "conf_space.h"
>  #include "conf_space_quirks.h"
>  
> -#define DRV_NAME     "xen-pciback"
>  static int permissive;
>  module_param(permissive, bool, 0644);
>  
> --- 3.1-rc6/drivers/xen/xen-pciback/conf_space_header.c
> +++ 3.1-rc6-xen-pciback-misc/drivers/xen/xen-pciback/conf_space_header.c
> @@ -15,7 +15,6 @@ struct pci_bar_info {
>       int which;
>  };
>  
> -#define DRV_NAME     "xen-pciback"
>  #define is_enable_cmd(value) ((value)&(PCI_COMMAND_MEMORY|PCI_COMMAND_IO))
>  #define is_master_cmd(value) ((value)&PCI_COMMAND_MASTER)
>  
> @@ -25,7 +24,7 @@ static int command_read(struct pci_dev *
>       int ret;
>  
>       ret = xen_pcibk_read_config_word(dev, offset, value, data);
> -     if (!atomic_read(&dev->enable_cnt))
> +     if (!pci_is_enabled(dev))
>               return ret;
>  
>       for (i = 0; i < PCI_ROM_RESOURCE; i++) {
> @@ -187,7 +186,7 @@ static inline void read_dev_bar(struct p
>  
>       bar_info->val = res[pos].start |
>                       (res[pos].flags & PCI_REGION_FLAG_MASK);
> -     bar_info->len_val = res[pos].end - res[pos].start + 1;
> +     bar_info->len_val = resource_size(res + pos);
>  }
>  
>  static void *bar_init(struct pci_dev *dev, int offset)
> --- 3.1-rc6/drivers/xen/xen-pciback/conf_space_quirks.c
> +++ 3.1-rc6-xen-pciback-misc/drivers/xen/xen-pciback/conf_space_quirks.c
> @@ -12,7 +12,6 @@
>  #include "conf_space_quirks.h"
>  
>  LIST_HEAD(xen_pcibk_quirks);
> -#define      DRV_NAME        "xen-pciback"
>  static inline const struct pci_device_id *
>  match_one_device(const struct pci_device_id *id, const struct pci_dev *dev)
>  {
> @@ -36,7 +35,7 @@ static struct xen_pcibk_config_quirk *xe
>                       goto out;
>       tmp_quirk = NULL;
>       printk(KERN_DEBUG DRV_NAME
> -            ":quirk didn't match any device xen_pciback knows about\n");
> +            ": quirk didn't match any device known\n");
>  out:
>       return tmp_quirk;
>  }
> --- 3.1-rc6/drivers/xen/xen-pciback/passthrough.c
> +++ 3.1-rc6-xen-pciback-misc/drivers/xen/xen-pciback/passthrough.c
> @@ -182,7 +182,7 @@ static int __xen_pcibk_get_pcifront_dev(
>       return 1;
>  }
>  
> -struct xen_pcibk_backend xen_pcibk_passthrough_backend = {
> +const struct xen_pcibk_backend xen_pcibk_passthrough_backend = {
>       .name           = "passthrough",
>       .init           = __xen_pcibk_init_devices,
>       .free           = __xen_pcibk_release_devices,
> --- 3.1-rc6/drivers/xen/xen-pciback/pci_stub.c
> +++ 3.1-rc6-xen-pciback-misc/drivers/xen/xen-pciback/pci_stub.c
> @@ -21,8 +21,6 @@
>  #include "conf_space.h"
>  #include "conf_space_quirks.h"
>  
> -#define DRV_NAME     "xen-pciback"
> -
>  static char *pci_devs_to_hide;
>  wait_queue_head_t xen_pcibk_aer_wait_queue;
>  /*Add sem for sync AER handling and xen_pcibk remove/reconfigue ops,
> @@ -514,12 +512,14 @@ static void kill_domain_by_device(struct
>       int err;
>       char nodename[PCI_NODENAME_MAX];
>  
> -     if (!psdev)
> +     if (!psdev) {
>               dev_err(&psdev->dev->dev,
>                       "device is NULL when do AER recovery/kill_domain\n");
> +             return;
> +     }
> +
>       snprintf(nodename, PCI_NODENAME_MAX, "/local/domain/0/backend/pci/%d/0",
>               psdev->pdev->xdev->otherend_id);
> -     nodename[strlen(nodename)] = '\0';
>  
>  again:
>       err = xenbus_transaction_start(&xbt);
> @@ -605,7 +605,7 @@ static pci_ers_result_t common_process(s
>       if (test_bit(_XEN_PCIF_active,
>               (unsigned long *)&psdev->pdev->sh_info->flags)) {
>               dev_dbg(&psdev->dev->dev,
> -                     "schedule pci_conf service in xen_pcibk\n");
> +                     "schedule pci_conf service in " DRV_NAME "\n");
>               xen_pcibk_test_and_schedule_op(psdev->pdev);
>       }
>  
> @@ -995,8 +995,7 @@ out:
>               err = count;
>       return err;
>  }
> -
> -DRIVER_ATTR(new_slot, S_IWUSR, NULL, pcistub_slot_add);
> +static DRIVER_ATTR(new_slot, S_IWUSR, NULL, pcistub_slot_add);
>  
>  static ssize_t pcistub_slot_remove(struct device_driver *drv, const char 
> *buf,
>                                  size_t count)
> @@ -1015,8 +1014,7 @@ out:
>               err = count;
>       return err;
>  }
> -
> -DRIVER_ATTR(remove_slot, S_IWUSR, NULL, pcistub_slot_remove);
> +static DRIVER_ATTR(remove_slot, S_IWUSR, NULL, pcistub_slot_remove);
>  
>  static ssize_t pcistub_slot_show(struct device_driver *drv, char *buf)
>  {
> @@ -1039,8 +1037,7 @@ static ssize_t pcistub_slot_show(struct 
>  
>       return count;
>  }
> -
> -DRIVER_ATTR(slots, S_IRUSR, pcistub_slot_show, NULL);
> +static DRIVER_ATTR(slots, S_IRUSR, pcistub_slot_show, NULL);
>  
>  static ssize_t pcistub_irq_handler_show(struct device_driver *drv, char *buf)
>  {
> @@ -1069,8 +1066,7 @@ static ssize_t pcistub_irq_handler_show(
>       spin_unlock_irqrestore(&pcistub_devices_lock, flags);
>       return count;
>  }
> -
> -DRIVER_ATTR(irq_handlers, S_IRUSR, pcistub_irq_handler_show, NULL);
> +static DRIVER_ATTR(irq_handlers, S_IRUSR, pcistub_irq_handler_show, NULL);
>  
>  static ssize_t pcistub_irq_handler_switch(struct device_driver *drv,
>                                         const char *buf,
> @@ -1106,7 +1102,7 @@ out:
>               err = count;
>       return err;
>  }
> -DRIVER_ATTR(irq_handler_state, S_IWUSR, NULL, pcistub_irq_handler_switch);
> +static DRIVER_ATTR(irq_handler_state, S_IWUSR, NULL, 
> pcistub_irq_handler_switch);
>  
>  static ssize_t pcistub_quirk_add(struct device_driver *drv, const char *buf,
>                                size_t count)
> @@ -1170,8 +1166,7 @@ out:
>  
>       return count;
>  }
> -
> -DRIVER_ATTR(quirks, S_IRUSR | S_IWUSR, pcistub_quirk_show, 
> pcistub_quirk_add);
> +static DRIVER_ATTR(quirks, S_IRUSR | S_IWUSR, pcistub_quirk_show, 
> pcistub_quirk_add);
>  
>  static ssize_t permissive_add(struct device_driver *drv, const char *buf,
>                             size_t count)
> @@ -1236,8 +1231,7 @@ static ssize_t permissive_show(struct de
>       spin_unlock_irqrestore(&pcistub_devices_lock, flags);
>       return count;
>  }
> -
> -DRIVER_ATTR(permissive, S_IRUSR | S_IWUSR, permissive_show, permissive_add);
> +static DRIVER_ATTR(permissive, S_IRUSR | S_IWUSR, permissive_show, 
> permissive_add);
>  
>  static void pcistub_exit(void)
>  {
> @@ -1374,3 +1368,4 @@ module_init(xen_pcibk_init);
>  module_exit(xen_pcibk_cleanup);
>  
>  MODULE_LICENSE("Dual BSD/GPL");
> +MODULE_ALIAS("xen-backend:pci");
> --- 3.1-rc6/drivers/xen/xen-pciback/pciback.h
> +++ 3.1-rc6-xen-pciback-misc/drivers/xen/xen-pciback/pciback.h
> @@ -15,6 +15,8 @@
>  #include <linux/atomic.h>
>  #include <xen/interface/io/pciif.h>
>  
> +#define DRV_NAME     "xen-pciback"
> +
>  struct pci_dev_entry {
>       struct list_head list;
>       struct pci_dev *dev;
> @@ -89,7 +91,7 @@ typedef int (*publish_pci_root_cb) (stru
>   *  passthrough - BDFs are exactly like in the host.
>   */
>  struct xen_pcibk_backend {
> -     char *name;
> +     const char *name;
>       int (*init)(struct xen_pcibk_device *pdev);
>       void (*free)(struct xen_pcibk_device *pdev);
>       int (*find)(struct pci_dev *pcidev, struct xen_pcibk_device *pdev,
> @@ -104,9 +106,9 @@ struct xen_pcibk_backend {
>                              unsigned int devfn);
>  };
>  
> -extern struct xen_pcibk_backend xen_pcibk_vpci_backend;
> -extern struct xen_pcibk_backend xen_pcibk_passthrough_backend;
> -extern struct xen_pcibk_backend *xen_pcibk_backend;
> +extern const struct xen_pcibk_backend xen_pcibk_vpci_backend;
> +extern const struct xen_pcibk_backend xen_pcibk_passthrough_backend;
> +extern const struct xen_pcibk_backend *xen_pcibk_backend;
>  
>  static inline int xen_pcibk_add_pci_dev(struct xen_pcibk_device *pdev,
>                                       struct pci_dev *dev,
> @@ -116,13 +118,14 @@ static inline int xen_pcibk_add_pci_dev(
>       if (xen_pcibk_backend && xen_pcibk_backend->add)
>               return xen_pcibk_backend->add(pdev, dev, devid, publish_cb);
>       return -1;
> -};
> +}
> +
>  static inline void xen_pcibk_release_pci_dev(struct xen_pcibk_device *pdev,
>                                            struct pci_dev *dev)
>  {
>       if (xen_pcibk_backend && xen_pcibk_backend->free)
>               return xen_pcibk_backend->release(pdev, dev);
> -};
> +}
>  
>  static inline struct pci_dev *
>  xen_pcibk_get_pci_dev(struct xen_pcibk_device *pdev, unsigned int domain,
> @@ -131,7 +134,8 @@ xen_pcibk_get_pci_dev(struct xen_pcibk_d
>       if (xen_pcibk_backend && xen_pcibk_backend->get)
>               return xen_pcibk_backend->get(pdev, domain, bus, devfn);
>       return NULL;
> -};
> +}
> +
>  /**
>  * Add for domain0 PCIE-AER handling. Get guest domain/bus/devfn in xen_pcibk
>  * before sending aer request to pcifront, so that guest could identify
> @@ -148,25 +152,29 @@ static inline int xen_pcibk_get_pcifront
>               return xen_pcibk_backend->find(pcidev, pdev, domain, bus,
>                                              devfn);
>       return -1;
> -};
> +}
> +
>  static inline int xen_pcibk_init_devices(struct xen_pcibk_device *pdev)
>  {
>       if (xen_pcibk_backend && xen_pcibk_backend->init)
>               return xen_pcibk_backend->init(pdev);
>       return -1;
> -};
> +}
> +
>  static inline int xen_pcibk_publish_pci_roots(struct xen_pcibk_device *pdev,
>                                             publish_pci_root_cb cb)
>  {
>       if (xen_pcibk_backend && xen_pcibk_backend->publish)
>               return xen_pcibk_backend->publish(pdev, cb);
>       return -1;
> -};
> +}
> +
>  static inline void xen_pcibk_release_devices(struct xen_pcibk_device *pdev)
>  {
>       if (xen_pcibk_backend && xen_pcibk_backend->free)
>               return xen_pcibk_backend->free(pdev);
> -};
> +}
> +
>  /* Handles events from front-end */
>  irqreturn_t xen_pcibk_handle_event(int irq, void *dev_id);
>  void xen_pcibk_do_op(struct work_struct *data);
> --- 3.1-rc6/drivers/xen/xen-pciback/pciback_ops.c
> +++ 3.1-rc6-xen-pciback-misc/drivers/xen/xen-pciback/pciback_ops.c
> @@ -10,7 +10,6 @@
>  #include <linux/sched.h>
>  #include "pciback.h"
>  
> -#define DRV_NAME     "xen-pciback"
>  int verbose_request;
>  module_param(verbose_request, int, 0644);
>  
> --- 3.1-rc6/drivers/xen/xen-pciback/vpci.c
> +++ 3.1-rc6-xen-pciback-misc/drivers/xen/xen-pciback/vpci.c
> @@ -12,7 +12,6 @@
>  #include "pciback.h"
>  
>  #define PCI_SLOT_MAX 32
> -#define DRV_NAME     "xen-pciback"
>  
>  struct vpci_dev_data {
>       /* Access to dev_list must be protected by lock */
> @@ -150,9 +149,9 @@ static void __xen_pcibk_release_pci_dev(
>       spin_lock_irqsave(&vpci_dev->lock, flags);
>  
>       for (slot = 0; slot < PCI_SLOT_MAX; slot++) {
> -             struct pci_dev_entry *e, *tmp;
> -             list_for_each_entry_safe(e, tmp, &vpci_dev->dev_list[slot],
> -                                      list) {
> +             struct pci_dev_entry *e;
> +
> +             list_for_each_entry(e, &vpci_dev->dev_list[slot], list) {
>                       if (e->dev == dev) {
>                               list_del(&e->list);
>                               found_dev = e->dev;
> @@ -247,7 +246,7 @@ static int __xen_pcibk_get_pcifront_dev(
>       return found;
>  }
>  
> -struct xen_pcibk_backend xen_pcibk_vpci_backend = {
> +const struct xen_pcibk_backend xen_pcibk_vpci_backend = {
>       .name           = "vpci",
>       .init           = __xen_pcibk_init_devices,
>       .free           = __xen_pcibk_release_devices,
> --- 3.1-rc6/drivers/xen/xen-pciback/xenbus.c
> +++ 3.1-rc6-xen-pciback-misc/drivers/xen/xen-pciback/xenbus.c
> @@ -13,7 +13,6 @@
>  #include <asm/xen/pci.h>
>  #include "pciback.h"
>  
> -#define      DRV_NAME        "xen-pciback"
>  #define INVALID_EVTCHN_IRQ  (-1)
>  struct workqueue_struct *xen_pcibk_wq;
>  
> @@ -176,7 +175,7 @@ static int xen_pcibk_attach(struct xen_p
>       if (magic == NULL || strcmp(magic, XEN_PCI_MAGIC) != 0) {
>               xenbus_dev_fatal(pdev->xdev, -EFAULT,
>                                "version mismatch (%s/%s) with pcifront - "
> -                              "halting xen_pcibk",
> +                              "halting " DRV_NAME,
>                                magic, XEN_PCI_MAGIC);
>               goto out;
>       }
> @@ -724,7 +723,7 @@ static struct xenbus_driver xenbus_xen_p
>       .otherend_changed       = xen_pcibk_frontend_changed,
>  };
>  
> -struct xen_pcibk_backend *xen_pcibk_backend;
> +const struct xen_pcibk_backend *__read_mostly xen_pcibk_backend;
>  
>  int __init xen_pcibk_xenbus_register(void)
>  {
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel

<Prev in Thread] Current Thread [Next in Thread>