* Re: [PATCH 1/2] drivers: ipmi: Support raw i2c packet in IPMB [not found] <20191112023610.3644314-1-vijaykhemka@fb.com> @ 2019-11-12 12:36 ` Corey Minyard 2019-11-12 14:19 ` Asmaa Mnebhi 2019-11-12 17:57 ` Vijay Khemka [not found] ` <20191112023610.3644314-2-vijaykhemka@fb.com> 1 sibling, 2 replies; 14+ messages in thread From: Corey Minyard @ 2019-11-12 12:36 UTC (permalink / raw) To: Vijay Khemka Cc: Arnd Bergmann, Greg Kroah-Hartman, openipmi-developer, linux-kernel, cminyard, asmaa, joel, linux-aspeed, sdasari 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@fb.com> > --- > 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH 1/2] drivers: ipmi: Support raw i2c packet in IPMB 2019-11-12 12:36 ` [PATCH 1/2] drivers: ipmi: Support raw i2c packet in IPMB Corey Minyard @ 2019-11-12 14:19 ` Asmaa Mnebhi 2019-11-12 17:57 ` Vijay Khemka 1 sibling, 0 replies; 14+ messages in thread From: Asmaa Mnebhi @ 2019-11-12 14:19 UTC (permalink / raw) To: minyard@acm.org, Vijay Khemka Cc: Arnd Bergmann, Greg Kroah-Hartman, openipmi-developer@lists.sourceforge.net, linux-kernel@vger.kernel.org, cminyard@mvista.com, joel@jms.id.au, linux-aspeed@lists.ozlabs.org, sdasari@fb.com I agree with corey. You can take a look at the ipmi_ssif.c driver which does that. -----Original Message----- From: Corey Minyard <tcminyard@gmail.com> On Behalf Of Corey Minyard Sent: Tuesday, November 12, 2019 7:36 AM To: Vijay Khemka <vijaykhemka@fb.com> Cc: Arnd Bergmann <arnd@arndb.de>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; openipmi-developer@lists.sourceforge.net; linux-kernel@vger.kernel.org; cminyard@mvista.com; Asmaa Mnebhi <Asmaa@mellanox.com>; joel@jms.id.au; linux-aspeed@lists.ozlabs.org; sdasari@fb.com 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@fb.com> > --- > 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] drivers: ipmi: Support raw i2c packet in IPMB 2019-11-12 12:36 ` [PATCH 1/2] drivers: ipmi: Support raw i2c packet in IPMB Corey Minyard 2019-11-12 14:19 ` Asmaa Mnebhi @ 2019-11-12 17:57 ` Vijay Khemka 2019-11-12 18:41 ` Corey Minyard 1 sibling, 1 reply; 14+ messages in thread From: Vijay Khemka @ 2019-11-12 17:57 UTC (permalink / raw) To: minyard@acm.org Cc: Arnd Bergmann, Greg Kroah-Hartman, openipmi-developer@lists.sourceforge.net, linux-kernel@vger.kernel.org, cminyard@mvista.com, asmaa@mellanox.com, joel@jms.id.au, linux-aspeed@lists.ozlabs.org, Sai Dasari On 11/12/19, 4:36 AM, "Corey Minyard" <tcminyard@gmail.com on behalf of minyard@acm.org> 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@fb.com> > --- > 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 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] drivers: ipmi: Support raw i2c packet in IPMB 2019-11-12 17:57 ` Vijay Khemka @ 2019-11-12 18:41 ` Corey Minyard 2019-11-12 20:13 ` Vijay Khemka 0 siblings, 1 reply; 14+ messages in thread From: Corey Minyard @ 2019-11-12 18:41 UTC (permalink / raw) To: Vijay Khemka Cc: minyard@acm.org, Arnd Bergmann, Greg Kroah-Hartman, openipmi-developer@lists.sourceforge.net, linux-kernel@vger.kernel.org, asmaa@mellanox.com, joel@jms.id.au, linux-aspeed@lists.ozlabs.org, Sai Dasari On Tue, Nov 12, 2019 at 05:57:25PM +0000, Vijay Khemka wrote: > > > On 11/12/19, 4:36 AM, "Corey Minyard" <tcminyard@gmail.com on behalf of minyard@acm.org> 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 <vijaykhemka@fb.com> > > --- > > 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 > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] drivers: ipmi: Support raw i2c packet in IPMB 2019-11-12 18:41 ` Corey Minyard @ 2019-11-12 20:13 ` Vijay Khemka 0 siblings, 0 replies; 14+ messages in thread From: Vijay Khemka @ 2019-11-12 20:13 UTC (permalink / raw) To: cminyard@mvista.com Cc: minyard@acm.org, Arnd Bergmann, Greg Kroah-Hartman, openipmi-developer@lists.sourceforge.net, linux-kernel@vger.kernel.org, asmaa@mellanox.com, joel@jms.id.au, linux-aspeed@lists.ozlabs.org, Sai Dasari On 11/12/19, 10:41 AM, "Corey Minyard" <cminyard@mvista.com> wrote: On Tue, Nov 12, 2019 at 05:57:25PM +0000, Vijay Khemka wrote: > > > On 11/12/19, 4:36 AM, "Corey Minyard" <tcminyard@gmail.com on behalf of minyard@acm.org> 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@fb.com> > > --- > > 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 > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
[parent not found: <20191112023610.3644314-2-vijaykhemka@fb.com>]
* Re: [PATCH 2/2] drivers: ipmi: Modify max length of IPMB packet [not found] ` <20191112023610.3644314-2-vijaykhemka@fb.com> @ 2019-11-12 12:48 ` Corey Minyard 2019-11-12 19:56 ` Vijay Khemka 0 siblings, 1 reply; 14+ messages in thread From: Corey Minyard @ 2019-11-12 12:48 UTC (permalink / raw) To: Vijay Khemka Cc: Arnd Bergmann, Greg Kroah-Hartman, openipmi-developer, linux-kernel, cminyard, asmaa, joel, linux-aspeed, sdasari On Mon, Nov 11, 2019 at 06:36:10PM -0800, Vijay Khemka wrote: > As per IPMB specification, maximum packet size supported is 255, > modified Max length to 240 from 128 to accommodate more data. I couldn't find this in the IPMB specification. IIRC, the maximum on I2C is 32 byts, and table 6-9 in the IPMI spec, under "IPMB Output" states: The IPMB standard message length is specified as 32 bytes, maximum, including slave address. I'm not sure where 128 came from, but maybe it should be reduced to 31. -corey > > Signed-off-by: Vijay Khemka <vijaykhemka@fb.com> > --- > drivers/char/ipmi/ipmb_dev_int.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/char/ipmi/ipmb_dev_int.c b/drivers/char/ipmi/ipmb_dev_int.c > index 2419b9a928b2..7f9198bbce96 100644 > --- a/drivers/char/ipmi/ipmb_dev_int.c > +++ b/drivers/char/ipmi/ipmb_dev_int.c > @@ -19,7 +19,7 @@ > #include <linux/spinlock.h> > #include <linux/wait.h> > > -#define MAX_MSG_LEN 128 > +#define MAX_MSG_LEN 240 > #define IPMB_REQUEST_LEN_MIN 7 > #define NETFN_RSP_BIT_MASK 0x4 > #define REQUEST_QUEUE_MAX_LEN 256 > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] drivers: ipmi: Modify max length of IPMB packet 2019-11-12 12:48 ` [PATCH 2/2] drivers: ipmi: Modify max length of IPMB packet Corey Minyard @ 2019-11-12 19:56 ` Vijay Khemka 2019-11-12 20:29 ` Corey Minyard 0 siblings, 1 reply; 14+ messages in thread From: Vijay Khemka @ 2019-11-12 19:56 UTC (permalink / raw) To: minyard@acm.org Cc: Arnd Bergmann, Greg Kroah-Hartman, openipmi-developer@lists.sourceforge.net, linux-kernel@vger.kernel.org, cminyard@mvista.com, asmaa@mellanox.com, joel@jms.id.au, linux-aspeed@lists.ozlabs.org, Sai Dasari On 11/12/19, 4:48 AM, "Corey Minyard" <tcminyard@gmail.com on behalf of minyard@acm.org> wrote: On Mon, Nov 11, 2019 at 06:36:10PM -0800, Vijay Khemka wrote: > As per IPMB specification, maximum packet size supported is 255, > modified Max length to 240 from 128 to accommodate more data. I couldn't find this in the IPMB specification. IIRC, the maximum on I2C is 32 byts, and table 6-9 in the IPMI spec, under "IPMB Output" states: The IPMB standard message length is specified as 32 bytes, maximum, including slave address. We are using IPMI OEM messages and our response size is around 150 bytes For some of responses. That's why I had set it to 240 bytes. I'm not sure where 128 came from, but maybe it should be reduced to 31. -corey > > Signed-off-by: Vijay Khemka <vijaykhemka@fb.com> > --- > drivers/char/ipmi/ipmb_dev_int.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/char/ipmi/ipmb_dev_int.c b/drivers/char/ipmi/ipmb_dev_int.c > index 2419b9a928b2..7f9198bbce96 100644 > --- a/drivers/char/ipmi/ipmb_dev_int.c > +++ b/drivers/char/ipmi/ipmb_dev_int.c > @@ -19,7 +19,7 @@ > #include <linux/spinlock.h> > #include <linux/wait.h> > > -#define MAX_MSG_LEN 128 > +#define MAX_MSG_LEN 240 > #define IPMB_REQUEST_LEN_MIN 7 > #define NETFN_RSP_BIT_MASK 0x4 > #define REQUEST_QUEUE_MAX_LEN 256 > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] drivers: ipmi: Modify max length of IPMB packet 2019-11-12 19:56 ` Vijay Khemka @ 2019-11-12 20:29 ` Corey Minyard 2019-11-12 20:48 ` Asmaa Mnebhi 2019-11-12 22:06 ` Asmaa Mnebhi 0 siblings, 2 replies; 14+ messages in thread From: Corey Minyard @ 2019-11-12 20:29 UTC (permalink / raw) To: Vijay Khemka Cc: Arnd Bergmann, Greg Kroah-Hartman, openipmi-developer@lists.sourceforge.net, linux-kernel@vger.kernel.org, cminyard@mvista.com, asmaa@mellanox.com, joel@jms.id.au, linux-aspeed@lists.ozlabs.org, Sai Dasari On Tue, Nov 12, 2019 at 07:56:34PM +0000, Vijay Khemka wrote: > > > On 11/12/19, 4:48 AM, "Corey Minyard" <tcminyard@gmail.com on behalf of minyard@acm.org> wrote: > > On Mon, Nov 11, 2019 at 06:36:10PM -0800, Vijay Khemka wrote: > > As per IPMB specification, maximum packet size supported is 255, > > modified Max length to 240 from 128 to accommodate more data. > > I couldn't find this in the IPMB specification. > > IIRC, the maximum on I2C is 32 byts, and table 6-9 in the IPMI spec, > under "IPMB Output" states: The IPMB standard message length is > specified as 32 bytes, maximum, including slave address. > > We are using IPMI OEM messages and our response size is around 150 bytes > For some of responses. That's why I had set it to 240 bytes. Hmm. Well, that is a pretty significant violation of the spec, but there's nothing hard in the protocol that prohibits it, I guess. If Asmaa is ok with this, I'm ok with it, too. -corey > > I'm not sure where 128 came from, but maybe it should be reduced to 31. > > -corey > > > > > Signed-off-by: Vijay Khemka <vijaykhemka@fb.com> > > --- > > drivers/char/ipmi/ipmb_dev_int.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/char/ipmi/ipmb_dev_int.c b/drivers/char/ipmi/ipmb_dev_int.c > > index 2419b9a928b2..7f9198bbce96 100644 > > --- a/drivers/char/ipmi/ipmb_dev_int.c > > +++ b/drivers/char/ipmi/ipmb_dev_int.c > > @@ -19,7 +19,7 @@ > > #include <linux/spinlock.h> > > #include <linux/wait.h> > > > > -#define MAX_MSG_LEN 128 > > +#define MAX_MSG_LEN 240 > > #define IPMB_REQUEST_LEN_MIN 7 > > #define NETFN_RSP_BIT_MASK 0x4 > > #define REQUEST_QUEUE_MAX_LEN 256 > > -- > > 2.17.1 > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH 2/2] drivers: ipmi: Modify max length of IPMB packet 2019-11-12 20:29 ` Corey Minyard @ 2019-11-12 20:48 ` Asmaa Mnebhi 2019-11-12 22:06 ` Asmaa Mnebhi 1 sibling, 0 replies; 14+ messages in thread From: Asmaa Mnebhi @ 2019-11-12 20:48 UTC (permalink / raw) To: minyard@acm.org, Vijay Khemka Cc: Arnd Bergmann, Greg Kroah-Hartman, openipmi-developer@lists.sourceforge.net, linux-kernel@vger.kernel.org, cminyard@mvista.com, joel@jms.id.au, linux-aspeed@lists.ozlabs.org, Sai Dasari -----Original Message----- From: Corey Minyard <tcminyard@gmail.com> On Behalf Of Corey Minyard Sent: Tuesday, November 12, 2019 3:30 PM To: Vijay Khemka <vijaykhemka@fb.com> Cc: Arnd Bergmann <arnd@arndb.de>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; openipmi-developer@lists.sourceforge.net; linux-kernel@vger.kernel.org; cminyard@mvista.com; Asmaa Mnebhi <Asmaa@mellanox.com>; joel@jms.id.au; linux-aspeed@lists.ozlabs.org; Sai Dasari <sdasari@fb.com> Subject: Re: [PATCH 2/2] drivers: ipmi: Modify max length of IPMB packet On Tue, Nov 12, 2019 at 07:56:34PM +0000, Vijay Khemka wrote: > > > On 11/12/19, 4:48 AM, "Corey Minyard" <tcminyard@gmail.com on behalf of minyard@acm.org> wrote: > > On Mon, Nov 11, 2019 at 06:36:10PM -0800, Vijay Khemka wrote: > > As per IPMB specification, maximum packet size supported is 255, > > modified Max length to 240 from 128 to accommodate more data. > > I couldn't find this in the IPMB specification. > > IIRC, the maximum on I2C is 32 byts, and table 6-9 in the IPMI spec, > under "IPMB Output" states: The IPMB standard message length is > specified as 32 bytes, maximum, including slave address. > > We are using IPMI OEM messages and our response size is around 150 > bytes For some of responses. That's why I had set it to 240 bytes. Hmm. Well, that is a pretty significant violation of the spec, but there's nothing hard in the protocol that prohibits it, I guess. If Asmaa is ok with this, I'm ok with it, too. It doesn't bother me either. But I would like to do some experiment/testing first before validating it. I will get back to you. Vijay, how are you doing your testing? -corey > > I'm not sure where 128 came from, but maybe it should be reduced to 31. > > -corey > > > > > Signed-off-by: Vijay Khemka <vijaykhemka@fb.com> > > --- > > drivers/char/ipmi/ipmb_dev_int.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/char/ipmi/ipmb_dev_int.c b/drivers/char/ipmi/ipmb_dev_int.c > > index 2419b9a928b2..7f9198bbce96 100644 > > --- a/drivers/char/ipmi/ipmb_dev_int.c > > +++ b/drivers/char/ipmi/ipmb_dev_int.c > > @@ -19,7 +19,7 @@ > > #include <linux/spinlock.h> > > #include <linux/wait.h> > > > > -#define MAX_MSG_LEN 128 > > +#define MAX_MSG_LEN 240 > > #define IPMB_REQUEST_LEN_MIN 7 > > #define NETFN_RSP_BIT_MASK 0x4 > > #define REQUEST_QUEUE_MAX_LEN 256 > > -- > > 2.17.1 > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH 2/2] drivers: ipmi: Modify max length of IPMB packet 2019-11-12 20:29 ` Corey Minyard 2019-11-12 20:48 ` Asmaa Mnebhi @ 2019-11-12 22:06 ` Asmaa Mnebhi 2019-11-12 23:19 ` Vijay Khemka 2019-11-13 0:51 ` [Openipmi-developer] " Corey Minyard 1 sibling, 2 replies; 14+ messages in thread From: Asmaa Mnebhi @ 2019-11-12 22:06 UTC (permalink / raw) To: minyard@acm.org, Vijay Khemka Cc: Arnd Bergmann, Greg Kroah-Hartman, openipmi-developer@lists.sourceforge.net, linux-kernel@vger.kernel.org, cminyard@mvista.com, joel@jms.id.au, linux-aspeed@lists.ozlabs.org, Sai Dasari Also, let me clarify one thing. It doesn't matter how big the response is. In my testing, I also had some responses that are over 128 bytes, and this driver still works. It is the user space program which determines the last bytes received. The 128 bytes is the max number of bytes handled by your i2c/smbus driver at each i2c transaction. My i2c driver can only transmit 128 bytes at a time. So just like Corey pointed out, it would be better to pass this through ACPI/device tree. -----Original Message----- From: Corey Minyard <tcminyard@gmail.com> On Behalf Of Corey Minyard Sent: Tuesday, November 12, 2019 3:30 PM To: Vijay Khemka <vijaykhemka@fb.com> Cc: Arnd Bergmann <arnd@arndb.de>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; openipmi-developer@lists.sourceforge.net; linux-kernel@vger.kernel.org; cminyard@mvista.com; Asmaa Mnebhi <Asmaa@mellanox.com>; joel@jms.id.au; linux-aspeed@lists.ozlabs.org; Sai Dasari <sdasari@fb.com> Subject: Re: [PATCH 2/2] drivers: ipmi: Modify max length of IPMB packet On Tue, Nov 12, 2019 at 07:56:34PM +0000, Vijay Khemka wrote: > > > On 11/12/19, 4:48 AM, "Corey Minyard" <tcminyard@gmail.com on behalf of minyard@acm.org> wrote: > > On Mon, Nov 11, 2019 at 06:36:10PM -0800, Vijay Khemka wrote: > > As per IPMB specification, maximum packet size supported is 255, > > modified Max length to 240 from 128 to accommodate more data. > > I couldn't find this in the IPMB specification. > > IIRC, the maximum on I2C is 32 byts, and table 6-9 in the IPMI spec, > under "IPMB Output" states: The IPMB standard message length is > specified as 32 bytes, maximum, including slave address. > > We are using IPMI OEM messages and our response size is around 150 > bytes For some of responses. That's why I had set it to 240 bytes. Hmm. Well, that is a pretty significant violation of the spec, but there's nothing hard in the protocol that prohibits it, I guess. If Asmaa is ok with this, I'm ok with it, too. -corey > > I'm not sure where 128 came from, but maybe it should be reduced to 31. > > -corey > > > > > Signed-off-by: Vijay Khemka <vijaykhemka@fb.com> > > --- > > drivers/char/ipmi/ipmb_dev_int.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/char/ipmi/ipmb_dev_int.c b/drivers/char/ipmi/ipmb_dev_int.c > > index 2419b9a928b2..7f9198bbce96 100644 > > --- a/drivers/char/ipmi/ipmb_dev_int.c > > +++ b/drivers/char/ipmi/ipmb_dev_int.c > > @@ -19,7 +19,7 @@ > > #include <linux/spinlock.h> > > #include <linux/wait.h> > > > > -#define MAX_MSG_LEN 128 > > +#define MAX_MSG_LEN 240 > > #define IPMB_REQUEST_LEN_MIN 7 > > #define NETFN_RSP_BIT_MASK 0x4 > > #define REQUEST_QUEUE_MAX_LEN 256 > > -- > > 2.17.1 > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] drivers: ipmi: Modify max length of IPMB packet 2019-11-12 22:06 ` Asmaa Mnebhi @ 2019-11-12 23:19 ` Vijay Khemka 2019-11-13 0:51 ` [Openipmi-developer] " Corey Minyard 1 sibling, 0 replies; 14+ messages in thread From: Vijay Khemka @ 2019-11-12 23:19 UTC (permalink / raw) To: Asmaa Mnebhi, minyard@acm.org Cc: Arnd Bergmann, Greg Kroah-Hartman, openipmi-developer@lists.sourceforge.net, linux-kernel@vger.kernel.org, cminyard@mvista.com, joel@jms.id.au, linux-aspeed@lists.ozlabs.org, Sai Dasari On 11/12/19, 2:06 PM, "Asmaa Mnebhi" <Asmaa@mellanox.com> wrote: Also, let me clarify one thing. It doesn't matter how big the response is. In my testing, I also had some responses that are over 128 bytes, and this driver still works. It is the user space program which determines the last bytes received. The 128 bytes is the max number of bytes handled by your i2c/smbus driver at each i2c transaction. My i2c driver can only transmit 128 bytes at a time. So just like Corey pointed out, it would be better to pass this through ACPI/device tree. In case of smbus transfer it will work but if you are doing direct i2c_block where you don't fragment and transfer complete block. I am doing well with my testing. I have sent v2 for first patch using ioctl. And verified the same. It works perfectly fine. -----Original Message----- From: Corey Minyard <tcminyard@gmail.com> On Behalf Of Corey Minyard Sent: Tuesday, November 12, 2019 3:30 PM To: Vijay Khemka <vijaykhemka@fb.com> Cc: Arnd Bergmann <arnd@arndb.de>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; openipmi-developer@lists.sourceforge.net; linux-kernel@vger.kernel.org; cminyard@mvista.com; Asmaa Mnebhi <Asmaa@mellanox.com>; joel@jms.id.au; linux-aspeed@lists.ozlabs.org; Sai Dasari <sdasari@fb.com> Subject: Re: [PATCH 2/2] drivers: ipmi: Modify max length of IPMB packet On Tue, Nov 12, 2019 at 07:56:34PM +0000, Vijay Khemka wrote: > > > On 11/12/19, 4:48 AM, "Corey Minyard" <tcminyard@gmail.com on behalf of minyard@acm.org> wrote: > > On Mon, Nov 11, 2019 at 06:36:10PM -0800, Vijay Khemka wrote: > > As per IPMB specification, maximum packet size supported is 255, > > modified Max length to 240 from 128 to accommodate more data. > > I couldn't find this in the IPMB specification. > > IIRC, the maximum on I2C is 32 byts, and table 6-9 in the IPMI spec, > under "IPMB Output" states: The IPMB standard message length is > specified as 32 bytes, maximum, including slave address. > > We are using IPMI OEM messages and our response size is around 150 > bytes For some of responses. That's why I had set it to 240 bytes. Hmm. Well, that is a pretty significant violation of the spec, but there's nothing hard in the protocol that prohibits it, I guess. If Asmaa is ok with this, I'm ok with it, too. -corey > > I'm not sure where 128 came from, but maybe it should be reduced to 31. > > -corey > > > > > Signed-off-by: Vijay Khemka <vijaykhemka@fb.com> > > --- > > drivers/char/ipmi/ipmb_dev_int.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/char/ipmi/ipmb_dev_int.c b/drivers/char/ipmi/ipmb_dev_int.c > > index 2419b9a928b2..7f9198bbce96 100644 > > --- a/drivers/char/ipmi/ipmb_dev_int.c > > +++ b/drivers/char/ipmi/ipmb_dev_int.c > > @@ -19,7 +19,7 @@ > > #include <linux/spinlock.h> > > #include <linux/wait.h> > > > > -#define MAX_MSG_LEN 128 > > +#define MAX_MSG_LEN 240 > > #define IPMB_REQUEST_LEN_MIN 7 > > #define NETFN_RSP_BIT_MASK 0x4 > > #define REQUEST_QUEUE_MAX_LEN 256 > > -- > > 2.17.1 > > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Openipmi-developer] [PATCH 2/2] drivers: ipmi: Modify max length of IPMB packet 2019-11-12 22:06 ` Asmaa Mnebhi 2019-11-12 23:19 ` Vijay Khemka @ 2019-11-13 0:51 ` Corey Minyard 2019-11-13 17:40 ` Vijay Khemka 1 sibling, 1 reply; 14+ messages in thread From: Corey Minyard @ 2019-11-13 0:51 UTC (permalink / raw) To: Asmaa Mnebhi Cc: Vijay Khemka, cminyard@mvista.com, Sai Dasari, linux-aspeed@lists.ozlabs.org, Arnd Bergmann, Greg Kroah-Hartman, linux-kernel@vger.kernel.org, openipmi-developer@lists.sourceforge.net On Tue, Nov 12, 2019 at 10:06:00PM +0000, Asmaa Mnebhi wrote: > Also, let me clarify one thing. It doesn't matter how big the response is. In my testing, I also had some responses that are over 128 bytes, and this driver still works. It is the user space program which determines the last bytes received. The 128 bytes is the max number of bytes handled by your i2c/smbus driver at each i2c transaction. My i2c driver can only transmit 128 bytes at a time. So just like Corey pointed out, it would be better to pass this through ACPI/device tree. Yeah, I would really prefer device tree. That's what it's designed for, really. ioctls are not really what you want for this. sysfs is a better choice as a backup for device tree (so you can change it if it's wrong). -corey > > -----Original Message----- > From: Corey Minyard <tcminyard@gmail.com> On Behalf Of Corey Minyard > Sent: Tuesday, November 12, 2019 3:30 PM > To: Vijay Khemka <vijaykhemka@fb.com> > Cc: Arnd Bergmann <arnd@arndb.de>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; openipmi-developer@lists.sourceforge.net; linux-kernel@vger.kernel.org; cminyard@mvista.com; Asmaa Mnebhi <Asmaa@mellanox.com>; joel@jms.id.au; linux-aspeed@lists.ozlabs.org; Sai Dasari <sdasari@fb.com> > Subject: Re: [PATCH 2/2] drivers: ipmi: Modify max length of IPMB packet > > On Tue, Nov 12, 2019 at 07:56:34PM +0000, Vijay Khemka wrote: > > > > > > On 11/12/19, 4:48 AM, "Corey Minyard" <tcminyard@gmail.com on behalf of minyard@acm.org> wrote: > > > > On Mon, Nov 11, 2019 at 06:36:10PM -0800, Vijay Khemka wrote: > > > As per IPMB specification, maximum packet size supported is 255, > > > modified Max length to 240 from 128 to accommodate more data. > > > > I couldn't find this in the IPMB specification. > > > > IIRC, the maximum on I2C is 32 byts, and table 6-9 in the IPMI spec, > > under "IPMB Output" states: The IPMB standard message length is > > specified as 32 bytes, maximum, including slave address. > > > > We are using IPMI OEM messages and our response size is around 150 > > bytes For some of responses. That's why I had set it to 240 bytes. > > Hmm. Well, that is a pretty significant violation of the spec, but there's nothing hard in the protocol that prohibits it, I guess. > > If Asmaa is ok with this, I'm ok with it, too. > > -corey > > > > > I'm not sure where 128 came from, but maybe it should be reduced to 31. > > > > -corey > > > > > > > > Signed-off-by: Vijay Khemka <vijaykhemka@fb.com> > > > --- > > > drivers/char/ipmi/ipmb_dev_int.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/char/ipmi/ipmb_dev_int.c b/drivers/char/ipmi/ipmb_dev_int.c > > > index 2419b9a928b2..7f9198bbce96 100644 > > > --- a/drivers/char/ipmi/ipmb_dev_int.c > > > +++ b/drivers/char/ipmi/ipmb_dev_int.c > > > @@ -19,7 +19,7 @@ > > > #include <linux/spinlock.h> > > > #include <linux/wait.h> > > > > > > -#define MAX_MSG_LEN 128 > > > +#define MAX_MSG_LEN 240 > > > #define IPMB_REQUEST_LEN_MIN 7 > > > #define NETFN_RSP_BIT_MASK 0x4 > > > #define REQUEST_QUEUE_MAX_LEN 256 > > > -- > > > 2.17.1 > > > > > > > > > _______________________________________________ > Openipmi-developer mailing list > Openipmi-developer@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/openipmi-developer ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Openipmi-developer] [PATCH 2/2] drivers: ipmi: Modify max length of IPMB packet 2019-11-13 0:51 ` [Openipmi-developer] " Corey Minyard @ 2019-11-13 17:40 ` Vijay Khemka 2019-11-13 17:52 ` Asmaa Mnebhi 0 siblings, 1 reply; 14+ messages in thread From: Vijay Khemka @ 2019-11-13 17:40 UTC (permalink / raw) To: minyard@acm.org, Asmaa Mnebhi Cc: cminyard@mvista.com, Sai Dasari, linux-aspeed@lists.ozlabs.org, Arnd Bergmann, Greg Kroah-Hartman, linux-kernel@vger.kernel.org, openipmi-developer@lists.sourceforge.net On 11/12/19, 4:51 PM, "Corey Minyard" <tcminyard@gmail.com on behalf of minyard@acm.org> wrote: On Tue, Nov 12, 2019 at 10:06:00PM +0000, Asmaa Mnebhi wrote: > Also, let me clarify one thing. It doesn't matter how big the response is. In my testing, I also had some responses that are over 128 bytes, and this driver still works. It is the user space program which determines the last bytes received. The 128 bytes is the max number of bytes handled by your i2c/smbus driver at each i2c transaction. My i2c driver can only transmit 128 bytes at a time. So just like Corey pointed out, it would be better to pass this through ACPI/device tree. Yeah, I would really prefer device tree. That's what it's designed for, really. ioctls are not really what you want for this. sysfs is a better choice as a backup for device tree (so you can change it if it's wrong). Corey/Asmaa, Ok, I will pass this max size through device tree and change this patch. I have sent patch for i2c transfer using ioctl, I hope that should be fine. Please review that v2 patch. -corey > > -----Original Message----- > From: Corey Minyard <tcminyard@gmail.com> On Behalf Of Corey Minyard > Sent: Tuesday, November 12, 2019 3:30 PM > To: Vijay Khemka <vijaykhemka@fb.com> > Cc: Arnd Bergmann <arnd@arndb.de>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; openipmi-developer@lists.sourceforge.net; linux-kernel@vger.kernel.org; cminyard@mvista.com; Asmaa Mnebhi <Asmaa@mellanox.com>; joel@jms.id.au; linux-aspeed@lists.ozlabs.org; Sai Dasari <sdasari@fb.com> > Subject: Re: [PATCH 2/2] drivers: ipmi: Modify max length of IPMB packet > > On Tue, Nov 12, 2019 at 07:56:34PM +0000, Vijay Khemka wrote: > > > > > > On 11/12/19, 4:48 AM, "Corey Minyard" <tcminyard@gmail.com on behalf of minyard@acm.org> wrote: > > > > On Mon, Nov 11, 2019 at 06:36:10PM -0800, Vijay Khemka wrote: > > > As per IPMB specification, maximum packet size supported is 255, > > > modified Max length to 240 from 128 to accommodate more data. > > > > I couldn't find this in the IPMB specification. > > > > IIRC, the maximum on I2C is 32 byts, and table 6-9 in the IPMI spec, > > under "IPMB Output" states: The IPMB standard message length is > > specified as 32 bytes, maximum, including slave address. > > > > We are using IPMI OEM messages and our response size is around 150 > > bytes For some of responses. That's why I had set it to 240 bytes. > > Hmm. Well, that is a pretty significant violation of the spec, but there's nothing hard in the protocol that prohibits it, I guess. > > If Asmaa is ok with this, I'm ok with it, too. > > -corey > > > > > I'm not sure where 128 came from, but maybe it should be reduced to 31. > > > > -corey > > > > > > > > Signed-off-by: Vijay Khemka <vijaykhemka@fb.com> > > > --- > > > drivers/char/ipmi/ipmb_dev_int.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/char/ipmi/ipmb_dev_int.c b/drivers/char/ipmi/ipmb_dev_int.c > > > index 2419b9a928b2..7f9198bbce96 100644 > > > --- a/drivers/char/ipmi/ipmb_dev_int.c > > > +++ b/drivers/char/ipmi/ipmb_dev_int.c > > > @@ -19,7 +19,7 @@ > > > #include <linux/spinlock.h> > > > #include <linux/wait.h> > > > > > > -#define MAX_MSG_LEN 128 > > > +#define MAX_MSG_LEN 240 > > > #define IPMB_REQUEST_LEN_MIN 7 > > > #define NETFN_RSP_BIT_MASK 0x4 > > > #define REQUEST_QUEUE_MAX_LEN 256 > > > -- > > > 2.17.1 > > > > > > > > > _______________________________________________ > Openipmi-developer mailing list > Openipmi-developer@lists.sourceforge.net > https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.sourceforge.net_lists_listinfo_openipmi-2Ddeveloper&d=DwIDaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=v9MU0Ki9pWnTXCWwjHPVgpnCR80vXkkcrIaqU7USl5g&m=QFO5ClAjZ5KmfurkhkRpQQxx33h0Q3NZ9eFRbR3vGyk&s=mZUIyiVF1tBXO1v7ZhLWOW_BwIERBToytaV4ULjXhkM&e= ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [Openipmi-developer] [PATCH 2/2] drivers: ipmi: Modify max length of IPMB packet 2019-11-13 17:40 ` Vijay Khemka @ 2019-11-13 17:52 ` Asmaa Mnebhi 0 siblings, 0 replies; 14+ messages in thread From: Asmaa Mnebhi @ 2019-11-13 17:52 UTC (permalink / raw) To: Vijay Khemka, minyard@acm.org Cc: cminyard@mvista.com, Sai Dasari, linux-aspeed@lists.ozlabs.org, Arnd Bergmann, Greg Kroah-Hartman, linux-kernel@vger.kernel.org, openipmi-developer@lists.sourceforge.net Hi Vijay -----Original Message----- From: Vijay Khemka <vijaykhemka@fb.com> Sent: Wednesday, November 13, 2019 12:41 PM To: minyard@acm.org; Asmaa Mnebhi <Asmaa@mellanox.com> Cc: cminyard@mvista.com; Sai Dasari <sdasari@fb.com>; linux-aspeed@lists.ozlabs.org; Arnd Bergmann <arnd@arndb.de>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; linux-kernel@vger.kernel.org; openipmi-developer@lists.sourceforge.net Subject: Re: [Openipmi-developer] [PATCH 2/2] drivers: ipmi: Modify max length of IPMB packet On 11/12/19, 4:51 PM, "Corey Minyard" <tcminyard@gmail.com on behalf of minyard@acm.org> wrote: On Tue, Nov 12, 2019 at 10:06:00PM +0000, Asmaa Mnebhi wrote: > Also, let me clarify one thing. It doesn't matter how big the response is. In my testing, I also had some responses that are over 128 bytes, and this driver still works. It is the user space program which determines the last bytes received. The 128 bytes is the max number of bytes handled by your i2c/smbus driver at each i2c transaction. My i2c driver can only transmit 128 bytes at a time. So just like Corey pointed out, it would be better to pass this through ACPI/device tree. Yeah, I would really prefer device tree. That's what it's designed for, really. ioctls are not really what you want for this. sysfs is a better choice as a backup for device tree (so you can change it if it's wrong). Corey/Asmaa, Ok, I will pass this max size through device tree and change this patch. I have sent patch for i2c transfer using ioctl, I hope that should be fine. Please review that v2 patch. >> why can't you pass this information through device tree/ACPI as well? All you need in your DT/ACPI table is a variable that indicates whether it is i2c or smbus. You check that variable in the ipmb driver, then decide which code path to take. I prefer not to use ioctl for system configuration. -corey > > -----Original Message----- > From: Corey Minyard <tcminyard@gmail.com> On Behalf Of Corey Minyard > Sent: Tuesday, November 12, 2019 3:30 PM > To: Vijay Khemka <vijaykhemka@fb.com> > Cc: Arnd Bergmann <arnd@arndb.de>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; openipmi-developer@lists.sourceforge.net; linux-kernel@vger.kernel.org; cminyard@mvista.com; Asmaa Mnebhi <Asmaa@mellanox.com>; joel@jms.id.au; linux-aspeed@lists.ozlabs.org; Sai Dasari <sdasari@fb.com> > Subject: Re: [PATCH 2/2] drivers: ipmi: Modify max length of IPMB packet > > On Tue, Nov 12, 2019 at 07:56:34PM +0000, Vijay Khemka wrote: > > > > > > On 11/12/19, 4:48 AM, "Corey Minyard" <tcminyard@gmail.com on behalf of minyard@acm.org> wrote: > > > > On Mon, Nov 11, 2019 at 06:36:10PM -0800, Vijay Khemka wrote: > > > As per IPMB specification, maximum packet size supported is 255, > > > modified Max length to 240 from 128 to accommodate more data. > > > > I couldn't find this in the IPMB specification. > > > > IIRC, the maximum on I2C is 32 byts, and table 6-9 in the IPMI spec, > > under "IPMB Output" states: The IPMB standard message length is > > specified as 32 bytes, maximum, including slave address. > > > > We are using IPMI OEM messages and our response size is around 150 > > bytes For some of responses. That's why I had set it to 240 bytes. > > Hmm. Well, that is a pretty significant violation of the spec, but there's nothing hard in the protocol that prohibits it, I guess. > > If Asmaa is ok with this, I'm ok with it, too. > > -corey > > > > > I'm not sure where 128 came from, but maybe it should be reduced to 31. > > > > -corey > > > > > > > > Signed-off-by: Vijay Khemka <vijaykhemka@fb.com> > > > --- > > > drivers/char/ipmi/ipmb_dev_int.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/char/ipmi/ipmb_dev_int.c b/drivers/char/ipmi/ipmb_dev_int.c > > > index 2419b9a928b2..7f9198bbce96 100644 > > > --- a/drivers/char/ipmi/ipmb_dev_int.c > > > +++ b/drivers/char/ipmi/ipmb_dev_int.c > > > @@ -19,7 +19,7 @@ > > > #include <linux/spinlock.h> > > > #include <linux/wait.h> > > > > > > -#define MAX_MSG_LEN 128 > > > +#define MAX_MSG_LEN 240 > > > #define IPMB_REQUEST_LEN_MIN 7 > > > #define NETFN_RSP_BIT_MASK 0x4 > > > #define REQUEST_QUEUE_MAX_LEN 256 > > > -- > > > 2.17.1 > > > > > > > > > _______________________________________________ > Openipmi-developer mailing list > Openipmi-developer@lists.sourceforge.net > https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.sourceforge.net_lists_listinfo_openipmi-2Ddeveloper&d=DwIDaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=v9MU0Ki9pWnTXCWwjHPVgpnCR80vXkkcrIaqU7USl5g&m=QFO5ClAjZ5KmfurkhkRpQQxx33h0Q3NZ9eFRbR3vGyk&s=mZUIyiVF1tBXO1v7ZhLWOW_BwIERBToytaV4ULjXhkM&e= ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-11-13 17:53 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20191112023610.3644314-1-vijaykhemka@fb.com>
2019-11-12 12:36 ` [PATCH 1/2] drivers: ipmi: Support raw i2c packet in IPMB Corey Minyard
2019-11-12 14:19 ` Asmaa Mnebhi
2019-11-12 17:57 ` Vijay Khemka
2019-11-12 18:41 ` Corey Minyard
2019-11-12 20:13 ` Vijay Khemka
[not found] ` <20191112023610.3644314-2-vijaykhemka@fb.com>
2019-11-12 12:48 ` [PATCH 2/2] drivers: ipmi: Modify max length of IPMB packet Corey Minyard
2019-11-12 19:56 ` Vijay Khemka
2019-11-12 20:29 ` Corey Minyard
2019-11-12 20:48 ` Asmaa Mnebhi
2019-11-12 22:06 ` Asmaa Mnebhi
2019-11-12 23:19 ` Vijay Khemka
2019-11-13 0:51 ` [Openipmi-developer] " Corey Minyard
2019-11-13 17:40 ` Vijay Khemka
2019-11-13 17:52 ` Asmaa Mnebhi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox