Hi Marek, On Thu, 1 Apr 2021 at 12:19, Marek Vasut <[email protected]> wrote: > > On 4/1/21 12:50 AM, Simon Glass wrote: > > Hi Marek, > > > > On Thu, 1 Apr 2021 at 11:39, Marek Vasut <[email protected]> wrote: > >> > >> On 3/31/21 11:45 PM, Tom Rini wrote: > >>> On Wed, Mar 31, 2021 at 10:05:28PM +0200, Marek Vasut wrote: > >>>> On 3/31/21 9:40 PM, Tom Rini wrote: > >>>>> On Wed, Mar 31, 2021 at 03:26:27PM -0400, Tom Rini wrote: > >>>>>> On Wed, Mar 31, 2021 at 09:18:28PM +0200, Marek Vasut wrote: > >>>>>>> On 3/29/21 4:40 AM, Marek Vasut wrote: > >>>>>>>> On 3/29/21 4:09 AM, Marek Vasut wrote: > >>>>>>>>> On 3/29/21 3:59 AM, Tom Rini wrote: > >>>>>>>>>> On Mon, Mar 29, 2021 at 03:32:51AM +0200, Marek Vasut wrote: > >>>>>>>>>>> On 3/28/21 11:30 PM, Tom Rini wrote: > >>>>>>>>>>>> On Fri, Feb 26, 2021 at 03:21:24PM +0100, Marek Vasut wrote: > >>>>>>>>>>>> > >>>>>>>>>>>>> The spi_get_bus_and_cs() may be called on the same bus and > >>>>>>>>>>>>> chipselect > >>>>>>>>>>>>> with different frequency or mode. This is valid usecase, but > >>>>>>>>>>>>> the code > >>>>>>>>>>>>> fails to notify the controller of such a configuration change. > >>>>>>>>>>>>> Call > >>>>>>>>>>>>> spi_set_speed_mode() in case bus frequency or bus mode changed > >>>>>>>>>>>>> to let > >>>>>>>>>>>>> the controller update the configuration. > >>>>>>>>>>>>> > >>>>>>>>>>>>> The problem can easily be triggered using the sspi command: > >>>>>>>>>>>>> => sspi 0:0@1000 > >>>>>>>>>>>>> => sspi 0:0@2000 > >>>>>>>>>>>>> Without this patch, both transfers happen at 1000 > >>>>>>>>>>>>> Hz. With this patch, > >>>>>>>>>>>>> the later transfer happens correctly at 2000 Hz. > >>>>>>>>>>>>> > >>>>>>>>>>>>> Signed-off-by: Marek Vasut <[email protected]> > >>>>>>>>>>>>> Cc: Jagan Teki <[email protected]> > >>>>>>>>>>>>> Cc: Patrick Delaunay <[email protected]> > >>>>>>>>>>>> > >>>>>>>>>>>> So, very reliably I can make: > >>>>>>>>>>>> https://source.denx.de/u-boot/u-boot/-/jobs/245517 > >>>>>>>>>>>> happen locally as well building with clang. It's not > >>>>>>>>>>>> obvious to me why > >>>>>>>>>>>> the test now fails however. > >>>>>>>>>>> > >>>>>>>>>>> Can you please be more specific / clear ? I have no idea what > >>>>>>>>>>> those 300 > >>>>>>>>>>> lines of cryptic output mean, nor what are you trying to say > >>>>>>>>>>> by the above, > >>>>>>>>>>> sorry. > >>>>>>>>>> > >>>>>>>>>> If you build with clang, for sandbox, and run the tests, U-Boot > >>>>>>>>>> crashes > >>>>>>>>>> in the unit tests that you start with "ut dm". > >>>>>>>>> > >>>>>>>>> And that is related to this patch somehow ? How ? > >>>>>>>> > >>>>>>>> ... after further discussion off-list to get a better test case > >>>>>>>> description ... > >>>>>>>> > >>>>>>>> It seems the problem is in sound_beep() and it is unrelated to this > >>>>>>>> patch, as the same problem happens with / without this patch being > >>>>>>>> applied, on: > >>>>>>>> > >>>>>>>> a7d3ac61482 ("Merge branch '2021-03-28-assorted-bugfixes'") > >>>>>>>> > >>>>>>>> $ clang --version > >>>>>>>> Debian clang version 11.0.1-2 > >>>>>>>> ... > >>>>>>>> $ make CC=clang HOSTCC=clang sandbox_defconfig > >>>>>>>> $ make CC=clang HOSTCC=clang > >>>>>>>> $ gdb --args ./u-boot -d arch/sandbox/dts/test.dtb > >>>>>>>> => ut dm > >>>>>>>> ... > >>>>>>>> Program received signal SIGSEGV, Segmentation fault. > >>>>>>>> dlfree (mem=<optimized out>, mem@entry=0x15c19c50) at > >>>>>>>> common/dlmalloc.c:1623 > >>>>>>>> 1623 if (!(inuse_bit_at_offset(next, nextsz))) /* consolidate > >>>>>>>> forward */ > >>>>>>>> (gdb) bt > >>>>>>>> #0 dlfree (mem=<optimized out>, mem@entry=0x15c19c50) at > >>>>>>>> common/dlmalloc.c:1623 > >>>>>>>> #1 0x00000000004759e3 in sound_beep (dev=0x15c05580, > >>>>>>>> msecs=<optimized > >>>>>>>> out>, frequency_hz=100) > >>>>>>>> at drivers/sound/sound-uclass.c:118 > >>>>>>> > >>>>>>> Tom, any further comments ? It seems your claim that this bugfix is > >>>>>>> causing > >>>>>>> problems with clang is not correct, so I think it should still be > >>>>>>> applied to > >>>>>>> v2021.04 , unless you can provide more details about the clang > >>>>>>> problem ? > >>>>>> > >>>>>> There's at least a few funny things going on. Yes, apparently outside > >>>>>> of CI building sandbox with clang and running the tests manually > >>>>>> crashes > >>>>>> in sound, before it gets to the SPI tests. Except when it crashes on > >>>>>> the SPI tests instead, which I did get to happen once. So there's some > >>>>>> sort of use-after-free or similar bug around here somewhere, similar to > >>>>>> how we can't use the -fstack-protector patch as it exposes other real > >>>>>> problems that need to be fixed first. > >>>>>> > >>>>>> So, I'll see if with a new tag and a few other changes having been made > >>>>>> we once again get to the point where your patch doesn't trigger this > >>>>>> issue. But since it's also not a regression fix, if no one can figure > >>>>>> out what to do about fixing what clang shows us, then it can wait for > >>>>>> v2021.07. > >>>>> > >>>>> Nope, still gets things to blow up: > >>>>> https://source.denx.de/u-boot/u-boot/-/jobs/246848 > >>>>> > >>>>> and yes, it would be nice if when the reason a pytest fails is that > >>>>> U-Boot crashed, it would say something more clear than "OSError: [Errno > >>>>> 5] Input/output error" and you have to know that means U-Boot died. > >>>> > >>>> Can you please provide a detailed test case how to reproduce this ? > >>>> So far I don't have one, the only test description I got is "install > >>>> random > >>>> version of clang, run ut dm and it fails", but that kind of imprecise > >>>> test > >>>> description is really not useful. Please provide more specific > >>>> instructions. > >>> > >>> Yes, follow what CI does. It's done in a container, so you can have the > >>> exact same runtime and the tests are in .azure-pipelines.yml and > >>> .gitlab-ci.yml or if you look at > >>> https://source.denx.de/u-boot/u-boot/-/jobs/246848/raw you can see the > >>> commands in there. > >> > >> Can you please provide me the exact commands to run to reproduce this > >> problem ? I just spent a lot of time trying to find this one single > >> command in a wall of text, "ut dm spi_find", which I think is the one > >> failing for you ? It seems to work for me with and without the patch: > >> > >> $ ./u-boot -d arch/sandbox/dts/test.dtb > >> sandbox: starting... > >> > >> > >> U-Boot 2021.04-rc5-00005-g583b6858733 (Apr 01 2021 - 00:34:09 +0200) > >> > >> Model: sandbox > >> DRAM: 128 MiB > >> WDT: Started with servicing (60s timeout) > >> MMC: mmc2: 2 (SD), mmc1: 1 (SD), mmc0: 0 (SD) > >> In: serial > >> Out: vidconsole > >> Err: vidconsole > >> Model: sandbox > >> SCSI: > >> Net: eth0: eth@10002000, eth5: eth@10003000, eth3: sbe5, eth6: > >> eth@10004000 > >> Hit any key to stop autoboot: 0 > >> > >> Without patch > >> => ut dm spi_find > >> Test: dm_test_spi_find: spi.c > >> test/dm/spi.c:71, dm_test_spi_find(): 0 == spi_get_bus_and_cs(busnum, > >> cs, speed, mode, "jedec_spi_nor", "name", &bus, &slave): Expected 0x0 > >> (0), got 0xfffffffb (-5) > >> Test: dm_test_spi_find: spi.c (flat tree) > >> test/dm/spi.c:71, dm_test_spi_find(): 0 == spi_get_bus_and_cs(busnum, > >> cs, speed, mode, "jedec_spi_nor", "name", &bus, &slave): Expected 0x0 > >> (0), got 0xfffffffb (-5) > >> Failures: 2 > >> > >> With patch > >> => ut dm spi_find > >> Test: dm_test_spi_find: spi.c > >> test/dm/spi.c:71, dm_test_spi_find(): 0 == spi_get_bus_and_cs(busnum, > >> cs, speed, mode, "jedec_spi_nor", "name", &bus, &slave): Expected 0x0 > >> (0), got 0xfffffffb (-5) > >> Test: dm_test_spi_find: spi.c (flat tree) > >> test/dm/spi.c:71, dm_test_spi_find(): 0 == spi_get_bus_and_cs(busnum, > >> cs, speed, mode, "jedec_spi_nor", "name", &bus, &slave): Expected 0x0 > >> (0), got 0xfffffffb (-5) > >> Failures: 2 > > > > The Python test sets up a SPI-flash file (spi.bin) that is needed by the > > test. > > > > I use this alias: > > > > # Run a pytest on sandbox > > # $1: Name of test to run (optional, else run all) > > function pyt { > > test/py/test.py -B sandbox --build-dir /tmp/b/sandbox ${1:+"-k $1"} > > } > > > > then 'pyt spi_find' > > > > Once you've done that once you can do what you had before: > > > > ./u-boot -T > > Do you also have a minimal test which does not depend on pytest at all ? > A testcase for a bug should be minimal and as simple as possible, it > shouldn't depend on all kinds of variables which are only adding > complexity and confusion.
There is quite a bit more documentation languishing in the -next branch, e.g.: https://github.com/u-boot/u-boot/blob/next/doc/develop/tests_sandbox.rst The C SPI tests could perhaps be updated to automatically create the spi.bin file, but several of them share it, so it would need to be a shared function. I certainly only used pytest when needed, hence the substantial effort I made to document how to run sandbox tests directly, but I should point out that pytest is the overall framework that we use. Regards, Simon
