lkml.org 
[lkml]   [2020]   [Jan]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [RFC v2 4/4] i3c: add i3cdev module to expose i3c dev in /dev
On Wed, Jan 29, 2020 at 1:17 PM Vitor Soares <Vitor.Soares@synopsys.com> wrote:
>
> +
> +struct i3cdev_data {
> + struct list_head list;
> + struct i3c_device *i3c;
> + struct cdev cdev;
> + struct device *dev;
> + int id;
> +};
> +
> +static DEFINE_IDA(i3cdev_ida);
> +static dev_t i3cdev_number;
> +#define I3C_MINORS 16 /* 16 I3C devices supported for now */
> +
> +static LIST_HEAD(i3cdev_list);
> +static DEFINE_SPINLOCK(i3cdev_list_lock);

Please try to avoid arbitrarily limiting the number of devices you support.

Searching through the list feels a little clumsy. If the i3c user interface is
supposed to become a standard feature of the subsystem, it would seem
appropriate to put a pointer into the device to simplify the lookup, or
just embed the cdev inside of i3c_device.

> +static int
> +i3cdev_do_priv_xfer(struct i3c_device *dev, struct i3c_ioc_priv_xfer *xfers,
> + unsigned int nxfers)
> +{
> + struct i3c_priv_xfer *k_xfers;
> + u8 **data_ptrs;
> + int i, ret = 0;
> +
> + k_xfers = kcalloc(nxfers, sizeof(*k_xfers), GFP_KERNEL);
> + if (!k_xfers)
> + return -ENOMEM;
> +
> + data_ptrs = kcalloc(nxfers, sizeof(*data_ptrs), GFP_KERNEL);
> + if (!data_ptrs) {
> + ret = -ENOMEM;
> + goto err_free_k_xfer;
> + }

Maybe use a combined allocation to simplify the error handling?

> + for (i = 0; i < nxfers; i++) {
> + data_ptrs[i] = memdup_user((const u8 __user *)
> + (uintptr_t)xfers[i].data,
> + xfers[i].len);

> + if (xfers[i].rnw) {
> + if (copy_to_user((void __user *)(uintptr_t)xfers[i].data,
> + data_ptrs[i], xfers[i].len))

Use u64_to_user_ptr() here.

> +
> +static struct i3c_ioc_priv_xfer *
> +i3cdev_get_ioc_priv_xfer(unsigned int cmd, struct i3c_ioc_priv_xfer *u_xfers,
> + unsigned int *nxfers)
> +{
> + u32 tmp = _IOC_SIZE(cmd);
> +
> + if ((tmp % sizeof(struct i3c_ioc_priv_xfer)) != 0)
> + return ERR_PTR(-EINVAL);
> +
> + *nxfers = tmp / sizeof(struct i3c_ioc_priv_xfer);
> + if (*nxfers == 0)
> + return NULL;
> +
> + return memdup_user(u_xfers, tmp);
> +}
> +
> +static int
> +i3cdev_ioc_priv_xfer(struct i3c_device *i3c, unsigned int cmd,
> + struct i3c_ioc_priv_xfer *u_xfers)
> +{
> + struct i3c_ioc_priv_xfer *k_xfers;
> + unsigned int nxfers;
> + int ret;
> +
> + k_xfers = i3cdev_get_ioc_priv_xfer(cmd, u_xfers, &nxfers);
> + if (IS_ERR_OR_NULL(k_xfers))
> + return PTR_ERR(k_xfers);
> +
> + ret = i3cdev_do_priv_xfer(i3c, k_xfers, nxfers);

The IS_ERR_OR_NULL() usage looks suspicious. It's generally
better to avoid interfaces that require this. What does it mean to
return NULL from i3cdev_get_ioc_priv_xfer() and turn that into
success? Could you handle this condition in the caller instead,
or turn it into an error?

> + /* Keep track of busses which have devices to add or remove later */
> + res = bus_register_notifier(&i3c_bus_type, &i3c_notifier);
> + if (res)
> + goto out_unreg_class;
> +
> + /* Bind to already existing device without driver right away */
> + i3c_for_each_dev(NULL, i3cdev_attach);

The combination of the notifier and searching through the devices
seems to be racy. What happens when a device appears just before
or during the i3c_for_each_dev() traversal?

What happens when a driver attaches to a device that is currently
transferring data on the user interface?

Is there any guarantee that the notifiers for attach and detach
are serialized?

> +/**
> + * struct i3c_ioc_priv_xfer - I3C SDR ioctl private transfer
> + * @data: Holds pointer to userspace buffer with transmit data.
> + * @len: Length of data buffer buffers, in bytes.
> + * @rnw: encodes the transfer direction. true for a read, false for a write
> + */
> +struct i3c_ioc_priv_xfer {
> + __u64 data;
> + __u16 len;
> + __u8 rnw;
> + __u8 pad[5];
> +};
> +
> +
> +#define I3C_PRIV_XFER_SIZE(N) \
> + ((((sizeof(struct i3c_ioc_priv_xfer)) * (N)) < (1 << _IOC_SIZEBITS)) \
> + ? ((sizeof(struct i3c_ioc_priv_xfer)) * (N)) : 0)
> +
> +#define I3C_IOC_PRIV_XFER(N) \
> + _IOC(_IOC_READ|_IOC_WRITE, I3C_DEV_IOC_MAGIC, 30, I3C_PRIV_XFER_SIZE(N))

This looks like a reasonable ioctl definition, avoiding the usual problems
with compat mode etc.

Arnd

\
 
 \ /
  Last update: 2020-01-29 15:31    [W:0.140 / U:0.036 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site