This patch simply adds support for PCI devices. Also reverses
bus_set_iommu in tegra_smmu_remove().
Signed-off-by: Nicolin Chen <[email protected]>
---
Changelog
v1->v2
* Added error-out labels in tegra_smmu_probe()
* Dropped pci_request_acs() since IOMMU core would call it.
drivers/iommu/tegra-smmu.c | 33 ++++++++++++++++++++++++---------
1 file changed, 24 insertions(+), 9 deletions(-)
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index cf4981369828..74d84908679e 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -10,6 +10,7 @@
#include <linux/kernel.h>
#include <linux/of.h>
#include <linux/of_device.h>
+#include <linux/pci.h>
#include <linux/platform_device.h>
#include <linux/slab.h>
#include <linux/spinlock.h>
@@ -856,6 +857,7 @@ static struct iommu_group *tegra_smmu_device_group(struct device *dev)
const struct tegra_smmu_group_soc *soc;
unsigned int swgroup = fwspec->ids[0];
struct tegra_smmu_group *group;
+ bool pci = dev_is_pci(dev);
struct iommu_group *grp;
/* Find group_soc associating with swgroup */
@@ -882,7 +884,7 @@ static struct iommu_group *tegra_smmu_device_group(struct device *dev)
group->smmu = smmu;
group->soc = soc;
- group->group = iommu_group_alloc();
+ group->group = pci ? pci_device_group(dev) : iommu_group_alloc();
if (IS_ERR(group->group)) {
devm_kfree(smmu->dev, group);
mutex_unlock(&smmu->lock);
@@ -1079,26 +1081,39 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
iommu_device_set_fwnode(&smmu->iommu, dev->fwnode);
err = iommu_device_register(&smmu->iommu);
- if (err) {
- iommu_device_sysfs_remove(&smmu->iommu);
- return ERR_PTR(err);
- }
+ if (err)
+ goto err_sysfs;
err = bus_set_iommu(&platform_bus_type, &tegra_smmu_ops);
- if (err < 0) {
- iommu_device_unregister(&smmu->iommu);
- iommu_device_sysfs_remove(&smmu->iommu);
- return ERR_PTR(err);
+ if (err < 0)
+ goto err_unregister;
+
+#ifdef CONFIG_PCI
+ if (!iommu_present(&pci_bus_type)) {
+ err = bus_set_iommu(&pci_bus_type, &tegra_smmu_ops);
+ if (err < 0)
+ goto err_bus_set;
}
+#endif
if (IS_ENABLED(CONFIG_DEBUG_FS))
tegra_smmu_debugfs_init(smmu);
return smmu;
+
+err_bus_set:
+ bus_set_iommu(&platform_bus_type, NULL);
+err_unregister:
+ iommu_device_unregister(&smmu->iommu);
+err_sysfs:
+ iommu_device_sysfs_remove(&smmu->iommu);
+
+ return ERR_PTR(err);
}
void tegra_smmu_remove(struct tegra_smmu *smmu)
{
+ bus_set_iommu(&platform_bus_type, NULL);
iommu_device_unregister(&smmu->iommu);
iommu_device_sysfs_remove(&smmu->iommu);
--
2.17.1
30.09.2020 03:30, Nicolin Chen пишет:
...
> +#ifdef CONFIG_PCI
> + if (!iommu_present(&pci_bus_type)) {
Could you please explain why this check is needed?
30.09.2020 03:30, Nicolin Chen пишет:
> - group->group = iommu_group_alloc();
> + group->group = pci ? pci_device_group(dev) : iommu_group_alloc();
This will be nicer to write as:
if (dev_is_pci(dev))
group->group = pci_device_group(dev);
else
group->group = generic_device_group(dev);
30.09.2020 03:30, Nicolin Chen пишет:
> void tegra_smmu_remove(struct tegra_smmu *smmu)
> {
> + bus_set_iommu(&platform_bus_type, NULL);
Why only platform_bus? Is this really needed at all?
> iommu_device_unregister(&smmu->iommu);
> iommu_device_sysfs_remove(&smmu->iommu);
On Wed, Sep 30, 2020 at 08:10:00AM +0300, Dmitry Osipenko wrote:
> 30.09.2020 03:30, Nicolin Chen пишет:
> ...
> > +#ifdef CONFIG_PCI
> > + if (!iommu_present(&pci_bus_type)) {
>
> Could you please explain why this check is needed?
That's referencing what's in the arm-smmu.c file, since it does
not hurt to have a check.
Hi Dmitry,
On Wed, Sep 30, 2020 at 08:10:07AM +0300, Dmitry Osipenko wrote:
> 30.09.2020 03:30, Nicolin Chen пишет:
> > - group->group = iommu_group_alloc();
> > + group->group = pci ? pci_device_group(dev) : iommu_group_alloc();
>
> This will be nicer to write as:
>
> if (dev_is_pci(dev))
> group->group = pci_device_group(dev);
> else
> group->group = generic_device_group(dev);
Why is that nicer? I don't feel mine is hard to read at all...
On Wed, Sep 30, 2020 at 08:10:35AM +0300, Dmitry Osipenko wrote:
> 30.09.2020 03:30, Nicolin Chen пишет:
> > void tegra_smmu_remove(struct tegra_smmu *smmu)
> > {
> > + bus_set_iommu(&platform_bus_type, NULL);
>
> Why only platform_bus? Is this really needed at all?
I see qcom_iommu.c file set to NULL in remove(), Probably should
have added pci_bus_type too though.
Or are you sure that there's no need at all?
30.09.2020 08:29, Nicolin Chen пишет:
> Hi Dmitry,
>
> On Wed, Sep 30, 2020 at 08:10:07AM +0300, Dmitry Osipenko wrote:
>> 30.09.2020 03:30, Nicolin Chen пишет:
>>> - group->group = iommu_group_alloc();
>>> + group->group = pci ? pci_device_group(dev) : iommu_group_alloc();
>>
>> This will be nicer to write as:
>>
>> if (dev_is_pci(dev))
>> group->group = pci_device_group(dev);
>> else
>> group->group = generic_device_group(dev);
>
> Why is that nicer? I don't feel mine is hard to read at all...
>
Previously you said that you're going to add USB support, so I assume
there will be something about USB.
It's also a kinda common style that majority of Tegra drivers use in
upstream kernel. But yours variant is good too if there won't be a need
to change it later on.
30.09.2020 08:34, Nicolin Chen пишет:
> On Wed, Sep 30, 2020 at 08:10:35AM +0300, Dmitry Osipenko wrote:
>> 30.09.2020 03:30, Nicolin Chen пишет:
>>> void tegra_smmu_remove(struct tegra_smmu *smmu)
>>> {
>>> + bus_set_iommu(&platform_bus_type, NULL);
>>
>> Why only platform_bus? Is this really needed at all?
>
> I see qcom_iommu.c file set to NULL in remove(), Probably should
> have added pci_bus_type too though.
>
> Or are you sure that there's no need at all?
>
It wasn't here before this patch and platform_bus is unrelated to the
topic of this patch. But it probably should be there.
On the other hand, the tegra_smmu_remove() is unused and maybe it could
be better to get rid of this unused function at all.
On Wed, Sep 30, 2020 at 09:01:09AM +0300, Dmitry Osipenko wrote:
> 30.09.2020 08:34, Nicolin Chen пишет:
> > On Wed, Sep 30, 2020 at 08:10:35AM +0300, Dmitry Osipenko wrote:
> >> 30.09.2020 03:30, Nicolin Chen пишет:
> >>> void tegra_smmu_remove(struct tegra_smmu *smmu)
> >>> {
> >>> + bus_set_iommu(&platform_bus_type, NULL);
> >>
> >> Why only platform_bus? Is this really needed at all?
> >
> > I see qcom_iommu.c file set to NULL in remove(), Probably should
> > have added pci_bus_type too though.
> >
> > Or are you sure that there's no need at all?
> >
>
> It wasn't here before this patch and platform_bus is unrelated to the
> topic of this patch. But it probably should be there.
>
> On the other hand, the tegra_smmu_remove() is unused and maybe it could
> be better to get rid of this unused function at all.
I will drop this line then, since no one is calling it. And maybe
we can submit a clean up patch later.
On Wed, Sep 30, 2020 at 08:58:50AM +0300, Dmitry Osipenko wrote:
> 30.09.2020 08:29, Nicolin Chen пишет:
> > Hi Dmitry,
> >
> > On Wed, Sep 30, 2020 at 08:10:07AM +0300, Dmitry Osipenko wrote:
> >> 30.09.2020 03:30, Nicolin Chen пишет:
> >>> - group->group = iommu_group_alloc();
> >>> + group->group = pci ? pci_device_group(dev) : iommu_group_alloc();
> >>
> >> This will be nicer to write as:
> >>
> >> if (dev_is_pci(dev))
> >> group->group = pci_device_group(dev);
> >> else
> >> group->group = generic_device_group(dev);
> >
> > Why is that nicer? I don't feel mine is hard to read at all...
> >
>
> Previously you said that you're going to add USB support, so I assume
> there will be something about USB.
I was hoping that it'd work for USB. Yet USB doesn't seem to have
an different function for device_group().
> It's also a kinda common style that majority of Tegra drivers use in
> upstream kernel. But yours variant is good too if there won't be a need
> to change it later on.
Okay..I'll use yours then.
Thanks