netdev - Re: [net-next v2] net: wwan: t7xx: PCIe reset rescan

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <764cf372-fcf3-410d-b48e-3fadc01f4707@redhat.com>
Date: Tue, 13 Aug 2024 11:50:20 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: 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: linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
 linux-doc@...r.kernel.org, angelogioacchino.delregno@...labora.com,
 linux-arm-kernel@...ts.infradead.org, matthias.bgg@...il.com,
 corbet@....net, linux-mediatek@...ts.infradead.org, helgaas@...nel.org,
 danielwinkler@...gle.com, korneld@...gle.com
Subject: Re: [net-next v2] net: wwan: t7xx: PCIe reset rescan

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.

> 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...

> 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.

Cheers,

Paolo