Re: [PATCH 01/61] staging: rtl8723bs: remove redundant rtw_ap_set_group_key wrapper

[Greg Kroah-Hartman]

On Wed, Jul 15, 2026 at 03:15:41PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Jul 15, 2026 at 07:47:29PM +0700, Arsenii Pashchenko wrote:
> > Eliminate the redundant rtw_ap_set_group_key() wrapper function by
> > calling rtw_ap_set_key() directly with the set_tx parameter set to 1.
> > This reduces boilerplate code and cleans up the driver interface.
> > 
> > Signed-off-by: Arsenii Pashchenko <ulijg308@xxxxxxxxx>
> > ---
> >  drivers/staging/rtl8723bs/core/rtw_ap.c           | 7 +------
> >  drivers/staging/rtl8723bs/include/rtw_ap.h        | 2 +-
> >  drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c | 8 ++++++--
> >  3 files changed, 8 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/staging/rtl8723bs/core/rtw_ap.c b/drivers/staging/rtl8723bs/core/rtw_ap.c
> > index 065850a9e..a6640cd7a 100644
> > --- a/drivers/staging/rtl8723bs/core/rtw_ap.c
> > +++ b/drivers/staging/rtl8723bs/core/rtw_ap.c
> > @@ -1195,7 +1195,7 @@ u8 rtw_ap_set_pairwise_key(struct adapter *padapter, struct sta_info *psta)
> >  	return res;
> >  }
> >  
> > -static int rtw_ap_set_key(struct adapter *padapter,
> > +int rtw_ap_set_key(struct adapter *padapter,
> >  			  u8 *key,
> >  			  u8 alg,
> >  			  int keyid,
> > @@ -1258,11 +1258,6 @@ static int rtw_ap_set_key(struct adapter *padapter,
> >  	return res;
> >  }
> >  
> > -int rtw_ap_set_group_key(struct adapter *padapter, u8 *key, u8 alg, int keyid)
> > -{
> > -	return rtw_ap_set_key(padapter, key, alg, keyid, 1);
> > -}
> > -
> >  int rtw_ap_set_wep_key(struct adapter *padapter,
> >  		       u8 *key,
> >  		       u8 keylen,
> > diff --git a/drivers/staging/rtl8723bs/include/rtw_ap.h b/drivers/staging/rtl8723bs/include/rtw_ap.h
> > index 83e835eb6..cdd9f291f 100644
> > --- a/drivers/staging/rtl8723bs/include/rtw_ap.h
> > +++ b/drivers/staging/rtl8723bs/include/rtw_ap.h
> > @@ -22,7 +22,7 @@ int rtw_acl_add_sta(struct adapter *padapter, u8 *addr);
> >  void rtw_acl_remove_sta(struct adapter *padapter, u8 *addr);
> >  
> >  u8 rtw_ap_set_pairwise_key(struct adapter *padapter, struct sta_info *psta);
> > -int rtw_ap_set_group_key(struct adapter *padapter, u8 *key, u8 alg, int keyid);
> > +int rtw_ap_set_key(struct adapter *padapter, u8 *key, u8 alg, int keyid, u8 set_tx);
> >  int rtw_ap_set_wep_key(struct adapter *padapter, u8 *key, u8 keylen, int keyid, u8 set_tx);
> >  
> >  void associated_clients_update(struct adapter *padapter, u8 updated);
> > diff --git a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
> > index 1484336d7..ce76a8586 100644
> > --- a/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
> > +++ b/drivers/staging/rtl8723bs/os_dep/ioctl_cfg80211.c
> > @@ -604,7 +604,9 @@ static int rtw_cfg80211_ap_set_encryption(struct net_device *dev, struct ieee_pa
> >  
> >  			psecuritypriv->dot11PrivacyAlgrthm = psecuritypriv->dot118021XGrpPrivacy;/*  */
> >  
> > -			rtw_ap_set_group_key(padapter, param->u.crypt.key, psecuritypriv->dot118021XGrpPrivacy, param->u.crypt.idx);
> > +			rtw_ap_set_key(padapter, param->u.crypt.key,
> > +						   psecuritypriv->dot118021XGrpPrivacy,
> > +						   param->u.crypt.idx, 1);
> 
> Ok, first patch of the series, this isn't correct.
> 
> I'm all for cleanups, but remember, drivers/staging/ is here to get
> experience with kernel development.  Sending a 60+ series as your first
> contribution to the kernel isn't the best way to start out.
> 
> Take your time, don't use a LLM, and fix issues one at a time and then
> batch up patches, maybe 5 at a time and send them out, get reviews, fix
> them up, wait for them to be accepted, and learn the process.
> 
> As-is, with the very first patch in this series being incorrect, I can't
> do anything with these at all as-is.

As for _why_ this patch is wrong, there's the obvious "coding style is
incorrect" which is easy to see.  The real problem is that it doesn't
actually make anything "simpler" at all.  You are perputating an
incorrect api here.  Think about why you are setting a value to 1, and
what that does and if that makes sense at all.  Walk it back and see if
the variable is always set to the same value, why is that variable
needed at all?  And if it's not needed, why is the function parameter
needed?

Unwind the mess, and work to clean things up properly, by learning and
understanding the code, not by throwing a fuzzy-text-matching-tool at
the thing.  If we wanted that type of cleanup, we would have done that
ages ago, that's not the point of this kernel code at all.

thanks,

greg k-h