* [PATCH v3] drivers: ipmi: Support raw i2c packet in IPMB @ 2019-11-13 19:23 Vijay Khemka 2019-11-13 20:32 ` Asmaa Mnebhi 0 siblings, 1 reply; 9+ messages in thread From: Vijay Khemka @ 2019-11-13 19:23 UTC (permalink / raw) To: linux-aspeed 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. Signed-off-by: Vijay Khemka <vijaykhemka@fb.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; }; 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); + if (!i2c_buf) + return -EFAULT; + + /* 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); + 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)) + ipmb_dev->use_i2c = true; + else + ipmb_dev->use_i2c = false; + ipmb_dev->client = client; i2c_set_clientdata(client, ipmb_dev); ret = i2c_slave_register(client, ipmb_slave_cb); -- 2.17.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v3] drivers: ipmi: Support raw i2c packet in IPMB 2019-11-13 19:23 [PATCH v3] drivers: ipmi: Support raw i2c packet in IPMB Vijay Khemka @ 2019-11-13 20:32 ` Asmaa Mnebhi 0 siblings, 0 replies; 9+ messages in thread From: Asmaa Mnebhi @ 2019-11-13 20:32 UTC (permalink / raw) To: linux-aspeed Inline response: -----Original Message----- From: Vijay Khemka <vijaykhemka@fb.com> Sent: Wednesday, November 13, 2019 2:23 PM To: Corey Minyard <minyard@acm.org>; Arnd Bergmann <arnd@arndb.de>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; openipmi-developer at lists.sourceforge.net; linux-kernel at vger.kernel.org Cc: vijaykhemka at fb.com; cminyard at mvista.com; Asmaa Mnebhi <Asmaa@mellanox.com>; joel at jms.id.au; linux-aspeed at lists.ozlabs.org; sdasari at fb.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@fb.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 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. 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. + if (!i2c_buf) + return -EFAULT; + + /* 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. + 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. + ipmb_dev->use_i2c = true; + else + ipmb_dev->use_i2c = false; + ipmb_dev->client = client; i2c_set_clientdata(client, ipmb_dev); ret = i2c_slave_register(client, ipmb_slave_cb); -- 2.17.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* RE: [PATCH v3] drivers: ipmi: Support raw i2c packet in IPMB @ 2019-11-13 20:32 ` Asmaa Mnebhi 0 siblings, 0 replies; 9+ messages in thread From: Asmaa Mnebhi @ 2019-11-13 20:32 UTC (permalink / raw) To: Vijay Khemka, Corey Minyard, Arnd Bergmann, Greg Kroah-Hartman, openipmi-developer@lists.sourceforge.net, linux-kernel@vger.kernel.org Cc: cminyard@mvista.com, joel@jms.id.au, linux-aspeed@lists.ozlabs.org, sdasari@fb.com Inline response: -----Original Message----- From: Vijay Khemka <vijaykhemka@fb.com> Sent: Wednesday, November 13, 2019 2:23 PM To: Corey Minyard <minyard@acm.org>; Arnd Bergmann <arnd@arndb.de>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; openipmi-developer@lists.sourceforge.net; linux-kernel@vger.kernel.org Cc: vijaykhemka@fb.com; cminyard@mvista.com; Asmaa Mnebhi <Asmaa@mellanox.com>; joel@jms.id.au; linux-aspeed@lists.ozlabs.org; sdasari@fb.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@fb.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 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. 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. + if (!i2c_buf) + return -EFAULT; + + /* 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. + 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. + ipmb_dev->use_i2c = true; + else + ipmb_dev->use_i2c = false; + ipmb_dev->client = client; i2c_set_clientdata(client, ipmb_dev); ret = i2c_slave_register(client, ipmb_slave_cb); -- 2.17.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v3] drivers: ipmi: Support raw i2c packet in IPMB 2019-11-13 20:32 ` Asmaa Mnebhi @ 2019-11-13 21:28 ` Vijay Khemka -1 siblings, 0 replies; 9+ messages in thread From: Vijay Khemka @ 2019-11-13 21:28 UTC (permalink / raw) To: linux-aspeed ?On 11/13/19, 12:33 PM, "Asmaa Mnebhi" <Asmaa@mellanox.com> wrote: Inline response: -----Original Message----- From: Vijay Khemka <vijaykhemka@fb.com> Sent: Wednesday, November 13, 2019 2:23 PM To: Corey Minyard <minyard@acm.org>; Arnd Bergmann <arnd@arndb.de>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; openipmi-developer at lists.sourceforge.net; linux-kernel at vger.kernel.org Cc: vijaykhemka at fb.com; cminyard at mvista.com; Asmaa Mnebhi <Asmaa@mellanox.com>; joel at jms.id.au; linux-aspeed at lists.ozlabs.org; sdasari at fb.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@fb.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. + if (!i2c_buf) + return -EFAULT; + + /* 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. + ipmb_dev->use_i2c = true; + else + ipmb_dev->use_i2c = false; + ipmb_dev->client = client; i2c_set_clientdata(client, ipmb_dev); ret = i2c_slave_register(client, ipmb_slave_cb); -- 2.17.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] drivers: ipmi: Support raw i2c packet in IPMB @ 2019-11-13 21:28 ` Vijay Khemka 0 siblings, 0 replies; 9+ messages in thread From: Vijay Khemka @ 2019-11-13 21:28 UTC (permalink / raw) To: Asmaa Mnebhi, Corey Minyard, Arnd Bergmann, Greg Kroah-Hartman, openipmi-developer@lists.sourceforge.net, linux-kernel@vger.kernel.org Cc: cminyard@mvista.com, joel@jms.id.au, linux-aspeed@lists.ozlabs.org, Sai Dasari, wsa@the-dreams.de On 11/13/19, 12:33 PM, "Asmaa Mnebhi" <Asmaa@mellanox.com> wrote: Inline response: -----Original Message----- From: Vijay Khemka <vijaykhemka@fb.com> Sent: Wednesday, November 13, 2019 2:23 PM To: Corey Minyard <minyard@acm.org>; Arnd Bergmann <arnd@arndb.de>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; openipmi-developer@lists.sourceforge.net; linux-kernel@vger.kernel.org Cc: vijaykhemka@fb.com; cminyard@mvista.com; Asmaa Mnebhi <Asmaa@mellanox.com>; joel@jms.id.au; linux-aspeed@lists.ozlabs.org; sdasari@fb.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@fb.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. + if (!i2c_buf) + return -EFAULT; + + /* 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. + ipmb_dev->use_i2c = true; + else + ipmb_dev->use_i2c = false; + ipmb_dev->client = client; i2c_set_clientdata(client, ipmb_dev); ret = i2c_slave_register(client, ipmb_slave_cb); -- 2.17.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3] drivers: ipmi: Support raw i2c packet in IPMB 2019-11-13 21:28 ` Vijay Khemka @ 2019-11-13 21:47 ` Corey Minyard -1 siblings, 0 replies; 9+ messages in thread From: Corey Minyard @ 2019-11-13 21:47 UTC (permalink / raw) To: linux-aspeed On Wed, Nov 13, 2019 at 09:28:10PM +0000, Vijay Khemka wrote: > > > ?On 11/13/19, 12:33 PM, "Asmaa Mnebhi" <Asmaa@mellanox.com> wrote: > > Inline response: > > -----Original Message----- > From: Vijay Khemka <vijaykhemka@fb.com> > Sent: Wednesday, November 13, 2019 2:23 PM > To: Corey Minyard <minyard@acm.org>; Arnd Bergmann <arnd@arndb.de>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; openipmi-developer at lists.sourceforge.net; linux-kernel at vger.kernel.org > Cc: vijaykhemka at fb.com; cminyard at mvista.com; Asmaa Mnebhi <Asmaa@mellanox.com>; joel at jms.id.au; linux-aspeed at lists.ozlabs.org; sdasari at fb.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@fb.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) > + > + /* 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. -corey > + > ipmb_dev->client = client; > i2c_set_clientdata(client, ipmb_dev); > ret = i2c_slave_register(client, ipmb_slave_cb); > -- > 2.17.1 > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] drivers: ipmi: Support raw i2c packet in IPMB @ 2019-11-13 21:47 ` Corey Minyard 0 siblings, 0 replies; 9+ messages in thread From: Corey Minyard @ 2019-11-13 21:47 UTC (permalink / raw) To: Vijay Khemka Cc: Asmaa Mnebhi, Arnd Bergmann, Greg Kroah-Hartman, openipmi-developer@lists.sourceforge.net, linux-kernel@vger.kernel.org, cminyard@mvista.com, joel@jms.id.au, linux-aspeed@lists.ozlabs.org, Sai Dasari, wsa@the-dreams.de On Wed, Nov 13, 2019 at 09:28:10PM +0000, Vijay Khemka wrote: > > > On 11/13/19, 12:33 PM, "Asmaa Mnebhi" <Asmaa@mellanox.com> wrote: > > Inline response: > > -----Original Message----- > From: Vijay Khemka <vijaykhemka@fb.com> > Sent: Wednesday, November 13, 2019 2:23 PM > To: Corey Minyard <minyard@acm.org>; Arnd Bergmann <arnd@arndb.de>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; openipmi-developer@lists.sourceforge.net; linux-kernel@vger.kernel.org > Cc: vijaykhemka@fb.com; cminyard@mvista.com; Asmaa Mnebhi <Asmaa@mellanox.com>; joel@jms.id.au; linux-aspeed@lists.ozlabs.org; sdasari@fb.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@fb.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) > + > + /* 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. -corey > + > ipmb_dev->client = client; > i2c_set_clientdata(client, ipmb_dev); > ret = i2c_slave_register(client, ipmb_slave_cb); > -- > 2.17.1 > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3] drivers: ipmi: Support raw i2c packet in IPMB 2019-11-13 21:47 ` Corey Minyard @ 2019-11-13 22:58 ` Vijay Khemka -1 siblings, 0 replies; 9+ messages in thread From: Vijay Khemka @ 2019-11-13 22:58 UTC (permalink / raw) To: linux-aspeed ?On 11/13/19, 1:47 PM, "Corey Minyard" <tcminyard at gmail.com on behalf of minyard@acm.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@mellanox.com> wrote: > > Inline response: > > -----Original Message----- > From: Vijay Khemka <vijaykhemka@fb.com> > Sent: Wednesday, November 13, 2019 2:23 PM > To: Corey Minyard <minyard@acm.org>; Arnd Bergmann <arnd@arndb.de>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; openipmi-developer at lists.sourceforge.net; linux-kernel at vger.kernel.org > Cc: vijaykhemka at fb.com; cminyard at mvista.com; Asmaa Mnebhi <Asmaa@mellanox.com>; joel at jms.id.au; linux-aspeed at lists.ozlabs.org; sdasari at fb.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@fb.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 > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] drivers: ipmi: Support raw i2c packet in IPMB @ 2019-11-13 22:58 ` Vijay Khemka 0 siblings, 0 replies; 9+ messages in thread From: Vijay Khemka @ 2019-11-13 22:58 UTC (permalink / raw) To: minyard@acm.org Cc: Asmaa Mnebhi, Arnd Bergmann, Greg Kroah-Hartman, openipmi-developer@lists.sourceforge.net, linux-kernel@vger.kernel.org, cminyard@mvista.com, joel@jms.id.au, linux-aspeed@lists.ozlabs.org, Sai Dasari, wsa@the-dreams.de On 11/13/19, 1:47 PM, "Corey Minyard" <tcminyard@gmail.com on behalf of minyard@acm.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@mellanox.com> wrote: > > Inline response: > > -----Original Message----- > From: Vijay Khemka <vijaykhemka@fb.com> > Sent: Wednesday, November 13, 2019 2:23 PM > To: Corey Minyard <minyard@acm.org>; Arnd Bergmann <arnd@arndb.de>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; openipmi-developer@lists.sourceforge.net; linux-kernel@vger.kernel.org > Cc: vijaykhemka@fb.com; cminyard@mvista.com; Asmaa Mnebhi <Asmaa@mellanox.com>; joel@jms.id.au; linux-aspeed@lists.ozlabs.org; sdasari@fb.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@fb.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 > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-11-13 22:59 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-11-13 19:23 [PATCH v3] drivers: ipmi: Support raw i2c packet in IPMB Vijay Khemka 2019-11-13 20:32 ` Asmaa Mnebhi 2019-11-13 20:32 ` Asmaa Mnebhi 2019-11-13 21:28 ` Vijay Khemka 2019-11-13 21:28 ` Vijay Khemka 2019-11-13 21:47 ` Corey Minyard 2019-11-13 21:47 ` Corey Minyard 2019-11-13 22:58 ` Vijay Khemka 2019-11-13 22:58 ` Vijay Khemka
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.