Linux-i3c Archive on lore.kernel.org
 help / color / Atom feed
From: Arnd Bergmann <arnd@arndb.de>
To: Vitor Soares <Vitor.Soares@synopsys.com>
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
Subject: Re: [RFC v2 4/4] i3c: add i3cdev module to expose i3c dev in /dev
Date: Wed, 29 Jan 2020 15:30:56 +0100
Message-ID: <CAK8P3a0uFjhuO7e-i3r_RiA_ni=S8MfYoZUwZzmbXRcd=+kMKw@mail.gmail.com> (raw)
In-Reply-To: <442a0c2c52223f9ff1a1d1018ff863fb23105389.1580299067.git.vitor.soares@synopsys.com>

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

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

  reply index

Thread overview: 26+ 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 ` [RFC v2 1/4] i3c: master: export i3c_masterdev_type Vitor Soares
2020-02-17 14:56   ` 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 ` [RFC v2 3/4] i3c: master: add i3c_for_each_dev helper 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 14:30   ` Arnd Bergmann [this message]
2020-01-29 17:00     ` Vitor Soares
2020-01-29 19:39       ` Arnd Bergmann
2020-02-04 13:19         ` Vitor Soares
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 15:06   ` Arnd Bergmann
2020-02-17 15:36     ` Boris Brezillon
2020-02-17 15:55       ` Vitor Soares
2020-02-17 16:03         ` gregkh
2020-02-17 16:12           ` Vitor Soares
2020-02-17 16:23         ` Boris Brezillon
2020-02-17 16:31           ` Arnd Bergmann
2020-02-17 17:06             ` Boris Brezillon
2020-02-17 16:19       ` Arnd Bergmann
2020-02-17 16:34         ` Boris Brezillon
2020-02-17 15:32   ` Vitor Soares
2020-02-17 15:52     ` Boris Brezillon
2020-02-17 17:37   ` Boris Brezillon

Reply instructions:

You may reply publically 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='CAK8P3a0uFjhuO7e-i3r_RiA_ni=S8MfYoZUwZzmbXRcd=+kMKw@mail.gmail.com' \
    --to=arnd@arndb.de \
    --cc=Joao.Pinto@synopsys.com \
    --cc=Jose.Abreu@synopsys.com \
    --cc=Vitor.Soares@synopsys.com \
    --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: link

Linux-i3c Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-i3c/0 linux-i3c/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-i3c linux-i3c/ https://lore.kernel.org/linux-i3c \
		linux-i3c@lists.infradead.org
	public-inbox-index linux-i3c

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-i3c


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git