Hi,
here is the second version of the patch-set to wrap the
invocation of iommu_ops->add/remove_device() into functions.
The functions will do more setup stuff later when the the
iommu-related pointers in 'struct device' are consolidated.
Since version one this patch-set was rebased to v4.20-rc6
and I removed the pointer checks for the function pointers,
as suggested by Robin. I checked all 16 drivers and all of
them implement the add/remove_device call-backs.
Please review, if there are no objections I plan to queue
these patches in the IOMMU tree.
Thanks,
Joerg
Joerg Roedel (4):
iommu/sysfs: Rename iommu_release_device()
iommu: Consolitate ->add/remove_device() calls
iommu/of: Don't call iommu_ops->add_device directly
ACPI/IORT: Don't call iommu_ops->add_device directly
drivers/acpi/arm64/iort.c | 4 +--
drivers/iommu/iommu-sysfs.c | 12 ++++-----
drivers/iommu/iommu.c | 51 ++++++++++++++++++-------------------
drivers/iommu/of_iommu.c | 6 ++---
include/linux/iommu.h | 3 +++
5 files changed, 39 insertions(+), 37 deletions(-)
--
2.17.1
From: Joerg Roedel <[email protected]>
Make sure to invoke this call-back through the proper
function of the IOMMU-API.
Signed-off-by: Joerg Roedel <[email protected]>
---
drivers/iommu/of_iommu.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
index c5dd63072529..4d4847de727e 100644
--- a/drivers/iommu/of_iommu.c
+++ b/drivers/iommu/of_iommu.c
@@ -218,10 +218,10 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
ops = dev->iommu_fwspec->ops;
/*
* If we have reason to believe the IOMMU driver missed the initial
- * add_device callback for dev, replay it to get things in order.
+ * probe for dev, replay it to get things in order.
*/
- if (ops && ops->add_device && dev->bus && !dev->iommu_group)
- err = ops->add_device(dev);
+ if (dev->bus && !dev->iommu_group)
+ err = iommu_probe_device(dev);
/* Ignore all other errors apart from EPROBE_DEFER */
if (err == -EPROBE_DEFER) {
--
2.17.1
From: Joerg Roedel <[email protected]>
Put them into separate functions and call those where the
plain ops have been called before.
Signed-off-by: Joerg Roedel <[email protected]>
---
drivers/iommu/iommu.c | 51 +++++++++++++++++++++----------------------
include/linux/iommu.h | 3 +++
2 files changed, 28 insertions(+), 26 deletions(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index edbdf5d6962c..1e8f4e0c9198 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -110,6 +110,23 @@ void iommu_device_unregister(struct iommu_device *iommu)
spin_unlock(&iommu_device_lock);
}
+int iommu_probe_device(struct device *dev)
+{
+ const struct iommu_ops *ops = dev->bus->iommu_ops;
+
+ WARN_ON(dev->iommu_group);
+
+ return ops->add_device(dev);
+}
+
+void iommu_release_device(struct device *dev)
+{
+ const struct iommu_ops *ops = dev->bus->iommu_ops;
+
+ if (dev->iommu_group)
+ ops->remove_device(dev);
+}
+
static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
unsigned type);
static int __iommu_attach_device(struct iommu_domain *domain,
@@ -1117,16 +1134,7 @@ struct iommu_domain *iommu_group_default_domain(struct iommu_group *group)
static int add_iommu_group(struct device *dev, void *data)
{
- struct iommu_callback_data *cb = data;
- const struct iommu_ops *ops = cb->ops;
- int ret;
-
- if (!ops->add_device)
- return 0;
-
- WARN_ON(dev->iommu_group);
-
- ret = ops->add_device(dev);
+ int ret = iommu_probe_device(dev);
/*
* We ignore -ENODEV errors for now, as they just mean that the
@@ -1141,11 +1149,7 @@ static int add_iommu_group(struct device *dev, void *data)
static int remove_iommu_group(struct device *dev, void *data)
{
- struct iommu_callback_data *cb = data;
- const struct iommu_ops *ops = cb->ops;
-
- if (ops->remove_device && dev->iommu_group)
- ops->remove_device(dev);
+ iommu_release_device(dev);
return 0;
}
@@ -1153,27 +1157,22 @@ static int remove_iommu_group(struct device *dev, void *data)
static int iommu_bus_notifier(struct notifier_block *nb,
unsigned long action, void *data)
{
+ unsigned long group_action = 0;
struct device *dev = data;
- const struct iommu_ops *ops = dev->bus->iommu_ops;
struct iommu_group *group;
- unsigned long group_action = 0;
/*
* ADD/DEL call into iommu driver ops if provided, which may
* result in ADD/DEL notifiers to group->notifier
*/
if (action == BUS_NOTIFY_ADD_DEVICE) {
- if (ops->add_device) {
- int ret;
+ int ret;
- ret = ops->add_device(dev);
- return (ret) ? NOTIFY_DONE : NOTIFY_OK;
- }
+ ret = iommu_probe_device(dev);
+ return (ret) ? NOTIFY_DONE : NOTIFY_OK;
} else if (action == BUS_NOTIFY_REMOVED_DEVICE) {
- if (ops->remove_device && dev->iommu_group) {
- ops->remove_device(dev);
- return 0;
- }
+ iommu_release_device(dev);
+ return NOTIFY_OK;
}
/*
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index a1d28f42cb77..cb36f32cd3c2 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -398,6 +398,9 @@ void iommu_fwspec_free(struct device *dev);
int iommu_fwspec_add_ids(struct device *dev, u32 *ids, int num_ids);
const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode);
+int iommu_probe_device(struct device *dev);
+void iommu_release_device(struct device *dev);
+
#else /* CONFIG_IOMMU_API */
struct iommu_ops {};
--
2.17.1
From: Joerg Roedel <[email protected]>
Make sure to invoke this call-back through the proper
function of the IOMMU-API.
Signed-off-by: Joerg Roedel <[email protected]>
---
drivers/acpi/arm64/iort.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index 70f4e80b9246..d4f7c1adc048 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -805,8 +805,8 @@ static inline int iort_add_device_replay(const struct iommu_ops *ops,
{
int err = 0;
- if (ops->add_device && dev->bus && !dev->iommu_group)
- err = ops->add_device(dev);
+ if (dev->bus && !dev->iommu_group)
+ err = iommu_probe_device(dev);
return err;
}
--
2.17.1
From: Joerg Roedel <[email protected]>
Remove the iommu_ prefix from the function and a few other
static data structures so that the iommu_release_device name
can be re-used in iommu core code.
Signed-off-by: Joerg Roedel <[email protected]>
---
drivers/iommu/iommu-sysfs.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/iommu/iommu-sysfs.c b/drivers/iommu/iommu-sysfs.c
index 36d1a7ce7fc4..71c2249d3260 100644
--- a/drivers/iommu/iommu-sysfs.c
+++ b/drivers/iommu/iommu-sysfs.c
@@ -22,25 +22,25 @@ static struct attribute *devices_attr[] = {
NULL,
};
-static const struct attribute_group iommu_devices_attr_group = {
+static const struct attribute_group devices_attr_group = {
.name = "devices",
.attrs = devices_attr,
};
-static const struct attribute_group *iommu_dev_groups[] = {
- &iommu_devices_attr_group,
+static const struct attribute_group *dev_groups[] = {
+ &devices_attr_group,
NULL,
};
-static void iommu_release_device(struct device *dev)
+static void release_device(struct device *dev)
{
kfree(dev);
}
static struct class iommu_class = {
.name = "iommu",
- .dev_release = iommu_release_device,
- .dev_groups = iommu_dev_groups,
+ .dev_release = release_device,
+ .dev_groups = dev_groups,
};
static int __init iommu_dev_init(void)
--
2.17.1
On 2018/12/11 23:05, Joerg Roedel wrote:
> From: Joerg Roedel <[email protected]>
>
> Make sure to invoke this call-back through the proper
> function of the IOMMU-API.
>
> Signed-off-by: Joerg Roedel <[email protected]>
> ---
> drivers/acpi/arm64/iort.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
Robin and Lorenzo know this better than me, but it's
pretty straight forward to me,
Acked-by: Hanjun Guo <[email protected]>
Thanks
Hanjun
Hi Joerg,
This patch landed in today's linux-next and causes a regression.
On 2018-12-11 16:05, Joerg Roedel wrote:
> From: Joerg Roedel <[email protected]>
>
> Make sure to invoke this call-back through the proper
> function of the IOMMU-API.
>
> Signed-off-by: Joerg Roedel <[email protected]>
> ---
> drivers/iommu/of_iommu.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> index c5dd63072529..4d4847de727e 100644
> --- a/drivers/iommu/of_iommu.c
> +++ b/drivers/iommu/of_iommu.c
> @@ -218,10 +218,10 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
> ops = dev->iommu_fwspec->ops;
> /*
> * If we have reason to believe the IOMMU driver missed the initial
> - * add_device callback for dev, replay it to get things in order.
> + * probe for dev, replay it to get things in order.
> */
> - if (ops && ops->add_device && dev->bus && !dev->iommu_group)
> - err = ops->add_device(dev);
> + if (dev->bus && !dev->iommu_group)
> + err = iommu_probe_device(dev);
This change removes a check for NULL ops, what causes NULL pointer
exception on first device without IOMMU.
I'm also not sure if this is a good idea to call iommu_probe_device(),
which comes from dev->bus->iommu_ops, which might be different from ops
from local variable.
>
> /* Ignore all other errors apart from EPROBE_DEFER */
> if (err == -EPROBE_DEFER) {
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
Hi Joerg,
On 2018-12-19 15:34, Joerg Roedel wrote:
> Hi Marek,
>
> thanks for the report!
>
> On Wed, Dec 19, 2018 at 10:54:18AM +0100, Marek Szyprowski wrote:
>> On 2018-12-11 16:05, Joerg Roedel wrote:
>>> From: Joerg Roedel <[email protected]>
>>>
>>> Make sure to invoke this call-back through the proper
>>> function of the IOMMU-API.
>>>
>>> Signed-off-by: Joerg Roedel <[email protected]>
>>> ---
>>> drivers/iommu/of_iommu.c | 6 +++---
>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
>>> index c5dd63072529..4d4847de727e 100644
>>> --- a/drivers/iommu/of_iommu.c
>>> +++ b/drivers/iommu/of_iommu.c
>>> @@ -218,10 +218,10 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
>>> ops = dev->iommu_fwspec->ops;
>>> /*
>>> * If we have reason to believe the IOMMU driver missed the initial
>>> - * add_device callback for dev, replay it to get things in order.
>>> + * probe for dev, replay it to get things in order.
>>> */
>>> - if (ops && ops->add_device && dev->bus && !dev->iommu_group)
>>> - err = ops->add_device(dev);
>>> + if (dev->bus && !dev->iommu_group)
>>> + err = iommu_probe_device(dev);
>> This change removes a check for NULL ops, what causes NULL pointer
>> exception on first device without IOMMU.
> Bummer, this check was supposed to be in iommu_probe_device(), but
> apparently it got lost. Does the attached patch fix it?
Yes, it fixes this issue.
>> I'm also not sure if this is a good idea to call iommu_probe_device(),
>> which comes from dev->bus->iommu_ops, which might be different from ops
>> from local variable.
> The local variable comes from dev->iommu_fwspec->ops, which should be
> exactly the same as dev->bus->iommu_ops. I'll leave that for now until
> it turns out to be a problem (which I don't expect).
>
>
> Regards,
>
> Joerg
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index a2131751dcff..3ed4db334341 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -114,10 +114,14 @@ void iommu_device_unregister(struct iommu_device *iommu)
> int iommu_probe_device(struct device *dev)
> {
> const struct iommu_ops *ops = dev->bus->iommu_ops;
> + int ret = -EINVAL;
>
> WARN_ON(dev->iommu_group);
>
> - return ops->add_device(dev);
> + if (ops)
> + ret = ops->add_device(dev);
> +
> + return ret;
> }
>
> void iommu_release_device(struct device *dev)
>
>
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
Hi Marek,
thanks for the report!
On Wed, Dec 19, 2018 at 10:54:18AM +0100, Marek Szyprowski wrote:
> On 2018-12-11 16:05, Joerg Roedel wrote:
> > From: Joerg Roedel <[email protected]>
> >
> > Make sure to invoke this call-back through the proper
> > function of the IOMMU-API.
> >
> > Signed-off-by: Joerg Roedel <[email protected]>
> > ---
> > drivers/iommu/of_iommu.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> > index c5dd63072529..4d4847de727e 100644
> > --- a/drivers/iommu/of_iommu.c
> > +++ b/drivers/iommu/of_iommu.c
> > @@ -218,10 +218,10 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
> > ops = dev->iommu_fwspec->ops;
> > /*
> > * If we have reason to believe the IOMMU driver missed the initial
> > - * add_device callback for dev, replay it to get things in order.
> > + * probe for dev, replay it to get things in order.
> > */
> > - if (ops && ops->add_device && dev->bus && !dev->iommu_group)
> > - err = ops->add_device(dev);
> > + if (dev->bus && !dev->iommu_group)
> > + err = iommu_probe_device(dev);
>
> This change removes a check for NULL ops, what causes NULL pointer
> exception on first device without IOMMU.
Bummer, this check was supposed to be in iommu_probe_device(), but
apparently it got lost. Does the attached patch fix it?
> I'm also not sure if this is a good idea to call iommu_probe_device(),
> which comes from dev->bus->iommu_ops, which might be different from ops
> from local variable.
The local variable comes from dev->iommu_fwspec->ops, which should be
exactly the same as dev->bus->iommu_ops. I'll leave that for now until
it turns out to be a problem (which I don't expect).
Regards,
Joerg
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index a2131751dcff..3ed4db334341 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -114,10 +114,14 @@ void iommu_device_unregister(struct iommu_device *iommu)
int iommu_probe_device(struct device *dev)
{
const struct iommu_ops *ops = dev->bus->iommu_ops;
+ int ret = -EINVAL;
WARN_ON(dev->iommu_group);
- return ops->add_device(dev);
+ if (ops)
+ ret = ops->add_device(dev);
+
+ return ret;
}
void iommu_release_device(struct device *dev)
Hi Marek,
On Wed, Dec 19, 2018 at 03:53:51PM +0100, Marek Szyprowski wrote:
> Yes, it fixes this issue.
Thanks a lot, patch is sent out and in iommu tree.
Regards,
Joerg
Hi Jörg,
On Wed, Dec 19, 2018 at 5:51 PM Joerg Roedel <[email protected]> wrote:
> On Wed, Dec 19, 2018 at 10:54:18AM +0100, Marek Szyprowski wrote:
> > On 2018-12-11 16:05, Joerg Roedel wrote:
> > > From: Joerg Roedel <[email protected]>
> > >
> > > Make sure to invoke this call-back through the proper
> > > function of the IOMMU-API.
> > >
> > > Signed-off-by: Joerg Roedel <[email protected]>
> > > ---
> > > drivers/iommu/of_iommu.c | 6 +++---
> > > 1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> > > index c5dd63072529..4d4847de727e 100644
> > > --- a/drivers/iommu/of_iommu.c
> > > +++ b/drivers/iommu/of_iommu.c
> > > @@ -218,10 +218,10 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
> > > ops = dev->iommu_fwspec->ops;
> > > /*
> > > * If we have reason to believe the IOMMU driver missed the initial
> > > - * add_device callback for dev, replay it to get things in order.
> > > + * probe for dev, replay it to get things in order.
> > > */
> > > - if (ops && ops->add_device && dev->bus && !dev->iommu_group)
> > > - err = ops->add_device(dev);
> > > + if (dev->bus && !dev->iommu_group)
> > > + err = iommu_probe_device(dev);
> >
> > This change removes a check for NULL ops, what causes NULL pointer
> > exception on first device without IOMMU.
>
> Bummer, this check was supposed to be in iommu_probe_device(), but
> apparently it got lost. Does the attached patch fix it?
>
> > I'm also not sure if this is a good idea to call iommu_probe_device(),
> > which comes from dev->bus->iommu_ops, which might be different from ops
> > from local variable.
>
> The local variable comes from dev->iommu_fwspec->ops, which should be
> exactly the same as dev->bus->iommu_ops. I'll leave that for now until
> it turns out to be a problem (which I don't expect).
>
>
> Regards,
>
> Joerg
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index a2131751dcff..3ed4db334341 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -114,10 +114,14 @@ void iommu_device_unregister(struct iommu_device *iommu)
> int iommu_probe_device(struct device *dev)
> {
> const struct iommu_ops *ops = dev->bus->iommu_ops;
> + int ret = -EINVAL;
>
> WARN_ON(dev->iommu_group);
>
> - return ops->add_device(dev);
> + if (ops)
Is this sufficient? The old code checked for ops->add_device != NULL,
too.
> + ret = ops->add_device(dev);
> +
> + return ret;
> }
>
> void iommu_release_device(struct device *dev)
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi Geert,
On Thu, Dec 20, 2018 at 04:42:17PM +0100, Geert Uytterhoeven wrote:
> > - return ops->add_device(dev);
> > + if (ops)
>
> Is this sufficient? The old code checked for ops->add_device != NULL,
> too.
Robin brought up that all iommu-ops implementations support the
add_device call-back, so we can remove that check and make the call-back
mandatory for new implementations too.
Regards,
Joerg