| Message ID | 1624566520-20406-1-git-send-email-loic.poulain@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] bus: mhi: Add inbound buffers allocation flag | expand |
On Thu 24 Jun 15:28 CDT 2021, Loic Poulain wrote: > 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, > 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 <loic.poulain@linaro.org> > Tested-by: Bhaumik Bhatt <bbhatt@codeaurora.org> > Reviewed-by: Bhaumik Bhatt <bbhatt@codeaurora.org> > Reviewed-by: Hemant Kumar <hemantk@codeaurora.org> > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > Acked-by: Jakub Kicinski <kuba@kernel.org> > Link: https://lore.kernel.org/r/1621603519-16773-1-git-send-email-loic.poulain@linaro.org > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> What's the intention with this patch? Why is Mani the last S-o-b when you're the one sending it and why is it sent only to linux-arm-msm@? And completely separate of the practical matters, is it true that qrtr is the only client that use this pre-allocation feature? Would it be a net gain to simplify the core and add buffer allocation to qrtr instead? Regards, Bjorn > --- > v2: Update API in mhi_wwan_ctrl driver > v3: Do not use enum for flags > > drivers/bus/mhi/core/internal.h | 2 +- > drivers/bus/mhi/core/main.c | 9 ++++++--- > drivers/net/mhi/net.c | 2 +- > drivers/net/wwan/mhi_wwan_ctrl.c | 2 +- > include/linux/mhi.h | 7 ++++++- > net/qrtr/mhi.c | 2 +- > 6 files changed, 16 insertions(+), 8 deletions(-) > > diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h > index 5b9ea66..bc239a1 100644 > --- a/drivers/bus/mhi/core/internal.h > +++ b/drivers/bus/mhi/core/internal.h > @@ -682,7 +682,7 @@ void mhi_rddm_prepare(struct mhi_controller *mhi_cntrl, > struct image_info *img_info); > void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl); > int mhi_prepare_channel(struct mhi_controller *mhi_cntrl, > - struct mhi_chan *mhi_chan); > + struct mhi_chan *mhi_chan, unsigned int flags); > int mhi_init_chan_ctxt(struct mhi_controller *mhi_cntrl, > struct mhi_chan *mhi_chan); > void mhi_deinit_chan_ctxt(struct mhi_controller *mhi_cntrl, > diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c > index 050381d..594fe37 100644 > --- a/drivers/bus/mhi/core/main.c > +++ b/drivers/bus/mhi/core/main.c > @@ -1433,7 +1433,7 @@ static void mhi_unprepare_channel(struct mhi_controller *mhi_cntrl, > } > > int mhi_prepare_channel(struct mhi_controller *mhi_cntrl, > - struct mhi_chan *mhi_chan) > + struct mhi_chan *mhi_chan, unsigned int flags) > { > int ret = 0; > struct device *dev = &mhi_chan->mhi_dev->dev; > @@ -1458,6 +1458,9 @@ int mhi_prepare_channel(struct mhi_controller *mhi_cntrl, > if (ret) > goto error_pm_state; > > + if (mhi_chan->dir == DMA_FROM_DEVICE) > + mhi_chan->pre_alloc = !!(flags & MHI_CH_INBOUND_ALLOC_BUFS); > + > /* Pre-allocate buffer for xfer ring */ > if (mhi_chan->pre_alloc) { > int nr_el = get_nr_avail_ring_elements(mhi_cntrl, > @@ -1613,7 +1616,7 @@ void mhi_reset_chan(struct mhi_controller *mhi_cntrl, struct mhi_chan *mhi_chan) > } > > /* Move channel to start state */ > -int mhi_prepare_for_transfer(struct mhi_device *mhi_dev) > +int mhi_prepare_for_transfer(struct mhi_device *mhi_dev, unsigned int flags) > { > int ret, dir; > struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl; > @@ -1624,7 +1627,7 @@ int mhi_prepare_for_transfer(struct mhi_device *mhi_dev) > if (!mhi_chan) > continue; > > - ret = mhi_prepare_channel(mhi_cntrl, mhi_chan); > + ret = mhi_prepare_channel(mhi_cntrl, mhi_chan, flags); > if (ret) > goto error_open_chan; > } > diff --git a/drivers/net/mhi/net.c b/drivers/net/mhi/net.c > index 0d8293a..774e329 100644 > --- a/drivers/net/mhi/net.c > +++ b/drivers/net/mhi/net.c > @@ -327,7 +327,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); > if (err) > goto out_err; > > diff --git a/drivers/net/wwan/mhi_wwan_ctrl.c b/drivers/net/wwan/mhi_wwan_ctrl.c > index 1bc6b69..1e18420 100644 > --- a/drivers/net/wwan/mhi_wwan_ctrl.c > +++ b/drivers/net/wwan/mhi_wwan_ctrl.c > @@ -110,7 +110,7 @@ static int mhi_wwan_ctrl_start(struct wwan_port *port) > int ret; > > /* Start mhi device's channel(s) */ > - ret = mhi_prepare_for_transfer(mhiwwan->mhi_dev); > + ret = mhi_prepare_for_transfer(mhiwwan->mhi_dev, 0); > if (ret) > return ret; > > diff --git a/include/linux/mhi.h b/include/linux/mhi.h > index 5eb626a..eed949c 100644 > --- a/include/linux/mhi.h > +++ b/include/linux/mhi.h > @@ -719,8 +719,13 @@ void mhi_device_put(struct mhi_device *mhi_dev); > * host and device execution environments match and > * channels are in a DISABLED state. > * @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, > + unsigned int flags); > + > +/* Automatically allocate and queue inbound buffers */ > +#define MHI_CH_INBOUND_ALLOC_BUFS BIT(0) > > /** > * mhi_unprepare_from_transfer - Reset UL and DL channels for data transfer. > diff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c > index fa61167..29b4fa3 100644 > --- a/net/qrtr/mhi.c > +++ b/net/qrtr/mhi.c > @@ -79,7 +79,7 @@ static int qcom_mhi_qrtr_probe(struct mhi_device *mhi_dev, > int rc; > > /* start channels */ > - rc = mhi_prepare_for_transfer(mhi_dev); > + rc = mhi_prepare_for_transfer(mhi_dev, MHI_CH_INBOUND_ALLOC_BUFS); > if (rc) > return rc; > > -- > 2.7.4 >
On 2021-06-24 03:45 PM, Loic Poulain wrote: > Hi Bjorn, > > On Thu, 24 Jun 2021 at 22:30, Bjorn Andersson > <bjorn.andersson@linaro.org> wrote: >> >> On Thu 24 Jun 15:28 CDT 2021, Loic Poulain wrote: >> >> > 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, >> > 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 <loic.poulain@linaro.org> >> > Tested-by: Bhaumik Bhatt <bbhatt@codeaurora.org> >> > Reviewed-by: Bhaumik Bhatt <bbhatt@codeaurora.org> >> > Reviewed-by: Hemant Kumar <hemantk@codeaurora.org> >> > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> >> > Acked-by: Jakub Kicinski <kuba@kernel.org> >> > Link: https://lore.kernel.org/r/1621603519-16773-1-git-send-email-loic.poulain@linaro.org >> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> >> >> What's the intention with this patch? Why is Mani the last S-o-b when >> you're the one sending it and why is it sent only to linux-arm-msm@? > > Actually the previous version of that patch has already been applied > to mhi-next, but has been nacked as part of Mani's PR, so it's a quick > follow-up fix to address the issue. > >> And completely separate of the practical matters, is it true that qrtr >> is the only client that use this pre-allocation feature? > > yes. > >> Would it be a net gain to simplify the core and add buffer allocation >> to qrtr instead? > > Yes, I 100% agree, but I however would prefer to keep that for a > follow-up series since this patch fixes a real issue for MHI/PCI > modems (no inbound QRTR buffers allocated). > > Regards, > Loic I had discussed this with Chris Lew a while ago and he also agreed qrtr can queue inbound buffers. This patch is good and we can continue to have flexibility for clients since this allows MHI controller to not have to bother about some of the unnecessary plugins/booleans we maintain on behalf of clients. They can have more control this way and less headache for us. Thanks, Bhaumik --- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
On Thu 24 Jun 17:45 CDT 2021, Loic Poulain wrote: > Hi Bjorn, > > On Thu, 24 Jun 2021 at 22:30, Bjorn Andersson > <bjorn.andersson@linaro.org> wrote: > > > > On Thu 24 Jun 15:28 CDT 2021, Loic Poulain wrote: > > > > > 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, > > > 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 <loic.poulain@linaro.org> > > > Tested-by: Bhaumik Bhatt <bbhatt@codeaurora.org> > > > Reviewed-by: Bhaumik Bhatt <bbhatt@codeaurora.org> > > > Reviewed-by: Hemant Kumar <hemantk@codeaurora.org> > > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > Acked-by: Jakub Kicinski <kuba@kernel.org> > > > Link: https://lore.kernel.org/r/1621603519-16773-1-git-send-email-loic.poulain@linaro.org > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > What's the intention with this patch? Why is Mani the last S-o-b when > > you're the one sending it and why is it sent only to linux-arm-msm@? > > Actually the previous version of that patch has already been applied > to mhi-next, but has been nacked as part of Mani's PR, so it's a quick > follow-up fix to address the issue. > Thanks, that makes sense. > > And completely separate of the practical matters, is it true that qrtr > > is the only client that use this pre-allocation feature? > > yes. > Then I think we should fix qrtr instead. > > Would it be a net gain to simplify the core and add buffer allocation to qrtr instead? > > Yes, I 100% agree, but I however would prefer to keep that for a > follow-up series since this patch fixes a real issue for MHI/PCI > modems (no inbound QRTR buffers allocated). > I certainly don't mind this patch going upstream while we put such refactoring in the backlog. Thanks, Bjorn
Hi Bjorn, On Thu, 24 Jun 2021 at 22:30, Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > > On Thu 24 Jun 15:28 CDT 2021, Loic Poulain wrote: > > > 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, > > 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 <loic.poulain@linaro.org> > > Tested-by: Bhaumik Bhatt <bbhatt@codeaurora.org> > > Reviewed-by: Bhaumik Bhatt <bbhatt@codeaurora.org> > > Reviewed-by: Hemant Kumar <hemantk@codeaurora.org> > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > Acked-by: Jakub Kicinski <kuba@kernel.org> > > Link: https://lore.kernel.org/r/1621603519-16773-1-git-send-email-loic.poulain@linaro.org > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > What's the intention with this patch? Why is Mani the last S-o-b when > you're the one sending it and why is it sent only to linux-arm-msm@? Actually the previous version of that patch has already been applied to mhi-next, but has been nacked as part of Mani's PR, so it's a quick follow-up fix to address the issue. > And completely separate of the practical matters, is it true that qrtr > is the only client that use this pre-allocation feature? yes. > Would it be a net gain to simplify the core and add buffer allocation to qrtr instead? Yes, I 100% agree, but I however would prefer to keep that for a follow-up series since this patch fixes a real issue for MHI/PCI modems (no inbound QRTR buffers allocated). Regards, Loic
On Fri, Jun 25, 2021 at 12:45:25AM +0200, Loic Poulain wrote: > Hi Bjorn, > > On Thu, 24 Jun 2021 at 22:30, Bjorn Andersson > <bjorn.andersson@linaro.org> wrote: > > > > On Thu 24 Jun 15:28 CDT 2021, Loic Poulain wrote: > > > > > 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, > > > 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 <loic.poulain@linaro.org> > > > Tested-by: Bhaumik Bhatt <bbhatt@codeaurora.org> > > > Reviewed-by: Bhaumik Bhatt <bbhatt@codeaurora.org> > > > Reviewed-by: Hemant Kumar <hemantk@codeaurora.org> > > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > Acked-by: Jakub Kicinski <kuba@kernel.org> > > > Link: https://lore.kernel.org/r/1621603519-16773-1-git-send-email-loic.poulain@linaro.org > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > What's the intention with this patch? Why is Mani the last S-o-b when > > you're the one sending it and why is it sent only to linux-arm-msm@? > > Actually the previous version of that patch has already been applied > to mhi-next, but has been nacked as part of Mani's PR, so it's a quick > follow-up fix to address the issue. > Thanks but you could've removed my s-o-b and "Link" as my script will attach them again while applying ;) But no worries, I'll deal with it. > > And completely separate of the practical matters, is it true that qrtr > > is the only client that use this pre-allocation feature? > > yes. > > > Would it be a net gain to simplify the core and add buffer allocation to qrtr instead? > > Yes, I 100% agree, but I however would prefer to keep that for a > follow-up series since this patch fixes a real issue for MHI/PCI > modems (no inbound QRTR buffers allocated). > Yeah, let's do that in 5.15. Thanks, Mani > Regards, > Loic
diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h index 5b9ea66..bc239a1 100644 --- a/drivers/bus/mhi/core/internal.h +++ b/drivers/bus/mhi/core/internal.h @@ -682,7 +682,7 @@ void mhi_rddm_prepare(struct mhi_controller *mhi_cntrl, struct image_info *img_info); void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl); int mhi_prepare_channel(struct mhi_controller *mhi_cntrl, - struct mhi_chan *mhi_chan); + struct mhi_chan *mhi_chan, unsigned int flags); int mhi_init_chan_ctxt(struct mhi_controller *mhi_cntrl, struct mhi_chan *mhi_chan); void mhi_deinit_chan_ctxt(struct mhi_controller *mhi_cntrl, diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c index 050381d..594fe37 100644 --- a/drivers/bus/mhi/core/main.c +++ b/drivers/bus/mhi/core/main.c @@ -1433,7 +1433,7 @@ static void mhi_unprepare_channel(struct mhi_controller *mhi_cntrl, } int mhi_prepare_channel(struct mhi_controller *mhi_cntrl, - struct mhi_chan *mhi_chan) + struct mhi_chan *mhi_chan, unsigned int flags) { int ret = 0; struct device *dev = &mhi_chan->mhi_dev->dev; @@ -1458,6 +1458,9 @@ int mhi_prepare_channel(struct mhi_controller *mhi_cntrl, if (ret) goto error_pm_state; + if (mhi_chan->dir == DMA_FROM_DEVICE) + mhi_chan->pre_alloc = !!(flags & MHI_CH_INBOUND_ALLOC_BUFS); + /* Pre-allocate buffer for xfer ring */ if (mhi_chan->pre_alloc) { int nr_el = get_nr_avail_ring_elements(mhi_cntrl, @@ -1613,7 +1616,7 @@ void mhi_reset_chan(struct mhi_controller *mhi_cntrl, struct mhi_chan *mhi_chan) } /* Move channel to start state */ -int mhi_prepare_for_transfer(struct mhi_device *mhi_dev) +int mhi_prepare_for_transfer(struct mhi_device *mhi_dev, unsigned int flags) { int ret, dir; struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl; @@ -1624,7 +1627,7 @@ int mhi_prepare_for_transfer(struct mhi_device *mhi_dev) if (!mhi_chan) continue; - ret = mhi_prepare_channel(mhi_cntrl, mhi_chan); + ret = mhi_prepare_channel(mhi_cntrl, mhi_chan, flags); if (ret) goto error_open_chan; } diff --git a/drivers/net/mhi/net.c b/drivers/net/mhi/net.c index 0d8293a..774e329 100644 --- a/drivers/net/mhi/net.c +++ b/drivers/net/mhi/net.c @@ -327,7 +327,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); if (err) goto out_err; diff --git a/drivers/net/wwan/mhi_wwan_ctrl.c b/drivers/net/wwan/mhi_wwan_ctrl.c index 1bc6b69..1e18420 100644 --- a/drivers/net/wwan/mhi_wwan_ctrl.c +++ b/drivers/net/wwan/mhi_wwan_ctrl.c @@ -110,7 +110,7 @@ static int mhi_wwan_ctrl_start(struct wwan_port *port) int ret; /* Start mhi device's channel(s) */ - ret = mhi_prepare_for_transfer(mhiwwan->mhi_dev); + ret = mhi_prepare_for_transfer(mhiwwan->mhi_dev, 0); if (ret) return ret; diff --git a/include/linux/mhi.h b/include/linux/mhi.h index 5eb626a..eed949c 100644 --- a/include/linux/mhi.h +++ b/include/linux/mhi.h @@ -719,8 +719,13 @@ void mhi_device_put(struct mhi_device *mhi_dev); * host and device execution environments match and * channels are in a DISABLED state. * @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, + unsigned int flags); + +/* Automatically allocate and queue inbound buffers */ +#define MHI_CH_INBOUND_ALLOC_BUFS BIT(0) /** * mhi_unprepare_from_transfer - Reset UL and DL channels for data transfer. diff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c index fa61167..29b4fa3 100644 --- a/net/qrtr/mhi.c +++ b/net/qrtr/mhi.c @@ -79,7 +79,7 @@ static int qcom_mhi_qrtr_probe(struct mhi_device *mhi_dev, int rc; /* start channels */ - rc = mhi_prepare_for_transfer(mhi_dev); + rc = mhi_prepare_for_transfer(mhi_dev, MHI_CH_INBOUND_ALLOC_BUFS); if (rc) return rc;