Re: [PATCH 4/5] iommu: Regulate errno in ->attach_dev callback functions

From: Nicolin Chen <nicolinc@nvidia.com>
To: Jean-Philippe Brucker <jean-philippe@linaro.org>
Cc: heiko@sntech.de, konrad.dybcio@somainline.org,
	bjorn.andersson@linaro.org, linux-tegra@vger.kernel.org,
	thierry.reding@gmail.com, will@kernel.org, zhang.lyra@gmail.com,
	joro@8bytes.org, jon@solid-run.com, jonathanh@nvidia.com,
	linux-rockchip@lists.infradead.org, iommu@lists.linux.dev,
	agross@kernel.org, linux-arm-kernel@lists.infradead.org,
	jgg@nvidia.com, yangyingliang@huawei.com, orsonzhai@gmail.com,
	linux-arm-msm@vger.kernel.org, robin.murphy@arm.com,
	christophe.jaillet@wanadoo.fr, baolin.wang@linux.alibaba.com,
	thunder.leizhen@huawei.com, matthias.bgg@gmail.com,
	tglx@linutronix.de, virtualization@lists.linux-foundation.org,
	yong.wu@mediatek.com, dwmw2@infradead.org,
	linux-kernel@vger.kernel.org,
	shameerali.kolothum.thodi@huawei.com, robdclark@gmail.com,
	suravee.suthikulpanit@amd.com,
	linux-mediatek@lists.infradead.org, sricharan@codeaurora.org,
	baolu.lu@linux.intel.com
Subject: Re: [PATCH 4/5] iommu: Regulate errno in ->attach_dev callback functions
Date: Tue, 13 Sep 2022 13:14:24 -0700	[thread overview]
Message-ID: <YyDkoIqWb8fy+vsi@Asurada-Nvidia> (raw)
In-Reply-To: <YyB3F/o3RfymqiFW@myrica>

Hi Jean,

On Tue, Sep 13, 2022 at 01:27:03PM +0100, Jean-Philippe Brucker wrote:
> External email: Use caution opening links or attachments
> 
> 
> Hi Nicolin,
> 
> On Tue, Sep 13, 2022 at 01:24:47AM -0700, Nicolin Chen wrote:
> > Following the new rules in include/linux/iommu.h kdocs, update all drivers
> > ->attach_dev callback functions to return ENODEV error code for all device
> > specific errors. It particularly excludes EINVAL from being used for such
> > error cases. For the same purpose, also replace one EINVAL with ENOMEM in
> > mtk_iommu driver.
> >
> > Note that the virtio-iommu does a viommu_domain_map_identity() call, which
> > returns either 0 or ENOMEM at this moment. Change to "return ret" directly
> > to allow it to pass an EINVAL in the future.
> [...]
> > diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
> > index 80151176ba12..874c01634d2b 100644
> > --- a/drivers/iommu/virtio-iommu.c
> > +++ b/drivers/iommu/virtio-iommu.c
> > @@ -696,7 +696,7 @@ static int viommu_domain_finalise(struct viommu_endpoint *vdev,
> >               if (ret) {
> >                       ida_free(&viommu->domain_ids, vdomain->id);
> >                       vdomain->viommu = NULL;
> > -                     return -EOPNOTSUPP;
> > +                     return ret;
> 
> 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().
> 
> For the same reason I do prefer this solution over EMEDIUMTYPE, because
> it's too tempting to use exotic errno when they seem appropriate instead
> of boring ENODEV and EINVAL. The alternative would be adding a special
> purpose code to linux/errno.h, similarly to EPROBE_DEFER, but that might
> be excessive.
> 
> Since we can't guarantee that APIs like virtio or ida won't ever return
> EINVAL, we should set all return values:

Thanks for the inputs. Assuming your attached patch isn't officially
sent, I will group it into my next version.

Similarly, I will also double check other drivers, to make sure all
of them have explicit return values, other than "ret".

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel