On Tue, Jan 25, 2022 at 12:02:33PM +0300, Dan Carpenter wrote:
> On Mon, Jan 24, 2022 at 10:44:07PM +0000, Phillip Potter wrote:
> > void rtw_sctx_done_err(struct submit_ctx **sctx, int status)
> > {
> > if (*sctx) {
> > - if (rtw_sctx_chk_waring_status(status))
> > - DBG_88E("%s status:%d\n", __func__, status);
> > + rtw_sctx_chk_waring_status(status);
>
> In a follow on patch, can you just remove the rtw_sctx_chk_waring_status()
> function?
>
> Looking through these warnings, a mass delete is the right thing. The
> debugging messages are garbage. Use ftrace. Some of the errors are
> debug level so they will never be seen. If you enable printing error
> messages, it print a debug information but at error level so that's
> useless. Forget about it. Just delete it.
>
> You're going to need to redo these again because kbuild found some
> build warnings...
>
> Normally we would say if you're deleting a printk() and that makes the
> curly braces unnecessary, then you have to delete the curly braces in
> the same patch. But in this case, you're changing a ton of code and
> deleting the curly braces makes it hard to review. I would prefer it
> if you just deleted the printks and fixed up the curly braces in a
> follow up patch. And please don't fix long line warnings either. Do
> that in another even later patch if you want.
>
> You do need to fix any build warnings about unused variables etc, but
> don't worry about checkpatch until the end.
>
> Basically as much as possible I'd prefer to only see deleted lines in
> this patch. No insertions.
>
> 12 files changed, 66 insertions(+), 472 deletions(-)
>
> There was one function where you removed the padatper argument because
> it wasn't necessary. That's great but do it in a follow on patch.
> Keep a notepad app next to your vim and write down notes as you go:
>
> 0) Re-run checkpatch and delete unnecessary curly braces
> 1) Delete rtw_sctx_chk_waring_status()
> 2) padatper not needed for frob_whatever()
> 3) Fix long lines
>
> regards,
> dan carpenter
Hi Dan,
Thank you for the feedback - reason I removed the padapter arg is
because I was the one that added it. To be clear, you're happy with this
and the brace changes etc. as long as I take them out within a separate
patch at the end of the series?
I had quite a few unused variable errors along the way, but evidently
with the way my build system is setup it doesn't currently show set but
unused variables. I will fix this and deal with the kernel test robot
warnings. Many thanks.
Regards,
Phil
