| Message ID | 20230102182532.2411125-1-jaswinder.singh@linaro.org |
|---|---|
| Headers | show |
| Series | FWU: Handle meta-data in common code | expand |
Hi, On 1/2/23 19:25, Jassi Brar wrote: > 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. First of all I have strong suspicious that this series are pretty much two series at once. You can look at https://lore.kernel.org/all/20230102182532.2411125-1-jaswinder.singh@linaro.org/#r Where two 3/5 patches are listed [PATCHv3 3/5] fwu: gpt: implement read_mdata and write_mdata callbacks [PATCHv3 3/5] dt: fwu: developerbox: enable fwu banks and mdata regions I think you are maybe updating the same cover letter with previous IDs. I would recommend you to use patman to handle this properly. The second issue is that you are sending patches from Jassi Brar <jassisinghbrar@gmail.com> but SOB is Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org> And Tom said in past that they should match. There is a hook for it to check it which everybody should be using. That's why please fix this in the next series. I will comment other things in the particular patch. Thanks, Michal
On Wed, Jan 18, 2023 at 7:28 AM Michal Simek <michal.simek@amd.com> wrote: > > Hi, > > On 1/2/23 19:25, Jassi Brar wrote: > > 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. > > First of all I have strong suspicious that this series are pretty much two > series at once. > Yes, I submitted two patchsets. 1) Optimizing the api of current fwu. 2) Introduce support for mtd backed storage (DeveloperBox platform as an instance) using the new api. They appear just fine in my inbox. Do they appear bad to you? > > The second issue is that you are sending patches from > Jassi Brar <jassisinghbrar@gmail.com> > but SOB is > Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org> > > And Tom said in past that they should match. There is a hook for it to check it > which everybody should be using. That's why please fix this in the next series. > I have submitted dozens of patches and pull requests over the last many years. This never occurred to anybody. BTW, the 'Author' and 'Signed-off-by' appear consistent in git log. And there are very recent instances in uboot git log where even they actually differ. But if Tom really wants, I am happy to send-email from my other account. Thanks.
On Wed, Jan 18, 2023 at 08:13:01AM -0600, Jassi Brar wrote: > On Wed, Jan 18, 2023 at 7:28 AM Michal Simek <michal.simek@amd.com> wrote: > > > > Hi, > > > > On 1/2/23 19:25, Jassi Brar wrote: > > > 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. > > > > First of all I have strong suspicious that this series are pretty much two > > series at once. > > > Yes, I submitted two patchsets. > 1) Optimizing the api of current fwu. > 2) Introduce support for mtd backed storage (DeveloperBox platform as > an instance) using the new api. > > They appear just fine in my inbox. Do they appear bad to you? > > > > > The second issue is that you are sending patches from > > Jassi Brar <jassisinghbrar@gmail.com> > > but SOB is > > Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org> > > > > And Tom said in past that they should match. There is a hook for it to check it > > which everybody should be using. That's why please fix this in the next series. > > > I have submitted dozens of patches and pull requests over the last > many years. This never occurred to anybody. > BTW, the 'Author' and 'Signed-off-by' appear consistent in git log. > And there are very recent instances in uboot git log where even they > actually differ. > > But if Tom really wants, I am happy to send-email from my other account. It doesn't matter what the email you send from is, but Author and Signed-off-by really really should match in the commit itself. It can be annoying, depending on corporate setup / policies, to have send-email work directly with your corporate account, but if you're allowed to be submitting patches, that's where the match between Author and Signed-off-by matters.
On 1/18/23 15:13, Jassi Brar wrote: > On Wed, Jan 18, 2023 at 7:28 AM Michal Simek <michal.simek@amd.com> wrote: >> >> Hi, >> >> On 1/2/23 19:25, Jassi Brar wrote: >>> 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. >> >> First of all I have strong suspicious that this series are pretty much two >> series at once. >> > Yes, I submitted two patchsets. > 1) Optimizing the api of current fwu. > 2) Introduce support for mtd backed storage (DeveloperBox platform as > an instance) using the new api. > > They appear just fine in my inbox. Do they appear bad to you? Take a look here. https://lore.kernel.org/all/20230102182532.2411125-1-jaswinder.singh@linaro.org/#r where you can see two series in the same thread. And this pretty much confuse b4. > >> >> The second issue is that you are sending patches from >> Jassi Brar <jassisinghbrar@gmail.com> >> but SOB is >> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org> >> >> And Tom said in past that they should match. There is a hook for it to check it >> which everybody should be using. That's why please fix this in the next series. >> > I have submitted dozens of patches and pull requests over the last > many years. This never occurred to anybody. It really depends how you download that patches. Thanks, Michal
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 v2: * Drop whitespace changes * Fix missing mdata copy before return Changes since v1: * Fix typos and misc cosmetic changes * Catch error returns Jassi Brar (5): 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 +- drivers/fwu-mdata/fwu-mdata-uclass.c | 151 +------------- drivers/fwu-mdata/gpt_blk.c | 175 +++++----------- include/fwu.h | 198 ++---------------- lib/fwu_updates/fwu.c | 301 ++++++++++++--------------- 5 files changed, 214 insertions(+), 628 deletions(-)