| 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
| ||
|
Message-ID: <00D9E632-E2D5-413B-BFF8-6791D599943D@fb.com>
Date: Wed, 13 Nov 2019 22:58:46 +0000
From: Vijay Khemka <vijaykhemka@...com>
To: "minyard@....org" <minyard@....org>
CC: Asmaa Mnebhi <Asmaa@...lanox.com>, Arnd Bergmann <arnd@...db.de>,
"Greg Kroah-Hartman" <gregkh@...uxfoundation.org>,
"openipmi-developer@...ts.sourceforge.net"
<openipmi-developer@...ts.sourceforge.net>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"cminyard@...sta.com" <cminyard@...sta.com>,
"joel@....id.au" <joel@....id.au>,
"linux-aspeed@...ts.ozlabs.org" <linux-aspeed@...ts.ozlabs.org>,
Sai Dasari <sdasari@...com>,
"wsa@...-dreams.de" <wsa@...-dreams.de>
Subject: Re: [PATCH v3] drivers: ipmi: Support raw i2c packet in IPMB
On 11/13/19, 1:47 PM, "Corey Minyard" <tcminyard@...il.com on behalf of minyard@....org> wrote:
On Wed, Nov 13, 2019 at 09:28:10PM +0000, Vijay Khemka wrote:
>
>
> On 11/13/19, 12:33 PM, "Asmaa Mnebhi" <Asmaa@...lanox.com> wrote:
>
> Inline response:
>
> -----Original Message-----
> From: Vijay Khemka <vijaykhemka@...com>
> Sent: Wednesday, November 13, 2019 2:23 PM
> To: Corey Minyard <minyard@....org>; Arnd Bergmann <arnd@...db.de>; Greg Kroah-Hartman <gregkh@...uxfoundation.org>; openipmi-developer@...ts.sourceforge.net; linux-kernel@...r.kernel.org
> Cc: vijaykhemka@...com; cminyard@...sta.com; Asmaa Mnebhi <Asmaa@...lanox.com>; joel@....id.au; linux-aspeed@...ts.ozlabs.org; sdasari@...com
> Subject: [PATCH v3] drivers: ipmi: Support raw i2c packet in IPMB
>
> Many IPMB devices doesn't support smbus protocol and current driver support only smbus devices. Added support for raw i2c packets.
>
> User can define use-i2c-block in device tree to use i2c raw transfer.
>
> Asmaa>> Fix the description: "The ipmb_dev_int driver only supports the smbus protocol at the moment. Add support for the i2c protocol as well. There will be a variable passed by though the device tree or ACPI table which sets the configures the protocol as either i2c or smbus."
>
> Signed-off-by: Vijay Khemka <vijaykhemka@...com>
> ---
> drivers/char/ipmi/ipmb_dev_int.c | 48 ++++++++++++++++++++++++++++++++
> 1 file changed, 48 insertions(+)
>
> diff --git a/drivers/char/ipmi/ipmb_dev_int.c b/drivers/char/ipmi/ipmb_dev_int.c
> index ae3bfba27526..16d5d4b636a9 100644
> --- a/drivers/char/ipmi/ipmb_dev_int.c
> +++ b/drivers/char/ipmi/ipmb_dev_int.c
> @@ -63,6 +63,7 @@ struct ipmb_dev {
> spinlock_t lock;
> wait_queue_head_t wait_queue;
> struct mutex file_mutex;
> + bool use_i2c;
> };
>
> Asmaa>> rename this variable : is_i2c_protocol
> Done.
>
> static inline struct ipmb_dev *to_ipmb_dev(struct file *file) @@ -112,6 +113,39 @@ static ssize_t ipmb_read(struct file *file, char __user *buf, size_t count,
> return ret < 0 ? ret : count;
> }
>
> +static int ipmb_i2c_write(struct i2c_client *client, u8 *msg) {
> + unsigned char *i2c_buf;
> + struct i2c_msg i2c_msg;
> + ssize_t ret;
> + u8 msg_len;
> +
> + /*
> + * subtract 1 byte (rq_sa) from the length of the msg passed to
> + * raw i2c_transfer
> + */
> + msg_len = msg[IPMB_MSG_LEN_IDX] - 1;
> +
> + i2c_buf = kzalloc(msg_len, GFP_KERNEL);
>
> Asmaa >> We do not want to use kzalloc every time you execute this write function. It would create so much fragmentation.
> You don't really need to use kzalloc anyways.
> We need to allocate memory to pass to i2c_transfer. That's what being done in i2c_smbus_xfer function as well.
>
> Also, this code chunk is short, so you can call it directly from the write function. I don't think you need a separate function for it.
> I wanted to keep this change as clean as possible.
I'd agree. Fragmentation is not a big deal here.
However, why not just pass in msg + 2? That would be cleaner, faster,
and less wasteful.
>
> + if (!i2c_buf)
> + return -EFAULT;
-ENOMEM? (Assuming you keep the malloc)
Done.
> +
> + /* Copy message to buffer except first 2 bytes (length and address) */
> + memcpy(i2c_buf, msg+2, msg_len);
> +
> + i2c_msg.addr = GET_7BIT_ADDR(msg[RQ_SA_8BIT_IDX]);
> + i2c_msg.flags = client->flags &
> + (I2C_M_TEN | I2C_CLIENT_PEC | I2C_CLIENT_SCCB);
> Asmaa>> I don't think ipmb supports 10 bit addresses. The max number of bits in the IPMB address field is 8.
> Done.
>
> + i2c_msg.len = msg_len;
> + i2c_msg.buf = i2c_buf;
> +
> + ret = i2c_transfer(client->adapter, &i2c_msg, 1);
> + kfree(i2c_buf);
> +
> + return ret;
> +
> +}
> +
> static ssize_t ipmb_write(struct file *file, const char __user *buf,
> size_t count, loff_t *ppos)
> {
> @@ -133,6 +167,12 @@ static ssize_t ipmb_write(struct file *file, const char __user *buf,
> rq_sa = GET_7BIT_ADDR(msg[RQ_SA_8BIT_IDX]);
> netf_rq_lun = msg[NETFN_LUN_IDX];
>
> + /* Check i2c block transfer vs smbus */
> + if (ipmb_dev->use_i2c) {
> + ret = ipmb_i2c_write(ipmb_dev->client, msg);
> + return (ret == 1) ? count : ret;
> + }
> +
> /*
> * subtract rq_sa and netf_rq_lun from the length of the msg passed to
> * i2c_smbus_xfer
> @@ -277,6 +317,7 @@ static int ipmb_probe(struct i2c_client *client,
> const struct i2c_device_id *id)
> {
> struct ipmb_dev *ipmb_dev;
> + struct device_node *np;
> int ret;
>
> ipmb_dev = devm_kzalloc(&client->dev, sizeof(*ipmb_dev), @@ -302,6 +343,13 @@ static int ipmb_probe(struct i2c_client *client,
> if (ret)
> return ret;
>
> + /* Check if i2c block xmit needs to use instead of smbus */
> + np = client->dev.of_node;
> + if (np && of_get_property(np, "use-i2c-block", NULL))
> Asmaa>> Rename this variable i2c-protocol. And also, apply this to ACPI as well.
> Done.
I don't think ACPI is that important at the moment. Rename is good.
> + ipmb_dev->use_i2c = true;
> + else
> + ipmb_dev->use_i2c = false;
The above two lines are unnecessary.
Done, will send v4 soon.
-corey
> +
> ipmb_dev->client = client;
> i2c_set_clientdata(client, ipmb_dev);
> ret = i2c_slave_register(client, ipmb_slave_cb);
> --
> 2.17.1
>
>
>
Powered by blists - more mailing lists