From: Ram Pai <linuxram@us.ibm.com>
To: Bjorn Helgaas <bhelgaas@google.com>
Cc: Yinghai Lu <yinghai@kernel.org>, Ram Pai <linuxram@us.ibm.com>,
linux-pci@vger.kernel.org
Subject: Re: [RFC PATCH] methods to access resources of a struct pci_dev
Date: Thu, 16 Aug 2012 11:26:02 +0800 [thread overview]
Message-ID: <20120816032602.GN2449@ram-ThinkPad-T61> (raw)
In-Reply-To: <CAErSpo40Ok9NuMm0zxGnZ0DmFmGT19Y23JC4BCQV1RSxsmakCw@mail.gmail.com>
On Wed, Aug 15, 2012 at 03:25:06PM -0600, Bjorn Helgaas wrote:
> On Mon, Jun 18, 2012 at 12:30 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> > On Sun, Jun 17, 2012 at 10:03 PM, Ram Pai <linuxram@us.ibm.com> wrote:
> >> PCI: methods to access resources of struct pci_dev
> >>
> >> Currently pci_dev structure holds an array of 17 PCI resources; six base
> >> BARs, one ROM BAR, four BRIDGE BARs, six sriov BARs. This is wasteful.
> >> A bridge device just needs the 4 bridge resources. A non-bridge device
> >> just needs the six base resources and one ROM resource. The sriov
> >> resources are needed only if the device has SRIOV capability.
> >>
> >> The pci_dev structure needs to be re-organized to avoid unnecessary
> >> bloating. However too much code outside the pci-bus driver, assumes the
> >> internal details of the pci_dev structure, thus making it hard to
> >> re-organize the datastructure.
> >>
> >> As a first step this patch provides generic methods to access the
> >> resource structure of the pci_dev.
> >>
> >> Once this patch is accepted, I have another 40+ patches that modify all
> >> the files that directly access the resource structure, to use the new
> >> methods provided in the first step.
> >>
> >> Finally we can re-organize the resource structure in the pci_dev
> >> structure and correspondingly update the methods.
> >
> > I have patchset on this, please check
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-yinghai.git
> > for-pci-for-each-res-addon
>
> I don't like the pci_dev.resource[] table very well either, and I
> don't like the fact that drivers access it directly. So I do think we
> need some sort of abstraction that lets us change the way we store the
> resources without affecting the callers.
>
> Both of these (Ram's patch:
> http://marc.info/?l=linux-pci&m=133999604128815&w=2 and Yinghai's
> patch: http://git.kernel.org/?p=linux/kernel/git/yinghai/linux-yinghai.git;a=commitdiff;h=cd192f0ed93203ef6bac2a44c138899190fb5793)
> have a similar approach of adding many iterators.
>
> Yinghai's adds:
> for_each_pci_dev_all_resource
> for_each_pci_dev_nobridge_resource
> for_each_pci_dev_base_resource
> for_each_pci_dev_base_norom_resource
> for_each_pci_dev_base_iov_norom_resource
> for_each_pci_dev_noiov_resource
> for_each_pci_dev_std_resource
> for_each_pci_dev_iov_resource
> for_each_pci_dev_bridge_resource
> for_each_pci_dev_addon_resource
>
> Ram's adds:
> for_each_resource
> for_each_std_resource
> for_each_std_and_rom_resource
> for_each_sriov_resource
> for_each_bridge_resource
>
> It seems like we have a combinatorial explosion of iterators based on
> various orthogonal selectors (base, rom, iov, bridge window). That
> doesn't seem understandable or maintainable to me.
>
> I wonder if we could get by with *one* iterator, and select the
> resources of interest either by supplying a bitmask of things we care
> about, or by doing similar filtering in the caller.
I am fine with this approach. I have never encountered the need for 'no'
based iterator like 'for_each_pci_dev_noiov_resource' or
'for_each_pci_dev_base_norom_resource'. While abstracting the code and
replacing explicit references to the resources in various peices of code
including the drivers, I just encountered the need for the 'yes' based
iterators like the one that I added.
However if there is a need for 'no' based iterators, it should be easy
to incorporate them using flags. Something like
for_each_pci_resource(dev, res, i, flags)
where flags can be
#define PCI_STD_RES 0x01
#define PCI_ROM_RES 0x02
#define PCI_BRIDGE_RES 0x04
#define PCI_IOV_RES 0x08
#define PCI_ALL_RES PCI_STD_RES|PCI_ROM_RES|PCI_BRIDGE_RES|PCI_IOV_RES
#define PCI_NOSTD_RES PCI_ALL_RES&(^PCI_STD_RES)
#define PCI_NOIOV_RES PCI_ALL_RES&(^PCI_IOV_RES)
so on and so forth
Yinghai if you are ok with this approach, let me code up all the
iterators. You can incorporate your patches based on those iterators and
I can change all my 40+ patches that change various driver sources to
use this iterator.
agree?
RP
next prev parent reply other threads:[~2012-08-16 3:26 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-06-18 5:03 [RFC PATCH] methods to access resources of a struct pci_dev Ram Pai
2012-06-18 18:30 ` Yinghai Lu
2012-06-19 1:46 ` Ram Pai
2012-06-19 2:57 ` Yinghai Lu
2012-08-15 21:25 ` Bjorn Helgaas
2012-08-16 3:26 ` Ram Pai [this message]
2012-08-16 4:11 ` Yinghai Lu
2012-08-16 4:41 ` Ram Pai
2012-08-21 15:13 ` [RFC PATCH v2 ]pci: pci resource iterator Ram Pai
2012-08-21 23:22 ` Yinghai Lu
2012-08-22 10:15 ` Ram Pai
2012-08-22 17:31 ` Yinghai Lu
2012-08-22 17:35 ` Yinghai Lu
2012-08-23 0:28 ` Yinghai Lu
2012-08-23 5:09 ` [RFC PATCH v3 " Ram Pai
2012-08-23 19:30 ` Yinghai Lu
2012-08-27 7:33 ` Ram Pai
2012-09-03 8:07 ` Yinghai Lu
2012-09-03 9:08 ` Ram Pai
2012-09-03 18:20 ` Yinghai Lu
2012-09-04 3:27 ` Ram Pai
2012-09-18 0:03 ` Yinghai Lu
2012-09-21 6:18 ` Ram Pai
2012-09-21 6: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=20120816032602.GN2449@ram-ThinkPad-T61 \
--to=linuxram@us.ibm.com \
--cc=bhelgaas@google.com \
--cc=linux-pci@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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).