Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <1606533966-22821-1-git-send-email-hemantk@codeaurora.org> <1606533966-22821-5-git-send-email-hemantk@codeaurora.org>
In-Reply-To: <1606533966-22821-5-git-send-email-hemantk@codeaurora.org>
From:   Loic Poulain <loic.poulain@linaro.org>
Date:   Mon, 30 Nov 2020 19:22:44 +0100
Message-ID: <CAMZdPi8z+-qFqgZ7AFJcNAUMbDQtNN5Hz-geMBcp4azrUGm9iA@mail.gmail.com>
Subject: Re: [PATCH v13 4/4] bus: mhi: Add userspace client interface driver
To:     Hemant Kumar <hemantk@codeaurora.org>
Cc:     Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-arm-msm <linux-arm-msm@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>,
        Jeffrey Hugo <jhugo@codeaurora.org>,
        Bhaumik Bhatt <bbhatt@codeaurora.org>,
        Network Development <netdev@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sat, 28 Nov 2020 at 04:26, Hemant Kumar <hemantk@codeaurora.org> 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

[...]

> +struct uci_chan {
> +       struct uci_dev *udev;
> +       wait_queue_head_t ul_wq;
> +
> +       /* ul channel lock to synchronize multiple writes */
> +       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;
> +};

[...]

> + * struct uci_dev - MHI UCI device
> + * @minor: UCI device node minor number
> + * @mhi_dev: associated mhi device object
> + * @uchan: UCI uplink and downlink channel object
> + * @mtu: max TRE buffer length
> + * @enabled: Flag to track the state of the UCI device
> + * @lock: mutex lock to manage uchan object
> + * @ref_count: uci_dev reference count
> + */
> +struct uci_dev {
> +       unsigned int minor;
> +       struct mhi_device *mhi_dev;
> +       struct uci_chan *uchan;

Why a pointer to uci_chan and not just plainly integrating the
structure here, AFAIU uci_chan describes the channels and is just a
subpart of uci_dev. That would reduce the number of dynamic
allocations you manage and the extra kref. do you even need a separate
structure for this?

[...]

> +static int mhi_uci_dev_start_chan(struct uci_dev *udev)
> +{
> +       int ret = 0;
> +       struct uci_chan *uchan;
> +
> +       mutex_lock(&udev->lock);
> +       if (!udev->uchan || !kref_get_unless_zero(&udev->uchan->ref_count)) {

This test is suspicious,  kref_get_unless_zero should be enough to test, right?

if (kref_get_unless_zero(&udev->ref))
    goto skip_init;

> +               uchan = kzalloc(sizeof(*uchan), GFP_KERNEL);
> +               if (!uchan) {
> +                       ret = -ENOMEM;
> +                       goto error_chan_start;
> +               }
> +
> +               udev->uchan = uchan;
> +               uchan->udev = udev;
> +               init_waitqueue_head(&uchan->ul_wq);
> +               init_waitqueue_head(&uchan->dl_wq);
> +               mutex_init(&uchan->write_lock);
> +               mutex_init(&uchan->read_lock);
> +               spin_lock_init(&uchan->dl_pending_lock);
> +               INIT_LIST_HEAD(&uchan->dl_pending);
> +
> +               ret = mhi_prepare_for_transfer(udev->mhi_dev);
> +               if (ret) {
> +                       dev_err(&udev->mhi_dev->dev, "Error starting transfer channels\n");
> +                       goto error_chan_cleanup;
> +               }
> +
> +               ret = mhi_queue_inbound(udev);
> +               if (ret)
> +                       goto error_chan_cleanup;
> +
> +               kref_init(&uchan->ref_count);
> +       }
> +
> +       mutex_unlock(&udev->lock);
> +
> +       return 0;
> +
> +error_chan_cleanup:
> +       mhi_uci_dev_chan_release(&uchan->ref_count);
> +error_chan_start:
> +       mutex_unlock(&udev->lock);
> +       return ret;
> +}

Regards,
Loic