linux-kernel - Re: [PATCH 2/2] drivers: input: mouse: alps: drop unneeded likely() call around IS

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 20 Aug 2019 16:22:04 +0200
From:   Pali Rohár <pali.rohar@...il.com>
To:     "Enrico Weigelt, metux IT consult" <lkml@...ux.net>
Cc:     "Enrico Weigelt, metux IT consult" <info@...ux.net>,
        linux-kernel@...r.kernel.org, dmitry.torokhov@...il.com,
        linux-input@...r.kernel.org, linux-ntfs-dev@...ts.sourceforge.net
Subject: Re: [PATCH 2/2] drivers: input: mouse: alps: drop unneeded likely()
 call around IS_ERR()

On Tuesday 20 August 2019 14:21:33 Enrico Weigelt, metux IT consult wrote:
> On 20.08.19 13:17, Pali Rohár wrote:
> > On Tuesday 20 August 2019 12:56:12 Enrico Weigelt, metux IT consult wrote:
> > > From: Enrico Weigelt <info@...ux.net>
> > > 
> > > IS_ERR() already calls unlikely(), so this extra unlikely() call
> > > around IS_ERR() is not needed.
> > > 
> > > Signed-off-by: Enrico Weigelt <info@...ux.net>
> > > ---
> > >   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 34700ed..ed16614 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,
> > 
> > Hello! Two months ago I already saw this patch. See discussion:
> > https://patchwork.kernel.org/patch/10977099/
> 
> phuh, that's long chain of links to folow ;-)
> 
> So, your primary argument is just *documenting* that this supposed to
> be a rare condition ?

Yes. Also Dmitry said that prefer explicit unlikely.

> In that case, wouldn't a comment be more suitable for that ?

And why to add comment if current state of code is more-readable and
does not need it?

I do not see anything wrong on this code path.

People normally add comments to code which is problematic to understand
or is somehow tricky, no so obvious or document how should code behave.

> It seems that this issue is coming up again and again ... when people
> run cocci scans (like I didn't), I'd suspect this to happen even more
> in the future.

People first need to understand that static code analysis cannot work
100% reliably and always is needed to properly review changes.

And if the only reason for this change is to satisfy some static code
analysis program, would not it better to teach this program to no
generate such false-positive results?

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