On 3/26/26 22:38, Jakub Kicinski wrote:
On Wed, 25 Mar 2026 07:26:52 +0100 Przemek Kitszel wrote:
Current API makes it possible to access shared devlink instance's priv
data:
void *devlink_shd_get_priv(struct devlink *devlink);
but it is easy to forget (especially during rebase from "before shared
devlinks" era) and call:
void *devlink_priv(struct devlink *devlink);
which even has the same signature, so it's hard to catch the error.
The implicit conversion may make things hard to reason about.
hmm...
now we have simple devlink code with a possibility of hard to detect bug
in drivers, plus hypothetical drivers code that we will need to reason
about. I would replace that for:
"complex devlink code", which will be harder to review, but then without
bugs and with hypothetical drivers code that would not require reasoning
at all (one API call == no decision, and no checking for correctness
later, when someone will be hunting an unrelated bug)
Anyway, I have used "hypothetical drivers code" twice, so maybe it will
be best to forgo this patch. I will just call correct functions in ice,
and we will wait for other users with priv data on shd devlink to see
if they struggle.
The priv_to_devlink issue that Alex pointed out will remain unresolved,
although it's easy to just keep the devlink pointer in priv (as I will
do in ice).
Thank you.
Are you sure you actually mean that it's "easy to forget" or
it's easier for OOT transition?
I have literally forgot to change (written from scratch for upstream)
code, in the part that was rebased conflict-free. I guess, I was the
only person that were in need of devlink-shared, with code ready to
rebase over Jiri's work. And I'm done, so there is not much relevance
now for this point.
If we are worried about misuse we should instead add an accessor
for "individual" (better name welcome) instance and WARN_ON()
when devlink_priv() is used in the shared setup.
that would require the same amount of code as this patch (curr ver)
has, only with WARN_ON() instead proper value (IOW: we detect what
developer wanted, and give them big warning instead)
I'm not interested :)
Or add a third argument to devlink_priv() which will pass the size
of the LHS ptr, and warn on attempts to access priv of the wrong
size?
this could incidentally match sizeof(devlink_shd) :P
