Patchwork [RFC] use linux kernel list datastruct for registered masters list

login
register
about
Submitter Antony Pavlov
Date 2015-06-14 23:05:39
Message ID <1434323139-2009-1-git-send-email-antonynpavlov@gmail.com>
Download mbox | patch
Permalink /patch/4301/
State New
Headers show

Comments

Antony Pavlov - 2015-06-14 23:05:39
Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
---
 cli_classic.c |  13 +++---
 flashrom.c    |   1 -
 list.h        | 130 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 programmer.c  |  27 ++++++------
 programmer.h  |   7 ++--
 5 files changed, 153 insertions(+), 25 deletions(-)
Antony Pavlov - 2015-06-17 07:46:54
On Mon, 15 Jun 2015 02:05:39 +0300
Antony Pavlov <antonynpavlov@gmail.com> wrote:

Stefan!

This patch make it possible to eliminate MASTERS_MAX limit.

Could you please comment on this patch?

> Signed-off-by: Antony Pavlov <antonynpavlov@gmail.com>
> ---
>  cli_classic.c |  13 +++---
>  flashrom.c    |   1 -
>  list.h        | 130 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  programmer.c  |  27 ++++++------
>  programmer.h  |   7 ++--
>  5 files changed, 153 insertions(+), 25 deletions(-)
> 
> diff --git a/cli_classic.c b/cli_classic.c
> index a2c2014..06ae454 100644
> --- a/cli_classic.c
> +++ b/cli_classic.c
> @@ -96,7 +96,7 @@ int main(int argc, char *argv[])
>  	struct flashctx flashes[8] = {{0}};
>  	struct flashctx *fill_flash;
>  	const char *name;
> -	int namelen, opt, i, j;
> +	int namelen, opt, i;
>  	int startchip = -1, chipcount = 0, option_index = 0, force = 0;
>  #if CONFIG_PRINT_WIKI == 1
>  	int list_supported_wiki = 0;
> @@ -134,6 +134,7 @@ int main(int argc, char *argv[])
>  #endif /* !STANDALONE */
>  	char *tempstr = NULL;
>  	char *pparam = NULL;
> +	struct registered_master *master;
>  
>  	print_version();
>  	print_banner();
> @@ -425,10 +426,10 @@ int main(int argc, char *argv[])
>  	msg_pdbg("The following protocols are supported: %s.\n", tempstr);
>  	free(tempstr);
>  
> -	for (j = 0; j < registered_master_count; j++) {
> +	list_for_each_entry(master, &registered_masters, node) {
>  		startchip = 0;
>  		while (chipcount < ARRAY_SIZE(flashes)) {
> -			startchip = probe_flash(&registered_masters[j], startchip, &flashes[chipcount], 0);
> +			startchip = probe_flash(master, startchip, &flashes[chipcount], 0);
>  			if (startchip == -1)
>  				break;
>  			chipcount++;
> @@ -455,8 +456,7 @@ int main(int argc, char *argv[])
>  			int compatible_masters = 0;
>  			msg_cinfo("Force read (-f -r -c) requested, pretending the chip is there:\n");
>  			/* This loop just counts compatible controllers. */
> -			for (j = 0; j < registered_master_count; j++) {
> -				mst = &registered_masters[j];
> +			list_for_each_entry(mst, &registered_masters, node) {
>  				/* chip is still set from the chip_to_probe earlier in this function. */
>  				if (mst->buses_supported & chip->bustype)
>  					compatible_masters++;
> @@ -469,8 +469,7 @@ int main(int argc, char *argv[])
>  			if (compatible_masters > 1)
>  				msg_cinfo("More than one compatible controller found for the requested flash "
>  					  "chip, using the first one.\n");
> -			for (j = 0; j < registered_master_count; j++) {
> -				mst = &registered_masters[j];
> +			list_for_each_entry(mst, &registered_masters, node) {
>  				startchip = probe_flash(mst, 0, &flashes[0], 1);
>  				if (startchip != -1)
>  					break;
> diff --git a/flashrom.c b/flashrom.c
> index a389cb2..bd770c2 100644
> --- a/flashrom.c
> +++ b/flashrom.c
> @@ -494,7 +494,6 @@ int programmer_shutdown(void)
>  	}
>  
>  	programmer_param = NULL;
> -	registered_master_count = 0;
>  
>  	return ret;
>  }
> diff --git a/list.h b/list.h
> new file mode 100644
> index 0000000..7567742
> --- /dev/null
> +++ b/list.h
> @@ -0,0 +1,130 @@
> +#ifndef LIST_H
> +#define LIST_H
> +
> +/*
> + * Copied from include/linux/...
> + */
> +
> +#undef offsetof
> +#define offsetof(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER)
> +
> +/**
> + * container_of - cast a member of a structure out to the containing structure
> + * @ptr:        the pointer to the member.
> + * @type:       the type of the container struct this is embedded in.
> + * @member:     the name of the member within the struct.
> + *
> + */
> +#define container_of(ptr, type, member) ({                      \
> +	const typeof( ((type *)0)->member ) *__mptr = (ptr);    \
> +	(type *)( (char *)__mptr - offsetof(type,member) );})
> +
> +
> +struct list_head {
> +	struct list_head *next, *prev;
> +};
> +
> +#define LIST_HEAD_INIT(name) { &(name), &(name) }
> +
> +#define LIST_HEAD(name) \
> +	struct list_head name = LIST_HEAD_INIT(name)
> +
> +/**
> + * list_entry - get the struct for this entry
> + * @ptr:	the &struct list_head pointer.
> + * @type:	the type of the struct this is embedded in.
> + * @member:	the name of the list_head within the struct.
> + */
> +#define list_entry(ptr, type, member) \
> +	container_of(ptr, type, member)
> +
> +/**
> + * list_for_each_entry	-	iterate over list of given type
> + * @pos:	the type * to use as a loop cursor.
> + * @head:	the head for your list.
> + * @member:	the name of the list_head within the struct.
> + */
> +#define list_for_each_entry(pos, head, member)				\
> +	for (pos = list_entry((head)->next, typeof(*pos), member);	\
> +	     &pos->member != (head);	\
> +	     pos = list_entry(pos->member.next, typeof(*pos), member))
> +
> +/**
> + * list_for_each_entry_safe - iterate over list of given type safe against removal of list entry
> + * @pos:	the type * to use as a loop cursor.
> + * @n:		another type * to use as temporary storage
> + * @head:	the head for your list.
> + * @member:	the name of the list_head within the struct.
> + */
> +#define list_for_each_entry_safe(pos, n, head, member)			\
> +	for (pos = list_entry((head)->next, typeof(*pos), member),	\
> +		n = list_entry(pos->member.next, typeof(*pos), member);	\
> +	     &pos->member != (head);					\
> +	     pos = n, n = list_entry(n->member.next, typeof(*n), member))
> +
> +/**
> + * list_empty - tests whether a list is empty
> + * @head: the list to test.
> + */
> +static inline int list_empty(const struct list_head *head)
> +{
> +	return head->next == head;
> +}
> +
> +/*
> + * Insert a new entry between two known consecutive entries.
> + *
> + * This is only for internal list manipulation where we know
> + * the prev/next entries already!
> + */
> +static inline void __list_add(struct list_head *_new,
> +			      struct list_head *prev,
> +			      struct list_head *next)
> +{
> +	next->prev = _new;
> +	_new->next = next;
> +	_new->prev = prev;
> +	prev->next = _new;
> +}
> +
> +/**
> + * list_add_tail - add a new entry
> + * @new: new entry to be added
> + * @head: list head to add it before
> + *
> + * Insert a new entry before the specified head.
> + * This is useful for implementing queues.
> + */
> +static inline void list_add_tail(struct list_head *_new, struct list_head *head)
> +{
> +	__list_add(_new, head->prev, head);
> +}
> +
> +/*
> + * Delete a list entry by making the prev/next entries
> + * point to each other.
> + *
> + * This is only for internal list manipulation where we know
> + * the prev/next entries already!
> + */
> +static inline void __list_del(struct list_head *prev, struct list_head *next)
> +{
> +	next->prev = prev;
> +	prev->next = next;
> +}
> +
> +#define LIST_POISON1  ((void *) 0x00100100)
> +#define LIST_POISON2  ((void *) 0x00200200)
> +/**
> + * list_del - deletes entry from list.
> + * @entry: the element to delete from the list.
> + * Note: list_empty() on entry does not return true after this, the entry is
> + * in an undefined state.
> + */
> +static inline void list_del(struct list_head *entry)
> +{
> +	__list_del(entry->prev, entry->next);
> +	entry->next = (struct list_head*)LIST_POISON1;
> +	entry->prev = (struct list_head*)LIST_POISON2;
> +}
> +#endif
> diff --git a/programmer.c b/programmer.c
> index 1b27c3c..2a5df92 100644
> --- a/programmer.c
> +++ b/programmer.c
> @@ -18,6 +18,8 @@
>   * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
>   */
>  
> +#include <malloc.h>
> +#include <string.h>
>  #include "flash.h"
>  #include "programmer.h"
>  
> @@ -114,31 +116,28 @@ int register_par_master(const struct par_master *mst,
>  }
>  
>  /* The limit of 4 is totally arbitrary. */
> -#define MASTERS_MAX 4
> -struct registered_master registered_masters[MASTERS_MAX];
> -int registered_master_count = 0;
> +LIST_HEAD(registered_masters);
>  
>  /* This function copies the struct registered_master parameter. */
> -int register_master(const struct registered_master *mst)
> +int register_master(struct registered_master *mst)
>  {
> -	if (registered_master_count >= MASTERS_MAX) {
> -		msg_perr("Tried to register more than %i master "
> -			 "interfaces.\n", MASTERS_MAX);
> -		return ERROR_FLASHROM_LIMIT;
> -	}
> -	registered_masters[registered_master_count] = *mst;
> -	registered_master_count++;
> +	struct registered_master *t;
> +
> +	t = malloc(sizeof(struct registered_master));
> +	memcpy(t, mst, sizeof(struct registered_master));
> +	list_add_tail(&t->node, &registered_masters);
>  
>  	return 0;
>  }
>  
>  enum chipbustype get_buses_supported(void)
>  {
> -	int i;
> +	struct registered_master *mst;
>  	enum chipbustype ret = BUS_NONE;
>  
> -	for (i = 0; i < registered_master_count; i++)
> -		ret |= registered_masters[i].buses_supported;
> +	list_for_each_entry(mst, &registered_masters, node) {
> +		ret |= mst->buses_supported;
> +	}
>  
>  	return ret;
>  }
> diff --git a/programmer.h b/programmer.h
> index 913522b..54d5634 100644
> --- a/programmer.h
> +++ b/programmer.h
> @@ -25,6 +25,7 @@
>  #define __PROGRAMMER_H__ 1
>  
>  #include "flash.h"	/* for chipaddr and flashctx */
> +#include "list.h"
>  
>  enum programmer {
>  #if CONFIG_INTERNAL == 1
> @@ -691,6 +692,7 @@ struct par_master {
>  };
>  int register_par_master(const struct par_master *mst, const enum chipbustype buses);
>  struct registered_master {
> +	struct list_head node;
>  	enum chipbustype buses_supported;
>  	union {
>  		struct par_master par;
> @@ -698,9 +700,8 @@ struct registered_master {
>  		struct opaque_master opaque;
>  	};
>  };
> -extern struct registered_master registered_masters[];
> -extern int registered_master_count;
> -int register_master(const struct registered_master *mst);
> +extern struct list_head registered_masters;
> +int register_master(struct registered_master *mst);
>  
>  /* serprog.c */
>  #if CONFIG_SERPROG == 1
> -- 
> 2.1.4
>
Stefan Tauner - 2015-06-17 07:50:38
On Wed, 17 Jun 2015 10:46:54 +0300
Antony Pavlov <antonynpavlov@gmail.com> wrote:

> Stefan!
> 
> This patch make it possible to eliminate MASTERS_MAX limit.
> 
> Could you please comment on this patch?

Hi,

we need to integrate a lot of other patches before starting with such
fundamental changes like adding generic containers/collection. If we
would do it the other way round we would have to rewrite/fix a lot of
*existing* patches.

Patch

diff --git a/cli_classic.c b/cli_classic.c
index a2c2014..06ae454 100644
--- a/cli_classic.c
+++ b/cli_classic.c
@@ -96,7 +96,7 @@  int main(int argc, char *argv[])
 	struct flashctx flashes[8] = {{0}};
 	struct flashctx *fill_flash;
 	const char *name;
-	int namelen, opt, i, j;
+	int namelen, opt, i;
 	int startchip = -1, chipcount = 0, option_index = 0, force = 0;
 #if CONFIG_PRINT_WIKI == 1
 	int list_supported_wiki = 0;
@@ -134,6 +134,7 @@  int main(int argc, char *argv[])
 #endif /* !STANDALONE */
 	char *tempstr = NULL;
 	char *pparam = NULL;
+	struct registered_master *master;
 
 	print_version();
 	print_banner();
@@ -425,10 +426,10 @@  int main(int argc, char *argv[])
 	msg_pdbg("The following protocols are supported: %s.\n", tempstr);
 	free(tempstr);
 
-	for (j = 0; j < registered_master_count; j++) {
+	list_for_each_entry(master, &registered_masters, node) {
 		startchip = 0;
 		while (chipcount < ARRAY_SIZE(flashes)) {
-			startchip = probe_flash(&registered_masters[j], startchip, &flashes[chipcount], 0);
+			startchip = probe_flash(master, startchip, &flashes[chipcount], 0);
 			if (startchip == -1)
 				break;
 			chipcount++;
@@ -455,8 +456,7 @@  int main(int argc, char *argv[])
 			int compatible_masters = 0;
 			msg_cinfo("Force read (-f -r -c) requested, pretending the chip is there:\n");
 			/* This loop just counts compatible controllers. */
-			for (j = 0; j < registered_master_count; j++) {
-				mst = &registered_masters[j];
+			list_for_each_entry(mst, &registered_masters, node) {
 				/* chip is still set from the chip_to_probe earlier in this function. */
 				if (mst->buses_supported & chip->bustype)
 					compatible_masters++;
@@ -469,8 +469,7 @@  int main(int argc, char *argv[])
 			if (compatible_masters > 1)
 				msg_cinfo("More than one compatible controller found for the requested flash "
 					  "chip, using the first one.\n");
-			for (j = 0; j < registered_master_count; j++) {
-				mst = &registered_masters[j];
+			list_for_each_entry(mst, &registered_masters, node) {
 				startchip = probe_flash(mst, 0, &flashes[0], 1);
 				if (startchip != -1)
 					break;
diff --git a/flashrom.c b/flashrom.c
index a389cb2..bd770c2 100644
--- a/flashrom.c
+++ b/flashrom.c
@@ -494,7 +494,6 @@  int programmer_shutdown(void)
 	}
 
 	programmer_param = NULL;
-	registered_master_count = 0;
 
 	return ret;
 }
diff --git a/list.h b/list.h
new file mode 100644
index 0000000..7567742
--- /dev/null
+++ b/list.h
@@ -0,0 +1,130 @@ 
+#ifndef LIST_H
+#define LIST_H
+
+/*
+ * Copied from include/linux/...
+ */
+
+#undef offsetof
+#define offsetof(TYPE, MEMBER) ((size_t) &((TYPE *)0)->MEMBER)
+
+/**
+ * container_of - cast a member of a structure out to the containing structure
+ * @ptr:        the pointer to the member.
+ * @type:       the type of the container struct this is embedded in.
+ * @member:     the name of the member within the struct.
+ *
+ */
+#define container_of(ptr, type, member) ({                      \
+	const typeof( ((type *)0)->member ) *__mptr = (ptr);    \
+	(type *)( (char *)__mptr - offsetof(type,member) );})
+
+
+struct list_head {
+	struct list_head *next, *prev;
+};
+
+#define LIST_HEAD_INIT(name) { &(name), &(name) }
+
+#define LIST_HEAD(name) \
+	struct list_head name = LIST_HEAD_INIT(name)
+
+/**
+ * list_entry - get the struct for this entry
+ * @ptr:	the &struct list_head pointer.
+ * @type:	the type of the struct this is embedded in.
+ * @member:	the name of the list_head within the struct.
+ */
+#define list_entry(ptr, type, member) \
+	container_of(ptr, type, member)
+
+/**
+ * list_for_each_entry	-	iterate over list of given type
+ * @pos:	the type * to use as a loop cursor.
+ * @head:	the head for your list.
+ * @member:	the name of the list_head within the struct.
+ */
+#define list_for_each_entry(pos, head, member)				\
+	for (pos = list_entry((head)->next, typeof(*pos), member);	\
+	     &pos->member != (head);	\
+	     pos = list_entry(pos->member.next, typeof(*pos), member))
+
+/**
+ * list_for_each_entry_safe - iterate over list of given type safe against removal of list entry
+ * @pos:	the type * to use as a loop cursor.
+ * @n:		another type * to use as temporary storage
+ * @head:	the head for your list.
+ * @member:	the name of the list_head within the struct.
+ */
+#define list_for_each_entry_safe(pos, n, head, member)			\
+	for (pos = list_entry((head)->next, typeof(*pos), member),	\
+		n = list_entry(pos->member.next, typeof(*pos), member);	\
+	     &pos->member != (head);					\
+	     pos = n, n = list_entry(n->member.next, typeof(*n), member))
+
+/**
+ * list_empty - tests whether a list is empty
+ * @head: the list to test.
+ */
+static inline int list_empty(const struct list_head *head)
+{
+	return head->next == head;
+}
+
+/*
+ * Insert a new entry between two known consecutive entries.
+ *
+ * This is only for internal list manipulation where we know
+ * the prev/next entries already!
+ */
+static inline void __list_add(struct list_head *_new,
+			      struct list_head *prev,
+			      struct list_head *next)
+{
+	next->prev = _new;
+	_new->next = next;
+	_new->prev = prev;
+	prev->next = _new;
+}
+
+/**
+ * list_add_tail - add a new entry
+ * @new: new entry to be added
+ * @head: list head to add it before
+ *
+ * Insert a new entry before the specified head.
+ * This is useful for implementing queues.
+ */
+static inline void list_add_tail(struct list_head *_new, struct list_head *head)
+{
+	__list_add(_new, head->prev, head);
+}
+
+/*
+ * Delete a list entry by making the prev/next entries
+ * point to each other.
+ *
+ * This is only for internal list manipulation where we know
+ * the prev/next entries already!
+ */
+static inline void __list_del(struct list_head *prev, struct list_head *next)
+{
+	next->prev = prev;
+	prev->next = next;
+}
+
+#define LIST_POISON1  ((void *) 0x00100100)
+#define LIST_POISON2  ((void *) 0x00200200)
+/**
+ * list_del - deletes entry from list.
+ * @entry: the element to delete from the list.
+ * Note: list_empty() on entry does not return true after this, the entry is
+ * in an undefined state.
+ */
+static inline void list_del(struct list_head *entry)
+{
+	__list_del(entry->prev, entry->next);
+	entry->next = (struct list_head*)LIST_POISON1;
+	entry->prev = (struct list_head*)LIST_POISON2;
+}
+#endif
diff --git a/programmer.c b/programmer.c
index 1b27c3c..2a5df92 100644
--- a/programmer.c
+++ b/programmer.c
@@ -18,6 +18,8 @@ 
  * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
  */
 
+#include <malloc.h>
+#include <string.h>
 #include "flash.h"
 #include "programmer.h"
 
@@ -114,31 +116,28 @@  int register_par_master(const struct par_master *mst,
 }
 
 /* The limit of 4 is totally arbitrary. */
-#define MASTERS_MAX 4
-struct registered_master registered_masters[MASTERS_MAX];
-int registered_master_count = 0;
+LIST_HEAD(registered_masters);
 
 /* This function copies the struct registered_master parameter. */
-int register_master(const struct registered_master *mst)
+int register_master(struct registered_master *mst)
 {
-	if (registered_master_count >= MASTERS_MAX) {
-		msg_perr("Tried to register more than %i master "
-			 "interfaces.\n", MASTERS_MAX);
-		return ERROR_FLASHROM_LIMIT;
-	}
-	registered_masters[registered_master_count] = *mst;
-	registered_master_count++;
+	struct registered_master *t;
+
+	t = malloc(sizeof(struct registered_master));
+	memcpy(t, mst, sizeof(struct registered_master));
+	list_add_tail(&t->node, &registered_masters);
 
 	return 0;
 }
 
 enum chipbustype get_buses_supported(void)
 {
-	int i;
+	struct registered_master *mst;
 	enum chipbustype ret = BUS_NONE;
 
-	for (i = 0; i < registered_master_count; i++)
-		ret |= registered_masters[i].buses_supported;
+	list_for_each_entry(mst, &registered_masters, node) {
+		ret |= mst->buses_supported;
+	}
 
 	return ret;
 }
diff --git a/programmer.h b/programmer.h
index 913522b..54d5634 100644
--- a/programmer.h
+++ b/programmer.h
@@ -25,6 +25,7 @@ 
 #define __PROGRAMMER_H__ 1
 
 #include "flash.h"	/* for chipaddr and flashctx */
+#include "list.h"
 
 enum programmer {
 #if CONFIG_INTERNAL == 1
@@ -691,6 +692,7 @@  struct par_master {
 };
 int register_par_master(const struct par_master *mst, const enum chipbustype buses);
 struct registered_master {
+	struct list_head node;
 	enum chipbustype buses_supported;
 	union {
 		struct par_master par;
@@ -698,9 +700,8 @@  struct registered_master {
 		struct opaque_master opaque;
 	};
 };
-extern struct registered_master registered_masters[];
-extern int registered_master_count;
-int register_master(const struct registered_master *mst);
+extern struct list_head registered_masters;
+int register_master(struct registered_master *mst);
 
 /* serprog.c */
 #if CONFIG_SERPROG == 1