| 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
| ||
|
Date: Thu, 18 Aug 2022 17:58:24 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: m.chetan.kumar@...el.com
cc: Netdev <netdev@...r.kernel.org>, kuba@...nel.org,
davem@...emloft.net, johannes@...solutions.net,
ryazanov.s.a@...il.com, loic.poulain@...aro.org,
krishna.c.sudi@...el.com, m.chetan.kumar@...ux.intel.com,
linuxwwan@...el.com, Haijun Liu <haijun.liu@...iatek.com>,
Madhusmita Sahu <madhusmita.sahu@...el.com>,
Ricardo Martinez <ricardo.martinez@...ux.intel.com>,
Devegowda Chandrashekar <chandrashekar.devegowda@...el.com>
Subject: Re: [PATCH net-next 3/5] net: wwan: t7xx: PCIe reset rescan
On Tue, 16 Aug 2022, m.chetan.kumar@...el.com wrote:
> From: Haijun Liu <haijun.liu@...iatek.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@...iatek.com>
> Co-developed-by: Madhusmita Sahu <madhusmita.sahu@...el.com>
> Signed-off-by: Madhusmita Sahu <madhusmita.sahu@...el.com>
> Signed-off-by: Ricardo Martinez <ricardo.martinez@...ux.intel.com>
> Signed-off-by: M Chetan Kumar <m.chetan.kumar@...ux.intel.com>
> Signed-off-by: Devegowda Chandrashekar <chandrashekar.devegowda@...el.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.
Powered by blists - more mailing lists