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
Attachment:
signature.asc
Description: PGP signature
