lkml.org 
[lkml]   [2020]   [Feb]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [RFC v2 4/4] i3c: add i3cdev module to expose i3c dev in /dev
Date
From: Arnd Bergmann <arnd@arndb.de>
Date: Wed, Jan 29, 2020 at 19:39:41

> 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.

I changed the detach to be done in BUS_NOTIFY_BIND_DRIVER.

>
> > >
> > > > +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.

As we have nferxs, there is no problem to allocate k_xfers more than
needed, right?

>
> > > > + /* 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.

A mutex during the transfer will solve the detach issue, I doing some
tests but even with the change to BUS_NOTIFY_BIND_DRIVER I'm not sure if
it can race with driver probe function.

>
> > > 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.
>

The i3c code is only locked during the i3c_for_each_dev().

> > > > +/**
> > > > + * 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

Best regards,
Vitor Soares
\
 
 \ /
  Last update: 2020-02-04 14:20    [W:0.064 / U:1.860 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site