[PATCH v4 3/9] PCI: epf-mhi: Add support for handling D-state notify from EPC

[PATCH v4 3/9] PCI: epf-mhi: Add support for handling D-state notify from EPC

[Krishna chaitanya chundru]

Add support for handling D-state notify for MHI EPF.

Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
 drivers/pci/endpoint/functions/pci-epf-mhi.c | 11 +++++++++++
 include/linux/mhi_ep.h                       |  3 +++
 2 files changed, 14 insertions(+)

diff --git a/drivers/pci/endpoint/functions/pci-epf-mhi.c b/drivers/pci/endpoint/functions/pci-epf-mhi.c
index 9c1f5a1..ee91bfc 100644
--- a/drivers/pci/endpoint/functions/pci-epf-mhi.c
+++ b/drivers/pci/endpoint/functions/pci-epf-mhi.c
@@ -339,6 +339,16 @@ static int pci_epf_mhi_bme(struct pci_epf *epf)
 	return 0;
 }
 
+static int pci_epf_mhi_dstate_notify(struct pci_epf *epf, pci_power_t state)
+{
+	struct pci_epf_mhi *epf_mhi = epf_get_drvdata(epf);
+	struct mhi_ep_cntrl *mhi_cntrl = &epf_mhi->mhi_cntrl;
+
+	mhi_cntrl->dstate = state;
+
+	return 0;
+}
+
 static int pci_epf_mhi_bind(struct pci_epf *epf)
 {
 	struct pci_epf_mhi *epf_mhi = epf_get_drvdata(epf);
@@ -394,6 +404,7 @@ static struct pci_epc_event_ops pci_epf_mhi_event_ops = {
 	.link_up = pci_epf_mhi_link_up,
 	.link_down = pci_epf_mhi_link_down,
 	.bme = pci_epf_mhi_bme,
+	.dstate_notify = pci_epf_mhi_dstate_notify,
 };
 
 static int pci_epf_mhi_probe(struct pci_epf *epf,
diff --git a/include/linux/mhi_ep.h b/include/linux/mhi_ep.h
index f198a8a..c3a0685 100644
--- a/include/linux/mhi_ep.h
+++ b/include/linux/mhi_ep.h
@@ -8,6 +8,7 @@
 
 #include <linux/dma-direction.h>
 #include <linux/mhi.h>
+#include <linux/pci.h>
 
 #define MHI_EP_DEFAULT_MTU 0x8000
 
@@ -139,6 +140,8 @@ struct mhi_ep_cntrl {
 
 	enum mhi_state mhi_state;
 
+	pci_power_t dstate;
+
 	u32 max_chan;
 	u32 mru;
 	u32 event_rings;
-- 
2.7.4

Re: [PATCH v4 3/9] PCI: epf-mhi: Add support for handling D-state notify from EPC

[Manivannan Sadhasivam]

On Thu, Jul 13, 2023 at 12:40:12PM +0530, Krishna chaitanya chundru wrote:
> Add support for handling D-state notify for MHI EPF.
> 
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
>  drivers/pci/endpoint/functions/pci-epf-mhi.c | 11 +++++++++++
>  include/linux/mhi_ep.h                       |  3 +++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-mhi.c b/drivers/pci/endpoint/functions/pci-epf-mhi.c
> index 9c1f5a1..ee91bfc 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-mhi.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-mhi.c
> @@ -339,6 +339,16 @@ static int pci_epf_mhi_bme(struct pci_epf *epf)
>  	return 0;
>  }
>  
> +static int pci_epf_mhi_dstate_notify(struct pci_epf *epf, pci_power_t state)
> +{
> +	struct pci_epf_mhi *epf_mhi = epf_get_drvdata(epf);
> +	struct mhi_ep_cntrl *mhi_cntrl = &epf_mhi->mhi_cntrl;
> +
> +	mhi_cntrl->dstate = state;

Where is this variable being used? Also, don't we need any locking?

- Mani

> +
> +	return 0;
> +}
> +
>  static int pci_epf_mhi_bind(struct pci_epf *epf)
>  {
>  	struct pci_epf_mhi *epf_mhi = epf_get_drvdata(epf);
> @@ -394,6 +404,7 @@ static struct pci_epc_event_ops pci_epf_mhi_event_ops = {
>  	.link_up = pci_epf_mhi_link_up,
>  	.link_down = pci_epf_mhi_link_down,
>  	.bme = pci_epf_mhi_bme,
> +	.dstate_notify = pci_epf_mhi_dstate_notify,
>  };
>  
>  static int pci_epf_mhi_probe(struct pci_epf *epf,
> diff --git a/include/linux/mhi_ep.h b/include/linux/mhi_ep.h
> index f198a8a..c3a0685 100644
> --- a/include/linux/mhi_ep.h
> +++ b/include/linux/mhi_ep.h
> @@ -8,6 +8,7 @@
>  
>  #include <linux/dma-direction.h>
>  #include <linux/mhi.h>
> +#include <linux/pci.h>
>  
>  #define MHI_EP_DEFAULT_MTU 0x8000
>  
> @@ -139,6 +140,8 @@ struct mhi_ep_cntrl {
>  
>  	enum mhi_state mhi_state;
>  
> +	pci_power_t dstate;
> +
>  	u32 max_chan;
>  	u32 mru;
>  	u32 event_rings;
> -- 
> 2.7.4
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 3/9] PCI: epf-mhi: Add support for handling D-state notify from EPC

[Krishna Chaitanya Chundru]

On 7/28/2023 9:39 AM, Manivannan Sadhasivam wrote:
> On Thu, Jul 13, 2023 at 12:40:12PM +0530, Krishna chaitanya chundru wrote:
>> Add support for handling D-state notify for MHI EPF.
>>
>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>> ---
>>   drivers/pci/endpoint/functions/pci-epf-mhi.c | 11 +++++++++++
>>   include/linux/mhi_ep.h                       |  3 +++
>>   2 files changed, 14 insertions(+)
>>
>> diff --git a/drivers/pci/endpoint/functions/pci-epf-mhi.c b/drivers/pci/endpoint/functions/pci-epf-mhi.c
>> index 9c1f5a1..ee91bfc 100644
>> --- a/drivers/pci/endpoint/functions/pci-epf-mhi.c
>> +++ b/drivers/pci/endpoint/functions/pci-epf-mhi.c
>> @@ -339,6 +339,16 @@ static int pci_epf_mhi_bme(struct pci_epf *epf)
>>   	return 0;
>>   }
>>   
>> +static int pci_epf_mhi_dstate_notify(struct pci_epf *epf, pci_power_t state)
>> +{
>> +	struct pci_epf_mhi *epf_mhi = epf_get_drvdata(epf);
>> +	struct mhi_ep_cntrl *mhi_cntrl = &epf_mhi->mhi_cntrl;
>> +
>> +	mhi_cntrl->dstate = state;
> Where is this variable being used? Also, don't we need any locking?
>
> - Mani

we are using this variable in wakeup host op which is introduced on 
patch [PATCH v4 8/9] PCI: epf-mhi: Add wakeup host op

I will add lock in my next series.

- KC

>
>> +
>> +	return 0;
>> +}
>> +
>>   static int pci_epf_mhi_bind(struct pci_epf *epf)
>>   {
>>   	struct pci_epf_mhi *epf_mhi = epf_get_drvdata(epf);
>> @@ -394,6 +404,7 @@ static struct pci_epc_event_ops pci_epf_mhi_event_ops = {
>>   	.link_up = pci_epf_mhi_link_up,
>>   	.link_down = pci_epf_mhi_link_down,
>>   	.bme = pci_epf_mhi_bme,
>> +	.dstate_notify = pci_epf_mhi_dstate_notify,
>>   };
>>   
>>   static int pci_epf_mhi_probe(struct pci_epf *epf,
>> diff --git a/include/linux/mhi_ep.h b/include/linux/mhi_ep.h
>> index f198a8a..c3a0685 100644
>> --- a/include/linux/mhi_ep.h
>> +++ b/include/linux/mhi_ep.h
>> @@ -8,6 +8,7 @@
>>   
>>   #include <linux/dma-direction.h>
>>   #include <linux/mhi.h>
>> +#include <linux/pci.h>
>>   
>>   #define MHI_EP_DEFAULT_MTU 0x8000
>>   
>> @@ -139,6 +140,8 @@ struct mhi_ep_cntrl {
>>   
>>   	enum mhi_state mhi_state;
>>   
>> +	pci_power_t dstate;
>> +
>>   	u32 max_chan;
>>   	u32 mru;
>>   	u32 event_rings;
>> -- 
>> 2.7.4
>>

Re: [PATCH v4 3/9] PCI: epf-mhi: Add support for handling D-state notify from EPC

[Krishna Chaitanya Chundru]

On 7/31/2023 11:05 AM, Krishna Chaitanya Chundru wrote:
>
> On 7/28/2023 9:39 AM, Manivannan Sadhasivam wrote:
>> On Thu, Jul 13, 2023 at 12:40:12PM +0530, Krishna chaitanya chundru 
>> wrote:
>>> Add support for handling D-state notify for MHI EPF.
>>>
>>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>>> ---
>>>   drivers/pci/endpoint/functions/pci-epf-mhi.c | 11 +++++++++++
>>>   include/linux/mhi_ep.h                       |  3 +++
>>>   2 files changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/pci/endpoint/functions/pci-epf-mhi.c 
>>> b/drivers/pci/endpoint/functions/pci-epf-mhi.c
>>> index 9c1f5a1..ee91bfc 100644
>>> --- a/drivers/pci/endpoint/functions/pci-epf-mhi.c
>>> +++ b/drivers/pci/endpoint/functions/pci-epf-mhi.c
>>> @@ -339,6 +339,16 @@ static int pci_epf_mhi_bme(struct pci_epf *epf)
>>>       return 0;
>>>   }
>>>   +static int pci_epf_mhi_dstate_notify(struct pci_epf *epf, 
>>> pci_power_t state)
>>> +{
>>> +    struct pci_epf_mhi *epf_mhi = epf_get_drvdata(epf);
>>> +    struct mhi_ep_cntrl *mhi_cntrl = &epf_mhi->mhi_cntrl;
>>> +
>>> +    mhi_cntrl->dstate = state;
>> Where is this variable being used? Also, don't we need any locking?
>>
>> - Mani
>
> we are using this variable in wakeup host op which is introduced on 
> patch [PATCH v4 8/9] PCI: epf-mhi: Add wakeup host op
>
> I will add lock in my next series.
>
> - KC

Mani, as this is being called from IRQ context do we need to add any 
lock here.

- KC

>
>>
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   static int pci_epf_mhi_bind(struct pci_epf *epf)
>>>   {
>>>       struct pci_epf_mhi *epf_mhi = epf_get_drvdata(epf);
>>> @@ -394,6 +404,7 @@ static struct pci_epc_event_ops 
>>> pci_epf_mhi_event_ops = {
>>>       .link_up = pci_epf_mhi_link_up,
>>>       .link_down = pci_epf_mhi_link_down,
>>>       .bme = pci_epf_mhi_bme,
>>> +    .dstate_notify = pci_epf_mhi_dstate_notify,
>>>   };
>>>     static int pci_epf_mhi_probe(struct pci_epf *epf,
>>> diff --git a/include/linux/mhi_ep.h b/include/linux/mhi_ep.h
>>> index f198a8a..c3a0685 100644
>>> --- a/include/linux/mhi_ep.h
>>> +++ b/include/linux/mhi_ep.h
>>> @@ -8,6 +8,7 @@
>>>     #include <linux/dma-direction.h>
>>>   #include <linux/mhi.h>
>>> +#include <linux/pci.h>
>>>     #define MHI_EP_DEFAULT_MTU 0x8000
>>>   @@ -139,6 +140,8 @@ struct mhi_ep_cntrl {
>>>         enum mhi_state mhi_state;
>>>   +    pci_power_t dstate;
>>> +
>>>       u32 max_chan;
>>>       u32 mru;
>>>       u32 event_rings;
>>> -- 
>>> 2.7.4
>>>
>

Re: [PATCH v4 3/9] PCI: epf-mhi: Add support for handling D-state notify from EPC

[Manivannan Sadhasivam]

On Tue, Aug 01, 2023 at 10:31:42AM +0530, Krishna Chaitanya Chundru wrote:
> 
> On 7/31/2023 11:05 AM, Krishna Chaitanya Chundru wrote:
> > 
> > On 7/28/2023 9:39 AM, Manivannan Sadhasivam wrote:
> > > On Thu, Jul 13, 2023 at 12:40:12PM +0530, Krishna chaitanya chundru
> > > wrote:
> > > > Add support for handling D-state notify for MHI EPF.
> > > > 
> > > > Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> > > > ---
> > > >   drivers/pci/endpoint/functions/pci-epf-mhi.c | 11 +++++++++++
> > > >   include/linux/mhi_ep.h                       |  3 +++
> > > >   2 files changed, 14 insertions(+)
> > > > 
> > > > diff --git a/drivers/pci/endpoint/functions/pci-epf-mhi.c
> > > > b/drivers/pci/endpoint/functions/pci-epf-mhi.c
> > > > index 9c1f5a1..ee91bfc 100644
> > > > --- a/drivers/pci/endpoint/functions/pci-epf-mhi.c
> > > > +++ b/drivers/pci/endpoint/functions/pci-epf-mhi.c
> > > > @@ -339,6 +339,16 @@ static int pci_epf_mhi_bme(struct pci_epf *epf)
> > > >       return 0;
> > > >   }
> > > >   +static int pci_epf_mhi_dstate_notify(struct pci_epf *epf,
> > > > pci_power_t state)
> > > > +{
> > > > +    struct pci_epf_mhi *epf_mhi = epf_get_drvdata(epf);
> > > > +    struct mhi_ep_cntrl *mhi_cntrl = &epf_mhi->mhi_cntrl;
> > > > +
> > > > +    mhi_cntrl->dstate = state;
> > > Where is this variable being used? Also, don't we need any locking?
> > > 
> > > - Mani
> > 
> > we are using this variable in wakeup host op which is introduced on
> > patch [PATCH v4 8/9] PCI: epf-mhi: Add wakeup host op
> > 
> > I will add lock in my next series.
> > 
> > - KC
> 
> Mani, as this is being called from IRQ context do we need to add any lock
> here.
> 

Notifiers are invoked in process context. And here, the context doesn't matter
as either way you need locking to prevent concurrent access to dstate variable.

But I think it is safe to ignore lock for now provided that wakeup_host callback
is only called while MHI is in M3 state. Even if dstate changes while processing
wakeup_host, it won't affect the behavior.

- Mani

> - KC
> 
> > 
> > > 
> > > > +
> > > > +    return 0;
> > > > +}
> > > > +
> > > >   static int pci_epf_mhi_bind(struct pci_epf *epf)
> > > >   {
> > > >       struct pci_epf_mhi *epf_mhi = epf_get_drvdata(epf);
> > > > @@ -394,6 +404,7 @@ static struct pci_epc_event_ops
> > > > pci_epf_mhi_event_ops = {
> > > >       .link_up = pci_epf_mhi_link_up,
> > > >       .link_down = pci_epf_mhi_link_down,
> > > >       .bme = pci_epf_mhi_bme,
> > > > +    .dstate_notify = pci_epf_mhi_dstate_notify,
> > > >   };
> > > >     static int pci_epf_mhi_probe(struct pci_epf *epf,
> > > > diff --git a/include/linux/mhi_ep.h b/include/linux/mhi_ep.h
> > > > index f198a8a..c3a0685 100644
> > > > --- a/include/linux/mhi_ep.h
> > > > +++ b/include/linux/mhi_ep.h
> > > > @@ -8,6 +8,7 @@
> > > >     #include <linux/dma-direction.h>
> > > >   #include <linux/mhi.h>
> > > > +#include <linux/pci.h>
> > > >     #define MHI_EP_DEFAULT_MTU 0x8000
> > > >   @@ -139,6 +140,8 @@ struct mhi_ep_cntrl {
> > > >         enum mhi_state mhi_state;
> > > >   +    pci_power_t dstate;
> > > > +
> > > >       u32 max_chan;
> > > >       u32 mru;
> > > >       u32 event_rings;
> > > > -- 
> > > > 2.7.4
> > > > 
> > 

-- 
மணிவண்ணன் சதாசிவம்

[PATCH v4 8/9] PCI: epf-mhi: Add wakeup host op

[Krishna chaitanya chundru]

Add wakeup host op for MHI EPF.
If the D-state is in D3cold toggle wake signal, otherwise send PME.

Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
 drivers/pci/endpoint/functions/pci-epf-mhi.c | 16 ++++++++++++++++
 include/linux/mhi_ep.h                       |  1 +
 2 files changed, 17 insertions(+)

diff --git a/drivers/pci/endpoint/functions/pci-epf-mhi.c b/drivers/pci/endpoint/functions/pci-epf-mhi.c
index ee91bfc..b608505 100644
--- a/drivers/pci/endpoint/functions/pci-epf-mhi.c
+++ b/drivers/pci/endpoint/functions/pci-epf-mhi.c
@@ -237,6 +237,21 @@ static int pci_epf_mhi_write_to_host(struct mhi_ep_cntrl *mhi_cntrl,
 	return 0;
 }
 
+static int pci_epf_mhi_wakeup_host(struct mhi_ep_cntrl *mhi_cntrl)
+{
+	struct pci_epf_mhi *epf_mhi = to_epf_mhi(mhi_cntrl);
+	struct pci_epf *epf = epf_mhi->epf;
+	enum pci_epc_wakeup_host_type type;
+	struct pci_epc *epc = epf->epc;
+
+	type = PCI_WAKEUP_SEND_PME;
+	if (mhi_cntrl->dstate == PCI_D3cold)
+		type = PCI_WAKEUP_TOGGLE_WAKE;
+
+	return pci_epc_wakeup_host(epc, epf->func_no, epf->vfunc_no, type);
+
+}
+
 static int pci_epf_mhi_core_init(struct pci_epf *epf)
 {
 	struct pci_epf_mhi *epf_mhi = epf_get_drvdata(epf);
@@ -293,6 +308,7 @@ static int pci_epf_mhi_link_up(struct pci_epf *epf)
 	mhi_cntrl->unmap_free = pci_epf_mhi_unmap_free;
 	mhi_cntrl->read_from_host = pci_epf_mhi_read_from_host;
 	mhi_cntrl->write_to_host = pci_epf_mhi_write_to_host;
+	mhi_cntrl->wakeup_host = pci_epf_mhi_wakeup_host;
 
 	/* Register the MHI EP controller */
 	ret = mhi_ep_register_controller(mhi_cntrl, info->config);
diff --git a/include/linux/mhi_ep.h b/include/linux/mhi_ep.h
index c3a0685..e353c429 100644
--- a/include/linux/mhi_ep.h
+++ b/include/linux/mhi_ep.h
@@ -137,6 +137,7 @@ struct mhi_ep_cntrl {
 			   void __iomem *virt, size_t size);
 	int (*read_from_host)(struct mhi_ep_cntrl *mhi_cntrl, u64 from, void *to, size_t size);
 	int (*write_to_host)(struct mhi_ep_cntrl *mhi_cntrl, void *from, u64 to, size_t size);
+	int (*wakeup_host)(struct mhi_ep_cntrl *mhi_cntrl);
 
 	enum mhi_state mhi_state;
 
-- 
2.7.4

[PATCH v4 9/9] bus: mhi: ep: wake up host if the MHI state is in M3

[Krishna chaitanya chundru]

If the MHI state is in M3 then the most probably the host kept the
device in D3 hot or D3 cold, due to that endpoint transctions will not
be read by the host, so endpoint wakes up host to bring the host to D0
which eventually bring back the MHI state to M0.

Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
 drivers/bus/mhi/ep/main.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c
index 6008818..46a888e 100644
--- a/drivers/bus/mhi/ep/main.c
+++ b/drivers/bus/mhi/ep/main.c
@@ -25,6 +25,26 @@ static DEFINE_IDA(mhi_ep_cntrl_ida);
 static int mhi_ep_create_device(struct mhi_ep_cntrl *mhi_cntrl, u32 ch_id);
 static int mhi_ep_destroy_device(struct device *dev, void *data);
 
+static int mhi_ep_wake_host(struct mhi_ep_cntrl *mhi_cntrl)
+{
+	enum mhi_state state;
+	bool mhi_reset;
+	u32 count = 0;
+
+	mhi_cntrl->wakeup_host(mhi_cntrl);
+
+	/* Wait for Host to set the M0 state */
+	while (count++ < M0_WAIT_COUNT) {
+		msleep(M0_WAIT_DELAY_MS);
+
+		mhi_ep_mmio_get_mhi_state(mhi_cntrl, &state, &mhi_reset);
+		if (state == MHI_STATE_M0)
+			return 0;
+	}
+
+	return -ENODEV;
+}
+
 static int mhi_ep_send_event(struct mhi_ep_cntrl *mhi_cntrl, u32 ring_idx,
 			     struct mhi_ring_element *el, bool bei)
 {
@@ -464,6 +484,13 @@ int mhi_ep_queue_skb(struct mhi_ep_device *mhi_dev, struct sk_buff *skb)
 	buf_left = skb->len;
 	ring = &mhi_cntrl->mhi_chan[mhi_chan->chan].ring;
 
+	if (mhi_cntrl->mhi_state == MHI_STATE_M3) {
+		if (mhi_ep_wake_host(mhi_cntrl)) {
+			dev_err(dev, "Failed to wakeup host\n");
+			return -ENODEV;
+		}
+	}
+
 	mutex_lock(&mhi_chan->lock);
 
 	do {
-- 
2.7.4

Re: [PATCH v4 9/9] bus: mhi: ep: wake up host if the MHI state is in M3

[Manivannan Sadhasivam]

On Thu, Jul 13, 2023 at 12:40:18PM +0530, Krishna chaitanya chundru wrote:
> If the MHI state is in M3 then the most probably the host kept the

s/then the/then

> device in D3 hot or D3 cold, due to that endpoint transctions will not

s/transctions/transactions

> be read by the host, so endpoint wakes up host to bring the host to D0

endpoint needs to wake up the host to bring the device to D0 state...

> which eventually bring back the MHI state to M0.
> 
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
>  drivers/bus/mhi/ep/main.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/bus/mhi/ep/main.c b/drivers/bus/mhi/ep/main.c
> index 6008818..46a888e 100644
> --- a/drivers/bus/mhi/ep/main.c
> +++ b/drivers/bus/mhi/ep/main.c
> @@ -25,6 +25,26 @@ static DEFINE_IDA(mhi_ep_cntrl_ida);
>  static int mhi_ep_create_device(struct mhi_ep_cntrl *mhi_cntrl, u32 ch_id);
>  static int mhi_ep_destroy_device(struct device *dev, void *data);
>  
> +static int mhi_ep_wake_host(struct mhi_ep_cntrl *mhi_cntrl)
> +{
> +	enum mhi_state state;
> +	bool mhi_reset;
> +	u32 count = 0;
> +
> +	mhi_cntrl->wakeup_host(mhi_cntrl);
> +
> +	/* Wait for Host to set the M0 state */
> +	while (count++ < M0_WAIT_COUNT) {
> +		msleep(M0_WAIT_DELAY_MS);
> +
> +		mhi_ep_mmio_get_mhi_state(mhi_cntrl, &state, &mhi_reset);
> +		if (state == MHI_STATE_M0)
> +			return 0;
> +	}
> +
> +	return -ENODEV;

ENODEV or ETIMEDOUT?

> +}
> +
>  static int mhi_ep_send_event(struct mhi_ep_cntrl *mhi_cntrl, u32 ring_idx,
>  			     struct mhi_ring_element *el, bool bei)
>  {
> @@ -464,6 +484,13 @@ int mhi_ep_queue_skb(struct mhi_ep_device *mhi_dev, struct sk_buff *skb)
>  	buf_left = skb->len;
>  	ring = &mhi_cntrl->mhi_chan[mhi_chan->chan].ring;
>  
> +	if (mhi_cntrl->mhi_state == MHI_STATE_M3) {
> +		if (mhi_ep_wake_host(mhi_cntrl)) {

Don't you need lock here in the case of multiple queue requests?

- Mani

> +			dev_err(dev, "Failed to wakeup host\n");
> +			return -ENODEV;
> +		}
> +	}
> +
>  	mutex_lock(&mhi_chan->lock);
>  
>  	do {
> -- 
> 2.7.4
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 9/9] bus: mhi: ep: wake up host if the MHI state is in M3

[Dan Carpenter]

On Fri, Jul 28, 2023 at 10:04:52AM +0530, Manivannan Sadhasivam wrote:
> > @@ -464,6 +484,13 @@ int mhi_ep_queue_skb(struct mhi_ep_device *mhi_dev, struct sk_buff *skb)
> >  	buf_left = skb->len;
> >  	ring = &mhi_cntrl->mhi_chan[mhi_chan->chan].ring;
> >  
> > +	if (mhi_cntrl->mhi_state == MHI_STATE_M3) {
> > +		if (mhi_ep_wake_host(mhi_cntrl)) {
> 
> Don't you need lock here in the case of multiple queue requests?
> 
> - Mani
> 
> > +			dev_err(dev, "Failed to wakeup host\n");
> > +			return -ENODEV;
> > +		}
> > +	}
> > +
> >  	mutex_lock(&mhi_chan->lock);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^
This lock isn't enough?

regards,
dan carpenter

Re: [PATCH v4 9/9] bus: mhi: ep: wake up host if the MHI state is in M3

[Manivannan Sadhasivam]

On Fri, Jul 28, 2023 at 08:50:55AM +0300, Dan Carpenter wrote:
> On Fri, Jul 28, 2023 at 10:04:52AM +0530, Manivannan Sadhasivam wrote:
> > > @@ -464,6 +484,13 @@ int mhi_ep_queue_skb(struct mhi_ep_device *mhi_dev, struct sk_buff *skb)
> > >  	buf_left = skb->len;
> > >  	ring = &mhi_cntrl->mhi_chan[mhi_chan->chan].ring;
> > >  
> > > +	if (mhi_cntrl->mhi_state == MHI_STATE_M3) {
> > > +		if (mhi_ep_wake_host(mhi_cntrl)) {
> > 
> > Don't you need lock here in the case of multiple queue requests?
> > 
> > - Mani
> > 
> > > +			dev_err(dev, "Failed to wakeup host\n");
> > > +			return -ENODEV;
> > > +		}
> > > +	}
> > > +
> > >  	mutex_lock(&mhi_chan->lock);
>         ^^^^^^^^^^^^^^^^^^^^^^^^^^
> This lock isn't enough?
> 

The position of this lock won't prevent cocurrent access to mhi_ep_wake_host().

- Mani

> regards,
> dan carpenter

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v4 9/9] bus: mhi: ep: wake up host if the MHI state is in M3

[Krishna Chaitanya Chundru]

On 7/28/2023 9:05 PM, Manivannan Sadhasivam wrote:
> On Fri, Jul 28, 2023 at 08:50:55AM +0300, Dan Carpenter wrote:
>> On Fri, Jul 28, 2023 at 10:04:52AM +0530, Manivannan Sadhasivam wrote:
>>>> @@ -464,6 +484,13 @@ int mhi_ep_queue_skb(struct mhi_ep_device *mhi_dev, struct sk_buff *skb)
>>>>   	buf_left = skb->len;
>>>>   	ring = &mhi_cntrl->mhi_chan[mhi_chan->chan].ring;
>>>>   
>>>> +	if (mhi_cntrl->mhi_state == MHI_STATE_M3) {
>>>> +		if (mhi_ep_wake_host(mhi_cntrl)) {
>>> Don't you need lock here in the case of multiple queue requests?
>>>
>>> - Mani
>>>
>>>> +			dev_err(dev, "Failed to wakeup host\n");
>>>> +			return -ENODEV;
>>>> +		}
>>>> +	}
>>>> +
>>>>   	mutex_lock(&mhi_chan->lock);
>>          ^^^^^^^^^^^^^^^^^^^^^^^^^^
>> This lock isn't enough?
>>
> The position of this lock won't prevent cocurrent access to mhi_ep_wake_host().
>
> - Mani
I will add the lock in the next series.
>> regards,
>> dan carpenter

Re: [PATCH v4 9/9] bus: mhi: ep: wake up host if the MHI state is in M3

[Dan Carpenter]

On Thu, Jul 13, 2023 at 12:40:18PM +0530, Krishna chaitanya chundru wrote:
> @@ -464,6 +484,13 @@ int mhi_ep_queue_skb(struct mhi_ep_device *mhi_dev, struct sk_buff *skb)
>  	buf_left = skb->len;
>  	ring = &mhi_cntrl->mhi_chan[mhi_chan->chan].ring;
>  
> +	if (mhi_cntrl->mhi_state == MHI_STATE_M3) {
> +		if (mhi_ep_wake_host(mhi_cntrl)) {
> +			dev_err(dev, "Failed to wakeup host\n");
> +			return -ENODEV;
> +		}

Since you're going to be redoing this patch anyway could you please
propage the error code:

		ret = mhi_ep_wake_host(mhi_cntrl);
		if (ret) {
			dev_err(dev, "Failed to wakeup host\n");
			return ret;
		}

regards,
dan carpenter

Re: [PATCH v4 9/9] bus: mhi: ep: wake up host if the MHI state is in M3

[Krishna Chaitanya Chundru]

On 7/28/2023 11:21 AM, Dan Carpenter wrote:
> On Thu, Jul 13, 2023 at 12:40:18PM +0530, Krishna chaitanya chundru wrote:
>> @@ -464,6 +484,13 @@ int mhi_ep_queue_skb(struct mhi_ep_device *mhi_dev, struct sk_buff *skb)
>>   	buf_left = skb->len;
>>   	ring = &mhi_cntrl->mhi_chan[mhi_chan->chan].ring;
>>   
>> +	if (mhi_cntrl->mhi_state == MHI_STATE_M3) {
>> +		if (mhi_ep_wake_host(mhi_cntrl)) {
>> +			dev_err(dev, "Failed to wakeup host\n");
>> +			return -ENODEV;
>> +		}
> Since you're going to be redoing this patch anyway could you please
> propage the error code:
>
> 		ret = mhi_ep_wake_host(mhi_cntrl);
> 		if (ret) {
> 			dev_err(dev, "Failed to wakeup host\n");
> 			return ret;
> 		}
>
> regards,
> dan carpenter

I will add this in next series.

- KC

>