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

From: Vijay Khemka
Date: Tue Nov 12 2019 - 15:14:34 EST




ïOn 11/12/19, 10:41 AM, "Corey Minyard" <cminyard@xxxxxxxxxx> wrote:

On Tue, Nov 12, 2019 at 05:57:25PM +0000, Vijay Khemka wrote:
>
>
> 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.

IMHO, device tree is the right way to do this. You should also have a
sysfs setting for this, I think.

I think ioctl is also a good option and can control from application. I have already
wrote ioctl approach. Testing the same and will send new patch once verified.

-corey

>
> 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
> >
>
>