LinuxLists.cc - [PATCH v1 0/5] staging: wfx: fix checkpatch warnings

[permalink] [raw]

Subject: 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

2019-10-19 15:10:11

by Jules Irenge

[permalink] [raw]

Subject: 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

2019-10-19 15:18:12

[permalink] [raw]

Subject: 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

2019-10-19 18:09:48

[permalink] [raw]

Subject: 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

2019-10-19 20:03:18

[permalink] [raw]

Subject: 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<foo> and str<foo> 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 = &tx_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 *)&ricb->ipv6_hash_key[0], init_hash_seed, 40);
- memcpy((void *)&ricb->ipv4_hash_key[0], init_hash_seed, 16);
+ memcpy(&ricb->ipv6_hash_key[0], init_hash_seed, 40);
+ memcpy(&ricb->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 int ql_configure_rings(struct ql_

for (i = 0; i < qdev->tx_ring_count; i++) {
tx_ring = &qdev->tx_ring[i];
- memset((void *)tx_ring, 0, sizeof(*tx_ring));
+ memset(tx_ring, 0, sizeof(*tx_ring));
tx_ring->qdev = qdev;
tx_ring->wq_id = i;
tx_ring->wq_len = qdev->tx_ring_size;
@@ -3999,7 +3999,7 @@ static int ql_configure_rings(struct ql_

for (i = 0; i < qdev->rx_ring_count; i++) {
rx_ring = &qdev->rx_ring[i];
- memset((void *)rx_ring, 0, sizeof(*rx_ring));
+ memset(rx_ring, 0, sizeof(*rx_ring));
rx_ring->qdev = qdev;
rx_ring->cq_id = i;
rx_ring->cpu = i % cpu_cnt; /* CPU to run handler on. */
@@ -4414,7 +4414,7 @@ static int ql_init_device(struct pci_dev
struct ql_adapter *qdev = netdev_priv(ndev);
int err = 0;

- memset((void *)qdev, 0, sizeof(*qdev));
+ memset(qdev, 0, sizeof(*qdev));
err = pci_enable_device(pdev);
if (err) {
dev_err(&pdev->dev, "PCI device enable failed.\n");
diff -u -p a/rtl8188eu/core/rtw_ieee80211.c b/rtl8188eu/core/rtw_ieee80211.c
--- a/rtl8188eu/core/rtw_ieee80211.c
+++ b/rtl8188eu/core/rtw_ieee80211.c
@@ -132,7 +132,7 @@ u8 *rtw_set_ie
*(pbuf + 1) = (u8)len;

if (len > 0)
- memcpy((void *)(pbuf + 2), (void *)source, len);
+ memcpy((pbuf + 2), source, len);

*frlen = *frlen + (len + 2);

diff -u -p a/rtl8188eu/core/rtw_mlme_ext.c b/rtl8188eu/core/rtw_mlme_ext.c
--- a/rtl8188eu/core/rtw_mlme_ext.c
+++ b/rtl8188eu/core/rtw_mlme_ext.c
@@ -2858,7 +2858,7 @@ static unsigned int OnAuthClient(struct
if (p == NULL)
goto authclnt_fail;

- memcpy((void *)(pmlmeinfo->chg_txt), (void *)(p + 2), len);
+ memcpy((void *)(pmlmeinfo->chg_txt), (p + 2), len);
pmlmeinfo->auth_seq = 3;
issue_auth(padapter, NULL, 0);
set_link_timer(pmlmeext, REAUTH_TO);
diff -u -p a/rtl8192e/rtl819x_HTProc.c b/rtl8192e/rtl819x_HTProc.c
--- a/rtl8192e/rtl819x_HTProc.c
+++ b/rtl8192e/rtl819x_HTProc.c
@@ -655,13 +655,13 @@ void HTInitializeHTInfo(struct rtllib_de
pHTInfo->CurrentMPDUDensity = pHTInfo->MPDU_Density;
pHTInfo->CurrentAMPDUFactor = pHTInfo->AMPDU_Factor;

- memset((void *)(&(pHTInfo->SelfHTCap)), 0,
+ memset((&(pHTInfo->SelfHTCap)), 0,
sizeof(pHTInfo->SelfHTCap));
- memset((void *)(&(pHTInfo->SelfHTInfo)), 0,
+ memset((&(pHTInfo->SelfHTInfo)), 0,
sizeof(pHTInfo->SelfHTInfo));
- memset((void *)(&(pHTInfo->PeerHTCapBuf)), 0,
+ memset((&(pHTInfo->PeerHTCapBuf)), 0,
sizeof(pHTInfo->PeerHTCapBuf));
- memset((void *)(&(pHTInfo->PeerHTInfoBuf)), 0,
+ memset((&(pHTInfo->PeerHTInfoBuf)), 0,
sizeof(pHTInfo->PeerHTInfoBuf));

pHTInfo->bSwBwInProgress = false;
diff -u -p a/rtl8192u/r819xU_cmdpkt.c b/rtl8192u/r819xU_cmdpkt.c
--- a/rtl8192u/r819xU_cmdpkt.c
+++ b/rtl8192u/r819xU_cmdpkt.c
@@ -373,7 +373,7 @@ static void cmpk_handle_tx_status(struct
{
cmpk_tx_status_t rx_tx_sts;

- memcpy((void *)&rx_tx_sts, (void *)pmsg, sizeof(cmpk_tx_status_t));
+ memcpy(&rx_tx_sts, pmsg, sizeof(cmpk_tx_status_t));
/* 2. Use tx feedback info to count TX statistics. */
cmpk_count_tx_status(dev, &rx_tx_sts);
}
diff -u -p a/rtl8712/ieee80211.c b/rtl8712/ieee80211.c
--- a/rtl8712/ieee80211.c
+++ b/rtl8712/ieee80211.c
@@ -90,7 +90,7 @@ u8 *r8712_set_ie(u8 *pbuf, sint index, u
*pbuf = (u8)index;
*(pbuf + 1) = (u8)len;
if (len > 0)
- memcpy((void *)(pbuf + 2), (void *)source, len);
+ memcpy((pbuf + 2), source, len);
*frlen = *frlen + (len + 2);
return pbuf + len + 2;
}
diff -u -p a/rtl8723bs/core/rtw_ieee80211.c b/rtl8723bs/core/rtw_ieee80211.c
--- a/rtl8723bs/core/rtw_ieee80211.c
+++ b/rtl8723bs/core/rtw_ieee80211.c
@@ -112,7 +112,7 @@ int rtw_check_network_type(unsigned char
u8 *rtw_set_fixed_ie(unsigned char *pbuf, unsigned int len, unsigned char *source,
unsigned int *frlen)
{
- memcpy((void *)pbuf, (void *)source, len);
+ memcpy(pbuf, source, len);
*frlen = *frlen + len;
return pbuf + len;
}
@@ -132,7 +132,7 @@ u8 *rtw_set_ie
*(pbuf + 1) = (u8)len;

if (len > 0)
- memcpy((void *)(pbuf + 2), (void *)source, len);
+ memcpy((pbuf + 2), source, len);

*frlen = *frlen + (len + 2);

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);
pmlmeinfo->auth_seq = 3;
issue_auth(padapter, NULL, 0);
set_link_timer(pmlmeext, REAUTH_TO);
diff -u -p a/rtl8723bs/hal/rtl8723b_hal_init.c b/rtl8723bs/hal/rtl8723b_hal_init.c
--- a/rtl8723bs/hal/rtl8723b_hal_init.c
+++ b/rtl8723bs/hal/rtl8723b_hal_init.c
@@ -2432,13 +2432,13 @@ void Hal_InitPGData(struct adapter *pada
if (!pEEPROM->EepromOrEfuse) {
/* Read EFUSE real map to shadow. */
EFUSE_ShadowMapUpdate(padapter, EFUSE_WIFI, false);
- memcpy((void *)PROMContent, (void *)pEEPROM->efuse_eeprom_data, HWSET_MAX_SIZE_8723B);
+ memcpy(PROMContent, (void *)pEEPROM->efuse_eeprom_data, HWSET_MAX_SIZE_8723B);
}
} else {/* autoload fail */
RT_TRACE(_module_hci_hal_init_c_, _drv_notice_, ("AutoLoad Fail reported from CR9346!!\n"));
if (!pEEPROM->EepromOrEfuse)
EFUSE_ShadowMapUpdate(padapter, EFUSE_WIFI, false);
- memcpy((void *)PROMContent, (void *)pEEPROM->efuse_eeprom_data, HWSET_MAX_SIZE_8723B);
+ memcpy(PROMContent, (void *)pEEPROM->efuse_eeprom_data, HWSET_MAX_SIZE_8723B);
}
}

diff -u -p a/rtl8723bs/os_dep/ioctl_cfg80211.c b/rtl8723bs/os_dep/ioctl_cfg80211.c
--- a/rtl8723bs/os_dep/ioctl_cfg80211.c
+++ b/rtl8723bs/os_dep/ioctl_cfg80211.c
@@ -98,7 +98,7 @@ static struct ieee80211_channel rtw_2ghz

static void rtw_2g_channels_init(struct ieee80211_channel *channels)
{
- memcpy((void*)channels, (void*)rtw_2ghz_channels,
+ memcpy(channels, (void*)rtw_2ghz_channels,
sizeof(struct ieee80211_channel)*RTW_2G_CHANNELS_NUM
);
}
@@ -2548,7 +2548,7 @@ static netdev_tx_t rtw_cfg80211_monitor_

pframe = (u8 *)(pmgntframe->buf_addr) + TXDESC_OFFSET;

- memcpy(pframe, (void*)buf, len);
+ memcpy(pframe, buf, len);
pattrib->pktlen = len;

pwlanhdr = (struct ieee80211_hdr *)pframe;
@@ -2750,7 +2750,7 @@ static int rtw_add_beacon(struct adapter
return -ENOMEM;

memcpy(pbuf, (void *)head+24, head_len-24);/* 24 =beacon header len. */
- memcpy(pbuf+head_len-24, (void *)tail, tail_len);
+ memcpy(pbuf+head_len-24, tail, tail_len);

len = head_len+tail_len-24;

@@ -3041,7 +3041,7 @@ static int _cfg80211_rtw_mgmt_tx(struct

pframe = (u8 *)(pmgntframe->buf_addr) + TXDESC_OFFSET;

- memcpy(pframe, (void*)buf, len);
+ memcpy(pframe, buf, len);
pattrib->pktlen = len;

pwlanhdr = (struct ieee80211_hdr *)pframe;
diff -u -p a/vt6655/rxtx.c b/vt6655/rxtx.c
--- a/vt6655/rxtx.c
+++ b/vt6655/rxtx.c
@@ -1170,7 +1170,7 @@ s_cbFillTxBufHead(struct vnt_private *pD

td_info->mic_hdr = pMICHDR;

- memset((void *)(pbyTxBufferAddr + wTxBufSize), 0, (cbHeaderLength - wTxBufSize));
+ memset((pbyTxBufferAddr + wTxBufSize), 0, (cbHeaderLength - wTxBufSize));

/* Fill FIFO,RrvTime,RTS,and CTS */
s_vGenerateTxParameter(pDevice, byPktType, tx_buffer_head, pvRrvTime, pvRTS, pvCTS,

2019-10-20 19:19:04

[permalink] [raw]

Subject: 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

2019-10-20 19:33:57

[permalink] [raw]

Subject: 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 [email protected].
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20191020191759.GJ24678%40kadam.
>

2019-10-20 19:37:32

[permalink] [raw]

Subject: 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(&pHTInfo->SelfHTCap, 0, sizeof(pHTInfo->SelfHTCap));

like it is here:

drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c:1056: memset(&pHTInfo->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.

2019-10-20 19:49:43

[permalink] [raw]

Subject: 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(&pHTInfo->SelfHTCap, 0, sizeof(pHTInfo->SelfHTCap));
>
> like it is here:
>
> drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c:1056: memset(&pHTInfo->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 [email protected].
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/6e6bc92cac0858fe5bd37b28f688d3da043f4bef.camel%40perches.com.
>

2019-10-20 19:53:31

[permalink] [raw]

Subject: 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(&pHTInfo->SelfHTCap, 0, sizeof(pHTInfo->SelfHTCap));
>
> like it is here:
>
> drivers/staging/rtl8192u/ieee80211/rtl819x_HTProc.c:1056: memset(&pHTInfo->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 [email protected].
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/6e6bc92cac0858fe5bd37b28f688d3da043f4bef.camel%40perches.com.
>

2019-10-20 20:17:23

[permalink] [raw]

Subject: 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<foo>, 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.

2019-10-20 20:30:19

[permalink] [raw]

Subject: 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<foo>, 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.
>
>
>

2019-10-21 06:54:02

[permalink] [raw]

Subject: 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

2019-10-21 08:24:28

by Jérôme Pouiller

[permalink] [raw]

Subject: Re: [PATCH v1 1/5] staging: wfx: fix warnings of no space is necessary

On Saturday 19 October 2019 16:24:43 CEST 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.

Hello Dan, Jules,

Indeed, this code should be improved.

Each u16 from src is byte-swapped before to be sent to SPI and restored
before to return from the function:

for (i = 0; i < count / 2; i++)
swab16s(&src16[i]);
[...]
spi_sync(bus->func, &m);
[...]
for (i = 0; i < count / 2; i++)
swab16s(&src16[i]);

So, src is same than original, but it is not const.

This is exactly the purpose of the FIXME just before the cast: "use a
bounce buffer". However, I did not yet make this change because I worry
about a possible performance penalty.

--
J?r?me Pouiller

2019-10-21 08:56:58

[permalink] [raw]

Subject: 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

2019-10-22 08:59:37