Hi Joursoir,
On Sun, 2022-04-10 at 14:41 +0300, Joursoir wrote:
Hello Thomas and Anastasia,
Due to current circumstances, Google isn't accepting participants from the country where I'm currently located. So, I can't take part in GSOC 2022. But I am still interested in participating in flashrom and this project. What should I do now? Send small patches and study the code until 20th May? And if no one will choose this project, then I can take part in it, right?
You're welcome to contribute to flashrom outside GSoC, too. The projects are not exclusive for GSoC contributors. Sending small patches is always a good starting point to get familiar with the contribution process. Also, when you contribute as open source developer and not through GSoC we will help you with this project.
So, about project details. I've understood everything, except one thing. Eventually, are we not going to remove shutdown() func from the struct spi_master and move it in struct programmer_entry? Will we keep shutdown() in the struct spi_master and not in the struct programmer_entry for SPI programmers?
For the first steps the shutdown function can be part of the bus master struct. We can discourse the further placement as part of the project. I'd made a suggestion for a possible solution in the mail sent earlier.
-- Thomas
-- Joursoir
On Sat, 9 Apr 2022 20:29:57 +1000 Anastasia Klimchuk aklm@chromium.org wrote:
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:
- Register a callback by its own
if (register_shutdown(serprog_shutdown, NULL)) goto init_err_cleanup_exit;
- 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 src@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:
- Register a callback by its own
if (register_shutdown(serprog_shutdown, NULL)) goto init_err_cleanup_exit;
- 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-leave@flashrom.org