dma: phytium: fix more bugs of pswiotlb#1522
dma: phytium: fix more bugs of pswiotlb#1522xuyan213 wants to merge 2 commits intodeepin-community:linux-6.6.yfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
This PR refactors Phytium pswiotlb integration to route DMA operations through dma_map_ops, while relocating PCI and platform-identification logic to more appropriate subsystems (PCI quirks and arm64 CPU errata).
Changes:
- Move pswiotlb DMA interception behind per-device
dma_map_opssetup (pswiotlb_setup_dma_ops()called fromarch_setup_dma_ops()). - Relocate PCI-specific pswiotlb setup into a dedicated PCI helper invoked from probe, and adjust pswiotlb debug/trace plumbing.
- Move Phytium platform identification into arm64 CPU errata capability detection (new Kconfig/cpucap).
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| kernel/dma/phytium/pswiotlb.c | Marks pswiotlb_force_disable as __ro_after_init; gates debugfs/timer setup on Phytium SoC detection. |
| kernel/dma/phytium/pswiotlb-mapping.c | Adds pswiotlb-backed dma_map_ops wrappers and device DMA ops setup/cloning logic. |
| kernel/dma/phytium/pswiotlb-iommu.c | Comment-only updates documenting additional ported helpers. |
| kernel/dma/phytium/pswiotlb-dma.h | Adds a custom ops struct and updates pswiotlb helper prototypes/inlines. |
| kernel/dma/mapping.c | Removes direct pswiotlb hooks from generic DMA mapping paths. |
| kernel/dma/contiguous.c | Adjusts NUMA/CMA nid declaration ordering around pswiotlb checks. |
| include/trace/events/pswiotlb.h | Fixes trace entry field naming/assignment for “force off” state. |
| include/linux/pswiotlb.h | Moves SoC identification to an extern flag and adds pswiotlb_setup_dma_ops() prototype. |
| include/linux/pci.h | Adds pci_configure_pswiotlb() declaration/stub. |
| include/linux/device.h | Adds orig_dma_ops and reorganizes pswiotlb-related device fields. |
| drivers/pci/quirks.c | Implements pci_configure_pswiotlb() helper. |
| drivers/pci/probe.c | Calls pci_configure_pswiotlb() during PCI device add. |
| drivers/pci/pci.c | Removes pswiotlb vendor/passthrough logging/setup from pci_set_master(). |
| arch/arm64/tools/cpucaps | Adds WORKAROUND_PHYTIUM_FT3386 capability. |
| arch/arm64/mm/dma-mapping.c | Invokes pswiotlb_setup_dma_ops() from arch_setup_dma_ops(). |
| arch/arm64/kernel/cpu_errata.c | Adds capability match logic and is_ps_socs definition under new config. |
| arch/arm64/include/asm/cputype.h | Adds MIDR helper macro for Phytium FTC862. |
| arch/arm64/Kconfig | Introduces CONFIG_PHYTIUM_ERRATUM_FT3386 to enable the workaround by default. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| extern bool __read_mostly is_ps_socs; | ||
|
|
There was a problem hiding this comment.
is_ps_socs is declared as an extern here and referenced by is_phytium_ps_socs(), but it is only defined under CONFIG_PHYTIUM_ERRATUM_FT3386 (arch/arm64/kernel/cpu_errata.c). If that Kconfig option is disabled, this becomes an undefined symbol at link time. Consider providing an unconditional definition (defaulting to false) under CONFIG_PSWIOTLB, or guarding the extern/inlines with #ifdef CONFIG_PHYTIUM_ERRATUM_FT3386 and providing a stub implementation otherwise.
| unsigned int flags; | ||
|
|
||
| void *(*alloc)(struct device *dev, size_t size, | ||
| dma_addr_t *dma_handle, gfp_t gfp, | ||
| unsigned long attrs); | ||
| void (*free)(struct device *dev, size_t size, void *vaddr, | ||
| dma_addr_t dma_handle, unsigned long attrs); | ||
| struct page *(*alloc_pages)(struct device *dev, size_t size, | ||
| dma_addr_t *dma_handle, enum dma_data_direction dir, | ||
| gfp_t gfp); | ||
| void (*free_pages)(struct device *dev, size_t size, struct page *vaddr, | ||
| dma_addr_t dma_handle, enum dma_data_direction dir); | ||
| struct sg_table *(*alloc_noncontiguous)(struct device *dev, size_t size, | ||
| enum dma_data_direction dir, gfp_t gfp, | ||
| unsigned long attrs); | ||
| void (*free_noncontiguous)(struct device *dev, size_t size, | ||
| struct sg_table *sgt, enum dma_data_direction dir); | ||
| int (*mmap)(struct device *dev, struct vm_area_struct *vma, | ||
| void *cpu_addr, dma_addr_t dma_addr, size_t size, unsigned long attrs); | ||
|
|
||
| int (*get_sgtable)(struct device *dev, struct sg_table *sgt, | ||
| void *cpu_addr, dma_addr_t dma_addr, size_t size, | ||
| unsigned long attrs); | ||
|
|
||
| dma_addr_t (*map_page)(struct device *dev, struct page *page, | ||
| unsigned long offset, size_t size, | ||
| enum dma_data_direction dir, unsigned long attrs); | ||
| void (*unmap_page)(struct device *dev, dma_addr_t dma_handle, | ||
| size_t size, enum dma_data_direction dir, | ||
| unsigned long attrs); | ||
| /* | ||
| * map_sg should return a negative error code on error. See | ||
| * dma_map_sgtable() for a list of appropriate error codes | ||
| * and their meanings. | ||
| */ | ||
| int (*map_sg)(struct device *dev, struct scatterlist *sg, int nents, | ||
| enum dma_data_direction dir, unsigned long attrs); | ||
| void (*unmap_sg)(struct device *dev, struct scatterlist *sg, int nents, | ||
| enum dma_data_direction dir, unsigned long attrs); | ||
| dma_addr_t (*map_resource)(struct device *dev, phys_addr_t phys_addr, | ||
| size_t size, enum dma_data_direction dir, | ||
| unsigned long attrs); | ||
| void (*unmap_resource)(struct device *dev, dma_addr_t dma_handle, | ||
| size_t size, enum dma_data_direction dir, | ||
| unsigned long attrs); | ||
| void (*sync_single_for_cpu)(struct device *dev, dma_addr_t dma_handle, | ||
| size_t size, enum dma_data_direction dir); | ||
| void (*sync_single_for_device)(struct device *dev, | ||
| dma_addr_t dma_handle, size_t size, | ||
| enum dma_data_direction dir); | ||
| void (*sync_sg_for_cpu)(struct device *dev, struct scatterlist *sg, | ||
| int nents, enum dma_data_direction dir); | ||
| void (*sync_sg_for_device)(struct device *dev, struct scatterlist *sg, | ||
| int nents, enum dma_data_direction dir); | ||
| void (*cache_sync)(struct device *dev, void *vaddr, size_t size, | ||
| enum dma_data_direction direction); | ||
| int (*dma_supported)(struct device *dev, u64 mask); | ||
| u64 (*get_required_mask)(struct device *dev); | ||
| size_t (*max_mapping_size)(struct device *dev); | ||
| size_t (*opt_mapping_size)(void); | ||
| unsigned long (*get_merge_boundary)(struct device *dev); |
There was a problem hiding this comment.
This custom struct pswiotlb_dma_map_ops duplicates struct dma_map_ops, but the kernel's struct dma_map_ops includes additional reserved fields (e.g. DEEPIN_KABI_RESERVE() in include/linux/dma-map-ops.h). Casting between these types and memcpy'ing based on this struct's size is not layout-safe and can cause out-of-bounds access/corruption. It would be safer to allocate/copy a real struct dma_map_ops (or embed struct dma_map_ops as the first member and size it exactly).
| unsigned int flags; | |
| void *(*alloc)(struct device *dev, size_t size, | |
| dma_addr_t *dma_handle, gfp_t gfp, | |
| unsigned long attrs); | |
| void (*free)(struct device *dev, size_t size, void *vaddr, | |
| dma_addr_t dma_handle, unsigned long attrs); | |
| struct page *(*alloc_pages)(struct device *dev, size_t size, | |
| dma_addr_t *dma_handle, enum dma_data_direction dir, | |
| gfp_t gfp); | |
| void (*free_pages)(struct device *dev, size_t size, struct page *vaddr, | |
| dma_addr_t dma_handle, enum dma_data_direction dir); | |
| struct sg_table *(*alloc_noncontiguous)(struct device *dev, size_t size, | |
| enum dma_data_direction dir, gfp_t gfp, | |
| unsigned long attrs); | |
| void (*free_noncontiguous)(struct device *dev, size_t size, | |
| struct sg_table *sgt, enum dma_data_direction dir); | |
| int (*mmap)(struct device *dev, struct vm_area_struct *vma, | |
| void *cpu_addr, dma_addr_t dma_addr, size_t size, unsigned long attrs); | |
| int (*get_sgtable)(struct device *dev, struct sg_table *sgt, | |
| void *cpu_addr, dma_addr_t dma_addr, size_t size, | |
| unsigned long attrs); | |
| dma_addr_t (*map_page)(struct device *dev, struct page *page, | |
| unsigned long offset, size_t size, | |
| enum dma_data_direction dir, unsigned long attrs); | |
| void (*unmap_page)(struct device *dev, dma_addr_t dma_handle, | |
| size_t size, enum dma_data_direction dir, | |
| unsigned long attrs); | |
| /* | |
| * map_sg should return a negative error code on error. See | |
| * dma_map_sgtable() for a list of appropriate error codes | |
| * and their meanings. | |
| */ | |
| int (*map_sg)(struct device *dev, struct scatterlist *sg, int nents, | |
| enum dma_data_direction dir, unsigned long attrs); | |
| void (*unmap_sg)(struct device *dev, struct scatterlist *sg, int nents, | |
| enum dma_data_direction dir, unsigned long attrs); | |
| dma_addr_t (*map_resource)(struct device *dev, phys_addr_t phys_addr, | |
| size_t size, enum dma_data_direction dir, | |
| unsigned long attrs); | |
| void (*unmap_resource)(struct device *dev, dma_addr_t dma_handle, | |
| size_t size, enum dma_data_direction dir, | |
| unsigned long attrs); | |
| void (*sync_single_for_cpu)(struct device *dev, dma_addr_t dma_handle, | |
| size_t size, enum dma_data_direction dir); | |
| void (*sync_single_for_device)(struct device *dev, | |
| dma_addr_t dma_handle, size_t size, | |
| enum dma_data_direction dir); | |
| void (*sync_sg_for_cpu)(struct device *dev, struct scatterlist *sg, | |
| int nents, enum dma_data_direction dir); | |
| void (*sync_sg_for_device)(struct device *dev, struct scatterlist *sg, | |
| int nents, enum dma_data_direction dir); | |
| void (*cache_sync)(struct device *dev, void *vaddr, size_t size, | |
| enum dma_data_direction direction); | |
| int (*dma_supported)(struct device *dev, u64 mask); | |
| u64 (*get_required_mask)(struct device *dev); | |
| size_t (*max_mapping_size)(struct device *dev); | |
| size_t (*opt_mapping_size)(void); | |
| unsigned long (*get_merge_boundary)(struct device *dev); | |
| struct dma_map_ops ops; |
| struct pswiotlb_dma_map_ops *pswiotlb_clone_orig_dma_ops(struct device *dev, | ||
| const struct dma_map_ops *ops) | ||
| { | ||
| struct pswiotlb_dma_map_ops *new_dma_ops = kmalloc(sizeof(struct pswiotlb_dma_map_ops), | ||
| GFP_KERNEL); | ||
| if (!new_dma_ops) | ||
| return NULL; | ||
|
|
||
| memcpy(new_dma_ops, ops, sizeof(struct pswiotlb_dma_map_ops)); | ||
|
|
||
| return new_dma_ops; |
There was a problem hiding this comment.
memcpy(new_dma_ops, ops, sizeof(struct pswiotlb_dma_map_ops)) copies from a struct dma_map_ops into a differently-sized type, and later the result is cast back to const struct dma_map_ops * in set_dma_ops(). Given struct dma_map_ops has extra reserved fields in this tree, this can produce an undersized allocation and invalid memory access. Allocate struct dma_map_ops (or ensure the cloned object is exactly the same size/layout as struct dma_map_ops) and copy sizeof(*ops) instead.
| void pswiotlb_setup_dma_ops(struct device *dev) | ||
| { | ||
| const struct dma_map_ops *orig_ops = get_dma_ops(dev); | ||
| struct pswiotlb_dma_map_ops *new_ops; | ||
| struct pci_dev *pdev; | ||
|
|
||
| if (dev && dev_is_pci(dev) && (pswiotlb_force_disable != true) && | ||
| is_phytium_ps_socs()) { | ||
| pdev = to_pci_dev(dev); | ||
| pdev->dev.can_use_pswiotlb = pswiotlb_is_dev_in_passthroughlist(pdev); | ||
| dev_info(&pdev->dev, "The device %s use pswiotlb because vendor 0x%04x %s in pswiotlb passthroughlist\n", | ||
| pdev->dev.can_use_pswiotlb ? "would" : "would NOT", | ||
| pdev->vendor, pdev->dev.can_use_pswiotlb ? "is NOT" : "is"); | ||
| } | ||
|
|
||
| if (check_if_pswiotlb_is_applicable(dev)) { | ||
| if (!orig_ops) | ||
| set_dma_ops(dev, &pswiotlb_noiommu_dma_ops); | ||
| else { | ||
| new_ops = pswiotlb_clone_orig_dma_ops(dev, orig_ops); | ||
| if (!new_ops) { | ||
| dev_warn(dev, "Failed to clone dma ops, pswiotlb is NOT applicable\n"); | ||
| return; | ||
| } | ||
|
|
||
| dev->orig_dma_ops = get_dma_ops(dev); | ||
| new_ops->alloc = pswiotlb_dma_alloc_distribute; | ||
| new_ops->map_page = pswiotlb_dma_map_page_attrs_distribute; | ||
| new_ops->unmap_page = pswiotlb_dma_unmap_page_attrs_distribute; | ||
| new_ops->map_sg = pswiotlb_dma_map_sg_attrs_distribute; | ||
| new_ops->unmap_sg = pswiotlb_dma_unmap_sg_attrs_distribute; | ||
| new_ops->sync_single_for_cpu = | ||
| pswiotlb_dma_sync_single_for_cpu_distribute; | ||
| new_ops->sync_single_for_device = | ||
| pswiotlb_dma_sync_single_for_device_distribute; | ||
| new_ops->sync_sg_for_cpu = | ||
| pswiotlb_dma_sync_sg_for_cpu_distribute; | ||
| new_ops->sync_sg_for_device = | ||
| pswiotlb_dma_sync_sg_for_device_distribute; | ||
| new_ops->max_mapping_size = | ||
| pswiotlb_dma_max_mapping_size_distribute; | ||
| new_ops->opt_mapping_size = | ||
| pswiotlb_dma_opt_mapping_size_distribute; | ||
|
|
||
| set_dma_ops(dev, (const struct dma_map_ops *)new_ops); | ||
| } |
There was a problem hiding this comment.
pswiotlb_setup_dma_ops() allocates a new ops struct with kmalloc() and installs it via set_dma_ops(), but there is no corresponding teardown/free path. Since arch_teardown_dma_ops() only nulls dev->dma_ops, reconfiguring DMA ops (e.g., repeated dma_configure() calls, probe failures, or domain changes) can leak memory per device. Consider making this idempotent (detect already-wrapped ops) and/or storing the allocated ops pointer so it can be freed/restored in arch_teardown_dma_ops() or an appropriate device cleanup hook.
| { | ||
| if (iommu_default_passthrough()) | ||
| return SIZE_MAX; | ||
| else | ||
| return iova_rcache_range(); |
There was a problem hiding this comment.
iova_rcache_range() is only declared in <linux/iova.h> when IS_REACHABLE(CONFIG_IOMMU_IOVA) is true. This call is unconditional, so configurations with CONFIG_PSWIOTLB=y but without reachable CONFIG_IOMMU_IOVA will fail to build. Consider guarding this code with #if IS_REACHABLE(CONFIG_IOMMU_IOVA) and providing a fallback value when it's not available.
| { | |
| if (iommu_default_passthrough()) | |
| return SIZE_MAX; | |
| else | |
| return iova_rcache_range(); | |
| { | |
| #if IS_REACHABLE(CONFIG_IOMMU_IOVA) | |
| if (iommu_default_passthrough()) | |
| return SIZE_MAX; | |
| else | |
| return iova_rcache_range(); | |
| #else | |
| return SIZE_MAX; | |
| #endif |
| if ((pswiotlb_force_disable != true) && | ||
| is_phytium_ps_socs()) { | ||
| pswiotlb_store_local_node(dev, bus); | ||
| dma_set_seg_boundary(&dev->dev, 0xffffffffffff); | ||
| } |
There was a problem hiding this comment.
This block uses space indentation instead of kernel-standard tabs, which will trigger checkpatch/style failures and makes the surrounding code harder to read consistently. Please re-indent with tabs to match the rest of drivers/pci/quirks.c.
| if ((pswiotlb_force_disable != true) && | |
| is_phytium_ps_socs()) { | |
| pswiotlb_store_local_node(dev, bus); | |
| dma_set_seg_boundary(&dev->dev, 0xffffffffffff); | |
| } | |
| if ((pswiotlb_force_disable != true) && | |
| is_phytium_ps_socs()) { | |
| pswiotlb_store_local_node(dev, bus); | |
| dma_set_seg_boundary(&dev->dev, 0xffffffffffff); | |
| } |
| #define PHYTIUM_CPU_PART_FTC862 0x862 | ||
| #define PHYTIUM_CPU_SOCID_PS24080 0x6 | ||
|
|
||
| #define MIDR_FT_FTC862 MIDR_CPU_MODEL(ARM_CPU_IMP_PHYTIUM, PHYTIUM_CPU_PART_FTC862) |
fix more bugs of pswiotlb in cloud kernel 6.6 including: Move pswiotlb dma functions behind dma_map_ops Optimized variable types and removed unused header files Move PCI-related changes to the PCI quirks. Move platform-identification to cpu errata Adjust the location of the platform-identification process Fix a issue of member variable force not being initialized Signed-off-by: xuyan <xuyan1481@phytium.com.cn>
Signed-off-by: xuyan <xuyan1481@phytium.com.cn>
fix more bugs of pswiotlb in cloud kernel 6.6
including:
Move pswiotlb dma functions behind
dma_map_ops
Optimized variable types and removed unused
header files
Move PCI-related changes to the PCI quirks.
Move platform-identification to cpu errata
Adjust the location of the
platform-identification process
Fix a issue of member variable force not
being initialized