Hi Brian,
> From: Brian Norris <[email protected]>
> Sent: Saturday, May 25, 2024 6:55 AM
> To: David Lin <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; Pete Hsieh
> <[email protected]>; Francesco Dolcini
> <[email protected]>
> Subject: Re: [EXT] Re: [PATCH v10 1/2] wifi: mwifiex: add host mlme for client
> mode
>
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report
> this email' button
>
>
> On Fri, May 24, 2024 at 3:01 PM David Lin <[email protected]> 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.
>
> Brian
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.
That is the reason that I suggest that we put comments and TODO to the code.
David
> From: David Lin <[email protected]>
> Sent: Saturday, May 25, 2024 8:51 AM
> To: Brian Norris <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; Pete Hsieh
> <[email protected]>; Francesco Dolcini
> <[email protected]>
> Subject: RE: [EXT] Re: [PATCH v10 1/2] wifi: mwifiex: add host mlme for client
> mode
>
> Hi Brian,
>
> > From: Brian Norris <[email protected]>
> > Sent: Saturday, May 25, 2024 6:55 AM
> > To: David Lin <[email protected]>
> > Cc: [email protected]; [email protected];
> > [email protected]; [email protected]; Pete Hsieh
> > <[email protected]>; Francesco Dolcini
> > <[email protected]>
> > Subject: Re: [EXT] Re: [PATCH v10 1/2] wifi: mwifiex: add host mlme
> > for client mode
> >
> > Caution: This is an external email. Please take care when clicking
> > links or opening attachments. When in doubt, report the message using
> > the 'Report this email' button
> >
> >
> > On Fri, May 24, 2024 at 3:01 PM David Lin <[email protected]> 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.
> >
> > Brian
>
> 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.
>
> That is the reason that I suggest that we put comments and TODO to the
> code.
>
> David
If you agree that this extra check can be optional, maybe I can just put your suggested comments to the code.
David
Hi Brian,
> From: David Lin <[email protected]>
> Sent: Saturday, May 25, 2024 8:51 AM
> To: Brian Norris <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; Pete Hsieh
> <[email protected]>; Francesco Dolcini
> <[email protected]>
> Subject: RE: [EXT] Re: [PATCH v10 1/2] wifi: mwifiex: add host mlme for client
> mode
>
> Hi Brian,
>
> > From: Brian Norris <[email protected]>
> > Sent: Saturday, May 25, 2024 6:55 AM
> > To: David Lin <[email protected]>
> > Cc: [email protected]; [email protected];
> > [email protected]; [email protected]; Pete Hsieh
> > <[email protected]>; Francesco Dolcini
> > <[email protected]>
> > Subject: Re: [EXT] Re: [PATCH v10 1/2] wifi: mwifiex: add host mlme
> > for client mode
> >
> > Caution: This is an external email. Please take care when clicking
> > links or opening attachments. When in doubt, report the message using
> > the 'Report this email' button
> >
> >
> > On Fri, May 24, 2024 at 3:01 PM David Lin <[email protected]> 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.
> >
> > Brian
>
> 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.
>
> That is the reason that I suggest that we put comments and TODO to the code.
>
> David
> If you agree that this extra check can be optional, maybe I can just put your suggested comments to the code.
> David
Please let me know what else should I do to let this patch be ACKed by you.
Thanks,
David