Message-ID: <20190506181148.GA20968@kunai>
Date:   Mon, 6 May 2019 20:11:48 +0200
From:   Wolfram Sang <wsa@...-dreams.de>
To:     Asmaa Mnebhi <Asmaa@...lanox.com>
Cc:     minyard@....org, vadimp@...lanox.com, michaelsh@...lanox.com,
        linux-kernel@...r.kernel.org, linux-i2c@...r.kernel.org
Subject: Re: [PATCH v7 1/1] Add support for IPMB driver

Hi,

some more words from the I2C world.

> +For instance, you can instantiate the ipmb-dev-int device from
> +user space at the 7 bit address 0x10 on bus 2:
> +
> +  # echo ipmb-dev 0x10 > /sys/bus/i2c/devices/i2c-2/new_device

"0x1010" as described in Documentation/i2c/slave-interface

> +config IPMB_DEVICE_INTERFACE
> +       tristate 'IPMB Interface handler'
> +       depends on I2C && I2C_SLAVE

Minor nit: I2C could be dropped because I2C_SLAVE depends on it.

> --- a/drivers/char/ipmi/Makefile
> +++ b/drivers/char/ipmi/Makefile
> @@ -26,3 +26,4 @@ obj-$(CONFIG_IPMI_KCS_BMC) += kcs_bmc.o
>  obj-$(CONFIG_ASPEED_BT_IPMI_BMC) += bt-bmc.o
>  obj-$(CONFIG_ASPEED_KCS_IPMI_BMC) += kcs_bmc_aspeed.o
>  obj-$(CONFIG_NPCM7XX_KCS_IPMI_BMC) += kcs_bmc_npcm7xx.o
> +obj-$(CONFIG_IPMB_DEVICE_INTERFACE) += ipmb_dev_int.o

Dunno if IPMI maintainers care about sorting here?

> +#define	dev_fmt(fmt) "ipmb_dev_int: " fmt

I think this can go now. dev_* with miscchar device (as it is done now)
should be all good.

> +#define	MAX_MSG_LEN		128
> +#define	IPMB_REQUEST_LEN_MIN	7
> +#define	NETFN_RSP_BIT_MASK	0x4
> +#define	REQUEST_QUEUE_MAX_LEN	256
> +
> +#define	IPMB_MSG_LEN_IDX	0
> +#define	RQ_SA_8BIT_IDX		1
> +#define	NETFN_LUN_IDX		2
> +
> +#define	IPMB_MSG_PAYLOAD_LEN_MAX (MAX_MSG_LEN - IPMB_REQUEST_LEN_MIN - 1)
> +
> +#define	SMBUS_MSG_HEADER_LENGTH	2
> +#define	SMBUS_MSG_IDX_OFFSET	(SMBUS_MSG_HEADER_LENGTH + 1)
> +
> +#define	GET_8BIT_ADDR(addr_7bit) ((addr_7bit << 1) && 0xff)

Still wondering about the tabs after define.

> +static int receive_ipmb_request(struct ipmb_dev *ipmb_dev_p,
> +				bool non_blocking,
> +				struct ipmb_msg *ipmb_request)
> +{
> +	struct ipmb_request_elem *queue_elem;
> +	unsigned long flags;
> +	int res;
> +
> +	spin_lock_irqsave(&ipmb_dev_p->lock, flags);
> +
> +	while (!atomic_read(&ipmb_dev_p->request_queue_len)) {
> +		spin_unlock_irqrestore(&ipmb_dev_p->lock, flags);
> +		if (non_blocking)
> +			return -EAGAIN;
> +
> +		res = wait_event_interruptible(ipmb_dev_p->wait_queue,
> +				atomic_read(&ipmb_dev_p->request_queue_len));
> +		if (res)
> +			return res;
> +
> +		spin_lock_irqsave(&ipmb_dev_p->lock, flags);
> +	}
> +
> +	if (list_empty(&ipmb_dev_p->request_queue)) {
> +		dev_err(&ipmb_dev_p->client->dev, "request_queue is empty\n");

Spinlock still held?? Kinda proves that the flow of code is still hard
to read. (I mean it is way better without the goto, but still...)

> +		return -EIO;
> +	}
> +
> +	queue_elem = list_first_entry(&ipmb_dev_p->request_queue,
> +					struct ipmb_request_elem, list);
> +	memcpy(ipmb_request, &queue_elem->request, sizeof(*ipmb_request));
> +	list_del(&queue_elem->list);
> +	kfree(queue_elem);
> +	atomic_dec(&ipmb_dev_p->request_queue_len);
> +
> +	spin_unlock_irqrestore(&ipmb_dev_p->lock, flags);
> +
> +	return 0;
> +}
> +

...

> +static s32 i2c_smbus_write_block_data_local(struct i2c_client *client,
> +					u8 command, u8 length,
> +					u16 requester_i2c_addr,
> +					const char *msg)
> +{
> +	union i2c_smbus_data data;
> +	int ret;
> +
> +	if (length > I2C_SMBUS_BLOCK_MAX)
> +		length = I2C_SMBUS_BLOCK_MAX;
> +
> +	data.block[0] = length;
> +	memcpy(&data.block[1], msg, length);
> +
> +	ret = i2c_smbus_xfer(client->adapter, requester_i2c_addr,
> +				client->flags,
> +				I2C_SMBUS_WRITE, command,
> +				I2C_SMBUS_BLOCK_DATA, &data);
> +
> +	return ret;
> +}

This function must go. You need it solely to pass 'requester_i2c_addr'
along, but this shows that you are using the wrong i2c_client struct.
And, in deed, you don't want your own here because you don't want to
send to yourself here. Usually, you'd register a new i2c_client with the
address you want to talk to using 'i2c_new_device'. However, since this
is a userspace interface, I guess we can do something similar as in
i2c-dev.c, namely an anonymous i2c_client. Read on.

> +
> +static ssize_t ipmb_write(struct file *file, const char __user *buf,
> +			size_t count, loff_t *ppos)
> +{
> +	struct ipmb_dev *ipmb_dev_p = to_ipmb_dev(file);
> +	u8 msg[MAX_MSG_LEN];
> +	ssize_t ret;
> +	u8 rq_sa, netf_rq_lun, msg_len;

From the top of my head, not sure if I got all details right:

+	struct i2c_client rq_client = { };

> +	if (count > sizeof(msg))
> +		return -EINVAL;
> +
> +	if (copy_from_user(&msg, buf, count) || count < msg[0])
> +		return -EFAULT;
> +
> +	rq_sa = msg[RQ_SA_8BIT_IDX] >> 1;
> +	netf_rq_lun = msg[NETFN_LUN_IDX];

+	rq_client.name = "IPMB anonymous"
+	rq_client.adapter = ipmb_dev_p->client->adapter;
+	rq_client.addr = rq_sa;

> +	/*
> +	 * subtract rq_sa and netf_rq_lun from the length of the msg passed to
> +	 * i2c_smbus_write_block_data_local
> +	 */
> +	msg_len = msg[IPMB_MSG_LEN_IDX] - SMBUS_MSG_HEADER_LENGTH;
> +
> +	mutex_lock(&ipmb_dev_p->file_mutex);
> +	ret = i2c_smbus_write_block_data_local(ipmb_dev_p->client,
> +					netf_rq_lun, msg_len, rq_sa, msg +
> +					SMBUS_MSG_IDX_OFFSET);

and then replace the call above with

	ret = i2c_smbus_write_block_data(&rq_client, ...)

> +	mutex_unlock(&ipmb_dev_p->file_mutex);
> +
> +	return ret ? : count;
> +}
> +

...

> +static int ipmb_probe(struct i2c_client *client,
> +			const struct i2c_device_id *id)
> +{
> +	struct ipmb_dev *ipmb_dev_p;

And I still dislike the _p suffix. It is not common Kernel coding style
and unless it is common in this subsystem, I'd suggest to drop it for
consistency reasons.

Thanks for keeping at this driver,

   Wolfram


Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists