From: Vitor Soares <Vitor.Soares@synopsys.com> To: Arnd Bergmann <arnd@arndb.de> Cc: Jose Abreu <Jose.Abreu@synopsys.com>, Joao Pinto <Joao.Pinto@synopsys.com>, Boris Brezillon <bbrezillon@kernel.org>, gregkh <gregkh@linuxfoundation.org>, Wolfram Sang <wsa@the-dreams.de>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, Mark Brown <broonie@kernel.org>, "linux-i3c@lists.infradead.org" <linux-i3c@lists.infradead.org> 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 [thread overview] Message-ID: <CH2PR12MB4216ED068AD93C43B2C421A8AE050@CH2PR12MB4216.namprd12.prod.outlook.com> (raw) In-Reply-To: <CAK8P3a0uFjhuO7e-i3r_RiA_ni=S8MfYoZUwZzmbXRcd=+kMKw@mail.gmail.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: Vitor Soares <Vitor.Soares@synopsys.com> To: Arnd Bergmann <arnd@arndb.de> Cc: Jose Abreu <Jose.Abreu@synopsys.com>, Joao Pinto <Joao.Pinto@synopsys.com>, Wolfram Sang <wsa@the-dreams.de>, gregkh <gregkh@linuxfoundation.org>, Boris Brezillon <bbrezillon@kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, Mark Brown <broonie@kernel.org>, "linux-i3c@lists.infradead.org" <linux-i3c@lists.infradead.org> 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 [thread overview] Message-ID: <CH2PR12MB4216ED068AD93C43B2C421A8AE050@CH2PR12MB4216.namprd12.prod.outlook.com> (raw) In-Reply-To: <CAK8P3a0uFjhuO7e-i3r_RiA_ni=S8MfYoZUwZzmbXRcd=+kMKw@mail.gmail.com> 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 _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c
next prev parent reply other threads:[~2020-01-29 17:00 UTC|newest] Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-01-29 12:17 [RFC v2 0/4] Introduce i3c device userspace interface Vitor Soares 2020-01-29 12:17 ` Vitor Soares 2020-01-29 12:17 ` [RFC v2 1/4] i3c: master: export i3c_masterdev_type Vitor Soares 2020-01-29 12:17 ` Vitor Soares 2020-02-17 14:56 ` Boris Brezillon 2020-02-17 14:56 ` Boris Brezillon 2020-02-17 14:59 ` Boris Brezillon 2020-02-17 14:59 ` Boris Brezillon 2020-01-29 12:17 ` [RFC v2 2/4] i3c: master: export i3c_bus_type symbol Vitor Soares 2020-01-29 12:17 ` Vitor Soares 2020-01-29 12:17 ` [RFC v2 3/4] i3c: master: add i3c_for_each_dev helper Vitor Soares 2020-01-29 12:17 ` Vitor Soares 2020-01-29 12:17 ` [RFC v2 4/4] i3c: add i3cdev module to expose i3c dev in /dev Vitor Soares 2020-01-29 12:17 ` Vitor Soares 2020-01-29 14:30 ` Arnd Bergmann 2020-01-29 14:30 ` Arnd Bergmann 2020-01-29 17:00 ` Vitor Soares [this message] 2020-01-29 17:00 ` Vitor Soares 2020-01-29 19:39 ` Arnd Bergmann 2020-01-29 19:39 ` Arnd Bergmann 2020-02-04 13:19 ` Vitor Soares 2020-02-04 13:19 ` Vitor Soares 2020-02-17 15:26 ` Boris Brezillon 2020-02-17 15:26 ` Boris Brezillon 2020-02-17 14:51 ` [RFC v2 0/4] Introduce i3c device userspace interface Boris Brezillon 2020-02-17 14:51 ` Boris Brezillon 2020-02-17 15:06 ` Arnd Bergmann 2020-02-17 15:06 ` Arnd Bergmann 2020-02-17 15:36 ` Boris Brezillon 2020-02-17 15:36 ` Boris Brezillon 2020-02-17 15:55 ` Vitor Soares 2020-02-17 15:55 ` Vitor Soares 2020-02-17 16:03 ` gregkh 2020-02-17 16:03 ` gregkh 2020-02-17 16:12 ` Vitor Soares 2020-02-17 16:12 ` Vitor Soares 2020-02-17 16:23 ` Boris Brezillon 2020-02-17 16:23 ` Boris Brezillon 2020-02-17 16:31 ` Arnd Bergmann 2020-02-17 16:31 ` Arnd Bergmann 2020-02-17 17:06 ` Boris Brezillon 2020-02-17 17:06 ` Boris Brezillon 2020-02-17 16:19 ` Arnd Bergmann 2020-02-17 16:19 ` Arnd Bergmann 2020-02-17 16:34 ` Boris Brezillon 2020-02-17 16:34 ` Boris Brezillon 2020-02-17 15:32 ` Vitor Soares 2020-02-17 15:32 ` Vitor Soares 2020-02-17 15:52 ` Boris Brezillon 2020-02-17 15:52 ` Boris Brezillon 2020-02-17 17:37 ` Boris Brezillon 2020-02-17 17:37 ` Boris Brezillon
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CH2PR12MB4216ED068AD93C43B2C421A8AE050@CH2PR12MB4216.namprd12.prod.outlook.com \ --to=vitor.soares@synopsys.com \ --cc=Joao.Pinto@synopsys.com \ --cc=Jose.Abreu@synopsys.com \ --cc=arnd@arndb.de \ --cc=bbrezillon@kernel.org \ --cc=broonie@kernel.org \ --cc=gregkh@linuxfoundation.org \ --cc=linux-i3c@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=wsa@the-dreams.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.