All of lore.kernel.org
 help / color / mirror / Atom feed
From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
To: Yinghai Lu <yinghai@kernel.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>,
	David Miller <davem@davemloft.net>,
	David Ahern <david.ahern@oracle.com>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	"sparclinux@vger.kernel.org" <sparclinux@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"stable@vger.kernel.org" <stable@vger.kernel.org>
Subject: Re: [PATCH 3/3] PCI: Set pref for mem64 resource of pcie device
Date: Thu, 09 Apr 2015 13:17:39 +1000	[thread overview]
Message-ID: <1428549459.18187.56.camel@kernel.crashing.org> (raw)
In-Reply-To: <CAE9FiQXQN-VqN3ynLEhHh5WUYE+8tLR8ubKKMeHZ3h=BQ0rA9A@mail.gmail.com>

On Wed, 2015-04-08 at 17:06 -0700, Yinghai Lu wrote:
> On Wed, Apr 8, 2015 at 2:12 PM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> 
> > Thanks Bjorn. We can fix Yinghai patch for 4.2, it would be indeed handy
> > even for us to be able to support putting 64-bit NP BARs in prefetch
> > windows (For some SR-IOV adapters for example) too, but we need to do it
> > right.
> 
> Please check if you are ok with attached.

I'll let Bjorn be the final judge here but I am not fan of the way you
set/clear/set/clear the IORESOURCE_PREFETCH bit with
pci_set_pref_under_pref(). It's error prone and confusing, the code is
already barely readable as it is ...

I would rather you replace those various masks compares with a helper
that does something like pci_resource_compatible(parent_res, child_res),
which has the logic to test. That or a helper that does something like
pci_calc_compatible_res_flags() which returns a "flags" that has
PREFETCH set, which you use in place of res->flags in the various
allocation path.

As-is, your patch looks like a band-aid and smells like a hack :-)

Ben.



WARNING: multiple messages have this Message-ID (diff)
From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
To: Yinghai Lu <yinghai@kernel.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>,
	David Miller <davem@davemloft.net>,
	David Ahern <david.ahern@oracle.com>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	"sparclinux@vger.kernel.org" <sparclinux@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"stable@vger.kernel.org" <stable@vger.kernel.org>
Subject: Re: [PATCH 3/3] PCI: Set pref for mem64 resource of pcie device
Date: Thu, 09 Apr 2015 03:17:39 +0000	[thread overview]
Message-ID: <1428549459.18187.56.camel@kernel.crashing.org> (raw)
In-Reply-To: <CAE9FiQXQN-VqN3ynLEhHh5WUYE+8tLR8ubKKMeHZ3h=BQ0rA9A@mail.gmail.com>

On Wed, 2015-04-08 at 17:06 -0700, Yinghai Lu wrote:
> On Wed, Apr 8, 2015 at 2:12 PM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> 
> > Thanks Bjorn. We can fix Yinghai patch for 4.2, it would be indeed handy
> > even for us to be able to support putting 64-bit NP BARs in prefetch
> > windows (For some SR-IOV adapters for example) too, but we need to do it
> > right.
> 
> Please check if you are ok with attached.

I'll let Bjorn be the final judge here but I am not fan of the way you
set/clear/set/clear the IORESOURCE_PREFETCH bit with
pci_set_pref_under_pref(). It's error prone and confusing, the code is
already barely readable as it is ...

I would rather you replace those various masks compares with a helper
that does something like pci_resource_compatible(parent_res, child_res),
which has the logic to test. That or a helper that does something like
pci_calc_compatible_res_flags() which returns a "flags" that has
PREFETCH set, which you use in place of res->flags in the various
allocation path.

As-is, your patch looks like a band-aid and smells like a hack :-)

Ben.



  reply	other threads:[~2015-04-09  3:17 UTC|newest]

Thread overview: 99+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-04-01  2:57 [PATCH 0/3] PCI/sparc: Fix booting with T5-8 Yinghai Lu
2015-04-01  2:57 ` Yinghai Lu
2015-04-01  2:57 ` [PATCH 1/3] PCI: Introduce pci_bus_addr_t Yinghai Lu
2015-04-01  2:57   ` Yinghai Lu
2015-04-03 18:59   ` Bjorn Helgaas
2015-04-03 18:59     ` Bjorn Helgaas
2015-04-03 19:05     ` David Miller
2015-04-03 19:05       ` David Miller
2015-04-04  3:40     ` Yinghai Lu
2015-04-04  3:40       ` Yinghai Lu
2015-04-03 19:32   ` Bjorn Helgaas
2015-04-03 19:32     ` Bjorn Helgaas
2015-04-03 20:52     ` Bjorn Helgaas
2015-04-03 20:52       ` Bjorn Helgaas
2015-04-03 20:52       ` Bjorn Helgaas
2015-04-03 20:52       ` Bjorn Helgaas
2015-04-04  3:34       ` Yinghai Lu
2015-04-04  3:34         ` Yinghai Lu
2015-04-04  3:34         ` Yinghai Lu
2015-04-04  3:34         ` Yinghai Lu
2015-04-04 12:46         ` Bjorn Helgaas
2015-04-04 12:46           ` Bjorn Helgaas
2015-04-04 12:46           ` Bjorn Helgaas
2015-04-04 12:46           ` Bjorn Helgaas
2015-04-04 19:48           ` Rob Herring
2015-04-04 19:48             ` Rob Herring
2015-04-04 19:48             ` Rob Herring
2015-04-04 19:48             ` Rob Herring
2015-04-05  3:25             ` Bjorn Helgaas
2015-04-05  3:25               ` Bjorn Helgaas
2015-04-05  3:25               ` Bjorn Helgaas
2015-04-05  3:25               ` Bjorn Helgaas
2015-04-06 13:05               ` Rob Herring
2015-04-06 13:05                 ` Rob Herring
2015-04-06 13:05                 ` Rob Herring
2015-04-06 13:05                 ` Rob Herring
2015-04-01  2:57 ` [PATCH 2/3] sparc/PCI: Add mem64 resource parsing for root bus Yinghai Lu
2015-04-01  2:57   ` Yinghai Lu
2015-04-03 20:46   ` Bjorn Helgaas
2015-04-03 20:46     ` Bjorn Helgaas
2015-04-03 20:46     ` Bjorn Helgaas
2015-04-01  2:57 ` [PATCH 3/3] PCI: Set pref for mem64 resource of pcie device Yinghai Lu
2015-04-01  2:57   ` Yinghai Lu
2015-04-06 22:06   ` Bjorn Helgaas
2015-04-06 22:06     ` Bjorn Helgaas
2015-04-06 22:35     ` Yinghai Lu
2015-04-06 22:35       ` Yinghai Lu
2015-04-06 22:49       ` Bjorn Helgaas
2015-04-06 22:49         ` Bjorn Helgaas
2015-04-07  1:13         ` Yinghai Lu
2015-04-07  1:13           ` Yinghai Lu
2015-04-07  3:43           ` Bjorn Helgaas
2015-04-07  3:43             ` Bjorn Helgaas
2015-04-07  5:23             ` Yinghai Lu
2015-04-07  5:23               ` Yinghai Lu
2015-04-07 12:18               ` Bjorn Helgaas
2015-04-07 12:18                 ` Bjorn Helgaas
2015-04-07  0:35     ` David Miller
2015-04-07  0:35       ` David Miller
2015-04-07 16:48       ` Bjorn Helgaas
2015-04-07 16:48         ` Bjorn Helgaas
2015-04-08 15:47         ` Bjorn Helgaas
2015-04-08 15:47           ` Bjorn Helgaas
2015-04-08 16:08           ` David Miller
2015-04-08 16:08             ` David Miller
2015-04-08 21:12           ` Benjamin Herrenschmidt
2015-04-08 21:12             ` Benjamin Herrenschmidt
2015-04-09  0:06             ` Yinghai Lu
2015-04-09  0:06               ` Yinghai Lu
2015-04-09  3:17               ` Benjamin Herrenschmidt [this message]
2015-04-09  3:17                 ` Benjamin Herrenschmidt
2015-04-09  4:11                 ` Yinghai Lu
2015-04-09  4:11                   ` Yinghai Lu
2015-04-09  8:56                   ` Benjamin Herrenschmidt
2015-04-09  8:56                     ` Benjamin Herrenschmidt
2015-04-09  4:26                 ` Bjorn Helgaas
2015-04-09  4:26                   ` Bjorn Helgaas
2015-04-09  8:54                   ` Benjamin Herrenschmidt
2015-04-09  8:54                     ` Benjamin Herrenschmidt
2015-04-09 18:31                     ` Yinghai Lu
2015-04-09 18:31                       ` Yinghai Lu
2015-04-09 23:31                       ` Benjamin Herrenschmidt
2015-04-09 23:31                         ` Benjamin Herrenschmidt
2015-04-10  4:13                         ` Yinghai Lu
2015-04-10  4:13                           ` Yinghai Lu
2015-04-02 20:37 ` [PATCH 0/3] PCI/sparc: Fix booting with T5-8 David Miller
2015-04-02 20:37   ` David Miller
2015-04-02 22:07   ` Yinghai Lu
2015-04-02 22:07     ` Yinghai Lu
2015-04-02 22:13     ` Bjorn Helgaas
2015-04-02 22:13       ` Bjorn Helgaas
2015-04-03  0:42     ` David Miller
2015-04-03  0:42       ` David Miller
2015-05-16 15:25 ` Bjorn Helgaas
2015-05-16 15:25   ` Bjorn Helgaas
2015-05-16 15:28   ` Bjorn Helgaas
2015-05-16 15:28     ` Bjorn Helgaas
2015-05-27 23:27     ` Yinghai Lu
2015-05-27 23:27       ` Yinghai Lu

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=1428549459.18187.56.camel@kernel.crashing.org \
    --to=benh@kernel.crashing.org \
    --cc=bhelgaas@google.com \
    --cc=davem@davemloft.net \
    --cc=david.ahern@oracle.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=sparclinux@vger.kernel.org \
    --cc=stable@vger.kernel.org \
    --cc=yinghai@kernel.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.