diff mbox series

[U-Boot,2/5] common: add blkcache init

Message ID 20191123224752.384355-1-angelo.dureghello@timesys.com
State Changes Requested
Delegated to: Tom Rini
Headers show
Series None | expand

Commit Message

Angelo Dureghello Nov. 23, 2019, 10:47 p.m. UTC 
From: Angelo Durgehello <angelo.dureghello@timesys.com>

On m68k, block_cache list is relocated, but next and prev list
pointers are not adjusted to the relocated struct list_head address,
so the first iteration over the block_cache list hangs.

This patch initializes the block_cache list after relocation.

Signed-off-by: Angelo Durgehello <angelo.dureghello@timesys.com>
---
 common/board_r.c         | 12 ++++++++++++
 drivers/block/blkcache.c |  7 ++++++-
 include/blk.h            |  6 ++++++
 3 files changed, 24 insertions(+), 1 deletion(-)

Comments

Eric Nelson Nov. 24, 2019, 4 p.m. UTC | #1 
Hi Angelo,

On 11/23/19 3:47 PM, Angelo Dureghello wrote:
> From: Angelo Durgehello <angelo.dureghello@timesys.com>
> 
> On m68k, block_cache list is relocated, but next and prev list
> pointers are not adjusted to the relocated struct list_head address,
> so the first iteration over the block_cache list hangs.
> 
> This patch initializes the block_cache list after relocation.
> 
> Signed-off-by: Angelo Durgehello <angelo.dureghello@timesys.com>
> ---
>   common/board_r.c         | 12 ++++++++++++
>   drivers/block/blkcache.c |  7 ++++++-
>   include/blk.h            |  6 ++++++
>   3 files changed, 24 insertions(+), 1 deletion(-)
> 
> diff --git a/common/board_r.c b/common/board_r.c
> index 65720849cd..13e70a5ffb 100644
> --- a/common/board_r.c
> +++ b/common/board_r.c
> @@ -628,6 +628,15 @@ static int initr_bedbug(void)
>   }
>   #endif
>   
> +#ifdef CONFIG_BLOCK_CACHE
> +static int initr_blkcache(void)
> +{
> +	blkcache_init();
> +
> +	return 0;
> +}
> +#endif
> +

Why the extra level of indirection?

>   static int run_main_loop(void)
>   {
>   #ifdef CONFIG_SANDBOX
> @@ -832,6 +841,9 @@ static init_fnc_t init_sequence_r[] = {
>   #endif
>   #if defined(CONFIG_PRAM)
>   	initr_mem,
> +#endif
> +#ifdef CONFIG_BLOCK_CACHE

It seems you could call blkcache_init from here directly:

> +	initr_blkcache,
>   #endif
>   	run_main_loop,
>   };
> diff --git a/drivers/block/blkcache.c b/drivers/block/blkcache.c
> index 1fa64989d3..bf0fa1ea6f 100644
> --- a/drivers/block/blkcache.c
> +++ b/drivers/block/blkcache.c
> @@ -21,13 +21,18 @@ struct block_cache_node {
>   	char *cache;
>   };
>   
> -static LIST_HEAD(block_cache);
> +static struct list_head block_cache;
>   
>   static struct block_cache_stats _stats = {
>   	.max_blocks_per_entry = 8,
>   	.max_entries = 32
>   };
>   
> +void blkcache_init(void)
> +{
> +	INIT_LIST_HEAD(&block_cache);
> +}
> +
>   static struct block_cache_node *cache_find(int iftype, int devnum,
>   					   lbaint_t start, lbaint_t blkcnt,
>   					   unsigned long blksz)
> diff --git a/include/blk.h b/include/blk.h
> index d0c033aece..7070fd6af3 100644
> --- a/include/blk.h
> +++ b/include/blk.h
> @@ -113,6 +113,12 @@ struct blk_desc {
>   	(PAD_SIZE(size, blk_desc->blksz))
>   
>   #if CONFIG_IS_ENABLED(BLOCK_CACHE)
> +
> +/**
> + * blkcache_init() - initialize the block cache list pointers
> + */
> +void blkcache_init(void);
> +
>   /**
>    * blkcache_read() - attempt to read a set of blocks from cache
>    *
>
Angelo Dureghello Nov. 25, 2019, 9:59 a.m. UTC | #2 
Hi Eric,

On Sun, Nov 24, 2019 at 5:00 PM Eric Nelson <ericnelsonaz@gmail.com> wrote:
>
> Hi Angelo,
>
> On 11/23/19 3:47 PM, Angelo Dureghello wrote:
> > From: Angelo Durgehello <angelo.dureghello@timesys.com>
> >
> > On m68k, block_cache list is relocated, but next and prev list
> > pointers are not adjusted to the relocated struct list_head address,
> > so the first iteration over the block_cache list hangs.
> >
> > This patch initializes the block_cache list after relocation.
> >
> > Signed-off-by: Angelo Durgehello <angelo.dureghello@timesys.com>
> > ---
> >   common/board_r.c         | 12 ++++++++++++
> >   drivers/block/blkcache.c |  7 ++++++-
> >   include/blk.h            |  6 ++++++
> >   3 files changed, 24 insertions(+), 1 deletion(-)
> >
> > diff --git a/common/board_r.c b/common/board_r.c
> > index 65720849cd..13e70a5ffb 100644
> > --- a/common/board_r.c
> > +++ b/common/board_r.c
> > @@ -628,6 +628,15 @@ static int initr_bedbug(void)
> >   }
> >   #endif
> >
> > +#ifdef CONFIG_BLOCK_CACHE
> > +static int initr_blkcache(void)
> > +{
> > +     blkcache_init();
> > +
> > +     return 0;
> > +}
> > +#endif
> > +
>
> Why the extra level of indirection?
>
> >   static int run_main_loop(void)
> >   {
> >   #ifdef CONFIG_SANDBOX
> > @@ -832,6 +841,9 @@ static init_fnc_t init_sequence_r[] = {
> >   #endif
> >   #if defined(CONFIG_PRAM)
> >       initr_mem,
> > +#endif
> > +#ifdef CONFIG_BLOCK_CACHE
>
> It seems you could call blkcache_init from here directly:
>

reason for this is to maintain "initr_" naming convention, used from
quite all the initr_ calls,
as i.e. static int initr_mmc(void) that's doing the same.


>
> > +     initr_blkcache,
> >   #endif
> >       run_main_loop,
> >   };
> > diff --git a/drivers/block/blkcache.c b/drivers/block/blkcache.c
> > index 1fa64989d3..bf0fa1ea6f 100644
> > --- a/drivers/block/blkcache.c
> > +++ b/drivers/block/blkcache.c
> > @@ -21,13 +21,18 @@ struct block_cache_node {
> >       char *cache;
> >   };
> >
> > -static LIST_HEAD(block_cache);
> > +static struct list_head block_cache;
> >
> >   static struct block_cache_stats _stats = {
> >       .max_blocks_per_entry = 8,
> >       .max_entries = 32
> >   };
> >
> > +void blkcache_init(void)
> > +{
> > +     INIT_LIST_HEAD(&block_cache);
> > +}
> > +
> >   static struct block_cache_node *cache_find(int iftype, int devnum,
> >                                          lbaint_t start, lbaint_t blkcnt,
> >                                          unsigned long blksz)
> > diff --git a/include/blk.h b/include/blk.h
> > index d0c033aece..7070fd6af3 100644
> > --- a/include/blk.h
> > +++ b/include/blk.h
> > @@ -113,6 +113,12 @@ struct blk_desc {
> >       (PAD_SIZE(size, blk_desc->blksz))
> >
> >   #if CONFIG_IS_ENABLED(BLOCK_CACHE)
> > +
> > +/**
> > + * blkcache_init() - initialize the block cache list pointers
> > + */
> > +void blkcache_init(void);
> > +
> >   /**
> >    * blkcache_read() - attempt to read a set of blocks from cache
> >    *
> >
>
Regards,
--
Angelo Dureghello
Eric Nelson Nov. 25, 2019, 3:56 p.m. UTC | #3 
Hi Angelo,

On 11/25/19 2:59 AM, Angelo Dureghello wrote:
> Hi Eric,
> 
> On Sun, Nov 24, 2019 at 5:00 PM Eric Nelson <ericnelsonaz@gmail.com> wrote:
>>
>> Hi Angelo,
>>
>> On 11/23/19 3:47 PM, Angelo Dureghello wrote:
>>> From: Angelo Durgehello <angelo.dureghello@timesys.com>
>>>
>>> On m68k, block_cache list is relocated, but next and prev list
>>> pointers are not adjusted to the relocated struct list_head address,
>>> so the first iteration over the block_cache list hangs.
>>>
>>> This patch initializes the block_cache list after relocation.
>>>
>>> Signed-off-by: Angelo Durgehello <angelo.dureghello@timesys.com>
>>> ---
>>>    common/board_r.c         | 12 ++++++++++++
>>>    drivers/block/blkcache.c |  7 ++++++-
>>>    include/blk.h            |  6 ++++++
>>>    3 files changed, 24 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/common/board_r.c b/common/board_r.c
>>> index 65720849cd..13e70a5ffb 100644
>>> --- a/common/board_r.c
>>> +++ b/common/board_r.c
>>> @@ -628,6 +628,15 @@ static int initr_bedbug(void)
>>>    }
>>>    #endif
>>>
>>> +#ifdef CONFIG_BLOCK_CACHE
>>> +static int initr_blkcache(void)
>>> +{
>>> +     blkcache_init();
>>> +
>>> +     return 0;
>>> +}
>>> +#endif
>>> +
>>
>> Why the extra level of indirection?
>>
>>>    static int run_main_loop(void)
>>>    {
>>>    #ifdef CONFIG_SANDBOX
>>> @@ -832,6 +841,9 @@ static init_fnc_t init_sequence_r[] = {
>>>    #endif
>>>    #if defined(CONFIG_PRAM)
>>>        initr_mem,
>>> +#endif
>>> +#ifdef CONFIG_BLOCK_CACHE
>>
>> It seems you could call blkcache_init from here directly:
>>
> 
> reason for this is to maintain "initr_" naming convention, used from
> quite all the initr_ calls,
> as i.e. static int initr_mmc(void) that's doing the same.
> 

Okay. I think this isn't a hard rule though (see log_init,
stdio_init_tables, etc).

I'm not sure that it would be a bad thing to rename blkcache_init
to initr_blkcache to indicate the usage.

>>
>>> +     initr_blkcache,
>>>    #endif
>>>        run_main_loop,
>>>    };
>>> diff --git a/drivers/block/blkcache.c b/drivers/block/blkcache.c
>>> index 1fa64989d3..bf0fa1ea6f 100644
>>> --- a/drivers/block/blkcache.c
>>> +++ b/drivers/block/blkcache.c
>>> @@ -21,13 +21,18 @@ struct block_cache_node {
>>>        char *cache;
>>>    };
>>>
>>> -static LIST_HEAD(block_cache);
>>> +static struct list_head block_cache;
>>>
>>>    static struct block_cache_stats _stats = {
>>>        .max_blocks_per_entry = 8,
>>>        .max_entries = 32
>>>    };
>>>
>>> +void blkcache_init(void)
>>> +{
>>> +     INIT_LIST_HEAD(&block_cache);
>>> +}
>>> +
>>>    static struct block_cache_node *cache_find(int iftype, int devnum,
>>>                                           lbaint_t start, lbaint_t blkcnt,
>>>                                           unsigned long blksz)
>>> diff --git a/include/blk.h b/include/blk.h
>>> index d0c033aece..7070fd6af3 100644
>>> --- a/include/blk.h
>>> +++ b/include/blk.h
>>> @@ -113,6 +113,12 @@ struct blk_desc {
>>>        (PAD_SIZE(size, blk_desc->blksz))
>>>
>>>    #if CONFIG_IS_ENABLED(BLOCK_CACHE)
>>> +
>>> +/**
>>> + * blkcache_init() - initialize the block cache list pointers
>>> + */
>>> +void blkcache_init(void);
>>> +
>>>    /**
>>>     * blkcache_read() - attempt to read a set of blocks from cache
>>>     *
>>>
>>
> Regards,
> --
> Angelo Dureghello
>
Eric Nelson Nov. 25, 2019, 3:56 p.m. UTC | #4 
Hi Angelo,

On 11/25/19 2:59 AM, Angelo Dureghello wrote:
> Hi Eric,
> 
> On Sun, Nov 24, 2019 at 5:00 PM Eric Nelson <ericnelsonaz@gmail.com> wrote:
>>
>> Hi Angelo,
>>
>> On 11/23/19 3:47 PM, Angelo Dureghello wrote:
>>> From: Angelo Durgehello <angelo.dureghello@timesys.com>
>>>
>>> On m68k, block_cache list is relocated, but next and prev list
>>> pointers are not adjusted to the relocated struct list_head address,
>>> so the first iteration over the block_cache list hangs.
>>>
>>> This patch initializes the block_cache list after relocation.
>>>
>>> Signed-off-by: Angelo Durgehello <angelo.dureghello@timesys.com>
>>> ---
>>>    common/board_r.c         | 12 ++++++++++++
>>>    drivers/block/blkcache.c |  7 ++++++-
>>>    include/blk.h            |  6 ++++++
>>>    3 files changed, 24 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/common/board_r.c b/common/board_r.c
>>> index 65720849cd..13e70a5ffb 100644
>>> --- a/common/board_r.c
>>> +++ b/common/board_r.c
>>> @@ -628,6 +628,15 @@ static int initr_bedbug(void)
>>>    }
>>>    #endif
>>>
>>> +#ifdef CONFIG_BLOCK_CACHE
>>> +static int initr_blkcache(void)
>>> +{
>>> +     blkcache_init();
>>> +
>>> +     return 0;
>>> +}
>>> +#endif
>>> +
>>
>> Why the extra level of indirection?
>>
>>>    static int run_main_loop(void)
>>>    {
>>>    #ifdef CONFIG_SANDBOX
>>> @@ -832,6 +841,9 @@ static init_fnc_t init_sequence_r[] = {
>>>    #endif
>>>    #if defined(CONFIG_PRAM)
>>>        initr_mem,
>>> +#endif
>>> +#ifdef CONFIG_BLOCK_CACHE
>>
>> It seems you could call blkcache_init from here directly:
>>
> 
> reason for this is to maintain "initr_" naming convention, used from
> quite all the initr_ calls,
> as i.e. static int initr_mmc(void) that's doing the same.
> 

Okay. I think this isn't a hard rule though (see log_init,
stdio_init_tables, etc).

I'm not sure that it would be a bad thing to rename blkcache_init
to initr_blkcache to indicate the usage.

>>
>>> +     initr_blkcache,
>>>    #endif
>>>        run_main_loop,
>>>    };
>>> diff --git a/drivers/block/blkcache.c b/drivers/block/blkcache.c
>>> index 1fa64989d3..bf0fa1ea6f 100644
>>> --- a/drivers/block/blkcache.c
>>> +++ b/drivers/block/blkcache.c
>>> @@ -21,13 +21,18 @@ struct block_cache_node {
>>>        char *cache;
>>>    };
>>>
>>> -static LIST_HEAD(block_cache);
>>> +static struct list_head block_cache;
>>>
>>>    static struct block_cache_stats _stats = {
>>>        .max_blocks_per_entry = 8,
>>>        .max_entries = 32
>>>    };
>>>
>>> +void blkcache_init(void)
>>> +{
>>> +     INIT_LIST_HEAD(&block_cache);
>>> +}
>>> +
>>>    static struct block_cache_node *cache_find(int iftype, int devnum,
>>>                                           lbaint_t start, lbaint_t blkcnt,
>>>                                           unsigned long blksz)
>>> diff --git a/include/blk.h b/include/blk.h
>>> index d0c033aece..7070fd6af3 100644
>>> --- a/include/blk.h
>>> +++ b/include/blk.h
>>> @@ -113,6 +113,12 @@ struct blk_desc {
>>>        (PAD_SIZE(size, blk_desc->blksz))
>>>
>>>    #if CONFIG_IS_ENABLED(BLOCK_CACHE)
>>> +
>>> +/**
>>> + * blkcache_init() - initialize the block cache list pointers
>>> + */
>>> +void blkcache_init(void);
>>> +
>>>    /**
>>>     * blkcache_read() - attempt to read a set of blocks from cache
>>>     *
>>>
>>
> Regards,
> --
> Angelo Dureghello
>
diff mbox series

Patch

diff --git a/common/board_r.c b/common/board_r.c
index 65720849cd..13e70a5ffb 100644
--- a/common/board_r.c
+++ b/common/board_r.c
@@ -628,6 +628,15 @@  static int initr_bedbug(void)
 }
 #endif
 
+#ifdef CONFIG_BLOCK_CACHE
+static int initr_blkcache(void)
+{
+	blkcache_init();
+
+	return 0;
+}
+#endif
+
 static int run_main_loop(void)
 {
 #ifdef CONFIG_SANDBOX
@@ -832,6 +841,9 @@  static init_fnc_t init_sequence_r[] = {
 #endif
 #if defined(CONFIG_PRAM)
 	initr_mem,
+#endif
+#ifdef CONFIG_BLOCK_CACHE
+	initr_blkcache,
 #endif
 	run_main_loop,
 };
diff --git a/drivers/block/blkcache.c b/drivers/block/blkcache.c
index 1fa64989d3..bf0fa1ea6f 100644
--- a/drivers/block/blkcache.c
+++ b/drivers/block/blkcache.c
@@ -21,13 +21,18 @@  struct block_cache_node {
 	char *cache;
 };
 
-static LIST_HEAD(block_cache);
+static struct list_head block_cache;
 
 static struct block_cache_stats _stats = {
 	.max_blocks_per_entry = 8,
 	.max_entries = 32
 };
 
+void blkcache_init(void)
+{
+	INIT_LIST_HEAD(&block_cache);
+}
+
 static struct block_cache_node *cache_find(int iftype, int devnum,
 					   lbaint_t start, lbaint_t blkcnt,
 					   unsigned long blksz)
diff --git a/include/blk.h b/include/blk.h
index d0c033aece..7070fd6af3 100644
--- a/include/blk.h
+++ b/include/blk.h
@@ -113,6 +113,12 @@  struct blk_desc {
 	(PAD_SIZE(size, blk_desc->blksz))
 
 #if CONFIG_IS_ENABLED(BLOCK_CACHE)
+
+/**
+ * blkcache_init() - initialize the block cache list pointers
+ */
+void blkcache_init(void);
+
 /**
  * blkcache_read() - attempt to read a set of blocks from cache
  *