Use eth_broadcast_addr() to assign broadcast address.
Signed-off-by: Liu Shixin <[email protected]>
---
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
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
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
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.
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?
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
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,