| Message ID | 20190829165025.15750-9-efremov@linux.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [v3,01/11] checkpatch: check for nested (un)?likely() calls | expand |
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 >
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 > > >
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
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.
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.
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,
"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(-)