On Mon, 2019-08-26 at 14:43 +0200, Felix Brack wrote:
Hello Weijie,
On 26.08.19 10:19, Weijie Gao wrote:
On Thu, 2019-08-22 at 15:58 +0200, Felix Brack wrote:
On 11.07.19 09:10, Weijie Gao wrote:
Some storage devices have multiple hw partitions and both address from zero, for example eMMC. However currently block cache invalidation only applies to block write/erase. This can cause a problem that data of current hw partition is cached before switching to another hw partition. And the following read operation of the latter hw partition will get wrong data when reading from the addresses that have been cached previously.
To solve this problem, invalidate block cache after a successful select_hwpart operation.
Signed-off-by: Weijie Gao weijie.gao@mediatek.com
This patch breaks correct operation of PDU001 board.
drivers/block/blk-uclass.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/block/blk-uclass.c b/drivers/block/blk-uclass.c index baaf431e5e0..c23b6682a6c 100644 --- a/drivers/block/blk-uclass.c +++ b/drivers/block/blk-uclass.c @@ -208,7 +208,11 @@ int blk_select_hwpart_devnum(enum if_type if_type, int devnum, int hwpart) if (ret) return ret;
- return blk_select_hwpart(dev, hwpart);
- ret = blk_select_hwpart(dev, hwpart);
- if (!ret)
blkcache_invalidate(if_type, devnum);- return ret;
}
int blk_list_part(enum if_type if_type) @@ -348,7 +352,13 @@ int blk_select_hwpart(struct udevice *dev, int hwpart)
int blk_dselect_hwpart(struct blk_desc *desc, int hwpart) {
- return blk_select_hwpart(desc->bdev, hwpart);
- int ret;
- ret = blk_select_hwpart(desc->bdev, hwpart);
- if (!ret)
blkcache_invalidate(desc->if_type, desc->devnum);Commenting the invalidation of the block cache on this line results in proper working of the board.
- return ret;
}
int blk_first_device(int if_type, struct udevice **devp)
With the patch active, files from the boot FAT partition of the SD-card (mmc device 1) do not load anymore, hence booting fails. Using the eMMC (mmc device 0) instead works fine, files can bee loaded and the board boots.
To isolate the problem I have modified the configuration to only load a 10k test file (test.bin) instead of loading the DTB and zImage files followed by booting LINUX. Furthermore I have enabled debugging for some parts of the code.
When I boot from the SD-card (which fails) I get the following logged:
CPU : AM335X-GP rev 1.0
This time the file test.bin gets loaded correctly.
To be honest I don't really see why things are going wrong. It looks like the block cache gets filled but then again invalidated before it's data is used. I also feel that this problem is related to the SD-card not being the first (number 0) but the second device (number 1). Can anybody shed some light on this and give me a hint?
many thanks, Felix
Hi Felix,
I've found the root cause.
Many thanks for looking into this!
There is a bug in the FAT driver. In function file_fat_read_at(), malloc_cache_aligned() allocates memory for the itr. The itr is a pointer to struct fat_itr. fat_itr has a member block[MAX_CLUSTSIZE] used for storing blocks read from MMC.
The FAT driver resolves the file table and stores the required dir entry in the member block. Then the pointer of the dir entry is passed to get_contents() to get the contents of the file.
However the itr is freed BEFORE the call to get_contents(). This means the contents dir entry points to is no longer valid. And it results to your situation.
Indeed this is the root of all trouble. The freeing of itr of course also invalidates the assignment made earlier:
dir_entry *dentptr = itr->dent;
As dentptr is used in the call to get_contents() the evil takes its course...
Again many thanks for analyzing and explaining this as detailed as you did.
But this only happens when CONFIG_BLOCK_CACHE=y and the patch applied. I've also found its cause.
The MMC driver calls blk_dselect_hwpart() unconditionally in mmc_bread(). This causes block cache being invalidated and refilled many times during the FAT load operation.
Yes I have noticed this frequent invalidations of the cache due to calls to blk_dselect_hwpart(). Some optimization, if possible, would be great.
The frequent block cache invalidation and refilling finally results in frequent memory allocation and freeing. There is a mechanism in the dlmalloc.c: if the total free space reaches a threshold, the malloc_trim() will be called. malloc_trim() calls sbrk(), which fills the memory being freed with zeros and of course the data of itr is damaged, otherwise the original data remains unchanged.
So this is the reason the bug only appears when block cache is enabled and cache invalidation is called after every select_hwpart.
I see and now understand the different behavior of eMMC and SD-card.
I have made a patch, could you please have a try? I've tested it worked. If it works for you too, I will submit it to u-boot. And I will submit another patch to optimize the cache invalidation for select_hwpart.
--- a/fs/fat/fat.c +++ b/fs/fat/fat.c @@ -1174,10 +1174,6 @@ int file_fat_read_at(const char *filename, loff_t pos, void *buffer, /* For saving default max clustersize memory allocated to malloc pool */ dir_entry *dentptr = itr->dent;
- free(itr);
- itr = NULL;
- ret = get_contents(&fsdata, dentptr, pos, buffer, maxsize, actread);
out_free_both:
I can confirm that your patch works perfect, thanks!
Please CC me when submitting the two patches you mentioned above. It will be my pleasure to test them on my hardware.
BTW, I reproduced this failure on a MT7623 board, which also has an eMMC and a SD. And this patch is indeed used for MT7623 to solve the bug on switching hwpart on eMMC.
Best Regards,
Weijie
regard, Felix
Hi Felix,
I found there's already a commit merged 6 hours ago (mailed on Aug 20) that fixed this issue: d7af2a863017be1f5fd1b65a858ddc7e87d7b876
So there's no need to send patch again. What you need is to pull the latest master branch.
I will still send a patch to optimize the invalidation in select_hwpart.
Best Regards,
Weijie