diff mbox series

[v2,05/12] iommu: Change the return value of dev_iommu_get()

Message ID 20230727054837.147050-6-baolu.lu@linux.intel.com
State New
Headers
Series iommu: Prepare to deliver page faults to user space |

Commit Message

Baolu Lu July 27, 2023, 5:48 a.m. UTC 
  Make dev_iommu_get() return 0 for success and error numbers for failure.
This will make the code neat and readable. No functionality changes.

Reviewed-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/iommu.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

Comments

Tian, Kevin Aug. 3, 2023, 7:59 a.m. UTC | #1 
> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Thursday, July 27, 2023 1:49 PM
> 
> Make dev_iommu_get() return 0 for success and error numbers for failure.
> This will make the code neat and readable. No functionality changes.
> 
> Reviewed-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/iommu.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 00309f66153b..4ba3bb692993 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -290,20 +290,20 @@ void iommu_device_unregister(struct
> iommu_device *iommu)
>  }
>  EXPORT_SYMBOL_GPL(iommu_device_unregister);
> 
> -static struct dev_iommu *dev_iommu_get(struct device *dev)
> +static int dev_iommu_get(struct device *dev)
>  {
>  	struct dev_iommu *param = dev->iommu;
> 
>  	if (param)
> -		return param;
> +		return 0;
> 
>  	param = kzalloc(sizeof(*param), GFP_KERNEL);
>  	if (!param)
> -		return NULL;
> +		return -ENOMEM;
> 
>  	mutex_init(&param->lock);
>  	dev->iommu = param;
> -	return param;
> +	return 0;
>  }
> 

Jason's series [1] has been queued. Time to refine according to
the discussion in [2].

[1] https://lore.kernel.org/linux-iommu/ZLFYXlSBZrlxFpHM@8bytes.org/
[2] https://lore.kernel.org/linux-iommu/c815fa2b-00df-91e1-8353-8258773957e4@linux.intel.com/
Baolu Lu Aug. 4, 2023, 3:10 a.m. UTC | #2 
On 2023/8/3 15:59, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> Sent: Thursday, July 27, 2023 1:49 PM
>>
>> Make dev_iommu_get() return 0 for success and error numbers for failure.
>> This will make the code neat and readable. No functionality changes.
>>
>> Reviewed-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> ---
>>   drivers/iommu/iommu.c | 19 +++++++++++--------
>>   1 file changed, 11 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 00309f66153b..4ba3bb692993 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -290,20 +290,20 @@ void iommu_device_unregister(struct
>> iommu_device *iommu)
>>   }
>>   EXPORT_SYMBOL_GPL(iommu_device_unregister);
>>
>> -static struct dev_iommu *dev_iommu_get(struct device *dev)
>> +static int dev_iommu_get(struct device *dev)
>>   {
>>   	struct dev_iommu *param = dev->iommu;
>>
>>   	if (param)
>> -		return param;
>> +		return 0;
>>
>>   	param = kzalloc(sizeof(*param), GFP_KERNEL);
>>   	if (!param)
>> -		return NULL;
>> +		return -ENOMEM;
>>
>>   	mutex_init(&param->lock);
>>   	dev->iommu = param;
>> -	return param;
>> +	return 0;
>>   }
>>
> 
> Jason's series [1] has been queued. Time to refine according to
> the discussion in [2].
> 
> [1] https://lore.kernel.org/linux-iommu/ZLFYXlSBZrlxFpHM@8bytes.org/
> [2] https://lore.kernel.org/linux-iommu/c815fa2b-00df-91e1-8353-8258773957e4@linux.intel.com/

I'm not sure I understand your point here. This only changes the return
value of dev_iommu_get() to make the code more concise.

Best regards,
baolu
Tian, Kevin Aug. 4, 2023, 3:55 a.m. UTC | #3 
> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Friday, August 4, 2023 11:10 AM
> 
> On 2023/8/3 15:59, Tian, Kevin wrote:
> >> From: Lu Baolu <baolu.lu@linux.intel.com>
> >> Sent: Thursday, July 27, 2023 1:49 PM
> >>
> >> Make dev_iommu_get() return 0 for success and error numbers for failure.
> >> This will make the code neat and readable. No functionality changes.
> >>
> >> Reviewed-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> >> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> >> ---
> >>   drivers/iommu/iommu.c | 19 +++++++++++--------
> >>   1 file changed, 11 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> >> index 00309f66153b..4ba3bb692993 100644
> >> --- a/drivers/iommu/iommu.c
> >> +++ b/drivers/iommu/iommu.c
> >> @@ -290,20 +290,20 @@ void iommu_device_unregister(struct
> >> iommu_device *iommu)
> >>   }
> >>   EXPORT_SYMBOL_GPL(iommu_device_unregister);
> >>
> >> -static struct dev_iommu *dev_iommu_get(struct device *dev)
> >> +static int dev_iommu_get(struct device *dev)
> >>   {
> >>   	struct dev_iommu *param = dev->iommu;
> >>
> >>   	if (param)
> >> -		return param;
> >> +		return 0;
> >>
> >>   	param = kzalloc(sizeof(*param), GFP_KERNEL);
> >>   	if (!param)
> >> -		return NULL;
> >> +		return -ENOMEM;
> >>
> >>   	mutex_init(&param->lock);
> >>   	dev->iommu = param;
> >> -	return param;
> >> +	return 0;
> >>   }
> >>
> >
> > Jason's series [1] has been queued. Time to refine according to
> > the discussion in [2].
> >
> > [1] https://lore.kernel.org/linux-iommu/ZLFYXlSBZrlxFpHM@8bytes.org/
> > [2] https://lore.kernel.org/linux-iommu/c815fa2b-00df-91e1-8353-
> 8258773957e4@linux.intel.com/
> 
> I'm not sure I understand your point here. This only changes the return
> value of dev_iommu_get() to make the code more concise.
> 

I thought the purpose of this patch was to prepare for next patch which
moves dev->fault_param initialization to dev_iommu_get().

with Jason's rework IMHO that initialization more fits in iommu_init_device().

that's my real point. If you still want to clean up dev_iommu_get() it's fine
but then it may not belong to this series. 😊
Baolu Lu Aug. 4, 2023, 5:33 a.m. UTC | #4 
On 8/4/23 11:55 AM, Tian, Kevin wrote:
>> From: Baolu Lu<baolu.lu@linux.intel.com>
>> Sent: Friday, August 4, 2023 11:10 AM
>>
>> On 2023/8/3 15:59, Tian, Kevin wrote:
>>>> From: Lu Baolu<baolu.lu@linux.intel.com>
>>>> Sent: Thursday, July 27, 2023 1:49 PM
>>>>
>>>> Make dev_iommu_get() return 0 for success and error numbers for failure.
>>>> This will make the code neat and readable. No functionality changes.
>>>>
>>>> Reviewed-by: Jacob Pan<jacob.jun.pan@linux.intel.com>
>>>> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
>>>> ---
>>>>    drivers/iommu/iommu.c | 19 +++++++++++--------
>>>>    1 file changed, 11 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>>> index 00309f66153b..4ba3bb692993 100644
>>>> --- a/drivers/iommu/iommu.c
>>>> +++ b/drivers/iommu/iommu.c
>>>> @@ -290,20 +290,20 @@ void iommu_device_unregister(struct
>>>> iommu_device *iommu)
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(iommu_device_unregister);
>>>>
>>>> -static struct dev_iommu *dev_iommu_get(struct device *dev)
>>>> +static int dev_iommu_get(struct device *dev)
>>>>    {
>>>>    	struct dev_iommu *param = dev->iommu;
>>>>
>>>>    	if (param)
>>>> -		return param;
>>>> +		return 0;
>>>>
>>>>    	param = kzalloc(sizeof(*param), GFP_KERNEL);
>>>>    	if (!param)
>>>> -		return NULL;
>>>> +		return -ENOMEM;
>>>>
>>>>    	mutex_init(&param->lock);
>>>>    	dev->iommu = param;
>>>> -	return param;
>>>> +	return 0;
>>>>    }
>>>>
>>> Jason's series [1] has been queued. Time to refine according to
>>> the discussion in [2].
>>>
>>> [1]https://lore.kernel.org/linux-iommu/ZLFYXlSBZrlxFpHM@8bytes.org/
>>> [2]https://lore.kernel.org/linux-iommu/c815fa2b-00df-91e1-8353-
>> 8258773957e4@linux.intel.com/
>>
>> I'm not sure I understand your point here. This only changes the return
>> value of dev_iommu_get() to make the code more concise.
>>
> I thought the purpose of this patch was to prepare for next patch which
> moves dev->fault_param initialization to dev_iommu_get().

Yes.

> 
> with Jason's rework IMHO that initialization more fits in iommu_init_device().
> 
> that's my real point. If you still want to clean up dev_iommu_get() it's fine
> but then it may not belong to this series. 😊

Ah, I see. Let me make a choice in the next version.

Best regards,
baolu
Jason Gunthorpe Aug. 9, 2023, 4:58 p.m. UTC | #5 
On Thu, Jul 27, 2023 at 01:48:30PM +0800, Lu Baolu wrote:
> Make dev_iommu_get() return 0 for success and error numbers for failure.
> This will make the code neat and readable. No functionality changes.
> 
> Reviewed-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/iommu.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 00309f66153b..4ba3bb692993 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -290,20 +290,20 @@ void iommu_device_unregister(struct iommu_device *iommu)
>  }
>  EXPORT_SYMBOL_GPL(iommu_device_unregister);

It could probably use a nicer name too?

iommu_alloc_dev_iommu() ?

Also with Joerg's current tree we can add a device_lock_assert() to
this function, from what I can tell.

Jason
Baolu Lu Aug. 10, 2023, 2:30 a.m. UTC | #6 
On 2023/8/10 0:58, Jason Gunthorpe wrote:
> On Thu, Jul 27, 2023 at 01:48:30PM +0800, Lu Baolu wrote:
>> Make dev_iommu_get() return 0 for success and error numbers for failure.
>> This will make the code neat and readable. No functionality changes.
>>
>> Reviewed-by: Jacob Pan<jacob.jun.pan@linux.intel.com>
>> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
>> ---
>>   drivers/iommu/iommu.c | 19 +++++++++++--------
>>   1 file changed, 11 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 00309f66153b..4ba3bb692993 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -290,20 +290,20 @@ void iommu_device_unregister(struct iommu_device *iommu)
>>   }
>>   EXPORT_SYMBOL_GPL(iommu_device_unregister);
> It could probably use a nicer name too?
> 
> iommu_alloc_dev_iommu() ?
> 
> Also with Joerg's current tree we can add a device_lock_assert() to
> this function, from what I can tell.

Sure. Will update them in the next version.

Best regards,
baolu
diff mbox series

Patch

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 00309f66153b..4ba3bb692993 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -290,20 +290,20 @@  void iommu_device_unregister(struct iommu_device *iommu)
 }
 EXPORT_SYMBOL_GPL(iommu_device_unregister);
 
-static struct dev_iommu *dev_iommu_get(struct device *dev)
+static int dev_iommu_get(struct device *dev)
 {
 	struct dev_iommu *param = dev->iommu;
 
 	if (param)
-		return param;
+		return 0;
 
 	param = kzalloc(sizeof(*param), GFP_KERNEL);
 	if (!param)
-		return NULL;
+		return -ENOMEM;
 
 	mutex_init(&param->lock);
 	dev->iommu = param;
-	return param;
+	return 0;
 }
 
 static void dev_iommu_free(struct device *dev)
@@ -346,8 +346,9 @@  static int iommu_init_device(struct device *dev, const struct iommu_ops *ops)
 	struct iommu_group *group;
 	int ret;
 
-	if (!dev_iommu_get(dev))
-		return -ENOMEM;
+	ret = dev_iommu_get(dev);
+	if (ret)
+		return ret;
 
 	if (!try_module_get(ops->owner)) {
 		ret = -EINVAL;
@@ -2743,12 +2744,14 @@  int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
 		      const struct iommu_ops *ops)
 {
 	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+	int ret;
 
 	if (fwspec)
 		return ops == fwspec->ops ? 0 : -EINVAL;
 
-	if (!dev_iommu_get(dev))
-		return -ENOMEM;
+	ret = dev_iommu_get(dev);
+	if (ret)
+		return ret;
 
 	/* Preallocate for the overwhelmingly common case of 1 ID */
 	fwspec = kzalloc(struct_size(fwspec, ids, 1), GFP_KERNEL);