The bus_set_iommu() in tegra_smmu_probe() enumerates all clients
to call in tegra_smmu_probe_device() where each client searches
its DT node for smmu pointer and swgroup ID, so as to configure
an fwspec. But this requires a valid smmu pointer even before mc
and smmu drivers are probed. So in tegra_smmu_probe() we added a
line of code to fill mc->smmu, marking "a bit of a hack".
This works for most of clients in the DTB, however, doesn't work
for a client that doesn't exist in DTB, a PCI device for example.
Actually, if we return ERR_PTR(-ENODEV) in ->probe_device() when
it's called from bus_set_iommu(), iommu core will let everything
carry on. Then when a client gets probed, of_iommu_configure() in
iommu core will search DTB for swgroup ID and call ->of_xlate()
to prepare an fwspec, similar to tegra_smmu_probe_device() and
tegra_smmu_configure(). Then it'll call tegra_smmu_probe_device()
again, and this time we shall return smmu->iommu pointer properly.
So we can get rid of tegra_smmu_find() and tegra_smmu_configure()
along with DT polling code by letting the iommu core handle every
thing, except a problem that we search iommus property in DTB not
only for swgroup ID but also for mc node to get mc->smmu pointer
to call dev_iommu_priv_set() and return the smmu->iommu pointer.
So we'll need to find another way to get smmu pointer.
Referencing the implementation of sun50i-iommu driver, of_xlate()
has client's dev pointer, mc node and swgroup ID. This means that
we can call dev_iommu_priv_set() in of_xlate() instead, so we can
simply get smmu pointer in ->probe_device().
This patch reworks tegra_smmu_probe_device() by:
1) Removing mc->smmu hack in tegra_smmu_probe() so as to return
ERR_PTR(-ENODEV) in tegra_smmu_probe_device() during stage of
tegra_smmu_probe/tegra_mc_probe().
2) Moving dev_iommu_priv_set() to of_xlate() so we can get smmu
pointer in tegra_smmu_probe_device() to replace DTB polling.
3) Removing tegra_smmu_configure() accordingly since iommu core
takes care of it.
Signed-off-by: Nicolin Chen <[email protected]>
---
Changelog
v3->v4
* Moved dev_iommu_priv_set() to of_xlate() so we don't need
to poll DTB for smmu pointer.
* Removed the hack in tegra_smmu_probe() by returning ERR_PTR(
-ENODEV) in tegra_smmu_probe_device() to let iommu core call
in again.
* Removed tegra_smmu_find() and tegra_smmu_configure() as iommu
core takes care of fwspec.
v2->v3
* Used devm_tegra_get_memory_controller() to get mc pointer
* Replaced IS_ERR_OR_NULL with IS_ERR in .probe_device()
v1->v2
* Replaced in .probe_device() tegra_smmu_find/tegra_smmu_configure()
with tegra_get_memory_controller call.
* Dropped the hack in tegra_smmu_probe().
drivers/iommu/tegra-smmu.c | 90 ++++----------------------------------
1 file changed, 9 insertions(+), 81 deletions(-)
diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index a573a5151c69..02d02b0c55c4 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -797,75 +797,9 @@ static phys_addr_t tegra_smmu_iova_to_phys(struct iommu_domain *domain,
return SMMU_PFN_PHYS(pfn) + SMMU_OFFSET_IN_PAGE(iova);
}
-static struct tegra_smmu *tegra_smmu_find(struct device_node *np)
-{
- struct platform_device *pdev;
- struct tegra_mc *mc;
-
- pdev = of_find_device_by_node(np);
- if (!pdev)
- return NULL;
-
- mc = platform_get_drvdata(pdev);
- if (!mc)
- return NULL;
-
- return mc->smmu;
-}
-
-static int tegra_smmu_configure(struct tegra_smmu *smmu, struct device *dev,
- struct of_phandle_args *args)
-{
- const struct iommu_ops *ops = smmu->iommu.ops;
- int err;
-
- err = iommu_fwspec_init(dev, &dev->of_node->fwnode, ops);
- if (err < 0) {
- dev_err(dev, "failed to initialize fwspec: %d\n", err);
- return err;
- }
-
- err = ops->of_xlate(dev, args);
- if (err < 0) {
- dev_err(dev, "failed to parse SW group ID: %d\n", err);
- iommu_fwspec_free(dev);
- return err;
- }
-
- return 0;
-}
-
static struct iommu_device *tegra_smmu_probe_device(struct device *dev)
{
- struct device_node *np = dev->of_node;
- struct tegra_smmu *smmu = NULL;
- struct of_phandle_args args;
- unsigned int index = 0;
- int err;
-
- while (of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
- &args) == 0) {
- smmu = tegra_smmu_find(args.np);
- if (smmu) {
- err = tegra_smmu_configure(smmu, dev, &args);
- of_node_put(args.np);
-
- if (err < 0)
- return ERR_PTR(err);
-
- /*
- * Only a single IOMMU master interface is currently
- * supported by the Linux kernel, so abort after the
- * first match.
- */
- dev_iommu_priv_set(dev, smmu);
-
- break;
- }
-
- of_node_put(args.np);
- index++;
- }
+ struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
if (!smmu)
return ERR_PTR(-ENODEV);
@@ -873,10 +807,7 @@ static struct iommu_device *tegra_smmu_probe_device(struct device *dev)
return &smmu->iommu;
}
-static void tegra_smmu_release_device(struct device *dev)
-{
- dev_iommu_priv_set(dev, NULL);
-}
+static void tegra_smmu_release_device(struct device *dev) {}
static const struct tegra_smmu_group_soc *
tegra_smmu_find_group(struct tegra_smmu *smmu, unsigned int swgroup)
@@ -953,8 +884,17 @@ static struct iommu_group *tegra_smmu_device_group(struct device *dev)
static int tegra_smmu_of_xlate(struct device *dev,
struct of_phandle_args *args)
{
+ struct platform_device *iommu_pdev = of_find_device_by_node(args->np);
+ struct tegra_mc *mc = platform_get_drvdata(iommu_pdev);
u32 id = args->args[0];
+ of_node_put(args->np);
+
+ if (!mc || !mc->smmu)
+ return -EPROBE_DEFER;
+
+ dev_iommu_priv_set(dev, mc->smmu);
+
return iommu_fwspec_add_ids(dev, &id, 1);
}
@@ -1079,16 +1017,6 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
if (!smmu)
return ERR_PTR(-ENOMEM);
- /*
- * This is a bit of a hack. Ideally we'd want to simply return this
- * value. However the IOMMU registration process will attempt to add
- * all devices to the IOMMU when bus_set_iommu() is called. In order
- * not to rely on global variables to track the IOMMU instance, we
- * set it here so that it can be looked up from the .probe_device()
- * callback via the IOMMU device's .drvdata field.
- */
- mc->smmu = smmu;
-
size = BITS_TO_LONGS(soc->num_asids) * sizeof(long);
smmu->asids = devm_kzalloc(dev, size, GFP_KERNEL);
--
2.17.1
02.10.2020 09:08, Nicolin Chen пишет:
> -static void tegra_smmu_release_device(struct device *dev)
> -{
> - dev_iommu_priv_set(dev, NULL);
> -}
> +static void tegra_smmu_release_device(struct device *dev) {}
Please keep the braces as-is.
02.10.2020 09:08, Nicolin Chen пишет:
> static struct iommu_device *tegra_smmu_probe_device(struct device *dev)
> {
> - struct device_node *np = dev->of_node;
> - struct tegra_smmu *smmu = NULL;
> - struct of_phandle_args args;
> - unsigned int index = 0;
> - int err;
> -
> - while (of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
> - &args) == 0) {
> - smmu = tegra_smmu_find(args.np);
> - if (smmu) {
> - err = tegra_smmu_configure(smmu, dev, &args);
> - of_node_put(args.np);
> -
> - if (err < 0)
> - return ERR_PTR(err);
> -
> - /*
> - * Only a single IOMMU master interface is currently
> - * supported by the Linux kernel, so abort after the
> - * first match.
> - */
> - dev_iommu_priv_set(dev, smmu);
> -
> - break;
> - }
> -
> - of_node_put(args.np);
> - index++;
> - }
> + struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
>
> if (!smmu)
> return ERR_PTR(-ENODEV);
The !smmu can't ever be true now, isn't it? Then please remove it.
02.10.2020 09:08, Nicolin Chen пишет:
> static int tegra_smmu_of_xlate(struct device *dev,
> struct of_phandle_args *args)
> {
> + struct platform_device *iommu_pdev = of_find_device_by_node(args->np);
> + struct tegra_mc *mc = platform_get_drvdata(iommu_pdev);
> u32 id = args->args[0];
>
> + of_node_put(args->np);
> +
> + if (!mc || !mc->smmu)
> + return -EPROBE_DEFER;
platform_get_drvdata(NULL) will crash.
> + dev_iommu_priv_set(dev, mc->smmu);
I think put_device(mc->dev) is missed here, doesn't it?
Why sun50i-iommu driver doesn't have this error-checking? Is it really
needed at all?
02.10.2020 17:22, Dmitry Osipenko пишет:
>> static int tegra_smmu_of_xlate(struct device *dev,
>> struct of_phandle_args *args)
>> {
>> + struct platform_device *iommu_pdev = of_find_device_by_node(args->np);
>> + struct tegra_mc *mc = platform_get_drvdata(iommu_pdev);
>> u32 id = args->args[0];
>>
>> + of_node_put(args->np);
>> +
>> + if (!mc || !mc->smmu)
>> + return -EPROBE_DEFER;
> platform_get_drvdata(NULL) will crash.
>
Actually, platform_get_drvdata(NULL) can't happen. I overlooked this.
02.10.2020 17:22, Dmitry Osipenko пишет:
> 02.10.2020 09:08, Nicolin Chen пишет:
>> -static void tegra_smmu_release_device(struct device *dev)
>> -{
>> - dev_iommu_priv_set(dev, NULL);
>> -}
>> +static void tegra_smmu_release_device(struct device *dev) {}
>
> Please keep the braces as-is.
>
I noticed that you borrowed this style from the sun50i-iommu driver, but
this is a bit unusual coding style for the c files. At least to me it's
unusual to see header-style function stub in a middle of c file. But
maybe it's just me.
02.10.2020 09:08, Nicolin Chen пишет:
> static int tegra_smmu_of_xlate(struct device *dev,
> struct of_phandle_args *args)
> {
> + struct platform_device *iommu_pdev = of_find_device_by_node(args->np);
> + struct tegra_mc *mc = platform_get_drvdata(iommu_pdev);
> u32 id = args->args[0];
>
> + of_node_put(args->np);
of_find_device_by_node() takes device reference and not the np
reference. This is a bug, please remove of_node_put().
02.10.2020 09:08, Nicolin Chen пишет:
> Then when a client gets probed, of_iommu_configure() in
> iommu core will search DTB for swgroup ID and call ->of_xlate()
> to prepare an fwspec, similar to tegra_smmu_probe_device() and
> tegra_smmu_configure(). Then it'll call tegra_smmu_probe_device()
> again, and this time we shall return smmu->iommu pointer properly.
I don't quite see where IOMMU core calls of_xlate().
Have tried to at least boot-test this patch?
02.10.2020 18:23, Dmitry Osipenko пишет:
> 02.10.2020 09:08, Nicolin Chen пишет:
>> Then when a client gets probed, of_iommu_configure() in
>> iommu core will search DTB for swgroup ID and call ->of_xlate()
>> to prepare an fwspec, similar to tegra_smmu_probe_device() and
>> tegra_smmu_configure(). Then it'll call tegra_smmu_probe_device()
>> again, and this time we shall return smmu->iommu pointer properly.
>
> I don't quite see where IOMMU core calls of_xlate().
>
> Have tried to at least boot-test this patch?
>
I don't see how it ever could work because of_xlate() is only invoked from:
fsl_mc_dma_configure()->of_dma_configure_id()->of_iommu_configure()
Looks like the tegra_smmu_configure() is still needed.
I don't know how sun50i driver could work to be honest. Seems IOMMU is
broken on sun50i, but maybe I'm missing something.
I added Maxime Ripard to this thread, who is the author of the
sun50i-iommu driver.
02.10.2020 19:00, Dmitry Osipenko пишет:
> 02.10.2020 18:23, Dmitry Osipenko пишет:
>> 02.10.2020 09:08, Nicolin Chen пишет:
>>> Then when a client gets probed, of_iommu_configure() in
>>> iommu core will search DTB for swgroup ID and call ->of_xlate()
>>> to prepare an fwspec, similar to tegra_smmu_probe_device() and
>>> tegra_smmu_configure(). Then it'll call tegra_smmu_probe_device()
>>> again, and this time we shall return smmu->iommu pointer properly.
>>
>> I don't quite see where IOMMU core calls of_xlate().
>>
>> Have tried to at least boot-test this patch?
>>
>
> I don't see how it ever could work because of_xlate() is only invoked from:
>
> fsl_mc_dma_configure()->of_dma_configure_id()->of_iommu_configure()
>
> Looks like the tegra_smmu_configure() is still needed.
>
> I don't know how sun50i driver could work to be honest. Seems IOMMU is
> broken on sun50i, but maybe I'm missing something.
>
> I added Maxime Ripard to this thread, who is the author of the
> sun50i-iommu driver.
>
Actually, I now see that the other IOMMU drivers (qcom, exynos, etc) do
the same. So obviously I'm missing something and it should work..
02.10.2020 19:37, Dmitry Osipenko пишет:
> 02.10.2020 19:00, Dmitry Osipenko пишет:
>> 02.10.2020 18:23, Dmitry Osipenko пишет:
>>> 02.10.2020 09:08, Nicolin Chen пишет:
>>>> Then when a client gets probed, of_iommu_configure() in
>>>> iommu core will search DTB for swgroup ID and call ->of_xlate()
>>>> to prepare an fwspec, similar to tegra_smmu_probe_device() and
>>>> tegra_smmu_configure(). Then it'll call tegra_smmu_probe_device()
>>>> again, and this time we shall return smmu->iommu pointer properly.
>>>
>>> I don't quite see where IOMMU core calls of_xlate().
>>>
>>> Have tried to at least boot-test this patch?
>>>
>>
>> I don't see how it ever could work because of_xlate() is only invoked from:
>>
>> fsl_mc_dma_configure()->of_dma_configure_id()->of_iommu_configure()
>>
>> Looks like the tegra_smmu_configure() is still needed.
>>
>> I don't know how sun50i driver could work to be honest. Seems IOMMU is
>> broken on sun50i, but maybe I'm missing something.
>>
>> I added Maxime Ripard to this thread, who is the author of the
>> sun50i-iommu driver.
>>
>
> Actually, I now see that the other IOMMU drivers (qcom, exynos, etc) do
> the same. So obviously I'm missing something and it should work..
>
Okay, somehow I was oblivious to that of_dma_configure() invokes
of_dma_configure_id(). Should be good :)
On Fri, Oct 02, 2020 at 05:23:14PM +0300, Dmitry Osipenko wrote:
> 02.10.2020 09:08, Nicolin Chen пишет:
> > static struct iommu_device *tegra_smmu_probe_device(struct device *dev)
> > {
> > - struct device_node *np = dev->of_node;
> > - struct tegra_smmu *smmu = NULL;
> > - struct of_phandle_args args;
> > - unsigned int index = 0;
> > - int err;
> > -
> > - while (of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
> > - &args) == 0) {
> > - smmu = tegra_smmu_find(args.np);
> > - if (smmu) {
> > - err = tegra_smmu_configure(smmu, dev, &args);
> > - of_node_put(args.np);
> > -
> > - if (err < 0)
> > - return ERR_PTR(err);
> > -
> > - /*
> > - * Only a single IOMMU master interface is currently
> > - * supported by the Linux kernel, so abort after the
> > - * first match.
> > - */
> > - dev_iommu_priv_set(dev, smmu);
> > -
> > - break;
> > - }
> > -
> > - of_node_put(args.np);
> > - index++;
> > - }
> > + struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
> >
> > if (!smmu)
> > return ERR_PTR(-ENODEV);
>
> The !smmu can't ever be true now, isn't it? Then please remove it.
How can you be so sure? Have you read my commit message? The whole
point of removing the hack in tegra_smmu_probe() is to return the
ERR_PTR(-ENODEV) here. The bus_set_iommu() will call this function
when mc->smmu is not assigned it, as it's assigned after we return
tegra_smmu_probe() while bus_set_iommu() is still in the middle of
the tegra_smmu_probe().
02.10.2020 21:01, Nicolin Chen пишет:
> On Fri, Oct 02, 2020 at 05:23:14PM +0300, Dmitry Osipenko wrote:
>> 02.10.2020 09:08, Nicolin Chen пишет:
>>> static struct iommu_device *tegra_smmu_probe_device(struct device *dev)
>>> {
>>> - struct device_node *np = dev->of_node;
>>> - struct tegra_smmu *smmu = NULL;
>>> - struct of_phandle_args args;
>>> - unsigned int index = 0;
>>> - int err;
>>> -
>>> - while (of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
>>> - &args) == 0) {
>>> - smmu = tegra_smmu_find(args.np);
>>> - if (smmu) {
>>> - err = tegra_smmu_configure(smmu, dev, &args);
>>> - of_node_put(args.np);
>>> -
>>> - if (err < 0)
>>> - return ERR_PTR(err);
>>> -
>>> - /*
>>> - * Only a single IOMMU master interface is currently
>>> - * supported by the Linux kernel, so abort after the
>>> - * first match.
>>> - */
>>> - dev_iommu_priv_set(dev, smmu);
>>> -
>>> - break;
>>> - }
>>> -
>>> - of_node_put(args.np);
>>> - index++;
>>> - }
>>> + struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
>>>
>>> if (!smmu)
>>> return ERR_PTR(-ENODEV);
>>
>> The !smmu can't ever be true now, isn't it? Then please remove it.
>
> How can you be so sure? Have you read my commit message? The whole
> point of removing the hack in tegra_smmu_probe() is to return the
> ERR_PTR(-ENODEV) here. The bus_set_iommu() will call this function
> when mc->smmu is not assigned it, as it's assigned after we return
> tegra_smmu_probe() while bus_set_iommu() is still in the middle of
> the tegra_smmu_probe().
>
My bad, I probably missed that was looking at the probe_device(), looks
good then.
On Fri, Oct 02, 2020 at 06:02:18PM +0300, Dmitry Osipenko wrote:
> 02.10.2020 09:08, Nicolin Chen пишет:
> > static int tegra_smmu_of_xlate(struct device *dev,
> > struct of_phandle_args *args)
> > {
> > + struct platform_device *iommu_pdev = of_find_device_by_node(args->np);
> > + struct tegra_mc *mc = platform_get_drvdata(iommu_pdev);
> > u32 id = args->args[0];
> >
> > + of_node_put(args->np);
>
> of_find_device_by_node() takes device reference and not the np
> reference. This is a bug, please remove of_node_put().
Looks like so. Replacing it with put_device(&iommu_pdev->dev);
Thanks
On Fri, Oct 02, 2020 at 05:58:29PM +0300, Dmitry Osipenko wrote:
> 02.10.2020 17:22, Dmitry Osipenko пишет:
> > 02.10.2020 09:08, Nicolin Chen пишет:
> >> -static void tegra_smmu_release_device(struct device *dev)
> >> -{
> >> - dev_iommu_priv_set(dev, NULL);
> >> -}
> >> +static void tegra_smmu_release_device(struct device *dev) {}
> >
> > Please keep the braces as-is.
> >
>
> I noticed that you borrowed this style from the sun50i-iommu driver, but
> this is a bit unusual coding style for the c files. At least to me it's
> unusual to see header-style function stub in a middle of c file. But
> maybe it's just me.
I don't see a rule in ./Documentation/process/coding-style.rst
against this, and there're plenty of drivers doing so. If you
feel uncomfortable with this style, you may add a rule to that
doc so everyone will follow :)
On Fri, Oct 02, 2020 at 12:53:28PM -0700, Nicolin Chen wrote:
> On Fri, Oct 02, 2020 at 05:58:29PM +0300, Dmitry Osipenko wrote:
> > 02.10.2020 17:22, Dmitry Osipenko пишет:
> > > 02.10.2020 09:08, Nicolin Chen пишет:
> > >> -static void tegra_smmu_release_device(struct device *dev)
> > >> -{
> > >> - dev_iommu_priv_set(dev, NULL);
> > >> -}
> > >> +static void tegra_smmu_release_device(struct device *dev) {}
> > >
> > > Please keep the braces as-is.
> > >
> >
> > I noticed that you borrowed this style from the sun50i-iommu driver, but
> > this is a bit unusual coding style for the c files. At least to me it's
> > unusual to see header-style function stub in a middle of c file. But
> > maybe it's just me.
>
> I don't see a rule in ./Documentation/process/coding-style.rst
> against this, and there're plenty of drivers doing so. If you
> feel uncomfortable with this style, you may add a rule to that
> doc so everyone will follow :)
I also prefer braces on separate lines. Even better would be to just
drop this entirely and make ->release_device() optional. At least the
following drivers could be cleaned up that way:
* fsl-pamu
* msm-iommu
* sun50i-iommu
* tegra-gart
* tegra-smmu
And it looks like mtk-iommu and mtk-iommu-v1 do only iommu_fwspec_free()
in their ->release_device() implementations, but that's already done via
iommu_release_device().
Thierry
On Fri, Oct 02, 2020 at 05:22:41PM +0300, Dmitry Osipenko wrote:
> 02.10.2020 09:08, Nicolin Chen пишет:
> > static int tegra_smmu_of_xlate(struct device *dev,
> > struct of_phandle_args *args)
> > {
> > + struct platform_device *iommu_pdev = of_find_device_by_node(args->np);
> > + struct tegra_mc *mc = platform_get_drvdata(iommu_pdev);
> > u32 id = args->args[0];
> >
> > + of_node_put(args->np);
> > +
> > + if (!mc || !mc->smmu)
> > + return -EPROBE_DEFER;
>
> platform_get_drvdata(NULL) will crash.
>
> > + dev_iommu_priv_set(dev, mc->smmu);
>
> I think put_device(mc->dev) is missed here, doesn't it?
Yeah, I think we'd need that here, otherwise we'd be leaking a
reference. Worse, even, mc->dev is the same device that owns the SMMU,
so we're basically incrementing our own reference here and never
releasing it. We also need that put_device(mc->dev) in the error case
above because we already hold the reference there.
Thierry
On Fri, Oct 02, 2020 at 05:50:08PM +0300, Dmitry Osipenko wrote:
> 02.10.2020 17:22, Dmitry Osipenko пишет:
> >> static int tegra_smmu_of_xlate(struct device *dev,
> >> struct of_phandle_args *args)
> >> {
> >> + struct platform_device *iommu_pdev = of_find_device_by_node(args->np);
> >> + struct tegra_mc *mc = platform_get_drvdata(iommu_pdev);
> >> u32 id = args->args[0];
> >>
> >> + of_node_put(args->np);
> >> +
> >> + if (!mc || !mc->smmu)
> >> + return -EPROBE_DEFER;
> > platform_get_drvdata(NULL) will crash.
> >
>
> Actually, platform_get_drvdata(NULL) can't happen. I overlooked this.
How so? It's technically possible for the iommus property to reference a
device tree node for which no platform device will ever be created, in
which case of_find_device_by_node() will return NULL. That's very
unlikely and perhaps worth just crashing on to make sure it gets fixed
immediately.
Thierry
On Fri, Oct 02, 2020 at 11:58:29AM -0700, Nicolin Chen wrote:
> On Fri, Oct 02, 2020 at 06:02:18PM +0300, Dmitry Osipenko wrote:
> > 02.10.2020 09:08, Nicolin Chen пишет:
> > > static int tegra_smmu_of_xlate(struct device *dev,
> > > struct of_phandle_args *args)
> > > {
> > > + struct platform_device *iommu_pdev = of_find_device_by_node(args->np);
> > > + struct tegra_mc *mc = platform_get_drvdata(iommu_pdev);
> > > u32 id = args->args[0];
> > >
> > > + of_node_put(args->np);
> >
> > of_find_device_by_node() takes device reference and not the np
> > reference. This is a bug, please remove of_node_put().
>
> Looks like so. Replacing it with put_device(&iommu_pdev->dev);
Putting the put_device() here is wrong, though. You need to make sure
you keep a reference to it as long as you keep accessing the data that
is owned by it.
Like I said earlier, this is a bit weird in this case because we're
self-referencing, so iommu_pdev->dev is going to stay around as long as
the SMMU is. However, it might be worth to properly track the lifetime
anyway just so that the code can serve as a good example of how to do
things.
If you decide to go for the shortcut and not track this reference
properly, then at least you need to add a comment as to why it is safe
to do in this case. This ensures that readers are away of the
circumstances and don't copy this bad code into a context where the
circumstances are different.
Thierry
05.10.2020 12:53, Thierry Reding пишет:
> On Fri, Oct 02, 2020 at 05:50:08PM +0300, Dmitry Osipenko wrote:
>> 02.10.2020 17:22, Dmitry Osipenko пишет:
>>>> static int tegra_smmu_of_xlate(struct device *dev,
>>>> struct of_phandle_args *args)
>>>> {
>>>> + struct platform_device *iommu_pdev = of_find_device_by_node(args->np);
>>>> + struct tegra_mc *mc = platform_get_drvdata(iommu_pdev);
>>>> u32 id = args->args[0];
>>>>
>>>> + of_node_put(args->np);
>>>> +
>>>> + if (!mc || !mc->smmu)
>>>> + return -EPROBE_DEFER;
>>> platform_get_drvdata(NULL) will crash.
>>>
>>
>> Actually, platform_get_drvdata(NULL) can't happen. I overlooked this.
>
> How so? It's technically possible for the iommus property to reference a
> device tree node for which no platform device will ever be created, in
> which case of_find_device_by_node() will return NULL. That's very
> unlikely and perhaps worth just crashing on to make sure it gets fixed
> immediately.
The tegra_smmu_ops are registered from the SMMU driver itself and MC
driver sets platform data before SMMU is initialized, hence device is
guaranteed to exist and mc can't be NULL.
On Mon, Oct 05, 2020 at 01:36:55PM +0300, Dmitry Osipenko wrote:
> 05.10.2020 12:53, Thierry Reding пишет:
> > On Fri, Oct 02, 2020 at 05:50:08PM +0300, Dmitry Osipenko wrote:
> >> 02.10.2020 17:22, Dmitry Osipenko пишет:
> >>>> static int tegra_smmu_of_xlate(struct device *dev,
> >>>> struct of_phandle_args *args)
> >>>> {
> >>>> + struct platform_device *iommu_pdev = of_find_device_by_node(args->np);
> >>>> + struct tegra_mc *mc = platform_get_drvdata(iommu_pdev);
> >>>> u32 id = args->args[0];
> >>>>
> >>>> + of_node_put(args->np);
> >>>> +
> >>>> + if (!mc || !mc->smmu)
> >>>> + return -EPROBE_DEFER;
> >>> platform_get_drvdata(NULL) will crash.
> >>>
> >>
> >> Actually, platform_get_drvdata(NULL) can't happen. I overlooked this.
> >
> > How so? It's technically possible for the iommus property to reference a
> > device tree node for which no platform device will ever be created, in
> > which case of_find_device_by_node() will return NULL. That's very
> > unlikely and perhaps worth just crashing on to make sure it gets fixed
> > immediately.
>
> The tegra_smmu_ops are registered from the SMMU driver itself and MC
> driver sets platform data before SMMU is initialized, hence device is
> guaranteed to exist and mc can't be NULL.
Yes, but that assumes that args->np points to the memory controller's
device tree node. It's obviously a mistake to do this, but I don't think
anyone will prevent you from doing this:
iommus = <&{/chosen} 0>;
In that case, since no platform device is created for the /chosen node,
iommu_pdev will end up being NULL and platform_get_drvdata() will crash.
That said, I'm fine with not adding a check for that. If anyone really
does end up messing this up they deserve the crash.
I'm still a bit undecided about the mc->smmu check because I haven't
convinced myself yet that it can't happen.
Thierry
05.10.2020 14:15, Thierry Reding пишет:
> On Mon, Oct 05, 2020 at 01:36:55PM +0300, Dmitry Osipenko wrote:
>> 05.10.2020 12:53, Thierry Reding пишет:
>>> On Fri, Oct 02, 2020 at 05:50:08PM +0300, Dmitry Osipenko wrote:
>>>> 02.10.2020 17:22, Dmitry Osipenko пишет:
>>>>>> static int tegra_smmu_of_xlate(struct device *dev,
>>>>>> struct of_phandle_args *args)
>>>>>> {
>>>>>> + struct platform_device *iommu_pdev = of_find_device_by_node(args->np);
>>>>>> + struct tegra_mc *mc = platform_get_drvdata(iommu_pdev);
>>>>>> u32 id = args->args[0];
>>>>>>
>>>>>> + of_node_put(args->np);
>>>>>> +
>>>>>> + if (!mc || !mc->smmu)
>>>>>> + return -EPROBE_DEFER;
>>>>> platform_get_drvdata(NULL) will crash.
>>>>>
>>>>
>>>> Actually, platform_get_drvdata(NULL) can't happen. I overlooked this.
>>>
>>> How so? It's technically possible for the iommus property to reference a
>>> device tree node for which no platform device will ever be created, in
>>> which case of_find_device_by_node() will return NULL. That's very
>>> unlikely and perhaps worth just crashing on to make sure it gets fixed
>>> immediately.
>>
>> The tegra_smmu_ops are registered from the SMMU driver itself and MC
>> driver sets platform data before SMMU is initialized, hence device is
>> guaranteed to exist and mc can't be NULL.
>
> Yes, but that assumes that args->np points to the memory controller's
> device tree node. It's obviously a mistake to do this, but I don't think
> anyone will prevent you from doing this:
>
> iommus = <&{/chosen} 0>;
>
> In that case, since no platform device is created for the /chosen node,
> iommu_pdev will end up being NULL and platform_get_drvdata() will crash.
But then Tegra SMMU isn't associated with the device's IOMMU path, and
thus, tegra_smmu_of_xlate() won't be invoked for this device.
> That said, I'm fine with not adding a check for that. If anyone really
> does end up messing this up they deserve the crash.
>
> I'm still a bit undecided about the mc->smmu check because I haven't
> convinced myself yet that it can't happen.
For now I can't see any realistic situation where mc->smmu could be NULL.
On Mon, Oct 05, 2020 at 04:28:53PM +0300, Dmitry Osipenko wrote:
> 05.10.2020 14:15, Thierry Reding пишет:
> > On Mon, Oct 05, 2020 at 01:36:55PM +0300, Dmitry Osipenko wrote:
> >> 05.10.2020 12:53, Thierry Reding пишет:
> >>> On Fri, Oct 02, 2020 at 05:50:08PM +0300, Dmitry Osipenko wrote:
> >>>> 02.10.2020 17:22, Dmitry Osipenko пишет:
> >>>>>> static int tegra_smmu_of_xlate(struct device *dev,
> >>>>>> struct of_phandle_args *args)
> >>>>>> {
> >>>>>> + struct platform_device *iommu_pdev = of_find_device_by_node(args->np);
> >>>>>> + struct tegra_mc *mc = platform_get_drvdata(iommu_pdev);
> >>>>>> u32 id = args->args[0];
> >>>>>>
> >>>>>> + of_node_put(args->np);
> >>>>>> +
> >>>>>> + if (!mc || !mc->smmu)
> >>>>>> + return -EPROBE_DEFER;
> >>>>> platform_get_drvdata(NULL) will crash.
> >>>>>
> >>>>
> >>>> Actually, platform_get_drvdata(NULL) can't happen. I overlooked this.
> >>>
> >>> How so? It's technically possible for the iommus property to reference a
> >>> device tree node for which no platform device will ever be created, in
> >>> which case of_find_device_by_node() will return NULL. That's very
> >>> unlikely and perhaps worth just crashing on to make sure it gets fixed
> >>> immediately.
> >>
> >> The tegra_smmu_ops are registered from the SMMU driver itself and MC
> >> driver sets platform data before SMMU is initialized, hence device is
> >> guaranteed to exist and mc can't be NULL.
> >
> > Yes, but that assumes that args->np points to the memory controller's
> > device tree node. It's obviously a mistake to do this, but I don't think
> > anyone will prevent you from doing this:
> >
> > iommus = <&{/chosen} 0>;
> >
> > In that case, since no platform device is created for the /chosen node,
> > iommu_pdev will end up being NULL and platform_get_drvdata() will crash.
>
> But then Tegra SMMU isn't associated with the device's IOMMU path, and
> thus, tegra_smmu_of_xlate() won't be invoked for this device.
Indeed, you're right! It used to be that ops were assigned to the bus
without any knowledge about the specific instances that might exist, but
nowadays there's struct iommu_device which properly encapsulates all of
that, so yeah, I don't think this can ever be NULL.
Although that makes me wonder why we aren't going one step further and
pass struct iommu_device * into ->of_xlate(), which would avoid the need
for us to do the look up once more.
Thierry
On Mon, Oct 05, 2020 at 11:57:54AM +0200, Thierry Reding wrote:
> On Fri, Oct 02, 2020 at 11:58:29AM -0700, Nicolin Chen wrote:
> > On Fri, Oct 02, 2020 at 06:02:18PM +0300, Dmitry Osipenko wrote:
> > > 02.10.2020 09:08, Nicolin Chen пишет:
> > > > static int tegra_smmu_of_xlate(struct device *dev,
> > > > struct of_phandle_args *args)
> > > > {
> > > > + struct platform_device *iommu_pdev = of_find_device_by_node(args->np);
> > > > + struct tegra_mc *mc = platform_get_drvdata(iommu_pdev);
> > > > u32 id = args->args[0];
> > > >
> > > > + of_node_put(args->np);
> > >
> > > of_find_device_by_node() takes device reference and not the np
> > > reference. This is a bug, please remove of_node_put().
> >
> > Looks like so. Replacing it with put_device(&iommu_pdev->dev);
>
> Putting the put_device() here is wrong, though. You need to make sure
> you keep a reference to it as long as you keep accessing the data that
> is owned by it.
I am confused. You said in the other reply (to Dmitry) that we do
need to put_device(mc->dev), where mc->dev should be the same as
iommu_pdev->dev. But here your comments sounds that we should not
put_device at all since ->probe_device/group_device/attach_dev()
will use it later.
> Like I said earlier, this is a bit weird in this case because we're
> self-referencing, so iommu_pdev->dev is going to stay around as long as
> the SMMU is. However, it might be worth to properly track the lifetime
> anyway just so that the code can serve as a good example of how to do
> things.
What's this "track-the-lifetime"?
> If you decide to go for the shortcut and not track this reference
> properly, then at least you need to add a comment as to why it is safe
> to do in this case. This ensures that readers are away of the
> circumstances and don't copy this bad code into a context where the
> circumstances are different.
I don't quite get this "shortcut" here either...mind elaborating?
On Mon, Oct 05, 2020 at 06:05:46PM -0700, Nicolin Chen wrote:
> On Mon, Oct 05, 2020 at 11:57:54AM +0200, Thierry Reding wrote:
> > On Fri, Oct 02, 2020 at 11:58:29AM -0700, Nicolin Chen wrote:
> > > On Fri, Oct 02, 2020 at 06:02:18PM +0300, Dmitry Osipenko wrote:
> > > > 02.10.2020 09:08, Nicolin Chen пишет:
> > > > > static int tegra_smmu_of_xlate(struct device *dev,
> > > > > struct of_phandle_args *args)
> > > > > {
> > > > > + struct platform_device *iommu_pdev = of_find_device_by_node(args->np);
> > > > > + struct tegra_mc *mc = platform_get_drvdata(iommu_pdev);
> > > > > u32 id = args->args[0];
> > > > >
> > > > > + of_node_put(args->np);
> > > >
> > > > of_find_device_by_node() takes device reference and not the np
> > > > reference. This is a bug, please remove of_node_put().
> > >
> > > Looks like so. Replacing it with put_device(&iommu_pdev->dev);
> >
> > Putting the put_device() here is wrong, though. You need to make sure
> > you keep a reference to it as long as you keep accessing the data that
> > is owned by it.
>
> I am confused. You said in the other reply (to Dmitry) that we do
> need to put_device(mc->dev), where mc->dev should be the same as
> iommu_pdev->dev. But here your comments sounds that we should not
> put_device at all since ->probe_device/group_device/attach_dev()
> will use it later.
You need to call put_device() at some point to release the reference
that you acquired by calling of_find_device_by_node(). If you don't
release it, you're leaking the reference and the kernel isn't going to
know when it's safe to delete the device.
So what I'm saying is that we either release it here, which isn't quite
right because we do reference data relating to the device later on. And
because it isn't quite right there should be a reason to justify it,
which is that the SMMU parent device is the same as the MC, so the
reference count isn't strictly necessary. But that's not quite obvious,
so highlighting it in a comment makes sense.
The other alternative is to not call put_device() here and keep on to
the reference as long as you keep using "mc". This might be difficult to
implement because it may not be obvious where to release it. I think
this is the better alternative, but if it's too complicated to implement
it might not be worth it.
> > Like I said earlier, this is a bit weird in this case because we're
> > self-referencing, so iommu_pdev->dev is going to stay around as long as
> > the SMMU is. However, it might be worth to properly track the lifetime
> > anyway just so that the code can serve as a good example of how to do
> > things.
>
> What's this "track-the-lifetime"?
This basically just means that SMMU needs to ensure that MC stays alive
(by holding a reference to it) as long as SMMU uses it. If the last
reference to MC is dropped, then the mc pointer and potentially anything
that it points to will become dangling. If you were to drop the last
reference at this point, then on the next line the mc pointer could
already be invalid.
That's how it generally works, anyway. What's special about this use-
case is that the SMMU and MC are the same device, so it should be safe
to omit this additional tracking because the IOMMU tracking should take
care of that already.
> > If you decide to go for the shortcut and not track this reference
> > properly, then at least you need to add a comment as to why it is safe
> > to do in this case. This ensures that readers are away of the
> > circumstances and don't copy this bad code into a context where the
> > circumstances are different.
>
> I don't quite get this "shortcut" here either...mind elaborating?
The shortcut is taking advantage of the knowledge that the SMMU and the
MC are the same device and therefore not properly track the MC object.
Given that their code is located in different locations, this isn't
obvious to the casual reader of the code, so they may assume that this
is the normal way to do things. To avoid that, the code should have a
comment explaining why that is.
Thierry
On Thu, Oct 08, 2020 at 11:53:43AM +0200, Thierry Reding wrote:
> On Mon, Oct 05, 2020 at 06:05:46PM -0700, Nicolin Chen wrote:
> > On Mon, Oct 05, 2020 at 11:57:54AM +0200, Thierry Reding wrote:
> > > On Fri, Oct 02, 2020 at 11:58:29AM -0700, Nicolin Chen wrote:
> > > > On Fri, Oct 02, 2020 at 06:02:18PM +0300, Dmitry Osipenko wrote:
> > > > > 02.10.2020 09:08, Nicolin Chen пишет:
> > > > > > static int tegra_smmu_of_xlate(struct device *dev,
> > > > > > struct of_phandle_args *args)
> > > > > > {
> > > > > > + struct platform_device *iommu_pdev = of_find_device_by_node(args->np);
> > > > > > + struct tegra_mc *mc = platform_get_drvdata(iommu_pdev);
> > > > > > u32 id = args->args[0];
> > > > > >
> > > > > > + of_node_put(args->np);
> > > > >
> > > > > of_find_device_by_node() takes device reference and not the np
> > > > > reference. This is a bug, please remove of_node_put().
> > > >
> > > > Looks like so. Replacing it with put_device(&iommu_pdev->dev);
> > >
> > > Putting the put_device() here is wrong, though. You need to make sure
> > > you keep a reference to it as long as you keep accessing the data that
> > > is owned by it.
> >
> > I am confused. You said in the other reply (to Dmitry) that we do
> > need to put_device(mc->dev), where mc->dev should be the same as
> > iommu_pdev->dev. But here your comments sounds that we should not
> > put_device at all since ->probe_device/group_device/attach_dev()
> > will use it later.
>
> You need to call put_device() at some point to release the reference
> that you acquired by calling of_find_device_by_node(). If you don't
> release it, you're leaking the reference and the kernel isn't going to
> know when it's safe to delete the device.
>
> So what I'm saying is that we either release it here, which isn't quite
> right because we do reference data relating to the device later on. And
I see. A small question here by the way: By looking at other IOMMU
drivers that are calling driver_find_device_by_fwnode() function,
I found that most of them put_device right after the function call,
and dev_get_drvdata() after putting the device..
Feels like they are doing it wrongly?
> because it isn't quite right there should be a reason to justify it,
> which is that the SMMU parent device is the same as the MC, so the
> reference count isn't strictly necessary. But that's not quite obvious,
> so highlighting it in a comment makes sense.
>
> The other alternative is to not call put_device() here and keep on to
> the reference as long as you keep using "mc". This might be difficult to
> implement because it may not be obvious where to release it. I think
> this is the better alternative, but if it's too complicated to implement
> it might not be worth it.
I feel so too. The dev is got at of_xlate() that does not have an
obvious counterpart function. So I'll just remove put_device() and
put a line of comments, as you suggested.
> > > Like I said earlier, this is a bit weird in this case because we're
> > > self-referencing, so iommu_pdev->dev is going to stay around as long as
> > > the SMMU is. However, it might be worth to properly track the lifetime
> > > anyway just so that the code can serve as a good example of how to do
> > > things.
> >
> > What's this "track-the-lifetime"?
>
> This basically just means that SMMU needs to ensure that MC stays alive
> (by holding a reference to it) as long as SMMU uses it. If the last
> reference to MC is dropped, then the mc pointer and potentially anything
> that it points to will become dangling. If you were to drop the last
> reference at this point, then on the next line the mc pointer could
> already be invalid.
>
> That's how it generally works, anyway. What's special about this use-
> case is that the SMMU and MC are the same device, so it should be safe
> to omit this additional tracking because the IOMMU tracking should take
> care of that already.
Okay.
> > > If you decide to go for the shortcut and not track this reference
> > > properly, then at least you need to add a comment as to why it is safe
> > > to do in this case. This ensures that readers are away of the
> > > circumstances and don't copy this bad code into a context where the
> > > circumstances are different.
> >
> > I don't quite get this "shortcut" here either...mind elaborating?
>
> The shortcut is taking advantage of the knowledge that the SMMU and the
> MC are the same device and therefore not properly track the MC object.
> Given that their code is located in different locations, this isn't
> obvious to the casual reader of the code, so they may assume that this
> is the normal way to do things. To avoid that, the code should have a
> comment explaining why that is.
Got it. Thanks!
On Thu, Oct 08, 2020 at 02:12:10PM -0700, Nicolin Chen wrote:
> On Thu, Oct 08, 2020 at 11:53:43AM +0200, Thierry Reding wrote:
> > On Mon, Oct 05, 2020 at 06:05:46PM -0700, Nicolin Chen wrote:
> > > On Mon, Oct 05, 2020 at 11:57:54AM +0200, Thierry Reding wrote:
> > > > On Fri, Oct 02, 2020 at 11:58:29AM -0700, Nicolin Chen wrote:
> > > > > On Fri, Oct 02, 2020 at 06:02:18PM +0300, Dmitry Osipenko wrote:
> > > > > > 02.10.2020 09:08, Nicolin Chen пишет:
> > > > > > > static int tegra_smmu_of_xlate(struct device *dev,
> > > > > > > struct of_phandle_args *args)
> > > > > > > {
> > > > > > > + struct platform_device *iommu_pdev = of_find_device_by_node(args->np);
> > > > > > > + struct tegra_mc *mc = platform_get_drvdata(iommu_pdev);
> > > > > > > u32 id = args->args[0];
> > > > > > >
> > > > > > > + of_node_put(args->np);
> > > > > >
> > > > > > of_find_device_by_node() takes device reference and not the np
> > > > > > reference. This is a bug, please remove of_node_put().
> > > > >
> > > > > Looks like so. Replacing it with put_device(&iommu_pdev->dev);
> > > >
> > > > Putting the put_device() here is wrong, though. You need to make sure
> > > > you keep a reference to it as long as you keep accessing the data that
> > > > is owned by it.
> > >
> > > I am confused. You said in the other reply (to Dmitry) that we do
> > > need to put_device(mc->dev), where mc->dev should be the same as
> > > iommu_pdev->dev. But here your comments sounds that we should not
> > > put_device at all since ->probe_device/group_device/attach_dev()
> > > will use it later.
> >
> > You need to call put_device() at some point to release the reference
> > that you acquired by calling of_find_device_by_node(). If you don't
> > release it, you're leaking the reference and the kernel isn't going to
> > know when it's safe to delete the device.
> >
> > So what I'm saying is that we either release it here, which isn't quite
> > right because we do reference data relating to the device later on. And
>
> I see. A small question here by the way: By looking at other IOMMU
> drivers that are calling driver_find_device_by_fwnode() function,
> I found that most of them put_device right after the function call,
> and dev_get_drvdata() after putting the device..
>
> Feels like they are doing it wrongly?
Well, like I said this is somewhat academic because these are all
referencing the IOMMU that by definition still needs to be around
when this code is called, and there's locks in place to ensure
these don't go away. So it's not like these drivers are doing it
wrong, they're just not doing it pedantically right.
>
> > because it isn't quite right there should be a reason to justify it,
> > which is that the SMMU parent device is the same as the MC, so the
> > reference count isn't strictly necessary. But that's not quite obvious,
> > so highlighting it in a comment makes sense.
> >
> > The other alternative is to not call put_device() here and keep on to
> > the reference as long as you keep using "mc". This might be difficult to
> > implement because it may not be obvious where to release it. I think
> > this is the better alternative, but if it's too complicated to implement
> > it might not be worth it.
>
> I feel so too. The dev is got at of_xlate() that does not have an
> obvious counterpart function. So I'll just remove put_device() and
> put a line of comments, as you suggested.
I think you misunderstood. Not calling put_device() would be wrong
because that leaks a reference to the SMMU that you can't get back. My
suggestion was rather to keep put_device() here, but add a comment as to
why it's okay to call the put_device() here, even though you keep using
its private data later beyond this point, which typically would be wrong
to do.
Thierry
On Fri, Oct 09, 2020 at 02:25:56PM +0200, Thierry Reding wrote:
> On Thu, Oct 08, 2020 at 02:12:10PM -0700, Nicolin Chen wrote:
> > On Thu, Oct 08, 2020 at 11:53:43AM +0200, Thierry Reding wrote:
> > > On Mon, Oct 05, 2020 at 06:05:46PM -0700, Nicolin Chen wrote:
> > > > On Mon, Oct 05, 2020 at 11:57:54AM +0200, Thierry Reding wrote:
> > > > > On Fri, Oct 02, 2020 at 11:58:29AM -0700, Nicolin Chen wrote:
> > > > > > On Fri, Oct 02, 2020 at 06:02:18PM +0300, Dmitry Osipenko wrote:
> > > > > > > 02.10.2020 09:08, Nicolin Chen пишет:
> > > > > > > > static int tegra_smmu_of_xlate(struct device *dev,
> > > > > > > > struct of_phandle_args *args)
> > > > > > > > {
> > > > > > > > + struct platform_device *iommu_pdev = of_find_device_by_node(args->np);
> > > > > > > > + struct tegra_mc *mc = platform_get_drvdata(iommu_pdev);
> > > > > > > > u32 id = args->args[0];
> > > > > > > >
> > > > > > > > + of_node_put(args->np);
> > > > > > >
> > > > > > > of_find_device_by_node() takes device reference and not the np
> > > > > > > reference. This is a bug, please remove of_node_put().
> > > > > >
> > > > > > Looks like so. Replacing it with put_device(&iommu_pdev->dev);
> > > > >
> > > > > Putting the put_device() here is wrong, though. You need to make sure
> > > > > you keep a reference to it as long as you keep accessing the data that
> > > > > is owned by it.
> > > >
> > > > I am confused. You said in the other reply (to Dmitry) that we do
> > > > need to put_device(mc->dev), where mc->dev should be the same as
> > > > iommu_pdev->dev. But here your comments sounds that we should not
> > > > put_device at all since ->probe_device/group_device/attach_dev()
> > > > will use it later.
> > >
> > > You need to call put_device() at some point to release the reference
> > > that you acquired by calling of_find_device_by_node(). If you don't
> > > release it, you're leaking the reference and the kernel isn't going to
> > > know when it's safe to delete the device.
> > >
> > > So what I'm saying is that we either release it here, which isn't quite
> > > right because we do reference data relating to the device later on. And
> >
> > I see. A small question here by the way: By looking at other IOMMU
> > drivers that are calling driver_find_device_by_fwnode() function,
> > I found that most of them put_device right after the function call,
> > and dev_get_drvdata() after putting the device..
> >
> > Feels like they are doing it wrongly?
>
> Well, like I said this is somewhat academic because these are all
> referencing the IOMMU that by definition still needs to be around
> when this code is called, and there's locks in place to ensure
> these don't go away. So it's not like these drivers are doing it
> wrong, they're just not doing it pedantically right.
>
> >
> > > because it isn't quite right there should be a reason to justify it,
> > > which is that the SMMU parent device is the same as the MC, so the
> > > reference count isn't strictly necessary. But that's not quite obvious,
> > > so highlighting it in a comment makes sense.
> > >
> > > The other alternative is to not call put_device() here and keep on to
> > > the reference as long as you keep using "mc". This might be difficult to
> > > implement because it may not be obvious where to release it. I think
> > > this is the better alternative, but if it's too complicated to implement
> > > it might not be worth it.
> >
> > I feel so too. The dev is got at of_xlate() that does not have an
> > obvious counterpart function. So I'll just remove put_device() and
> > put a line of comments, as you suggested.
>
> I think you misunderstood. Not calling put_device() would be wrong
> because that leaks a reference to the SMMU that you can't get back. My
> suggestion was rather to keep put_device() here, but add a comment as to
> why it's okay to call the put_device() here, even though you keep using
> its private data later beyond this point, which typically would be wrong
> to do.
I see. Thanks for clarification! Will send v6 soon.