10 Apr
2022
10 Apr
'22
4:41 a.m.
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 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:
> >
> > 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 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:
> > >
> > > 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.