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?
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? -- Joursoir On Sat, 9 Apr 2022 20:29:57 +1000 Anastasia Klimchuk <a...@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: > > > > 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