Re: [PATCH -next 2/2] staging: r8188eu: use eth_broadcast_addr() to assign broadcast address

Re: [PATCH -next 2/2] staging: r8188eu: use eth_broadcast_addr() to assign broadcast address

[Greg Kroah-Hartman]

On Tue, Jun 08, 2021 at 10:16:20PM +0800, Liu Shixin wrote:
> Use eth_broadcast_addr() to assign broadcast address.

That says what you do, but not _why_ you are doing this?

Why make this change?  What benifit does it provide?

thanks,

greg k-h

[PATCH -next 2/2] staging: r8188eu: use eth_broadcast_addr() to assign broadcast address

[Liu Shixin]

Use eth_broadcast_addr() to assign broadcast address.

Signed-off-by: Liu Shixin <liushixin2@huawei.com>
---
 drivers/staging/rtl8188eu/core/rtw_mlme_ext.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c b/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
index 8c1bc04dd830..119110a150b2 100644
--- a/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
+++ b/drivers/staging/rtl8188eu/core/rtw_mlme_ext.c
@@ -315,7 +315,6 @@ static void issue_beacon(struct adapter *padapter, int timeout_ms)
 	struct mlme_ext_priv *pmlmeext = &padapter->mlmeextpriv;
 	struct mlme_ext_info *pmlmeinfo = &pmlmeext->mlmext_info;
 	struct wlan_bssid_ex *cur_network = &pmlmeinfo->network;
-	u8 bc_addr[] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
 
 	pmgntframe = alloc_mgtxmitframe(pxmitpriv);
 	if (!pmgntframe) {
@@ -339,7 +338,7 @@ static void issue_beacon(struct adapter *padapter, int timeout_ms)
 	fctrl = &pwlanhdr->frame_control;
 	*(fctrl) = 0;
 
-	ether_addr_copy(pwlanhdr->addr1, bc_addr);
+	eth_broadcast_addr(pwlanhdr->addr1);
 	ether_addr_copy(pwlanhdr->addr2, myid(&padapter->eeprompriv));
 	ether_addr_copy(pwlanhdr->addr3, cur_network->MacAddress);
 
@@ -605,7 +604,6 @@ static int issue_probereq(struct adapter *padapter,
 	struct mlme_priv *pmlmepriv = &padapter->mlmepriv;
 	struct mlme_ext_priv *pmlmeext = &padapter->mlmeextpriv;
 	int bssrate_len = 0;
-	u8 bc_addr[] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
 
 	RT_TRACE(_module_rtl871x_mlme_c_, _drv_notice_, ("+%s\n", __func__));
 
@@ -633,8 +631,8 @@ static int issue_probereq(struct adapter *padapter,
 		ether_addr_copy(pwlanhdr->addr3, da);
 	} else {
 		/*	broadcast probe request frame */
-		ether_addr_copy(pwlanhdr->addr1, bc_addr);
-		ether_addr_copy(pwlanhdr->addr3, bc_addr);
+		eth_broadcast_addr(pwlanhdr->addr1);
+		eth_broadcast_addr(pwlanhdr->addr3);
 	}
 
 	ether_addr_copy(pwlanhdr->addr2, mac);
-- 
2.18.0.huawei.25

Re: [PATCH -next 2/2] staging: r8188eu: use eth_broadcast_addr() to assign broadcast address

[Joe Perches]

On Tue, 2021-06-08 at 16:12 +0200, Greg Kroah-Hartman wrote:
> On Tue, Jun 08, 2021 at 10:16:20PM +0800, Liu Shixin wrote:
> > Use eth_broadcast_addr() to assign broadcast address.
> 
> That says what you do, but not _why_ you are doing this?
> 
> Why make this change?  What benifit does it provide?

The commit message is clear and concise as using available kernel
mechanisms is better than homegrown or duplicative ones.

Are you asking merely becuse Liu Shixin hasn't had many staging
commits?

Re: [PATCH -next 2/2] staging: r8188eu: use eth_broadcast_addr() to assign broadcast address

[Greg Kroah-Hartman]

On Tue, Jun 08, 2021 at 09:45:49AM -0700, Joe Perches wrote:
> On Tue, 2021-06-08 at 16:12 +0200, Greg Kroah-Hartman wrote:
> > On Tue, Jun 08, 2021 at 10:16:20PM +0800, Liu Shixin wrote:
> > > Use eth_broadcast_addr() to assign broadcast address.
> > 
> > That says what you do, but not _why_ you are doing this?
> > 
> > Why make this change?  What benifit does it provide?
> 
> The commit message is clear and concise as using available kernel
> mechanisms is better than homegrown or duplicative ones.
> 
> Are you asking merely becuse Liu Shixin hasn't had many staging
> commits?

I'm asking because this changelog text does not explain why this is
needed at all and needs to be changed to do so.

thanks,

greg k-h

Re: [PATCH -next 2/2] staging: r8188eu: use eth_broadcast_addr() to assign broadcast address

[Joe Perches]

On Tue, 2021-06-08 at 19:01 +0200, Greg Kroah-Hartman wrote:
> On Tue, Jun 08, 2021 at 09:45:49AM -0700, Joe Perches wrote:
> > On Tue, 2021-06-08 at 16:12 +0200, Greg Kroah-Hartman wrote:
> > > On Tue, Jun 08, 2021 at 10:16:20PM +0800, Liu Shixin wrote:
> > > > Use eth_broadcast_addr() to assign broadcast address.
> > > 
> > > That says what you do, but not _why_ you are doing this?
> > > 
> > > Why make this change?  What benifit does it provide?
> > 
> > The commit message is clear and concise as using available kernel
> > mechanisms is better than homegrown or duplicative ones.
> > 
> > Are you asking merely becuse Liu Shixin hasn't had many staging
> > commits?
> 
> I'm asking because this changelog text does not explain why this is
> needed at all and needs to be changed to do so.

IYO.

IMO it's obvious and fine as is and you are asking for overly
fine-grained analyses in commit messages.

The subject is clear though the commit message is merely duplicative.

It _could_ show the reduction in object size for some versions of gcc.

$ size drivers/staging/rtl8188eu/core/rtw_mlme_ext.o*
   text	   data	    bss	    dec	    hex	filename
  53259	    372	   2430	  56061	   dafd	drivers/staging/rtl8188eu/core/rtw_mlme_ext.o.gcc6.new
  53355	    372	   2430	  56157	   db5d	drivers/staging/rtl8188eu/core/rtw_mlme_ext.o.gcc6.old
  54673	    372	   2430	  57475	   e083	drivers/staging/rtl8188eu/core/rtw_mlme_ext.o.gcc10.new
  54673	    372	   2430	  57475	   e083	drivers/staging/rtl8188eu/core/rtw_mlme_ext.o.gcc10.old

It _could_ describe how the kernel mechanisms depend on a minimum
alignment of __aligned(2) in the tested address and also show that
the address is properly minimum aligned.

struct ieee80211_hdr {
	__le16 frame_control;
	__le16 duration_id;
	u8 addr1[ETH_ALEN];
	u8 addr2[ETH_ALEN];
	u8 addr3[ETH_ALEN];
	__le16 seq_ctrl;
	u8 addr4[ETH_ALEN];
} __packed __aligned(2);
[...]
	struct ieee80211_hdr *pwlanhdr;
[...]
-	ether_addr_copy(pwlanhdr->addr1, bc_addr);
+	eth_broadcast_addr(pwlanhdr->addr1);

It _could_ show that the commit has some effect on runtime.
It _could_ show that it passes some (unavailable) regression test.

IMO: None of those are really necessary here.

Re: [PATCH -next 2/2] staging: r8188eu: use eth_broadcast_addr() to assign broadcast address

[Joe Perches]

On Tue, 2021-06-08 at 19:01 +0200, Greg Kroah-Hartman wrote:
> On Tue, Jun 08, 2021 at 09:45:49AM -0700, Joe Perches wrote:
> > On Tue, 2021-06-08 at 16:12 +0200, Greg Kroah-Hartman wrote:
> > > On Tue, Jun 08, 2021 at 10:16:20PM +0800, Liu Shixin wrote:
> > > > Use eth_broadcast_addr() to assign broadcast address.
> > > 
> > > That says what you do, but not _why_ you are doing this?
> > > 
> > > Why make this change?  What benifit does it provide?
> > 
> > The commit message is clear and concise as using available kernel
> > mechanisms is better than homegrown or duplicative ones.
> > 
> > Are you asking merely becuse Liu Shixin hasn't had many staging
> > commits?
> 
> I'm asking because this changelog text does not explain why this is
> needed at all and needs to be changed to do so.

IYO.

IMO it's obvious and fine as is and you are asking for overly
fine-grained analyses in commit messages.

The subject is clear though the commit message is merely duplicative.

It _could_ show the reduction in object size for some versions of gcc.

$ size drivers/staging/rtl8188eu/core/rtw_mlme_ext.o*
   text	   data	    bss	    dec	    hex	filename
  53259	    372	   2430	  56061	   dafd	drivers/staging/rtl8188eu/core/rtw_mlme_ext.o.gcc6.new
  53355	    372	   2430	  56157	   db5d	drivers/staging/rtl8188eu/core/rtw_mlme_ext.o.gcc6.old
  54673	    372	   2430	  57475	   e083	drivers/staging/rtl8188eu/core/rtw_mlme_ext.o.gcc10.new
  54673	    372	   2430	  57475	   e083	drivers/staging/rtl8188eu/core/rtw_mlme_ext.o.gcc10.old

It _could_ describe how the kernel mechanisms depend on a minimum
alignment of __aligned(2) in the tested address and also show that
the address is properly minimum aligned.

struct ieee80211_hdr {
	__le16 frame_control;
	__le16 duration_id;
	u8 addr1[ETH_ALEN];
	u8 addr2[ETH_ALEN];
	u8 addr3[ETH_ALEN];
	__le16 seq_ctrl;
	u8 addr4[ETH_ALEN];
} __packed __aligned(2);
[...]
	struct ieee80211_hdr *pwlanhdr;
[...]
-	ether_addr_copy(pwlanhdr->addr1, bc_addr);
+	eth_broadcast_addr(pwlanhdr->addr1);

It _could_ show that the commit has some effect on runtime.
It _could_ show that it passes some (unavailable) regression test.

IMO: None of those are really necessary here.

Re: [PATCH -next 2/2] staging: r8188eu: use eth_broadcast_addr() to assign broadcast address

[Liu Shixin]

On 2021/6/9 1:34, Joe Perches wrote:
> On Tue, 2021-06-08 at 19:01 +0200, Greg Kroah-Hartman wrote:
>> On Tue, Jun 08, 2021 at 09:45:49AM -0700, Joe Perches wrote:
>>> On Tue, 2021-06-08 at 16:12 +0200, Greg Kroah-Hartman wrote:
>>>> On Tue, Jun 08, 2021 at 10:16:20PM +0800, Liu Shixin wrote:
>>>>> Use eth_broadcast_addr() to assign broadcast address.
>>>> That says what you do, but not _why_ you are doing this?
>>>>
>>>> Why make this change?  What benifit does it provide?
>>> The commit message is clear and concise as using available kernel
>>> mechanisms is better than homegrown or duplicative ones.
>>>
>>> Are you asking merely becuse Liu Shixin hasn't had many staging
>>> commits?
>> I'm asking because this changelog text does not explain why this is
>> needed at all and needs to be changed to do so.
> IYO.
>
> IMO it's obvious and fine as is and you are asking for overly
> fine-grained analyses in commit messages.
>
> The subject is clear though the commit message is merely duplicative.
>
> It _could_ show the reduction in object size for some versions of gcc.
>
> $ size drivers/staging/rtl8188eu/core/rtw_mlme_ext.o*
>    text	   data	    bss	    dec	    hex	filename
>   53259	    372	   2430	  56061	   dafd	drivers/staging/rtl8188eu/core/rtw_mlme_ext.o.gcc6.new
>   53355	    372	   2430	  56157	   db5d	drivers/staging/rtl8188eu/core/rtw_mlme_ext.o.gcc6.old
>   54673	    372	   2430	  57475	   e083	drivers/staging/rtl8188eu/core/rtw_mlme_ext.o.gcc10.new
>   54673	    372	   2430	  57475	   e083	drivers/staging/rtl8188eu/core/rtw_mlme_ext.o.gcc10.old
>
> It _could_ describe how the kernel mechanisms depend on a minimum
> alignment of __aligned(2) in the tested address and also show that
> the address is properly minimum aligned.
>
> struct ieee80211_hdr {
> 	__le16 frame_control;
> 	__le16 duration_id;
> 	u8 addr1[ETH_ALEN];
> 	u8 addr2[ETH_ALEN];
> 	u8 addr3[ETH_ALEN];
> 	__le16 seq_ctrl;
> 	u8 addr4[ETH_ALEN];
> } __packed __aligned(2);
> [...]
> 	struct ieee80211_hdr *pwlanhdr;
> [...]
> -	ether_addr_copy(pwlanhdr->addr1, bc_addr);
> +	eth_broadcast_addr(pwlanhdr->addr1);
>
> It _could_ show that the commit has some effect on runtime.
> It _could_ show that it passes some (unavailable) regression test.
>
> IMO: None of those are really necessary here.
>
>
> .
>
The variable bc_addr is repeated many times in the code and looks like magic number. I want to simplify the code by remoing unnecessary bc_addr.
And I think it's better using eth_broadcast_addr() directly rather than using ether_addr_copy() + bc_addr.

Thanks to Joe for the data.

Thanks,

Re: [PATCH -next 2/2] staging: r8188eu: use eth_broadcast_addr() to assign broadcast address

[Fabio Aiuto]

Hi Liu,

On Wed, Jun 09, 2021 at 11:01:18AM +0800, Liu Shixin wrote:
> On 2021/6/9 1:34, Joe Perches wrote:
> > On Tue, 2021-06-08 at 19:01 +0200, Greg Kroah-Hartman wrote:
> >> On Tue, Jun 08, 2021 at 09:45:49AM -0700, Joe Perches wrote:
> >>> On Tue, 2021-06-08 at 16:12 +0200, Greg Kroah-Hartman wrote:
> >>>> On Tue, Jun 08, 2021 at 10:16:20PM +0800, Liu Shixin wrote:
> >>>>> Use eth_broadcast_addr() to assign broadcast address.
> >>>> That says what you do, but not _why_ you are doing this?
> >>>>
> >>>> Why make this change?  What benifit does it provide?
> >>> The commit message is clear and concise as using available kernel
> >>> mechanisms is better than homegrown or duplicative ones.
> >>>
> >>> Are you asking merely becuse Liu Shixin hasn't had many staging
> >>> commits?
> >> I'm asking because this changelog text does not explain why this is
> >> needed at all and needs to be changed to do so.
> > IYO.
> >
> > IMO it's obvious and fine as is and you are asking for overly
> > fine-grained analyses in commit messages.
> >
> > The subject is clear though the commit message is merely duplicative.
> >
> > It _could_ show the reduction in object size for some versions of gcc.
> >
> > $ size drivers/staging/rtl8188eu/core/rtw_mlme_ext.o*
> >    text	   data	    bss	    dec	    hex	filename
> >   53259	    372	   2430	  56061	   dafd	drivers/staging/rtl8188eu/core/rtw_mlme_ext.o.gcc6.new
> >   53355	    372	   2430	  56157	   db5d	drivers/staging/rtl8188eu/core/rtw_mlme_ext.o.gcc6.old
> >   54673	    372	   2430	  57475	   e083	drivers/staging/rtl8188eu/core/rtw_mlme_ext.o.gcc10.new
> >   54673	    372	   2430	  57475	   e083	drivers/staging/rtl8188eu/core/rtw_mlme_ext.o.gcc10.old
> >
> > It _could_ describe how the kernel mechanisms depend on a minimum
> > alignment of __aligned(2) in the tested address and also show that
> > the address is properly minimum aligned.
> >
> > struct ieee80211_hdr {
> > 	__le16 frame_control;
> > 	__le16 duration_id;
> > 	u8 addr1[ETH_ALEN];
> > 	u8 addr2[ETH_ALEN];
> > 	u8 addr3[ETH_ALEN];
> > 	__le16 seq_ctrl;
> > 	u8 addr4[ETH_ALEN];
> > } __packed __aligned(2);
> > [...]
> > 	struct ieee80211_hdr *pwlanhdr;
> > [...]
> > -	ether_addr_copy(pwlanhdr->addr1, bc_addr);
> > +	eth_broadcast_addr(pwlanhdr->addr1);
> >
> > It _could_ show that the commit has some effect on runtime.
> > It _could_ show that it passes some (unavailable) regression test.
> >
> > IMO: None of those are really necessary here.
> >
> >
> > .
> >
> The variable bc_addr is repeated many times in the code and looks like magic number. I want to simplify the code by remoing unnecessary bc_addr.
> And I think it's better using eth_broadcast_addr() directly rather than using ether_addr_copy() + bc_addr.
> 
> Thanks to Joe for the data.
> 
> Thanks,
> 
> 
> 

I would change the subject line using the proper driver name: 

	'staging: rtl8188eu: ...'

and not the compiled module name that I think it needs to be fixed (r8188eu).

Thank you,

fabio