RE: [PATCH 01/04] OMAP3 SRF: Generic shared resource f/w

["Rajendra Nayak" <rnayak@ti.com>]

> -----Original Message-----
> From: Dasgupta, Romit [mailto:romit@ti.com] 
> Sent: Friday, October 17, 2008 11:47 AM
> To: Nayak, Rajendra; linux-omap@vger.kernel.org
> Subject: RE: [PATCH 01/04] OMAP3 SRF: Generic shared resource f/w
> 
> Rajendra,
>                    Few bugs in the code
> 1) curr_level of the shared_resource is not updated in 
> update_resource_level

Yes, its not. Its done as part of the platform specific change_level call.

> 2) resource_request invokes spin_lock_irqsave and then if it 
> is a request for a new device it invokes get_user().
> get_user() calls kmalloc with GFP_KERNEL. So it can sleep. 
> Hence you will sleep with spinlocks held!!

Right, so I'll probably have to add a GFP_ATOMIC flag to that.
I am now thinking If I really need spinlocks, think I can do with mutex's instead.
The spinlocks were put in place to take care of the omap-pm hooks from 
clock f/w which no longer seem to be needed.
 
> 
> Trying to review the remaining patches.
> 
> Regards,
> -Romit
> 
> 
> >-----Original Message-----
> >From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> >owner@vger.kernel.org] On Behalf Of Nayak, Rajendra
> >Sent: Thursday, October 16, 2008 7:42 PM
> >To: linux-omap@vger.kernel.org
> >Subject: [PATCH 01/04] OMAP3 SRF: Generic shared resource f/w
> >
> >This patch defines and implements the Generic
> >Shared Resource Framework structures and API's
> >
> >Signed-off-by: Rajendra Nayak <rnayak@ti.com>
> >---
> > arch/arm/plat-omap/include/mach/resource.h |   79 +++++
> > arch/arm/plat-omap/resource.c              |  433 
> +++++++++++++++++++++++++++++
> > 2 files changed, 512 insertions(+)
> >
> >Index: linux-omap-2.6/arch/arm/plat-omap/resource.c
> >================================================================
> >===
> >--- /dev/null  1970-01-01 00:00:00.000000000 +0000
> >+++ linux-omap-2.6/arch/arm/plat-omap/resource.c       2008-10-16
> >18:04:05.000000000 +0530
> >@@ -0,0 +1,433 @@
> >+/*
> >+ * linux/arch/arm/plat-omap/resource.c
> >+ * Shared Resource Framework API implementation
> >+ *
> >+ * Copyright (C) 2007-2008 Texas Instruments, Inc.
> >+ * Rajendra Nayak <rnayak@ti.com>
> >+ *
> >+ * This program is free software; you can redistribute it 
> and/or modify
> >+ * it under the terms of the GNU General Public License version 2 as
> >+ * published by the Free Software Foundation.
> >+ *
> >+ * THIS PACKAGE IS PROVIDED ``AS IS'' AND WITHOUT ANY EXPRESS OR
> >+ * IMPLIED WARRANTIES, INCLUDING, WITHOUT LIMITATION, THE IMPLIED
> >+ * WARRANTIES OF MERCHANTIBILITY AND FITNESS FOR A PARTICULAR
> >PURPOSE.
> >+ * History:
> >+ *
> >+ */
> >+
> >+#include <linux/errno.h>
> >+#include <linux/err.h>
> >+#include <mach/resource.h>
> >+#include <linux/slab.h>
> >+
> >+/*
> >+ * This is for statically defining the users pool. This 
> static pool is
> >+ * used early at bootup till kmalloc becomes available.
> >+ */
> >+#define MAX_USERS     10
> >+#define UNUSED                0x0
> >+#define       DYNAMIC_ALLOC   0x1
> >+#define STATIC_ALLOC  0x2
> >+
> >+/* res_list contains all registered struct shared_resource */
> >+static LIST_HEAD(res_list);
> >+
> >+/* res_lock protects res_list add and del ops */
> >+static DEFINE_SPINLOCK(res_lock);
> >+
> >+/* Static Pool of users for a resource used till kmalloc 
> becomes available */
> >+struct  users_list usr_list[MAX_USERS];
> >+
> >+/* Private/Internal functions */
> >+
> >+/**
> >+ * _resource_lookup - loop up a resource by its name, 
> return a pointer
> >+ * @name: The name of the resource to lookup
> >+ *
> >+ * Looks for a registered resource by its name. Returns a pointer to
> >+ * the struct shared_resource if found, else returns NULL.
> >+ * The function is not lock safe.
> >+ */
> >+static struct shared_resource *_resource_lookup(const char *name)
> >+{
> >+      struct shared_resource *res, *tmp_res;
> >+
> >+      if (!name)
> >+              return NULL;
> >+
> >+      res = NULL;
> >+
> >+      list_for_each_entry(tmp_res, &res_list, node) {
> >+              if (!strcmp(name, tmp_res->name)) {
> >+                      res = tmp_res;
> >+                      break;
> >+              }
> >+      }
> >+      return res;
> >+}
> >+
> >+/**
> >+ * resource_lookup - loop up a resource by its name, return 
> a pointer
> >+ * @name: The name of the resource to lookup
> >+ *
> >+ * Looks for a registered resource by its name. Returns a pointer to
> >+ * the struct shared_resource if found, else returns NULL.
> >+ * The function holds spinlocks and takes care of atomicity.
> >+ */
> >+static struct shared_resource *resource_lookup(const char *name)
> >+{
> >+      struct shared_resource *res;
> >+      unsigned long flags;
> >+
> >+      if (!name)
> >+              return NULL;
> >+      spin_lock_irqsave(&res_lock, flags);
> >+      res = _resource_lookup(name);
> >+      spin_unlock_irqrestore(&res_lock, flags);
> >+
> >+      return res;
> >+}
> >+
> >+/**
> >+ * update_resource_level - Regenerates and updates the 
> curr_level of the res
> >+ * @resp: Pointer to the resource
> >+ *
> >+ * This function looks at all the users of the given 
> resource and the levels
> >+ * requested by each of them, and recomputes a target level 
> for the resource
> >+ * acceptable to all its current usres. It then calls 
> platform specific
> >+ * change_level to change the level of the resource.
> >+ * Returns 0 on success, else a non-zero value returned by 
> the platform
> >+ * specific change_level function.
> >+ **/
> >+static int update_resource_level(struct shared_resource *resp)
> >+{
> >+      struct users_list *user;
> >+      unsigned long target_level;
> >+      int ret;
> >+
> >+      /* Regenerate the target_value for the resource */
> >+      target_level = RES_DEFAULTLEVEL;
> >+      list_for_each_entry(user, &resp->users_list, node)
> >+              if (user->level > target_level)
> >+                      target_level = user->level;
> >+
> >+      pr_debug("SRF: Changing Level for resource %s to %ld\n",
> >+                              resp->name, target_level);
> >+      ret = resp->ops->change_level(resp, target_level);
> >+      if (ret) {
> >+              printk(KERN_ERR "Unable to Change"
> >+                                      "level for resource 
> %s to %ld\n",
> >+              resp->name, target_level);
> >+      }
> >+      return ret;
> >+}
> >+
> >+/**
> >+ * get_user - gets a new users_list struct from static pool 
> or dynamically
> >+ *
> >+ * This function initally looks for availability in the 
> static pool and
> >+ * tries to dynamcially allocate only once the static pool is empty.
> >+ * We hope that during bootup by the time we hit a case of 
> dynamic allocation
> >+ * slab initialization would have happened.
> >+ * Returns a pointer users_list struct on success. On 
> dynamic allocation failure
> >+ * returns a ERR_PTR(-ENOMEM).
> >+ */
> >+static struct users_list *get_user(void)
> >+{
> >+      int ind = 0;
> >+      struct users_list *user;
> >+
> >+      /* See if something available in the static pool */
> >+      while (ind < MAX_USERS) {
> >+              if (usr_list[ind].usage == UNUSED)
> >+                      break;
> >+              else
> >+                      ind++;
> >+      }
> >+      if (ind < MAX_USERS) {
> >+              /* Pick from the static pool */
> >+              user = &usr_list[ind];
> >+              user->usage = STATIC_ALLOC;
> >+      } else {
> >+              /* By this time we hope slab is initialized */
> >+              if (slab_is_available()) {
> >+                      user = kmalloc(sizeof(struct  
> users_list), GFP_KERNEL);
> >+                      if (!user) {
> >+                              printk(KERN_ERR "SRF:FATAL 
> ERROR: kmalloc"
> >+                                                      "failed\n");
> >+                              return ERR_PTR(-ENOMEM);
> >+                      }
> >+                      user->usage = DYNAMIC_ALLOC;
> >+              } else {
> >+                      /* Dynamic alloc not available yet */
> >+                      printk(KERN_ERR "SRF: FATAL ERROR: users_list"
> >+                              "initial POOL EMPTY before 
> slab init\n");
> >+                      return ERR_PTR(-ENOMEM);
> >+              }
> >+      }
> >+      return user;
> >+}
> >+
> >+/**
> >+ * free_user - frees the dynamic users_list and marks the 
> static one unused
> >+ * @user: The struct users_list to be freed
> >+ *
> >+ * Looks at the usage flag and either frees the users_list if it was
> >+ * dynamically allocated, and if its from the static pool, 
> marks it unused.
> >+ * No return value.
> >+ */
> >+void free_user(struct users_list *user)
> >+{
> >+      if (user->usage == DYNAMIC_ALLOC) {
> >+              kfree(user);
> >+      } else {
> >+              user->usage = UNUSED;
> >+              user->level = RES_DEFAULTLEVEL;
> >+              user->dev = NULL;
> >+      }
> >+}
> >+
> >+/**
> >+ * resource_init - Initializes the Shared resource framework.
> >+ * @resources: List of all the resources modelled
> >+ *
> >+ * Loops through the list of resources and registers all that
> >+ * are available for the current CPU.
> >+ * No return value
> >+ */
> >+void resource_init(struct shared_resource **resources)
> >+{
> >+      struct shared_resource **resp;
> >+      int ind;
> >+
> >+      pr_debug("Initializing Shared Resource Framework\n");
> >+
> >+      if (!cpu_is_omap34xx()) {
> >+              /* This CPU is not supported */
> >+              printk(KERN_WARNING "Shared Resource 
> Framework does not"
> >+                      "support this CPU type.\n");
> >+              WARN_ON(1);
> >+      }
> >+
> >+      /* Init the users_list POOL */
> >+      for (ind = 0; ind < MAX_USERS; ind++) {
> >+              usr_list[ind].usage = UNUSED;
> >+              usr_list[ind].dev = NULL;
> >+              usr_list[ind].level = RES_DEFAULTLEVEL;
> >+      }
> >+
> >+      if (resources)
> >+              for (resp = resources; *resp; resp++)
> >+                      resource_register(*resp);
> >+}
> >+
> >+/**
> >+ * resource_register - registers and initializes a resource
> >+ * @res: struct shared_resource * to register
> >+ *
> >+ * Initializes the given resource and adds it to the resource list
> >+ * for the current CPU.
> >+ * Returns 0 on success, -EINVAL if given a NULL pointer, 
> -EEXIST if the
> >+ * resource is already registered.
> >+ */
> >+int resource_register(struct shared_resource *resp)
> >+{
> >+      unsigned long flags;
> >+
> >+      if (!resp)
> >+              return -EINVAL;
> >+
> >+      if (!omap_chip_is(resp->omap_chip))
> >+              return -EINVAL;
> >+
> >+      /* Verify that the resource is not already registered */
> >+      if (resource_lookup(resp->name))
> >+              return -EEXIST;
> >+
> >+      INIT_LIST_HEAD(&resp->users_list);
> >+
> >+      spin_lock_irqsave(&res_lock, flags);
> >+      /* Add the resource to the resource list */
> >+      list_add(&resp->node, &res_list);
> >+
> >+      /* Call the resource specific init*/
> >+      if (resp->ops->init)
> >+              resp->ops->init(resp);
> >+
> >+      spin_unlock_irqrestore(&res_lock, flags);
> >+      pr_debug("resource: registered %s\n", resp->name);
> >+
> >+      return 0;
> >+}
> >+EXPORT_SYMBOL(resource_register);
> >+
> >+/**
> >+ * resource_unregister - unregister a resource
> >+ * @res: struct shared_resource * to unregister
> >+ *
> >+ * Removes a resource from the resource list.
> >+ * Returns 0 on success, -EINVAL if passed a NULL pointer.
> >+ */
> >+int resource_unregister(struct shared_resource *resp)
> >+{
> >+      unsigned long flags;
> >+
> >+      if (!resp)
> >+              return -EINVAL;
> >+
> >+      spin_lock_irqsave(&res_lock, flags);
> >+      /* delete the resource from the resource list */
> >+      list_del(&resp->node);
> >+      spin_unlock_irqrestore(&res_lock, flags);
> >+
> >+      pr_debug("resource: unregistered %s\n", resp->name);
> >+
> >+      return 0;
> >+}
> >+EXPORT_SYMBOL(resource_unregister);
> >+
> >+/**
> >+ * resource_request - Request for a required level of a resource
> >+ * @name: The name of the resource requested
> >+ * @dev: Uniquely identifes the caller
> >+ * @level: The requested level for the resource
> >+ *
> >+ * This function recomputes the target level of the 
> resource based on
> >+ * the level requested by the user. The level of the resource is
> >+ * changed to the target level, if it is not the same as 
> the existing level
> >+ * of the resource. Multiple calls to this function by the 
> same device will
> >+ * replace the previous level requested
> >+ * Returns 0 on success, -EINVAL if the resource name 
> passed in invalid.
> >+ * -ENOMEM if no static pool available or dynamic allocations fails.
> >+ * Else returns a non-zero error value returned by one of 
> the failing
> >+ * shared_resource_ops.
> >+ */
> >+int resource_request(const char *name, struct device *dev,
> >+                                      unsigned long level)
> >+{
> >+      struct shared_resource *resp;
> >+      struct  users_list *user;
> >+      int     found = 0, ret = 0;
> >+      unsigned long flags;
> >+
> >+      spin_lock_irqsave(&res_lock, flags);
> >+      resp = _resource_lookup(name);
> >+      if (!resp) {
> >+              printk(KERN_ERR "resource_request: Invalid 
> resource name\n");
> >+              ret = -EINVAL;
> >+              goto res_unlock;
> >+      }
> >+
> >+      /* Call the resource specific validate function */
> >+      if (resp->ops->validate_level) {
> >+              ret = resp->ops->validate_level(resp, level);
> >+              if (ret)
> >+                      goto res_unlock;
> >+      }
> >+
> >+      list_for_each_entry(user, &resp->users_list, node) {
> >+              if (user->dev == dev) {
> >+                      found = 1;
> >+                      break;
> >+              }
> >+      }
> >+
> >+      if (!found) {
> >+              /* First time user */
> >+              user = get_user();
> >+              if (IS_ERR(user)) {
> >+                      ret = -ENOMEM;
> >+                      goto res_unlock;
> >+              }
> >+              user->dev = dev;
> >+              list_add(&user->node, &resp->users_list);
> >+              resp->no_of_users++;
> >+      }
> >+      user->level = level;
> >+
> >+      /* Recompute and set the current level for the resource */
> >+      ret = update_resource_level(resp);
> >+res_unlock:
> >+      spin_unlock_irqrestore(&res_lock, flags);
> >+      return ret;
> >+}
> >+EXPORT_SYMBOL(resource_request);
> >+
> >+/**
> >+ * resource_release - Release a previously requested level 
> of a resource
> >+ * @name: The name of the resource to be released
> >+ * @dev: Uniquely identifes the caller
> >+ *
> >+ * This function recomputes the target level of the 
> resource after removing
> >+ * the level requested by the user. The level of the resource is
> >+ * changed to the target level, if it is not the same as 
> the existing level
> >+ * of the resource.
> >+ * Returns 0 on success, -EINVAL if the resource name or 
> dev structure
> >+ * is invalid.
> >+ */
> >+int resource_release(const char *name, struct device *dev)
> >+{
> >+      struct shared_resource *resp;
> >+      struct users_list *user;
> >+      int found = 0, ret = 0;
> >+      unsigned long flags;
> >+
> >+      spin_lock_irqsave(&res_lock, flags);
> >+      resp = _resource_lookup(name);
> >+      if (!resp) {
> >+              printk(KERN_ERR "resource_release: Invalid 
> resource name\n");
> >+              ret = -EINVAL;
> >+              goto res_unlock;
> >+      }
> >+
> >+      list_for_each_entry(user, &resp->users_list, node) {
> >+              if (user->dev == dev) {
> >+                      found = 1;
> >+                      break;
> >+              }
> >+      }
> >+
> >+      if (!found) {
> >+              /* No such user exists */
> >+              ret = -EINVAL;
> >+              goto res_unlock;
> >+      }
> >+
> >+      resp->no_of_users--;
> >+      list_del(&user->node);
> >+      free_user(user);
> >+
> >+      /* Recompute and set the current level for the resource */
> >+      ret = update_resource_level(resp);
> >+res_unlock:
> >+      spin_unlock_irqrestore(&res_lock, flags);
> >+      return ret;
> >+}
> >+EXPORT_SYMBOL(resource_release);
> >+
> >+/**
> >+ * resource_get_level - Returns the current level of the resource
> >+ * @name: Name of the resource
> >+ *
> >+ * Returns the current level of the resource if found, else returns
> >+ * -EINVAL if the resource name is invalid.
> >+ */
> >+int resource_get_level(const char *name)
> >+{
> >+      struct shared_resource *resp;
> >+      u32 ret;
> >+      unsigned long flags;
> >+
> >+      spin_lock_irqsave(&res_lock, flags);
> >+      resp = _resource_lookup(name);
> >+      if (!resp) {
> >+              printk(KERN_ERR "resource_release: Invalid 
> resource name\n");
> >+              spin_unlock_irqrestore(&res_lock, flags);
> >+              return -EINVAL;
> >+      }
> >+      ret = resp->curr_level;
> >+      spin_unlock_irqrestore(&res_lock, flags);
> >+      return ret;
> >+}
> >+EXPORT_SYMBOL(resource_get_level);
> >Index: linux-omap-2.6/arch/arm/plat-omap/include/mach/resource.h
> >================================================================
> >===
> >--- /dev/null  1970-01-01 00:00:00.000000000 +0000
> >+++ 
> linux-omap-2.6/arch/arm/plat-omap/include/mach/resource.h  2008-10-16
> >18:01:57.000000000 +0530
> >@@ -0,0 +1,79 @@
> >+/*
> >+ * linux/include/asm-arm/arch-omap/resource.h
> >+ * Structure definitions for Shared resource Framework
> >+ *
> >+ * Copyright (C) 2007-2008 Texas Instruments, Inc.
> >+ * Written by Rajendra Nayak <rnayak@ti.com>
> >+ *
> >+ * This program is free software; you can redistribute it 
> and/or modify
> >+ * it under the terms of the GNU General Public License version 2 as
> >+ * published by the Free Software Foundation.
> >+ *
> >+ * THIS PACKAGE IS PROVIDED ``AS IS'' AND WITHOUT ANY EXPRESS OR
> >+ * IMPLIED WARRANTIES, INCLUDING, WITHOUT LIMITATION, THE IMPLIED
> >+ * WARRANTIES OF MERCHANTIBILITY AND FITNESS FOR A PARTICULAR
> >PURPOSE.
> >+ *
> >+ * History:
> >+ *
> >+ */
> >+
> >+#ifndef __ARCH_ARM_OMAP_RESOURCE_H
> >+#define __ARCH_ARM_OMAP_RESOURCE_H
> >+
> >+#include <linux/list.h>
> >+#include <linux/mutex.h>
> >+#include <linux/device.h>
> >+#include <mach/cpu.h>
> >+
> >+#define RES_DEFAULTLEVEL      0x0
> >+
> >+struct shared_resource_ops; /* forward declaration */
> >+
> >+/* Used to model a Shared Multilevel Resource */
> >+struct shared_resource {
> >+      /* Resource name */
> >+      const char *name;
> >+      /* Used to represent the OMAP chip types containing 
> this res */
> >+      const struct omap_chip_id omap_chip;
> >+      /* Total no of users at any point of this resource */
> >+      u8 no_of_users;
> >+      /* Current level of this resource */
> >+      u32 curr_level;
> >+      /* Used to store any resource specific data */
> >+      void  *resource_data;
> >+      /* List of all the current users for this resource */
> >+      struct list_head users_list;
> >+      /* Shared resource operations */
> >+      struct shared_resource_ops *ops;
> >+      struct list_head node;
> >+};
> >+
> >+struct shared_resource_ops {
> >+      /* Init function for the resource */
> >+      void (*init)(struct shared_resource *res);
> >+      /* Function to change the level of the resource */
> >+      int (*change_level)(struct shared_resource *res, u32 
> target_level);
> >+      /* Function to validate the requested level of the resource */
> >+      int (*validate_level)(struct shared_resource *res, 
> u32 target_level);
> >+};
> >+
> >+/* Used to represent a user of a shared resource */
> >+struct users_list {
> >+      /* Device pointer used to uniquely identify the user */
> >+      struct device *dev;
> >+      /* Current level as requested for the resource by the user */
> >+      u32 level;
> >+      struct list_head node;
> >+      u8 usage;
> >+};
> >+
> >+/* Shared resource Framework API's */
> >+void resource_init(struct shared_resource **resources);
> >+int resource_register(struct shared_resource *res);
> >+int resource_unregister(struct shared_resource *res);
> >+int resource_request(const char *name, struct device *dev,
> >+                                               unsigned long level);
> >+int resource_release(const char *name, struct device *dev);
> >+int resource_get_level(const char *name);
> >+
> >+#endif /* __ARCH_ARM_OMAP_RESOURCE_H */
> >
> >--
> >To unsubscribe from this list: send the line "unsubscribe 
> linux-omap" in
> >the body of a message to majordomo@vger.kernel.org
> >More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
>