RE: [PATCH 1/2] drivers: ipmi: Support raw i2c packet in IPMB

[Asmaa Mnebhi]

I agree with corey. You can take a look at the ipmi_ssif.c driver which does that.

-----Original Message-----
From: Corey Minyard <tcminyard@xxxxxxxxx> On Behalf Of Corey Minyard
Sent: Tuesday, November 12, 2019 7:36 AM
To: Vijay Khemka <vijaykhemka@xxxxxx>
Cc: Arnd Bergmann <arnd@xxxxxxxx>; Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; openipmi-developer@xxxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; cminyard@xxxxxxxxxx; Asmaa Mnebhi <Asmaa@xxxxxxxxxxxx>; joel@xxxxxxxxx; linux-aspeed@xxxxxxxxxxxxxxxx; sdasari@xxxxxx
Subject: Re: [PATCH 1/2] drivers: ipmi: Support raw i2c packet in IPMB

On Mon, Nov 11, 2019 at 06:36:09PM -0800, Vijay Khemka wrote:
> Many IPMB devices doesn't support smbus protocol and current driver 
> support only smbus devices. So added support for raw i2c packets.

I haven't reviewed this, really, because I have a more general concern...

Is it possible to not do this with a config item?  Can you add something to the device tree and/or via an ioctl to make this dynamically configurable?  That's more flexible (it can support mixed devices) and is friendlier to users (don't have to get the config right).

Config items for adding new functionality are generally ok.  Config items for choosing between two mutually exclusive choices are generally not.

-corey

> 
> Signed-off-by: Vijay Khemka <vijaykhemka@xxxxxx>
> ---
>  drivers/char/ipmi/Kconfig        |  6 ++++++
>  drivers/char/ipmi/ipmb_dev_int.c | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 36 insertions(+)
> 
> diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig 
> index a9cfe4c05e64..e5268443b478 100644
> --- a/drivers/char/ipmi/Kconfig
> +++ b/drivers/char/ipmi/Kconfig
> @@ -139,3 +139,9 @@ config IPMB_DEVICE_INTERFACE
>  	  Provides a driver for a device (Satellite MC) to
>  	  receive requests and send responses back to the BMC via
>  	  the IPMB interface. This module requires I2C support.
> +
> +config IPMB_SMBUS_DISABLE
> +	bool 'Disable SMBUS protocol for sending packet to IPMB device'
> +	depends on IPMB_DEVICE_INTERFACE
> +	help
> +	  provides functionality of sending raw i2c packets to IPMB device.
> diff --git a/drivers/char/ipmi/ipmb_dev_int.c 
> b/drivers/char/ipmi/ipmb_dev_int.c
> index ae3bfba27526..2419b9a928b2 100644
> --- a/drivers/char/ipmi/ipmb_dev_int.c
> +++ b/drivers/char/ipmi/ipmb_dev_int.c
> @@ -118,6 +118,10 @@ static ssize_t ipmb_write(struct file *file, const char __user *buf,
>  	struct ipmb_dev *ipmb_dev = to_ipmb_dev(file);
>  	u8 rq_sa, netf_rq_lun, msg_len;
>  	union i2c_smbus_data data;
> +#ifdef CONFIG_IPMB_SMBUS_DISABLE
> +	unsigned char *i2c_buf;
> +	struct i2c_msg i2c_msg;
> +#endif
>  	u8 msg[MAX_MSG_LEN];
>  	ssize_t ret;
>  
> @@ -133,6 +137,31 @@ 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];
>  
> +#ifdef CONFIG_IPMB_SMBUS_DISABLE
> +	/*
> +	 * 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 = rq_sa;
> +	i2c_msg.flags = ipmb_dev->client->flags &
> +			(I2C_M_TEN | I2C_CLIENT_PEC | I2C_CLIENT_SCCB);
> +	i2c_msg.len = msg_len;
> +	i2c_msg.buf = i2c_buf;
> +
> +	ret = i2c_transfer(ipmb_dev->client->adapter, &i2c_msg, 1);
> +	kfree(i2c_buf);
> +
> +	return (ret == 1) ? count : ret;
> +#else
>  	/*
>  	 * subtract rq_sa and netf_rq_lun from the length of the msg passed to
>  	 * i2c_smbus_xfer
> @@ -149,6 +178,7 @@ static ssize_t ipmb_write(struct file *file, const char __user *buf,
>  			     I2C_SMBUS_BLOCK_DATA, &data);
>  
>  	return ret ? : count;
> +#endif
>  }
>  
>  static unsigned int ipmb_poll(struct file *file, poll_table *wait)
> --
> 2.17.1
>