Re: [PATCH 2/3] staging/rtl8192e: use s8 instead of char
Arnd Bergmann writes: > On Tuesday, July 19, 2016 12:05:00 PM CEST Jes Sorensen wrote: >> Arnd Bergmann writes: >> I think that would be better, albeit not a big issue. > > Ok, and since Kalle applied the first patch to his tree, I'm now sending > a series of three patches that are all for Greg, which also avoids some > possible confusion. Awesome! >> I'd like to get rid of all the drivers/staging/rtl* drivers eventually > > That would be great, yes. > > Can you clarify what the long-term plan is? I see that > drivers/net/wireless/realtek/rtlwifi/ has most of the PCIe parts > and one USB device (rtl8192cu/rtl8188cus) while > drivers/net/wireless/rtl8xxx has all the USB parts including > that one. > > Does that mean we want the staging drivers for PCIe devices > to get merged into rtlwifi, and the remaining USB drivers to get > replaced by r8xxxu? Well it really all depends on how much time I have and how much others step up and help contribute to the code. For rtl8xxxu my plans are as follows: 1) rtl8188eu support, since this is the most widely distributed USB dongle which isn't currently supported by a non staging driver. I am currently working on this together with Andrea Merello. 2) Beacon support for IBSS and AP mode - hopefully this should make it possible to default rtl8xxxu for rtl8192cu/rtl8188cu devices and disable them in rtlwifi. 3) SDIO device support 4) PCI device support 5) 802.11ac device support 3/4/5 not necessarily in that order. There really is no reason why rtl8xxxu shouldn't have SDIO and PCI device support added so the core code can be shared. > As one data point that I can provide (but you are probably > aware of), I could never get my rtl8188cus stick to work with > rtlwifi, but I found the older r8712u device to work fine with > the staging/rtl8712 driver. I'd love to hear if the rtl8188cus works better with rtl8xxxu. For the rtl8712 device, rtl8192su?, then potentially that could be added to rtl8xxxu as well, but it's not a top priority on my list right now. Cheers, Jes
Re: [PATCH 2/3] staging/rtl8192e: use s8 instead of char
Arnd Bergmann writes:
> On Tuesday, July 19, 2016 11:46:04 AM CEST Jes Sorensen wrote:
>> > diff --git a/drivers/staging/rtl8192e/rtl819x_TSProc.c
>> > b/drivers/staging/rtl8192e/rtl819x_TSProc.c
>> > index 2c8a526773ed..e0a2fe5e6148 100644
>> > --- a/drivers/staging/rtl8192e/rtl819x_TSProc.c
>> > +++ b/drivers/staging/rtl8192e/rtl819x_TSProc.c
>> > @@ -323,7 +323,7 @@ bool GetTs(struct rtllib_device *ieee, struct
>> > ts_common_info **ppTS,
>> > if (ieee->current_network.qos_data.supported == 0) {
>> > UP = 0;
>> > } else {
>> > - if (!IsACValid(TID)) {
>> > + if (!IsACValid((s8)TID)) {
>> > netdev_warn(ieee->dev, "%s(): TID(%d) is not
>> > valid\n",
>> > __func__, TID);
>> > return false;
>>
>> TID is a 4-bit field, it should never go negative. The cast to s8 seems
>> wrong to me, if anything it should be using u8. I do realize the macro
>> IsACValid checks against negative too, but that just looks silly to me.
>
> Ok, I'll remove the extra comparison then to avoid the warning:
>
> staging/rtl8192e/rtl819x_TSProc.c:326:14: error: comparison is always
> true due to limited range of data type [-Werror=type-limits]
>
> I guess it should be a separate patch. I had just stumbled over the
> same thing before resending the patch but decided not to change it
> to keep the patch simple.
I think that would be better, albeit not a big issue. I'd like to get
rid of all the drivers/staging/rtl* drivers eventually :)
Cheers,
Jes
Re: [PATCH 3/3] staging/rtl8192u: use s8 instead of char
Arnd Bergmann writes: > Compiling the rtlwifi drivers for ARM with gcc -Wextra warns about lots of > incorrect code that results from 'char' being unsigned here, e.g. > > staging/rtl8192u/r8192U_core.c:4150:16: error: comparison is always false due > to limited range of data type [-Werror=type-limits] > staging/rtl8192u/r8192U_dm.c:646:50: error: comparison is always false due to > limited range of data type [-Werror=type-limits] > > This patch changes all uses of 'char' in this driver that refer to > 8-bit integers to use 's8' instead, which is signed on all architectures. > > Signed-off-by: Arnd Bergmann > --- > drivers/staging/rtl8192u/ieee80211/ieee80211.h | 4 ++-- > drivers/staging/rtl8192u/r8192U.h | 4 ++-- > drivers/staging/rtl8192u/r8192U_core.c | 14 +++--- > 3 files changed, 11 insertions(+), 11 deletions(-) Looks good to me Acked-by: Jes Sorensen
Re: [PATCH 2/3] staging/rtl8192e: use s8 instead of char
Arnd Bergmann writes:
> Compiling the rtlwifi drivers for ARM with gcc -Wextra warns about lots of
> incorrect code that results from 'char' being unsigned here, e.g.
>
> staging/rtl8192e/rtl8192e/r8192E_phy.c:1072:36: error: comparison is always
> false due to limited range of data type [-Werror=type-limits]
> staging/rtl8192e/rtl8192e/r8192E_phy.c:1104:36: error: comparison is always
> false due to limited range of data type [-Werror=type-limits]
> staging/rtl8192e/rtl8192e/rtl_core.c:1987:16: error: comparison is always
> false due to limited range of data type [-Werror=type-limits]
> staging/rtl8192e/rtl8192e/rtl_dm.c:782:37: error: comparison is always false
> due to limited range of data type [-Werror=type-limits]
> staging/rtl8192e/rtl819x_TSProc.c:326:14: error: comparison is always true
> due to limited range of data type [-Werror=type-limits]
> staging/rtl8192e/rtllib_softmac_wx.c:465:16: error: comparison is always
> false due to limited range of data type [-Werror=type-limits]
>
> This patch changes all uses of 'char' in this driver that refer to
> 8-bit integers to use 's8' instead, which is signed on all architectures.
>
> Signed-off-by: Arnd Bergmann
> ---
> drivers/staging/rtl8192e/rtl8192e/r8192E_dev.c | 8
> drivers/staging/rtl8192e/rtl8192e/r8192E_phy.c | 2 +-
> drivers/staging/rtl8192e/rtl8192e/rtl_core.c | 6 +++---
> drivers/staging/rtl8192e/rtl8192e/rtl_core.h | 8
> drivers/staging/rtl8192e/rtl819x_TSProc.c | 2 +-
> 5 files changed, 13 insertions(+), 13 deletions(-)
>
Most of this looks fine to me. One issue stands out which I don't think
is right:
> diff --git a/drivers/staging/rtl8192e/rtl819x_TSProc.c
> b/drivers/staging/rtl8192e/rtl819x_TSProc.c
> index 2c8a526773ed..e0a2fe5e6148 100644
> --- a/drivers/staging/rtl8192e/rtl819x_TSProc.c
> +++ b/drivers/staging/rtl8192e/rtl819x_TSProc.c
> @@ -323,7 +323,7 @@ bool GetTs(struct rtllib_device *ieee, struct
> ts_common_info **ppTS,
> if (ieee->current_network.qos_data.supported == 0) {
> UP = 0;
> } else {
> - if (!IsACValid(TID)) {
> + if (!IsACValid((s8)TID)) {
> netdev_warn(ieee->dev, "%s(): TID(%d) is not valid\n",
> __func__, TID);
> return false;
TID is a 4-bit field, it should never go negative. The cast to s8 seems
wrong to me, if anything it should be using u8. I do realize the macro
IsACValid checks against negative too, but that just looks silly to me.
Cheers,
Jes
Re: [PATCH] rtlwifi: use s8 instead of char
David Laight writes: > From: Arnd Bergmann >> On Wednesday, June 15, 2016 5:10:51 PM CEST Jes Sorensen wrote: >> > >> > Arnd, >> > >> > rtlwifi and rtl8xxxu are two distinct drivers managed by different >> > people. I'd be really nice if you could split this into a per driver >> > patch. >> > >> > That said, the use of char in rtl8xxxu is all as a flag indicator, so I >> > don't think the s/char/s8/ conversion is justified. I used char rather >> > than ugly bool to reduce the size of the struct. >> >> Makes sense, I'll resend without that change. If anything, the flag >> should become u8, not s8 anyway. > > Does bool:8 work ? Maybe, but bool is such an ugly datatype, so I'd rather use the other ones. Jes
Re: [PATCH] rtlwifi: use s8 instead of char
Arnd Bergmann writes: > Compiling the rtlwifi drivers for ARM with gcc -Wextra warns about lots of > incorrect code that results from 'char' being unsigned here, e.g. > > realtek/rtlwifi/rc.c:113:18: error: comparison is always true due to limited > range of data type [-Werror=type-limits] > realtek/rtlwifi/rtl8188ee/dm.c:1070:22: error: comparison is always false due > to limited range of data type [-Werror=type-limits] > realtek/rtlwifi/rtl8192ce/trx.c:54:16: error: comparison is always false due > to limited range of data type [-Werror=type-limits] > realtek/rtlwifi/rtl8192cu/mac.c:601:16: error: comparison is always false due > to limited range of data type [-Werror=type-limits] > realtek/rtlwifi/rtl8192de/trx.c:53:16: error: comparison is always false due > to limited range of data type [-Werror=type-limits] > realtek/rtlwifi/rtl8192ee/phy.c:1268:12: error: comparison is always true due > to limited range of data type [-Werror=type-limits] > realtek/rtlwifi/rtl8192se/rf.c:150:20: error: comparison is always false due > to limited range of data type [-Werror=type-limits] > realtek/rtlwifi/rtl8723be/dm.c:877:29: error: comparison is always false due > to limited range of data type [-Werror=type-limits] > realtek/rtlwifi/rtl8723be/phy.c:386:16: error: comparison is always true due > to limited range of data type [-Werror=type-limits] > realtek/rtlwifi/rtl8821ae/dm.c:1514:38: error: comparison is always false due > to limited range of data type [-Werror=type-limits] > realtek/rtlwifi/rtl8821ae/phy.c:1558:11: error: comparison is always false > due to limited range of data type [-Werror=type-limits] > realtek/rtlwifi/rtl8821ae/phy.c:386:24: error: comparison is always false due > to limited range of data type [-Werror=type-limits] > realtek/rtlwifi/rtl8821ae/trx.c:55:12: error: comparison is always false due > to limited range of data type [-Werror=type-limits] > realtek/rtlwifi/stats.c:31:16: error: comparison is always false due to > limited range of data type [-Werror=type-limits] > > This patch changes all uses of 'char' in this driver that refer to > 8-bit integers to use 's8' instead, which is signed on all architectures. > > Signed-off-by: Arnd Bergmann > --- > drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h | 6 +- > .../realtek/rtlwifi/btcoexist/halbtcoutsrc.c | 2 +- > .../realtek/rtlwifi/btcoexist/halbtcoutsrc.h | 4 +- > drivers/net/wireless/realtek/rtlwifi/rc.c | 2 +- > .../net/wireless/realtek/rtlwifi/rtl8188ee/dm.c| 4 +- > .../net/wireless/realtek/rtlwifi/rtl8188ee/trx.c | 2 +- > .../net/wireless/realtek/rtlwifi/rtl8188ee/trx.h | 4 +- > .../wireless/realtek/rtlwifi/rtl8192c/dm_common.h | 2 +- > .../wireless/realtek/rtlwifi/rtl8192c/phy_common.c | 4 +- > .../wireless/realtek/rtlwifi/rtl8192c/phy_common.h | 2 +- > .../net/wireless/realtek/rtlwifi/rtl8192ce/phy.h | 2 +- > .../net/wireless/realtek/rtlwifi/rtl8192ce/trx.c | 6 +- > .../net/wireless/realtek/rtlwifi/rtl8192ce/trx.h | 4 +- > .../net/wireless/realtek/rtlwifi/rtl8192cu/mac.c | 6 +- > .../net/wireless/realtek/rtlwifi/rtl8192cu/mac.h | 4 +- > .../net/wireless/realtek/rtlwifi/rtl8192de/phy.c | 2 +- > .../net/wireless/realtek/rtlwifi/rtl8192de/phy.h | 2 +- > .../net/wireless/realtek/rtlwifi/rtl8192de/trx.c | 6 +- > .../net/wireless/realtek/rtlwifi/rtl8192de/trx.h | 4 +- > .../net/wireless/realtek/rtlwifi/rtl8192ee/phy.c | 10 ++-- > .../net/wireless/realtek/rtlwifi/rtl8192ee/phy.h | 2 +- > .../net/wireless/realtek/rtlwifi/rtl8192ee/trx.c | 2 +- > .../net/wireless/realtek/rtlwifi/rtl8192ee/trx.h | 4 +- > .../net/wireless/realtek/rtlwifi/rtl8192se/rf.c| 2 +- > .../net/wireless/realtek/rtlwifi/rtl8723ae/trx.h | 4 +- > .../net/wireless/realtek/rtlwifi/rtl8723be/dm.c| 4 +- > .../net/wireless/realtek/rtlwifi/rtl8723be/phy.c | 4 +- > .../net/wireless/realtek/rtlwifi/rtl8723be/trx.c | 2 +- > .../net/wireless/realtek/rtlwifi/rtl8723be/trx.h | 8 +-- > .../net/wireless/realtek/rtlwifi/rtl8821ae/dm.c| 4 +- > .../net/wireless/realtek/rtlwifi/rtl8821ae/phy.c | 48 +++ > .../net/wireless/realtek/rtlwifi/rtl8821ae/phy.h | 2 +- > .../net/wireless/realtek/rtlwifi/rtl8821ae/trx.c | 12 ++-- > .../net/wireless/realtek/rtlwifi/rtl8821ae/trx.h | 10 ++-- > drivers/net/wireless/realtek/rtlwifi/stats.c | 6 +- > drivers/net/wireless/realtek/rtlwifi/stats.h | 4 +- > drivers/net/wireless/realtek/rtlwifi/wifi.h| 68 > +++-- Arnd, rtlwifi and rtl8xxxu are two distinct drivers managed by different people. I'd be really nice if you could split this into a per driver patch. That said, the use of char in rtl8xxxu is all as a flag indicator, so I don't think the s/char/s8/ conversion is justified. I used char rather than ugly bool to reduce the size of the struct. Cheers, Jes
Re: [PATCH 0/2] *** r8723au: Replace semaphore lock with mutex ***
Binoy Jayan writes: > Hi, > > These are a set of patches which removes semaphores from: > > drivers/staging/rtl8723au > > These are part of a bigger effort to eliminate all semaphores > from the linux kernel. > > They build correctly (individually and as a whole). > NB: I have not tested this as I do not have the following hardware: > > R8723AU > "Realtek RTL8723AU Wireless LAN NIC driver" I am not against this, but please note this driver is already marked for deletion in a future kernel release. Cheers, Jes
Re: [PATCH] rtl8xxxu: fix typo on variable name, compare against correct variable
Jes Sorensen writes:
> Colin King writes:
>> From: Colin Ian King
>>
>> path_b_ok is being assigned but immediately after path_a_ok is being
>> compared to the value 0x03. This appears to be a typo on the
>> variable name, compare path_b_ok instead.
>>
>> Signed-off-by: Colin Ian King
>> ---
>> drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c
>> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c
>> index fe19ace..b04cf30 100644
>> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c
>> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c
>> @@ -1149,7 +1149,7 @@ static void rtl8192eu_phy_iqcalibrate(struct
>> rtl8xxxu_priv *priv,
>>
>> for (i = 0; i < retry; i++) {
>> path_b_ok = rtl8192eu_rx_iqk_path_b(priv);
>> -if (path_a_ok == 0x03) {
>> +if (path_b_ok == 0x03) {
>> val32 = rtl8xxxu_read32(priv,
>>
>> REG_RX_POWER_BEFORE_IQK_B_2);
>> result[t][6] = (val32 >> 16) & 0x3ff;
>
> Nice catch, that does indeed look like a bug!
>
> Thanks, I want to test it, but I plan to apply it to rtl8xxxu-devel
> shortly.
Tested and applied!
Thanks,
Jes
Re: [PATCH] rtl8xxxu: fix typo on variable name, compare against correct variable
Colin King writes:
> From: Colin Ian King
>
> path_b_ok is being assigned but immediately after path_a_ok is being
> compared to the value 0x03. This appears to be a typo on the
> variable name, compare path_b_ok instead.
>
> Signed-off-by: Colin Ian King
> ---
> drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c
> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c
> index fe19ace..b04cf30 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c
> @@ -1149,7 +1149,7 @@ static void rtl8192eu_phy_iqcalibrate(struct
> rtl8xxxu_priv *priv,
>
> for (i = 0; i < retry; i++) {
> path_b_ok = rtl8192eu_rx_iqk_path_b(priv);
> - if (path_a_ok == 0x03) {
> + if (path_b_ok == 0x03) {
> val32 = rtl8xxxu_read32(priv,
>
> REG_RX_POWER_BEFORE_IQK_B_2);
> result[t][6] = (val32 >> 16) & 0x3ff;
Nice catch, that does indeed look like a bug!
Thanks, I want to test it, but I plan to apply it to rtl8xxxu-devel
shortly.
Cheers,
Jes
Re: [PATCH 0/5] add bus driver for Unisys s-Par paravirtualized devices to arch/x86
Greg KH writes: > On Tue, May 17, 2016 at 07:49:53PM -0400, Jes Sorensen wrote: >> > Ok, but still no need to put it under arch/ anything, it should go in >> > drivers/ like all other drivers and busses are, no matter what the arch >> > it happens to run on is. >> >> I don't think thats obvious. arch/x86/kvm is an example of this, Sparc >> and PPC also have their stuff under arch/. > > For some things, yes, but let's not make the same mistakes as others :) > > Look at drivers/hv/ for an example of a very x86-only bus and driver > subsystem living in drivers/ Please don't burry driver stuff in arch/ > the ARM developers are trying to fix their mistakes of the past and move > all of their cruft out of arch/ for that reason. Works for me. So should they put it in drivers/visorbus or drivers/bus/visorbus? What about drivers/virt? Would you suggest hv and xen gets moved in there? Cheers, Jes
Re: [PATCH 0/5] add bus driver for Unisys s-Par paravirtualized devices to arch/x86
Greg KH writes: > On Tue, May 17, 2016 at 10:01:55AM -0400, Jes Sorensen wrote: >> Greg KH writes: >> > On Tue, May 17, 2016 at 03:27:56AM -0400, David Kershner wrote: >> >> This patchset moves the visorbus driver >> >> (fromdrivers/staging/unisys/visorbus) >> >> and its dependent headers files (from drivers/staging/unisys/include) >> >> out of staging into the main kernel tree. >> >> >> >> The visorbus driver is a bus driver for various paravirtualized devices >> >> presented within a Unisys s-Par guest environment. Drivers for these >> >> devices are also currently present under drivers/staging/unisys/, which we >> >> intend to also move out of staging immediately after visorbus. All of >> >> these other drivers are dependent upon visorbus and the include directory, >> >> which is why we would like to move these first. >> >> >> >> Our initial consultations with various members of the community have led >> >> us >> >> to the conclusion that the most appropriate locations for these is: >> >> arch/x86/visorbus/ (driver) >> >> include/linux/visorbus/ (header files) >> >> >> >> The rationale is that visorbus is dependent on x86-64 architecture. >> > >> > What makes it dependent on x86? What prevents it from running on some >> > other architecture (not the fact that no one has made such hardware, >> > just the code reasons please.) >> >> It's dependent on system firmware which is only available on the S-Par >> platform which is x86_64 only. The closest similarity is probably what >> you find on the PPC and Sparc platforms. > > Ok, but still no need to put it under arch/ anything, it should go in > drivers/ like all other drivers and busses are, no matter what the arch > it happens to run on is. I don't think thats obvious. arch/x86/kvm is an example of this, Sparc and PPC also have their stuff under arch/. I am open, if people prefer to have drivers/visorbus I can support that. I find right now it's really messy with things being put all over the place, and it's not obvious what the real placement is for all of this. Jes
Re: [PATCH 0/5] add bus driver for Unisys s-Par paravirtualized devices to arch/x86
Greg KH writes: > On Tue, May 17, 2016 at 03:27:56AM -0400, David Kershner wrote: >> This patchset moves the visorbus driver (fromdrivers/staging/unisys/visorbus) >> and its dependent headers files (from drivers/staging/unisys/include) >> out of staging into the main kernel tree. >> >> The visorbus driver is a bus driver for various paravirtualized devices >> presented within a Unisys s-Par guest environment. Drivers for these >> devices are also currently present under drivers/staging/unisys/, which we >> intend to also move out of staging immediately after visorbus. All of >> these other drivers are dependent upon visorbus and the include directory, >> which is why we would like to move these first. >> >> Our initial consultations with various members of the community have led us >> to the conclusion that the most appropriate locations for these is: >> arch/x86/visorbus/ (driver) >> include/linux/visorbus/ (header files) >> >> The rationale is that visorbus is dependent on x86-64 architecture. > > What makes it dependent on x86? What prevents it from running on some > other architecture (not the fact that no one has made such hardware, > just the code reasons please.) It's dependent on system firmware which is only available on the S-Par platform which is x86_64 only. The closest similarity is probably what you find on the PPC and Sparc platforms. Jes
Re: [PATCH v2] staging: r8723au: This patch tries to fix some byte order issues that is found by sparse check.
Jandy Gou writes:
> make C=1 M=drivers/staging/rtl8723au/
>
> drivers/staging/rtl8723au/hal/rtl8723a_cmd.c:96:38: warning: cast to
> restricted __le16
> drivers/staging/rtl8723au/hal/rtl8723a_cmd.c:100:27: warning: cast to
> restricted __le32
>
> Signed-off-by: Jandy Gou
> ---
> drivers/staging/rtl8723au/hal/rtl8723a_cmd.c | 12 +---
> 1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
> b/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
> index 1662c03c..fff652c 100644
> --- a/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
> +++ b/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
> @@ -56,8 +56,8 @@ int FillH2CCmd(struct rtw_adapter *padapter, u8 ElementID,
> u32 CmdLen,
> u32 msgbox_addr;
> u32 msgbox_ex_addr;
> struct hal_data_8723a *pHalData;
> - u32 h2c_cmd = 0;
> - u16 h2c_cmd_ex = 0;
> + __le32 h2c_cmd = 0;
> + __le16 h2c_cmd_ex = 0;
> int ret = _FAIL;
>
> padapter = GET_PRIMARY_ADAPTER(padapter);
> @@ -91,14 +91,12 @@ int FillH2CCmd(struct rtw_adapter *padapter, u8
> ElementID, u32 CmdLen,
>
> *(u8 *)(&h2c_cmd) |= ElementID;
>
> - if (h2c_cmd & BIT(7)) {
> + if (le32_to_cpu(h2c_cmd) & BIT(7)) {
> msgbox_ex_addr = REG_HMEBOX_EXT_0 + (h2c_box_num *
> EX_MESSAGE_BOX_SIZE);
> - h2c_cmd_ex = le16_to_cpu(h2c_cmd_ex);
> - rtl8723au_write16(padapter, msgbox_ex_addr, h2c_cmd_ex);
> + rtl8723au_write16(padapter, msgbox_ex_addr,
> le16_to_cpu(h2c_cmd_ex));
> }
This actually changes the behavior on big-endian systems. Did you test
this?
Second, the last line makes the line more than 80 charaters whish is
wrong.
Jes
Re: [PATCH] rtl8xxxu: hide unused tables
Kalle Valo writes:
> Jes Sorensen writes:
>
>> Arnd Bergmann writes:
>>> The references to some arrays in the rtl8xxxu driver were moved inside
>>> of an #ifdef, but the symbols remain outside, resulting in build warnings:
>>>
>>> rtl8xxxu/rtl8xxxu.c:1506:33: error:
>>> 'rtl8188ru_radioa_1t_highpa_table' defined but not used
>>> rtl8xxxu/rtl8xxxu.c:1431:33: error:
>>> 'rtl8192cu_radioa_1t_init_table' defined but not used
>>> rtl8xxxu/rtl8xxxu.c:1407:33: error:
>>> 'rtl8192cu_radiob_2t_init_table' defined but not used
>>> rtl8xxxu/rtl8xxxu.c:1332:33: error:
>>> 'rtl8192cu_radioa_2t_init_table' defined but not used
>>> rtl8xxxu/rtl8xxxu.c:239:35: error: 'rtl8192c_power_base' defined but not
>>> used
>>> rtl8xxxu/rtl8xxxu.c:217:35: error: 'rtl8188r_power_base' defined but not
>>> used
>>>
>>> This adds an extra #ifdef around them to shut up the warnings.
>>>
>>> Signed-off-by: Arnd Bergmann
>>> Fixes: 2fc0b8e5a17d ("rtl8xxxu: Add TX power base values for gen1 parts")
>>> Fixes: 4062b8ffec36 ("rtl8xxxu: Move PHY RF init into device
>>> specific functions")
>>> ---
>>> drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c | 4
>>> 1 file changed, 4 insertions(+)
>>
>> I'll apply it to my tree!
>
> Actually I would prefer to apply this directly to wireless-drivers-next
> so that the warnings are quickly fixed.
Thats fine with me, I already applied it here before I started doing any
of the refactoring work, so we should be in sync.
Cheers,
Jes
Re: [PATCH] rtl8xxxu: hide unused tables
Arnd Bergmann writes:
> The references to some arrays in the rtl8xxxu driver were moved inside
> of an #ifdef, but the symbols remain outside, resulting in build warnings:
>
> rtl8xxxu/rtl8xxxu.c:1506:33: error: 'rtl8188ru_radioa_1t_highpa_table'
> defined but not used
> rtl8xxxu/rtl8xxxu.c:1431:33: error: 'rtl8192cu_radioa_1t_init_table' defined
> but not used
> rtl8xxxu/rtl8xxxu.c:1407:33: error: 'rtl8192cu_radiob_2t_init_table' defined
> but not used
> rtl8xxxu/rtl8xxxu.c:1332:33: error: 'rtl8192cu_radioa_2t_init_table' defined
> but not used
> rtl8xxxu/rtl8xxxu.c:239:35: error: 'rtl8192c_power_base' defined but not used
> rtl8xxxu/rtl8xxxu.c:217:35: error: 'rtl8188r_power_base' defined but not used
>
> This adds an extra #ifdef around them to shut up the warnings.
>
> Signed-off-by: Arnd Bergmann
> Fixes: 2fc0b8e5a17d ("rtl8xxxu: Add TX power base values for gen1 parts")
> Fixes: 4062b8ffec36 ("rtl8xxxu: Move PHY RF init into device specific
> functions")
> ---
> drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c | 4
> 1 file changed, 4 insertions(+)
I'll apply it to my tree!
Thanks,
Jes
Re: [PATCH] rtl8xxxu: fix uninitialized return value in ret
Colin King writes:
> From: Colin Ian King
>
> several functions are not initializing a return status in ret
> resulting in garbage to be returned instead of 0 for success.
> Currently, the calls to these functions are not checking the
> return, however, it seems prudent to return the correct status
> in case they are to be checked at a later date.
>
> Signed-off-by: Colin Ian King
> ---
> drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
Thanks for the patch! I'm surprised the compiler didn't warn about this.
I'll add it to my queue for rtl8xxxu.
Cheers,
Jes
>
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c
> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c
> index abdff45..9262aad 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c
> @@ -5231,7 +5231,7 @@ static void rtl8xxxu_set_ampdu_min_space(struct
> rtl8xxxu_priv *priv, u8 density)
> static int rtl8xxxu_active_to_emu(struct rtl8xxxu_priv *priv)
> {
> u8 val8;
> - int count, ret;
> + int count, ret = 0;
>
> /* Start of rtl8723AU_card_enable_flow */
> /* Act to Cardemu sequence*/
> @@ -5281,7 +5281,7 @@ static int rtl8723bu_active_to_emu(struct rtl8xxxu_priv
> *priv)
> u8 val8;
> u16 val16;
> u32 val32;
> - int count, ret;
> + int count, ret = 0;
>
> /* Turn off RF */
> rtl8xxxu_write8(priv, REG_RF_CTRL, 0);
> @@ -5338,7 +5338,7 @@ static int rtl8xxxu_active_to_lps(struct rtl8xxxu_priv
> *priv)
> {
> u8 val8;
> u8 val32;
> - int count, ret;
> + int count, ret = 0;
>
> rtl8xxxu_write8(priv, REG_TXPAUSE, 0xff);
Re: [PATCH] Staging: rtl8723au: Remove unused functions
Bhumika Goyal writes: > The functions rtw_get_oper_bw23a and rtw_get_oper_ch23aoffset are never > used anywhere in the kernel. So, remove their definition and prototype. > Grepped to find occurences. > > Signed-off-by: Bhumika Goyal > --- > drivers/staging/rtl8723au/core/rtw_wlan_util.c | 10 -- > drivers/staging/rtl8723au/include/rtw_mlme_ext.h | 2 -- > 2 files changed, 12 deletions(-) Acked-by: Jes Sorensen
Re: [PATCH] Staging: rtl8723au: Remove function rtw_enqueue_{recvbuf23a/recvbuf23a_to_head}
Bhumika Goyal writes:
> The functions rtw_enqueue_recvbuf23a and rtw_enqueue_recvbuf23a_to_head
> are never used anywhere in the kernel. So, remove their definition and
> prototype.
> Grepped to find occurences.
>
> Signed-off-by: Bhumika Goyal
> ---
> drivers/staging/rtl8723au/core/rtw_recv.c| 25 -
> drivers/staging/rtl8723au/include/rtw_recv.h | 2 --
> 2 files changed, 27 deletions(-)
Looks reasonable to me.
Acked-by: Jes Sorensen
Jes
> diff --git a/drivers/staging/rtl8723au/core/rtw_recv.c
> b/drivers/staging/rtl8723au/core/rtw_recv.c
> index 989ed07..150dabc 100644
> --- a/drivers/staging/rtl8723au/core/rtw_recv.c
> +++ b/drivers/staging/rtl8723au/core/rtw_recv.c
> @@ -211,31 +211,6 @@ u32 rtw_free_uc_swdec_pending_queue23a(struct
> rtw_adapter *adapter)
> return cnt;
> }
>
> -int rtw_enqueue_recvbuf23a_to_head(struct recv_buf *precvbuf, struct
> rtw_queue *queue)
> -{
> - spin_lock_bh(&queue->lock);
> -
> - list_del_init(&precvbuf->list);
> - list_add(&precvbuf->list, get_list_head(queue));
> -
> - spin_unlock_bh(&queue->lock);
> -
> - return _SUCCESS;
> -}
> -
> -int rtw_enqueue_recvbuf23a(struct recv_buf *precvbuf, struct rtw_queue
> *queue)
> -{
> - unsigned long irqL;
> -
> - spin_lock_irqsave(&queue->lock, irqL);
> -
> - list_del_init(&precvbuf->list);
> -
> - list_add_tail(&precvbuf->list, get_list_head(queue));
> - spin_unlock_irqrestore(&queue->lock, irqL);
> - return _SUCCESS;
> -}
> -
> struct recv_buf *rtw_dequeue_recvbuf23a (struct rtw_queue *queue)
> {
> unsigned long irqL;
> diff --git a/drivers/staging/rtl8723au/include/rtw_recv.h
> b/drivers/staging/rtl8723au/include/rtw_recv.h
> index dc784be..85a5edb 100644
> --- a/drivers/staging/rtl8723au/include/rtw_recv.h
> +++ b/drivers/staging/rtl8723au/include/rtw_recv.h
> @@ -279,8 +279,6 @@ int rtw_enqueue_recvframe23a(struct recv_frame
> *precvframe, struct rtw_queue *qu
>
> u32 rtw_free_uc_swdec_pending_queue23a(struct rtw_adapter *adapter);
>
> -int rtw_enqueue_recvbuf23a_to_head(struct recv_buf *precvbuf, struct
> rtw_queue *queue);
> -int rtw_enqueue_recvbuf23a(struct recv_buf *precvbuf, struct rtw_queue
> *queue);
> struct recv_buf *rtw_dequeue_recvbuf23a(struct rtw_queue *queue);
>
> void rtw_reordering_ctrl_timeout_handler23a(unsigned long pcontext);
Re: [PATCH] staging: r8723au: This patch tries to fix some byte order issues that is found by sparse check.
Julian Calaby writes:
> Hi Jandy,
>
> On Thu, Mar 17, 2016 at 7:03 PM, Jandy Gou
> wrote:
>> make C=1 M=drivers/staging/rtl8723au/
>>
>> drivers/staging/rtl8723au/hal/rtl8723a_cmd.c:96:38: warning: cast to
>> restricted __le16
>> drivers/staging/rtl8723au/hal/rtl8723a_cmd.c:100:27: warning: cast to
>> restricted __le32
>>
>> Signed-off-by: Jandy Gou
>
> I'm not sure your change is correct. Perhaps you should add temporary
> variables for h2c_cmd_ex and h2c_cmd while they're cpu-endian?
>
> Jes,
>
> I'm pretty sure this isn't the first time this has come up. Do you
> have any ideas for a solution? Or should we ignore this as this driver
> will eventually be going away?
Temp variables as you suggest would be a clean way to accomplish this.
Technically this might work, but esthetically this is so gross I wish I
had never seen it. There is a reason why we have the byteorder specific
types, and le{16,32}_to_cpus() violates that.
I am surprised we still have these macros around, tbh I didn't even know
they existed. A quick search for le16_to_cpus() shows that it's really
very old drivers and some more recent Intel Ethernet drivers that use
them - maybe this would be a good time to get rid of them completely.
Cheers,
Jes
Re: [PATCH] staging: r8723au: This patch tries to fix some byte order issues that is found by sparse check.
Jandy Gou writes:
> make C=1 M=drivers/staging/rtl8723au/
>
> drivers/staging/rtl8723au/hal/rtl8723a_cmd.c:96:38: warning: cast to
> restricted __le16
> drivers/staging/rtl8723au/hal/rtl8723a_cmd.c:100:27: warning: cast to
> restricted __le32
>
> Signed-off-by: Jandy Gou
> ---
> drivers/staging/rtl8723au/hal/rtl8723a_cmd.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
> b/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
> index 1662c03c..d82fd8a 100644
> --- a/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
> +++ b/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
> @@ -93,11 +93,11 @@ int FillH2CCmd(struct rtw_adapter *padapter, u8
> ElementID, u32 CmdLen,
>
> if (h2c_cmd & BIT(7)) {
> msgbox_ex_addr = REG_HMEBOX_EXT_0 + (h2c_box_num *
> EX_MESSAGE_BOX_SIZE);
> - h2c_cmd_ex = le16_to_cpu(h2c_cmd_ex);
> + le16_to_cpus(&h2c_cmd_ex);
> rtl8723au_write16(padapter, msgbox_ex_addr, h2c_cmd_ex);
> }
> msgbox_addr = REG_HMEBOX_0 + (h2c_box_num * MESSAGE_BOX_SIZE);
> - h2c_cmd = le32_to_cpu(h2c_cmd);
> + le32_to_cpus(&h2c_cmd);
> rtl8723au_write32(padapter, msgbox_addr, h2c_cmd);
Please do *not* use le{16,32}_to_cpus() that is so gross it's hard to
even put words on it. Use a temp variable of the correct type instead.
NAK
Jes
Re: [PATCH] staging: rtl8723au: use list_first_entry*
Geliang Tang writes:
> Use list_first_entry*() instead of container_of() to simplify the code.
>
> Signed-off-by: Geliang Tang
> ---
> drivers/staging/rtl8723au/core/rtw_recv.c | 49
> +--
> drivers/staging/rtl8723au/core/rtw_xmit.c | 26 +---
> 2 files changed, 22 insertions(+), 53 deletions(-)
This looks fine to me. When these changes gets large, it may be better
to break them down into multiple patches as it's easier to debug if
there is a bug somewhere.
Acked-by: Jes Sorensen
>
> diff --git a/drivers/staging/rtl8723au/core/rtw_recv.c
> b/drivers/staging/rtl8723au/core/rtw_recv.c
> index 0a7741c..b095d09 100644
> --- a/drivers/staging/rtl8723au/core/rtw_recv.c
> +++ b/drivers/staging/rtl8723au/core/rtw_recv.c
> @@ -104,21 +104,14 @@ void _rtw_free_recv_priv23a(struct recv_priv *precvpriv)
> struct recv_frame *rtw_alloc_recvframe23a(struct rtw_queue *pfree_recv_queue)
> {
> struct recv_frame *pframe;
> - struct list_head *plist, *phead;
> struct rtw_adapter *padapter;
> struct recv_priv *precvpriv;
>
> spin_lock_bh(&pfree_recv_queue->lock);
>
> - if (list_empty(&pfree_recv_queue->queue))
> - pframe = NULL;
> - else {
> - phead = get_list_head(pfree_recv_queue);
> -
> - plist = phead->next;
> -
> - pframe = container_of(plist, struct recv_frame, list);
> -
> + pframe = list_first_entry_or_null(&pfree_recv_queue->queue,
> + struct recv_frame, list);
> + if (pframe) {
> list_del_init(&pframe->list);
> padapter = pframe->adapter;
> if (padapter) {
> @@ -243,25 +236,17 @@ int rtw_enqueue_recvbuf23a(struct recv_buf *precvbuf,
> struct rtw_queue *queue)
> return _SUCCESS;
> }
>
> -struct recv_buf *rtw_dequeue_recvbuf23a (struct rtw_queue *queue)
> +struct recv_buf *rtw_dequeue_recvbuf23a(struct rtw_queue *queue)
> {
> unsigned long irqL;
> struct recv_buf *precvbuf;
> - struct list_head *plist, *phead;
>
> spin_lock_irqsave(&queue->lock, irqL);
>
> - if (list_empty(&queue->queue)) {
> - precvbuf = NULL;
> - } else {
> - phead = get_list_head(queue);
> -
> - plist = phead->next;
> -
> - precvbuf = container_of(plist, struct recv_buf, list);
> -
> + precvbuf = list_first_entry_or_null(&queue->queue,
> + struct recv_buf, list);
> + if (precvbuf)
> list_del_init(&precvbuf->list);
> - }
>
> spin_unlock_irqrestore(&queue->lock, irqL);
>
> @@ -1079,22 +1064,17 @@ static int validate_recv_ctrl_frame(struct
> rtw_adapter *padapter,
>
> if ((psta->state & WIFI_SLEEP_STATE) &&
> (pstapriv->sta_dz_bitmap & CHKBIT(psta->aid))) {
> - struct list_head *xmitframe_plist, *xmitframe_phead;
> + struct list_head *xmitframe_phead;
> struct xmit_frame *pxmitframe;
> struct xmit_priv *pxmitpriv = &padapter->xmitpriv;
>
> spin_lock_bh(&pxmitpriv->lock);
>
> xmitframe_phead = get_list_head(&psta->sleep_q);
> - xmitframe_plist = xmitframe_phead->next;
> -
> - if (!list_empty(xmitframe_phead)) {
> - pxmitframe = container_of(xmitframe_plist,
> - struct xmit_frame,
> - list);
> -
> - xmitframe_plist = xmitframe_plist->next;
> -
> + pxmitframe = list_first_entry_or_null(xmitframe_phead,
> + struct xmit_frame,
> + list);
> + if (pxmitframe) {
> list_del_init(&pxmitframe->list);
>
> psta->sleepq_len--;
> @@ -1542,7 +1522,7 @@ struct recv_frame *recvframe_defrag(struct rtw_adapter
> *adapter,
> struct recv_frame *recvframe_defrag(struct rtw_adapter *adapter,
> struct rtw_queue *defrag_q)
> {
> - struct list_head *plist, *phead;
> + struct list_head *phead;
> u8 wlanhdr_offset;
> u8 curfragnum;
> struct recv_frame *pnfhdr, *ptmp;
> @@ -1554,8 +1534,7 @@
Re: [PATCH v5 0/3] staging: rtl8723au: use list_for_each_entry*() and cleaning
Geliang Tang writes: > On Wed, Feb 17, 2016 at 12:28:33PM -0500, Jes Sorensen wrote: >> kbuild test robot writes: >> > All errors (new ones prefixed by >>): >> > >> >drivers/staging/rtl8723au/core/rtw_recv.c: In function >> > 'rtw_free_recvframe23a_queue': >> >>> drivers/staging/rtl8723au/core/rtw_recv.c:203:2: error: 'plist' >> >>> undeclared (first use in this function) >> > plist = phead->next; >> > ^ >> >> This doesn't work, you cannot break interim builds. Your patch 2 needs >> to go before patch 1, and you must always check that they compile for >> each patch you apply. >> >> NACK > > Sorry for the trouble caused. I updated my patchs to fix this problem. > > - Geliang > > This patch set uses list_for_each_entry*() instead of > list_for_each*(), removes useless codes, and cleans > whitespaces and blank lines. No worries, I much prefer taking a few rounds to get it right. This set looks good to me. Acked-by: Jes Sorensen
Re: [PATCH v4 1/3] staging: rtl8723au: use list_for_each_entry*()
kbuild test robot writes: > Hi Geliang, > > [auto build test ERROR on staging/staging-testing] > [also build test ERROR on v4.5-rc4 next-20160217] > [if your patch is applied to the wrong git tree, please drop us a note to > help improving the system] > > url: > https://github.com/0day-ci/linux/commits/Geliang-Tang/staging-rtl8723au-use-list_for_each_entry/20160217-220638 > config: i386-randconfig-s1-201607 (attached as .config) > reproduce: > # save the attached .config to linux build tree > make ARCH=i386 > > Note: the > linux-review/Geliang-Tang/staging-rtl8723au-use-list_for_each_entry/20160217-220638 > HEAD 495811a52aba181af76c3baf57da3d81a79c2fe8 builds fine. > It only hurts bisectibility. > > All errors (new ones prefixed by >>): > >drivers/staging/rtl8723au/core/rtw_recv.c: In function > 'rtw_free_recvframe23a_queue': >>> drivers/staging/rtl8723au/core/rtw_recv.c:203:2: error: 'plist' undeclared >>> (first use in this function) > plist = phead->next; > ^ This doesn't work, you cannot break interim builds. Your patch 2 needs to go before patch 1, and you must always check that they compile for each patch you apply. NACK Jes
Re: [PATCH v3 3/3] staging: rtl8723au: whitespace and blank line cleaning
Julian Calaby writes: > Hi Geliang, > > On Sun, Feb 7, 2016 at 2:30 PM, Geliang Tang wrote: >> This patch cleans whitespaces and blank lines surrounding >> list_for_each_entry*(). > > It does a lot more than this, including: > - Removing some unnecessary brackets > - Whitespace changes well away from the list_for_each_entry*() calls > > You need to either specify all of these in the changelog or split it > up into multiple patches. > > Thanks, > > Julian Calaby I agree, I think a more descriptive patch message will suffice here. Jes
Re: [PATCH v3 2/3] staging: rtl8723au: core: rtw_recv: remove useless codes
Geliang Tang writes:
> There are some useless codes in rtw_free_recvframe23a_queue() and
> recvframe_defrag(), so remove them.
>
> Signed-off-by: Geliang Tang
> ---
> Changes in v3:
> - split it into three patches.
> Changes in v2:
> - drop the coding style fixing in v1.
> ---
> drivers/staging/rtl8723au/core/rtw_recv.c | 7 +--
> 1 file changed, 1 insertion(+), 6 deletions(-)
I assume this requires patch 1/3 applied first?
Acked-by: Jes Sorensen
>
> diff --git a/drivers/staging/rtl8723au/core/rtw_recv.c
> b/drivers/staging/rtl8723au/core/rtw_recv.c
> index 18b7d03..b36bc6b 100644
> --- a/drivers/staging/rtl8723au/core/rtw_recv.c
> +++ b/drivers/staging/rtl8723au/core/rtw_recv.c
> @@ -201,7 +201,6 @@ static void rtw_free_recvframe23a_queue(struct rtw_queue
> *pframequeue)
> spin_lock(&pframequeue->lock);
>
> phead = get_list_head(pframequeue);
> - plist = phead->next;
>
> list_for_each_entry_safe(hdr, ptmp, phead, list) {
> rtw_free_recvframe23a(hdr);
> @@ -1567,7 +1566,7 @@ struct recv_frame *recvframe_defrag(struct rtw_adapter
> *adapter,
> struct rtw_queue *defrag_q)
> {
> struct list_head *plist, *phead;
> - u8 *data, wlanhdr_offset;
> + u8 wlanhdr_offset;
> u8 curfragnum;
> struct recv_frame *pnfhdr, *ptmp;
> struct recv_frame *prframe, *pnextrframe;
> @@ -1596,10 +1595,6 @@ struct recv_frame *recvframe_defrag(struct rtw_adapter
> *adapter,
>
> curfragnum++;
>
> - phead = get_list_head(defrag_q);
> -
> - data = prframe->pkt->data;
> -
> list_for_each_entry_safe(pnfhdr, ptmp, phead, list) {
> pnextrframe = (struct recv_frame *)pnfhdr;
> /* check the fragment sequence (2nd ~n fragment frame) */
Re: [PATCH v3 1/3] staging: rtl8723au: use list_for_each_entry*()
Geliang Tang writes:
> Use list_for_each_entry*() instead of list_for_each*() to simplify
> the code.
>
> Signed-off-by: Geliang Tang
> ---
> Changes in v3:
> - split it into three patches.
> Changes in v2:
> - drop the coding style fixing in v1.
> ---
> drivers/staging/rtl8723au/core/rtw_ap.c | 55 ---
> drivers/staging/rtl8723au/core/rtw_mlme.c | 26 -
> drivers/staging/rtl8723au/core/rtw_mlme_ext.c | 10 ++--
> drivers/staging/rtl8723au/core/rtw_recv.c | 22
> drivers/staging/rtl8723au/core/rtw_sta_mgt.c | 25 -
> drivers/staging/rtl8723au/core/rtw_xmit.c | 64
> ++-
> drivers/staging/rtl8723au/os_dep/ioctl_cfg80211.c | 14 +++--
> drivers/staging/rtl8723au/os_dep/usb_ops_linux.c | 9 ++--
> 8 files changed, 96 insertions(+), 129 deletions(-)
This one is mostly OK, however when you do multi patch sets, always
include a cover letter describing the overall changes of the set.
A few nit picks:
> diff --git a/drivers/staging/rtl8723au/core/rtw_recv.c
> b/drivers/staging/rtl8723au/core/rtw_recv.c
> index 404b618..18b7d03 100644
> --- a/drivers/staging/rtl8723au/core/rtw_recv.c
> +++ b/drivers/staging/rtl8723au/core/rtw_recv.c
> @@ -88,13 +88,13 @@ int _rtw_init_recv_priv23a(struct recv_priv *precvpriv,
> void _rtw_free_recv_priv23a (struct recv_priv *precvpriv)
> {
> struct rtw_adapter *padapter = precvpriv->adapter;
> - struct recv_frame *precvframe;
> - struct list_head *plist, *ptmp;
> + struct recv_frame *precvframe, *ptmp;
>
> rtw_free_uc_swdec_pending_queue23a(padapter);
>
> - list_for_each_safe(plist, ptmp, &precvpriv->free_recv_queue.queue) {
> - precvframe = container_of(plist, struct recv_frame, list);
> + list_for_each_entry_safe(precvframe, ptmp,
> + &precvpriv->free_recv_queue.queue,
> + list) {
> list_del_init(&precvframe->list);
> kfree(precvframe);
> }
Too aggressive line breaking, the 'list' fits within 80 characters on
the line above.
> @@ -195,16 +195,15 @@ using spinlock to protect
>
> static void rtw_free_recvframe23a_queue(struct rtw_queue *pframequeue)
> {
> - struct recv_frame *hdr;
> - struct list_head *plist, *phead, *ptmp;
> + struct recv_frame *hdr, *ptmp;
> + struct list_head *phead;
>
> spin_lock(&pframequeue->lock);
>
> phead = get_list_head(pframequeue);
> plist = phead->next;
>
> - list_for_each_safe(plist, ptmp, phead) {
> - hdr = container_of(plist, struct recv_frame, list);
> + list_for_each_entry_safe(hdr, ptmp, phead, list) {
> rtw_free_recvframe23a(hdr);
> }
>
You could remove the brackets here, since you are fixing that specific
line. I am fine with this as is, some of the checkpatch police force
might bite over it.
On overall this patch is a lot better than the first version.
All set and done, I am thinking of removing this driver once Kalle pulls
in my currently posted set of changes for rtl8xxxu, plus the next one I
have lined up. I seems to me rtl8xxxu can replace rtl8723au at this
point.
I will probably mark rtl8723au deprecated after 4.6 comes out, and
remove the driver around 4.7.
Cheers,
Jes
Re: [PATCH v2] staging: rtl8723au: use list_for_each_entry*()
Geliang Tang writes:
> Use list_for_each_entry*() instead of list_for_each*() to simplify
> the code.
>
> Signed-off-by: Geliang Tang
> ---
> Changes in v2:
> - drop the coding style fixing in v1.
> ---
> drivers/staging/rtl8723au/core/rtw_ap.c | 92
> ++-
> drivers/staging/rtl8723au/core/rtw_mlme.c | 38 +++---
> drivers/staging/rtl8723au/core/rtw_mlme_ext.c | 15 +---
> drivers/staging/rtl8723au/core/rtw_recv.c | 43 ---
> drivers/staging/rtl8723au/core/rtw_sta_mgt.c | 36 +++--
> drivers/staging/rtl8723au/core/rtw_xmit.c | 90 --
> drivers/staging/rtl8723au/os_dep/ioctl_cfg80211.c | 22 ++
> drivers/staging/rtl8723au/os_dep/usb_ops_linux.c | 9 +--
> 8 files changed, 106 insertions(+), 239 deletions(-)
You still include too much whitespace and blank line cleaning with the
actual code changes. Please split this into two patches, one that
changes the code, and one that does the whitespace cleanups.
Jes
>
> diff --git a/drivers/staging/rtl8723au/core/rtw_ap.c
> b/drivers/staging/rtl8723au/core/rtw_ap.c
> index 1aa9b26..ce4b589 100644
> --- a/drivers/staging/rtl8723au/core/rtw_ap.c
> +++ b/drivers/staging/rtl8723au/core/rtw_ap.c
> @@ -171,24 +171,20 @@ static u8 chk_sta_is_alive(struct sta_info *psta)
> return ret;
> }
>
> -void expire_timeout_chk23a(struct rtw_adapter *padapter)
> +void expire_timeout_chk23a(struct rtw_adapter *padapter)
> {
> - struct list_head *phead, *plist, *ptmp;
> + struct list_head *phead;
> u8 updated = 0;
> - struct sta_info *psta;
> + struct sta_info *psta, *ptmp;
> struct sta_priv *pstapriv = &padapter->stapriv;
> u8 chk_alive_num = 0;
> struct sta_info *chk_alive_list[NUM_STA];
> int i;
>
> spin_lock_bh(&pstapriv->auth_list_lock);
> -
> phead = &pstapriv->auth_list;
> -
> /* check auth_queue */
> - list_for_each_safe(plist, ptmp, phead) {
> - psta = container_of(plist, struct sta_info, auth_list);
> -
> + list_for_each_entry_safe(psta, ptmp, phead, auth_list) {
> if (psta->expire_to > 0) {
> psta->expire_to--;
> if (psta->expire_to == 0) {
> @@ -206,19 +202,13 @@ voidexpire_timeout_chk23a(struct rtw_adapter
> *padapter)
> spin_lock_bh(&pstapriv->auth_list_lock);
> }
> }
> -
> }
> -
> spin_unlock_bh(&pstapriv->auth_list_lock);
>
> spin_lock_bh(&pstapriv->asoc_list_lock);
> -
> phead = &pstapriv->asoc_list;
> -
> /* check asoc_queue */
> - list_for_each_safe(plist, ptmp, phead) {
> - psta = container_of(plist, struct sta_info, asoc_list);
> -
> + list_for_each_entry_safe(psta, ptmp, phead, asoc_list) {
> if (chk_sta_is_alive(psta) || !psta->expire_to) {
> psta->expire_to = pstapriv->expire_to;
> psta->keep_alive_trycnt = 0;
> @@ -283,7 +273,6 @@ void expire_timeout_chk23a(struct rtw_adapter
> *padapter)
> }
> }
> }
> -
> spin_unlock_bh(&pstapriv->asoc_list_lock);
>
> if (chk_alive_num) {
> @@ -1059,7 +1048,7 @@ void rtw_set_macaddr_acl23a(struct rtw_adapter
> *padapter, int mode)
>
> int rtw_acl_add_sta23a(struct rtw_adapter *padapter, u8 *addr)
> {
> - struct list_head *plist, *phead;
> + struct list_head *phead;
> u8 added = false;
> int i, ret = 0;
> struct rtw_wlan_acl_node *paclnode;
> @@ -1073,12 +1062,8 @@ int rtw_acl_add_sta23a(struct rtw_adapter *padapter,
> u8 *addr)
> return -1;
>
> spin_lock_bh(&pacl_node_q->lock);
> -
> phead = get_list_head(pacl_node_q);
> -
> - list_for_each(plist, phead) {
> - paclnode = container_of(plist, struct rtw_wlan_acl_node, list);
> -
> + list_for_each_entry(paclnode, phead, list) {
> if (!memcmp(paclnode->addr, addr, ETH_ALEN)) {
> if (paclnode->valid == true) {
> added = true;
> @@ -1087,7 +1072,6 @@ int rtw_acl_add_sta23a(struct rtw_adapter *padapter, u8
> *addr)
> }
> }
> }
> -
> spin_unlock_bh(&pacl_node_q->lock);
>
> if (added)
> @@ -1121,8 +1105,8 @@ int rtw_acl_add_sta23a(struct rtw_adapter *padapter, u8
> *addr)
>
> int rtw_acl_remove_sta23a(struct rtw_adapter *padapter, u8 *addr)
> {
> - struct list_head *plist, *phead, *ptmp;
> - struct rtw_wlan_acl_node *paclnode;
> + struct list_head *phead;
> + struct rtw_wlan_acl_node *paclnode, *ptmp;
> struct sta_priv *pstapriv = &padapter->stapriv;
> struct wlan_acl_pool *pacl_list = &pstapriv->acl_list;
> struct rtw_queue *pacl_node_q = &pacl_list->acl_node_q;
> @@ -1130,12 +1114,8 @@ int rtw_acl_remove_sta23a(struct rtw_adapter
> *padapter, u8 *a
Re: WANTED new maintainer for Linux/md (and mdadm)
NeilBrown writes: > On Wed, Jan 20 2016, Artur Paszkiewicz wrote: >> >> Hi Neil, >> >> Thank you for your work and time spent on maintaining MD/mdadm. I would also >> like to offer help for the emerging maintainership team. I've been >> working with >> MD RAID for more than 4 years, mostly testing and developing the IMSM-related >> parts on behalf of my employer - Intel. I realize that I was not very visible >> on this mailing list, but I think I have a pretty good knowledge about mdadm >> and the MD drivers. Now I have Intel's approval to take on maintaining MD >> RAID >> as part of my job, not focusing primarily on IMSM. I definitely feel more >> confident with maintaining mdadm, but I would certainly like to learn more >> about the kernel MD stack and help with it as much as I can. > > Hi Arthur, > thanks for all your IMSM work over the years! It is great that you can > continue contributing and do so more broadly. > > For the moment Jes Serensen will be co-ordinating mdadm (once I make a > release ... tomorrow?) and Shaohua Li will be looking after the > kernel/md side. I'm sure there is plenty of room for you too though. > Only one person (at a time) can queue patches, but several can > collaborate at development and support and bug fixing and testing and > > I suggest you find ways to co-ordinate with them. > > This: > https://bugzilla.kernel.org/show_bug.cgi?id=108741 > is currently most important in my mind, but there are other things to > do. Hi Arthur, I have setup a git repository for mdadm here: git://git.kernel.org/pub/scm/utils/mdadm/mdadm.git I'll be relying heavily on your contributions in managing this now that Neil is leaving it to us. I'll do my best to maintain it in a similar manner to Neil, albeit I am probably going to screw up a lot more often. Please fire away those patches and do not be afraid to yell at me if I get something wrong! Cheers, Jes
Re: [PATCH] staging: rtl8723au: use list_for_each_entry*()
Geliang Tang writes:
> Use list_for_each_entry*() instead of list_for_each*() to simplify
> the code. Fix coding style by the way.
>
> Signed-off-by: Geliang Tang
> ---
> drivers/staging/rtl8723au/core/rtw_ap.c | 166
> --
> drivers/staging/rtl8723au/core/rtw_mlme.c | 38 ++---
> drivers/staging/rtl8723au/core/rtw_mlme_ext.c | 13 +-
> drivers/staging/rtl8723au/core/rtw_recv.c | 43 ++
> drivers/staging/rtl8723au/core/rtw_sta_mgt.c | 34 ++---
> drivers/staging/rtl8723au/core/rtw_xmit.c | 84 ---
> drivers/staging/rtl8723au/os_dep/ioctl_cfg80211.c | 22 +--
> drivers/staging/rtl8723au/os_dep/usb_ops_linux.c | 9 +-
> 8 files changed, 140 insertions(+), 269 deletions(-)
In principle this is fine, but you need to stick to doing one thing per
patch. Do the list_for_each_entry() in one patch, and the formatting
issues in another.
Jes
> diff --git a/drivers/staging/rtl8723au/core/rtw_ap.c
> b/drivers/staging/rtl8723au/core/rtw_ap.c
> index 1aa9b26..66315f9 100644
> --- a/drivers/staging/rtl8723au/core/rtw_ap.c
> +++ b/drivers/staging/rtl8723au/core/rtw_ap.c
> @@ -171,24 +171,20 @@ static u8 chk_sta_is_alive(struct sta_info *psta)
> return ret;
> }
>
> -void expire_timeout_chk23a(struct rtw_adapter *padapter)
> +void expire_timeout_chk23a(struct rtw_adapter *padapter)
> {
> - struct list_head *phead, *plist, *ptmp;
> + struct list_head *phead;
> u8 updated = 0;
> - struct sta_info *psta;
> + struct sta_info *psta, *ptmp;
> struct sta_priv *pstapriv = &padapter->stapriv;
> u8 chk_alive_num = 0;
> struct sta_info *chk_alive_list[NUM_STA];
> int i;
>
> spin_lock_bh(&pstapriv->auth_list_lock);
> -
> phead = &pstapriv->auth_list;
> -
> /* check auth_queue */
> - list_for_each_safe(plist, ptmp, phead) {
> - psta = container_of(plist, struct sta_info, auth_list);
> -
> + list_for_each_entry_safe(psta, ptmp, phead, auth_list) {
> if (psta->expire_to > 0) {
> psta->expire_to--;
> if (psta->expire_to == 0) {
> @@ -206,19 +202,13 @@ voidexpire_timeout_chk23a(struct rtw_adapter
> *padapter)
> spin_lock_bh(&pstapriv->auth_list_lock);
> }
> }
> -
> }
> -
> spin_unlock_bh(&pstapriv->auth_list_lock);
>
> spin_lock_bh(&pstapriv->asoc_list_lock);
> -
> phead = &pstapriv->asoc_list;
> -
> /* check asoc_queue */
> - list_for_each_safe(plist, ptmp, phead) {
> - psta = container_of(plist, struct sta_info, asoc_list);
> -
> + list_for_each_entry_safe(psta, ptmp, phead, asoc_list) {
> if (chk_sta_is_alive(psta) || !psta->expire_to) {
> psta->expire_to = pstapriv->expire_to;
> psta->keep_alive_trycnt = 0;
> @@ -283,7 +273,6 @@ void expire_timeout_chk23a(struct rtw_adapter
> *padapter)
> }
> }
> }
> -
> spin_unlock_bh(&pstapriv->asoc_list_lock);
>
> if (chk_alive_num) {
> @@ -299,51 +288,55 @@ voidexpire_timeout_chk23a(struct rtw_adapter
> *padapter)
> SelectChannel23a(padapter, pmlmeext->cur_channel);
> }
>
> - /* issue null data to check sta alive */
> - for (i = 0; i < chk_alive_num; i++) {
> + /* issue null data to check sta alive */
> + for (i = 0; i < chk_alive_num; i++) {
>
> - int ret = _FAIL;
> + int ret = _FAIL;
>
> - psta = chk_alive_list[i];
> - if (!(psta->state & _FW_LINKED))
> - continue;
> + psta = chk_alive_list[i];
> + if (!(psta->state & _FW_LINKED))
> + continue;
>
> - if (psta->state & WIFI_SLEEP_STATE)
> - ret = issue_nulldata23a(padapter, psta->hwaddr, 0, 1,
> 50);
> - else
> - ret = issue_nulldata23a(padapter, psta->hwaddr, 0, 3,
> 50);
> + if (psta->state & WIFI_SLEEP_STATE)
> + ret = issue_nulldata23a(padapter, psta->hwaddr,
> + 0, 1, 50);
> + else
> + ret = issue_nulldata23a(padapter, psta->hwaddr,
> + 0, 3, 50);
> +
> + psta->keep_alive_trycnt++;
> + if (ret == _SUCCESS) {
> + DBG_8723A("asoc check, sta(%pM) is alive\n",
> + psta->hwaddr);
> + psta->expire_to = pstapriv->expire_to;
> + psta->keep_alive_trycnt = 0;
> + continue;
> + } else if (
Re: [PATCH v2] staging: rtl8723au: Fixes unnecessary return warning
Bhaktipriya Shridhar writes: > This patch fixes checkpatch.pl warning in rtw_mlme_ext.c file. > WARNING: void function return statements are not generally useful > > Signed-off-by: Bhaktipriya Shridhar > --- > Changes in v2: >- Removed the unnecessary blank lines. > drivers/staging/rtl8723au/core/rtw_mlme_ext.c | 20 > 1 file changed, 20 deletions(-) Harmless, albeit pointless, so fine with me. Jes
Re: [PATCH] staging: rtl8723au: Fixes unnecessary return warning
Julian Calaby writes: > Hi Bhaktipriya, > > On Sat, Jan 30, 2016 at 5:00 AM, Jes Sorensen wrote: >> Bhaktipriya Shridhar writes: >> If you insist on pushing this rather unncessary change, please do it >> properly, and remove the blank line before the return statement as well. > > As Jes said, you need to remove the blank lines before the returns > too. checkpatch should have picked this up, you did run the patch > through checkpatch before you sent it, right? > > Jes, > > I know you have strong feelings on coding style, but there are a lot > of people out there who see deviations from the standard as bugs to be > fixed, so stuff like this isn't going to stop until it matches the > coding style document's spec. Julian, rtl8723au is pretty dead development wise, so I don't care too much. checkpatch is broken and has effectively turned into a policing tool for a few people who wish to apply their narrow view onto everyone else. I'll continue top reject broken patches to my code pushed out under those rules. Maybe it's time to introduce checkpatchconsideredharmful.com Jes
Re: [PATCH] staging: rtl8723au: Fixes unnecessary return warning
Bhaktipriya Shridhar writes: > This patch fixes checkpatch.pl warning in rtw_mlme_ext.c file. > WARNING: void function return statements are not generally useful > > Signed-off-by: Bhaktipriya Shridhar > --- > drivers/staging/rtl8723au/core/rtw_mlme_ext.c | 10 -- > 1 file changed, 10 deletions(-) > > diff --git a/drivers/staging/rtl8723au/core/rtw_mlme_ext.c > b/drivers/staging/rtl8723au/core/rtw_mlme_ext.c > index d28f29a..e8a16b9 100644 > --- a/drivers/staging/rtl8723au/core/rtw_mlme_ext.c > +++ b/drivers/staging/rtl8723au/core/rtw_mlme_ext.c > @@ -2657,7 +2657,6 @@ static void issue_probersp(struct rtw_adapter > *padapter, unsigned char *da) > > dump_mgntframe23a(padapter, pmgntframe); > > - return; > } If you insist on pushing this rather unncessary change, please do it properly, and remove the blank line before the return statement as well. Jes
Re: [dm-devel] WANTED new maintainer for Linux/md (and mdadm)
Brassow Jonathan writes: > Many thanks Neil for all the work you’ve done and the help you gave me > while working on the DM/MD interactions bits. I’m happy you are > sticking around for the raid1-cluster and raid5-journal bits and I’m > interested to see what comes out of those. > > I know there are a number of folks around Red Hat who are capable and > possibly interested to share the load. They should be back from PTO > soon and we’ll make sure they know about the opportunity. Thanks Neil! I mentioned to Neil last year that I could probably be convinced, bribed, shamed, into maintaining mdadm. The kernel MD stack I think really needs a team to run it. There is just too much internal knowledge sitting in Neil's head and I personally am a little scared of taking on that. Shaohua Li would be a good candidate for that team IMHO. Cheers, Jes > thanks, > brassow > >> On Dec 21, 2015, at 12:10 AM, NeilBrown wrote: >> >> >> hi, >> I became maintainer for md (Linux Software RAID) in late 2001 and on >> the whole it has been fun and a valuable experience. But I have been >> losing interest in recent years (https://lwn.net/Articles/511073/) and >> as was mentioned at the kernel summit, I would like to resign. Some >> years ago I managed to hand over nfsd to the excellent Bruce Fields, >> but I do not seem to have the gift that Linus has of attracting >> maintainers. While there are a number of people who know quite a bit >> about md and/or have contributed to development, there is no obvious >> candidate for replacement maintainer - no one who has already been >> doing significant parts of the maintainer role. >> >> So I have decided to fall back on the mechanism by which I ended up >> being maintainer in the first place. I will create a vacuum and hope >> someone fills it (yes: I was sucked-in). So as of 1st February >> 2016 I will be resigning. >> >> At the kernel summit in October Linus talked about the value of >> maintainership teams (https://lwn.net/Articles/662979/). I think it >> would be great if a (small) team formed to continue to oversee md >> rather than just a single individual (or maybe the dm team could extend >> to include md??). If I had managed to be part of a team rather than >> "going it alone" for so long, I might feel less tired of the whole >> thing now. >> >> I don't see it as my place to appoint that team or any individuals, or >> even to nominate any candidates. A very important attribute of a >> maintainer is that they need to care about the code and the subsystem >> and I cannot tell other people to care (or even know if they do). It >> is really up to individuals to volunteer. A few people have been >> mentioned to me in earlier less-public conversations. Any of them may >> well be suitable, but I would rather they named themselves if >> interested. >> >> So I'm hoping to get one or more volunteers to be maintainer: >> - to gather and manage patches and outstanding issues, >> - to review patches or get them reviewed >> - to follow up bug reports and get them resolved >> - to feed patches upstream, maybe directly to Linus, >> maybe through some other maintainer, depending on what >> relationships already exist or can be formed, >> - to guide the longer term direction (saying "no" is important >> sometimes), >> - to care, >> but also to be aware that maintainership takes real effort and time, as >> does anything that is really worthwhile. >> >> This all applies to mdadm as well as md (except you would ultimately >> *be* upstream for mdadm, not needing to send it anywhere). Even if a >> clear team doesn't form it would be great if different people >> maintained mdadm and md. >> >> One part of the job that I have put a lot of time in to is following >> the linux-r...@vger.kernel.org list and providing support. This makes >> people feel good about md and so more adventurous in using it. >> Consequently I tend to hear about bugs and usability issues nice and >> early (well before paying customers hit them in most cases) and that is >> a big win. >> In recent times I've been doing less of this and have been absolutely >> thrilled that the gap has been more than filled by other very competent >> community members. Not developers particular but a number of md users >> have been providing excellent support. I'd particularly like to >> high-light Phil Turmel who is very forthcoming with excellent advice, >> but he is certainly not the only one who deserves a lot of thanks. >> So "Thank you" to everyone who answers questions on linux-raid. >> >> This would be a good place for any future maintainer to hang out to >> receive wisdom as well as to provide support. >> >> I will still be around. I can certainly help out in some sort of >> mentor role, and can probably be convinced to review patches and >> comment on designs. But I really want to head towards spending less >> time on md (there are so many other interesting things to learn
Re: [PATCH] staging: rtl8723au: use %pM and %ph formatting
mele...@gmail.com writes: > From: Daniil Leshchev > > use %pM and %ph specifiers instead of placing each byte on stack. > (staging/rtl8723au/TODO) > > Signed-off-by: Daniil Leshchev > --- > drivers/staging/rtl8723au/core/rtw_recv.c | 32 > +- > .../staging/rtl8723au/hal/rtl8723a_bt-coexist.c| 24 > drivers/staging/rtl8723au/hal/usb_halinit.c| 6 ++-- > 3 files changed, 15 insertions(+), 47 deletions(-) Looks fine to me. Acked-by: Jes Sorensen -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] staging: rtl8723au: fix byte order problems
Julian Calaby writes:
> Hi Sven,
>
> On Tue, Jan 5, 2016 at 2:29 AM, Sven Dziadek wrote:
>> diff --git a/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
>> b/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
>> index 1662c03c..57f5941 100644
>> --- a/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
>> +++ b/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
>> @@ -93,11 +93,9 @@ int FillH2CCmd(struct rtw_adapter *padapter, u8
>> ElementID, u32 CmdLen,
>>
>> if (h2c_cmd & BIT(7)) {
>> msgbox_ex_addr = REG_HMEBOX_EXT_0 + (h2c_box_num *
>> EX_MESSAGE_BOX_SIZE);
>> - h2c_cmd_ex = le16_to_cpu(h2c_cmd_ex);
>> rtl8723au_write16(padapter, msgbox_ex_addr,
>> h2c_cmd_ex);
>> }
>> msgbox_addr = REG_HMEBOX_0 + (h2c_box_num *
>> MESSAGE_BOX_SIZE);
>> - h2c_cmd = le32_to_cpu(h2c_cmd);
>> rtl8723au_write32(padapter, msgbox_addr, h2c_cmd);
>
> While Jes has NACK'd this change, it does highlight that the h2c_cmd
> and h2c_cmd_ex variables are being used to hold both cpu-endian and
> little-endian data. A worthwhile change here might be to move the
> conversion into the function call following these lines so that they
> remain "clean".
>
> That said, I'm not sure this particular snippet of code would work on
> big-endian at all as I'm pretty sure that BIT() produces cpu-endian
> values and we know from the line you remove that h2c_cmd is
> little-endian at this point.
I am not opposed to cleaning it up, however I hope we can just remove
all of the code down the line instead. You may want to look at the h2c
implementation I did for rtl8xxxu. I believe it does work on big-endian,
at least I know Larry has been able to test it on big-endian systems.
Cheers,
Jes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH] staging: rtl8723au: fix byte order problems
Sven Dziadek writes:
> Remove byte order conversions.
> Conversion is already done in usb_ops_linux.c when accessing usb port.
> The deleted lines convert to little-endian and then call FillH2CCmd to
> convert back. Additionally, they are applied to wrong types and
> process wrong parts of variables later on.
>
> Signed-off-by: Sven Dziadek
> ---
>
> I was looking for some Sparse warnings that I could fix and found this:
>
> drivers/staging/rtl8723au//hal/rtl8723a_cmd.c:96:38: warning: cast to
> restricted __le16
> drivers/staging/rtl8723au//hal/rtl8723a_cmd.c:100:27: warning: cast to
> restricted __le32
> drivers/staging/rtl8723au//hal/rtl8723a_cmd.c:118:25: warning: incorrect
> type in assignment (different base types)
> drivers/staging/rtl8723au//hal/rtl8723a_cmd.c:118:25:expected
> unsigned int [unsigned] [usertype]
> drivers/staging/rtl8723au//hal/rtl8723a_cmd.c:118:25:got restricted
> __le32 [usertype]
>
> The rtl8723au drivers seems to work on many machines already, but
> probably mostly on little-endian machines.
> The file staging/rtl8723au/hal/rtl8723a_cmd.c contains four conversions
> from or to little-endian that look suspicious.
> The best example is:
>
> int rtl8723a_set_rssi_cmd(struct rtw_adapter *padapter, u8 *param)
> {
> *((u32 *)param) = cpu_to_le32(*((u32 *)param));
>
> FillH2CCmd(padapter, RSSI_SETTING_EID, 3, param);
>
> return _SUCCESS;
> }
>
> On little-endian, the conversion does nothing, but on big-endian, it
> swaps the order and then only 3 bytes are used in FillH2CCmd (3rd
> argument). So it actually ignores the original argument param.
>
> I think it is best to delete these conversions because the actual
> conversions are done when transferring data to or from the usb port already.
>
> Unfortunately, I do not have the hardware to test it.
> What do you think about the patch?
Be *very* careful with this - there is a lot of dodgy passing back and
forth to the hardware and the format needs to match what the firmware
expects.
Unless you are going to test this and confirm that it actually works,
then please stay away from this.
NAK
Thanks,
Jes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2 v3] drivers: staging: rtl8723au: Changed rssi_cmd to little-endian param
Jacob Kiefer writes:
> From: Jacob Kiefer
>
> Changed rssi_cmd interface to accept le32 param instead of
> unnecessary u8 * conversion. Updated existing calls to rssi_cmd.
> This patch pushes responsibility to caller to convert to
> le32. This cleans up the code quite a bit.
> Also removed magic numbers.
>
> This patch fixes the following sparse error:
>
> CHECK drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
> ...
> drivers/staging/rtl8723au/hal/rtl8723a_cmd.c:118:25: \
> warning: incorrect type in assignment (different base types)
> drivers/staging/rtl8723au/hal/rtl8723a_cmd.c:118:25: \
> expected unsigned int [unsigned] [usertype]
> drivers/staging/rtl8723au/hal/rtl8723a_cmd.c:118:25: \
> got restricted __le32 [usertype]
> ...
>
> Signed-off-by: Jacob Kiefer
> ---
> In v3, opted to change the interface rather than just the internal
> code to clear the sparse errors and make the code more sane.
I am fine with this code in principle, but has it been tested? This
stuff will break the driver miserably if it's wrong.
Thanks,
Jes
> ---
> drivers/staging/rtl8723au/hal/odm.c | 3 ++-
> drivers/staging/rtl8723au/hal/rtl8723a_cmd.c | 7 +++
> drivers/staging/rtl8723au/include/rtl8723a_cmd.h | 2 +-
> 3 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/rtl8723au/hal/odm.c
> b/drivers/staging/rtl8723au/hal/odm.c
> index 6b9dbef..c7f45c7 100644
> --- a/drivers/staging/rtl8723au/hal/odm.c
> +++ b/drivers/staging/rtl8723au/hal/odm.c
> @@ -1274,7 +1274,8 @@ static void odm_RSSIMonitorCheck(struct dm_odm_t
> *pDM_Odm)
>
> for (i = 0; i < sta_cnt; i++) {
> if (PWDB_rssi[i] != (0))
> - rtl8723a_set_rssi_cmd(Adapter, (u8 *)&PWDB_rssi[i]);
> + rtl8723a_set_rssi_cmd(Adapter,
> + cpu_to_le32(PWDB_rssi[i]));
> }
>
> pdmpriv->EntryMaxUndecoratedSmoothedPWDB = MaxDB;
> diff --git a/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
> b/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
> index 9733aa6..97d23c3 100644
> --- a/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
> +++ b/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
> @@ -25,6 +25,7 @@
> #define RTL92C_MAX_CMD_LEN 5
> #define MESSAGE_BOX_SIZE 4
> #define EX_MESSAGE_BOX_SIZE 2
> +#define RSSI_CMD_LEN 3
>
> static u8 _is_fw_read_cmd_down(struct rtw_adapter *padapter, u8 msgbox_num)
> {
> @@ -113,11 +114,9 @@ exit:
> return ret;
> }
>
> -int rtl8723a_set_rssi_cmd(struct rtw_adapter *padapter, u8 *param)
> +int rtl8723a_set_rssi_cmd(struct rtw_adapter *padapter, __le32 param)
> {
> - *((u32 *)param) = cpu_to_le32(*((u32 *)param));
> -
> - FillH2CCmd(padapter, RSSI_SETTING_EID, 3, param);
> + FillH2CCmd(padapter, RSSI_SETTING_EID, RSSI_CMD_LEN, (u8 *)¶m);
>
> return _SUCCESS;
> }
> diff --git a/drivers/staging/rtl8723au/include/rtl8723a_cmd.h
> b/drivers/staging/rtl8723au/include/rtl8723a_cmd.h
> index 014c02e..e281543 100644
> --- a/drivers/staging/rtl8723au/include/rtl8723a_cmd.h
> +++ b/drivers/staging/rtl8723au/include/rtl8723a_cmd.h
> @@ -149,7 +149,7 @@ void rtl8723a_set_BTCoex_AP_mode_FwRsvdPkt_cmd(struct
> rtw_adapter *padapter);
> #else
> #define rtl8723a_set_BTCoex_AP_mode_FwRsvdPkt_cmd(padapter) do {} while(0)
> #endif
> -int rtl8723a_set_rssi_cmd(struct rtw_adapter *padapter, u8 *param);
> +int rtl8723a_set_rssi_cmd(struct rtw_adapter *padapter, __le32 param);
> int rtl8723a_set_raid_cmd(struct rtw_adapter *padapter, u32 mask, u8 arg);
> void rtl8723a_add_rateatid(struct rtw_adapter *padapter, u32 bitmap, u8 arg,
> u8 rssi_level);
>
> --
> 1.8.3.2
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] staging: rtl8723au: Fix resource leak
Mateusz Kulikowski writes:
> Firmware was not released properly if kmemdup fails.
>
> Addresses-Coverity-Id: 1269118
> Signed-off-by: Mateusz Kulikowski
> ---
> drivers/staging/rtl8723au/hal/rtl8723a_hal_init.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
Acked-by: Jes Sorensen
>
> diff --git a/drivers/staging/rtl8723au/hal/rtl8723a_hal_init.c
> b/drivers/staging/rtl8723au/hal/rtl8723a_hal_init.c
> index cd014f7..ecf54ee 100644
> --- a/drivers/staging/rtl8723au/hal/rtl8723a_hal_init.c
> +++ b/drivers/staging/rtl8723au/hal/rtl8723a_hal_init.c
> @@ -249,13 +249,13 @@ int rtl8723a_FirmwareDownload(struct rtw_adapter
> *padapter)
> goto Exit;
> }
> firmware_buf = kmemdup(fw->data, fw->size, GFP_KERNEL);
> + fw_size = fw->size;
> + release_firmware(fw);
> if (!firmware_buf) {
> rtStatus = _FAIL;
> goto Exit;
> }
> buf = firmware_buf;
> - fw_size = fw->size;
> - release_firmware(fw);
>
> /* To Check Fw header. Added by tynli. 2009.12.04. */
> pFwHdr = (struct rt_8723a_firmware_hdr *)firmware_buf;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Removed extra spaces, and error from checkpatch.pl
Pinkesh Badjatiya writes: > Removed trailing spaces. > Fixed some errors from checkpatch.pl verification. > No changes made to actual functional code. > > Signed-off-by: Pinkesh Badjatiya > --- > drivers/staging/rtl8723au/include/rtw_cmd.h | 162 > ++-- > 1 file changed, 80 insertions(+), 82 deletions(-) Please read the guidelines for how to post a patch for staging. The subject line needs to have the correct prefixes for it to be considered. Jes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4 1/3] Staging: rtl8723au: core: Bool tests don't need comparisons
Shraddha Barke writes:
> This patch removes comparisons to true/false values on bool variables.
>
Please do not send patches for multiple different drivers in the same
patch set. In addition, if you post more than one patch, you need to
include a cover letter.
Jes
>
>
> Signed-off-by: Shraddha Barke
> ---
> Changes in v4-
> No change
>
> drivers/staging/rtl8723au/core/rtw_ap.c | 10 +-
> drivers/staging/rtl8723au/core/rtw_mlme_ext.c | 28
> +--
> 2 files changed, 19 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/staging/rtl8723au/core/rtw_ap.c
> b/drivers/staging/rtl8723au/core/rtw_ap.c
> index b96e7b6..7f46b7a 100644
> --- a/drivers/staging/rtl8723au/core/rtw_ap.c
> +++ b/drivers/staging/rtl8723au/core/rtw_ap.c
> @@ -409,7 +409,7 @@ void add_RATid23a(struct rtw_adapter *padapter, struct
> sta_info *psta, u8 rssi_l
>
> arg |= BIT(7);/* support entry 2~31 */
>
> - if (shortGIrate == true)
> + if (shortGIrate)
> arg |= BIT(5);
>
> tx_ra_bitmap |= ((raid<<28)&0xf000);
> @@ -424,7 +424,7 @@ void add_RATid23a(struct rtw_adapter *padapter, struct
> sta_info *psta, u8 rssi_l
> /* arg[5] = Short GI */
> rtl8723a_add_rateatid(padapter, tx_ra_bitmap, arg, rssi_level);
>
> - if (shortGIrate == true)
> + if (shortGIrate)
> init_rate |= BIT(6);
>
> /* set ra_id, init_rate */
> @@ -662,7 +662,7 @@ static void start_bss_network(struct rtw_adapter
> *padapter, u8 *pbuf)
> update_hw_ht_param(padapter);
> }
>
> - if (pmlmepriv->cur_network.join_res != true) {
> + if (!pmlmepriv->cur_network.join_res) {
> /* setting only at first time */
> /* WEP Key will be set before this function, do not clear CAM.
> */
> if (psecuritypriv->dot11PrivacyAlgrthm !=
> @@ -1370,7 +1370,7 @@ static int rtw_ht_operation_update(struct rtw_adapter
> *padapter)
> void associated_clients_update23a(struct rtw_adapter *padapter, u8 updated)
> {
> /* update associated stations cap. */
> - if (updated == true) {
> + if (updated) {
> struct list_head *phead, *plist, *ptmp;
> struct sta_info *psta;
> struct sta_priv *pstapriv = &padapter->stapriv;
> @@ -1882,7 +1882,7 @@ void stop_ap_mode23a(struct rtw_adapter *padapter)
> list_for_each_safe(plist, ptmp, phead) {
> paclnode = container_of(plist, struct rtw_wlan_acl_node, list);
>
> - if (paclnode->valid == true) {
> + if (paclnode->valid) {
> paclnode->valid = false;
>
> list_del_init(&paclnode->list);
> diff --git a/drivers/staging/rtl8723au/core/rtw_mlme_ext.c
> b/drivers/staging/rtl8723au/core/rtw_mlme_ext.c
> index be9a3d5..65ef4a4 100644
> --- a/drivers/staging/rtl8723au/core/rtw_mlme_ext.c
> +++ b/drivers/staging/rtl8723au/core/rtw_mlme_ext.c
> @@ -602,7 +602,7 @@ void free_mlme_ext_priv23a (struct mlme_ext_priv
> *pmlmeext)
> if (!padapter)
> return;
>
> - if (padapter->bDriverStopped == true) {
> + if (padapter->bDriverStopped) {
> del_timer_sync(&pmlmeext->survey_timer);
> del_timer_sync(&pmlmeext->link_timer);
> /* del_timer_sync(&pmlmeext->ADDBA_timer); */
> @@ -959,7 +959,7 @@ OnAuth23a(struct rtw_adapter *padapter, struct recv_frame
> *precv_frame)
> goto auth_fail;
> }
>
> - if (rtw_access_ctrl23a(padapter, sa) == false) {
> + if (!rtw_access_ctrl23a(padapter, sa)) {
> status = WLAN_STATUS_AP_UNABLE_TO_HANDLE_NEW_STA;
> goto auth_fail;
> }
> @@ -2049,7 +2049,7 @@ static int OnAction23a_back23a(struct rtw_adapter
> *padapter,
> sizeof(struct ADDBA_request));
> process_addba_req23a(padapter,
>(u8 *)&pmlmeinfo->ADDBA_req, addr);
> - if (pmlmeinfo->bAcceptAddbaReq == true)
> + if (pmlmeinfo->bAcceptAddbaReq)
> issue_action_BA23a(padapter, addr,
> WLAN_ACTION_ADDBA_RESP, 0);
> else {
> @@ -2253,8 +2253,8 @@ void update_mgntframe_attrib23a(struct rtw_adapter
> *padapter,
> void dump_mgntframe23a(struct rtw_adapter *padapter,
> struct xmit_frame *pmgntframe)
> {
> - if (padapter->bSurpriseRemoved == true ||
> - padapter->bDriverStopped == true)
> + if (padapter->bSurpriseRemoved ||
> + padapter->bDriverStopped)
> return;
>
> rtl8723au_mgnt_xmit(padapter, pmgntframe);
> @@ -2269,8 +2269,8 @@ int dump_mgntframe23a_and_wait(struct rtw_adapter
> *padapter,
> struct xmit_buf *pxmitbuf = pmgntframe->pxmitbuf
Re: [PATCH] staging: rtl8723au: remove unnecessary le32_to_cpu
Michał Bartoszkiewicz writes:
> The values passed to le32_to_cpu are already in the correct byte order.
> This fixes four "cast to restricted __le32" sparse warnings.
>
> Signed-off-by: Michał Bartoszkiewicz
> ---
> drivers/staging/rtl8723au/core/rtw_security.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
Sounds OK to me - the fact we never hit problems with this is a good
indicator the hw crypt functions work.
Cheers,
Jes
>
> diff --git a/drivers/staging/rtl8723au/core/rtw_security.c
> b/drivers/staging/rtl8723au/core/rtw_security.c
> index 3d40bab..9d1cce1 100644
> --- a/drivers/staging/rtl8723au/core/rtw_security.c
> +++ b/drivers/staging/rtl8723au/core/rtw_security.c
> @@ -245,8 +245,8 @@ void rtw_wep_decrypt23a(struct rtw_adapter *padapter,
> arcfour_encrypt(&mycontext, payload, payload, length);
>
> /* calculate icv and compare the icv */
> - actual_crc = le32_to_cpu(getcrc32(payload, length - 4));
> - expected_crc = le32_to_cpu(get_unaligned_le32(&payload[length - 4]));
> + actual_crc = getcrc32(payload, length - 4);
> + expected_crc = get_unaligned_le32(&payload[length - 4]);
>
> if (actual_crc != expected_crc) {
> RT_TRACE(_module_rtl871x_security_c_, _drv_err_,
> @@ -773,8 +773,8 @@ int rtw_tkip_decrypt23a(struct rtw_adapter *padapter,
> arcfour_init(&mycontext, rc4key, 16);
> arcfour_encrypt(&mycontext, payload, payload, length);
>
> - actual_crc = le32_to_cpu(getcrc32(payload, length - 4));
> - expected_crc = le32_to_cpu(get_unaligned_le32(&payload[length - 4]));
> + actual_crc = getcrc32(payload, length - 4);
> + expected_crc = get_unaligned_le32(&payload[length - 4]);
>
> if (actual_crc != expected_crc) {
> RT_TRACE(_module_rtl871x_security_c_, _drv_err_,
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] staging/rtl8723au: Use %pM format specifier to print mac address
Alexander Kuleshov writes: > printk() supports %pM format specifier for printing 6-byte MAC/FDDI > addresses in hex notation small buffers, let's use it intead of %x:%x... > > Signed-off-by: Alexander Kuleshov > --- > drivers/staging/rtl8723au/core/rtw_mlme_ext.c | 7 ++- > 1 file changed, 2 insertions(+), 5 deletions(-) Works for me Acked-by: Jes Sorensen -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: rtl8723au: rtl8723a_hal_init: Improve code readability
Johannes Postma writes: > This patch improves code readability in the function > rtl8723a_cal_txdesc_chksum. It improves the readability of the argument > of the function le16_to_cpu. > > Signed-off-by: Johannes Postma > --- > drivers/staging/rtl8723au/hal/rtl8723a_hal_init.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Acked-by: Jes Sorensen -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: rtl8723au: Fix sparse warning: cast to restricted __le16
Johannes Postma writes: > On 06/08/15 at 08:21am, Jes Sorensen wrote: >> >> Looks OK to me. Probably worth changing the *(usPtr + index) to be >> usPtr[index] as well to make the code easier to read. >> >> Jes >> > > Thank you for reviewing. I will make a seperate patch for that. I will > send it after this one is accepted. Or should I combine them into a > patch serie? Either is fine with me. Jes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: rtl8723au: Fix sparse warning: cast to restricted __le16
Johannes Postma writes:
> usPtr is used as __le16 *, but was defined as u16 *.
> This was reported by sparse as:
> drivers/staging/rtl8723au/hal/rtl8723a_hal_init.c:1850:29: warning: cast to
> restricted __le16
>
> This patch fixes the type of usPtr.
>
> Signed-off-by: Johannes Postma
> ---
> drivers/staging/rtl8723au/hal/rtl8723a_hal_init.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Looks OK to me. Probably worth changing the *(usPtr + index) to be
usPtr[index] as well to make the code easier to read.
Jes
> diff --git a/drivers/staging/rtl8723au/hal/rtl8723a_hal_init.c
> b/drivers/staging/rtl8723au/hal/rtl8723a_hal_init.c
> index cb5076a..eb76ac4 100644
> --- a/drivers/staging/rtl8723au/hal/rtl8723a_hal_init.c
> +++ b/drivers/staging/rtl8723au/hal/rtl8723a_hal_init.c
> @@ -1838,7 +1838,7 @@ Hal_EfuseParseThermalMeter_8723A(struct rtw_adapter
> *padapter,
>
> static void rtl8723a_cal_txdesc_chksum(struct tx_desc *ptxdesc)
> {
> - u16 *usPtr = (u16 *) ptxdesc;
> + __le16 *usPtr = (__le16 *)ptxdesc;
> u32 count = 16; /* (32 bytes / 2 bytes per XOR) => 16 times */
> u32 index;
> u16 checksum = 0;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2] lib/mpi: Teach the code inline 101
From: Jes Sorensen The broken extern inline usage resulted in gcc5 duplicating the functions causing link errors due to duplicate symbols. Signed-off-by: Jes Sorensen --- v2: Removed bogus comment in Subject line lib/mpi/mpi-inline.h | 2 +- lib/mpi/mpi-internal.h | 18 ++ 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/lib/mpi/mpi-inline.h b/lib/mpi/mpi-inline.h index e2b3985..c245ea3 100644 --- a/lib/mpi/mpi-inline.h +++ b/lib/mpi/mpi-inline.h @@ -30,7 +30,7 @@ #define G10_MPI_INLINE_H #ifndef G10_MPI_INLINE_DECL -#define G10_MPI_INLINE_DECL extern inline +#define G10_MPI_INLINE_DECL static inline #endif G10_MPI_INLINE_DECL mpi_limb_t diff --git a/lib/mpi/mpi-internal.h b/lib/mpi/mpi-internal.h index 60cf765..8eeb6b5 100644 --- a/lib/mpi/mpi-internal.h +++ b/lib/mpi/mpi-internal.h @@ -168,20 +168,22 @@ void mpi_rshift_limbs(MPI a, unsigned int count); int mpi_lshift_limbs(MPI a, unsigned int count); /*-- mpihelp-add.c --*/ -mpi_limb_t mpihelp_add_1(mpi_ptr_t res_ptr, mpi_ptr_t s1_ptr, -mpi_size_t s1_size, mpi_limb_t s2_limb); +static inline mpi_limb_t mpihelp_add_1(mpi_ptr_t res_ptr, mpi_ptr_t s1_ptr, + mpi_size_t s1_size, mpi_limb_t s2_limb); mpi_limb_t mpihelp_add_n(mpi_ptr_t res_ptr, mpi_ptr_t s1_ptr, mpi_ptr_t s2_ptr, mpi_size_t size); -mpi_limb_t mpihelp_add(mpi_ptr_t res_ptr, mpi_ptr_t s1_ptr, mpi_size_t s1_size, - mpi_ptr_t s2_ptr, mpi_size_t s2_size); +static inline mpi_limb_t mpihelp_add(mpi_ptr_t res_ptr, mpi_ptr_t s1_ptr, +mpi_size_t s1_size, mpi_ptr_t s2_ptr, +mpi_size_t s2_size); /*-- mpihelp-sub.c --*/ -mpi_limb_t mpihelp_sub_1(mpi_ptr_t res_ptr, mpi_ptr_t s1_ptr, -mpi_size_t s1_size, mpi_limb_t s2_limb); +static inline mpi_limb_t mpihelp_sub_1(mpi_ptr_t res_ptr, mpi_ptr_t s1_ptr, + mpi_size_t s1_size, mpi_limb_t s2_limb); mpi_limb_t mpihelp_sub_n(mpi_ptr_t res_ptr, mpi_ptr_t s1_ptr, mpi_ptr_t s2_ptr, mpi_size_t size); -mpi_limb_t mpihelp_sub(mpi_ptr_t res_ptr, mpi_ptr_t s1_ptr, mpi_size_t s1_size, - mpi_ptr_t s2_ptr, mpi_size_t s2_size); +static inline mpi_limb_t mpihelp_sub(mpi_ptr_t res_ptr, mpi_ptr_t s1_ptr, +mpi_size_t s1_size, mpi_ptr_t s2_ptr, +mpi_size_t s2_size); /*-- mpihelp-cmp.c --*/ int mpihelp_cmp(mpi_ptr_t op1_ptr, mpi_ptr_t op2_ptr, mpi_size_t size); -- 2.4.3 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[RHEL7.2 PATCH] lib/mpi: Teach the code inline 101
From: Jes Sorensen The broken extern inline usage resulted in gcc5 duplicating the functions causing link errors due to duplicate symbols. Signed-off-by: Jes Sorensen --- lib/mpi/mpi-inline.h | 2 +- lib/mpi/mpi-internal.h | 18 ++ 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/lib/mpi/mpi-inline.h b/lib/mpi/mpi-inline.h index e2b3985..c245ea3 100644 --- a/lib/mpi/mpi-inline.h +++ b/lib/mpi/mpi-inline.h @@ -30,7 +30,7 @@ #define G10_MPI_INLINE_H #ifndef G10_MPI_INLINE_DECL -#define G10_MPI_INLINE_DECL extern inline +#define G10_MPI_INLINE_DECL static inline #endif G10_MPI_INLINE_DECL mpi_limb_t diff --git a/lib/mpi/mpi-internal.h b/lib/mpi/mpi-internal.h index 60cf765..8eeb6b5 100644 --- a/lib/mpi/mpi-internal.h +++ b/lib/mpi/mpi-internal.h @@ -168,20 +168,22 @@ void mpi_rshift_limbs(MPI a, unsigned int count); int mpi_lshift_limbs(MPI a, unsigned int count); /*-- mpihelp-add.c --*/ -mpi_limb_t mpihelp_add_1(mpi_ptr_t res_ptr, mpi_ptr_t s1_ptr, -mpi_size_t s1_size, mpi_limb_t s2_limb); +static inline mpi_limb_t mpihelp_add_1(mpi_ptr_t res_ptr, mpi_ptr_t s1_ptr, + mpi_size_t s1_size, mpi_limb_t s2_limb); mpi_limb_t mpihelp_add_n(mpi_ptr_t res_ptr, mpi_ptr_t s1_ptr, mpi_ptr_t s2_ptr, mpi_size_t size); -mpi_limb_t mpihelp_add(mpi_ptr_t res_ptr, mpi_ptr_t s1_ptr, mpi_size_t s1_size, - mpi_ptr_t s2_ptr, mpi_size_t s2_size); +static inline mpi_limb_t mpihelp_add(mpi_ptr_t res_ptr, mpi_ptr_t s1_ptr, +mpi_size_t s1_size, mpi_ptr_t s2_ptr, +mpi_size_t s2_size); /*-- mpihelp-sub.c --*/ -mpi_limb_t mpihelp_sub_1(mpi_ptr_t res_ptr, mpi_ptr_t s1_ptr, -mpi_size_t s1_size, mpi_limb_t s2_limb); +static inline mpi_limb_t mpihelp_sub_1(mpi_ptr_t res_ptr, mpi_ptr_t s1_ptr, + mpi_size_t s1_size, mpi_limb_t s2_limb); mpi_limb_t mpihelp_sub_n(mpi_ptr_t res_ptr, mpi_ptr_t s1_ptr, mpi_ptr_t s2_ptr, mpi_size_t size); -mpi_limb_t mpihelp_sub(mpi_ptr_t res_ptr, mpi_ptr_t s1_ptr, mpi_size_t s1_size, - mpi_ptr_t s2_ptr, mpi_size_t s2_size); +static inline mpi_limb_t mpihelp_sub(mpi_ptr_t res_ptr, mpi_ptr_t s1_ptr, +mpi_size_t s1_size, mpi_ptr_t s2_ptr, +mpi_size_t s2_size); /*-- mpihelp-cmp.c --*/ int mpihelp_cmp(mpi_ptr_t op1_ptr, mpi_ptr_t op2_ptr, mpi_size_t size); -- 2.4.3 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] kthread: Export kthread functions
yalin wang writes: >> On Aug 1, 2015, at 21:32, Neil Horman wrote: >>> strange, this is my test result: >>> >>> size built-in.o* >>> text data bss dec hex filename >>> 743937 50786 56008 850731 cfb2b built-in.o // with the patch >>> 744069 50786 56008 850863 cfbaf built-in.o_old // with out the >>> patch >>> >> So you're willing to expose the internals of kthread_park in exchange for the >> hope of saving 132 bytes of text. >> >> Thats just dumb. I agree with tglx, this shouldn't change. >> >> Neil > not just size, mainly for performance, > without inline: > > ffcd26b0: 97fff4aa bl ffccf958 > ffcd26b4: 53001c00uxtbw0, w0 > > if kthread_should_park() inline: > ffcd1a44: f85c8020 ldr x0, [x1,#-56] // kthread_should_park > line > ffcd1a48: 36100300 tbz w0, #2, ffcd1aa8 > // kthread_should_park line > > still use 2 instructions, but don’t need a function call, > maybe can do more optimisation by gcc sometimes . > Anyway, this is just a suggest, > it is up to you apply it or not. :) kthread_park() isn't exactly a performance critical function call. Saving two instructions does not outway the cost of exposing the internals of the kthread API. Jes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging:rtl8723au:core:rtw_sreset.c:Fix for space related error
"Joglekar, Tejas (T.)" writes: > From: Joglekar Tejas > > This patch fixes the error given by checkpatch.pl > > Signed-off-by: Joglekar Tejas > --- > drivers/staging/rtl8723au/core/rtw_sreset.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/rtl8723au/core/rtw_sreset.c > b/drivers/staging/rtl8723au/core/rtw_sreset.c > index 29a29d9..48b7723 100644 > --- a/drivers/staging/rtl8723au/core/rtw_sreset.c > +++ b/drivers/staging/rtl8723au/core/rtw_sreset.c > @@ -71,7 +71,7 @@ static void sreset_restore_security_station(struct > rtw_adapter *padapter) > /* pairwise key */ > rtw_setstakey_cmd23a(padapter, (unsigned char *)psta, > true); > /* group key */ > - rtw_set_key23a(padapter,&padapter->securitypriv, > padapter->securitypriv.dot118021XGrpKeyid, 0); > + rtw_set_key23a(padapter, &padapter->securitypriv, > padapter->securitypriv.dot118021XGrpKeyid, 0); > } > } > } NAK! If you are 'fixing' checkpatch 'errors', do it properly and break down the line and solve the 80 character limit 'error' as well. Jes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: rtl8723au: fix incorrect type in assignment warning
Steve Pennington writes:
> Repaced calls to htons and memcpy with a single call to put_unaligned_be16
You may want an 'l' in replaced, but not a biggie to me.
> to fix the following sparse warning:
> drivers/staging/rtl8723au/core/rtw_recv.c:1557:21: warning: incorrect type in
> assignment (different base types)
> drivers/staging/rtl8723au/core/rtw_recv.c:1557:21:expected unsigned short
> [unsigned] [assigned] [usertype] len
> drivers/staging/rtl8723au/core/rtw_recv.c:1557:21:got restricted __be16
> [usertype]
>
> Signed-off-by: Steve Pennington
> ---
> drivers/staging/rtl8723au/core/rtw_recv.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/staging/rtl8723au/core/rtw_recv.c
> b/drivers/staging/rtl8723au/core/rtw_recv.c
> index 274a4b6..ad0549c 100644
> --- a/drivers/staging/rtl8723au/core/rtw_recv.c
> +++ b/drivers/staging/rtl8723au/core/rtw_recv.c
> @@ -1554,8 +1554,7 @@ static int wlanhdr_to_ethhdr (struct recv_frame
> *precvframe)
> ether_addr_copy(ptr + ETH_ALEN, pattrib->src);
>
> if (!bsnaphdr) {
> - len = htons(len);
> - memcpy(ptr + 12, &len, 2);
> + put_unaligned_be16(len, ptr + 12);
> }
This one looks good to me.
Jes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: rtl8723au: fix incorrect type in assignment warning
Steve Pennington writes:
> Repaced call to htons with call to cpu_to_be16s to fix the
> following sparse warning:
> drivers/staging/rtl8723au/core/rtw_recv.c:1557:21: warning: incorrect type in
> assignment (different base types)
> drivers/staging/rtl8723au/core/rtw_recv.c:1557:21:expected unsigned short
> [unsigned] [assigned] [usertype] len
> drivers/staging/rtl8723au/core/rtw_recv.c:1557:21:got restricted __be16
> [usertype]
>
> Signed-off-by: Steve Pennington
> ---
> drivers/staging/rtl8723au/core/rtw_recv.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/rtl8723au/core/rtw_recv.c
> b/drivers/staging/rtl8723au/core/rtw_recv.c
> index 274a4b6..39318ae 100644
> --- a/drivers/staging/rtl8723au/core/rtw_recv.c
> +++ b/drivers/staging/rtl8723au/core/rtw_recv.c
> @@ -1554,7 +1554,7 @@ static int wlanhdr_to_ethhdr (struct recv_frame
> *precvframe)
> ether_addr_copy(ptr + ETH_ALEN, pattrib->src);
>
> if (!bsnaphdr) {
> - len = htons(len);
> + cpu_to_be16s(&len);
> memcpy(ptr + 12, &len, 2);
> }
Thats an awful fix - use put_unaligned_le16() instead.
Jes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 0/2] staging: rtl8723au: core: endianness issues
David Decotigny writes: > The code shows a couple inconsistencies (described in commit > descriptions) which would not be an issue on little-endian cpus, but > could cause breakage on non-LE cpus. Note: I could not test on real > hardware, these patches created based on sparse reports. > > Hostory: > - resending the same patches to correct recipients, only changed > commit descriptions (credits to Dan Carpenter) > > > # Patch Set Summary: > > David Decotigny (2): > staging: rtl8723au: core: avoid bitwise arithmetic with forced > endianness > staging: rtl8723au: core: remove redundant endianness conversion > > drivers/staging/rtl8723au/core/rtw_mlme_ext.c | 6 ++ > 1 file changed, 2 insertions(+), 4 deletions(-) Looks fine to me, however if you fiddle with this same value twice, wouldn't it be better to do it in one patch? Cheers, Jes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [BUG ?] staging: rtl8723au: condition with no effect
Nicholas Mc Guire writes:
> On Sat, 13 Jun 2015, Jes Sorensen wrote:
>
>> Nicholas Mc Guire writes:
>> > scanning for trivial bug-patters with coccinelle spatches returned:
>> > ./drivers/staging/rtl8723au/hal/rtl8723a_hal_init.c:1395
>> >WARNING: condition with no effect (if branch == else)
>> >
>> > drivers/staging/rtl8723au/hal/rtl8723a_hal_init.c - line numbers
>> > from 4.1-rc7
>> > 1395if (bWithoutHWSM) {
>> > 1396/* value16 |= (APDM_HOST | FSM_HSUS |/PFM_ALDN); */
>> > 1397/* 2010/08/31 According to Filen description, we need to
>> > 1398use HW to shut down 8051 automatically. */
>> > 1399/* Because suspend operation need the asistance of 8051
>> > 1400to wait for 3ms. */
>> > 1401value16 = APDM_HOST | AFSM_HSUS | PFM_ALDN;
>> > 1402} else {
>> > 1403value16 = APDM_HOST | AFSM_HSUS | PFM_ALDN;
>> > 1404}
>> > 1405
>> > 1406rtl8723au_write16(padapter, REG_APS_FSMCO, value16);/* 0x4802
>> > */
>> >
>> > Not clear what the intent is but this looks like a typo/bug in the
>> > assigment
>> > of value16 as the condition here has no effect.
>>
>> Doesn't look like a typo, looks like someone at some point had commented
>> out PFM_ALDN in the bWithoutHWSM case. Why they did that and then later
>> overrode it, I have no idea.
>>
> as far as its traceable in git log if == else was in there from the
> very beginning (including that comment) - both the if and else were
> updated in commit cffca68d7b2f ("staging: rtl8723au: _DisableAnalog(): Avoid
> zero-init variables unnecessaril") but without changing PFM_ALDN - as there
> are no comments to the meaning of these bits in the header file there is
> no way to really come up with a resonable patch. anyway its either wrong
> settings or the condition should be removed.
There's a much longer history of this code going back to the vendor
origin - it shouldn't be removed unless we can get clarification from
them why that was.
Jes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [BUG ?] staging: rtl8723au: condition with no effect
Nicholas Mc Guire writes:
> scanning for trivial bug-patters with coccinelle spatches returned:
> ./drivers/staging/rtl8723au/hal/rtl8723a_hal_init.c:1395
> WARNING: condition with no effect (if branch == else)
>
> drivers/staging/rtl8723au/hal/rtl8723a_hal_init.c - line numbers from 4.1-rc7
> 1395if (bWithoutHWSM) {
> 1396/* value16 |= (APDM_HOST | FSM_HSUS |/PFM_ALDN); */
> 1397/* 2010/08/31 According to Filen description, we need to
> 1398use HW to shut down 8051 automatically. */
> 1399/* Because suspend operation need the asistance of 8051
> 1400to wait for 3ms. */
> 1401value16 = APDM_HOST | AFSM_HSUS | PFM_ALDN;
> 1402} else {
> 1403value16 = APDM_HOST | AFSM_HSUS | PFM_ALDN;
> 1404}
> 1405
> 1406rtl8723au_write16(padapter, REG_APS_FSMCO, value16);/* 0x4802 */
>
> Not clear what the intent is but this looks like a typo/bug in the assigment
> of value16 as the condition here has no effect.
Doesn't look like a typo, looks like someone at some point had commented
out PFM_ALDN in the bWithoutHWSM case. Why they did that and then later
overrode it, I have no idea.
Jes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1 linux-next] staging: rtl8723au: use swap() in WMMOnAssocRsp23a()
Fabian Frederick writes:
> Use kernel.h macro definition.
>
> Thanks to Julia Lawall for Coccinelle scripting support.
>
> Signed-off-by: Fabian Frederick
> ---
> drivers/staging/rtl8723au/core/rtw_wlan_util.c | 12 +++-
> 1 file changed, 3 insertions(+), 9 deletions(-)
Please make sure you base your patches off the staging tree, not off
linux-next.
Jes
>
> diff --git a/drivers/staging/rtl8723au/core/rtw_wlan_util.c
> b/drivers/staging/rtl8723au/core/rtw_wlan_util.c
> index 5280338..3c1315fc 100644
> --- a/drivers/staging/rtl8723au/core/rtw_wlan_util.c
> +++ b/drivers/staging/rtl8723au/core/rtw_wlan_util.c
> @@ -573,7 +573,7 @@ void WMMOnAssocRsp23a(struct rtw_adapter *padapter)
> inx[0] = 0; inx[1] = 1; inx[2] = 2; inx[3] = 3;
>
> if (pregpriv->wifi_spec == 1) {
> - u32 j, tmp, change_inx = false;
> + u32 j, change_inx = false;
>
> /* entry indx: 0->vo, 1->vi, 2->be, 3->bk. */
> for (i = 0; i < 4; i++) {
> @@ -589,14 +589,8 @@ void WMMOnAssocRsp23a(struct rtw_adapter *padapter)
> }
>
> if (change_inx) {
> - tmp = edca[i];
> - edca[i] = edca[j];
> - edca[j] = tmp;
> -
> - tmp = inx[i];
> - inx[i] = inx[j];
> - inx[j] = tmp;
> -
> + swap(edca[i], edca[j]);
> + swap(inx[i], inx[j]);
> change_inx = false;
> }
> }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: rtl8723au: fix sparse warning
Juston Li writes:
> change cast to __le16 to fix the following warning:
> drivers/staging/rtl8723au/hal/rtl8723a_hal_init.c:1488:20: warning: cast to
> restricted __le16
>
> Signed-off-by: Juston Li
> ---
> drivers/staging/rtl8723au/hal/rtl8723a_hal_init.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Looks OK to me
Jes
>
> diff --git a/drivers/staging/rtl8723au/hal/rtl8723a_hal_init.c
> b/drivers/staging/rtl8723au/hal/rtl8723a_hal_init.c
> index 04d0183..e23af8e 100644
> --- a/drivers/staging/rtl8723au/hal/rtl8723a_hal_init.c
> +++ b/drivers/staging/rtl8723au/hal/rtl8723a_hal_init.c
> @@ -1485,7 +1485,7 @@ void Hal_EfuseParseIDCode(struct rtw_adapter *padapter,
> u8 *hwinfo)
> u16 EEPROMId;
>
> /* Checl 0x8129 again for making sure autoload status!! */
> - EEPROMId = le16_to_cpu(*((u16 *) hwinfo));
> + EEPROMId = le16_to_cpu(*((__le16 *) hwinfo));
> if (EEPROMId != RTL_EEPROM_ID) {
> DBG_8723A("EEPROM ID(%#x) is invalid!!\n", EEPROMId);
> pEEPROM->bautoload_fail_flag = true;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] drivers: staging: unisys: visorbus: visorchipset.c: private functions should be declared static
tolga ceylan writes:
> On 05/15/2015 09:22 PM, Tolga Ceylan wrote:
>> visorchipset_file_init() and visorchipset_file_cleanup() functions
>> do not seem to be used from anywhere else and now are declared
>> as static. Sparse emitted "not declared" warnings for these two
>> functions.
>>
>> Signed-off-by: Tolga Ceylan
>> ---
>> drivers/staging/unisys/visorbus/visorchipset.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/unisys/visorbus/visorchipset.c
>> b/drivers/staging/unisys/visorbus/visorchipset.c
>> index ca22f49..66ae3d0 100644
>> --- a/drivers/staging/unisys/visorbus/visorchipset.c
>> +++ b/drivers/staging/unisys/visorbus/visorchipset.c
>> @@ -2351,7 +2351,7 @@ static const struct file_operations visorchipset_fops
>> = {
>> .mmap = visorchipset_mmap,
>> };
>>
>> -int
>> +static int
>> visorchipset_file_init(dev_t major_dev, struct visorchannel
>> **controlvm_channel)
>> {
>> int rc = 0;
>> @@ -2460,7 +2460,7 @@ cleanup:
>> return rc;
>> }
>>
>> -void
>> +static void
>> visorchipset_file_cleanup(dev_t major_dev)
>> {
>> if (file_cdev.ops)
>>
>
> I haven't received a response for this patch. Just checking back.
>
> Regards,
> Tolga Ceylan
I don't think there is anything wrong with your patch. However, there is
a lot of active development going to clean up the code in the unisys
tree right now, and your patch conflicts with that. I would prefer to
hold off with this for now, until it settles down a bit.
Cheers,
Jes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Staging: unisys: fix function declaration format in visorchipset.c
Wim de With writes:
> This is a patch that fixes the function declarations in
> visorbus/visorchipset.c by removing newlines after the function return
> type
This patch doesn't fix things, it makes things worse!
If you want to post patches to this, do it properly and check the output
first.
NACK
Jes
> Signed-off-by: Wim de With
> ---
> drivers/staging/unisys/visorbus/visorchipset.c | 225
> ++---
> 1 file changed, 85 insertions(+), 140 deletions(-)
>
> diff --git a/drivers/staging/unisys/visorbus/visorchipset.c
> b/drivers/staging/unisys/visorbus/visorchipset.c
> index ca22f49..f9192b6 100644
> --- a/drivers/staging/unisys/visorbus/visorchipset.c
> +++ b/drivers/staging/unisys/visorbus/visorchipset.c
> @@ -63,8 +63,7 @@ static int visorchipset_visorbusregwait = 1;/*
> default is on */
> static int visorchipset_holdchipsetready;
> static unsigned long controlvm_payload_bytes_buffered;
>
> -static int
> -visorchipset_open(struct inode *inode, struct file *file)
> +static int visorchipset_open(struct inode *inode, struct file *file)
> {
> unsigned minor_number = iminor(inode);
>
> @@ -74,8 +73,7 @@ visorchipset_open(struct inode *inode, struct file *file)
> return 0;
> }
>
> -static int
> -visorchipset_release(struct inode *inode, struct file *file)
> +static int visorchipset_release(struct inode *inode, struct file *file)
> {
> return 0;
> }
> @@ -375,8 +373,8 @@ static void controlvm_respond_physdev_changestate(
>
> static void parser_done(struct parser_context *ctx);
>
> -static struct parser_context *
> -parser_init_byte_stream(u64 addr, u32 bytes, bool local, bool *retry)
> +static struct parser_context *parser_init_byte_stream(u64 addr, u32 bytes,
> + bool local, bool *retry)
This is *way* worse than what was there before.
> {
> int allocbytes = sizeof(struct parser_context) + bytes;
> struct parser_context *rc = NULL;
> @@ -451,8 +449,7 @@ cleanup:
> return rc;
> }
>
> -static uuid_le
> -parser_id_get(struct parser_context *ctx)
> +static uuid_le parser_id_get(struct parser_context *ctx)
> {
> struct spar_controlvm_parameters_header *phdr = NULL;
>
> @@ -473,8 +470,7 @@ enum PARSER_WHICH_STRING {
> PARSERSTRING_NAME, /* TODO: only PARSERSTRING_NAME is used ? */
> };
>
> -static void
> -parser_param_start(struct parser_context *ctx,
> +static void parser_param_start(struct parser_context *ctx,
> enum PARSER_WHICH_STRING which_string)
> {
> struct spar_controlvm_parameters_header *phdr = NULL;
> @@ -515,8 +511,7 @@ static void parser_done(struct parser_context *ctx)
> kfree(ctx);
> }
>
> -static void *
> -parser_string_get(struct parser_context *ctx)
> +static void *parser_string_get(struct parser_context *ctx)
> {
> u8 *pscan;
> unsigned long nscan;
> @@ -707,8 +702,7 @@ static ssize_t remaining_steps_store(struct device *dev,
> return count;
> }
>
> -static void
> -bus_info_clear(void *v)
> +static void bus_info_clear(void *v)
> {
> struct visorchipset_bus_info *p = (struct visorchipset_bus_info *) v;
>
> @@ -717,8 +711,7 @@ bus_info_clear(void *v)
> memset(p, 0, sizeof(struct visorchipset_bus_info));
> }
>
> -static void
> -dev_info_clear(void *v)
> +static void dev_info_clear(void *v)
> {
> struct visorchipset_device_info *p =
> (struct visorchipset_device_info *) v;
> @@ -726,8 +719,8 @@ dev_info_clear(void *v)
> memset(p, 0, sizeof(struct visorchipset_device_info));
> }
>
> -static struct visorchipset_bus_info *
> -bus_find(struct list_head *list, u32 bus_no)
> +static struct visorchipset_bus_info *bus_find(struct list_head *list,
> + u32 bus_no)
> {
> struct visorchipset_bus_info *p;
>
> @@ -739,8 +732,8 @@ bus_find(struct list_head *list, u32 bus_no)
> return NULL;
> }
>
> -static struct visorchipset_device_info *
> -device_find(struct list_head *list, u32 bus_no, u32 dev_no)
> +static struct visorchipset_device_info *device_find(struct list_head *list,
> + u32 bus_no, u32 dev_no)
> {
> struct visorchipset_device_info *p;
>
> @@ -764,8 +757,7 @@ static void busdevices_del(struct list_head *list, u32
> bus_no)
> }
> }
>
> -static u8
> -check_chipset_events(void)
> +static u8 check_chipset_events(void)
> {
> int i;
> u8 send_msg = 1;
> @@ -775,8 +767,7 @@ check_chipset_events(void)
> return send_msg;
> }
>
> -static void
> -clear_chipset_events(void)
> +static void clear_chipset_events(void)
> {
> int i;
> /* Clear chipset_events */
> @@ -784,8 +775,7 @@ clear_chipset_events(void)
> chipset_events[i] = 0;
> }
>
> -void
> -visorchipset_register_busdev(
> +void visorchipset_register_busdev(
> struct visorchipset_busdev_notifiers *notifie
Re: [PATCH 3/8] rtl8192u: don't trample on struct namespace
Paul Gortmaker writes: > [Re: [PATCH 3/8] rtl8192u: don't trample on struct > namespace] On 30/04/2015 (Thu 09:52) Jes Sorensen wrote: > >> Paul Gortmaker writes: >> > In order to start reducing the duplicated code/constants/macros in this >> > driver, we need to include to provide the defacto >> > versions. However this driver has structs with the same name as the >> > ones in the main include, so namespace collision prevents us from doing >> > step #1. >> > >> > Since the structs actually differ in their respective fields, we can't >> > simply delete the local ones without impacting the runtime; a conversion >> > to use the global ones can be considered at a later date if desired. > ^^^ >> > >> > Rename the ones here with a vendor specific prefix so that we won't have >> > the namespace collision, and hence can continue on with the cleanup. >> > >> > Automated conversion done with: >> > >> > for i in `find . -name '*.[ch]'` ; do \ >> > sed -i 's/struct ieee80211_hdr/struct rtl_80211_hdr/g' $i ; \ >> > done >> > >> > Signed-off-by: Paul Gortmaker >> > --- >> > drivers/staging/rtl8192u/ieee80211/ieee80211.h | 44 +++--- >> > .../rtl8192u/ieee80211/ieee80211_crypt_ccmp.c | 10 ++-- >> > .../rtl8192u/ieee80211/ieee80211_crypt_tkip.c | 26 - >> > drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c | 68 > +++--- >> > .../staging/rtl8192u/ieee80211/ieee80211_softmac.c | 32 +- >> > drivers/staging/rtl8192u/ieee80211/ieee80211_tx.c | 14 ++--- >> > .../staging/rtl8192u/ieee80211/rtl819x_BAProc.c| 48 +++ >> > drivers/staging/rtl8192u/r8192U_core.c | 12 ++-- >> > 8 files changed, 127 insertions(+), 127 deletions(-) >> >> Rather than just renaming these headers to avoid the conflict, it seems >> to me the better solution is to convert the code to use the ieee80211.h >> provided ones from the kernel? > > That is what I said in paragraph #2 above. If someone wants to do this > later, then fine. And then it can be done incrementally and tested by > someone who has the actual hardware. In the meantime, this is better > than what was there, and since it is in staging, a realisitc expectation > is multiple small incremental improvements IMHO. Sorry too far behind on emails, I missed that part. I am fine with this approach - we just need a volunteer! Jes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/8] rtl8192u: don't trample on struct namespace
Paul Gortmaker writes:
> In order to start reducing the duplicated code/constants/macros in this
> driver, we need to include to provide the defacto
> versions. However this driver has structs with the same name as the
> ones in the main include, so namespace collision prevents us from doing
> step #1.
>
> Since the structs actually differ in their respective fields, we can't
> simply delete the local ones without impacting the runtime; a conversion
> to use the global ones can be considered at a later date if desired.
>
> Rename the ones here with a vendor specific prefix so that we won't have
> the namespace collision, and hence can continue on with the cleanup.
>
> Automated conversion done with:
>
> for i in `find . -name '*.[ch]'` ; do \
> sed -i 's/struct ieee80211_hdr/struct rtl_80211_hdr/g' $i ; \
> done
>
> Signed-off-by: Paul Gortmaker
> ---
> drivers/staging/rtl8192u/ieee80211/ieee80211.h | 44 +++---
> .../rtl8192u/ieee80211/ieee80211_crypt_ccmp.c | 10 ++--
> .../rtl8192u/ieee80211/ieee80211_crypt_tkip.c | 26 -
> drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c | 68
> +++---
> .../staging/rtl8192u/ieee80211/ieee80211_softmac.c | 32 +-
> drivers/staging/rtl8192u/ieee80211/ieee80211_tx.c | 14 ++---
> .../staging/rtl8192u/ieee80211/rtl819x_BAProc.c| 48 +++
> drivers/staging/rtl8192u/r8192U_core.c | 12 ++--
> 8 files changed, 127 insertions(+), 127 deletions(-)
Rather than just renaming these headers to avoid the conflict, it seems
to me the better solution is to convert the code to use the ieee80211.h
provided ones from the kernel?
Cheers,
Jes
> diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211.h
> b/drivers/staging/rtl8192u/ieee80211/ieee80211.h
> index 0f53c6a97578..bdad6d07c574 100644
> --- a/drivers/staging/rtl8192u/ieee80211/ieee80211.h
> +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211.h
> @@ -1020,20 +1020,20 @@ enum ieee80211_mfie {
> /* Minimal header; can be used for passing 802.11 frames with sufficient
> * information to determine what type of underlying data type is actually
> * stored in the data. */
> -struct ieee80211_hdr {
> +struct rtl_80211_hdr {
> __le16 frame_ctl;
> __le16 duration_id;
> u8 payload[0];
> } __packed;
>
> -struct ieee80211_hdr_1addr {
> +struct rtl_80211_hdr_1addr {
> __le16 frame_ctl;
> __le16 duration_id;
> u8 addr1[ETH_ALEN];
> u8 payload[0];
> } __packed;
>
> -struct ieee80211_hdr_2addr {
> +struct rtl_80211_hdr_2addr {
> __le16 frame_ctl;
> __le16 duration_id;
> u8 addr1[ETH_ALEN];
> @@ -1041,7 +1041,7 @@ struct ieee80211_hdr_2addr {
> u8 payload[0];
> } __packed;
>
> -struct ieee80211_hdr_3addr {
> +struct rtl_80211_hdr_3addr {
> __le16 frame_ctl;
> __le16 duration_id;
> u8 addr1[ETH_ALEN];
> @@ -1051,7 +1051,7 @@ struct ieee80211_hdr_3addr {
> u8 payload[0];
> } __packed;
>
> -struct ieee80211_hdr_4addr {
> +struct rtl_80211_hdr_4addr {
> __le16 frame_ctl;
> __le16 duration_id;
> u8 addr1[ETH_ALEN];
> @@ -1062,7 +1062,7 @@ struct ieee80211_hdr_4addr {
> u8 payload[0];
> } __packed;
>
> -struct ieee80211_hdr_3addrqos {
> +struct rtl_80211_hdr_3addrqos {
> __le16 frame_ctl;
> __le16 duration_id;
> u8 addr1[ETH_ALEN];
> @@ -1073,7 +1073,7 @@ struct ieee80211_hdr_3addrqos {
> __le16 qos_ctl;
> } __packed;
>
> -struct ieee80211_hdr_4addrqos {
> +struct rtl_80211_hdr_4addrqos {
> __le16 frame_ctl;
> __le16 duration_id;
> u8 addr1[ETH_ALEN];
> @@ -1092,7 +1092,7 @@ struct ieee80211_info_element {
> } __packed;
>
> struct ieee80211_authentication {
> - struct ieee80211_hdr_3addr header;
> + struct rtl_80211_hdr_3addr header;
> __le16 algorithm;
> __le16 transaction;
> __le16 status;
> @@ -1101,18 +1101,18 @@ struct ieee80211_authentication {
> } __packed;
>
> struct ieee80211_disassoc {
> - struct ieee80211_hdr_3addr header;
> + struct rtl_80211_hdr_3addr header;
> __le16 reason;
> } __packed;
>
> struct ieee80211_probe_request {
> - struct ieee80211_hdr_3addr header;
> + struct rtl_80211_hdr_3addr header;
> /* SSID, supported rates */
> struct ieee80211_info_element info_element[0];
> } __packed;
>
> struct ieee80211_probe_response {
> - struct ieee80211_hdr_3addr header;
> + struct rtl_80211_hdr_3addr header;
> __le32 time_stamp[2];
> __le16 beacon_interval;
> __le16 capability;
> @@ -1125,7 +1125,7 @@ struct ieee80211_probe_response {
> #define ieee80211_beacon ieee80211_probe_response
>
> struct ieee80211_assoc_request_frame {
> - struct ieee80211_hdr_3addr header;
> + struct rtl_80211_hdr_3addr header;
> __le16 capability;
> __le16 listen_interval;
> /* SSID, supported rates, RSN */
> @@ -1133,7 +1133,7 @@ struct ieee80211_assoc_
Re: [PATCH] drivers: staging: rtl8723au: fix "warning: cast to restricted __le16"
Dan Carpenter writes: > This doesn't look right and it doesn't have a changelog explainly the > weirdness. > ... and the fix is as ugly as it gets! If something like this is needed, creating a __le16 *ptr at the beginnging of the function would be a lot better. Jes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: rtl8723au: Update RT_TRACE macro and uses
Joe Perches writes: > Create an rt_trace function using %pV to reduce overall code size. > Update the macro uses to remove unnecessary and now harmful parentheses. > > Miscellanea around these changes: > > o Coalesce formats > o Realign arguments > o Remove commented-out RT_TRACE uses > o Spelling fixes in formats > o Add missing newlines to formats > o Remove multiple newlines from formats > o Neaten formats where noticed > o Use %pM in one instance > > Reduces code size ~20KB > > Signed-off-by: Joe Perches > --- > Mostly done by various scripts&emacs. > Compiled, untested, no hardware, This could be further improved by fixing up all the places where the function name is hard coded into the print statement, instead of using __func__. In particular as a lot of it is carried over from old code and has been renamed since. It's OK with me to do this in a follow-on patch though. Jes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: rtl8723au: Remove uses of MAC_FMT and MAC_ARG
Joe Perches writes: > Use the standard vsprintf kernel extension to format > mac addresses. > > This reduces object code size a bit. > > Miscellanea: > > o Coalesce formats > o Realign arguments > o Remove the now unused MAC_FMT and MAC_ARG #defines > > Signed-off-by: Joe Perches > --- > drivers/staging/rtl8723au/core/rtw_ap.c | 41 ++--- > drivers/staging/rtl8723au/core/rtw_mlme.c | 26 > drivers/staging/rtl8723au/core/rtw_mlme_ext.c | 72 > +++ > drivers/staging/rtl8723au/core/rtw_recv.c | 42 + > drivers/staging/rtl8723au/core/rtw_wlan_util.c| 6 +- > drivers/staging/rtl8723au/core/rtw_xmit.c | 8 +-- > drivers/staging/rtl8723au/include/ieee80211.h | 3 - > drivers/staging/rtl8723au/os_dep/ioctl_cfg80211.c | 18 +++--- > drivers/staging/rtl8723au/os_dep/os_intfs.c | 7 +-- > 9 files changed, 101 insertions(+), 122 deletions(-) Looks good to me Acked-by: Jes Sorensen -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 15/16] staging: rtl8723au: Correct a typo in a trace log line
"M. Vefa Bicakci" writes:
> Correct a typo in rtl8723au's rtw_security.c which was most likely
> caused by a copy and paste mistake. Prior to this commit, the TKIP
> decryption function referred to WEP in its trace log output.
>
> Signed-off-by: M. Vefa Bicakci
> ---
> drivers/staging/rtl8723au/core/rtw_security.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/rtl8723au/core/rtw_security.c
> b/drivers/staging/rtl8723au/core/rtw_security.c
> index eb3544866a..e8d3ce53bb 100644
> --- a/drivers/staging/rtl8723au/core/rtw_security.c
> +++ b/drivers/staging/rtl8723au/core/rtw_security.c
> @@ -757,7 +757,7 @@ int rtw_tkip_decrypt23a(struct rtw_adapter *padapter,
>
> if (actual_crc != expected_crc) {
> RT_TRACE(_module_rtl871x_security_c_, _drv_err_,
> - ("rtw_wep_decrypt23a:icv CRC mismatch: "
> + ("rtw_tkip_decrypt23a:icv CRC mismatch: "
If you made this ("%s:icv ", __func__,
you wouldn't have to worry about matching the function name to the debug
text, in case the function later got renamed.
Cheers,
Jes
> "actual: %08x, expected: %08x\n",
> actual_crc, expected_crc));
> res = _FAIL;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 10/16] staging: rtl8723au: that open brace should be on the previous line
"M. Vefa Bicakci" writes:
> Correct two instances of the checkpatch.pl error indicating that the
> opening curly braces should not be on new lines:
> ERROR: that open brace { should be on the previous line
>
> Signed-off-by: M. Vefa Bicakci
> ---
> drivers/staging/rtl8723au/core/rtw_security.c | 6 ++
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/rtl8723au/core/rtw_security.c
> b/drivers/staging/rtl8723au/core/rtw_security.c
> index 803f8965a4..2d762276d3 100644
> --- a/drivers/staging/rtl8723au/core/rtw_security.c
> +++ b/drivers/staging/rtl8723au/core/rtw_security.c
> @@ -760,8 +760,7 @@ int rtw_tkip_decrypt23a(struct rtw_adapter *padapter,
>
> *((u32 *)crc) = le32_to_cpu(getcrc32(payload, length - 4));
>
> - if (crc[3] != payload[length - 1] || crc[2] != payload[length - 2] ||
> crc[1] != payload[length - 3] || crc[0] != payload[length - 4])
> - {
> + if (crc[3] != payload[length - 1] || crc[2] != payload[length - 2] ||
> crc[1] != payload[length - 3] || crc[0] != payload[length - 4]) {
> RT_TRACE(_module_rtl871x_security_c_, _drv_err_,
> ("rtw_wep_decrypt23a:icv error crc[3](%x)!= payload[length-1](%x) ||
> crc[2](%x)!= payload[length-2](%x) || crc[1](%x)!= payload[length-3](%x) ||
> crc[0](%x)!= payload[length-4](%x)\n",
> crc[3], payload[length - 1], crc[2],
> payload[length - 2], crc[1], payload[length - 3], crc[0], payload[length -
> 4]));
> res = _FAIL;
If you are mangling a line like this, don't just fix one ugliness, fix
it up properly by breaking down the line to avoid the > 80 character
mess.
Cheers,
Jes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 00/16] checkpatch clean-up of rtl8723au's rtw_security.c
"M. Vefa Bicakci" writes: > Hello Matthias, > > Thank you for reviewing the patches! > > I have based and tested these commits on Stephen Rothwell's > linux-next.git/master branch. It is possible that I made a mistake > by doing this. > > Nevertheless, I have just verified that these patches cleanly apply to the > current version of Greg Kroah-Hartman's staging.git/staging-next branch, [1] > as well as Stephen Rothwell's linux-next.git/master branch. [2] > > In my local repo, here are my remotes: > > $ git remote -v | grep -E "staging|next" | grep "(fetch)$" > next > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git > (fetch) > staging > https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git > (fetch) > > > I have just fetched both of these remotes. > > $ git log -1 --oneline next/master > a68b04c773 Add linux-next specific files for 20150313 > > $ git log -1 --oneline staging/staging-next > 7aa5d5097d Staging: rtl8192u: Use __packed instead of __attribute__((packed)) > > > And here are my two local branches with my commits applied: > > # Output slightly edited to reduce line length. > $ git branch -vv | grep -E "staging|next" > * linux-next c8ab62fb9c [next/master: ahead 16] staging: rtl8723au: > Remove unneeded comments > staging-next 63f451681e [staging/staging-next: ahead 16] staging: > rtl8723au: Remove unneeded comments > > > I hope this clarifies my set-up. Is there something I am doing incorrectly? > If there is anything I can assist with, please let me know. > > Thank you, > > Vefa > > [1] > https://git.kernel.org/cgit/linux/kernel/git/gregkh/staging.git/log/?h=staging-next > [2] https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/log/ For staging patches, you should always post patches against the staging-next tree. Please rebase them and repost them. Jes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: rtl8723au: Fix sparse warning cast to restricted __le16
Marcus Folkesson writes:
> This patch fixes the following sparse warnings:
>
> CHECK drivers/staging/rtl8723au/hal/rtl8723a_hal_init.c
> drivers/staging/rtl8723au/hal/rtl8723a_hal_init.c:265:37: warning:
> cast to restricted __le16
> drivers/staging/rtl8723au/hal/rtl8723a_hal_init.c:267:39: warning:
> cast to restricted __le16
>
> Signed-off-by: Marcus Folkesson
> ---
> drivers/staging/rtl8723au/include/rtl8723a_hal.h | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/rtl8723au/include/rtl8723a_hal.h
> b/drivers/staging/rtl8723au/include/rtl8723a_hal.h
> index e146336..f642b11 100644
> --- a/drivers/staging/rtl8723au/include/rtl8723a_hal.h
> +++ b/drivers/staging/rtl8723au/include/rtl8723a_hal.h
> @@ -255,10 +255,10 @@ struct hal_data_8723a {
> struct hal_version VersionID;
> enum rt_customer_id CustomerID;
>
> - u16 FirmwareVersion;
> - u16 FirmwareVersionRev;
> - u16 FirmwareSubVersion;
> - u16 FirmwareSignature;
> + __le16 FirmwareVersion;
> + __le16 FirmwareVersionRev;
> + __le16 FirmwareSubVersion;
> + __le16 FirmwareSignature;
Ehm I am pretty sure it doesn't:
rtl8723au_hal_init.c:265
pHalData->FirmwareVersion = le16_to_cpu(pFwHdr->Version);
pHalData->FirmwareSubVersion = pFwHdr->Subversion;
pHalData->FirmwareSignature = le16_to_cpu(pFwHdr->Signature);
If anything, the second assignment there should be changed to use
le16_to_cpu(), but your conversion is definitely wrong.
NACK
Jes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] staging: rtl8723au: Remove unnecessary OOM message
Joe Perches writes: > On Fri, 2015-03-06 at 11:08 -0500, Jes Sorensen wrote: >> Julia Lawall writes: >> > On Fri, 6 Mar 2015, Jes Sorensen wrote: >> >> Quentin Lambert writes: >> >> > This patch reduces the kernel size by removing error messages >> >> > that duplicate >> >> > the normal OOM message. >> >> > A simplified version of the semantic patch that finds this problem is as >> >> > follows: (http://coccinelle.lip6.fr) >> >> This patch removes useful warnings about what allocation failed. The >> >> messages removed are NOT duplicate! >> > Is it really the case that the information can't be reconstructed from the >> > information generated by kmalloc on failure? To my understanding there is >> > a stack trace, and from scanning through the changes I see only one change >> > per function, so perhaps the stack trace already makes it clear where the >> > problem occurred? >> It may be possible to backtrack, but this change just makes it harder. >> There are tons of real issues to fix in this driver, this patch just >> increases the risk of patch conflicts for no real gain. > > Making the allocation less likely to fail for > low memory systems is a gain. > > The allocation failures themselves are low > likelihood events. Determining which specific > memory allocation failure occurred has near > nil value. Joe, That is bologna, knowing which allocation failed has a lot of value, it allows the developer to go back and look at the allocation sizes, parameters applied etc. This is a classic case of blindly applied script 'fixes' causing more harm than good. Jes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] staging: rtl8723au: Remove unnecessary OOM message
Julia Lawall writes: > On Fri, 6 Mar 2015, Jes Sorensen wrote: > >> Quentin Lambert writes: >> > This patch reduces the kernel size by removing error messages that >> > duplicate >> > the normal OOM message. >> > >> > A simplified version of the semantic patch that finds this problem is as >> > follows: (http://coccinelle.lip6.fr) >> >> This patch removes useful warnings about what allocation failed. The >> messages removed are NOT duplicate! > > Is it really the case that the information can't be reconstructed from the > information generated by kmalloc on failure? To my understanding there is > a stack trace, and from scanning through the changes I see only one change > per function, so perhaps the stack trace already makes it clear where the > problem occurred? It may be possible to backtrack, but this change just makes it harder. There are tons of real issues to fix in this driver, this patch just increases the risk of patch conflicts for no real gain. Jes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] staging: rtl8723au: Remove unnecessary OOM message
Quentin Lambert writes:
> This patch reduces the kernel size by removing error messages that duplicate
> the normal OOM message.
>
> A simplified version of the semantic patch that finds this problem is as
> follows: (http://coccinelle.lip6.fr)
This patch removes useful warnings about what allocation failed. The
messages removed are NOT duplicate!
NACK
Jes
>
> @@
> identifier f,print,l;
> expression e;
> constant char[] c;
> @@
>
> e = \(kzalloc\|kmalloc\|devm_kzalloc\|devm_kmalloc\)(...);
> if (e == NULL) {
> <+...
> - print(...,c,...);
> ... when any
> (
> goto l;
> |
> return ...;
> )
> ...+> }
>
> Signed-off-by: Quentin Lambert
> ---
> drivers/staging/rtl8723au/hal/rtl8723a_cmd.c | 8 ++--
> drivers/staging/rtl8723au/hal/rtl8723a_hal_init.c | 8 ++--
> drivers/staging/rtl8723au/hal/usb_ops_linux.c | 6 +-
> drivers/staging/rtl8723au/os_dep/ioctl_cfg80211.c | 5 -
> 4 files changed, 5 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
> b/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
> index 7b56411..38f5b7f 100644
> --- a/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
> +++ b/drivers/staging/rtl8723au/hal/rtl8723a_cmd.c
> @@ -462,10 +462,8 @@ static void SetFwRsvdPagePkt(struct rtw_adapter
> *padapter, bool bDLFinished)
> DBG_8723A("%s\n", __func__);
>
> ReservedPagePacket = kzalloc(1000, GFP_KERNEL);
> - if (ReservedPagePacket == NULL) {
> - DBG_8723A("%s: alloc ReservedPagePacket fail!\n", __func__);
> + if (ReservedPagePacket == NULL)
> return;
> - }
>
> pHalData = GET_HAL_DATA(padapter);
> pxmitpriv = &padapter->xmitpriv;
> @@ -669,10 +667,8 @@ static void SetFwRsvdPagePkt_BTCoex(struct rtw_adapter
> *padapter)
> DBG_8723A("+%s\n", __func__);
>
> ReservedPagePacket = kzalloc(1024, GFP_KERNEL);
> - if (ReservedPagePacket == NULL) {
> - DBG_8723A("%s: alloc ReservedPagePacket fail!\n", __func__);
> + if (ReservedPagePacket == NULL)
> return;
> - }
>
> pHalData = GET_HAL_DATA(padapter);
> pxmitpriv = &padapter->xmitpriv;
> diff --git a/drivers/staging/rtl8723au/hal/rtl8723a_hal_init.c
> b/drivers/staging/rtl8723au/hal/rtl8723a_hal_init.c
> index a5eadd4..6d50b09 100644
> --- a/drivers/staging/rtl8723au/hal/rtl8723a_hal_init.c
> +++ b/drivers/staging/rtl8723au/hal/rtl8723a_hal_init.c
> @@ -401,10 +401,8 @@ hal_ReadEFuse_WiFi(struct rtw_adapter *padapter,
> }
>
> efuseTbl = kmalloc(EFUSE_MAP_LEN_8723A, GFP_KERNEL);
> - if (efuseTbl == NULL) {
> - DBG_8723A("%s: alloc efuseTbl fail!\n", __func__);
> + if (efuseTbl == NULL)
> return;
> - }
> /* 0xff will be efuse default value instead of 0x00. */
> memset(efuseTbl, 0xFF, EFUSE_MAP_LEN_8723A);
>
> @@ -494,10 +492,8 @@ hal_ReadEFuse_BT(struct rtw_adapter *padapter,
> }
>
> efuseTbl = kmalloc(EFUSE_BT_MAP_LEN, GFP_KERNEL);
> - if (efuseTbl == NULL) {
> - DBG_8723A("%s: efuseTbl malloc fail!\n", __func__);
> + if (efuseTbl == NULL)
> return;
> - }
> /* 0xff will be efuse default value instead of 0x00. */
> memset(efuseTbl, 0xFF, EFUSE_BT_MAP_LEN);
>
> diff --git a/drivers/staging/rtl8723au/hal/usb_ops_linux.c
> b/drivers/staging/rtl8723au/hal/usb_ops_linux.c
> index a6d16ad..f1e9202 100644
> --- a/drivers/staging/rtl8723au/hal/usb_ops_linux.c
> +++ b/drivers/staging/rtl8723au/hal/usb_ops_linux.c
> @@ -256,12 +256,8 @@ static void usb_read_interrupt_complete(struct urb *purb)
> c2w = kmalloc(sizeof(struct evt_work),
> GFP_ATOMIC);
>
> - if (!c2w) {
> - printk(KERN_WARNING "%s: unable to "
> -"allocate work buffer\n",
> -__func__);
> + if (!c2w)
> goto urb_submit;
> - }
>
> c2w->adapter = padapter;
> INIT_WORK(&c2w->work, rtw_evt_work);
> diff --git a/drivers/staging/rtl8723au/os_dep/ioctl_cfg80211.c
> b/drivers/staging/rtl8723au/os_dep/ioctl_cfg80211.c
> index 537bd82..40bdf4b 100644
> --- a/drivers/staging/rtl8723au/os_dep/ioctl_cfg80211.c
> +++ b/drivers/staging/rtl8723au/os_dep/ioctl_cfg80211.c
> @@ -1327,8 +1327,6 @@ static int rtw_cfg80211_set_probe_req_wpsp2pie(struct
> rtw_adapter *padapter,
> pmlmepriv->wps_probe_req_ie = kmemdup(wps_ie, wps_ie[1],
> GFP_KERNEL);
> if (pmlmepriv->wps_probe_req_ie == NULL) {
> - DBG_8723A("%s()-%d: kmalloc() ERROR!\n",
> - __f
Re: [PATCH 3.10 49/53] md/raid5: Fix livelock when array is both resyncing and degraded.
Greg Kroah-Hartman writes:
> 3.10-stable review patch. If anyone has any objections, please let me know.
>
> --
For all 4 stable branches:
Acked-by: Jes Sorensen
>
> From: NeilBrown
>
> commit 26ac107378c4742978216be1005b7291b799c7b2 upstream.
>
> Commit a7854487cd7128a30a7f4f5259de9f67d5efb95f:
> md: When RAID5 is dirty, force reconstruct-write instead of
> read-modify-write.
>
> Causes an RCW cycle to be forced even when the array is degraded.
> A degraded array cannot support RCW as that requires reading all data
> blocks, and one may be missing.
>
> Forcing an RCW when it is not possible causes a live-lock and the code
> spins, repeatedly deciding to do something that cannot succeed.
>
> So change the condition to only force RCW on non-degraded arrays.
>
> Reported-by: Manibalan P
> Bisected-by: Jes Sorensen
> Tested-by: Jes Sorensen
> Signed-off-by: NeilBrown
> Fixes: a7854487cd7128a30a7f4f5259de9f67d5efb95f
> Signed-off-by: Greg Kroah-Hartman
>
> ---
> drivers/md/raid5.c |3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -2853,7 +2853,8 @@ static void handle_stripe_dirtying(struc
>* generate correct data from the parity.
>*/
> if (conf->max_degraded == 2 ||
> - (recovery_cp < MaxSector && sh->sector >= recovery_cp)) {
> + (recovery_cp < MaxSector && sh->sector >= recovery_cp &&
> + s->failed == 0)) {
> /* Calculate the real rcw later - for now make it
>* look like rcw is cheaper
>*/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: rtl8723au: multiple condition with no effect - if identical to else
Nicholas Mc Guire writes: > A number if/else if/else branches are identical - so the condition has no > effect on the effective code and can be significantly simplified - this > patch removes the condition and the duplicated code. > > Signed-off-by: Nicholas Mc Guire > --- > > This looks like the output of some broken code-generator - unlikely someone > wrote this by hand. In any case it needs a review by someone that knows the > details of the driver. > > Anyway the number of useless code repetition is potentially record breaking ! > > Patch was compile tested for x86_64_defconfig + CONFIG_STAGING=y > CONFIG_R8723AU=m, CONFIG_8723AU_BT_COEXIST=y > > Patch is against 3.0.19-rc7 (localversoin = -next-20150203) > > .../staging/rtl8723au/hal/rtl8723a_bt-coexist.c| 60 > +++- > 1 file changed, 8 insertions(+), 52 deletions(-) Why make it simple if you can make it complicated :) I presume it's against 3.19-rc7 since a 3.0.19 would be rather obsolete. Signed-off-by: Jes Sorensen -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: rtl8723au: hal: usb_ops_linux: Removed variables that is never used
Julian Calaby writes:
> Hi Rickard,
>
> On Mon, Feb 2, 2015 at 10:17 AM, Rickard Strandqvist
> wrote:
>> 2015-02-02 0:01 GMT+01:00 Jes Sorensen :
>>> Rickard Strandqvist writes:
>>>> Variable was assigned a value that was never used.
>>>> I have also removed all the code that thereby serves no purpose.
>>>>
>>>> This was found using a static code analysis program called cppcheck
>>>>
>>>> Signed-off-by: Rickard Strandqvist
>>>> ---
>>>> drivers/staging/rtl8723au/hal/usb_ops_linux.c |9 +++--
>>>> 1 file changed, 3 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/rtl8723au/hal/usb_ops_linux.c
>>>> b/drivers/staging/rtl8723au/hal/usb_ops_linux.c
>>>> index a6d16ad..4ae0a8a 100644
>>>> --- a/drivers/staging/rtl8723au/hal/usb_ops_linux.c
>>>> +++ b/drivers/staging/rtl8723au/hal/usb_ops_linux.c
>>>> @@ -26,11 +26,10 @@ u8 rtl8723au_read8(struct rtw_adapter *padapter, u16
>>>> addr)
>>>> {
>>>> struct dvobj_priv *pdvobjpriv = adapter_to_dvobj(padapter);
>>>> struct usb_device *udev = pdvobjpriv->pusbdev;
>>>> - int len;
>>>> u8 data;
>>>>
>>>> mutex_lock(&pdvobjpriv->usb_vendor_req_mutex);
>>>> - len = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
>>>> + usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
>>>> REALTEK_USB_VENQT_CMD_REQ,
>>>> REALTEK_USB_VENQT_READ,
>>>> addr, 0, &pdvobjpriv->usb_buf.val8,
>>>> sizeof(data),
>>>> RTW_USB_CONTROL_MSG_TIMEOUT);
>>>> @@ -45,11 +44,10 @@ u16 rtl8723au_read16(struct rtw_adapter *padapter, u16
>>>> addr)
>>>> {
>>>> struct dvobj_priv *pdvobjpriv = adapter_to_dvobj(padapter);
>>>> struct usb_device *udev = pdvobjpriv->pusbdev;
>>>> - int len;
>>>> u16 data;
>>>>
>>>> mutex_lock(&pdvobjpriv->usb_vendor_req_mutex);
>>>> - len = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
>>>> + usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
>>>> REALTEK_USB_VENQT_CMD_REQ,
>>>> REALTEK_USB_VENQT_READ,
>>>> addr, 0, &pdvobjpriv->usb_buf.val16,
>>>> sizeof(data),
>>>> RTW_USB_CONTROL_MSG_TIMEOUT);
>>>> @@ -64,11 +62,10 @@ u32 rtl8723au_read32(struct rtw_adapter *padapter, u16
>>>> addr)
>>>> {
>>>> struct dvobj_priv *pdvobjpriv = adapter_to_dvobj(padapter);
>>>> struct usb_device *udev = pdvobjpriv->pusbdev;
>>>> - int len;
>>>> u32 data;
>>>>
>>>> mutex_lock(&pdvobjpriv->usb_vendor_req_mutex);
>>>> - len = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
>>>> + usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
>>>> REALTEK_USB_VENQT_CMD_REQ,
>>>> REALTEK_USB_VENQT_READ,
>>>> addr, 0, &pdvobjpriv->usb_buf.val32,
>>>> sizeof(data),
>>>> RTW_USB_CONTROL_MSG_TIMEOUT);
>>>
>>> Again, fix the formatting if you want to make this change.
>>>
>>> NACK
>>>
>>> Jes
>>
>>
>> Hi
>>
>> I am sorry, please ignore this.
>> I will not send any more patches.
>
> The issue isn't you sending patches, the issue is that you don't spend
> enough time on them.
>
> As I understand it, the formatting for function calls is that if the
> arguments flow onto a new line, they start in the column after the
> opening bracket of the function call.
>
> I.e.
>
> do_something(arg, arg, arg,
> /* 13 cols */arg, arg, arg);
>
> (Using tabs and spaces instead of the comment of course)
>
> In this case, you're dropping 6 characters from the line with the
> function call. You then need to re-flow the arguments to suit.
This is correct, of all the automated patches that flow in the staging
tree, I'd see these are some of the more useful ones, because they get
rid of unused clutter, making it easier to follow the actual code.
However, breaking coding style using automated tools is just going to
result in someone else having to go back and fix that up in a second
patch.
As Julian points out, please spend the little extra effort so we get it
right in the first patch.
Cheers,
Jes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: rtl8723au: core: rtw_mlme: Removed variables that is never used
Rickard Strandqvist writes:
> Variable was assigned a value that was never used.
> I have also removed all the code that thereby serves no purpose.
>
> This was found using a static code analysis program called cppcheck
>
> Signed-off-by: Rickard Strandqvist
> ---
> drivers/staging/rtl8723au/core/rtw_mlme.c | 21 ++---
> 1 file changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/staging/rtl8723au/core/rtw_mlme.c
> b/drivers/staging/rtl8723au/core/rtw_mlme.c
> index 7299ef0..8b2db4b 100644
> --- a/drivers/staging/rtl8723au/core/rtw_mlme.c
> +++ b/drivers/staging/rtl8723au/core/rtw_mlme.c
> @@ -2123,7 +2123,6 @@ bool rtw_restructure_ht_ie23a(struct rtw_adapter
> *padapter, u8 *in_ie,
> {
> u32 out_len;
> int max_rx_ampdu_factor;
> - unsigned char *pframe;
> const u8 *p;
> struct ieee80211_ht_cap ht_capie;
> u8 WMM_IE[7] = {0x00, 0x50, 0xf2, 0x02, 0x00, 0x01, 0x00};
> @@ -2139,10 +2138,10 @@ bool rtw_restructure_ht_ie23a(struct rtw_adapter
> *padapter, u8 *in_ie,
>
> if (pmlmepriv->qos_option == 0) {
> out_len = *pout_len;
> - pframe = rtw_set_ie23a(out_ie + out_len,
> -WLAN_EID_VENDOR_SPECIFIC,
> -sizeof(WMM_IE), WMM_IE,
> -pout_len);
> + rtw_set_ie23a(out_ie + out_len,
> + WLAN_EID_VENDOR_SPECIFIC,
> + sizeof(WMM_IE), WMM_IE,
> + pout_len);
And again here - do it properly, please.
Jes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: rtl8723au: hal: usb_ops_linux: Removed variables that is never used
Rickard Strandqvist writes:
> Variable was assigned a value that was never used.
> I have also removed all the code that thereby serves no purpose.
>
> This was found using a static code analysis program called cppcheck
>
> Signed-off-by: Rickard Strandqvist
> ---
> drivers/staging/rtl8723au/hal/usb_ops_linux.c |9 +++--
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/rtl8723au/hal/usb_ops_linux.c
> b/drivers/staging/rtl8723au/hal/usb_ops_linux.c
> index a6d16ad..4ae0a8a 100644
> --- a/drivers/staging/rtl8723au/hal/usb_ops_linux.c
> +++ b/drivers/staging/rtl8723au/hal/usb_ops_linux.c
> @@ -26,11 +26,10 @@ u8 rtl8723au_read8(struct rtw_adapter *padapter, u16 addr)
> {
> struct dvobj_priv *pdvobjpriv = adapter_to_dvobj(padapter);
> struct usb_device *udev = pdvobjpriv->pusbdev;
> - int len;
> u8 data;
>
> mutex_lock(&pdvobjpriv->usb_vendor_req_mutex);
> - len = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
> + usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
> REALTEK_USB_VENQT_CMD_REQ, REALTEK_USB_VENQT_READ,
> addr, 0, &pdvobjpriv->usb_buf.val8, sizeof(data),
> RTW_USB_CONTROL_MSG_TIMEOUT);
> @@ -45,11 +44,10 @@ u16 rtl8723au_read16(struct rtw_adapter *padapter, u16
> addr)
> {
> struct dvobj_priv *pdvobjpriv = adapter_to_dvobj(padapter);
> struct usb_device *udev = pdvobjpriv->pusbdev;
> - int len;
> u16 data;
>
> mutex_lock(&pdvobjpriv->usb_vendor_req_mutex);
> - len = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
> + usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
> REALTEK_USB_VENQT_CMD_REQ, REALTEK_USB_VENQT_READ,
> addr, 0, &pdvobjpriv->usb_buf.val16, sizeof(data),
> RTW_USB_CONTROL_MSG_TIMEOUT);
> @@ -64,11 +62,10 @@ u32 rtl8723au_read32(struct rtw_adapter *padapter, u16
> addr)
> {
> struct dvobj_priv *pdvobjpriv = adapter_to_dvobj(padapter);
> struct usb_device *udev = pdvobjpriv->pusbdev;
> - int len;
> u32 data;
>
> mutex_lock(&pdvobjpriv->usb_vendor_req_mutex);
> - len = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
> + usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
> REALTEK_USB_VENQT_CMD_REQ, REALTEK_USB_VENQT_READ,
> addr, 0, &pdvobjpriv->usb_buf.val32, sizeof(data),
> RTW_USB_CONTROL_MSG_TIMEOUT);
Again, fix the formatting if you want to make this change.
NACK
Jes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: rtl8723au: hal: HalDMOutSrc8723A_CE: Removed variables that is never used
Rickard Strandqvist writes:
> Variable was assigned a value that was never used.
> I have also removed all the code that thereby serves no purpose.
>
> This was found using a static code analysis program called cppcheck
>
> Signed-off-by: Rickard Strandqvist
> ---
> drivers/staging/rtl8723au/hal/HalDMOutSrc8723A_CE.c |6 ++
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/rtl8723au/hal/HalDMOutSrc8723A_CE.c
> b/drivers/staging/rtl8723au/hal/HalDMOutSrc8723A_CE.c
> index 179a1ba..3cbbfb7 100644
> --- a/drivers/staging/rtl8723au/hal/HalDMOutSrc8723A_CE.c
> +++ b/drivers/staging/rtl8723au/hal/HalDMOutSrc8723A_CE.c
> @@ -743,10 +743,8 @@ static void _PHY_IQCalibrate(struct rtw_adapter
> *pAdapter, int result[][8], u8 t
> /* Note: IQ calibration must be performed after loading */
> /* PHY_REG.txt , and radio_a, radio_b.txt */
>
> - u32 bbvalue;
> -
> if (t == 0) {
> - bbvalue = PHY_QueryBBReg(pAdapter, rFPGA0_RFMOD, bMaskDWord);
> + PHY_QueryBBReg(pAdapter, rFPGA0_RFMOD, bMaskDWord);
In this particular case, I'd say to get rid of the call completely.
FPGA0_RFMOD should not have magic side effects just from being read.
Jes
>
> /* Save ADDA parameters, turn Path A ADDA on */
> _PHY_SaveADDARegisters(pAdapter, ADDA_REG,
> pdmpriv->ADDA_backup, IQK_ADDA_REG_NUM);
> @@ -1047,7 +1045,7 @@ void rtl8723a_phy_iq_calibrate(struct rtw_adapter
> *pAdapter, bool bReCovery)
> bPathAOK = bPathBOK = true;
> } else {
> RegE94 = RegEB4 = pdmpriv->RegE94 = pdmpriv->RegEB4 = 0x100;
> /* X default value */
> - RegE9C = RegEBC = pdmpriv->RegE9C = pdmpriv->RegEBC = 0x0;
> /* Y default value */
> + pdmpriv->RegE9C = pdmpriv->RegEBC = 0x0;/* Y
> default value */
> }
>
> if ((RegE94 != 0)/*&&(RegEA4 != 0)*/)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: rtl8723au: core: rtw_mlme_ext: Removed variables that is never used
Rickard Strandqvist writes:
> Variable was assigned a value that was never used.
> I have also removed all the code that thereby serves no purpose.
>
> This was found using a static code analysis program called cppcheck
>
> Signed-off-by: Rickard Strandqvist
> ---
> drivers/staging/rtl8723au/core/rtw_mlme_ext.c |7 +--
> 1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/staging/rtl8723au/core/rtw_mlme_ext.c
> b/drivers/staging/rtl8723au/core/rtw_mlme_ext.c
> index 0e0f73c..7c60fed 100644
> --- a/drivers/staging/rtl8723au/core/rtw_mlme_ext.c
> +++ b/drivers/staging/rtl8723au/core/rtw_mlme_ext.c
> @@ -2515,7 +2515,6 @@ static void issue_probersp(struct rtw_adapter
> *padapter, unsigned char *da,
> unsigned char *mac, *bssid;
> struct xmit_priv *pxmitpriv = &padapter->xmitpriv;
> #ifdef CONFIG_8723AU_AP_MODE
> - const u8 *pwps_ie;
> u8 *ssid_ie;
> int ssid_ielen;
> int ssid_ielen_diff;
> @@ -2576,7 +2575,7 @@ static void issue_probersp(struct rtw_adapter
> *padapter, unsigned char *da,
>
> #ifdef CONFIG_8723AU_AP_MODE
> if ((pmlmeinfo->state & 0x03) == MSR_AP) {
> - pwps_ie = cfg80211_find_vendor_ie(WLAN_OUI_MICROSOFT,
> + cfg80211_find_vendor_ie(WLAN_OUI_MICROSOFT,
> WLAN_OUI_TYPE_MICROSOFT_WPS,
> cur_network->IEs,
> cur_network->IELength);
If you want to modify this, you need to fix up the formatting to go with
it.
NACK
Jes
> @@ -6196,13 +6195,9 @@ int set_chplan_hdl23a(struct rtw_adapter *padapter,
> const u8 *pbuf)
>
> int led_blink_hdl23a(struct rtw_adapter *padapter, const u8 *pbuf)
> {
> - struct LedBlink_param *ledBlink_param;
> -
> if (!pbuf)
> return H2C_PARAMETERS_ERROR;
>
> - ledBlink_param = (struct LedBlink_param *)pbuf;
> -
> return H2C_SUCCESS;
> }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] drivers: staging: rtl8723au: get rid of unneeded memset/memcpy
Greg Kroah-Hartman writes:
> On Sun, Dec 07, 2014 at 03:37:20PM +0100, Emil Renner Berthing wrote:
>> This also fixes a sparse warning.
>
> What sparse warning? What's wrong with the original code? Unless Jes
> resends this to me, I don't see the need to apply it, sorry.
I agree, I cannot see what this is fixing, it does however add an ugly
cast.
If you want to change the paramters passed to FillH2CCmd() then change
the prototype and create something like struct h2c_cmd_arg{} and apply
that across the board.
NACK
Jes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: rtl8723au: changes stop ap mode to use reset function
Matthew Emerson writes: > Changes stop_ap_mode23a() to use rtw_reset_securitypriv23a. This makes > the code cleaner and fixes two checkpatch.pl errors, one line over 80 > characters, and one spacing issue. > > Signed-off-by: Matthew Emerson > --- > drivers/staging/rtl8723au/core/rtw_ap.c | 6 ++ > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/staging/rtl8723au/core/rtw_ap.c > b/drivers/staging/rtl8723au/core/rtw_ap.c > index e394d12c36b0..510ee212496f 100644 > --- a/drivers/staging/rtl8723au/core/rtw_ap.c > +++ b/drivers/staging/rtl8723au/core/rtw_ap.c > @@ -1870,10 +1870,8 @@ void stop_ap_mode23a(struct rtw_adapter *padapter) > pmlmepriv->update_bcn = false; > pmlmeext->bstart_bss = false; > > - /* reset and init security priv , this can refine with > rtw_reset_securitypriv23a */ > - memset((unsigned char *)&padapter->securitypriv, 0, sizeof (struct > security_priv)); > - padapter->securitypriv.ndisauthtype = Ndis802_11AuthModeOpen; > - padapter->securitypriv.ndisencryptstatus = Ndis802_11WEPDisabled; > + /* reset and init security priv */ > + rtw_reset_securitypriv23a(padapter); This is broken, rtw_reset_securitypriv23a() takes different actions based on the value of adapter->securitypriv.dot11AuthAlgrthm - you leave it at it's old value, which does not have the same effect as the above. If anything you should remove the memset() and call rtw_reset_securitypriv23a() after the two assignments. NACK Jes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: rtl8723au: os_dep: usb_intf.c: Remove some unused functions
Rickard Strandqvist writes:
> Removes some functions that are not used anywhere:
> rtw_hw_resume23a() rtw_hw_suspend23a()
>
> This was partially found by using a static code analysis program called
> cppcheck.
>
> Signed-off-by: Rickard Strandqvist
> ---
> drivers/staging/rtl8723au/include/osdep_intf.h |3 -
> drivers/staging/rtl8723au/os_dep/usb_intf.c| 100
>
> 2 files changed, 103 deletions(-)
Signed-off-by: Jes Sorensen
>
> diff --git a/drivers/staging/rtl8723au/include/osdep_intf.h
> b/drivers/staging/rtl8723au/include/osdep_intf.h
> index 33afa62..a157eb2 100644
> --- a/drivers/staging/rtl8723au/include/osdep_intf.h
> +++ b/drivers/staging/rtl8723au/include/osdep_intf.h
> @@ -19,9 +19,6 @@
> #include
> #include
>
> -int rtw_hw_suspend23a(struct rtw_adapter *padapter);
> -int rtw_hw_resume23a(struct rtw_adapter *padapter);
> -
> int rtw_init_drv_sw23a(struct rtw_adapter *padapter);
> int rtw_free_drv_sw23a(struct rtw_adapter *padapter);
> int rtw_reset_drv_sw23a(struct rtw_adapter *padapter);
> diff --git a/drivers/staging/rtl8723au/os_dep/usb_intf.c
> b/drivers/staging/rtl8723au/os_dep/usb_intf.c
> index 865743e..42eb90b 100644
> --- a/drivers/staging/rtl8723au/os_dep/usb_intf.c
> +++ b/drivers/staging/rtl8723au/os_dep/usb_intf.c
> @@ -287,106 +287,6 @@ static void rtw_dev_unload(struct rtw_adapter *padapter)
> RT_TRACE(_module_hci_intfs_c_, _drv_err_, ("-rtw_dev_unload\n"));
> }
>
> -int rtw_hw_suspend23a(struct rtw_adapter *padapter)
> -{
> - struct pwrctrl_priv *pwrpriv = &padapter->pwrctrlpriv;
> - struct net_device *pnetdev = padapter->pnetdev;
> - struct mlme_priv *pmlmepriv = &padapter->mlmepriv;
> -
> - if ((!padapter->bup) || (padapter->bDriverStopped) ||
> - (padapter->bSurpriseRemoved)) {
> - DBG_8723A("padapter->bup =%d bDriverStopped =%d
> bSurpriseRemoved = %d\n",
> - padapter->bup, padapter->bDriverStopped,
> - padapter->bSurpriseRemoved);
> - goto error_exit;
> - }
> -
> - if (padapter) { /* system suspend */
> - LeaveAllPowerSaveMode23a(padapter);
> -
> - DBG_8723A("==> rtw_hw_suspend23a\n");
> - down(&pwrpriv->lock);
> - pwrpriv->bips_processing = true;
> - /* padapter->net_closed = true; */
> - /* s1. */
> - if (pnetdev) {
> - netif_carrier_off(pnetdev);
> - netif_tx_stop_all_queues(pnetdev);
> - }
> -
> - /* s2. */
> - rtw_disassoc_cmd23a(padapter, 500, false);
> -
> - /* s2-2. indicate disconnect to os */
> - /* rtw_indicate_disconnect23a(padapter); */
> - if (check_fwstate(pmlmepriv, _FW_LINKED)) {
> - _clr_fwstate_(pmlmepriv, _FW_LINKED);
> -
> - rtw_led_control(padapter, LED_CTL_NO_LINK);
> -
> - rtw_os_indicate_disconnect23a(padapter);
> -
> - /* donnot enqueue cmd */
> - rtw_lps_ctrl_wk_cmd23a(padapter,
> -LPS_CTRL_DISCONNECT, 0);
> - }
> - /* s2-3. */
> - rtw_free_assoc_resources23a(padapter, 1);
> -
> - /* s2-4. */
> - rtw_free_network_queue23a(padapter);
> - rtw_ips_dev_unload23a(padapter);
> - pwrpriv->rf_pwrstate = rf_off;
> - pwrpriv->bips_processing = false;
> - up(&pwrpriv->lock);
> - } else {
> - goto error_exit;
> - }
> - return 0;
> -error_exit:
> - DBG_8723A("%s, failed\n", __func__);
> - return -1;
> -}
> -
> -int rtw_hw_resume23a(struct rtw_adapter *padapter)
> -{
> - struct pwrctrl_priv *pwrpriv = &padapter->pwrctrlpriv;
> - struct net_device *pnetdev = padapter->pnetdev;
> -
> - if (padapter) { /* system resume */
> - DBG_8723A("==> rtw_hw_resume23a\n");
> - down(&pwrpriv->lock);
> - pwrpriv->bips_processing = true;
> - rtw_reset_drv_sw23a(padapter);
> -
> - if (pm_netdev_open23a(pnetdev, false)) {
> - up(&pwrpriv->lock);
> - goto error_exit;
> - }
> -
> - netif_device_attach(pnetdev);
> - netif_carrier_on(pnetdev);
> -
> - if (!rtw_netif_queue_stopped(pnetd
Re: [PATCH v2] staging: rtl8723au: core: fixing "foo * bar" should be "foo *bar"
Asaf Vertz writes:
> Fixed a coding style error, "foo * bar" should be "foo *bar"
>
> Signed-off-by: Asaf Vertz
> ---
> Changes in v2:
> - fix ugly multiline arguments to the functions
>
> drivers/staging/rtl8723au/core/rtw_efuse.c | 32 ++-
> 1 files changed, 7 insertions(+), 25 deletions(-)
Signed-off-by: Jes Sorensen
> diff --git a/drivers/staging/rtl8723au/core/rtw_efuse.c
> b/drivers/staging/rtl8723au/core/rtw_efuse.c
> index 81960e7..a6deddc 100644
> --- a/drivers/staging/rtl8723au/core/rtw_efuse.c
> +++ b/drivers/staging/rtl8723au/core/rtw_efuse.c
> @@ -327,15 +327,9 @@ EFUSE_Read1Byte23a(struct rtw_adapter *Adapter, u16
> Address)
>
> *---*/
>
> void
> -EFUSE_Write1Byte(
> - struct rtw_adapter *Adapter,
> - u16 Address,
> - u8 Value);
> +EFUSE_Write1Byte(struct rtw_adapter *Adapter, u16 Address, u8 Value);
> void
> -EFUSE_Write1Byte(
> - struct rtw_adapter *Adapter,
> - u16 Address,
> - u8 Value)
> +EFUSE_Write1Byte(struct rtw_adapter *Adapter, u16 Address, u8 Value)
> {
> u8 Bytetemp = {0x00};
> u8 temp = {0x00};
> @@ -635,10 +629,7 @@ Efuse_ReadAllMap(struct rtw_adapter *pAdapter, u8
> efuseType, u8 *Efuse)
> *
>
> *---*/
> static void
> -efuse_ShadowRead1Byte(
> - struct rtw_adapter *pAdapter,
> - u16 Offset,
> - u8 *Value)
> +efuse_ShadowRead1Byte(struct rtw_adapter *pAdapter, u16 Offset, u8 *Value)
> {
> struct eeprom_priv *pEEPROM = GET_EEPROM_EFUSE_PRIV(pAdapter);
>
> @@ -647,10 +638,7 @@ efuse_ShadowRead1Byte(
>
> /* Read Two Bytes */
> static void
> -efuse_ShadowRead2Byte(
> - struct rtw_adapter *pAdapter,
> - u16 Offset,
> - u16 *Value)
> +efuse_ShadowRead2Byte(struct rtw_adapter *pAdapter, u16 Offset, u16 *Value)
> {
> struct eeprom_priv *pEEPROM = GET_EEPROM_EFUSE_PRIV(pAdapter);
>
> @@ -660,10 +648,7 @@ efuse_ShadowRead2Byte(
>
> /* Read Four Bytes */
> static void
> -efuse_ShadowRead4Byte(
> - struct rtw_adapter *pAdapter,
> - u16 Offset,
> - u32 *Value)
> +efuse_ShadowRead4Byte(struct rtw_adapter *pAdapter, u16 Offset, u32 *Value)
> {
> struct eeprom_priv *pEEPROM = GET_EEPROM_EFUSE_PRIV(pAdapter);
>
> @@ -722,11 +707,8 @@ void EFUSE_ShadowMapUpdate23a(struct rtw_adapter
> *pAdapter, u8 efuseType)
> *
>
> *---*/
> void
> -EFUSE_ShadowRead23a(
> - struct rtw_adapter *pAdapter,
> - u8 Type,
> - u16 Offset,
> - u32 *Value)
> +EFUSE_ShadowRead23a(struct rtw_adapter *pAdapter,
> + u8 Type, u16 Offset, u32 *Value)
> {
> if (Type == 1)
> efuse_ShadowRead1Byte(pAdapter, Offset, (u8 *)Value);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: rtl8723au: core: fixing "foo * bar" should be "foo *bar"
Asaf Vertz writes:
> Fixed a coding style error, "foo * bar" should be "foo *bar"
>
> Signed-off-by: Asaf Vertz
> ---
> drivers/staging/rtl8723au/core/rtw_efuse.c | 12 ++--
> 1 files changed, 6 insertions(+), 6 deletions(-)
If you want to fix the 'error' here, include it with a cleanup that also
fixes the ugly multiline arguments to the functions.
Jes
> diff --git a/drivers/staging/rtl8723au/core/rtw_efuse.c
> b/drivers/staging/rtl8723au/core/rtw_efuse.c
> index 81960e7..0e64d12 100644
> --- a/drivers/staging/rtl8723au/core/rtw_efuse.c
> +++ b/drivers/staging/rtl8723au/core/rtw_efuse.c
> @@ -328,12 +328,12 @@ EFUSE_Read1Byte23a(struct rtw_adapter *Adapter, u16
> Address)
>
> void
> EFUSE_Write1Byte(
> - struct rtw_adapter *Adapter,
> + struct rtw_adapter *Adapter,
> u16 Address,
> u8 Value);
> void
> EFUSE_Write1Byte(
> - struct rtw_adapter *Adapter,
> + struct rtw_adapter *Adapter,
> u16 Address,
> u8 Value)
> {
> @@ -636,7 +636,7 @@ Efuse_ReadAllMap(struct rtw_adapter *pAdapter, u8
> efuseType, u8 *Efuse)
>
> *---*/
> static void
> efuse_ShadowRead1Byte(
> - struct rtw_adapter *pAdapter,
> + struct rtw_adapter *pAdapter,
> u16 Offset,
> u8 *Value)
> {
> @@ -648,7 +648,7 @@ efuse_ShadowRead1Byte(
> /* Read Two Bytes */
> static void
> efuse_ShadowRead2Byte(
> - struct rtw_adapter *pAdapter,
> + struct rtw_adapter *pAdapter,
> u16 Offset,
> u16 *Value)
> {
> @@ -661,7 +661,7 @@ efuse_ShadowRead2Byte(
> /* Read Four Bytes */
> static void
> efuse_ShadowRead4Byte(
> - struct rtw_adapter *pAdapter,
> + struct rtw_adapter *pAdapter,
> u16 Offset,
> u32 *Value)
> {
> @@ -723,7 +723,7 @@ void EFUSE_ShadowMapUpdate23a(struct rtw_adapter
> *pAdapter, u8 efuseType)
>
> *---*/
> void
> EFUSE_ShadowRead23a(
> - struct rtw_adapter *pAdapter,
> + struct rtw_adapter *pAdapter,
> u8 Type,
> u16 Offset,
> u32 *Value)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: rtl8723au: os_dep: usb_intf.c: Fix for possible null pointer dereference
Dan Carpenter writes:
> On Sun, Dec 14, 2014 at 11:39:14PM +0100, Rickard Strandqvist wrote:
>> There is otherwise a risk of a possible null pointer dereference.
>>
>> Was largely found by using a static code analysis program called cppcheck.
>>
>> Signed-off-by: Rickard Strandqvist
>> ---
>> drivers/staging/rtl8723au/os_dep/usb_intf.c |3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/rtl8723au/os_dep/usb_intf.c
>> b/drivers/staging/rtl8723au/os_dep/usb_intf.c
>> index 865743e..71a6330 100644
>> --- a/drivers/staging/rtl8723au/os_dep/usb_intf.c
>> +++ b/drivers/staging/rtl8723au/os_dep/usb_intf.c
>> @@ -351,10 +351,11 @@ error_exit:
>> int rtw_hw_resume23a(struct rtw_adapter *padapter)
>
> That's weird. Is this function even called?
[jes@ultrasam jes.git]$ find drivers/staging/rtl8723au -name \*.[ch] |xargs
grep rtw_hw_resume
drivers/staging/rtl8723au/include/osdep_intf.h:int rtw_hw_resume23a(struct
rtw_adapter *padapter);
drivers/staging/rtl8723au/os_dep/usb_intf.c:int rtw_hw_resume23a(struct
rtw_adapter *padapter)
drivers/staging/rtl8723au/os_dep/usb_intf.c:DBG_8723A("==>
rtw_hw_resume23a\n");
[jes@ultrasam jes.git]$ find drivers/staging/rtl8723au -name \*.[ch] |xargs
grep rtw_hw_suspend
drivers/staging/rtl8723au/include/osdep_intf.h:int rtw_hw_suspend23a(struct
rtw_adapter *padapter);
drivers/staging/rtl8723au/os_dep/usb_intf.c:int rtw_hw_suspend23a(struct
rtw_adapter *padapter)
drivers/staging/rtl8723au/os_dep/usb_intf.c:DBG_8723A("==>
rtw_hw_suspend23a\n");
A more useful patch would be one removing those two functions IMHO.
Jes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: rtl8723au: Fix sparse warnings
Krzysztof Konopko writes:
> Some struct fields in wifi.h are meant to be __le16 but were declared as
> unsigned short. This was reported by sparse:
>
> rtw_wlan_util.c:538:24: warning: cast to restricted __le16
> rtw_wlan_util.c:1544:29: warning: cast to restricted __le16
> rtw_wlan_util.c:1546:25: warning: cast to restricted __le16
>
> This patch updates the types of the fields in `AC_param` and
> `ADDBA_request` structs to be consistent with relevant structs in
> include/linux/ieee80211.h.
>
> Signed-off-by: Krzysztof Konopko
> ---
> drivers/staging/rtl8723au/include/wifi.h | 14 +++---
> 1 file changed, 7 insertions(+), 7 deletions(-)
Well the u8 change is really in the nit picking space, but I am fine
with that too.
Signed-off-by: Jes Sorensen
>
> diff --git a/drivers/staging/rtl8723au/include/wifi.h
> b/drivers/staging/rtl8723au/include/wifi.h
> index fd3da3b..25d573c 100644
> --- a/drivers/staging/rtl8723au/include/wifi.h
> +++ b/drivers/staging/rtl8723au/include/wifi.h
> @@ -26,9 +26,9 @@
>
> --*/
>
> struct AC_param {
> - unsigned char ACI_AIFSN;
> - unsigned char CW;
> - unsigned short TXOP_limit;
> + u8 ACI_AIFSN;
> + u8 CW;
> + __le16 TXOP_limit;
> } __packed;
>
> struct WMM_para_element {
> @@ -38,10 +38,10 @@ struct WMM_para_element {
> } __packed;
>
> struct ADDBA_request {
> - unsigned char dialog_token;
> - unsigned short BA_para_set;
> - unsigned short BA_timeout_value;
> - unsigned short BA_starting_seqctrl;
> + u8 dialog_token;
> + __le16 BA_para_set;
> + __le16 BA_timeout_value;
> + __le16 BA_starting_seqctrl;
> } __packed;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: rtl8723au: Fix sparse warnings
Krzysztof Konopko writes:
> Some struct fields in wifi.h are meant to be __le16 but were declared as
> unsigned short. This was reported by sparse:
>
> rtw_wlan_util.c:538:24: warning: cast to restricted __le16
> rtw_wlan_util.c:1544:29: warning: cast to restricted __le16
> rtw_wlan_util.c:1546:25: warning: cast to restricted __le16
>
> This patch changes the types of the struct fields involved to be
> little-endian which is what is received over the air and consistent with
> relevant structs in include/linux/ieee80211.h.
>
> Signed-off-by: Krzysztof Konopko
> ---
> drivers/staging/rtl8723au/include/wifi.h | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
Looks OK
Signed-off-by: Jes Sorensen
>
> diff --git a/drivers/staging/rtl8723au/include/wifi.h
> b/drivers/staging/rtl8723au/include/wifi.h
> index fd3da3b..266c43e 100644
> --- a/drivers/staging/rtl8723au/include/wifi.h
> +++ b/drivers/staging/rtl8723au/include/wifi.h
> @@ -28,7 +28,7 @@
> struct AC_param {
> unsigned char ACI_AIFSN;
> unsigned char CW;
> - unsigned short TXOP_limit;
> + __le16 TXOP_limit;
> } __packed;
>
> struct WMM_para_element {
> @@ -39,9 +39,9 @@ struct WMM_para_element {
>
> struct ADDBA_request {
> unsigned char dialog_token;
> - unsigned short BA_para_set;
> - unsigned short BA_timeout_value;
> - unsigned short BA_starting_seqctrl;
> + __le16 BA_para_set;
> + __le16 BA_timeout_value;
> + __le16 BA_starting_seqctrl;
> } __packed;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: rtl8723au: Fix sparse warnings
Krzysztof Konopko writes: > On 12/12/14 19:52, Jes Sorensen wrote: >> Larry Finger writes: >>> On 12/12/2014 05:35 AM, Krzysztof Konopko wrote: >>>> I was hunting particularly for inconsistencies with `sparse` and came >>>> across this one. But I dug a bit further and I wonder why the driver is >>>> not using standard stuff like the one in `include/linux/ieee80211.h` >>>> where any data wider than one byte is clearly declared as __le? >>> >>> That is a good question. One possibility is that those definitions do >>> not exist on some of the older kernels that Realtek supports. They >>> generally work with 2.6.18 and newer. >> >> The reason the 8723au driver doesn't use the defines from there is that >> in ieee80211.h they are part of struct ieee80211_mgmt, while the 8723au >> driver access the addba etc. elements without the full struct in place. >> > > And why is that the case? > (I'm trying to understand, not debunk) > > Looks to me that this driver has been kept out of the tree for quite a > while (by Realtek) and now suffers from locally invented stuff. I > understand this is a lot of work to unify the codebase with ieee80211.h, > but are there any technical hurdles? I'm just curious. The main issue is that the RTL driver maintains a partial copy of the management frame, ie. without the front block containing the MAC addresses. Switching this over to carry a full copy of the frame is extremely intrusive as it's mixed in pretty much everywhere in the driver. The driver is derived from Realtek's multi-OS vendor driver, which included code for pretty much every OS on the planet. If you want to see how it looked initially, check out Larry's github tree and go back to the initial checkins Realtek seem to copy it into a new tree and devel from there whenever they do a new chip, so the legacy in this codebase is huge. Cheers, Jes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: rtl8723au: Fix sparse warnings
Larry Finger writes: > On 12/12/2014 05:35 AM, Krzysztof Konopko wrote: >> I was hunting particularly for inconsistencies with `sparse` and came >> across this one. But I dug a bit further and I wonder why the driver is >> not using standard stuff like the one in `include/linux/ieee80211.h` >> where any data wider than one byte is clearly declared as __le? > > That is a good question. One possibility is that those definitions do > not exist on some of the older kernels that Realtek supports. They > generally work with 2.6.18 and newer. The reason the 8723au driver doesn't use the defines from there is that in ieee80211.h they are part of struct ieee80211_mgmt, while the 8723au driver access the addba etc. elements without the full struct in place. Jes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: rtl8723au: Fix sparse warnings
Krzysztof Konopko writes:
> Some struct fields in wifi.h are meant to be __le16 bu were declared as
> unsigned short. This was reported by sparse:
>
> rtw_wlan_util.c:538:24: warning: cast to restricted __le16
> rtw_wlan_util.c:1544:29: warning: cast to restricted __le16
> rtw_wlan_util.c:1546:25: warning: cast to restricted __le16
>
> This patch changes declared types of the struct fields involved.
>
> Signed-off-by: Krzysztof Konopko
> ---
> drivers/staging/rtl8723au/include/wifi.h | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/rtl8723au/include/wifi.h
> b/drivers/staging/rtl8723au/include/wifi.h
> index fd3da3b..8a2adc5 100644
> --- a/drivers/staging/rtl8723au/include/wifi.h
> +++ b/drivers/staging/rtl8723au/include/wifi.h
> @@ -28,7 +28,7 @@
> struct AC_param {
> unsigned char ACI_AIFSN;
> unsigned char CW;
> - unsigned short TXOP_limit;
> + __le16 TXOP_limit;
> } __packed;
>
> struct WMM_para_element {
> @@ -39,9 +39,9 @@ struct WMM_para_element {
>
> struct ADDBA_request {
> unsigned char dialog_token;
> - unsigned short BA_para_set;
> + __le16 BA_para_set;
> unsigned short BA_timeout_value;
> - unsigned short BA_starting_seqctrl;
> + __le16 BA_starting_seqctrl;
> } __packed;
If you are going to make the struct comply with the on-wire data format,
be consistent. Don't just change half the elements of the struct - that
will just lead to confusion.
Jes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: rtl8723au: Fix sparse warnings
Krzysztof Konopko writes: > On 12/12/14 00:53, Larry Finger wrote: >> In RTL8188EU, both BA_starting_seqctrl and TXOP_limit are unsigned short. >> > > That's not quite the case. `TXOP_limit` is __le16 in RTL8188EU [1]. > It's __le16 even in your GitHub repo [2]. And that made me thinking > that there's probably some inconsistency in the header. > > I'm _far_ from being a wireless expert but doesn't data coming out of > the wire/air have the endianess defined explicitly? And both `AC_param` > and `ADDBA_request` come out of air? > > I was hunting particularly for inconsistencies with `sparse` and came > across this one. But I dug a bit further and I wonder why the driver is > not using standard stuff like the one in `include/linux/ieee80211.h` > where any data wider than one byte is clearly declared as __le? In general all over the wire data is little-endian. The driver has been slowly moved towards using the standard defines from the Linux headers, but this is a *lot* of work, and it requires testing. I really don't get warm fuzzy feelings from patches that blindly make these kinds of changes without also testing them. Jes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Drivers:staging:rtl8723au:core: Fixed checkpatch error
Athira Lekshmi writes: > Fixed the checkpatch error: > ERROR: spaces required around that '>' > > Signed-off-by: Athira Lekshmi > --- > drivers/staging/rtl8723au/core/rtw_cmd.c |4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/staging/rtl8723au/core/rtw_cmd.c > b/drivers/staging/rtl8723au/core/rtw_cmd.c > index 44eae8e..9f1cdd4 100644 > --- a/drivers/staging/rtl8723au/core/rtw_cmd.c > +++ b/drivers/staging/rtl8723au/core/rtw_cmd.c > @@ -960,7 +960,7 @@ static void traffic_status_watchdog(struct rtw_adapter > *padapter) > /* check traffic for powersaving. */ > if (((pmlmepriv->LinkDetectInfo.NumRxUnicastOkInPeriod + > pmlmepriv->LinkDetectInfo.NumTxOkInPeriod) > 8) || > - pmlmepriv->LinkDetectInfo.NumRxUnicastOkInPeriod >2) > + pmlmepriv->LinkDetectInfo.NumRxUnicastOkInPeriod > > 2) > bEnterPS = false; > else > bEnterPS = true; NACK - I already stated this! Making the line longer than 80 characters is worse then the 'problem' this fixes. Jes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: rtl8723au: change typecast to match type returned by htons()
Arend van Spriel writes: > On 10-11-14 21:21, Jes Sorensen wrote: >> Chris Ruffin writes: >>> Using a u16 pointer typecast for a result from htons() results in >>> the following warning from sparse: >>> >>> drivers/staging/rtl8723au/core/rtw_xmit.c:1279:36: warning: >>> incorrect type in assignment (different base types) >>> drivers/staging/rtl8723au/core/rtw_xmit.c:1279:36: expected >>> unsigned short [unsigned] [short] [usertype] >>> drivers/staging/rtl8723au/core/rtw_xmit.c:1279:36: got restricted >>> __be16 [usertype] >>> >>> This patch fixes the issue by using an endian-specific typecast >>> that will always match the type returned by htons(). >>> >>> Signed-off-by: Chris Ruffin >>> --- >>> drivers/staging/rtl8723au/core/rtw_xmit.c |2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> Looks fine to me >> >> Signed-off-by: Jes Sorensen >> >>> >>> diff --git a/drivers/staging/rtl8723au/core/rtw_xmit.c >>> b/drivers/staging/rtl8723au/core/rtw_xmit.c >>> index a0f7e27..44ef55c 100644 >>> --- a/drivers/staging/rtl8723au/core/rtw_xmit.c >>> +++ b/drivers/staging/rtl8723au/core/rtw_xmit.c >>> @@ -1276,7 +1276,7 @@ s32 rtw_put_snap23a(u8 *data, u16 h_proto) >>> snap->oui[0] = oui[0]; >>> snap->oui[1] = oui[1]; >>> snap->oui[2] = oui[2]; >>> - *(u16 *)(data + SNAP_SIZE) = htons(h_proto); >>> + *(__be16 *)(data + SNAP_SIZE) = htons(h_proto); > > Could (data + SNAP_SIZE) be on a unaligned address? It shouldn't but probably better to take it into account. I am moving things around in this code right now, so I'll just fix it in a follow-on patch to this one. Cheers, Jes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: rtl8723au: change typecast to match type returned by htons()
Chris Ruffin writes: > Using a u16 pointer typecast for a result from htons() results in the > following warning from sparse: > > drivers/staging/rtl8723au/core/rtw_xmit.c:1279:36: warning: incorrect type in > assignment (different base types) > drivers/staging/rtl8723au/core/rtw_xmit.c:1279:36:expected unsigned short > [unsigned] [short] [usertype] > drivers/staging/rtl8723au/core/rtw_xmit.c:1279:36:got restricted __be16 > [usertype] > > This patch fixes the issue by using an endian-specific typecast > that will always match the type returned by htons(). > > Signed-off-by: Chris Ruffin > --- > drivers/staging/rtl8723au/core/rtw_xmit.c |2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Looks fine to me Signed-off-by: Jes Sorensen > > diff --git a/drivers/staging/rtl8723au/core/rtw_xmit.c > b/drivers/staging/rtl8723au/core/rtw_xmit.c > index a0f7e27..44ef55c 100644 > --- a/drivers/staging/rtl8723au/core/rtw_xmit.c > +++ b/drivers/staging/rtl8723au/core/rtw_xmit.c > @@ -1276,7 +1276,7 @@ s32 rtw_put_snap23a(u8 *data, u16 h_proto) > snap->oui[0] = oui[0]; > snap->oui[1] = oui[1]; > snap->oui[2] = oui[2]; > - *(u16 *)(data + SNAP_SIZE) = htons(h_proto); > + *(__be16 *)(data + SNAP_SIZE) = htons(h_proto); > return SNAP_SIZE + sizeof(u16); > } -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging:rtl8723au: core: Fix Warning reported by checkpatch.
"Sharma, Sanjeev" writes:
> -Original Message-
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Thursday, November 06, 2014 9:13 PM
> To: Sharma, Sanjeev
> Cc: larry.fin...@lwfinger.net; jes.soren...@redhat.com;
> de...@driverdev.osuosl.org; linux-wirel...@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] staging:rtl8723au: core: Fix Warning reported by
> checkpatch.
>
> On Thu, Nov 06, 2014 at 12:06:36PM +0530, Sanjeev Sharma wrote:
>> This is a patch to the rtw_cmd.c file that fixes following Warning by
>> introducing temporary structure.
>>
>> WARNING: line over 80 characters
>>
>> Signed-off-by: Sanjeev Sharma
>> ---
>> drivers/staging/rtl8723au/core/rtw_cmd.c | 123
>> +++
>> 1 file changed, 60 insertions(+), 63 deletions(-)
>
> Same as the other patch, give us a hint as to the warning in the subject.
>
> This patch is Fix of Warning introduced in Previous patch while fixing "
> ERROR: spaces required around that '>' (ctx:WxV)".Can I mentioned dependency
> or hint in subject line or do we have
> another way to described these type of fix.(One patch introduced another
> Warning/Error)
When replying to these messages, please do it properly with proper
quoting. Do NOT resend other peoples' emails with just an
"-Original Message-" line and no comments and no indentation of
the message you respond to.
If you have any questions, please read:
https://www.ietf.org/rfc/rfc1855.txt
Jes
>
>>
>> diff --git a/drivers/staging/rtl8723au/core/rtw_cmd.c
>> b/drivers/staging/rtl8723au/core/rtw_cmd.c
>> index 4eaa502..6186575 100644
>> --- a/drivers/staging/rtl8723au/core/rtw_cmd.c
>> +++ b/drivers/staging/rtl8723au/core/rtw_cmd.c
>> @@ -919,34 +919,34 @@ static void traffic_status_watchdog(struct rtw_adapter
>> *padapter)
>> u8 bHigherBusyTxTraffic = false;
>> struct mlme_priv *pmlmepriv = &padapter->mlmepriv;
>> int BusyThreshold = 100;
>> +struct rt_link_detect *ldi = &pmlmepriv->LinkDetectInfo;
>> +
>> /* */
>> /* Determine if our traffic is busy now */
>> /* */
>> if (check_fwstate(pmlmepriv, _FW_LINKED)) {
>> if (rtl8723a_BT_coexist(padapter))
>> BusyThreshold = 50;
>> -else if (pmlmepriv->LinkDetectInfo.bBusyTraffic)
>> +else if (ldi->bBusyTraffic)
>> BusyThreshold = 75;
>> /* if we raise bBusyTraffic in last watchdog, using
>> lower threshold. */
>> -if (pmlmepriv->LinkDetectInfo.NumRxOkInPeriod > BusyThreshold ||
>> -pmlmepriv->LinkDetectInfo.NumTxOkInPeriod > BusyThreshold) {
>> +if (ldi->NumRxOkInPeriod > BusyThreshold ||
>> +ldi->NumTxOkInPeriod > BusyThreshold) {
>> bBusyTraffic = true;
>>
>> -if (pmlmepriv->LinkDetectInfo.NumRxOkInPeriod >
>> -pmlmepriv->LinkDetectInfo.NumTxOkInPeriod)
>> +if (ldi->NumRxOkInPeriod > ldi->NumTxOkInPeriod)
>> bRxBusyTraffic = true;
>> else
>> bTxBusyTraffic = true;
>> }
>>
>> /* Higher Tx/Rx data. */
>> -if (pmlmepriv->LinkDetectInfo.NumRxOkInPeriod > 4000 ||
>> -pmlmepriv->LinkDetectInfo.NumTxOkInPeriod > 4000) {
>> +if (ldi->NumRxOkInPeriod > 4000 ||
>> +ldi->NumTxOkInPeriod > 4000) {
>> bHigherBusyTraffic = true;
>>
>> -if (pmlmepriv->LinkDetectInfo.NumRxOkInPeriod >
>> -pmlmepriv->LinkDetectInfo.NumTxOkInPeriod)
>> +if (ldi->NumRxOkInPeriod > ldi->NumTxOkInPeriod)
>> bHigherBusyRxTraffic = true;
>> else
>> bHigherBusyTxTraffic = true;
>> @@ -955,9 +955,9 @@ static void traffic_status_watchdog(struct rtw_adapter
>> *padapter)
>> if (!rtl8723a_BT_coexist(padapter) ||
>> !rtl8723a_BT_using_antenna_1(padapter)) {
>> /* check traffic for powersaving. */
>> -if (((pmlmepriv->LinkDetectInfo.NumRxUnicastOkInPeriod +
>> - pmlmepriv->LinkDetectInfo.NumTxOkInPeriod) > 8) ||
>> -pmlmepriv->LinkDetectInfo.NumRxUnicastOkInPeriod >2)
>> +if (((ldi->NumRxUnicastOkInPeriod +
>> + ldi->NumTxOkInPeriod) > 8) ||
>> +ldi->NumRxUnicastOkInPeriod > 2)
>> bEnterPS = false;
>> else
>> bEnterPS = true;
>> @@ -971,15 +971,15 @@ static void traffic_status_watchdog(struct rtw_adapter
>> *padapter)
>> } else
>> LPS_Leave23a(padapter);
>>
>> -pmlmepriv->LinkDetectInfo.NumRxOkInPeriod = 0;
>> -pmlmepriv->LinkDe
Re: [PATCH] staging:rtl8723au: core: Fix error reported by checkpatch.
"Sharma, Sanjeev" writes:
> -Original Message-
> From: Joe Perches [mailto:j...@perches.com]
> Sent: Monday, October 27, 2014 8:23 PM
> To: Jes Sorensen
> Cc: Sharma, Sanjeev; larry.fin...@lwfinger.net;
> gre...@linuxfoundation.org; linux-wirel...@vger.kernel.org;
> de...@driverdev.osuosl.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] staging:rtl8723au: core: Fix error reported by
> checkpatch.
>
> On Mon, 2014-10-27 at 09:43 +0100, Jes Sorensen wrote:
>> Sanjeev Sharma writes:
>> > This is a patch to the rtw_cmd.c file that fixes Error reported by
>> > checkpatch.
> []
>> > diff --git a/drivers/staging/rtl8723au/core/rtw_cmd.c
>> > b/drivers/staging/rtl8723au/core/rtw_cmd.c
> []
>> > @@ -957,7 +957,7 @@ static void traffic_status_watchdog(struct rtw_adapter
>> > *padapter)
>> >/* check traffic for powersaving. */
>> >if (((pmlmepriv->LinkDetectInfo.NumRxUnicastOkInPeriod +
>> > pmlmepriv->LinkDetectInfo.NumTxOkInPeriod) > 8) ||
>> > - pmlmepriv->LinkDetectInfo.NumRxUnicastOkInPeriod >2)
>> > + pmlmepriv->LinkDetectInfo.NumRxUnicastOkInPeriod >
>> > 2)
>> >bEnterPS = false;
>> >else
>> >bEnterPS = true;
>>
>> This makes the line longer than 80 characters, that is worse than the
>> 'problem' you are fixing.
>
> The code already has dozens of long lines already.
>
> This is generally a problem because the variable names are pretty long so
> strict 80 column adherence generally isn't possible.
>
> A possible way to shorten these relatively long variable name/line lengths is
> to use a temporary for
>
> pmlmeprv->LinkDetectInfo
>
> struct rt_link_detect *ldi = &pmlmeprv->LinkDetectInfo;
>
> so:
>
> I am agree on this approach but Let's wait for Jes opinion on it.
>
> Sanjeev Sharma
>
> drivers/staging/rtl8723au/core/rtw_cmd.c | 46
>
> 1 file changed, 23 insertions(+), 23 deletions(-)
This is fine with me.
Jes
>
> diff --git a/drivers/staging/rtl8723au/core/rtw_cmd.c
> b/drivers/staging/rtl8723au/core/rtw_cmd.c
> index d2d7edf..1b24945 100644
> --- a/drivers/staging/rtl8723au/core/rtw_cmd.c
> +++ b/drivers/staging/rtl8723au/core/rtw_cmd.c
> @@ -922,34 +922,33 @@ static void traffic_status_watchdog(struct rtw_adapter
> *padapter)
> u8 bHigherBusyTxTraffic = false;
> struct mlme_priv *pmlmepriv = &padapter->mlmepriv;
> int BusyThreshold = 100;
> + struct rt_link_detect *ldi = &pmlmepriv->LinkDetectInfo;
> +
> /* */
> /* Determine if our traffic is busy now */
> /* */
> if (check_fwstate(pmlmepriv, _FW_LINKED)) {
> if (rtl8723a_BT_coexist(padapter))
> BusyThreshold = 50;
> - else if (pmlmepriv->LinkDetectInfo.bBusyTraffic)
> + else if (ldi->bBusyTraffic)
> BusyThreshold = 75;
> /* if we raise bBusyTraffic in last watchdog, using
> lower threshold. */
> - if (pmlmepriv->LinkDetectInfo.NumRxOkInPeriod > BusyThreshold ||
> - pmlmepriv->LinkDetectInfo.NumTxOkInPeriod > BusyThreshold) {
> + if (ldi->NumRxOkInPeriod > BusyThreshold ||
> + ldi->NumTxOkInPeriod > BusyThreshold) {
> bBusyTraffic = true;
>
> - if (pmlmepriv->LinkDetectInfo.NumRxOkInPeriod >
> - pmlmepriv->LinkDetectInfo.NumTxOkInPeriod)
> + if (ldi->NumRxOkInPeriod > ldi->NumTxOkInPeriod)
> bRxBusyTraffic = true;
> else
> bTxBusyTraffic = true;
> }
>
> /* Higher Tx/Rx data. */
> - if (pmlmepriv->LinkDetectInfo.NumRxOkInPeriod > 4000 ||
> - pmlmepriv->LinkDetectInfo.NumTxOkInPeriod > 4000) {
> + if (ldi->NumRxOkInPeriod > 4000 ||
> + ldi->NumTxOkInPeriod > 4000) {
> bHigherBusyTraffic = true;
> -
> - if (pmlmepriv->LinkDetectInfo.NumRxOkInPeriod >
> - pmlmepriv->LinkDetectInfo.NumTxOkInPeriod)
> + if (ldi->NumRxOkInPeriod > ldi->NumTxOkInPeriod)
> bHig
Re: [PATCH] staging:rtl8723au: core: Fix error reported by checkpatch.
"Sharma, Sanjeev" writes: > -Original Message----- > From: Jes Sorensen [mailto:jes.soren...@redhat.com] > Sent: Monday, October 27, 2014 2:13 PM > To: Sharma, Sanjeev > Cc: larry.fin...@lwfinger.net; gre...@linuxfoundation.org; > linux-wirel...@vger.kernel.org; de...@driverdev.osuosl.org; > linux-kernel@vger.kernel.org > Subject: Re: [PATCH] staging:rtl8723au: core: Fix error reported by > checkpatch. > > Sanjeev Sharma writes: >> This is a patch to the rtw_cmd.c file that fixes Error reported by >> checkpatch. >> >> Signed-off-by: Sanjeev Sharma >> --- >> drivers/staging/rtl8723au/core/rtw_cmd.c | 83 >> +++- >> 1 file changed, 40 insertions(+), 43 deletions(-) >> >> diff --git a/drivers/staging/rtl8723au/core/rtw_cmd.c >> b/drivers/staging/rtl8723au/core/rtw_cmd.c >> index 4eaa502..c1f6254 100644 >> --- a/drivers/staging/rtl8723au/core/rtw_cmd.c >> +++ b/drivers/staging/rtl8723au/core/rtw_cmd.c >> @@ -957,7 +957,7 @@ static void traffic_status_watchdog(struct rtw_adapter >> *padapter) >> /* check traffic for powersaving. */ >> if (((pmlmepriv->LinkDetectInfo.NumRxUnicastOkInPeriod + >>pmlmepriv->LinkDetectInfo.NumTxOkInPeriod) > 8) || >> -pmlmepriv->LinkDetectInfo.NumRxUnicastOkInPeriod >2) >> +pmlmepriv->LinkDetectInfo.NumRxUnicastOkInPeriod > >> 2) >> bEnterPS = false; >> else >> bEnterPS = true; > > This makes the line longer than 80 characters, that is worse than the > 'problem' you are fixing. > > Jes > > Hello jes, > > How it can be worse because checkpatch treating this as an Error and > line longer than 80 character is warning reported by checkpatch and I > could see that in entire staging directory, > every maintainer most of the time ignore the 80 column limit and give > priority to Error. > > Please let me know your comment . > > Sanjeev Sharma checkpatch has it's ideas, doesn't mean it's blindly right at all times. In this particular case it keeps the code more readable to keep it below 80 characters than it does to add those two blanks and make the line longer. I agree the long variable names are nasty, and it doesn't help they were done in StUdLyCaPs. If you want to attack it from that front, please go ahead. However, on formatting, please respond with proper email using proper quoting when replying. Thanks, Jes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging:rtl8723au: core: Fix error reported by checkpatch.
Joe Perches writes:
> On Mon, 2014-10-27 at 09:43 +0100, Jes Sorensen wrote:
>> Sanjeev Sharma writes:
>> > This is a patch to the rtw_cmd.c file that fixes
>> > Error reported by checkpatch.
> []
>> > diff --git a/drivers/staging/rtl8723au/core/rtw_cmd.c
>> > b/drivers/staging/rtl8723au/core/rtw_cmd.c
> []
>> > @@ -957,7 +957,7 @@ static void traffic_status_watchdog(struct rtw_adapter
>> > *padapter)
>> >/* check traffic for powersaving. */
>> >if (((pmlmepriv->LinkDetectInfo.NumRxUnicastOkInPeriod +
>> > pmlmepriv->LinkDetectInfo.NumTxOkInPeriod) > 8) ||
>> > - pmlmepriv->LinkDetectInfo.NumRxUnicastOkInPeriod >2)
>> > + pmlmepriv->LinkDetectInfo.NumRxUnicastOkInPeriod >
>> > 2)
>> >bEnterPS = false;
>> >else
>> >bEnterPS = true;
>>
>> This makes the line longer than 80 characters, that is worse than the
>> 'problem' you are fixing.
>
> The code already has dozens of long lines already.
>
> This is generally a problem because the variable names
> are pretty long so strict 80 column adherence generally
> isn't possible.
It does, but that is not a reason to add more. I have generally tried to
trim it down when I cleaned up pieces of it.
Jes
> A possible way to shorten these relatively long variable
> name/line lengths is to use a temporary for
>
> pmlmeprv->LinkDetectInfo
>
> struct rt_link_detect *ldi = &pmlmeprv->LinkDetectInfo;
>
> so:
>
> drivers/staging/rtl8723au/core/rtw_cmd.c | 46
>
> 1 file changed, 23 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/staging/rtl8723au/core/rtw_cmd.c
> b/drivers/staging/rtl8723au/core/rtw_cmd.c
> index d2d7edf..1b24945 100644
> --- a/drivers/staging/rtl8723au/core/rtw_cmd.c
> +++ b/drivers/staging/rtl8723au/core/rtw_cmd.c
> @@ -922,34 +922,33 @@ static void traffic_status_watchdog(struct rtw_adapter
> *padapter)
> u8 bHigherBusyTxTraffic = false;
> struct mlme_priv *pmlmepriv = &padapter->mlmepriv;
> int BusyThreshold = 100;
> + struct rt_link_detect *ldi = &pmlmepriv->LinkDetectInfo;
> +
> /* */
> /* Determine if our traffic is busy now */
> /* */
> if (check_fwstate(pmlmepriv, _FW_LINKED)) {
> if (rtl8723a_BT_coexist(padapter))
> BusyThreshold = 50;
> - else if (pmlmepriv->LinkDetectInfo.bBusyTraffic)
> + else if (ldi->bBusyTraffic)
> BusyThreshold = 75;
> /* if we raise bBusyTraffic in last watchdog, using
> lower threshold. */
> - if (pmlmepriv->LinkDetectInfo.NumRxOkInPeriod > BusyThreshold ||
> - pmlmepriv->LinkDetectInfo.NumTxOkInPeriod > BusyThreshold) {
> + if (ldi->NumRxOkInPeriod > BusyThreshold ||
> + ldi->NumTxOkInPeriod > BusyThreshold) {
> bBusyTraffic = true;
>
> - if (pmlmepriv->LinkDetectInfo.NumRxOkInPeriod >
> - pmlmepriv->LinkDetectInfo.NumTxOkInPeriod)
> + if (ldi->NumRxOkInPeriod > ldi->NumTxOkInPeriod)
> bRxBusyTraffic = true;
> else
> bTxBusyTraffic = true;
> }
>
> /* Higher Tx/Rx data. */
> - if (pmlmepriv->LinkDetectInfo.NumRxOkInPeriod > 4000 ||
> - pmlmepriv->LinkDetectInfo.NumTxOkInPeriod > 4000) {
> + if (ldi->NumRxOkInPeriod > 4000 ||
> + ldi->NumTxOkInPeriod > 4000) {
> bHigherBusyTraffic = true;
> -
> - if (pmlmepriv->LinkDetectInfo.NumRxOkInPeriod >
> - pmlmepriv->LinkDetectInfo.NumTxOkInPeriod)
> + if (ldi->NumRxOkInPeriod > ldi->NumTxOkInPeriod)
> bHigherBusyRxTraffic = true;
> else
> bHigherBusyTxTraffic = true;
> @@ -958,9 +957,9 @@ static void traffic_status_watchdog(struct rtw_adapter
> *padapter)
> if (!rtl8723a_BT_coexist(padapter) ||
> !rtl8723a_BT_using_antenna_1(padapter)) {
> /* check traffic for powersaving. */
> - if (((pmlmepriv->
Re: [PATCH] staging:rtl8723au: core: Fix error reported by checkpatch.
Sanjeev Sharma writes: > This is a patch to the rtw_cmd.c file that fixes > Error reported by checkpatch. > > Signed-off-by: Sanjeev Sharma > --- > drivers/staging/rtl8723au/core/rtw_cmd.c | 83 > +++- > 1 file changed, 40 insertions(+), 43 deletions(-) > > diff --git a/drivers/staging/rtl8723au/core/rtw_cmd.c > b/drivers/staging/rtl8723au/core/rtw_cmd.c > index 4eaa502..c1f6254 100644 > --- a/drivers/staging/rtl8723au/core/rtw_cmd.c > +++ b/drivers/staging/rtl8723au/core/rtw_cmd.c > @@ -957,7 +957,7 @@ static void traffic_status_watchdog(struct rtw_adapter > *padapter) > /* check traffic for powersaving. */ > if (((pmlmepriv->LinkDetectInfo.NumRxUnicastOkInPeriod + > pmlmepriv->LinkDetectInfo.NumTxOkInPeriod) > 8) || > - pmlmepriv->LinkDetectInfo.NumRxUnicastOkInPeriod >2) > + pmlmepriv->LinkDetectInfo.NumRxUnicastOkInPeriod > > 2) > bEnterPS = false; > else > bEnterPS = true; This makes the line longer than 80 characters, that is worse than the 'problem' you are fixing. Jes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] staging: rtl8723au:core
Joe Perches writes:
> On Mon, 2014-10-27 at 07:16 +0100, Jes Sorensen wrote:
>> Joe Perches writes:
>> > On Mon, 2014-10-27 at 06:45 +0100, Jes Sorensen wrote:
>> >> Joe Perches writes:
>> >> > On Sun, 2014-10-26 at 16:18 +, Paul McQuade wrote:
>> >> >> ERROR: spaces required around that ':' (ctx:VxE)
>> > []
>> >> >> diff --git a/drivers/staging/rtl8723au/core/rtw_ieee80211.c
>> >> >> b/drivers/staging/rtl8723au/core/rtw_ieee80211.c
>> > []
>> >> >> @@ -794,28 +794,28 @@ u16 rtw_mcs_rate23a(u8 rf_type, u8 bw_40MHz, u8
>> >> >> short_GI_20, u8 short_GI_40,
>> >> >>
>> >> >>if (rf_type == RF_1T1R) {
>> >> >>if (mcs->rx_mask[0] & BIT(7))
>> >> >> - max_rate = (bw_40MHz) ?
>> >> >> ((short_GI_40)?1500:1350):
>> >> >> + max_rate = (bw_40MHz) ?
>> >> >> ((short_GI_40)?1500:1350) :
>> > []
>> >> >
>> >> > A macro could help intelligibility here - maybe something like:
>> >> >
>> >> > #define get_max_rate(r1, r2, r3, r4)
>> >> > \
>> >> > (bw_40MHz ? (short_GI_40 ? r1 : r2) : (short_GI_20 ? r3 : r4))
>> >> > and:
>> >> > if (mcs->rx_mask[0] & BIT(7))
>> >> > max_rate = get_max_rate(1500, 1350, 722, 650);
>> >>
>> >> Hiding a parameter to a macro like that is bad coding practice, so don't
>> >> do that please!
>> >
>> > Yes and no.
>> >
>> > Adding the other 3 arguments to the macro doesn't help legibility.
>> >
>> > Keeping the macro definition local to the place that it's used
>> > can help avoid typos.
>>
>> It's wrong, so just don't do it here!
>
> It's not "wrong", you just don't like it.
I consider it wrong, and I am the co-maintainer of this driver and
I NACK it.
There's a gazillion real issues to fix in this driver, adding a bad
macro isn't on that list.
Jes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] staging: rtl8723au:core
Joe Perches writes:
> On Mon, 2014-10-27 at 06:45 +0100, Jes Sorensen wrote:
>> Joe Perches writes:
>> > On Sun, 2014-10-26 at 16:18 +, Paul McQuade wrote:
>> >> ERROR: spaces required around that ':' (ctx:VxE)
> []
>> >> diff --git a/drivers/staging/rtl8723au/core/rtw_ieee80211.c
>> >> b/drivers/staging/rtl8723au/core/rtw_ieee80211.c
> []
>> >> @@ -794,28 +794,28 @@ u16 rtw_mcs_rate23a(u8 rf_type, u8 bw_40MHz, u8
>> >> short_GI_20, u8 short_GI_40,
>> >>
>> >> if (rf_type == RF_1T1R) {
>> >> if (mcs->rx_mask[0] & BIT(7))
>> >> - max_rate = (bw_40MHz) ? ((short_GI_40)?1500:1350):
>> >> + max_rate = (bw_40MHz) ? ((short_GI_40)?1500:1350) :
> []
>> >
>> > A macro could help intelligibility here - maybe something like:
>> >
>> > #define get_max_rate(r1, r2, r3, r4)
>> > \
>> >(bw_40MHz ? (short_GI_40 ? r1 : r2) : (short_GI_20 ? r3 : r4))
>> > and:
>> >if (mcs->rx_mask[0] & BIT(7))
>> >max_rate = get_max_rate(1500, 1350, 722, 650);
>>
>> Hiding a parameter to a macro like that is bad coding practice, so don't
>> do that please!
>
> Yes and no.
>
> Adding the other 3 arguments to the macro doesn't help legibility.
>
> Keeping the macro definition local to the place that it's used
> can help avoid typos.
It's wrong, so just don't do it here!
Thank you,
Jes
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] staging: rtl8723au: create macro get_max_rate
Paul McQuade writes: > create marco for max_rate values > > Signed-off-by: Paul McQuade > --- > drivers/staging/rtl8723au/core/rtw_ieee80211.c | 58 > -- > 1 file changed, 26 insertions(+), 32 deletions(-) > > diff --git a/drivers/staging/rtl8723au/core/rtw_ieee80211.c > b/drivers/staging/rtl8723au/core/rtw_ieee80211.c > index 6274cb3..4c0414d 100644 > --- a/drivers/staging/rtl8723au/core/rtw_ieee80211.c > +++ b/drivers/staging/rtl8723au/core/rtw_ieee80211.c > @@ -13,6 +13,8 @@ > * > > **/ > #define _IEEE80211_C > +#define get_max_rate(r1, r2, r3, r4) \ > + (bw_40MHz ? (short_GI_40 ? r1 : r2) : (short_GI_20 ? r3 : r4)) NACK! Don't hide parameters like that! Jes -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
