| 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
| ||
|
Date: Fri, 18 Nov 2016 02:50:32 +0000
From: Vadim Pasternak <vadimp@...lanox.com>
To: Wolfram Sang <wsa-dev@...g-engineering.com>
CC: "wsa@...-dreams.de" <wsa@...-dreams.de>,
"linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"jiri@...nulli.us" <jiri@...nulli.us>,
Michael Shych <michaelsh@...lanox.com>
Subject: RE: [v7,1/1] i2c: add master driver for mellanox systems
Hi Wolfram,
Thank you for review.
> -----Original Message-----
> From: Wolfram Sang [mailto:wsa-dev@...g-engineering.com]
> Sent: Friday, November 18, 2016 12:15 AM
> To: Vadim Pasternak <vadimp@...lanox.com>
> Cc: wsa@...-dreams.de; linux-i2c@...r.kernel.org; linux-
> kernel@...r.kernel.org; jiri@...nulli.us; Michael Shych
> <michaelsh@...lanox.com>
> Subject: Re: [v7,1/1] i2c: add master driver for mellanox systems
>
> Hi,
>
> thanks for the patch and the prompt reworks. Also thank you to Vladimir for
> initial reviews!
>
> > Device supports:
> > - Master mode
> > - One physical bus
> > - Polling mode
>
> Out of interest: is there any interrupt at all?
Yes, it's possible to configure interrupt mode in HW, but we've never used it.
>
> > diff --git a/MAINTAINERS b/MAINTAINERS index 411e3b8..26d05f8 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -7881,6 +7881,14 @@ W: http://www.mellanox.com
> > Q: http://patchwork.ozlabs.org/project/netdev/list/
> > F: drivers/net/ethernet/mellanox/mlxsw/
> >
> > +MELLANOX MLXCPLD I2C DRIVER
> > +M: Vadim Pasternak <vadimp@...lanox.com>
> > +M: Michael Shych <michaelsh@...lanox.com>
> > +L: linux-i2c@...r.kernel.org
> > +S: Supported
> > +F: drivers/i2c/busses/i2c-mlxcpld.c
> > +F: Documentation/i2c/busses/i2c-mlxcpld
> > +
>
> No need to change right now, but I think you could simplify all your drivers in
> one entry with
>
> F: *mlxcpld*
>
> or something similar to keep MAINTAINERS compact.
>
> > +/**
> > + * struct mlxcpld_i2c_curr_xfer - current transaction parameters:
> > + * @cmd: command;
> > + * @addr_width: address width;
> > + * @data_len: data length;
> > + * @cmd: command register;
> > + * @msg_num: message number;
> > + * @msg: pointer to message buffer;
> > + */
>
> While I value effort to describe struct members, this is so self-explaining that I
> think it could be left out.
>
> > +/**
> > + * struct mlxcpld_i2c_priv - private controller data:
> > + * @adap: i2c adapter;
> > + * @base_addr: base IO address;
> > + * @lock: bus access lock;
> > + * @xfer: current i2c transfer block;
> > + * @dev: device handle;
> > + */
>
> ditto
>
> > +/*
> > + * Check validity of current i2c message and all transfer.
> > + * Calculate also common length of all i2c messages in transfer.
> > + */
> > +static int mlxcpld_i2c_invalid_len(struct mlxcpld_i2c_priv *priv,
> > + const struct i2c_msg *msg, u8 *comm_len) {
> > + u8 max_len = msg->flags == I2C_M_RD ? MLXCPLD_I2C_DATA_REG_SZ -
> > + MLXCPLD_I2C_MAX_ADDR_LEN :
> MLXCPLD_I2C_DATA_REG_SZ;
> > +
> > + if (msg->len > max_len)
> > + return -EINVAL;
>
> If you populate a 'struct i2c_adapter_quirks' the core will do this check for you.
>
> > + *comm_len += msg->len;
> > + if (*comm_len > MLXCPLD_I2C_DATA_REG_SZ)
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * Check validity of received i2c messages parameters.
> > + * Returns 0 if OK, other - in case of invalid parameters
> > + * or common length of data that should be passed to CPLD */ static
> > +int mlxcpld_i2c_check_msg_params(struct mlxcpld_i2c_priv *priv,
> > + struct i2c_msg *msgs, int num,
> > + u8 *comm_len)
> > +{
> > + int i;
> > +
> > + if (!num) {
> > + dev_err(priv->dev, "Incorrect 0 num of messages\n");
> > + return -EINVAL;
> > + }
> > +
> > + if (unlikely(msgs[0].addr > 0x7f)) {
> > + dev_err(priv->dev, "Invalid address 0x%03x\n",
> > + msgs[0].addr);
> > + return -EINVAL;
> > + }
> > +
> > + for (i = 0; i < num; ++i) {
> > + if (unlikely(!msgs[i].buf)) {
> > + dev_err(priv->dev, "Invalid buf in msg[%d]\n",
> > + i);
> > + return -EINVAL;
> > + }
>
> I was about to write "the core will check this for you", but wow, we are not
> good at this... I will try to come up with some patches soon. No need for you to
> changes this right now.
>
> ...
>
> > + case MLXCPLD_LPCI2C_ACK_IND:
> > + if (priv->xfer.cmd != I2C_M_RD)
> > + return (priv->xfer.addr_width + priv->xfer.data_len);
> > +
> > + if (priv->xfer.msg_num == 1)
> > + i = 0;
> > + else
> > + i = 1;
> > +
> > + if (!priv->xfer.msg[i].buf)
> > + return -EINVAL;
> > +
> > + /*
> > + * Actual read data len will be always the same as
> > + * requested len. 0xff (line pull-up) will be returned
> > + * if slave has no data to return. Thus don't read
> > + * MLXCPLD_LPCI2C_NUM_DAT_REG reg from CPLD.
> > + */
> > + datalen = priv->xfer.data_len;
> > +
> > + mlxcpld_i2c_read_comm(priv, MLXCPLD_LPCI2C_DATA_REG,
> > + priv->xfer.msg[i].buf, datalen);
> > +
> > + return datalen;
> > +
> > + case MLXCPLD_LPCI2C_NACK_IND:
> > + return -EAGAIN;
>
> -EAGAIN is for arbitration lost. -ENXIO is for NACK. See
> Documentation/i2c/fault-codes.
>
> What kind of testing have you done with the driver?
>
Actually we are using these driver on a wide range of different Mellanox systems (about 10 systems, TORs and modular systems), which we have on the filed about two years.
After updating it with the upstream review comments, I performed some basic set of the functional testing on a couple of our systems - access to all the devices (1, 2, 4 byte width), access to broken device (transaction TO), some kind of stress test.
> Thanks,
>
> Wolfram
Thanks,
Vadim.
Powered by blists - more mailing lists