This two patches tries to do some cleanup in iommu.
V2:
* add patch 2
Wei Yang (2):
iommu: remove the iommu_callback_data
iommu: remove sysfs_link to device in iommu_group/devices when failed
drivers/iommu/iommu.c | 22 +++++++---------------
1 file changed, 7 insertions(+), 15 deletions(-)
--
1.7.9.5
The original code forgets to remove the sysfs_link to a device in
iommu_group/devices directory, when the creation fails or conflicts on the
name.
This patch tries to remove the sysfs_link on the failure.
Signed-off-by: Wei Yang <[email protected]>
---
drivers/iommu/iommu.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 2696a38..8f480ba 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -403,6 +403,7 @@ rename:
ret = sysfs_create_link_nowarn(group->devices_kobj,
&dev->kobj, device->name);
if (ret) {
+ sysfs_remove_link(group->devices_kobj, device->name);
kfree(device->name);
if (ret == -EEXIST && i >= 0) {
/*
--
1.7.9.5
According to the code path, iommu_callback_data is passed in
iommu_bus_init() and just used in {add/remove}_iommu_group, by when the
bus->iommu_ops is already set up properly.
This patch removes the iommu_callback_data by retrieving iommu_ops from
bus->iommu_ops directly.
Signed-off-by: Wei Yang <[email protected]>
---
drivers/iommu/iommu.c | 21 ++++++---------------
1 file changed, 6 insertions(+), 15 deletions(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 0e3b009..2696a38 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -37,10 +37,6 @@ static struct kset *iommu_group_kset;
static struct ida iommu_group_ida;
static struct mutex iommu_group_mutex;
-struct iommu_callback_data {
- const struct iommu_ops *ops;
-};
-
struct iommu_group {
struct kobject kobj;
struct kobject *devices_kobj;
@@ -867,8 +863,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;
+ const struct iommu_ops *ops = dev->bus->iommu_ops;
int ret;
if (!ops->add_device)
@@ -891,8 +886,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;
+ const struct iommu_ops *ops = dev->bus->iommu_ops;
if (ops->remove_device && dev->iommu_group)
ops->remove_device(dev);
@@ -953,13 +947,10 @@ static int iommu_bus_notifier(struct notifier_block *nb,
return 0;
}
-static int iommu_bus_init(struct bus_type *bus, const struct iommu_ops *ops)
+static int iommu_bus_init(struct bus_type *bus)
{
int err;
struct notifier_block *nb;
- struct iommu_callback_data cb = {
- .ops = ops,
- };
nb = kzalloc(sizeof(struct notifier_block), GFP_KERNEL);
if (!nb)
@@ -971,7 +962,7 @@ static int iommu_bus_init(struct bus_type *bus, const struct iommu_ops *ops)
if (err)
goto out_free;
- err = bus_for_each_dev(bus, NULL, &cb, add_iommu_group);
+ err = bus_for_each_dev(bus, NULL, NULL, add_iommu_group);
if (err)
goto out_err;
@@ -980,7 +971,7 @@ static int iommu_bus_init(struct bus_type *bus, const struct iommu_ops *ops)
out_err:
/* Clean up */
- bus_for_each_dev(bus, NULL, &cb, remove_iommu_group);
+ bus_for_each_dev(bus, NULL, NULL, remove_iommu_group);
bus_unregister_notifier(bus, nb);
out_free:
@@ -1012,7 +1003,7 @@ int bus_set_iommu(struct bus_type *bus, const struct iommu_ops *ops)
bus->iommu_ops = ops;
/* Do IOMMU specific setup for this bus-type */
- err = iommu_bus_init(bus, ops);
+ err = iommu_bus_init(bus);
if (err)
bus->iommu_ops = NULL;
--
1.7.9.5
On Wed, 23 Mar 2016 22:25:11 +0000
Wei Yang <[email protected]> wrote:
> The original code forgets to remove the sysfs_link to a device in
> iommu_group/devices directory, when the creation fails or conflicts on the
> name.
>
> This patch tries to remove the sysfs_link on the failure.
>
> Signed-off-by: Wei Yang <[email protected]>
> ---
> drivers/iommu/iommu.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 2696a38..8f480ba 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -403,6 +403,7 @@ rename:
> ret = sysfs_create_link_nowarn(group->devices_kobj,
> &dev->kobj, device->name);
> if (ret) {
> + sysfs_remove_link(group->devices_kobj, device->name);
> kfree(device->name);
> if (ret == -EEXIST && i >= 0) {
> /*
If we failed to create a link, potentially due to a conflicting link
already present, then aren't we arbitrarily removing that conflicting
link with this change? If sysfs_create_link_nowarn() fails then we
haven't created a link of our own to remove. This looks wrong. Thanks,
Alex
On Wed, Mar 23, 2016 at 05:04:15PM -0600, Alex Williamson wrote:
>On Wed, 23 Mar 2016 22:25:11 +0000
>Wei Yang <[email protected]> wrote:
>
>> The original code forgets to remove the sysfs_link to a device in
>> iommu_group/devices directory, when the creation fails or conflicts on the
>> name.
>>
>> This patch tries to remove the sysfs_link on the failure.
>>
>> Signed-off-by: Wei Yang <[email protected]>
>> ---
>> drivers/iommu/iommu.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 2696a38..8f480ba 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -403,6 +403,7 @@ rename:
>> ret = sysfs_create_link_nowarn(group->devices_kobj,
>> &dev->kobj, device->name);
>> if (ret) {
>> + sysfs_remove_link(group->devices_kobj, device->name);
>> kfree(device->name);
>> if (ret == -EEXIST && i >= 0) {
>> /*
>
>If we failed to create a link, potentially due to a conflicting link
>already present, then aren't we arbitrarily removing that conflicting
>link with this change? If sysfs_create_link_nowarn() fails then we
>haven't created a link of our own to remove. This looks wrong. Thanks,
>
Hmm... you are right, that's my bad.
Do you have comments on the first patch?
>Alex
--
Wei Yang
Help you, Help me
Hi, Alex
I think this one is correct. It applies on v4.5 and works fine.
Or I missed something else?
On Wed, Mar 23, 2016 at 10:25:10PM +0000, Wei Yang wrote:
>According to the code path, iommu_callback_data is passed in
>iommu_bus_init() and just used in {add/remove}_iommu_group, by when the
>bus->iommu_ops is already set up properly.
>
>This patch removes the iommu_callback_data by retrieving iommu_ops from
>bus->iommu_ops directly.
>
>Signed-off-by: Wei Yang <[email protected]>
>---
> drivers/iommu/iommu.c | 21 ++++++---------------
> 1 file changed, 6 insertions(+), 15 deletions(-)
>
>diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>index 0e3b009..2696a38 100644
>--- a/drivers/iommu/iommu.c
>+++ b/drivers/iommu/iommu.c
>@@ -37,10 +37,6 @@ static struct kset *iommu_group_kset;
> static struct ida iommu_group_ida;
> static struct mutex iommu_group_mutex;
>
>-struct iommu_callback_data {
>- const struct iommu_ops *ops;
>-};
>-
> struct iommu_group {
> struct kobject kobj;
> struct kobject *devices_kobj;
>@@ -867,8 +863,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;
>+ const struct iommu_ops *ops = dev->bus->iommu_ops;
> int ret;
>
> if (!ops->add_device)
>@@ -891,8 +886,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;
>+ const struct iommu_ops *ops = dev->bus->iommu_ops;
>
> if (ops->remove_device && dev->iommu_group)
> ops->remove_device(dev);
>@@ -953,13 +947,10 @@ static int iommu_bus_notifier(struct notifier_block *nb,
> return 0;
> }
>
>-static int iommu_bus_init(struct bus_type *bus, const struct iommu_ops *ops)
>+static int iommu_bus_init(struct bus_type *bus)
> {
> int err;
> struct notifier_block *nb;
>- struct iommu_callback_data cb = {
>- .ops = ops,
>- };
>
> nb = kzalloc(sizeof(struct notifier_block), GFP_KERNEL);
> if (!nb)
>@@ -971,7 +962,7 @@ static int iommu_bus_init(struct bus_type *bus, const struct iommu_ops *ops)
> if (err)
> goto out_free;
>
>- err = bus_for_each_dev(bus, NULL, &cb, add_iommu_group);
>+ err = bus_for_each_dev(bus, NULL, NULL, add_iommu_group);
> if (err)
> goto out_err;
>
>@@ -980,7 +971,7 @@ static int iommu_bus_init(struct bus_type *bus, const struct iommu_ops *ops)
>
> out_err:
> /* Clean up */
>- bus_for_each_dev(bus, NULL, &cb, remove_iommu_group);
>+ bus_for_each_dev(bus, NULL, NULL, remove_iommu_group);
> bus_unregister_notifier(bus, nb);
>
> out_free:
>@@ -1012,7 +1003,7 @@ int bus_set_iommu(struct bus_type *bus, const struct iommu_ops *ops)
> bus->iommu_ops = ops;
>
> /* Do IOMMU specific setup for this bus-type */
>- err = iommu_bus_init(bus, ops);
>+ err = iommu_bus_init(bus);
> if (err)
> bus->iommu_ops = NULL;
>
>--
>1.7.9.5
--
Wei Yang
Help you, Help me