Hi Joursoir,
Sorry for some delay, I wanted to respond more to the topic and
finally found time :)
> As I understood, they use the following approaches:
>
> 1. Register a callback by its own
>
> if (register_shutdown(serprog_shutdown, NULL))
> goto init_err_cleanup_exit;
>
> 2. Fill `struct spi_master` shutdown-callback
>
> static struct spi_master spi_master_dediprog = {
> ...
> .shutdown = dediprog_shutdown,
> };
>
> Is there any significant difference in this approaches? Or does it just
> depend on code flow?
As a long-term goal, we are replacing all #1 with #2. Lots of #1 cases
were migrated to #2 last year, however some of the occurrences of #2
still left. They need to be migrated too. So if you see a programmer
which is calling `register_shutdown` in its code, then it needs to be
migrated, and I would count this as a task in scope of the
“restructure shutdown functions” gsoc project.
Some background why #2 is preferred and we are migrating into it:
Every master needs to register itself at the end of init function. For
example for spi masters it is `register_spi_master`. You can see this
function is called at the end of initialisation.
As a part of registration, a master needs to register its shutdown function.
Now, #1 means registering shutdown function and registering the master
itself are two different function calls, and every master needs to
take care of two calls. Which means, you can call one registration and
forget the other, you can call them in different order, you can have
an error and fail in between calls, and leak resources… In other
words, #1 means there are many ways to use the registration API (too
many), and most of them are wrong ways :( Very common problem was
leaking memory and other resources on error paths.
#2 approach means there is only one call to do: register master.
Registering shutdown function is done inside it. #2 does not solve all
possible problems, but it does solve some of them. It is less
error-prone, and that’s important.
Summary: I would count converting all remaining cases of #1 into #2 as
a part of the project.
> I went ahead and started looking into shutdown functions. Almost of
> them use global variables
They shouldn’t use global variables. SImilarly to my previous point,
lots of masters were migrated last year not to use global variables.
Shutdown function has an argument `data` and it should use it.
In the project ideas list, there is a separate idea “remove global
state”. But as Thomas mentioned, it is very related to “restructure
shutdown functions”, and yes it is. In reality, that would be a
project that has both goals touched. But that’s fine, that’s even more
fun :)
More information on the topic.
Given that the main side-effect of shutdown functions issues is memory
leak and other resources leak, there is another idea. I have to admit,
idea is currently exists in my head but still:
At the moment every master has to do its own memory management,
allocate and free memory for its own state. Memory management can be
also moved under API, so that master cannot “forget” to free
resources.
Another way to enforce memory management is to write lifecycle unit
tests for all programmers. Unit tests are all configured with memory
checks, so if you run a scenario “initialise programmer -> shutdown
programmer” and memory leaks after that, then the test will fail.
There is a gsoc project idea for unit tests :) As a fact from flashrom
history, the very first lifecycle unit test I wrote found a memory
leak!
And the last thing, there is an idea, an open question at the moment,
whether it should be required for master to have a shutdown function
(it is not required at the moment). If we decide positive on that,
there is work to do to make it possible to have shutdown function
required.
I think that’s all that I wanted to say. But I am wondering what
Thomas is thinking about it?
In any case there is plenty of useful stuff that can be done, and it
is not a problem to wrap it as a project.
Let us know what you think! ;)
On Wed, Apr 6, 2022 at 4:24 AM Thomas Heijligen <s...@posteo.de> wrote:
>
> Hi Joursoir,
>
> On Mon, 2022-04-04 at 14:57 +0300, Joursoir wrote:
> > Hello Thomas,
> >
> > No problem, thanks for your reply. I have one more question. I have
> > noticed
> > that some programmers already have their own context (for example
> > pony_spi.c,
> > rayer_spi.c. serprog.c, dediprog.c and others). I note that they are
> > all
> > SPI. As I understood, they use the following approaches:
> >
> > 1. Register a callback by its own
> >
> > if (register_shutdown(serprog_shutdown, NULL))
> > goto init_err_cleanup_exit;
> >
> > 2. Fill `struct spi_master` shutdown-callback
> >
> > static struct spi_master spi_master_dediprog = {
> > ...
> > .shutdown = dediprog_shutdown,
> > };
> >
> > Is there any significant difference in this approaches? Or does it
> > just
> > depend on code flow?
> >
> The goal is the same. To reduce the global state and the number of
> calls to register_something. The differnt is the way how to use the
> shutdown stack. The shutdown function as part of the programmer struct
> is a more declarative approch. when working this further, the bus
> master could also be declared in the programmer struct. But that's a
> topic for a different project.
>
> Anastasia may give you more details about the shutdown function in the
> *_master struct.
>
> -- Thomas
--
Anastasia.
_______________________________________________
flashrom mailing list -- flashrom@flashrom.org
To unsubscribe send an email to flashrom-le...@flashrom.org
