Re: [PATCH v2 4/8] iommu: Factor out some helpers

[Jason Gunthorpe]

On Mon, Jan 30, 2023 at 11:33:54PM +0000, Robin Murphy wrote:

> Please understand that I'm not going to this length just to be
> objectionable; this is me genuinely being unable to rationalise your
> argument and spending my entire evening on a deep dive into my own thought
> process to lay it out and check for any obvious errors.

Sorry, I don't want to use up your time like this. It is a minor style
remark, if you don't like it then you should go ahead with your
original.

It is hard to debate style, everyone has their own viewpoint of what
is better style or not.

But to elaborate my feeling, I find this:

	const struct iommu_ops *ops = dev_iommu_ops(dev);

	if (!ops)
	   return -ENODEV

To be nicer code and more kernely then this:

	const struct iommu_ops *ops;

	if (!dev_iomm_ops_valid(dev))
	   return -ENODEV
        ops = dev_iommu_ops(dev);

In part because the former is a harder to type wrong and when used
consistently it is alot more obvious that the NULL test is
missing. static checkers like smatch/coverity will even warn on the
obvious possible NULL deref if someone misses the NULL test.

In general in kernel we have the API flow of call a function and check
the result for error, asking permission to call an API is less
typical.

This case is complicated because of the effort to try and document
cases that can't fail. I'm not sure if this is worthwhile, but..

Documentation of special cases is better by labeling them as a special
case, eg with a special function name. Think
rcu_dereference_protected(). Making them special by observing a
missing related function call is trickier to learn and remember.

Also if you rely on ops testing you are sort of forced into a second
function because the static tools will complain about all the places
that don't test for null if only one API is provided.

Basically, if you have dev_iommu_ops/dev_iommu_ops_safe then the
choice to use safe is obviously documented in the code and if you
mis-use dev_iommu_ops then a static tool will complain. Reviewers who
see 'safe' in a diff are reminded to look at it for safety rules.

If you have dev_iommu_ops/dev_iommu_ops_valid then static tools will
never complain and the choice to use 'safe' is implicitly documented
by not calling dev_iommu_ops_valid. Reviewers have to remember to
check every call to dev_iommu_ops() to see if it should have the valid
check.

Regards,
Jason