Messages in this thread | | | From | Arnd Bergmann <> | Date | Wed, 29 Jan 2020 20:39:41 +0100 | Subject | Re: [RFC v2 4/4] i3c: add i3cdev module to expose i3c dev in /dev |
| |
On Wed, Jan 29, 2020 at 6:00 PM Vitor Soares <Vitor.Soares@synopsys.com> wrote: > > 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?
Yes, I'm fairly sure that if you use a dynamic major number, there is no downside in using all of them.
> > 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 ?
I was thinking you could add another member in i3c_device, next to ->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.
Ok, I think drvdata should work, but you should check that this is correct when the device goes back between being bound to a device driver and used through the chardev.
> > > > > +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?
Something like
k_xfers = kcalloc(nxfers, sizeof(*k_xfers) + sizeof(*data_ptrs), GFP_KERNEL); data_ptrs = (void *)k_xfers + (nxfers, sizeof(*k_xfers));
This would need a comment to explain the pointer math, but the resulting object code is slightly simpler.
> > > + /* 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.
Ok.
> > 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?
If the notifier is blocking, you could hold another mutex during the transfer I think.
> > Is there any guarantee that the notifiers for attach and detach > > are serialized? > > > > Sorry I didn't get this part.
I think you answered this above: if the i3c code is locked while calling the notifier, this cannot happen.
> > > +/** > > > + * 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?
No, what I meant is that I like it the way it is.
Arnd
| |