Re: [PATCH v2 2/2] console: Don't start/stop console if stdio device invalid
On Fri, Feb 05, 2021 at 09:31:00PM +0200, Andy Shevchenko wrote: > On Fri, Feb 05, 2021 at 01:07:02PM -0500, Tom Rini wrote: > > On Fri, Feb 05, 2021 at 12:50:56PM -0500, Tom Rini wrote: > > > On Fri, Feb 05, 2021 at 07:06:54PM +0200, Andy Shevchenko wrote: > > > > On Wed, Feb 03, 2021 at 11:50:53AM +0200, Andy Shevchenko wrote: > > ... > > > > > Okay, as promised, I prepared a branch [1] with new approach, but while > > > > I will > > > > be busy with other stuff, I would like you to test on real hardware and > > > > tell if > > > > it helps. At least it passes test cases. > > > > > > > > If it works, I would like to get a Tested-by tag and will prepare and > > > > submit > > > > the formal series. > > > > > > > > [1]: https://github.com/andy-shev/u-boot/tree/iomux > > > > > > I reliably get: > > It's one of the my patches revealed this, I reverted for now. Please, retest. pytest now passes, thanks. -- Tom signature.asc Description: PGP signature
Re: [PATCH v2 2/2] console: Don't start/stop console if stdio device invalid
On Fri, Feb 05, 2021 at 01:07:02PM -0500, Tom Rini wrote: > On Fri, Feb 05, 2021 at 12:50:56PM -0500, Tom Rini wrote: > > On Fri, Feb 05, 2021 at 07:06:54PM +0200, Andy Shevchenko wrote: > > > On Wed, Feb 03, 2021 at 11:50:53AM +0200, Andy Shevchenko wrote: ... > > > Okay, as promised, I prepared a branch [1] with new approach, but while I > > > will > > > be busy with other stuff, I would like you to test on real hardware and > > > tell if > > > it helps. At least it passes test cases. > > > > > > If it works, I would like to get a Tested-by tag and will prepare and > > > submit > > > the formal series. > > > > > > [1]: https://github.com/andy-shev/u-boot/tree/iomux > > > > I reliably get: It's one of the my patches revealed this, I reverted for now. Please, retest. -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 2/2] console: Don't start/stop console if stdio device invalid
On Fri, Feb 05, 2021 at 12:50:56PM -0500, Tom Rini wrote:
> On Fri, Feb 05, 2021 at 07:06:54PM +0200, Andy Shevchenko wrote:
> > On Wed, Feb 03, 2021 at 11:50:53AM +0200, Andy Shevchenko wrote:
> > > On Mon, Feb 1, 2021 at 9:29 PM Tom Rini wrote:
> > > >
> > > > On Fri, Jan 29, 2021 at 09:50:50AM +0100, Matthias Brugger wrote:
> > > > >
> > > > >
> > > > > On 28/01/2021 16:52, Andy Shevchenko wrote:
> > > > > > On Thu, Jan 28, 2021 at 02:12:40PM +0100, Nicolas Saenz Julienne
> > > > > > wrote:
> > > > > >> Don't start/stop an stdio device that might have been already
> > > > > >> freed.
> > > > > >>
> > > > > >> Signed-off-by: Nicolas Saenz Julienne
> > > > > >> Fixes: 70c2525c0d3c ("IOMUX: Stop dropped consoles")
> > > > > >
> > > > > > ...
> > > > > >
> > > > > >> + /*
> > > > > >> + * TODO: This is a workaround to avoid accessing freed memory:
> > > > > >> + * console_stop() might be called on an stdio_dev that has
> > > > > >> already been
> > > > > >> + * de-registered, due to the fact that stdio_deregister_dev()
> > > > > >> + * doesn't update the global console_devices array.
> > > > > >> + */
> > > > > >
> > > > > > I see now. I think I have experienced this issue from time to time.
> > > > > > I will look
> > > > > > at it. Tom, Simon, please hold on applying these for a while.
> > > > > >
> > > > >
> > > > > Just for the notes, the failing tests hold back Nicolas series to
> > > > > support
> > > > > RPi400/CM4 [1] as it does not run the new tests added successfully.
> > > > > If it takes
> > > > > a long time to fix the test environment, I'd vote to take this series
> > > > > as a
> > > > > stop-gap so that we can support that HW in the next release.
> > > >
> > > > Andy, since you're working on a better solution, do you want more time
> > > > for that or should I pick this series up for now and you can revert it
> > > > as part of your better fix? Thanks!
> > >
> > > Sorry for the delayed reply. Give me a couple of days, and if I will
> > > come up without any (good) solution, apply this series.
> >
> > Okay, as promised, I prepared a branch [1] with new approach, but while I
> > will
> > be busy with other stuff, I would like you to test on real hardware and
> > tell if
> > it helps. At least it passes test cases.
> >
> > If it works, I would like to get a Tested-by tag and will prepare and submit
> > the formal series.
> >
> > [1]: https://github.com/andy-shev/u-boot/tree/iomux
>
> I reliably get:
>
> == FAILURES
> ===
> _ test_ut[ut_dm_fdt_livetree_writing]
> _
>
> u_boot_console = 0x7fe251aeca90>
> ut_subtest = 'dm fdt_livetree_writing'
>
> def test_ut(u_boot_console, ut_subtest):
> """Execute a "ut" subtest.
>
> The subtests are collected in function generate_ut_subtest() from
> linker
> generated lists by applying a regular expression to the lines of file
> u-boot.sym. The list entries are created using the C macro
> UNIT_TEST().
>
> Strict naming conventions have to be followed to match the regular
> expression. Use UNIT_TEST(foo_test_bar, _flags, foo_test) for a test
> bar in
> test suite foo that can be executed via command 'ut foo bar' and is
> implemented in C function foo_test_bar().
>
> Args:
> u_boot_console (ConsoleBase): U-Boot console
> ut_subtest (str): test to be executed via command ut, e.g 'foo
> bar' to
> execute command 'ut foo bar'
> """
>
> output = u_boot_console.run_command('ut ' + ut_subtest)
> > assert output.endswith('Failures: 0')
> E AssertionError: assert False
> E+ where False = 0x7fe2516c5da0>('Failures: 0')
> E+where 0x7fe2516c5da0> = 'Test: dm_test_fdt_livetree_writing:
> test-fdt.c\r\r\ntest/dm/test-fdt.c:837, dm_test_fdt_livetree_writing():
> FDT_ADDR_...\r\nTest: dm_test_fdt_livetree_writing: test-fdt.c (flat
> tree)\r\r\nLive tree not active; ignore test\r\r\nFailures: 1'.endswith
>
> test/py/tests/test_ut.py:43: AssertionError
> Captured stdout call
> -
> => ut dm fdt_livetree_writing
> Test: dm_test_fdt_livetree_writing: test-fdt.c
> test/dm/test-fdt.c:837, dm_test_fdt_livetree_writing(): FDT_ADDR_T_NONE ==
> dev_read_addr(dev): Expected 0x (-1), got 0x42 (66)
> Test: dm_test_fdt_livetree_writing: test-fdt.c (flat tree)
> Live tree not active; ignore test
> Failures: 1
> =>
>
> On sandbox. I'm going to take sandbox out of my testing loop for the
> moment and see what Pi and a few others do.
As expected, the rest of the tests (which did pass in sandbox) also pass
on Pi 3 and a few other platforms I have here. Didn't test USB, etc,
etc, just pytest.
--
Tom
signature.asc
Description: PGP signature
Re: [PATCH v2 2/2] console: Don't start/stop console if stdio device invalid
On Fri, Feb 05, 2021 at 07:06:54PM +0200, Andy Shevchenko wrote:
> On Wed, Feb 03, 2021 at 11:50:53AM +0200, Andy Shevchenko wrote:
> > On Mon, Feb 1, 2021 at 9:29 PM Tom Rini wrote:
> > >
> > > On Fri, Jan 29, 2021 at 09:50:50AM +0100, Matthias Brugger wrote:
> > > >
> > > >
> > > > On 28/01/2021 16:52, Andy Shevchenko wrote:
> > > > > On Thu, Jan 28, 2021 at 02:12:40PM +0100, Nicolas Saenz Julienne
> > > > > wrote:
> > > > >> Don't start/stop an stdio device that might have been already freed.
> > > > >>
> > > > >> Signed-off-by: Nicolas Saenz Julienne
> > > > >> Fixes: 70c2525c0d3c ("IOMUX: Stop dropped consoles")
> > > > >
> > > > > ...
> > > > >
> > > > >> + /*
> > > > >> + * TODO: This is a workaround to avoid accessing freed memory:
> > > > >> + * console_stop() might be called on an stdio_dev that has
> > > > >> already been
> > > > >> + * de-registered, due to the fact that stdio_deregister_dev()
> > > > >> + * doesn't update the global console_devices array.
> > > > >> + */
> > > > >
> > > > > I see now. I think I have experienced this issue from time to time. I
> > > > > will look
> > > > > at it. Tom, Simon, please hold on applying these for a while.
> > > > >
> > > >
> > > > Just for the notes, the failing tests hold back Nicolas series to
> > > > support
> > > > RPi400/CM4 [1] as it does not run the new tests added successfully. If
> > > > it takes
> > > > a long time to fix the test environment, I'd vote to take this series
> > > > as a
> > > > stop-gap so that we can support that HW in the next release.
> > >
> > > Andy, since you're working on a better solution, do you want more time
> > > for that or should I pick this series up for now and you can revert it
> > > as part of your better fix? Thanks!
> >
> > Sorry for the delayed reply. Give me a couple of days, and if I will
> > come up without any (good) solution, apply this series.
>
> Okay, as promised, I prepared a branch [1] with new approach, but while I will
> be busy with other stuff, I would like you to test on real hardware and tell
> if
> it helps. At least it passes test cases.
>
> If it works, I would like to get a Tested-by tag and will prepare and submit
> the formal series.
>
> [1]: https://github.com/andy-shev/u-boot/tree/iomux
I reliably get:
== FAILURES
===
_ test_ut[ut_dm_fdt_livetree_writing]
_
u_boot_console =
ut_subtest = 'dm fdt_livetree_writing'
def test_ut(u_boot_console, ut_subtest):
"""Execute a "ut" subtest.
The subtests are collected in function generate_ut_subtest() from linker
generated lists by applying a regular expression to the lines of file
u-boot.sym. The list entries are created using the C macro UNIT_TEST().
Strict naming conventions have to be followed to match the regular
expression. Use UNIT_TEST(foo_test_bar, _flags, foo_test) for a test
bar in
test suite foo that can be executed via command 'ut foo bar' and is
implemented in C function foo_test_bar().
Args:
u_boot_console (ConsoleBase): U-Boot console
ut_subtest (str): test to be executed via command ut, e.g 'foo bar'
to
execute command 'ut foo bar'
"""
output = u_boot_console.run_command('ut ' + ut_subtest)
> assert output.endswith('Failures: 0')
E AssertionError: assert False
E+ where False = ('Failures: 0')
E+where
= 'Test: dm_test_fdt_livetree_writing: test-fdt.c\r\r\ntest/dm/test-fdt.c:837,
dm_test_fdt_livetree_writing(): FDT_ADDR_...\r\nTest:
dm_test_fdt_livetree_writing: test-fdt.c (flat tree)\r\r\nLive tree not active;
ignore test\r\r\nFailures: 1'.endswith
test/py/tests/test_ut.py:43: AssertionError
Captured stdout call
-
=> ut dm fdt_livetree_writing
Test: dm_test_fdt_livetree_writing: test-fdt.c
test/dm/test-fdt.c:837, dm_test_fdt_livetree_writing(): FDT_ADDR_T_NONE ==
dev_read_addr(dev): Expected 0x (-1), got 0x42 (66)
Test: dm_test_fdt_livetree_writing: test-fdt.c (flat tree)
Live tree not active; ignore test
Failures: 1
=>
On sandbox. I'm going to take sandbox out of my testing loop for the
moment and see what Pi and a few others do.
--
Tom
signature.asc
Description: PGP signature
Re: [PATCH v2 2/2] console: Don't start/stop console if stdio device invalid
On Wed, Feb 03, 2021 at 11:50:53AM +0200, Andy Shevchenko wrote:
> On Mon, Feb 1, 2021 at 9:29 PM Tom Rini wrote:
> >
> > On Fri, Jan 29, 2021 at 09:50:50AM +0100, Matthias Brugger wrote:
> > >
> > >
> > > On 28/01/2021 16:52, Andy Shevchenko wrote:
> > > > On Thu, Jan 28, 2021 at 02:12:40PM +0100, Nicolas Saenz Julienne wrote:
> > > >> Don't start/stop an stdio device that might have been already freed.
> > > >>
> > > >> Signed-off-by: Nicolas Saenz Julienne
> > > >> Fixes: 70c2525c0d3c ("IOMUX: Stop dropped consoles")
> > > >
> > > > ...
> > > >
> > > >> + /*
> > > >> + * TODO: This is a workaround to avoid accessing freed memory:
> > > >> + * console_stop() might be called on an stdio_dev that has already
> > > >> been
> > > >> + * de-registered, due to the fact that stdio_deregister_dev()
> > > >> + * doesn't update the global console_devices array.
> > > >> + */
> > > >
> > > > I see now. I think I have experienced this issue from time to time. I
> > > > will look
> > > > at it. Tom, Simon, please hold on applying these for a while.
> > > >
> > >
> > > Just for the notes, the failing tests hold back Nicolas series to support
> > > RPi400/CM4 [1] as it does not run the new tests added successfully. If it
> > > takes
> > > a long time to fix the test environment, I'd vote to take this series as a
> > > stop-gap so that we can support that HW in the next release.
> >
> > Andy, since you're working on a better solution, do you want more time
> > for that or should I pick this series up for now and you can revert it
> > as part of your better fix? Thanks!
>
> Sorry for the delayed reply. Give me a couple of days, and if I will
> come up without any (good) solution, apply this series.
Okay, as promised, I prepared a branch [1] with new approach, but while I will
be busy with other stuff, I would like you to test on real hardware and tell if
it helps. At least it passes test cases.
If it works, I would like to get a Tested-by tag and will prepare and submit
the formal series.
[1]: https://github.com/andy-shev/u-boot/tree/iomux
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v2 2/2] console: Don't start/stop console if stdio device invalid
On Mon, Feb 1, 2021 at 9:29 PM Tom Rini wrote:
>
> On Fri, Jan 29, 2021 at 09:50:50AM +0100, Matthias Brugger wrote:
> >
> >
> > On 28/01/2021 16:52, Andy Shevchenko wrote:
> > > On Thu, Jan 28, 2021 at 02:12:40PM +0100, Nicolas Saenz Julienne wrote:
> > >> Don't start/stop an stdio device that might have been already freed.
> > >>
> > >> Signed-off-by: Nicolas Saenz Julienne
> > >> Fixes: 70c2525c0d3c ("IOMUX: Stop dropped consoles")
> > >
> > > ...
> > >
> > >> + /*
> > >> + * TODO: This is a workaround to avoid accessing freed memory:
> > >> + * console_stop() might be called on an stdio_dev that has already
> > >> been
> > >> + * de-registered, due to the fact that stdio_deregister_dev()
> > >> + * doesn't update the global console_devices array.
> > >> + */
> > >
> > > I see now. I think I have experienced this issue from time to time. I
> > > will look
> > > at it. Tom, Simon, please hold on applying these for a while.
> > >
> >
> > Just for the notes, the failing tests hold back Nicolas series to support
> > RPi400/CM4 [1] as it does not run the new tests added successfully. If it
> > takes
> > a long time to fix the test environment, I'd vote to take this series as a
> > stop-gap so that we can support that HW in the next release.
>
> Andy, since you're working on a better solution, do you want more time
> for that or should I pick this series up for now and you can revert it
> as part of your better fix? Thanks!
Sorry for the delayed reply. Give me a couple of days, and if I will
come up without any (good) solution, apply this series.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v2 2/2] console: Don't start/stop console if stdio device invalid
Hi Nicolas,
On Thu, 28 Jan 2021 at 06:12, Nicolas Saenz Julienne
wrote:
>
> Don't start/stop an stdio device that might have been already freed.
>
> Signed-off-by: Nicolas Saenz Julienne
> Fixes: 70c2525c0d3c ("IOMUX: Stop dropped consoles")
>
> ---
> Changes since v1:
> - Add comment stating this should be properly fixed
>
> common/console.c | 9 +
> 1 file changed, 9 insertions(+)
>
Reviewed-by: Simon Glass
Since this says it is a stopgap, when does the real fix come?
Regards,
Simon
Re: [PATCH v2 2/2] console: Don't start/stop console if stdio device invalid
On Fri, Jan 29, 2021 at 09:50:50AM +0100, Matthias Brugger wrote:
>
>
> On 28/01/2021 16:52, Andy Shevchenko wrote:
> > On Thu, Jan 28, 2021 at 02:12:40PM +0100, Nicolas Saenz Julienne wrote:
> >> Don't start/stop an stdio device that might have been already freed.
> >>
> >> Signed-off-by: Nicolas Saenz Julienne
> >> Fixes: 70c2525c0d3c ("IOMUX: Stop dropped consoles")
> >
> > ...
> >
> >> + /*
> >> + * TODO: This is a workaround to avoid accessing freed memory:
> >> + * console_stop() might be called on an stdio_dev that has already been
> >> + * de-registered, due to the fact that stdio_deregister_dev()
> >> + * doesn't update the global console_devices array.
> >> + */
> >
> > I see now. I think I have experienced this issue from time to time. I will
> > look
> > at it. Tom, Simon, please hold on applying these for a while.
> >
>
> Just for the notes, the failing tests hold back Nicolas series to support
> RPi400/CM4 [1] as it does not run the new tests added successfully. If it
> takes
> a long time to fix the test environment, I'd vote to take this series as a
> stop-gap so that we can support that HW in the next release.
Andy, since you're working on a better solution, do you want more time
for that or should I pick this series up for now and you can revert it
as part of your better fix? Thanks!
--
Tom
signature.asc
Description: PGP signature
Re: [PATCH v2 2/2] console: Don't start/stop console if stdio device invalid
On 28/01/2021 16:52, Andy Shevchenko wrote:
> On Thu, Jan 28, 2021 at 02:12:40PM +0100, Nicolas Saenz Julienne wrote:
>> Don't start/stop an stdio device that might have been already freed.
>>
>> Signed-off-by: Nicolas Saenz Julienne
>> Fixes: 70c2525c0d3c ("IOMUX: Stop dropped consoles")
>
> ...
>
>> +/*
>> + * TODO: This is a workaround to avoid accessing freed memory:
>> + * console_stop() might be called on an stdio_dev that has already been
>> + * de-registered, due to the fact that stdio_deregister_dev()
>> + * doesn't update the global console_devices array.
>> + */
>
> I see now. I think I have experienced this issue from time to time. I will
> look
> at it. Tom, Simon, please hold on applying these for a while.
>
Just for the notes, the failing tests hold back Nicolas series to support
RPi400/CM4 [1] as it does not run the new tests added successfully. If it takes
a long time to fix the test environment, I'd vote to take this series as a
stop-gap so that we can support that HW in the next release.
Regards,
Matthias
[1] https://patchwork.ozlabs.org/project/uboot/list/?series=223890
Re: [PATCH v2 2/2] console: Don't start/stop console if stdio device invalid
On Thu, Jan 28, 2021 at 02:12:40PM +0100, Nicolas Saenz Julienne wrote:
> Don't start/stop an stdio device that might have been already freed.
>
> Signed-off-by: Nicolas Saenz Julienne
> Fixes: 70c2525c0d3c ("IOMUX: Stop dropped consoles")
...
> + /*
> + * TODO: This is a workaround to avoid accessing freed memory:
> + * console_stop() might be called on an stdio_dev that has already been
> + * de-registered, due to the fact that stdio_deregister_dev()
> + * doesn't update the global console_devices array.
> + */
I see now. I think I have experienced this issue from time to time. I will look
at it. Tom, Simon, please hold on applying these for a while.
> + if (!stdio_valid(sdev))
> + return false;
--
With Best Regards,
Andy Shevchenko
