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 <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
_______________________________________________
flashrom mailing list -- flashrom@flashrom.org
To unsubscribe send an email to flashrom-le...@flashrom.org
