linux-kernel - Re: [v7,1/1] i2c: add master driver for mellanox systems

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: <20161117221522.GA10875@katana>
Date:   Thu, 17 Nov 2016 23:15:22 +0100
From:   Wolfram Sang <wsa-dev@...g-engineering.com>
To:     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?

> 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?

Thanks,

   Wolfram

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