| Message ID | 87v7sppmqu.wl-kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | lib/string_choices: Add new helpers | expand |
On Tue, Mar 4, 2025 at 4:12 AM Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote: > > > Hi Kees, Andy > > This is v2 patch-set of "add new helpers". First of all, please rebase your series against the top of the Kees' branch: https://web.git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=for-next/hardening. Also use --base when preparing the patch series. When you do that, you will note that the helpers now are ordered alphabetically (which your series doesn't follow). So, you will need the next version. But wait a bit (2-3 days) before sending it out to give a chance to others to comment on your series and samples. > I would like to use string_choices helper to cleanup the code, but it is > missing some of well used string pair in kernel. This patch-set adds it. > > Step1 > Add new helpers (This patch-set) > Step2 > Each driver/framework use new helper > > You can see "git diff --stat" of Step2 on last of this mail. > One note is that it is including the patch which is using only existing > helper (= not using new helper). The mentioned patch you can send as is separately. > I added sample patch of Step2 in this patch-set as [SAMPLE]. > One concern is that it adds "pass/fail", but we can find many similar > strings, like below. I have choiced "pass/fail" and use it on all cases > in my local branch (You can see some of them in [SAMPLE]. > I need your opinion > > passed / failed > succeed / failed > success / failed > successful / failed > succeeded / failed > worked / failed It looks like something with the 'successful' / 'succeeded' needs to be present. But in general here is the BIG NOTE: some of the strings may be used in ABI, which must not change them.
Hi Andy Thank you for the reply > First of all, please rebase your series against the top of the Kees' > branch: Also use --base when preparing the patch series. > > When you do that, you will note that the helpers now are ordered > alphabetically (which your series doesn't follow). So, you will need > the next version. But wait a bit (2-3 days) before sending it out to > give a chance to others to comment on your series and samples. OK, will post v3 in next week. > > passed / failed > > succeed / failed > > success / failed > > successful / failed > > succeeded / failed > > worked / failed > > It looks like something with the 'successful' / 'succeeded' needs to be > present. > > But in general here is the BIG NOTE: some of the strings may be used > in ABI, which must not change them. It is indeed one of my concern. Not only above, but some drivers are using capital letters at the beginning, but helper uses all lowercase... I agree we should not change string if it was used on dev_info() or something. But I think it can be more flexible if it was used on dev_dbg() ? not sure... I will indicate it (= the strings might be changed) on the patch in Step2, and follow each maintainer / reviewer opinion. Is it OK for you ? But hmm... I will double check the new helper, some of them might not needed if it was used only on dev_info()... Thank you for your help !! Best regards --- Kuninori Morimoto
On Tue, Mar 4, 2025 at 8:53 AM Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote: ... > > > passed / failed > > > succeed / failed > > > success / failed > > > successful / failed > > > succeeded / failed > > > worked / failed > > > > It looks like something with the 'successful' / 'succeeded' needs to be > > present. > > > > But in general here is the BIG NOTE: some of the strings may be used > > in ABI, which must not change them. > > It is indeed one of my concern. > Not only above, but some drivers are using capital letters at the beginning, > but helper uses all lowercase... > > I agree we should not change string if it was used on dev_info() or something. dev_*() are not the ABI. > But I think it can be more flexible if it was used on dev_dbg() ? not sure... All dev_*() and pr_*() and their derivatives may use these helpers without thinking of any breakages. On the contrary sysfs nodes, s*printf() when used to prepare strings to communicate with the user space and so on are all the ABIs. You need to study case-by-case. That's why I suggest you reduce this series to just helpers necessary for let say gpio-crystalcove and gpio-wcove drivers which I can review and ack and that's what will have all chances to go in this cycle. Then repeat for other helpers, no need to send tons of helpers with tons of conversion patches/samples. Do small steps at a time. That said, I would expect the series out of 3/4 patches 1) add str_in_out() and/or str_input_output(); 2) use it in gpio-crystalcove.c 3) use it in gpio-wcove.c This can be easily combined into an immutable tag/branch and pulled by the affected subsystems. With your lengthy series it will be ages to get it in. > I will indicate it (= the strings might be changed) on the patch in Step2, > and follow each maintainer / reviewer opinion. Is it OK for you ? > > But hmm... I will double check the new helper, some of them might not needed > if it was used only on dev_info()...
On Tue, Mar 04, 2025 at 02:12:41AM +0000, Kuninori Morimoto wrote: > I would like to use string_choices helper to cleanup the code, but it is > missing some of well used string pair in kernel. This patch-set adds it. > > Step1 > Add new helpers (This patch-set) > Step2 > Each driver/framework use new helper The Coccinelle rules are very easy to write. For example: @str_input_output@ expression E; @@ - (E ? "input" : "output") + str_input_output(E) @str_output_input@ expression E; @@ - (E ? "output" : "input") + str_output_input(E) Then running those will generate the treewide patches. I wrote and ran al of these, and the results for me were: str_in_out.patch: 66 files changed, 130 insertions(+), 136 deletions(-) str_tx_rx.patch: 33 files changed, 48 insertions(+), 53 deletions(-) str_input_output.patch: 17 files changed, 27 insertions(+), 29 deletions(-) str_enabling_disabling.patch: 16 files changed, 25 insertions(+), 25 deletions(-) str_Y_N.patch: 13 files changed, 98 insertions(+), 89 deletions(-) str_kernel_user.patch: 7 files changed, 8 insertions(+), 7 deletions(-) str_level_edge.patch: 5 files changed, 6 insertions(+), 6 deletions(-) str_to_from.patch: 3 files changed, 4 insertions(+), 4 deletions(-) str_pass_fail.patch: 2 files changed, 2 insertions(+), 2 deletions(-) str_attach_detach.patch: 1 file changed, 4 insertions(+), 4 deletions(-) So, IMO, the first 5 are clear winners. The others are probably fine, but pass/fail and attach/detach are really not used much. > One concern is that it adds "pass/fail", but we can find many similar > strings, like below. I have choiced "pass/fail" and use it on all cases > in my local branch (You can see some of them in [SAMPLE]. > I need your opinion > > passed / failed > succeed / failed > success / failed > successful / failed > succeeded / failed > worked / failed As Andy said, don't change any existing strings into something else. However, of the above 6 styles, I'd say find the most common and do those conversions. > 244 files changed, 604 insertions(+), 602 deletions(-) I only saw: 161 files changed, 352 insertions(+), 355 deletions(-) You have a lot in arch/ that I don't have, and when I spot-checked those files, I don't see what from this series you were converted? Perhaps you were doing more than those from this series (like "write"/"read")? -Kees
Hi Kees, Andy Thank you for your feedbacks > That said, I would expect the series out of 3/4 patches > 1) add str_in_out() and/or str_input_output(); > 2) use it in gpio-crystalcove.c > 3) use it in gpio-wcove.c > > This can be easily combined into an immutable tag/branch and pulled by > the affected subsystems. With your lengthy series it will be ages to > get it in. (snip) > str_in_out.patch: 66 files changed, 130 insertions(+), 136 deletions(-) > str_tx_rx.patch: 33 files changed, 48 insertions(+), 53 deletions(-) > str_input_output.patch: 17 files changed, 27 insertions(+), 29 deletions(-) > str_enabling_disabling.patch: 16 files changed, 25 insertions(+), 25 deletions(-) > str_Y_N.patch: 13 files changed, 98 insertions(+), 89 deletions(-) > str_kernel_user.patch: 7 files changed, 8 insertions(+), 7 deletions(-) > str_level_edge.patch: 5 files changed, 6 insertions(+), 6 deletions(-) > str_to_from.patch: 3 files changed, 4 insertions(+), 4 deletions(-) > str_pass_fail.patch: 2 files changed, 2 insertions(+), 2 deletions(-) > str_attach_detach.patch: 1 file changed, 4 insertions(+), 4 deletions(-) > > So, IMO, the first 5 are clear winners. The others are probably fine, > but pass/fail and attach/detach are really not used much. Thanks. My plan was add new helper 1st, and use helper (both new and exists) 2nd on each driver/framwork. But indeed it is not so good from reviewer point of view. And (aside from the "success/failed" topic) indeed some of new helper are not good/needed. I will try to post v3 patch, but want to cleanup before that. v3 will be above top 5 helpers only. Thank you for your help !! Best regards --- Kuninori Morimoto