Messages in this thread | | | From | Vitor Soares <> | Subject | RE: [RFC v2 4/4] i3c: add i3cdev module to expose i3c dev in /dev | Date | Wed, 29 Jan 2020 17:00:11 +0000 |
| |
Hi Arnd,
From: Arnd Bergmann <arnd@arndb.de> Date: Wed, Jan 29, 2020 at 14:30:56
> 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.
Should I use all minors range instead?
> > 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,
Do you mean i3c->dev ?
> or > just embed the cdev inside of i3c_device.
I would prefer to have a pointer in i3c_device for i3cdev_data, but I see others using it in drvdata.
> > > +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?
Could you please provide an example?
> > > + 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.
You are right, it wasn't available went I did it.
> > > + > > +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?
In both cases something is not correct. I will turn both conditions to return ERR_PTR(-EINVAL) and them just check if (IS_ERR(k_xfer)).
> > > + /* 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?
The i3c core is locked during this phase.
> > What happens when a driver attaches to a device that is currently > transferring data on the user interface? >
It may lost references for inode and file. I need to guarantee there no tranfer going on during the detach. Do you have any suggestion?
> Is there any guarantee that the notifiers for attach and detach > are serialized? >
Sorry I didn't get this part.
> > +/** > > + * 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.
Do you think I should add more reserved fields for future?
> > Arnd > > _______________________________________________ > linux-i3c mailing list > linux-i3c@lists.infradead.org > https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.infradead.org_mailman_listinfo_linux-2Di3c&d=DwICAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=pv8xU_wOpDLOkwdQiuBDso73EKvNPX2jXLtBHDVWRFo&s=S-Tesk8Hi3Ok6y9d_ysocHXGt2dmnn-WcM0BxurcDdQ&e=
Thanks for your comments 😊
Best regards, Vitor Soares
| |