Re: [PATCH] Staging: tidspbridge: Use hashtable implementation
This is not really an issue with this patch, it's something from the
original code so it doesn't affect merging the patch.
On Sun, Jan 05, 2014 at 08:55:47PM +0200, Ivaylo Dimitrov wrote:
> >Again, reverse the IS_ERR() check and return directly. Use the struct
> >pointer instead of the address of the first member.
> >
> > sym = gh_find(lib->sym_tab, (char *)name);
> > if (IS_ERR(sym))
> > return NULL;
> >
> > return (struct dbll_symbol *)sym;
> >
> >That way is easier to read. gh_find() should accept const pointers btw,
> >we shouldn't have to cast away the const in each of the callers.
> I don't think it is a good idea to return the structdbll_symbol*
> here - the function return type is structdynload_symbol* not
> structdbll_symbol*.
Oops.
> And that will break if we exchange value and name members of
> structdbll_symbol.
It will break anyway if we do that. It's one of those things where you
can't worry too much about what crazy people might try to do later and
then no one reviews the code and no one tests it. If we start doing
that sort of thing we are screwed already so it's not worth worrying
about.
I feel like the types in this driver are not beautiful.
The names are not clear. Take "struct dbll_tar_obj" for example. I'm
not clear what "dbll_" means. I think the "d" stands for dynmic. What
information does the "_obj" add? It would be better to call it
"struct dbll_target".
"dbll_alloc" is a verb instead of a noun so it sounds like a function
not a struct.
But mostly there is too much nonsense casting throughout.
dbll_find_symbol() takes a struct dynamic_loader_sym pointer but we
immediatly cast it to struct ldr_symbol. The struct ldr_symbol is
defined as:
struct ldr_symbol {
struct dynamic_loader_sym dl_symbol;
struct dbll_library_obj *lib;
};
The reason it does this is because it was originally C++ code and it
was ported to C. I think we could move the "lib pointer to the end
of the "dynamic_loader_sym" struct. We could make it a "void *data".
That would remove a source of casting.
There is a lot of this kind of stuff going on:
struct dbll_library_obj *zl_lib = (struct dbll_library_obj *)lib;
"lib" is already a dbll_library_obj struct. And "lib" is a better name,
what does zl_lib mean? I think it's Hungarian notation which we don't
like. Sometimes it uses pzl_ and I think the "p" means pointer. Ugh.
Inside of functions then we don't need to prefix local variables. It's
only for global variables where you run into naming clashes.
Anyway... This driver needs a lot of fixing where data types are
concerned.
regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Staging: tidspbridge: Use hashtable implementation
Minor nits. Nothing major.
On Wed, Dec 25, 2013 at 07:29:52PM +0200, Ivaylo DImitrov wrote:
> From: Ivaylo Dimitrov
>
> Use upstream hashtable implementation instead of generic code
>
> Signed-off-by: Ivaylo Dimitrov
Send from the same email you are using to Sign so we can at least
verify that small thing.
> ---
> drivers/staging/tidspbridge/gen/gh.c | 141
> +++-
> drivers/staging/tidspbridge/include/dspbridge/gh.h |6 +-
> drivers/staging/tidspbridge/pmgr/dbll.c| 47 ---
> 3 files changed, 78 insertions(+), 116 deletions(-)
>
> diff --git a/drivers/staging/tidspbridge/gen/gh.c
> b/drivers/staging/tidspbridge/gen/gh.c
> index 25eaef7..41a0a4f 100644
> --- a/drivers/staging/tidspbridge/gen/gh.c
> +++ b/drivers/staging/tidspbridge/gen/gh.c
> @@ -14,56 +14,46 @@
> * WARRANTIES OF MERCHANTIBILITY AND FITNESS FOR A PARTICULAR PURPOSE.
> */
>
> -#include
> +#include
> +#include
> +#include
>
> -#include
> -#include
> -
> -struct element {
> - struct element *next;
> +struct gh_node {
> + struct hlist_node hl;
> u8 data[1];
Use a zero size array here so that you can remove the " - 1" from
"sizeof(struct gh_node) - 1 + hash_tab->val_size". (Fix in later
patches).
> };
>
> +#define GH_HASH_ORDER 8
> +
> struct gh_t_hash_tab {
> - u16 max_bucket;
> - u16 val_size;
> - struct element **buckets;
> - u16(*hash) (void *, u16);
> - bool(*match) (void *, void *);
> - void (*delete) (void *);
> + u32 val_size;
> + DECLARE_HASHTABLE(hash_table, GH_HASH_ORDER);
> + u32 (*hash)(void *);
> + bool (*match)(void *, void *);
> + void (*delete)(void *);
> };
>
> -static void noop(void *p);
> -
> /*
> * gh_create
> */
>
> -struct gh_t_hash_tab *gh_create(u16 max_bucket, u16 val_size,
> - u16(*hash) (void *, u16), bool(*match) (void *,
> - void *),
> - void (*delete) (void *))
> +struct gh_t_hash_tab *gh_create(u32 val_size,u32 (*hash)(void *),
> + bool (*match)(void *, void *),
> + void (*delete)(void *))
> {
> struct gh_t_hash_tab *hash_tab;
> - u16 i;
> +
> hash_tab = kzalloc(sizeof(struct gh_t_hash_tab), GFP_KERNEL);
> - if (hash_tab == NULL)
> - return NULL;
> - hash_tab->max_bucket = max_bucket;
> +
Don't put a blank line here between the call and the error handling.
> + if (!hash_tab)
> + return ERR_PTR(-ENOMEM);
> +
> + hash_init(hash_tab->hash_table);
> +
> hash_tab->val_size = val_size;
> hash_tab->hash = hash;
> hash_tab->match = match;
> - hash_tab->delete = delete == NULL ? noop : delete;
> -
> - hash_tab->buckets =
> - kzalloc(sizeof(struct element *) * max_bucket, GFP_KERNEL);
> - if (hash_tab->buckets == NULL) {
> - gh_delete(hash_tab);
> - return NULL;
> - }
> -
> - for (i = 0; i < max_bucket; i++)
> - hash_tab->buckets[i] = NULL;
> + hash_tab->delete = delete;
>
> return hash_tab;
> }
> @@ -73,21 +63,16 @@ struct gh_t_hash_tab *gh_create(u16 max_bucket, u16
> val_size,
> */
> void gh_delete(struct gh_t_hash_tab *hash_tab)
> {
> - struct element *elem, *next;
> - u16 i;
> -
> - if (hash_tab != NULL) {
> - if (hash_tab->buckets != NULL) {
> - for (i = 0; i < hash_tab->max_bucket; i++) {
> - for (elem = hash_tab->buckets[i]; elem != NULL;
> - elem = next) {
> - next = elem->next;
> - (*hash_tab->delete) (elem->data);
> - kfree(elem);
> - }
> - }
> -
> - kfree(hash_tab->buckets);
> + struct gh_node *n;
> + struct hlist_node *tmp;
> + u32 i;
> +
> + if (hash_tab) {
> + hash_for_each_safe(hash_tab->hash_table, i, tmp, n, hl) {
> + hash_del(&n->hl);
> + if(hash_tab->delete)
Use checkpatch.pl.
> + hash_tab->delete(n->data);
> + kfree(n);
> }
>
> kfree(hash_tab);
> @@ -100,16 +85,14 @@ void gh_delete(struct gh_t_hash_tab *hash_tab)
>
> void *gh_find(struct gh_t_hash_tab *hash_tab, void *key)
> {
> - struct element *elem;
> -
> - elem = hash_tab->buckets[(*hash_tab->hash) (key, hash_tab->max_bucket)];
> + struct gh_node *n;
> + u32 key_hash = hash_tab->hash(key);
>
> - for (; elem; elem = elem->next) {
> - if ((*hash_tab->match) (key, elem->data))
> - return elem->data;
> - }
> + hash_for_each_possible(hash_tab->hash_table, n, hl, key
Re: [PATCH] Staging: tidspbridge: Use hashtable implementation
Hi!
> From: Ivaylo Dimitrov
>
> Use upstream hashtable implementation instead of generic code
>
> Signed-off-by: Ivaylo Dimitrov
> @@ -991,8 +989,11 @@ static struct dynload_symbol
> *dbll_add_to_symbol_table(struct dynamic_loader_sym
> sym_ptr =
> (struct dbll_symbol *)gh_insert(lib->sym_tab, (void *)name,
> (void *)&symbol);
> - if (sym_ptr == NULL)
> + if (IS_ERR(sym_ptr))
> + {
You want this { to go to the previous line. Otherwise looks good,
Reviewed-by: Pavel Machek
Thanks,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
