| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <a5ea5263-8acb-48dd-a4e1-bc48a9bdf791@quicinc.com>
Date: Wed, 17 Apr 2024 12:41:25 +0800
From: Qiang Yu <quic_qianyu@...cinc.com>
To: Mayank Rana <quic_mrana@...cinc.com>, <mani@...nel.org>,
<quic_jhugo@...cinc.com>
CC: <mhi@...ts.linux.dev>, <linux-arm-msm@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <quic_cang@...cinc.com>
Subject: Re: [PATCH v3 3/3] bus: mhi: host: pci_generic: Add edl callback to
enter EDL
On 4/17/2024 11:01 AM, Qiang Yu wrote:
>
> On 4/17/2024 2:12 AM, Mayank Rana wrote:
>>
>>
>> On 4/15/2024 8:50 PM, Qiang Yu wrote:
>>>
>>> On 4/16/2024 7:53 AM, Mayank Rana wrote:
>>>> Hi Qiang
>>>>
>>>> On 4/15/2024 1:49 AM, Qiang Yu wrote:
>>>>> Add mhi_pci_generic_edl_trigger as edl_trigger for some devices
>>>>> (eg. SDX65)
>>>>> to enter EDL mode by writing the 0xEDEDEDED cookie to the channel 91
>>>>> doorbell register and forcing an SOC reset afterwards.
>>>>>
>>>>> Signed-off-by: Qiang Yu <quic_qianyu@...cinc.com>
>>>>> ---
>>>>> drivers/bus/mhi/host/pci_generic.c | 47
>>>>> ++++++++++++++++++++++++++++++++++++++
>>>>> 1 file changed, 47 insertions(+)
>>>>>
>>>>> diff --git a/drivers/bus/mhi/host/pci_generic.c
>>>>> b/drivers/bus/mhi/host/pci_generic.c
>>>>> index 51639bf..cbf8a58 100644
>>>>> --- a/drivers/bus/mhi/host/pci_generic.c
>>>>> +++ b/drivers/bus/mhi/host/pci_generic.c
>>>>> @@ -27,12 +27,19 @@
>>>>> #define PCI_VENDOR_ID_THALES 0x1269
>>>>> #define PCI_VENDOR_ID_QUECTEL 0x1eac
>>>>> +#define MHI_EDL_DB 91
>>>>> +#define MHI_EDL_COOKIE 0xEDEDEDED
>>>>> +
>>>>> +/* Device can enter EDL by first setting edl cookie then issuing
>>>>> inband reset*/
>>>>> +#define MHI_PCI_GENERIC_EDL_TRIGGER BIT(0)
>>>>> +
>>>>> /**
>>>>> * struct mhi_pci_dev_info - MHI PCI device specific information
>>>>> * @config: MHI controller configuration
>>>>> * @name: name of the PCI module
>>>>> * @fw: firmware path (if any)
>>>>> * @edl: emergency download mode firmware path (if any)
>>>>> + * @edl_trigger: each bit represents a different way to enter EDL
>>>>> * @bar_num: PCI base address register to use for MHI MMIO
>>>>> register space
>>>>> * @dma_data_width: DMA transfer word size (32 or 64 bits)
>>>>> * @mru_default: default MRU size for MBIM network packets
>>>>> @@ -44,6 +51,7 @@ struct mhi_pci_dev_info {
>>>>> const char *name;
>>>>> const char *fw;
>>>>> const char *edl;
>>>>> + unsigned int edl_trigger;
>>>>> unsigned int bar_num;
>>>>> unsigned int dma_data_width;
>>>>> unsigned int mru_default;
>>>>> @@ -292,6 +300,7 @@ static const struct mhi_pci_dev_info
>>>>> mhi_qcom_sdx75_info = {
>>>>> .name = "qcom-sdx75m",
>>>>> .fw = "qcom/sdx75m/xbl.elf",
>>>>> .edl = "qcom/sdx75m/edl.mbn",
>>>>> + .edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
>>>>> .config = &modem_qcom_v2_mhiv_config,
>>>>> .bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>>>>> .dma_data_width = 32,
>>>>> @@ -302,6 +311,7 @@ static const struct mhi_pci_dev_info
>>>>> mhi_qcom_sdx65_info = {
>>>>> .name = "qcom-sdx65m",
>>>>> .fw = "qcom/sdx65m/xbl.elf",
>>>>> .edl = "qcom/sdx65m/edl.mbn",
>>>>> + .edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
>>>>> .config = &modem_qcom_v1_mhiv_config,
>>>>> .bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>>>>> .dma_data_width = 32,
>>>>> @@ -312,6 +322,7 @@ static const struct mhi_pci_dev_info
>>>>> mhi_qcom_sdx55_info = {
>>>>> .name = "qcom-sdx55m",
>>>>> .fw = "qcom/sdx55m/sbl1.mbn",
>>>>> .edl = "qcom/sdx55m/edl.mbn",
>>>>> + .edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
>>>>> .config = &modem_qcom_v1_mhiv_config,
>>>>> .bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>>>>> .dma_data_width = 32,
>>>>> @@ -928,6 +939,39 @@ static void health_check(struct timer_list *t)
>>>>> mod_timer(&mhi_pdev->health_check_timer, jiffies +
>>>>> HEALTH_CHECK_PERIOD);
>>>>> }
>>>>> +static int mhi_pci_generic_edl_trigger(struct mhi_controller
>>>>> *mhi_cntrl)
>>>>> +{
>>>>> + void __iomem *base = mhi_cntrl->regs;
>>>>> + void __iomem *edl_db;
>>>>> + int ret;
>>>>> + u32 val;
>>>>> +
>>>>> + ret = mhi_device_get_sync(mhi_cntrl->mhi_dev);
>>>>> + if (ret) {
>>>>> + dev_err(mhi_cntrl->cntrl_dev, "Wake up device fail before
>>>>> trigger EDL\n");
>>>>> + return ret;
>>>>> + }
>>>>> +
>>>>> + pm_wakeup_event(&mhi_cntrl->mhi_dev->dev, 0);
>>>>> + mhi_cntrl->runtime_get(mhi_cntrl);
>>>>> +
>>>>> + ret = mhi_get_channel_doorbell(mhi_cntrl, &val);
>>>>> + if (ret)
>>>>> + return ret;
>>>> Don't we need error handling part here i.e. calling
>>>> mhi_cntrl->runtime_put() as well mhi_device_put() ?
>>>
>>> Hi Mayank
>>>
>>> After soc_reset, device will reboot to EDL mode and MHI state will
>>> be SYSERR. So host will fail to suspend
>>> anyway. The "error handling" we need here is actually to enter EDL
>>> mode, this will be done by SYSERR irq.
>>> Here, mhi_cntrl->runtime_put() and mhi_device_put() are only to
>>> balance mhi_cntrl->runtime_get() and
>>> mhi_device_get_sync().
>>>
>>> Thanks,
>>> Qiang
>> I am saying is there possibility that mhi_get_channel_doorbell()
>> returns error ?
>> If yes, then why don't we need error handling as part of it. you are
>> exiting if this API return error without doing anything.
>
> I think here mhi_get_channel_doorbell will not return error. But I still
> add a error checking because it invoked mhi_read_reg, which is a must
> check
> API. For modem mhi controller, this API eventually does a memory read.
> This memory read will return a normal value if link is up and all f's
> if link
> is bad.
>
> Thanks,
> Qiang
Actually, mhi_get_channel_doorbell should also be used in mhi_init_mmio to
replace the getting chdb operation by invoking mhi_read_reg as Mani
commented.
In mhi_init_mmio, we invoke mhi_read_reg many times, but there is also not
additionnal error handling.
I'm not very sure the reason but perhaps if mhi_read_reg returns error
(I don't
know which controller will provide a memory read callback returning
errors), most
probably something is wrong in PCIe, which is not predictable by MHI and
we can
not add err handling every time invoking mhi_read_reg. But we have a
timer to
do health_check in pci_generic.c. If link is down, we will do
pci_function_reset
to try to reovery.
Hi Mani, sorry, may I know the purpose of adding must_check attribute to
mhi_read_reg? In which case will mhi controller provide a callback that
returns error?
Thanks,
Qiang
>>>>> + edl_db = base + val + (8 * MHI_EDL_DB);
>>>>> +
>>>>> + mhi_cntrl->write_reg(mhi_cntrl, edl_db + 4,
>>>>> upper_32_bits(MHI_EDL_COOKIE));
>>>>> + mhi_cntrl->write_reg(mhi_cntrl, edl_db,
>>>>> lower_32_bits(MHI_EDL_COOKIE));
>>>>> +
>>>>> + mhi_soc_reset(mhi_cntrl);
>>>>> +
>>>>> + mhi_cntrl->runtime_put(mhi_cntrl);
>>>>> + mhi_device_put(mhi_cntrl->mhi_dev);
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> static int mhi_pci_probe(struct pci_dev *pdev, const struct
>>>>> pci_device_id *id)
>>>>> {
>>>>> const struct mhi_pci_dev_info *info = (struct
>>>>> mhi_pci_dev_info *) id->driver_data;
>>>>> @@ -962,6 +1006,9 @@ static int mhi_pci_probe(struct pci_dev
>>>>> *pdev, const struct pci_device_id *id)
>>>>> mhi_cntrl->runtime_put = mhi_pci_runtime_put;
>>>>> mhi_cntrl->mru = info->mru_default;
>>>>> + if (info->edl_trigger & MHI_PCI_GENERIC_EDL_TRIGGER)
>>>>> + mhi_cntrl->edl_trigger = mhi_pci_generic_edl_trigger;
>>>>> +
>>>>> if (info->sideband_wake) {
>>>>> mhi_cntrl->wake_get = mhi_pci_wake_get_nop;
>>>>> mhi_cntrl->wake_put = mhi_pci_wake_put_nop;
>>>> Regards,
>>>> Mayank
>
Powered by blists - more mailing lists