| Message ID | 1600424219-6674-1-git-send-email-loic.poulain@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | bus: mhi: Remove auto-start option | expand |
On 2020-09-18 03:16, Loic Poulain wrote: > There is really no point having an auto-start for channels. > This is confusing for the device drivers, some have to enable the > channels, others don't have... and waste resources (e.g. pre allocated > buffers) that may never be used. > > This is really up to the MHI device(channel) driver to manage the state > of its channels. > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> > --- > drivers/bus/mhi/core/init.c | 9 --------- > drivers/bus/mhi/core/internal.h | 1 - > include/linux/mhi.h | 2 -- > net/qrtr/mhi.c | 5 +++++ > 4 files changed, 5 insertions(+), 12 deletions(-) > > diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c > index ac19067..7aef6b7 100644 > --- a/drivers/bus/mhi/core/init.c > +++ b/drivers/bus/mhi/core/init.c > @@ -727,7 +727,6 @@ static int parse_ch_cfg(struct mhi_controller > *mhi_cntrl, > mhi_chan->offload_ch = ch_cfg->offload_channel; > mhi_chan->db_cfg.reset_req = ch_cfg->doorbell_mode_switch; > mhi_chan->pre_alloc = ch_cfg->auto_queue; > - mhi_chan->auto_start = ch_cfg->auto_start; > > /* > * If MHI host allocates buffers, then the channel direction > @@ -1125,11 +1124,6 @@ static int mhi_driver_probe(struct device *dev) > goto exit_probe; > > ul_chan->xfer_cb = mhi_drv->ul_xfer_cb; > - if (ul_chan->auto_start) { > - ret = mhi_prepare_channel(mhi_cntrl, ul_chan); > - if (ret) > - goto exit_probe; > - } > } > > ret = -EINVAL; > @@ -1163,9 +1157,6 @@ static int mhi_driver_probe(struct device *dev) > if (ret) > goto exit_probe; > > - if (dl_chan && dl_chan->auto_start) > - mhi_prepare_channel(mhi_cntrl, dl_chan); > - > mhi_device_put(mhi_dev); > > return ret; > diff --git a/drivers/bus/mhi/core/internal.h > b/drivers/bus/mhi/core/internal.h > index 5a81a42..73b52a0 100644 > --- a/drivers/bus/mhi/core/internal.h > +++ b/drivers/bus/mhi/core/internal.h > @@ -563,7 +563,6 @@ struct mhi_chan { > bool configured; > bool offload_ch; > bool pre_alloc; > - bool auto_start; > bool wake_capable; > }; > > diff --git a/include/linux/mhi.h b/include/linux/mhi.h > index 008b8f6..742dabe 100644 > --- a/include/linux/mhi.h > +++ b/include/linux/mhi.h > @@ -214,7 +214,6 @@ enum mhi_db_brst_mode { > * @offload_channel: The client manages the channel completely > * @doorbell_mode_switch: Channel switches to doorbell mode on M0 > transition > * @auto_queue: Framework will automatically queue buffers for DL > traffic > - * @auto_start: Automatically start (open) this channel > * @wake-capable: Channel capable of waking up the system > */ > struct mhi_channel_config { > @@ -232,7 +231,6 @@ struct mhi_channel_config { > bool offload_channel; > bool doorbell_mode_switch; > bool auto_queue; > - bool auto_start; > bool wake_capable; > }; > > diff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c > index ff0c414..7100f0b 100644 > --- a/net/qrtr/mhi.c > +++ b/net/qrtr/mhi.c > @@ -76,6 +76,11 @@ static int qcom_mhi_qrtr_probe(struct mhi_device > *mhi_dev, > struct qrtr_mhi_dev *qdev; > int rc; > > + /* start channels */ > + rc = mhi_prepare_for_transfer(mhi_dev); > + if (rc) > + return rc; > + > qdev = devm_kzalloc(&mhi_dev->dev, sizeof(*qdev), GFP_KERNEL); > if (!qdev) > return -ENOMEM; Hi Loic, Thank you for your patch. Hemant and I discussed and we are OK with this patch. Adding Chris to ensure that QRTR changes are acceptable. Thanks, Bhaumik The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project'
Hi Loic, On 18 September 2020 3:46:59 PM IST, Loic Poulain <loic.poulain@linaro.org> wrote: >There is really no point having an auto-start for channels. >This is confusing for the device drivers, some have to enable the >channels, others don't have... and waste resources (e.g. pre allocated >buffers) that may never be used. > >This is really up to the MHI device(channel) driver to manage the state >of its channels. > >Signed-off-by: Loic Poulain <loic.poulain@linaro.org> The patch content looks good but you should split the qrtr change to a separate patch. I can't queue the networking change through mhi tree. Thanks, Mani >--- > drivers/bus/mhi/core/init.c | 9 --------- > drivers/bus/mhi/core/internal.h | 1 - > include/linux/mhi.h | 2 -- > net/qrtr/mhi.c | 5 +++++ > 4 files changed, 5 insertions(+), 12 deletions(-) > >diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c >index ac19067..7aef6b7 100644 >--- a/drivers/bus/mhi/core/init.c >+++ b/drivers/bus/mhi/core/init.c >@@ -727,7 +727,6 @@ static int parse_ch_cfg(struct mhi_controller >*mhi_cntrl, > mhi_chan->offload_ch = ch_cfg->offload_channel; > mhi_chan->db_cfg.reset_req = ch_cfg->doorbell_mode_switch; > mhi_chan->pre_alloc = ch_cfg->auto_queue; >- mhi_chan->auto_start = ch_cfg->auto_start; > > /* > * If MHI host allocates buffers, then the channel direction >@@ -1125,11 +1124,6 @@ static int mhi_driver_probe(struct device *dev) > goto exit_probe; > > ul_chan->xfer_cb = mhi_drv->ul_xfer_cb; >- if (ul_chan->auto_start) { >- ret = mhi_prepare_channel(mhi_cntrl, ul_chan); >- if (ret) >- goto exit_probe; >- } > } > > ret = -EINVAL; >@@ -1163,9 +1157,6 @@ static int mhi_driver_probe(struct device *dev) > if (ret) > goto exit_probe; > >- if (dl_chan && dl_chan->auto_start) >- mhi_prepare_channel(mhi_cntrl, dl_chan); >- > mhi_device_put(mhi_dev); > > return ret; >diff --git a/drivers/bus/mhi/core/internal.h >b/drivers/bus/mhi/core/internal.h >index 5a81a42..73b52a0 100644 >--- a/drivers/bus/mhi/core/internal.h >+++ b/drivers/bus/mhi/core/internal.h >@@ -563,7 +563,6 @@ struct mhi_chan { > bool configured; > bool offload_ch; > bool pre_alloc; >- bool auto_start; > bool wake_capable; > }; > >diff --git a/include/linux/mhi.h b/include/linux/mhi.h >index 008b8f6..742dabe 100644 >--- a/include/linux/mhi.h >+++ b/include/linux/mhi.h >@@ -214,7 +214,6 @@ enum mhi_db_brst_mode { > * @offload_channel: The client manages the channel completely >* @doorbell_mode_switch: Channel switches to doorbell mode on M0 >transition >* @auto_queue: Framework will automatically queue buffers for DL >traffic >- * @auto_start: Automatically start (open) this channel > * @wake-capable: Channel capable of waking up the system > */ > struct mhi_channel_config { >@@ -232,7 +231,6 @@ struct mhi_channel_config { > bool offload_channel; > bool doorbell_mode_switch; > bool auto_queue; >- bool auto_start; > bool wake_capable; > }; > >diff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c >index ff0c414..7100f0b 100644 >--- a/net/qrtr/mhi.c >+++ b/net/qrtr/mhi.c >@@ -76,6 +76,11 @@ static int qcom_mhi_qrtr_probe(struct mhi_device >*mhi_dev, > struct qrtr_mhi_dev *qdev; > int rc; > >+ /* start channels */ >+ rc = mhi_prepare_for_transfer(mhi_dev); >+ if (rc) >+ return rc; >+ > qdev = devm_kzalloc(&mhi_dev->dev, sizeof(*qdev), GFP_KERNEL); > if (!qdev) > return -ENOMEM;
On Sat, 19 Sep 2020 at 13:27, Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote: > > Hi Loic, > > On 18 September 2020 3:46:59 PM IST, Loic Poulain <loic.poulain@linaro.org> wrote: > >There is really no point having an auto-start for channels. > >This is confusing for the device drivers, some have to enable the > >channels, others don't have... and waste resources (e.g. pre allocated > >buffers) that may never be used. > > > >This is really up to the MHI device(channel) driver to manage the state > >of its channels. > > > >Signed-off-by: Loic Poulain <loic.poulain@linaro.org> > > The patch content looks good but you should split the qrtr change to a separate patch. I can't queue the networking change through mhi tree. Ok going to split that. Thanks, Loic
diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c index ac19067..7aef6b7 100644 --- a/drivers/bus/mhi/core/init.c +++ b/drivers/bus/mhi/core/init.c @@ -727,7 +727,6 @@ static int parse_ch_cfg(struct mhi_controller *mhi_cntrl, mhi_chan->offload_ch = ch_cfg->offload_channel; mhi_chan->db_cfg.reset_req = ch_cfg->doorbell_mode_switch; mhi_chan->pre_alloc = ch_cfg->auto_queue; - mhi_chan->auto_start = ch_cfg->auto_start; /* * If MHI host allocates buffers, then the channel direction @@ -1125,11 +1124,6 @@ static int mhi_driver_probe(struct device *dev) goto exit_probe; ul_chan->xfer_cb = mhi_drv->ul_xfer_cb; - if (ul_chan->auto_start) { - ret = mhi_prepare_channel(mhi_cntrl, ul_chan); - if (ret) - goto exit_probe; - } } ret = -EINVAL; @@ -1163,9 +1157,6 @@ static int mhi_driver_probe(struct device *dev) if (ret) goto exit_probe; - if (dl_chan && dl_chan->auto_start) - mhi_prepare_channel(mhi_cntrl, dl_chan); - mhi_device_put(mhi_dev); return ret; diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h index 5a81a42..73b52a0 100644 --- a/drivers/bus/mhi/core/internal.h +++ b/drivers/bus/mhi/core/internal.h @@ -563,7 +563,6 @@ struct mhi_chan { bool configured; bool offload_ch; bool pre_alloc; - bool auto_start; bool wake_capable; }; diff --git a/include/linux/mhi.h b/include/linux/mhi.h index 008b8f6..742dabe 100644 --- a/include/linux/mhi.h +++ b/include/linux/mhi.h @@ -214,7 +214,6 @@ enum mhi_db_brst_mode { * @offload_channel: The client manages the channel completely * @doorbell_mode_switch: Channel switches to doorbell mode on M0 transition * @auto_queue: Framework will automatically queue buffers for DL traffic - * @auto_start: Automatically start (open) this channel * @wake-capable: Channel capable of waking up the system */ struct mhi_channel_config { @@ -232,7 +231,6 @@ struct mhi_channel_config { bool offload_channel; bool doorbell_mode_switch; bool auto_queue; - bool auto_start; bool wake_capable; }; diff --git a/net/qrtr/mhi.c b/net/qrtr/mhi.c index ff0c414..7100f0b 100644 --- a/net/qrtr/mhi.c +++ b/net/qrtr/mhi.c @@ -76,6 +76,11 @@ static int qcom_mhi_qrtr_probe(struct mhi_device *mhi_dev, struct qrtr_mhi_dev *qdev; int rc; + /* start channels */ + rc = mhi_prepare_for_transfer(mhi_dev); + if (rc) + return rc; + qdev = devm_kzalloc(&mhi_dev->dev, sizeof(*qdev), GFP_KERNEL); if (!qdev) return -ENOMEM;
There is really no point having an auto-start for channels. This is confusing for the device drivers, some have to enable the channels, others don't have... and waste resources (e.g. pre allocated buffers) that may never be used. This is really up to the MHI device(channel) driver to manage the state of its channels. Signed-off-by: Loic Poulain <loic.poulain@linaro.org> --- drivers/bus/mhi/core/init.c | 9 --------- drivers/bus/mhi/core/internal.h | 1 - include/linux/mhi.h | 2 -- net/qrtr/mhi.c | 5 +++++ 4 files changed, 5 insertions(+), 12 deletions(-)