[PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls

[PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls

[Denis Efremov]

IS_ERR(), IS_ERR_OR_NULL(), IS_ERR_VALUE() and WARN*() already contain
unlikely() optimization internally. Thus, there is no point in calling
these functions and defines under likely()/unlikely().

This check is based on the coccinelle rule developed by Enrico Weigelt
https://lore.kernel.org/lkml/1559767582-11081-1-git-send-email-info@metux.net/

Signed-off-by: Denis Efremov <efremov@linux.com>
Cc: Joe Perches <joe@perches.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 93a7edfe0f05..56969ce06df4 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -6480,6 +6480,12 @@ sub process {
 			     "Using $1 should generally have parentheses around the comparison\n" . $herecurr);
 		}
 
+# nested likely/unlikely calls
+		if ($line =~ /\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) {
+			WARN("LIKELY_MISUSE",
+			     "nested (un)?likely() calls, $1 already uses unlikely() internally\n" . $herecurr);
+		}
+
 # whine mightly about in_atomic
 		if ($line =~ /\bin_atomic\s*\(/) {
 			if ($realfile =~ m@^drivers/@) {
-- 
2.21.0

[PATCH v3 02/11] drm/msm: remove unlikely() from WARN_ON() conditions

[Denis Efremov]

"unlikely(WARN_ON(x))" is excessive. WARN_ON() already uses unlikely()
internally.

Signed-off-by: Denis Efremov <efremov@linux.com>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Sean Paul <sean@poorly.run>
Cc: Joe Perches <joe@perches.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-arm-msm@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c | 4 ++--
 drivers/gpu/drm/msm/disp/mdp_format.c    | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c
index 4804cf40de14..030279d7b64b 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c
@@ -253,7 +253,7 @@ int mdp5_ctl_set_cursor(struct mdp5_ctl *ctl, struct mdp5_pipeline *pipeline,
 	u32 blend_cfg;
 	struct mdp5_hw_mixer *mixer = pipeline->mixer;
 
-	if (unlikely(WARN_ON(!mixer))) {
+	if (WARN_ON(!mixer)) {
 		DRM_DEV_ERROR(ctl_mgr->dev->dev, "CTL %d cannot find LM",
 			ctl->id);
 		return -EINVAL;
@@ -695,7 +695,7 @@ struct mdp5_ctl_manager *mdp5_ctlm_init(struct drm_device *dev,
 		goto fail;
 	}
 
-	if (unlikely(WARN_ON(ctl_cfg->count > MAX_CTL))) {
+	if (WARN_ON(ctl_cfg->count > MAX_CTL)) {
 		DRM_DEV_ERROR(dev->dev, "Increase static pool size to at least %d\n",
 				ctl_cfg->count);
 		ret = -ENOSPC;
diff --git a/drivers/gpu/drm/msm/disp/mdp_format.c b/drivers/gpu/drm/msm/disp/mdp_format.c
index 8afb0f9c04bb..5495d8b3f5b9 100644
--- a/drivers/gpu/drm/msm/disp/mdp_format.c
+++ b/drivers/gpu/drm/msm/disp/mdp_format.c
@@ -174,7 +174,7 @@ const struct msm_format *mdp_get_format(struct msm_kms *kms, uint32_t format,
 
 struct csc_cfg *mdp_get_default_csc_cfg(enum csc_type type)
 {
-	if (unlikely(WARN_ON(type >= CSC_MAX)))
+	if (WARN_ON(type >= CSC_MAX))
 		return NULL;
 
 	return &csc_convert[type];
-- 
2.21.0

[PATCH v3 03/11] net/mlx5e: Remove unlikely() from WARN*() condition

[Denis Efremov]

"unlikely(WARN_ON_ONCE(x))" is excessive. WARN_ON_ONCE() already uses
unlikely() internally.

Signed-off-by: Denis Efremov <efremov@linux.com>
Cc: Boris Pismenny <borisp@mellanox.com>
Cc: Saeed Mahameed <saeedm@mellanox.com>
Cc: Leon Romanovsky <leon@kernel.org>
Cc: Joe Perches <joe@perches.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: netdev@vger.kernel.org
---
 drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
index 7833ddef0427..e5222d17df35 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
@@ -408,7 +408,7 @@ struct sk_buff *mlx5e_ktls_handle_tx_skb(struct net_device *netdev,
 		goto out;
 
 	tls_ctx = tls_get_ctx(skb->sk);
-	if (unlikely(WARN_ON_ONCE(tls_ctx->netdev != netdev)))
+	if (WARN_ON_ONCE(tls_ctx->netdev != netdev))
 		goto err_out;
 
 	priv_tx = mlx5e_get_ktls_tx_priv_ctx(tls_ctx);
-- 
2.21.0

[PATCH v3 04/11] xen/events: Remove unlikely() from WARN() condition

[Denis Efremov]

"unlikely(WARN(x))" is excessive. WARN() already uses unlikely()
internally.

Signed-off-by: Denis Efremov <efremov@linux.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Joe Perches <joe@perches.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: xen-devel@lists.xenproject.org
---
 drivers/xen/events/events_base.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 2e8570c09789..6c8843968a52 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -247,7 +247,7 @@ static void xen_irq_info_cleanup(struct irq_info *info)
  */
 unsigned int evtchn_from_irq(unsigned irq)
 {
-	if (unlikely(WARN(irq >= nr_irqs, "Invalid irq %d!\n", irq)))
+	if (WARN(irq >= nr_irqs, "Invalid irq %d!\n", irq))
 		return 0;
 
 	return info_for_irq(irq)->evtchn;
-- 
2.21.0

[PATCH v3 05/11] fs: remove unlikely() from WARN_ON() condition

[Denis Efremov]

"unlikely(WARN_ON(x))" is excessive. WARN_ON() already uses unlikely()
internally.

Signed-off-by: Denis Efremov <efremov@linux.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Joe Perches <joe@perches.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-fsdevel@vger.kernel.org
---
 fs/open.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/open.c b/fs/open.c
index a59abe3c669a..9432cd5a8294 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -776,7 +776,7 @@ static int do_dentry_open(struct file *f,
 		f->f_mode |= FMODE_ATOMIC_POS;
 
 	f->f_op = fops_get(inode->i_fop);
-	if (unlikely(WARN_ON(!f->f_op))) {
+	if (WARN_ON(!f->f_op)) {
 		error = -ENODEV;
 		goto cleanup_all;
 	}
-- 
2.21.0

[PATCH v3 06/11] wimax/i2400m: remove unlikely() from WARN*() condition

[Denis Efremov]

"unlikely(WARN_ON(x))" is excessive. WARN_ON() already uses unlikely()
internally.

Signed-off-by: Denis Efremov <efremov@linux.com>
Cc: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>
Cc: Joe Perches <joe@perches.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-wimax@intel.com
---
 drivers/net/wimax/i2400m/tx.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/wimax/i2400m/tx.c b/drivers/net/wimax/i2400m/tx.c
index ebd64e083726..1255302e251e 100644
--- a/drivers/net/wimax/i2400m/tx.c
+++ b/drivers/net/wimax/i2400m/tx.c
@@ -654,8 +654,7 @@ void i2400m_tx_close(struct i2400m *i2400m)
 	padding = aligned_size - tx_msg_moved->size;
 	if (padding > 0) {
 		pad_buf = i2400m_tx_fifo_push(i2400m, padding, 0, 0);
-		if (unlikely(WARN_ON(pad_buf == NULL
-				     || pad_buf == TAIL_FULL))) {
+		if (WARN_ON(pad_buf == NULL || pad_buf == TAIL_FULL)) {
 			/* This should not happen -- append should verify
 			 * there is always space left at least to append
 			 * tx_block_size */
-- 
2.21.0

[PATCH v3 07/11] xfs: remove unlikely() from WARN_ON() condition

[Denis Efremov]

"unlikely(WARN_ON(x))" is excessive. WARN_ON() already uses unlikely()
internally.

Signed-off-by: Denis Efremov <efremov@linux.com>
Cc: "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: Joe Perches <joe@perches.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-xfs@vger.kernel.org
---
 fs/xfs/xfs_buf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index ca0849043f54..4389d87ff0f0 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -2096,7 +2096,7 @@ xfs_verify_magic(
 	int			idx;
 
 	idx = xfs_sb_version_hascrc(&mp->m_sb);
-	if (unlikely(WARN_ON(!bp->b_ops || !bp->b_ops->magic[idx])))
+	if (WARN_ON(!bp->b_ops || !bp->b_ops->magic[idx]))
 		return false;
 	return dmagic == bp->b_ops->magic[idx];
 }
@@ -2114,7 +2114,7 @@ xfs_verify_magic16(
 	int			idx;
 
 	idx = xfs_sb_version_hascrc(&mp->m_sb);
-	if (unlikely(WARN_ON(!bp->b_ops || !bp->b_ops->magic16[idx])))
+	if (WARN_ON(!bp->b_ops || !bp->b_ops->magic16[idx]))
 		return false;
 	return dmagic == bp->b_ops->magic16[idx];
 }
-- 
2.21.0

[PATCH v3 08/11] IB/hfi1: Remove unlikely() from IS_ERR*() condition

[Denis Efremov]

"unlikely(IS_ERR_OR_NULL(x))" is excessive. IS_ERR_OR_NULL() already uses
unlikely() internally.

Signed-off-by: Denis Efremov <efremov@linux.com>
Cc: Mike Marciniszyn <mike.marciniszyn@intel.com>
Cc: Dennis Dalessandro <dennis.dalessandro@intel.com>
Cc: Joe Perches <joe@perches.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-rdma@vger.kernel.org
---
 drivers/infiniband/hw/hfi1/verbs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/hfi1/verbs.c b/drivers/infiniband/hw/hfi1/verbs.c
index 646f61545ed6..1c52b19dd0f8 100644
--- a/drivers/infiniband/hw/hfi1/verbs.c
+++ b/drivers/infiniband/hw/hfi1/verbs.c
@@ -1040,7 +1040,7 @@ int hfi1_verbs_send_pio(struct rvt_qp *qp, struct hfi1_pkt_state *ps,
 	if (cb)
 		iowait_pio_inc(&priv->s_iowait);
 	pbuf = sc_buffer_alloc(sc, plen, cb, qp);
-	if (unlikely(IS_ERR_OR_NULL(pbuf))) {
+	if (IS_ERR_OR_NULL(pbuf)) {
 		if (cb)
 			verbs_pio_complete(qp, 0);
 		if (IS_ERR(pbuf)) {
-- 
2.21.0

[PATCH v3 09/11] Input: alps - remove unlikely() from IS_ERR*() condition

[Denis Efremov]

"unlikely(IS_ERR_OR_NULL(x))" is excessive. IS_ERR_OR_NULL() already uses
unlikely() internally.

Signed-off-by: Denis Efremov <efremov@linux.com>
Cc: "Pali Rohár" <pali.rohar@gmail.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Joe Perches <joe@perches.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-input@vger.kernel.org
---
 drivers/input/mouse/alps.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 34700eda0429..ed1661434899 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -1476,7 +1476,7 @@ static void alps_report_bare_ps2_packet(struct psmouse *psmouse,
 		/* On V2 devices the DualPoint Stick reports bare packets */
 		dev = priv->dev2;
 		dev2 = psmouse->dev;
-	} else if (unlikely(IS_ERR_OR_NULL(priv->dev3))) {
+	} else if (IS_ERR_OR_NULL(priv->dev3)) {
 		/* Register dev3 mouse if we received PS/2 packet first time */
 		if (!IS_ERR(priv->dev3))
 			psmouse_queue_work(psmouse, &priv->dev3_register_work,
-- 
2.21.0

[PATCH v3 10/11] udp: Remove unlikely() from IS_ERR*() condition

[Denis Efremov]

"unlikely(IS_ERR_OR_NULL(x))" is excessive. IS_ERR_OR_NULL() already uses
unlikely() internally.

Signed-off-by: Denis Efremov <efremov@linux.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Joe Perches <joe@perches.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: netdev@vger.kernel.org
---
 include/net/udp.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/net/udp.h b/include/net/udp.h
index 79d141d2103b..bad74f780831 100644
--- a/include/net/udp.h
+++ b/include/net/udp.h
@@ -480,7 +480,7 @@ static inline struct sk_buff *udp_rcv_segment(struct sock *sk,
 	 * CB fragment
 	 */
 	segs = __skb_gso_segment(skb, features, false);
-	if (unlikely(IS_ERR_OR_NULL(segs))) {
+	if (IS_ERR_OR_NULL(segs)) {
 		int segs_nr = skb_shinfo(skb)->gso_segs;
 
 		atomic_add(segs_nr, &sk->sk_drops);
-- 
2.21.0

[PATCH v3 11/11] ntfs: remove (un)?likely() from IS_ERR() conditions

[Denis Efremov]

"likely(!IS_ERR(x))" is excessive. IS_ERR() already uses
unlikely() internally.

Signed-off-by: Denis Efremov <efremov@linux.com>
Cc: Anton Altaparmakov <anton@tuxera.com>
Cc: Joe Perches <joe@perches.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-ntfs-dev@lists.sourceforge.net
---
 fs/ntfs/mft.c     | 12 ++++++------
 fs/ntfs/namei.c   |  2 +-
 fs/ntfs/runlist.c |  2 +-
 fs/ntfs/super.c   |  2 +-
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/fs/ntfs/mft.c b/fs/ntfs/mft.c
index 20c841a906f2..3aac5c917afe 100644
--- a/fs/ntfs/mft.c
+++ b/fs/ntfs/mft.c
@@ -71,7 +71,7 @@ static inline MFT_RECORD *map_mft_record_page(ntfs_inode *ni)
 	}
 	/* Read, map, and pin the page. */
 	page = ntfs_map_page(mft_vi->i_mapping, index);
-	if (likely(!IS_ERR(page))) {
+	if (!IS_ERR(page)) {
 		/* Catch multi sector transfer fixup errors. */
 		if (likely(ntfs_is_mft_recordp((le32*)(page_address(page) +
 				ofs)))) {
@@ -154,7 +154,7 @@ MFT_RECORD *map_mft_record(ntfs_inode *ni)
 	mutex_lock(&ni->mrec_lock);
 
 	m = map_mft_record_page(ni);
-	if (likely(!IS_ERR(m)))
+	if (!IS_ERR(m))
 		return m;
 
 	mutex_unlock(&ni->mrec_lock);
@@ -271,7 +271,7 @@ MFT_RECORD *map_extent_mft_record(ntfs_inode *base_ni, MFT_REF mref,
 		m = map_mft_record(ni);
 		/* map_mft_record() has incremented this on success. */
 		atomic_dec(&ni->count);
-		if (likely(!IS_ERR(m))) {
+		if (!IS_ERR(m)) {
 			/* Verify the sequence number. */
 			if (likely(le16_to_cpu(m->sequence_number) == seq_no)) {
 				ntfs_debug("Done 1.");
@@ -1303,7 +1303,7 @@ static int ntfs_mft_bitmap_extend_allocation_nolock(ntfs_volume *vol)
 	read_unlock_irqrestore(&mftbmp_ni->size_lock, flags);
 	rl = ntfs_attr_find_vcn_nolock(mftbmp_ni,
 			(ll - 1) >> vol->cluster_size_bits, NULL);
-	if (unlikely(IS_ERR(rl) || !rl->length || rl->lcn < 0)) {
+	if (IS_ERR(rl) || unlikely(!rl->length || rl->lcn < 0)) {
 		up_write(&mftbmp_ni->runlist.lock);
 		ntfs_error(vol->sb, "Failed to determine last allocated "
 				"cluster of mft bitmap attribute.");
@@ -1734,7 +1734,7 @@ static int ntfs_mft_data_extend_allocation_nolock(ntfs_volume *vol)
 	read_unlock_irqrestore(&mft_ni->size_lock, flags);
 	rl = ntfs_attr_find_vcn_nolock(mft_ni,
 			(ll - 1) >> vol->cluster_size_bits, NULL);
-	if (unlikely(IS_ERR(rl) || !rl->length || rl->lcn < 0)) {
+	if (IS_ERR(rl) || unlikely(!rl->length || rl->lcn < 0)) {
 		up_write(&mft_ni->runlist.lock);
 		ntfs_error(vol->sb, "Failed to determine last allocated "
 				"cluster of mft data attribute.");
@@ -1776,7 +1776,7 @@ static int ntfs_mft_data_extend_allocation_nolock(ntfs_volume *vol)
 	do {
 		rl2 = ntfs_cluster_alloc(vol, old_last_vcn, nr, lcn, MFT_ZONE,
 				true);
-		if (likely(!IS_ERR(rl2)))
+		if (!IS_ERR(rl2))
 			break;
 		if (PTR_ERR(rl2) != -ENOSPC || nr == min_nr) {
 			ntfs_error(vol->sb, "Failed to allocate the minimal "
diff --git a/fs/ntfs/namei.c b/fs/ntfs/namei.c
index 2d3cc9e3395d..4e6a44bc654c 100644
--- a/fs/ntfs/namei.c
+++ b/fs/ntfs/namei.c
@@ -115,7 +115,7 @@ static struct dentry *ntfs_lookup(struct inode *dir_ino, struct dentry *dent,
 		dent_ino = MREF(mref);
 		ntfs_debug("Found inode 0x%lx. Calling ntfs_iget.", dent_ino);
 		dent_inode = ntfs_iget(vol->sb, dent_ino);
-		if (likely(!IS_ERR(dent_inode))) {
+		if (!IS_ERR(dent_inode)) {
 			/* Consistency check. */
 			if (is_bad_inode(dent_inode) || MSEQNO(mref) ==
 					NTFS_I(dent_inode)->seq_no ||
diff --git a/fs/ntfs/runlist.c b/fs/ntfs/runlist.c
index 508744a93180..97932fb5179c 100644
--- a/fs/ntfs/runlist.c
+++ b/fs/ntfs/runlist.c
@@ -951,7 +951,7 @@ runlist_element *ntfs_mapping_pairs_decompress(const ntfs_volume *vol,
 	}
 	/* Now combine the new and old runlists checking for overlaps. */
 	old_rl = ntfs_runlists_merge(old_rl, rl);
-	if (likely(!IS_ERR(old_rl)))
+	if (!IS_ERR(old_rl))
 		return old_rl;
 	ntfs_free(rl);
 	ntfs_error(vol->sb, "Failed to merge runlists.");
diff --git a/fs/ntfs/super.c b/fs/ntfs/super.c
index 29621d40f448..7dc3bc604f78 100644
--- a/fs/ntfs/super.c
+++ b/fs/ntfs/super.c
@@ -1475,7 +1475,7 @@ static bool load_and_init_usnjrnl(ntfs_volume *vol)
 	kfree(name);
 	/* Get the inode. */
 	tmp_ino = ntfs_iget(vol->sb, MREF(mref));
-	if (unlikely(IS_ERR(tmp_ino) || is_bad_inode(tmp_ino))) {
+	if (IS_ERR(tmp_ino) || unlikely(is_bad_inode(tmp_ino))) {
 		if (!IS_ERR(tmp_ino))
 			iput(tmp_ino);
 		ntfs_error(vol->sb, "Failed to load $UsnJrnl.");
-- 
2.21.0

Re: [PATCH v3 09/11] Input: alps - remove unlikely() from IS_ERR*() condition

[Dmitry Torokhov]

On Thu, Aug 29, 2019 at 07:50:23PM +0300, Denis Efremov wrote:
> "unlikely(IS_ERR_OR_NULL(x))" is excessive. IS_ERR_OR_NULL() already uses
> unlikely() internally.

The keyword here is _internally_.

https://lore.kernel.org/lkml/20190821174857.GD76194@dtor-ws/

So please no.

> 
> Signed-off-by: Denis Efremov <efremov@linux.com>
> Cc: "Pali Rohár" <pali.rohar@gmail.com>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Joe Perches <joe@perches.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: linux-input@vger.kernel.org
> ---
>  drivers/input/mouse/alps.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> index 34700eda0429..ed1661434899 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -1476,7 +1476,7 @@ static void alps_report_bare_ps2_packet(struct psmouse *psmouse,
>  		/* On V2 devices the DualPoint Stick reports bare packets */
>  		dev = priv->dev2;
>  		dev2 = psmouse->dev;
> -	} else if (unlikely(IS_ERR_OR_NULL(priv->dev3))) {
> +	} else if (IS_ERR_OR_NULL(priv->dev3)) {
>  		/* Register dev3 mouse if we received PS/2 packet first time */
>  		if (!IS_ERR(priv->dev3))
>  			psmouse_queue_work(psmouse, &priv->dev3_register_work,
> -- 
> 2.21.0
> 

-- 
Dmitry

Re: [PATCH v3 07/11] xfs: remove unlikely() from WARN_ON() condition

[Darrick J. Wong]

On Thu, Aug 29, 2019 at 07:50:21PM +0300, Denis Efremov wrote:
> "unlikely(WARN_ON(x))" is excessive. WARN_ON() already uses unlikely()
> internally.
> 
> Signed-off-by: Denis Efremov <efremov@linux.com>
> Cc: "Darrick J. Wong" <darrick.wong@oracle.com>
> Cc: Joe Perches <joe@perches.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: linux-xfs@vger.kernel.org

LGTM,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_buf.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index ca0849043f54..4389d87ff0f0 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -2096,7 +2096,7 @@ xfs_verify_magic(
>  	int			idx;
>  
>  	idx = xfs_sb_version_hascrc(&mp->m_sb);
> -	if (unlikely(WARN_ON(!bp->b_ops || !bp->b_ops->magic[idx])))
> +	if (WARN_ON(!bp->b_ops || !bp->b_ops->magic[idx]))
>  		return false;
>  	return dmagic == bp->b_ops->magic[idx];
>  }
> @@ -2114,7 +2114,7 @@ xfs_verify_magic16(
>  	int			idx;
>  
>  	idx = xfs_sb_version_hascrc(&mp->m_sb);
> -	if (unlikely(WARN_ON(!bp->b_ops || !bp->b_ops->magic16[idx])))
> +	if (WARN_ON(!bp->b_ops || !bp->b_ops->magic16[idx]))
>  		return false;
>  	return dmagic == bp->b_ops->magic16[idx];
>  }
> -- 
> 2.21.0
> 

Re: [PATCH v3 03/11] net/mlx5e: Remove unlikely() from WARN*() condition

[Saeed Mahameed]

On Thu, 2019-08-29 at 19:50 +0300, Denis Efremov wrote:
> "unlikely(WARN_ON_ONCE(x))" is excessive. WARN_ON_ONCE() already uses
> unlikely() internally.
> 
> Signed-off-by: Denis Efremov <efremov@linux.com>
> Cc: Boris Pismenny <borisp@mellanox.com>
> Cc: Saeed Mahameed <saeedm@mellanox.com>
> Cc: Leon Romanovsky <leon@kernel.org>
> Cc: Joe Perches <joe@perches.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: netdev@vger.kernel.org
> ---
>  drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git
> a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
> b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
> index 7833ddef0427..e5222d17df35 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
> @@ -408,7 +408,7 @@ struct sk_buff *mlx5e_ktls_handle_tx_skb(struct
> net_device *netdev,
>  		goto out;
>  
>  	tls_ctx = tls_get_ctx(skb->sk);
> -	if (unlikely(WARN_ON_ONCE(tls_ctx->netdev != netdev)))
> +	if (WARN_ON_ONCE(tls_ctx->netdev != netdev))
>  		goto err_out;
>  
>  	priv_tx = mlx5e_get_ktls_tx_priv_ctx(tls_ctx);

Acked-by: Saeed Mahameed <saeedm@mellanox.com>

Dave, you can take this one.

Re: [PATCH v3 07/11] xfs: remove unlikely() from WARN_ON() condition

[Christoph Hellwig]

On Thu, Aug 29, 2019 at 07:50:21PM +0300, Denis Efremov wrote:
> "unlikely(WARN_ON(x))" is excessive. WARN_ON() already uses unlikely()
> internally.

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v3 03/11] net/mlx5e: Remove unlikely() from WARN*() condition

[David Miller]

From: Saeed Mahameed <saeedm@mellanox.com>
Date: Thu, 29 Aug 2019 21:23:30 +0000

> On Thu, 2019-08-29 at 19:50 +0300, Denis Efremov wrote:
>> "unlikely(WARN_ON_ONCE(x))" is excessive. WARN_ON_ONCE() already uses
>> unlikely() internally.
>> 
>> Signed-off-by: Denis Efremov <efremov@linux.com>
>> Cc: Boris Pismenny <borisp@mellanox.com>
>> Cc: Saeed Mahameed <saeedm@mellanox.com>
>> Cc: Leon Romanovsky <leon@kernel.org>
>> Cc: Joe Perches <joe@perches.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: netdev@vger.kernel.org
>> ---
>>  drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git
>> a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
>> b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
>> index 7833ddef0427..e5222d17df35 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c
>> @@ -408,7 +408,7 @@ struct sk_buff *mlx5e_ktls_handle_tx_skb(struct
>> net_device *netdev,
>>  		goto out;
>>  
>>  	tls_ctx = tls_get_ctx(skb->sk);
>> -	if (unlikely(WARN_ON_ONCE(tls_ctx->netdev != netdev)))
>> +	if (WARN_ON_ONCE(tls_ctx->netdev != netdev))
>>  		goto err_out;
>>  
>>  	priv_tx = mlx5e_get_ktls_tx_priv_ctx(tls_ctx);
> 
> Acked-by: Saeed Mahameed <saeedm@mellanox.com>
> 
> Dave, you can take this one.

Ok, applied to net-next as well as the UDP patch.

Re: [PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls

[Markus Elfring]

> +# nested likely/unlikely calls
> +		if ($line =~ /\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) {
> +			WARN("LIKELY_MISUSE",

How do you think about to use the specification “(?:IS_ERR(?:_(?:OR_NULL|VALUE))?|WARN)”
in this regular expression?

Regards,
Markus

Re: [PATCH v3 06/11] wimax/i2400m: remove unlikely() from WARN*() condition

[Markus Elfring]

>  		pad_buf = i2400m_tx_fifo_push(i2400m, padding, 0, 0);
> -		if (unlikely(WARN_ON(pad_buf == NULL
> -				     || pad_buf == TAIL_FULL))) {
> +		if (WARN_ON(pad_buf == NULL || pad_buf == TAIL_FULL)) {

How do you think about to use the following code variant?

+		if (WARN_ON(!pad_buf || pad_buf == TAIL_FULL)) {


Regards,
Markus

Re: [PATCH v3 09/11] Input: alps - remove unlikely() from IS_ERR*() condition

[Pali Rohár]

[-- Attachment #1: Type: text/plain, Size: 1652 bytes --]

On Thursday 29 August 2019 10:50:39 Dmitry Torokhov wrote:
> On Thu, Aug 29, 2019 at 07:50:23PM +0300, Denis Efremov wrote:
> > "unlikely(IS_ERR_OR_NULL(x))" is excessive. IS_ERR_OR_NULL() already uses
> > unlikely() internally.
> 
> The keyword here is _internally_.
> 
> https://lore.kernel.org/lkml/20190821174857.GD76194@dtor-ws/
> 
> So please no.

Dmitry and I already rejected this patch, see also linked-list:
https://lore.kernel.org/lkml/20190820111719.7blyk5jstgwde2ae@pali/

> > 
> > Signed-off-by: Denis Efremov <efremov@linux.com>
> > Cc: "Pali Rohár" <pali.rohar@gmail.com>
> > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > Cc: Joe Perches <joe@perches.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: linux-input@vger.kernel.org
> > ---
> >  drivers/input/mouse/alps.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> > index 34700eda0429..ed1661434899 100644
> > --- a/drivers/input/mouse/alps.c
> > +++ b/drivers/input/mouse/alps.c
> > @@ -1476,7 +1476,7 @@ static void alps_report_bare_ps2_packet(struct psmouse *psmouse,
> >  		/* On V2 devices the DualPoint Stick reports bare packets */
> >  		dev = priv->dev2;
> >  		dev2 = psmouse->dev;
> > -	} else if (unlikely(IS_ERR_OR_NULL(priv->dev3))) {
> > +	} else if (IS_ERR_OR_NULL(priv->dev3)) {
> >  		/* Register dev3 mouse if we received PS/2 packet first time */
> >  		if (!IS_ERR(priv->dev3))
> >  			psmouse_queue_work(psmouse, &priv->dev3_register_work,
> > -- 
> > 2.21.0
> > 
> 

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH v3 09/11] Input: alps - remove unlikely() from IS_ERR*() condition

[Denis Efremov]

On 31.08.2019 18:25, Pali Rohár wrote:
> On Thursday 29 August 2019 10:50:39 Dmitry Torokhov wrote:
>> On Thu, Aug 29, 2019 at 07:50:23PM +0300, Denis Efremov wrote:
>>> "unlikely(IS_ERR_OR_NULL(x))" is excessive. IS_ERR_OR_NULL() already uses
>>> unlikely() internally.
>>
>> The keyword here is _internally_.
>>
>> https://lore.kernel.org/lkml/20190821174857.GD76194@dtor-ws/
>>
>> So please no.
> 
> Dmitry and I already rejected this patch, see also linked-list:
> https://lore.kernel.org/lkml/20190820111719.7blyk5jstgwde2ae@pali/
>

Looks like this is a very long recurring story with this patch.
Thanks, for the clarification.

Regards,
Denis

Re: [PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls

[Denis Efremov]

On 31.08.2019 12:15, Markus Elfring wrote:
>> +# nested likely/unlikely calls
>> +        if ($line =~ /\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) {
>> +            WARN("LIKELY_MISUSE",
> 
> How do you think about to use the specification “(?:IS_ERR(?:_(?:OR_NULL|VALUE))?|WARN)”
> in this regular expression?

Hmm, 
(?:   <- Catch group is required here, since it is used in diagnostic message,
         see $1
   IS_ERR
   (?:_ <- Another atomic group just to show that '_' is a common prefix?
           I'm not sure about this. Usually, Perl interpreter is very good at optimizing such things.
           You could see this optimization if you run perl with -Mre=debug.
     (?:OR_NULL|VALUE))?|WARN)

Regards,
Denis

Re: [PATCH v3 06/11] wimax/i2400m: remove unlikely() from WARN*() condition

[Denis Efremov]

On 31.08.2019 14:25, Markus Elfring wrote:
>>          pad_buf = i2400m_tx_fifo_push(i2400m, padding, 0, 0);
>> -        if (unlikely(WARN_ON(pad_buf == NULL
>> -                     || pad_buf == TAIL_FULL))) {
>> +        if (WARN_ON(pad_buf == NULL || pad_buf == TAIL_FULL)) {
> 
> How do you think about to use the following code variant?
> 
> +        if (WARN_ON(!pad_buf || pad_buf == TAIL_FULL)) {
> 

Well, I thought about it, because coccinelle warns about style here.
But this condition looks more symmetric with direct comparison.
I've decided that it will be better to save the original style.

Thanks,
Denis

Re: [PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls

[Markus Elfring]

>>> +# nested likely/unlikely calls
>>> +        if ($line =~ /\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) {
>>> +            WARN("LIKELY_MISUSE",
>>
>> How do you think about to use the specification “(?:IS_ERR(?:_(?:OR_NULL|VALUE))?|WARN)”
>> in this regular expression?
…
>    IS_ERR
>    (?:_ <- Another atomic group just to show that '_' is a common prefix?

Yes. - I hope that this specification detail can help a bit.


>            Usually, Perl interpreter is very good at optimizing such things.

Would you like to help this software component by omitting a pair of
non-capturing parentheses at the beginning?

\b(?:un)?likely\s*


Regards,
Markus

Re: [PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls

[Denis Efremov]

On 31.08.2019 19:45, Markus Elfring wrote:
>>>> +# nested likely/unlikely calls
>>>> +        if ($line =~ /\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) {
>>>> +            WARN("LIKELY_MISUSE",
>>>
>>> How do you think about to use the specification “(?:IS_ERR(?:_(?:OR_NULL|VALUE))?|WARN)”
>>> in this regular expression?
> …
>>    IS_ERR
>>    (?:_ <- Another atomic group just to show that '_' is a common prefix?
> 
> Yes. - I hope that this specification detail can help a bit.

I'm not sure that another pair of brackets for a single char worth it.

>>            Usually, Perl interpreter is very good at optimizing such things.

The interpreter optimizes it internally:
echo 'IS_ERR_OR_NULL' | perl -Mre=debug -ne '/IS_ERR(?:_OR_NULL|_VALUE)?/ && print'
Compiling REx "IS_ERR(?:_OR_NULL|_VALUE)?"
Final program:
   1: EXACT <IS_ERR> (4)
   4: CURLYX[0]{0,1} (16)
   6:   EXACT <_> (8)      <-- common prefix
   8:   TRIE-EXACT[OV] (15)
        <OR_NULL> 
        <VALUE>
...
> 
> Would you like to help this software component by omitting a pair of
> non-capturing parentheses at the beginning?
> 
> \b(?:un)?likely\s*

This pair of brackets is required to match "unlikely" and it's
optional in order to match "likely".

Regards,
Denis

Re: [PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls

[Markus Elfring]

>>>>> +# nested likely/unlikely calls
>>>>> +        if ($line =~ /\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) {
>>>>> +            WARN("LIKELY_MISUSE",
…
>> \b(?:un)?likely\s*
>
> This pair of brackets is required to match "unlikely"
> and it's optional in order to match "likely".

I agree also to this view if you refer to the shortened regular expression here.
But I got an other development opinion for an extra pair of non-capturing parentheses
at the front (from the version which you suggested).

Regards,
Markus

Re: [PATCH v3 09/11] Input: alps - remove unlikely() from IS_ERR*() condition

[Joe Perches]

On Sat, 2019-08-31 at 17:25 +0200, Pali Rohár wrote:
> On Thursday 29 August 2019 10:50:39 Dmitry Torokhov wrote:
> > On Thu, Aug 29, 2019 at 07:50:23PM +0300, Denis Efremov wrote:
> > > "unlikely(IS_ERR_OR_NULL(x))" is excessive. IS_ERR_OR_NULL() already uses
> > > unlikely() internally.
> > 
> > The keyword here is _internally_.
> > 
> > https://lore.kernel.org/lkml/20190821174857.GD76194@dtor-ws/
> > 
> > So please no.

I think it poor form not to simply restate your original
objection from 4 message levels below this link

https://lists.gt.net/linux/kernel/2269724

   Hm... I do not like this change. If I read code 
    
    if (unlikely(IS_ERR_OR_NULL(priv->dev3))) 
    
   then I know that it is really unlikely that condition will be truth and 
   so this is some case of error/exception or something that normally does 
   not happen too much. 
    
   But if I read code 
    
    if (IS_ERR_OR_NULL(priv->dev3)) 
    
   I know nothing about chance that this condition will be truth. Explicit 
   unlikely in previous example give me more information. 
    
I alslo think this argument is dubious as it also applies
to any IS_ERR and all the unlikely uses have been removed
from those.

Re: [PATCH v3 09/11] Input: alps - remove unlikely() from IS_ERR*() condition

[Dmitry Torokhov]

On Sat, Aug 31, 2019 at 01:32:02PM -0700, Joe Perches wrote:
> On Sat, 2019-08-31 at 17:25 +0200, Pali Rohár wrote:
> > On Thursday 29 August 2019 10:50:39 Dmitry Torokhov wrote:
> > > On Thu, Aug 29, 2019 at 07:50:23PM +0300, Denis Efremov wrote:
> > > > "unlikely(IS_ERR_OR_NULL(x))" is excessive. IS_ERR_OR_NULL() already uses
> > > > unlikely() internally.
> > > 
> > > The keyword here is _internally_.
> > > 
> > > https://lore.kernel.org/lkml/20190821174857.GD76194@dtor-ws/
> > > 
> > > So please no.
> 
> I think it poor form not to simply restate your original
> objection from 4 message levels below this link

Thank you for the lesson in etiquette, but I posted reference to the
very message I wanted.

> 
> https://lists.gt.net/linux/kernel/2269724
> 
>    Hm... I do not like this change. If I read code 
>     
>     if (unlikely(IS_ERR_OR_NULL(priv->dev3))) 
>     
>    then I know that it is really unlikely that condition will be truth and 
>    so this is some case of error/exception or something that normally does 
>    not happen too much. 
>     
>    But if I read code 
>     
>     if (IS_ERR_OR_NULL(priv->dev3)) 
>     
>    I know nothing about chance that this condition will be truth. Explicit 
>    unlikely in previous example give me more information. 
>     
> I alslo think this argument is dubious as it also applies
> to any IS_ERR and all the unlikely uses have been removed
> from those.

No, if you read the reference I posted, the argument does not apply to
all IS_ERR() instances. Majority of them are in probe() paths where we
do not really care about likely/unlikely. Here we are dealing with
IS_ERR in a [fairly] hot path.

Thanks.

-- 
Dmitry

Re: [PATCH v3 02/11] drm/msm: remove unlikely() from WARN_ON() conditions

[Bjorn Andersson]

On Thu 29 Aug 09:50 PDT 2019, Denis Efremov wrote:

> "unlikely(WARN_ON(x))" is excessive. WARN_ON() already uses unlikely()
> internally.
> 
> Signed-off-by: Denis Efremov <efremov@linux.com>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: Joe Perches <joe@perches.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: linux-arm-msm@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

> ---
>  drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c | 4 ++--
>  drivers/gpu/drm/msm/disp/mdp_format.c    | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c
> index 4804cf40de14..030279d7b64b 100644
> --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c
> +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c
> @@ -253,7 +253,7 @@ int mdp5_ctl_set_cursor(struct mdp5_ctl *ctl, struct mdp5_pipeline *pipeline,
>  	u32 blend_cfg;
>  	struct mdp5_hw_mixer *mixer = pipeline->mixer;
>  
> -	if (unlikely(WARN_ON(!mixer))) {
> +	if (WARN_ON(!mixer)) {
>  		DRM_DEV_ERROR(ctl_mgr->dev->dev, "CTL %d cannot find LM",
>  			ctl->id);
>  		return -EINVAL;
> @@ -695,7 +695,7 @@ struct mdp5_ctl_manager *mdp5_ctlm_init(struct drm_device *dev,
>  		goto fail;
>  	}
>  
> -	if (unlikely(WARN_ON(ctl_cfg->count > MAX_CTL))) {
> +	if (WARN_ON(ctl_cfg->count > MAX_CTL)) {
>  		DRM_DEV_ERROR(dev->dev, "Increase static pool size to at least %d\n",
>  				ctl_cfg->count);
>  		ret = -ENOSPC;
> diff --git a/drivers/gpu/drm/msm/disp/mdp_format.c b/drivers/gpu/drm/msm/disp/mdp_format.c
> index 8afb0f9c04bb..5495d8b3f5b9 100644
> --- a/drivers/gpu/drm/msm/disp/mdp_format.c
> +++ b/drivers/gpu/drm/msm/disp/mdp_format.c
> @@ -174,7 +174,7 @@ const struct msm_format *mdp_get_format(struct msm_kms *kms, uint32_t format,
>  
>  struct csc_cfg *mdp_get_default_csc_cfg(enum csc_type type)
>  {
> -	if (unlikely(WARN_ON(type >= CSC_MAX)))
> +	if (WARN_ON(type >= CSC_MAX))
>  		return NULL;
>  
>  	return &csc_convert[type];
> -- 
> 2.21.0
>