Re: [PATCH v11 4/4] bus: mhi: Add userspace client interface driver

[Manivannan Sadhasivam]

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