* Re: [PATCH] soc: fsl: qe: convert QE interrupt controller to platform_device
From: Dan Carpenter @ 2021-07-10 14:55 UTC (permalink / raw)
To: kbuild, Maxim Kochetkov, linuxppc-dev
Cc: kbuild-all, lkp, saravanak, gregkh, linux-kernel, leoyang.li,
Maxim Kochetkov, linux-arm-kernel, qiang.zhao
In-Reply-To: <20210705111250.1513634-1-fido_max@inbox.ru>
Hi Maxim,
url: https://github.com/0day-ci/linux/commits/Maxim-Kochetkov/soc-fsl-qe-convert-QE-interrupt-controller-to-platform_device/20210705-191227
base: https://git.kernel.org/pub/scm/linux/kernel/git/soc/soc.git for-next
config: openrisc-randconfig-m031-20210709 (attached as .config)
compiler: or1k-linux-gcc (GCC) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
smatch warnings:
drivers/soc/fsl/qe/qe_ic.c:461 qe_ic_init() warn: 'qe_ic->regs' not released on lines: 442.
vim +461 drivers/soc/fsl/qe/qe_ic.c
43f09464f68dbb drivers/soc/fsl/qe/qe_ic.c Maxim Kochetkov 2021-07-05 408 static int qe_ic_init(struct platform_device *pdev)
9865853851313e arch/powerpc/sysdev/qe_lib/qe_ic.c Li Yang 2006-10-03 409 {
4e0e161d3cc403 drivers/soc/fsl/qe/qe_ic.c Rasmus Villemoes 2019-11-28 410 void (*low_handler)(struct irq_desc *desc);
4e0e161d3cc403 drivers/soc/fsl/qe/qe_ic.c Rasmus Villemoes 2019-11-28 411 void (*high_handler)(struct irq_desc *desc);
9865853851313e arch/powerpc/sysdev/qe_lib/qe_ic.c Li Yang 2006-10-03 412 struct qe_ic *qe_ic;
9865853851313e arch/powerpc/sysdev/qe_lib/qe_ic.c Li Yang 2006-10-03 413 struct resource res;
43f09464f68dbb drivers/soc/fsl/qe/qe_ic.c Maxim Kochetkov 2021-07-05 414 struct device_node *node = pdev->dev.of_node;
882c626d1d4650 drivers/soc/fsl/qe/qe_ic.c Rasmus Villemoes 2019-11-28 415 u32 ret;
9865853851313e arch/powerpc/sysdev/qe_lib/qe_ic.c Li Yang 2006-10-03 416
2272a55f16c998 arch/powerpc/sysdev/qe_lib/qe_ic.c Michael Ellerman 2008-05-26 417 ret = of_address_to_resource(node, 0, &res);
2272a55f16c998 arch/powerpc/sysdev/qe_lib/qe_ic.c Michael Ellerman 2008-05-26 418 if (ret)
43f09464f68dbb drivers/soc/fsl/qe/qe_ic.c Maxim Kochetkov 2021-07-05 419 return -ENODEV;
2272a55f16c998 arch/powerpc/sysdev/qe_lib/qe_ic.c Michael Ellerman 2008-05-26 420
ea96025a26ab89 arch/powerpc/sysdev/qe_lib/qe_ic.c Anton Vorontsov 2009-07-01 421 qe_ic = kzalloc(sizeof(*qe_ic), GFP_KERNEL);
9865853851313e arch/powerpc/sysdev/qe_lib/qe_ic.c Li Yang 2006-10-03 422 if (qe_ic == NULL)
43f09464f68dbb drivers/soc/fsl/qe/qe_ic.c Maxim Kochetkov 2021-07-05 423 return -ENOMEM;
9865853851313e arch/powerpc/sysdev/qe_lib/qe_ic.c Li Yang 2006-10-03 424
a8db8cf0d894df arch/powerpc/sysdev/qe_lib/qe_ic.c Grant Likely 2012-02-14 425 qe_ic->irqhost = irq_domain_add_linear(node, NR_QE_IC_INTS,
a8db8cf0d894df arch/powerpc/sysdev/qe_lib/qe_ic.c Grant Likely 2012-02-14 426 &qe_ic_host_ops, qe_ic);
3475dd8a68a7c7 arch/powerpc/sysdev/qe_lib/qe_ic.c Julia Lawall 2009-08-01 427 if (qe_ic->irqhost == NULL) {
^^^^^^^^^^^^^^
Does this need to be cleaned up?
3475dd8a68a7c7 arch/powerpc/sysdev/qe_lib/qe_ic.c Julia Lawall 2009-08-01 428 kfree(qe_ic);
43f09464f68dbb drivers/soc/fsl/qe/qe_ic.c Maxim Kochetkov 2021-07-05 429 return -ENODEV;
3475dd8a68a7c7 arch/powerpc/sysdev/qe_lib/qe_ic.c Julia Lawall 2009-08-01 430 }
9865853851313e arch/powerpc/sysdev/qe_lib/qe_ic.c Li Yang 2006-10-03 431
28f65c11f2ffb3 arch/powerpc/sysdev/qe_lib/qe_ic.c Joe Perches 2011-06-09 432 qe_ic->regs = ioremap(res.start, resource_size(&res));
^^^^^^^^^^^^^^^^^^^^^
9865853851313e arch/powerpc/sysdev/qe_lib/qe_ic.c Li Yang 2006-10-03 433
9865853851313e arch/powerpc/sysdev/qe_lib/qe_ic.c Li Yang 2006-10-03 434 qe_ic->hc_irq = qe_ic_irq_chip;
9865853851313e arch/powerpc/sysdev/qe_lib/qe_ic.c Li Yang 2006-10-03 435
9865853851313e arch/powerpc/sysdev/qe_lib/qe_ic.c Li Yang 2006-10-03 436 qe_ic->virq_high = irq_of_parse_and_map(node, 0);
9865853851313e arch/powerpc/sysdev/qe_lib/qe_ic.c Li Yang 2006-10-03 437 qe_ic->virq_low = irq_of_parse_and_map(node, 1);
9865853851313e arch/powerpc/sysdev/qe_lib/qe_ic.c Li Yang 2006-10-03 438
10d7930dbb51a8 drivers/soc/fsl/qe/qe_ic.c Rasmus Villemoes 2019-11-28 439 if (!qe_ic->virq_low) {
9865853851313e arch/powerpc/sysdev/qe_lib/qe_ic.c Li Yang 2006-10-03 440 printk(KERN_ERR "Failed to map QE_IC low IRQ\n");
3475dd8a68a7c7 arch/powerpc/sysdev/qe_lib/qe_ic.c Julia Lawall 2009-08-01 441 kfree(qe_ic);
43f09464f68dbb drivers/soc/fsl/qe/qe_ic.c Maxim Kochetkov 2021-07-05 442 return -ENODEV;
Call iounmap() before returning?
9865853851313e arch/powerpc/sysdev/qe_lib/qe_ic.c Li Yang 2006-10-03 443 }
4e0e161d3cc403 drivers/soc/fsl/qe/qe_ic.c Rasmus Villemoes 2019-11-28 444 if (qe_ic->virq_high != qe_ic->virq_low) {
523eef1d206a67 drivers/soc/fsl/qe/qe_ic.c Rasmus Villemoes 2019-11-28 445 low_handler = qe_ic_cascade_low;
523eef1d206a67 drivers/soc/fsl/qe/qe_ic.c Rasmus Villemoes 2019-11-28 446 high_handler = qe_ic_cascade_high;
4e0e161d3cc403 drivers/soc/fsl/qe/qe_ic.c Rasmus Villemoes 2019-11-28 447 } else {
4e0e161d3cc403 drivers/soc/fsl/qe/qe_ic.c Rasmus Villemoes 2019-11-28 448 low_handler = qe_ic_cascade_muxed_mpic;
4e0e161d3cc403 drivers/soc/fsl/qe/qe_ic.c Rasmus Villemoes 2019-11-28 449 high_handler = NULL;
4e0e161d3cc403 drivers/soc/fsl/qe/qe_ic.c Rasmus Villemoes 2019-11-28 450 }
9865853851313e arch/powerpc/sysdev/qe_lib/qe_ic.c Li Yang 2006-10-03 451
882c626d1d4650 drivers/soc/fsl/qe/qe_ic.c Rasmus Villemoes 2019-11-28 452 qe_ic_write(qe_ic->regs, QEIC_CICR, 0);
9865853851313e arch/powerpc/sysdev/qe_lib/qe_ic.c Li Yang 2006-10-03 453
ec775d0e70eb6b arch/powerpc/sysdev/qe_lib/qe_ic.c Thomas Gleixner 2011-03-25 454 irq_set_handler_data(qe_ic->virq_low, qe_ic);
ec775d0e70eb6b arch/powerpc/sysdev/qe_lib/qe_ic.c Thomas Gleixner 2011-03-25 455 irq_set_chained_handler(qe_ic->virq_low, low_handler);
9865853851313e arch/powerpc/sysdev/qe_lib/qe_ic.c Li Yang 2006-10-03 456
10d7930dbb51a8 drivers/soc/fsl/qe/qe_ic.c Rasmus Villemoes 2019-11-28 457 if (qe_ic->virq_high && qe_ic->virq_high != qe_ic->virq_low) {
ec775d0e70eb6b arch/powerpc/sysdev/qe_lib/qe_ic.c Thomas Gleixner 2011-03-25 458 irq_set_handler_data(qe_ic->virq_high, qe_ic);
ec775d0e70eb6b arch/powerpc/sysdev/qe_lib/qe_ic.c Thomas Gleixner 2011-03-25 459 irq_set_chained_handler(qe_ic->virq_high, high_handler);
9865853851313e arch/powerpc/sysdev/qe_lib/qe_ic.c Li Yang 2006-10-03 460 }
43f09464f68dbb drivers/soc/fsl/qe/qe_ic.c Maxim Kochetkov 2021-07-05 @461 return 0;
9865853851313e arch/powerpc/sysdev/qe_lib/qe_ic.c Li Yang 2006-10-03 462 }
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
^ permalink raw reply
* [PATCH AUTOSEL 5.12 13/43] PCI: pciehp: Ignore Link Down/Up caused by DPC
From: Sasha Levin @ 2021-07-10 23:48 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Sasha Levin, Kuppuswamy Sathyanarayanan, Ashok Raj, linux-pci,
Sinan Kaya, Yicong Yang, Keith Busch, Lukas Wunner, Ethan Zhao,
Bjorn Helgaas, Dan Williams, linuxppc-dev
In-Reply-To: <20210710234915.3220342-1-sashal@kernel.org>
From: Lukas Wunner <lukas@wunner.de>
[ Upstream commit a97396c6eb13f65bea894dbe7739b2e883d40a3e ]
Downstream Port Containment (PCIe r5.0, sec. 6.2.10) disables the link upon
an error and attempts to re-enable it when instructed by the DPC driver.
A slot which is both DPC- and hotplug-capable is currently powered off by
pciehp once DPC is triggered (due to the link change) and powered back up
on successful recovery. That's undesirable, the slot should remain powered
so the hotplugged device remains bound to its driver. DPC notifies the
driver of the error and of successful recovery in pcie_do_recovery() and
the driver may then restore the device to working state.
Moreover, Sinan points out that turning off slot power by pciehp may foil
recovery by DPC: Power off/on is a cold reset concurrently to DPC's warm
reset. Sathyanarayanan reports extended delays or failure in link
retraining by DPC if pciehp brings down the slot.
Fix by detecting whether a Link Down event is caused by DPC and awaiting
recovery if so. On successful recovery, ignore both the Link Down and the
subsequent Link Up event.
Afterwards, check whether the link is down to detect surprise-removal or
another DPC event immediately after DPC recovery. Ensure that the
corresponding DLLSC event is not ignored by synthesizing it and invoking
irq_wake_thread() to trigger a re-run of pciehp_ist().
The IRQ threads of the hotplug and DPC drivers, pciehp_ist() and
dpc_handler(), race against each other. If pciehp is faster than DPC, it
will wait until DPC recovery completes.
Recovery consists of two steps: The first step (waiting for link
disablement) is recognizable by pciehp through a set DPC Trigger Status
bit. The second step (waiting for link retraining) is recognizable through
a newly introduced PCI_DPC_RECOVERING flag.
If DPC is faster than pciehp, neither of the two flags will be set and
pciehp may glean the recovery status from the new PCI_DPC_RECOVERED flag.
The flag is zero if DPC didn't occur at all, hence DLLSC events are not
ignored by default.
pciehp waits up to 4 seconds before assuming that DPC recovery failed and
bringing down the slot. This timeout is not taken from the spec (it
doesn't mandate one) but based on a report from Yicong Yang that DPC may
take a bit more than 3 seconds on HiSilicon's Kunpeng platform.
The timeout is necessary because the DPC Trigger Status bit may never
clear: On Root Ports which support RP Extensions for DPC, the DPC driver
polls the DPC RP Busy bit for up to 1 second before giving up on DPC
recovery. Without the timeout, pciehp would then wait indefinitely for DPC
to complete.
This commit draws inspiration from previous attempts to synchronize DPC
with pciehp:
By Sinan Kaya, August 2018:
https://lore.kernel.org/linux-pci/20180818065126.77912-1-okaya@kernel.org/
By Ethan Zhao, October 2020:
https://lore.kernel.org/linux-pci/20201007113158.48933-1-haifeng.zhao@intel.com/
By Kuppuswamy Sathyanarayanan, March 2021:
https://lore.kernel.org/linux-pci/59cb30f5e5ac6d65427ceaadf1012b2ba8dbf66c.1615606143.git.sathyanarayanan.kuppuswamy@linux.intel.com/
Link: https://lore.kernel.org/r/0be565d97438fe2a6d57354b3aa4e8626952a00b.1619857124.git.lukas@wunner.de
Reported-by: Sinan Kaya <okaya@kernel.org>
Reported-by: Ethan Zhao <haifeng.zhao@intel.com>
Reported-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Tested-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Tested-by: Yicong Yang <yangyicong@hisilicon.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Ashok Raj <ashok.raj@intel.com>
Cc: Keith Busch <kbusch@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/pci/hotplug/pciehp_hpc.c | 36 ++++++++++++++++
drivers/pci/pci.h | 4 ++
drivers/pci/pcie/dpc.c | 74 +++++++++++++++++++++++++++++---
3 files changed, 109 insertions(+), 5 deletions(-)
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index fb3840e222ad..9d06939736c0 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -563,6 +563,32 @@ void pciehp_power_off_slot(struct controller *ctrl)
PCI_EXP_SLTCTL_PWR_OFF);
}
+static void pciehp_ignore_dpc_link_change(struct controller *ctrl,
+ struct pci_dev *pdev, int irq)
+{
+ /*
+ * Ignore link changes which occurred while waiting for DPC recovery.
+ * Could be several if DPC triggered multiple times consecutively.
+ */
+ synchronize_hardirq(irq);
+ atomic_and(~PCI_EXP_SLTSTA_DLLSC, &ctrl->pending_events);
+ if (pciehp_poll_mode)
+ pcie_capability_write_word(pdev, PCI_EXP_SLTSTA,
+ PCI_EXP_SLTSTA_DLLSC);
+ ctrl_info(ctrl, "Slot(%s): Link Down/Up ignored (recovered by DPC)\n",
+ slot_name(ctrl));
+
+ /*
+ * If the link is unexpectedly down after successful recovery,
+ * the corresponding link change may have been ignored above.
+ * Synthesize it to ensure that it is acted on.
+ */
+ down_read(&ctrl->reset_lock);
+ if (!pciehp_check_link_active(ctrl))
+ pciehp_request(ctrl, PCI_EXP_SLTSTA_DLLSC);
+ up_read(&ctrl->reset_lock);
+}
+
static irqreturn_t pciehp_isr(int irq, void *dev_id)
{
struct controller *ctrl = (struct controller *)dev_id;
@@ -706,6 +732,16 @@ static irqreturn_t pciehp_ist(int irq, void *dev_id)
PCI_EXP_SLTCTL_ATTN_IND_ON);
}
+ /*
+ * Ignore Link Down/Up events caused by Downstream Port Containment
+ * if recovery from the error succeeded.
+ */
+ if ((events & PCI_EXP_SLTSTA_DLLSC) && pci_dpc_recovered(pdev) &&
+ ctrl->state == ON_STATE) {
+ events &= ~PCI_EXP_SLTSTA_DLLSC;
+ pciehp_ignore_dpc_link_change(ctrl, pdev, irq);
+ }
+
/*
* Disable requests have higher priority than Presence Detect Changed
* or Data Link Layer State Changed events.
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 9684b468267f..e5ae5e860431 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -392,6 +392,8 @@ static inline bool pci_dev_is_disconnected(const struct pci_dev *dev)
/* pci_dev priv_flags */
#define PCI_DEV_ADDED 0
+#define PCI_DPC_RECOVERED 1
+#define PCI_DPC_RECOVERING 2
static inline void pci_dev_assign_added(struct pci_dev *dev, bool added)
{
@@ -446,10 +448,12 @@ void pci_restore_dpc_state(struct pci_dev *dev);
void pci_dpc_init(struct pci_dev *pdev);
void dpc_process_error(struct pci_dev *pdev);
pci_ers_result_t dpc_reset_link(struct pci_dev *pdev);
+bool pci_dpc_recovered(struct pci_dev *pdev);
#else
static inline void pci_save_dpc_state(struct pci_dev *dev) {}
static inline void pci_restore_dpc_state(struct pci_dev *dev) {}
static inline void pci_dpc_init(struct pci_dev *pdev) {}
+static inline bool pci_dpc_recovered(struct pci_dev *pdev) { return false; }
#endif
#ifdef CONFIG_PCIEPORTBUS
diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index e05aba86a317..c556e7beafe3 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -71,6 +71,58 @@ void pci_restore_dpc_state(struct pci_dev *dev)
pci_write_config_word(dev, dev->dpc_cap + PCI_EXP_DPC_CTL, *cap);
}
+static DECLARE_WAIT_QUEUE_HEAD(dpc_completed_waitqueue);
+
+#ifdef CONFIG_HOTPLUG_PCI_PCIE
+static bool dpc_completed(struct pci_dev *pdev)
+{
+ u16 status;
+
+ pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_STATUS, &status);
+ if ((status != 0xffff) && (status & PCI_EXP_DPC_STATUS_TRIGGER))
+ return false;
+
+ if (test_bit(PCI_DPC_RECOVERING, &pdev->priv_flags))
+ return false;
+
+ return true;
+}
+
+/**
+ * pci_dpc_recovered - whether DPC triggered and has recovered successfully
+ * @pdev: PCI device
+ *
+ * Return true if DPC was triggered for @pdev and has recovered successfully.
+ * Wait for recovery if it hasn't completed yet. Called from the PCIe hotplug
+ * driver to recognize and ignore Link Down/Up events caused by DPC.
+ */
+bool pci_dpc_recovered(struct pci_dev *pdev)
+{
+ struct pci_host_bridge *host;
+
+ if (!pdev->dpc_cap)
+ return false;
+
+ /*
+ * Synchronization between hotplug and DPC is not supported
+ * if DPC is owned by firmware and EDR is not enabled.
+ */
+ host = pci_find_host_bridge(pdev->bus);
+ if (!host->native_dpc && !IS_ENABLED(CONFIG_PCIE_EDR))
+ return false;
+
+ /*
+ * Need a timeout in case DPC never completes due to failure of
+ * dpc_wait_rp_inactive(). The spec doesn't mandate a time limit,
+ * but reports indicate that DPC completes within 4 seconds.
+ */
+ wait_event_timeout(dpc_completed_waitqueue, dpc_completed(pdev),
+ msecs_to_jiffies(4000));
+
+ return test_and_clear_bit(PCI_DPC_RECOVERED, &pdev->priv_flags);
+}
+#endif /* CONFIG_HOTPLUG_PCI_PCIE */
+
static int dpc_wait_rp_inactive(struct pci_dev *pdev)
{
unsigned long timeout = jiffies + HZ;
@@ -91,8 +143,11 @@ static int dpc_wait_rp_inactive(struct pci_dev *pdev)
pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
{
+ pci_ers_result_t ret;
u16 cap;
+ set_bit(PCI_DPC_RECOVERING, &pdev->priv_flags);
+
/*
* DPC disables the Link automatically in hardware, so it has
* already been reset by the time we get here.
@@ -106,18 +161,27 @@ pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
if (!pcie_wait_for_link(pdev, false))
pci_info(pdev, "Data Link Layer Link Active not cleared in 1000 msec\n");
- if (pdev->dpc_rp_extensions && dpc_wait_rp_inactive(pdev))
- return PCI_ERS_RESULT_DISCONNECT;
+ if (pdev->dpc_rp_extensions && dpc_wait_rp_inactive(pdev)) {
+ clear_bit(PCI_DPC_RECOVERED, &pdev->priv_flags);
+ ret = PCI_ERS_RESULT_DISCONNECT;
+ goto out;
+ }
pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS,
PCI_EXP_DPC_STATUS_TRIGGER);
if (!pcie_wait_for_link(pdev, true)) {
pci_info(pdev, "Data Link Layer Link Active not set in 1000 msec\n");
- return PCI_ERS_RESULT_DISCONNECT;
+ clear_bit(PCI_DPC_RECOVERED, &pdev->priv_flags);
+ ret = PCI_ERS_RESULT_DISCONNECT;
+ } else {
+ set_bit(PCI_DPC_RECOVERED, &pdev->priv_flags);
+ ret = PCI_ERS_RESULT_RECOVERED;
}
-
- return PCI_ERS_RESULT_RECOVERED;
+out:
+ clear_bit(PCI_DPC_RECOVERING, &pdev->priv_flags);
+ wake_up_all(&dpc_completed_waitqueue);
+ return ret;
}
static void dpc_process_rp_pio_error(struct pci_dev *pdev)
--
2.30.2
^ permalink raw reply related
* [PATCH AUTOSEL 5.10 13/37] PCI: pciehp: Ignore Link Down/Up caused by DPC
From: Sasha Levin @ 2021-07-10 23:49 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Sasha Levin, Kuppuswamy Sathyanarayanan, Ashok Raj, linux-pci,
Sinan Kaya, Yicong Yang, Keith Busch, Lukas Wunner, Ethan Zhao,
Bjorn Helgaas, Dan Williams, linuxppc-dev
In-Reply-To: <20210710235016.3221124-1-sashal@kernel.org>
From: Lukas Wunner <lukas@wunner.de>
[ Upstream commit a97396c6eb13f65bea894dbe7739b2e883d40a3e ]
Downstream Port Containment (PCIe r5.0, sec. 6.2.10) disables the link upon
an error and attempts to re-enable it when instructed by the DPC driver.
A slot which is both DPC- and hotplug-capable is currently powered off by
pciehp once DPC is triggered (due to the link change) and powered back up
on successful recovery. That's undesirable, the slot should remain powered
so the hotplugged device remains bound to its driver. DPC notifies the
driver of the error and of successful recovery in pcie_do_recovery() and
the driver may then restore the device to working state.
Moreover, Sinan points out that turning off slot power by pciehp may foil
recovery by DPC: Power off/on is a cold reset concurrently to DPC's warm
reset. Sathyanarayanan reports extended delays or failure in link
retraining by DPC if pciehp brings down the slot.
Fix by detecting whether a Link Down event is caused by DPC and awaiting
recovery if so. On successful recovery, ignore both the Link Down and the
subsequent Link Up event.
Afterwards, check whether the link is down to detect surprise-removal or
another DPC event immediately after DPC recovery. Ensure that the
corresponding DLLSC event is not ignored by synthesizing it and invoking
irq_wake_thread() to trigger a re-run of pciehp_ist().
The IRQ threads of the hotplug and DPC drivers, pciehp_ist() and
dpc_handler(), race against each other. If pciehp is faster than DPC, it
will wait until DPC recovery completes.
Recovery consists of two steps: The first step (waiting for link
disablement) is recognizable by pciehp through a set DPC Trigger Status
bit. The second step (waiting for link retraining) is recognizable through
a newly introduced PCI_DPC_RECOVERING flag.
If DPC is faster than pciehp, neither of the two flags will be set and
pciehp may glean the recovery status from the new PCI_DPC_RECOVERED flag.
The flag is zero if DPC didn't occur at all, hence DLLSC events are not
ignored by default.
pciehp waits up to 4 seconds before assuming that DPC recovery failed and
bringing down the slot. This timeout is not taken from the spec (it
doesn't mandate one) but based on a report from Yicong Yang that DPC may
take a bit more than 3 seconds on HiSilicon's Kunpeng platform.
The timeout is necessary because the DPC Trigger Status bit may never
clear: On Root Ports which support RP Extensions for DPC, the DPC driver
polls the DPC RP Busy bit for up to 1 second before giving up on DPC
recovery. Without the timeout, pciehp would then wait indefinitely for DPC
to complete.
This commit draws inspiration from previous attempts to synchronize DPC
with pciehp:
By Sinan Kaya, August 2018:
https://lore.kernel.org/linux-pci/20180818065126.77912-1-okaya@kernel.org/
By Ethan Zhao, October 2020:
https://lore.kernel.org/linux-pci/20201007113158.48933-1-haifeng.zhao@intel.com/
By Kuppuswamy Sathyanarayanan, March 2021:
https://lore.kernel.org/linux-pci/59cb30f5e5ac6d65427ceaadf1012b2ba8dbf66c.1615606143.git.sathyanarayanan.kuppuswamy@linux.intel.com/
Link: https://lore.kernel.org/r/0be565d97438fe2a6d57354b3aa4e8626952a00b.1619857124.git.lukas@wunner.de
Reported-by: Sinan Kaya <okaya@kernel.org>
Reported-by: Ethan Zhao <haifeng.zhao@intel.com>
Reported-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Tested-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Tested-by: Yicong Yang <yangyicong@hisilicon.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Ashok Raj <ashok.raj@intel.com>
Cc: Keith Busch <kbusch@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/pci/hotplug/pciehp_hpc.c | 36 ++++++++++++++++
drivers/pci/pci.h | 4 ++
drivers/pci/pcie/dpc.c | 74 +++++++++++++++++++++++++++++---
3 files changed, 109 insertions(+), 5 deletions(-)
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index fb3840e222ad..9d06939736c0 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -563,6 +563,32 @@ void pciehp_power_off_slot(struct controller *ctrl)
PCI_EXP_SLTCTL_PWR_OFF);
}
+static void pciehp_ignore_dpc_link_change(struct controller *ctrl,
+ struct pci_dev *pdev, int irq)
+{
+ /*
+ * Ignore link changes which occurred while waiting for DPC recovery.
+ * Could be several if DPC triggered multiple times consecutively.
+ */
+ synchronize_hardirq(irq);
+ atomic_and(~PCI_EXP_SLTSTA_DLLSC, &ctrl->pending_events);
+ if (pciehp_poll_mode)
+ pcie_capability_write_word(pdev, PCI_EXP_SLTSTA,
+ PCI_EXP_SLTSTA_DLLSC);
+ ctrl_info(ctrl, "Slot(%s): Link Down/Up ignored (recovered by DPC)\n",
+ slot_name(ctrl));
+
+ /*
+ * If the link is unexpectedly down after successful recovery,
+ * the corresponding link change may have been ignored above.
+ * Synthesize it to ensure that it is acted on.
+ */
+ down_read(&ctrl->reset_lock);
+ if (!pciehp_check_link_active(ctrl))
+ pciehp_request(ctrl, PCI_EXP_SLTSTA_DLLSC);
+ up_read(&ctrl->reset_lock);
+}
+
static irqreturn_t pciehp_isr(int irq, void *dev_id)
{
struct controller *ctrl = (struct controller *)dev_id;
@@ -706,6 +732,16 @@ static irqreturn_t pciehp_ist(int irq, void *dev_id)
PCI_EXP_SLTCTL_ATTN_IND_ON);
}
+ /*
+ * Ignore Link Down/Up events caused by Downstream Port Containment
+ * if recovery from the error succeeded.
+ */
+ if ((events & PCI_EXP_SLTSTA_DLLSC) && pci_dpc_recovered(pdev) &&
+ ctrl->state == ON_STATE) {
+ events &= ~PCI_EXP_SLTSTA_DLLSC;
+ pciehp_ignore_dpc_link_change(ctrl, pdev, irq);
+ }
+
/*
* Disable requests have higher priority than Presence Detect Changed
* or Data Link Layer State Changed events.
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 09ebc134d0d7..a96dc6f53076 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -409,6 +409,8 @@ static inline bool pci_dev_is_disconnected(const struct pci_dev *dev)
/* pci_dev priv_flags */
#define PCI_DEV_ADDED 0
+#define PCI_DPC_RECOVERED 1
+#define PCI_DPC_RECOVERING 2
static inline void pci_dev_assign_added(struct pci_dev *dev, bool added)
{
@@ -454,10 +456,12 @@ void pci_restore_dpc_state(struct pci_dev *dev);
void pci_dpc_init(struct pci_dev *pdev);
void dpc_process_error(struct pci_dev *pdev);
pci_ers_result_t dpc_reset_link(struct pci_dev *pdev);
+bool pci_dpc_recovered(struct pci_dev *pdev);
#else
static inline void pci_save_dpc_state(struct pci_dev *dev) {}
static inline void pci_restore_dpc_state(struct pci_dev *dev) {}
static inline void pci_dpc_init(struct pci_dev *pdev) {}
+static inline bool pci_dpc_recovered(struct pci_dev *pdev) { return false; }
#endif
#ifdef CONFIG_PCI_ATS
diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index e05aba86a317..c556e7beafe3 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -71,6 +71,58 @@ void pci_restore_dpc_state(struct pci_dev *dev)
pci_write_config_word(dev, dev->dpc_cap + PCI_EXP_DPC_CTL, *cap);
}
+static DECLARE_WAIT_QUEUE_HEAD(dpc_completed_waitqueue);
+
+#ifdef CONFIG_HOTPLUG_PCI_PCIE
+static bool dpc_completed(struct pci_dev *pdev)
+{
+ u16 status;
+
+ pci_read_config_word(pdev, pdev->dpc_cap + PCI_EXP_DPC_STATUS, &status);
+ if ((status != 0xffff) && (status & PCI_EXP_DPC_STATUS_TRIGGER))
+ return false;
+
+ if (test_bit(PCI_DPC_RECOVERING, &pdev->priv_flags))
+ return false;
+
+ return true;
+}
+
+/**
+ * pci_dpc_recovered - whether DPC triggered and has recovered successfully
+ * @pdev: PCI device
+ *
+ * Return true if DPC was triggered for @pdev and has recovered successfully.
+ * Wait for recovery if it hasn't completed yet. Called from the PCIe hotplug
+ * driver to recognize and ignore Link Down/Up events caused by DPC.
+ */
+bool pci_dpc_recovered(struct pci_dev *pdev)
+{
+ struct pci_host_bridge *host;
+
+ if (!pdev->dpc_cap)
+ return false;
+
+ /*
+ * Synchronization between hotplug and DPC is not supported
+ * if DPC is owned by firmware and EDR is not enabled.
+ */
+ host = pci_find_host_bridge(pdev->bus);
+ if (!host->native_dpc && !IS_ENABLED(CONFIG_PCIE_EDR))
+ return false;
+
+ /*
+ * Need a timeout in case DPC never completes due to failure of
+ * dpc_wait_rp_inactive(). The spec doesn't mandate a time limit,
+ * but reports indicate that DPC completes within 4 seconds.
+ */
+ wait_event_timeout(dpc_completed_waitqueue, dpc_completed(pdev),
+ msecs_to_jiffies(4000));
+
+ return test_and_clear_bit(PCI_DPC_RECOVERED, &pdev->priv_flags);
+}
+#endif /* CONFIG_HOTPLUG_PCI_PCIE */
+
static int dpc_wait_rp_inactive(struct pci_dev *pdev)
{
unsigned long timeout = jiffies + HZ;
@@ -91,8 +143,11 @@ static int dpc_wait_rp_inactive(struct pci_dev *pdev)
pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
{
+ pci_ers_result_t ret;
u16 cap;
+ set_bit(PCI_DPC_RECOVERING, &pdev->priv_flags);
+
/*
* DPC disables the Link automatically in hardware, so it has
* already been reset by the time we get here.
@@ -106,18 +161,27 @@ pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
if (!pcie_wait_for_link(pdev, false))
pci_info(pdev, "Data Link Layer Link Active not cleared in 1000 msec\n");
- if (pdev->dpc_rp_extensions && dpc_wait_rp_inactive(pdev))
- return PCI_ERS_RESULT_DISCONNECT;
+ if (pdev->dpc_rp_extensions && dpc_wait_rp_inactive(pdev)) {
+ clear_bit(PCI_DPC_RECOVERED, &pdev->priv_flags);
+ ret = PCI_ERS_RESULT_DISCONNECT;
+ goto out;
+ }
pci_write_config_word(pdev, cap + PCI_EXP_DPC_STATUS,
PCI_EXP_DPC_STATUS_TRIGGER);
if (!pcie_wait_for_link(pdev, true)) {
pci_info(pdev, "Data Link Layer Link Active not set in 1000 msec\n");
- return PCI_ERS_RESULT_DISCONNECT;
+ clear_bit(PCI_DPC_RECOVERED, &pdev->priv_flags);
+ ret = PCI_ERS_RESULT_DISCONNECT;
+ } else {
+ set_bit(PCI_DPC_RECOVERED, &pdev->priv_flags);
+ ret = PCI_ERS_RESULT_RECOVERED;
}
-
- return PCI_ERS_RESULT_RECOVERED;
+out:
+ clear_bit(PCI_DPC_RECOVERING, &pdev->priv_flags);
+ wake_up_all(&dpc_completed_waitqueue);
+ return ret;
}
static void dpc_process_rp_pio_error(struct pci_dev *pdev)
--
2.30.2
^ permalink raw reply related
* [PATCH v3 0/2] DMA fixes for PS3 gelic network driver
From: Geoff Levand @ 2021-07-11 4:15 UTC (permalink / raw)
To: David S. Miller, Jakub Kicinski; +Cc: netdev, linuxppc-dev
Hi Dave, Jakub,
This set of patches fixes various DMA related problems in the PS3 gelic
network driver and adds better error checking and improved message logging.
Please consider.
Changes from v2:
Rebase to latest net-next.
Changes from v1:
Split the v1 series into two, one series with powerpc changes, and one series
with gelic network driver changes.
-Geoff
The following changes since commit 5e437416ff66981d8154687cfdf7de50b1d82bfc:
Merge branch 'dsa-mv88e6xxx-topaz-fixes' (2021-07-01 11:51:36 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/geoff/ps3-linux.git for-merge-dma-net
for you to fetch changes up to ffb7b2f4ac085986f563131e3851e07393cd514f:
net/ps3_gelic: Cleanups, improve logging (2021-07-10 20:42:42 -0700)
----------------------------------------------------------------
Geoff Levand (2):
net/ps3_gelic: Add gelic_descr structures
net/ps3_gelic: Cleanups, improve logging
drivers/net/ethernet/toshiba/ps3_gelic_net.c | 968 +++++++++++++++------------
drivers/net/ethernet/toshiba/ps3_gelic_net.h | 24 +-
2 files changed, 557 insertions(+), 435 deletions(-)
--
2.25.1
^ permalink raw reply
* [PATCH v3 2/2] net/ps3_gelic: Cleanups, improve logging
From: Geoff Levand @ 2021-07-11 4:15 UTC (permalink / raw)
To: David S. Miller, Jakub Kicinski; +Cc: netdev, linuxppc-dev
In-Reply-To: <cover.1625976141.git.geoff@infradead.org>
General source cleanups and improved logging messages.
Signed-off-by: Geoff Levand <geoff@infradead.org>
---
drivers/net/ethernet/toshiba/ps3_gelic_net.c | 395 ++++++++++---------
1 file changed, 216 insertions(+), 179 deletions(-)
diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
index e01938128882..9dbcb7c4ec80 100644
--- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
+++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
@@ -44,17 +44,17 @@ MODULE_AUTHOR("SCE Inc.");
MODULE_DESCRIPTION("Gelic Network driver");
MODULE_LICENSE("GPL");
-
-/* set irq_mask */
int gelic_card_set_irq_mask(struct gelic_card *card, u64 mask)
{
+ struct device *dev = ctodev(card);
int status;
status = lv1_net_set_interrupt_mask(bus_id(card), dev_id(card),
mask, 0);
- if (status)
- dev_info(ctodev(card),
- "%s failed %d\n", __func__, status);
+ if (status) {
+ dev_err(dev, "%s:%d failed: %d\n", __func__, __LINE__, status);
+ }
+
return status;
}
@@ -63,6 +63,7 @@ static void gelic_card_rx_irq_on(struct gelic_card *card)
card->irq_mask |= GELIC_CARD_RXINT;
gelic_card_set_irq_mask(card, card->irq_mask);
}
+
static void gelic_card_rx_irq_off(struct gelic_card *card)
{
card->irq_mask &= ~GELIC_CARD_RXINT;
@@ -70,15 +71,14 @@ static void gelic_card_rx_irq_off(struct gelic_card *card)
}
static void gelic_card_get_ether_port_status(struct gelic_card *card,
- int inform)
+ int inform)
{
u64 v2;
struct net_device *ether_netdev;
lv1_net_control(bus_id(card), dev_id(card),
- GELIC_LV1_GET_ETH_PORT_STATUS,
- GELIC_LV1_VLAN_TX_ETHERNET_0, 0, 0,
- &card->ether_port_status, &v2);
+ GELIC_LV1_GET_ETH_PORT_STATUS, GELIC_LV1_VLAN_TX_ETHERNET_0, 0,
+ 0, &card->ether_port_status, &v2);
if (inform) {
ether_netdev = card->netdev[GELIC_PORT_ETHERNET_0];
@@ -105,15 +105,17 @@ gelic_descr_get_status(struct gelic_descr *descr)
static int gelic_card_set_link_mode(struct gelic_card *card, int mode)
{
+ struct device *dev = ctodev(card);
int status;
u64 v1, v2;
status = lv1_net_control(bus_id(card), dev_id(card),
- GELIC_LV1_SET_NEGOTIATION_MODE,
- GELIC_LV1_PHY_ETHERNET_0, mode, 0, &v1, &v2);
+ GELIC_LV1_SET_NEGOTIATION_MODE, GELIC_LV1_PHY_ETHERNET_0, mode,
+ 0, &v1, &v2);
+
if (status) {
- pr_info("%s: failed setting negotiation mode %d\n", __func__,
- status);
+ dev_err(dev, "%s:%d: Failed setting negotiation mode: %d\n",
+ __func__, __LINE__, status);
return -EBUSY;
}
@@ -130,13 +132,16 @@ static int gelic_card_set_link_mode(struct gelic_card *card, int mode)
*/
static void gelic_card_disable_txdmac(struct gelic_card *card)
{
+ struct device *dev = ctodev(card);
int status;
/* this hvc blocks until the DMA in progress really stopped */
status = lv1_net_stop_tx_dma(bus_id(card), dev_id(card));
- if (status)
- dev_err(ctodev(card),
- "lv1_net_stop_tx_dma failed, status=%d\n", status);
+
+ if (status) {
+ dev_err(dev, "%s:%d: lv1_net_stop_tx_dma failed: %d\n",
+ __func__, __LINE__, status);
+ }
}
/**
@@ -187,13 +192,16 @@ static void gelic_card_enable_rxdmac(struct gelic_card *card)
*/
static void gelic_card_disable_rxdmac(struct gelic_card *card)
{
+ struct device *dev = ctodev(card);
int status;
/* this hvc blocks until the DMA in progress really stopped */
status = lv1_net_stop_rx_dma(bus_id(card), dev_id(card));
- if (status)
- dev_err(ctodev(card),
- "lv1_net_stop_rx_dma failed, %d\n", status);
+
+ if (status) {
+ dev_err(dev, "%s:%d: lv1_net_stop_rx_dma failed: %d\n",
+ __func__, __LINE__, status);
+ }
}
/**
@@ -216,6 +224,7 @@ static void gelic_descr_set_status(struct gelic_descr *descr,
* Usually caller of this function wants to inform that to the
* hardware, so we assure here the hardware sees the change.
*/
+
wmb();
}
@@ -229,8 +238,7 @@ static void gelic_descr_set_status(struct gelic_descr *descr,
* and re-initialize the hardware chain for later use
*/
static void gelic_card_reset_chain(struct gelic_card *card,
- struct gelic_descr_chain *chain,
- struct gelic_descr *start_descr)
+ struct gelic_descr_chain *chain, struct gelic_descr *start_descr)
{
struct gelic_descr *descr;
@@ -248,45 +256,41 @@ static void gelic_card_reset_chain(struct gelic_card *card,
void gelic_card_up(struct gelic_card *card)
{
- pr_debug("%s: called\n", __func__);
+ struct device *dev = ctodev(card);
+
mutex_lock(&card->updown_lock);
if (atomic_inc_return(&card->users) == 1) {
- pr_debug("%s: real do\n", __func__);
- /* enable irq */
+ dev_dbg(dev, "%s:%d: Starting...\n", __func__, __LINE__);
gelic_card_set_irq_mask(card, card->irq_mask);
- /* start rx */
gelic_card_enable_rxdmac(card);
-
napi_enable(&card->napi);
}
mutex_unlock(&card->updown_lock);
- pr_debug("%s: done\n", __func__);
}
void gelic_card_down(struct gelic_card *card)
{
+ struct device *dev = ctodev(card);
u64 mask;
- pr_debug("%s: called\n", __func__);
+
mutex_lock(&card->updown_lock);
if (atomic_dec_if_positive(&card->users) == 0) {
- pr_debug("%s: real do\n", __func__);
+ dev_dbg(dev, "%s:%d: Stopping...\n", __func__, __LINE__);
napi_disable(&card->napi);
/*
- * Disable irq. Wireless interrupts will
- * be disabled later if any
+ * Disable irq. Wireless interrupts will be disabled later.
*/
mask = card->irq_mask & (GELIC_CARD_WLAN_EVENT_RECEIVED |
- GELIC_CARD_WLAN_COMMAND_COMPLETED);
+ GELIC_CARD_WLAN_COMMAND_COMPLETED);
gelic_card_set_irq_mask(card, mask);
- /* stop rx */
+
gelic_card_disable_rxdmac(card);
gelic_card_reset_chain(card, &card->rx_chain,
- card->descr + GELIC_NET_TX_DESCRIPTORS);
- /* stop tx */
+ card->descr + GELIC_NET_TX_DESCRIPTORS);
+
gelic_card_disable_txdmac(card);
}
mutex_unlock(&card->updown_lock);
- pr_debug("%s: done\n", __func__);
}
static void gelic_unmap_link(struct device *dev, struct gelic_descr *descr)
@@ -652,6 +656,7 @@ static void gelic_card_release_tx_chain(struct gelic_card *card, int stop)
void gelic_net_set_multi(struct net_device *netdev)
{
struct gelic_card *card = netdev_card(netdev);
+ struct device *dev = ctodev(card);
struct netdev_hw_addr *ha;
unsigned int i;
uint8_t *p;
@@ -661,27 +666,31 @@ void gelic_net_set_multi(struct net_device *netdev)
/* clear all multicast address */
status = lv1_net_remove_multicast_address(bus_id(card), dev_id(card),
0, 1);
- if (status)
- dev_err(ctodev(card),
- "lv1_net_remove_multicast_address failed %d\n",
- status);
+ if (status) {
+ dev_err(dev,
+ "%s:%d: lv1_net_remove_multicast_address failed %d\n",
+ __func__, __LINE__, status);
+ }
+
/* set broadcast address */
status = lv1_net_add_multicast_address(bus_id(card), dev_id(card),
GELIC_NET_BROADCAST_ADDR, 0);
- if (status)
- dev_err(ctodev(card),
- "lv1_net_add_multicast_address failed, %d\n",
- status);
+ if (status) {
+ dev_err(dev,
+ "%s:%d: lv1_net_add_multicast_address failed, %d\n",
+ __func__, __LINE__, status);
+ }
if ((netdev->flags & IFF_ALLMULTI) ||
(netdev_mc_count(netdev) > GELIC_NET_MC_COUNT_MAX)) {
status = lv1_net_add_multicast_address(bus_id(card),
dev_id(card),
0, 1);
- if (status)
- dev_err(ctodev(card),
- "lv1_net_add_multicast_address failed, %d\n",
- status);
+ if (status) {
+ dev_err(dev,
+ "%s:%d: lv1_net_add_multicast_address failed, %d\n",
+ __func__, __LINE__, status);
+ }
return;
}
@@ -694,12 +703,13 @@ void gelic_net_set_multi(struct net_device *netdev)
addr |= *p++;
}
status = lv1_net_add_multicast_address(bus_id(card),
- dev_id(card),
- addr, 0);
- if (status)
- dev_err(ctodev(card),
- "lv1_net_add_multicast_address failed, %d\n",
- status);
+ dev_id(card), addr, 0);
+
+ if (status) {
+ dev_err(dev,
+ "%s:%d: lv1_net_add_multicast_address failed, %d\n",
+ __func__, __LINE__, status);
+ }
}
}
@@ -711,17 +721,17 @@ void gelic_net_set_multi(struct net_device *netdev)
*/
int gelic_net_stop(struct net_device *netdev)
{
- struct gelic_card *card;
+ struct gelic_card *card = netdev_card(netdev);
+ struct device *dev = ctodev(card);
- pr_debug("%s: start\n", __func__);
+ dev_dbg(dev, "%s:%d: >\n", __func__, __LINE__);
netif_stop_queue(netdev);
netif_carrier_off(netdev);
- card = netdev_card(netdev);
gelic_card_down(card);
- pr_debug("%s: done\n", __func__);
+ dev_dbg(dev, "%s:%d: <\n", __func__, __LINE__);
return 0;
}
@@ -736,14 +746,15 @@ gelic_card_get_next_tx_descr(struct gelic_card *card)
{
if (!card->tx_chain.head)
return NULL;
+
/* see if the next descriptor is free */
- if (card->tx_chain.tail != card->tx_chain.head->next &&
- gelic_descr_get_status(card->tx_chain.head) ==
- GELIC_DESCR_DMA_NOT_IN_USE)
+ if ((card->tx_chain.tail != card->tx_chain.head->next)
+ && (gelic_descr_get_status(card->tx_chain.head)
+ == GELIC_DESCR_DMA_NOT_IN_USE)) {
return card->tx_chain.head;
- else
- return NULL;
+ }
+ return NULL;
}
/**
@@ -787,14 +798,15 @@ static void gelic_descr_set_tx_cmdstat(struct gelic_descr *descr,
}
static struct sk_buff *gelic_put_vlan_tag(struct sk_buff *skb,
- unsigned short tag)
+ unsigned short tag)
{
struct vlan_ethhdr *veth;
static unsigned int c;
if (skb_headroom(skb) < VLAN_HLEN) {
struct sk_buff *sk_tmp = skb;
- pr_debug("%s: hd=%d c=%ud\n", __func__, skb_headroom(skb), c);
+ pr_debug("%s:%d: hd=%d c=%ud\n", __func__, __LINE__,
+ skb_headroom(skb), c);
skb = skb_realloc_headroom(sk_tmp, VLAN_HLEN);
if (!skb)
return NULL;
@@ -818,7 +830,6 @@ static struct sk_buff *gelic_put_vlan_tag(struct sk_buff *skb,
* @skb: packet to use
*
* returns 0 on success, <0 on failure.
- *
*/
static int gelic_descr_prepare_tx(struct gelic_card *card,
struct gelic_descr *descr, struct sk_buff *skb)
@@ -1161,9 +1172,9 @@ static int gelic_net_poll(struct napi_struct *napi, int budget)
int packets_done = 0;
while (packets_done < budget) {
- if (!gelic_card_decode_one_descr(card))
+ if (!gelic_card_decode_one_descr(card)) {
break;
-
+ }
packets_done++;
}
@@ -1171,6 +1182,7 @@ static int gelic_net_poll(struct napi_struct *napi, int budget)
napi_complete_done(napi, packets_done);
gelic_card_rx_irq_on(card);
}
+
return packets_done;
}
@@ -1185,8 +1197,9 @@ static irqreturn_t gelic_card_interrupt(int irq, void *ptr)
status = card->irq_status;
- if (!status)
+ if (!status) {
return IRQ_NONE;
+ }
status &= card->irq_mask;
@@ -1205,13 +1218,15 @@ static irqreturn_t gelic_card_interrupt(int irq, void *ptr)
}
/* ether port status changed */
- if (status & GELIC_CARD_PORT_STATUS_CHANGED)
+ if (status & GELIC_CARD_PORT_STATUS_CHANGED) {
gelic_card_get_ether_port_status(card, 1);
+ }
#ifdef CONFIG_GELIC_WIRELESS
if (status & (GELIC_CARD_WLAN_EVENT_RECEIVED |
- GELIC_CARD_WLAN_COMMAND_COMPLETED))
+ GELIC_CARD_WLAN_COMMAND_COMPLETED)) {
gelic_wl_interrupt(card->netdev[GELIC_PORT_WIRELESS], status);
+ }
#endif
return IRQ_HANDLED;
@@ -1247,19 +1262,16 @@ int gelic_net_open(struct net_device *netdev)
{
struct gelic_card *card = netdev_card(netdev);
- dev_dbg(ctodev(card), " -> %s %p\n", __func__, netdev);
-
gelic_card_up(card);
netif_start_queue(netdev);
gelic_card_get_ether_port_status(card, 1);
- dev_dbg(ctodev(card), " <- %s\n", __func__);
return 0;
}
void gelic_net_get_drvinfo(struct net_device *netdev,
- struct ethtool_drvinfo *info)
+ struct ethtool_drvinfo *info)
{
strlcpy(info->driver, DRV_NAME, sizeof(info->driver));
strlcpy(info->version, DRV_VERSION, sizeof(info->version));
@@ -1317,11 +1329,11 @@ static int gelic_ether_get_link_ksettings(struct net_device *netdev,
return 0;
}
-static int
-gelic_ether_set_link_ksettings(struct net_device *netdev,
- const struct ethtool_link_ksettings *cmd)
+static int gelic_ether_set_link_ksettings(struct net_device *netdev,
+ const struct ethtool_link_ksettings *cmd)
{
struct gelic_card *card = netdev_card(netdev);
+ struct device *dev = ctodev(card);
u64 mode;
int ret;
@@ -1341,94 +1353,100 @@ gelic_ether_set_link_ksettings(struct net_device *netdev,
default:
return -EINVAL;
}
+
if (cmd->base.duplex == DUPLEX_FULL) {
mode |= GELIC_LV1_ETHER_FULL_DUPLEX;
} else if (cmd->base.speed == SPEED_1000) {
- pr_info("1000 half duplex is not supported.\n");
+ dev_dbg(dev,
+ "%s:%d: 1000 half duplex is not supported.\n",
+ __func__, __LINE__);
return -EINVAL;
}
}
ret = gelic_card_set_link_mode(card, mode);
- if (ret)
+ if (ret) {
return ret;
+ }
return 0;
}
static void gelic_net_get_wol(struct net_device *netdev,
- struct ethtool_wolinfo *wol)
+ struct ethtool_wolinfo *wol)
{
- if (0 <= ps3_compare_firmware_version(2, 2, 0))
+ if (0 <= ps3_compare_firmware_version(2, 2, 0)) {
wol->supported = WAKE_MAGIC;
- else
+ } else {
wol->supported = 0;
+ }
wol->wolopts = ps3_sys_manager_get_wol() ? wol->supported : 0;
memset(&wol->sopass, 0, sizeof(wol->sopass));
}
+
static int gelic_net_set_wol(struct net_device *netdev,
- struct ethtool_wolinfo *wol)
+ struct ethtool_wolinfo *wol)
{
+ struct gelic_card *card = netdev_card(netdev);
+ struct device *dev = ctodev(card);
int status;
- struct gelic_card *card;
u64 v1, v2;
if (ps3_compare_firmware_version(2, 2, 0) < 0 ||
- !capable(CAP_NET_ADMIN))
+ !capable(CAP_NET_ADMIN)) {
return -EPERM;
+ }
- if (wol->wolopts & ~WAKE_MAGIC)
+ if (wol->wolopts & ~WAKE_MAGIC) {
return -EINVAL;
+ }
- card = netdev_card(netdev);
if (wol->wolopts & WAKE_MAGIC) {
status = lv1_net_control(bus_id(card), dev_id(card),
- GELIC_LV1_SET_WOL,
- GELIC_LV1_WOL_MAGIC_PACKET,
- 0, GELIC_LV1_WOL_MP_ENABLE,
- &v1, &v2);
+ GELIC_LV1_SET_WOL, GELIC_LV1_WOL_MAGIC_PACKET, 0,
+ GELIC_LV1_WOL_MP_ENABLE, &v1, &v2);
+
if (status) {
- pr_info("%s: enabling WOL failed %d\n", __func__,
- status);
+ dev_dbg(dev, "%s:%d: enabling WOL failed: %d\n",
+ __func__, __LINE__, status);
status = -EIO;
goto done;
}
+
status = lv1_net_control(bus_id(card), dev_id(card),
- GELIC_LV1_SET_WOL,
- GELIC_LV1_WOL_ADD_MATCH_ADDR,
- 0, GELIC_LV1_WOL_MATCH_ALL,
- &v1, &v2);
- if (!status)
+ GELIC_LV1_SET_WOL, GELIC_LV1_WOL_ADD_MATCH_ADDR, 0,
+ GELIC_LV1_WOL_MATCH_ALL, &v1, &v2);
+
+ if (!status) {
ps3_sys_manager_set_wol(1);
- else {
- pr_info("%s: enabling WOL filter failed %d\n",
- __func__, status);
+ } else {
+ dev_dbg(dev, "%s:%d: Enabling WOL filter failed: %d\n",
+ __func__, __LINE__, status);
status = -EIO;
}
} else {
status = lv1_net_control(bus_id(card), dev_id(card),
- GELIC_LV1_SET_WOL,
- GELIC_LV1_WOL_MAGIC_PACKET,
- 0, GELIC_LV1_WOL_MP_DISABLE,
- &v1, &v2);
+ GELIC_LV1_SET_WOL, GELIC_LV1_WOL_MAGIC_PACKET, 0,
+ GELIC_LV1_WOL_MP_DISABLE, &v1, &v2);
+
if (status) {
- pr_info("%s: disabling WOL failed %d\n", __func__,
- status);
+ dev_dbg(dev, "%s:%d: Disabling WOL failed: %d\n",
+ __func__, __LINE__, status);
status = -EIO;
goto done;
}
+
status = lv1_net_control(bus_id(card), dev_id(card),
- GELIC_LV1_SET_WOL,
- GELIC_LV1_WOL_DELETE_MATCH_ADDR,
- 0, GELIC_LV1_WOL_MATCH_ALL,
- &v1, &v2);
+ GELIC_LV1_SET_WOL, GELIC_LV1_WOL_DELETE_MATCH_ADDR, 0,
+ GELIC_LV1_WOL_MATCH_ALL, &v1, &v2);
+
if (!status)
ps3_sys_manager_set_wol(0);
else {
- pr_info("%s: removing WOL filter failed %d\n",
- __func__, status);
+ dev_dbg(dev, "%s:%d: Removing WOL filter failed: %d\n",
+ __func__, __LINE__, status);
status = -EIO;
}
}
@@ -1445,6 +1463,11 @@ static const struct ethtool_ops gelic_ether_ethtool_ops = {
.set_link_ksettings = gelic_ether_set_link_ksettings,
};
+static struct gelic_card *gelic_work_to_card(struct work_struct *work)
+{
+ return container_of(work, struct gelic_card, tx_timeout_task);
+}
+
/**
* gelic_net_tx_timeout_task - task scheduled by the watchdog timeout
* function (to be called not under interrupt status)
@@ -1454,14 +1477,15 @@ static const struct ethtool_ops gelic_ether_ethtool_ops = {
*/
static void gelic_net_tx_timeout_task(struct work_struct *work)
{
- struct gelic_card *card =
- container_of(work, struct gelic_card, tx_timeout_task);
+ struct gelic_card *card = gelic_work_to_card(work);
struct net_device *netdev = card->netdev[GELIC_PORT_ETHERNET_0];
+ struct device *dev = ctodev(card);
- dev_info(ctodev(card), "%s:Timed out. Restarting...\n", __func__);
+ dev_info(dev, "%s:%d: Timed out. Restarting...\n", __func__, __LINE__);
- if (!(netdev->flags & IFF_UP))
+ if (!(netdev->flags & IFF_UP)) {
goto out;
+ }
netif_device_detach(netdev);
gelic_net_stop(netdev);
@@ -1486,10 +1510,12 @@ void gelic_net_tx_timeout(struct net_device *netdev, unsigned int txqueue)
card = netdev_card(netdev);
atomic_inc(&card->tx_timeout_task_counter);
- if (netdev->flags & IFF_UP)
+
+ if (netdev->flags & IFF_UP) {
schedule_work(&card->tx_timeout_task);
- else
+ } else {
atomic_dec(&card->tx_timeout_task_counter);
+ }
}
static const struct net_device_ops gelic_netdevice_ops = {
@@ -1513,7 +1539,7 @@ static const struct net_device_ops gelic_netdevice_ops = {
* fills out function pointers in the net_device structure
*/
static void gelic_ether_setup_netdev_ops(struct net_device *netdev,
- struct napi_struct *napi)
+ struct napi_struct *napi)
{
netdev->watchdog_timeo = GELIC_NET_WATCHDOG_TIMEOUT;
/* NAPI */
@@ -1534,25 +1560,28 @@ static void gelic_ether_setup_netdev_ops(struct net_device *netdev,
**/
int gelic_net_setup_netdev(struct net_device *netdev, struct gelic_card *card)
{
+ struct device *dev = ctodev(card);
int status;
u64 v1, v2;
netdev->hw_features = NETIF_F_IP_CSUM | NETIF_F_RXCSUM;
-
netdev->features = NETIF_F_IP_CSUM;
- if (GELIC_CARD_RX_CSUM_DEFAULT)
+
+ if (GELIC_CARD_RX_CSUM_DEFAULT) {
netdev->features |= NETIF_F_RXCSUM;
+ }
status = lv1_net_control(bus_id(card), dev_id(card),
- GELIC_LV1_GET_MAC_ADDRESS,
- 0, 0, 0, &v1, &v2);
+ GELIC_LV1_GET_MAC_ADDRESS, 0, 0, 0, &v1, &v2);
+
v1 <<= 16;
+
if (status || !is_valid_ether_addr((u8 *)&v1)) {
- dev_info(ctodev(card),
- "%s:lv1_net_control GET_MAC_ADDR failed %d\n",
- __func__, status);
+ dev_dbg(dev, "%s:%d: lv1_net_control GET_MAC_ADDR failed: %d\n",
+ __func__, __LINE__, status);
return -EINVAL;
}
+
memcpy(netdev->dev_addr, &v1, ETH_ALEN);
if (card->vlan_required) {
@@ -1569,13 +1598,15 @@ int gelic_net_setup_netdev(struct net_device *netdev, struct gelic_card *card)
netdev->max_mtu = GELIC_NET_MAX_MTU;
status = register_netdev(netdev);
+
if (status) {
- dev_err(ctodev(card), "%s:Couldn't register %s %d\n",
- __func__, netdev->name, status);
+ dev_err(dev, "%s:%d: Couldn't register %s: %d\n", __func__,
+ __LINE__, netdev->name, status);
return status;
}
- dev_info(ctodev(card), "%s: MAC addr %pM\n",
- netdev->name, netdev->dev_addr);
+
+ dev_info(dev, "%s:%d: %s MAC addr %pxM\n", __func__, __LINE__,
+ netdev->name, netdev->dev_addr);
return 0;
}
@@ -1600,34 +1631,33 @@ static struct gelic_card *gelic_alloc_card_net(struct net_device **netdev)
*/
BUILD_BUG_ON(offsetof(struct gelic_card, irq_status) % 8);
BUILD_BUG_ON(offsetof(struct gelic_card, descr) % 32);
- alloc_size =
- sizeof(struct gelic_card) +
+
+ alloc_size = sizeof(struct gelic_card) +
sizeof(struct gelic_descr) * GELIC_NET_RX_DESCRIPTORS +
sizeof(struct gelic_descr) * GELIC_NET_TX_DESCRIPTORS +
GELIC_ALIGN - 1;
p = kzalloc(alloc_size, GFP_KERNEL);
- if (!p)
+
+ if (!p) {
return NULL;
+ }
+
card = PTR_ALIGN(p, GELIC_ALIGN);
card->unalign = p;
- /*
- * alloc netdev
- */
*netdev = alloc_etherdev(sizeof(struct gelic_port));
+
if (!*netdev) {
kfree(card->unalign);
return NULL;
}
- port = netdev_priv(*netdev);
- /* gelic_port */
+ port = netdev_priv(*netdev);
port->netdev = *netdev;
port->card = card;
port->type = GELIC_PORT_ETHERNET_0;
- /* gelic_card */
card->netdev[GELIC_PORT_ETHERNET_0] = *netdev;
INIT_WORK(&card->tx_timeout_task, gelic_net_tx_timeout_task);
@@ -1641,13 +1671,15 @@ static struct gelic_card *gelic_alloc_card_net(struct net_device **netdev)
static void gelic_card_get_vlan_info(struct gelic_card *card)
{
+ struct device *dev = ctodev(card);
+ unsigned int i;
u64 v1, v2;
int status;
- unsigned int i;
- struct {
+ struct vlan_id {
int tx;
int rx;
- } vlan_id_ix[2] = {
+ };
+ struct vlan_id vlan_id_ix[2] = {
[GELIC_PORT_ETHERNET_0] = {
.tx = GELIC_LV1_VLAN_TX_ETHERNET_0,
.rx = GELIC_LV1_VLAN_RX_ETHERNET_0
@@ -1661,38 +1693,44 @@ static void gelic_card_get_vlan_info(struct gelic_card *card)
for (i = 0; i < ARRAY_SIZE(vlan_id_ix); i++) {
/* tx tag */
status = lv1_net_control(bus_id(card), dev_id(card),
- GELIC_LV1_GET_VLAN_ID,
- vlan_id_ix[i].tx,
- 0, 0, &v1, &v2);
+ GELIC_LV1_GET_VLAN_ID, vlan_id_ix[i].tx, 0, 0, &v1,
+ &v2);
+
if (status || !v1) {
- if (status != LV1_NO_ENTRY)
- dev_dbg(ctodev(card),
- "get vlan id for tx(%d) failed(%d)\n",
- vlan_id_ix[i].tx, status);
+ if (status != LV1_NO_ENTRY) {
+ dev_dbg(dev,
+ "%s:%d: Get vlan id for tx(%d) failed: %d\n",
+ __func__, __LINE__, vlan_id_ix[i].tx,
+ status);
+ }
card->vlan[i].tx = 0;
card->vlan[i].rx = 0;
continue;
}
+
card->vlan[i].tx = (u16)v1;
/* rx tag */
status = lv1_net_control(bus_id(card), dev_id(card),
- GELIC_LV1_GET_VLAN_ID,
- vlan_id_ix[i].rx,
- 0, 0, &v1, &v2);
+ GELIC_LV1_GET_VLAN_ID, vlan_id_ix[i].rx, 0, 0, &v1,
+ &v2);
+
if (status || !v1) {
- if (status != LV1_NO_ENTRY)
- dev_info(ctodev(card),
- "get vlan id for rx(%d) failed(%d)\n",
- vlan_id_ix[i].rx, status);
+ if (status != LV1_NO_ENTRY) {
+ dev_dbg(dev,
+ "%s:%d: Get vlan id for rx(%d) failed: %d\n",
+ __func__, __LINE__, vlan_id_ix[i].rx,
+ status);
+ }
card->vlan[i].tx = 0;
card->vlan[i].rx = 0;
continue;
}
+
card->vlan[i].rx = (u16)v1;
- dev_dbg(ctodev(card), "vlan_id[%d] tx=%02x rx=%02x\n",
- i, card->vlan[i].tx, card->vlan[i].rx);
+ dev_dbg(dev, "%s:%d: vlan_id[%d] tx=%02x rx=%02x\n", __func__,
+ __LINE__, i, card->vlan[i].tx, card->vlan[i].rx);
}
if (card->vlan[GELIC_PORT_ETHERNET_0].tx) {
@@ -1707,9 +1745,10 @@ static void gelic_card_get_vlan_info(struct gelic_card *card)
card->vlan[GELIC_PORT_WIRELESS].rx = 0;
}
- dev_info(ctodev(card), "internal vlan %s\n",
- card->vlan_required? "enabled" : "disabled");
+ dev_dbg(dev, "%s:%d: internal vlan %s\n", __func__, __LINE__,
+ card->vlan_required? "enabled" : "disabled");
}
+
/*
* ps3_gelic_driver_probe - add a device to the control of this driver
*/
@@ -1872,26 +1911,25 @@ static int ps3_gelic_driver_probe(struct ps3_system_bus_device *sb_dev)
* ps3_gelic_driver_remove - remove a device from the control of this driver
*/
-static void ps3_gelic_driver_remove(struct ps3_system_bus_device *dev)
+static void ps3_gelic_driver_remove(struct ps3_system_bus_device *sb_dev)
{
- struct gelic_card *card = ps3_system_bus_get_drvdata(dev);
+ struct gelic_card *card = ps3_system_bus_get_drvdata(sb_dev);
+ struct device *dev = &sb_dev->core;
struct net_device *netdev0;
- pr_debug("%s: called\n", __func__);
- /* set auto-negotiation */
+ dev_dbg(dev, "%s:%d: >\n", __func__, __LINE__);
+
gelic_card_set_link_mode(card, GELIC_LV1_ETHER_AUTO_NEG);
#ifdef CONFIG_GELIC_WIRELESS
gelic_wl_driver_remove(card);
#endif
- /* stop interrupt */
+
gelic_card_set_irq_mask(card, 0);
- /* turn off DMA, force end */
gelic_card_disable_rxdmac(card);
gelic_card_disable_txdmac(card);
- /* release chains */
gelic_card_release_tx_chain(card, 1);
gelic_card_release_rx_chain(card);
@@ -1899,28 +1937,28 @@ static void ps3_gelic_driver_remove(struct ps3_system_bus_device *dev)
gelic_card_free_chain(card, card->rx_top);
netdev0 = card->netdev[GELIC_PORT_ETHERNET_0];
- /* disconnect event port */
+
free_irq(card->irq, card);
netdev0->irq = 0;
ps3_sb_event_receive_port_destroy(card->dev, card->irq);
wait_event(card->waitq,
- atomic_read(&card->tx_timeout_task_counter) == 0);
+ atomic_read(&card->tx_timeout_task_counter) == 0);
lv1_net_set_interrupt_status_indicator(bus_id(card), dev_id(card),
- 0 , 0);
+ 0 , 0);
unregister_netdev(netdev0);
kfree(netdev_card(netdev0)->unalign);
free_netdev(netdev0);
- ps3_system_bus_set_drvdata(dev, NULL);
+ ps3_system_bus_set_drvdata(sb_dev, NULL);
- ps3_dma_region_free(dev->d_region);
+ ps3_dma_region_free(sb_dev->d_region);
- ps3_close_hv_device(dev);
+ ps3_close_hv_device(sb_dev);
- pr_debug("%s: done\n", __func__);
+ dev_dbg(dev, "%s:%d: <\n", __func__, __LINE__);
}
static struct ps3_system_bus_driver ps3_gelic_driver = {
@@ -1932,14 +1970,14 @@ static struct ps3_system_bus_driver ps3_gelic_driver = {
.core.owner = THIS_MODULE,
};
-static int __init ps3_gelic_driver_init (void)
+static int __init ps3_gelic_driver_init(void)
{
return firmware_has_feature(FW_FEATURE_PS3_LV1)
? ps3_system_bus_driver_register(&ps3_gelic_driver)
: -ENODEV;
}
-static void __exit ps3_gelic_driver_exit (void)
+static void __exit ps3_gelic_driver_exit(void)
{
ps3_system_bus_driver_unregister(&ps3_gelic_driver);
}
@@ -1948,4 +1986,3 @@ module_init(ps3_gelic_driver_init);
module_exit(ps3_gelic_driver_exit);
MODULE_ALIAS(PS3_MODULE_ALIAS_GELIC);
-
--
2.25.1
^ permalink raw reply related
* [PATCH v3 1/2] net/ps3_gelic: Add gelic_descr structures
From: Geoff Levand @ 2021-07-11 4:15 UTC (permalink / raw)
To: David S. Miller, Jakub Kicinski; +Cc: netdev, linuxppc-dev
In-Reply-To: <cover.1625976141.git.geoff@infradead.org>
Create two new structures, struct gelic_hw_regs and struct gelic_chain_link,
and replace the corresponding members of struct gelic_descr with the new
structures. struct gelic_hw_regs holds the register variables used by the
gelic hardware device. struct gelic_chain_link holds variables used to
manage the driver's linked list of gelic descr structures.
Fixes several DMA mapping problems with the PS3's gelic network driver:
* Change from checking the return value of dma_map_single to using the
dma_mapping_error routine.
* Use the correct buffer length when mapping the RX skb.
* Improved error checking and debug logging.
Fixes runtime errors like these, and also other randomly occurring errors:
IP-Config: Complete:
DMA-API: ps3_gelic_driver sb_05: device driver failed to check map error
WARNING: CPU: 0 PID: 0 at kernel/dma/debug.c:1027 .check_unmap+0x888/0x8dc
Signed-off-by: Geoff Levand <geoff@infradead.org>
---
drivers/net/ethernet/toshiba/ps3_gelic_net.c | 573 +++++++++++--------
drivers/net/ethernet/toshiba/ps3_gelic_net.h | 24 +-
2 files changed, 341 insertions(+), 256 deletions(-)
diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
index 55e652624bd7..e01938128882 100644
--- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
+++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
@@ -96,9 +96,11 @@ static void gelic_card_get_ether_port_status(struct gelic_card *card,
* returns the status as in the dmac_cmd_status field of the descriptor
*/
static enum gelic_descr_dma_status
+
gelic_descr_get_status(struct gelic_descr *descr)
{
- return be32_to_cpu(descr->dmac_cmd_status) & GELIC_DESCR_DMA_STAT_MASK;
+ return be32_to_cpu(descr->hw_regs.dmac_cmd_status)
+ & GELIC_DESCR_DMA_STAT_MASK;
}
static int gelic_card_set_link_mode(struct gelic_card *card, int mode)
@@ -146,24 +148,34 @@ static void gelic_card_disable_txdmac(struct gelic_card *card)
*/
static void gelic_card_enable_rxdmac(struct gelic_card *card)
{
+ struct device *dev = ctodev(card);
int status;
+#if defined(DEBUG)
+ static const int debug_build = 1;
+#else
+ static const int debug_build = 0;
+#endif
-#ifdef DEBUG
- if (gelic_descr_get_status(card->rx_chain.head) !=
- GELIC_DESCR_DMA_CARDOWNED) {
- printk(KERN_ERR "%s: status=%x\n", __func__,
- be32_to_cpu(card->rx_chain.head->dmac_cmd_status));
- printk(KERN_ERR "%s: nextphy=%x\n", __func__,
- be32_to_cpu(card->rx_chain.head->next_descr_addr));
- printk(KERN_ERR "%s: head=%p\n", __func__,
- card->rx_chain.head);
+ if (debug_build
+ && (gelic_descr_get_status(card->rx_chain.head)
+ != GELIC_DESCR_DMA_CARDOWNED)) {
+ dev_err(dev, "%s:%d: status=%x\n", __func__, __LINE__,
+ be32_to_cpu(
+ card->rx_chain.head->hw_regs.dmac_cmd_status));
+ dev_err(dev, "%s:%d: nextphy=%x\n", __func__, __LINE__,
+ be32_to_cpu(
+ card->rx_chain.head->hw_regs.next_descr_addr));
+ dev_err(dev, "%s:%d: head=%px\n", __func__, __LINE__,
+ card->rx_chain.head);
}
-#endif
+
status = lv1_net_start_rx_dma(bus_id(card), dev_id(card),
- card->rx_chain.head->bus_addr, 0);
- if (status)
- dev_info(ctodev(card),
- "lv1_net_start_rx_dma failed, status=%d\n", status);
+ card->rx_chain.head->link.cpu_addr, 0);
+
+ if (status) {
+ dev_err(dev, "%s:%d: lv1_net_start_rx_dma failed: %d\n",
+ __func__, __LINE__, status);
+ }
}
/**
@@ -193,11 +205,11 @@ static void gelic_card_disable_rxdmac(struct gelic_card *card)
* in the status
*/
static void gelic_descr_set_status(struct gelic_descr *descr,
- enum gelic_descr_dma_status status)
+ enum gelic_descr_dma_status status)
{
- descr->dmac_cmd_status = cpu_to_be32(status |
- (be32_to_cpu(descr->dmac_cmd_status) &
- ~GELIC_DESCR_DMA_STAT_MASK));
+ descr->hw_regs.dmac_cmd_status = cpu_to_be32(status
+ | (be32_to_cpu(descr->hw_regs.dmac_cmd_status)
+ & ~GELIC_DESCR_DMA_STAT_MASK));
/*
* dma_cmd_status field is used to indicate whether the descriptor
* is valid or not.
@@ -224,13 +236,14 @@ static void gelic_card_reset_chain(struct gelic_card *card,
for (descr = start_descr; start_descr != descr->next; descr++) {
gelic_descr_set_status(descr, GELIC_DESCR_DMA_CARDOWNED);
- descr->next_descr_addr = cpu_to_be32(descr->next->bus_addr);
+ descr->hw_regs.next_descr_addr
+ = cpu_to_be32(descr->next->link.cpu_addr);
}
chain->head = start_descr;
chain->tail = (descr - 1);
- (descr - 1)->next_descr_addr = 0;
+ (descr - 1)->hw_regs.next_descr_addr = 0;
}
void gelic_card_up(struct gelic_card *card)
@@ -276,20 +289,35 @@ void gelic_card_down(struct gelic_card *card)
pr_debug("%s: done\n", __func__);
}
+static void gelic_unmap_link(struct device *dev, struct gelic_descr *descr)
+{
+ BUG_ON(descr->hw_regs.payload.dev_addr);
+ BUG_ON(descr->hw_regs.payload.size);
+
+ BUG_ON(!descr->link.cpu_addr);
+ BUG_ON(!descr->link.size);
+
+ dma_unmap_single(dev, descr->link.cpu_addr, descr->link.size,
+ DMA_BIDIRECTIONAL);
+
+ descr->link.cpu_addr = 0;
+ descr->link.size = 0;
+}
+
/**
* gelic_card_free_chain - free descriptor chain
* @card: card structure
* @descr_in: address of desc
*/
static void gelic_card_free_chain(struct gelic_card *card,
- struct gelic_descr *descr_in)
+ struct gelic_descr *descr_in)
{
+ struct device *dev = ctodev(card);
struct gelic_descr *descr;
- for (descr = descr_in; descr && descr->bus_addr; descr = descr->next) {
- dma_unmap_single(ctodev(card), descr->bus_addr,
- GELIC_DESCR_SIZE, DMA_BIDIRECTIONAL);
- descr->bus_addr = 0;
+ for (descr = descr_in; descr && descr->link.cpu_addr;
+ descr = descr->next) {
+ gelic_unmap_link(dev, descr);
}
}
@@ -298,7 +326,7 @@ static void gelic_card_free_chain(struct gelic_card *card,
* @card: card structure
* @chain: address of chain
* @start_descr: address of descriptor array
- * @no: number of descriptors
+ * @descr_count: number of descriptors
*
* we manage a circular list that mirrors the hardware structure,
* except that the hardware uses bus addresses.
@@ -306,54 +334,57 @@ static void gelic_card_free_chain(struct gelic_card *card,
* returns 0 on success, <0 on failure
*/
static int gelic_card_init_chain(struct gelic_card *card,
- struct gelic_descr_chain *chain,
- struct gelic_descr *start_descr, int no)
+ struct gelic_descr_chain *chain, struct gelic_descr *start_descr,
+ int descr_count)
{
- int i;
- struct gelic_descr *descr;
+ struct gelic_descr *descr = start_descr;
+ struct device *dev = ctodev(card);
+ unsigned int index;
- descr = start_descr;
- memset(descr, 0, sizeof(*descr) * no);
+ memset(start_descr, 0, descr_count * sizeof(*start_descr));
- /* set up the hardware pointers in each descriptor */
- for (i = 0; i < no; i++, descr++) {
+ for (index = 0, descr = start_descr; index < descr_count;
+ index++, descr++) {
gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE);
- descr->bus_addr =
- dma_map_single(ctodev(card), descr,
- GELIC_DESCR_SIZE,
- DMA_BIDIRECTIONAL);
- if (!descr->bus_addr)
- goto iommu_error;
+ descr->link.size = sizeof(struct gelic_hw_regs);
+ descr->link.cpu_addr = dma_map_single(dev, descr,
+ descr->link.size, DMA_BIDIRECTIONAL);
+
+ if (unlikely(dma_mapping_error(dev, descr->link.cpu_addr))) {
+ dev_err(dev, "%s:%d: dma_mapping_error\n", __func__,
+ __LINE__);
+
+ for (index--, descr--; index > 0; index--, descr--) {
+ if (descr->link.cpu_addr) {
+ gelic_unmap_link(dev, descr);
+ }
+ }
+ return -ENOMEM;
+ }
descr->next = descr + 1;
descr->prev = descr - 1;
}
- /* make them as ring */
+
(descr - 1)->next = start_descr;
start_descr->prev = (descr - 1);
/* chain bus addr of hw descriptor */
- descr = start_descr;
- for (i = 0; i < no; i++, descr++) {
- descr->next_descr_addr = cpu_to_be32(descr->next->bus_addr);
+
+ for (index = 0, descr = start_descr; index < descr_count;
+ index++, descr++) {
+ descr->hw_regs.next_descr_addr
+ = cpu_to_be32(descr->next->link.cpu_addr);
}
chain->head = start_descr;
chain->tail = start_descr;
/* do not chain last hw descriptor */
- (descr - 1)->next_descr_addr = 0;
+ (descr - 1)->hw_regs.next_descr_addr = 0;
return 0;
-
-iommu_error:
- for (i--, descr--; 0 <= i; i--, descr--)
- if (descr->bus_addr)
- dma_unmap_single(ctodev(card), descr->bus_addr,
- GELIC_DESCR_SIZE,
- DMA_BIDIRECTIONAL);
- return -ENOMEM;
}
/**
@@ -367,49 +398,66 @@ static int gelic_card_init_chain(struct gelic_card *card,
* Activate the descriptor state-wise
*/
static int gelic_descr_prepare_rx(struct gelic_card *card,
- struct gelic_descr *descr)
+ struct gelic_descr *descr)
{
- int offset;
- unsigned int bufsize;
+ struct device *dev = ctodev(card);
+ struct aligned_buff {
+ unsigned int total_bytes;
+ unsigned int offset;
+ };
+ struct aligned_buff a_buf;
+ dma_addr_t cpu_addr;
+
+ if (gelic_descr_get_status(descr) != GELIC_DESCR_DMA_NOT_IN_USE) {
+ dev_err(dev, "%s:%d: ERROR status\n", __func__, __LINE__);
+ }
- if (gelic_descr_get_status(descr) != GELIC_DESCR_DMA_NOT_IN_USE)
- dev_info(ctodev(card), "%s: ERROR status\n", __func__);
- /* we need to round up the buffer size to a multiple of 128 */
- bufsize = ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN);
+ a_buf.total_bytes = ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN)
+ + GELIC_NET_RXBUF_ALIGN;
+
+ descr->skb = dev_alloc_skb(a_buf.total_bytes);
- /* and we need to have it 128 byte aligned, therefore we allocate a
- * bit more */
- descr->skb = dev_alloc_skb(bufsize + GELIC_NET_RXBUF_ALIGN - 1);
if (!descr->skb) {
- descr->buf_addr = 0; /* tell DMAC don't touch memory */
+ descr->hw_regs.payload.dev_addr = 0;
+ descr->hw_regs.payload.size = 0;
return -ENOMEM;
}
- descr->buf_size = cpu_to_be32(bufsize);
- descr->dmac_cmd_status = 0;
- descr->result_size = 0;
- descr->valid_size = 0;
- descr->data_error = 0;
-
- offset = ((unsigned long)descr->skb->data) &
- (GELIC_NET_RXBUF_ALIGN - 1);
- if (offset)
- skb_reserve(descr->skb, GELIC_NET_RXBUF_ALIGN - offset);
- /* io-mmu-map the skb */
- descr->buf_addr = cpu_to_be32(dma_map_single(ctodev(card),
- descr->skb->data,
- GELIC_NET_MAX_MTU,
- DMA_FROM_DEVICE));
- if (!descr->buf_addr) {
+
+ a_buf.offset = PTR_ALIGN(descr->skb->data, GELIC_NET_RXBUF_ALIGN)
+ - descr->skb->data;
+
+ if (a_buf.offset) {
+ dev_dbg(dev, "%s:%d: offset=%u\n", __func__, __LINE__,
+ a_buf.offset);
+ skb_reserve(descr->skb, a_buf.offset);
+ }
+
+ descr->hw_regs.dmac_cmd_status = 0;
+ descr->hw_regs.result_size = 0;
+ descr->hw_regs.valid_size = 0;
+ descr->hw_regs.data_error = 0;
+
+ descr->hw_regs.payload.size = a_buf.total_bytes - a_buf.offset;
+ cpu_addr = dma_map_single(dev, descr->skb->data,
+ descr->hw_regs.payload.size, DMA_FROM_DEVICE);
+ descr->hw_regs.payload.dev_addr = cpu_to_be32(cpu_addr);
+
+ if (unlikely(dma_mapping_error(dev, cpu_addr))) {
+ dev_err(dev, "%s:%d: dma_mapping_error\n", __func__, __LINE__);
+
+ descr->hw_regs.payload.dev_addr = 0;
+ descr->hw_regs.payload.size = 0;
+
dev_kfree_skb_any(descr->skb);
descr->skb = NULL;
- dev_info(ctodev(card),
- "%s:Could not iommu-map rx buffer\n", __func__);
+
gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE);
+
return -ENOMEM;
- } else {
- gelic_descr_set_status(descr, GELIC_DESCR_DMA_CARDOWNED);
- return 0;
}
+
+ gelic_descr_set_status(descr, GELIC_DESCR_DMA_CARDOWNED);
+ return 0;
}
/**
@@ -420,19 +468,24 @@ static int gelic_descr_prepare_rx(struct gelic_card *card,
static void gelic_card_release_rx_chain(struct gelic_card *card)
{
struct gelic_descr *descr = card->rx_chain.head;
+ struct device *dev = ctodev(card);
do {
if (descr->skb) {
- dma_unmap_single(ctodev(card),
- be32_to_cpu(descr->buf_addr),
- descr->skb->len,
- DMA_FROM_DEVICE);
- descr->buf_addr = 0;
+ dma_unmap_single(dev,
+ be32_to_cpu(descr->hw_regs.payload.dev_addr),
+ descr->hw_regs.payload.size, DMA_FROM_DEVICE);
+
dev_kfree_skb_any(descr->skb);
descr->skb = NULL;
+
gelic_descr_set_status(descr,
- GELIC_DESCR_DMA_NOT_IN_USE);
+ GELIC_DESCR_DMA_NOT_IN_USE);
}
+
+ descr->hw_regs.payload.dev_addr = 0;
+ descr->hw_regs.payload.size = 0;
+
descr = descr->next;
} while (descr != card->rx_chain.head);
}
@@ -489,26 +542,29 @@ static int gelic_card_alloc_rx_skbs(struct gelic_card *card)
* releases a used tx descriptor (unmapping, freeing of skb)
*/
static void gelic_descr_release_tx(struct gelic_card *card,
- struct gelic_descr *descr)
+ struct gelic_descr *descr)
{
struct sk_buff *skb = descr->skb;
+ struct device *dev = ctodev(card);
- BUG_ON(!(be32_to_cpu(descr->data_status) & GELIC_DESCR_TX_TAIL));
+ BUG_ON(!(be32_to_cpu(descr->hw_regs.data_status)
+ & GELIC_DESCR_TX_TAIL));
- dma_unmap_single(ctodev(card), be32_to_cpu(descr->buf_addr), skb->len,
- DMA_TO_DEVICE);
- dev_kfree_skb_any(skb);
+ dma_unmap_single(dev, be32_to_cpu(descr->hw_regs.payload.dev_addr),
+ descr->hw_regs.payload.size, DMA_TO_DEVICE);
+
+ descr->hw_regs.payload.dev_addr = 0;
+ descr->hw_regs.payload.size = 0;
- descr->buf_addr = 0;
- descr->buf_size = 0;
- descr->next_descr_addr = 0;
- descr->result_size = 0;
- descr->valid_size = 0;
- descr->data_status = 0;
- descr->data_error = 0;
+ dev_kfree_skb_any(skb);
descr->skb = NULL;
- /* set descr status */
+ descr->hw_regs.next_descr_addr = 0;
+ descr->hw_regs.result_size = 0;
+ descr->hw_regs.valid_size = 0;
+ descr->hw_regs.data_status = 0;
+ descr->hw_regs.data_error = 0;
+
gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE);
}
@@ -536,48 +592,53 @@ static void gelic_card_wake_queues(struct gelic_card *card)
static void gelic_card_release_tx_chain(struct gelic_card *card, int stop)
{
struct gelic_descr_chain *tx_chain;
- enum gelic_descr_dma_status status;
- struct net_device *netdev;
- int release = 0;
+ struct device *dev = ctodev(card);
+ int release;
- for (tx_chain = &card->tx_chain;
+ for (release = 0, tx_chain = &card->tx_chain;
tx_chain->head != tx_chain->tail && tx_chain->tail;
tx_chain->tail = tx_chain->tail->next) {
- status = gelic_descr_get_status(tx_chain->tail);
- netdev = tx_chain->tail->skb->dev;
+ enum gelic_descr_dma_status status;
+ struct gelic_descr *descr;
+ struct net_device *netdev;
+
+ descr = tx_chain->tail;
+ status= gelic_descr_get_status(descr);
+ netdev = descr->skb->dev;
+
switch (status) {
case GELIC_DESCR_DMA_RESPONSE_ERROR:
case GELIC_DESCR_DMA_PROTECTION_ERROR:
case GELIC_DESCR_DMA_FORCE_END:
- if (printk_ratelimit())
- dev_info(ctodev(card),
- "%s: forcing end of tx descriptor " \
- "with status %x\n",
- __func__, status);
+ if (printk_ratelimit()) {
+ dev_info(dev,
+ "%s:%d: forcing end of tx descriptor with status %x\n",
+ __func__, __LINE__, status);
+ }
netdev->stats.tx_dropped++;
break;
case GELIC_DESCR_DMA_COMPLETE:
- if (tx_chain->tail->skb) {
+ if (descr->skb) {
netdev->stats.tx_packets++;
- netdev->stats.tx_bytes +=
- tx_chain->tail->skb->len;
+ netdev->stats.tx_bytes += descr->skb->len;
}
break;
case GELIC_DESCR_DMA_CARDOWNED:
- /* pending tx request */
default:
- /* any other value (== GELIC_DESCR_DMA_NOT_IN_USE) */
- if (!stop)
+ if (!stop) {
goto out;
+ }
}
- gelic_descr_release_tx(card, tx_chain->tail);
- release ++;
+
+ gelic_descr_release_tx(card, descr);
+ release++;
}
out:
- if (!stop && release)
+ if (!stop && release) {
gelic_card_wake_queues(card);
+ }
}
/**
@@ -695,30 +756,30 @@ gelic_card_get_next_tx_descr(struct gelic_card *card)
* has executed before.
*/
static void gelic_descr_set_tx_cmdstat(struct gelic_descr *descr,
- struct sk_buff *skb)
+ struct sk_buff *skb)
{
if (skb->ip_summed != CHECKSUM_PARTIAL)
- descr->dmac_cmd_status =
+ descr->hw_regs.dmac_cmd_status =
cpu_to_be32(GELIC_DESCR_DMA_CMD_NO_CHKSUM |
- GELIC_DESCR_TX_DMA_FRAME_TAIL);
+ GELIC_DESCR_TX_DMA_FRAME_TAIL);
else {
/* is packet ip?
* if yes: tcp? udp? */
if (skb->protocol == htons(ETH_P_IP)) {
if (ip_hdr(skb)->protocol == IPPROTO_TCP)
- descr->dmac_cmd_status =
+ descr->hw_regs.dmac_cmd_status =
cpu_to_be32(GELIC_DESCR_DMA_CMD_TCP_CHKSUM |
GELIC_DESCR_TX_DMA_FRAME_TAIL);
else if (ip_hdr(skb)->protocol == IPPROTO_UDP)
- descr->dmac_cmd_status =
+ descr->hw_regs.dmac_cmd_status =
cpu_to_be32(GELIC_DESCR_DMA_CMD_UDP_CHKSUM |
GELIC_DESCR_TX_DMA_FRAME_TAIL);
else /*
* the stack should checksum non-tcp and non-udp
* packets on his own: NETIF_F_IP_CSUM
*/
- descr->dmac_cmd_status =
+ descr->hw_regs.dmac_cmd_status =
cpu_to_be32(GELIC_DESCR_DMA_CMD_NO_CHKSUM |
GELIC_DESCR_TX_DMA_FRAME_TAIL);
}
@@ -760,37 +821,41 @@ static struct sk_buff *gelic_put_vlan_tag(struct sk_buff *skb,
*
*/
static int gelic_descr_prepare_tx(struct gelic_card *card,
- struct gelic_descr *descr,
- struct sk_buff *skb)
+ struct gelic_descr *descr, struct sk_buff *skb)
{
- dma_addr_t buf;
+ struct device *dev = ctodev(card);
+ dma_addr_t cpu_addr;
if (card->vlan_required) {
struct sk_buff *skb_tmp;
enum gelic_port_type type;
type = netdev_port(skb->dev)->type;
- skb_tmp = gelic_put_vlan_tag(skb,
- card->vlan[type].tx);
- if (!skb_tmp)
+ skb_tmp = gelic_put_vlan_tag(skb, card->vlan[type].tx);
+
+ if (!skb_tmp) {
return -ENOMEM;
+ }
+
skb = skb_tmp;
}
- buf = dma_map_single(ctodev(card), skb->data, skb->len, DMA_TO_DEVICE);
+ descr->hw_regs.payload.size = skb->len;
+ cpu_addr = dma_map_single(dev, skb->data, descr->hw_regs.payload.size,
+ DMA_TO_DEVICE);
+ descr->hw_regs.payload.dev_addr = cpu_to_be32(cpu_addr);
- if (!buf) {
- dev_err(ctodev(card),
- "dma map 2 failed (%p, %i). Dropping packet\n",
- skb->data, skb->len);
+ if (unlikely(dma_mapping_error(dev, cpu_addr))) {
+ dev_err(dev, "%s:%d: dma_mapping_error\n", __func__, __LINE__);
+
+ descr->hw_regs.payload.dev_addr = 0;
+ descr->hw_regs.payload.size = 0;
return -ENOMEM;
}
- descr->buf_addr = cpu_to_be32(buf);
- descr->buf_size = cpu_to_be32(skb->len);
descr->skb = skb;
- descr->data_status = 0;
- descr->next_descr_addr = 0; /* terminate hw descr */
+ descr->hw_regs.data_status = 0;
+ descr->hw_regs.next_descr_addr = 0; /* terminate hw descr */
gelic_descr_set_tx_cmdstat(descr, skb);
/* bump free descriptor pointer */
@@ -805,8 +870,9 @@ static int gelic_descr_prepare_tx(struct gelic_card *card,
*
*/
static int gelic_card_kick_txdma(struct gelic_card *card,
- struct gelic_descr *descr)
+ struct gelic_descr *descr)
{
+ struct device *dev = ctodev(card);
int status = 0;
if (card->tx_dma_progress)
@@ -815,11 +881,11 @@ static int gelic_card_kick_txdma(struct gelic_card *card,
if (gelic_descr_get_status(descr) == GELIC_DESCR_DMA_CARDOWNED) {
card->tx_dma_progress = 1;
status = lv1_net_start_tx_dma(bus_id(card), dev_id(card),
- descr->bus_addr, 0);
+ descr->link.cpu_addr, 0);
if (status) {
card->tx_dma_progress = 0;
- dev_info(ctodev(card), "lv1_net_start_txdma failed," \
- "status=%d\n", status);
+ dev_info(dev, "%s:%d: lv1_net_start_txdma failed: %d\n",
+ __func__, __LINE__, status);
}
}
return status;
@@ -835,6 +901,7 @@ static int gelic_card_kick_txdma(struct gelic_card *card,
netdev_tx_t gelic_net_xmit(struct sk_buff *skb, struct net_device *netdev)
{
struct gelic_card *card = netdev_card(netdev);
+ struct device *dev = ctodev(card);
struct gelic_descr *descr;
int result;
unsigned long flags;
@@ -868,7 +935,9 @@ netdev_tx_t gelic_net_xmit(struct sk_buff *skb, struct net_device *netdev)
* link this prepared descriptor to previous one
* to achieve high performance
*/
- descr->prev->next_descr_addr = cpu_to_be32(descr->bus_addr);
+ descr->prev->hw_regs.next_descr_addr
+ = cpu_to_be32(descr->link.cpu_addr);
+
/*
* as hardware descriptor is modified in the above lines,
* ensure that the hardware sees it
@@ -881,13 +950,13 @@ netdev_tx_t gelic_net_xmit(struct sk_buff *skb, struct net_device *netdev)
*/
netdev->stats.tx_dropped++;
/* don't trigger BUG_ON() in gelic_descr_release_tx */
- descr->data_status = cpu_to_be32(GELIC_DESCR_TX_TAIL);
+ descr->hw_regs.data_status = cpu_to_be32(GELIC_DESCR_TX_TAIL);
gelic_descr_release_tx(card, descr);
/* reset head */
card->tx_chain.head = descr;
/* reset hw termination */
- descr->prev->next_descr_addr = 0;
- dev_info(ctodev(card), "%s: kick failure\n", __func__);
+ descr->prev->hw_regs.next_descr_addr = 0;
+ dev_info(dev, "%s:%d: kick failure\n", __func__, __LINE__);
}
spin_unlock_irqrestore(&card->tx_lock, flags);
@@ -904,28 +973,29 @@ netdev_tx_t gelic_net_xmit(struct sk_buff *skb, struct net_device *netdev)
* stack. The descriptor state is not changed.
*/
static void gelic_net_pass_skb_up(struct gelic_descr *descr,
- struct gelic_card *card,
- struct net_device *netdev)
-
+ struct gelic_card *card, struct net_device *netdev)
{
+ struct device *dev = ctodev(card);
struct sk_buff *skb = descr->skb;
u32 data_status, data_error;
- data_status = be32_to_cpu(descr->data_status);
- data_error = be32_to_cpu(descr->data_error);
- /* unmap skb buffer */
- dma_unmap_single(ctodev(card), be32_to_cpu(descr->buf_addr),
- GELIC_NET_MAX_MTU,
- DMA_FROM_DEVICE);
-
- skb_put(skb, be32_to_cpu(descr->valid_size)?
- be32_to_cpu(descr->valid_size) :
- be32_to_cpu(descr->result_size));
- if (!descr->valid_size)
- dev_info(ctodev(card), "buffer full %x %x %x\n",
- be32_to_cpu(descr->result_size),
- be32_to_cpu(descr->buf_size),
- be32_to_cpu(descr->dmac_cmd_status));
+ data_status = be32_to_cpu(descr->hw_regs.data_status);
+ data_error = be32_to_cpu(descr->hw_regs.data_error);
+
+ dma_unmap_single(dev, be32_to_cpu(descr->hw_regs.payload.dev_addr),
+ descr->hw_regs.payload.size, DMA_FROM_DEVICE);
+
+ skb_put(skb, be32_to_cpu(descr->hw_regs.valid_size)?
+ be32_to_cpu(descr->hw_regs.valid_size) :
+ be32_to_cpu(descr->hw_regs.result_size));
+
+ if (!descr->hw_regs.valid_size) {
+ dev_err(dev, "%s:%d: buffer full %x %x %x\n", __func__,
+ __LINE__,
+ be32_to_cpu(descr->hw_regs.result_size),
+ be32_to_cpu(descr->hw_regs.payload.size),
+ be32_to_cpu(descr->hw_regs.dmac_cmd_status));
+ }
descr->skb = NULL;
/*
@@ -964,9 +1034,10 @@ static void gelic_net_pass_skb_up(struct gelic_descr *descr,
*/
static int gelic_card_decode_one_descr(struct gelic_card *card)
{
- enum gelic_descr_dma_status status;
struct gelic_descr_chain *chain = &card->rx_chain;
struct gelic_descr *descr = chain->head;
+ enum gelic_descr_dma_status status;
+ struct device *dev = ctodev(card);
struct net_device *netdev = NULL;
int dmac_chain_ended;
@@ -976,7 +1047,8 @@ static int gelic_card_decode_one_descr(struct gelic_card *card)
return 0;
if (status == GELIC_DESCR_DMA_NOT_IN_USE) {
- dev_dbg(ctodev(card), "dormant descr? %p\n", descr);
+ dev_dbg(dev, "%s:%d: dormant descr? %px\n", __func__, __LINE__,
+ descr);
return 0;
}
@@ -992,7 +1064,8 @@ static int gelic_card_decode_one_descr(struct gelic_card *card)
}
}
if (GELIC_PORT_MAX <= i) {
- pr_info("%s: unknown packet vid=%x\n", __func__, vid);
+ dev_info(dev, "%s:%d: unknown packet vid=%x\n",
+ __func__, __LINE__, vid);
goto refill;
}
} else
@@ -1001,8 +1074,8 @@ static int gelic_card_decode_one_descr(struct gelic_card *card)
if ((status == GELIC_DESCR_DMA_RESPONSE_ERROR) ||
(status == GELIC_DESCR_DMA_PROTECTION_ERROR) ||
(status == GELIC_DESCR_DMA_FORCE_END)) {
- dev_info(ctodev(card), "dropping RX descriptor with state %x\n",
- status);
+ dev_info(dev, "%s:%d: dropping RX descriptor with state %x\n",
+ __func__, __LINE__, status);
netdev->stats.rx_dropped++;
goto refill;
}
@@ -1017,7 +1090,7 @@ static int gelic_card_decode_one_descr(struct gelic_card *card)
* Anyway this frame was longer than the MTU,
* just drop it.
*/
- dev_info(ctodev(card), "overlength frame\n");
+ dev_info(dev, "%s:%d: overlength frame\n", __func__, __LINE__);
goto refill;
}
/*
@@ -1025,8 +1098,8 @@ static int gelic_card_decode_one_descr(struct gelic_card *card)
* be treated as error.
*/
if (status != GELIC_DESCR_DMA_FRAME_END) {
- dev_dbg(ctodev(card), "RX descriptor with state %x\n",
- status);
+ dev_dbg(dev, "%s:%d: RX descriptor with state %x\n", __func__,
+ __LINE__, status);
goto refill;
}
@@ -1035,15 +1108,14 @@ static int gelic_card_decode_one_descr(struct gelic_card *card)
refill:
/* is the current descriptor terminated with next_descr == NULL? */
- dmac_chain_ended =
- be32_to_cpu(descr->dmac_cmd_status) &
+ dmac_chain_ended = be32_to_cpu(descr->hw_regs.dmac_cmd_status) &
GELIC_DESCR_RX_DMA_CHAIN_END;
/*
* So that always DMAC can see the end
* of the descriptor chain to avoid
* from unwanted DMAC overrun.
*/
- descr->next_descr_addr = 0;
+ descr->hw_regs.next_descr_addr = 0;
/* change the descriptor state: */
gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE);
@@ -1060,15 +1132,17 @@ static int gelic_card_decode_one_descr(struct gelic_card *card)
/*
* Set this descriptor the end of the chain.
*/
- descr->prev->next_descr_addr = cpu_to_be32(descr->bus_addr);
+ descr->prev->hw_regs.next_descr_addr
+ = cpu_to_be32(descr->link.cpu_addr);
/*
* If dmac chain was met, DMAC stopped.
* thus re-enable it
*/
- if (dmac_chain_ended)
+ if (dmac_chain_ended) {
gelic_card_enable_rxdmac(card);
+ }
return 1;
}
@@ -1192,9 +1266,10 @@ void gelic_net_get_drvinfo(struct net_device *netdev,
}
static int gelic_ether_get_link_ksettings(struct net_device *netdev,
- struct ethtool_link_ksettings *cmd)
+ struct ethtool_link_ksettings *cmd)
{
struct gelic_card *card = netdev_card(netdev);
+ struct device *dev = ctodev(card);
u32 supported, advertising;
gelic_card_get_ether_port_status(card, 0);
@@ -1215,16 +1290,17 @@ static int gelic_ether_get_link_ksettings(struct net_device *netdev,
cmd->base.speed = SPEED_1000;
break;
default:
- pr_info("%s: speed unknown\n", __func__);
+ dev_dbg(dev, "%s:%d: speed unknown\n", __func__, __LINE__);
cmd->base.speed = SPEED_10;
break;
}
supported = SUPPORTED_TP | SUPPORTED_Autoneg |
- SUPPORTED_10baseT_Half | SUPPORTED_10baseT_Full |
- SUPPORTED_100baseT_Half | SUPPORTED_100baseT_Full |
- SUPPORTED_1000baseT_Full;
+ SUPPORTED_10baseT_Half | SUPPORTED_10baseT_Full |
+ SUPPORTED_100baseT_Half | SUPPORTED_100baseT_Full |
+ SUPPORTED_1000baseT_Full;
advertising = supported;
+
if (card->link_mode & GELIC_LV1_ETHER_AUTO_NEG) {
cmd->base.autoneg = AUTONEG_ENABLE;
} else {
@@ -1234,9 +1310,9 @@ static int gelic_ether_get_link_ksettings(struct net_device *netdev,
cmd->base.port = PORT_TP;
ethtool_convert_legacy_u32_to_link_mode(cmd->link_modes.supported,
- supported);
+ supported);
ethtool_convert_legacy_u32_to_link_mode(cmd->link_modes.advertising,
- advertising);
+ advertising);
return 0;
}
@@ -1637,130 +1713,134 @@ static void gelic_card_get_vlan_info(struct gelic_card *card)
/*
* ps3_gelic_driver_probe - add a device to the control of this driver
*/
-static int ps3_gelic_driver_probe(struct ps3_system_bus_device *dev)
+static int ps3_gelic_driver_probe(struct ps3_system_bus_device *sb_dev)
{
- struct gelic_card *card;
+ struct device *dev = &sb_dev->core;
struct net_device *netdev;
+ struct gelic_card *card;
int result;
- pr_debug("%s: called\n", __func__);
+ dev_dbg(dev, "%s:%d: >\n", __func__, __LINE__);
udbg_shutdown_ps3gelic();
- result = ps3_open_hv_device(dev);
+ result = ps3_open_hv_device(sb_dev);
if (result) {
- dev_dbg(&dev->core, "%s:ps3_open_hv_device failed\n",
- __func__);
+ dev_err(dev, "%s:%d: ps3_open_hv_device failed: %d\n",
+ __func__, __LINE__, result);
goto fail_open;
}
- result = ps3_dma_region_create(dev->d_region);
+ result = ps3_dma_region_create(sb_dev->d_region);
if (result) {
- dev_dbg(&dev->core, "%s:ps3_dma_region_create failed(%d)\n",
- __func__, result);
+ dev_err(dev, "%s:%d: ps3_dma_region_create failed: %d\n",
+ __func__, __LINE__, result);
BUG_ON("check region type");
goto fail_dma_region;
}
- /* alloc card/netdevice */
card = gelic_alloc_card_net(&netdev);
+
if (!card) {
- dev_info(&dev->core, "%s:gelic_net_alloc_card failed\n",
- __func__);
+ dev_info(dev, "%s:%d: gelic_net_alloc_card failed.\n", __func__,
+ __LINE__);
result = -ENOMEM;
goto fail_alloc_card;
}
- ps3_system_bus_set_drvdata(dev, card);
- card->dev = dev;
- /* get internal vlan info */
+ ps3_system_bus_set_drvdata(sb_dev, card);
+ card->dev = sb_dev;
gelic_card_get_vlan_info(card);
-
card->link_mode = GELIC_LV1_ETHER_AUTO_NEG;
- /* setup interrupt */
result = lv1_net_set_interrupt_status_indicator(bus_id(card),
- dev_id(card),
- ps3_mm_phys_to_lpar(__pa(&card->irq_status)),
- 0);
+ dev_id(card), ps3_mm_phys_to_lpar(__pa(&card->irq_status)), 0);
if (result) {
- dev_dbg(&dev->core,
- "%s:set_interrupt_status_indicator failed: %s\n",
- __func__, ps3_result(result));
+ dev_dbg(dev,
+ "%s:%d: set_interrupt_status_indicator failed: %s\n",
+ __func__, __LINE__, ps3_result(result));
result = -EIO;
goto fail_status_indicator;
}
- result = ps3_sb_event_receive_port_setup(dev, PS3_BINDING_CPU_ANY,
+ result = ps3_sb_event_receive_port_setup(sb_dev, PS3_BINDING_CPU_ANY,
&card->irq);
if (result) {
- dev_info(ctodev(card),
- "%s:gelic_net_open_device failed (%d)\n",
- __func__, result);
+ dev_dbg(dev, "%s:%d: gelic_net_open_device failed: %d\n",
+ __func__, __LINE__, result);
result = -EPERM;
goto fail_alloc_irq;
}
+
result = request_irq(card->irq, gelic_card_interrupt,
0, netdev->name, card);
if (result) {
- dev_info(ctodev(card), "%s:request_irq failed (%d)\n",
- __func__, result);
+ dev_dbg(dev, "%s:%d: request_irq failed: %d\n",
+ __func__, __LINE__, result);
goto fail_request_irq;
}
- /* setup card structure */
card->irq_mask = GELIC_CARD_RXINT | GELIC_CARD_TXINT |
GELIC_CARD_PORT_STATUS_CHANGED;
+ result = gelic_card_init_chain(card, &card->tx_chain, card->descr,
+ GELIC_NET_TX_DESCRIPTORS);
- result = gelic_card_init_chain(card, &card->tx_chain,
- card->descr, GELIC_NET_TX_DESCRIPTORS);
- if (result)
+ if (result) {
goto fail_alloc_tx;
+ }
+
result = gelic_card_init_chain(card, &card->rx_chain,
- card->descr + GELIC_NET_TX_DESCRIPTORS,
- GELIC_NET_RX_DESCRIPTORS);
- if (result)
+ card->descr + GELIC_NET_TX_DESCRIPTORS,
+ GELIC_NET_RX_DESCRIPTORS);
+
+ if (result) {
goto fail_alloc_rx;
+ }
- /* head of chain */
card->tx_top = card->tx_chain.head;
card->rx_top = card->rx_chain.head;
- dev_dbg(ctodev(card), "descr rx %p, tx %p, size %#lx, num %#x\n",
- card->rx_top, card->tx_top, sizeof(struct gelic_descr),
- GELIC_NET_RX_DESCRIPTORS);
- /* allocate rx skbs */
+
+ dev_dbg(dev, "%s:%d: descr rx %px, tx %px, size %#lx, num %#x\n",
+ __func__, __LINE__, card->rx_top, card->tx_top,
+ sizeof(struct gelic_descr), GELIC_NET_RX_DESCRIPTORS);
+
result = gelic_card_alloc_rx_skbs(card);
- if (result)
+
+ if (result) {
goto fail_alloc_skbs;
+ }
spin_lock_init(&card->tx_lock);
card->tx_dma_progress = 0;
- /* setup net_device structure */
netdev->irq = card->irq;
- SET_NETDEV_DEV(netdev, &card->dev->core);
+ SET_NETDEV_DEV(netdev, dev);
gelic_ether_setup_netdev_ops(netdev, &card->napi);
+
result = gelic_net_setup_netdev(netdev, card);
+
if (result) {
- dev_dbg(&dev->core, "%s: setup_netdev failed %d\n",
- __func__, result);
+ dev_err(dev, "%s:%d: setup_netdev failed: %d\n", __func__,
+ __LINE__, result);
goto fail_setup_netdev;
}
#ifdef CONFIG_GELIC_WIRELESS
result = gelic_wl_driver_probe(card);
+
if (result) {
- dev_dbg(&dev->core, "%s: WL init failed\n", __func__);
+ dev_dbg(dev, "%s:%d: WL init failed\n", __func__, __LINE__);
goto fail_setup_netdev;
}
#endif
- pr_debug("%s: done\n", __func__);
+
+ dev_dbg(dev, "%s:%d: < OK\n", __func__, __LINE__);
return 0;
fail_setup_netdev:
@@ -1772,20 +1852,19 @@ static int ps3_gelic_driver_probe(struct ps3_system_bus_device *dev)
free_irq(card->irq, card);
netdev->irq = 0;
fail_request_irq:
- ps3_sb_event_receive_port_destroy(dev, card->irq);
+ ps3_sb_event_receive_port_destroy(sb_dev, card->irq);
fail_alloc_irq:
- lv1_net_set_interrupt_status_indicator(bus_id(card),
- bus_id(card),
- 0, 0);
+ lv1_net_set_interrupt_status_indicator(bus_id(card), bus_id(card), 0, 0);
fail_status_indicator:
- ps3_system_bus_set_drvdata(dev, NULL);
+ ps3_system_bus_set_drvdata(sb_dev, NULL);
kfree(netdev_card(netdev)->unalign);
free_netdev(netdev);
fail_alloc_card:
- ps3_dma_region_free(dev->d_region);
+ ps3_dma_region_free(sb_dev->d_region);
fail_dma_region:
- ps3_close_hv_device(dev);
+ ps3_close_hv_device(sb_dev);
fail_open:
+ dev_dbg(dev, "%s:%d: < error\n", __func__, __LINE__);
return result;
}
diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.h b/drivers/net/ethernet/toshiba/ps3_gelic_net.h
index 68f324ed4eaf..fb0d314d53e5 100644
--- a/drivers/net/ethernet/toshiba/ps3_gelic_net.h
+++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.h
@@ -220,29 +220,35 @@ enum gelic_lv1_phy {
GELIC_LV1_PHY_ETHERNET_0 = 0x0000000000000002L,
};
-/* size of hardware part of gelic descriptor */
-#define GELIC_DESCR_SIZE (32)
-
enum gelic_port_type {
GELIC_PORT_ETHERNET_0 = 0,
GELIC_PORT_WIRELESS = 1,
GELIC_PORT_MAX
};
-struct gelic_descr {
- /* as defined by the hardware */
- __be32 buf_addr;
- __be32 buf_size;
+/* as defined by the hardware */
+struct gelic_hw_regs {
+ struct {
+ __be32 dev_addr;
+ __be32 size;
+ } __attribute__ ((packed)) payload;
__be32 next_descr_addr;
__be32 dmac_cmd_status;
__be32 result_size;
__be32 valid_size; /* all zeroes for tx */
__be32 data_status;
__be32 data_error; /* all zeroes for tx */
+} __attribute__ ((packed));
- /* used in the driver */
+struct gelic_chain_link {
+ dma_addr_t cpu_addr;
+ unsigned int size;
+};
+
+struct gelic_descr {
+ struct gelic_hw_regs hw_regs;
+ struct gelic_chain_link link;
struct sk_buff *skb;
- dma_addr_t bus_addr;
struct gelic_descr *next;
struct gelic_descr *prev;
} __attribute__((aligned(32)));
--
2.25.1
^ permalink raw reply related
* [PATCH] powerpc/perf: Enable PMU counters post partition migration if PMU is active
From: Athira Rajeev @ 2021-07-11 7:06 UTC (permalink / raw)
To: mpe; +Cc: kjain, maddy, linuxppc-dev, rnsastry
During Live Partition Migration (LPM), it is observed that after
migration completion, perf counter values reports 0 incase of
system/cpu wide monitoring. However 'perf stat' with workload
continues to show counts post migration since PMU gets disabled/enabled
during sched switches.
Example:
./perf stat -e r1001e -I 1000
time counts unit events
1.001010437 22,137,414 r1001e
2.002495447 15,455,821 r1001e
<<>> As seen in next below logs, the counter values shows zero
after migration is completed.
<<>>
86.142535370 129,392,333,440 r1001e
87.144714617 0 r1001e
88.146526636 0 r1001e
89.148085029 0 r1001e
Here PMU is enabled during start of perf session and counter
values are read at intervals. Counters are only disabled at the
end of session. The powerpc mobility code presently does not handle
disabling and enabling back of PMU counters during partition
migration. Also since the PMU register values are not saved/restored
during migration, PMU registers like Monitor Mode Control Register 0
(MMCR0), Monitor Mode Control Register 1 (MMCR1) will not contain
the value it was programmed with. Hence PMU counters will not be
enabled correctly post migration.
Fix this in mobility code by handling disabling and enabling of
PMU in all cpu's before and after migration. Patch introduces two
functions 'mobility_pmu_disable' and 'mobility_pmu_enable'.
mobility_pmu_disable() is called before the processor threads goes
to suspend state so as to disable the PMU counters. And disable is
done only if there are any active events running on that cpu.
mobility_pmu_enable() is called after the processor threads are
back online to enable back the PMU counters.
Since the performance Monitor counters ( PMCs) are not
saved/restored during LPM, results in PMC value being zero and the
'event->hw.prev_count' being non-zero value. This causes problem
during updation of event->count since we always accumulate
(event->hw.prev_count - PMC value) in event->count. If
event->hw.prev_count is greater PMC value, event->count becomes
negative. Fix this by re-initialising 'prev_count' also for all
events while enabling back the events. A new variable 'migrate' is
introduced in 'struct cpu_hw_event' to achieve this for LPM cases
in power_pmu_enable. Use the 'migrate' value to clear the PMC
index (stored in event->hw.idx) for all events so that event
count settings will get re-initialised correctly.
Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
---
arch/powerpc/include/asm/rtas.h | 4 +++
arch/powerpc/perf/core-book3s.c | 43 ++++++++++++++++++++++++++++---
arch/powerpc/platforms/pseries/mobility.c | 4 +++
3 files changed, 48 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h
index 9dc97d2..3fc478a 100644
--- a/arch/powerpc/include/asm/rtas.h
+++ b/arch/powerpc/include/asm/rtas.h
@@ -376,8 +376,12 @@ static inline void rtas_initialize(void) { }
#ifdef CONFIG_HV_PERF_CTRS
void read_24x7_sys_info(void);
+void mobility_pmu_disable(void);
+void mobility_pmu_enable(void);
#else
static inline void read_24x7_sys_info(void) { }
+static inline void mobility_pmu_disable(void) { }
+static inline void mobility_pmu_enable(void) { }
#endif
#endif /* __KERNEL__ */
diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index bb0ee71..e86df45 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -58,6 +58,7 @@ struct cpu_hw_events {
/* Store the PMC values */
unsigned long pmcs[MAX_HWEVENTS];
+ int migrate;
};
static DEFINE_PER_CPU(struct cpu_hw_events, cpu_hw_events);
@@ -1335,6 +1336,22 @@ static void power_pmu_disable(struct pmu *pmu)
}
/*
+ * Called from powerpc mobility code
+ * before migration to disable counters
+ * if the PMU is active.
+ */
+void mobility_pmu_disable(void)
+{
+ struct cpu_hw_events *cpuhw;
+
+ cpuhw = this_cpu_ptr(&cpu_hw_events);
+ if (cpuhw->n_events != 0) {
+ power_pmu_disable(NULL);
+ cpuhw->migrate = 1;
+ }
+}
+
+/*
* Re-enable all events if disable == 0.
* If we were previously disabled and events were added, then
* put the new config on the PMU.
@@ -1379,8 +1396,10 @@ static void power_pmu_enable(struct pmu *pmu)
* no need to recalculate MMCR* settings and reset the PMCs.
* Just reenable the PMU with the current MMCR* settings
* (possibly updated for removal of events).
+ * While reenabling PMU during partition migration, continue
+ * with normal flow.
*/
- if (!cpuhw->n_added) {
+ if (!cpuhw->n_added && !cpuhw->migrate) {
mtspr(SPRN_MMCRA, cpuhw->mmcr.mmcra & ~MMCRA_SAMPLE_ENABLE);
mtspr(SPRN_MMCR1, cpuhw->mmcr.mmcr1);
if (ppmu->flags & PPMU_ARCH_31)
@@ -1434,11 +1453,15 @@ static void power_pmu_enable(struct pmu *pmu)
/*
* Read off any pre-existing events that need to move
* to another PMC.
+ * While enabling PMU during partition migration,
+ * skip power_pmu_read since all event count settings
+ * needs to be re-initialised after migration.
*/
for (i = 0; i < cpuhw->n_events; ++i) {
event = cpuhw->event[i];
- if (event->hw.idx && event->hw.idx != hwc_index[i] + 1) {
- power_pmu_read(event);
+ if ((event->hw.idx && event->hw.idx != hwc_index[i] + 1) || (cpuhw->migrate)) {
+ if (!cpuhw->migrate)
+ power_pmu_read(event);
write_pmc(event->hw.idx, 0);
event->hw.idx = 0;
}
@@ -1506,6 +1529,20 @@ static void power_pmu_enable(struct pmu *pmu)
local_irq_restore(flags);
}
+/*
+ * Called from powerpc mobility code
+ * during migration completion to
+ * enable back PMU counters.
+ */
+void mobility_pmu_enable(void)
+{
+ struct cpu_hw_events *cpuhw;
+
+ cpuhw = this_cpu_ptr(&cpu_hw_events);
+ power_pmu_enable(NULL);
+ cpuhw->migrate = 0;
+}
+
static int collect_events(struct perf_event *group, int max_count,
struct perf_event *ctrs[], u64 *events,
unsigned int *flags)
diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
index e83e089..ff7a77c 100644
--- a/arch/powerpc/platforms/pseries/mobility.c
+++ b/arch/powerpc/platforms/pseries/mobility.c
@@ -476,6 +476,8 @@ static int do_join(void *arg)
retry:
/* Must ensure MSR.EE off for H_JOIN. */
hard_irq_disable();
+ /* Disable PMU before suspend */
+ mobility_pmu_disable();
hvrc = plpar_hcall_norets(H_JOIN);
switch (hvrc) {
@@ -530,6 +532,8 @@ static int do_join(void *arg)
* reset the watchdog.
*/
touch_nmi_watchdog();
+ /* Enable PMU after resuming */
+ mobility_pmu_enable();
return ret;
}
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH] powerpc/perf: Enable PMU counters post partition migration if PMU is active
From: kernel test robot @ 2021-07-11 9:05 UTC (permalink / raw)
To: Athira Rajeev, mpe; +Cc: kjain, maddy, kbuild-all, linuxppc-dev, rnsastry
In-Reply-To: <1625987205-1649-1-git-send-email-atrajeev@linux.vnet.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 8601 bytes --]
Hi Athira,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on powerpc/next]
[also build test WARNING on v5.13 next-20210709]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Athira-Rajeev/powerpc-perf-Enable-PMU-counters-post-partition-migration-if-PMU-is-active/20210711-150741
base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-allyesconfig (attached as .config)
compiler: powerpc64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/2050c82afb3abd9eaa57fee45e71e7fccabfb81f
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Athira-Rajeev/powerpc-perf-Enable-PMU-counters-post-partition-migration-if-PMU-is-active/20210711-150741
git checkout 2050c82afb3abd9eaa57fee45e71e7fccabfb81f
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=powerpc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> arch/powerpc/perf/core-book3s.c:1343:6: warning: no previous prototype for 'mobility_pmu_disable' [-Wmissing-prototypes]
1343 | void mobility_pmu_disable(void)
| ^~~~~~~~~~~~~~~~~~~~
>> arch/powerpc/perf/core-book3s.c:1537:6: warning: no previous prototype for 'mobility_pmu_enable' [-Wmissing-prototypes]
1537 | void mobility_pmu_enable(void)
| ^~~~~~~~~~~~~~~~~~~
vim +/mobility_pmu_disable +1343 arch/powerpc/perf/core-book3s.c
1337
1338 /*
1339 * Called from powerpc mobility code
1340 * before migration to disable counters
1341 * if the PMU is active.
1342 */
> 1343 void mobility_pmu_disable(void)
1344 {
1345 struct cpu_hw_events *cpuhw;
1346
1347 cpuhw = this_cpu_ptr(&cpu_hw_events);
1348 if (cpuhw->n_events != 0) {
1349 power_pmu_disable(NULL);
1350 cpuhw->migrate = 1;
1351 }
1352 }
1353
1354 /*
1355 * Re-enable all events if disable == 0.
1356 * If we were previously disabled and events were added, then
1357 * put the new config on the PMU.
1358 */
1359 static void power_pmu_enable(struct pmu *pmu)
1360 {
1361 struct perf_event *event;
1362 struct cpu_hw_events *cpuhw;
1363 unsigned long flags;
1364 long i;
1365 unsigned long val, mmcr0;
1366 s64 left;
1367 unsigned int hwc_index[MAX_HWEVENTS];
1368 int n_lim;
1369 int idx;
1370 bool ebb;
1371
1372 if (!ppmu)
1373 return;
1374 local_irq_save(flags);
1375
1376 cpuhw = this_cpu_ptr(&cpu_hw_events);
1377 if (!cpuhw->disabled)
1378 goto out;
1379
1380 if (cpuhw->n_events == 0) {
1381 ppc_set_pmu_inuse(0);
1382 goto out;
1383 }
1384
1385 cpuhw->disabled = 0;
1386
1387 /*
1388 * EBB requires an exclusive group and all events must have the EBB
1389 * flag set, or not set, so we can just check a single event. Also we
1390 * know we have at least one event.
1391 */
1392 ebb = is_ebb_event(cpuhw->event[0]);
1393
1394 /*
1395 * If we didn't change anything, or only removed events,
1396 * no need to recalculate MMCR* settings and reset the PMCs.
1397 * Just reenable the PMU with the current MMCR* settings
1398 * (possibly updated for removal of events).
1399 * While reenabling PMU during partition migration, continue
1400 * with normal flow.
1401 */
1402 if (!cpuhw->n_added && !cpuhw->migrate) {
1403 mtspr(SPRN_MMCRA, cpuhw->mmcr.mmcra & ~MMCRA_SAMPLE_ENABLE);
1404 mtspr(SPRN_MMCR1, cpuhw->mmcr.mmcr1);
1405 if (ppmu->flags & PPMU_ARCH_31)
1406 mtspr(SPRN_MMCR3, cpuhw->mmcr.mmcr3);
1407 goto out_enable;
1408 }
1409
1410 /*
1411 * Clear all MMCR settings and recompute them for the new set of events.
1412 */
1413 memset(&cpuhw->mmcr, 0, sizeof(cpuhw->mmcr));
1414
1415 if (ppmu->compute_mmcr(cpuhw->events, cpuhw->n_events, hwc_index,
1416 &cpuhw->mmcr, cpuhw->event, ppmu->flags)) {
1417 /* shouldn't ever get here */
1418 printk(KERN_ERR "oops compute_mmcr failed\n");
1419 goto out;
1420 }
1421
1422 if (!(ppmu->flags & PPMU_ARCH_207S)) {
1423 /*
1424 * Add in MMCR0 freeze bits corresponding to the attr.exclude_*
1425 * bits for the first event. We have already checked that all
1426 * events have the same value for these bits as the first event.
1427 */
1428 event = cpuhw->event[0];
1429 if (event->attr.exclude_user)
1430 cpuhw->mmcr.mmcr0 |= MMCR0_FCP;
1431 if (event->attr.exclude_kernel)
1432 cpuhw->mmcr.mmcr0 |= freeze_events_kernel;
1433 if (event->attr.exclude_hv)
1434 cpuhw->mmcr.mmcr0 |= MMCR0_FCHV;
1435 }
1436
1437 /*
1438 * Write the new configuration to MMCR* with the freeze
1439 * bit set and set the hardware events to their initial values.
1440 * Then unfreeze the events.
1441 */
1442 ppc_set_pmu_inuse(1);
1443 mtspr(SPRN_MMCRA, cpuhw->mmcr.mmcra & ~MMCRA_SAMPLE_ENABLE);
1444 mtspr(SPRN_MMCR1, cpuhw->mmcr.mmcr1);
1445 mtspr(SPRN_MMCR0, (cpuhw->mmcr.mmcr0 & ~(MMCR0_PMC1CE | MMCR0_PMCjCE))
1446 | MMCR0_FC);
1447 if (ppmu->flags & PPMU_ARCH_207S)
1448 mtspr(SPRN_MMCR2, cpuhw->mmcr.mmcr2);
1449
1450 if (ppmu->flags & PPMU_ARCH_31)
1451 mtspr(SPRN_MMCR3, cpuhw->mmcr.mmcr3);
1452
1453 /*
1454 * Read off any pre-existing events that need to move
1455 * to another PMC.
1456 * While enabling PMU during partition migration,
1457 * skip power_pmu_read since all event count settings
1458 * needs to be re-initialised after migration.
1459 */
1460 for (i = 0; i < cpuhw->n_events; ++i) {
1461 event = cpuhw->event[i];
1462 if ((event->hw.idx && event->hw.idx != hwc_index[i] + 1) || (cpuhw->migrate)) {
1463 if (!cpuhw->migrate)
1464 power_pmu_read(event);
1465 write_pmc(event->hw.idx, 0);
1466 event->hw.idx = 0;
1467 }
1468 }
1469
1470 /*
1471 * Initialize the PMCs for all the new and moved events.
1472 */
1473 cpuhw->n_limited = n_lim = 0;
1474 for (i = 0; i < cpuhw->n_events; ++i) {
1475 event = cpuhw->event[i];
1476 if (event->hw.idx)
1477 continue;
1478 idx = hwc_index[i] + 1;
1479 if (is_limited_pmc(idx)) {
1480 cpuhw->limited_counter[n_lim] = event;
1481 cpuhw->limited_hwidx[n_lim] = idx;
1482 ++n_lim;
1483 continue;
1484 }
1485
1486 if (ebb)
1487 val = local64_read(&event->hw.prev_count);
1488 else {
1489 val = 0;
1490 if (event->hw.sample_period) {
1491 left = local64_read(&event->hw.period_left);
1492 if (left < 0x80000000L)
1493 val = 0x80000000L - left;
1494 }
1495 local64_set(&event->hw.prev_count, val);
1496 }
1497
1498 event->hw.idx = idx;
1499 if (event->hw.state & PERF_HES_STOPPED)
1500 val = 0;
1501 write_pmc(idx, val);
1502
1503 perf_event_update_userpage(event);
1504 }
1505 cpuhw->n_limited = n_lim;
1506 cpuhw->mmcr.mmcr0 |= MMCR0_PMXE | MMCR0_FCECE;
1507
1508 out_enable:
1509 pmao_restore_workaround(ebb);
1510
1511 mmcr0 = ebb_switch_in(ebb, cpuhw);
1512
1513 mb();
1514 if (cpuhw->bhrb_users)
1515 ppmu->config_bhrb(cpuhw->bhrb_filter);
1516
1517 write_mmcr0(cpuhw, mmcr0);
1518
1519 /*
1520 * Enable instruction sampling if necessary
1521 */
1522 if (cpuhw->mmcr.mmcra & MMCRA_SAMPLE_ENABLE) {
1523 mb();
1524 mtspr(SPRN_MMCRA, cpuhw->mmcr.mmcra);
1525 }
1526
1527 out:
1528
1529 local_irq_restore(flags);
1530 }
1531
1532 /*
1533 * Called from powerpc mobility code
1534 * during migration completion to
1535 * enable back PMU counters.
1536 */
> 1537 void mobility_pmu_enable(void)
1538 {
1539 struct cpu_hw_events *cpuhw;
1540
1541 cpuhw = this_cpu_ptr(&cpu_hw_events);
1542 power_pmu_enable(NULL);
1543 cpuhw->migrate = 0;
1544 }
1545
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 73240 bytes --]
^ permalink raw reply
* [PATCH] arch:powerpc:mm: cleanup __FUNCTION__ usage
From: Dwaipayan Ray @ 2021-07-11 8:45 UTC (permalink / raw)
To: mpe, benh, paulus
Cc: lukas.bulwahn, linuxppc-dev, linux-kernel, Dwaipayan Ray
__FUNCTION__ exists only for backwards compatibility reasons
with old gcc versions. Replace it with __func__.
Signed-off-by: Dwaipayan Ray <dwaipayanray1@gmail.com>
---
arch/powerpc/mm/numa.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
index f2bf98bdcea2..3d3f570c925c 100644
--- a/arch/powerpc/mm/numa.c
+++ b/arch/powerpc/mm/numa.c
@@ -1236,7 +1236,7 @@ int find_and_online_cpu_nid(int cpu)
#endif
}
- pr_debug("%s:%d cpu %d nid %d\n", __FUNCTION__, __LINE__,
+ pr_debug("%s:%d cpu %d nid %d\n", __func__, __LINE__,
cpu, new_nid);
return new_nid;
}
--
2.28.0
^ permalink raw reply related
* [PATCH] arch:powerpc:platforms:powernv: cleanup __FUNCTION__ usage
From: Dwaipayan Ray @ 2021-07-11 8:48 UTC (permalink / raw)
To: mpe, benh, paulus
Cc: lukas.bulwahn, linuxppc-dev, linux-kernel, Dwaipayan Ray
__FUNCTION__ exists only for backwards compatibility reasons
with old gcc versions. Replace it with __func__.
Signed-off-by: Dwaipayan Ray <dwaipayanray1@gmail.com>
---
arch/powerpc/platforms/powernv/opal-imc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/powernv/opal-imc.c b/arch/powerpc/platforms/powernv/opal-imc.c
index 7824cc364bc4..5bafbf34e820 100644
--- a/arch/powerpc/platforms/powernv/opal-imc.c
+++ b/arch/powerpc/platforms/powernv/opal-imc.c
@@ -211,7 +211,7 @@ static void disable_core_pmu_counters(void)
get_hard_smp_processor_id(cpu));
if (rc)
pr_err("%s: Failed to stop Core (cpu = %d)\n",
- __FUNCTION__, cpu);
+ __func__, cpu);
}
put_online_cpus();
}
--
2.28.0
^ permalink raw reply related
* [PATCH V2] powerpc/perf: Enable PMU counters post partition migration if PMU is active
From: Athira Rajeev @ 2021-07-11 12:25 UTC (permalink / raw)
To: mpe; +Cc: kjain, maddy, linuxppc-dev, rnsastry
During Live Partition Migration (LPM), it is observed that perf
counter values reports zero post migration completion. However
'perf stat' with workload continues to show counts post migration
since PMU gets disabled/enabled during sched switches. But incase
of system/cpu wide monitoring, zero counts were reported with 'perf
stat' after migration completion.
Example:
./perf stat -e r1001e -I 1000
time counts unit events
1.001010437 22,137,414 r1001e
2.002495447 15,455,821 r1001e
<<>> As seen in next below logs, the counter values shows zero
after migration is completed.
<<>>
86.142535370 129,392,333,440 r1001e
87.144714617 0 r1001e
88.146526636 0 r1001e
89.148085029 0 r1001e
Here PMU is enabled during start of perf session and counter
values are read at intervals. Counters are only disabled at the
end of session. The powerpc mobility code presently does not handle
disabling and enabling back of PMU counters during partition
migration. Also since the PMU register values are not saved/restored
during migration, PMU registers like Monitor Mode Control Register 0
(MMCR0), Monitor Mode Control Register 1 (MMCR1) will not contain
the value it was programmed with. Hence PMU counters will not be
enabled correctly post migration.
Fix this in mobility code by handling disabling and enabling of
PMU in all cpu's before and after migration. Patch introduces two
functions 'mobility_pmu_disable' and 'mobility_pmu_enable'.
mobility_pmu_disable() is called before the processor threads goes
to suspend state so as to disable the PMU counters. And disable is
done only if there are any active events running on that cpu.
mobility_pmu_enable() is called after the processor threads are
back online to enable back the PMU counters.
Since the performance Monitor counters ( PMCs) are not
saved/restored during LPM, results in PMC value being zero and the
'event->hw.prev_count' being non-zero value. This causes problem
during updation of event->count since we always accumulate
(event->hw.prev_count - PMC value) in event->count. If
event->hw.prev_count is greater PMC value, event->count becomes
negative. Fix this by re-initialising 'prev_count' also for all
events while enabling back the events. A new variable 'migrate' is
introduced in 'struct cpu_hw_event' to achieve this for LPM cases
in power_pmu_enable. Use the 'migrate' value to clear the PMC
index (stored in event->hw.idx) for all events so that event
count settings will get re-initialised correctly.
Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
[ Fixed compilation error reported by kernel test robot ]
Reported-by: kernel test robot <lkp@intel.com>
---
Change from v1 -> v2:
- Moved the mobility_pmu_enable and mobility_pmu_disable
declarations under CONFIG_PPC_PERF_CTRS in rtas.h.
Also included 'asm/rtas.h' in core-book3s to fix the
compilation warning reported by kernel test robot.
arch/powerpc/include/asm/rtas.h | 8 ++++++
arch/powerpc/perf/core-book3s.c | 44 ++++++++++++++++++++++++++++---
arch/powerpc/platforms/pseries/mobility.c | 4 +++
3 files changed, 53 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h
index 9dc97d2..cea72d7 100644
--- a/arch/powerpc/include/asm/rtas.h
+++ b/arch/powerpc/include/asm/rtas.h
@@ -380,5 +380,13 @@ static inline void rtas_initialize(void) { }
static inline void read_24x7_sys_info(void) { }
#endif
+#ifdef CONFIG_PPC_PERF_CTRS
+void mobility_pmu_disable(void);
+void mobility_pmu_enable(void);
+#else
+static inline void mobility_pmu_disable(void) { }
+static inline void mobility_pmu_enable(void) { }
+#endif
+
#endif /* __KERNEL__ */
#endif /* _POWERPC_RTAS_H */
diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index bb0ee71..90da7fa 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -18,6 +18,7 @@
#include <asm/ptrace.h>
#include <asm/code-patching.h>
#include <asm/interrupt.h>
+#include <asm/rtas.h>
#ifdef CONFIG_PPC64
#include "internal.h"
@@ -58,6 +59,7 @@ struct cpu_hw_events {
/* Store the PMC values */
unsigned long pmcs[MAX_HWEVENTS];
+ int migrate;
};
static DEFINE_PER_CPU(struct cpu_hw_events, cpu_hw_events);
@@ -1335,6 +1337,22 @@ static void power_pmu_disable(struct pmu *pmu)
}
/*
+ * Called from powerpc mobility code
+ * before migration to disable counters
+ * if the PMU is active.
+ */
+void mobility_pmu_disable(void)
+{
+ struct cpu_hw_events *cpuhw;
+
+ cpuhw = this_cpu_ptr(&cpu_hw_events);
+ if (cpuhw->n_events != 0) {
+ power_pmu_disable(NULL);
+ cpuhw->migrate = 1;
+ }
+}
+
+/*
* Re-enable all events if disable == 0.
* If we were previously disabled and events were added, then
* put the new config on the PMU.
@@ -1379,8 +1397,10 @@ static void power_pmu_enable(struct pmu *pmu)
* no need to recalculate MMCR* settings and reset the PMCs.
* Just reenable the PMU with the current MMCR* settings
* (possibly updated for removal of events).
+ * While reenabling PMU during partition migration, continue
+ * with normal flow.
*/
- if (!cpuhw->n_added) {
+ if (!cpuhw->n_added && !cpuhw->migrate) {
mtspr(SPRN_MMCRA, cpuhw->mmcr.mmcra & ~MMCRA_SAMPLE_ENABLE);
mtspr(SPRN_MMCR1, cpuhw->mmcr.mmcr1);
if (ppmu->flags & PPMU_ARCH_31)
@@ -1434,11 +1454,15 @@ static void power_pmu_enable(struct pmu *pmu)
/*
* Read off any pre-existing events that need to move
* to another PMC.
+ * While enabling PMU during partition migration,
+ * skip power_pmu_read since all event count settings
+ * needs to be re-initialised after migration.
*/
for (i = 0; i < cpuhw->n_events; ++i) {
event = cpuhw->event[i];
- if (event->hw.idx && event->hw.idx != hwc_index[i] + 1) {
- power_pmu_read(event);
+ if ((event->hw.idx && event->hw.idx != hwc_index[i] + 1) || (cpuhw->migrate)) {
+ if (!cpuhw->migrate)
+ power_pmu_read(event);
write_pmc(event->hw.idx, 0);
event->hw.idx = 0;
}
@@ -1506,6 +1530,20 @@ static void power_pmu_enable(struct pmu *pmu)
local_irq_restore(flags);
}
+/*
+ * Called from powerpc mobility code
+ * during migration completion to
+ * enable back PMU counters.
+ */
+void mobility_pmu_enable(void)
+{
+ struct cpu_hw_events *cpuhw;
+
+ cpuhw = this_cpu_ptr(&cpu_hw_events);
+ power_pmu_enable(NULL);
+ cpuhw->migrate = 0;
+}
+
static int collect_events(struct perf_event *group, int max_count,
struct perf_event *ctrs[], u64 *events,
unsigned int *flags)
diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
index e83e089..ff7a77c 100644
--- a/arch/powerpc/platforms/pseries/mobility.c
+++ b/arch/powerpc/platforms/pseries/mobility.c
@@ -476,6 +476,8 @@ static int do_join(void *arg)
retry:
/* Must ensure MSR.EE off for H_JOIN. */
hard_irq_disable();
+ /* Disable PMU before suspend */
+ mobility_pmu_disable();
hvrc = plpar_hcall_norets(H_JOIN);
switch (hvrc) {
@@ -530,6 +532,8 @@ static int do_join(void *arg)
* reset the watchdog.
*/
touch_nmi_watchdog();
+ /* Enable PMU after resuming */
+ mobility_pmu_enable();
return ret;
}
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH v3 1/2] net/ps3_gelic: Add gelic_descr structures
From: Christophe Leroy @ 2021-07-11 14:03 UTC (permalink / raw)
To: Geoff Levand; +Cc: netdev, linuxppc-dev, David S. Miller, Jakub Kicinski
In-Reply-To: <e63dc0564b7c6e4f699c306bf36feb4bd9c30f26.1625976141.git.geoff@infradead.org>
Geoff Levand <geoff@infradead.org> a écrit :
> Create two new structures, struct gelic_hw_regs and struct gelic_chain_link,
> and replace the corresponding members of struct gelic_descr with the new
> structures. struct gelic_hw_regs holds the register variables used by the
> gelic hardware device. struct gelic_chain_link holds variables used to
> manage the driver's linked list of gelic descr structures.
>
> Fixes several DMA mapping problems with the PS3's gelic network driver:
>
> * Change from checking the return value of dma_map_single to using the
> dma_mapping_error routine.
> * Use the correct buffer length when mapping the RX skb.
> * Improved error checking and debug logging.
Your patch has a lot of cosmetic changes. Several of them are just
wrong. The other ones belong to another patch. This patch should focus
only on the changes it targets.
Your patch is way too big and addresses several different topics.
Should be split in several patches.
I suggest you run checkpatch.pl --strict on your patch
>
> Fixes runtime errors like these, and also other randomly occurring errors:
>
> IP-Config: Complete:
> DMA-API: ps3_gelic_driver sb_05: device driver failed to check map error
> WARNING: CPU: 0 PID: 0 at kernel/dma/debug.c:1027 .check_unmap+0x888/0x8dc
>
> Signed-off-by: Geoff Levand <geoff@infradead.org>
> ---
> drivers/net/ethernet/toshiba/ps3_gelic_net.c | 573 +++++++++++--------
> drivers/net/ethernet/toshiba/ps3_gelic_net.h | 24 +-
> 2 files changed, 341 insertions(+), 256 deletions(-)
>
> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> index 55e652624bd7..e01938128882 100644
> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> @@ -96,9 +96,11 @@ static void
> gelic_card_get_ether_port_status(struct gelic_card *card,
> * returns the status as in the dmac_cmd_status field of the descriptor
> */
> static enum gelic_descr_dma_status
> +
This blank line is pointless and misleading
> gelic_descr_get_status(struct gelic_descr *descr)
> {
> - return be32_to_cpu(descr->dmac_cmd_status) & GELIC_DESCR_DMA_STAT_MASK;
> + return be32_to_cpu(descr->hw_regs.dmac_cmd_status)
> + & GELIC_DESCR_DMA_STAT_MASK;
The & should be at the end of previous line and the second line should
be aligned to the open parenthesis
> }
>
> static int gelic_card_set_link_mode(struct gelic_card *card, int mode)
> @@ -146,24 +148,34 @@ static void gelic_card_disable_txdmac(struct
> gelic_card *card)
> */
> static void gelic_card_enable_rxdmac(struct gelic_card *card)
> {
> + struct device *dev = ctodev(card);
> int status;
> +#if defined(DEBUG)
> + static const int debug_build = 1;
> +#else
> + static const int debug_build = 0;
> +#endif
Useless
You can directly use __is_defined(DEBUG) below
>
> -#ifdef DEBUG
> - if (gelic_descr_get_status(card->rx_chain.head) !=
> - GELIC_DESCR_DMA_CARDOWNED) {
> - printk(KERN_ERR "%s: status=%x\n", __func__,
> - be32_to_cpu(card->rx_chain.head->dmac_cmd_status));
> - printk(KERN_ERR "%s: nextphy=%x\n", __func__,
> - be32_to_cpu(card->rx_chain.head->next_descr_addr));
> - printk(KERN_ERR "%s: head=%p\n", __func__,
> - card->rx_chain.head);
> + if (debug_build
&& must be at the en of first line
> + && (gelic_descr_get_status(card->rx_chain.head)
> + != GELIC_DESCR_DMA_CARDOWNED)) {
!= must be at end of previous line
> + dev_err(dev, "%s:%d: status=%x\n", __func__, __LINE__,
> + be32_to_cpu(
> + card->rx_chain.head->hw_regs.dmac_cmd_status));
Alignment should match open parentesis. And lines can be 100 chars
long when nécessary
> + dev_err(dev, "%s:%d: nextphy=%x\n", __func__, __LINE__,
> + be32_to_cpu(
> + card->rx_chain.head->hw_regs.next_descr_addr));
> + dev_err(dev, "%s:%d: head=%px\n", __func__, __LINE__,
> + card->rx_chain.head);
> }
> -#endif
> +
> status = lv1_net_start_rx_dma(bus_id(card), dev_id(card),
> - card->rx_chain.head->bus_addr, 0);
> - if (status)
> - dev_info(ctodev(card),
> - "lv1_net_start_rx_dma failed, status=%d\n", status);
> + card->rx_chain.head->link.cpu_addr, 0);
> +
> + if (status) {
No { } for single lines
> + dev_err(dev, "%s:%d: lv1_net_start_rx_dma failed: %d\n",
> + __func__, __LINE__, status);
> + }
> }
>
> /**
> @@ -193,11 +205,11 @@ static void gelic_card_disable_rxdmac(struct
> gelic_card *card)
> * in the status
> */
> static void gelic_descr_set_status(struct gelic_descr *descr,
> - enum gelic_descr_dma_status status)
> + enum gelic_descr_dma_status status)
> {
> - descr->dmac_cmd_status = cpu_to_be32(status |
> - (be32_to_cpu(descr->dmac_cmd_status) &
> - ~GELIC_DESCR_DMA_STAT_MASK));
> + descr->hw_regs.dmac_cmd_status = cpu_to_be32(status
> + | (be32_to_cpu(descr->hw_regs.dmac_cmd_status)
> + & ~GELIC_DESCR_DMA_STAT_MASK));
> /*
> * dma_cmd_status field is used to indicate whether the descriptor
> * is valid or not.
> @@ -224,13 +236,14 @@ static void gelic_card_reset_chain(struct
> gelic_card *card,
>
> for (descr = start_descr; start_descr != descr->next; descr++) {
> gelic_descr_set_status(descr, GELIC_DESCR_DMA_CARDOWNED);
> - descr->next_descr_addr = cpu_to_be32(descr->next->bus_addr);
> + descr->hw_regs.next_descr_addr
> + = cpu_to_be32(descr->next->link.cpu_addr);
> }
>
> chain->head = start_descr;
> chain->tail = (descr - 1);
>
> - (descr - 1)->next_descr_addr = 0;
> + (descr - 1)->hw_regs.next_descr_addr = 0;
> }
>
> void gelic_card_up(struct gelic_card *card)
> @@ -276,20 +289,35 @@ void gelic_card_down(struct gelic_card *card)
> pr_debug("%s: done\n", __func__);
> }
>
> +static void gelic_unmap_link(struct device *dev, struct gelic_descr *descr)
> +{
> + BUG_ON(descr->hw_regs.payload.dev_addr);
> + BUG_ON(descr->hw_regs.payload.size);
> +
> + BUG_ON(!descr->link.cpu_addr);
> + BUG_ON(!descr->link.size);
Don't add BUG_ON() unless absolutely necessary. Gracefully handle
errors when possible
> +
> + dma_unmap_single(dev, descr->link.cpu_addr, descr->link.size,
> + DMA_BIDIRECTIONAL);
> +
> + descr->link.cpu_addr = 0;
> + descr->link.size = 0;
> +}
> +
> /**
> * gelic_card_free_chain - free descriptor chain
> * @card: card structure
> * @descr_in: address of desc
> */
> static void gelic_card_free_chain(struct gelic_card *card,
> - struct gelic_descr *descr_in)
> + struct gelic_descr *descr_in)
Bad indent, should match open parenthesis
> {
> + struct device *dev = ctodev(card);
> struct gelic_descr *descr;
>
> - for (descr = descr_in; descr && descr->bus_addr; descr = descr->next) {
> - dma_unmap_single(ctodev(card), descr->bus_addr,
> - GELIC_DESCR_SIZE, DMA_BIDIRECTIONAL);
> - descr->bus_addr = 0;
> + for (descr = descr_in; descr && descr->link.cpu_addr;
> + descr = descr->next) {
> + gelic_unmap_link(dev, descr);
> }
> }
>
> @@ -298,7 +326,7 @@ static void gelic_card_free_chain(struct
> gelic_card *card,
> * @card: card structure
> * @chain: address of chain
> * @start_descr: address of descriptor array
> - * @no: number of descriptors
> + * @descr_count: number of descriptors
> *
> * we manage a circular list that mirrors the hardware structure,
> * except that the hardware uses bus addresses.
> @@ -306,54 +334,57 @@ static void gelic_card_free_chain(struct
> gelic_card *card,
> * returns 0 on success, <0 on failure
> */
> static int gelic_card_init_chain(struct gelic_card *card,
> - struct gelic_descr_chain *chain,
> - struct gelic_descr *start_descr, int no)
> + struct gelic_descr_chain *chain, struct gelic_descr *start_descr,
> + int descr_count)
> {
> - int i;
> - struct gelic_descr *descr;
> + struct gelic_descr *descr = start_descr;
> + struct device *dev = ctodev(card);
> + unsigned int index;
>
> - descr = start_descr;
> - memset(descr, 0, sizeof(*descr) * no);
> + memset(start_descr, 0, descr_count * sizeof(*start_descr));
>
> - /* set up the hardware pointers in each descriptor */
> - for (i = 0; i < no; i++, descr++) {
> + for (index = 0, descr = start_descr; index < descr_count;
> + index++, descr++) {
> gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE);
> - descr->bus_addr =
> - dma_map_single(ctodev(card), descr,
> - GELIC_DESCR_SIZE,
> - DMA_BIDIRECTIONAL);
>
> - if (!descr->bus_addr)
> - goto iommu_error;
> + descr->link.size = sizeof(struct gelic_hw_regs);
> + descr->link.cpu_addr = dma_map_single(dev, descr,
> + descr->link.size, DMA_BIDIRECTIONAL);
> +
> + if (unlikely(dma_mapping_error(dev, descr->link.cpu_addr))) {
> + dev_err(dev, "%s:%d: dma_mapping_error\n", __func__,
> + __LINE__);
> +
> + for (index--, descr--; index > 0; index--, descr--) {
> + if (descr->link.cpu_addr) {
> + gelic_unmap_link(dev, descr);
> + }
> + }
> + return -ENOMEM;
> + }
>
> descr->next = descr + 1;
> descr->prev = descr - 1;
> }
> - /* make them as ring */
> +
> (descr - 1)->next = start_descr;
> start_descr->prev = (descr - 1);
>
> /* chain bus addr of hw descriptor */
> - descr = start_descr;
> - for (i = 0; i < no; i++, descr++) {
> - descr->next_descr_addr = cpu_to_be32(descr->next->bus_addr);
> +
> + for (index = 0, descr = start_descr; index < descr_count;
> + index++, descr++) {
> + descr->hw_regs.next_descr_addr
> + = cpu_to_be32(descr->next->link.cpu_addr);
> }
>
> chain->head = start_descr;
> chain->tail = start_descr;
>
> /* do not chain last hw descriptor */
> - (descr - 1)->next_descr_addr = 0;
> + (descr - 1)->hw_regs.next_descr_addr = 0;
>
> return 0;
> -
> -iommu_error:
> - for (i--, descr--; 0 <= i; i--, descr--)
> - if (descr->bus_addr)
> - dma_unmap_single(ctodev(card), descr->bus_addr,
> - GELIC_DESCR_SIZE,
> - DMA_BIDIRECTIONAL);
> - return -ENOMEM;
> }
>
> /**
> @@ -367,49 +398,66 @@ static int gelic_card_init_chain(struct
> gelic_card *card,
> * Activate the descriptor state-wise
> */
> static int gelic_descr_prepare_rx(struct gelic_card *card,
> - struct gelic_descr *descr)
> + struct gelic_descr *descr)
> {
> - int offset;
> - unsigned int bufsize;
> + struct device *dev = ctodev(card);
> + struct aligned_buff {
> + unsigned int total_bytes;
> + unsigned int offset;
> + };
> + struct aligned_buff a_buf;
> + dma_addr_t cpu_addr;
> +
> + if (gelic_descr_get_status(descr) != GELIC_DESCR_DMA_NOT_IN_USE) {
> + dev_err(dev, "%s:%d: ERROR status\n", __func__, __LINE__);
> + }
>
> - if (gelic_descr_get_status(descr) != GELIC_DESCR_DMA_NOT_IN_USE)
> - dev_info(ctodev(card), "%s: ERROR status\n", __func__);
> - /* we need to round up the buffer size to a multiple of 128 */
> - bufsize = ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN);
> + a_buf.total_bytes = ALIGN(GELIC_NET_MAX_MTU, GELIC_NET_RXBUF_ALIGN)
> + + GELIC_NET_RXBUF_ALIGN;
> +
> + descr->skb = dev_alloc_skb(a_buf.total_bytes);
>
> - /* and we need to have it 128 byte aligned, therefore we allocate a
> - * bit more */
> - descr->skb = dev_alloc_skb(bufsize + GELIC_NET_RXBUF_ALIGN - 1);
> if (!descr->skb) {
> - descr->buf_addr = 0; /* tell DMAC don't touch memory */
> + descr->hw_regs.payload.dev_addr = 0;
> + descr->hw_regs.payload.size = 0;
> return -ENOMEM;
> }
> - descr->buf_size = cpu_to_be32(bufsize);
> - descr->dmac_cmd_status = 0;
> - descr->result_size = 0;
> - descr->valid_size = 0;
> - descr->data_error = 0;
> -
> - offset = ((unsigned long)descr->skb->data) &
> - (GELIC_NET_RXBUF_ALIGN - 1);
> - if (offset)
> - skb_reserve(descr->skb, GELIC_NET_RXBUF_ALIGN - offset);
> - /* io-mmu-map the skb */
> - descr->buf_addr = cpu_to_be32(dma_map_single(ctodev(card),
> - descr->skb->data,
> - GELIC_NET_MAX_MTU,
> - DMA_FROM_DEVICE));
> - if (!descr->buf_addr) {
> +
> + a_buf.offset = PTR_ALIGN(descr->skb->data, GELIC_NET_RXBUF_ALIGN)
> + - descr->skb->data;
> +
> + if (a_buf.offset) {
> + dev_dbg(dev, "%s:%d: offset=%u\n", __func__, __LINE__,
> + a_buf.offset);
> + skb_reserve(descr->skb, a_buf.offset);
> + }
> +
> + descr->hw_regs.dmac_cmd_status = 0;
> + descr->hw_regs.result_size = 0;
> + descr->hw_regs.valid_size = 0;
> + descr->hw_regs.data_error = 0;
> +
> + descr->hw_regs.payload.size = a_buf.total_bytes - a_buf.offset;
> + cpu_addr = dma_map_single(dev, descr->skb->data,
> + descr->hw_regs.payload.size, DMA_FROM_DEVICE);
> + descr->hw_regs.payload.dev_addr = cpu_to_be32(cpu_addr);
> +
> + if (unlikely(dma_mapping_error(dev, cpu_addr))) {
> + dev_err(dev, "%s:%d: dma_mapping_error\n", __func__, __LINE__);
> +
> + descr->hw_regs.payload.dev_addr = 0;
> + descr->hw_regs.payload.size = 0;
> +
> dev_kfree_skb_any(descr->skb);
> descr->skb = NULL;
> - dev_info(ctodev(card),
> - "%s:Could not iommu-map rx buffer\n", __func__);
> +
> gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE);
> +
> return -ENOMEM;
> - } else {
> - gelic_descr_set_status(descr, GELIC_DESCR_DMA_CARDOWNED);
> - return 0;
> }
> +
> + gelic_descr_set_status(descr, GELIC_DESCR_DMA_CARDOWNED);
> + return 0;
> }
>
> /**
> @@ -420,19 +468,24 @@ static int gelic_descr_prepare_rx(struct
> gelic_card *card,
> static void gelic_card_release_rx_chain(struct gelic_card *card)
> {
> struct gelic_descr *descr = card->rx_chain.head;
> + struct device *dev = ctodev(card);
>
> do {
> if (descr->skb) {
> - dma_unmap_single(ctodev(card),
> - be32_to_cpu(descr->buf_addr),
> - descr->skb->len,
> - DMA_FROM_DEVICE);
> - descr->buf_addr = 0;
> + dma_unmap_single(dev,
> + be32_to_cpu(descr->hw_regs.payload.dev_addr),
> + descr->hw_regs.payload.size, DMA_FROM_DEVICE);
> +
> dev_kfree_skb_any(descr->skb);
> descr->skb = NULL;
> +
> gelic_descr_set_status(descr,
> - GELIC_DESCR_DMA_NOT_IN_USE);
> + GELIC_DESCR_DMA_NOT_IN_USE);
> }
> +
> + descr->hw_regs.payload.dev_addr = 0;
> + descr->hw_regs.payload.size = 0;
> +
> descr = descr->next;
> } while (descr != card->rx_chain.head);
> }
> @@ -489,26 +542,29 @@ static int gelic_card_alloc_rx_skbs(struct
> gelic_card *card)
> * releases a used tx descriptor (unmapping, freeing of skb)
> */
> static void gelic_descr_release_tx(struct gelic_card *card,
> - struct gelic_descr *descr)
> + struct gelic_descr *descr)
> {
> struct sk_buff *skb = descr->skb;
> + struct device *dev = ctodev(card);
>
> - BUG_ON(!(be32_to_cpu(descr->data_status) & GELIC_DESCR_TX_TAIL));
> + BUG_ON(!(be32_to_cpu(descr->hw_regs.data_status)
> + & GELIC_DESCR_TX_TAIL));
>
> - dma_unmap_single(ctodev(card), be32_to_cpu(descr->buf_addr), skb->len,
> - DMA_TO_DEVICE);
> - dev_kfree_skb_any(skb);
> + dma_unmap_single(dev, be32_to_cpu(descr->hw_regs.payload.dev_addr),
> + descr->hw_regs.payload.size, DMA_TO_DEVICE);
> +
> + descr->hw_regs.payload.dev_addr = 0;
> + descr->hw_regs.payload.size = 0;
>
> - descr->buf_addr = 0;
> - descr->buf_size = 0;
> - descr->next_descr_addr = 0;
> - descr->result_size = 0;
> - descr->valid_size = 0;
> - descr->data_status = 0;
> - descr->data_error = 0;
> + dev_kfree_skb_any(skb);
> descr->skb = NULL;
>
> - /* set descr status */
> + descr->hw_regs.next_descr_addr = 0;
> + descr->hw_regs.result_size = 0;
> + descr->hw_regs.valid_size = 0;
> + descr->hw_regs.data_status = 0;
> + descr->hw_regs.data_error = 0;
> +
> gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE);
> }
>
> @@ -536,48 +592,53 @@ static void gelic_card_wake_queues(struct
> gelic_card *card)
> static void gelic_card_release_tx_chain(struct gelic_card *card, int stop)
> {
> struct gelic_descr_chain *tx_chain;
> - enum gelic_descr_dma_status status;
> - struct net_device *netdev;
> - int release = 0;
> + struct device *dev = ctodev(card);
> + int release;
>
> - for (tx_chain = &card->tx_chain;
> + for (release = 0, tx_chain = &card->tx_chain;
> tx_chain->head != tx_chain->tail && tx_chain->tail;
> tx_chain->tail = tx_chain->tail->next) {
> - status = gelic_descr_get_status(tx_chain->tail);
> - netdev = tx_chain->tail->skb->dev;
> + enum gelic_descr_dma_status status;
> + struct gelic_descr *descr;
> + struct net_device *netdev;
> +
> + descr = tx_chain->tail;
> + status= gelic_descr_get_status(descr);
> + netdev = descr->skb->dev;
> +
> switch (status) {
> case GELIC_DESCR_DMA_RESPONSE_ERROR:
> case GELIC_DESCR_DMA_PROTECTION_ERROR:
> case GELIC_DESCR_DMA_FORCE_END:
> - if (printk_ratelimit())
> - dev_info(ctodev(card),
> - "%s: forcing end of tx descriptor " \
> - "with status %x\n",
> - __func__, status);
> + if (printk_ratelimit()) {
> + dev_info(dev,
> + "%s:%d: forcing end of tx descriptor with status %x\n",
> + __func__, __LINE__, status);
> + }
> netdev->stats.tx_dropped++;
> break;
>
> case GELIC_DESCR_DMA_COMPLETE:
> - if (tx_chain->tail->skb) {
> + if (descr->skb) {
> netdev->stats.tx_packets++;
> - netdev->stats.tx_bytes +=
> - tx_chain->tail->skb->len;
> + netdev->stats.tx_bytes += descr->skb->len;
> }
> break;
>
> case GELIC_DESCR_DMA_CARDOWNED:
> - /* pending tx request */
> default:
> - /* any other value (== GELIC_DESCR_DMA_NOT_IN_USE) */
> - if (!stop)
> + if (!stop) {
> goto out;
> + }
> }
> - gelic_descr_release_tx(card, tx_chain->tail);
> - release ++;
> +
> + gelic_descr_release_tx(card, descr);
> + release++;
> }
> out:
> - if (!stop && release)
> + if (!stop && release) {
> gelic_card_wake_queues(card);
> + }
> }
>
> /**
> @@ -695,30 +756,30 @@ gelic_card_get_next_tx_descr(struct gelic_card *card)
> * has executed before.
> */
> static void gelic_descr_set_tx_cmdstat(struct gelic_descr *descr,
> - struct sk_buff *skb)
> + struct sk_buff *skb)
> {
> if (skb->ip_summed != CHECKSUM_PARTIAL)
> - descr->dmac_cmd_status =
> + descr->hw_regs.dmac_cmd_status =
> cpu_to_be32(GELIC_DESCR_DMA_CMD_NO_CHKSUM |
> - GELIC_DESCR_TX_DMA_FRAME_TAIL);
> + GELIC_DESCR_TX_DMA_FRAME_TAIL);
> else {
> /* is packet ip?
> * if yes: tcp? udp? */
> if (skb->protocol == htons(ETH_P_IP)) {
> if (ip_hdr(skb)->protocol == IPPROTO_TCP)
> - descr->dmac_cmd_status =
> + descr->hw_regs.dmac_cmd_status =
> cpu_to_be32(GELIC_DESCR_DMA_CMD_TCP_CHKSUM |
> GELIC_DESCR_TX_DMA_FRAME_TAIL);
>
> else if (ip_hdr(skb)->protocol == IPPROTO_UDP)
> - descr->dmac_cmd_status =
> + descr->hw_regs.dmac_cmd_status =
> cpu_to_be32(GELIC_DESCR_DMA_CMD_UDP_CHKSUM |
> GELIC_DESCR_TX_DMA_FRAME_TAIL);
> else /*
> * the stack should checksum non-tcp and non-udp
> * packets on his own: NETIF_F_IP_CSUM
> */
> - descr->dmac_cmd_status =
> + descr->hw_regs.dmac_cmd_status =
> cpu_to_be32(GELIC_DESCR_DMA_CMD_NO_CHKSUM |
> GELIC_DESCR_TX_DMA_FRAME_TAIL);
> }
> @@ -760,37 +821,41 @@ static struct sk_buff
> *gelic_put_vlan_tag(struct sk_buff *skb,
> *
> */
> static int gelic_descr_prepare_tx(struct gelic_card *card,
> - struct gelic_descr *descr,
> - struct sk_buff *skb)
> + struct gelic_descr *descr, struct sk_buff *skb)
> {
> - dma_addr_t buf;
> + struct device *dev = ctodev(card);
> + dma_addr_t cpu_addr;
>
> if (card->vlan_required) {
> struct sk_buff *skb_tmp;
> enum gelic_port_type type;
>
> type = netdev_port(skb->dev)->type;
> - skb_tmp = gelic_put_vlan_tag(skb,
> - card->vlan[type].tx);
> - if (!skb_tmp)
> + skb_tmp = gelic_put_vlan_tag(skb, card->vlan[type].tx);
> +
> + if (!skb_tmp) {
> return -ENOMEM;
> + }
> +
> skb = skb_tmp;
> }
>
> - buf = dma_map_single(ctodev(card), skb->data, skb->len, DMA_TO_DEVICE);
> + descr->hw_regs.payload.size = skb->len;
> + cpu_addr = dma_map_single(dev, skb->data, descr->hw_regs.payload.size,
> + DMA_TO_DEVICE);
> + descr->hw_regs.payload.dev_addr = cpu_to_be32(cpu_addr);
>
> - if (!buf) {
> - dev_err(ctodev(card),
> - "dma map 2 failed (%p, %i). Dropping packet\n",
> - skb->data, skb->len);
> + if (unlikely(dma_mapping_error(dev, cpu_addr))) {
> + dev_err(dev, "%s:%d: dma_mapping_error\n", __func__, __LINE__);
> +
> + descr->hw_regs.payload.dev_addr = 0;
> + descr->hw_regs.payload.size = 0;
> return -ENOMEM;
> }
>
> - descr->buf_addr = cpu_to_be32(buf);
> - descr->buf_size = cpu_to_be32(skb->len);
> descr->skb = skb;
> - descr->data_status = 0;
> - descr->next_descr_addr = 0; /* terminate hw descr */
> + descr->hw_regs.data_status = 0;
> + descr->hw_regs.next_descr_addr = 0; /* terminate hw descr */
> gelic_descr_set_tx_cmdstat(descr, skb);
>
> /* bump free descriptor pointer */
> @@ -805,8 +870,9 @@ static int gelic_descr_prepare_tx(struct
> gelic_card *card,
> *
> */
> static int gelic_card_kick_txdma(struct gelic_card *card,
> - struct gelic_descr *descr)
> + struct gelic_descr *descr)
> {
> + struct device *dev = ctodev(card);
> int status = 0;
>
> if (card->tx_dma_progress)
> @@ -815,11 +881,11 @@ static int gelic_card_kick_txdma(struct
> gelic_card *card,
> if (gelic_descr_get_status(descr) == GELIC_DESCR_DMA_CARDOWNED) {
> card->tx_dma_progress = 1;
> status = lv1_net_start_tx_dma(bus_id(card), dev_id(card),
> - descr->bus_addr, 0);
> + descr->link.cpu_addr, 0);
> if (status) {
> card->tx_dma_progress = 0;
> - dev_info(ctodev(card), "lv1_net_start_txdma failed," \
> - "status=%d\n", status);
> + dev_info(dev, "%s:%d: lv1_net_start_txdma failed: %d\n",
> + __func__, __LINE__, status);
> }
> }
> return status;
> @@ -835,6 +901,7 @@ static int gelic_card_kick_txdma(struct gelic_card *card,
> netdev_tx_t gelic_net_xmit(struct sk_buff *skb, struct net_device *netdev)
> {
> struct gelic_card *card = netdev_card(netdev);
> + struct device *dev = ctodev(card);
> struct gelic_descr *descr;
> int result;
> unsigned long flags;
> @@ -868,7 +935,9 @@ netdev_tx_t gelic_net_xmit(struct sk_buff *skb,
> struct net_device *netdev)
> * link this prepared descriptor to previous one
> * to achieve high performance
> */
> - descr->prev->next_descr_addr = cpu_to_be32(descr->bus_addr);
> + descr->prev->hw_regs.next_descr_addr
> + = cpu_to_be32(descr->link.cpu_addr);
> +
> /*
> * as hardware descriptor is modified in the above lines,
> * ensure that the hardware sees it
> @@ -881,13 +950,13 @@ netdev_tx_t gelic_net_xmit(struct sk_buff
> *skb, struct net_device *netdev)
> */
> netdev->stats.tx_dropped++;
> /* don't trigger BUG_ON() in gelic_descr_release_tx */
> - descr->data_status = cpu_to_be32(GELIC_DESCR_TX_TAIL);
> + descr->hw_regs.data_status = cpu_to_be32(GELIC_DESCR_TX_TAIL);
> gelic_descr_release_tx(card, descr);
> /* reset head */
> card->tx_chain.head = descr;
> /* reset hw termination */
> - descr->prev->next_descr_addr = 0;
> - dev_info(ctodev(card), "%s: kick failure\n", __func__);
> + descr->prev->hw_regs.next_descr_addr = 0;
> + dev_info(dev, "%s:%d: kick failure\n", __func__, __LINE__);
> }
>
> spin_unlock_irqrestore(&card->tx_lock, flags);
> @@ -904,28 +973,29 @@ netdev_tx_t gelic_net_xmit(struct sk_buff
> *skb, struct net_device *netdev)
> * stack. The descriptor state is not changed.
> */
> static void gelic_net_pass_skb_up(struct gelic_descr *descr,
> - struct gelic_card *card,
> - struct net_device *netdev)
> -
> + struct gelic_card *card, struct net_device *netdev)
> {
> + struct device *dev = ctodev(card);
> struct sk_buff *skb = descr->skb;
> u32 data_status, data_error;
>
> - data_status = be32_to_cpu(descr->data_status);
> - data_error = be32_to_cpu(descr->data_error);
> - /* unmap skb buffer */
> - dma_unmap_single(ctodev(card), be32_to_cpu(descr->buf_addr),
> - GELIC_NET_MAX_MTU,
> - DMA_FROM_DEVICE);
> -
> - skb_put(skb, be32_to_cpu(descr->valid_size)?
> - be32_to_cpu(descr->valid_size) :
> - be32_to_cpu(descr->result_size));
> - if (!descr->valid_size)
> - dev_info(ctodev(card), "buffer full %x %x %x\n",
> - be32_to_cpu(descr->result_size),
> - be32_to_cpu(descr->buf_size),
> - be32_to_cpu(descr->dmac_cmd_status));
> + data_status = be32_to_cpu(descr->hw_regs.data_status);
> + data_error = be32_to_cpu(descr->hw_regs.data_error);
> +
> + dma_unmap_single(dev, be32_to_cpu(descr->hw_regs.payload.dev_addr),
> + descr->hw_regs.payload.size, DMA_FROM_DEVICE);
> +
> + skb_put(skb, be32_to_cpu(descr->hw_regs.valid_size)?
> + be32_to_cpu(descr->hw_regs.valid_size) :
> + be32_to_cpu(descr->hw_regs.result_size));
> +
> + if (!descr->hw_regs.valid_size) {
> + dev_err(dev, "%s:%d: buffer full %x %x %x\n", __func__,
> + __LINE__,
> + be32_to_cpu(descr->hw_regs.result_size),
> + be32_to_cpu(descr->hw_regs.payload.size),
> + be32_to_cpu(descr->hw_regs.dmac_cmd_status));
> + }
>
> descr->skb = NULL;
> /*
> @@ -964,9 +1034,10 @@ static void gelic_net_pass_skb_up(struct
> gelic_descr *descr,
> */
> static int gelic_card_decode_one_descr(struct gelic_card *card)
> {
> - enum gelic_descr_dma_status status;
> struct gelic_descr_chain *chain = &card->rx_chain;
> struct gelic_descr *descr = chain->head;
> + enum gelic_descr_dma_status status;
> + struct device *dev = ctodev(card);
> struct net_device *netdev = NULL;
> int dmac_chain_ended;
>
> @@ -976,7 +1047,8 @@ static int gelic_card_decode_one_descr(struct
> gelic_card *card)
> return 0;
>
> if (status == GELIC_DESCR_DMA_NOT_IN_USE) {
> - dev_dbg(ctodev(card), "dormant descr? %p\n", descr);
> + dev_dbg(dev, "%s:%d: dormant descr? %px\n", __func__, __LINE__,
> + descr);
> return 0;
> }
>
> @@ -992,7 +1064,8 @@ static int gelic_card_decode_one_descr(struct
> gelic_card *card)
> }
> }
> if (GELIC_PORT_MAX <= i) {
> - pr_info("%s: unknown packet vid=%x\n", __func__, vid);
> + dev_info(dev, "%s:%d: unknown packet vid=%x\n",
> + __func__, __LINE__, vid);
> goto refill;
> }
> } else
> @@ -1001,8 +1074,8 @@ static int gelic_card_decode_one_descr(struct
> gelic_card *card)
> if ((status == GELIC_DESCR_DMA_RESPONSE_ERROR) ||
> (status == GELIC_DESCR_DMA_PROTECTION_ERROR) ||
> (status == GELIC_DESCR_DMA_FORCE_END)) {
> - dev_info(ctodev(card), "dropping RX descriptor with state %x\n",
> - status);
> + dev_info(dev, "%s:%d: dropping RX descriptor with state %x\n",
> + __func__, __LINE__, status);
> netdev->stats.rx_dropped++;
> goto refill;
> }
> @@ -1017,7 +1090,7 @@ static int gelic_card_decode_one_descr(struct
> gelic_card *card)
> * Anyway this frame was longer than the MTU,
> * just drop it.
> */
> - dev_info(ctodev(card), "overlength frame\n");
> + dev_info(dev, "%s:%d: overlength frame\n", __func__, __LINE__);
> goto refill;
> }
> /*
> @@ -1025,8 +1098,8 @@ static int gelic_card_decode_one_descr(struct
> gelic_card *card)
> * be treated as error.
> */
> if (status != GELIC_DESCR_DMA_FRAME_END) {
> - dev_dbg(ctodev(card), "RX descriptor with state %x\n",
> - status);
> + dev_dbg(dev, "%s:%d: RX descriptor with state %x\n", __func__,
> + __LINE__, status);
> goto refill;
> }
>
> @@ -1035,15 +1108,14 @@ static int
> gelic_card_decode_one_descr(struct gelic_card *card)
> refill:
>
> /* is the current descriptor terminated with next_descr == NULL? */
> - dmac_chain_ended =
> - be32_to_cpu(descr->dmac_cmd_status) &
> + dmac_chain_ended = be32_to_cpu(descr->hw_regs.dmac_cmd_status) &
> GELIC_DESCR_RX_DMA_CHAIN_END;
> /*
> * So that always DMAC can see the end
> * of the descriptor chain to avoid
> * from unwanted DMAC overrun.
> */
> - descr->next_descr_addr = 0;
> + descr->hw_regs.next_descr_addr = 0;
>
> /* change the descriptor state: */
> gelic_descr_set_status(descr, GELIC_DESCR_DMA_NOT_IN_USE);
> @@ -1060,15 +1132,17 @@ static int
> gelic_card_decode_one_descr(struct gelic_card *card)
> /*
> * Set this descriptor the end of the chain.
> */
> - descr->prev->next_descr_addr = cpu_to_be32(descr->bus_addr);
> + descr->prev->hw_regs.next_descr_addr
> + = cpu_to_be32(descr->link.cpu_addr);
>
> /*
> * If dmac chain was met, DMAC stopped.
> * thus re-enable it
> */
>
> - if (dmac_chain_ended)
> + if (dmac_chain_ended) {
> gelic_card_enable_rxdmac(card);
> + }
>
> return 1;
> }
> @@ -1192,9 +1266,10 @@ void gelic_net_get_drvinfo(struct net_device *netdev,
> }
>
> static int gelic_ether_get_link_ksettings(struct net_device *netdev,
> - struct ethtool_link_ksettings *cmd)
> + struct ethtool_link_ksettings *cmd)
> {
> struct gelic_card *card = netdev_card(netdev);
> + struct device *dev = ctodev(card);
> u32 supported, advertising;
>
> gelic_card_get_ether_port_status(card, 0);
> @@ -1215,16 +1290,17 @@ static int
> gelic_ether_get_link_ksettings(struct net_device *netdev,
> cmd->base.speed = SPEED_1000;
> break;
> default:
> - pr_info("%s: speed unknown\n", __func__);
> + dev_dbg(dev, "%s:%d: speed unknown\n", __func__, __LINE__);
> cmd->base.speed = SPEED_10;
> break;
> }
>
> supported = SUPPORTED_TP | SUPPORTED_Autoneg |
> - SUPPORTED_10baseT_Half | SUPPORTED_10baseT_Full |
> - SUPPORTED_100baseT_Half | SUPPORTED_100baseT_Full |
> - SUPPORTED_1000baseT_Full;
> + SUPPORTED_10baseT_Half | SUPPORTED_10baseT_Full |
> + SUPPORTED_100baseT_Half | SUPPORTED_100baseT_Full |
> + SUPPORTED_1000baseT_Full;
> advertising = supported;
> +
> if (card->link_mode & GELIC_LV1_ETHER_AUTO_NEG) {
> cmd->base.autoneg = AUTONEG_ENABLE;
> } else {
> @@ -1234,9 +1310,9 @@ static int
> gelic_ether_get_link_ksettings(struct net_device *netdev,
> cmd->base.port = PORT_TP;
>
> ethtool_convert_legacy_u32_to_link_mode(cmd->link_modes.supported,
> - supported);
> + supported);
> ethtool_convert_legacy_u32_to_link_mode(cmd->link_modes.advertising,
> - advertising);
> + advertising);
>
> return 0;
> }
> @@ -1637,130 +1713,134 @@ static void
> gelic_card_get_vlan_info(struct gelic_card *card)
> /*
> * ps3_gelic_driver_probe - add a device to the control of this driver
> */
> -static int ps3_gelic_driver_probe(struct ps3_system_bus_device *dev)
> +static int ps3_gelic_driver_probe(struct ps3_system_bus_device *sb_dev)
> {
> - struct gelic_card *card;
> + struct device *dev = &sb_dev->core;
> struct net_device *netdev;
> + struct gelic_card *card;
> int result;
>
> - pr_debug("%s: called\n", __func__);
> + dev_dbg(dev, "%s:%d: >\n", __func__, __LINE__);
>
> udbg_shutdown_ps3gelic();
>
> - result = ps3_open_hv_device(dev);
> + result = ps3_open_hv_device(sb_dev);
>
> if (result) {
> - dev_dbg(&dev->core, "%s:ps3_open_hv_device failed\n",
> - __func__);
> + dev_err(dev, "%s:%d: ps3_open_hv_device failed: %d\n",
> + __func__, __LINE__, result);
> goto fail_open;
> }
>
> - result = ps3_dma_region_create(dev->d_region);
> + result = ps3_dma_region_create(sb_dev->d_region);
>
> if (result) {
> - dev_dbg(&dev->core, "%s:ps3_dma_region_create failed(%d)\n",
> - __func__, result);
> + dev_err(dev, "%s:%d: ps3_dma_region_create failed: %d\n",
> + __func__, __LINE__, result);
> BUG_ON("check region type");
> goto fail_dma_region;
> }
>
> - /* alloc card/netdevice */
> card = gelic_alloc_card_net(&netdev);
> +
> if (!card) {
> - dev_info(&dev->core, "%s:gelic_net_alloc_card failed\n",
> - __func__);
> + dev_info(dev, "%s:%d: gelic_net_alloc_card failed.\n", __func__,
> + __LINE__);
> result = -ENOMEM;
> goto fail_alloc_card;
> }
> - ps3_system_bus_set_drvdata(dev, card);
> - card->dev = dev;
>
> - /* get internal vlan info */
> + ps3_system_bus_set_drvdata(sb_dev, card);
> + card->dev = sb_dev;
> gelic_card_get_vlan_info(card);
> -
> card->link_mode = GELIC_LV1_ETHER_AUTO_NEG;
>
> - /* setup interrupt */
> result = lv1_net_set_interrupt_status_indicator(bus_id(card),
> - dev_id(card),
> - ps3_mm_phys_to_lpar(__pa(&card->irq_status)),
> - 0);
> + dev_id(card), ps3_mm_phys_to_lpar(__pa(&card->irq_status)), 0);
>
> if (result) {
> - dev_dbg(&dev->core,
> - "%s:set_interrupt_status_indicator failed: %s\n",
> - __func__, ps3_result(result));
> + dev_dbg(dev,
> + "%s:%d: set_interrupt_status_indicator failed: %s\n",
> + __func__, __LINE__, ps3_result(result));
> result = -EIO;
> goto fail_status_indicator;
> }
>
> - result = ps3_sb_event_receive_port_setup(dev, PS3_BINDING_CPU_ANY,
> + result = ps3_sb_event_receive_port_setup(sb_dev, PS3_BINDING_CPU_ANY,
> &card->irq);
>
> if (result) {
> - dev_info(ctodev(card),
> - "%s:gelic_net_open_device failed (%d)\n",
> - __func__, result);
> + dev_dbg(dev, "%s:%d: gelic_net_open_device failed: %d\n",
> + __func__, __LINE__, result);
> result = -EPERM;
> goto fail_alloc_irq;
> }
> +
> result = request_irq(card->irq, gelic_card_interrupt,
> 0, netdev->name, card);
>
> if (result) {
> - dev_info(ctodev(card), "%s:request_irq failed (%d)\n",
> - __func__, result);
> + dev_dbg(dev, "%s:%d: request_irq failed: %d\n",
> + __func__, __LINE__, result);
> goto fail_request_irq;
> }
>
> - /* setup card structure */
> card->irq_mask = GELIC_CARD_RXINT | GELIC_CARD_TXINT |
> GELIC_CARD_PORT_STATUS_CHANGED;
>
> + result = gelic_card_init_chain(card, &card->tx_chain, card->descr,
> + GELIC_NET_TX_DESCRIPTORS);
>
> - result = gelic_card_init_chain(card, &card->tx_chain,
> - card->descr, GELIC_NET_TX_DESCRIPTORS);
> - if (result)
> + if (result) {
> goto fail_alloc_tx;
> + }
> +
> result = gelic_card_init_chain(card, &card->rx_chain,
> - card->descr + GELIC_NET_TX_DESCRIPTORS,
> - GELIC_NET_RX_DESCRIPTORS);
> - if (result)
> + card->descr + GELIC_NET_TX_DESCRIPTORS,
> + GELIC_NET_RX_DESCRIPTORS);
> +
> + if (result) {
> goto fail_alloc_rx;
> + }
>
> - /* head of chain */
> card->tx_top = card->tx_chain.head;
> card->rx_top = card->rx_chain.head;
> - dev_dbg(ctodev(card), "descr rx %p, tx %p, size %#lx, num %#x\n",
> - card->rx_top, card->tx_top, sizeof(struct gelic_descr),
> - GELIC_NET_RX_DESCRIPTORS);
> - /* allocate rx skbs */
> +
> + dev_dbg(dev, "%s:%d: descr rx %px, tx %px, size %#lx, num %#x\n",
> + __func__, __LINE__, card->rx_top, card->tx_top,
> + sizeof(struct gelic_descr), GELIC_NET_RX_DESCRIPTORS);
> +
> result = gelic_card_alloc_rx_skbs(card);
> - if (result)
> +
> + if (result) {
> goto fail_alloc_skbs;
> + }
>
> spin_lock_init(&card->tx_lock);
> card->tx_dma_progress = 0;
>
> - /* setup net_device structure */
> netdev->irq = card->irq;
> - SET_NETDEV_DEV(netdev, &card->dev->core);
> + SET_NETDEV_DEV(netdev, dev);
> gelic_ether_setup_netdev_ops(netdev, &card->napi);
> +
> result = gelic_net_setup_netdev(netdev, card);
> +
> if (result) {
> - dev_dbg(&dev->core, "%s: setup_netdev failed %d\n",
> - __func__, result);
> + dev_err(dev, "%s:%d: setup_netdev failed: %d\n", __func__,
> + __LINE__, result);
> goto fail_setup_netdev;
> }
>
> #ifdef CONFIG_GELIC_WIRELESS
> result = gelic_wl_driver_probe(card);
> +
> if (result) {
> - dev_dbg(&dev->core, "%s: WL init failed\n", __func__);
> + dev_dbg(dev, "%s:%d: WL init failed\n", __func__, __LINE__);
> goto fail_setup_netdev;
> }
> #endif
> - pr_debug("%s: done\n", __func__);
> +
> + dev_dbg(dev, "%s:%d: < OK\n", __func__, __LINE__);
> return 0;
>
> fail_setup_netdev:
> @@ -1772,20 +1852,19 @@ static int ps3_gelic_driver_probe(struct
> ps3_system_bus_device *dev)
> free_irq(card->irq, card);
> netdev->irq = 0;
> fail_request_irq:
> - ps3_sb_event_receive_port_destroy(dev, card->irq);
> + ps3_sb_event_receive_port_destroy(sb_dev, card->irq);
> fail_alloc_irq:
> - lv1_net_set_interrupt_status_indicator(bus_id(card),
> - bus_id(card),
> - 0, 0);
> + lv1_net_set_interrupt_status_indicator(bus_id(card), bus_id(card), 0, 0);
> fail_status_indicator:
> - ps3_system_bus_set_drvdata(dev, NULL);
> + ps3_system_bus_set_drvdata(sb_dev, NULL);
> kfree(netdev_card(netdev)->unalign);
> free_netdev(netdev);
> fail_alloc_card:
> - ps3_dma_region_free(dev->d_region);
> + ps3_dma_region_free(sb_dev->d_region);
> fail_dma_region:
> - ps3_close_hv_device(dev);
> + ps3_close_hv_device(sb_dev);
> fail_open:
> + dev_dbg(dev, "%s:%d: < error\n", __func__, __LINE__);
> return result;
> }
>
> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.h
> b/drivers/net/ethernet/toshiba/ps3_gelic_net.h
> index 68f324ed4eaf..fb0d314d53e5 100644
> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.h
> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.h
> @@ -220,29 +220,35 @@ enum gelic_lv1_phy {
> GELIC_LV1_PHY_ETHERNET_0 = 0x0000000000000002L,
> };
>
> -/* size of hardware part of gelic descriptor */
> -#define GELIC_DESCR_SIZE (32)
> -
> enum gelic_port_type {
> GELIC_PORT_ETHERNET_0 = 0,
> GELIC_PORT_WIRELESS = 1,
> GELIC_PORT_MAX
> };
>
> -struct gelic_descr {
> - /* as defined by the hardware */
> - __be32 buf_addr;
> - __be32 buf_size;
> +/* as defined by the hardware */
> +struct gelic_hw_regs {
> + struct {
> + __be32 dev_addr;
> + __be32 size;
> + } __attribute__ ((packed)) payload;
> __be32 next_descr_addr;
> __be32 dmac_cmd_status;
> __be32 result_size;
> __be32 valid_size; /* all zeroes for tx */
> __be32 data_status;
> __be32 data_error; /* all zeroes for tx */
> +} __attribute__ ((packed));
>
> - /* used in the driver */
> +struct gelic_chain_link {
> + dma_addr_t cpu_addr;
> + unsigned int size;
> +};
> +
> +struct gelic_descr {
> + struct gelic_hw_regs hw_regs;
> + struct gelic_chain_link link;
> struct sk_buff *skb;
> - dma_addr_t bus_addr;
> struct gelic_descr *next;
> struct gelic_descr *prev;
> } __attribute__((aligned(32)));
> --
> 2.25.1
^ permalink raw reply
* Re: [PATCH v3 2/2] net/ps3_gelic: Cleanups, improve logging
From: Christophe Leroy @ 2021-07-11 14:10 UTC (permalink / raw)
To: Geoff Levand; +Cc: netdev, linuxppc-dev, David S. Miller, Jakub Kicinski
In-Reply-To: <ffb7b2f4ac085986f563131e3851e07393cd514f.1625976141.git.geoff@infradead.org>
Geoff Levand <geoff@infradead.org> a écrit :
> General source cleanups and improved logging messages.
>
Describe a bit more what you do to cleanup and improve.
Some of your changes are not cleanup , they increase the mess.
You should read kernel coding style
> Signed-off-by: Geoff Levand <geoff@infradead.org>
> ---
> drivers/net/ethernet/toshiba/ps3_gelic_net.c | 395 ++++++++++---------
> 1 file changed, 216 insertions(+), 179 deletions(-)
>
> diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> index e01938128882..9dbcb7c4ec80 100644
> --- a/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> +++ b/drivers/net/ethernet/toshiba/ps3_gelic_net.c
> @@ -44,17 +44,17 @@ MODULE_AUTHOR("SCE Inc.");
> MODULE_DESCRIPTION("Gelic Network driver");
> MODULE_LICENSE("GPL");
>
> -
> -/* set irq_mask */
> int gelic_card_set_irq_mask(struct gelic_card *card, u64 mask)
> {
> + struct device *dev = ctodev(card);
> int status;
>
> status = lv1_net_set_interrupt_mask(bus_id(card), dev_id(card),
> mask, 0);
> - if (status)
> - dev_info(ctodev(card),
> - "%s failed %d\n", __func__, status);
> + if (status) {
No { } for single lines
> + dev_err(dev, "%s:%d failed: %d\n", __func__, __LINE__, status);
> + }
> +
> return status;
> }
>
> @@ -63,6 +63,7 @@ static void gelic_card_rx_irq_on(struct gelic_card *card)
> card->irq_mask |= GELIC_CARD_RXINT;
> gelic_card_set_irq_mask(card, card->irq_mask);
> }
> +
> static void gelic_card_rx_irq_off(struct gelic_card *card)
> {
> card->irq_mask &= ~GELIC_CARD_RXINT;
> @@ -70,15 +71,14 @@ static void gelic_card_rx_irq_off(struct
> gelic_card *card)
> }
>
> static void gelic_card_get_ether_port_status(struct gelic_card *card,
> - int inform)
> + int inform)
Bad indent
> {
> u64 v2;
> struct net_device *ether_netdev;
>
> lv1_net_control(bus_id(card), dev_id(card),
> - GELIC_LV1_GET_ETH_PORT_STATUS,
> - GELIC_LV1_VLAN_TX_ETHERNET_0, 0, 0,
> - &card->ether_port_status, &v2);
> + GELIC_LV1_GET_ETH_PORT_STATUS, GELIC_LV1_VLAN_TX_ETHERNET_0, 0,
> + 0, &card->ether_port_status, &v2);
Bad indent
>
> if (inform) {
> ether_netdev = card->netdev[GELIC_PORT_ETHERNET_0];
> @@ -105,15 +105,17 @@ gelic_descr_get_status(struct gelic_descr *descr)
>
> static int gelic_card_set_link_mode(struct gelic_card *card, int mode)
> {
> + struct device *dev = ctodev(card);
> int status;
> u64 v1, v2;
>
> status = lv1_net_control(bus_id(card), dev_id(card),
> - GELIC_LV1_SET_NEGOTIATION_MODE,
> - GELIC_LV1_PHY_ETHERNET_0, mode, 0, &v1, &v2);
> + GELIC_LV1_SET_NEGOTIATION_MODE, GELIC_LV1_PHY_ETHERNET_0, mode,
> + 0, &v1, &v2);
> +
> if (status) {
> - pr_info("%s: failed setting negotiation mode %d\n", __func__,
> - status);
> + dev_err(dev, "%s:%d: Failed setting negotiation mode: %d\n",
> + __func__, __LINE__, status);
> return -EBUSY;
> }
>
> @@ -130,13 +132,16 @@ static int gelic_card_set_link_mode(struct
> gelic_card *card, int mode)
> */
> static void gelic_card_disable_txdmac(struct gelic_card *card)
> {
> + struct device *dev = ctodev(card);
> int status;
>
> /* this hvc blocks until the DMA in progress really stopped */
> status = lv1_net_stop_tx_dma(bus_id(card), dev_id(card));
> - if (status)
> - dev_err(ctodev(card),
> - "lv1_net_stop_tx_dma failed, status=%d\n", status);
> +
> + if (status) {
> + dev_err(dev, "%s:%d: lv1_net_stop_tx_dma failed: %d\n",
> + __func__, __LINE__, status);
> + }
> }
>
> /**
> @@ -187,13 +192,16 @@ static void gelic_card_enable_rxdmac(struct
> gelic_card *card)
> */
> static void gelic_card_disable_rxdmac(struct gelic_card *card)
> {
> + struct device *dev = ctodev(card);
> int status;
>
> /* this hvc blocks until the DMA in progress really stopped */
> status = lv1_net_stop_rx_dma(bus_id(card), dev_id(card));
> - if (status)
> - dev_err(ctodev(card),
> - "lv1_net_stop_rx_dma failed, %d\n", status);
> +
> + if (status) {
> + dev_err(dev, "%s:%d: lv1_net_stop_rx_dma failed: %d\n",
> + __func__, __LINE__, status);
> + }
> }
>
> /**
> @@ -216,6 +224,7 @@ static void gelic_descr_set_status(struct
> gelic_descr *descr,
> * Usually caller of this function wants to inform that to the
> * hardware, so we assure here the hardware sees the change.
> */
> +
Bad blank line, it separates the comment from the commentee
> wmb();
> }
>
> @@ -229,8 +238,7 @@ static void gelic_descr_set_status(struct
> gelic_descr *descr,
> * and re-initialize the hardware chain for later use
> */
> static void gelic_card_reset_chain(struct gelic_card *card,
> - struct gelic_descr_chain *chain,
> - struct gelic_descr *start_descr)
> + struct gelic_descr_chain *chain, struct gelic_descr *start_descr)
> {
> struct gelic_descr *descr;
>
> @@ -248,45 +256,41 @@ static void gelic_card_reset_chain(struct
> gelic_card *card,
>
> void gelic_card_up(struct gelic_card *card)
> {
> - pr_debug("%s: called\n", __func__);
> + struct device *dev = ctodev(card);
> +
> mutex_lock(&card->updown_lock);
> if (atomic_inc_return(&card->users) == 1) {
> - pr_debug("%s: real do\n", __func__);
> - /* enable irq */
> + dev_dbg(dev, "%s:%d: Starting...\n", __func__, __LINE__);
> gelic_card_set_irq_mask(card, card->irq_mask);
> - /* start rx */
> gelic_card_enable_rxdmac(card);
> -
> napi_enable(&card->napi);
> }
> mutex_unlock(&card->updown_lock);
> - pr_debug("%s: done\n", __func__);
> }
>
> void gelic_card_down(struct gelic_card *card)
> {
> + struct device *dev = ctodev(card);
> u64 mask;
> - pr_debug("%s: called\n", __func__);
> +
> mutex_lock(&card->updown_lock);
> if (atomic_dec_if_positive(&card->users) == 0) {
> - pr_debug("%s: real do\n", __func__);
> + dev_dbg(dev, "%s:%d: Stopping...\n", __func__, __LINE__);
> napi_disable(&card->napi);
> /*
> - * Disable irq. Wireless interrupts will
> - * be disabled later if any
> + * Disable irq. Wireless interrupts will be disabled later.
> */
> mask = card->irq_mask & (GELIC_CARD_WLAN_EVENT_RECEIVED |
> - GELIC_CARD_WLAN_COMMAND_COMPLETED);
> + GELIC_CARD_WLAN_COMMAND_COMPLETED);
> gelic_card_set_irq_mask(card, mask);
> - /* stop rx */
> +
> gelic_card_disable_rxdmac(card);
> gelic_card_reset_chain(card, &card->rx_chain,
> - card->descr + GELIC_NET_TX_DESCRIPTORS);
> - /* stop tx */
> + card->descr + GELIC_NET_TX_DESCRIPTORS);
> +
> gelic_card_disable_txdmac(card);
> }
> mutex_unlock(&card->updown_lock);
> - pr_debug("%s: done\n", __func__);
> }
>
> static void gelic_unmap_link(struct device *dev, struct gelic_descr *descr)
> @@ -652,6 +656,7 @@ static void gelic_card_release_tx_chain(struct
> gelic_card *card, int stop)
> void gelic_net_set_multi(struct net_device *netdev)
> {
> struct gelic_card *card = netdev_card(netdev);
> + struct device *dev = ctodev(card);
> struct netdev_hw_addr *ha;
> unsigned int i;
> uint8_t *p;
> @@ -661,27 +666,31 @@ void gelic_net_set_multi(struct net_device *netdev)
> /* clear all multicast address */
> status = lv1_net_remove_multicast_address(bus_id(card), dev_id(card),
> 0, 1);
> - if (status)
> - dev_err(ctodev(card),
> - "lv1_net_remove_multicast_address failed %d\n",
> - status);
> + if (status) {
> + dev_err(dev,
> + "%s:%d: lv1_net_remove_multicast_address failed %d\n",
> + __func__, __LINE__, status);
> + }
> +
> /* set broadcast address */
> status = lv1_net_add_multicast_address(bus_id(card), dev_id(card),
> GELIC_NET_BROADCAST_ADDR, 0);
> - if (status)
> - dev_err(ctodev(card),
> - "lv1_net_add_multicast_address failed, %d\n",
> - status);
> + if (status) {
> + dev_err(dev,
> + "%s:%d: lv1_net_add_multicast_address failed, %d\n",
> + __func__, __LINE__, status);
> + }
>
> if ((netdev->flags & IFF_ALLMULTI) ||
> (netdev_mc_count(netdev) > GELIC_NET_MC_COUNT_MAX)) {
> status = lv1_net_add_multicast_address(bus_id(card),
> dev_id(card),
> 0, 1);
> - if (status)
> - dev_err(ctodev(card),
> - "lv1_net_add_multicast_address failed, %d\n",
> - status);
> + if (status) {
> + dev_err(dev,
> + "%s:%d: lv1_net_add_multicast_address failed, %d\n",
> + __func__, __LINE__, status);
> + }
> return;
> }
>
> @@ -694,12 +703,13 @@ void gelic_net_set_multi(struct net_device *netdev)
> addr |= *p++;
> }
> status = lv1_net_add_multicast_address(bus_id(card),
> - dev_id(card),
> - addr, 0);
> - if (status)
> - dev_err(ctodev(card),
> - "lv1_net_add_multicast_address failed, %d\n",
> - status);
> + dev_id(card), addr, 0);
> +
> + if (status) {
> + dev_err(dev,
> + "%s:%d: lv1_net_add_multicast_address failed, %d\n",
> + __func__, __LINE__, status);
> + }
> }
> }
>
> @@ -711,17 +721,17 @@ void gelic_net_set_multi(struct net_device *netdev)
> */
> int gelic_net_stop(struct net_device *netdev)
> {
> - struct gelic_card *card;
> + struct gelic_card *card = netdev_card(netdev);
> + struct device *dev = ctodev(card);
>
> - pr_debug("%s: start\n", __func__);
> + dev_dbg(dev, "%s:%d: >\n", __func__, __LINE__);
>
> netif_stop_queue(netdev);
> netif_carrier_off(netdev);
>
> - card = netdev_card(netdev);
> gelic_card_down(card);
>
> - pr_debug("%s: done\n", __func__);
> + dev_dbg(dev, "%s:%d: <\n", __func__, __LINE__);
> return 0;
> }
>
> @@ -736,14 +746,15 @@ gelic_card_get_next_tx_descr(struct gelic_card *card)
> {
> if (!card->tx_chain.head)
> return NULL;
> +
> /* see if the next descriptor is free */
> - if (card->tx_chain.tail != card->tx_chain.head->next &&
> - gelic_descr_get_status(card->tx_chain.head) ==
> - GELIC_DESCR_DMA_NOT_IN_USE)
> + if ((card->tx_chain.tail != card->tx_chain.head->next)
> + && (gelic_descr_get_status(card->tx_chain.head)
> + == GELIC_DESCR_DMA_NOT_IN_USE)) {
> return card->tx_chain.head;
> - else
> - return NULL;
> + }
>
> + return NULL;
> }
>
> /**
> @@ -787,14 +798,15 @@ static void gelic_descr_set_tx_cmdstat(struct
> gelic_descr *descr,
> }
>
> static struct sk_buff *gelic_put_vlan_tag(struct sk_buff *skb,
> - unsigned short tag)
> + unsigned short tag)
> {
> struct vlan_ethhdr *veth;
> static unsigned int c;
>
> if (skb_headroom(skb) < VLAN_HLEN) {
> struct sk_buff *sk_tmp = skb;
> - pr_debug("%s: hd=%d c=%ud\n", __func__, skb_headroom(skb), c);
> + pr_debug("%s:%d: hd=%d c=%ud\n", __func__, __LINE__,
> + skb_headroom(skb), c);
> skb = skb_realloc_headroom(sk_tmp, VLAN_HLEN);
> if (!skb)
> return NULL;
> @@ -818,7 +830,6 @@ static struct sk_buff *gelic_put_vlan_tag(struct
> sk_buff *skb,
> * @skb: packet to use
> *
> * returns 0 on success, <0 on failure.
> - *
> */
> static int gelic_descr_prepare_tx(struct gelic_card *card,
> struct gelic_descr *descr, struct sk_buff *skb)
> @@ -1161,9 +1172,9 @@ static int gelic_net_poll(struct napi_struct
> *napi, int budget)
> int packets_done = 0;
>
> while (packets_done < budget) {
> - if (!gelic_card_decode_one_descr(card))
> + if (!gelic_card_decode_one_descr(card)) {
> break;
> -
> + }
> packets_done++;
> }
>
> @@ -1171,6 +1182,7 @@ static int gelic_net_poll(struct napi_struct
> *napi, int budget)
> napi_complete_done(napi, packets_done);
> gelic_card_rx_irq_on(card);
> }
> +
> return packets_done;
> }
>
> @@ -1185,8 +1197,9 @@ static irqreturn_t gelic_card_interrupt(int
> irq, void *ptr)
>
> status = card->irq_status;
>
> - if (!status)
> + if (!status) {
> return IRQ_NONE;
> + }
>
> status &= card->irq_mask;
>
> @@ -1205,13 +1218,15 @@ static irqreturn_t gelic_card_interrupt(int
> irq, void *ptr)
> }
>
> /* ether port status changed */
> - if (status & GELIC_CARD_PORT_STATUS_CHANGED)
> + if (status & GELIC_CARD_PORT_STATUS_CHANGED) {
> gelic_card_get_ether_port_status(card, 1);
> + }
>
> #ifdef CONFIG_GELIC_WIRELESS
> if (status & (GELIC_CARD_WLAN_EVENT_RECEIVED |
> - GELIC_CARD_WLAN_COMMAND_COMPLETED))
> + GELIC_CARD_WLAN_COMMAND_COMPLETED)) {
> gelic_wl_interrupt(card->netdev[GELIC_PORT_WIRELESS], status);
> + }
> #endif
>
> return IRQ_HANDLED;
> @@ -1247,19 +1262,16 @@ int gelic_net_open(struct net_device *netdev)
> {
> struct gelic_card *card = netdev_card(netdev);
>
> - dev_dbg(ctodev(card), " -> %s %p\n", __func__, netdev);
> -
> gelic_card_up(card);
>
> netif_start_queue(netdev);
> gelic_card_get_ether_port_status(card, 1);
>
> - dev_dbg(ctodev(card), " <- %s\n", __func__);
> return 0;
> }
>
> void gelic_net_get_drvinfo(struct net_device *netdev,
> - struct ethtool_drvinfo *info)
> + struct ethtool_drvinfo *info)
> {
> strlcpy(info->driver, DRV_NAME, sizeof(info->driver));
> strlcpy(info->version, DRV_VERSION, sizeof(info->version));
> @@ -1317,11 +1329,11 @@ static int
> gelic_ether_get_link_ksettings(struct net_device *netdev,
> return 0;
> }
>
> -static int
> -gelic_ether_set_link_ksettings(struct net_device *netdev,
> - const struct ethtool_link_ksettings *cmd)
> +static int gelic_ether_set_link_ksettings(struct net_device *netdev,
> + const struct ethtool_link_ksettings *cmd)
> {
> struct gelic_card *card = netdev_card(netdev);
> + struct device *dev = ctodev(card);
> u64 mode;
> int ret;
>
> @@ -1341,94 +1353,100 @@ gelic_ether_set_link_ksettings(struct
> net_device *netdev,
> default:
> return -EINVAL;
> }
> +
> if (cmd->base.duplex == DUPLEX_FULL) {
> mode |= GELIC_LV1_ETHER_FULL_DUPLEX;
> } else if (cmd->base.speed == SPEED_1000) {
> - pr_info("1000 half duplex is not supported.\n");
> + dev_dbg(dev,
> + "%s:%d: 1000 half duplex is not supported.\n",
> + __func__, __LINE__);
> return -EINVAL;
> }
> }
>
> ret = gelic_card_set_link_mode(card, mode);
>
> - if (ret)
> + if (ret) {
> return ret;
> + }
>
> return 0;
> }
>
> static void gelic_net_get_wol(struct net_device *netdev,
> - struct ethtool_wolinfo *wol)
> + struct ethtool_wolinfo *wol)
> {
> - if (0 <= ps3_compare_firmware_version(2, 2, 0))
> + if (0 <= ps3_compare_firmware_version(2, 2, 0)) {
> wol->supported = WAKE_MAGIC;
> - else
> + } else {
> wol->supported = 0;
> + }
>
> wol->wolopts = ps3_sys_manager_get_wol() ? wol->supported : 0;
> memset(&wol->sopass, 0, sizeof(wol->sopass));
> }
> +
> static int gelic_net_set_wol(struct net_device *netdev,
> - struct ethtool_wolinfo *wol)
> + struct ethtool_wolinfo *wol)
> {
> + struct gelic_card *card = netdev_card(netdev);
> + struct device *dev = ctodev(card);
> int status;
> - struct gelic_card *card;
> u64 v1, v2;
>
> if (ps3_compare_firmware_version(2, 2, 0) < 0 ||
> - !capable(CAP_NET_ADMIN))
> + !capable(CAP_NET_ADMIN)) {
> return -EPERM;
> + }
>
> - if (wol->wolopts & ~WAKE_MAGIC)
> + if (wol->wolopts & ~WAKE_MAGIC) {
> return -EINVAL;
> + }
>
> - card = netdev_card(netdev);
> if (wol->wolopts & WAKE_MAGIC) {
> status = lv1_net_control(bus_id(card), dev_id(card),
> - GELIC_LV1_SET_WOL,
> - GELIC_LV1_WOL_MAGIC_PACKET,
> - 0, GELIC_LV1_WOL_MP_ENABLE,
> - &v1, &v2);
> + GELIC_LV1_SET_WOL, GELIC_LV1_WOL_MAGIC_PACKET, 0,
> + GELIC_LV1_WOL_MP_ENABLE, &v1, &v2);
> +
> if (status) {
> - pr_info("%s: enabling WOL failed %d\n", __func__,
> - status);
> + dev_dbg(dev, "%s:%d: enabling WOL failed: %d\n",
> + __func__, __LINE__, status);
> status = -EIO;
> goto done;
> }
> +
> status = lv1_net_control(bus_id(card), dev_id(card),
> - GELIC_LV1_SET_WOL,
> - GELIC_LV1_WOL_ADD_MATCH_ADDR,
> - 0, GELIC_LV1_WOL_MATCH_ALL,
> - &v1, &v2);
> - if (!status)
> + GELIC_LV1_SET_WOL, GELIC_LV1_WOL_ADD_MATCH_ADDR, 0,
> + GELIC_LV1_WOL_MATCH_ALL, &v1, &v2);
> +
> + if (!status) {
> ps3_sys_manager_set_wol(1);
> - else {
> - pr_info("%s: enabling WOL filter failed %d\n",
> - __func__, status);
> + } else {
> + dev_dbg(dev, "%s:%d: Enabling WOL filter failed: %d\n",
> + __func__, __LINE__, status);
> status = -EIO;
> }
> } else {
> status = lv1_net_control(bus_id(card), dev_id(card),
> - GELIC_LV1_SET_WOL,
> - GELIC_LV1_WOL_MAGIC_PACKET,
> - 0, GELIC_LV1_WOL_MP_DISABLE,
> - &v1, &v2);
> + GELIC_LV1_SET_WOL, GELIC_LV1_WOL_MAGIC_PACKET, 0,
> + GELIC_LV1_WOL_MP_DISABLE, &v1, &v2);
> +
> if (status) {
> - pr_info("%s: disabling WOL failed %d\n", __func__,
> - status);
> + dev_dbg(dev, "%s:%d: Disabling WOL failed: %d\n",
> + __func__, __LINE__, status);
> status = -EIO;
> goto done;
> }
> +
> status = lv1_net_control(bus_id(card), dev_id(card),
> - GELIC_LV1_SET_WOL,
> - GELIC_LV1_WOL_DELETE_MATCH_ADDR,
> - 0, GELIC_LV1_WOL_MATCH_ALL,
> - &v1, &v2);
> + GELIC_LV1_SET_WOL, GELIC_LV1_WOL_DELETE_MATCH_ADDR, 0,
> + GELIC_LV1_WOL_MATCH_ALL, &v1, &v2);
> +
> if (!status)
> ps3_sys_manager_set_wol(0);
> else {
> - pr_info("%s: removing WOL filter failed %d\n",
> - __func__, status);
> + dev_dbg(dev, "%s:%d: Removing WOL filter failed: %d\n",
> + __func__, __LINE__, status);
> status = -EIO;
> }
> }
> @@ -1445,6 +1463,11 @@ static const struct ethtool_ops
> gelic_ether_ethtool_ops = {
> .set_link_ksettings = gelic_ether_set_link_ksettings,
> };
>
> +static struct gelic_card *gelic_work_to_card(struct work_struct *work)
> +{
> + return container_of(work, struct gelic_card, tx_timeout_task);
> +}
> +
> /**
> * gelic_net_tx_timeout_task - task scheduled by the watchdog timeout
> * function (to be called not under interrupt status)
> @@ -1454,14 +1477,15 @@ static const struct ethtool_ops
> gelic_ether_ethtool_ops = {
> */
> static void gelic_net_tx_timeout_task(struct work_struct *work)
> {
> - struct gelic_card *card =
> - container_of(work, struct gelic_card, tx_timeout_task);
> + struct gelic_card *card = gelic_work_to_card(work);
> struct net_device *netdev = card->netdev[GELIC_PORT_ETHERNET_0];
> + struct device *dev = ctodev(card);
>
> - dev_info(ctodev(card), "%s:Timed out. Restarting...\n", __func__);
> + dev_info(dev, "%s:%d: Timed out. Restarting...\n", __func__, __LINE__);
>
> - if (!(netdev->flags & IFF_UP))
> + if (!(netdev->flags & IFF_UP)) {
> goto out;
> + }
>
> netif_device_detach(netdev);
> gelic_net_stop(netdev);
> @@ -1486,10 +1510,12 @@ void gelic_net_tx_timeout(struct net_device
> *netdev, unsigned int txqueue)
>
> card = netdev_card(netdev);
> atomic_inc(&card->tx_timeout_task_counter);
> - if (netdev->flags & IFF_UP)
> +
> + if (netdev->flags & IFF_UP) {
> schedule_work(&card->tx_timeout_task);
> - else
> + } else {
> atomic_dec(&card->tx_timeout_task_counter);
> + }
> }
>
> static const struct net_device_ops gelic_netdevice_ops = {
> @@ -1513,7 +1539,7 @@ static const struct net_device_ops
> gelic_netdevice_ops = {
> * fills out function pointers in the net_device structure
> */
> static void gelic_ether_setup_netdev_ops(struct net_device *netdev,
> - struct napi_struct *napi)
> + struct napi_struct *napi)
> {
> netdev->watchdog_timeo = GELIC_NET_WATCHDOG_TIMEOUT;
> /* NAPI */
> @@ -1534,25 +1560,28 @@ static void
> gelic_ether_setup_netdev_ops(struct net_device *netdev,
> **/
> int gelic_net_setup_netdev(struct net_device *netdev, struct
> gelic_card *card)
> {
> + struct device *dev = ctodev(card);
> int status;
> u64 v1, v2;
>
> netdev->hw_features = NETIF_F_IP_CSUM | NETIF_F_RXCSUM;
> -
> netdev->features = NETIF_F_IP_CSUM;
> - if (GELIC_CARD_RX_CSUM_DEFAULT)
> +
> + if (GELIC_CARD_RX_CSUM_DEFAULT) {
> netdev->features |= NETIF_F_RXCSUM;
> + }
>
> status = lv1_net_control(bus_id(card), dev_id(card),
> - GELIC_LV1_GET_MAC_ADDRESS,
> - 0, 0, 0, &v1, &v2);
> + GELIC_LV1_GET_MAC_ADDRESS, 0, 0, 0, &v1, &v2);
> +
> v1 <<= 16;
> +
> if (status || !is_valid_ether_addr((u8 *)&v1)) {
> - dev_info(ctodev(card),
> - "%s:lv1_net_control GET_MAC_ADDR failed %d\n",
> - __func__, status);
> + dev_dbg(dev, "%s:%d: lv1_net_control GET_MAC_ADDR failed: %d\n",
> + __func__, __LINE__, status);
> return -EINVAL;
> }
> +
> memcpy(netdev->dev_addr, &v1, ETH_ALEN);
>
> if (card->vlan_required) {
> @@ -1569,13 +1598,15 @@ int gelic_net_setup_netdev(struct net_device
> *netdev, struct gelic_card *card)
> netdev->max_mtu = GELIC_NET_MAX_MTU;
>
> status = register_netdev(netdev);
> +
> if (status) {
> - dev_err(ctodev(card), "%s:Couldn't register %s %d\n",
> - __func__, netdev->name, status);
> + dev_err(dev, "%s:%d: Couldn't register %s: %d\n", __func__,
> + __LINE__, netdev->name, status);
> return status;
> }
> - dev_info(ctodev(card), "%s: MAC addr %pM\n",
> - netdev->name, netdev->dev_addr);
> +
> + dev_info(dev, "%s:%d: %s MAC addr %pxM\n", __func__, __LINE__,
> + netdev->name, netdev->dev_addr);
>
> return 0;
> }
> @@ -1600,34 +1631,33 @@ static struct gelic_card
> *gelic_alloc_card_net(struct net_device **netdev)
> */
> BUILD_BUG_ON(offsetof(struct gelic_card, irq_status) % 8);
> BUILD_BUG_ON(offsetof(struct gelic_card, descr) % 32);
> - alloc_size =
> - sizeof(struct gelic_card) +
> +
> + alloc_size = sizeof(struct gelic_card) +
> sizeof(struct gelic_descr) * GELIC_NET_RX_DESCRIPTORS +
> sizeof(struct gelic_descr) * GELIC_NET_TX_DESCRIPTORS +
> GELIC_ALIGN - 1;
>
> p = kzalloc(alloc_size, GFP_KERNEL);
> - if (!p)
> +
> + if (!p) {
> return NULL;
> + }
> +
> card = PTR_ALIGN(p, GELIC_ALIGN);
> card->unalign = p;
>
> - /*
> - * alloc netdev
> - */
> *netdev = alloc_etherdev(sizeof(struct gelic_port));
> +
> if (!*netdev) {
> kfree(card->unalign);
> return NULL;
> }
> - port = netdev_priv(*netdev);
>
> - /* gelic_port */
> + port = netdev_priv(*netdev);
> port->netdev = *netdev;
> port->card = card;
> port->type = GELIC_PORT_ETHERNET_0;
>
> - /* gelic_card */
> card->netdev[GELIC_PORT_ETHERNET_0] = *netdev;
>
> INIT_WORK(&card->tx_timeout_task, gelic_net_tx_timeout_task);
> @@ -1641,13 +1671,15 @@ static struct gelic_card
> *gelic_alloc_card_net(struct net_device **netdev)
>
> static void gelic_card_get_vlan_info(struct gelic_card *card)
> {
> + struct device *dev = ctodev(card);
> + unsigned int i;
> u64 v1, v2;
> int status;
> - unsigned int i;
> - struct {
> + struct vlan_id {
> int tx;
> int rx;
> - } vlan_id_ix[2] = {
> + };
> + struct vlan_id vlan_id_ix[2] = {
> [GELIC_PORT_ETHERNET_0] = {
> .tx = GELIC_LV1_VLAN_TX_ETHERNET_0,
> .rx = GELIC_LV1_VLAN_RX_ETHERNET_0
> @@ -1661,38 +1693,44 @@ static void gelic_card_get_vlan_info(struct
> gelic_card *card)
> for (i = 0; i < ARRAY_SIZE(vlan_id_ix); i++) {
> /* tx tag */
> status = lv1_net_control(bus_id(card), dev_id(card),
> - GELIC_LV1_GET_VLAN_ID,
> - vlan_id_ix[i].tx,
> - 0, 0, &v1, &v2);
> + GELIC_LV1_GET_VLAN_ID, vlan_id_ix[i].tx, 0, 0, &v1,
> + &v2);
> +
> if (status || !v1) {
> - if (status != LV1_NO_ENTRY)
> - dev_dbg(ctodev(card),
> - "get vlan id for tx(%d) failed(%d)\n",
> - vlan_id_ix[i].tx, status);
> + if (status != LV1_NO_ENTRY) {
> + dev_dbg(dev,
> + "%s:%d: Get vlan id for tx(%d) failed: %d\n",
> + __func__, __LINE__, vlan_id_ix[i].tx,
> + status);
> + }
> card->vlan[i].tx = 0;
> card->vlan[i].rx = 0;
> continue;
> }
> +
> card->vlan[i].tx = (u16)v1;
>
> /* rx tag */
> status = lv1_net_control(bus_id(card), dev_id(card),
> - GELIC_LV1_GET_VLAN_ID,
> - vlan_id_ix[i].rx,
> - 0, 0, &v1, &v2);
> + GELIC_LV1_GET_VLAN_ID, vlan_id_ix[i].rx, 0, 0, &v1,
> + &v2);
> +
> if (status || !v1) {
> - if (status != LV1_NO_ENTRY)
> - dev_info(ctodev(card),
> - "get vlan id for rx(%d) failed(%d)\n",
> - vlan_id_ix[i].rx, status);
> + if (status != LV1_NO_ENTRY) {
> + dev_dbg(dev,
> + "%s:%d: Get vlan id for rx(%d) failed: %d\n",
> + __func__, __LINE__, vlan_id_ix[i].rx,
> + status);
> + }
> card->vlan[i].tx = 0;
> card->vlan[i].rx = 0;
> continue;
> }
> +
> card->vlan[i].rx = (u16)v1;
>
> - dev_dbg(ctodev(card), "vlan_id[%d] tx=%02x rx=%02x\n",
> - i, card->vlan[i].tx, card->vlan[i].rx);
> + dev_dbg(dev, "%s:%d: vlan_id[%d] tx=%02x rx=%02x\n", __func__,
> + __LINE__, i, card->vlan[i].tx, card->vlan[i].rx);
> }
>
> if (card->vlan[GELIC_PORT_ETHERNET_0].tx) {
> @@ -1707,9 +1745,10 @@ static void gelic_card_get_vlan_info(struct
> gelic_card *card)
> card->vlan[GELIC_PORT_WIRELESS].rx = 0;
> }
>
> - dev_info(ctodev(card), "internal vlan %s\n",
> - card->vlan_required? "enabled" : "disabled");
> + dev_dbg(dev, "%s:%d: internal vlan %s\n", __func__, __LINE__,
> + card->vlan_required? "enabled" : "disabled");
> }
> +
> /*
> * ps3_gelic_driver_probe - add a device to the control of this driver
> */
> @@ -1872,26 +1911,25 @@ static int ps3_gelic_driver_probe(struct
> ps3_system_bus_device *sb_dev)
> * ps3_gelic_driver_remove - remove a device from the control of this driver
> */
>
> -static void ps3_gelic_driver_remove(struct ps3_system_bus_device *dev)
> +static void ps3_gelic_driver_remove(struct ps3_system_bus_device *sb_dev)
> {
> - struct gelic_card *card = ps3_system_bus_get_drvdata(dev);
> + struct gelic_card *card = ps3_system_bus_get_drvdata(sb_dev);
> + struct device *dev = &sb_dev->core;
> struct net_device *netdev0;
> - pr_debug("%s: called\n", __func__);
>
> - /* set auto-negotiation */
> + dev_dbg(dev, "%s:%d: >\n", __func__, __LINE__);
> +
> gelic_card_set_link_mode(card, GELIC_LV1_ETHER_AUTO_NEG);
>
> #ifdef CONFIG_GELIC_WIRELESS
> gelic_wl_driver_remove(card);
> #endif
> - /* stop interrupt */
> +
> gelic_card_set_irq_mask(card, 0);
>
> - /* turn off DMA, force end */
> gelic_card_disable_rxdmac(card);
> gelic_card_disable_txdmac(card);
>
> - /* release chains */
> gelic_card_release_tx_chain(card, 1);
> gelic_card_release_rx_chain(card);
>
> @@ -1899,28 +1937,28 @@ static void ps3_gelic_driver_remove(struct
> ps3_system_bus_device *dev)
> gelic_card_free_chain(card, card->rx_top);
>
> netdev0 = card->netdev[GELIC_PORT_ETHERNET_0];
> - /* disconnect event port */
> +
> free_irq(card->irq, card);
> netdev0->irq = 0;
> ps3_sb_event_receive_port_destroy(card->dev, card->irq);
>
> wait_event(card->waitq,
> - atomic_read(&card->tx_timeout_task_counter) == 0);
> + atomic_read(&card->tx_timeout_task_counter) == 0);
>
> lv1_net_set_interrupt_status_indicator(bus_id(card), dev_id(card),
> - 0 , 0);
> + 0 , 0);
>
> unregister_netdev(netdev0);
> kfree(netdev_card(netdev0)->unalign);
> free_netdev(netdev0);
>
> - ps3_system_bus_set_drvdata(dev, NULL);
> + ps3_system_bus_set_drvdata(sb_dev, NULL);
>
> - ps3_dma_region_free(dev->d_region);
> + ps3_dma_region_free(sb_dev->d_region);
>
> - ps3_close_hv_device(dev);
> + ps3_close_hv_device(sb_dev);
>
> - pr_debug("%s: done\n", __func__);
> + dev_dbg(dev, "%s:%d: <\n", __func__, __LINE__);
> }
>
> static struct ps3_system_bus_driver ps3_gelic_driver = {
> @@ -1932,14 +1970,14 @@ static struct ps3_system_bus_driver
> ps3_gelic_driver = {
> .core.owner = THIS_MODULE,
> };
>
> -static int __init ps3_gelic_driver_init (void)
> +static int __init ps3_gelic_driver_init(void)
> {
> return firmware_has_feature(FW_FEATURE_PS3_LV1)
> ? ps3_system_bus_driver_register(&ps3_gelic_driver)
> : -ENODEV;
> }
>
> -static void __exit ps3_gelic_driver_exit (void)
> +static void __exit ps3_gelic_driver_exit(void)
> {
> ps3_system_bus_driver_unregister(&ps3_gelic_driver);
> }
> @@ -1948,4 +1986,3 @@ module_init(ps3_gelic_driver_init);
> module_exit(ps3_gelic_driver_exit);
>
> MODULE_ALIAS(PS3_MODULE_ALIAS_GELIC);
> -
> --
> 2.25.1
^ permalink raw reply
* [PATCH] KVM: PPC: Book3S HV P9: Fix guest TM support
From: Nicholas Piggin @ 2021-07-12 1:36 UTC (permalink / raw)
To: kvm-ppc; +Cc: Alexey Kardashevskiy, linuxppc-dev, Nicholas Piggin
The conversion to C introduced several bugs in TM handling that can
cause host crashes with TM bad thing interrupts. Mostly just simple
typos or missed logic in the conversion that got through due to my
not testing TM in the guest sufficiently.
- Early TM emulation for the softpatch interrupt should be done if fake
suspend mode is _not_ active.
- Early TM emulation wants to return immediately to the guest so as to
not doom transactions unnecessarily.
- And if exiting from the guest, the host MSR should include the TM[S]
bit if the guest was T/S, before it is treclaimed.
After this fix, all the TM selftests pass when running on a P9 processor
that implements TM with softpatch interrupt.
Reported-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Fixes: 89d35b2391015 ("KVM: PPC: Book3S HV P9: Implement the rest of the P9 path in C")
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kvm/book3s_hv_p9_entry.c | 24 +++++++++++++++++++++---
1 file changed, 21 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/kvm/book3s_hv_p9_entry.c b/arch/powerpc/kvm/book3s_hv_p9_entry.c
index 83f592eadcd2..becb0c08ebda 100644
--- a/arch/powerpc/kvm/book3s_hv_p9_entry.c
+++ b/arch/powerpc/kvm/book3s_hv_p9_entry.c
@@ -317,6 +317,9 @@ int kvmhv_vcpu_entry_p9(struct kvm_vcpu *vcpu, u64 time_limit, unsigned long lpc
*/
mtspr(SPRN_HDEC, hdec);
+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
+tm_return_to_guest:
+#endif
mtspr(SPRN_DAR, vcpu->arch.shregs.dar);
mtspr(SPRN_DSISR, vcpu->arch.shregs.dsisr);
mtspr(SPRN_SRR0, vcpu->arch.shregs.srr0);
@@ -415,11 +418,23 @@ int kvmhv_vcpu_entry_p9(struct kvm_vcpu *vcpu, u64 time_limit, unsigned long lpc
* is in real suspend mode and is trying to transition to
* transactional mode.
*/
- if (local_paca->kvm_hstate.fake_suspend &&
+ if (!local_paca->kvm_hstate.fake_suspend &&
(vcpu->arch.shregs.msr & MSR_TS_S)) {
if (kvmhv_p9_tm_emulation_early(vcpu)) {
- /* Prevent it being handled again. */
- trap = 0;
+ /*
+ * Go straight back into the guest with the
+ * new NIP/MSR as set by TM emulation.
+ */
+ mtspr(SPRN_HSRR0, vcpu->arch.regs.nip);
+ mtspr(SPRN_HSRR1, vcpu->arch.shregs.msr);
+
+ /*
+ * tm_return_to_guest re-loads SRR0/1, DAR,
+ * DSISR after RI is cleared, in case they had
+ * been clobbered by a MCE.
+ */
+ __mtmsrd(0, 1); /* clear RI */
+ goto tm_return_to_guest;
}
}
#endif
@@ -499,6 +514,9 @@ int kvmhv_vcpu_entry_p9(struct kvm_vcpu *vcpu, u64 time_limit, unsigned long lpc
* If we are in real mode, only switch MMU on after the MMU is
* switched to host, to avoid the P9_RADIX_PREFETCH_BUG.
*/
+ if (IS_ENABLED(CONFIG_PPC_TRANSACTIONAL_MEM) &&
+ vcpu->arch.shregs.msr & MSR_TS_MASK)
+ msr |= MSR_TS_S;
__mtmsrd(msr, 0);
end_timing(vcpu);
--
2.23.0
^ permalink raw reply related
* RE: [PATCH v2] fpga: dfl: fme: Fix cpu hotplug issue in performance reporting
From: Wu, Hao @ 2021-07-12 2:27 UTC (permalink / raw)
To: Kajol Jain, will@kernel.org, mark.rutland@arm.com, Xu, Yilun
Cc: maddy@linux.ibm.com, rnsastry@linux.ibm.com, trix@redhat.com,
linux-fpga@vger.kernel.org, linux-kernel@vger.kernel.org,
stable@vger.kernel.org, linux-perf-users@vger.kernel.org,
atrajeev@linux.vnet.ibm.com, mdf@kernel.org,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <20210709084319.33776-1-kjain@linux.ibm.com>
> Subject: [PATCH v2] fpga: dfl: fme: Fix cpu hotplug issue in performance
> reporting
>
> The performance reporting driver added cpu hotplug
> feature but it didn't add pmu migration call in cpu
> offline function.
> This can create an issue incase the current designated
> cpu being used to collect fme pmu data got offline,
> as based on current code we are not migrating fme pmu to
> new target cpu. Because of that perf will still try to
> fetch data from that offline cpu and hence we will not
> get counter data.
>
> Patch fixed this issue by adding pmu_migrate_context call
> in fme_perf_offline_cpu function.
>
> Fixes: 724142f8c42a ("fpga: dfl: fme: add performance reporting support")
> Tested-by: Xu Yilun <yilun.xu@intel.com>
> Signed-off-by: Kajol Jain <kjain@linux.ibm.com>
Thanks for this fixing! And thanks Yilun for the verification too. : )
> Cc: stable@vger.kernel.org
> ---
> drivers/fpga/dfl-fme-perf.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> ---
> Changelog:
> v1 -> v2:
> - Add stable@vger.kernel.org in cc list
>
> RFC -> PATCH v1
> - Remove RFC tag
> - Did nits changes on subject and commit message as suggested by Xu Yilun
> - Added Tested-by tag
> - Link to rfc patch: https://lkml.org/lkml/2021/6/28/112
> ---
> diff --git a/drivers/fpga/dfl-fme-perf.c b/drivers/fpga/dfl-fme-perf.c
> index 4299145ef347..b9a54583e505 100644
> --- a/drivers/fpga/dfl-fme-perf.c
> +++ b/drivers/fpga/dfl-fme-perf.c
> @@ -953,6 +953,10 @@ static int fme_perf_offline_cpu(unsigned int cpu, struct
> hlist_node *node)
> return 0;
>
> priv->cpu = target;
> +
> + /* Migrate fme_perf pmu events to the new target cpu */
Only one minor item, this line is not needed. : )
After remove it,
Acked-by: Wu Hao <hao.wu@intel.com>
Thanks
Hao
> + perf_pmu_migrate_context(&priv->pmu, cpu, target);
> +
> return 0;
> }
>
> --
> 2.31.1
^ permalink raw reply
* Re: [PATCH v8 3/6] KVM: PPC: Book3S HV: Add support for H_RPT_INVALIDATE
From: David Gibson @ 2021-07-12 2:28 UTC (permalink / raw)
To: Bharata B Rao; +Cc: farosas, aneesh.kumar, npiggin, kvm-ppc, linuxppc-dev
In-Reply-To: <YOZ/FNW1K/nzaaEe@in.ibm.com>
[-- Attachment #1: Type: text/plain, Size: 3031 bytes --]
On Thu, Jul 08, 2021 at 09:59:08AM +0530, Bharata B Rao wrote:
> On Thu, Jul 08, 2021 at 01:58:04PM +1000, David Gibson wrote:
> > On Tue, Jul 06, 2021 at 10:56:32AM +0530, Bharata B Rao wrote:
> > > On Mon, Jul 05, 2021 at 02:42:33PM +1000, David Gibson wrote:
> > > > On Mon, Jun 21, 2021 at 02:20:00PM +0530, Bharata B Rao wrote:
> > > > > diff --git a/arch/powerpc/include/asm/mmu_context.h b/arch/powerpc/include/asm/mmu_context.h
> > > > > index 4bc45d3ed8b0..b44f291fc909 100644
> > > > > --- a/arch/powerpc/include/asm/mmu_context.h
> > > > > +++ b/arch/powerpc/include/asm/mmu_context.h
> > > > > @@ -124,8 +124,17 @@ static inline bool need_extra_context(struct mm_struct *mm, unsigned long ea)
> > > > >
> > > > > #if defined(CONFIG_KVM_BOOK3S_HV_POSSIBLE) && defined(CONFIG_PPC_RADIX_MMU)
> > > > > extern void radix_kvm_prefetch_workaround(struct mm_struct *mm);
> > > > > +void do_h_rpt_invalidate_prt(unsigned long pid, unsigned long lpid,
> > > > > + unsigned long type, unsigned long pg_sizes,
> > > > > + unsigned long start, unsigned long end);
> > > > > #else
> > > > > static inline void radix_kvm_prefetch_workaround(struct mm_struct *mm) { }
> > > > > +static inline void do_h_rpt_invalidate_prt(unsigned long pid,
> > > > > + unsigned long lpid,
> > > > > + unsigned long type,
> > > > > + unsigned long pg_sizes,
> > > > > + unsigned long start,
> > > > > + unsigned long end) { }
> > > >
> > > > Since the only plausible caller is in KVM HV code, why do you need the
> > > > #else clause.
> > >
> > > The call to the above routine is prevented for non-radix guests
> > > in KVM HV code at runtime using kvm_is_radix() check and not by
> > > CONFIG_PPC_RADIX_MMU. Hence the #else version would be needed.
> >
> > kvm_is_radix() should evaluate to false at compile time if
> > !defined(CONFIG_PPC_RADIX_MMU), in which case, no you shouldn't need
> > the else version.
>
> At least in the latest mainline, I don't see the definition of
> kvm_is_radix() being conditional to CONFIG_PPC_RADIX_MMU.
That's probably worth fixing, then.
> Anyway this is what I see in practice if the #else version is
> removed and CONFIG_PPC_RADIX_MMU is turned off:
>
> arch/powerpc/kvm/book3s_hv.c: In function ‘kvmppc_h_rpt_invalidate’:
> arch/powerpc/kvm/book3s_hv.c:983:2: error: implicit declaration of function ‘do_h_rpt_invalidate_prt’; did you mean ‘do_h_rpt_invalidate_pat’? [-Werror=implicit-function-declaration]
> 983 | do_h_rpt_invalidate_prt(id, vcpu->kvm->arch.lpid,
> | ^~~~~~~~~~~~~~~~~~~~~~~
> | do_h_rpt_invalidate_pat
If you really do need the stub, it should probably be a BUG() rather
than a no-op, since this really never should get called if we don't
have radix.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH V3 1/1] powerpc/perf: Fix PMU callbacks to clear pending PMI before resetting an overflown PMC
From: Nicholas Piggin @ 2021-07-12 2:37 UTC (permalink / raw)
To: Athira Rajeev, mpe; +Cc: maddy, linuxppc-dev, rnsastry
In-Reply-To: <1625932694-1525-2-git-send-email-atrajeev@linux.vnet.ibm.com>
Excerpts from Athira Rajeev's message of July 11, 2021 1:58 am:
> Running perf fuzzer showed below in dmesg logs:
> "Can't find PMC that caused IRQ"
>
> This means a PMU exception happened, but none of the PMC's (Performance
> Monitor Counter) were found to be overflown. There are some corner cases
> that clears the PMCs after PMI gets masked. In such cases, the perf
> interrupt handler will not find the active PMC values that had caused
> the overflow and thus leads to this message while replaying.
>
> Case 1: PMU Interrupt happens during replay of other interrupts and
> counter values gets cleared by PMU callbacks before replay:
>
> During replay of interrupts like timer, __do_irq and doorbell exception, we
> conditionally enable interrupts via may_hard_irq_enable(). This could
> potentially create a window to generate a PMI. Since irq soft mask is set
> to ALL_DISABLED, the PMI will get masked here. We could get IPIs run before
> perf interrupt is replayed and the PMU events could deleted or stopped.
> This will change the PMU SPR values and resets the counters. Snippet of
> ftrace log showing PMU callbacks invoked in "__do_irq":
>
> <idle>-0 [051] dns. 132025441306354: __do_irq <-call_do_irq
> <idle>-0 [051] dns. 132025441306430: irq_enter <-__do_irq
> <idle>-0 [051] dns. 132025441306503: irq_enter_rcu <-__do_irq
> <idle>-0 [051] dnH. 132025441306599: xive_get_irq <-__do_irq
> <<>>
> <idle>-0 [051] dnH. 132025441307770: generic_smp_call_function_single_interrupt <-smp_ipi_demux_relaxed
> <idle>-0 [051] dnH. 132025441307839: flush_smp_call_function_queue <-smp_ipi_demux_relaxed
> <idle>-0 [051] dnH. 132025441308057: _raw_spin_lock <-event_function
> <idle>-0 [051] dnH. 132025441308206: power_pmu_disable <-perf_pmu_disable
> <idle>-0 [051] dnH. 132025441308337: power_pmu_del <-event_sched_out
> <idle>-0 [051] dnH. 132025441308407: power_pmu_read <-power_pmu_del
> <idle>-0 [051] dnH. 132025441308477: read_pmc <-power_pmu_read
> <idle>-0 [051] dnH. 132025441308590: isa207_disable_pmc <-power_pmu_del
> <idle>-0 [051] dnH. 132025441308663: write_pmc <-power_pmu_del
> <idle>-0 [051] dnH. 132025441308787: power_pmu_event_idx <-perf_event_update_userpage
> <idle>-0 [051] dnH. 132025441308859: rcu_read_unlock_strict <-perf_event_update_userpage
> <idle>-0 [051] dnH. 132025441308975: power_pmu_enable <-perf_pmu_enable
> <<>>
> <idle>-0 [051] dnH. 132025441311108: irq_exit <-__do_irq
> <idle>-0 [051] dns. 132025441311319: performance_monitor_exception <-replay_soft_interrupts
>
> Case 2: PMI's masked during local_* operations, example local_add.
> If the local_add operation happens within a local_irq_save, replay of
> PMI will be during local_irq_restore. Similar to case 1, this could
> also create a window before replay where PMU events gets deleted or
> stopped.
>
> Patch adds a fix to update the PMU callback function 'power_pmu_disable' to
> check for pending perf interrupt. If there is an overflown PMC and pending
> perf interrupt indicated in Paca, clear the PMI bit in paca to drop that
> sample. Clearing of PMI bit is done in 'power_pmu_disable' since disable is
> invoked before any event gets deleted/stopped. With this fix, if there are
> more than one event running in the PMU, there is a chance that we clear the
> PMI bit for the event which is not getting deleted/stopped. The other
> events may still remain active. Hence to make sure we don't drop valid
> sample in such cases, another check is added in power_pmu_enable. This
> checks if there is an overflown PMC found among the active events and if
> so enable back the PMI bit. Two new helper functions are introduced to
> clear/set the PMI, ie 'clear_pmi_irq_pending' and 'set_pmi_irq_pending'.
>
> Also there are corner cases which results in performance monitor interrupts
> getting triggered during power_pmu_disable. This happens since PMXE bit is
> not cleared along with disabling of other MMCR0 bits in the pmu_disable.
> Such PMI's could leave the PMU running and could trigger PMI again which
> will set MMCR0 PMAO bit. This could lead to spurious interrupts in some
> corner cases. Example, a timer after power_pmu_del which will re-enable
> interrupts and triggers a PMI again since PMAO bit is still set. But fails
> to find valid overflow since PMC get cleared in power_pmu_del. Patch
> fixes this by disabling PMXE along with disabling of other MMCR0 bits
> in power_pmu_disable.
>
> We can't just replay PMI any time. Hence this approach is preferred rather
> than replaying PMI before resetting overflown PMC. Patch also documents
> core-book3s on a race condition which can trigger these PMC messages during
> idle path in PowerNV.
>
> Fixes: f442d004806e ("powerpc/64s: Add support to mask perf interrupts and replay them")
> Reported-by: Nageswara R Sastry <nasastry@in.ibm.com>
> Suggested-by: Nicholas Piggin <npiggin@gmail.com>
> Suggested-by: Madhavan Srinivasan <maddy@linux.ibm.com>
> Signed-off-by: Athira Rajeev <atrajeev@linux.vnet.ibm.com>
> ---
> arch/powerpc/include/asm/hw_irq.h | 31 +++++++++++++++++++++++++
> arch/powerpc/perf/core-book3s.c | 49 ++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 79 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/include/asm/hw_irq.h b/arch/powerpc/include/asm/hw_irq.h
> index 21cc571..0a067bb 100644
> --- a/arch/powerpc/include/asm/hw_irq.h
> +++ b/arch/powerpc/include/asm/hw_irq.h
> @@ -224,6 +224,34 @@ static inline bool arch_irqs_disabled(void)
> return arch_irqs_disabled_flags(arch_local_save_flags());
> }
>
> +static inline void set_pmi_irq_pending(void)
> +{
> + /*
> + * Invoked currently from PMU callback functions
> + * to set PMI bit in Paca. This has to be called
> + * with irq's disabled ( via hard_irq_disable ).
> + */
> + if (IS_ENABLED(CONFIG_PPC_IRQ_SOFT_MASK_DEBUG))
> + WARN_ON_ONCE(mfmsr() & MSR_EE);
> + get_paca()->irq_happened |= PACA_IRQ_PMI;
> +}
> +
> +static inline void clear_pmi_irq_pending(void)
> +{
> + /*
> + * Some corner cases could clear the PMU counter overflow
> + * while a masked PMI is pending. One of such case is
> + * when a PMI happens during interrupt replay and perf
> + * counter values gets cleared by PMU callbacks before
> + * replay. So the pending PMI must be cleared here.
> + */
> + if (get_paca()->irq_happened & PACA_IRQ_PMI) {
> + if (IS_ENABLED(CONFIG_PPC_IRQ_SOFT_MASK_DEBUG))
> + WARN_ON_ONCE(mfmsr() & MSR_EE);
> + get_paca()->irq_happened &= ~PACA_IRQ_PMI;
I think you could just make this match set_pmi_irq_pending, that
is, always do the debug test, and remove the branch entirely.
> + }
> +}
> +
> #ifdef CONFIG_PPC_BOOK3S
> /*
> * To support disabling and enabling of irq with PMI, set of
> @@ -408,6 +436,9 @@ static inline void do_hard_irq_enable(void)
> BUILD_BUG();
> }
>
> +static inline void clear_pmi_irq_pending(void) { }
> +static inline void set_pmi_irq_pending(void) { }
> +
> static inline void irq_soft_mask_regs_set_state(struct pt_regs *regs, unsigned long val)
> {
> }
> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
> index bb0ee71..17463b2 100644
> --- a/arch/powerpc/perf/core-book3s.c
> +++ b/arch/powerpc/perf/core-book3s.c
> @@ -848,6 +848,19 @@ static void write_pmc(int idx, unsigned long val)
> }
> }
>
> +static int any_pmc_overflown(struct cpu_hw_events *cpuhw)
> +{
> + int i, idx;
> +
> + for (i = 0; i < cpuhw->n_events; i++) {
> + idx = cpuhw->event[i]->hw.idx;
> + if ((idx) && ((int)read_pmc(idx) < 0))
> + return idx;
> + }
> +
> + return 0;
> +}
> +
> /* Called from sysrq_handle_showregs() */
> void perf_event_print_debug(void)
> {
> @@ -1276,7 +1289,7 @@ static void power_pmu_disable(struct pmu *pmu)
> val = mmcr0 = mfspr(SPRN_MMCR0);
> val |= MMCR0_FC;
> val &= ~(MMCR0_EBE | MMCR0_BHRBA | MMCR0_PMCC | MMCR0_PMAO |
> - MMCR0_FC56);
> + MMCR0_PMXE | MMCR0_FC56);
Maybe a comment to say why you clear PMXE?
> /* Set mmcr0 PMCCEXT for p10 */
> if (ppmu->flags & PPMU_ARCH_31)
> val |= MMCR0_PMCCEXT;
> @@ -1290,6 +1303,16 @@ static void power_pmu_disable(struct pmu *pmu)
> mb();
> isync();
>
> + /*
> + * If any of PMC corresponding to the active PMU
> + * events is overflown, check if there is any pending
> + * perf interrupt set in paca. If so, disable the interrupt
> + * by clearing the paca bit for PMI since we are disabling
> + * the PMU now.
> + */
> + if (any_pmc_overflown(cpuhw))
> + clear_pmi_irq_pending();
Is there a need for the any_pmc_overflown() check here? Could you just
clear unconditionally? Would it be a bug if no pmc was overflown but we
had a PMI pending?
> +
> val = mmcra = cpuhw->mmcr.mmcra;
>
> /*
> @@ -1381,6 +1404,15 @@ static void power_pmu_enable(struct pmu *pmu)
> * (possibly updated for removal of events).
> */
> if (!cpuhw->n_added) {
> + /*
> + * If there is any active event with an overflown PMC
> + * value, Set back PACA_IRQ_PMI which would have got
> + * cleared in power_pmu_disable.
> + */
> + hard_irq_disable();
> + if (any_pmc_overflown(cpuhw))
> + set_pmi_irq_pending();
> +
> mtspr(SPRN_MMCRA, cpuhw->mmcr.mmcra & ~MMCRA_SAMPLE_ENABLE);
> mtspr(SPRN_MMCR1, cpuhw->mmcr.mmcr1);
> if (ppmu->flags & PPMU_ARCH_31)
> @@ -2336,6 +2368,12 @@ static void __perf_event_interrupt(struct pt_regs *regs)
> break;
> }
> }
> + /*
> + * Check if PACA_IRQ_PMI is set any chance
> + * by set_pmi_irq_pending() when PMU was enabled
> + * after accounting for interrupts.
"Clear PACA_IRQ_PMI in case it was set by..." ?
Aside from those minor things,
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
Thanks,
Nick
> + */
> + clear_pmi_irq_pending();
> if (!active)
> /* reset non active counters that have overflowed */
> write_pmc(i + 1, 0);
> @@ -2355,6 +2393,15 @@ static void __perf_event_interrupt(struct pt_regs *regs)
> }
> }
> }
> +
> + /*
> + * During system wide profling or while specific CPU
> + * is monitored for an event, some corner cases could
> + * cause PMC to overflow in idle path. This will trigger
> + * a PMI after waking up from idle. Since counter values
> + * are _not_ saved/restored in idle path, can lead to
> + * below "Can't find PMC" message.
> + */
> if (unlikely(!found) && !arch_irq_disabled_regs(regs))
> printk_ratelimited(KERN_WARNING "Can't find PMC that caused IRQ\n");
>
> --
> 1.8.3.1
>
>
^ permalink raw reply
* Re: [RFC PATCH 10/43] powerpc/64s: Always set PMU control registers to frozen/disabled when not in use
From: Nicholas Piggin @ 2021-07-12 2:41 UTC (permalink / raw)
To: Athira Rajeev; +Cc: linuxppc-dev, kvm-ppc
In-Reply-To: <C58A063A-3B5D-4188-80E2-4C19802785BF@linux.vnet.ibm.com>
Excerpts from Athira Rajeev's message of July 10, 2021 12:50 pm:
>
>
>> On 22-Jun-2021, at 4:27 PM, Nicholas Piggin <npiggin@gmail.com> wrote:
>>
>> KVM PMU management code looks for particular frozen/disabled bits in
>> the PMU registers so it knows whether it must clear them when coming
>> out of a guest or not. Setting this up helps KVM make these optimisations
>> without getting confused. Longer term the better approach might be to
>> move guest/host PMU switching to the perf subsystem.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> arch/powerpc/kernel/cpu_setup_power.c | 4 ++--
>> arch/powerpc/kernel/dt_cpu_ftrs.c | 6 +++---
>> arch/powerpc/kvm/book3s_hv.c | 5 +++++
>> arch/powerpc/perf/core-book3s.c | 7 +++++++
>> 4 files changed, 17 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/cpu_setup_power.c b/arch/powerpc/kernel/cpu_setup_power.c
>> index a29dc8326622..3dc61e203f37 100644
>> --- a/arch/powerpc/kernel/cpu_setup_power.c
>> +++ b/arch/powerpc/kernel/cpu_setup_power.c
>> @@ -109,7 +109,7 @@ static void init_PMU_HV_ISA207(void)
>> static void init_PMU(void)
>> {
>> mtspr(SPRN_MMCRA, 0);
>> - mtspr(SPRN_MMCR0, 0);
>> + mtspr(SPRN_MMCR0, MMCR0_FC);
>> mtspr(SPRN_MMCR1, 0);
>> mtspr(SPRN_MMCR2, 0);
>> }
>> @@ -123,7 +123,7 @@ static void init_PMU_ISA31(void)
>> {
>> mtspr(SPRN_MMCR3, 0);
>> mtspr(SPRN_MMCRA, MMCRA_BHRB_DISABLE);
>> - mtspr(SPRN_MMCR0, MMCR0_PMCCEXT);
>> + mtspr(SPRN_MMCR0, MMCR0_FC | MMCR0_PMCCEXT);
>> }
>>
>> /*
>> diff --git a/arch/powerpc/kernel/dt_cpu_ftrs.c b/arch/powerpc/kernel/dt_cpu_ftrs.c
>> index 0a6b36b4bda8..06a089fbeaa7 100644
>> --- a/arch/powerpc/kernel/dt_cpu_ftrs.c
>> +++ b/arch/powerpc/kernel/dt_cpu_ftrs.c
>> @@ -353,7 +353,7 @@ static void init_pmu_power8(void)
>> }
>>
>> mtspr(SPRN_MMCRA, 0);
>> - mtspr(SPRN_MMCR0, 0);
>> + mtspr(SPRN_MMCR0, MMCR0_FC);
>> mtspr(SPRN_MMCR1, 0);
>> mtspr(SPRN_MMCR2, 0);
>> mtspr(SPRN_MMCRS, 0);
>> @@ -392,7 +392,7 @@ static void init_pmu_power9(void)
>> mtspr(SPRN_MMCRC, 0);
>>
>> mtspr(SPRN_MMCRA, 0);
>> - mtspr(SPRN_MMCR0, 0);
>> + mtspr(SPRN_MMCR0, MMCR0_FC);
>> mtspr(SPRN_MMCR1, 0);
>> mtspr(SPRN_MMCR2, 0);
>> }
>> @@ -428,7 +428,7 @@ static void init_pmu_power10(void)
>>
>> mtspr(SPRN_MMCR3, 0);
>> mtspr(SPRN_MMCRA, MMCRA_BHRB_DISABLE);
>> - mtspr(SPRN_MMCR0, MMCR0_PMCCEXT);
>> + mtspr(SPRN_MMCR0, MMCR0_FC | MMCR0_PMCCEXT);
>> }
>>
>> static int __init feat_enable_pmu_power10(struct dt_cpu_feature *f)
>> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
>> index 1f30f98b09d1..f7349d150828 100644
>> --- a/arch/powerpc/kvm/book3s_hv.c
>> +++ b/arch/powerpc/kvm/book3s_hv.c
>> @@ -2593,6 +2593,11 @@ static int kvmppc_core_vcpu_create_hv(struct kvm_vcpu *vcpu)
>> #endif
>> #endif
>> vcpu->arch.mmcr[0] = MMCR0_FC;
>> + if (cpu_has_feature(CPU_FTR_ARCH_31)) {
>> + vcpu->arch.mmcr[0] |= MMCR0_PMCCEXT;
>> + vcpu->arch.mmcra = MMCRA_BHRB_DISABLE;
>> + }
>> +
>> vcpu->arch.ctrl = CTRL_RUNLATCH;
>> /* default to host PVR, since we can't spoof it */
>> kvmppc_set_pvr_hv(vcpu, mfspr(SPRN_PVR));
>> diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
>> index 51622411a7cc..e33b29ec1a65 100644
>> --- a/arch/powerpc/perf/core-book3s.c
>> +++ b/arch/powerpc/perf/core-book3s.c
>> @@ -1361,6 +1361,13 @@ static void power_pmu_enable(struct pmu *pmu)
>> goto out;
>>
>> if (cpuhw->n_events == 0) {
>> + if (cpu_has_feature(CPU_FTR_ARCH_31)) {
>> + mtspr(SPRN_MMCRA, MMCRA_BHRB_DISABLE);
>> + mtspr(SPRN_MMCR0, MMCR0_FC | MMCR0_PMCCEXT);
>> + } else {
>> + mtspr(SPRN_MMCRA, 0);
>> + mtspr(SPRN_MMCR0, MMCR0_FC);
>> + }
>
>
> Hi Nick,
>
> We are setting these bits in “power_pmu_disable” function. And disable will be called before any event gets deleted/stopped. Can you please help to understand why this is needed in power_pmu_enable path also ?
I'll have to go back and check what I needed it for.
Basically what I'm trying to achieve is that when the guest and host
are not running perf, then the registers should match. This way the host
can test them with mfspr but does not need to fix them with mtspr.
It's not too important for performance after TM facility demand faulting
and the nested guest PMU fix means you can usually just skip that
entirely, but I think it's cleaner. I'll double check my perf/ changes
it's possible that part should be split out or is unnecessary.
Thanks,
Nick
^ permalink raw reply
* Re: [RFC PATCH 11/43] KVM: PPC: Book3S HV P9: Implement PMU save/restore in C
From: Nicholas Piggin @ 2021-07-12 2:49 UTC (permalink / raw)
To: Athira Rajeev; +Cc: linuxppc-dev, kvm-ppc
In-Reply-To: <A647F37A-F32C-46B7-8A2E-C4D7CDB012E3@linux.vnet.ibm.com>
Excerpts from Athira Rajeev's message of July 10, 2021 12:47 pm:
>
>
>> On 22-Jun-2021, at 4:27 PM, Nicholas Piggin <npiggin@gmail.com> wrote:
>>
>> Implement the P9 path PMU save/restore code in C, and remove the
>> POWER9/10 code from the P7/8 path assembly.
>>
>> -449 cycles (8533) POWER9 virt-mode NULL hcall
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> arch/powerpc/include/asm/asm-prototypes.h | 5 -
>> arch/powerpc/kvm/book3s_hv.c | 205 ++++++++++++++++++++--
>> arch/powerpc/kvm/book3s_hv_interrupts.S | 13 +-
>> arch/powerpc/kvm/book3s_hv_rmhandlers.S | 43 +----
>> 4 files changed, 200 insertions(+), 66 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/asm-prototypes.h b/arch/powerpc/include/asm/asm-prototypes.h
>> index 02ee6f5ac9fe..928db8ef9a5a 100644
>> --- a/arch/powerpc/include/asm/asm-prototypes.h
>> +++ b/arch/powerpc/include/asm/asm-prototypes.h
>> @@ -136,11 +136,6 @@ static inline void kvmppc_restore_tm_hv(struct kvm_vcpu *vcpu, u64 msr,
>> bool preserve_nv) { }
>> #endif /* CONFIG_PPC_TRANSACTIONAL_MEM */
>>
>> -void kvmhv_save_host_pmu(void);
>> -void kvmhv_load_host_pmu(void);
>> -void kvmhv_save_guest_pmu(struct kvm_vcpu *vcpu, bool pmu_in_use);
>> -void kvmhv_load_guest_pmu(struct kvm_vcpu *vcpu);
>> -
>> void kvmppc_p9_enter_guest(struct kvm_vcpu *vcpu);
>>
>> long kvmppc_h_set_dabr(struct kvm_vcpu *vcpu, unsigned long dabr);
>> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
>> index f7349d150828..b1b94b3563b7 100644
>> --- a/arch/powerpc/kvm/book3s_hv.c
>> +++ b/arch/powerpc/kvm/book3s_hv.c
>> @@ -3635,6 +3635,188 @@ static noinline void kvmppc_run_core(struct kvmppc_vcore *vc)
>> trace_kvmppc_run_core(vc, 1);
>> }
>>
>> +/*
>> + * Privileged (non-hypervisor) host registers to save.
>> + */
>> +struct p9_host_os_sprs {
>> + unsigned long dscr;
>> + unsigned long tidr;
>> + unsigned long iamr;
>> + unsigned long amr;
>> + unsigned long fscr;
>> +
>> + unsigned int pmc1;
>> + unsigned int pmc2;
>> + unsigned int pmc3;
>> + unsigned int pmc4;
>> + unsigned int pmc5;
>> + unsigned int pmc6;
>> + unsigned long mmcr0;
>> + unsigned long mmcr1;
>> + unsigned long mmcr2;
>> + unsigned long mmcr3;
>> + unsigned long mmcra;
>> + unsigned long siar;
>> + unsigned long sier1;
>> + unsigned long sier2;
>> + unsigned long sier3;
>> + unsigned long sdar;
>> +};
>> +
>> +static void freeze_pmu(unsigned long mmcr0, unsigned long mmcra)
>> +{
>> + if (!(mmcr0 & MMCR0_FC))
>> + goto do_freeze;
>> + if (mmcra & MMCRA_SAMPLE_ENABLE)
>> + goto do_freeze;
>> + if (cpu_has_feature(CPU_FTR_ARCH_31)) {
>> + if (!(mmcr0 & MMCR0_PMCCEXT))
>> + goto do_freeze;
>> + if (!(mmcra & MMCRA_BHRB_DISABLE))
>> + goto do_freeze;
>> + }
>> + return;
>
>
> Hi Nick
>
> When freezing the PMU, do we need to also set pmcregs_in_use to zero ?
Not immediately, we still need to save out the values of the PMU
registers. If we clear pmcregs_in_use, then our hypervisor can discard
the contents of those registers at any time.
> Also, why we need these above conditions like MMCRA_SAMPLE_ENABLE, MMCR0_PMCCEXT checks also before freezing ?
Basically just because that's the condition we wnat to set the PMU to
before entering the guest if the guest does not have its own PMU
registers in use.
I'm not entirely sure this is correct / optimal for perf though, so we
should double check that. I think some of this stuff should be wrapped
up and put into perf/ subdirectory as I've said before. KVM shouldn't
need to know about exactly how PMU is to be set up and managed by
perf, we should just be able to ask perf to save/restore/switch state.
Thanks,
Nick
^ permalink raw reply
* Re: [RFC PATCH 27/43] KVM: PPC: Book3S HV P9: Move host OS save/restore functions to built-in
From: Nicholas Piggin @ 2021-07-12 2:50 UTC (permalink / raw)
To: Athira Rajeev; +Cc: linuxppc-dev, kvm-ppc
In-Reply-To: <983C1FE6-79CB-4DBD-BD00-8CFDA3685FEB@linux.vnet.ibm.com>
Excerpts from Athira Rajeev's message of July 8, 2021 3:32 pm:
>
>
>> On 22-Jun-2021, at 4:27 PM, Nicholas Piggin <npiggin@gmail.com> wrote:
>>
>> Move the P9 guest/host register switching functions to the built-in
>> P9 entry code, and export it for nested to use as well.
>>
>> This allows more flexibility in scheduling these supervisor privileged
>> SPR accesses with the HV privileged and PR SPR accesses in the low level
>> entry code.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> arch/powerpc/kvm/book3s_hv.c | 351 +-------------------------
>> arch/powerpc/kvm/book3s_hv.h | 39 +++
>> arch/powerpc/kvm/book3s_hv_p9_entry.c | 332 ++++++++++++++++++++++++
>> 3 files changed, 372 insertions(+), 350 deletions(-)
>> create mode 100644 arch/powerpc/kvm/book3s_hv.h
>>
>> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
>> index 35749b0b663f..a7660af22161 100644
>> --- a/arch/powerpc/kvm/book3s_hv.c
>> +++ b/arch/powerpc/kvm/book3s_hv.c
>> @@ -79,6 +79,7 @@
>> #include <asm/dtl.h>
>>
>> #include "book3s.h"
>> +#include "book3s_hv.h"
>>
>> #define CREATE_TRACE_POINTS
>> #include "trace_hv.h"
>> @@ -3675,356 +3676,6 @@ static noinline void kvmppc_run_core(struct kvmppc_vcore *vc)
>> trace_kvmppc_run_core(vc, 1);
>> }
>>
>> -/*
>> - * Privileged (non-hypervisor) host registers to save.
>> - */
>> -struct p9_host_os_sprs {
>> - unsigned long dscr;
>> - unsigned long tidr;
>> - unsigned long iamr;
>> - unsigned long amr;
>> - unsigned long fscr;
>> -
>> - unsigned int pmc1;
>> - unsigned int pmc2;
>> - unsigned int pmc3;
>> - unsigned int pmc4;
>> - unsigned int pmc5;
>> - unsigned int pmc6;
>> - unsigned long mmcr0;
>> - unsigned long mmcr1;
>> - unsigned long mmcr2;
>> - unsigned long mmcr3;
>> - unsigned long mmcra;
>> - unsigned long siar;
>> - unsigned long sier1;
>> - unsigned long sier2;
>> - unsigned long sier3;
>> - unsigned long sdar;
>> -};
>> -
>> -static void freeze_pmu(unsigned long mmcr0, unsigned long mmcra)
>> -{
>> - if (!(mmcr0 & MMCR0_FC))
>> - goto do_freeze;
>> - if (mmcra & MMCRA_SAMPLE_ENABLE)
>> - goto do_freeze;
>> - if (cpu_has_feature(CPU_FTR_ARCH_31)) {
>> - if (!(mmcr0 & MMCR0_PMCCEXT))
>> - goto do_freeze;
>> - if (!(mmcra & MMCRA_BHRB_DISABLE))
>> - goto do_freeze;
>> - }
>> - return;
>> -
>> -do_freeze:
>> - mmcr0 = MMCR0_FC;
>> - mmcra = 0;
>> - if (cpu_has_feature(CPU_FTR_ARCH_31)) {
>> - mmcr0 |= MMCR0_PMCCEXT;
>> - mmcra = MMCRA_BHRB_DISABLE;
>> - }
>> -
>> - mtspr(SPRN_MMCR0, mmcr0);
>> - mtspr(SPRN_MMCRA, mmcra);
>> - isync();
>> -}
>> -
>> -static void switch_pmu_to_guest(struct kvm_vcpu *vcpu,
>> - struct p9_host_os_sprs *host_os_sprs)
>> -{
>> - struct lppaca *lp;
>> - int load_pmu = 1;
>> -
>> - lp = vcpu->arch.vpa.pinned_addr;
>> - if (lp)
>> - load_pmu = lp->pmcregs_in_use;
>> -
>> - if (load_pmu)
>> - vcpu->arch.hfscr |= HFSCR_PM;
>> -
>> - /* Save host */
>> - if (ppc_get_pmu_inuse()) {
>> - /*
>> - * It might be better to put PMU handling (at least for the
>> - * host) in the perf subsystem because it knows more about what
>> - * is being used.
>> - */
>> -
>> - /* POWER9, POWER10 do not implement HPMC or SPMC */
>> -
>> - host_os_sprs->mmcr0 = mfspr(SPRN_MMCR0);
>> - host_os_sprs->mmcra = mfspr(SPRN_MMCRA);
>> -
>> - freeze_pmu(host_os_sprs->mmcr0, host_os_sprs->mmcra);
>> -
>> - host_os_sprs->pmc1 = mfspr(SPRN_PMC1);
>> - host_os_sprs->pmc2 = mfspr(SPRN_PMC2);
>> - host_os_sprs->pmc3 = mfspr(SPRN_PMC3);
>> - host_os_sprs->pmc4 = mfspr(SPRN_PMC4);
>> - host_os_sprs->pmc5 = mfspr(SPRN_PMC5);
>> - host_os_sprs->pmc6 = mfspr(SPRN_PMC6);
>> - host_os_sprs->mmcr1 = mfspr(SPRN_MMCR1);
>> - host_os_sprs->mmcr2 = mfspr(SPRN_MMCR2);
>> - host_os_sprs->sdar = mfspr(SPRN_SDAR);
>> - host_os_sprs->siar = mfspr(SPRN_SIAR);
>> - host_os_sprs->sier1 = mfspr(SPRN_SIER);
>> -
>> - if (cpu_has_feature(CPU_FTR_ARCH_31)) {
>> - host_os_sprs->mmcr3 = mfspr(SPRN_MMCR3);
>> - host_os_sprs->sier2 = mfspr(SPRN_SIER2);
>> - host_os_sprs->sier3 = mfspr(SPRN_SIER3);
>> - }
>> - }
>> -
>> -#ifdef CONFIG_PPC_PSERIES
>> - if (kvmhv_on_pseries()) {
>> - if (vcpu->arch.vpa.pinned_addr) {
>> - struct lppaca *lp = vcpu->arch.vpa.pinned_addr;
>> - get_lppaca()->pmcregs_in_use = lp->pmcregs_in_use;
>> - } else {
>> - get_lppaca()->pmcregs_in_use = 1;
>> - }
>> - }
>> -#endif
>> -
>> - /* Load guest */
>> - if (vcpu->arch.hfscr & HFSCR_PM) {
>> - mtspr(SPRN_PMC1, vcpu->arch.pmc[0]);
>> - mtspr(SPRN_PMC2, vcpu->arch.pmc[1]);
>> - mtspr(SPRN_PMC3, vcpu->arch.pmc[2]);
>> - mtspr(SPRN_PMC4, vcpu->arch.pmc[3]);
>> - mtspr(SPRN_PMC5, vcpu->arch.pmc[4]);
>> - mtspr(SPRN_PMC6, vcpu->arch.pmc[5]);
>> - mtspr(SPRN_MMCR1, vcpu->arch.mmcr[1]);
>> - mtspr(SPRN_MMCR2, vcpu->arch.mmcr[2]);
>> - mtspr(SPRN_SDAR, vcpu->arch.sdar);
>> - mtspr(SPRN_SIAR, vcpu->arch.siar);
>> - mtspr(SPRN_SIER, vcpu->arch.sier[0]);
>> -
>> - if (cpu_has_feature(CPU_FTR_ARCH_31)) {
>> - mtspr(SPRN_MMCR3, vcpu->arch.mmcr[4]);
>
>
> Hi Nick,
>
> Have a doubt here..
> For MMCR3, it is vcpu->arch.mmcr[3) ?
Hey, yea it is you're right. Good catch.
Thanks,
Nick
^ permalink raw reply
* Re: [RFC PATCH v0 1/1] powerpc/percpu: Use 2MB atom_size in percpu allocator on radix
From: Nicholas Piggin @ 2021-07-12 3:00 UTC (permalink / raw)
To: Bharata B Rao, linuxppc-dev; +Cc: aneesh.kumar
In-Reply-To: <20210708052946.1497495-1-bharata@linux.ibm.com>
Excerpts from Bharata B Rao's message of July 8, 2021 3:29 pm:
> The atom_size used by percpu allocator on powerpc is currently
> determined by mmu_linear_psize which is initialized to 4K and
> mmu_linear_psize is modified only by hash. Till now for radix
> the atom_size was defaulting to PAGE_SIZE(64K).
Looks like it was 1MB to me?
> Go for 2MB
> atom_size on radix if support for 2MB pages exist.
>
> 2MB atom_size on radix will allow using PMD mappings in the
> vmalloc area if and when support for higher sized vmalloc
> mappings is enabled for the pecpu allocator. However right now
That would be nice.
> this change will result in more number of units to be allocated
> within one allocation due to increased upa(units per allocation).
In that case is there any reason to do it until then?
>
> Signed-off-by: Bharata B Rao <bharata@linux.ibm.com>
> ---
> arch/powerpc/kernel/setup_64.c | 34 +++++++++++++++++++++++++---------
> 1 file changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
> index 1ff258f6c76c..45ce2d6e8112 100644
> --- a/arch/powerpc/kernel/setup_64.c
> +++ b/arch/powerpc/kernel/setup_64.c
> @@ -871,6 +871,30 @@ static void __init pcpu_populate_pte(unsigned long addr)
> __func__, PAGE_SIZE, PAGE_SIZE, PAGE_SIZE);
> }
>
> +static size_t pcpu_atom_size(void)
> +{
> + size_t atom_size = PAGE_SIZE;
> +
> + /*
> + * Radix: Use PAGE_SIZE by default or 2M if available.
> + */
> + if (radix_enabled()) {
> + if (mmu_psize_defs[MMU_PAGE_2M].shift)
> + atom_size = 1 << mmu_psize_defs[MMU_PAGE_2M].shift;
Looks like this changes behaviour for radix.
Also mmu_psize_defs is a pretty horrible interface you only need it in
some low level instruction encodings. You already explicitly know it's
2MB there, so you can just PMD_SHIFT.
If you want to know whether huge PMD is supported and enabled in vmalloc
memory, you would have to add some check which also accounts for
vmap_allow_huge, so that would be another patch.
Thanks,
Nick
> + goto out;
> + }
> +
> + /*
> + * Hash: Linear mapping is one of 4K, 1M and 16M. For 4K, no need
> + * to group units. For larger mappings, use 1M atom which
> + * should be large enough to contain a number of units.
> + */
> + if (mmu_linear_psize != MMU_PAGE_4K)
> + atom_size = 1 << 20;
> +
> +out:
> + return atom_size;
> +}
>
> void __init setup_per_cpu_areas(void)
> {
> @@ -880,15 +904,7 @@ void __init setup_per_cpu_areas(void)
> unsigned int cpu;
> int rc = -EINVAL;
>
> - /*
> - * Linear mapping is one of 4K, 1M and 16M. For 4K, no need
> - * to group units. For larger mappings, use 1M atom which
> - * should be large enough to contain a number of units.
> - */
> - if (mmu_linear_psize == MMU_PAGE_4K)
> - atom_size = PAGE_SIZE;
> - else
> - atom_size = 1 << 20;
> + atom_size = pcpu_atom_size();
>
> if (pcpu_chosen_fc != PCPU_FC_PAGE) {
> rc = pcpu_embed_first_chunk(0, dyn_size, atom_size, pcpu_cpu_distance,
> --
> 2.31.1
>
>
^ permalink raw reply
* Re: [RFC PATCH 10/43] powerpc/64s: Always set PMU control registers to frozen/disabled when not in use
From: Madhavan Srinivasan @ 2021-07-12 3:42 UTC (permalink / raw)
To: Nicholas Piggin, kvm-ppc; +Cc: linuxppc-dev
In-Reply-To: <1625185125.n8jy7yqojr.astroid@bobo.none>
On 7/2/21 5:57 AM, Nicholas Piggin wrote:
> Excerpts from Madhavan Srinivasan's message of July 1, 2021 11:17 pm:
>> On 6/22/21 4:27 PM, Nicholas Piggin wrote:
>>> KVM PMU management code looks for particular frozen/disabled bits in
>>> the PMU registers so it knows whether it must clear them when coming
>>> out of a guest or not. Setting this up helps KVM make these optimisations
>>> without getting confused. Longer term the better approach might be to
>>> move guest/host PMU switching to the perf subsystem.
>>>
>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>> ---
>>> arch/powerpc/kernel/cpu_setup_power.c | 4 ++--
>>> arch/powerpc/kernel/dt_cpu_ftrs.c | 6 +++---
>>> arch/powerpc/kvm/book3s_hv.c | 5 +++++
>>> arch/powerpc/perf/core-book3s.c | 7 +++++++
>>> 4 files changed, 17 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/powerpc/kernel/cpu_setup_power.c b/arch/powerpc/kernel/cpu_setup_power.c
>>> index a29dc8326622..3dc61e203f37 100644
>>> --- a/arch/powerpc/kernel/cpu_setup_power.c
>>> +++ b/arch/powerpc/kernel/cpu_setup_power.c
>>> @@ -109,7 +109,7 @@ static void init_PMU_HV_ISA207(void)
>>> static void init_PMU(void)
>>> {
>>> mtspr(SPRN_MMCRA, 0);
>>> - mtspr(SPRN_MMCR0, 0);
>>> + mtspr(SPRN_MMCR0, MMCR0_FC);
>> Sticky point here is, currently if not frozen, pmc5/6 will
>> keep countering. And not freezing them at boot is quiet useful
>> sometime, like say when running in a simulation where we could calculate
>> approx CPIs for micro benchmarks without perf subsystem.
Sorry for a delayed response
> You even can't use the sysfs files in this sim environment? In that case
Yes it is possible. Just that we need to have additional patch to maintain
for the sim.
> what if we added a boot option that could set some things up? In that
> case possibly you could even gather some more types of events too.
Having a boot option will be a over-kill :). I guess we could have
this under config option?
>
> Thanks,
> Nick
^ permalink raw reply
* Re: [RFC PATCH v0 1/1] powerpc/percpu: Use 2MB atom_size in percpu allocator on radix
From: Bharata B Rao @ 2021-07-12 4:05 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: aneesh.kumar, linuxppc-dev
In-Reply-To: <1626058374.3xlvhrcly8.astroid@bobo.none>
On Mon, Jul 12, 2021 at 01:00:10PM +1000, Nicholas Piggin wrote:
> Excerpts from Bharata B Rao's message of July 8, 2021 3:29 pm:
> > The atom_size used by percpu allocator on powerpc is currently
> > determined by mmu_linear_psize which is initialized to 4K and
> > mmu_linear_psize is modified only by hash. Till now for radix
> > the atom_size was defaulting to PAGE_SIZE(64K).
>
> Looks like it was 1MB to me?
Was it hash? Because atom_size will get set to 1MB on hash.
And both on baremetal and KVM radix, I see 64K atom_size.
>
> > Go for 2MB
> > atom_size on radix if support for 2MB pages exist.
> >
> > 2MB atom_size on radix will allow using PMD mappings in the
> > vmalloc area if and when support for higher sized vmalloc
> > mappings is enabled for the pecpu allocator. However right now
>
> That would be nice.
>
> > this change will result in more number of units to be allocated
> > within one allocation due to increased upa(units per allocation).
>
> In that case is there any reason to do it until then?
Not strictly. I observed a similar setting on x86 which has
been there for long, so was just checking if it makes sense
here too.
>
> >
> > Signed-off-by: Bharata B Rao <bharata@linux.ibm.com>
> > ---
> > arch/powerpc/kernel/setup_64.c | 34 +++++++++++++++++++++++++---------
> > 1 file changed, 25 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
> > index 1ff258f6c76c..45ce2d6e8112 100644
> > --- a/arch/powerpc/kernel/setup_64.c
> > +++ b/arch/powerpc/kernel/setup_64.c
> > @@ -871,6 +871,30 @@ static void __init pcpu_populate_pte(unsigned long addr)
> > __func__, PAGE_SIZE, PAGE_SIZE, PAGE_SIZE);
> > }
> >
> > +static size_t pcpu_atom_size(void)
> > +{
> > + size_t atom_size = PAGE_SIZE;
> > +
> > + /*
> > + * Radix: Use PAGE_SIZE by default or 2M if available.
> > + */
> > + if (radix_enabled()) {
> > + if (mmu_psize_defs[MMU_PAGE_2M].shift)
> > + atom_size = 1 << mmu_psize_defs[MMU_PAGE_2M].shift;
>
> Looks like this changes behaviour for radix.
Yes, it does as it increases the atom_size which results in higher
upa as noted. Did you mean some other behaviour change?
>
> Also mmu_psize_defs is a pretty horrible interface you only need it in
> some low level instruction encodings. You already explicitly know it's
> 2MB there, so you can just PMD_SHIFT.
Ok.
>
> If you want to know whether huge PMD is supported and enabled in vmalloc
> memory, you would have to add some check which also accounts for
> vmap_allow_huge, so that would be another patch.
Yes makes sense if we want to tie the setting of higher atom_size
to actual availability of PMD mappings in vmalloc.
Regards,
Bharata.
^ permalink raw reply
* Re: [PATCH v2 1/1] powerpc/pseries: Interface to represent PAPR firmware attributes
From: Pratik Sampat @ 2021-07-12 7:43 UTC (permalink / raw)
To: ego; +Cc: pratik.r.sampat, linux-kernel, kvm-ppc, paulus, linuxppc-dev
In-Reply-To: <20210708103543.GA20804@in.ibm.com>
On 08/07/21 4:05 pm, Gautham R Shenoy wrote:
> Hello Pratik,
>
> On Tue, Jul 06, 2021 at 01:54:00PM +0530, Pratik R. Sampat wrote:
>> Adds a generic interface to represent the energy and frequency related
>> PAPR attributes on the system using the new H_CALL
>> "H_GET_ENERGY_SCALE_INFO".
>>
>> H_GET_EM_PARMS H_CALL was previously responsible for exporting this
>> information in the lparcfg, however the H_GET_EM_PARMS H_CALL
>> will be deprecated P10 onwards.
>>
>> The H_GET_ENERGY_SCALE_INFO H_CALL is of the following call format:
>> hcall(
>> uint64 H_GET_ENERGY_SCALE_INFO, // Get energy scale info
>> uint64 flags, // Per the flag request
>> uint64 firstAttributeId,// The attribute id
>> uint64 bufferAddress, // Guest physical address of the output buffer
>> uint64 bufferSize // The size in bytes of the output buffer
>> );
>>
>> This H_CALL can query either all the attributes at once with
>> firstAttributeId = 0, flags = 0 as well as query only one attribute
>> at a time with firstAttributeId = id, flags = 1.
>>
>> The output buffer consists of the following
>> 1. number of attributes - 8 bytes
>> 2. array offset to the data location - 8 bytes
>> 3. version info - 1 byte
>> 4. A data array of size num attributes, which contains the following:
>> a. attribute ID - 8 bytes
>> b. attribute value in number - 8 bytes
>> c. attribute name in string - 64 bytes
>> d. attribute value in string - 64 bytes
>>
>> The new H_CALL exports information in direct string value format, hence
>> a new interface has been introduced in
>> /sys/firmware/papr/energy_scale_info to export this information to
>> userspace in an extensible pass-through format.
>>
>> The H_CALL returns the name, numeric value and string value (if exists)
>>
>> The format of exposing the sysfs information is as follows:
>> /sys/firmware/papr/energy_scale_info/
>> |-- <id>/
>> |-- desc
>> |-- value
>> |-- value_desc (if exists)
>> |-- <id>/
>> |-- desc
>> |-- value
>> |-- value_desc (if exists)
>> ...
>
> I like this implementation. Only one comment w.r.t versioning below:
>
> [..snip..]
>
>> @@ -631,6 +632,26 @@ struct hv_gpci_request_buffer {
>> uint8_t bytes[HGPCI_MAX_DATA_BYTES];
>> } __packed;
>>
>> +#define MAX_ESI_ATTRS 10
>> +#define MAX_BUF_SZ (sizeof(struct h_energy_scale_info_hdr) + \
>> + (sizeof(struct energy_scale_attribute) * MAX_ESI_ATTRS))
>> +
>> +struct energy_scale_attribute {
>> + __be64 id;
>> + __be64 value;
>> + unsigned char desc[64];
>> + unsigned char value_desc[64];
>> +} __packed;
>> +
>> +struct h_energy_scale_info_hdr {
>> + __be64 num_attrs;
>> + __be64 array_offset;
>> + __u8 data_header_version;
>> +} __packed;
>> +
>
> [..snip..]
>
>> +static int __init papr_init(void)
>> +{
>> + uint64_t num_attrs;
>> + int ret, idx, i;
>> + char *esi_buf;
>> +
>> + if (!firmware_has_feature(FW_FEATURE_LPAR))
>> + return -ENXIO;
>> +
>> + esi_buf = kmalloc(MAX_BUF_SZ, GFP_KERNEL);
>> + if (esi_buf == NULL)
>> + return -ENOMEM;
>> + /*
>> + * hcall(
>> + * uint64 H_GET_ENERGY_SCALE_INFO, // Get energy scale info
>> + * uint64 flags, // Per the flag request
>> + * uint64 firstAttributeId, // The attribute id
>> + * uint64 bufferAddress, // Guest physical address of the output buffer
>> + * uint64 bufferSize); // The size in bytes of the output buffer
>> + */
>> + ret = plpar_hcall_norets(H_GET_ENERGY_SCALE_INFO, ESI_FLAGS_ALL, 0,
>> + virt_to_phys(esi_buf), MAX_BUF_SZ);
>
> It is good that you are passing a character buffer here and
> interpreting it later. This helps in the cases where the header has
> more fields than what we are aware of changed. I think even a future
> platform adds newer fields to the header, those fields will be
> appended after the existing fields, so we should still be able to
> interpret the first three fields of the header that we are currently
> aware of.
>
>
>> + if (ret != H_SUCCESS) {
>> + pr_warn("hcall failed: H_GET_ENERGY_SCALE_INFO");
>> + goto out;
>> + }
>> +
>> + esi_hdr = (struct h_energy_scale_info_hdr *) esi_buf;
>> + num_attrs = be64_to_cpu(esi_hdr->num_attrs);
>
> Shouldn't we check for the esi_hdr->data_header_version here?
> Currently we are only aware of the version 1. If we happen to run this
> kernel code on a future platform which supports a different version,
> wouldn't it be safer to bail out here ?
>
> Otherwise this patch looks good to me.
>
> Reviewed-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
Thanks for the review Gautham.
Sure I'll make use of the header version to bail out.
That just seems like the safest approach.
I'll add that and spin a new version here
Thanks
Pratik
> --
> Thanks and Regards
> gautham.
^ permalink raw reply
* Re: [PATCH] memblock: make for_each_mem_range() traverse MEMBLOCK_HOTPLUG regions
From: Greg Kurz @ 2021-07-12 7:50 UTC (permalink / raw)
To: Mike Rapoport
Cc: linux-kernel, stable, Mike Rapoport, linux-mm, Andrew Morton,
linuxppc-dev
In-Reply-To: <20210712071132.20902-1-rppt@kernel.org>
On Mon, 12 Jul 2021 10:11:32 +0300
Mike Rapoport <rppt@kernel.org> wrote:
> From: Mike Rapoport <rppt@linux.ibm.com>
>
> Commit b10d6bca8720 ("arch, drivers: replace for_each_membock() with
> for_each_mem_range()") didn't take into account that when there is
> movable_node parameter in the kernel command line, for_each_mem_range()
> would skip ranges marked with MEMBLOCK_HOTPLUG.
>
> The page table setup code in POWER uses for_each_mem_range() to create the
> linear mapping of the physical memory and since the regions marked as
> MEMORY_HOTPLUG are skipped, they never make it to the linear map.
>
> A later access to the memory in those ranges will fail:
>
> [ 2.271743] BUG: Unable to handle kernel data access on write at 0xc000000400000000
> [ 2.271984] Faulting instruction address: 0xc00000000008a3c0
> [ 2.272568] Oops: Kernel access of bad area, sig: 11 [#1]
> [ 2.272683] LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 NUMA pSeries
> [ 2.273063] Modules linked in:
> [ 2.273435] CPU: 0 PID: 53 Comm: kworker/u2:0 Not tainted 5.13.0 #7
> [ 2.273832] NIP: c00000000008a3c0 LR: c0000000003c1ed8 CTR: 0000000000000040
> [ 2.273918] REGS: c000000008a57770 TRAP: 0300 Not tainted (5.13.0)
> [ 2.274036] MSR: 8000000002009033 <SF,VEC,EE,ME,IR,DR,RI,LE> CR: 84222202 XER: 20040000
> [ 2.274454] CFAR: c0000000003c1ed4 DAR: c000000400000000 DSISR: 42000000 IRQMASK: 0
> [ 2.274454] GPR00: c0000000003c1ed8 c000000008a57a10 c0000000019da700 c000000400000000
> [ 2.274454] GPR04: 0000000000000280 0000000000000180 0000000000000400 0000000000000200
> [ 2.274454] GPR08: 0000000000000100 0000000000000080 0000000000000040 0000000000000300
> [ 2.274454] GPR12: 0000000000000380 c000000001bc0000 c0000000001660c8 c000000006337e00
> [ 2.274454] GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> [ 2.274454] GPR20: 0000000040000000 0000000020000000 c000000001a81990 c000000008c30000
> [ 2.274454] GPR24: c000000008c20000 c000000001a81998 000fffffffff0000 c000000001a819a0
> [ 2.274454] GPR28: c000000001a81908 c00c000001000000 c000000008c40000 c000000008a64680
> [ 2.275520] NIP [c00000000008a3c0] clear_user_page+0x50/0x80
> [ 2.276333] LR [c0000000003c1ed8] __handle_mm_fault+0xc88/0x1910
> [ 2.276688] Call Trace:
> [ 2.276839] [c000000008a57a10] [c0000000003c1e94] __handle_mm_fault+0xc44/0x1910 (unreliable)
> [ 2.277142] [c000000008a57af0] [c0000000003c2c90] handle_mm_fault+0x130/0x2a0
> [ 2.277331] [c000000008a57b40] [c0000000003b5f08] __get_user_pages+0x248/0x610
> [ 2.277541] [c000000008a57c40] [c0000000003b848c] __get_user_pages_remote+0x12c/0x3e0
> [ 2.277768] [c000000008a57cd0] [c000000000473f24] get_arg_page+0x54/0xf0
> [ 2.277959] [c000000008a57d10] [c000000000474a7c] copy_string_kernel+0x11c/0x210
> [ 2.278159] [c000000008a57d80] [c00000000047663c] kernel_execve+0x16c/0x220
> [ 2.278361] [c000000008a57dd0] [c000000000166270] call_usermodehelper_exec_async+0x1b0/0x2f0
> [ 2.278543] [c000000008a57e10] [c00000000000d5ec] ret_from_kernel_thread+0x5c/0x70
> [ 2.278870] Instruction dump:
> [ 2.279214] 79280fa4 79271764 79261f24 794ae8e2 7ca94214 7d683a14 7c893a14 7d893050
> [ 2.279416] 7d4903a6 60000000 60000000 60000000 <7c001fec> 7c091fec 7c081fec 7c051fec
> [ 2.280193] ---[ end trace 490b8c67e6075e09 ]---
>
> Making for_each_mem_range() include MEMBLOCK_HOTPLUG regions in the
> traversal fixes this issue.
>
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=1976100
> Fixes: b10d6bca8720 ("arch, drivers: replace for_each_membock() with for_each_mem_range()")
> Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
> ---
This fixes the issue I was observing with both radix and hash.
Thanks !
Tested-by: Greg Kurz <groug@kaod.org>
Cc'ing linuxppc-dev so that POWER folks know about the fix
and stable.
Cc: stable@vger.kernel.org # v5.10
> include/linux/memblock.h | 4 ++--
> mm/memblock.c | 3 ++-
> 2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/memblock.h b/include/linux/memblock.h
> index cbf46f56d105..4a53c3ca86bd 100644
> --- a/include/linux/memblock.h
> +++ b/include/linux/memblock.h
> @@ -209,7 +209,7 @@ static inline void __next_physmem_range(u64 *idx, struct memblock_type *type,
> */
> #define for_each_mem_range(i, p_start, p_end) \
> __for_each_mem_range(i, &memblock.memory, NULL, NUMA_NO_NODE, \
> - MEMBLOCK_NONE, p_start, p_end, NULL)
> + MEMBLOCK_HOTPLUG, p_start, p_end, NULL)
>
> /**
> * for_each_mem_range_rev - reverse iterate through memblock areas from
> @@ -220,7 +220,7 @@ static inline void __next_physmem_range(u64 *idx, struct memblock_type *type,
> */
> #define for_each_mem_range_rev(i, p_start, p_end) \
> __for_each_mem_range_rev(i, &memblock.memory, NULL, NUMA_NO_NODE, \
> - MEMBLOCK_NONE, p_start, p_end, NULL)
> + MEMBLOCK_HOTPLUG, p_start, p_end, NULL)
>
> /**
> * for_each_reserved_mem_range - iterate over all reserved memblock areas
> diff --git a/mm/memblock.c b/mm/memblock.c
> index 0041ff62c584..de7b553baa50 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -947,7 +947,8 @@ static bool should_skip_region(struct memblock_type *type,
> return true;
>
> /* skip hotpluggable memory regions if needed */
> - if (movable_node_is_enabled() && memblock_is_hotpluggable(m))
> + if (movable_node_is_enabled() && memblock_is_hotpluggable(m) &&
> + !(flags & MEMBLOCK_HOTPLUG))
> return true;
>
> /* if we want mirror memory skip non-mirror memory regions */
>
> base-commit: e73f0f0ee7541171d89f2e2491130c7771ba58d3
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox