Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 1 Aug 2023 10:55:22 -0700
From:   Brian Norris <briannorris@chromium.org>
To:     Dmitry Antipov <dmantipov@yandex.ru>
Cc:     Kalle Valo <kvalo@kernel.org>, linux-wireless@vger.kernel.org,
        lvc-project@linuxtesting.org
Subject: Re: [PATCH 2/2] [v2] wifi: mwifiex: cleanup TX error handling
Message-ID: <ZMlHCmjf2ZovExsP@google.com>
References: <ZMFzBStAKemf+dLL@google.com>
 <20230728084407.101930-1-dmantipov@yandex.ru>
 <20230728084407.101930-2-dmantipov@yandex.ru>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230728084407.101930-2-dmantipov@yandex.ru>
Precedence: bulk

On Fri, Jul 28, 2023 at 11:43:43AM +0300, Dmitry Antipov wrote:
> Since 'mwifiex_process_tx()' is the only place from where both
> 'mwifiex_process_sta_txpd()' and 'mwifiex_process_uap_txpd()'
> are called, these functions may be converted to 'void' after
> moving skb layout check to the caller, which may be simplified
> as well. Also adjust somewhat obfuscating error messages and
> add 'mwifiex_interface_name()' to make them a bit more useful.

Do you actually run this driver on anything, or are you just compile
testing / running static analysis? Because IIUC, all these messages are
perfectly clear when using the mwifiex_dbg() macro, which usually ends
up in dev_info(), and includes things like "mwifiex_pcie",
"mwifiex_sdio", and "mwifiex_usb" already. So the
mwifiex_interface_name() stuff just seems superfluous. I'd suggest
removing that.

At the same time, it seems like you're working hard on trying *not* to
get your stuff merged in a timely manner, as you shift the goalposts
every time you refactor your series. Specifically, you're now violating
basic guidelines like these:

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#commit_messages

"If you find yourself listing out a number of changes in the commit
message as a bulleted list or similar, consider splitting up the patch
into discrete changes that each do one thing. Similarly, if one of the
additional considerations is refactoring, try to shift that into a
separate patch."

This started out as "avoid BUG_ON()", which is a great goal on its own.
But now you haven't even mentioned that in your laundry list of
refactors. This seems like you have at least two patch candidates here:
refactoring the error handling, and dropping the BUG_ON(). (If the
refactoring becomes extremely trivial, maybe this is OK as 1 patch. But
in its current form, it's not.)

Before you resubmit, please read the patch submission guidelines again.
I'd also suggest sticking this (as multiple patches) at the end of the
series, so the other patches (which have been reviewed/ack'd multiple
times) can land cleanly.

Brian

> Suggested-by: Brian Norris <briannorris@chromium.org>
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> ---
> v2: some redesign in attempt to integrate Brian's feedback
[...]