hi Hemant,
On Fri, Oct 30, 2020 at 06:26:38PM -0700, Hemant Kumar wrote:
> Hi Mani,
>
> On 10/30/20 3:34 AM, Manivannan Sadhasivam wrote:
> > Hi Hemant,
> >
> > On Thu, Oct 29, 2020 at 07:45:46PM -0700, Hemant Kumar wrote:
> > > This MHI client driver allows userspace clients to transfer
> > > raw data between MHI device and host using standard file operations.
> > > Driver instantiates UCI device object which is associated to device
> > > file node. UCI device object instantiates UCI channel object when device
> > > file node is opened. UCI channel object is used to manage MHI channels
> > > by calling MHI core APIs for read and write operations. MHI channels
> > > are started as part of device open(). MHI channels remain in start
> > > state until last release() is called on UCI device file node. Device
> > > file node is created with format
> > >
> > > /dev/mhi_<controller_name>_<mhi_device_name>
> > >
> > > Currently it supports LOOPBACK channel.
> > >
> > > Signed-off-by: Hemant Kumar <hemantk@xxxxxxxxxxxxxx>
> >
> > Thanks for continuously updating the series based on reviews, now the locking
> > part looks a _lot_ cleaner than it used to be. I just have one query (inline)
> > regarding the usage of refcount for uci_chan and uci_dev. Once you fix that,
> > I think this is good to go in.
> Thanks for reviewing my changes.
>
> [..]
>
> > > +#define DEVICE_NAME "mhi"
> > > +#define MHI_UCI_DRIVER_NAME "mhi_uci"
> > > +#define MAX_UCI_MINORS 128
> >
> > Prefix MHI for these.
> Done.
>
> >
> > > +
> > > +static DEFINE_IDR(uci_idr);
> > > +static DEFINE_MUTEX(uci_drv_mutex);
> > > +static struct class *uci_dev_class;
> > > +static int uci_dev_major;
> > > +
> > > +/**
> > > + * struct uci_chan - MHI channel for a UCI device
> > > + * @udev: associated UCI device object
> > > + * @ul_wq: wait queue for writer
> > > + * @write_lock: mutex write lock for ul channel
> > > + * @dl_wq: wait queue for reader
> > > + * @read_lock: mutex read lock for dl channel
> > > + * @dl_pending_lock: spin lock for dl_pending list
> > > + * @dl_pending: list of dl buffers userspace is waiting to read
> > > + * @cur_buf: current buffer userspace is reading
> > > + * @dl_size: size of the current dl buffer userspace is reading
> > > + * @ref_count: uci_chan reference count
> > > + */
> > > +struct uci_chan {
> > > + struct uci_dev *udev;
> > > + wait_queue_head_t ul_wq;
> > > +
> > > + /* ul channel lock to synchronize multiple writes */
> >
> > I asked you to move these comments to Kdoc in previous iteration.
> There are multiple revisions of UCI pushed after i responded on this one. On
> V7 i responded to your comment :)
>
> "This was added because checkpatch --strict required to add a comment when
> lock is added to struct, after adding inline comment, checkpatch error was
> gone."
>
> i was sticking to --strict option. Considering it is best to address what
> --strict is complaining for.
Ah okay.
> >
> > > + struct mutex write_lock;
> > > +
> > > + wait_queue_head_t dl_wq;
> > > +
> > > + /* dl channel lock to synchronize multiple reads */
> > > + struct mutex read_lock;
> > > +
> > > + /*
> > > + * protects pending list in bh context, channel release, read and
> > > + * poll
> > > + */
> > > + spinlock_t dl_pending_lock;
> > > +
> > > + struct list_head dl_pending;
> > > + struct uci_buf *cur_buf;
> > > + size_t dl_size;
> > > + struct kref ref_count;
> >
> > I'm now thinking that instead of having two reference counts for uci_chan and
> > uci_dev, why can't you club them together and just use uci_dev's refcount to
> > handle the channel management also.
> >
> > For instance in uci_open, you are incrementing the refcount for uci_dev before
> > starting the channel and then doing the same for uci_chan in
> > mhi_uci_dev_start_chan(). So why can't you just use a single refcount once the
> > mhi_uci_dev_start_chan() succeeds? The UCI device is useless without a channel,
> > isn't it?
> Main idea is to have the uci driver probed (uci device object is
> instantiated) but it is possible that device node is not opened or if it was
> opened before and release() was called after that. So UCI channel is not
> active but device node would continue to exist. Which can be opened again
> and channel would move to start state. So we dont want to couple mhi driver
> probe with starting of channels. We start channels only when it is really
> needed. This would allow MHI device to go to lower power state when channels
> are disabled.
>
Okay, makes sense! Please make sure you add it in Documentation.
> [..]
>
> > > +
> > > +static int mhi_queue_inbound(struct uci_dev *udev)
> > > +{
> > > + struct mhi_device *mhi_dev = udev->mhi_dev;
> > > + struct device *dev = &mhi_dev->dev;
> > > + int nr_trbs, i, ret = -EIO;
> >
> > s/nr_trbs/nr_desc
> Done.
> >
> > > + size_t dl_buf_size;
> > > + void *buf;
> > > + struct uci_buf *ubuf;
> > > +
> > > + /* dont queue if dl channel is not supported */
> > > + if (!udev->mhi_dev->dl_chan)
> > > + return 0;
> >
> > Not returning an error?
> Here we dont need to return error because when open is called it would call
> this function and if dl_chan is not supported we still want to return
> success for a uci device which only supports UL channel.
> Keeping this check inside function looks clean so i am not adding this check
> in open().
>
Hmm, okay. Please add a comment regarding this.
Thanks,
Mani
