Re: [PATCH] Staging: rtl8712: Addressed checkpatch.pl issues related to macro parameter wrapping in parentheses.
On Mon, Jun 15, 2020 at 10:28:51AM +0100, Ricardo Ferreira wrote:
> On Sun, 14 Jun 2020 at 15:05, Greg Kroah-Hartman
> wrote:
> >
> > On Sun, Jun 14, 2020 at 02:51:25PM +0100, Ricardo Ferreira wrote:
> > > #define init_h2fwcmd_w_parm_no_rsp(pcmd, pparm, code) \
> > > do {\
> > > - INIT_LIST_HEAD(&pcmd->list);\
> > > - pcmd->cmdcode = code;\
> > > - pcmd->parmbuf = (u8 *)(pparm);\
> > > - pcmd->cmdsz = sizeof(*pparm);\
> > > - pcmd->rsp = NULL;\
> > > - pcmd->rspsz = 0;\
> > > + INIT_LIST_HEAD(&(pcmd)->list);\
> > > + (pcmd)->cmdcode = code;\
> > > + (pcmd)->parmbuf = (u8 *)((pparm));\
> > > + (pcmd)->cmdsz = sizeof(*(pparm));\
> > > + (pcmd)->rsp = NULL;\
> > > + (pcmd)->rspsz = 0;\
> > > } while (0)
> >
> > Does that change really make any sense? checkpatch is a nice hint,
> > sometimes it is not correct...
>
> (Replying again since I mistakenly sent my comments only to Greg...)
>
> Yeah I was over-eager and applied some of checkpatche's patches
> without thinking twice... I guess the parenthesis wrapping only makes
> sense when you have an operator (either binary or unary). I've
> rechecked each macro identified by checkpatch to see if there is a
> need for parenthesis wrapping in their current usage.
Yes, please do that, and also test-build your patches. Sending patches
that break the build are a sure way to make maintainers grumpy :)
thanks,
greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: rtl8712: Addressed checkpatch.pl issues related to macro parameter wrapping in parentheses.
On Sun, 14 Jun 2020 at 15:05, Greg Kroah-Hartman
wrote:
>
> On Sun, Jun 14, 2020 at 02:51:25PM +0100, Ricardo Ferreira wrote:
> > #define init_h2fwcmd_w_parm_no_rsp(pcmd, pparm, code) \
> > do {\
> > - INIT_LIST_HEAD(&pcmd->list);\
> > - pcmd->cmdcode = code;\
> > - pcmd->parmbuf = (u8 *)(pparm);\
> > - pcmd->cmdsz = sizeof(*pparm);\
> > - pcmd->rsp = NULL;\
> > - pcmd->rspsz = 0;\
> > + INIT_LIST_HEAD(&(pcmd)->list);\
> > + (pcmd)->cmdcode = code;\
> > + (pcmd)->parmbuf = (u8 *)((pparm));\
> > + (pcmd)->cmdsz = sizeof(*(pparm));\
> > + (pcmd)->rsp = NULL;\
> > + (pcmd)->rspsz = 0;\
> > } while (0)
>
> Does that change really make any sense? checkpatch is a nice hint,
> sometimes it is not correct...
(Replying again since I mistakenly sent my comments only to Greg...)
Yeah I was over-eager and applied some of checkpatche's patches
without thinking twice... I guess the parenthesis wrapping only makes
sense when you have an operator (either binary or unary). I've
rechecked each macro identified by checkpatch to see if there is a
need for parenthesis wrapping in their current usage.
Regards,
Ricardo Ferreira.
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] Staging: rtl8712: Addressed checkpatch.pl issues related to macro parameter wrapping in parentheses.
Hi Ricardo, Thank you for the patch! Yet something to improve: [auto build test ERROR on staging/staging-testing] url: https://github.com/0day-ci/linux/commits/Ricardo-Ferreira/Staging-rtl8712-Addressed-checkpatch-pl-issues-related-to-macro-parameter-wrapping-in-parentheses/20200614-215316 base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git af7b4801030c07637840191c69eb666917e4135d config: x86_64-rhel-7.6 (attached as .config) compiler: gcc-9 (Debian 9.3.0-13) 9.3.0 reproduce (this is a W=1 build): # save the attached .config to linux build tree make W=1 ARCH=x86_64 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot All error/warnings (new ones prefixed by >>, old ones prefixed by <<): In file included from drivers/staging/rtl8712/rtl871x_cmd.h:22, from drivers/staging/rtl8712/drv_types.h:47, from drivers/staging/rtl8712/hal_init.c:26: drivers/staging/rtl8712/ieee80211.h:566:1: warning: alignment 1 of 'struct ieee80211_authentication' is less than 2 [-Wpacked-not-aligned] 566 | } __packed; | ^ drivers/staging/rtl8712/ieee80211.h:574:1: warning: alignment 1 of 'struct ieee80211_probe_response' is less than 2 [-Wpacked-not-aligned] 574 | } __packed; | ^ drivers/staging/rtl8712/ieee80211.h:578:1: warning: alignment 1 of 'struct ieee80211_probe_request' is less than 2 [-Wpacked-not-aligned] 578 | } __packed; | ^ drivers/staging/rtl8712/ieee80211.h:585:1: warning: alignment 1 of 'struct ieee80211_assoc_request_frame' is less than 2 [-Wpacked-not-aligned] 585 | } __packed; | ^ drivers/staging/rtl8712/ieee80211.h:592:1: warning: alignment 1 of 'struct ieee80211_assoc_response_frame' is less than 2 [-Wpacked-not-aligned] 592 | } __packed; | ^ In file included from drivers/staging/rtl8712/osdep_service.h:31, from drivers/staging/rtl8712/hal_init.c:25: drivers/staging/rtl8712/hal_init.c: In function 'chk_fwhdr': >> drivers/staging/rtl8712/basic_types.h:24:49: error: expected expression >> before ')' token 24 | #define FIELD_OFFSET(s, field) ((addr_t)&(((s) *)(0))->(field)) | ^ >> drivers/staging/rtl8712/hal_init.c:136:12: note: in expansion of macro >> 'FIELD_OFFSET' 136 | fwhdrsz = FIELD_OFFSET(struct fw_hdr, fwpriv) + pfwhdr->fw_priv_sz; |^~~~ drivers/staging/rtl8712/hal_init.c: In function 'rtl8712_dl_fw': >> drivers/staging/rtl8712/basic_types.h:24:49: error: expected expression >> before ')' token 24 | #define FIELD_OFFSET(s, field) ((addr_t)&(((s) *)(0))->(field)) | ^ drivers/staging/rtl8712/hal_init.c:176:26: note: in expansion of macro 'FIELD_OFFSET' 176 | ptr = (u8 *)mappedfw + FIELD_OFFSET(struct fw_hdr, fwpriv) + | ^~~~ -- In file included from drivers/staging/rtl8712/rtl871x_cmd.h:22, from drivers/staging/rtl8712/drv_types.h:47, from drivers/staging/rtl8712/rtl871x_ioctl_linux.c:21: drivers/staging/rtl8712/ieee80211.h:566:1: warning: alignment 1 of 'struct ieee80211_authentication' is less than 2 [-Wpacked-not-aligned] 566 | } __packed; | ^ drivers/staging/rtl8712/ieee80211.h:574:1: warning: alignment 1 of 'struct ieee80211_probe_response' is less than 2 [-Wpacked-not-aligned] 574 | } __packed; | ^ drivers/staging/rtl8712/ieee80211.h:578:1: warning: alignment 1 of 'struct ieee80211_probe_request' is less than 2 [-Wpacked-not-aligned] 578 | } __packed; | ^ drivers/staging/rtl8712/ieee80211.h:585:1: warning: alignment 1 of 'struct ieee80211_assoc_request_frame' is less than 2 [-Wpacked-not-aligned] 585 | } __packed; | ^ drivers/staging/rtl8712/ieee80211.h:592:1: warning: alignment 1 of 'struct ieee80211_assoc_response_frame' is less than 2 [-Wpacked-not-aligned] 592 | } __packed; | ^ In file included from drivers/staging/rtl8712/osdep_service.h:31, from drivers/staging/rtl8712/rtl871x_ioctl_linux.c:20: drivers/staging/rtl8712/rtl871x_ioctl_linux.c: In function 'wpa_set_encryption': >> drivers/staging/rtl8712/basic_types.h:24:49: error: expected expression >> before ')' token 24 | #define FIELD_OFFSET(s, field) ((addr_t)&(((s) *)(0))->(field)) | ^ >> drivers/staging/rtl8712/rtl871x_ioctl_linux.c:413:4: note: in expansion of >> macro 'FIELD_OFFSET' 413 |FIELD_OFFSET(struct NDIS_802_11_WEP, KeyMaterial); |^~~~ drivers/staging/rtl8712/rtl871x_ioctl_linux.c: In function 'r8711_wx_set_enc': >> drivers/staging/rtl8712/basic_types.h:24:49: error: expected expression >> before ')' token 24 | #define FIELD_OFFSET(s, field) ((addr_t)&(((s) *)(0))->(field)) | ^ drivers/staging/rtl8712/rtl871x_ioctl_linux.c:1561:9: note: in expansion of macro 'FIELD_OFFSET' 1561 | FIELD_OFFSET(struct NDIS_802_11_WEP, KeyMaterial); | ^~~~ In file included from drivers/staging/rtl8712/rtl871x_ioctl_linux.c:28: At top level: drivers/staging/rtl871
Re: [PATCH] Staging: rtl8712: Addressed checkpatch.pl issues related to macro parameter wrapping in parentheses.
Hi Ricardo, Thank you for the patch! Yet something to improve: [auto build test ERROR on staging/staging-testing] url: https://github.com/0day-ci/linux/commits/Ricardo-Ferreira/Staging-rtl8712-Addressed-checkpatch-pl-issues-related-to-macro-parameter-wrapping-in-parentheses/20200614-215316 base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git af7b4801030c07637840191c69eb666917e4135d config: x86_64-rhel (attached as .config) compiler: gcc-9 (Debian 9.3.0-13) 9.3.0 reproduce (this is a W=1 build): # save the attached .config to linux build tree make W=1 ARCH=x86_64 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot All errors (new ones prefixed by >>, old ones prefixed by <<): In file included from drivers/staging/rtl8712/rtl871x_cmd.h:22, from drivers/staging/rtl8712/drv_types.h:47, from drivers/staging/rtl8712/hal_init.c:26: drivers/staging/rtl8712/ieee80211.h:566:1: warning: alignment 1 of 'struct ieee80211_authentication' is less than 2 [-Wpacked-not-aligned] 566 | } __packed; | ^ drivers/staging/rtl8712/ieee80211.h:574:1: warning: alignment 1 of 'struct ieee80211_probe_response' is less than 2 [-Wpacked-not-aligned] 574 | } __packed; | ^ drivers/staging/rtl8712/ieee80211.h:578:1: warning: alignment 1 of 'struct ieee80211_probe_request' is less than 2 [-Wpacked-not-aligned] 578 | } __packed; | ^ drivers/staging/rtl8712/ieee80211.h:585:1: warning: alignment 1 of 'struct ieee80211_assoc_request_frame' is less than 2 [-Wpacked-not-aligned] 585 | } __packed; | ^ drivers/staging/rtl8712/ieee80211.h:592:1: warning: alignment 1 of 'struct ieee80211_assoc_response_frame' is less than 2 [-Wpacked-not-aligned] 592 | } __packed; | ^ In file included from drivers/staging/rtl8712/osdep_service.h:31, from drivers/staging/rtl8712/hal_init.c:25: drivers/staging/rtl8712/hal_init.c: In function 'chk_fwhdr': >> drivers/staging/rtl8712/basic_types.h:24:49: error: expected expression >> before ')' token 24 | #define FIELD_OFFSET(s, field) ((addr_t)&(((s) *)(0))->(field)) | ^ drivers/staging/rtl8712/hal_init.c:136:12: note: in expansion of macro 'FIELD_OFFSET' 136 | fwhdrsz = FIELD_OFFSET(struct fw_hdr, fwpriv) + pfwhdr->fw_priv_sz; |^~~~ drivers/staging/rtl8712/hal_init.c: In function 'rtl8712_dl_fw': >> drivers/staging/rtl8712/basic_types.h:24:49: error: expected expression >> before ')' token 24 | #define FIELD_OFFSET(s, field) ((addr_t)&(((s) *)(0))->(field)) | ^ drivers/staging/rtl8712/hal_init.c:176:26: note: in expansion of macro 'FIELD_OFFSET' 176 | ptr = (u8 *)mappedfw + FIELD_OFFSET(struct fw_hdr, fwpriv) + | ^~~~ -- In file included from drivers/staging/rtl8712/rtl871x_cmd.h:22, from drivers/staging/rtl8712/drv_types.h:47, from drivers/staging/rtl8712/rtl871x_ioctl_linux.c:21: drivers/staging/rtl8712/ieee80211.h:566:1: warning: alignment 1 of 'struct ieee80211_authentication' is less than 2 [-Wpacked-not-aligned] 566 | } __packed; | ^ drivers/staging/rtl8712/ieee80211.h:574:1: warning: alignment 1 of 'struct ieee80211_probe_response' is less than 2 [-Wpacked-not-aligned] 574 | } __packed; | ^ drivers/staging/rtl8712/ieee80211.h:578:1: warning: alignment 1 of 'struct ieee80211_probe_request' is less than 2 [-Wpacked-not-aligned] 578 | } __packed; | ^ drivers/staging/rtl8712/ieee80211.h:585:1: warning: alignment 1 of 'struct ieee80211_assoc_request_frame' is less than 2 [-Wpacked-not-aligned] 585 | } __packed; | ^ drivers/staging/rtl8712/ieee80211.h:592:1: warning: alignment 1 of 'struct ieee80211_assoc_response_frame' is less than 2 [-Wpacked-not-aligned] 592 | } __packed; | ^ In file included from drivers/staging/rtl8712/osdep_service.h:31, from drivers/staging/rtl8712/rtl871x_ioctl_linux.c:20: drivers/staging/rtl8712/rtl871x_ioctl_linux.c: In function 'wpa_set_encryption': >> drivers/staging/rtl8712/basic_types.h:24:49: error: expected expression >> before ')' token 24 | #define FIELD_OFFSET(s, field) ((addr_t)&(((s) *)(0))->(field)) | ^ drivers/staging/rtl8712/rtl871x_ioctl_linux.c:413:4: note: in expansion of macro 'FIELD_OFFSET' 413 |FIELD_OFFSET(struct NDIS_802_11_WEP, KeyMaterial); |^~~~ drivers/staging/rtl8712/rtl871x_ioctl_linux.c: In function 'r8711_wx_set_enc': >> drivers/staging/rtl8712/basic_types.h:24:49: error: expected expression >> before ')' token 24 | #define FIELD_OFFSET(s, field) ((addr_t)&(((s) *)(0))->(field)) | ^ drivers/staging/rtl8712/rtl871x_ioctl_linux.c:1561:9: note: in expansion of macro 'FIELD_OFFSET' 1561 | FIELD_OFFSET(struct NDIS_802_11_WEP, KeyMaterial); | ^~~~ In file included from drivers/staging/rtl8712/rtl871x_ioctl_linux.c:28: At top level: drivers/staging/rtl8712/rtl871x_mp_ioctl.h:256
Re: [PATCH] Staging: rtl8712: Addressed checkpatch.pl issues related to macro parameter wrapping in parentheses.
On Sun, Jun 14, 2020 at 02:51:25PM +0100, Ricardo Ferreira wrote:
> #define init_h2fwcmd_w_parm_no_rsp(pcmd, pparm, code) \
> do {\
> - INIT_LIST_HEAD(&pcmd->list);\
> - pcmd->cmdcode = code;\
> - pcmd->parmbuf = (u8 *)(pparm);\
> - pcmd->cmdsz = sizeof(*pparm);\
> - pcmd->rsp = NULL;\
> - pcmd->rspsz = 0;\
> + INIT_LIST_HEAD(&(pcmd)->list);\
> + (pcmd)->cmdcode = code;\
> + (pcmd)->parmbuf = (u8 *)((pparm));\
> + (pcmd)->cmdsz = sizeof(*(pparm));\
> + (pcmd)->rsp = NULL;\
> + (pcmd)->rspsz = 0;\
> } while (0)
Does that change really make any sense? checkpatch is a nice hint,
sometimes it is not correct...
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH] Staging: rtl8712: Addressed checkpatch.pl issues related to macro parameter wrapping in parentheses.
Attempting to wet my feet in kernel patch submission by submitting a checkstyle
fix for the rtl8712 driver.
Signed-off-by: Ricardo Ferreira
---
drivers/staging/rtl8712/basic_types.h | 2 +-
drivers/staging/rtl8712/osdep_intf.h | 2 +-
drivers/staging/rtl8712/rtl8712_efuse.h | 8 +--
drivers/staging/rtl8712/rtl8712_recv.h| 4 +-
drivers/staging/rtl8712/rtl871x_cmd.h | 12 ++---
.../staging/rtl8712/rtl871x_mp_phy_regdef.h | 4 +-
drivers/staging/rtl8712/rtl871x_security.h| 8 +--
drivers/staging/rtl8712/rtl871x_xmit.h| 50 +--
drivers/staging/rtl8712/wifi.h| 10 ++--
9 files changed, 50 insertions(+), 50 deletions(-)
diff --git a/drivers/staging/rtl8712/basic_types.h
b/drivers/staging/rtl8712/basic_types.h
index 4ad7f35b1644..3e6d4ff45a75 100644
--- a/drivers/staging/rtl8712/basic_types.h
+++ b/drivers/staging/rtl8712/basic_types.h
@@ -21,7 +21,7 @@
#define SIZE_T __kernel_size_t
#define sint signed int
-#define FIELD_OFFSET(s, field) ((addr_t)&((s *)(0))->field)
+#define FIELD_OFFSET(s, field) ((addr_t)&(((s) *)(0))->(field))
/* Should we extend this to be host_addr_t and target_addr_t for case:
* host : x86_64
diff --git a/drivers/staging/rtl8712/osdep_intf.h
b/drivers/staging/rtl8712/osdep_intf.h
index 2cc25db1a91d..058287fd0f85 100644
--- a/drivers/staging/rtl8712/osdep_intf.h
+++ b/drivers/staging/rtl8712/osdep_intf.h
@@ -17,7 +17,7 @@
#include "osdep_service.h"
#include "drv_types.h"
-#define RND4(x)(((x >> 2) + (((x & 3) == 0) ? 0 : 1)) << 2)
+#define RND4(x)x) >> 2) + x) & 3) == 0) ? 0 : 1)) << 2)
struct intf_priv {
u8 *intf_dev;
diff --git a/drivers/staging/rtl8712/rtl8712_efuse.h
b/drivers/staging/rtl8712/rtl8712_efuse.h
index 4969d307e978..f22993d94508 100644
--- a/drivers/staging/rtl8712/rtl8712_efuse.h
+++ b/drivers/staging/rtl8712/rtl8712_efuse.h
@@ -13,10 +13,10 @@
#define PGPKT_DATA_SIZE8 /* PGPKG_MAX_WORDS*2; BYTES sizeof(u8)*8*/
#define MAX_PGPKT_SIZE 9 /* 1 + PGPKT_DATA_SIZE; header + 2 * 4 words (BYTES)*/
-#define GET_EFUSE_OFFSET(header) ((header & 0xF0) >> 4)
-#define GET_EFUSE_WORD_EN(header) (header & 0x0F)
-#define MAKE_EFUSE_HEADER(offset, word_en) (((offset & 0x0F) << 4) | \
- (word_en & 0x0F))
+#define GET_EFUSE_OFFSET(header) (((header) & 0xF0) >> 4)
+#define GET_EFUSE_WORD_EN(header) ((header) & 0x0F)
+#define MAKE_EFUSE_HEADER(offset, word_en) offset) & 0x0F) << 4) | \
+ ((word_en) & 0x0F))
/*--*/
struct PGPKT_STRUCT {
u8 offset;
diff --git a/drivers/staging/rtl8712/rtl8712_recv.h
b/drivers/staging/rtl8712/rtl8712_recv.h
index 3e385b2242d8..cdce24efaeda 100644
--- a/drivers/staging/rtl8712/rtl8712_recv.h
+++ b/drivers/staging/rtl8712/rtl8712_recv.h
@@ -33,8 +33,8 @@
#define RECVBUFF_ALIGN_SZ 512
#define RSVD_ROOM_SZ (0)
/*These definition is used for Rx packet reordering.*/
-#define SN_LESS(a, b) (((a-b) & 0x800) != 0)
-#define SN_EQUAL(a, b) (a == b)
+#define SN_LESS(a, b) a)-(b)) & 0x800) != 0)
+#define SN_EQUAL(a, b) ((a) == (b))
#define REORDER_WAIT_TIME 30 /* (ms)*/
struct recv_stat {
diff --git a/drivers/staging/rtl8712/rtl871x_cmd.h
b/drivers/staging/rtl8712/rtl871x_cmd.h
index 254182a6ce8e..28b855091f23 100644
--- a/drivers/staging/rtl8712/rtl871x_cmd.h
+++ b/drivers/staging/rtl8712/rtl871x_cmd.h
@@ -71,12 +71,12 @@ struct evt_priv {
#define init_h2fwcmd_w_parm_no_rsp(pcmd, pparm, code) \
do {\
- INIT_LIST_HEAD(&pcmd->list);\
- pcmd->cmdcode = code;\
- pcmd->parmbuf = (u8 *)(pparm);\
- pcmd->cmdsz = sizeof(*pparm);\
- pcmd->rsp = NULL;\
- pcmd->rspsz = 0;\
+ INIT_LIST_HEAD(&(pcmd)->list);\
+ (pcmd)->cmdcode = code;\
+ (pcmd)->parmbuf = (u8 *)((pparm));\
+ (pcmd)->cmdsz = sizeof(*(pparm));\
+ (pcmd)->rsp = NULL;\
+ (pcmd)->rspsz = 0;\
} while (0)
void r8712_enqueue_cmd(struct cmd_priv *pcmdpriv, struct cmd_obj *obj);
diff --git a/drivers/staging/rtl8712/rtl871x_mp_phy_regdef.h
b/drivers/staging/rtl8712/rtl871x_mp_phy_regdef.h
index ca5072e11e22..c1c4a06f5342 100644
--- a/drivers/staging/rtl8712/rtl871x_mp_phy_regdef.h
+++ b/drivers/staging/rtl8712/rtl871x_mp_phy_regdef.h
@@ -427,8 +427,8 @@
#definebCCKTxCRC16 0x
#definebCCKTxStatus0x1
#definebOFDMTxStatus 0x2
-#define IS_BB_REG_OFFSET_92S(_Offset) ((_Offset >= 0x800) && \
- (_Offset <= 0xfff))
+#define IS_BB_REG_OFFSET_92S(_Offset) (((_Offset) >= 0x800) && \
+ ((_Offset) <= 0xfff))
/* 2. Page8(0x800) */
#definebRFMOD 0x1 /* Reg 0x800 rFPGA0
