Inline response:
-----Original Message-----
From: Vijay Khemka <[email protected]>
Sent: Wednesday, November 13, 2019 2:23 PM
To: Corey Minyard <[email protected]>; Arnd Bergmann <[email protected]>; Greg Kroah-Hartman <[email protected]>; [email protected]; [email protected]
Cc: [email protected]; [email protected]; Asmaa Mnebhi <[email protected]>; [email protected]; [email protected]; [email protected]
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 <[email protected]>
---
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
On 11/13/19, 12:33 PM, "Asmaa Mnebhi" <[email protected]> wrote:
Inline response:
-----Original Message-----
From: Vijay Khemka <[email protected]>
Sent: Wednesday, November 13, 2019 2:23 PM
To: Corey Minyard <[email protected]>; Arnd Bergmann <[email protected]>; Greg Kroah-Hartman <[email protected]>; [email protected]; [email protected]
Cc: [email protected]; [email protected]; Asmaa Mnebhi <[email protected]>; [email protected]; [email protected]; [email protected]
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 <[email protected]>
---
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
On Wed, Nov 13, 2019 at 09:28:10PM +0000, Vijay Khemka wrote:
>
>
> On 11/13/19, 12:33 PM, "Asmaa Mnebhi" <[email protected]> wrote:
>
> Inline response:
>
> -----Original Message-----
> From: Vijay Khemka <[email protected]>
> Sent: Wednesday, November 13, 2019 2:23 PM
> To: Corey Minyard <[email protected]>; Arnd Bergmann <[email protected]>; Greg Kroah-Hartman <[email protected]>; [email protected]; [email protected]
> Cc: [email protected]; [email protected]; Asmaa Mnebhi <[email protected]>; [email protected]; [email protected]; [email protected]
> 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 <[email protected]>
> ---
> 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
>
>
>
On 11/13/19, 1:47 PM, "Corey Minyard" <[email protected] on behalf of [email protected]> wrote:
On Wed, Nov 13, 2019 at 09:28:10PM +0000, Vijay Khemka wrote:
>
>
> On 11/13/19, 12:33 PM, "Asmaa Mnebhi" <[email protected]> wrote:
>
> Inline response:
>
> -----Original Message-----
> From: Vijay Khemka <[email protected]>
> Sent: Wednesday, November 13, 2019 2:23 PM
> To: Corey Minyard <[email protected]>; Arnd Bergmann <[email protected]>; Greg Kroah-Hartman <[email protected]>; [email protected]; [email protected]
> Cc: [email protected]; [email protected]; Asmaa Mnebhi <[email protected]>; [email protected]; [email protected]; [email protected]
> 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 <[email protected]>
> ---
> 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
>
>
>