> 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?
Yes, it’s a good plan! You can be an independent contributor. This will give you more flexibility, since you won’t depend on a strict timeline. Sending small patch(es) in the beginning is very useful, you will create a gerrit account, learn how to push a patch, learn how to go through code review - all of these are skills that you need. You can do a project from Easy Projects. For the first one “Fix scan-build issues” some issues have patches under review at the moment, but there are still issues left. You can also send a small patch on something more close to the topic of the project. You can suggest one, or I can think of something. > 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? Yes, shutdown lives in spi_master and it will stay there (and also par_master and opaque_master). The reason is that the shutdown function needs data, and data lives in spi_master struct. Also data is something that is used instead of global state (so every programmer is using its own data, or will be). I was trying to find relevant discussions from last year, and I think comments threads on these two patches are: https://review.coreboot.org/c/flashrom/+/55932 https://review.coreboot.org/c/flashrom/+/56103 This is also relevant to what Thomas was saying: > What do you think of exposing the bus master(s) and the shutdown > function through the programmer_entry struct instead of register them > in the programmer code. On Sun, Apr 10, 2022 at 9:43 PM Joursoir <[email protected]> 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? > > 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 <[email protected]> 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 <[email protected]> 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 -- [email protected] > To unsubscribe send an email to [email protected] -- Anastasia. _______________________________________________ flashrom mailing list -- [email protected] To unsubscribe send an email to [email protected]
