Re: [PATCH v1 1/5] staging: wfx: fix warnings of no space is necessary
On Sun, Oct 20, 2019 at 12:36:50PM -0700, Joe Perches wrote: > > It's sort of tricky to know what "one thing per patch means". > > It seems somewhat arbitrary and based on Greg's understanding > of the experience of the patch submitter and also the language > of the potential commit message. Of course the language of the commit message matters. You have to "sell" your commit and convince us why we should apply it. That's a life lesson right there... :P regards, dan carpenter
Re: [PATCH v1 1/5] staging: wfx: fix warnings of no space is necessary
On Mon, 2019-10-21 at 08:52 +0200, Julia Lawall wrote: > > btw2: > > > > I really dislike all the code inconsistencies and > > unnecessary code duplication with miscellaneous changes > > in the rtl staging drivers > > > > Horrid stuff. > > I'm not sure what you mean by "miscellaneous changes". Do you mean that > all issues should be fixed for one file before moving on to another one? > > Or that there are code clones, and all of the clones should be updated at > the same time? Neither really. But realtek drivers are basically code clones where realtek should prefer to have a single library used for multiple drivers. And staging is basically a dumping ground for realtek wireless drivers. https://lkml.org/lkml/2019/5/15/1405
Re: [PATCH v1 1/5] staging: wfx: fix warnings of no space is necessary
> btw2: > > I really dislike all the code inconsistencies and > unnecessary code duplication with miscellaneous changes > in the rtl staging drivers > > Horrid stuff. I'm not sure what you mean by "miscellaneous changes". Do you mean that all issues should be fixed for one file before moving on to another one? Or that there are code clones, and all of the clones should be updated at the same time? julia
Re: [Outreachy kernel] Re: [PATCH v1 1/5] staging: wfx: fix warnings of no space is necessary
On Sun, 20 Oct 2019, Joe Perches wrote: > On Sun, 2019-10-20 at 21:52 +0200, Julia Lawall wrote: > > On Sun, 20 Oct 2019, Joe Perches wrote: > [] > > > There's probably a generic cocci mechanism to check function > > > prototypes and then remove uses of unnecessary void pointer casts > > > in function calls. I'm not going to try to figure out that syntax. > > > > With the --recursive-includes option, perhaps: > > > > @r@ > > identifier f; > > parameter list[n] ps; > > type T; > > identifier i; > > @@ > > > > T f(ps, void *i, ...); > > > > @@ > > expression e; > > identifier r.f; > > expression list[r.n] es; > > @@ > > > > f(es, > > - (void *)(e) > > + e > > ,...) > > > > This of course only works for functions that have prototypes, and not for > > macros. It will also run slowly. > > You are not kidding about slow, but it doesn't seem to work > for mem, maybe because system includes aren't analyzed. No they are not. > Single file processing time on an XPS13 averages more than > 100 seconds per file. Not surprising. Actually, --include-headers-for-types should provide some benefit. That discards the header files after the type inference. > Also: > > expression e; > > could probably be better as: > > type T; > T *p; Good point. expression *e; would be sufficient. julia > > as some of the expressions cast to void are int or size_t > and it's probably better to restrict the conversions to > just pointer or array types. > > >
Re: [Outreachy kernel] Re: [PATCH v1 1/5] staging: wfx: fix warnings of no space is necessary
On Sun, 2019-10-20 at 21:52 +0200, Julia Lawall wrote: > On Sun, 20 Oct 2019, Joe Perches wrote: [] > > There's probably a generic cocci mechanism to check function > > prototypes and then remove uses of unnecessary void pointer casts > > in function calls. I'm not going to try to figure out that syntax. > > With the --recursive-includes option, perhaps: > > @r@ > identifier f; > parameter list[n] ps; > type T; > identifier i; > @@ > > T f(ps, void *i, ...); > > @@ > expression e; > identifier r.f; > expression list[r.n] es; > @@ > > f(es, > - (void *)(e) > + e > ,...) > > This of course only works for functions that have prototypes, and not for > macros. It will also run slowly. You are not kidding about slow, but it doesn't seem to work for mem, maybe because system includes aren't analyzed. Single file processing time on an XPS13 averages more than 100 seconds per file. Also: expression e; could probably be better as: type T; T *p; as some of the expressions cast to void are int or size_t and it's probably better to restrict the conversions to just pointer or array types.
Re: [Outreachy kernel] Re: [PATCH v1 1/5] staging: wfx: fix warnings of no space is necessary
On Sun, 20 Oct 2019, Joe Perches wrote: > On Sun, 2019-10-20 at 22:17 +0300, Dan Carpenter wrote: > > On Sat, Oct 19, 2019 at 01:02:31PM -0700, Joe Perches wrote: > > > diff -u -p a/rtl8723bs/core/rtw_mlme_ext.c b/rtl8723bs/core/rtw_mlme_ext.c > [] > > > @@ -1132,7 +1132,7 @@ unsigned int OnAuthClient(struct adapter > > > goto authclnt_fail; > > > } > > > > > > - memcpy((void *)(pmlmeinfo->chg_txt), (void *)(p + 2), > > > len); > > > + memcpy((void *)(pmlmeinfo->chg_txt), (p + 2), len); > > > > I wonder why it didn't remove the first void cast? > > drivers/staging/rtl8723bs/include/sta_info.h:151: unsigned char > chg_txt[128]; > > I think the cocci transforms for an array do not match a pointer > and I wrote the cocci script without much care. > > btw; > > There's probably a generic cocci mechanism to check function > prototypes and then remove uses of unnecessary void pointer casts > in function calls. I'm not going to try to figure out that syntax. With the --recursive-includes option, perhaps: @r@ identifier f; parameter list[n] ps; type T; identifier i; @@ T f(ps, void *i, ...); @@ expression e; identifier r.f; expression list[r.n] es; @@ f(es, - (void *)(e) + e ,...) This of course only works for functions that have prototypes, and not for macros. It will also run slowly. julia > > > [ The rest of the email is bonus comments for outreachy developers ]. > > > > And someone needs to check the final patch probably to remove the extra > > parentheses around "(p + 2)". Those were necessary when for the cast > > but not required after the cast is gone. > > > > > pmlmeinfo->auth_seq = 3; > > > issue_auth(padapter, NULL, 0); > > > set_link_timer(pmlmeext, REAUTH_TO); > > > > It's sort of tricky to know what "one thing per patch means". > > It seems somewhat arbitrary and based on Greg's understanding > of the experience of the patch submitter and also the language > of the potential commit message. > > > - memset((void *)(&(pHTInfo->SelfHTCap)), 0, > > + memset((&(pHTInfo->SelfHTCap)), 0, > > sizeof(pHTInfo->SelfHTCap)); > > > > Here the parentheses were never related to the cast so we should leave > > them as is. In other words, in the first example, if we didn't remove > > the cast that would be "half a thing per patch" and in the second > > example that would be "two things in one patch". > > For style patches, it's frequently easier and better to > do all the code transformation at once. > > IMO the last should be: > > memset(>SelfHTCap, 0, sizeof(pHTInfo->SelfHTCap)); > > like it is here: > > drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c:1056: > memset(>SelfHTCap, 0, sizeof(pHTInfo->SelfHTCap)); > > btw2: > > I really dislike all the code inconsistencies and > unnecessary code duplication with miscellaneous changes > in the rtl staging drivers > > Horrid stuff. > > -- > You received this message because you are subscribed to the Google Groups > "outreachy-kernel" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to outreachy-kernel+unsubscr...@googlegroups.com. > To view this discussion on the web visit > https://groups.google.com/d/msgid/outreachy-kernel/6e6bc92cac0858fe5bd37b28f688d3da043f4bef.camel%40perches.com. >
Re: [Outreachy kernel] Re: [PATCH v1 1/5] staging: wfx: fix warnings of no space is necessary
On Sun, 20 Oct 2019, Joe Perches wrote: > On Sun, 2019-10-20 at 22:17 +0300, Dan Carpenter wrote: > > On Sat, Oct 19, 2019 at 01:02:31PM -0700, Joe Perches wrote: > > > diff -u -p a/rtl8723bs/core/rtw_mlme_ext.c b/rtl8723bs/core/rtw_mlme_ext.c > [] > > > @@ -1132,7 +1132,7 @@ unsigned int OnAuthClient(struct adapter > > > goto authclnt_fail; > > > } > > > > > > - memcpy((void *)(pmlmeinfo->chg_txt), (void *)(p + 2), > > > len); > > > + memcpy((void *)(pmlmeinfo->chg_txt), (p + 2), len); > > > > I wonder why it didn't remove the first void cast? > > drivers/staging/rtl8723bs/include/sta_info.h:151: unsigned char > chg_txt[128]; > > I think the cocci transforms for an array do not match a pointer This is also correct. julia > and I wrote the cocci script without much care. > > btw; > > There's probably a generic cocci mechanism to check function > prototypes and then remove uses of unnecessary void pointer casts > in function calls. I'm not going to try to figure out that syntax. > > > [ The rest of the email is bonus comments for outreachy developers ]. > > > > And someone needs to check the final patch probably to remove the extra > > parentheses around "(p + 2)". Those were necessary when for the cast > > but not required after the cast is gone. > > > > > pmlmeinfo->auth_seq = 3; > > > issue_auth(padapter, NULL, 0); > > > set_link_timer(pmlmeext, REAUTH_TO); > > > > It's sort of tricky to know what "one thing per patch means". > > It seems somewhat arbitrary and based on Greg's understanding > of the experience of the patch submitter and also the language > of the potential commit message. > > > - memset((void *)(&(pHTInfo->SelfHTCap)), 0, > > + memset((&(pHTInfo->SelfHTCap)), 0, > > sizeof(pHTInfo->SelfHTCap)); > > > > Here the parentheses were never related to the cast so we should leave > > them as is. In other words, in the first example, if we didn't remove > > the cast that would be "half a thing per patch" and in the second > > example that would be "two things in one patch". > > For style patches, it's frequently easier and better to > do all the code transformation at once. > > IMO the last should be: > > memset(>SelfHTCap, 0, sizeof(pHTInfo->SelfHTCap)); > > like it is here: > > drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c:1056: > memset(>SelfHTCap, 0, sizeof(pHTInfo->SelfHTCap)); > > btw2: > > I really dislike all the code inconsistencies and > unnecessary code duplication with miscellaneous changes > in the rtl staging drivers > > Horrid stuff. > > -- > You received this message because you are subscribed to the Google Groups > "outreachy-kernel" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to outreachy-kernel+unsubscr...@googlegroups.com. > To view this discussion on the web visit > https://groups.google.com/d/msgid/outreachy-kernel/6e6bc92cac0858fe5bd37b28f688d3da043f4bef.camel%40perches.com. >
Re: [PATCH v1 1/5] staging: wfx: fix warnings of no space is necessary
On Sun, 2019-10-20 at 22:17 +0300, Dan Carpenter wrote: > On Sat, Oct 19, 2019 at 01:02:31PM -0700, Joe Perches wrote: > > diff -u -p a/rtl8723bs/core/rtw_mlme_ext.c b/rtl8723bs/core/rtw_mlme_ext.c [] > > @@ -1132,7 +1132,7 @@ unsigned int OnAuthClient(struct adapter > > goto authclnt_fail; > > } > > > > - memcpy((void *)(pmlmeinfo->chg_txt), (void *)(p + 2), > > len); > > + memcpy((void *)(pmlmeinfo->chg_txt), (p + 2), len); > > I wonder why it didn't remove the first void cast? drivers/staging/rtl8723bs/include/sta_info.h:151: unsigned char chg_txt[128]; I think the cocci transforms for an array do not match a pointer and I wrote the cocci script without much care. btw; There's probably a generic cocci mechanism to check function prototypes and then remove uses of unnecessary void pointer casts in function calls. I'm not going to try to figure out that syntax. > [ The rest of the email is bonus comments for outreachy developers ]. > > And someone needs to check the final patch probably to remove the extra > parentheses around "(p + 2)". Those were necessary when for the cast > but not required after the cast is gone. > > > pmlmeinfo->auth_seq = 3; > > issue_auth(padapter, NULL, 0); > > set_link_timer(pmlmeext, REAUTH_TO); > > It's sort of tricky to know what "one thing per patch means". It seems somewhat arbitrary and based on Greg's understanding of the experience of the patch submitter and also the language of the potential commit message. > - memset((void *)(&(pHTInfo->SelfHTCap)), 0, > + memset((&(pHTInfo->SelfHTCap)), 0, > sizeof(pHTInfo->SelfHTCap)); > > Here the parentheses were never related to the cast so we should leave > them as is. In other words, in the first example, if we didn't remove > the cast that would be "half a thing per patch" and in the second > example that would be "two things in one patch". For style patches, it's frequently easier and better to do all the code transformation at once. IMO the last should be: memset(>SelfHTCap, 0, sizeof(pHTInfo->SelfHTCap)); like it is here: drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c:1056: memset(>SelfHTCap, 0, sizeof(pHTInfo->SelfHTCap)); btw2: I really dislike all the code inconsistencies and unnecessary code duplication with miscellaneous changes in the rtl staging drivers Horrid stuff.
Re: [Outreachy kernel] Re: [PATCH v1 1/5] staging: wfx: fix warnings of no space is necessary
On Sun, 20 Oct 2019, Dan Carpenter wrote: > On Sat, Oct 19, 2019 at 01:02:31PM -0700, Joe Perches wrote: > > diff -u -p a/rtl8723bs/core/rtw_mlme_ext.c b/rtl8723bs/core/rtw_mlme_ext.c > > --- a/rtl8723bs/core/rtw_mlme_ext.c > > +++ b/rtl8723bs/core/rtw_mlme_ext.c > > @@ -1132,7 +1132,7 @@ unsigned int OnAuthClient(struct adapter > > goto authclnt_fail; > > } > > > > - memcpy((void *)(pmlmeinfo->chg_txt), (void *)(p + 2), > > len); > > + memcpy((void *)(pmlmeinfo->chg_txt), (p + 2), len); > > I wonder why it didn't remove the first void cast? If "it" is a semantic patch, it is probably because Coccinelle wasn't able to find the definition of the type of pmlmeinfo. By default, Coccinelle only consults a few header files (ones with the same names as the .c file or ones included with "" and located in the same directory). > > [ The rest of the email is bonus comments for outreachy developers ]. > > And someone needs to check the final patch probably to remove the extra > parentheses around "(p + 2)". Those were necessary when for the cast > but not required after the cast is gone. The rule could have contained - (void *)(e) + e That should also match cases with no parentheses. I think there is even something to put the parentheses back if they are needed, but overall the final patch should be checked carefully in any case. julia > > > pmlmeinfo->auth_seq = 3; > > issue_auth(padapter, NULL, 0); > > set_link_timer(pmlmeext, REAUTH_TO); > > It's sort of tricky to know what "one thing per patch means". > > - memset((void *)(&(pHTInfo->SelfHTCap)), 0, > + memset((&(pHTInfo->SelfHTCap)), 0, > sizeof(pHTInfo->SelfHTCap)); > > Here the parentheses were never related to the cast so we should leave > them as is. In other words, in the first example, if we didn't remove > the cast that would be "half a thing per patch" and in the second > example that would be "two things in one patch". > > regards, > dan carpenter > > -- > You received this message because you are subscribed to the Google Groups > "outreachy-kernel" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to outreachy-kernel+unsubscr...@googlegroups.com. > To view this discussion on the web visit > https://groups.google.com/d/msgid/outreachy-kernel/20191020191759.GJ24678%40kadam. >
Re: [PATCH v1 1/5] staging: wfx: fix warnings of no space is necessary
On Sat, Oct 19, 2019 at 01:02:31PM -0700, Joe Perches wrote: > diff -u -p a/rtl8723bs/core/rtw_mlme_ext.c b/rtl8723bs/core/rtw_mlme_ext.c > --- a/rtl8723bs/core/rtw_mlme_ext.c > +++ b/rtl8723bs/core/rtw_mlme_ext.c > @@ -1132,7 +1132,7 @@ unsigned int OnAuthClient(struct adapter > goto authclnt_fail; > } > > - memcpy((void *)(pmlmeinfo->chg_txt), (void *)(p + 2), > len); > + memcpy((void *)(pmlmeinfo->chg_txt), (p + 2), len); I wonder why it didn't remove the first void cast? [ The rest of the email is bonus comments for outreachy developers ]. And someone needs to check the final patch probably to remove the extra parentheses around "(p + 2)". Those were necessary when for the cast but not required after the cast is gone. > pmlmeinfo->auth_seq = 3; > issue_auth(padapter, NULL, 0); > set_link_timer(pmlmeext, REAUTH_TO); It's sort of tricky to know what "one thing per patch means". - memset((void *)(&(pHTInfo->SelfHTCap)), 0, + memset((&(pHTInfo->SelfHTCap)), 0, sizeof(pHTInfo->SelfHTCap)); Here the parentheses were never related to the cast so we should leave them as is. In other words, in the first example, if we didn't remove the cast that would be "half a thing per patch" and in the second example that would be "two things in one patch". regards, dan carpenter
Re: [PATCH v1 1/5] staging: wfx: fix warnings of no space is necessary
On Sat, 2019-10-19 at 21:05 +0300, Dan Carpenter wrote:
> On Sat, Oct 19, 2019 at 04:09:03PM +0100, Jules Irenge wrote:
> > Checkpatch was complaining about space between type cast and the
> > variable. I just get rid of the space. Well I don't know whether this was
> > false positive one.
> >
> > Thanks for the feedback. I will update the patch.
>
> No no. The patch is fine. I was saying that in the *future* you might
> want to look at the void casts. Some of them are not required and
> others are buggy code.
Hello Jules.
Frequently, checkpatch is not the best tool to find and
possibly correct various unnecessary code styles.
checkpatch is OK to look at some odd uses
coccinelle is frequently a better overall tool for these uses.
A trivial cocci script to remove some unnecessary void * casts
from mem and str calls might be like the below, but
watch out for __iomem and __force uses where coccinelle doesn't
understand the syntax.
$ cat unnecessary_void_ptr.cocci
@@
type T1;
type T2;
T1 *p1;
T2 *p2;
@@
\(memcpy\|memmove\|strcpy\|strncmp\)(
- (void *)
p1,
- (void *)
p2, ...)
@@
type T1;
type T2;
T1 *p1;
T2 *p2;
@@
\(memcpy\|memmove\|strcpy\|strncmp\)(
p1,
- (void *)
p2, ...)
@@
type T;
T *p;
@@
\(memcpy\|memset\|memmove\|memzero\|strcpy\|strncmp\|strnlen\)(
- (void *)
p, ...)
$
For instance, when run over today's drivers/staging, this produces:
$ spatch --very-quiet -sp-file unnecessary_void_ptr.cocci drivers/staging
---
diff -u -p a/exfat/exfat_core.c b/exfat/exfat_core.c
--- a/exfat/exfat_core.c
+++ b/exfat/exfat_core.c
@@ -3427,7 +3427,7 @@ s32 rename_file(struct inode *inode, str
return FFS_MEDIAERR;
}
- memcpy((void *)epnew, (void *)epold, DENTRY_SIZE);
+ memcpy(epnew, epold, DENTRY_SIZE);
if (fs_func->get_entry_type(epnew) == TYPE_FILE) {
fs_func->set_entry_attr(epnew,
fs_func->get_entry_attr(epnew) |
@@ -3449,7 +3449,7 @@ s32 rename_file(struct inode *inode, str
return FFS_MEDIAERR;
}
- memcpy((void *)epnew, (void *)epold, DENTRY_SIZE);
+ memcpy(epnew, epold, DENTRY_SIZE);
buf_modify(sb, sector_new);
buf_unlock(sb, sector_old);
}
@@ -3538,7 +3538,7 @@ s32 move_file(struct inode *inode, struc
return FFS_MEDIAERR;
}
- memcpy((void *)epnew, (void *)epmov, DENTRY_SIZE);
+ memcpy(epnew, epmov, DENTRY_SIZE);
if (fs_func->get_entry_type(epnew) == TYPE_FILE) {
fs_func->set_entry_attr(epnew, fs_func->get_entry_attr(epnew) |
ATTR_ARCHIVE);
@@ -3558,7 +3558,7 @@ s32 move_file(struct inode *inode, struc
return FFS_MEDIAERR;
}
- memcpy((void *)epnew, (void *)epmov, DENTRY_SIZE);
+ memcpy(epnew, epmov, DENTRY_SIZE);
buf_modify(sb, sector_new);
buf_unlock(sb, sector_mov);
} else if (fs_func->get_entry_type(epnew) == TYPE_DIR) {
diff -u -p a/qlge/qlge_main.c b/qlge/qlge_main.c
--- a/qlge/qlge_main.c
+++ b/qlge/qlge_main.c
@@ -2583,7 +2583,7 @@ static netdev_tx_t qlge_send(struct sk_b
}
tx_ring_desc = _ring->q[tx_ring->prod_idx];
mac_iocb_ptr = tx_ring_desc->queue_entry;
- memset((void *)mac_iocb_ptr, 0, sizeof(*mac_iocb_ptr));
+ memset(mac_iocb_ptr, 0, sizeof(*mac_iocb_ptr));
mac_iocb_ptr->opcode = OPCODE_OB_MAC_IOCB;
mac_iocb_ptr->tid = tx_ring_desc->index;
@@ -3029,7 +3029,7 @@ static int ql_start_rx_ring(struct ql_ad
/* PCI doorbell mem area + 0x1c */
rx_ring->sbq.prod_idx_db_reg = (u32 __iomem *)(doorbell_area + 0x1c);
- memset((void *)cqicb, 0, sizeof(struct cqicb));
+ memset(cqicb, 0, sizeof(struct cqicb));
cqicb->msix_vect = rx_ring->irq;
cqicb->len = cpu_to_le16(QLGE_FIT16(rx_ring->cq_len) | LEN_V |
@@ -3473,7 +3473,7 @@ static int ql_start_rss(struct ql_adapte
int i;
u8 *hash_id = (u8 *) ricb->hash_cq_id;
- memset((void *)ricb, 0, sizeof(*ricb));
+ memset(ricb, 0, sizeof(*ricb));
ricb->base_cq = RSS_L4K;
ricb->flags =
@@ -3486,8 +3486,8 @@ static int ql_start_rss(struct ql_adapte
for (i = 0; i < 1024; i++)
hash_id[i] = (i & (qdev->rss_ring_count - 1));
- memcpy((void *)>ipv6_hash_key[0], init_hash_seed, 40);
- memcpy((void *)>ipv4_hash_key[0], init_hash_seed, 16);
+ memcpy(>ipv6_hash_key[0], init_hash_seed, 40);
+ memcpy(>ipv4_hash_key[0], init_hash_seed, 16);
status = ql_write_cfg(qdev, ricb, sizeof(*ricb), CFG_LR, 0);
if (status) {
@@ -3983,7 +3983,7 @@ static
Re: [PATCH v1 1/5] staging: wfx: fix warnings of no space is necessary
On Sat, Oct 19, 2019 at 04:09:03PM +0100, Jules Irenge wrote: > Checkpatch was complaining about space between type cast and the > variable. I just get rid of the space. Well I don't know whether this was > false positive one. > > Thanks for the feedback. I will update the patch. No no. The patch is fine. I was saying that in the *future* you might want to look at the void casts. Some of them are not required and others are buggy code. regards, dan carpenter
Re: [Outreachy kernel] Re: [PATCH v1 1/5] staging: wfx: fix warnings of no space is necessary
On Sat, 19 Oct 2019, Jules Irenge wrote:
>
>
> On Sat, 19 Oct 2019, Dan Carpenter wrote:
>
> > On Sat, Oct 19, 2019 at 03:07:15PM +0100, Jules Irenge wrote:
> > > diff --git a/drivers/staging/wfx/bh.c b/drivers/staging/wfx/bh.c
> > > index 3355183fc86c..573216b08042 100644
> > > --- a/drivers/staging/wfx/bh.c
> > > +++ b/drivers/staging/wfx/bh.c
> > > @@ -69,13 +69,13 @@ static int rx_helper(struct wfx_dev *wdev, size_t
> > > read_len, int *is_cnf)
> > > if (wfx_data_read(wdev, skb->data, alloc_len))
> > > goto err;
> > >
> > > - piggyback = le16_to_cpup((u16 *) (skb->data + alloc_len - 2));
> > > + piggyback = le16_to_cpup((u16 *)(skb->data + alloc_len - 2));
> > > _trace_piggyback(piggyback, false);
> > >
> > > - hif = (struct hif_msg *) skb->data;
> > > + hif = (struct hif_msg *)skb->data;
> > > WARN(hif->encrypted & 0x1, "unsupported encryption type");
> > > if (hif->encrypted == 0x2) {
> > > - if (wfx_sl_decode(wdev, (void *) hif)) {
> > > + if (wfx_sl_decode(wdev, (void *)hif)) {
> >
> > In the future you may want to go through and remove the (void *) casts.
> > It's not required here.
> >
> > > diff --git a/drivers/staging/wfx/bus_spi.c b/drivers/staging/wfx/bus_spi.c
> > > index f65f7d75e731..effd07957753 100644
> > > --- a/drivers/staging/wfx/bus_spi.c
> > > +++ b/drivers/staging/wfx/bus_spi.c
> > > @@ -90,7 +90,7 @@ static int wfx_spi_copy_to_io(void *priv, unsigned int
> > > addr,
> > > struct wfx_spi_priv *bus = priv;
> > > u16 regaddr = (addr << 12) | (count / 2);
> > > // FIXME: use a bounce buffer
> > > - u16 *src16 = (void *) src;
> > > + u16 *src16 = (void *)src;
> >
> > Here we are just getting rid of the constness. Apparently we are doing
> > that so we can modify it without GCC pointing out the bug!! I don't
> > know the code but this seems very wrong.
> >
> Checkpatch was complaining about space between type cast and the
> variable. I just get rid of the space. Well I don't know whether this was
> false positive one.
I think you missed the point. It would be good to trace through the core
and try to figure out where this src value comes from. Is it really
const? Or is the const declaration there just to satisfy the type
checker, and is the actual data provided not const. This function is
stored in a hwbus_ops structure. It would be good to see what other
drivers that store a function in the same field of such a structure do,
and to see where the function is actually called (via a function pointer)
and where the argument comes from.
julia
Re: [PATCH v1 1/5] staging: wfx: fix warnings of no space is necessary
On Sat, 19 Oct 2019, Dan Carpenter wrote:
> On Sat, Oct 19, 2019 at 03:07:15PM +0100, Jules Irenge wrote:
> > diff --git a/drivers/staging/wfx/bh.c b/drivers/staging/wfx/bh.c
> > index 3355183fc86c..573216b08042 100644
> > --- a/drivers/staging/wfx/bh.c
> > +++ b/drivers/staging/wfx/bh.c
> > @@ -69,13 +69,13 @@ static int rx_helper(struct wfx_dev *wdev, size_t
> > read_len, int *is_cnf)
> > if (wfx_data_read(wdev, skb->data, alloc_len))
> > goto err;
> >
> > - piggyback = le16_to_cpup((u16 *) (skb->data + alloc_len - 2));
> > + piggyback = le16_to_cpup((u16 *)(skb->data + alloc_len - 2));
> > _trace_piggyback(piggyback, false);
> >
> > - hif = (struct hif_msg *) skb->data;
> > + hif = (struct hif_msg *)skb->data;
> > WARN(hif->encrypted & 0x1, "unsupported encryption type");
> > if (hif->encrypted == 0x2) {
> > - if (wfx_sl_decode(wdev, (void *) hif)) {
> > + if (wfx_sl_decode(wdev, (void *)hif)) {
>
> In the future you may want to go through and remove the (void *) casts.
> It's not required here.
>
> > diff --git a/drivers/staging/wfx/bus_spi.c b/drivers/staging/wfx/bus_spi.c
> > index f65f7d75e731..effd07957753 100644
> > --- a/drivers/staging/wfx/bus_spi.c
> > +++ b/drivers/staging/wfx/bus_spi.c
> > @@ -90,7 +90,7 @@ static int wfx_spi_copy_to_io(void *priv, unsigned int
> > addr,
> > struct wfx_spi_priv *bus = priv;
> > u16 regaddr = (addr << 12) | (count / 2);
> > // FIXME: use a bounce buffer
> > - u16 *src16 = (void *) src;
> > + u16 *src16 = (void *)src;
>
> Here we are just getting rid of the constness. Apparently we are doing
> that so we can modify it without GCC pointing out the bug!! I don't
> know the code but this seems very wrong.
>
Checkpatch was complaining about space between type cast and the
variable. I just get rid of the space. Well I don't know whether this was
false positive one.
Thanks for the feedback. I will update the patch.
Regards,
Jules
Re: [PATCH v1 1/5] staging: wfx: fix warnings of no space is necessary
On Sat, Oct 19, 2019 at 03:07:15PM +0100, Jules Irenge wrote:
> diff --git a/drivers/staging/wfx/bh.c b/drivers/staging/wfx/bh.c
> index 3355183fc86c..573216b08042 100644
> --- a/drivers/staging/wfx/bh.c
> +++ b/drivers/staging/wfx/bh.c
> @@ -69,13 +69,13 @@ static int rx_helper(struct wfx_dev *wdev, size_t
> read_len, int *is_cnf)
> if (wfx_data_read(wdev, skb->data, alloc_len))
> goto err;
>
> - piggyback = le16_to_cpup((u16 *) (skb->data + alloc_len - 2));
> + piggyback = le16_to_cpup((u16 *)(skb->data + alloc_len - 2));
> _trace_piggyback(piggyback, false);
>
> - hif = (struct hif_msg *) skb->data;
> + hif = (struct hif_msg *)skb->data;
> WARN(hif->encrypted & 0x1, "unsupported encryption type");
> if (hif->encrypted == 0x2) {
> - if (wfx_sl_decode(wdev, (void *) hif)) {
> + if (wfx_sl_decode(wdev, (void *)hif)) {
In the future you may want to go through and remove the (void *) casts.
It's not required here.
> diff --git a/drivers/staging/wfx/bus_spi.c b/drivers/staging/wfx/bus_spi.c
> index f65f7d75e731..effd07957753 100644
> --- a/drivers/staging/wfx/bus_spi.c
> +++ b/drivers/staging/wfx/bus_spi.c
> @@ -90,7 +90,7 @@ static int wfx_spi_copy_to_io(void *priv, unsigned int addr,
> struct wfx_spi_priv *bus = priv;
> u16 regaddr = (addr << 12) | (count / 2);
> // FIXME: use a bounce buffer
> - u16 *src16 = (void *) src;
> + u16 *src16 = (void *)src;
Here we are just getting rid of the constness. Apparently we are doing
that so we can modify it without GCC pointing out the bug!! I don't
know the code but this seems very wrong.
> int ret, i;
> struct spi_message m;
> struct spi_transfer t_addr = {
regards,
dan carpenter
[PATCH v1 1/5] staging: wfx: fix warnings of no space is necessary
Fix warnings of no space is necessary after a cast.
Issue detected by checkpatch tool.
Signed-off-by: Jules Irenge
---
drivers/staging/wfx/bh.c | 8
drivers/staging/wfx/bus_sdio.c | 6 +++---
drivers/staging/wfx/bus_spi.c | 2 +-
drivers/staging/wfx/data_rx.c | 8
drivers/staging/wfx/data_tx.c | 20 ++--
drivers/staging/wfx/data_tx.h | 4 ++--
6 files changed, 24 insertions(+), 24 deletions(-)
diff --git a/drivers/staging/wfx/bh.c b/drivers/staging/wfx/bh.c
index 3355183fc86c..573216b08042 100644
--- a/drivers/staging/wfx/bh.c
+++ b/drivers/staging/wfx/bh.c
@@ -69,13 +69,13 @@ static int rx_helper(struct wfx_dev *wdev, size_t read_len,
int *is_cnf)
if (wfx_data_read(wdev, skb->data, alloc_len))
goto err;
- piggyback = le16_to_cpup((u16 *) (skb->data + alloc_len - 2));
+ piggyback = le16_to_cpup((u16 *)(skb->data + alloc_len - 2));
_trace_piggyback(piggyback, false);
- hif = (struct hif_msg *) skb->data;
+ hif = (struct hif_msg *)skb->data;
WARN(hif->encrypted & 0x1, "unsupported encryption type");
if (hif->encrypted == 0x2) {
- if (wfx_sl_decode(wdev, (void *) hif)) {
+ if (wfx_sl_decode(wdev, (void *)hif)) {
dev_kfree_skb(skb);
// If frame was a confirmation, expect trouble in next
// exchange. However, it is harmless to fail to decode
@@ -102,7 +102,7 @@ static int rx_helper(struct wfx_dev *wdev, size_t read_len,
int *is_cnf)
if (!(hif->id & HIF_ID_IS_INDICATION)) {
(*is_cnf)++;
if (hif->id == HIF_CNF_ID_MULTI_TRANSMIT)
- release_count = le32_to_cpu(((struct
hif_cnf_multi_transmit *) hif->body)->num_tx_confs);
+ release_count = le32_to_cpu(((struct
hif_cnf_multi_transmit *)hif->body)->num_tx_confs);
else
release_count = 1;
WARN(wdev->hif.tx_buffers_used < release_count, "corrupted
buffer counter");
diff --git a/drivers/staging/wfx/bus_sdio.c b/drivers/staging/wfx/bus_sdio.c
index f97360513150..184e20dfdd62 100644
--- a/drivers/staging/wfx/bus_sdio.c
+++ b/drivers/staging/wfx/bus_sdio.c
@@ -38,7 +38,7 @@ static int wfx_sdio_copy_from_io(void *priv, unsigned int
reg_id,
int ret;
WARN(reg_id > 7, "chip only has 7 registers");
- WARN(((uintptr_t) dst) & 3, "unaligned buffer size");
+ WARN(((uintptr_t)dst) & 3, "unaligned buffer size");
WARN(count & 3, "unaligned buffer address");
/* Use queue mode buffers */
@@ -59,14 +59,14 @@ static int wfx_sdio_copy_to_io(void *priv, unsigned int
reg_id,
int ret;
WARN(reg_id > 7, "chip only has 7 registers");
- WARN(((uintptr_t) src) & 3, "unaligned buffer size");
+ WARN(((uintptr_t)src) & 3, "unaligned buffer size");
WARN(count & 3, "unaligned buffer address");
/* Use queue mode buffers */
if (reg_id == WFX_REG_IN_OUT_QUEUE)
sdio_addr |= bus->buf_id_tx << 7;
// FIXME: discards 'const' qualifier for src
- ret = sdio_memcpy_toio(bus->func, sdio_addr, (void *) src, count);
+ ret = sdio_memcpy_toio(bus->func, sdio_addr, (void *)src, count);
if (!ret && reg_id == WFX_REG_IN_OUT_QUEUE)
bus->buf_id_tx = (bus->buf_id_tx + 1) % 32;
diff --git a/drivers/staging/wfx/bus_spi.c b/drivers/staging/wfx/bus_spi.c
index f65f7d75e731..effd07957753 100644
--- a/drivers/staging/wfx/bus_spi.c
+++ b/drivers/staging/wfx/bus_spi.c
@@ -90,7 +90,7 @@ static int wfx_spi_copy_to_io(void *priv, unsigned int addr,
struct wfx_spi_priv *bus = priv;
u16 regaddr = (addr << 12) | (count / 2);
// FIXME: use a bounce buffer
- u16 *src16 = (void *) src;
+ u16 *src16 = (void *)src;
int ret, i;
struct spi_message m;
struct spi_transfer t_addr = {
diff --git a/drivers/staging/wfx/data_rx.c b/drivers/staging/wfx/data_rx.c
index 3a79089c8501..3a79ab93e97e 100644
--- a/drivers/staging/wfx/data_rx.c
+++ b/drivers/staging/wfx/data_rx.c
@@ -29,7 +29,7 @@ static int wfx_handle_pspoll(struct wfx_vif *wvif, struct
sk_buff *skb)
rcu_read_lock();
sta = ieee80211_find_sta(wvif->vif, pspoll->ta);
if (sta)
- link_id = ((struct wfx_sta_priv *) >drv_priv)->link_id;
+ link_id = ((struct wfx_sta_priv *)>drv_priv)->link_id;
rcu_read_unlock();
if (link_id)
pspoll_mask = BIT(link_id);
@@ -102,8 +102,8 @@ void wfx_rx_cb(struct wfx_vif *wvif, struct hif_ind_rx
*arg, struct sk_buff *skb
{
int link_id = arg->rx_flags.peer_sta_id;
struct ieee80211_rx_status *hdr = IEEE80211_SKB_RXCB(skb);
- struct ieee80211_hdr *frame = (struct ieee80211_hdr *) skb->data;
- struct ieee80211_mgmt *mgmt = (struct ieee80211_mgmt *) skb->data;
+
