netdev - Re: [PATCH net-next v5 2/2] mctp i2c: MCTP I2C binding driver

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]

Message-ID: <Yg0jMkt56EhrBybc@ninjato>
Date:   Wed, 16 Feb 2022 17:15:46 +0100
From:   Wolfram Sang <wsa@...nel.org>
To:     Matt Johnston <matt@...econstruct.com.au>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Jeremy Kerr <jk@...econstruct.com.au>,
        linux-i2c@...r.kernel.org, netdev@...r.kernel.org,
        Zev Weiss <zev@...ilderbeest.net>
Subject: Re: [PATCH net-next v5 2/2] mctp i2c: MCTP I2C binding driver

Hi Matt, all,

On Thu, Feb 10, 2022 at 02:36:51PM +0800, Matt Johnston wrote:
> Provides MCTP network transport over an I2C bus, as specified in
> DMTF DSP0237. All messages between nodes are sent as SMBus Block Writes.
> 
> Each I2C bus to be used for MCTP is flagged in devicetree by a
> 'mctp-controller' property on the bus node. Each flagged bus gets a
> mctpi2cX net device created based on the bus number. A
> 'mctp-i2c-controller' I2C client needs to be added under the adapter. In
> an I2C mux situation the mctp-i2c-controller node must be attached only
> to the root I2C bus. The I2C client will handle incoming I2C slave block
> write data for subordinate busses as well as its own bus.
> 
> In configurations without devicetree a driver instance can be attached
> to a bus using the I2C slave new_device mechanism.
> 
> The MCTP core will hold/release the MCTP I2C device while responses
> are pending (a 6 second timeout or once a socket is closed, response
> received etc). While held the MCTP I2C driver will lock the I2C bus so
> that the correct I2C mux remains selected while responses are received.
> 
> (Ideally we would just lock the mux to keep the current bus selected for
> the response rather than a full I2C bus lock, but that isn't exposed in
> the I2C mux API)
> 
> Signed-off-by: Matt Johnston <matt@...econstruct.com.au>
> Signed-off-by: Jeremy Kerr <jk@...econstruct.com.au>

So, I did a high level review regardings the I2C stuff. I did not check
locking, device lifetime, etc. My biggest general remark is the mixture
of multi-comment styles, like C++ style or no empty "/*" at the
beginning as per Kernel coding style. Some functions have nice
explanations in the header but not proper kdoc formatting. And also on
the nitbit side, I don't think '__func__' helps here on the error
messages. But that's me, I'll leave it to the netdev maintainers.

Now for the I2C part. It looks good. I have only one remark:

> +static const struct i2c_device_id mctp_i2c_id[] = {
> +	{ "mctp-i2c", 0 },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(i2c, mctp_i2c_id);

...

> +static struct i2c_driver mctp_i2c_driver = {
> +	.driver = {
> +		.name = "mctp-i2c",
> +		.of_match_table = mctp_i2c_of_match,
> +	},
> +	.probe_new = mctp_i2c_probe,
> +	.remove = mctp_i2c_remove,
> +	.id_table = mctp_i2c_id,
> +};

I'd suggest to add 'slave' to the 'mctp-i2c' string somewhere to make it
easily visible that this driver does not manage a remote device but
processes requests to its own address.

Thanks for the work!

   Wolfram


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