Re: [PATCH net-next 3/5] net: wwan: t7xx: PCIe reset rescan

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Tue, 16 Aug 2022, m.chetan.kumar@intel.com wrote:

> From: Haijun Liu <haijun.liu@mediatek.com>
> 
> PCI rescan module implements "rescan work queue". In firmware flashing
> or coredump collection procedure WWAN device is programmed to boot in
> fastboot mode and a work item is scheduled for removal & detection.
> The WWAN device is reset using APCI call as part driver removal flow.
> Work queue rescans pci bus at fixed interval for device detection,
> later when device is detect work queue exits.
> 
> Signed-off-by: Haijun Liu <haijun.liu@mediatek.com>
> Co-developed-by: Madhusmita Sahu <madhusmita.sahu@intel.com>
> Signed-off-by: Madhusmita Sahu <madhusmita.sahu@intel.com>
> Signed-off-by: Ricardo Martinez <ricardo.martinez@linux.intel.com>
> Signed-off-by: M Chetan Kumar <m.chetan.kumar@linux.intel.com>
> Signed-off-by: Devegowda Chandrashekar <chandrashekar.devegowda@intel.com>
> ---

> +       bool                    hp_enable;

Never written to.

> diff --git a/drivers/net/wwan/t7xx/t7xx_pci_rescan.c b/drivers/net/wwan/t7xx/t7xx_pci_rescan.c
> new file mode 100644
> index 000000000000..045777d8a843
> --- /dev/null
> +++ b/drivers/net/wwan/t7xx/t7xx_pci_rescan.c
> @@ -0,0 +1,117 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2021, MediaTek Inc.
> + * Copyright (c) 2021-2022, Intel Corporation.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ":t7xx:%s: " fmt, __func__
> +#define dev_fmt(fmt) "t7xx: " fmt
> +
> +#include <linux/delay.h>
> +#include <linux/pci.h>
> +#include <linux/spinlock.h>
> +#include <linux/workqueue.h>
> +
> +#include "t7xx_pci.h"
> +#include "t7xx_pci_rescan.h"
> +
> +static struct remove_rescan_context g_mtk_rescan_context;

Any particular reason for this name?

> +void t7xx_pci_dev_rescan(void)
> +{
> +	struct pci_bus *b = NULL;
> +
> +	pci_lock_rescan_remove();
> +	while ((b = pci_find_next_bus(b)))
> +		pci_rescan_bus(b);
> +
> +	pci_unlock_rescan_remove();

I'd remove the empty line to keep the critical sections grouped together. 

> +}
> +
> +void t7xx_rescan_done(void)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&g_mtk_rescan_context.dev_lock, flags);
> +	if (g_mtk_rescan_context.rescan_done == 0) {
> +		pr_debug("this is a rescan probe\n");
> +		g_mtk_rescan_context.rescan_done = 1;
> +	} else {
> +		pr_debug("this is a init probe\n");
> +	}
> +	spin_unlock_irqrestore(&g_mtk_rescan_context.dev_lock, flags);
> +}
> +
> +static void t7xx_remove_rescan(struct work_struct *work)
> +{
> +	struct pci_dev *pdev;
> +	int num_retries = RESCAN_RETRIES;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&g_mtk_rescan_context.dev_lock, flags);
> +	g_mtk_rescan_context.rescan_done = 0;
> +	pdev = g_mtk_rescan_context.dev;
> +	spin_unlock_irqrestore(&g_mtk_rescan_context.dev_lock, flags);
> +
> +	if (pdev) {
> +		pci_stop_and_remove_bus_device_locked(pdev);
> +		pr_debug("start remove and rescan flow\n");
> +	}
> +
> +	do {
> +		t7xx_pci_dev_rescan();
> +		spin_lock_irqsave(&g_mtk_rescan_context.dev_lock, flags);
> +		if (g_mtk_rescan_context.rescan_done) {
> +			spin_unlock_irqrestore(&g_mtk_rescan_context.dev_lock, flags);
> +			break;
> +		}
> +
> +		spin_unlock_irqrestore(&g_mtk_rescan_context.dev_lock, flags);

Ditto.

> +		msleep(DELAY_RESCAN_MTIME);
> +	} while (num_retries--);
> +}
> +
> +void t7xx_rescan_queue_work(struct pci_dev *pdev)
> +{
> +	unsigned long flags;
> +
> +	dev_info(&pdev->dev, "start queue_mtk_rescan_work\n");
> +	spin_lock_irqsave(&g_mtk_rescan_context.dev_lock, flags);
> +	if (!g_mtk_rescan_context.rescan_done) {
> +		dev_err(&pdev->dev, "rescan failed because last rescan undone\n");

The meaning of the message is hard to understand.

> +		spin_unlock_irqrestore(&g_mtk_rescan_context.dev_lock, flags);
> +		return;
> +	}
> +
> +	g_mtk_rescan_context.dev = pdev;
> +	spin_unlock_irqrestore(&g_mtk_rescan_context.dev_lock, flags);

Crit section newlines.

> +	queue_work(g_mtk_rescan_context.pcie_rescan_wq, &g_mtk_rescan_context.service_task);
> +}
> +
> +int t7xx_rescan_init(void)
> +{
> +	spin_lock_init(&g_mtk_rescan_context.dev_lock);
> +	g_mtk_rescan_context.rescan_done = 1;
> +	g_mtk_rescan_context.dev = NULL;
> +	g_mtk_rescan_context.pcie_rescan_wq = create_singlethread_workqueue(MTK_RESCAN_WQ);
> +	if (!g_mtk_rescan_context.pcie_rescan_wq) {
> +		pr_err("Failed to create workqueue: %s\n", MTK_RESCAN_WQ);
> +		return -ENOMEM;
> +	}
> +
> +	INIT_WORK(&g_mtk_rescan_context.service_task, t7xx_remove_rescan);
> +
> +	return 0;
> +}
> +
> +void t7xx_rescan_deinit(void)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&g_mtk_rescan_context.dev_lock, flags);
> +	g_mtk_rescan_context.rescan_done = 0;
> +	g_mtk_rescan_context.dev = NULL;
> +	spin_unlock_irqrestore(&g_mtk_rescan_context.dev_lock, flags);
> +	cancel_work_sync(&g_mtk_rescan_context.service_task);
> +	destroy_workqueue(g_mtk_rescan_context.pcie_rescan_wq);
> +}

In general, I felt this whole file was very heavy to read compared with 
the other parts of t7xx code. Maybe it will get better if 
g_mtk_rescan_context becomes shorter and re-newlining is done to better 
indicate the critical sections but we'll see.

> diff --git a/drivers/net/wwan/t7xx/t7xx_pci_rescan.h b/drivers/net/wwan/t7xx/t7xx_pci_rescan.h
> new file mode 100644
> index 000000000000..de4ca1363bb0
> --- /dev/null
> +++ b/drivers/net/wwan/t7xx/t7xx_pci_rescan.h
> @@ -0,0 +1,29 @@
> +/* SPDX-License-Identifier: GPL-2.0-only
> + *
> + * Copyright (c) 2021, MediaTek Inc.
> + * Copyright (c) 2021-2022, Intel Corporation.
> + */
> +
> +#ifndef __T7XX_PCI_RESCAN_H__
> +#define __T7XX_PCI_RESCAN_H__
> +
> +#define MTK_RESCAN_WQ "mtk_rescan_wq"
> +
> +#define DELAY_RESCAN_MTIME 1000
> +#define RESCAN_RETRIES 35
> +
> +struct remove_rescan_context {
> +	struct work_struct	 service_task;

Extra whitespace.

> +	struct workqueue_struct *pcie_rescan_wq;
> +	spinlock_t		dev_lock; /* protects device */

Perhaps use a tab before the comment instead to make some room.

> +	struct pci_dev		*dev;
> +	int			rescan_done;
> +};


-- 
 i.