Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <7f72b026-5808-ee0d-894d-bf50bf162023@quicinc.com>
Date:   Tue, 21 Jun 2022 17:24:45 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.0
Subject: Re: [PATCH] bus: mhi: Disable IRQs instead of freeing them during
 power down
Content-Language: en-US
To:     Jeffrey Hugo <quic_jhugo@quicinc.com>, <mani@kernel.org>,
        <quic_hemantk@quicinc.com>, <loic.poulain@linaro.org>
CC:     <mhi@lists.linux.dev>, <linux-arm-msm@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
References: <1654782215-70383-1-git-send-email-quic_qianyu@quicinc.com>
 <62d09e6f-9898-6233-dfd6-b5ba5d837571@quicinc.com>
 <c93aa921-b327-fdf4-adae-f0dd77e8789e@quicinc.com>
From:   Qiang Yu <quic_qianyu@quicinc.com>
In-Reply-To: <c93aa921-b327-fdf4-adae-f0dd77e8789e@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 6/21/2022 3:57 AM, Jeffrey Hugo wrote:
> On 6/9/2022 7:54 AM, Jeffrey Hugo wrote:
>> On 6/9/2022 7:43 AM, Qiang Yu wrote:
>>> EP tends to read MSI address/data once and cache them after BME is set.
>>> So host should avoid changing MSI address/data after BME is set.
>>>
>>> In pci reset function, host invokes free_irq(), which also clears MSI
>>> address/data in EP's PCIe config space. If the invalid address/data
>>> are cached and used by EP, MSI triggered by EP wouldn't be received by
>>> host, because an invalid MSI data is sent to an invalid MSI address.
>>>
>>> To fix this issue, after host runs request_irq() successfully during
>>> mhi driver probe, let's invoke enable_irq()/disable_irq() instead of
>>> request_irq()/free_irq() when we want to power on and power down MHI.
>>> Meanwhile, Host should invoke free_irq() when mhi host driver is
>>> removed.
>>
>> I don't think this works for hotplug, nor cases where there are 
>> multiple MHI devices on the system.
>>
>> The EP shouldn't be caching this information for multiple reasons. 
>> Masking the MSIs, disabling the MSIs, changing the address when the 
>> affinity changes, etc.
>>
>> It really feels like we are solving the problem in the wrong place.
>>
>> Right now, this gets a NACK from me.
>>
>>>
>>> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
>>> ---
>>>   drivers/bus/mhi/host/init.c        | 31 
>>> +++++++++++++++++++++++++++++++
>>>   drivers/bus/mhi/host/pci_generic.c |  2 ++
>>>   drivers/bus/mhi/host/pm.c          |  4 ++--
>>>   3 files changed, 35 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
>>> index cbb86b2..48cb093 100644
>>> --- a/drivers/bus/mhi/host/init.c
>>> +++ b/drivers/bus/mhi/host/init.c
>>> @@ -18,6 +18,7 @@
>>>   #include <linux/slab.h>
>>>   #include <linux/vmalloc.h>
>>>   #include <linux/wait.h>
>>> +#include <linux/irq.h>
>>
>> Should be in alphabetical order
>>
>>>   #include "internal.h"
>>>   static DEFINE_IDA(mhi_controller_ida);
>>> @@ -168,6 +169,22 @@ int mhi_init_irq_setup(struct mhi_controller 
>>> *mhi_cntrl)
>>>       unsigned long irq_flags = IRQF_SHARED | IRQF_NO_SUSPEND;
>>>       int i, ret;
>>> +    /*
>>> +     * if irq[0] has action, it represents all MSI IRQs have been
>>> +     * requested, so we just need to enable them.
>>> +     */
>>
>> This seems like an assumption about how the interrupts are allocated 
>> and assigned that may not hold true for all devices.
>
> Ah, I see.  This goes to the assumption that the BHI interrupt is 
> always line 0, even though as far as I am aware, the spec doesn't 
> require that.  The comment could be clearer I think.
>
>>
>>> +    if (irq_has_action(mhi_cntrl->irq[0])) {
>>> +        enable_irq(mhi_cntrl->irq[0]);
>>> +
>>> +        for (i = 0; i < mhi_cntrl->total_ev_rings; i++, mhi_event++) {
>>> +            if (mhi_event->offload_ev)
>>> +                continue;
>>> +
>>> +            enable_irq(mhi_cntrl->irq[mhi_event->irq]);
>>> +        }
>>> +        return 0;
>>> +    }
>
> Please no.  This overloads the function to have two different 
> behaviors, and it doesn't match the inline disables.
>
> Since you have inline disables, I would prefer inline enables so that 
> the code is "balanced".

I remove this in v2, directly invoke enables during power up.  It 
becomes like this:

mhi power up ->enable_irq,     mhi  power down ->disable irq

pci_probe->mhi_register_controller->request_irq, 
pci_remove->mhi_unregister_controller->free irq

>
>>> +
>>>       /* if controller driver has set irq_flags, use it */
>>>       if (mhi_cntrl->irq_flags)
>>>           irq_flags = mhi_cntrl->irq_flags;
>>> @@ -179,6 +196,11 @@ int mhi_init_irq_setup(struct mhi_controller 
>>> *mhi_cntrl)
>>>                      "bhi", mhi_cntrl);
>>>       if (ret)
>>>           return ret;
>>> +    /*
>>> +     * IRQ marked IRQF_SHARED isn't recommended to use IRQ_NOAUTOEN,
>>> +     * so disable it explicitly.
>>> +     */
>>> +    disable_irq(mhi_cntrl->irq[0]);
>>>       for (i = 0; i < mhi_cntrl->total_ev_rings; i++, mhi_event++) {
>>>           if (mhi_event->offload_ev)
>>> @@ -200,6 +222,8 @@ int mhi_init_irq_setup(struct mhi_controller 
>>> *mhi_cntrl)
>>>                   mhi_cntrl->irq[mhi_event->irq], i);
>>>               goto error_request;
>>>           }
>>> +
>>> +        disable_irq(mhi_cntrl->irq[mhi_event->irq]);
>>>       }
>>>       return 0;
>>> @@ -1003,8 +1027,14 @@ int mhi_register_controller(struct 
>>> mhi_controller *mhi_cntrl,
>>>       mhi_create_debugfs(mhi_cntrl);
>>> +    ret = mhi_init_irq_setup(mhi_cntrl);
>>> +    if (ret)
>>> +        goto error_setup_irq;
>>> +
>>>       return 0;
>>> +error_setup_irq:
>>> +    mhi_destroy_debugfs(mhi_cntrl);
>>>   err_release_dev:
>>>       put_device(&mhi_dev->dev);
>>>   err_ida_free:
>>> @@ -1027,6 +1057,7 @@ void mhi_unregister_controller(struct 
>>> mhi_controller *mhi_cntrl)
>>>       struct mhi_chan *mhi_chan = mhi_cntrl->mhi_chan;
>>>       unsigned int i;
>>> +    mhi_deinit_free_irq(mhi_cntrl);
>>>       mhi_destroy_debugfs(mhi_cntrl);
>>>       destroy_workqueue(mhi_cntrl->hiprio_wq);
>>> diff --git a/drivers/bus/mhi/host/pci_generic.c 
>>> b/drivers/bus/mhi/host/pci_generic.c
>>> index 6fbc591..60020d0 100644
>>> --- a/drivers/bus/mhi/host/pci_generic.c
>>> +++ b/drivers/bus/mhi/host/pci_generic.c
>>> @@ -945,6 +945,8 @@ static void mhi_pci_remove(struct pci_dev *pdev)
>>>       mhi_unregister_controller(mhi_cntrl);
>>>       pci_disable_pcie_error_reporting(pdev);
>>> +
>>> +    pci_free_irq_vectors(pdev);
>
> This seems like a random change that should be in a different patch. 
> Why is it included here?

During probe, host alloc irq venctor, so I add this change to free 
vector. I remove it in v2.

>
>>>   }
>>>   static void mhi_pci_shutdown(struct pci_dev *pdev)
>>> diff --git a/drivers/bus/mhi/host/pm.c b/drivers/bus/mhi/host/pm.c
>>> index dc2e8ff..190231c 100644
>>> --- a/drivers/bus/mhi/host/pm.c
>>> +++ b/drivers/bus/mhi/host/pm.c
>>> @@ -500,7 +500,7 @@ static void mhi_pm_disable_transition(struct 
>>> mhi_controller *mhi_cntrl)
>>>       for (i = 0; i < mhi_cntrl->total_ev_rings; i++, mhi_event++) {
>>>           if (mhi_event->offload_ev)
>>>               continue;
>>> -        free_irq(mhi_cntrl->irq[mhi_event->irq], mhi_event);
>>> +        disable_irq(mhi_cntrl->irq[mhi_event->irq]);
>>>           tasklet_kill(&mhi_event->task);
>>>       }
>>> @@ -1182,7 +1182,7 @@ void mhi_power_down(struct mhi_controller 
>>> *mhi_cntrl, bool graceful)
>>>       /* Wait for shutdown to complete */
>>>       flush_work(&mhi_cntrl->st_worker);
>>> -    free_irq(mhi_cntrl->irq[0], mhi_cntrl);
>>> +    disable_irq(mhi_cntrl->irq[0]);
>>>   }
>>>   EXPORT_SYMBOL_GPL(mhi_power_down);
>>
>>
>