[PATCH] mhi: use irq_flags if client driver configures it

[PATCH] mhi: use irq_flags if client driver configures it

[Carl Huang]

If client driver has specified the irq_flags, mhi uses this specified
irq_flags. Otherwise, mhi uses default irq_flags.

The purpose of this change is to support one MSI vector for QCA6390.
MHI will use one same MSI vector too in this scenario.

In case of one MSI vector, IRQ_NO_BALANCING is needed when irq handler
is requested. The reason is if irq migration happens, the msi_data may
change too. However, the msi_data is already programmed to QCA6390
hardware during initialization phase. This msi_data inconsistence will
result in crash in kernel.

Another issue is in case of one MSI vector, IRQF_NO_SUSPEND will trigger
WARNINGS because QCA6390 wants to disable the IRQ during the suspend.

To avoid above two issues, QCA6390 driver specifies the irq_flags in case
of one MSI vector when mhi_register_controller is called.

Signed-off-by: Carl Huang <cjhuang@codeaurora.org>
---
 drivers/bus/mhi/core/init.c | 9 +++++++--
 include/linux/mhi.h         | 1 +
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
index 0ffdebd..5f74e1e 100644
--- a/drivers/bus/mhi/core/init.c
+++ b/drivers/bus/mhi/core/init.c
@@ -148,12 +148,17 @@ int mhi_init_irq_setup(struct mhi_controller *mhi_cntrl)
 {
 	struct mhi_event *mhi_event = mhi_cntrl->mhi_event;
 	struct device *dev = &mhi_cntrl->mhi_dev->dev;
+	unsigned long irq_flags = IRQF_SHARED | IRQF_NO_SUSPEND;
 	int i, ret;
 
+	/* if client driver has set irq_flags, use it */
+	if (mhi_cntrl->irq_flags)
+		irq_flags = mhi_cntrl->irq_flags;
+
 	/* Setup BHI_INTVEC IRQ */
 	ret = request_threaded_irq(mhi_cntrl->irq[0], mhi_intvec_handler,
 				   mhi_intvec_threaded_handler,
-				   IRQF_SHARED | IRQF_NO_SUSPEND,
+				   irq_flags,
 				   "bhi", mhi_cntrl);
 	if (ret)
 		return ret;
@@ -171,7 +176,7 @@ int mhi_init_irq_setup(struct mhi_controller *mhi_cntrl)
 
 		ret = request_irq(mhi_cntrl->irq[mhi_event->irq],
 				  mhi_irq_handler,
-				  IRQF_SHARED | IRQF_NO_SUSPEND,
+				  irq_flags,
 				  "mhi", mhi_event);
 		if (ret) {
 			dev_err(dev, "Error requesting irq:%d for ev:%d\n",
diff --git a/include/linux/mhi.h b/include/linux/mhi.h
index d4841e5..f039e58 100644
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -442,6 +442,7 @@ struct mhi_controller {
 	bool fbc_download;
 	bool pre_init;
 	bool wake_set;
+	unsigned long irq_flags;
 };
 
 /**
-- 
2.7.4

Re: [PATCH] mhi: use irq_flags if client driver configures it

[Hemant Kumar]

On 12/7/20 7:55 PM, Carl Huang wrote:
> If client driver has specified the irq_flags, mhi uses this specified
> irq_flags. Otherwise, mhi uses default irq_flags.
> 
> The purpose of this change is to support one MSI vector for QCA6390.
> MHI will use one same MSI vector too in this scenario.
> 
> In case of one MSI vector, IRQ_NO_BALANCING is needed when irq handler
> is requested. The reason is if irq migration happens, the msi_data may
> change too. However, the msi_data is already programmed to QCA6390
> hardware during initialization phase. This msi_data inconsistence will
> result in crash in kernel.
> 
> Another issue is in case of one MSI vector, IRQF_NO_SUSPEND will trigger
> WARNINGS because QCA6390 wants to disable the IRQ during the suspend.
> 
> To avoid above two issues, QCA6390 driver specifies the irq_flags in case
> of one MSI vector when mhi_register_controller is called.
> 
> Signed-off-by: Carl Huang <cjhuang@codeaurora.org>
> ---
>   drivers/bus/mhi/core/init.c | 9 +++++++--
>   include/linux/mhi.h         | 1 +
>   2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> index 0ffdebd..5f74e1e 100644
> --- a/drivers/bus/mhi/core/init.c
> +++ b/drivers/bus/mhi/core/init.c
> @@ -148,12 +148,17 @@ int mhi_init_irq_setup(struct mhi_controller *mhi_cntrl)
>   {
>   	struct mhi_event *mhi_event = mhi_cntrl->mhi_event;
>   	struct device *dev = &mhi_cntrl->mhi_dev->dev;
> +	unsigned long irq_flags = IRQF_SHARED | IRQF_NO_SUSPEND;
>   	int i, ret;
>   
> +	/* if client driver has set irq_flags, use it */
> +	if (mhi_cntrl->irq_flags)
> +		irq_flags = mhi_cntrl->irq_flags;
Jeff if i remember correctly your use case also have one dedicated irq 
line for all the MSIs, just want to confirm if you are fine with this 
change ? i was wondering if any input check is required for irq_flags 
passed by controller, or responsibility is on controller for any 
undesired behavior. Like passing IRQF_SHARED and IRQF_ONESHOT when one 
irq line is shared among multiple MSIs.
> +
>   	/* Setup BHI_INTVEC IRQ */
>   	ret = request_threaded_irq(mhi_cntrl->irq[0], mhi_intvec_handler,
>   				   mhi_intvec_threaded_handler,
> -				   IRQF_SHARED | IRQF_NO_SUSPEND,
> +				   irq_flags,
>   				   "bhi", mhi_cntrl);
>   	if (ret)
>   		return ret;
> @@ -171,7 +176,7 @@ int mhi_init_irq_setup(struct mhi_controller *mhi_cntrl)
>   
>   		ret = request_irq(mhi_cntrl->irq[mhi_event->irq],
>   				  mhi_irq_handler,
> -				  IRQF_SHARED | IRQF_NO_SUSPEND,
> +				  irq_flags,
>   				  "mhi", mhi_event);
>   		if (ret) {
>   			dev_err(dev, "Error requesting irq:%d for ev:%d\n",
> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> index d4841e5..f039e58 100644
> --- a/include/linux/mhi.h
> +++ b/include/linux/mhi.h
> @@ -442,6 +442,7 @@ struct mhi_controller {
>   	bool fbc_download;
>   	bool pre_init;
>   	bool wake_set;
> +	unsigned long irq_flags;
>   };
>   
>   /**
> 

Thanks,
Hemant

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] mhi: use irq_flags if client driver configures it

[Jeffrey Hugo]

On 12/9/2020 11:34 AM, Hemant Kumar wrote:
> 
> 
> On 12/7/20 7:55 PM, Carl Huang wrote:
>> If client driver has specified the irq_flags, mhi uses this specified
>> irq_flags. Otherwise, mhi uses default irq_flags.
>>
>> The purpose of this change is to support one MSI vector for QCA6390.
>> MHI will use one same MSI vector too in this scenario.
>>
>> In case of one MSI vector, IRQ_NO_BALANCING is needed when irq handler
>> is requested. The reason is if irq migration happens, the msi_data may
>> change too. However, the msi_data is already programmed to QCA6390
>> hardware during initialization phase. This msi_data inconsistence will
>> result in crash in kernel.

I'm confused as to how this happens.

>>
>> Another issue is in case of one MSI vector, IRQF_NO_SUSPEND will trigger
>> WARNINGS because QCA6390 wants to disable the IRQ during the suspend.
>>
>> To avoid above two issues, QCA6390 driver specifies the irq_flags in case
>> of one MSI vector when mhi_register_controller is called.

Surely this change should be in a series where there is a following 
change which updates the QCA6390 driver?

>>
>> Signed-off-by: Carl Huang <cjhuang@codeaurora.org>
>> ---
>>   drivers/bus/mhi/core/init.c | 9 +++++++--
>>   include/linux/mhi.h         | 1 +
>>   2 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
>> index 0ffdebd..5f74e1e 100644
>> --- a/drivers/bus/mhi/core/init.c
>> +++ b/drivers/bus/mhi/core/init.c
>> @@ -148,12 +148,17 @@ int mhi_init_irq_setup(struct mhi_controller 
>> *mhi_cntrl)
>>   {
>>       struct mhi_event *mhi_event = mhi_cntrl->mhi_event;
>>       struct device *dev = &mhi_cntrl->mhi_dev->dev;
>> +    unsigned long irq_flags = IRQF_SHARED | IRQF_NO_SUSPEND;
>>       int i, ret;
>> +    /* if client driver has set irq_flags, use it */
>> +    if (mhi_cntrl->irq_flags)
>> +        irq_flags = mhi_cntrl->irq_flags;
> Jeff if i remember correctly your use case also have one dedicated irq 
> line for all the MSIs, just want to confirm if you are fine with this 
> change ? i was wondering if any input check is required for irq_flags 
> passed by controller, or responsibility is on controller for any 
> undesired behavior. Like passing IRQF_SHARED and IRQF_ONESHOT when one 
> irq line is shared among multiple MSIs.

This feels a bit weird to me, but I don't think it'll cause a problem.

If we are allowing the controller to specify flags, should they be in a 
per irq manner?

>> +
>>       /* Setup BHI_INTVEC IRQ */
>>       ret = request_threaded_irq(mhi_cntrl->irq[0], mhi_intvec_handler,
>>                      mhi_intvec_threaded_handler,
>> -                   IRQF_SHARED | IRQF_NO_SUSPEND,
>> +                   irq_flags,
>>                      "bhi", mhi_cntrl);
>>       if (ret)
>>           return ret;
>> @@ -171,7 +176,7 @@ int mhi_init_irq_setup(struct mhi_controller 
>> *mhi_cntrl)
>>           ret = request_irq(mhi_cntrl->irq[mhi_event->irq],
>>                     mhi_irq_handler,
>> -                  IRQF_SHARED | IRQF_NO_SUSPEND,
>> +                  irq_flags,
>>                     "mhi", mhi_event);
>>           if (ret) {
>>               dev_err(dev, "Error requesting irq:%d for ev:%d\n",
>> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
>> index d4841e5..f039e58 100644
>> --- a/include/linux/mhi.h
>> +++ b/include/linux/mhi.h
>> @@ -442,6 +442,7 @@ struct mhi_controller {
>>       bool fbc_download;
>>       bool pre_init;
>>       bool wake_set;
>> +    unsigned long irq_flags;

You don't document this.  That gets a NACK from me.

>>   };
>>   /**
>>
> 
> Thanks,
> Hemant
> 


-- 
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH] mhi: use irq_flags if client driver configures it

[Carl Huang]

On 2020-12-10 03:48, Jeffrey Hugo wrote:
> On 12/9/2020 11:34 AM, Hemant Kumar wrote:
>> 
>> 
>> On 12/7/20 7:55 PM, Carl Huang wrote:
>>> If client driver has specified the irq_flags, mhi uses this specified
>>> irq_flags. Otherwise, mhi uses default irq_flags.
>>> 
>>> The purpose of this change is to support one MSI vector for QCA6390.
>>> MHI will use one same MSI vector too in this scenario.
>>> 
>>> In case of one MSI vector, IRQ_NO_BALANCING is needed when irq 
>>> handler
>>> is requested. The reason is if irq migration happens, the msi_data 
>>> may
>>> change too. However, the msi_data is already programmed to QCA6390
>>> hardware during initialization phase. This msi_data inconsistence 
>>> will
>>> result in crash in kernel.
> 
> I'm confused as to how this happens.
> 
Host needs to program msi_data to QCA6390 hardware components(lots of 
standard
rings), and this msi_data is used to generate MSI interrupt.  If kernel 
has
re-assigned msi_data to QCA6390 when irq migration happens, and this 
re-assigned
msi_data is written to QCA6390 PCIe config space only, standard rings 
still use
previous msi_data.


>>> 
>>> Another issue is in case of one MSI vector, IRQF_NO_SUSPEND will 
>>> trigger
>>> WARNINGS because QCA6390 wants to disable the IRQ during the suspend.
>>> 
>>> To avoid above two issues, QCA6390 driver specifies the irq_flags in 
>>> case
>>> of one MSI vector when mhi_register_controller is called.
> 
> Surely this change should be in a series where there is a following
> change which updates the QCA6390 driver?
> 
Yes. This patch involves MHI module, so send it separately.
There is another patch set for QCA6390 to support one MSI vector.

>>> 
>>> Signed-off-by: Carl Huang <cjhuang@codeaurora.org>
>>> ---
>>>   drivers/bus/mhi/core/init.c | 9 +++++++--
>>>   include/linux/mhi.h         | 1 +
>>>   2 files changed, 8 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/drivers/bus/mhi/core/init.c 
>>> b/drivers/bus/mhi/core/init.c
>>> index 0ffdebd..5f74e1e 100644
>>> --- a/drivers/bus/mhi/core/init.c
>>> +++ b/drivers/bus/mhi/core/init.c
>>> @@ -148,12 +148,17 @@ int mhi_init_irq_setup(struct mhi_controller 
>>> *mhi_cntrl)
>>>   {
>>>       struct mhi_event *mhi_event = mhi_cntrl->mhi_event;
>>>       struct device *dev = &mhi_cntrl->mhi_dev->dev;
>>> +    unsigned long irq_flags = IRQF_SHARED | IRQF_NO_SUSPEND;
>>>       int i, ret;
>>> +    /* if client driver has set irq_flags, use it */
>>> +    if (mhi_cntrl->irq_flags)
>>> +        irq_flags = mhi_cntrl->irq_flags;
>> Jeff if i remember correctly your use case also have one dedicated irq 
>> line for all the MSIs, just want to confirm if you are fine with this 
>> change ? i was wondering if any input check is required for irq_flags 
>> passed by controller, or responsibility is on controller for any 
>> undesired behavior. Like passing IRQF_SHARED and IRQF_ONESHOT when one 
>> irq line is shared among multiple MSIs.
> 
> This feels a bit weird to me, but I don't think it'll cause a problem.
> 
> If we are allowing the controller to specify flags, should they be in
> a per irq manner?
> 
Not sure if per irq manner is needed for others, but ath11k doesn't need
per irq manner.

>>> +
>>>       /* Setup BHI_INTVEC IRQ */
>>>       ret = request_threaded_irq(mhi_cntrl->irq[0], 
>>> mhi_intvec_handler,
>>>                      mhi_intvec_threaded_handler,
>>> -                   IRQF_SHARED | IRQF_NO_SUSPEND,
>>> +                   irq_flags,
>>>                      "bhi", mhi_cntrl);
>>>       if (ret)
>>>           return ret;
>>> @@ -171,7 +176,7 @@ int mhi_init_irq_setup(struct mhi_controller 
>>> *mhi_cntrl)
>>>           ret = request_irq(mhi_cntrl->irq[mhi_event->irq],
>>>                     mhi_irq_handler,
>>> -                  IRQF_SHARED | IRQF_NO_SUSPEND,
>>> +                  irq_flags,
>>>                     "mhi", mhi_event);
>>>           if (ret) {
>>>               dev_err(dev, "Error requesting irq:%d for ev:%d\n",
>>> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
>>> index d4841e5..f039e58 100644
>>> --- a/include/linux/mhi.h
>>> +++ b/include/linux/mhi.h
>>> @@ -442,6 +442,7 @@ struct mhi_controller {
>>>       bool fbc_download;
>>>       bool pre_init;
>>>       bool wake_set;
>>> +    unsigned long irq_flags;
> 
> You don't document this.  That gets a NACK from me.
> 
Yes, will document this field in V2.

>>>   };
>>>   /**
>>> 
>> 
>> Thanks,
>> Hemant
>>