| Message ID | 20211016065734.28802-3-manivannan.sadhasivam@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | MHI patches for v5.16 | expand |
On Sat, Oct 16, 2021 at 12:27:33PM +0530, Manivannan Sadhasivam wrote: > From: Loic Poulain <loic.poulain@linaro.org> > > 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. This is a horrible api. Now one has to go and look up why "0" was added to a function as a parameter. If you don't want to allocate the buffer, then make a function of that name and call that. As you only have one "flag", don't try to make something generic here that is obviously not generic at all. You all can do better than this. thanks, greg k-h
On Sat, Oct 16, 2021 at 09:39:53AM +0200, Greg KH wrote: > On Sat, Oct 16, 2021 at 12:27:33PM +0530, Manivannan Sadhasivam wrote: > > From: Loic Poulain <loic.poulain@linaro.org> > > > > 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. > > This is a horrible api. Now one has to go and look up why "0" was added > to a function as a parameter. > > If you don't want to allocate the buffer, then make a function of that > name and call that. As you only have one "flag", don't try to make > something generic here that is obviously not generic at all. > This is the only API that can be used by the client drivers to pass the configurations to the MHI stack. So we wanted to have a flags parameter that could be extended in the future also. Regarding "0", the default behaviour is to not pre allocate the buffer at all. So it made less sense to add a separate flag or an API for that. Thanks, Mani > You all can do better than this. > > thanks, > > greg k-h
On Sat, Oct 16, 2021 at 10:01:28PM +0530, Manivannan Sadhasivam wrote: > On Sat, Oct 16, 2021 at 09:39:53AM +0200, Greg KH wrote: > > On Sat, Oct 16, 2021 at 12:27:33PM +0530, Manivannan Sadhasivam wrote: > > > From: Loic Poulain <loic.poulain@linaro.org> > > > > > > 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. > > > > This is a horrible api. Now one has to go and look up why "0" was added > > to a function as a parameter. > > > > If you don't want to allocate the buffer, then make a function of that > > name and call that. As you only have one "flag", don't try to make > > something generic here that is obviously not generic at all. > > > > This is the only API that can be used by the client drivers to pass the > configurations to the MHI stack. So we wanted to have a flags parameter that > could be extended in the future also. Worry about future issues then, in the future :) > Regarding "0", the default behaviour is to not pre allocate the buffer at all. > So it made less sense to add a separate flag or an API for that. But again, this is now hard to understand and if you run across a '0' in the call, you have to go and look it up, breaking your reading flow. Please just create a new function for this new option, and then have both of them call the common function with a boolean for this "allocate or not" type of thing. We do this all the time in the kernel to make it easier to read and understand over time. thanks, greg k-h
diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h index 3a732afaf73e..721739c5e0d5 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 b15c5bc37dd4..c01ec2fef02c 100644 --- a/drivers/bus/mhi/core/main.c +++ b/drivers/bus/mhi/core/main.c @@ -1430,7 +1430,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; @@ -1455,6 +1455,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, @@ -1610,7 +1613,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; @@ -1621,7 +1624,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 d127eb6e9257..975f7f9bdf4c 100644 --- a/drivers/net/mhi_net.c +++ b/drivers/net/mhi_net.c @@ -319,7 +319,7 @@ static int mhi_net_newlink(struct mhi_device *mhi_dev, struct net_device *ndev) 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 e4d0f696687f..d0a98f34c54d 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 723985879035..56e7934f8b23 100644 --- a/include/linux/mhi.h +++ b/include/linux/mhi.h @@ -723,8 +723,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 fa611678af05..29b4fa3b72ab 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;