linux-kernel - Re: [PATCH v2 3/4] OF: Simplify of_iommu

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] [thread-next>] [day] [month] [year] [list]

Date: Tue, 25 Jun 2024 19:44:04 +0100
From: Robin Murphy <robin.murphy@....com>
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: Will Deacon <will@...nel.org>, Joerg Roedel <joro@...tes.org>,
 linux-acpi@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org, iommu@...ts.linux.dev,
 devicetree@...r.kernel.org, Rob Herring <robh@...nel.org>,
 Saravana Kannan <saravanak@...gle.com>,
 Lorenzo Pieralisi <lpieralisi@...nel.org>, Hanjun Guo
 <guohanjun@...wei.com>, Sudeep Holla <sudeep.holla@....com>,
 "Rafael J. Wysocki" <rafael@...nel.org>, Len Brown <lenb@...nel.org>,
 Jean-Philippe Brucker <jean-philippe@...aro.org>
Subject: Re: [PATCH v2 3/4] OF: Simplify of_iommu_configure()

On 2024-06-22 11:23 pm, Andy Shevchenko wrote:
> On Fri, Jun 21, 2024 at 8:47 PM Robin Murphy <robin.murphy@....com> wrote:
>>
>> We no longer have a notion of partially-initialised fwspecs existing,
>> and we also no longer need to use an iommu_ops pointer to return status
>> to of_dma_configure(). Clean up the remains of those, which lends itself
>> to clarifying the logic around the dma_range_map allocation as well.
> 
> ...
> 
>> +       if (!err && dev->bus)
>> +               err = iommu_probe_device(dev);
>>
>> +       if (err && err != -EPROBE_DEFER)
>> +               dev_dbg(dev, "Adding to IOMMU failed: %d\n", err);
> 
> Hmm... I'm wondering if dev_err_probe() can be used here.

It's still possible to have other errors here benignly [1] (however 
questionable the underlying reason), and this has always been a 
dev_dbg(), it's just getting shuffled around again. The aim here is to 
carry on removing cruft to work towards getting rid of this 
iommu_probe_device() call altogether since it's fundamentally wrong, so 
I'm not inclined to add anything new or spend too much effort polishing 
code I still want to delete.

>>          return err;
> 
> ...
> 
>> +       dev_dbg(dev, "device is%sbehind an iommu\n",
>> +               !ret ? " " : " not ");
> 
> Why not a positive test?

Again, mostly because that's how it was written in 2014, same reason I'm 
not deduplicating the redundant space despite it still being the tiniest 
bit irritating. If you make me think about it, though, I suppose when 
both outcomes are otherwise equally weighted it does seems natural to 
consider "success" before "failure", thus the condition tests for success.

Thanks,
Robin.

[1] 
https://lore.kernel.org/linux-iommu/bbmhcoghrprmbdibnjum6lefix2eoquxrde7wyqeulm4xabmlm@b6jy32saugqh/