LKML Archive on
help / color / mirror / Atom feed
From: Wolfram Sang <>
To: Peter Rosin <>
Cc: Wenwen Wang <>, Kangjie Lu <>,
	"open list:I2C SUBSYSTEM" <>,
	open list <>
Subject: Re: [PATCH v2 2/2] i2c: core-smbus: fix a potential missing-check bug
Date: Thu, 17 May 2018 15:06:54 +0200	[thread overview]
Message-ID: <20180517130653.vp3sw7duqz6ygazk@ninjato> (raw)
In-Reply-To: <>

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

> > hopefully I have time to write a small coccinelle rule to find if
> > constant values are returned in a function declared as master_xfer.
> That would be a good thing.

Did that now and only found drivers which have a (meanwhile) needless
parameter check for 'num'. Will set you on CC for those fixes. I didn't
find any drivers incorrectly returning 0 instead of num. Except the ones
you already fixed.

> Maybe a long term goal is to simply return
> zero on success for .master_xfer, because currently the only expected
> success value is the number of messages sent, but the caller is in all
> likelihood already aware of that count, so it all seems rather like
> something that is just pointless and easy to get wrong...

Yes, the comment in the core is still true:

        /* REVISIT the fault reporting model here is weak:
         * ...

Returning 0 or -ERRNO sounds best to me, too. But we would need to make
sure there is no in-kernel user relying on the current behaviour. As you
said, this is a long-term goal at best.

> > What do you mean with "short success for some sequence" here?
> By "short" I mean not all requested messages transferred. By "success"
> I mean non-negative.

Ah, I understand now, thanks.

> I.e. when I look at rcar_i2c_master_xfer(), it sets things up for all
> messages to be transferred, starts things off and waits for completion
> (or timeout). But the driver is too involved for it to be easy to say
> that either all messages are transferred or a negative error is
> returned. I never managed to see that anyway.

Well, I am the maintainer of that driver, so I can say something about
that :) The HW design has a flaw for older SoCs which makes the driver
prone to a race condition. This is why we set up new messages in
interrupt context, too. Everything else turned out to be too expensive.

> And the function has that "ret = num - priv->msgs_left;" statement
> indicating that whomever wrote it thought it perfectly ok to return
> such a "short success" (which noone is expecting).

I copied over that old behaviour when refactoring the driver. But I see
what you mean, and couldn't also really see a path where a "short
success" could actually happen.



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

  reply	other threads:[~2018-05-17 13:06 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-05 13:02 Wenwen Wang
2018-05-10 11:16 ` Wolfram Sang
2018-05-10 13:36   ` Peter Rosin
2018-05-15  8:58     ` Wolfram Sang
2018-05-15 10:36       ` Peter Rosin
2018-05-17 13:06         ` Wolfram Sang [this message]
2018-05-17 13:38   ` Wolfram Sang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180517130653.vp3sw7duqz6ygazk@ninjato \ \ \ \ \ \ \
    --subject='Re: [PATCH v2 2/2] i2c: core-smbus: fix a potential missing-check bug' \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).