From: Yinghai Lu <yinghai@kernel.org>
To: Ram Pai <linuxram@us.ibm.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>, linux-pci@vger.kernel.org
Subject: Re: [RFC PATCH v3 ]pci: pci resource iterator
Date: Mon, 3 Sep 2012 01:07:46 -0700 [thread overview]
Message-ID: <CAE9FiQVEuH=ZwL4R+wyO0H0853hQ8bpGBYKQJwkAJ5g7FKtY7g@mail.gmail.com> (raw)
In-Reply-To: <20120827073335.GE20843@ram-ThinkPad-T61>
On Mon, Aug 27, 2012 at 12:33 AM, Ram Pai <linuxram@us.ibm.com> wrote:
> On Thu, Aug 23, 2012 at 12:30:05PM -0700, Yinghai Lu wrote:
>> Hi, Ram and Bjorn,
>>
>> On Wed, Aug 22, 2012 at 10:09 PM, Ram Pai <linuxram@us.ibm.com> wrote:
>> > +static inline int pci_next_resource_idx(int i, int flag)
>> > +{
>> > + while (++i < PCI_NUM_RESOURCES) {
>> > + if ((i >= 0 && i < PCI_ROM_RESOURCE && (flag & PCI_STD_RES)) ||
>> > + (i == PCI_ROM_RESOURCE && (flag & PCI_ROM_RES)) ||
>> > +#ifdef CONFIG_PCI_IOV
>> > + (i <= PCI_IOV_RESOURCE_END && (flag & PCI_IOV_RES)) ||
>> > +#endif
>> > + (i <= PCI_BRIDGE_RESOURCE_END && (flag & PCI_BRIDGE_RES)))
>> > + return i;
>> > + }
>> > + return -1;
>> > +}
>>
>> no, you can not merge them.
>> when it start as -1, and user only need the bridge resource, it will
>> loop from 0 to 16.
>
> True. But the logic by itself is not wrong. It is sub-optimal.
>
>>
>> I optimized it more to skip some searching. please check v5 and v6.
>> v5 will store aside the mask, and use the bit map later.
>> v6 will still to do the local checking, but will skip some ++i loop.
>
> :) The v5 patch; the bitmask one, took me some time to understand.
> But yes it does a good job.
>
> There is one thing I dont like in that patch. You have to call to
> pci_dev_resource_n(), but that function is not there anywhere in
> upstream tree. Its only in your tree. Can we bring pci_dev_resource_n()
> into this patch and make it self-sustained?
I'd like to keep every patch to be smaller.
>
> Also, please find below, one other patch proposal, which improves upon
> your v5 patch. Instead of having a array of bitmask, this patch
> maintains a single dimensional array with an entry for each resource.
> Each entry captures its type; ie PCI_RES_*, and index of the starting location
> of the next resource type. Using this information one can quickly cut down on
> the number of useless iterations. It also does compile time
> initialization of the array, unlike the v5 patch which does runtime
> initialization.
only one time. and could move the initialization call to other place.
>
> Please decide which version you want and lets take the next step. I
> think we have iterated over this interface quite a lot. Time to move on
> :)
>
>
> ---------------------------------------------------------------------
> Signed-off-by: Ram Pai <linuxram@us.ibm.com>
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 5e1ca3c..d95a31c 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -146,6 +146,54 @@ static int __init pcibus_class_init(void)
> }
> postcore_initcall(pcibus_class_init);
>
> +
> +#define PCI_RES_SHIFT 4
> +/*
> + * capture the starting index of the next resource type
> + * as well as the type of the resource
> + */
> +int res_idx[PCI_NUM_RESOURCES] = {
> +(PCI_ROM_RESOURCE << PCI_RES_SHIFT) | PCI_STD_RES, /* PCI_STD_RESOURCES */
> +(PCI_ROM_RESOURCE << PCI_RES_SHIFT) | PCI_STD_RES, /* PCI_STD_RESOURCES+1 */
> +(PCI_ROM_RESOURCE << PCI_RES_SHIFT) | PCI_STD_RES, /* PCI_STD_RESOURCES+2 */
> +(PCI_ROM_RESOURCE << PCI_RES_SHIFT) | PCI_STD_RES, /* PCI_STD_RESOURCES+3 */
> +(PCI_ROM_RESOURCE << PCI_RES_SHIFT) | PCI_STD_RES, /* PCI_STD_RESOURCES+4 */
> +(PCI_ROM_RESOURCE << PCI_RES_SHIFT) | PCI_STD_RES, /* PCI_STD_RESOURCES+5 */
> +(PCI_IOV_RESOURCES << PCI_RES_SHIFT) | PCI_ROM_RES, /* PCI_ROM_RESOURCE */
> +(PCI_BRIDGE_RESOURCES << PCI_RES_SHIFT) | PCI_IOV_RES, /* PCI_IOV_START */
> +(PCI_BRIDGE_RESOURCES << PCI_RES_SHIFT) | PCI_IOV_RES, /* PCI_IOV_START+1 */
> +(PCI_BRIDGE_RESOURCES << PCI_RES_SHIFT) | PCI_IOV_RES, /* PCI_IOV_START+2 */
> +(PCI_BRIDGE_RESOURCES << PCI_RES_SHIFT) | PCI_IOV_RES, /* PCI_IOV_START+3 */
> +(PCI_BRIDGE_RESOURCES << PCI_RES_SHIFT) | PCI_IOV_RES, /* PCI_IOV_START+4 */
> +(PCI_BRIDGE_RESOURCES << PCI_RES_SHIFT) | PCI_IOV_RES, /* PCI_IOV_START+5 */
> +(PCI_NUM_RESOURCES << PCI_RES_SHIFT) | PCI_BRIDGE_RES, /* PCI_BRIDGE_START */
> +(PCI_NUM_RESOURCES << PCI_RES_SHIFT) | PCI_BRIDGE_RES, /* PCI_BRIDGE_START+1 */
> +(PCI_NUM_RESOURCES << PCI_RES_SHIFT) | PCI_BRIDGE_RES, /* PCI_BRIDGE_START+2 */
> +(PCI_NUM_RESOURCES << PCI_RES_SHIFT) | PCI_BRIDGE_RES, /* PCI_BRIDGE_START+3 */
> +};
how about some one change the some type resource number?
better have some marco helper.
> +#define res_type(idx) (res_idx[idx] & ((0x1 << PCI_RES_SHIFT) - 1))
> +#define res_next(idx) (res_idx[idx] >> PCI_RES_SHIFT)
> +
> +int pci_next_resource_idx(int i, int flag)
> +{
> + i++;
> + while (i < PCI_NUM_RESOURCES) {
> + if (res_type(i) & flag)
> + return i;
> + i = res_next(i);
> + }
here still have some two extra loop when only need to loop STD + BRIDGE.
> + return -1;
> +}
> +
> +struct resource *pci_dev_resource_n(struct pci_dev *dev, int n)
> +{
> + if (n >= 0 && n < PCI_NUM_RESOURCES)
> + return &dev->resource[n];
> +
> + return NULL;
> +}
> +
> +
> static u64 pci_size(u64 base, u64 maxbase, u64 mask)
> {
> u64 size = mask & maxbase; /* Find the significant bits */
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index e444f5b..adae706 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1351,6 +1351,25 @@ static inline int pci_domain_nr(struct pci_bus *bus)
> (pci_resource_end((dev), (bar)) - \
> pci_resource_start((dev), (bar)) + 1))
>
> +#define PCI_STD_RES (1<<0)
> +#define PCI_ROM_RES (1<<1)
> +#define PCI_BRIDGE_RES (1<<2)
> +#define PCI_IOV_RES (1<<3)
> +#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)
> +#define PCI_NOROM_RES (PCI_ALL_RES & ~PCI_ROM_RES)
> +#define PCI_NOBRIDGE_RES (PCI_ALL_RES & ~PCI_BRIDGE_RES)
> +#define PCI_STD_ROM_RES (PCI_STD_RES | PCI_ROM_RES)
> +#define PCI_STD_IOV_RES (PCI_STD_RES | PCI_IOV_RES)
> +#define PCI_STD_ROM_IOV_RES (PCI_NOBRIDGE_RES)
> +#define for_each_pci_resource(dev, res, flag) \
> + for (i = pci_next_resource_idx(-1, flag), \
> + res = pci_dev_resource_n(dev, i); \
> + res; \
> + i = pci_next_resource_idx(i, flag), \
> + res = pci_dev_resource_n(dev, i))
> +
> /* Similar to the helpers above, these manipulate per-pci_dev
> * driver-specific data. They are really just a wrapper around
> * the generic device structure functions of these calls.
>
next prev parent reply other threads:[~2012-09-03 8:07 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
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 [this message]
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='CAE9FiQVEuH=ZwL4R+wyO0H0853hQ8bpGBYKQJwkAJ5g7FKtY7g@mail.gmail.com' \
--to=yinghai@kernel.org \
--cc=bhelgaas@google.com \
--cc=linux-pci@vger.kernel.org \
--cc=linuxram@us.ibm.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
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).