Re: [PATCH] bus: mhi: Add inbound buffers allocation flag

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

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:
> 
> > On Wed, Jan 06, 2021 at 02:43:43PM +0100, 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,
> >
> > We call them, "MHI client drivers"
> >
> 
> I'll fix that.
> 
> 
> > > 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>
> > > ---
> > >  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(-)
> > >
> >
> > [...]
> >
> > > 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.
> 
> 
> >
> > >       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 inbound
> > buffers
> > > + */
> > > +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 channels
> > > diff --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,
> > >       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