Linux-PCI Archive on lore.kernel.org
 help / color / Atom feed
From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
To: Rob Herring <robh@kernel.org>
Cc: "Chocron, Jonathan" <jonnyc@amazon.com>,
	"zhengdejin5@gmail.com" <zhengdejin5@gmail.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"thomas.petazzoni@bootlin.com" <thomas.petazzoni@bootlin.com>,
	"pratyush.anand@gmail.com" <pratyush.anand@gmail.com>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	"bhelgaas@google.com" <bhelgaas@google.com>,
	"tjoseph@cadence.com" <tjoseph@cadence.com>
Subject: Re: [PATCH v1] PCI: controller: Remove duplicate error message
Date: Mon, 6 Jul 2020 16:58:47 +0100
Message-ID: <20200706155847.GA32050@e121166-lin.cambridge.arm.com> (raw)
In-Reply-To: <CAL_JsqJWKfShzb6r=pXFv03T4L+nmNrCHvt+NkEy5EFuuD1HAA@mail.gmail.com>

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

  reply index

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-26 15:09 Dejin Zheng
2020-05-26 18:22 ` Chocron, Jonathan
2020-05-27 13:20   ` Dejin Zheng
2020-06-02  7:44     ` Chocron, Jonathan
2020-06-02 15:01       ` Rob Herring
2020-07-06 15:58         ` Lorenzo Pieralisi [this message]
2020-07-11  7:47           ` Dejin Zheng
2020-07-15 16:45             ` Lorenzo Pieralisi
2020-07-17 13:44               ` Dejin Zheng
2020-06-01 22:13 ` Rob Herring

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=20200706155847.GA32050@e121166-lin.cambridge.arm.com \
    --to=lorenzo.pieralisi@arm.com \
    --cc=bhelgaas@google.com \
    --cc=jonnyc@amazon.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=pratyush.anand@gmail.com \
    --cc=robh@kernel.org \
    --cc=thomas.petazzoni@bootlin.com \
    --cc=tjoseph@cadence.com \
    --cc=zhengdejin5@gmail.com \
    /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

Linux-PCI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pci/0 linux-pci/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pci linux-pci/ https://lore.kernel.org/linux-pci \
		linux-pci@vger.kernel.org
	public-inbox-index linux-pci

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pci


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git