Re: [PATCH] staging: wfx: refactor to avoid duplication at hif_tx.c
Hi Tomer,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on staging/staging-testing]
url:
https://github.com/0day-ci/linux/commits/Tomer-Samara/staging-wfx-refactor-to-avoid-duplication-at-hif_tx-c/20200805-165649
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
5bbd90550da8f7bdac769b5825597e67183c9411
config: m68k-allmodconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross
ARCH=m68k
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All warnings (new ones prefixed by >>):
In file included from arch/m68k/include/asm/io_mm.h:25,
from arch/m68k/include/asm/io.h:8,
from include/linux/scatterlist.h:9,
from include/linux/dma-mapping.h:11,
from include/linux/skbuff.h:31,
from include/linux/if_ether.h:19,
from include/linux/etherdevice.h:20,
from drivers/staging/wfx/hif_tx.c:9:
arch/m68k/include/asm/raw_io.h: In function 'raw_rom_outsb':
arch/m68k/include/asm/raw_io.h:83:7: warning: variable '__w' set but not
used [-Wunused-but-set-variable]
83 | ({u8 __w, __v = (b); u32 _addr = ((u32) (addr)); \
| ^~~
arch/m68k/include/asm/raw_io.h:430:3: note: in expansion of macro 'rom_out_8'
430 | rom_out_8(port, *buf++);
| ^
arch/m68k/include/asm/raw_io.h: In function 'raw_rom_outsw':
arch/m68k/include/asm/raw_io.h:86:8: warning: variable '__w' set but not
used [-Wunused-but-set-variable]
86 | ({u16 __w, __v = (w); u32 _addr = ((u32) (addr)); \
|^~~
arch/m68k/include/asm/raw_io.h:448:3: note: in expansion of macro
'rom_out_be16'
448 | rom_out_be16(port, *buf++);
| ^~~~
arch/m68k/include/asm/raw_io.h: In function 'raw_rom_outsw_swapw':
arch/m68k/include/asm/raw_io.h:90:8: warning: variable '__w' set but not
used [-Wunused-but-set-variable]
90 | ({u16 __w, __v = (w); u32 _addr = ((u32) (addr)); \
|^~~
arch/m68k/include/asm/raw_io.h:466:3: note: in expansion of macro
'rom_out_le16'
466 | rom_out_le16(port, *buf++);
| ^~~~
In file included from include/linux/kernel.h:11,
from include/linux/skbuff.h:13,
from include/linux/if_ether.h:19,
from include/linux/etherdevice.h:20,
from drivers/staging/wfx/hif_tx.c:9:
include/linux/scatterlist.h: In function 'sg_set_buf':
arch/m68k/include/asm/page_mm.h:169:49: warning: ordered comparison of
pointer with null pointer [-Wextra]
169 | #define virt_addr_valid(kaddr) ((void *)(kaddr) >= (void
*)PAGE_OFFSET && (void *)(kaddr) < high_memory)
| ^~
include/linux/compiler.h:78:42: note: in definition of macro 'unlikely'
78 | # define unlikely(x) __builtin_expect(!!(x), 0)
| ^
include/linux/scatterlist.h:143:2: note: in expansion of macro 'BUG_ON'
143 | BUG_ON(!virt_addr_valid(buf));
| ^~
include/linux/scatterlist.h:143:10: note: in expansion of macro
'virt_addr_valid'
143 | BUG_ON(!virt_addr_valid(buf));
| ^~~
In file included from arch/m68k/include/asm/bug.h:32,
from include/linux/bug.h:5,
from include/linux/thread_info.h:12,
from include/asm-generic/preempt.h:5,
from ./arch/m68k/include/generated/asm/preempt.h:1,
from include/linux/preempt.h:78,
from include/linux/spinlock.h:51,
from include/linux/seqlock.h:36,
from include/linux/time.h:6,
from include/linux/skbuff.h:15,
from include/linux/if_ether.h:19,
from include/linux/etherdevice.h:20,
from drivers/staging/wfx/hif_tx.c:9:
include/linux/dma-mapping.h: In function 'dma_map_resource':
arch/m68k/include/asm/page_mm.h:169:49: warning: ordered comparison of
pointer with null pointer [-Wextra]
169 | #define virt_addr_valid(kaddr) ((void *)(kaddr) >= (void
*)PAGE_OFFSET && (void *)(kaddr) < high_memory)
| ^~
include/asm-generic/bug.h:144:27: note: in definition of macro 'WARN_ON_ONCE'
144 | int __ret_warn_once = !!(condition); \
| ^
arch/m68k/include/asm/page_mm.h:170:25:
Re: [PATCH] staging: wfx: refactor to avoid duplication at hif_tx.c
On Wed, Aug 05, 2020 at 11:04:25AM +0200, Greg KH wrote: > On Wed, Aug 05, 2020 at 11:56:08AM +0300, Tomer Samara wrote: > > Add functions wfx_full_send(), wfx_full_send_no_reply_async(), > > wfx_full_send_no_reply() and wfx_full_send_no_reply_free() > > which works as follow: > > wfx_full_send() - simple wrapper for both wfx_fill_header() > > and wfx_cmd_send(). > > wfx_full_send_no_reply_async() - wrapper for both but with > > NULL as reply and size zero. > > wfx_full_send_no_reply() - same as wfx_full_send_no_reply_async() > >but with false async value > > wfx_full_send_no_reply_free() - same as wfx_full_send_no_reply() > > but also free the struct hif_msg. > > Please only do one-thing-per-patch. Why shouldn't this be a 4 patch > series? > > thanks, > > greg k-h All of the 4 functions are wrappers for the same duplicate code when every time there are different flags, so they are all connected, it is feel to me more legit to patch them all together, should I split them into 4 different patches? Thanks, Tomer Samara
Re: [PATCH] staging: wfx: refactor to avoid duplication at hif_tx.c
On Wed, Aug 05, 2020 at 11:56:08AM +0300, Tomer Samara wrote: > Add functions wfx_full_send(), wfx_full_send_no_reply_async(), > wfx_full_send_no_reply() and wfx_full_send_no_reply_free() > which works as follow: > wfx_full_send() - simple wrapper for both wfx_fill_header() > and wfx_cmd_send(). > wfx_full_send_no_reply_async() - wrapper for both but with > NULL as reply and size zero. > wfx_full_send_no_reply() - same as wfx_full_send_no_reply_async() >but with false async value > wfx_full_send_no_reply_free() - same as wfx_full_send_no_reply() > but also free the struct hif_msg. Please only do one-thing-per-patch. Why shouldn't this be a 4 patch series? thanks, greg k-h
[PATCH] staging: wfx: refactor to avoid duplication at hif_tx.c
Add functions wfx_full_send(), wfx_full_send_no_reply_async(),
wfx_full_send_no_reply() and wfx_full_send_no_reply_free()
which works as follow:
wfx_full_send() - simple wrapper for both wfx_fill_header()
and wfx_cmd_send().
wfx_full_send_no_reply_async() - wrapper for both but with
NULL as reply and size zero.
wfx_full_send_no_reply() - same as wfx_full_send_no_reply_async()
but with false async value
wfx_full_send_no_reply_free() - same as wfx_full_send_no_reply()
but also free the struct hif_msg.
Signed-off-by: Tomer Samara
---
drivers/staging/wfx/hif_tx.c | 179 ---
1 file changed, 79 insertions(+), 100 deletions(-)
diff --git a/drivers/staging/wfx/hif_tx.c b/drivers/staging/wfx/hif_tx.c
index 5110f9b93762..1ee84e5d47ef 100644
--- a/drivers/staging/wfx/hif_tx.c
+++ b/drivers/staging/wfx/hif_tx.c
@@ -40,7 +40,7 @@ static void wfx_fill_header(struct hif_msg *hif, int if_id,
static void *wfx_alloc_hif(size_t body_len, struct hif_msg **hif)
{
- *hif = kzalloc(sizeof(struct hif_msg) + body_len, GFP_KERNEL);
+ *hif = kzalloc(sizeof(*hif) + body_len, GFP_KERNEL);
if (*hif)
return (*hif)->body;
else
@@ -123,9 +123,38 @@ int wfx_cmd_send(struct wfx_dev *wdev, struct hif_msg
*request,
return ret;
}
+int wfx_full_send(struct wfx_dev *wdev, struct hif_msg *hif, void *reply,
size_t reply_len,
+ bool async, int if_id, unsigned int cmd, int size)
+{
+ wfx_fill_header(hif, if_id, cmd, size);
+ return wfx_cmd_send(wdev, hif, reply, reply_len, async);
+}
+
+int wfx_full_send_no_reply_async(struct wfx_dev *wdev, struct hif_msg *hif,
int if_id,
+unsigned int cmd, int size, bool async)
+{
+ return wfx_full_send(wdev, hif, NULL, 0, async, if_id, cmd, size);
+}
+
+int wfx_full_send_no_reply(struct wfx_dev *wdev, struct hif_msg *hif, int
if_id,
+ unsigned int cmd, int size)
+{
+ return wfx_full_send_no_reply_async(wdev, hif, if_id, cmd, size, false);
+}
+
+int wfx_full_send_no_reply_free(struct wfx_dev *wdev, struct hif_msg *hif, int
if_id,
+ unsigned int cmd, int size)
+{
+ int ret;
+
+ ret = wfx_full_send_no_reply(wdev, hif, if_id, cmd, size);
+ kfree(hif);
+ return ret;
+}
+
// This function is special. After HIF_REQ_ID_SHUT_DOWN, chip won't reply to
any
// request anymore. We need to slightly hack struct wfx_hif_cmd for that job.
Be
-// carefull to only call this funcion during device unregister.
+// careful to only call this function during device unregister.
int hif_shutdown(struct wfx_dev *wdev)
{
int ret;
@@ -136,8 +165,8 @@ int hif_shutdown(struct wfx_dev *wdev)
wfx_alloc_hif(0, );
if (!hif)
return -ENOMEM;
- wfx_fill_header(hif, -1, HIF_REQ_ID_SHUT_DOWN, 0);
- ret = wfx_cmd_send(wdev, hif, NULL, 0, true);
+ ret = wfx_full_send_no_reply_async(wdev, hif, -1, HIF_REQ_ID_SHUT_DOWN,
+ 0, true);
// After this command, chip won't reply. Be sure to give enough time to
// bh to send buffer:
msleep(100);
@@ -154,7 +183,6 @@ int hif_shutdown(struct wfx_dev *wdev)
int hif_configuration(struct wfx_dev *wdev, const u8 *conf, size_t len)
{
- int ret;
size_t buf_len = sizeof(struct hif_req_configuration) + len;
struct hif_msg *hif;
struct hif_req_configuration *body = wfx_alloc_hif(buf_len, );
@@ -163,25 +191,20 @@ int hif_configuration(struct wfx_dev *wdev, const u8
*conf, size_t len)
return -ENOMEM;
body->length = cpu_to_le16(len);
memcpy(body->pds_data, conf, len);
- wfx_fill_header(hif, -1, HIF_REQ_ID_CONFIGURATION, buf_len);
- ret = wfx_cmd_send(wdev, hif, NULL, 0, false);
- kfree(hif);
- return ret;
+ return wfx_full_send_no_reply_free(wdev, hif, -1,
HIF_REQ_ID_CONFIGURATION,
+ buf_len);
}
int hif_reset(struct wfx_vif *wvif, bool reset_stat)
{
- int ret;
struct hif_msg *hif;
struct hif_req_reset *body = wfx_alloc_hif(sizeof(*body), );
if (!hif)
return -ENOMEM;
body->reset_flags.reset_stat = reset_stat;
- wfx_fill_header(hif, wvif->id, HIF_REQ_ID_RESET, sizeof(*body));
- ret = wfx_cmd_send(wvif->wdev, hif, NULL, 0, false);
- kfree(hif);
- return ret;
+ return wfx_full_send_no_reply_free(wvif->wdev, hif, wvif->id,
+ HIF_REQ_ID_RESET, sizeof(*body));
}
int hif_read_mib(struct wfx_dev *wdev, int vif_id, u16 mib_id,
@@ -198,9 +221,8 @@ int hif_read_mib(struct wfx_dev *wdev, int vif_id, u16
mib_id,
goto out;
}
body->mib_id = cpu_to_le16(mib_id);
-
