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

From: Vijay Khemka
Date: Tue Nov 12 2019 - 12:58:03 EST




ïOn 11/12/19, 4:36 AM, "Corey Minyard" <tcminyard@xxxxxxxxx on behalf of minyard@xxxxxxx> wrote:

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).
I agree with you, I was also not comfortable using config and couldn't find other
Options, I will look into more option now and update patch.

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
>