Received-SPF: pass (google.com: domain of linux-kernel+bounces-38891-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Thu, 25 Jan 2024 17:26:23 +0100
From: Joel Granados <j.granados@samsung.com>
To: Lu Baolu <baolu.lu@linux.intel.com>
CC: Joerg Roedel <joro@8bytes.org>, Will Deacon <will@kernel.org>, Robin
	Murphy <robin.murphy@arm.com>, Jason Gunthorpe <jgg@ziepe.ca>, Kevin Tian
	<kevin.tian@intel.com>, Jean-Philippe Brucker <jean-philippe@linaro.org>,
	Nicolin Chen <nicolinc@nvidia.com>, Yi Liu <yi.l.liu@intel.com>, Jacob Pan
	<jacob.jun.pan@linux.intel.com>, Longfang Liu <liulongfang@huawei.com>, Yan
	Zhao <yan.y.zhao@intel.com>, <iommu@lists.linux.dev>, <kvm@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, Jason Gunthorpe <jgg@nvidia.com>
Subject: Re: [PATCH v10 16/16] iommu: Make iommu_report_device_fault()
 reutrn void
Message-ID: <20240125162623.l3hg2i5k5kyh57cc@localhost>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg="pgp-sha512";
	protocol="application/pgp-signature"; boundary="hgjoc62knflwjg6w"
Content-Disposition: inline
In-Reply-To: <20240122054308.23901-17-baolu.lu@linux.intel.com>
CMS-TYPE: 201P
References: <20240122054308.23901-1-baolu.lu@linux.intel.com>
	<20240122054308.23901-17-baolu.lu@linux.intel.com>
	<CGME20240125162625eucas1p1e9ae0f7cc7ba63f88316552048e77401@eucas1p1.samsung.com>

--hgjoc62knflwjg6w
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Mon, Jan 22, 2024 at 01:43:08PM +0800, Lu Baolu wrote:
> As the iommu_report_device_fault() has been converted to auto-respond a
> page fault if it fails to enqueue it, there's no need to return a code
> in any case. Make it return void.
>=20
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  include/linux/iommu.h                       |  5 ++---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c |  4 ++--
>  drivers/iommu/intel/svm.c                   | 19 +++++++----------
>  drivers/iommu/io-pgfault.c                  | 23 +++++++--------------
>  4 files changed, 19 insertions(+), 32 deletions(-)
>=20
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index d7b6f4017254..1ccad10e8164 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -1549,7 +1549,7 @@ struct iopf_queue *iopf_queue_alloc(const char *nam=
e);
>  void iopf_queue_free(struct iopf_queue *queue);
>  int iopf_queue_discard_partial(struct iopf_queue *queue);
>  void iopf_free_group(struct iopf_group *group);
> -int iommu_report_device_fault(struct device *dev, struct iopf_fault *evt=
);
> +void iommu_report_device_fault(struct device *dev, struct iopf_fault *ev=
t);
>  void iopf_group_response(struct iopf_group *group,
>  			 enum iommu_page_response_code status);
>  #else
> @@ -1587,10 +1587,9 @@ static inline void iopf_free_group(struct iopf_gro=
up *group)
>  {
>  }
> =20
> -static inline int
> +static inline void
>  iommu_report_device_fault(struct device *dev, struct iopf_fault *evt)
>  {
> -	return -ENODEV;
>  }
> =20
>  static inline void iopf_group_response(struct iopf_group *group,
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/=
arm/arm-smmu-v3/arm-smmu-v3.c
> index 42eb59cb99f4..02580364acda 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -1455,7 +1455,7 @@ arm_smmu_find_master(struct arm_smmu_device *smmu, =
u32 sid)
>  /* IRQ and event handlers */
>  static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
>  {
> -	int ret;
> +	int ret =3D 0;
>  	u32 perm =3D 0;
>  	struct arm_smmu_master *master;
>  	bool ssid_valid =3D evt[0] & EVTQ_0_SSV;
> @@ -1511,7 +1511,7 @@ static int arm_smmu_handle_evt(struct arm_smmu_devi=
ce *smmu, u64 *evt)
>  		goto out_unlock;
>  	}
> =20
> -	ret =3D iommu_report_device_fault(master->dev, &fault_evt);
> +	iommu_report_device_fault(master->dev, &fault_evt);
>  out_unlock:
>  	mutex_unlock(&smmu->streams_mutex);
>  	return ret;
> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
> index 2f8716636dbb..b644d57da841 100644
> --- a/drivers/iommu/intel/svm.c
> +++ b/drivers/iommu/intel/svm.c
> @@ -561,14 +561,11 @@ static int prq_to_iommu_prot(struct page_req_dsc *r=
eq)
>  	return prot;
>  }
> =20
> -static int intel_svm_prq_report(struct intel_iommu *iommu, struct device=
 *dev,
> -				struct page_req_dsc *desc)
> +static void intel_svm_prq_report(struct intel_iommu *iommu, struct devic=
e *dev,
> +				 struct page_req_dsc *desc)
>  {
>  	struct iopf_fault event =3D { };
> =20
> -	if (!dev || !dev_is_pci(dev))
> -		return -ENODEV;
> -
>  	/* Fill in event data for device specific processing */
>  	event.fault.type =3D IOMMU_FAULT_PAGE_REQ;
>  	event.fault.prm.addr =3D (u64)desc->addr << VTD_PAGE_SHIFT;
> @@ -601,7 +598,7 @@ static int intel_svm_prq_report(struct intel_iommu *i=
ommu, struct device *dev,
>  		event.fault.prm.private_data[0] =3D ktime_to_ns(ktime_get());
>  	}
> =20
> -	return iommu_report_device_fault(dev, &event);
> +	iommu_report_device_fault(dev, &event);
>  }
> =20
>  static void handle_bad_prq_event(struct intel_iommu *iommu,
> @@ -704,12 +701,10 @@ static irqreturn_t prq_event_thread(int irq, void *=
d)
>  		if (!pdev)
>  			goto bad_req;
> =20
> -		if (intel_svm_prq_report(iommu, &pdev->dev, req))
> -			handle_bad_prq_event(iommu, req, QI_RESP_INVALID);
> -		else
> -			trace_prq_report(iommu, &pdev->dev, req->qw_0, req->qw_1,
> -					 req->priv_data[0], req->priv_data[1],
> -					 iommu->prq_seq_number++);
> +		intel_svm_prq_report(iommu, &pdev->dev, req);
> +		trace_prq_report(iommu, &pdev->dev, req->qw_0, req->qw_1,
> +				 req->priv_data[0], req->priv_data[1],
> +				 iommu->prq_seq_number++);
>  		pci_dev_put(pdev);
>  prq_advance:
>  		head =3D (head + sizeof(*req)) & PRQ_RING_MASK;
> diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
> index 6e63e5a02884..b64229dab976 100644
> --- a/drivers/iommu/io-pgfault.c
> +++ b/drivers/iommu/io-pgfault.c
> @@ -179,23 +179,21 @@ static struct iopf_group *iopf_group_alloc(struct i=
ommu_fault_param *iopf_param,
>   *
>   * Return: 0 on success and <0 on error.
>   */
Should you remove the documentation that describes the return also?

> -int iommu_report_device_fault(struct device *dev, struct iopf_fault *evt)
> +void iommu_report_device_fault(struct device *dev, struct iopf_fault *ev=
t)
>  {
>  	struct iommu_fault *fault =3D &evt->fault;
>  	struct iommu_fault_param *iopf_param;
>  	struct iopf_group abort_group =3D {};
>  	struct iopf_group *group;
> -	int ret;
> =20
>  	iopf_param =3D iopf_get_dev_fault_param(dev);
>  	if (WARN_ON(!iopf_param))
> -		return -ENODEV;
> +		return;
> =20
>  	if (!(fault->prm.flags & IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE)) {
> -		ret =3D report_partial_fault(iopf_param, fault);
> +		report_partial_fault(iopf_param, fault);
>  		iopf_put_dev_fault_param(iopf_param);
>  		/* A request that is not the last does not need to be ack'd */
> -		return ret;
>  	}
> =20
>  	/*
> @@ -207,25 +205,21 @@ int iommu_report_device_fault(struct device *dev, s=
truct iopf_fault *evt)
>  	 * leaving, otherwise partial faults will be stuck.
>  	 */
>  	group =3D iopf_group_alloc(iopf_param, evt, &abort_group);
> -	if (group =3D=3D &abort_group) {
> -		ret =3D -ENOMEM;
> +	if (group =3D=3D &abort_group)
>  		goto err_abort;
> -	}
> =20
>  	group->domain =3D get_domain_for_iopf(dev, fault);
> -	if (!group->domain) {
> -		ret =3D -EINVAL;
> +	if (!group->domain)
>  		goto err_abort;
> -	}
> =20
>  	/*
>  	 * On success iopf_handler must call iopf_group_response() and
>  	 * iopf_free_group()
>  	 */
> -	ret =3D group->domain->iopf_handler(group);
> -	if (ret)
> +	if (group->domain->iopf_handler(group))
>  		goto err_abort;
> -	return 0;
> +
> +	return;
> =20
>  err_abort:
>  	iopf_group_response(group, IOMMU_PAGE_RESP_FAILURE);
> @@ -233,7 +227,6 @@ int iommu_report_device_fault(struct device *dev, str=
uct iopf_fault *evt)
>  		__iopf_free_group(group);
>  	else
>  		iopf_free_group(group);
> -	return ret;
>  }
>  EXPORT_SYMBOL_GPL(iommu_report_device_fault);
> =20
> --=20
> 2.34.1
>=20

--=20

Joel Granados

--hgjoc62knflwjg6w
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQGzBAABCgAdFiEErkcJVyXmMSXOyyeQupfNUreWQU8FAmWyi60ACgkQupfNUreW
QU/cXwv/WTYpxuukYeG1ULXhsDflDlE9WKL3NptK5ter/s3pTuIn84oUoimE8JSW
3kOtDV2Eepa9MtZ4wvm8ZeXzf4XXnitUnZTL3hdAxOexe6YuzDwIVKhyXLSTAfI2
C2jkaEKBT3UfREjzmsA7Ku7XTmzjKGuTwcwmJOdF5zI3SHPMcDD4cPDG/siWrt+Y
rI49Q4olNhoaO4OfCygFKgZqTRPO2fnYz+p5L932Yk3cETYunxw7IqBWL+cVfWvX
GhGxrul9DyTdczJs0ZuYooG9rHnij/XpkNyl9QrcWc9S4m6o9taVEq/H1Sf4C3fO
zEBuyjmH1Mlr1mcQ4ANFls8GZiMzUamJw3RW8jGooEjH9pgF5UeDTMdtdAT8Bxtj
ifUiQurIAqbhxA+2Ea3qZQG/hfl/KLn/EtSRz3HsGkGLSi7S/+NM/wHC63mFzCtc
XSjgwI73labQHTg1rJn3Rcmxlu5TqmjB/C+vrqzlWf4yj725ZjCwn8rpicpO/Lf1
mCOfum9j
=qbta
-----END PGP SIGNATURE-----

--hgjoc62knflwjg6w--