Linaro-mm-sig August 2024

linaro-mm-sig@lists.linaro.org

14 participants
29 discussions

Re: [PATCH 2/2] mtd: rawnand: nuvoton: add new driver for the Nuvoton MA35 SoC

by Krzysztof Kozlowski

On 12/08/2024 05:00, Hui-Ping Chen wrote: > Nuvoton MA35 SoCs NAND Flash Interface Controller > supports 2KB, 4KB and 8KB page size, and up to 8-bit, > 12-bit, and 24-bit hardware ECC calculation circuit > to protect data communication. > > Signed-off-by: Hui-Ping Chen <hpchen0nvt(a)gmail.com> > --- > drivers/mtd/nand/raw/Kconfig | 8 + > drivers/mtd/nand/raw/Makefile | 1 + > drivers/mtd/nand/raw/nuvoton_ma35d1_nand.c | 1109 ++++++++++++++++++++ > 3 files changed, 1118 insertions(+) > create mode 100644 drivers/mtd/nand/raw/nuvoton_ma35d1_nand.c > > diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig > index 614257308516..932bf2215470 100644 > --- a/drivers/mtd/nand/raw/Kconfig > +++ b/drivers/mtd/nand/raw/Kconfig > @@ -448,6 +448,14 @@ config MTD_NAND_RENESAS > Enables support for the NAND controller found on Renesas R-Car > Gen3 and RZ/N1 SoC families. > > +config MTD_NAND_NVT_MA35 > + tristate "Nuvoton MA35 SoC NAND controller" > + depends on ARCH_MA35 || COMPILE_TEST > + depends on OF > + help > + Enables support for the NAND controller found on > + the Nuvoton MA35 series SoCs. > + > comment "Misc" > > config MTD_SM_COMMON > diff --git a/drivers/mtd/nand/raw/Makefile b/drivers/mtd/nand/raw/Makefile > index 25120a4afada..cdfdfee3f5f3 100644 > --- a/drivers/mtd/nand/raw/Makefile > +++ b/drivers/mtd/nand/raw/Makefile > @@ -57,6 +57,7 @@ obj-$(CONFIG_MTD_NAND_INTEL_LGM) += intel-nand-controller.o > obj-$(CONFIG_MTD_NAND_ROCKCHIP) += rockchip-nand-controller.o > obj-$(CONFIG_MTD_NAND_PL35X) += pl35x-nand-controller.o > obj-$(CONFIG_MTD_NAND_RENESAS) += renesas-nand-controller.o > +obj-$(CONFIG_MTD_NAND_NVT_MA35) += nuvoton_ma35d1_nand.o > > nand-objs := nand_base.o nand_legacy.o nand_bbt.o nand_timings.o nand_ids.o > nand-objs += nand_onfi.o > diff --git a/drivers/mtd/nand/raw/nuvoton_ma35d1_nand.c b/drivers/mtd/nand/raw/nuvoton_ma35d1_nand.c > new file mode 100644 > index 000000000000..7445eb489c38 > --- /dev/null > +++ b/drivers/mtd/nand/raw/nuvoton_ma35d1_nand.c > @@ -0,0 +1,1109 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2024 Nuvoton Technology Corp. > + */ > +#include <linux/slab.h> > +#include <linux/init.h> > +#include <linux/module.h> > +#include <linux/interrupt.h> > +#include <linux/io.h> > +#include <linux/platform_device.h> > +#include <linux/delay.h> > +#include <linux/clk.h> > +#include <linux/err.h> > +#include <linux/of.h> > + > +#include <linux/mtd/mtd.h> > +#include <linux/mtd/partitions.h> > +#include <linux/mtd/rawnand.h> > +#include <linux/dma-mapping.h> > +#include <linux/dmaengine.h> > + > +/* NFI DMA Registers */ > +#define MA35_NFI_REG_BUFFER0 (0x000) > +#define MA35_NFI_REG_DMACTL (0x400) > +#define DMA_EN BIT(0) > +#define DMA_RST BIT(1) > +#define DMA_BUSY BIT(9) > + > +#define MA35_NFI_REG_DMASA (0x408) > +#define MA35_NFI_REG_DMABCNT (0x40C) > +#define MA35_NFI_REG_DMAINTEN (0x410) > +#define MA35_NFI_REG_DMAINTSTS (0x414) > + > +/* NFI Global Registers */ > +#define MA35_NFI_REG_GCTL (0x800) > +#define NAND_EN BIT(3) > +#define MA35_NFI_REG_GINTEN (0x804) > +#define MA35_NFI_REG_GINTSTS (0x808) > + > +/* NAND-type Flash Registers */ > +#define MA35_NFI_REG_NANDCTL (0x8A0) > +#define SWRST BIT(0) > +#define DMA_W_EN BIT(1) > +#define DMA_R_EN BIT(2) > +#define ECC_CHK BIT(7) > +#define PROT3BEN BIT(8) > +#define PSIZE_2K (1 << 16) > +#define PSIZE_4K (2 << 16) > +#define PSIZE_8K (3 << 16) > +#define PSIZE_MASK (3 << 16) > +#define BCH_T24 BIT(18) > +#define BCH_T8 BIT(20) > +#define BCH_T12 BIT(21) > +#define BCH_MASK (0x1f << 18) > +#define ECC_EN BIT(23) > +#define DISABLE_CS0 BIT(25) > + > +#define MA35_NFI_REG_NANDTMCTL (0x8A4) > +#define MA35_NFI_REG_NANDINTEN (0x8A8) > +#define MA35_NFI_REG_NANDINTSTS (0x8AC) > +#define INT_DMA BIT(0) > +#define INT_ECC BIT(2) > +#define INT_RB0 BIT(10) > +#define INT_RB0_STS BIT(18) > + > +#define MA35_NFI_REG_NANDCMD (0x8B0) > +#define MA35_NFI_REG_NANDADDR (0x8B4) > +#define ENDADDR BIT(31) > + > +#define MA35_NFI_REG_NANDDATA (0x8B8) > +#define MA35_NFI_REG_NANDRACTL (0x8BC) > +#define MA35_NFI_REG_NANDECTL (0x8C0) > +#define ENABLE_WP (0x0) > +#define DISABLE_WP BIT(0) > + > +#define MA35_NFI_REG_NANDECCES0 (0x8D0) > +#define ECC_STATUS_MASK (0x3) > +#define ECC_ERR_CNT_MASK (0x1f) > + > +#define MA35_NFI_REG_NANDECCES1 (0x8D4) > +#define MA35_NFI_REG_NANDECCES2 (0x8D8) > +#define MA35_NFI_REG_NANDECCES3 (0x8DC) > + > +/* NAND-type Flash BCH Error Address Registers */ > +#define MA35_NFI_REG_NANDECCEA0 (0x900) > +#define MA35_NFI_REG_NANDECCEA1 (0x904) > +#define MA35_NFI_REG_NANDECCEA2 (0x908) > +#define MA35_NFI_REG_NANDECCEA3 (0x90C) > +#define MA35_NFI_REG_NANDECCEA4 (0x910) > +#define MA35_NFI_REG_NANDECCEA5 (0x914) > +#define MA35_NFI_REG_NANDECCEA6 (0x918) > +#define MA35_NFI_REG_NANDECCEA7 (0x91C) > +#define MA35_NFI_REG_NANDECCEA8 (0x920) > +#define MA35_NFI_REG_NANDECCEA9 (0x924) > +#define MA35_NFI_REG_NANDECCEA10 (0x928) > +#define MA35_NFI_REG_NANDECCEA11 (0x92C) > + > +/* NAND-type Flash BCH Error Data Registers */ > +#define MA35_NFI_REG_NANDECCED0 (0x960) > +#define MA35_NFI_REG_NANDECCED1 (0x964) > +#define MA35_NFI_REG_NANDECCED2 (0x968) > +#define MA35_NFI_REG_NANDECCED3 (0x96C) > +#define MA35_NFI_REG_NANDECCED4 (0x970) > +#define MA35_NFI_REG_NANDECCED5 (0x974) > + > +/* NAND-type Flash Redundant Area Registers */ > +#define MA35_NFI_REG_NANDRA0 (0xA00) > +#define MA35_NFI_REG_NANDRA1 (0xA04) > + > +#define SKIP_SPARE_BYTES 4 > + > +/* BCH algorithm related constants and variables */ > +enum { > + eBCH_NONE = 0, > + eBCH_T8, > + eBCH_T12, > + eBCH_T24, > + eBCH_CNT > +} E_BCHALGORITHM; > + > +static const int g_i32BCHAlgoIdx[eBCH_CNT] = {BCH_T8, BCH_T8, BCH_T12, BCH_T24}; > +static struct nand_ecclayout_user ma35_nand_oob; Why this is file-scope? > +static const int g_i32ParityNum[3][eBCH_CNT] = { This is not a Linux coding style. Do not send Windows or some other platform-independent patches to review. > + {0, 60, 92, 90}, /* for 2K */ > + {0, 120, 184, 180}, /* for 4K */ > + {0, 240, 368, 360}, /* for 8K */ > +}; > + > + > +struct ma35_nand_info { > + struct nand_controller controller; > + struct device *dev; > + void __iomem *regs; > + int irq; > + struct clk *clk; > + struct completion complete; > + > + struct mtd_info mtd; > + struct nand_chip chip; > + struct mtd_partition *parts; > + int nr_parts; > + > + int eBCHAlgo; No, use Linux coding style. > + u8 *dma_buf; > + spinlock_t dma_lock; > + dma_addr_t dma_addr; > +}; > + > +static int ma35_ooblayout_ecc(struct mtd_info *mtd, int section, > + struct mtd_oob_region *oobregion) > +{ > + struct nand_chip *chip = mtd_to_nand(mtd); > + struct nand_ecc_ctrl *ecc = &chip->ecc; > + > + if (section || !ecc->total) > + return -ERANGE; > + > + oobregion->length = ecc->total; > + oobregion->offset = mtd->oobsize - oobregion->length; > + > + return 0; > +} ... this code looks poor, really poor. > +} > + > +static int ma35_nfi_correct(struct nand_chip *chip, unsigned long addr) > +{ > + struct ma35_nand_info *nand = nand_get_controller_data(chip); > + struct mtd_info *mtd = nand_to_mtd(chip); > + int uStatus, i, j, i32FieldNum = 0; > + int uReportErrCnt = 0; > + int uErrorCnt = 0; > + More of weird coding style. Use Linux coding style. Patch is for Linux. > + if ((readl(nand->regs + MA35_NFI_REG_NANDCTL) & BCH_MASK) == BCH_T24) > + i32FieldNum = mtd->writesize / 1024; > + else > + i32FieldNum = mtd->writesize / 512; ... > +static int ma35_nand_probe(struct platform_device *pdev) > +{ > + struct ma35_nand_info *nand; > + struct nand_chip *chip; > + struct mtd_info *mtd; > + int retval = 0; > + > + nand = devm_kzalloc(&pdev->dev, sizeof(struct ma35_nand_info), GFP_KERNEL); sizeof(*) > + if (!nand) > + return -ENOMEM; > + > + nand_controller_init(&nand->controller); > + > + nand->regs = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(nand->regs)) > + return PTR_ERR(nand->regs); > + > + nand->dev = &pdev->dev; > + chip = &nand->chip; > + mtd = nand_to_mtd(chip); > + nand_set_controller_data(chip, nand); > + nand_set_flash_node(chip, pdev->dev.of_node); > + > + mtd->priv = chip; > + mtd->owner = THIS_MODULE; > + mtd->dev.parent = &pdev->dev; > + > + nand->clk = of_clk_get(pdev->dev.of_node, 0); No, no. That's not how you get a clock. Use devm_clk_get(). > + if (IS_ERR(nand->clk)) > + return dev_err_probe(&pdev->dev, PTR_ERR(nand->clk), > + "failed to find nand clock\n"); > + > + retval = clk_prepare_enable(nand->clk); > + if (retval < 0) { > + dev_err(&pdev->dev, "Failed to enable clock\n"); > + return -ENXIO; > + } > + > + nand->chip.controller = &nand->controller; > + > + chip->legacy.cmdfunc = ma35_nand_command; > + chip->legacy.waitfunc = ma35_waitfunc; > + chip->legacy.read_byte = ma35_nand_read_byte; > + chip->legacy.select_chip = ma35_nand_select_chip; > + chip->legacy.read_buf = ma35_read_buf_dma; > + chip->legacy.write_buf = ma35_write_buf_dma; > + chip->legacy.dev_ready = ma35_nand_devready; > + chip->legacy.chip_delay = 25; /* us */ > + > + /* Read OOB data first, then HW read page */ > + chip->ecc.hwctl = ma35_nand_enable_hwecc; > + chip->ecc.calculate = ma35_nand_calculate_ecc; > + chip->ecc.correct = ma35_nand_correct_data; > + chip->ecc.write_page = ma35_nand_write_page_hwecc; > + chip->ecc.read_page = ma35_nand_read_page_hwecc_oob_first; > + chip->ecc.read_oob = ma35_nand_read_oob_hwecc; > + chip->options |= (NAND_NO_SUBPAGE_WRITE | NAND_USES_DMA); > + > + ma35_nand_initialize(nand); > + platform_set_drvdata(pdev, nand); > + > + nand->controller.ops = &ma35_nand_controller_ops; > + > + nand->irq = platform_get_irq(pdev, 0); > + if (nand->irq < 0) { > + dev_err(&pdev->dev, "failed to get platform irq\n"); Syntax is return dev_err_probe > + return -EINVAL; > + } > + > + if (request_irq(nand->irq, (irq_handler_t)&ma35_nand_irq, > + IRQF_TRIGGER_HIGH, "ma35d1-nand", nand)) { > + dev_err(&pdev->dev, "Error requesting NAND IRQ\n"); > + return -ENXIO; > + } > + > + retval = nand_scan(chip, 1); > + if (retval) > + return retval; > + > + if (mtd_device_register(mtd, nand->parts, nand->nr_parts)) { > + nand_cleanup(chip); > + devm_kfree(&pdev->dev, nand); > + return retval; > + } > + > + pr_info("ma35-nfi: registered successfully! mtdid=%s\n", mtd->name); Drop, does not help. Drivers do not use pr_xxx anyway. Why your code is so inconsistent? It's like random junk glued together. > + > + return retval; > +} > + > +static void ma35_nand_remove(struct platform_device *pdev) > +{ > + struct ma35_nand_info *nand = platform_get_drvdata(pdev); > + struct nand_chip *chip = &nand->chip; > + int ret; > + > + ret = mtd_device_unregister(nand_to_mtd(chip)); > + WARN_ON(ret); > + nand_cleanup(chip); > + > + clk_disable_unprepare(nand->clk); > + > + kfree(nand); > + platform_set_drvdata(pdev, NULL); > +} > + > +/* PM Support */ > +#ifdef CONFIG_PM > +static int ma35_nand_suspend(struct platform_device *pdev, pm_message_t pm) > +{ > + struct ma35_nand_info *nand = platform_get_drvdata(pdev); > + unsigned long timeo = jiffies + HZ/2; > + > + /* wait DMAC to ready */ > + while (1) { > + if ((readl(nand->regs + MA35_NFI_REG_DMACTL) & DMA_BUSY) == 0) > + break; > + if (time_after(jiffies, timeo)) > + return -ETIMEDOUT; > + } > + > + clk_disable(nand->clk); > + > + return 0; > +} > + > +static int ma35_nand_resume(struct platform_device *pdev) > +{ > + struct ma35_nand_info *nand = platform_get_drvdata(pdev); > + > + clk_enable(nand->clk); > + ma35_nand_hwecc_init(nand); > + ma35_nand_dmac_init(nand); > + > + return 0; > +} > + > +#else > +#define ma35_nand_suspend NULL > +#define ma35_nand_resume NULL > +#endif > + > +static const struct of_device_id ma35_nfi_of_match[] = { > + { .compatible = "nuvoton,ma35d1-nand" }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, ma35_nfi_of_match); > + > +static struct platform_driver ma35_nand_driver = { > + .driver = { > + .name = "ma35d1-nand", > + .owner = THIS_MODULE, > + .of_match_table = of_match_ptr(ma35_nfi_of_match), Drop of_match_ptr. You have warnings here. > + }, > + .probe = ma35_nand_probe, > + .remove_new = ma35_nand_remove, > + .suspend = ma35_nand_suspend, > + .resume = ma35_nand_resume, > +}; > + > +static int __init ma35_nand_init(void) > +{ > + int ret; > + > + pr_info("ma35 mtd nand driver\n"); NAK, drop, useless and really pointless. > + > + ret = platform_driver_register(&ma35_nand_driver); > + if (ret) { > + pr_warn("nand: failed to add device driver %s\n", > + ma35_nand_driver.driver.name); > + return ret; > + } > + > + return ret; > +} > + > +static void __exit ma35_nand_exit(void) > +{ > + platform_driver_unregister(&ma35_nand_driver); > + pr_info("nand: unregistered successfully!\n"); NAK, drop, useless and really pointless. > +} > + > +module_init(ma35_nand_init); > +module_exit(ma35_nand_exit); Do not open-code these functions, there are wrappers for this. > + > +MODULE_DESCRIPTION("Nuvoton ma35 NAND driver"); > +MODULE_AUTHOR("Hui-Ping Chen <hpchen0nvt(a)gmail.com>"); > +MODULE_LICENSE("GPL"); Best regards, Krzysztof

4 months, 2 weeks

Re: [PATCH 1/2] dt-bindings: mtd: nuvoton,ma35d1-nand: add new bindings

by Krzysztof Kozlowski

On 12/08/2024 05:00, Hui-Ping Chen wrote: > Add dt-bindings for the Nuvoton MA35 SoC NAND Controller. > > Signed-off-by: Hui-Ping Chen <hpchen0nvt(a)gmail.com> > --- > .../bindings/mtd/nuvoton,ma35d1-nand.yaml | 97 +++++++++++++++++++ > 1 file changed, 97 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mtd/nuvoton,ma35d1-nand.yaml > > diff --git a/Documentation/devicetree/bindings/mtd/nuvoton,ma35d1-nand.yaml b/Documentation/devicetree/bindings/mtd/nuvoton,ma35d1-nand.yaml > new file mode 100644 > index 000000000000..988c43bc6e99 > --- /dev/null > +++ b/Documentation/devicetree/bindings/mtd/nuvoton,ma35d1-nand.yaml > @@ -0,0 +1,97 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/mtd/nuvoton,ma35d1-nand.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Nuvoton MA35D1 NAND Flash Interface (NFI) Controller > + > +allOf: > + - $ref: nand-controller.yaml# allOf goes usually just before properties. > + > +maintainers: > + - Hui-Ping Chen <hpchen0nvt(a)gmail.com> > + > +properties: > + compatible: > + enum: > + - nuvoton,ma35d1-nand > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 1 > + > + clocks: > + minItems: 1 This cannot be min only. It's unconstrained. maxItems instead. There is no existing code like this so this should make you wonder... > + > +patternProperties: > + "^nand@[a-f0-9]$": > + type: object > + $ref: raw-nand-chip.yaml > + properties: > + nand-ecc-mode: > + const: hw No, drop the property. It does not make sense to have it const. Compatible defines it. > + > + nand-ecc-step-size: > + enum: [512, 1024] No defaults? So is this required? > + > + nand-ecc-strength: > + enum: [8, 12, 24] No defaults? So is this required? > + > + nand-bus-width: > + const: 8 Drop property. > + > + unevaluatedProperties: false > + > +required: > + - compatible > + - reg > + - interrupts > + - clocks > + > +unevaluatedProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + #include <dt-bindings/clock/nuvoton,ma35d1-clk.h> > + > + soc { > + #address-cells = <2>; > + #size-cells = <2>; > + > + nand-controller@401A0000 { > + compatible = "nuvoton,ma35d1-nand"; > + reg = <0x0 0x401A0000 0x0 0x1000>; > + interrupts = <GIC_SPI 38 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&clk NAND_GATE>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + nand@0 { > + reg = <0>; > + > + nand-on-flash-bbt; > + nand-ecc-mode = "hw"; Drop > + nand-ecc-step-size = <512>; > + nand-ecc-strength = <8>; > + nand-bus-width = <8>; Drop > + > + partitions { > + compatible = "fixed-partitions"; > + #address-cells = <1>; > + #size-cells = <1>; > + > + uboot@0 { > + label = "nand-uboot"; > + read-only; > + reg = <0x0 0x300000>; > + }; > + }; > + }; > + }; > + }; > + > +... Best regards, Krzysztof

4 months, 2 weeks

[PATCH rdma-next 0/8] Introducing Multi-Path DMA Support for mlx5 RDMA Driver

by Leon Romanovsky

From: Leon Romanovsky <leonro(a)nvidia.com> From Yishai, Overview -------- This patch series aims to enable multi-path DMA support, allowing an mlx5 RDMA device to issue DMA commands through multiple paths. This feature is critical for improving performance and reaching line rate in certain environments where issuing PCI transactions over one path may be significantly faster than over another. These differences can arise from various PCI generations in the system or the specific system topology. To achieve this functionality, we introduced a data direct DMA device that can serve the RDMA device by issuing DMA transactions on its behalf. The main key features and changes are described below. Multi-Path Discovery -------------------- API Implementation: * Introduced an API to discover multiple paths for a given mlx5 RDMA device. IOCTL Command: * Added a new IOCTL command, MLX5_IB_METHOD_GET_DATA_DIRECT_SYSFS_PATH, to the DEVICE object. When an affiliated Data-Direct/DMA device is present, its sysfs path is returned. Feature Activation by mlx5 RDMA Application ------------------------------------------- UVERBS Extension: * Extended UVERBS_METHOD_REG_DMABUF_MR over UVERBS_OBJECT_MR to include mlx5 extended flags. Access Flag: * Introduced the MLX5_IB_UAPI_REG_DMABUF_ACCESS_DATA_DIRECT flag, allowing applications to request the use of the affiliated DMA device for DMABUF registration. Data-Direct/DMA Device ---------------------- New Driver: * Introduced a new driver to manage the new DMA PF device ID (0x2100). Its registration/un-registration is handled as part of the mlx5_ib init/exit flows, with mlx5 IB devices as its clients. Functionality: * The driver does not interface directly with the firmware (no command interface, no caps, etc.) but works over PCI to activate its DMA functionality. It serves as the DMA device for efficiently accessing other PCI devices (e.g., GPU PF) and reads its VUID over PCI to handle NICs registrations with the same VUID. mlx5 IB RDMA Device --------------------------- VUID Query: * Reads its affiliated DMA PF VUID via the QUERY_VUID command with the data_direct bit set. Driver Registration: * Registers with the DMA PF driver to be notified upon bind/unbind. Application Request Handling: * Uses the DMA PF device upon application request as described above. DMABUF over Umem ---------------- Introduced an option to obtain a DMABUF UMEM using a different DMA device instead of the IB device, allowing the device to register over IOMMU with the expected DMA device for a given buffer registration. Further details are provided in the commit logs of the patches in this series. Thanks Yishai Hadas (8): net/mlx5: Add IFC related stuff for data direct RDMA/mlx5: Introduce the 'data direct' driver RDMA/mlx5: Add the initialization flow to utilize the 'data direct' device RDMA/umem: Add support for creating pinned DMABUF umem with a given dma device RDMA/umem: Introduce an option to revoke DMABUF umem RDMA: Pass uverbs_attr_bundle as part of '.reg_user_mr_dmabuf' API RDMA/mlx5: Add support for DMABUF MR registrations with Data-direct RDMA/mlx5: Introduce GET_DATA_DIRECT_SYSFS_PATH ioctl drivers/infiniband/core/umem_dmabuf.c | 66 +++- drivers/infiniband/core/uverbs_std_types_mr.c | 2 +- drivers/infiniband/hw/bnxt_re/ib_verbs.c | 3 +- drivers/infiniband/hw/bnxt_re/ib_verbs.h | 2 +- drivers/infiniband/hw/efa/efa.h | 2 +- drivers/infiniband/hw/efa/efa_verbs.c | 4 +- drivers/infiniband/hw/irdma/verbs.c | 2 +- drivers/infiniband/hw/mlx5/Makefile | 1 + drivers/infiniband/hw/mlx5/cmd.c | 21 ++ drivers/infiniband/hw/mlx5/cmd.h | 2 + drivers/infiniband/hw/mlx5/data_direct.c | 227 +++++++++++++ drivers/infiniband/hw/mlx5/data_direct.h | 23 ++ drivers/infiniband/hw/mlx5/main.c | 125 +++++++ drivers/infiniband/hw/mlx5/mlx5_ib.h | 22 +- drivers/infiniband/hw/mlx5/mr.c | 304 +++++++++++++++--- drivers/infiniband/hw/mlx5/odp.c | 5 +- drivers/infiniband/hw/mlx5/std_types.c | 55 +++- drivers/infiniband/hw/mlx5/umr.c | 93 ++++-- drivers/infiniband/hw/mlx5/umr.h | 1 + include/linux/mlx5/mlx5_ifc.h | 51 ++- include/rdma/ib_umem.h | 18 ++ include/rdma/ib_verbs.h | 2 +- include/uapi/rdma/mlx5_user_ioctl_cmds.h | 9 + include/uapi/rdma/mlx5_user_ioctl_verbs.h | 4 + 24 files changed, 944 insertions(+), 100 deletions(-) create mode 100644 drivers/infiniband/hw/mlx5/data_direct.c create mode 100644 drivers/infiniband/hw/mlx5/data_direct.h -- 2.45.2

4 months, 3 weeks

Re: [PATCH v2 0/5] Introduce DMA_HEAP_ALLOC_AND_READ_FILE heap flag

by Daniel Vetter

On Thu, Aug 01, 2024 at 10:53:45AM +0800, Huan Yang wrote: > > 在 2024/8/1 4:46, Daniel Vetter 写道: > > On Tue, Jul 30, 2024 at 08:04:04PM +0800, Huan Yang wrote: > > > 在 2024/7/30 17:05, Huan Yang 写道: > > > > 在 2024/7/30 16:56, Daniel Vetter 写道: > > > > > [????????? daniel.vetter(a)ffwll.ch ????????? > > > > > https://aka.ms/LearnAboutSenderIdentification?????????????] > > > > > > > > > > On Tue, Jul 30, 2024 at 03:57:44PM +0800, Huan Yang wrote: > > > > > > UDMA-BUF step: > > > > > > 1. memfd_create > > > > > > 2. open file(buffer/direct) > > > > > > 3. udmabuf create > > > > > > 4. mmap memfd > > > > > > 5. read file into memfd vaddr > > > > > Yeah this is really slow and the worst way to do it. You absolutely want > > > > > to start _all_ the io before you start creating the dma-buf, ideally > > > > > with > > > > > everything running in parallel. But just starting the direct I/O with > > > > > async and then creating the umdabuf should be a lot faster and avoid > > > > That's greate, Let me rephrase that, and please correct me if I'm wrong. > > > > > > > > UDMA-BUF step: > > > > 1. memfd_create > > > > 2. mmap memfd > > > > 3. open file(buffer/direct) > > > > 4. start thread to async read > > > > 3. udmabuf create > > > > > > > > With this, can improve > > > I just test with it. Step is: > > > > > > UDMA-BUF step: > > > 1. memfd_create > > > 2. mmap memfd > > > 3. open file(buffer/direct) > > > 4. start thread to async read > > > 5. udmabuf create > > > > > > 6 . join wait > > > > > > 3G file read all step cost 1,527,103,431ns, it's greate. > > Ok that's almost the throughput of your patch set, which I think is close > > enough. The remaining difference is probably just the mmap overhead, not > > sure whether/how we can do direct i/o to an fd directly ... in principle > > it's possible for any file that uses the standard pagecache. > > Yes, for mmap, IMO, now that we get all folios and pin it. That's mean all > pfn it's got when udmabuf created. > > So, I think mmap with page fault is helpless for save memory but increase > the mmap access cost.(maybe can save a little page table's memory) > > I want to offer a patchset to remove it and more suitable for folios > operate(And remove unpin list). And contains some fix patch. > > I'll send it when I test it's good. > > > About fd operation for direct I/O, maybe use sendfile or copy_file_range? > > sendfile base pipe buffer, it's low performance when I test is. > > copy_file_range can't work due to it's not the same file system. > > So, I can't find other way to do it. Can someone give some suggestions? Yeah direct I/O to pagecache without an mmap might be too niche to be supported. Maybe io_uring has something, but I guess as unlikely as anything else. -Sima -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

4 months, 3 weeks

Re: [PATCH 0/5] udmbuf bug fix and some improvements

by Michel Dänzer

On 2024-08-01 20:32, Kasireddy, Vivek wrote: > Hi Huan, > >> This patchset attempts to fix some errors in udmabuf and remove the >> upin_list structure. >> >> Some of this fix just gather the patches which I upload before. >> >> Patch1 >> === >> Try to remove page fault mmap and direct map it. >> Due to current udmabuf has already obtained and pinned the folio >> upon completion of the creation.This means that the physical memory has >> already been acquired, rather than being accessed dynamically. The >> current page fault method only saves some page table memory. >> >> As a result, the page fault mechanism has lost its purpose as a demanding >> page. Due to the fact that page fault requires trapping into kernel mode >> and filling in when accessing the corresponding virtual address in mmap, >> this means that user mode access to virtual addresses needs to trap into >> kernel mode. >> >> Therefore, when creating a large size udmabuf, this represents a >> considerable overhead. > Just want to mention that for the main use-case the udmabuf driver is designed for, > (sharing Qemu Guest FB with Host for GPU DMA), udmabufs are not created very > frequently. And, I think providing CPU access via mmap is just a backup, mainly > intended for debugging purposes. FYI, Mesa now uses udmabuf for supporting dma-bufs with software rendering. -- Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and Xwayland developer

5 months

Re: [PATCH 4/5] udmabuf: add get_sg_table helper function

by Christian König

Am 01.08.24 um 12:45 schrieb Huan Yang: > Currently, there are three duplicate pieces of code that retrieve > sg_table and update uduf->sg. > > Since the sgt is used to populate the page in both mmap and vmap.It is > necessary to ensure that ubuf->sg is set correctly. That is a really bad idea. Why are sg tables used to populated the page tables? Regards, Christian. > > This patch add a helper function, if ubuf->sg exist, just return it. > Or else, try alloc a new sgt, and cmpxchg to set it. > > When the swap fails, it means that another process has set sg correctly. > Therefore, we reuse the new sg. If trigger by device, need invoke map to > sync it. > > Signed-off-by: Huan Yang <link(a)vivo.com> > --- > drivers/dma-buf/udmabuf.c | 60 ++++++++++++++++++++++++++++----------- > 1 file changed, 43 insertions(+), 17 deletions(-) > > diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c > index 7ed532342d7f..677ebb2d462f 100644 > --- a/drivers/dma-buf/udmabuf.c > +++ b/drivers/dma-buf/udmabuf.c > @@ -38,8 +38,9 @@ struct udmabuf_folio { > struct list_head list; > }; > > -static struct sg_table *get_sg_table(struct device *dev, struct dma_buf *buf, > - enum dma_data_direction direction); > +static struct sg_table *udmabuf_get_sg_table(struct device *dev, > + struct dma_buf *buf, > + enum dma_data_direction direction); > > static int mmap_udmabuf(struct dma_buf *buf, struct vm_area_struct *vma) > { > @@ -52,12 +53,9 @@ static int mmap_udmabuf(struct dma_buf *buf, struct vm_area_struct *vma) > if ((vma->vm_flags & (VM_SHARED | VM_MAYSHARE)) == 0) > return -EINVAL; > > - if (!table) { > - table = get_sg_table(NULL, buf, 0); > - if (IS_ERR(table)) > - return PTR_ERR(table); > - ubuf->sg = table; > - } > + table = udmabuf_get_sg_table(NULL, buf, 0); > + if (IS_ERR(table)) > + return PTR_ERR(table); > > for_each_sgtable_page(table, &piter, vma->vm_pgoff) { > struct page *page = sg_page_iter_page(&piter); > @@ -84,12 +82,9 @@ static int vmap_udmabuf(struct dma_buf *buf, struct iosys_map *map) > > dma_resv_assert_held(buf->resv); > > - if (!sg) { > - sg = get_sg_table(NULL, buf, 0); > - if (IS_ERR(sg)) > - return PTR_ERR(sg); > - ubuf->sg = sg; > - } > + sg = udmabuf_get_sg_table(NULL, buf, 0); > + if (IS_ERR(sg)) > + return PTR_ERR(sg); > > pages = kvmalloc_array(ubuf->pagecount, sizeof(*pages), GFP_KERNEL); > if (!pages) > @@ -154,6 +149,39 @@ static struct sg_table *get_sg_table(struct device *dev, struct dma_buf *buf, > return ERR_PTR(ret); > } > > +static struct sg_table *udmabuf_get_sg_table(struct device *dev, > + struct dma_buf *buf, > + enum dma_data_direction direction) > +{ > + struct udmabuf *ubuf = buf->priv; > + struct sg_table *sg = READ_ONCE(ubuf->sg); > + int ret = 0; > + > + if (sg) > + return sg; > + > + sg = get_sg_table(dev, buf, direction); > + if (IS_ERR(sg)) > + return sg; > + > + // Success update ubuf's sg, just return. > + if (!cmpxchg(&ubuf->sg, NULL, sg)) > + return sg; > + > + // use the new sg table. > + sg_free_table(sg); > + kfree(sg); > + sg = READ_ONCE(ubuf->sg); > + > + if (dev) > + ret = dma_map_sgtable(dev, sg, direction, 0); > + > + if (ret < 0) > + return ERR_PTR(ret); > + > + return sg; > +} > + > static void put_sg_table(struct device *dev, struct sg_table *sg, > enum dma_data_direction direction) > { > @@ -230,12 +258,10 @@ static int begin_cpu_udmabuf(struct dma_buf *buf, > return 0; > } > > - sg = get_sg_table(dev, buf, direction); > + sg = udmabuf_get_sg_table(dev, buf, direction); > if (IS_ERR(sg)) > return PTR_ERR(sg); > > - ubuf->sg = sg; > - > return 0; > } >

5 months

Re: [PATCH 3/5] udmabuf: fix vmap_udmabuf error page set

by Christian König

Am 01.08.24 um 12:45 schrieb Huan Yang: > Currently vmap_udmabuf set page's array by each folio. > But, ubuf->folios is only contain's the folio's head page. > > That mean we repeatedly mapped the folio head page to the vmalloc area. > > This patch fix it, set each folio's page correct, so that pages array > contains right page, and then map into vmalloc area > > Signed-off-by: Huan Yang <link(a)vivo.com> > --- > drivers/dma-buf/udmabuf.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c > index a915714c5dce..7ed532342d7f 100644 > --- a/drivers/dma-buf/udmabuf.c > +++ b/drivers/dma-buf/udmabuf.c > @@ -77,18 +77,27 @@ static int mmap_udmabuf(struct dma_buf *buf, struct vm_area_struct *vma) > static int vmap_udmabuf(struct dma_buf *buf, struct iosys_map *map) > { > struct udmabuf *ubuf = buf->priv; > - struct page **pages; > + struct page **pages, **tmp; > + struct sg_table *sg = ubuf->sg; > + struct sg_page_iter piter; > void *vaddr; > - pgoff_t pg; > > dma_resv_assert_held(buf->resv); > > + if (!sg) { > + sg = get_sg_table(NULL, buf, 0); > + if (IS_ERR(sg)) > + return PTR_ERR(sg); > + ubuf->sg = sg; > + } > + > pages = kvmalloc_array(ubuf->pagecount, sizeof(*pages), GFP_KERNEL); > if (!pages) > return -ENOMEM; > + tmp = pages; > > - for (pg = 0; pg < ubuf->pagecount; pg++) > - pages[pg] = &ubuf->folios[pg]->page; > + for_each_sgtable_page(sg, &piter, 0) > + *tmp++ = sg_page_iter_page(&piter); Again don't abuse the sg table for that! Regards, Christian. > > vaddr = vm_map_ram(pages, ubuf->pagecount, -1); > kvfree(pages);

5 months

Re: [PATCH 2/5] udmabuf: change folios array from kmalloc to kvmalloc

by Christian König

Am 01.08.24 um 12:45 schrieb Huan Yang: > When PAGE_SIZE 4096, MAX_PAGE_ORDER 10, 64bit machine, > page_alloc only support 4MB. > If above this, trigger this warn and return NULL. > > udmabuf can change size limit, if change it to 3072(3GB), and then alloc > 3GB udmabuf, will fail create. > > [ 4080.876581] ------------[ cut here ]------------ > [ 4080.876843] WARNING: CPU: 3 PID: 2015 at mm/page_alloc.c:4556 __alloc_pages+0x2c8/0x350 > [ 4080.878839] RIP: 0010:__alloc_pages+0x2c8/0x350 > [ 4080.879470] Call Trace: > [ 4080.879473] <TASK> > [ 4080.879473] ? __alloc_pages+0x2c8/0x350 > [ 4080.879475] ? __warn.cold+0x8e/0xe8 > [ 4080.880647] ? __alloc_pages+0x2c8/0x350 > [ 4080.880909] ? report_bug+0xff/0x140 > [ 4080.881175] ? handle_bug+0x3c/0x80 > [ 4080.881556] ? exc_invalid_op+0x17/0x70 > [ 4080.881559] ? asm_exc_invalid_op+0x1a/0x20 > [ 4080.882077] ? udmabuf_create+0x131/0x400 > > Because MAX_PAGE_ORDER, kmalloc can max alloc 4096 * (1 << 10), 4MB > memory, each array entry is pointer(8byte), so can save 524288 pages(2GB). > > Further more, costly order(order 3) may not be guaranteed that it can be > applied for, due to fragmentation. > > This patch change udmabuf array use kvmalloc_array, this can fallback > alloc into vmalloc, which can guarantee allocation for any size and does > not affect the performance of kmalloc allocations. > > Signed-off-by: Huan Yang <link(a)vivo.com> Acked-by: Christian König <christian.koenig(a)amd.com> > --- > drivers/dma-buf/udmabuf.c | 26 +++++++++++++------------- > 1 file changed, 13 insertions(+), 13 deletions(-) > > diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c > index d69aeada7367..a915714c5dce 100644 > --- a/drivers/dma-buf/udmabuf.c > +++ b/drivers/dma-buf/udmabuf.c > @@ -83,7 +83,7 @@ static int vmap_udmabuf(struct dma_buf *buf, struct iosys_map *map) > > dma_resv_assert_held(buf->resv); > > - pages = kmalloc_array(ubuf->pagecount, sizeof(*pages), GFP_KERNEL); > + pages = kvmalloc_array(ubuf->pagecount, sizeof(*pages), GFP_KERNEL); > if (!pages) > return -ENOMEM; > > @@ -91,7 +91,7 @@ static int vmap_udmabuf(struct dma_buf *buf, struct iosys_map *map) > pages[pg] = &ubuf->folios[pg]->page; > > vaddr = vm_map_ram(pages, ubuf->pagecount, -1); > - kfree(pages); > + kvfree(pages); > if (!vaddr) > return -EINVAL; > > @@ -203,8 +203,8 @@ static void release_udmabuf(struct dma_buf *buf) > put_sg_table(dev, ubuf->sg, DMA_BIDIRECTIONAL); > > unpin_all_folios(&ubuf->unpin_list); > - kfree(ubuf->offsets); > - kfree(ubuf->folios); > + kvfree(ubuf->offsets); > + kvfree(ubuf->folios); > kfree(ubuf); > } > > @@ -330,14 +330,14 @@ static long udmabuf_create(struct miscdevice *device, > if (!ubuf->pagecount) > goto err; > > - ubuf->folios = kmalloc_array(ubuf->pagecount, sizeof(*ubuf->folios), > - GFP_KERNEL); > + ubuf->folios = kvmalloc_array(ubuf->pagecount, sizeof(*ubuf->folios), > + GFP_KERNEL); > if (!ubuf->folios) { > ret = -ENOMEM; > goto err; > } > - ubuf->offsets = kcalloc(ubuf->pagecount, sizeof(*ubuf->offsets), > - GFP_KERNEL); > + ubuf->offsets = > + kvcalloc(ubuf->pagecount, sizeof(*ubuf->offsets), GFP_KERNEL); > if (!ubuf->offsets) { > ret = -ENOMEM; > goto err; > @@ -351,7 +351,7 @@ static long udmabuf_create(struct miscdevice *device, > goto err; > > pgcnt = list[i].size >> PAGE_SHIFT; > - folios = kmalloc_array(pgcnt, sizeof(*folios), GFP_KERNEL); > + folios = kvmalloc_array(pgcnt, sizeof(*folios), GFP_KERNEL); > if (!folios) { > ret = -ENOMEM; > goto err; > @@ -361,7 +361,7 @@ static long udmabuf_create(struct miscdevice *device, > ret = memfd_pin_folios(memfd, list[i].offset, end, > folios, pgcnt, &pgoff); > if (ret <= 0) { > - kfree(folios); > + kvfree(folios); > if (!ret) > ret = -EINVAL; > goto err; > @@ -390,7 +390,7 @@ static long udmabuf_create(struct miscdevice *device, > } > } > > - kfree(folios); > + kvfree(folios); > fput(memfd); > memfd = NULL; > } > @@ -406,8 +406,8 @@ static long udmabuf_create(struct miscdevice *device, > if (memfd) > fput(memfd); > unpin_all_folios(&ubuf->unpin_list); > - kfree(ubuf->offsets); > - kfree(ubuf->folios); > + kvfree(ubuf->offsets); > + kvfree(ubuf->folios); > kfree(ubuf); > return ret; > }

5 months

Re: [PATCH 1/5] udmabuf: cancel mmap page fault, direct map it

by Christian König

Am 01.08.24 um 12:45 schrieb Huan Yang: > The current udmabuf mmap uses a page fault mechanism to populate the vma. > > However, the current udmabuf has already obtained and pinned the folio > upon completion of the creation.This means that the physical memory has > already been acquired, rather than being accessed dynamically. The > current page fault method only saves some page table memory. > > As a result, the page fault mechanism has lost its purpose as a demanding > page. Due to the fact that page fault requires trapping into kernel mode > and filling in when accessing the corresponding virtual address in mmap, > this means that user mode access to virtual addresses needs to trap into > kernel mode. > > Therefore, when creating a large size udmabuf, this represents a > considerable overhead. > > Therefore, the current patch removes the page fault method of mmap and > instead fills it directly when mmap is triggered. > > Signed-off-by: Huan Yang <link(a)vivo.com> > --- > drivers/dma-buf/udmabuf.c | 70 ++++++++++++++++++++++----------------- > 1 file changed, 39 insertions(+), 31 deletions(-) > > diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c > index 047c3cd2ceff..d69aeada7367 100644 > --- a/drivers/dma-buf/udmabuf.c > +++ b/drivers/dma-buf/udmabuf.c > @@ -38,36 +38,39 @@ struct udmabuf_folio { > struct list_head list; > }; > > -static vm_fault_t udmabuf_vm_fault(struct vm_fault *vmf) > -{ > - struct vm_area_struct *vma = vmf->vma; > - struct udmabuf *ubuf = vma->vm_private_data; > - pgoff_t pgoff = vmf->pgoff; > - unsigned long pfn; > - > - if (pgoff >= ubuf->pagecount) > - return VM_FAULT_SIGBUS; > - > - pfn = folio_pfn(ubuf->folios[pgoff]); > - pfn += ubuf->offsets[pgoff] >> PAGE_SHIFT; > - > - return vmf_insert_pfn(vma, vmf->address, pfn); > -} > - > -static const struct vm_operations_struct udmabuf_vm_ops = { > - .fault = udmabuf_vm_fault, > -}; > +static struct sg_table *get_sg_table(struct device *dev, struct dma_buf *buf, > + enum dma_data_direction direction); > > static int mmap_udmabuf(struct dma_buf *buf, struct vm_area_struct *vma) > { > struct udmabuf *ubuf = buf->priv; > + struct sg_table *table = ubuf->sg; > + unsigned long addr = vma->vm_start; > + struct sg_page_iter piter; > + int ret; > > if ((vma->vm_flags & (VM_SHARED | VM_MAYSHARE)) == 0) > return -EINVAL; > > - vma->vm_ops = &udmabuf_vm_ops; > - vma->vm_private_data = ubuf; > - vm_flags_set(vma, VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP); > + if (!table) { > + table = get_sg_table(NULL, buf, 0); > + if (IS_ERR(table)) > + return PTR_ERR(table); > + ubuf->sg = table; > + } > + > + for_each_sgtable_page(table, &piter, vma->vm_pgoff) { That might not work correctly. We intentionally remove the pages from the sgtable when it is shared between devices. Additional to that the sgtable is *not* a page container, but rather a DMA address container. So that here is also a rather bad idea from the design side. Regards, Christian. > + struct page *page = sg_page_iter_page(&piter); > + > + ret = remap_pfn_range(vma, addr, page_to_pfn(page), PAGE_SIZE, > + vma->vm_page_prot); > + if (ret) > + return ret; > + addr += PAGE_SIZE; > + if (addr >= vma->vm_end) > + return 0; > + } > + > return 0; > } > > @@ -126,6 +129,10 @@ static struct sg_table *get_sg_table(struct device *dev, struct dma_buf *buf, > sg_set_folio(sgl, ubuf->folios[i], PAGE_SIZE, > ubuf->offsets[i]); > > + // if dev is NULL, no need to sync. > + if (!dev) > + return sg; > + > ret = dma_map_sgtable(dev, sg, direction, 0); > if (ret < 0) > goto err_map; > @@ -206,20 +213,21 @@ static int begin_cpu_udmabuf(struct dma_buf *buf, > { > struct udmabuf *ubuf = buf->priv; > struct device *dev = ubuf->device->this_device; > - int ret = 0; > + struct sg_table *sg; > > - if (!ubuf->sg) { > - ubuf->sg = get_sg_table(dev, buf, direction); > - if (IS_ERR(ubuf->sg)) { > - ret = PTR_ERR(ubuf->sg); > - ubuf->sg = NULL; > - } > - } else { > + if (ubuf->sg) { > dma_sync_sg_for_cpu(dev, ubuf->sg->sgl, ubuf->sg->nents, > direction); > + return 0; > } > > - return ret; > + sg = get_sg_table(dev, buf, direction); > + if (IS_ERR(sg)) > + return PTR_ERR(sg); > + > + ubuf->sg = sg; > + > + return 0; > } > > static int end_cpu_udmabuf(struct dma_buf *buf,

5 months

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig August 2024