| 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: <20240814113253.5471-1-jinjian.song@fibocom.com>
Date: Wed, 14 Aug 2024 19:32:53 +0800
From: Jinjian Song <jinjian.song@...ocom.com>
To: pabeni@...hat.com,
Jinjian Song <jinjian.song@...ocom.com>,
chandrashekar.devegowda@...el.com,
chiranjeevi.rapolu@...ux.intel.com,
haijun.liu@...iatek.com,
m.chetan.kumar@...ux.intel.com,
ricardo.martinez@...ux.intel.com,
loic.poulain@...aro.org,
ryazanov.s.a@...il.com,
johannes@...solutions.net,
davem@...emloft.net,
edumazet@...gle.com,
kuba@...nel.org
Cc: angelogioacchino.delregno@...labora.com,
corbet@....net,
danielwinkler@...gle.com,
helgaas@...nel.org,
korneld@...gle.com,
linux-arm-kernel@...ts.infradead.org,
linux-doc@...r.kernel.org,
linux-kernel@...r.kernel.org,
linux-mediatek@...ts.infradead.org,
matthias.bgg@...il.com,
netdev@...r.kernel.org
Subject: Re: [net-next v2] net: wwan: t7xx: PCIe reset rescan
From: Paolo Abeni <pabeni@...hat.com>
>On 8/9/24 05:37, Jinjian Song wrote:
>> WWAN device is programmed to boot in normal mode or fastboot mode,
>> when triggering a device reset through ACPI call or fastboot switch
>> command.Maintain state machine synchronization and reprobe logic
> ^^^ ^^^
>Minor nits: missing space and missing article above.
Please let me modify it.
>> after a device reset.
>>
>> Suggestion from Bjorn:
>> Link: https://lore.kernel.org/all/20230127133034.GA1364550@bhelgaas/
>>
>> Signed-off-by: Jinjian Song <jinjian.song@...ocom.com>
>> ---
>> V2:
>> * initialize the variable 'ret' in t7xx_reset_device() function
>> ---
>> drivers/net/wwan/t7xx/t7xx_modem_ops.c | 47 ++++++++++++++++---
>> drivers/net/wwan/t7xx/t7xx_modem_ops.h | 9 +++-
>> drivers/net/wwan/t7xx/t7xx_pci.c | 53 ++++++++++++++++++----
>> drivers/net/wwan/t7xx/t7xx_pci.h | 3 ++
>> drivers/net/wwan/t7xx/t7xx_port_proxy.c | 1 -
>> drivers/net/wwan/t7xx/t7xx_port_trace.c | 1 +
>> drivers/net/wwan/t7xx/t7xx_state_monitor.c | 34 +++++---------
>> 7 files changed, 105 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/net/wwan/t7xx/t7xx_modem_ops.c b/drivers/net/wwan/t7xx/t7xx_modem_ops.c
>> index 8d864d4ed77f..79f17100f70b 100644
>> --- a/drivers/net/wwan/t7xx/t7xx_modem_ops.c
>> +++ b/drivers/net/wwan/t7xx/t7xx_modem_ops.c
>> @@ -53,6 +53,7 @@
>>
>> #define RGU_RESET_DELAY_MS 10
>> #define PORT_RESET_DELAY_MS 2000
>> +#define FASTBOOT_RESET_DELAY_MS 2000
>> #define EX_HS_TIMEOUT_MS 5000
>> #define EX_HS_POLL_DELAY_MS 10
>>
>> @@ -167,19 +168,52 @@ static int t7xx_acpi_reset(struct t7xx_pci_dev *t7xx_dev, char *fn_name)
>> }
>>
>> kfree(buffer.pointer);
>> +#else
>> + struct device *dev = &t7xx_dev->pdev->dev;
>> + int ret;
>>
>> + ret = pci_reset_function(t7xx_dev->pdev);
>> + if (ret) {
>> + dev_err(dev, "Failed to reset device, error:%d\n", ret);
>> + return ret;
>> + }
>> #endif
>> return 0;
>> }
>>
>> -int t7xx_acpi_fldr_func(struct t7xx_pci_dev *t7xx_dev)
>> +static void t7xx_host_event_notify(struct t7xx_pci_dev *t7xx_dev, unsigned int event_id)
>> {
>> - return t7xx_acpi_reset(t7xx_dev, "_RST");
>> + u32 value;
>> +
>> + value = ioread32(IREG_BASE(t7xx_dev) + T7XX_PCIE_MISC_DEV_STATUS);
>> + value &= ~HOST_EVENT_MASK;
>> + value |= FIELD_PREP(HOST_EVENT_MASK, event_id);
>> + iowrite32(value, IREG_BASE(t7xx_dev) + T7XX_PCIE_MISC_DEV_STATUS);
>> }
>>
>> -int t7xx_acpi_pldr_func(struct t7xx_pci_dev *t7xx_dev)
>> +int t7xx_reset_device(struct t7xx_pci_dev *t7xx_dev, enum reset_type type)
>> {
>> - return t7xx_acpi_reset(t7xx_dev, "MRST._RST");
>> + int ret = 0;
>> +
>> + pci_save_state(t7xx_dev->pdev);
>> + t7xx_pci_reprobe_early(t7xx_dev);
>> + t7xx_mode_update(t7xx_dev, T7XX_RESET);
>> +
>> + if (type == FLDR) {
>> + ret = t7xx_acpi_reset(t7xx_dev, "_RST");
>> + } else if (type == PLDR) {
>> + ret = t7xx_acpi_reset(t7xx_dev, "MRST._RST");
>> + } else if (type == FASTBOOT) {
>> + t7xx_host_event_notify(t7xx_dev, FASTBOOT_DL_NOTIFY);
>> + t7xx_mhccif_h2d_swint_trigger(t7xx_dev, H2D_CH_DEVICE_RESET);
>> + msleep(FASTBOOT_RESET_DELAY_MS);
>
>Did you test this with CONFIG_DEBUG_ATOMIC_SLEEP?
>
>AFAICS the above adds a 2s (FASTBOOT_RESET_DELAY_MS) unconditional sleep
>for 'fastboot' inside am IRQ thread function via:
>
>t7xx_rgu_isr_thread() -> t7xx_reset_device_via_pmic() -> t7xx_reset_device()
>
>Well, the existing code already looks broken with a 10 ms sleep there,
>still...
I have not test this with CONFIG_DEBUG_ATOMIC_SLEEP, The FATBOOT reset don't use
during IRQ thread, it only used by sysfs attribute interface, so I think we don't
have to worry about this.
Device IRQ -> t7xx_rgu_isr_thread()->t7xx_reset_device_via_pmic()-> only support FLDR and PLDR reset as before.
sysfs attribute 't7xx_mode' -> t7xx_reset_device(FASTBOOT), this way will use FASTBOOT reset.
>> diff --git a/drivers/net/wwan/t7xx/t7xx_pci.c b/drivers/net/wwan/t7xx/t7xx_pci.c
>> index 10a8c1080b10..2398f41046ce 100644
>> --- a/drivers/net/wwan/t7xx/t7xx_pci.c
>> +++ b/drivers/net/wwan/t7xx/t7xx_pci.c
>> @@ -67,6 +67,7 @@ static ssize_t t7xx_mode_store(struct device *dev,
>> struct device_attribute *attr,
>> const char *buf, size_t count)
>> {
>> + enum t7xx_mode mode;
>> struct t7xx_pci_dev *t7xx_dev;
>> struct pci_dev *pdev;
>> int index = 0;
>
>Minor nit: please respect the reverse xmas tree above.
>
Please let me modify it.
Thanks,
Best Regards,
Jinjian
Powered by blists - more mailing lists