linux-kernel - Re: [PATCH v1] PCI: controller: Remove duplicate error message

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]

Message-ID: <20200706155847.GA32050@e121166-lin.cambridge.arm.com>
Date:   Mon, 6 Jul 2020 16:58:47 +0100
From:   Lorenzo Pieralisi <lorenzo.pieralisi@....com>
To:     Rob Herring <robh@...nel.org>
Cc:     "Chocron, Jonathan" <jonnyc@...zon.com>,
        "zhengdejin5@...il.com" <zhengdejin5@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "thomas.petazzoni@...tlin.com" <thomas.petazzoni@...tlin.com>,
        "pratyush.anand@...il.com" <pratyush.anand@...il.com>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        "bhelgaas@...gle.com" <bhelgaas@...gle.com>,
        "tjoseph@...ence.com" <tjoseph@...ence.com>
Subject: Re: [PATCH v1] PCI: controller: Remove duplicate error message

On Tue, Jun 02, 2020 at 09:01:13AM -0600, Rob Herring wrote:

[...]

> > > In fact, I think its error handling is clear enough, It just goes
> > > wrong
> > > in three places, as follows:
> > >
> > > void __iomem *devm_pci_remap_cfg_resource(struct device *dev,
> > >                                           struct resource *res)
> > > {
> > >         resource_size_t size;
> > >         const char *name;
> > >         void __iomem *dest_ptr;
> > >
> > >         BUG_ON(!dev);
> > >
> > >         if (!res || resource_type(res) != IORESOURCE_MEM) {
> > >                 dev_err(dev, "invalid resource\n");
> > >                 return IOMEM_ERR_PTR(-EINVAL);
> > >         }
> > >
> > In the above error case there is no indication of which resource failed
> > (mainly relevant if the resource name is missing in the devicetree,
> > since in the drivers you are changing platform_get_resource_byname() is
> > mostly used). In the existing drivers' code, on return from this
> > function in this case, the name would be printed by the caller.
> 
> A driver should only have one call to devm_pci_remap_cfg_resource() as
> there's only 1 config space. However, it looks like this function is
> frequently used on what is not config space which is a bigger issue.

That certainly is and should be fixed.

> If this error happens, it's almost always going to be a NULL ptr as
> platform_get_resource_byname() would have set IORESOURCE_MEM. Perhaps
> a WARN here so you get a backtrace to the caller location.

+1

> > >         size = resource_size(res);
> > >         name = res->name ?: dev_name(dev);
> > >
> > >         if (!devm_request_mem_region(dev, res->start, size, name)) {
> > >                 dev_err(dev, "can't request region for resource
> > > %pR\n", res);
> > >                 return IOMEM_ERR_PTR(-EBUSY);
> > >         }
> > >
> > >         dest_ptr = devm_pci_remap_cfgspace(dev, res->start, size);
> > >         if (!dest_ptr) {
> > >                 dev_err(dev, "ioremap failed for resource %pR\n",
> > > res);
> > >                 devm_release_mem_region(dev, res->start, size);
> > >                 dest_ptr = IOMEM_ERR_PTR(-ENOMEM);
> > >         }
> > >
> > The other 2 error cases as well don't print the resource name as far as
> > I recall (they will at least print the resource start/end).
> 
> Start/end are what are important for why either of these functions
> failed.
> 
> But sure, we could add 'name' here. That's a separate patch IMO.

I agree. In sum, I think it is OK to proceed with this patch, provided
we send follow-ups as discussed here, are we in agreement ?

Lorenzo