Re: [U-Boot] [PATCH 1/1] scripts/Makefile.lib: remove overridden target $(obj)/helloworld.so:

[Brüns , Stefan]

On Sonntag, 3. September 2017 17:26:43 CEST Heinrich Schuchardt wrote:
> On 09/03/2017 02:19 PM, Alexander Graf wrote:
> > On 03.09.17 08:17, Heinrich Schuchardt wrote:
> >> The target
> >> $(obj)/helloworld.so:
> >> exists twice in Makefile.lib.
> >> 
> >> If you add an echo command to each of the two recipes you get
> >> warnings like:
> >> 
> >> scripts/Makefile.lib:383: warning:
> >> overriding recipe for target 'drivers/power/battery/helloworld.so'
> >> scripts/Makefile.lib:379: warning:
> >> ignoring old recipe for target 'drivers/power/battery/helloworld.so'
> >> 
> >> This patch removes the obsolete target.
> >> 
> >> Signed-off-by: Heinrich Schuchardt 
> >> ---
> >> Hello Alex,
> >> 
> >> could you, please, review the change as it relates to EFI.
> > 
> > My Makefile foo isn't quite as good as it should be, but doesn't the
> > existing code simply add another dependency to the required build chain?
> 
> The target is overridden so why should the dependency be executed?
> 
> https://www.gnu.org/software/make/manual/html_node/Overriding-Makefiles.html
> says:
> "However, it is invalid for two makefiles to give different recipes for
> the same target. I guess this will be valid for a single makefile too."
> 
> If you think the dependency is necessary, I can add it to the remaining
> target. Is this what you prefer?

Note there is a difference between prerequisite and recipe - specifying a 
target multiple times without recipe adds the the prerequisite to the existing 
set, see

https://www.gnu.org/software/make/manual/html_node/Multiple-Targets.html#Multiple-Targets

---
foo: a

foo: b c

a:
@echo "Creating prereq a"

%:
@echo "Creating $@"


This tries to create the default target foo (first specified target), which 
has three prerequisites (a, b, c), which are run in parallel.

Kind regards,

Stefan
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] fs: ext4: Fix journal overrun issue reported by Coverity

[Brüns , Stefan]

On Montag, 21. August 2017 04:30:15 CEST Tom Rini wrote:
> While _jdb[fs->blksz] is a valid expression (it points *one* char
> sized element past the end of the array, e.g. _jdb[fs->blksz + 1] is
> invalid (according to the C standard (C99/C11)).
> 
> Changing this to tag = (struct ext3_journal_block_tag *)(p_jdb + ofs);
> 
> Cc: Stefan Brüns 
> Suggested-by: Stefan Brüns 
> Reported-by: Coverity (CID: 165117, 165110)
> Signed-off-by: Tom Rini 
> ---
> Stefan, since this is your suggestion and message, if you want me to v2
> with you as Author, I'd be quite happy to, thanks again!

Hi Tom,

whatever you like, both is fine with me.

Kind regards,

Stefan

PS:
Reviewed-by: Stefan Brüns 

> ---
>  fs/ext4/ext4_journal.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/ext4_journal.c b/fs/ext4/ext4_journal.c
> index 5a25be4c8ac2..fed6287eac45 100644
> --- a/fs/ext4/ext4_journal.c
> +++ b/fs/ext4/ext4_journal.c
> @@ -355,7 +355,7 @@ void recover_transaction(int prev_desc_logical_no)
>   ofs = sizeof(struct journal_header_t);
> 
>   do {
> - tag = (struct ext3_journal_block_tag *)_jdb[ofs];
> + tag = (struct ext3_journal_block_tag *)(p_jdb + ofs);
>   ofs += sizeof(struct ext3_journal_block_tag);
> 
>   if (ofs > fs->blksz)
> @@ -466,7 +466,7 @@ int ext4fs_check_journal_state(int recovery_flag)
>   ofs = sizeof(struct journal_header_t);
>   do {
>   tag = (struct ext3_journal_block_tag *)
> - _jdb[ofs];
> + (p_jdb + ofs);
>   ofs += sizeof(struct ext3_journal_block_tag);
>   if (ofs > fs->blksz)
>   break;

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v2 3/8] fat/fs: convert to directory iterators

[Brüns , Stefan]

On Montag, 14. August 2017 15:16:15 CEST Rob Clark wrote:
> And drop a whole lot of ugly code!
> 
> Signed-off-by: Rob Clark 
> ---
>  fs/fat/fat.c  | 723
> ++ include/fat.h | 
>  6 -
>  2 files changed, 75 insertions(+), 654 deletions(-)

Nice, even after accounting for the ~300 lines added for the iterators :-)

Two comments inline ...
 
> diff --git a/fs/fat/fat.c b/fs/fat/fat.c
> index 69fa7f4cab..a50a10ba47 100644
> --- a/fs/fat/fat.c
> +++ b/fs/fat/fat.c
> @@ -119,22 +119,6 @@ int fat_register_device(struct blk_desc *dev_desc, int
> part_no) }
> 
[...]
> -
> -/*
>   * Extract zero terminated short name from a directory entry.
>   */
>  static void get_name(dir_entry *dirent, char *s_name)
> @@ -468,95 +452,6 @@ static int slot2str(dir_slot *slotptr, char *l_name,
> int *idx) return 0;
>  }
[...]
> -}
> -
>  /* Calculate short name checksum */
>  static __u8 mkcksum(const char name[8], const char ext[3])
>  {
> @@ -572,170 +467,11 @@ static __u8 mkcksum(const char name[8], const char
> ext[3]) return ret;
[...]
> 
>  /*
>   * Read boot sector and volume info from a FAT filesystem
> @@ -877,374 +613,6 @@ static int get_fs_info(fsdata *mydata)
>   return 0;
>  }
[...]
> -
> - while (isdir) {
> - int startsect = mydata->data_begin
> - + START(dentptr) * mydata->clust_size;
> - dir_entry dent;
> - char *nextname = NULL;
> -
> - dent = *dentptr;
> - dentptr = 
> -
> - idx = dirdelim(subname);
> -
> - if (idx >= 0) {
> - subname[idx] = '\0';
> - nextname = subname + idx + 1;
> - /* Handle multiple delimiters */
> - while (ISDIRDELIM(*nextname))
> - nextname++;
> - if (dols && *nextname == '\0')
> - firsttime = 0;
> - } else {
> - if (dols && firsttime) {
> - firsttime = 0;
> - } else {
> - isdir = 0;
> - }
> - }
> -
> - if (get_dentfromdir(mydata, startsect, subname, dentptr,
> -  isdir ? 0 : dols) == NULL) {
> - if (dols && !isdir)
> - *size = 0;
> - goto exit;
> - }
> -
> - if (isdir && !(dentptr->attr & ATTR_DIR))
> - goto exit;
> -
> - /*
> -  * If we are looking for a directory, and found a directory
> -  * type entry, and the entry is for the root directory (as
> -  * denoted by a cluster number of 0), jump back to the start
> -  * of the function, since at least on FAT12/16, the root dir
> -  * lives in a hard-coded location and needs special handling
> -  * to parse, rather than simply following the cluster linked
> -  * list in the FAT, like other directories.
> -  */

Have you checked this case still works? AFAICS this is not covered in fs-
test.sh. Examples of suitables sandbox commands are given in the commit 
message of:

18a10d46f267057ede0490ddba71c106475b4eb1 (fat: handle paths that include ../)

> - if (isdir && (dentptr->attr & ATTR_DIR) && !START(dentptr)) {
> - /*
> -  * Modify the filename to remove the prefix that gets
> -  * back to the root directory, so the initial root dir
> -  * parsing code can continue from where we are without
> -  * confusion.
> -  */
> - strcpy(fnamecopy, nextname ?: "");
> - /*
> -  * Set up state the same way as the function does when
> -  * first started. This is required for the root dir
> -  * parsing code operates in its expected environment.
> -  */
> - subname = "";
> - cursect = mydata->rootdir_sect;
> - isdir = 0;
> - goto root_reparse;
> - }
> -
> - if (idx >= 0)
> - subname = nextname;
> - }
> -
> - if (dogetsize) {
> - *size = FAT2CPU32(dentptr->size);
> - ret = 0;
> - } else {
> - ret = get_contents(mydata, dentptr, pos, buffer, maxsize, size);
> - }
> - debug("Size: %u, got: %llu\n", FAT2CPU32(dentptr->size), *size);
> -
> -exit:
> - free(mydata->fatbuf);
> - return ret;
> -}
> -
> 
>  /*
>   * Directory iterator, to simplify filesystem traversal
> @@ -1571,12 +939,6 @@ static int fat_itr_resolve(fat_itr *itr, const char
> *path, unsigned type) return -ENOENT;
>  }
> 
> -int 

Re: [U-Boot] [PATCH v0 01/20] fs: add fs_readdir()

[Brüns , Stefan]

On Freitag, 4. August 2017 21:31:43 CEST Rob Clark wrote:
> Needed to support efi file protocol.  The fallback.efi loader wants
> to be able to read the contents of the /EFI directory to find an OS
> to boot.
> 
> For reference, the expected EFI semantics are described in (v2.7 of UEFI
> spec) in section 13.5 (page 609).  Or for convenience, see:
> 
>   http://wiki.phoenix.com/wiki/index.php/EFI_FILE_PROTOCOL#Read.28.29
> 
> The EFI level semantics are implemented in a later patch, so they are
> not too important to the understanding of this patch.
> 
> Signed-off-by: Rob Clark 
> ---
>  fs/fs.c  | 25 +
>  include/fs.h | 21 +
>  2 files changed, 46 insertions(+)

Still, the commit message is in no way helpful when trying to understand what 
your changes are actually doing.

You introduce an arbitrary new API in the filesystem level (you neither expose 
an existing API, nor are you implementing the API requested by EFI, nor 
anything roughly resembling it).

The API you expose adds an index-based directory lookup, while EFI wants an 
POSIX-like directory stream. After reading through both the EFI spec and U-
Boots file system code, its clear you want to have some matching layer between 
the mostly stateless U-Boot filesystem layer and the stateful EFI API.

Please provide a thorough description why you create this new API in the fs 
layer, state that it is a hack to achieve what you want. If sometime later 
someone else wants to clean this up (both the FAT implementation, and the 
API), she/he should not have to go through all the code.

Regards,

Stefan


___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] fs: add fs_readdir()

[Brüns , Stefan]

On Donnerstag, 3. August 2017 18:54:30 CEST Rob Clark wrote:
> Needed to support efi file protocol.  The fallback.efi loader wants
> to be able to read the contents of the /EFI directory to find an OS
> to boot.
> 
> Currently only implemented for FAT, but that is all that UEFI is
> required to support.
> 
> Signed-off-by: Rob Clark 
> ---
>  fs/fat/fat.c  | 59
> ++- fs/fs.c   |
> 25 +
>  include/fat.h |  4 +++-
>  include/fs.h  | 21 +
>  4 files changed, 95 insertions(+), 14 deletions(-)
> 

NAK

1. The current code is already much to convoluted. Your changes add to this 
significantly

2. Your patch description neither references the exact part of the EFI 
specification you want to support (which took me some time, for reference it 
is "13.: Protocols - Media Access, 13.5: File Protocol"), nor are you 
specifying the required semantics (which is "open", "read", "close", where 
each read returns a single directory entry, similar to the POSIX opendir(), 
readdir() calls. 

3. Usage of an index too lookup the next entry is also quite convoluted.

4. As far as I can see, your code will fail to find files in the root 
directory (look for LS_ROOT).

I think it would be much better to first restructure the current code to use 
an readdir like interface internally, and then do everything EFI needs on top.

This would get rid of the 4 almost identical copies to print the current 
directory entry (dols == LS_ROOT || dols == LS_YES), 2 copies of the remaining 
directory traversal and and also avoid the bug in (4.).

Kind regards,

Stefan
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] efi_loader: make pool allocations cacheline aligned

[Brüns , Stefan]

On Mittwoch, 2. August 2017 11:28:37 CEST Rob Clark wrote:
> On Tue, Aug 1, 2017 at 9:10 PM, Heinrich Schuchardt
[...]
> >> 
> >> @@ -356,7 +356,8 @@ efi_status_t efi_allocate_pool(int pool_type,
> >> unsigned long size,>> 
> >>  {
> >>  
> >>   efi_status_t r;
> >>   efi_physical_addr_t t;
> >> 
> >> - u64 num_pages = (size + sizeof(u64) + EFI_PAGE_MASK) >>
> >> EFI_PAGE_SHIFT; + u64 num_pages = DIV_ROUND_UP(size + sizeof(struct
> >> efi_pool_allocation), +  EFI_PAGE_SIZE);
> >> 
> >>   if (size == 0) {
> >>   
> >>   *buffer = NULL;
> > 
> > NAK
> > 
> > With DIV_ROUND_UP you introduce a 64bit division. Depending on the
> > architecture this is only available via stdlib which is not available in
> > U-Boot.
> 
> The divisor is a constant power of two so compiler should turn this into a
> shift
> > Please, use
> > + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
> > as in the original line.
> 
> This was actually incorrect (missing a "- 1"), which is why I decided
> to stop open-coding DIV_ROUND_UP().

EFI_PAGE_MASK == EFI_PAGE_SIZE - 1

Kind regards,

Stefan
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] efi_loader: fix bug in efi_get_memory_map

[Brüns , Stefan]

On Mittwoch, 26. Juli 2017 22:25:29 CEST Rob Clark wrote:
> On Wed, Jul 26, 2017 at 4:10 PM, Alexander Graf  wrote:
> > On 26.07.17 20:34, Rob Clark wrote:
> >> When booting shim -> fallback -> shim -> grub -> linux the memory map is
> >> a bit larger than the size linux passes in on the first call.  But in
> >> the EFI_BUFFER_TOO_SMALL case we were not passing back the updated size
> >> to linux so it would loop forever.
> >> 
> >> Signed-off-by: Rob Clark 
> > 
> > The spec is actually very explicit about this case. It says in the
> > EFI_BUFFER_TOO_SMALL case, we *have* to return the map size.
> 
> yes, that is what I fixed.  We *weren't* returning the required buffer
> size before :-)

Sigh, yes, this was correct in the first 3 versions of the patch series, but 
unfortunately broken in v4 which was actually committed ...

See: https://lists.denx.de/pipermail/u-boot/2016-October/268766.html

Actually, the map_size variable is no longer needed, if you assign to 
*memory_map_size directly.

Anyway, this patch is:
Reviewed-by: Stefan Brüns 

Kind regards,

Stefan

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] TPM2.0 support in u-boot

[Brüns , Stefan]

On Montag, 29. Mai 2017 10:28:38 CEST peter.hu...@infineon.com wrote:
> Hi,
> 
> I was wondering if anyone is currently working on u-boot support for TPM2.0
> chips, especially the "native" spi versions. (i.e. direct access to the tpm
> via spi, not via something like the pch) Having support for TPM2.0 in
> u-boot is crucial in order to achieve some measured boot scenarios.
> 
> I scanned through the mailing list but only found someone asking whether
> this was possible
> https://lists.denx.de/pipermail/u-boot/2016-December/275317.html
> but before I might start working on it I was wondering if anyone else is
> currently doing this and/or someone is interested in doing so.

I think Andreas Faerber is working on some Infineon TPMs, although it may be 
linux only.

Kind regards,

Stefan
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v5 02/19] usb: dwc2: Use separate input and output buffers

[Brüns , Stefan]

On Montag, 3. April 2017 20:18:42 CEST Brüns, Stefan wrote:
> On Montag, 3. April 2017 17:38:40 CEST you wrote:
> > 
> > What am I missing?
> 
> The following is a gross oversimplification, but might explain it:
> 
> 1. Assume all of the 64kB of the aligned_buffer is dirty. (Likely, if the
> buffer is calloced.)
> 2. We are doing some transfers. All transfers are small, e.g. 64 byte.
> 3. In accordance with the two cases you mentioned above, the first 64 byte
> are no longer dirty, as the last out transfer has flushed the cacheline. 4.
> We are doing our first large in transfer (i.e. larger than 64 byte). 5. Bad
> Things (TM) may happen to any data at aligned_buffer[64] and beyond.
> 
> If this holds, a single invalidate_dcache_range(aligned_buffer,
> aligned_buffer +65536,...) during the initialization of the controller
> would suffice.

I just read "[U-Boot] [PATCH] usb: dwc3: gadget: make cache-maintenance on 
event buffers more robust" from Philipp Tomsich, which mentions the same 
problem:

The original code was doing the following (on AArch64, which
translates a 'flush' into a 'clean + invalidate'):
  # during initialisation:
  1. allocate buffers via memalign
 => buffers may still be modified (cached, dirty)
  # during interrupt processing
  2. clean + invalidate buffers
 => may commit stale data from a modified cacheline
  3. read from buffers

This could lead to garbage info being written to buffers before
reading them during even-processing.

To make the event processing more robust, we use the following sequence
for the cache-maintenance:
  # during initialisation:
  1. allocate buffers via memalign
  2. clean + invalidate buffers
 (we only need the 'invalidate' part, but dwc3_flush_cache()
  always performs a 'clean + invalidate')


Kind regards,

Stefan
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v5 02/19] usb: dwc2: Use separate input and output buffers

[Brüns , Stefan]

On Montag, 3. April 2017 17:38:40 CEST you wrote:
> Hi Stefan,
> 
> On 3 April 2017 at 08:26, Brüns, Stefan <stefan.bru...@rwth-aachen.de> 
wrote:
> > On Montag, 3. April 2017 01:23:17 CEST you wrote:
> >> Hi Stefan,
> >> 
> >> On 2 April 2017 at 15:34, Stefan Bruens <stefan.bru...@rwth-aachen.de>
> > 
> > wrote:
> >> > On Sonntag, 2. April 2017 17:43:38 CEST Simon Glass wrote:
> >> >> Hi Stefan,
> >> >> 
> >> >> On 2 April 2017 at 07:10, Stefan Bruens <stefan.bru...@rwth-aachen.de>
> >> > 
> >> > wrote:
> >> >> > On Sonntag, 2. April 2017 05:01:41 CEST Marek Vasut wrote:
> >> >> >> On 04/02/2017 01:40 AM, Simon Glass wrote:
> >> >> >> > Hi Marek,
> >> >> >> > 
> >> >> >> > On 1 April 2017 at 14:15, Marek Vasut <ma...@denx.de> wrote:
> >> >> >> >> On 04/01/2017 08:05 PM, Simon Glass wrote:
> >> >> >> >>> On Raspberry Pi 2 and 3 a problem was noticed when enabling
> >> >> >> >>> driver
> >> >> >> >>> model
> >> >> >> >>> for USB: the cache invalidate after an incoming transfer does
> >> >> >> >>> not
> >> >> >> >>> seem
> >> >> >> >>> to
> >> >> >> >>> work correctly.
> >> >> >> >>> 
> >> >> >> >>> This may be a problem with the underlying caching
> >> >> >> >>> implementation
> >> >> >> >>> on
> >> >> >> >>> armv7
> >> >> >> >>> and armv8 but this seems very unlikely. As a work-around, use
> >> >> >> >>> separate
> >> >> >> >>> buffers for input and output. This ensures that the input
> >> >> >> >>> buffer
> >> >> >> >>> will
> >> >> >> >>> not
> >> >> >> >>> hold dirty cache data.
> >> >> >> >> 
> >> >> >> >> What do you think of this patch:
> >> >> >> >> [U-Boot] usb: dwc2: invalidate the dcache before starting the
> >> >> >> >> DMA
> >> >> >> > 
> >> >> >> > Yes that matches what I did as a hack. I didn't realise that the
> >> >> >> > DMA
> >> >> >> > would go through the cache. Thanks for the pointer.
> >> >> >> 
> >> >> >> DMA should not go through the cache. I have yet to review that
> >> >> >> patch,
> >> >> >> but IMO it's relevant to this problem you observe.
> >> >> > 
> >> >> > DMA transfers not going through the cache is probably the problem
> >> >> > here:
> >> >> > 
> >> >> > Assume we have the aligned_buffer at address 0xdead
> >> >> > 
> >> >> > 1. The cpu writes to address 0xdead0002. This is fine, as it is the
> >> >> > current
> >> >> > owner of the address. The cacheline is marked dirty.
> >> >> > 2. The cpu no longer needs the corresponding address range, and it
> >> >> > is
> >> >> > reallocated (i.e. freed and then allocated from dwc2) or reused
> >> >> > (i.e.
> >> >> > formerly out buffer, now in buffer).
> >> >> > 3. The CPU starts the DMA transfer
> >> >> > 4. The DMA transfer writes to e.g. 0xdead-0xdead0200 in memory.
> >> >> > 5. The CPU fetches an address aliasing with 0xdead. The dirty
> >> >> > cache
> >> >> > line is evicted, and the 0xdead-0xdead0040 memory contents are
> >> >> > overwritten.
> >> >> 
> >> >> This is the part I don't understand. This should be an invalidate, not
> >> >> a clean and invalidate, so there should be not memory write.
> >> >> 
> >> >> Also if the CPU fetches from cached 0xdead without an invalidate,
> >> >> it will not cause a cash clean. It will simple read the data from the
> >> >> cache and ignore what the DMA wrote.
> >> > 
> >> > The CPU does not fetch 0xdead

Re: [U-Boot] usb: dwc2: invalidate the dcache before starting the DMA

[Brüns , Stefan]

On Samstag, 1. April 2017 08:51:39 CEST Eddie Cai wrote:
> We should invalidate the dcache before starting the DMA. In case there are
> any dirty lines from the DMA buffer in the cache, subsequent cache-line
> replacements may corrupt the buffer in memory while the DMA is still going
> on. Cache-line replacement can happen if the CPU tries to bring some other
> memory locations into the cache while the DMA is going on.
> 
> Signed-off-by: Eddie Cai 

Can you please run the patch through checkpatch, I can at least spot some 
missing whitespace.

You can add my

Reviewed-by: Stefan Brüns 

to your v2.

Kind regards,

Stefan

> ---
>  drivers/usb/host/dwc2.c | 17 +++--
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c
> index 5ac602e..a151ba6 100644
> --- a/drivers/usb/host/dwc2.c
> +++ b/drivers/usb/host/dwc2.c
> @@ -811,12 +811,17 @@ static int transfer_chunk(struct dwc2_hc_regs
> *hc_regs, void *aligned_buffer, (*pid << DWC2_HCTSIZ_PID_OFFSET),
>  _regs->hctsiz);
> 
> - if (!in && xfer_len) {
> - memcpy(aligned_buffer, buffer, xfer_len);
> -
> - flush_dcache_range((unsigned long)aligned_buffer,
> -(unsigned long)aligned_buffer +
> -roundup(xfer_len, ARCH_DMA_MINALIGN));
> + if (xfer_len) {
> + if(in){
> + invalidate_dcache_range((unsigned long)aligned_buffer,
> +(unsigned long)aligned_buffer +
> +roundup(xfer_len, 
> ARCH_DMA_MINALIGN));
> + }else{
> + memcpy(aligned_buffer, buffer, xfer_len);
> + flush_dcache_range((unsigned long)aligned_buffer,
> +(unsigned long)aligned_buffer +
> +roundup(xfer_len, 
> ARCH_DMA_MINALIGN));
> + }
>   }
> 
>   writel(phys_to_bus((unsigned long)aligned_buffer), _regs->hcdma);

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH v5 02/19] usb: dwc2: Use separate input and output buffers

[Brüns , Stefan]

On Montag, 3. April 2017 01:23:17 CEST you wrote:
> Hi Stefan,
> 
> On 2 April 2017 at 15:34, Stefan Bruens  
wrote:
> > On Sonntag, 2. April 2017 17:43:38 CEST Simon Glass wrote:
> >> Hi Stefan,
> >> 
> >> On 2 April 2017 at 07:10, Stefan Bruens 
> > 
> > wrote:
> >> > On Sonntag, 2. April 2017 05:01:41 CEST Marek Vasut wrote:
> >> >> On 04/02/2017 01:40 AM, Simon Glass wrote:
> >> >> > Hi Marek,
> >> >> > 
> >> >> > On 1 April 2017 at 14:15, Marek Vasut  wrote:
> >> >> >> On 04/01/2017 08:05 PM, Simon Glass wrote:
> >> >> >>> On Raspberry Pi 2 and 3 a problem was noticed when enabling driver
> >> >> >>> model
> >> >> >>> for USB: the cache invalidate after an incoming transfer does not
> >> >> >>> seem
> >> >> >>> to
> >> >> >>> work correctly.
> >> >> >>> 
> >> >> >>> This may be a problem with the underlying caching implementation
> >> >> >>> on
> >> >> >>> armv7
> >> >> >>> and armv8 but this seems very unlikely. As a work-around, use
> >> >> >>> separate
> >> >> >>> buffers for input and output. This ensures that the input buffer
> >> >> >>> will
> >> >> >>> not
> >> >> >>> hold dirty cache data.
> >> >> >> 
> >> >> >> What do you think of this patch:
> >> >> >> [U-Boot] usb: dwc2: invalidate the dcache before starting the DMA
> >> >> > 
> >> >> > Yes that matches what I did as a hack. I didn't realise that the DMA
> >> >> > would go through the cache. Thanks for the pointer.
> >> >> 
> >> >> DMA should not go through the cache. I have yet to review that patch,
> >> >> but IMO it's relevant to this problem you observe.
> >> > 
> >> > DMA transfers not going through the cache is probably the problem here:
> >> > 
> >> > Assume we have the aligned_buffer at address 0xdead
> >> > 
> >> > 1. The cpu writes to address 0xdead0002. This is fine, as it is the
> >> > current
> >> > owner of the address. The cacheline is marked dirty.
> >> > 2. The cpu no longer needs the corresponding address range, and it is
> >> > reallocated (i.e. freed and then allocated from dwc2) or reused (i.e.
> >> > formerly out buffer, now in buffer).
> >> > 3. The CPU starts the DMA transfer
> >> > 4. The DMA transfer writes to e.g. 0xdead-0xdead0200 in memory.
> >> > 5. The CPU fetches an address aliasing with 0xdead. The dirty cache
> >> > line is evicted, and the 0xdead-0xdead0040 memory contents are
> >> > overwritten.
> >> 
> >> This is the part I don't understand. This should be an invalidate, not
> >> a clean and invalidate, so there should be not memory write.
> >> 
> >> Also if the CPU fetches from cached 0xdead without an invalidate,
> >> it will not cause a cash clean. It will simple read the data from the
> >> cache and ignore what the DMA wrote.
> > 
> > The CPU does not fetch 0xdead, but from an address *aliasing* with
> > 0xdead000. As 0xdead is *dirty* (we have neither flushed (clears dirty
> > bit) or invalidated (implicitly clears dirty for the address)), the cache
> > controller has to write out the 0xdead cache line to memory.
> 
> That doesn't make any sense to me. Can you explain it a bit more?
> 
> If the CPU fetches from a cache-alias of 0xdead, say 0xa11a5000
> then I expect the cache line would contain the data for that alias,
> not for 0xdead.

The important part is the dirty flag in the 0xdead cacheline. By reading 
an aliasing address, you are causing eviction of the current cache line 
contents, and writing back its contents to memory. Reading of an address may 
cause write of a different address. You can see it as an dcache_flush_range 
done by the cache controller.

I think you are assuming a write-through cache here, which leads to your 
confusion.

> So a later invalidate of 0xdead should at most
> clean the cache line and write to memory at 0xa11a5000. If it were to
> write cached data intended for 0xa11a5000 to memory at 0xdead,
> then surely this would be a bug?

After the cache line for 0xdead has been evicted, any flush/invalidate 
operations are noops for that address.

> Therefore I cannot see the situation where the CPU should write to
> 0xdead when that address is invalidated.

It is not the invalidation which causes the write, but eviction from the 
cache.

 > >> On armv8 we appear not to suppose invalidate in the code, so it makes
> >> sense for rpi_3.
> >> 
> >> But for rpi_2 which seems to do a proper invalidate, I still don't see
> >> the problem.
> > 
> > Which part of the code is different between rpi2 and rpi3? The dwc2 code
> > is
> > identical, is the memory invalidated in some other place?
> 
> It is the invalidate_dcache_range() function.

Thats for pointing that out, for anyone not having read the code:

ARMv7 has different operations for flush_dcache_range and 
invalidate_dcache_range, the former does a writeback of dirty cache lines and 
sets the invalid tags for the corresponding cache lines, the latter only does 
the invalidate part.

Re: [U-Boot] [PATCH] bcm2835_wdt: support for the BCM2835/2836 watchdog

[Brüns , Stefan]

On Freitag, 27. Januar 2017 10:59:05 CET Paolo Pisati wrote:
> Signed-off-by: Paolo Pisati 
> ---
>  arch/arm/mach-bcm283x/reset.c  | 21 ++---
>  board/raspberrypi/rpi/rpi.c|  4 
>  drivers/watchdog/Makefile  |  1 +
>  drivers/watchdog/bcm2835_wdt.c | 34 ++
>  include/configs/rpi.h  |  3 +++
>  scripts/config_whitelist.txt   |  1 +
>  6 files changed, 61 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/watchdog/bcm2835_wdt.c
> 
> diff --git a/arch/arm/mach-bcm283x/reset.c b/arch/arm/mach-bcm283x/reset.c
> index 685815c..b62cb8a 100644
> --- a/arch/arm/mach-bcm283x/reset.c
> +++ b/arch/arm/mach-bcm283x/reset.c
> @@ -21,18 +21,33 @@
>   */
>  #define BCM2835_WDOG_RSTS_RASPBERRYPI_HALT   0x555
> 
> +/* max ticks timeout */
> +#define BCM2835_WDOG_MAX_TIMEOUT 0x000f
> +
> +#ifdef CONFIG_BCM2835_WDT
> +extern void hw_watchdog_disable(void);
> +#else
> +void hw_watchdog_disable(void) {}
> +#endif
> +
>  __efi_runtime_data struct bcm2835_wdog_regs *wdog_regs =
>   (struct bcm2835_wdog_regs *)BCM2835_WDOG_PHYSADDR;
> 
> -void __efi_runtime reset_cpu(ulong addr)
> +void __efi_runtime reset_cpu(ulong ticks)
>  {
> - uint32_t rstc;
> + uint32_t rstc, timeout;
> +
> + if (ticks == 0) {
> + hw_watchdog_disable();
> + timeout = RESET_TIMEOUT;

This is wrong. The efi runtime reset function calls this as reset_cpu(0), and 
then tries to call hw_watchdog_disable, which is not marked as __efi_runtime.

Actually, I can see no reason the watchdog setup piggybacks on the reset_cpu 
funtion.

> + } else
> + timeout = ticks & BCM2835_WDOG_MAX_TIMEOUT;
> 
>   rstc = readl(_regs->rstc);
>   rstc &= ~BCM2835_WDOG_RSTC_WRCFG_MASK;
>   rstc |= BCM2835_WDOG_RSTC_WRCFG_FULL_RESET;
> 
> - writel(BCM2835_WDOG_PASSWORD | RESET_TIMEOUT, _regs->wdog);
> + writel(BCM2835_WDOG_PASSWORD | timeout, _regs->wdog);
>   writel(BCM2835_WDOG_PASSWORD | rstc, _regs->rstc);
>  }
> 
> diff --git a/board/raspberrypi/rpi/rpi.c b/board/raspberrypi/rpi/rpi.c
> index 22e87a2..106e518 100644
> --- a/board/raspberrypi/rpi/rpi.c
> +++ b/board/raspberrypi/rpi/rpi.c
[...]
> --- a/scripts/config_whitelist.txt
> +++ b/scripts/config_whitelist.txt
> @@ -294,6 +294,7 @@ CONFIG_BCH_CONST_M
>  CONFIG_BCH_CONST_PARAMS
>  CONFIG_BCH_CONST_T
>  CONFIG_BCM2835_GPIO
> +CONFIG_BCM2835_WDT
>  CONFIG_BCM283X_MU_SERIAL
>  CONFIG_BCM_SF2_ETH
>  CONFIG_BCM_SF2_ETH_DEFAULT_PORT

I think the rule is no new CONFIG_xxx options, but appropriate options in 
KConfig.

Kind regards,

Stefan
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH v2] fs/fat: Fix unaligned __u16 reads for FAT12 access

[Brüns , Stefan]

Doing unaligned reads is not supported on all architectures, use
byte sized reads of the little endian buffer.
Rename off16 to off8, as it reflects the buffer offset in byte
granularity (offset is in entry, i.e. 12 bit, granularity).
Fix a regression introduced in 8d48c92b45aea91e2a2be90f2ed93677e85526f1

Reported-by: Oleksandr Tymoshenko 
Signed-off-by: Stefan Brüns 
Tested-by: Oleksandr Tymoshenko 
---
 fs/fat/fat.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

v2: added parentheses to fix operator precedence
added Tested-by: Oleksandr Tymoshenko 

diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index fe899d0442..23ec280071 100644
--- a/fs/fat/fat.c
+++ b/fs/fat/fat.c
@@ -179,7 +179,7 @@ int flush_dirty_fat_buffer(fsdata *mydata)
 static __u32 get_fatent(fsdata *mydata, __u32 entry)
 {
__u32 bufnum;
-   __u32 off16, offset;
+   __u32 offset, off8;
__u32 ret = 0x00;
 
if (CHECK_CLUST(entry, mydata->fatsize)) {
@@ -242,8 +242,9 @@ static __u32 get_fatent(fsdata *mydata, __u32 entry)
ret = FAT2CPU16(((__u16 *) mydata->fatbuf)[offset]);
break;
case 12:
-   off16 = (offset * 3) / 2;
-   ret = FAT2CPU16(*(__u16 *)(mydata->fatbuf + off16));
+   off8 = (offset * 3) / 2;
+   /* fatbut + off8 may be unaligned, read in byte granularity */
+   ret = mydata->fatbuf[off8] + (mydata->fatbuf[off8 + 1] << 8);
 
if (offset & 0x1)
ret >>= 4;
-- 
2.11.0
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH] fs/fat: Fix unaligned __u16 reads for FAT12 access

[Brüns , Stefan]

Doing unaligned reads is not supported on all architectures, use
byte sized reads of the little endian buffer.
Rename off16 to off8, as it reflects the buffer offset in byte
granularity (offset is in entry, i.e. 12 bit, granularity).
Fix a regression introduced in 8d48c92b45aea91e2a2be90f2ed93677e85526f1

Reported-by: Oleksandr Tymoshenko 
Signed-off-by: Stefan Brüns 
---
 fs/fat/fat.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/fat/fat.c b/fs/fat/fat.c
index fe899d0442..06088e2421 100644
--- a/fs/fat/fat.c
+++ b/fs/fat/fat.c
@@ -179,7 +179,7 @@ int flush_dirty_fat_buffer(fsdata *mydata)
 static __u32 get_fatent(fsdata *mydata, __u32 entry)
 {
__u32 bufnum;
-   __u32 off16, offset;
+   __u32 offset, off8;
__u32 ret = 0x00;
 
if (CHECK_CLUST(entry, mydata->fatsize)) {
@@ -242,8 +242,9 @@ static __u32 get_fatent(fsdata *mydata, __u32 entry)
ret = FAT2CPU16(((__u16 *) mydata->fatbuf)[offset]);
break;
case 12:
-   off16 = (offset * 3) / 2;
-   ret = FAT2CPU16(*(__u16 *)(mydata->fatbuf + off16));
+   off8 = (offset * 3) / 2;
+   /* fatbut + off8 may be unaligned, read in byte granularity */
+   ret = mydata->fatbuf[off8] + mydata->fatbuf[off8 + 1] << 8;
 
if (offset & 0x1)
ret >>= 4;
-- 
2.11.0
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] ext4: crash when writing a file

[Brüns , Stefan]

On Freitag, 20. Januar 2017 18:32:49 CET Sébastien Szymanski wrote:
[...]
> Then under the sandbox I do the following:
> 
> U-Boot 2017.01-2-g558e41e-dirty (Jan 20 2017 - 16:19:47 +0100)
> 
> DRAM:  256 MiB
> MMC:
> Using default environment
> 
> In:serial
> Out:   serial
> Err:   serial
> SCSI:  Net:   No ethernet found.
> IDE:   Bus 0: not available
> 
> => host bind 0 /tftproot/rootfs.raw
> 
> => ls host 0:2
>1024 .
>1024 ..
>   16384 lost+found
>1024 var
>1024 run
>1024 root
>1024 media
>1024 mnt
>1024 tmp
>   3 lib32
>1024 usr
>1024 proc
>1024 dev
>1024 boot
>1024 sys
>3072 sbin
>3072 bin
>  11 linuxrc
>1024 etc
>1024 opt
>3072 lib
> 
> => ls host 0:2 /boot
>1024 .
>1024 ..
>26909 imx6ul-opos6uldev.dtb
>   1 dtbs
>  5359984 opos6ul-linux.bin
> 
> => host load hostfs - 0 /tftproot/opos6ul-linux.bin
> 5359984 bytes read in 2 ms (2.5 GiB/s)
> 
> => printenv filesize
> filesize=51c970
> 
> => ext4write host 0:2 0 /boot/opos6ul-linux.bin ${filesize}
> File System is consistent
> file found, deleting
> update journal finished
> File System is consistent
> update journal finished
> Segmentation fault

As you can repeat this under sandbox, the next step would be to run sandbox 
under gdb, e.g.:
$> gdb --args sandbox -c "host bind 0 /tftproot/rootfs.raw ; host load hostfs 
- 0 /tftproot/opos6ul-linux.bin ; ext4write host 0:2 0 /boot/opos6ul-linux.bin 
${filesize}"

Kind regards,

Stefan
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] ext4: crash when writing a file

[Brüns , Stefan]

On Dienstag, 29. November 2016 14:10:54 CET Sébastien Szymanski wrote:
> 
> > Btw, which u-boot version are you using?
> 
> I first noticed the issue on U-Boot 2016.05 so I rebase on master from
> http://git.denx.de/u-boot.git
> 
> Regards,

That still doesn't make clear on which version you see this issue. 2016.05? 
Master? Which date/tag/hash?

U-Boot 2016.11 has received a huge number of fixes, and current master has 
some more.

Regards,

Stefan
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] ext4: crash when writing a file

[Brüns , Stefan]

On Dienstag, 29. November 2016 10:50:45 CET you wrote:
> Hello,
> 
> I am working on a i.MX6UL based board with a 4GB eMMC partitioned as
> following:
> 
> Device Boot   Start End Sectors  Size Id Type
> /dev/sdb1  2048  264191  262144  128M 83 Linux
> /dev/sdb2264192 4458495 41943042G 83 Linux
> /dev/sdb3   4458496 7634943 3176448  1.5G 83 Linux
> 
> On the 2nd partition, I write this ext4 filesystem file generated by
> Buildroot:
> Filesystem volume name:   "ROOTFS"
> Last mounted on:  
> Filesystem UUID:  b9833a36-e89d-429a-b120-c3b00bcb7785
> Filesystem magic number:  0xEF53
> Filesystem revision #:1 (dynamic)
> Filesystem features:  has_journal dir_index filetype extent
> sparse_super uninit_bg
> Filesystem flags: signed_directory_hash
> Default mount options:(none)
> Filesystem state: clean
> Errors behavior:  Unknown (continue)
> Filesystem OS type:   Linux
> Inode count:  3456
> Block count:  91756
91756 blocks ...

> Reserved block count: 4587
> Free blocks:  13458
> Free inodes:  488
> First block:  1
> Block size:   1024
1k each -> 91 MByte filesystem

> Fragment size:1024
> Blocks per group: 7648
> Fragments per group:  7648
> Inodes per group: 288
> Inode blocks per group:   36
> Last mount time:  n/a
> Last write time:  Tue Nov 29 09:44:52 2016
> Mount count:  0
> Maximum mount count:  -1
> Last checked: Tue Nov 29 09:44:52 2016
> Check interval:   0 ()
> Reserved blocks uid:  0 (user root)
> Reserved blocks gid:  0 (group root)
> First inode:  11
> Inode size:   128
> Journal inode:8
> Default directory hash:   half_md4
> Directory Hash Seed:  a583a07f-6b59-442d-8e08-9be305f78d17
> Journal backup:   inode blocks
> 
> This filesystem is written with the following commands:
> 
> BIOS> setexpr nbblocks ${filesize} / 0x200
> BIOS> setexpr nbblocks ${nbblocks} + 1
> BIOS> mmc write ${loadaddr} 0x40800 ${nbblocks}
> MMC write: dev # 0, block # 264192, count 183513 ... 183513 blocks
> written: OK
> 
> I can boot Linux with it without any issues, however if I try to write a
> file in it I get the following crash:
> 
> BIOS> ext4write mmc 0:2 ${loadaddr} /boot/${kernelimg} ${filesize}

What are you trying to achieve here? What is the value of $filesize?

Btw, which u-boot version are you using?

Regards,

Stefan

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v3 2/3] spl: dfu: move DFU Kconfig to SPL Kconfig

[Brüns , Stefan]

On Dienstag, 15. November 2016 13:02:45 CET Stefan Agner wrote:
> From: Stefan Agner 
> 
> diff --git a/common/spl/Kconfig b/common/spl/Kconfig
> index bb99f1f..54bcba3 100644
> --- a/common/spl/Kconfig
> +++ b/common/spl/Kconfig
> @@ -497,6 +497,32 @@ config SPL_USB_SUPPORT
> config options. This enables loading from USB using a configured
> device.
> 
> +config SPL_DFU_SUPPORT
> + bool "Support DFU (Device Firmware Upgarde)"
Typo: Upgrade

> + depends on SPL
> + select SPL_HASH_SUPPORT
> + help
> +   This feature enables the DFU (Device Firmware Upgarde) in SPL with
dito

> +   RAM memory device support. The ROM code will load and execute
> +   the SPL built with dfu. The user can load binaries (u-boot/kernel) to
> +   selected device partition from host-pc using dfu-utils.
a selected /or/ partitions

> +   This feature is useful to flash the binaries to factory or bare-metal
> +   boards using USB interface.
> +

Kind regards,

Stefan
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v3] drivers: usb: fsl-dt-fixup: Fix the dt for multiple USB nodes in single traversal of device tree

[Brüns , Stefan]

On Freitag, 28. Oktober 2016 01:52:26 CEST Marek Vasut wrote:
[...]
> > +
> > +enum fdt_fsl_usb_erratum {
> > +   A006261,
> > +   A007075,
> > +   A007792,
> > +   A005697,
> > +   A008751,
> > +   FSL_USB_ERRATUM_END
> 
> The compiler can assign completely arbitrary numbers to the enum
> elements. Moreover, you don't need this "terminating entry" anyway, see
> below.

Not true. According to 
http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf

6.7.2.2  Enumeration specifiers
...
An enumerator with = defines its enumeration constant as the value of the 
constant expression.  If the first enumerator has no =, the value of its 
enumeration constant is 0. Each subsequent enumerator with no = defines its 
enumeration constant as the value of the constant expression obtained by 
adding 1 to the value of the previous enumeration constant.

Kind regards,

Stefan
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] test/py: Fix exception, do not parametrize with empty set

[Brüns , Stefan]

On Montag, 17. Oktober 2016 12:11:26 CEST Stephen Warren wrote:
> On 10/16/2016 12:17 PM, Stefan Brüns wrote:
> > If the parameter set is empty, the pytest setup fails:
> > ---
> > 
> > call:  > ['env__dfu_config'], function test_dfu at [...]test_dfu.py:107>
> > 
> > ---
> > 
> > This aborts pytest_runtest_makereport and later leads to an exception
> > during the report generation, as the call to log.start_section(...)
> > is never executed:
> > ---
> > 
> > Exception: Block nesting mismatch:
> > "test_dfu[env__usb_dev_port0-env__dfu_config0]" ""
> > 
> > ---
> 
> How do you trigger this? I believe my test setup has many cases where
> the test you added would trigger, but without the issues you mention,
> but I'm not 100% sure since I don't know for sure what is causing this
> issue.
> 
> Which pytest version do you have? I appear to have 2.5.1 (on Ubuntu Trusty)

1. You need "dfu_configs" to be empty
2. Maybe its pytest version dependent, IIRC I have 3.0.2

The first error above can be seen when running pytest with "--debug", it is 
written to IIRC pytestdebug.log

Kind regards,

Stefan
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] git send-email and patch headers/subject line

[Brüns , Stefan]

On Montag, 3. Oktober 2016 17:02:37 CEST Stephen Arnold wrote:
> Howdy:
> 
> I could swear this worked the last time I sent patches to the OE list
> (at least it didn't need the gmail insecure app workaround so I guess
> it was a while ago).
> 
> Anyway, the real commit msg starts with ARM, I git format-patch and
> this time didn't touch the patches, then:
> 
> git send-email --to=t...@lists.denx.de --confirm=always -M -1
> --subject-prefix="U-Boot][PATCH v3"  outgoing/*

You should *not* add the [U-Boot] tag to the message, this is done by the 
mailinglist software.

The wiki states to use:
$> git format-patch --subject-prefix="PATCH v2"

Current git allows a simpler variant, "--reroll-count", or short "-v":
$> git format-patch -v 2


"git send-email" has no "--subject-prefix" nor "-M" nor "-" option. Just 
use
$> git send-email --to= --cc  outgoing/*

Formatting and sending patches are two independent steps. You can't apply/copy 
options from one command to the other.

Prior to sending the patch, you *should* edit the -cover-letter.patch.

Kind regards,

Stefan

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 2/6] efi_loader: Fix memory map size check to avoid out-of-bounds access

[Brüns , Stefan]

On Freitag, 30. September 2016 14:25:40 CEST Alexander Graf wrote:
> On 30.09.16 02:03, Stefan Brüns wrote:
> > memory_map_size as IN parameter specifies the size of the provided buffer.
> > If the buffer is to small, memory_map_size is updated to indicate the
> > required size, and an error code is returned.
> > 
> > Signed-off-by: Stefan Brüns 
> 
> This patch doesn't actually change anything, does it?

It does ...

> 
> Alex
> 
> > ---
> > 
> >  lib/efi_loader/efi_memory.c | 8 +---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> > index ebe8e94..5d71fdf 100644
> > --- a/lib/efi_loader/efi_memory.c
> > +++ b/lib/efi_loader/efi_memory.c
> > @@ -342,16 +342,18 @@ efi_status_t efi_get_memory_map(unsigned long
> > *memory_map_size,> 
> > map_size = map_entries * sizeof(struct efi_mem_desc);
> > 
> > -   *memory_map_size = map_size;

The caller provided buffer size was changed here

> > -
> > 
> > if (descriptor_size)
> > 
> > *descriptor_size = sizeof(struct efi_mem_desc);
> > 
> > if (descriptor_version)
> > 
> > *descriptor_version = EFI_MEMORY_DESCRIPTOR_VERSION;
> > 
> > -   if (*memory_map_size < map_size)

-> this check was always false

> > +   if (*memory_map_size < map_size) {
> > +   *memory_map_size = map_size;
> > 
> > return EFI_BUFFER_TOO_SMALL;
> > 
> > +   }
> > +
> > +   *memory_map_size = map_size;
> > 
> > /* Copy list into array */
> > if (memory_map) {

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 2/3] test/fs: Check ext4 behaviour if dirent is first entry in directory block

[Brüns , Stefan]

On Dienstag, 13. September 2016 12:33:15 CEST Stephen Warren wrote:
> On 09/12/2016 03:48 PM, Stefan Bruens wrote:
> > On Montag, 12. September 2016 12:39:35 CEST you wrote:
> >> On 09/11/2016 02:46 PM, Stefan Brüns wrote:
> >>> This is a regression test for a crash happening if the first dirent
> >>> in the block matches. Code tried to access a predecessor entry which
> >>> does not exist.
> >>> The crash happened for any block, but "." is always the first entry in
> >>> the first directory block and thus easy to check for.
> >>> 
> >>> diff --git a/test/fs/fs-test.sh b/test/fs/fs-test.sh
> >>> 
> >>> +# Next test case checks writing a file whose dirent
> >>> +# is the first in the block, which is always true for "."
> >>> +# The write should fail, but the lookup should work
> >>> +# Test Case 12 - Check directory traversal
> >>> +${PREFIX}${WRITE} host${SUFFIX} $addr ${FPATH}. 0x10
> >> 
> >> Doesn't that attempt to write a file named ".", which would end up
> >> over-writing the directory content? Unless I'm misunderstanding, that
> >> doesn't seem like a good idea.
> >> 
> >> Also, ${FPATH} might be "", "/", "something", or "something/". Appending
> >> "." to some of those won't work the same way as some others.
> > 
> > "something" can not happen.
> 
> I don't see any code that prevents that. I think it's fairer to say that
> nothing currently happens to call the function with FPATH with a
> trailing /. Someone could easily edit the script later and add such a
> call without knowing about the undocumented restriction.
> 
> I note that in patch 1, the following logic:
> 
> + if [ ! -z "$6" ]; then
> + FPATH=${6}/${FPATH}
>   fi
> 
> ... ends up with FPATH having a leading / for the second invocation of
> test_image by the main script body, since ${6} has the value
> `pwd`/$MOUNT_DIR. That seems to violate FAT's requirement for pathnames
> not to have a leading /.

There are 6 different test runs: { sb, fs, nonfs } x { fat, ext4 }

fs and nonfs can be treated the same way, its just e.g. "ext4ls", "ls" or 
"fatls", "ls" resp. Thats what I called "native" in the other mail.

Now in the "sb" cases, it does not matter if the mounted image is fat or ext4 
- it always uses the full absolute path to the mountpoint.

The specific requirements of ext4 (absolute path) and fat (relative path) only 
apply to the nonfs and fs test runs.

Kind regards,

Stefan

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 3/3] test/fs: Check writes using "." (same dir) relative path

[Brüns , Stefan]

On Dienstag, 13. September 2016 12:36:26 CEST you wrote:
> On 09/12/2016 04:04 PM, Stefan Bruens wrote:
> > On Montag, 12. September 2016 12:44:08 CEST you wrote:
> >> On 09/11/2016 02:46 PM, Stefan Brüns wrote:
> >>> / and /./ should reference the same file.
> >>> 
> >>> diff --git a/test/fs/fs-test.sh b/test/fs/fs-test.sh
> >>> 
> >>> +# Read 1MB from small file
> >>> +${PREFIX}load host${SUFFIX} $addr ${FPATH}$FILE_SMALL
> >> 
> >> I think the same issue with $FPATH ending/not-ending in / applies here
> >> too, and for all commands in this patch.
> > 
> > FPATH is either "" for native fat, "/" for native ext4, or / for
> > hostfs, so this is correct. Specifically, for fat, we dont want any "/" in
> > front of $FILE_foo.
> 
> I believe FPATH can be either "", "/" or "/foo/bar" here, due to the
> issue I just mentioned in the other email.

FPATH is either "", "/", "/foo/bar" + "/" or "/foo/bar" + "/" + "/". The last 
one is not to too nice, but still correct.

FPATH is *never* "/foo/bar", i.e. without trailing slash.

 
> >>> @@ -482,6 +499,16 @@ function check_results() {
> >>> 
> >>> + # Check directory traversal
> >>> + grep -A6 "Test Case 13a " "$1" | \
> >>> + egrep -q '1048576 bytes written|update journal'
> >> 
> >> Why is "update journal" considered successful? Surely the "n bytes
> >> written" message is always printed irrespective of whether anything
> >> journal-related happened?
> > 
> > Thats a question left to the author of Test Case 11, where the fragment
> > was
> > copied from.
> 
> I don't quite agree; you're adding the new test, so should make sure the
> validation code makes sense.

I did not claim this is correct. I just stated this problem exists already for 
the older test cases.
 
> > Ext4 unfortunately is quite verbose, it inserts "File system is
> > consistent"
> > and "update journal finished" lines in the output. I think these lines
> > where better stripped from the log prior to any further parsing.
> 
> ext4 may print some extra output, but that doesn't mean that any of it
> should be used in the validation. I think the simplest thing is to just
> ignore it in the validation code. Using egrep -q '1048576 bytes written'
> should do that just fine, and not get a false-positive if ext4 does say
> "update journal" without saying the required "1048576 bytes written".

No, this will not work for ext4 *and* fat.

Every check starts with a "grep -Axx 'Test Case n'" match. If "-Axx" is 
extended to *always* include the "yyy bytes written", then it will work for 
ext4, but it will also match "Test Case n \n failed \n Test Case n+1 \n yyy 
bytes written".

Kind Regards,

Stefan
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] ext4: fix possible crash on directory traversal, ignore deleted entries

[Brüns , Stefan]

On Freitag, 2. September 2016 10:53:08 CEST you wrote:
> > 
> > Adding this to the current test script is somewhat problematic. The test
> > runs all tests for fat and ext4, so each testcase should be file system
> > agnostic. Unfortunately fat and ext4 (at least as implemented in U-Boot)
> > have different semantics, as ext4 in U-Boot requires all path to absolute
> > paths, whereas fat seems to require something else (relative path?
> > absolute path, but without leading '/'?).
> > 
> > Calling 'fatwrite host 0 0 /. 0x10' happily creates a directory! called
> > '/.', 'fatwrite host 0 0 /./foo 0x10' creates a file and copletely messes
> > up the filesystem (according to fsck.vfat and mounting the fs in linux).
> > 
> > Any advise?
> 
> Can we fix this up in the argument parsing?  This sounds like it's
> showing some bugs in the fatwrite parsing code itself.

The fatwrite code is hardly doing any parsing at all. It does not strip any 
"/" or "\" characters, does not interpret these as dir delimiters, and just 
pushes whatever it gets into the directory.

For the lookup, it uses a function which is quite similar to the fatload/fatls 
function, but still different. It only traverses the root directory.

The whole fatwrite seems to be a 50% almost verbatim copy of the read 
implementation and shares hardly any code. The problem is the "almost" copy, 
most functions have minor differences.

I think lots of code could be removed from fatwrite if the read implementation 
where better structured, but e.g. the main entry point is a huge function 
which, depending on some flags either prints the directory listing while 
walking/traversing the tree, returns the file size, loads a specified file 
into a buffer, or errors out in case some path element was not reachable.

So, currently there are two options for the bad fatwrite behaviour:
a) add even more duplicate code to fatwrite
b) restructure fatread to be reusable

Opinions, please!

Kind regards,

Stefan
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 2/2] fs/fat: Optimizes memory size with single global variable instead of 3

[Brüns , Stefan]

On Sonntag, 14. August 2016 00:57:38 CEST Benoît Thébaudeau wrote:
> Hi,
> 
> On Tue, Aug 2, 2016 at 9:35 PM, Benoît Thébaudeau
> 
>  wrote:
> > On Tue, Aug 2, 2016 at 8:53 PM, Stephen Warren  
wrote:
> >> On 07/28/2016 12:11 AM, Tien Fong Chee wrote:
> >>> Single 64KB get_contents_vfatname_block global variable would be used
> >>> for
> >>> all FAT implementation instead of allocating additional two global
> >>> variables
> >>> which are get_denfromdir_block and do_fat_read_at_block. This
> >>> implementation
> >>> can help in saving up 128KB memory space.
> >> 
> >> The series,
> >> 
> >> Tested-by: Stephen Warren 
> >> (via DFU's FAT reading/writing on various Tegra; likely primarily FAT
> >> rather than VFAT though)
> >> 
> >> Reviewed-by: Stephen Warren 
> > 
> > I suspect that reading a filename with VFAT entries crossing a cluster
> > boundary in a FAT32 root directory will be broken by this series. I do
> > not have time to test this and other corner cases right now though,
> > but it will be possible in the next few weeks.
> 
> I have tested VFAT long filenames with the current implementation on
> Sandbox. It's completely broken:
>  - There is a length limit somewhere between 111 and 120 characters,
> instead of 256 characters. Beyond this limit, the files are invisible
> with ls.

That one is expected. U-Boot limits the extended name to 9 "slots", each 26 
bytes. As filenames are encoded as UTF-16/UCS-2, each ASCII character uses two 
bytes -> 9 * 26 / 2 = 117.

>  - Some filenames are truncated or mixed up between files. I have
> tested 111-character random filenames for 1000 empty files in the root
> directory. Most filenames had the expected length, but a few were
> shorter or longer.

Where there any filenames with characters outside the ASCII range? There may 
have been some double en-/decoding of UTF-8 vs UTF-16.

Kind regards,

Stefan
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] ext4 delete file fails when ext4 extents enabled in filesystem

[Brüns , Stefan]

On Freitag, 2. September 2016 14:51:59 CEST Thomas Schaefer wrote:
> > -Ursprüngliche Nachricht-
> > Von: Brüns, Stefan [mailto:stefan.bru...@rwth-aachen.de]
> > Gesendet: Freitag, 2. September 2016 13:43
> > An: Thomas Schaefer
> > Cc: u-boot@lists.denx.de; Michael Walle
> > Betreff: Re: ext4 delete file fails when ext4 extents enabled in
> > filesystem
> > 
> > On Donnerstag, 1. September 2016 19:25:30 CEST you wrote:
> > > On Donnerstag, 1. September 2016 16:08:51 CEST you wrote:
> > > > Hi Stefan,
> > > > 
> > > > applying patch [U-Boot,v4,06/13]ext4 and Michael Walles patch
> > > > [U-Boot,v4,3/4]ext4, I'm now able to write into directories on ext4
> > > > fs from u-boot. However, when deleting a given file (i.e. when
> > > > writing to an existing filename), u-boot crashes when ext4 extents
> > > > are enabled.
> > > > 
> > > > Some debugging showd that blknr from 'read_allocated_block' function
> > > > returns negative value. I can only guess, maybe its due to 64 bit
> > > > values calculated from ee_start_hi and ee_start_lo entries in the
> > > > ext4_extent structure.
> > > > 
> > > > When disabling extents in the ext4 fs, deleting a given file is
> > > > working.
[...]
> > Hi Thomas,
> > 
> > short followup:
> > 
> > read_allocated_blocks returns either 0 or -1 in case of an error.
> > Unfortunately, the return value is only checked for 0 equality in
> > most/all?
> > cases, and seemingly my patch series introduced some more occasions.
> > 
> > 
> > Now, what *should* read_allocated_blocks return in case of an error?
> > Either:
> > 
> > - 0: a file block can never be allocated as block 0, as that is always in
> > use by the superblock and/or the bootsector block.
> > 
> > - <0: Extents allow 48 bit block numbers. "Limiting" the return value to
> > the positive half of int64_t for valid block numbers and and reserving
> > negative values for error codes is fine.
> > 
> > I would go for negative error codes, as these are more expressive.
> > Comments/ opinions welcome!

Following up on this, the correct behaviour is <0 on "real" errors, like -
ENOMEM, and 0 on blocks not backed on device (e.g. sparse files).

Followup patch in progress.


[...]
> Hi Stefan,
> 
> the attachment contains an image file that causes u-boot to crash when
> trying to overwrite existing files in ext4 fs.

Could reproduce this. The problem seems to be an out-of-bound access of an in-
memory struct ext2_inode, and mixing up its size with fs->inodesz. I have 
found at least one place, will investigate further.

Kind regards,

Stefan


___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] ext4 delete file fails when ext4 extents enabled in filesystem

[Brüns , Stefan]

On Donnerstag, 1. September 2016 19:25:30 CEST you wrote:
> On Donnerstag, 1. September 2016 16:08:51 CEST you wrote:
> > Hi Stefan,
> > 
> > applying patch [U-Boot,v4,06/13]ext4 and Michael Walles patch
> > [U-Boot,v4,3/4]ext4, I'm now able to write into directories on ext4 fs
> > from
> > u-boot. However, when deleting a given file (i.e. when writing to an
> > existing filename), u-boot crashes when ext4 extents are enabled.
> > 
> > Some debugging showd that blknr from 'read_allocated_block' function
> > returns negative value. I can only guess, maybe its due to 64 bit values
> > calculated from ee_start_hi and ee_start_lo entries in the ext4_extent
> > structure.
> > 
> > When disabling extents in the ext4 fs, deleting a given file is working.
> 
> Hi Thomas,
> 
> U-boots ext4 implementation currently does not support 64bit or even 48bit
> block numbers, so this may be the cause.
> 
> Can you provide some information about your test setup?
> 
> You can use the debugsfs ext tool to gather some information about the
> problematic file. Just access the filesystem with:
> 
> /sbin/debugfs /dev/sda1  ;  (or whatever your partion name is)
> 
>   or
> 
> /sbin/debugfs /path/to/imagefile
> 
> debugfs supports commands like cd, stat, ls. stat gives you the block number
> list.
> 
> Kind regards,
> 
> Stefan

Hi Thomas,

short followup:

read_allocated_blocks returns either 0 or -1 in case of an error. 
Unfortunately, the return value is only checked for 0 equality in most/all? 
cases, and seemingly my patch series introduced some more occasions.


Now, what *should* read_allocated_blocks return in case of an error? Either:

- 0: a file block can never be allocated as block 0, as that is always in use 
by the superblock and/or the bootsector block.

- <0: Extents allow 48 bit block numbers. "Limiting" the return value to the 
positive half of int64_t for valid block numbers and and reserving negative 
values for error codes is fine.

I would go for negative error codes, as these are more expressive. Comments/
opinions welcome!


I will update the patch series for correct checking of read_allocated_blocks 
return values and fix all the other block number checks.

Anyway, it would be good to know why *exactly* read_allocated_blocks returns 
an error code in your case. Do you remember the exact negative value returned 
(there are -EINVAL and -ENOMEN, and many several unspecific uses of 0 and -1). 

Can you provide a disk image of the failing file system?

Kind regards,

Stefan
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [RFC PATCH 3/4] ext4: fix endianess problems in ext4 write support

[Brüns , Stefan]

On Dienstag, 16. August 2016 13:41:21 CEST you wrote:
> Hi Stefan,
> 
> Am 2016-08-14 03:50, schrieb Stefan Bruens:
> > On Freitag, 12. August 2016 15:16:20 CEST Michael Walle wrote:
> >> All fields were accessed directly instead of using the proper byte
> >> swap
> >> functions. Thus, ext4 write support was only usable on little-endian
> >> architectures. Fix this.
> >> 
> >> Signed-off-by: Michael Walle 
> > 
> > I have tested this on sandbox (x86_64), no regressions found. Some
> > remarks
> > below.
> > 
> > Reviewed-by: Stefan Brüns 
> > Tested-by: Stefan Brüns 
> 
> [snip]
> 
> >> @@ -2234,7 +2246,7 @@ int ext4fs_mount(unsigned part_length)
> >> 
> >> * and we do not support metadata_csum (and cannot reliably find
> >> * files when it is set.  Refuse to mount.
> >> */
> >> 
> >> -  if (data->sblock.feature_incompat & EXT4_FEATURE_INCOMPAT_64BIT) {
> >> +  if (le32_to_cpu(data->sblock.feature_incompat) &
> >> EXT4_FEATURE_INCOMPAT_64BIT) { printf("Unsupported feature found
> >> (64bit,
> >> possibly metadata_csum), not mounting\n"); goto fail;
> >> 
> >>}
> > 
> > This should have a if ((data->sblock.revision_level !=0) && ... in
> > front,
> > features are not defined for revision 0. Applies to other places as
> > well ...
> 
> are you sure about that? I can't find any code in the kernel where
> features are only valid if revision > 0. Also, I couldn't find anything
> in the ext4 documentation wiki:
> 
> https://ext4.wiki.kernel.org/index.php/Ext4_Disk_Layout

This document states that inode_size and all later fields are only defined for 
revision 1.

The statement seems to originate from here:
http://git.kernel.org/cgit/fs/ext2/e2fsprogs.git/tree/lib/ext2fs/
ext2_fs.h#n651

Now, if the remainder of the superblock after def_resgid is guaranteed to be 
zeroed the feature checks are ok for any revision, whereas a zero inode size 
would be bad and defaulting to 128 is needed.

As e2fsprogs is skipping any revision-level tests for feature checks, it seems 
to be ok to assume remaining fields to be zeroed.

Kind regards,

Stefan
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot