devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
To: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
Cc: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>,
	"linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org"
	<linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org>,
	iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org,
	joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org,
	"devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
	<devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
	Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
Subject: Re: [PATCH] drivers/of_iommu: ignore SMMU DT nodes with status 'disabled'
Date: Wed, 3 May 2017 11:58:30 +0100	[thread overview]
Message-ID: <4B3EA199-183C-40CE-8C7B-E55CD92FD402@linaro.org> (raw)
In-Reply-To: <1c0463e6-f82f-1179-4f54-b2d3de63dc87-5wv7dgnIgG8@public.gmane.org>


> On 3 May 2017, at 11:32, Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org> wrote:
> 
>> On 28/04/17 14:22, Ard Biesheuvel wrote:
>>> On 28 April 2017 at 14:17, Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote:
>>>> On Fri, Apr 28, 2017 at 02:14:49PM +0100, Ard Biesheuvel wrote:
>>>>> On 28 April 2017 at 14:11, Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org> wrote:
>>>>> Hi Ard,
>>>>> 
>>>>> [+ devicetree@]
>>>>> 
>>>>>> On Fri, Apr 14, 2017 at 01:43:15PM +0100, Ard Biesheuvel wrote:
>>>>>> DT nodes may have a status property, and if they do, such nodes should
>>>>>> only be considered present if the status property is set to 'okay'.
>>>>>> 
>>>>>> Currently, we call the init function of IOMMUs described by the device
>>>>>> tree without taking this into account, which may result in the output
>>>>>> below on systems where some SMMUs may be legally disabled.
>>>>>> 
>>>>>> Failed to initialise IOMMU /smb/smmu@e0200000
>>>>>> Failed to initialise IOMMU /smb/smmu@e0c00000
>>>>>> arm-smmu e0a00000.smmu: probing hardware configuration...
>>>>>> arm-smmu e0a00000.smmu: SMMUv1 with:
>>>>>> arm-smmu e0a00000.smmu:  stage 2 translation
>>>>>> arm-smmu e0a00000.smmu:  coherent table walk
>>>>>> arm-smmu e0a00000.smmu:  stream matching with 32 register groups, mask 0x7fff
>>>>>> arm-smmu e0a00000.smmu:  8 context banks (8 stage-2 only)
>>>>>> arm-smmu e0a00000.smmu:  Supported page sizes: 0x60211000
>>>>>> arm-smmu e0a00000.smmu:  Stage-2: 40-bit IPA -> 40-bit PA
>>>>>> Failed to initialise IOMMU /smb/smmu@e0600000
>>>>>> Failed to initialise IOMMU /smb/smmu@e0800000
>>>>>> 
>>>>>> Since this is not an error condition, only call the init function if
>>>>>> the device is enabled, which also inhibits the spurious error messages.
>>>>>> 
>>>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>>>>> ---
>>>>>> drivers/iommu/of_iommu.c | 2 +-
>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>> 
>>>>>> diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
>>>>>> index 2683e9fc0dcf..2dd1206e6c0d 100644
>>>>>> --- a/drivers/iommu/of_iommu.c
>>>>>> +++ b/drivers/iommu/of_iommu.c
>>>>>> @@ -183,7 +183,7 @@ static int __init of_iommu_init(void)
>>>>>>      for_each_matching_node_and_match(np, matches, &match) {
>>>>>>              const of_iommu_init_fn init_fn = match->data;
>>>>>> 
>>>>>> -             if (init_fn(np))
>>>>>> +             if (of_device_is_available(np) && init_fn(np))
>>>>>>                      pr_err("Failed to initialise IOMMU %s\n",
>>>>>>                              of_node_full_name(np));
>>>>>>      }
>>>>> 
>>>>> Is there a definition of what status = "disabled" is supposed to mean for an
>>>>> IOMMU? For example, that could mean that the firmware has pre-programmed the
>>>>> SMMU with particular translations or memory attributes (a bit like the
>>>>> CCA=1, CPM=1, DACS=0 case in ACPI IORT), or even disabled DMA traffic
>>>>> altogether.
>>>>> 
>>>>> So I think we'd need an update to the generic IOMMU binding text to say
>>>>> exactly what the semantics are supposed to be here.
>>>>> 
>>>> 
>>>> I agree that it might make sense to describe the behavior of the IOMMU
>>>> when it is left in the state we found it in. But that is not the same
>>>> as status=disabled.
>>>> 
>>>> The DTS subtree contains loads and loads of boilerplate
>>>> configurations, where only some pieces are enabled in the final image
>>>> by setting status=okay. So a node that has status 'disabled' should be
>>>> treated as 'not present', not as 'present but can be ignored under
>>>> assumptions such and such'
>>>> 
>>>> In other words, I think we are talking about two different issues here.
>>> 
>>> I'm not so sure... if we have a master device that has an iommus= property
>>> pointing to an IOMMU with status="disabled", I really don't know whether we
>>> should:
>>> 
>>>  1. Assume the master can do DMA with a 1:1 mapping of memory and no
>>>     changes to memory attributes
>>> 
>>>  2. Assume the master can do DMA with a 1:1 mapping of memory, but
>>>     potentially with changes to the attributes
>>> 
>>>  3. Assume the master can do DMA, but with some pre-existing translation
>>>     (what?)
>>> 
>>>  4. Assume the master can't do DMA
>>> 
>>> and I also don't know whether the "dma-coherent" property remains valid.
>>> 
>> 
>> Ah yes. Good point.
>> 
>> So indeed, there should be some IOMMU specific status property that
>> can convey all of the above, or 1. and 4. at the minimum
> 
> FWIW, the underlying issue being addressed here should be going away now
> anyway, since the now-queued probe deferral series obviates the init_fn
> early-device-creation bodge. I've been deliberately ignoring it for some
> time for precisely that reason ;)
> 


Ok. I have also updated the Seattle firmware to remove the smmu nodes and the associated iommus/iommu-map properties entirely when disabling SMMU support in the firmware, which should address Will's concern regarding unspecified behavior of a disabled SMMU.

IOW, this patch can be disregarded. Thanks.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

      parent reply	other threads:[~2017-05-03 10:58 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20170414124315.2401-1-ard.biesheuvel@linaro.org>
     [not found] ` <20170414124315.2401-1-ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
2017-04-28 13:11   ` [PATCH] drivers/of_iommu: ignore SMMU DT nodes with status 'disabled' Will Deacon
     [not found]     ` <20170428131133.GJ13675-5wv7dgnIgG8@public.gmane.org>
2017-04-28 13:14       ` Ard Biesheuvel
     [not found]         ` <CAKv+Gu_G9fw0kwSetXmGjomc8Y_nu95O=rEVzgfafNRs0dtgvg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-04-28 13:17           ` Will Deacon
     [not found]             ` <20170428131744.GL13675-5wv7dgnIgG8@public.gmane.org>
2017-04-28 13:22               ` Ard Biesheuvel
     [not found]                 ` <CAKv+Gu8L1iyzBitt8zzvWDDEXk_ysGfPRxHJbQQK5o8NMWF5MA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2017-05-03 10:32                   ` Robin Murphy
     [not found]                     ` <1c0463e6-f82f-1179-4f54-b2d3de63dc87-5wv7dgnIgG8@public.gmane.org>
2017-05-03 10:58                       ` Ard Biesheuvel [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4B3EA199-183C-40CE-8C7B-E55CD92FD402@linaro.org \
    --to=ard.biesheuvel-qsej5fyqhm4dnm+yrofe0a@public.gmane.org \
    --cc=devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org \
    --cc=joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org \
    --cc=linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org \
    --cc=mark.rutland-5wv7dgnIgG8@public.gmane.org \
    --cc=robin.murphy-5wv7dgnIgG8@public.gmane.org \
    --cc=will.deacon-5wv7dgnIgG8@public.gmane.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).