Good day,
I am a student and very interested in flashrom. I fit the description of experienced C programmer. Open-source and low-level programming are my interests. I already have real experience with flashrom (CH341A programmer + chip "MX25L6436E") and want to become a GSoC 2022 contributor in this project.
I find the "Restructure Shutdown Function" project interestring to me. But right now it has no mentors. Maybe someone from the mailing can help me.
I have gone through sources and understand how things are now. The project description says "have a defined shutdown function in the programmer API". But we already have it. It's programmer_shutdown(). Or no?
The first and easiest thing that comes to my mind is to refuse using callbacks and use enums, thusly:
enum shutdown_func { SERPROG = 1 << 0, SPI = 1 << 1, ... };
int programmer_shutdown(enum shutdown_func execfunc) { ...
if(execfunc & SERPROG) ret |= serprog_shutdown(...);
if(execfunc & SPI) { /* the problem is that SPI programmers have own shutdown funcs, like dlc5_shutdown, byteblaster_shutdown, stlinkv3_spi_shutdown, so use struct spi_master, that contains shutdown callback */ ret |= spi_master_shutdown(...); }
... }
This system has disadvantages, but it'll make easier to track the code flow. What are the potential problems in this implementation?
What's with struct flashrom_programmer? Is not implemented yet?
I would appreciate any feedback.
-- Joursoir
Hi Joursoir,
The idea for the "Restructure Shutdown Functions" project is to extend the struct programmer_entry with a function to handle programmer related shutdowns. Currently, the init function of each programmer calls, mostly indirect, register_shutdown on each item separately to handle its shutdown later. In this project, the state tracking and shutdown of the programmer own data should be moved to each programmer.
E.g.: const struct programmer_entry example_programmer = { ... .init = example_init, .shutdown = example_shutdown, };
struct example_data { int reg_a; };
static int example_init(struct example_data* data) { ... // keep track of the original value data->reg_a = pci_read_word(dev, PCI_MAGIC_ADDR, PCI_MAGIC_VALUE); // write new value pci_write_word(dev, PCI_MAGIC_ADDR, 0x42); }
static int example_shutdown(struct example_data* data) { // restore the original value pci_write_word(dev, PCI_MAGIC_ADDR, data->reg_a); }
Feel free to contact me if you have further questions.
-- Thomas
On Fri, 2022-03-18 at 17:46 +0300, Joursoir wrote:
Good day,
I am a student and very interested in flashrom. I fit the description of experienced C programmer. Open-source and low-level programming are my interests. I already have real experience with flashrom (CH341A programmer + chip "MX25L6436E") and want to become a GSoC 2022 contributor in this project.
I find the "Restructure Shutdown Function" project interestring to me. But right now it has no mentors. Maybe someone from the mailing can help me.
I have gone through sources and understand how things are now. The project description says "have a defined shutdown function in the programmer API". But we already have it. It's programmer_shutdown(). Or no?
The first and easiest thing that comes to my mind is to refuse using callbacks and use enums, thusly:
enum shutdown_func { SERPROG = 1 << 0, SPI = 1 << 1, ... };
int programmer_shutdown(enum shutdown_func execfunc) { ...
if(execfunc & SERPROG) ret |= serprog_shutdown(...);
if(execfunc & SPI) { /* the problem is that SPI programmers have own shutdown funcs, like dlc5_shutdown, byteblaster_shutdown, stlinkv3_spi_shutdown, so use struct spi_master, that contains shutdown callback */ ret |= spi_master_shutdown(...); }
... }
This system has disadvantages, but it'll make easier to track the code flow. What are the potential problems in this implementation?
What's with struct flashrom_programmer? Is not implemented yet?
I would appreciate any feedback.
-- Joursoir _______________________________________________ flashrom mailing list -- flashrom@flashrom.org To unsubscribe send an email to flashrom-leave@flashrom.org
Hello Joursoir,
Nice to have you very interested in flashrom! One thing I wanted to say: it would be great to include the mailing list if you have discussions about the project. That would mean the community can join and potentially help too, and this is really useful.
Thomas, thank you so much for replying, this is awesome! :)
On Sat, Mar 19, 2022 at 2:43 AM Thomas Heijligen src@posteo.de wrote:
Hi Joursoir,
The idea for the "Restructure Shutdown Functions" project is to extend the struct programmer_entry with a function to handle programmer related shutdowns. Currently, the init function of each programmer calls, mostly indirect, register_shutdown on each item separately to handle its shutdown later. In this project, the state tracking and shutdown of the programmer own data should be moved to each programmer.
E.g.: const struct programmer_entry example_programmer = { ... .init = example_init, .shutdown = example_shutdown, };
struct example_data { int reg_a; };
static int example_init(struct example_data* data) { ... // keep track of the original value data->reg_a = pci_read_word(dev, PCI_MAGIC_ADDR, PCI_MAGIC_VALUE); // write new value pci_write_word(dev, PCI_MAGIC_ADDR, 0x42); }
static int example_shutdown(struct example_data* data) { // restore the original value pci_write_word(dev, PCI_MAGIC_ADDR, data->reg_a); }
Feel free to contact me if you have further questions.
-- Thomas
On Fri, 2022-03-18 at 17:46 +0300, Joursoir wrote:
Good day,
I am a student and very interested in flashrom. I fit the description of experienced C programmer. Open-source and low-level programming are my interests. I already have real experience with flashrom (CH341A programmer + chip "MX25L6436E") and want to become a GSoC 2022 contributor in this project.
I find the "Restructure Shutdown Function" project interestring to me. But right now it has no mentors. Maybe someone from the mailing can help me.
I have gone through sources and understand how things are now. The project description says "have a defined shutdown function in the programmer API". But we already have it. It's programmer_shutdown(). Or no?
The first and easiest thing that comes to my mind is to refuse using callbacks and use enums, thusly:
enum shutdown_func { SERPROG = 1 << 0, SPI = 1 << 1, ... };
int programmer_shutdown(enum shutdown_func execfunc) { ...
if(execfunc & SERPROG) ret |= serprog_shutdown(...); if(execfunc & SPI) { /* the problem is that SPI programmers have own shutdownfuncs, like dlc5_shutdown, byteblaster_shutdown, stlinkv3_spi_shutdown, so use struct spi_master, that contains shutdown callback */ ret |= spi_master_shutdown(...); }
...}
This system has disadvantages, but it'll make easier to track the code flow. What are the potential problems in this implementation?
What's with struct flashrom_programmer? Is not implemented yet?
I would appreciate any feedback.
-- Joursoir _______________________________________________ flashrom mailing list -- flashrom@flashrom.org To unsubscribe send an email to flashrom-leave@flashrom.org
flashrom mailing list -- flashrom@flashrom.org To unsubscribe send an email to flashrom-leave@flashrom.org
Hello Thomas and Anastasia,
Thank you for the quick answer. Now I understand the idea of the project more correctly. It really will be great if programmers have their own shutdown functions.
In your example you have used pci_write_*(), but the real flashrom code uses only rpci_write_* (it registers an undo-callback). Saving the data to some structure on every pci_write_*() is unnecessary complexity. So, we can continue to use rpci_write_*(), which will do work for us, but in a different way. The main problem is that register_shutdown() often occurs not in example_init(), but in its called funcs. Let's see on the example of internal programmer:
internal_init() -> chipset_flash_enable() -> -> chipset_enables[i].doit() -> enable_flash_vt823x()
The last one can contain 1 or more rpci_write_*(). I suggest not to register shutdown in rcpi_write_*(), but to fill a linked list with value that should be restored. It will be restored in example_shutdown().
Should global variables be used? In the case of an internal programmer, struct example_data will go a long way before data is written to it.
Hi Joursoir,
Beside the internal programmer, most programmer manipulate only a few things, so it is not that much. A data struct for each programmer should be used to store the context of the programmer instead of setting global variables. This gives the programmer a defined lifecycle . The shutdown() function should take care of all initialized items from the init() function. That can also be a pointer to memory mapped I/O, a pci_dev or libusb context.
When we take a simple programmer like the drkaiser pci programmer, there is a pci device, a pointer to memory mapped I/O and one register. This can easily be handled in a few lines.
* The internal programmer is some kind of special, since it was part of the flashrom core before other programmers exits. * How to implement certain aspects is then part of the project. * We try to reduce the amount of global variables. This project is part of it.
-- Thomas
On Sat, 2022-03-19 at 15:59 +0300, Joursoir wrote:
Hello Thomas and Anastasia,
Thank you for the quick answer. Now I understand the idea of the project more correctly. It really will be great if programmers have their own shutdown functions.
In your example you have used pci_write_*(), but the real flashrom code uses only rpci_write_* (it registers an undo-callback). Saving the data to some structure on every pci_write_*() is unnecessary complexity. So, we can continue to use rpci_write_*(), which will do work for us, but in a different way. The main problem is that register_shutdown() often occurs not in example_init(), but in its called funcs. Let's see on the example of internal programmer:
internal_init() -> chipset_flash_enable() -> -> chipset_enables[i].doit() -> enable_flash_vt823x()
The last one can contain 1 or more rpci_write_*(). I suggest not to register shutdown in rcpi_write_*(), but to fill a linked list with value that should be restored. It will be restored in example_shutdown().
Should global variables be used? In the case of an internal programmer, struct example_data will go a long way before data is written to it. _______________________________________________ flashrom mailing list -- flashrom@flashrom.org To unsubscribe send an email to flashrom-leave@flashrom.org
Hello Thomas,
I went ahead and started looking into shutdown functions. Almost of them use global variables, but I already have ideas on how to rewrite it. Now I start coding a prototype and want to implement struct example_data. But I have run into a problem with its initialization:
1) In theory, we can declare a static variable within each programmer's file. It would be convenient, but this method has a big disadvantage. We allocate private_data for every programmers but use only one.
static struct example_data { ... } private_data;
2) It's not possible to add a variable to struct programmer_entry because the structure is read only (structures in programmer.h are declared as const).
3) Use a static global variable in flashrom.c. Lesser of two evils principle as they say
static const struct programmer_entry *programmer = NULL; static const char *programmer_param = NULL; static void *programmer_data = NULL;
The next issue is the initialization of programmer_data:
a) Do it inside programmer->init(). The problem here is the duplication of programmer_data init code in each function.
b) Do it outside programmer->init(). The problem here is that we can't find out the size of example_data (it can be drkaised_data, it85spi_data and etc)
programmer_data = calloc(1, sizeof(EXAMPLE_DATA)); if (!data) { ... } ... ret = programmer->init(&programmer_data);
Perhaps there is some simpler solution, but I don't notice it.
Hi Joursoir,
I'm going to reply to your questions on Monday. Sorry for the delay, I'm currently not in reach of my Computer and want to look a few things up before answering.
-- Thomas
On 1 April 2022 20:43:10 WEST, Joursoir chat@joursoir.net wrote:
Hello Thomas,
I went ahead and started looking into shutdown functions. Almost of them use global variables, but I already have ideas on how to rewrite it. Now I start coding a prototype and want to implement struct example_data. But I have run into a problem with its initialization:
- In theory, we can declare a static variable within each programmer's
file. It would be convenient, but this method has a big disadvantage. We allocate private_data for every programmers but use only one.
static struct example_data { ... } private_data;
- It's not possible to add a variable to struct programmer_entry
because the structure is read only (structures in programmer.h are declared as const).
- Use a static global variable in flashrom.c. Lesser of two evils
principle as they say
static const struct programmer_entry *programmer = NULL; static const char *programmer_param = NULL; static void *programmer_data = NULL;
The next issue is the initialization of programmer_data:
a) Do it inside programmer->init(). The problem here is the duplication of programmer_data init code in each function.
b) Do it outside programmer->init(). The problem here is that we can't find out the size of example_data (it can be drkaised_data, it85spi_data and etc)
programmer_data = calloc(1, sizeof(EXAMPLE_DATA)); if (!data) { ... } ... ret = programmer->init(&programmer_data);
Perhaps there is some simpler solution, but I don't notice it.
Hi Joursoir,
It is awesome to have you interested in doing a project for flashrom! Please keep going! :)
A reminder from me, is that we want you to send a small patch if you want to get accepted for a project. Have a look at the guidelines https://www.flashrom.org/GSoC You can of course ask questions if you have any. Good luck!
On Sat, Apr 2, 2022 at 8:19 PM Thomas Heijligen src@posteo.de wrote:
Hi Joursoir,
I'm going to reply to your questions on Monday. Sorry for the delay, I'm currently not in reach of my Computer and want to look a few things up before answering.
-- Thomas
On 1 April 2022 20:43:10 WEST, Joursoir chat@joursoir.net wrote:
Hello Thomas,
I went ahead and started looking into shutdown functions. Almost of them use global variables, but I already have ideas on how to rewrite it. Now I start coding a prototype and want to implement struct example_data. But I have run into a problem with its initialization:
- In theory, we can declare a static variable within each programmer's
file. It would be convenient, but this method has a big disadvantage. We allocate private_data for every programmers but use only one.
static struct example_data { ... } private_data;
- It's not possible to add a variable to struct programmer_entry
because the structure is read only (structures in programmer.h are declared as const).
- Use a static global variable in flashrom.c. Lesser of two evils
principle as they say
static const struct programmer_entry *programmer = NULL; static const char *programmer_param = NULL; static void *programmer_data = NULL;
The next issue is the initialization of programmer_data:
a) Do it inside programmer->init(). The problem here is the duplication of programmer_data init code in each function.
b) Do it outside programmer->init(). The problem here is that we can't find out the size of example_data (it can be drkaised_data, it85spi_data and etc)
programmer_data = calloc(1, sizeof(EXAMPLE_DATA)); if (!data) { ... } ... ret = programmer->init(&programmer_data);
Perhaps there is some simpler solution, but I don't notice it.
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?
It is awesome to have you interested in doing a project for flashrom! Please keep going! :)
A reminder from me, is that we want you to send a small patch if you want to get accepted for a project. Have a look at the guidelines https://www.flashrom.org/GSoC You can of course ask questions if you have any. Good luck!
Thank you! I will do it as soon as I can
On Sat, 02 Apr 2022 09:19:05 +0000 Thomas Heijligen src@posteo.de wrote:
Hi Joursoir,
I'm going to reply to your questions on Monday. Sorry for the delay, I'm currently not in reach of my Computer and want to look a few things up before answering.
-- Thomas
On 1 April 2022 20:43:10 WEST, Joursoir chat@joursoir.net wrote:
Hello Thomas,
I went ahead and started looking into shutdown functions. Almost of them use global variables, but I already have ideas on how to rewrite it. Now I start coding a prototype and want to implement struct example_data. But I have run into a problem with its initialization:
- In theory, we can declare a static variable within each programmer's
file. It would be convenient, but this method has a big disadvantage. We allocate private_data for every programmers but use only one.
static struct example_data { ... } private_data;
- It's not possible to add a variable to struct programmer_entry
because the structure is read only (structures in programmer.h are declared as const).
- Use a static global variable in flashrom.c. Lesser of two evils
principle as they say
static const struct programmer_entry *programmer = NULL; static const char *programmer_param = NULL; static void *programmer_data = NULL;
The next issue is the initialization of programmer_data:
a) Do it inside programmer->init(). The problem here is the duplication of programmer_data init code in each function.
b) Do it outside programmer->init(). The problem here is that we can't find out the size of example_data (it can be drkaised_data, it85spi_data and etc)
programmer_data = calloc(1, sizeof(EXAMPLE_DATA)); if (!data) { ... } ... ret = programmer->init(&programmer_data);
Perhaps there is some simpler solution, but I don't notice it.
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
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
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?
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
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 chat@joursoir.net 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 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
On Sat, 2022-04-09 at 20:29 +1000, Anastasia Klimchuk wrote:
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! ;)
To register the shutdown function with the bus master is a good step and in any case part of the project. 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. This would make the programmers "API" declarative and move all manipulation of global structs, shutdown stack & bus master stack, out of the individual programmer into a common logic.
-- Thomas
Hi Joursoir
On Fri, 2022-04-01 at 22:43 +0300, Joursoir wrote:
Hello Thomas,
I went ahead and started looking into shutdown functions.
Gread that you've already looked so deep into the problem. Over that, plese don't forget the other importent tasks at https://www.flashrom.org/GSoC%C2%A0to make a successfull application for GSoC.
Almost of them use global variables, but I already have ideas on how to rewrite it. Now I start coding a prototype and want to implement struct example_data. But I have run into a problem with its initialization:
- In theory, we can declare a static variable within each
programmer's file. It would be convenient, but this method has a big disadvantage. We allocate private_data for every programmers but use only one.
Yes. Also this would keep the global state at the same place.
- It's not possible to add a variable to struct programmer_entry
because the structure is read only (structures in programmer.h are declared as const)
This could be possible. When converting `static const struct programmer_entry *programmer` from flashrom.c into `struct programmer_entry` and copying the function pointer from the selected progtrammer. Beside that, there is `flashrom_programmer` in the libflashrom api which is currently an unused placeholder. This could used to hold the programmer_entry and the data.
- Use a static global variable in flashrom.c. Lesser of two evils
principle as they say
static const struct programmer_entry *programmer = NULL; static const char *programmer_param = NULL; static void *programmer_data = NULL;
This is the easiest version and would also the variant I would begin with.
The next issue is the initialization of programmer_data:
a) Do it inside programmer->init(). The problem here is the duplication of programmer_data init code in each function.
b) Do it outside programmer->init(). The problem here is that we can't find out the size of example_data (it can be drkaised_data, it85spi_data and etc)
programmer_data = calloc(1, sizeof(EXAMPLE_DATA)); if (!data) { ... } ... ret = programmer->init(&programmer_data);
Since the programmer data is unique to each programmer it can only be initialized in that programmer init code. The location of the programmer data can be hand over to the init function. `programmer_init(void **data);` and the called like `programmer-
init(&programmer_data);
Perhaps there is some simpler solution, but I don't notice it.
I hope this answers your questions enough. Otherwise please ask again.
--Thomas
-
Anastasia Klimchuk -
Joursoir -
Thomas Heijligen