From: Robin Murphy <robin.murphy@arm.com>
To: Nicolin Chen <nicolinc@nvidia.com>,
Jason Gunthorpe <jgg@nvidia.com>,
Jean-Philippe Brucker <jean-philippe@linaro.org>
Cc: joro@8bytes.org, suravee.suthikulpanit@amd.com, will@kernel.org,
robdclark@gmail.com, dwmw2@infradead.org,
baolu.lu@linux.intel.com, agross@kernel.org,
bjorn.andersson@linaro.org, konrad.dybcio@somainline.org,
matthias.bgg@gmail.com, heiko@sntech.de, orsonzhai@gmail.com,
baolin.wang@linux.alibaba.com, zhang.lyra@gmail.com,
thierry.reding@gmail.com, sricharan@codeaurora.org,
yong.wu@mediatek.com, vdumpa@nvidia.com, jonathanh@nvidia.com,
tglx@linutronix.de, shameerali.kolothum.thodi@huawei.com,
thunder.leizhen@huawei.com, christophe.jaillet@wanadoo.fr,
yangyingliang@huawei.com, jon@solid-run.com,
iommu@lists.linux.dev, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-arm-msm@vger.kernel.org,
linux-mediatek@lists.infradead.org,
linux-rockchip@lists.infradead.org, linux-tegra@vger.kernel.org,
virtualization@lists.linux-foundation.org
Subject: Re: [PATCH 4/5] iommu: Regulate errno in ->attach_dev callback functions
Date: Wed, 14 Sep 2022 20:53:07 +0100 [thread overview]
Message-ID: <5f049bb8-76e6-901a-9f8e-b48d270bc4a0@arm.com> (raw)
In-Reply-To: <YyIWQ6rX6AR9KX5E@Asurada-Nvidia>
On 2022-09-14 18:58, Nicolin Chen wrote:
> On Wed, Sep 14, 2022 at 10:49:42AM +0100, Jean-Philippe Brucker wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On Wed, Sep 14, 2022 at 06:11:06AM -0300, Jason Gunthorpe wrote:
>>> On Tue, Sep 13, 2022 at 01:27:03PM +0100, Jean-Philippe Brucker wrote:
>>>> I think in the future it will be too easy to forget about the constrained
>>>> return value of attach() while modifying some other part of the driver,
>>>> and let an external helper return EINVAL. So I'd rather not propagate ret
>>>> from outside of viommu_domain_attach() and finalise().
>>>
>>> Fortunately, if -EINVAL is wrongly returned it only creates an
>>> inefficiency, not a functional problem. So we do not need to be
>>> precise here.
>>
>> Ah fair. In that case the attach_dev() documentation should indicate that
>> EINVAL is a hint, so that callers don't rely on it (currently words "must"
>> and "exclusively" indicate that returning EINVAL for anything other than
>> device-domain incompatibility is unacceptable). The virtio-iommu
>> implementation may well return EINVAL from the virtio stack or from the
>> host response.
>
> How about this?
>
> + * * EINVAL - mainly, device and domain are incompatible, or something went
> + * wrong with the domain. It's suggested to avoid kernel prints
> + * along with this errno. And it's better to convert any EINVAL
> + * returned from kAPIs to ENODEV if it is device-specific, or to
> + * some other reasonable errno being listed below
FWIW, I'd say something like:
"The device and domain are incompatible. If this is due to some previous
configuration of the domain, drivers should not log an error, since it
is legitimate for callers to test reuse of an existing domain.
Otherwise, it may still represent some fundamental problem."
And then at the public interfaces state it from other angle:
"The device and domain are incompatible. If the domain has already been
used or configured in some way, attaching the same device to a different
domain may be expected to succeed. Otherwise, it may still represent
some fundamental problem."
[ and to save another mail, I'm not sure copying the default comment for
ENOSPC is all that helpful either - what is "space" for something that
isn't a storage device? I'd guess limited hardware resources in some
form, but in the IOMMU context, potential confusion with address space
is maybe a little too close for comfort? ]
>>>> Since we can't guarantee that APIs like virtio or ida won't ever return
>>>> EINVAL, we should set all return values:
>>>
>>> I dislike this alot, it squashes all return codes to try to optimize
>>> an obscure failure path :(
>
> Hmm...should I revert all the driver changes back to this version?
Yeah, I don't think we need to go too mad here. Drivers shouldn't emit
their *own* -EINVAL unless appropriate, but if it comes back from some
external API then that implies something's gone unexpectedly wrong
anyway - maybe it's a transient condition and a subsequent different
attach might actually work out OK? We can't really say in general.
Besides, if the driver sees an error which implies it's done something
wrong itself, it probably shouldn't be trusted to try to reason about it
further. The caller can handle any error as long as we set their
expectations correctly.
Thanks,
Robin.
next prev parent reply other threads:[~2022-09-14 19:53 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-13 8:24 [PATCH 0/5] iommu: Define EINVAL as device/domain incompatibility Nicolin Chen
2022-09-13 8:24 ` [PATCH 1/5] iommu/msm: Add missing __disable_clocks calls Nicolin Chen
2022-09-13 8:24 ` [PATCH 2/5] iommu/amd: Drop unnecessary checks in amd_iommu_attach_device() Nicolin Chen
2022-09-13 8:24 ` [PATCH 3/5] iommu: Add return errno rules to ->attach_dev ops Nicolin Chen
2022-09-13 18:41 ` Jeff Johnson
2022-09-13 20:00 ` Nicolin Chen
2022-09-13 8:24 ` [PATCH 4/5] iommu: Regulate errno in ->attach_dev callback functions Nicolin Chen
2022-09-13 12:27 ` Jean-Philippe Brucker
2022-09-13 20:14 ` Nicolin Chen
2022-09-14 9:11 ` Jason Gunthorpe
2022-09-14 9:49 ` Jean-Philippe Brucker
2022-09-14 17:58 ` Nicolin Chen
2022-09-14 19:53 ` Robin Murphy [this message]
2022-09-14 20:55 ` Nicolin Chen
2022-09-13 8:24 ` [PATCH 5/5] iommu: Use EINVAL for incompatible device/domain in ->attach_dev Nicolin Chen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5f049bb8-76e6-901a-9f8e-b48d270bc4a0@arm.com \
--to=robin.murphy@arm.com \
--cc=agross@kernel.org \
--cc=baolin.wang@linux.alibaba.com \
--cc=baolu.lu@linux.intel.com \
--cc=bjorn.andersson@linaro.org \
--cc=christophe.jaillet@wanadoo.fr \
--cc=dwmw2@infradead.org \
--cc=heiko@sntech.de \
--cc=iommu@lists.linux.dev \
--cc=jean-philippe@linaro.org \
--cc=jgg@nvidia.com \
--cc=jon@solid-run.com \
--cc=jonathanh@nvidia.com \
--cc=joro@8bytes.org \
--cc=konrad.dybcio@somainline.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=linux-rockchip@lists.infradead.org \
--cc=linux-tegra@vger.kernel.org \
--cc=matthias.bgg@gmail.com \
--cc=nicolinc@nvidia.com \
--cc=orsonzhai@gmail.com \
--cc=robdclark@gmail.com \
--cc=shameerali.kolothum.thodi@huawei.com \
--cc=sricharan@codeaurora.org \
--cc=suravee.suthikulpanit@amd.com \
--cc=tglx@linutronix.de \
--cc=thierry.reding@gmail.com \
--cc=thunder.leizhen@huawei.com \
--cc=vdumpa@nvidia.com \
--cc=virtualization@lists.linux-foundation.org \
--cc=will@kernel.org \
--cc=yangyingliang@huawei.com \
--cc=yong.wu@mediatek.com \
--cc=zhang.lyra@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).