Re: [PATCH v3] drivers: ipmi: Support raw i2c packet in IPMB

[Vijay Khemka]

ïOn 11/13/19, 12:33 PM, "Asmaa Mnebhi" <Asmaa@xxxxxxxxxxxx> wrote:

    Inline response:
    
    -----Original Message-----
    From: Vijay Khemka <vijaykhemka@xxxxxx> 
    Sent: Wednesday, November 13, 2019 2:23 PM
    To: Corey Minyard <minyard@xxxxxxx>; Arnd Bergmann <arnd@xxxxxxxx>; Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; openipmi-developer@xxxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
    Cc: vijaykhemka@xxxxxx; cminyard@xxxxxxxxxx; Asmaa Mnebhi <Asmaa@xxxxxxxxxxxx>; joel@xxxxxxxxx; linux-aspeed@xxxxxxxxxxxxxxxx; sdasari@xxxxxx
    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@xxxxxx>
    ---
     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