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]

Date:   Thu, 17 Feb 2022 15:39:37 +0800
From:   Matt Johnston <matt@...econstruct.com.au>
To:     Wolfram Sang <wsa@...nel.org>
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 Wolfram,

On Wed, 2022-02-16 at 17:15 +0100, Wolfram Sang wrote:
> 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.

I'll tidy up the comments. A filled /* first line is part of the netdev
style.
> 
> 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.

I think 'slave' might be a bit unclear - the driver's acting as an I2C master
too. It also is more baggage moving to inclusive naming. Maybe mctp-i2c-
transport or mctp-i2c-interface would suit?

Cheers,
Matt