Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Asmaa Mnebhi <Asmaa@mellanox.com>
To:     "minyard@acm.org" <minyard@acm.org>,
        Vijay Khemka <vijaykhemka@fb.com>
CC:     Arnd Bergmann <arnd@arndb.de>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "openipmi-developer@lists.sourceforge.net" 
        <openipmi-developer@lists.sourceforge.net>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "cminyard@mvista.com" <cminyard@mvista.com>,
        "joel@jms.id.au" <joel@jms.id.au>,
        "linux-aspeed@lists.ozlabs.org" <linux-aspeed@lists.ozlabs.org>,
        "sdasari@fb.com" <sdasari@fb.com>
Subject: RE: [PATCH 1/2] drivers: ipmi: Support raw i2c packet in IPMB
Thread-Topic: [PATCH 1/2] drivers: ipmi: Support raw i2c packet in IPMB
Thread-Index: AQHVmQJhVFa2qHxMQE+ZOphOmDlBEKeHeXcAgAAc2cA=
Date:   Tue, 12 Nov 2019 14:19:43 +0000
Message-ID: <DB6PR0501MB271273D1EC0CF9CAA67A22CCDA770@DB6PR0501MB2712.eurprd05.prod.outlook.com>
References: <20191112023610.3644314-1-vijaykhemka@fb.com>
 <20191112123602.GD2882@minyard.net>
In-Reply-To: <20191112123602.GD2882@minyard.net>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: mellanox.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: c3e66bec-4079-4eee-037a-08d7677b5cf8
X-MS-Exchange-CrossTenant-originalarrivaltime: 12 Nov 2019 14:19:43.3471
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 16k3ZLW91NV+s9QAiSkiMWB8eC2D2BdsqXC/Ue4Egsrhl7/wkpPmh0KDf5w08FeBwoMbyjVesluloRKI2ziXZw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB6PR0501MB2599
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

I agree with corey. You can take a look at the ipmi_ssif.c driver which doe=
s 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@linuxfoundati=
on.org>; openipmi-developer@lists.sourceforge.net; linux-kernel@vger.kernel=
.org; cminyard@mvista.com; Asmaa Mnebhi <Asmaa@mellanox.com>; joel@jms.id.a=
u; 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=20
> 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 f=
or choosing between two mutually exclusive choices are generally not.

-corey

>=20
> 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(+)
>=20
> diff --git a/drivers/char/ipmi/Kconfig b/drivers/char/ipmi/Kconfig=20
> 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=20
> 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 c=
har __user *buf,
>  	struct ipmb_dev *ipmb_dev =3D 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;
> =20
> @@ -133,6 +137,31 @@ static ssize_t ipmb_write(struct file *file, const c=
har __user *buf,
>  	rq_sa =3D GET_7BIT_ADDR(msg[RQ_SA_8BIT_IDX]);
>  	netf_rq_lun =3D msg[NETFN_LUN_IDX];
> =20
> +#ifdef CONFIG_IPMB_SMBUS_DISABLE
> +	/*
> +	 * subtract 1 byte (rq_sa) from the length of the msg passed to
> +	 * raw i2c_transfer
> +	 */
> +	msg_len =3D msg[IPMB_MSG_LEN_IDX] - 1;
> +
> +	i2c_buf =3D 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 =3D rq_sa;
> +	i2c_msg.flags =3D ipmb_dev->client->flags &
> +			(I2C_M_TEN | I2C_CLIENT_PEC | I2C_CLIENT_SCCB);
> +	i2c_msg.len =3D msg_len;
> +	i2c_msg.buf =3D i2c_buf;
> +
> +	ret =3D i2c_transfer(ipmb_dev->client->adapter, &i2c_msg, 1);
> +	kfree(i2c_buf);
> +
> +	return (ret =3D=3D 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 ch=
ar __user *buf,
>  			     I2C_SMBUS_BLOCK_DATA, &data);
> =20
>  	return ret ? : count;
> +#endif
>  }
> =20
>  static unsigned int ipmb_poll(struct file *file, poll_table *wait)
> --
> 2.17.1
>=20