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 <[email protected]> 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 <[email protected]> [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/[email protected]?part=23
