linux-kernel - Re: [RFC Patch v3 1/3] i2c debug counters as sysfs attributes

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] [day] [month] [year] [list]

Message-ID: <YgEYEk355t8C4J1x@shikoro>
Date:   Mon, 7 Feb 2022 14:01:06 +0100
From:   Wolfram Sang <wsa@...nel.org>
To:     Sui Chen <suichen@...gle.com>
Cc:     linux-kernel@...r.kernel.org, linux-i2c@...r.kernel.org,
        openbmc@...ts.ozlabs.org, joel@....id.au, andrew@...id.au,
        tali.perry1@...il.com, benjaminfair@...gle.com, krellan@...gle.com,
        kernel test robot <lkp@...el.com>
Subject: Re: [RFC Patch v3 1/3] i2c debug counters as sysfs attributes

Hi,

I finally had some time to look at your proposal. As I wrote last time,
you convinced me to have the stats in sysfs for apple-to-apple
comparisons.

One change I'd like to see is to let the I2C core handle the stats and
not the individual bus drivers. From what I see, the I2C core could
handle all this if the bus drivers use proper fault codes.

> - ber_cnt (bus error count)

I'm not sure what exactly "bus error" means in this case. But I think it
can be translated to any not-otherwise handled errno returned by
__i2c_transfer() or __i2c_smbus_transfer(). I also think it should be
named "bus_errors". Do we really need the "cnt" suffix?

> - nack_cnt (NACK count)

This would be -ENXIO for __i2c_transfer and friends. Name should be
"NACKs"?

> - rec_fail_cnt, rec_succ_cnt (recovery failure/success count)

This would be the return code of i2c_recover_bus(). Names should be
"recovery_failures" and "recovery_successes"?

> - timeout_cnt (timeout count)

This would be -ETIMEDOUT for __i2c_transfer and friends. Name should be
"timeouts"?

> - i2c_speed (bus frequency)

Yes, we can have that. I don't think this is really a stat, though. It
is an attribute of an adapter. It has been requested before:

http://patchwork.ozlabs.org/project/linux-i2c/patch/1413403411-8895-4-git-send-email-octavian.purdila@intel.com/
http://patchwork.ozlabs.org/project/linux-i2c/patch/20181210084111.6938-2-tudor.ambarus@microchip.com/
http://patchwork.ozlabs.org/project/linux-i2c/patch/20201013100314.216154-1-tali.perry1@gmail.com/

So, I think we can tackle it again but it is orthogonal from the stats
series.

> - tx_complete_cnt (transaction completed, including both as an initiator
>   and as a target)

This would be retval == num_msgs for __i2c_transfer and friends. I also
think it should be named "transfers_completed". "tx" often goes with
"rx" as a pair. I really wondered "why only tx" first.

So, let's keep at the high level first. What do you think about my
suggestions?

Thanks and happy hacking,

   Wolfram

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)