Message ID | 20191123224752.384355-1-angelo.dureghello@timesys.com |
---|---|
State | Changes Requested |
Delegated to: | Tom Rini |
Headers | show |
Series | None | expand |
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 > * >
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
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 >
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 --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 *