Re: [PATCH] bus: mhi: Add inbound buffers allocation flag
From: Manivannan Sadhasivam <hidden>
Date: 2021-01-08 15:31:35
On Fri, Jan 08, 2021 at 03:01:59PM +0100, Loic Poulain wrote:
Hi Mani, On Fri, 8 Jan 2021 at 14:44, Manivannan Sadhasivam < manivannan.sadhasivam@linaro.org> wrote:quoted
On Wed, Jan 06, 2021 at 02:43:43PM +0100, Loic Poulain wrote:quoted
Currently, the MHI controller driver defines which channels should have their inbound buffers allocated and queued. But ideally, this is something that should be decided by the MHI device driver instead,We call them, "MHI client drivers"I'll fix that.quoted
quoted
which actually deals with that buffers. Add a flag parameter to mhi_prepare_for_transfer allowing to specify if buffers have to be allocated and queued by the MHI stack. Keep auto_queue flag for now, but should be removed at some point. Signed-off-by: Loic Poulain <redacted> --- drivers/bus/mhi/core/internal.h | 2 +- drivers/bus/mhi/core/main.c | 11 ++++++++--- drivers/net/mhi_net.c | 2 +- include/linux/mhi.h | 12 +++++++++++- net/qrtr/mhi.c | 2 +- 5 files changed, 22 insertions(+), 7 deletions(-)[...]quoted
diff --git a/drivers/net/mhi_net.c b/drivers/net/mhi_net.c index fa41d8c..b7f7f2e 100644 --- a/drivers/net/mhi_net.c +++ b/drivers/net/mhi_net.c@@ -265,7 +265,7 @@ static int mhi_net_probe(struct mhi_device *mhi_dev, u64_stats_init(&mhi_netdev->stats.tx_syncp); /* Start MHI channels */ - err = mhi_prepare_for_transfer(mhi_dev); + err = mhi_prepare_for_transfer(mhi_dev, 0);Eventhough I'd like Hemant to comment on this patch, AFAIU this looks to me a controller dependent behaviour. The controller should have the information whether a particular channel can auto queue or not then the client driver can be agnostic.The client driver can not be agnostic if this information is defined on the controller side. In one case client driver needs to allocate (and queue) its own buffers and in the other case it uses the pre-allocated ones. Moreover, that will break compatibility if we have one controller (e.g. a Wifi MHI controller) that e.g. defines IPCR channels as pre-allocated and another one that defines IPCR channels as non-pre-allocated. Having pre-allocated channels is not something related to the MHI device but to how the host (client driver) wants to manage buffers. It would then make sense to let this choice to the client driver.quoted
quoted
if (err) goto out_err;diff --git a/include/linux/mhi.h b/include/linux/mhi.h index 209b335..6723339 100644 --- a/include/linux/mhi.h +++ b/include/linux/mhi.h@@ -60,6 +60,14 @@ enum mhi_flags { }; /** + * enum mhi_chan_flags - MHI channel flags + * @MHI_CH_INBOUND_ALLOC_BUFS: Automatically allocate and queue inboundbuffersquoted
+ */ +enum mhi_chan_flags { + MHI_CH_INBOUND_ALLOC_BUFS = BIT(0), +}; + +/** * enum mhi_device_type - Device types * @MHI_DEVICE_XFER: Handles data transfer * @MHI_DEVICE_CONTROLLER: Control device@@ -705,8 +713,10 @@ void mhi_device_put(struct mhi_device *mhi_dev); /** * mhi_prepare_for_transfer - Setup channel for data transfer * @mhi_dev: Device associated with the channels + * @flags: MHI channel flags */ -int mhi_prepare_for_transfer(struct mhi_device *mhi_dev); +int mhi_prepare_for_transfer(struct mhi_device *mhi_dev, + enum mhi_chan_flags flags); /** * mhi_unprepare_from_transfer - Unprepare the channelsdiff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c index 2bf2b19..47afded 100644 --- a/net/qrtr/mhi.c +++ b/net/qrtr/mhi.c@@ -77,7 +77,7 @@ static int qcom_mhi_qrtr_probe(struct mhi_device*mhi_dev,quoted
int rc; /* start channels */ - rc = mhi_prepare_for_transfer(mhi_dev); + rc = mhi_prepare_for_transfer(mhi_dev, MHI_CH_INBOUND_ALLOC_BUFS);Are you sure it requires auto queued channel?This is how mhi-qrtr has been implemented, yes.
skb is allocated in qrtr_endpoint_post(). Then how the host can pre allocate the buffer here? Am I missing something? Thanks, Mani
Regards, Loic