linux-kernel - Re: [EXT] Re: [PATCH v10 1/2] wifi: mwifiex: add host mlme for client mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 20 Jun 2024 10:53:02 -0700
From: Brian Norris <briannorris@...omium.org>
To: David Lin <yu-hao.lin@....com>
Cc: "linux-wireless@...r.kernel.org" <linux-wireless@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"kvalo@...nel.org" <kvalo@...nel.org>,
	"francesco@...cini.it" <francesco@...cini.it>,
	Pete Hsieh <tsung-hsien.hsieh@....com>,
	Francesco Dolcini <francesco.dolcini@...adex.com>
Subject: Re: [EXT] Re: [PATCH v10 1/2] wifi: mwifiex: add host mlme for
 client mode

Hi David,

On Sat, May 25, 2024 at 12:50:59AM +0000, David Lin wrote:
> 
> > From: Brian Norris <briannorris@...omium.org>
> > Sent: Saturday, May 25, 2024 6:55 AM
> > 
> > On Fri, May 24, 2024 at 3:01 PM David Lin <yu-hao.lin@....com> wrote:
> > > I think it needs time to support probe client. Can we put your
> > > suggested comments to the code used to hook probe_client() and add
> > >
> > > "TODO: support probe client" to mwifiex_cfg80211_probe_client().
> > 
> > Are you suggesting that you plan to actually implement proper probe_client
> > support? Did you already do what I suggested, and understand why hostapd
> > needs probe_client support? This seems to be a common pattern -- that
> > reviewers are asking for you to do your research, and it takes several
> > requests before you actually do it.
> > 
> > Now that I've tried to do that research for you ... it looks like hostapd uses
> > probe_client to augment TX MGMT acks, as a proxy for station presence /
> > inactivity. If a station is inactive and non-responsive, we disconnect it
> > eventually. So that looks to me like probe_client support should actually be
> > optional, if your driver reports TX status? And in that case, I'd still
> > recommend you try to fix hostapd.
> > 
> > But if you're really planning to implement proper probe_client support, then
> > I suppose the TODO approach is also OK.
> > 
> > I'd also request that you please actually do your research when reviewers
> > ask questions. I'm frankly not sure why I'm spending my time on the above
> > research, when the onus should be on the submitter to explain why they're
> > doing what they're doing.
> 
> Yes. I know when aging time of station is out, hostapd will use probe_client to check if station is still there before really disconnect it.
> 
> Without this feature, it won't really affect mayor function of hostapd.

I'm glad *you* know all about the above behavior, but *I* didn't know
about it until I went and researched what this API does, and how hostapd
is using it. But that isn't my job -- it's your job, as the code
submitter, to explain your reasoning and reduce the amount of work that
readers/reviewers/maintainers have to do to understand your code and
agree that it is the right thing to do.

It's not clear to me that you've really learned the above lesson, and
it's really affecting the rate at which I review your code. This is by
far not the first time that you've placed the burden on the reader. And
if you're going to make the job difficult, then I'll prioritize enjoying
my free time, or stuff that actually pays me at $DAY_JOB, or ...

> That is the reason that I suggest that we put comments and TODO to the code.

OK, I suppose that works for me.

Brian