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 <[email protected]>
> ---
> 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
>
I agree with corey. You can take a look at the ipmi_ssif.c driver which does that.
-----Original Message-----
From: Corey Minyard <[email protected]> On Behalf Of Corey Minyard
Sent: Tuesday, November 12, 2019 7:36 AM
To: Vijay Khemka <[email protected]>
Cc: Arnd Bergmann <[email protected]>; Greg Kroah-Hartman <[email protected]>; [email protected]; [email protected]; [email protected]; Asmaa Mnebhi <[email protected]>; [email protected]; [email protected]; [email protected]
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 <[email protected]>
> ---
> 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
>
On 11/12/19, 4:36 AM, "Corey Minyard" <[email protected] on behalf of [email protected]> 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 <[email protected]>
> ---
> 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
>
On Tue, Nov 12, 2019 at 05:57:25PM +0000, Vijay Khemka wrote:
>
>
> On 11/12/19, 4:36 AM, "Corey Minyard" <[email protected] on behalf of [email protected]> 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.
-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 <[email protected]>
> > ---
> > 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
> >
>
>
On 11/12/19, 10:41 AM, "Corey Minyard" <[email protected]> wrote:
On Tue, Nov 12, 2019 at 05:57:25PM +0000, Vijay Khemka wrote:
>
>
> On 11/12/19, 4:36 AM, "Corey Minyard" <[email protected] on behalf of [email protected]> 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 <[email protected]>
> > ---
> > 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
> >
>
>