| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 28 Jan 2022 10:48:42 +0100
From: Christian König <christian.koenig@....com>
To: Lucas De Marchi <lucas.demarchi@...el.com>
Cc: linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
srinivas.kandagatla@...aro.org, gregkh@...uxfoundation.org,
sumit.semwal@...aro.org, daniel.vetter@...ll.ch, airlied@...ux.ie,
lyude@...hat.com, tzimmermann@...e.de, linux-media@...r.kernel.org,
nouveau@...ts.freedesktop.org
Subject: Re: [PATCH 00/14] Rename dma-buf-map
Am 28.01.22 um 10:40 schrieb Lucas De Marchi:
> On Fri, Jan 28, 2022 at 10:22:00AM +0100, Christian König wrote:
>> Am 28.01.22 um 10:12 schrieb Lucas De Marchi:
>>> On Fri, Jan 28, 2022 at 09:41:14AM +0100, Christian König wrote:
>>>> Rule #1 is to never ever break the build.
>>>>
>>>> Because of this all those patches needs to be squashed into a
>>>> single one as far as I can see.
>>>
>>> what config are you building on?
>>
>> Well I'm not building at all, I'm just looking at the patches as an
>> engineer with 25 years of experience with Linux patches.
>>
>> Just take a look at patch number 2:
>>
>> -static int fastrpc_vmap(struct dma_buf *dmabuf, struct dma_buf_map
>> *map)
>> +static int fastrpc_vmap(struct dma_buf *dmabuf, struct iosys_map *map)
>>
>> You are changing the functions signature without changing any of the
>> callers.
>>
>> At bare minimum that causes a warning and on runtime this only works
>> by coincident now because the structure pointers just happen to have
>> the same layout. This is not something we usually do.
>
> you missed the magic/hack on patch 1:
>
> 1) dma-buf-map.h includes iosys-map.h _at the end_
> 2) iosys-map.h includes dma-buf-map.h at the beginning
> and initially does a "define iosys_map dma_buf_map".
>
> So, it doesn't work by coincidence, It's because it was done to allow
> converting it piecemeal.
Oh, my. Please never do stuff like that again.
>
> But as I said, I don't really have a preference. When crossing
> subsystems one thing that is hard is that different people have different
> preferences on these things. At least squashing now is much easier than
> if I had to split it
>
> Try to imagine how much complain I received on going the other way in
> 25985edcedea6396277003854657b5f3cb31a628 with
> 2463 files changed, 4252 insertions(+), 4252 deletions(-)
Well exactly that is perfectly fine.
What you do here is applying your personal hack which is absolutely not
welcomed.
Regards,
Christian.
> :)
>
>
> Lucas De Marchi
>
>>
>> Regards,
>> Christian.
>>
>>> I built this series, full config with
>>> CONFIG_COMPILE_TEST and doing:
>>>
>>> git rebase -i <base> -x "make -j$(nproc)"
>>>
>>> I split these patches in a way that wouldn't break the build on
>>> purpose.
>>> There were a couple that I couldn't build without cross compiling:
>>> tegra
>>> and rockchip. The others were ok.
>>>
>>> I'm not really against squashing everything in one to merge, though.
>>> It will be hard on the conflicts later, but should get the job done
>>> much
>>> quicker.
>>>
>>> Lucas De Marchi
>>>
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>> Am 28.01.22 um 09:36 schrieb Lucas De Marchi:
>>>>> Motivation for this started in
>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F20220126203702.1784589-1-lucas.demarchi%40intel.com%2F&data=04%7C01%7Cchristian.koenig%40amd.com%7C635084a520994d35a16e08d9e2423319%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637789596221829397%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=ruHpD3DbyyqQuZIFEQU%2B2RH31OwsdFnn1v7N4z75U0Y%3D&reserved=0
>>>>>
>>>>>
>>>>> when trying to extend the dma-buf-map API to cover new use cases:
>>>>> help a
>>>>> single driver with allocations and sharing code paths for IO and
>>>>> system
>>>>> memory. I'm leaving the API additions aside and first renaming the
>>>>> interface as requested.
>>>>>
>>>>> There are already some users in tree outside the context of dma-buf
>>>>> importer/exporter. So before extending the API, let's dissociate
>>>>> it from
>>>>> dma-buf.
>>>>>
>>>>> The iosys-map.h is introduced in the first patch in a way that allows
>>>>> the conversion of each driver to happen separately. After all the
>>>>> conversions are done we can remove the old one, which is the last
>>>>> patch.
>>>>> Another possible way is to squash everything and merge together,
>>>>> but I believe this would make much harder for review.
>>>>>
>>>>> The conversion was done with the following semantic patch:
>>>>>
>>>>> @r1@
>>>>> @@
>>>>> - struct dma_buf_map
>>>>> + struct iosys_map
>>>>>
>>>>> @r2@
>>>>> @@
>>>>> (
>>>>> - DMA_BUF_MAP_INIT_VADDR
>>>>> + IOSYS_MAP_INIT_VADDR
>>>>> |
>>>>> - dma_buf_map_set_vaddr
>>>>> + iosys_map_set_vaddr
>>>>> |
>>>>> - dma_buf_map_set_vaddr_iomem
>>>>> + iosys_map_set_vaddr_iomem
>>>>> |
>>>>> - dma_buf_map_is_equal
>>>>> + iosys_map_is_equal
>>>>> |
>>>>> - dma_buf_map_is_null
>>>>> + iosys_map_is_null
>>>>> |
>>>>> - dma_buf_map_is_set
>>>>> + iosys_map_is_set
>>>>> |
>>>>> - dma_buf_map_clear
>>>>> + iosys_map_clear
>>>>> |
>>>>> - dma_buf_map_memcpy_to
>>>>> + iosys_map_memcpy_to
>>>>> |
>>>>> - dma_buf_map_incr
>>>>> + iosys_map_incr
>>>>> )
>>>>>
>>>>> @@
>>>>> @@
>>>>> - #include <linux/dma-buf-map.h>
>>>>> + #include <linux/iosys-map.h>
>>>>>
>>>>> and then some files had their includes adjusted so we can build
>>>>> everything on each commit in this series. Also some comments were
>>>>> update
>>>>> to remove mentions to dma-buf-map. Simply doing a sed to rename
>>>>> didn't
>>>>> work as dma-buf has some APIs using the dma_buf_map prefix.
>>>>>
>>>>> Once finalized, I think most of this, if not all, could go through
>>>>> the
>>>>> drm-misc-next branch. I split i915, msm, nouveau, and radeon in their
>>>>> own patches in case it's preferred to take those through their own
>>>>> trees.
>>>>>
>>>>> Lucas De Marchi
>>>>>
>>>>> Lucas De Marchi (14):
>>>>> iosys-map: Introduce renamed dma-buf-map
>>>>> misc: fastrpc: Replace dma-buf-map with iosys-map
>>>>> dma-buf: Replace dma-buf-map with iosys-map
>>>>> media: Replace dma-buf-map with iosys-map
>>>>> drm/ttm: Replace dma-buf-map with iosys-map
>>>>> drm: Replace dma-buf-map with iosys-map in drivers
>>>>> drm/i915: Replace dma-buf-map with iosys-map
>>>>> drm/msm: Replace dma-buf-map with iosys-map
>>>>> drm/nouveau: Replace dma-buf-map with iosys-map
>>>>> drm/tegra: Replace dma-buf-map with iosys-map
>>>>> drm/radeon: Replace dma-buf-map with iosys-map
>>>>> drm: Replace dma-buf-map with iosys-map in common code
>>>>> Documentation: Refer to iosys-map instead of dma-buf-map
>>>>> dma-buf-map: Remove API in favor of iosys-map
>>>>>
>>>>> Documentation/driver-api/dma-buf.rst | 4 +-
>>>>> Documentation/gpu/todo.rst | 20 +-
>>>>> MAINTAINERS | 2 +-
>>>>> drivers/dma-buf/dma-buf.c | 22 +-
>>>>> drivers/dma-buf/heaps/cma_heap.c | 10 +-
>>>>> drivers/dma-buf/heaps/system_heap.c | 10 +-
>>>>> drivers/gpu/drm/ast/ast_drv.h | 2 +-
>>>>> drivers/gpu/drm/ast/ast_mode.c | 8 +-
>>>>> drivers/gpu/drm/drm_cache.c | 18 +-
>>>>> drivers/gpu/drm/drm_client.c | 9 +-
>>>>> drivers/gpu/drm/drm_fb_helper.c | 12 +-
>>>>> drivers/gpu/drm/drm_gem.c | 12 +-
>>>>> drivers/gpu/drm/drm_gem_cma_helper.c | 9 +-
>>>>> drivers/gpu/drm/drm_gem_framebuffer_helper.c | 16 +-
>>>>> drivers/gpu/drm/drm_gem_shmem_helper.c | 15 +-
>>>>> drivers/gpu/drm/drm_gem_ttm_helper.c | 4 +-
>>>>> drivers/gpu/drm/drm_gem_vram_helper.c | 25 +-
>>>>> drivers/gpu/drm/drm_internal.h | 6 +-
>>>>> drivers/gpu/drm/drm_mipi_dbi.c | 8 +-
>>>>> drivers/gpu/drm/drm_prime.c | 4 +-
>>>>> drivers/gpu/drm/etnaviv/etnaviv_drv.h | 2 +-
>>>>> drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c | 8 +-
>>>>> drivers/gpu/drm/gud/gud_pipe.c | 4 +-
>>>>> drivers/gpu/drm/hyperv/hyperv_drm_modeset.c | 5 +-
>>>>> drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 8 +-
>>>>> .../drm/i915/gem/selftests/i915_gem_dmabuf.c | 6 +-
>>>>> .../gpu/drm/i915/gem/selftests/mock_dmabuf.c | 6 +-
>>>>> drivers/gpu/drm/lima/lima_gem.c | 3 +-
>>>>> drivers/gpu/drm/lima/lima_sched.c | 4 +-
>>>>> drivers/gpu/drm/mediatek/mtk_drm_gem.c | 7 +-
>>>>> drivers/gpu/drm/mediatek/mtk_drm_gem.h | 5 +-
>>>>> drivers/gpu/drm/mgag200/mgag200_mode.c | 4 +-
>>>>> drivers/gpu/drm/msm/msm_drv.h | 4 +-
>>>>> drivers/gpu/drm/msm/msm_gem_prime.c | 6 +-
>>>>> drivers/gpu/drm/nouveau/nouveau_gem.c | 2 +
>>>>> drivers/gpu/drm/panfrost/panfrost_perfcnt.c | 13 +-
>>>>> drivers/gpu/drm/qxl/qxl_display.c | 8 +-
>>>>> drivers/gpu/drm/qxl/qxl_draw.c | 6 +-
>>>>> drivers/gpu/drm/qxl/qxl_drv.h | 10 +-
>>>>> drivers/gpu/drm/qxl/qxl_object.c | 8 +-
>>>>> drivers/gpu/drm/qxl/qxl_object.h | 4 +-
>>>>> drivers/gpu/drm/qxl/qxl_prime.c | 4 +-
>>>>> drivers/gpu/drm/radeon/radeon_gem.c | 1 +
>>>>> drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 9 +-
>>>>> drivers/gpu/drm/rockchip/rockchip_drm_gem.h | 5 +-
>>>>> drivers/gpu/drm/tegra/gem.c | 10 +-
>>>>> drivers/gpu/drm/tiny/cirrus.c | 8 +-
>>>>> drivers/gpu/drm/tiny/gm12u320.c | 7 +-
>>>>> drivers/gpu/drm/ttm/ttm_bo_util.c | 16 +-
>>>>> drivers/gpu/drm/ttm/ttm_resource.c | 26 +-
>>>>> drivers/gpu/drm/ttm/ttm_tt.c | 6 +-
>>>>> drivers/gpu/drm/udl/udl_modeset.c | 3 +-
>>>>> drivers/gpu/drm/vboxvideo/vbox_mode.c | 4 +-
>>>>> drivers/gpu/drm/virtio/virtgpu_prime.c | 1 +
>>>>> drivers/gpu/drm/vkms/vkms_composer.c | 4 +-
>>>>> drivers/gpu/drm/vkms/vkms_drv.h | 6 +-
>>>>> drivers/gpu/drm/vkms/vkms_plane.c | 2 +-
>>>>> drivers/gpu/drm/vkms/vkms_writeback.c | 2 +-
>>>>> drivers/gpu/drm/xen/xen_drm_front_gem.c | 7 +-
>>>>> drivers/gpu/drm/xen/xen_drm_front_gem.h | 6 +-
>>>>> .../common/videobuf2/videobuf2-dma-contig.c | 8 +-
>>>>> .../media/common/videobuf2/videobuf2-dma-sg.c | 9 +-
>>>>> .../common/videobuf2/videobuf2-vmalloc.c | 11 +-
>>>>> drivers/misc/fastrpc.c | 4 +-
>>>>> include/drm/drm_cache.h | 6 +-
>>>>> include/drm/drm_client.h | 7 +-
>>>>> include/drm/drm_gem.h | 6 +-
>>>>> include/drm/drm_gem_atomic_helper.h | 6 +-
>>>>> include/drm/drm_gem_cma_helper.h | 6 +-
>>>>> include/drm/drm_gem_framebuffer_helper.h | 8 +-
>>>>> include/drm/drm_gem_shmem_helper.h | 12 +-
>>>>> include/drm/drm_gem_ttm_helper.h | 6 +-
>>>>> include/drm/drm_gem_vram_helper.h | 9 +-
>>>>> include/drm/drm_prime.h | 6 +-
>>>>> include/drm/ttm/ttm_bo_api.h | 10 +-
>>>>> include/drm/ttm/ttm_kmap_iter.h | 10 +-
>>>>> include/drm/ttm/ttm_resource.h | 6 +-
>>>>> include/linux/dma-buf-map.h | 266
>>>>> ------------------
>>>>> include/linux/dma-buf.h | 12 +-
>>>>> include/linux/iosys-map.h | 257
>>>>> +++++++++++++++++
>>>>> 80 files changed, 579 insertions(+), 552 deletions(-)
>>>>> delete mode 100644 include/linux/dma-buf-map.h
>>>>> create mode 100644 include/linux/iosys-map.h
>>>>>
>>>>
>>
Powered by blists - more mailing lists