From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.7 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3CFB0C71156 for ; Tue, 1 Dec 2020 17:30:57 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id D5C152151B for ; Tue, 1 Dec 2020 17:30:56 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="z1hQQuuH" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2404159AbgLARap (ORCPT ); Tue, 1 Dec 2020 12:30:45 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46142 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2404132AbgLARao (ORCPT ); Tue, 1 Dec 2020 12:30:44 -0500 Received: from mail-ej1-x644.google.com (mail-ej1-x644.google.com [IPv6:2a00:1450:4864:20::644]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E4C91C061A48 for ; Tue, 1 Dec 2020 09:29:55 -0800 (PST) Received: by mail-ej1-x644.google.com with SMTP id x16so5766143ejj.7 for ; Tue, 01 Dec 2020 09:29:55 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=3kgOHBtixErnriEtsg2lfKIcWc7GFIxP/NgjDBm37sI=; b=z1hQQuuHgm+BjH29ILhlPeLmWwwCdZq4ZBQW4CzFPkSQebnGsozAhhN9l71quZlehJ 6ALc5hrDwb/hpuRn7V1XHQXfEGV18XQUXGdJD967uAxqJ1NrOWLQsDNPAKKJ3UkYc5QU 5HBUjG2fuiEv1J8DdDk/HNbBBfuB8cB4eoRZqktoKSXiipAMsEKBMqS76bk/CxpBYWUa 6DcWsCIkzMqD+W2pMjhQy38TZsJ6xrTDFPGBKVyr747VovFB1hD5NEzhHT5bP0sF6+is OkjkJqJA7LZOA4bx2JoCiNVmFgUheXOIkMegvc+JUAlCgeesf8HQSsdiPxUTWyB2q5ZE 9zhg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=3kgOHBtixErnriEtsg2lfKIcWc7GFIxP/NgjDBm37sI=; b=byNQO1DoePmHs8srA6XLsSVrjONE5mItPXGmls6xpkl0A3n7X1S27KtVuqPw4uGoJ4 C0m1+TzTcuVPcEIsEIF7Oz1ha2/gUzntCSKB5jl8/pjTYHRs8XMXfkAt+xvGdUzfXQNK YbxwgIAHwT/6Z0LCwkmS61ixHjGLYmUbptJtfLOxA9hiVQt9SjjLcbSjILGKKzatdLvm bE1FRmWVrfgpK31X8Fk8Vl3aClU+D2RYunx67j5Xugyz2SZadHTX/2bE5R1+ZegiKpzj Yeru8uo1QjmZGjhWoNUbCcY6TZVJTUPlifiaZNXr4QcDBHO4bU05NAHAKWct12yiO2aU 8vBA== X-Gm-Message-State: AOAM532hZV/4yK6/HELcZpce7DtwkV/LLTQNmyz6Sbeqif+osE33L8j3 uWnZYSZeED0iNxkST9SjY/5m+id2FK9iPiMKX5I4+A== X-Google-Smtp-Source: ABdhPJy02S0PbdqT+IuCzCZMaQvDeZJBuyN6C6ly+NWkFyziycGrzVIIQyOTk0uFU37LCSPHtCmpSAU1JYK3zQ2D3Rc= X-Received: by 2002:a17:906:411b:: with SMTP id j27mr4045057ejk.466.1606843794518; Tue, 01 Dec 2020 09:29:54 -0800 (PST) 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: From: Loic Poulain Date: Tue, 1 Dec 2020 18:36:09 +0100 Message-ID: Subject: Re: [PATCH v13 4/4] bus: mhi: Add userspace client interface driver To: Hemant Kumar Cc: Manivannan Sadhasivam , Greg Kroah-Hartman , linux-arm-msm , open list , Jeffrey Hugo , Bhaumik Bhatt , Network Development Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 1 Dec 2020 at 02:16, Hemant Kumar wrote: > > Hi Loic, > > On 11/30/20 10:22 AM, Loic Poulain wrote: > > On Sat, 28 Nov 2020 at 04:26, 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 > > > > [...] > > > >> +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? > > This goes back to one of my patch versions i tried to address concern > from Greg. Since we need to ref count the channel as well as the uci > device i decoupled the two objects and used two reference counts for two > different objects. What Greg complained about is the two kref in the same structure and that you were using kref as an open() counter. But splitting your struct in two in order to keep the two kref does not make the much code better (and simpler). I'm still a bit puzzled about the driver complexity, it's supposed to be just a passthrough interface to MHI after all. I would suggest several changes, that IMHO would simplify reviewing: - Use only one structure representing the 'uci' context (uci_dev) - Keep the read path simple (mhi_uci_read), do no use an intermediate cur_buf pointer, only dequeue the buffer when it is fully consumed. - As I commented before, take care of the dl_pending list access concurrency, even in wait_event. - You don't need to count the number of open() calls, AFAIK, mhi_prepare_for_transfer() simply fails if channels are already started... For testing purpose, I've implemented those changes on my side (based on your driver): https://git.linaro.org/landing-teams/working/telit/linux.git/commit/?h=mhi_uci_test&id=45ff60703cc26913061a26260e39cf3ab3e57c2b Feel free to pick (or not), I'm not going to 'block' that series if others are fine with the current implementation. Anyway, I've tested your series and it works on my side with libqmi/qmicli (controlling SDX55 modem): Tested-by: Loic Poulain Regards, Loic