| Message ID | 20230228005135.1635726-1-jassisinghbrar@gmail.com |
|---|---|
| Headers | show |
| Series | FWU: Handle meta-data in common code | expand |
On Mon, Feb 27, 2023 at 06:51:35PM -0600, jassisinghbrar@gmail.com wrote: > From: Jassi Brar <jaswinder.singh@linaro.org> > > The patchset reduces ~400 lines of code, while keeping the functionality same and making > meta-data operations much faster (by using cached structures). > > Issue: > meta-data copies (primary and secondary) are being handled by the backend/storage layer > instead of the common core in fwu.c (as also noted by Ilias) that is, gpt_blk.c manages > meta-data and similarly raw_mtd.c will have to do the same when it arrives. The code > could by make smaller, cleaner and optimised. > > Basic idea: > Introduce .read_mdata() and .write_mdata() in fwu_mdata_ops that simply read/write > meta-data copy. The core code takes care of integrity and redundancy of the meta-data, > as a result we can get rid of every other callback .get_mdata() .update_mdata() > .get_mdata_part_num() .read_mdata_partition() .write_mdata_partition() and the > corresponding wrapper functions thereby making the code 100s of LOC smaller. > > Get rid of fwu_check_mdata_validity() and fwu_mdata_check() which expected underlying > layer to manage and verify mdata copies. > Implement fwu_get_verified_mdata(struct fwu_mdata *mdata) public function that reads, > verifies and, if needed, fixes the meta-data copies. > > Verified copy of meta-data is now cached as 'g_mdata' in fwu.c, which avoids multiple > low-level expensive read and parse calls. > gpt meta-data partition numbers are now cached in gpt_blk.c, so that we don't have to do expensive part_get_info() and uid ops. > > Changes since v4: > * Change fwu-mdata-mtd bindings to not require external changes > * Handle 'part == BOTH_PARTS' in fwu_sync_mdata > * use parts_ok[] and parts_mdata[] instead of pri/sec_ok and p/s_mdata Did you run this through CI / build sandbox? This doesn't read like you fixed the problem I reported in CI, when I was trying to merge v4.
On Mon, 27 Feb 2023 at 18:58, Tom Rini <trini@konsulko.com> wrote: > > On Mon, Feb 27, 2023 at 06:51:35PM -0600, jassisinghbrar@gmail.com wrote: > > > From: Jassi Brar <jaswinder.singh@linaro.org> > > > > The patchset reduces ~400 lines of code, while keeping the functionality same and making > > meta-data operations much faster (by using cached structures). > > > > Issue: > > meta-data copies (primary and secondary) are being handled by the backend/storage layer > > instead of the common core in fwu.c (as also noted by Ilias) that is, gpt_blk.c manages > > meta-data and similarly raw_mtd.c will have to do the same when it arrives. The code > > could by make smaller, cleaner and optimised. > > > > Basic idea: > > Introduce .read_mdata() and .write_mdata() in fwu_mdata_ops that simply read/write > > meta-data copy. The core code takes care of integrity and redundancy of the meta-data, > > as a result we can get rid of every other callback .get_mdata() .update_mdata() > > .get_mdata_part_num() .read_mdata_partition() .write_mdata_partition() and the > > corresponding wrapper functions thereby making the code 100s of LOC smaller. > > > > Get rid of fwu_check_mdata_validity() and fwu_mdata_check() which expected underlying > > layer to manage and verify mdata copies. > > Implement fwu_get_verified_mdata(struct fwu_mdata *mdata) public function that reads, > > verifies and, if needed, fixes the meta-data copies. > > > > Verified copy of meta-data is now cached as 'g_mdata' in fwu.c, which avoids multiple > > low-level expensive read and parse calls. > > gpt meta-data partition numbers are now cached in gpt_blk.c, so that we don't have to do expensive part_get_info() and uid ops. > > > > Changes since v4: > > * Change fwu-mdata-mtd bindings to not require external changes > > * Handle 'part == BOTH_PARTS' in fwu_sync_mdata > > * use parts_ok[] and parts_mdata[] instead of pri/sec_ok and p/s_mdata > > Did you run this through CI / build sandbox? This doesn't read like you > fixed the problem I reported in CI, when I was trying to merge v4. > I know that remains to be done. The dt-bindings for fwu-mdata is changed in this patchset and I thought any testcase may be impacted by it. thanks.
On Mon, Feb 27, 2023 at 07:00:10PM -0600, Jassi Brar wrote: > On Mon, 27 Feb 2023 at 18:58, Tom Rini <trini@konsulko.com> wrote: > > > > On Mon, Feb 27, 2023 at 06:51:35PM -0600, jassisinghbrar@gmail.com wrote: > > > > > From: Jassi Brar <jaswinder.singh@linaro.org> > > > > > > The patchset reduces ~400 lines of code, while keeping the functionality same and making > > > meta-data operations much faster (by using cached structures). > > > > > > Issue: > > > meta-data copies (primary and secondary) are being handled by the backend/storage layer > > > instead of the common core in fwu.c (as also noted by Ilias) that is, gpt_blk.c manages > > > meta-data and similarly raw_mtd.c will have to do the same when it arrives. The code > > > could by make smaller, cleaner and optimised. > > > > > > Basic idea: > > > Introduce .read_mdata() and .write_mdata() in fwu_mdata_ops that simply read/write > > > meta-data copy. The core code takes care of integrity and redundancy of the meta-data, > > > as a result we can get rid of every other callback .get_mdata() .update_mdata() > > > .get_mdata_part_num() .read_mdata_partition() .write_mdata_partition() and the > > > corresponding wrapper functions thereby making the code 100s of LOC smaller. > > > > > > Get rid of fwu_check_mdata_validity() and fwu_mdata_check() which expected underlying > > > layer to manage and verify mdata copies. > > > Implement fwu_get_verified_mdata(struct fwu_mdata *mdata) public function that reads, > > > verifies and, if needed, fixes the meta-data copies. > > > > > > Verified copy of meta-data is now cached as 'g_mdata' in fwu.c, which avoids multiple > > > low-level expensive read and parse calls. > > > gpt meta-data partition numbers are now cached in gpt_blk.c, so that we don't have to do expensive part_get_info() and uid ops. > > > > > > Changes since v4: > > > * Change fwu-mdata-mtd bindings to not require external changes > > > * Handle 'part == BOTH_PARTS' in fwu_sync_mdata > > > * use parts_ok[] and parts_mdata[] instead of pri/sec_ok and p/s_mdata > > > > Did you run this through CI / build sandbox? This doesn't read like you > > fixed the problem I reported in CI, when I was trying to merge v4. > > > I know that remains to be done. > The dt-bindings for fwu-mdata is changed in this patchset and I > thought any testcase may be impacted by it. So you're not expecting this iteration to be merged, as CI doesn't pass, and that's known? OK.
On Mon, Feb 27, 2023 at 7:28 PM Tom Rini <trini@konsulko.com> wrote: > > On Mon, Feb 27, 2023 at 07:00:10PM -0600, Jassi Brar wrote: > > On Mon, 27 Feb 2023 at 18:58, Tom Rini <trini@konsulko.com> wrote: > > > > > > On Mon, Feb 27, 2023 at 06:51:35PM -0600, jassisinghbrar@gmail.com wrote: > > > > > > > From: Jassi Brar <jaswinder.singh@linaro.org> > > > > > > > > The patchset reduces ~400 lines of code, while keeping the functionality same and making > > > > meta-data operations much faster (by using cached structures). > > > > > > > > Issue: > > > > meta-data copies (primary and secondary) are being handled by the backend/storage layer > > > > instead of the common core in fwu.c (as also noted by Ilias) that is, gpt_blk.c manages > > > > meta-data and similarly raw_mtd.c will have to do the same when it arrives. The code > > > > could by make smaller, cleaner and optimised. > > > > > > > > Basic idea: > > > > Introduce .read_mdata() and .write_mdata() in fwu_mdata_ops that simply read/write > > > > meta-data copy. The core code takes care of integrity and redundancy of the meta-data, > > > > as a result we can get rid of every other callback .get_mdata() .update_mdata() > > > > .get_mdata_part_num() .read_mdata_partition() .write_mdata_partition() and the > > > > corresponding wrapper functions thereby making the code 100s of LOC smaller. > > > > > > > > Get rid of fwu_check_mdata_validity() and fwu_mdata_check() which expected underlying > > > > layer to manage and verify mdata copies. > > > > Implement fwu_get_verified_mdata(struct fwu_mdata *mdata) public function that reads, > > > > verifies and, if needed, fixes the meta-data copies. > > > > > > > > Verified copy of meta-data is now cached as 'g_mdata' in fwu.c, which avoids multiple > > > > low-level expensive read and parse calls. > > > > gpt meta-data partition numbers are now cached in gpt_blk.c, so that we don't have to do expensive part_get_info() and uid ops. > > > > > > > > Changes since v4: > > > > * Change fwu-mdata-mtd bindings to not require external changes > > > > * Handle 'part == BOTH_PARTS' in fwu_sync_mdata > > > > * use parts_ok[] and parts_mdata[] instead of pri/sec_ok and p/s_mdata > > > > > > Did you run this through CI / build sandbox? This doesn't read like you > > > fixed the problem I reported in CI, when I was trying to merge v4. > > > > > I know that remains to be done. > > The dt-bindings for fwu-mdata is changed in this patchset and I > > thought any testcase may be impacted by it. > > So you're not expecting this iteration to be merged, as CI doesn't pass, > and that's known? OK. > I am more concerned if the bindings displease someone and I have to drop any code. If I get acks on all patches, the next revision will only add the test cases. thanks
On Mon, Feb 27, 2023 at 7:28 PM Tom Rini <trini@konsulko.com> wrote: > > On Mon, Feb 27, 2023 at 07:00:10PM -0600, Jassi Brar wrote: > > On Mon, 27 Feb 2023 at 18:58, Tom Rini <trini@konsulko.com> wrote: > > > > > > On Mon, Feb 27, 2023 at 06:51:35PM -0600, jassisinghbrar@gmail.com wrote: > > > > > > > From: Jassi Brar <jaswinder.singh@linaro.org> > > > > > > > > The patchset reduces ~400 lines of code, while keeping the functionality same and making > > > > meta-data operations much faster (by using cached structures). > > > > > > > > Issue: > > > > meta-data copies (primary and secondary) are being handled by the backend/storage layer > > > > instead of the common core in fwu.c (as also noted by Ilias) that is, gpt_blk.c manages > > > > meta-data and similarly raw_mtd.c will have to do the same when it arrives. The code > > > > could by make smaller, cleaner and optimised. > > > > > > > > Basic idea: > > > > Introduce .read_mdata() and .write_mdata() in fwu_mdata_ops that simply read/write > > > > meta-data copy. The core code takes care of integrity and redundancy of the meta-data, > > > > as a result we can get rid of every other callback .get_mdata() .update_mdata() > > > > .get_mdata_part_num() .read_mdata_partition() .write_mdata_partition() and the > > > > corresponding wrapper functions thereby making the code 100s of LOC smaller. > > > > > > > > Get rid of fwu_check_mdata_validity() and fwu_mdata_check() which expected underlying > > > > layer to manage and verify mdata copies. > > > > Implement fwu_get_verified_mdata(struct fwu_mdata *mdata) public function that reads, > > > > verifies and, if needed, fixes the meta-data copies. > > > > > > > > Verified copy of meta-data is now cached as 'g_mdata' in fwu.c, which avoids multiple > > > > low-level expensive read and parse calls. > > > > gpt meta-data partition numbers are now cached in gpt_blk.c, so that we don't have to do expensive part_get_info() and uid ops. > > > > > > > > Changes since v4: > > > > * Change fwu-mdata-mtd bindings to not require external changes > > > > * Handle 'part == BOTH_PARTS' in fwu_sync_mdata > > > > * use parts_ok[] and parts_mdata[] instead of pri/sec_ok and p/s_mdata > > > > > > Did you run this through CI / build sandbox? This doesn't read like you > > > fixed the problem I reported in CI, when I was trying to merge v4. > > > > > I know that remains to be done. > > The dt-bindings for fwu-mdata is changed in this patchset and I > > thought any testcase may be impacted by it. > > So you're not expecting this iteration to be merged, as CI doesn't pass, > and that's known? OK. > Sorry I got confused. I thought new test cases are expected to be written, whereas you only wanted the sandbox compilation breakage fixed. My replies must have sounded so stupid :( I have a patch to fix sandbox compilation in the v6 revision. thanks.
From: Jassi Brar <jaswinder.singh@linaro.org> The patchset reduces ~400 lines of code, while keeping the functionality same and making meta-data operations much faster (by using cached structures). Issue: meta-data copies (primary and secondary) are being handled by the backend/storage layer instead of the common core in fwu.c (as also noted by Ilias) that is, gpt_blk.c manages meta-data and similarly raw_mtd.c will have to do the same when it arrives. The code could by make smaller, cleaner and optimised. Basic idea: Introduce .read_mdata() and .write_mdata() in fwu_mdata_ops that simply read/write meta-data copy. The core code takes care of integrity and redundancy of the meta-data, as a result we can get rid of every other callback .get_mdata() .update_mdata() .get_mdata_part_num() .read_mdata_partition() .write_mdata_partition() and the corresponding wrapper functions thereby making the code 100s of LOC smaller. Get rid of fwu_check_mdata_validity() and fwu_mdata_check() which expected underlying layer to manage and verify mdata copies. Implement fwu_get_verified_mdata(struct fwu_mdata *mdata) public function that reads, verifies and, if needed, fixes the meta-data copies. Verified copy of meta-data is now cached as 'g_mdata' in fwu.c, which avoids multiple low-level expensive read and parse calls. gpt meta-data partition numbers are now cached in gpt_blk.c, so that we don't have to do expensive part_get_info() and uid ops. Changes since v4: * Change fwu-mdata-mtd bindings to not require external changes * Handle 'part == BOTH_PARTS' in fwu_sync_mdata * use parts_ok[] and parts_mdata[] instead of pri/sec_ok and p/s_mdata Changes since v3: * Fix error log wording * call fwu_write_mdata() with part & PRIMARY_PART ? true: false Changes since v2: * Drop whitespace changes * Fix missing mdata copy before return Changes since v1: * Fix typos and misc cosmetic changes * Catch error returns Jassi Brar (6): dt/bindings: fwu-mdata-mtd: drop changes outside FWU fwu: gpt: use cached meta-data partition numbers fwu: move meta-data management in core fwu: gpt: implement read_mdata and write_mdata callbacks fwu: meta-data: switch to management by common code fwu: rename fwu_get_verified_mdata to fwu_get_mdata cmd/fwu_mdata.c | 17 +- .../firmware/fwu-mdata-mtd.yaml | 105 ++++++- drivers/fwu-mdata/fwu-mdata-uclass.c | 151 +-------- drivers/fwu-mdata/gpt_blk.c | 175 +++-------- include/fwu.h | 198 ++---------- lib/fwu_updates/fwu.c | 295 +++++++----------- 6 files changed, 295 insertions(+), 646 deletions(-)