Re: [PATCH v3 00/30] staging: rtl8723bs: remove RT_TRACE logs in core/*
On Sat, Apr 03, 2021 at 01:02:04PM -0700, Joe Perches wrote: > On Sat, 2021-04-03 at 19:28 +0200, Fabio Aiuto wrote: > > On Sat, Apr 03, 2021 at 09:17:37AM -0700, Joe Perches wrote: > > > On Sat, 2021-04-03 at 17:21 +0200, Fabio Aiuto wrote: > > > > On Sat, Apr 03, 2021 at 08:02:25AM -0700, Joe Perches wrote: > > > > > On Sat, 2021-04-03 at 11:13 +0200, Fabio Aiuto wrote: > > > > > > This patchset removes all RT_TRACE usages in core/ files. > > > > > > > > > > and hal and include and os_dep > > > > > > > > Hi, > > > > > > > > I was just about to send the second patchset relative to hal/ files. > > > > The whole has been split up in directories in order to reduce the > > > > number of patch per patchset > > > > > > > It's a good idea, but the patches relative to RT_TRACE removal > > > > could be huge > > > > > > That's really not a significant issue. > > > Simplicity in review is also important. > > > Mechanization of patch creation can reduce review efforts. > > > > Maybe I wrongly associated simplicity with patch dimensions, but maybe > > for patches this simple have expert reviewers some tool for > > automatic review? > > Coccinelle is a relatively trusted tool and using it as a scripting > mechanism where the script is shown as part of the commit message > gives confidence that the change it produces can be applied without > significant doubt. > > To improve confidence that any change that does not have an output > object code delta, comparing the object code produced before and > after the change is useful. Showing that the code has been both > compiled and compared in the commit message also improves confidence > that the change is useful and can be applied. > > which tool provides an object code delta output suitable for a commit message? I dare a proposal apologizing in advance if this question should sound off topic, if it is please tell me how to ask it, is anyone of you all available being my mentor? I realized that what you told me in last weeks helps me a lot, and maybe a mentor can address me working towards more useful code bases. I choosed rtl8723bs quite by random, just to make experience in cleaning up code, and get more confident with required dev tools, but if there is some other task with higher priority that I could carry on, I will be glad to follow directives of a mentor. I really would like to become an expert kernel developer, and the way community works gets me more and more involved. Thank you again to all people helping me, fabio
Re: [PATCH v3 00/30] staging: rtl8723bs: remove RT_TRACE logs in core/*
On Sat, 2021-04-03 at 19:28 +0200, Fabio Aiuto wrote: > On Sat, Apr 03, 2021 at 09:17:37AM -0700, Joe Perches wrote: > > On Sat, 2021-04-03 at 17:21 +0200, Fabio Aiuto wrote: > > > On Sat, Apr 03, 2021 at 08:02:25AM -0700, Joe Perches wrote: > > > > On Sat, 2021-04-03 at 11:13 +0200, Fabio Aiuto wrote: > > > > > This patchset removes all RT_TRACE usages in core/ files. > > > > > > > > and hal and include and os_dep > > > > > > Hi, > > > > > > I was just about to send the second patchset relative to hal/ files. > > > The whole has been split up in directories in order to reduce the > > > number of patch per patchset > > > > > It's a good idea, but the patches relative to RT_TRACE removal > > > could be huge > > > > That's really not a significant issue. > > Simplicity in review is also important. > > Mechanization of patch creation can reduce review efforts. > > Maybe I wrongly associated simplicity with patch dimensions, but maybe > for patches this simple have expert reviewers some tool for > automatic review? Coccinelle is a relatively trusted tool and using it as a scripting mechanism where the script is shown as part of the commit message gives confidence that the change it produces can be applied without significant doubt. To improve confidence that any change that does not have an output object code delta, comparing the object code produced before and after the change is useful. Showing that the code has been both compiled and compared in the commit message also improves confidence that the change is useful and can be applied.
Re: [PATCH v3 00/30] staging: rtl8723bs: remove RT_TRACE logs in core/*
On Sat, Apr 03, 2021 at 09:17:37AM -0700, Joe Perches wrote: > On Sat, 2021-04-03 at 17:21 +0200, Fabio Aiuto wrote: > > On Sat, Apr 03, 2021 at 08:02:25AM -0700, Joe Perches wrote: > > > On Sat, 2021-04-03 at 11:13 +0200, Fabio Aiuto wrote: > > > > This patchset removes all RT_TRACE usages in core/ files. > > > > > > and hal and include and os_dep > > > > Hi, > > > > I was just about to send the second patchset relative to hal/ files. > > The whole has been split up in directories in order to reduce the > > number of patch per patchset > > > It's a good idea, but the patches relative to RT_TRACE removal > > could be huge > > That's really not a significant issue. > Simplicity in review is also important. > Mechanization of patch creation can reduce review efforts. Maybe I wrongly associated simplicity with patch dimensions, but maybe for patches this simple have expert reviewers some tool for automatic review? Is automatic review possible? > > Few people are actively working on this particular codebase. > As far as I can tell no logical defect is being corrected here. > None of this is likely to be backported. > > Applying each individual patch has a 'cost' in maintainer time > and review effort. got it > > Fewer patches create lower overall costs. > > Good luck... > I like your idea, and sure I will work in that direction, for this particular case I wait maintainer's opinion. If patchsets will be rejected again I will apply the scheme you proposed, if it will be accepted I will apply the scheme for next patchsets. Thank you, fabio
Re: [PATCH v3 00/30] staging: rtl8723bs: remove RT_TRACE logs in core/*
On Sat, 2021-04-03 at 17:21 +0200, Fabio Aiuto wrote: > On Sat, Apr 03, 2021 at 08:02:25AM -0700, Joe Perches wrote: > > On Sat, 2021-04-03 at 11:13 +0200, Fabio Aiuto wrote: > > > This patchset removes all RT_TRACE usages in core/ files. > > > > and hal and include and os_dep > > Hi, > > I was just about to send the second patchset relative to hal/ files. > The whole has been split up in directories in order to reduce the > number of patch per patchset > It's a good idea, but the patches relative to RT_TRACE removal > could be huge That's really not a significant issue. Simplicity in review is also important. Mechanization of patch creation can reduce review efforts. Few people are actively working on this particular codebase. As far as I can tell no logical defect is being corrected here. None of this is likely to be backported. Applying each individual patch has a 'cost' in maintainer time and review effort. Fewer patches create lower overall costs. Good luck...
Re: [PATCH v3 00/30] staging: rtl8723bs: remove RT_TRACE logs in core/*
On Sat, Apr 03, 2021 at 08:02:25AM -0700, Joe Perches wrote:
> On Sat, 2021-04-03 at 11:13 +0200, Fabio Aiuto wrote:
> > This patchset removes all RT_TRACE usages in core/ files.
>
> and hal and include and os_dep
Hi,
I was just about to send the second patchset relative to hal/ files.
The whole has been split up in directories in order to reduce the
number of patch per patchset
>
> >
> > This is the first of a series aimed at removing RT_TRACE macro.
> >
> > The whole private tracing system is not tied to a configuration
> > symbol and the default behaviour is _trace nothing_. It's verbose
> > and relies on a private log level tracing doomed to be
> > removed.
>
> It's nice, but individual patches per file done by hand are difficult
> to review because you are interleaving removal patches with cleanup
> patches.
>
> I believe this should be a patch series with a single patch to remove
> all RT_TRACE macro uses using coccinelle and then use separate patches
> to do whatever cleanups around these removals you want to do.
It's a good idea, but the patches relative to RT_TRACE removal
could be huge
>
> All of these below should be done for all files in drivers/staging/rtl8723bs
> at once instead of submitting per-file patches.
>
> IMO something like:
>
> Cover-letter: Explain why you are doing this
> Patch 1 of N: Remove all RT_TRACE macro uses using a coccinelle script
> and include the coccinelle script in the commit message
> Patch 2 of N: Remove commented out RT_TRACE uses
> Patch 3 of N: Remove RT_TRACE macro definition
> Patch 4 of N: Cleanup coccinelle generated {} uses, if/else braces and
> the now unnecessary if tests around the RT_TRACE removals
> Patch 5 of N: Cleanup whitespace
> Patcn x of N: Whatever else related to these RT_TRACE sites...
>
> https://lore.kernel.org/lkml/c845d8ea7d0d8e7a613471edb53d780d660142a9.ca...@perches.com/
>
> Using a sequence like the above would be much easier to review and
> would be a significant shorter patch set.
>
moreover every non RT_TRACE deletion patch (clean up patch) is near
to the contextual deletion patch (parent patch or grand-parent)
but I do not have experience in code reviewing, so I will do like you
say. Maybe I wait for other opinions, but what you say is good and
elegant.
thank you,
fabio
Re: [PATCH v3 00/30] staging: rtl8723bs: remove RT_TRACE logs in core/*
On Sat, 2021-04-03 at 11:13 +0200, Fabio Aiuto wrote:
> This patchset removes all RT_TRACE usages in core/ files.
and hal and include and os_dep
>
> This is the first of a series aimed at removing RT_TRACE macro.
>
> The whole private tracing system is not tied to a configuration
> symbol and the default behaviour is _trace nothing_. It's verbose
> and relies on a private log level tracing doomed to be
> removed.
It's nice, but individual patches per file done by hand are difficult
to review because you are interleaving removal patches with cleanup
patches.
I believe this should be a patch series with a single patch to remove
all RT_TRACE macro uses using coccinelle and then use separate patches
to do whatever cleanups around these removals you want to do.
All of these below should be done for all files in drivers/staging/rtl8723bs
at once instead of submitting per-file patches.
IMO something like:
Cover-letter: Explain why you are doing this
Patch 1 of N: Remove all RT_TRACE macro uses using a coccinelle script
and include the coccinelle script in the commit message
Patch 2 of N: Remove commented out RT_TRACE uses
Patch 3 of N: Remove RT_TRACE macro definition
Patch 4 of N: Cleanup coccinelle generated {} uses, if/else braces and
the now unnecessary if tests around the RT_TRACE removals
Patch 5 of N: Cleanup whitespace
Patcn x of N: Whatever else related to these RT_TRACE sites...
https://lore.kernel.org/lkml/c845d8ea7d0d8e7a613471edb53d780d660142a9.ca...@perches.com/
Using a sequence like the above would be much easier to review and
would be a significant shorter patch set.
[PATCH v3 00/30] staging: rtl8723bs: remove RT_TRACE logs in core/*
This patchset removes all RT_TRACE usages in core/ files. This is the first of a series aimed at removing RT_TRACE macro. The whole private tracing system is not tied to a configuration symbol and the default behaviour is _trace nothing_. It's verbose and relies on a private log level tracing doomed to be removed. --- Changes in v3: - written better changelog in single patches Changes in v2: - isolate checkpatch fixes in separate patches - removed two if conditions in core/rtw_wlan_util.c Fabio Aiuto (30): staging: rtl8723bs: remove RT_TRACE logs in core/rtw_xmit.c staging: rtl8723bs: fix condition in if statement in core/rtw_xmit.c staging: rtl8723bs: remove RT_TRACE logs in core/rtw_security.c staging: rtl8723bs: fix line exceed warning in core/rtw_security.c staging: rtl8723bs: fix spaces around operator issues in core/rtw_security.c staging: rtl8723bs: remove all RT_TRACE logs in core/rtw_eeprom.c staging: rtl8723bs: fix error prone if conditions in core/rtw_eeprom.c staging: rtl8723bs: remove all RT_TRACE logs in core/rtw_pwrctrl.c staging: rtl8723bs: fix logical continuation issue in core/rtw_pwrctrl.c staging: rtl8723bs: remove unnecessary parentheses in if-condition in core/rtw_pwrctrl.c staging: rtl8723bs: remove RT_TRACE logs in core/rtw_cmd.c staging: rtl8723bs: fix null check conditions in core/rtw_cmd.c staging: rtl8723bs: remove unnecessary parentheses in if condition in core/rtw_cmd.c staging: rtl8723bs: remove commented RT_TRACE calls in core/rtw_mlme.c staging: rtl8723bs: remove RT_TRACE logs in core/rtw_mlme.c staging: rtl8723bs: tidy up some error handling in core/rtw_mlme.c staging: rtl8723bs: remove RT_TRACE logs in core/rtw_mlme_ext.c staging: rtl8723bs: remove commented RT_TRACE calls in core/rtw_recv.c staging: rtl8723bs: remove RT_TRACE logs in core/rtw_recv.c staging: rtl8723bs: added spaces around operator in core/rtw_recv.c staging: rtl8723bs: split long line in core/rtw_recv.c staging: rtl8723bs: remove unnecessary parentheses in core/rtw_recv.c staging: rtl8723bs: fix comparison in if condition in core/rtw_recv.c staging: rtl8723bs: remove commented RT_TRACE call in core/rtw_ioctl_set.c staging: rtl8723bs: remove RT_TRACE logs in core/rtw_ioctl_set.c staging: rtl8723bs: place constant on the right side of the test in core/rtw_ioctl_set.c staging: rtl8723bs: remove all RT_TRACE logs in core/rtw_wlan_util.c staging: rtl8723bs: remove RT_TRACE logs in core/rtw_sta_mgt.c staging: rtl8723bs: remove RT_TRACE logs in core/rtw_ieee80211.c staging: rtl8723bs: add spaces around operators in core/rtw_ieee80211.c drivers/staging/rtl8723bs/core/rtw_cmd.c | 53 +-- drivers/staging/rtl8723bs/core/rtw_eeprom.c | 56 +++ .../staging/rtl8723bs/core/rtw_ieee80211.c| 90 ++- .../staging/rtl8723bs/core/rtw_ioctl_set.c| 79 +- drivers/staging/rtl8723bs/core/rtw_mlme.c | 124 +++ drivers/staging/rtl8723bs/core/rtw_mlme_ext.c | 39 + drivers/staging/rtl8723bs/core/rtw_pwrctrl.c | 56 +-- drivers/staging/rtl8723bs/core/rtw_recv.c | 147 +- drivers/staging/rtl8723bs/core/rtw_security.c | 41 + drivers/staging/rtl8723bs/core/rtw_sta_mgt.c | 25 --- .../staging/rtl8723bs/core/rtw_wlan_util.c| 24 +-- drivers/staging/rtl8723bs/core/rtw_xmit.c | 82 +- 12 files changed, 101 insertions(+), 715 deletions(-) -- 2.20.1
