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>,
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 20:39:41 +0100 [thread overview]
Message-ID: <CAK8P3a384ksr95FTxcxr=48G-ytUqmAru7g1JT-Pdfpt1DcLMg@mail.gmail.com> (raw)
In-Reply-To: <CH2PR12MB4216ED068AD93C43B2C421A8AE050@CH2PR12MB4216.namprd12.prod.outlook.com>
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
_______________________________________________
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 19:40 UTC|newest]
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
2020-01-29 17:00 ` Vitor Soares
2020-01-29 19:39 ` Arnd Bergmann [this message]
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 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='CAK8P3a384ksr95FTxcxr=48G-ytUqmAru7g1JT-Pdfpt1DcLMg@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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).