Re: [PATCH v2 23/23] dma-buf: Add kunit tests for mapping type selection

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Low] dma-buf: duplicate KUNIT_EXPECT_PTR_EQ() added in
  test_mock_attach()
- [Low] dma-buf: duplicated callback setup and dead field in
  mock_mapping
- [Low] dma-buf: commit message omits failed map and pin selection tests
--

commit 71d342095c556fe9ee3e5b69defcc3e410afbe6f
Author: Alistair Popple <apopple@nvidia.com>

dma-buf: Add kunit tests for mapping type selection

This commit adds a configurable offer list, a select_mapping() callback, and
map/unmap operations for the mock mapping types in the dma-buf KUnit test
suite. It also introduces new KUnit test cases to verify the mapping type
selection contract across attach, map, pin, and failure paths.

>     dma-buf: Add kunit tests for mapping type selection
>     
>     Add an offer list and a select_mapping() callback to the mock exporter
>     along with map/unmap for the two mock types.
>     
>     Adds tests for the exporter/importer contract.
>     
>      - test_match_set_is_final: a later importer attaching, even one the
>        exporter answers differently, does not change an existing
>        attachment's match set or default. Later attachments influence what
>        the exporter selects at pin and map time, not previous attachments.
>     
>      - test_select_at_map: the exporter chooses a non-default entry
>        at the first map and the result is tagged with it. A pin taken
>        under the live mapping inherits the type and the choice survives
>        unmap/remap even though the exporter would answer differently. Only
>        dma_buf_invalidate_mappings() allows the exporter to make a different
>        selection. An -EBUSY from selection leaves nothing established.
>     
>      - test_select_at_pin: pin selects, and the choice survives map, unmap
>        and unpin so a pinned importer that is never invalidated keeps its
>        first type for the life of the import.
>     
>     Signed-off-by: Alistair Popple <apopple@nvidia.com>

[Severity: Low]
This isn't a bug, but the commit message only lists test_match_set_is_final,
test_select_at_map, and test_select_at_pin, while the patch also adds
mock_dmabuf_pin(), mock_dmabuf_unpin(), failure injection state, and two
additional test cases in dma_mapping_cases (test_select_survives_failed_map
and test_select_after_failed_pin).

Could the commit message be updated to describe these two failed map and pin
selection tests as well?

> diff --git a/drivers/dma-buf/st-dma-mapping.c b/drivers/dma-buf/st-dma-mapping.c
> index cbfa99aeb0ff5..11010731150ae 100644
> --- a/drivers/dma-buf/st-dma-mapping.c
> +++ b/drivers/dma-buf/st-dma-mapping.c

[ ... ]

> @@ -370,11 +391,478 @@ static void test_mock_attach(struct kunit *test)
>  	KUNIT_EXPECT_PTR_EQ(test, dma_buf_attach_map_type(attach)->type,
>  			    &mock_type_a);
>  	KUNIT_EXPECT_PTR_EQ(test, mock_match_dev_arg, dev);
> +	KUNIT_EXPECT_PTR_EQ(test, mock_match_dev_arg, dev);

[Severity: Low]
This isn't a bug, but was this second KUNIT_EXPECT_PTR_EQ(test,
mock_match_dev_arg, dev) in test_mock_attach() added by accident?

[ ... ]

> +/* Mock mapping result for the generic map path */
> +struct mock_mapping {
> +	struct dma_buf_mapping base;
> +	const struct dma_buf_mapping_type *made_by;
> +};
> +
> +static struct dma_buf_mapping *mock_type_map(struct dma_buf_attachment *attach,
> +					     enum dma_data_direction dir)
> +{
> +	struct mock_mapping *m;
> +
> +	if (mock_map_err)
> +		return ERR_PTR(mock_map_err);
> +	m = kzalloc_obj(*m);
> +	if (!m)
> +		return ERR_PTR(-ENOMEM);
> +	m->base.type = dma_buf_attach_map_type(attach)->type;
> +	m->made_by = dma_buf_attach_map_type(attach)->type;
> +	return &m->base;
> +}

[ ... ]

> +static void test_select_at_map(struct kunit *test)
> +{
> +	static const struct dma_buf_mapping_match offer_ab[] = {
> +		{ .type = &mock_type_a },
> +		{ .type = &mock_type_b },
> +	};
> +	struct dma_buf_mapping_match imp[] = {
> +		{ .type = &mock_type_a },
> +		{ .type = &mock_type_b },
> +	};
> +	MOCK_IMPORTER_OPS(importer_ops, imp);
> +	struct dma_buf_attachment *attach;
> +	struct dma_buf_mapping *mapping;
> +	struct dma_buf *dmabuf;
> +	struct device *dev;
> +
> +	mock_type_a.map = mock_type_map;
> +	mock_type_a.unmap = mock_type_unmap;
> +	mock_type_b.map = mock_type_map;
> +	mock_type_b.unmap = mock_type_unmap;

[Severity: Low]
This isn't a bug, but test_select_at_map(), test_select_at_pin(),
test_select_survives_failed_map(), and test_select_after_failed_pin() each
manually assign map and unmap on mock_type_a and mock_type_b at the start of
the test and clear them to NULL at the end. If any KUNIT_ASSERT_NOT_ERR_OR_NULL
in those tests fails, the test aborts before clearing the pointers and
reset_mock_state() does not reset them.

Since none of the other tests in drivers/dma-buf/st-dma-mapping.c require
mock_type_a.map or mock_type_b.map to be NULL, could map and unmap be
initialized directly in the static definitions of mock_type_a and mock_type_b
instead? Also, is the made_by field in struct mock_mapping needed given that
mock_type_map() sets m->base.type to the same value and made_by is never read?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-dma_mapping_type_v4-v2-0-78b8dc9ae80a@nvidia.com?part=23