This pattern for picking the first device out of the group list is
repeated a few times now, so it's clearly worth factoring out.
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
drivers/iommu/iommu.c | 22 ++++++++++------------
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index bc53ffbba4de..5b37766a09e2 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1084,6 +1084,11 @@ void iommu_group_remove_device(struct device *dev)
}
EXPORT_SYMBOL_GPL(iommu_group_remove_device);
+static struct device *iommu_group_first_dev(struct iommu_group *group)
+{
+ return list_first_entry(&group->devices, struct group_device, list)->dev;
+}
+
static int iommu_group_device_count(struct iommu_group *group)
{
struct group_device *entry;
@@ -2835,7 +2840,6 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
struct device *prev_dev, int type)
{
struct iommu_domain *prev_dom;
- struct group_device *grp_dev;
int ret, dev_def_dom;
struct device *dev;
@@ -2867,8 +2871,7 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
}
/* Since group has only one device */
- grp_dev = list_first_entry(&group->devices, struct group_device, list);
- dev = grp_dev->dev;
+ dev = iommu_group_first_dev(group);
if (prev_dev != dev) {
dev_err_ratelimited(prev_dev, "Cannot change default domain: Device has been changed\n");
@@ -2965,7 +2968,6 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
static ssize_t iommu_group_store_type(struct iommu_group *group,
const char *buf, size_t count)
{
- struct group_device *grp_dev;
struct device *dev;
int ret, req_type;
@@ -3000,8 +3002,7 @@ static ssize_t iommu_group_store_type(struct iommu_group *group,
}
/* Since group has only one device */
- grp_dev = list_first_entry(&group->devices, struct group_device, list);
- dev = grp_dev->dev;
+ dev = iommu_group_first_dev(group);
get_device(dev);
/*
@@ -3126,21 +3127,18 @@ void iommu_device_unuse_default_domain(struct device *dev)
static int __iommu_group_alloc_blocking_domain(struct iommu_group *group)
{
- struct group_device *dev =
- list_first_entry(&group->devices, struct group_device, list);
+ struct device *dev = iommu_group_first_dev(group);
if (group->blocking_domain)
return 0;
- group->blocking_domain =
- __iommu_domain_alloc(dev->dev->bus, IOMMU_DOMAIN_BLOCKED);
+ group->blocking_domain = __iommu_domain_alloc(dev->bus, IOMMU_DOMAIN_BLOCKED);
if (!group->blocking_domain) {
/*
* For drivers that do not yet understand IOMMU_DOMAIN_BLOCKED
* create an empty domain instead.
*/
- group->blocking_domain = __iommu_domain_alloc(
- dev->dev->bus, IOMMU_DOMAIN_UNMANAGED);
+ group->blocking_domain = __iommu_domain_alloc(dev->bus, IOMMU_DOMAIN_UNMANAGED);
if (!group->blocking_domain)
return -EINVAL;
}
--
2.36.1.dirtyOn Thu, Jan 19, 2023 at 07:18:21PM +0000, Robin Murphy wrote:
> This pattern for picking the first device out of the group list is
> repeated a few times now, so it's clearly worth factoring out.
>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
> drivers/iommu/iommu.c | 22 ++++++++++------------
> 1 file changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index bc53ffbba4de..5b37766a09e2 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1084,6 +1084,11 @@ void iommu_group_remove_device(struct device *dev)
> }
> EXPORT_SYMBOL_GPL(iommu_group_remove_device);
>
> +static struct device *iommu_group_first_dev(struct iommu_group *group)
> +{
Add a
lockdep_assert_held(&group->lock);
?
> - group->blocking_domain =
> - __iommu_domain_alloc(dev->dev->bus, IOMMU_DOMAIN_BLOCKED);
> + group->blocking_domain = __iommu_domain_alloc(dev->bus, IOMMU_DOMAIN_BLOCKED);
> if (!group->blocking_domain) {
> /*
> * For drivers that do not yet understand IOMMU_DOMAIN_BLOCKED
> * create an empty domain instead.
> */
> - group->blocking_domain = __iommu_domain_alloc(
> - dev->dev->bus, IOMMU_DOMAIN_UNMANAGED);
> + group->blocking_domain = __iommu_domain_alloc(dev->bus, IOMMU_DOMAIN_UNMANAGED);
> if (!group->blocking_domain)
> return -EINVAL;
These are extra hunks?
Jason
On 19/01/2023 7:23 pm, Jason Gunthorpe wrote:
> On Thu, Jan 19, 2023 at 07:18:21PM +0000, Robin Murphy wrote:
>> This pattern for picking the first device out of the group list is
>> repeated a few times now, so it's clearly worth factoring out.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>> drivers/iommu/iommu.c | 22 ++++++++++------------
>> 1 file changed, 10 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index bc53ffbba4de..5b37766a09e2 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -1084,6 +1084,11 @@ void iommu_group_remove_device(struct device *dev)
>> }
>> EXPORT_SYMBOL_GPL(iommu_group_remove_device);
>>
>> +static struct device *iommu_group_first_dev(struct iommu_group *group)
>> +{
>
> Add a
> lockdep_assert_held(&group->lock);
>
> ?
Sure, could do. I guess I didn't consider it since
iommu_group_device_count() and __iommu_group_for_each_dev() don't assert
it either, but that's not to say they couldn't change too.
>> - group->blocking_domain =
>> - __iommu_domain_alloc(dev->dev->bus, IOMMU_DOMAIN_BLOCKED);
>> + group->blocking_domain = __iommu_domain_alloc(dev->bus, IOMMU_DOMAIN_BLOCKED);
>> if (!group->blocking_domain) {
>> /*
>> * For drivers that do not yet understand IOMMU_DOMAIN_BLOCKED
>> * create an empty domain instead.
>> */
>> - group->blocking_domain = __iommu_domain_alloc(
>> - dev->dev->bus, IOMMU_DOMAIN_UNMANAGED);
>> + group->blocking_domain = __iommu_domain_alloc(dev->bus, IOMMU_DOMAIN_UNMANAGED);
>> if (!group->blocking_domain)
>> return -EINVAL;
>
> These are extra hunks?
The annoyingly-subtle difference between "dev->dev->bus" and "dev->bus"
is precisely one of the reasons I think hiding the group_device behind a
helper is worthwhile ;)
Thanks,
Robin.
© 2016 - 2025 Red Hat, Inc.