linux-kernel - Re: [PATCH V4 3/4] ACPI/IORT: Ignore all errors except EPROBE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <0c599944-5815-778b-57ae-5b3a6105c8d4@codeaurora.org>
Date:   Mon, 22 May 2017 18:32:29 +0530
From:   Sricharan R <sricharan@...eaurora.org>
To:     Lorenzo Pieralisi <lorenzo.pieralisi@....com>
Cc:     robin.murphy@....com, will.deacon@....com, joro@...tes.org,
        iommu@...ts.linux-foundation.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-arm-msm@...r.kernel.org, m.szyprowski@...sung.com,
        bhelgaas@...gle.com, linux-pci@...r.kernel.org,
        linux-acpi@...r.kernel.org, tn@...ihalf.com, hanjun.guo@...aro.org,
        okaya@...eaurora.org, robh+dt@...nel.org, frowand.list@...il.com,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        sudeep.holla@....com, rjw@...ysocki.net, lenb@...nel.org,
        catalin.marinas@....com, arnd@...db.de, linux-arch@...r.kernel.org,
        laurent.pinchart@...asonboard.com, j.neuschaefer@....net,
        geert@...ux-m68k.org, magnus.damm@...il.com
Subject: Re: [PATCH V4 3/4] ACPI/IORT: Ignore all errors except EPROBE_DEFER

Hi Lorenzo,

On 5/22/2017 5:01 PM, Lorenzo Pieralisi wrote:
> On Mon, May 22, 2017 at 04:35:43PM +0530, Sricharan R wrote:
>> Hi Lorenzo,
>>
>> On 5/22/2017 4:07 PM, Lorenzo Pieralisi wrote:
>>> Hi Sricharan,
>>>
>>> On Thu, May 18, 2017 at 08:24:16PM +0530, Sricharan R wrote:
>>>> While deferring the probe of IOMMU masters, xlate and
>>>> add_device callbacks called from iort_iommu_configure
>>>> can pass back error values like -ENODEV, which means
>>>> the IOMMU cannot be connected with that master for real
>>>> reasons. Before the IOMMU probe deferral, all such errors
>>>> were ignored. Now all those errors are propagated back,
>>>> killing the master's probe for such errors. Instead ignore
>>>> all the errors except EPROBE_DEFER, which is the only one
>>>> of concern and let the master work without IOMMU, thus
>>>> restoring the old behavior.
>>>>
>>>> Fixes: 5a1bb638d567 ("drivers: acpi: Handle IOMMU lookup failure with deferred probing or error")
>>>> Signed-off-by: Sricharan R <sricharan@...eaurora.org>
>>>> ---
>>>> [V4] Added this patch newly.
>>>>
>>>>  drivers/acpi/arm64/iort.c | 6 ++++++
>>>>  1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
>>>> index c5fecf9..16e101f 100644
>>>> --- a/drivers/acpi/arm64/iort.c
>>>> +++ b/drivers/acpi/arm64/iort.c
>>>> @@ -782,6 +782,12 @@ const struct iommu_ops *iort_iommu_configure(struct device *dev)
>>>>  	if (err)
>>>>  		ops = ERR_PTR(err);
>>>>  
>>>> +	/* Ignore all other errors apart from EPROBE_DEFER */
>>>> +	if (IS_ERR(ops) && (PTR_ERR(ops) != -EPROBE_DEFER)) {
>>>> +		dev_dbg(dev, "Adding to IOMMU failed: %ld\n", PTR_ERR(ops));
>>>> +		ops = NULL;
>>>> +	}
>>>> +
>>>>  	return ops;
>>>>  }
>>>>  
>>>
>>> It is getting a bit convoluted. Is not it better to propagate the
>>> error back and let acpi_dma_configure() make a decision accordingly ?
>>>
>>
>> ok, I was trying to keep it in same way as DT, where of_dma_configure
>> (here acpi_dma_configure) calls of_iommu_configure(here iort_iommu_configure)
>> which ends up doing the check. So will have to be changed in both places
>> for symmetry.
>>
>>> Something like:
>>>
>>> -- >8 --
>>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
>>> index e39ec7b..3a10d757 100644
>>> --- a/drivers/acpi/scan.c
>>> +++ b/drivers/acpi/scan.c
>>> @@ -1371,8 +1371,8 @@ int acpi_dma_configure(struct device *dev, enum dev_dma_attr attr)
>>>  	iort_set_dma_mask(dev);
>>>  
>>>  	iommu = iort_iommu_configure(dev);
>>> -	if (IS_ERR(iommu))
>>> -		return PTR_ERR(iommu);
>>> +	if (IS_ERR(iommu) && PTR_ERR(iommu) == -EPROBE_DEFER)
>>> +		return -EPROBE_DEFER;
>>
>> In case of other errors apart from EPROBE_DEFER,
>> iommu = NULL should be set, before passing it on to
>> arch_setup_dma_ops.
> 
> Ah yes, sorry, that makes it uglier :(
> 
> Point is, it does not make sense to me to have of/acpi_dma_configure()
> check a return code with (IS_ERR()) whilst we know the only error
> code you are allowed to handle is -EPROBE_DEFER, while at it it is
> better to make the -EPROBE_DEFER check explicit (otherwise it may
> seem that of/acpi_dma_configure() can handle an error code that
> is different from -EPROBE_DEFER - but they don't), no big deal either
> way though.
> 

ok, in which case, the above check that you have shown
to be added additionally to the original patch.

Regards,
 Sricharan

-- 
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation