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

[Vijay Khemka]

ïOn 11/13/19, 1:47 PM, "Corey Minyard" <tcminyard@xxxxxxxxx on behalf of minyard@xxxxxxx> wrote:

    On Wed, Nov 13, 2019 at 09:28:10PM +0000, Vijay Khemka wrote:
    > 
    > 
    > 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.
    
    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
    >     
    >     
    >