LinuxPPC-Dev Archive on lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH 04/28] proc: Supply PDE attribute setting accessor functions [RFC]
From: Bjorn Helgaas @ 2013-04-18 16:42 UTC (permalink / raw)
  To: David Howells
  Cc: alsa-devel, netdev, linux-wireless, linux-kernel@vger.kernel.org,
	netfilter-devel, Al Viro, linux-pci@vger.kernel.org,
	linux-fsdevel, linuxppc-dev, linux-media
In-Reply-To: <20130416182606.27773.55054.stgit@warthog.procyon.org.uk>

On Tue, Apr 16, 2013 at 12:26 PM, David Howells <dhowells@redhat.com> wrote:
> Supply accessor functions to set attributes in proc_dir_entry structs.
>
> The following are supplied: proc_set_size() and proc_set_user().
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: linuxppc-dev@lists.ozlabs.org
> cc: linux-media@vger.kernel.org
> cc: netdev@vger.kernel.org
> cc: linux-wireless@vger.kernel.org
> cc: linux-pci@vger.kernel.org
> cc: netfilter-devel@vger.kernel.org
> cc: alsa-devel@alsa-project.org
> ---
>
>  arch/powerpc/kernel/proc_powerpc.c        |    2 +-
>  arch/powerpc/platforms/pseries/reconfig.c |    2 +-
>  drivers/media/pci/ttpci/av7110_ir.c       |    2 +-
>  drivers/net/irda/vlsi_ir.c                |    2 +-
>  drivers/net/wireless/airo.c               |   34 +++++++++--------------------
>  drivers/pci/proc.c                        |    2 +-

For the drivers/pci part:

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

>  fs/proc/generic.c                         |   13 +++++++++++
>  include/linux/proc_fs.h                   |    5 ++++
>  kernel/configs.c                          |    2 +-
>  kernel/profile.c                          |    2 +-
>  net/netfilter/xt_recent.c                 |    3 +--
>  sound/core/info.c                         |    2 +-
>  12 files changed, 38 insertions(+), 33 deletions(-)
>
> diff --git a/arch/powerpc/kernel/proc_powerpc.c b/arch/powerpc/kernel/proc_powerpc.c
> index 41d8ee9..feb8580 100644
> --- a/arch/powerpc/kernel/proc_powerpc.c
> +++ b/arch/powerpc/kernel/proc_powerpc.c
> @@ -83,7 +83,7 @@ static int __init proc_ppc64_init(void)
>                                &page_map_fops, vdso_data);
>         if (!pde)
>                 return 1;
> -       pde->size = PAGE_SIZE;
> +       proc_set_size(pde, PAGE_SIZE);
>
>         return 0;
>  }
> diff --git a/arch/powerpc/platforms/pseries/reconfig.c b/arch/powerpc/platforms/pseries/reconfig.c
> index d6491bd..f93cdf5 100644
> --- a/arch/powerpc/platforms/pseries/reconfig.c
> +++ b/arch/powerpc/platforms/pseries/reconfig.c
> @@ -452,7 +452,7 @@ static int proc_ppc64_create_ofdt(void)
>
>         ent = proc_create("powerpc/ofdt", S_IWUSR, NULL, &ofdt_fops);
>         if (ent)
> -               ent->size = 0;
> +               proc_set_size(ent, 0);
>
>         return 0;
>  }
> diff --git a/drivers/media/pci/ttpci/av7110_ir.c b/drivers/media/pci/ttpci/av7110_ir.c
> index eb82286..0e763a7 100644
> --- a/drivers/media/pci/ttpci/av7110_ir.c
> +++ b/drivers/media/pci/ttpci/av7110_ir.c
> @@ -375,7 +375,7 @@ int av7110_ir_init(struct av7110 *av7110)
>         if (av_cnt == 1) {
>                 e = proc_create("av7110_ir", S_IWUSR, NULL, &av7110_ir_proc_fops);
>                 if (e)
> -                       e->size = 4 + 256 * sizeof(u16);
> +                       proc_set_size(e, 4 + 256 * sizeof(u16));
>         }
>
>         tasklet_init(&av7110->ir.ir_tasklet, av7110_emit_key, (unsigned long) &av7110->ir);
> diff --git a/drivers/net/irda/vlsi_ir.c b/drivers/net/irda/vlsi_ir.c
> index e22cd4e..5f47584 100644
> --- a/drivers/net/irda/vlsi_ir.c
> +++ b/drivers/net/irda/vlsi_ir.c
> @@ -1678,7 +1678,7 @@ vlsi_irda_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>                         IRDA_WARNING("%s: failed to create proc entry\n",
>                                      __func__);
>                 } else {
> -                       ent->size = 0;
> +                       proc_set_size(ent, 0);
>                 }
>                 idev->proc_entry = ent;
>         }
> diff --git a/drivers/net/wireless/airo.c b/drivers/net/wireless/airo.c
> index 66e398d..21d0233 100644
> --- a/drivers/net/wireless/airo.c
> +++ b/drivers/net/wireless/airo.c
> @@ -4507,73 +4507,63 @@ static int setup_proc_entry( struct net_device *dev,
>                                             airo_entry);
>         if (!apriv->proc_entry)
>                 goto fail;
> -       apriv->proc_entry->uid = proc_kuid;
> -       apriv->proc_entry->gid = proc_kgid;
> +       proc_set_user(apriv->proc_entry, proc_kuid, proc_kgid);
>
>         /* Setup the StatsDelta */
>         entry = proc_create_data("StatsDelta", S_IRUGO & proc_perm,
>                                  apriv->proc_entry, &proc_statsdelta_ops, dev);
>         if (!entry)
>                 goto fail_stats_delta;
> -       entry->uid = proc_kuid;
> -       entry->gid = proc_kgid;
> +       proc_set_user(entry, proc_kuid, proc_kgid);
>
>         /* Setup the Stats */
>         entry = proc_create_data("Stats", S_IRUGO & proc_perm,
>                                  apriv->proc_entry, &proc_stats_ops, dev);
>         if (!entry)
>                 goto fail_stats;
> -       entry->uid = proc_kuid;
> -       entry->gid = proc_kgid;
> +       proc_set_user(entry, proc_kuid, proc_kgid);
>
>         /* Setup the Status */
>         entry = proc_create_data("Status", S_IRUGO & proc_perm,
>                                  apriv->proc_entry, &proc_status_ops, dev);
>         if (!entry)
>                 goto fail_status;
> -       entry->uid = proc_kuid;
> -       entry->gid = proc_kgid;
> +       proc_set_user(entry, proc_kuid, proc_kgid);
>
>         /* Setup the Config */
>         entry = proc_create_data("Config", proc_perm,
>                                  apriv->proc_entry, &proc_config_ops, dev);
>         if (!entry)
>                 goto fail_config;
> -       entry->uid = proc_kuid;
> -       entry->gid = proc_kgid;
> +       proc_set_user(entry, proc_kuid, proc_kgid);
>
>         /* Setup the SSID */
>         entry = proc_create_data("SSID", proc_perm,
>                                  apriv->proc_entry, &proc_SSID_ops, dev);
>         if (!entry)
>                 goto fail_ssid;
> -       entry->uid = proc_kuid;
> -       entry->gid = proc_kgid;
> +       proc_set_user(entry, proc_kuid, proc_kgid);
>
>         /* Setup the APList */
>         entry = proc_create_data("APList", proc_perm,
>                                  apriv->proc_entry, &proc_APList_ops, dev);
>         if (!entry)
>                 goto fail_aplist;
> -       entry->uid = proc_kuid;
> -       entry->gid = proc_kgid;
> +       proc_set_user(entry, proc_kuid, proc_kgid);
>
>         /* Setup the BSSList */
>         entry = proc_create_data("BSSList", proc_perm,
>                                  apriv->proc_entry, &proc_BSSList_ops, dev);
>         if (!entry)
>                 goto fail_bsslist;
> -       entry->uid = proc_kuid;
> -       entry->gid = proc_kgid;
> +       proc_set_user(entry, proc_kuid, proc_kgid);
>
>         /* Setup the WepKey */
>         entry = proc_create_data("WepKey", proc_perm,
>                                  apriv->proc_entry, &proc_wepkey_ops, dev);
>         if (!entry)
>                 goto fail_wepkey;
> -       entry->uid = proc_kuid;
> -       entry->gid = proc_kgid;
> -
> +       proc_set_user(entry, proc_kuid, proc_kgid);
>         return 0;
>
>  fail_wepkey:
> @@ -5695,10 +5685,8 @@ static int __init airo_init_module( void )
>
>         airo_entry = proc_mkdir_mode("driver/aironet", airo_perm, NULL);
>
> -       if (airo_entry) {
> -               airo_entry->uid = proc_kuid;
> -               airo_entry->gid = proc_kgid;
> -       }
> +       if (airo_entry)
> +               proc_set_user(airo_entry, proc_kuid, proc_kgid);
>
>         for (i = 0; i < 4 && io[i] && irq[i]; i++) {
>                 airo_print_info("", "Trying to configure ISA adapter at irq=%d "
> diff --git a/drivers/pci/proc.c b/drivers/pci/proc.c
> index 12e4fb5..7cde7c1 100644
> --- a/drivers/pci/proc.c
> +++ b/drivers/pci/proc.c
> @@ -419,7 +419,7 @@ int pci_proc_attach_device(struct pci_dev *dev)
>                              &proc_bus_pci_operations, dev);
>         if (!e)
>                 return -ENOMEM;
> -       e->size = dev->cfg_size;
> +       proc_set_size(e, dev->cfg_size);
>         dev->procent = e;
>
>         return 0;
> diff --git a/fs/proc/generic.c b/fs/proc/generic.c
> index 1c07cad..5f6f6c3 100644
> --- a/fs/proc/generic.c
> +++ b/fs/proc/generic.c
> @@ -498,6 +498,19 @@ out:
>         return NULL;
>  }
>  EXPORT_SYMBOL(proc_create_data);
> +
> +void proc_set_size(struct proc_dir_entry *de, loff_t size)
> +{
> +       de->size = size;
> +}
> +EXPORT_SYMBOL(proc_set_size);
> +
> +void proc_set_user(struct proc_dir_entry *de, kuid_t uid, kgid_t gid)
> +{
> +       de->uid = uid;
> +       de->gid = gid;
> +}
> +EXPORT_SYMBOL(proc_set_user);
>
>  static void free_proc_entry(struct proc_dir_entry *de)
>  {
> diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
> index 805edac..28a4d7e 100644
> --- a/include/linux/proc_fs.h
> +++ b/include/linux/proc_fs.h
> @@ -130,6 +130,9 @@ static inline struct proc_dir_entry *proc_create(const char *name, umode_t mode,
>  extern struct proc_dir_entry *proc_net_mkdir(struct net *net, const char *name,
>         struct proc_dir_entry *parent);
>
> +extern void proc_set_size(struct proc_dir_entry *, loff_t);
> +extern void proc_set_user(struct proc_dir_entry *, kuid_t, kgid_t);
> +
>  extern struct file *proc_ns_fget(int fd);
>  extern bool proc_ns_inode(struct inode *inode);
>
> @@ -158,6 +161,8 @@ static inline struct proc_dir_entry *proc_mkdir(const char *name,
>         struct proc_dir_entry *parent) {return NULL;}
>  static inline struct proc_dir_entry *proc_mkdir_mode(const char *name,
>         umode_t mode, struct proc_dir_entry *parent) { return NULL; }
> +static inline void proc_set_size(struct proc_dir_entry *de, loff_t size) {}
> +static inline void proc_set_user(struct proc_dir_entry *de, kuid_t uid, kgid_t gid) {}
>
>  struct tty_driver;
>  static inline void proc_tty_register_driver(struct tty_driver *driver) {};
> diff --git a/kernel/configs.c b/kernel/configs.c
> index 42e8fa0..c18b1f1 100644
> --- a/kernel/configs.c
> +++ b/kernel/configs.c
> @@ -79,7 +79,7 @@ static int __init ikconfig_init(void)
>         if (!entry)
>                 return -ENOMEM;
>
> -       entry->size = kernel_config_data_size;
> +       proc_set_size(entry, kernel_config_data_size);
>
>         return 0;
>  }
> diff --git a/kernel/profile.c b/kernel/profile.c
> index 524ce5e..0bf4007 100644
> --- a/kernel/profile.c
> +++ b/kernel/profile.c
> @@ -600,7 +600,7 @@ int __ref create_proc_profile(void) /* false positive from hotcpu_notifier */
>                             NULL, &proc_profile_operations);
>         if (!entry)
>                 return 0;
> -       entry->size = (1+prof_len) * sizeof(atomic_t);
> +       proc_set_size(entry, (1 + prof_len) * sizeof(atomic_t));
>         hotcpu_notifier(profile_cpu_callback, 0);
>         return 0;
>  }
> diff --git a/net/netfilter/xt_recent.c b/net/netfilter/xt_recent.c
> index 3db2d38..1e657cf 100644
> --- a/net/netfilter/xt_recent.c
> +++ b/net/netfilter/xt_recent.c
> @@ -401,8 +401,7 @@ static int recent_mt_check(const struct xt_mtchk_param *par,
>                 ret = -ENOMEM;
>                 goto out;
>         }
> -       pde->uid = uid;
> -       pde->gid = gid;
> +       proc_set_user(pde, uid, gid);
>  #endif
>         spin_lock_bh(&recent_lock);
>         list_add_tail(&t->list, &recent_net->tables);
> diff --git a/sound/core/info.c b/sound/core/info.c
> index 3aa8864..c7f41c3 100644
> --- a/sound/core/info.c
> +++ b/sound/core/info.c
> @@ -970,7 +970,7 @@ int snd_info_register(struct snd_info_entry * entry)
>                         mutex_unlock(&info_mutex);
>                         return -ENOMEM;
>                 }
> -               p->size = entry->size;
> +               proc_set_size(p, entry->size);
>         }
>         entry->p = p;
>         if (entry->parent)
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v7 2/3] of/pci: Provide support for parsing PCI DT ranges property
From: Andrew Murray @ 2013-04-18 15:38 UTC (permalink / raw)
  To: Grant Likely
  Cc: linux-mips@linux-mips.org, siva.kallam@samsung.com,
	linux-pci@vger.kernel.org, linus.walleij@linaro.org,
	thierry.reding@avionic-design.de, Liviu Dudau, paulus@samba.org,
	linux-samsung-soc@vger.kernel.org, linux@arm.linux.org.uk,
	jg1.han@samsung.com, jgunthorpe@obsidianresearch.com,
	thomas.abraham@linaro.org, arnd@arndb.de,
	devicetree-discuss@lists.ozlabs.org, rob.herring@calxeda.com,
	kgene.kim@samsung.com, bhelgaas@google.com,
	linux-arm-kernel@lists.infradead.org,
	thomas.petazzoni@free-electrons.com, monstr@monstr.eu,
	linux-kernel@vger.kernel.org, suren.reddy@samsung.com,
	linuxppc-dev@lists.ozlabs.org
In-Reply-To: <CACxGe6u7yoecyTR2r-EzpcRxLuSw-p9KC7jA12wEQZs4of3vFA@mail.gmail.com>

On Thu, Apr 18, 2013 at 04:29:54PM +0100, Grant Likely wrote:
> On Thu, Apr 18, 2013 at 3:24 PM, Andrew Murray <andrew.murray@arm.com> wrote:
> > On Thu, Apr 18, 2013 at 02:44:01PM +0100, Grant Likely wrote:
> >> On Tue, 16 Apr 2013 11:18:27 +0100, Andrew Murray <Andrew.Murray@arm.com> wrote:
> >> >             /* Act based on address space type */
> >> >             res = NULL;
> >> > -           switch ((pci_space >> 24) & 0x3) {
> >> > -           case 1:         /* PCI IO space */
> >> > +           res_type = range.flags & IORESOURCE_TYPE_BITS;
> >> > +           if (res_type == IORESOURCE_IO) {
> >>
> >> Why the change from switch() to an if/else if sequence?
> >
> > Russell King suggested this change - see
> > https://patchwork.kernel.org/patch/2323941/
> 
> Umm, that isn't what that link shows. That link shows the patch
> already changing to an if/else if construct, and rmk is commenting on
> that.

Arh yes sorry about that. I can't find or remember any good reason why I
changed it from a switch to an if/else :\

Andrew Murray

^ permalink raw reply

* Re: [PATCH v7 2/3] of/pci: Provide support for parsing PCI DT ranges property
From: Grant Likely @ 2013-04-18 15:29 UTC (permalink / raw)
  To: Andrew Murray
  Cc: linux-mips@linux-mips.org, siva.kallam@samsung.com,
	linux-pci@vger.kernel.org, linus.walleij@linaro.org,
	thierry.reding@avionic-design.de, Liviu Dudau, paulus@samba.org,
	linux-samsung-soc@vger.kernel.org, linux@arm.linux.org.uk,
	jg1.han@samsung.com, jgunthorpe@obsidianresearch.com,
	thomas.abraham@linaro.org, arnd@arndb.de,
	devicetree-discuss@lists.ozlabs.org, rob.herring@calxeda.com,
	kgene.kim@samsung.com, bhelgaas@google.com,
	linux-arm-kernel@lists.infradead.org,
	thomas.petazzoni@free-electrons.com, monstr@monstr.eu,
	linux-kernel@vger.kernel.org, suren.reddy@samsung.com,
	linuxppc-dev@lists.ozlabs.org
In-Reply-To: <20130418142434.GA18881@arm.com>

On Thu, Apr 18, 2013 at 3:24 PM, Andrew Murray <andrew.murray@arm.com> wrote:
> On Thu, Apr 18, 2013 at 02:44:01PM +0100, Grant Likely wrote:
>> On Tue, 16 Apr 2013 11:18:27 +0100, Andrew Murray <Andrew.Murray@arm.com> wrote:
>> >             /* Act based on address space type */
>> >             res = NULL;
>> > -           switch ((pci_space >> 24) & 0x3) {
>> > -           case 1:         /* PCI IO space */
>> > +           res_type = range.flags & IORESOURCE_TYPE_BITS;
>> > +           if (res_type == IORESOURCE_IO) {
>>
>> Why the change from switch() to an if/else if sequence?
>
> Russell King suggested this change - see
> https://patchwork.kernel.org/patch/2323941/

Umm, that isn't what that link shows. That link shows the patch
already changing to an if/else if construct, and rmk is commenting on
that.

g.

^ permalink raw reply

* Re: [PATCH v7 3/3] of/pci: mips: convert to common of_pci_range_parser
From: Andrew Murray @ 2013-04-18 14:26 UTC (permalink / raw)
  To: Grant Likely
  Cc: linux-mips@linux-mips.org, siva.kallam@samsung.com,
	linux-pci@vger.kernel.org, linus.walleij@linaro.org,
	thierry.reding@avionic-design.de, Liviu Dudau, paulus@samba.org,
	linux-samsung-soc@vger.kernel.org, linux@arm.linux.org.uk,
	jg1.han@samsung.com, jgunthorpe@obsidianresearch.com,
	thomas.abraham@linaro.org, arnd@arndb.de,
	devicetree-discuss@lists.ozlabs.org, rob.herring@calxeda.com,
	kgene.kim@samsung.com, bhelgaas@google.com,
	linux-arm-kernel@lists.infradead.org,
	thomas.petazzoni@free-electrons.com, monstr@monstr.eu,
	linux-kernel@vger.kernel.org, suren.reddy@samsung.com,
	linuxppc-dev@lists.ozlabs.org
In-Reply-To: <20130418134535.5189E3E1319@localhost>

On Thu, Apr 18, 2013 at 02:45:35PM +0100, Grant Likely wrote:
> On Tue, 16 Apr 2013 11:18:28 +0100, Andrew Murray <Andrew.Murray@arm.com> wrote:
> > This patch converts the pci_load_of_ranges function to use the new common
> > of_pci_range_parser.
> > 
> > Signed-off-by: Andrew Murray <Andrew.Murray@arm.com>
> > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> > Reviewed-by: Rob Herring <rob.herring@calxeda.com>
> 
> Looks okay to me, and thank you very much for doing the legwork to merge
> the common code.

No problem, though I think there is more than can be done in this area -
there is a lot of similar PCI code floating about.

> 
> Reviewed-by: Grant Likely <grant.likely@secretlab.ca>

Thanks for the review.

Andrew Murray

^ permalink raw reply

* Re: [PATCH v7 2/3] of/pci: Provide support for parsing PCI DT ranges property
From: Andrew Murray @ 2013-04-18 14:24 UTC (permalink / raw)
  To: Grant Likely
  Cc: linux-mips@linux-mips.org, siva.kallam@samsung.com,
	linux-pci@vger.kernel.org, linus.walleij@linaro.org,
	thierry.reding@avionic-design.de, Liviu Dudau, paulus@samba.org,
	linux-samsung-soc@vger.kernel.org, linux@arm.linux.org.uk,
	jg1.han@samsung.com, jgunthorpe@obsidianresearch.com,
	thomas.abraham@linaro.org, arnd@arndb.de,
	devicetree-discuss@lists.ozlabs.org, rob.herring@calxeda.com,
	kgene.kim@samsung.com, bhelgaas@google.com,
	linux-arm-kernel@lists.infradead.org,
	thomas.petazzoni@free-electrons.com, monstr@monstr.eu,
	linux-kernel@vger.kernel.org, suren.reddy@samsung.com,
	linuxppc-dev@lists.ozlabs.org
In-Reply-To: <20130418134401.84AEE3E1319@localhost>

On Thu, Apr 18, 2013 at 02:44:01PM +0100, Grant Likely wrote:
> On Tue, 16 Apr 2013 11:18:27 +0100, Andrew Murray <Andrew.Murray@arm.com> wrote:
> > This patch factors out common implementation patterns to reduce overall kernel
> > code and provide a means for host bridge drivers to directly obtain struct
> > resources from the DT's ranges property without relying on architecture specific
> > DT handling. This will make it easier to write archiecture independent host bridge
> > drivers and mitigate against further duplication of DT parsing code.
> > 
> > This patch can be used in the following way:
> > 
> > 	struct of_pci_range_parser parser;
> > 	struct of_pci_range range;
> > 
> > 	if (of_pci_range_parser(&parser, np))
> > 		; //no ranges property
> > 
> > 	for_each_of_pci_range(&parser, &range) {
> > 
> > 		/*
> > 			directly access properties of the address range, e.g.:
> > 			range.pci_space, range.pci_addr, range.cpu_addr,
> > 			range.size, range.flags
> > 
> > 			alternatively obtain a struct resource, e.g.:
> > 			struct resource res;
> > 			of_pci_range_to_resource(&range, np, &res);
> > 		*/
> > 	}
> > 
> > Additionally the implementation takes care of adjacent ranges and merges them
> > into a single range (as was the case with powerpc and microblaze).
> > 
> > Signed-off-by: Andrew Murray <Andrew.Murray@arm.com>
> > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> > Reviewed-by: Rob Herring <rob.herring@calxeda.com>
> > Tested-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> > Tested-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Acked-by: Grant Likely <grant.likely@secretlab.ca>
> 
> But comments below...
> 
> > ---
> >  drivers/of/address.c       |   67 ++++++++++++++++++++++++++
> >  drivers/of/of_pci.c        |  113 ++++++++++++++++----------------------------
> >  include/linux/of_address.h |   46 ++++++++++++++++++
> >  3 files changed, 154 insertions(+), 72 deletions(-)
> > 
> > diff --git a/drivers/of/address.c b/drivers/of/address.c
> > index 04da786..6eec70c 100644
> > --- a/drivers/of/address.c
> > +++ b/drivers/of/address.c
> > @@ -227,6 +227,73 @@ int of_pci_address_to_resource(struct device_node *dev, int bar,
> >  	return __of_address_to_resource(dev, addrp, size, flags, NULL, r);
> >  }
> >  EXPORT_SYMBOL_GPL(of_pci_address_to_resource);
> > +
> > +int of_pci_range_parser(struct of_pci_range_parser *parser,
> > +			struct device_node *node)
> > +{
> > +	const int na = 3, ns = 2;
> > +	int rlen;
> > +
> > +	parser->node = node;
> > +	parser->pna = of_n_addr_cells(node);
> > +	parser->np = parser->pna + na + ns;
> > +
> > +	parser->range = of_get_property(node, "ranges", &rlen);
> > +	if (parser->range == NULL)
> > +		return -ENOENT;
> > +
> > +	parser->end = parser->range + rlen / sizeof(__be32);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(of_pci_range_parser);
> 
> "of_pci_range_parser_init" would be a clearer name
> 
> > +struct of_pci_range *of_pci_process_ranges(struct of_pci_range_parser *parser,
> > +						struct of_pci_range *range)
> 
> Similarly, "of_pci_range_parser_one" would be more consistent.
> 
> > +{
> > +	const int na = 3, ns = 2;
> > +
> > +	if (!range)
> > +		return NULL;
> > +
> > +	if (!parser->range || parser->range + parser->np > parser->end)
> > +		return NULL;
> > +
> > +	range->pci_space = parser->range[0];
> > +	range->flags = of_bus_pci_get_flags(parser->range);
> > +	range->pci_addr = of_read_number(parser->range + 1, ns);
> > +	range->cpu_addr = of_translate_address(parser->node,
> > +				parser->range + na);
> > +	range->size = of_read_number(parser->range + parser->pna + na, ns);
> > +
> > +	parser->range += parser->np;
> > +
> > +	/* Now consume following elements while they are contiguous */
> > +	while (parser->range + parser->np <= parser->end) {
> > +		u32 flags, pci_space;
> > +		u64 pci_addr, cpu_addr, size;
> > +
> > +		pci_space = be32_to_cpup(parser->range);
> > +		flags = of_bus_pci_get_flags(parser->range);
> > +		pci_addr = of_read_number(parser->range + 1, ns);
> > +		cpu_addr = of_translate_address(parser->node,
> > +				parser->range + na);
> > +		size = of_read_number(parser->range + parser->pna + na, ns);
> > +
> > +		if (flags != range->flags)
> > +			break;
> > +		if (pci_addr != range->pci_addr + range->size ||
> > +		    cpu_addr != range->cpu_addr + range->size)
> > +			break;
> > +
> > +		range->size += size;
> > +		parser->range += parser->np;
> > +	}
> > +
> > +	return range;
> > +}
> > +EXPORT_SYMBOL_GPL(of_pci_process_ranges);
> > +
> >  #endif /* CONFIG_PCI */
> >  
> >  /*
> > diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
> > index 1626172..e5ab604 100644
> > --- a/drivers/of/of_pci.c
> > +++ b/drivers/of/of_pci.c
> > @@ -2,6 +2,7 @@
> >  #include <linux/export.h>
> >  #include <linux/of.h>
> >  #include <linux/of_pci.h>
> > +#include <linux/of_address.h>
> >  #include <asm/prom.h>
> >  
> >  #if defined(CONFIG_PPC32) || defined(CONFIG_PPC64) || defined(CONFIG_MICROBLAZE)
> > @@ -82,67 +83,43 @@ EXPORT_SYMBOL_GPL(of_pci_find_child_device);
> >  void pci_process_bridge_OF_ranges(struct pci_controller *hose,
> >  				  struct device_node *dev, int primary)
> >  {
> > -	const u32 *ranges;
> > -	int rlen;
> > -	int pna = of_n_addr_cells(dev);
> > -	int np = pna + 5;
> >  	int memno = 0, isa_hole = -1;
> > -	u32 pci_space;
> > -	unsigned long long pci_addr, cpu_addr, pci_next, cpu_next, size;
> >  	unsigned long long isa_mb = 0;
> >  	struct resource *res;
> > +	struct of_pci_range range;
> > +	struct of_pci_range_parser parser;
> > +	u32 res_type;
> >  
> >  	pr_info("PCI host bridge %s %s ranges:\n",
> >  	       dev->full_name, primary ? "(primary)" : "");
> >  
> > -	/* Get ranges property */
> > -	ranges = of_get_property(dev, "ranges", &rlen);
> > -	if (ranges == NULL)
> > +	/* Check for ranges property */
> > +	if (of_pci_range_parser(&parser, dev))
> >  		return;
> >  
> > -	/* Parse it */
> >  	pr_debug("Parsing ranges property...\n");
> > -	while ((rlen -= np * 4) >= 0) {
> > +	for_each_of_pci_range(&parser, &range) {
> >  		/* Read next ranges element */
> > -		pci_space = ranges[0];
> > -		pci_addr = of_read_number(ranges + 1, 2);
> > -		cpu_addr = of_translate_address(dev, ranges + 3);
> > -		size = of_read_number(ranges + pna + 3, 2);
> 
> Tip: the diff on this function would be a whole lot simpler if the
> above locals were kept, but updated from the ranges structure. Not at
> all a big problem, but it is something that makes changes like this
> easier to review. The removal of the locals could also be split into a
> separate patch to end up with the same result.
> 
> > -
> > -		pr_debug("pci_space: 0x%08x pci_addr:0x%016llx ",
> > -				pci_space, pci_addr);
> > -		pr_debug("cpu_addr:0x%016llx size:0x%016llx\n",
> > -					cpu_addr, size);
> > -
> > -		ranges += np;
> > +		pr_debug("pci_space: 0x%08x pci_addr: 0x%016llx ",
> > +				range.pci_space, range.pci_addr);
> > +		pr_debug("cpu_addr: 0x%016llx size: 0x%016llx\n",
> > +				range.cpu_addr, range.size);
> 
> Nit: the patch changed whitespace on the pr_debug() statements, so even
> though the first line of each is identical, they look different in the
> patch.
> 
> >  
> >  		/* If we failed translation or got a zero-sized region
> >  		 * (some FW try to feed us with non sensical zero sized regions
> >  		 * such as power3 which look like some kind of attempt
> >  		 * at exposing the VGA memory hole)
> >  		 */
> > -		if (cpu_addr == OF_BAD_ADDR || size == 0)
> > +		if (range.cpu_addr == OF_BAD_ADDR || range.size == 0)
> >  			continue;
> 
> Can this also be rolled into the parsing iterator?
> 

Yes I think that would make sense.

> >  
> > -		/* Now consume following elements while they are contiguous */
> > -		for (; rlen >= np * sizeof(u32);
> > -		     ranges += np, rlen -= np * 4) {
> > -			if (ranges[0] != pci_space)
> > -				break;
> > -			pci_next = of_read_number(ranges + 1, 2);
> > -			cpu_next = of_translate_address(dev, ranges + 3);
> > -			if (pci_next != pci_addr + size ||
> > -			    cpu_next != cpu_addr + size)
> > -				break;
> > -			size += of_read_number(ranges + pna + 3, 2);
> > -		}
> > -
> >  		/* Act based on address space type */
> >  		res = NULL;
> > -		switch ((pci_space >> 24) & 0x3) {
> > -		case 1:		/* PCI IO space */
> > +		res_type = range.flags & IORESOURCE_TYPE_BITS;
> > +		if (res_type == IORESOURCE_IO) {
> 
> Why the change from switch() to an if/else if sequence?

Russell King suggested this change - see 
https://patchwork.kernel.org/patch/2323941/

> 
> But those are mostly nitpicks. If this is deferred to v3.10 then I would
> suggest fixing them up and posting for another round of review.

Thanks for the feedback. I'll update the patch in the future.

Andrew Murray

^ permalink raw reply

* Re: [PATCH v7 3/3] of/pci: mips: convert to common of_pci_range_parser
From: Grant Likely @ 2013-04-18 13:45 UTC (permalink / raw)
  To: Andrew Murray, linux-mips, linuxppc-dev
  Cc: siva.kallam, linux-pci, linus.walleij, thierry.reding,
	Liviu.Dudau, paulus, linux-samsung-soc, linux, jg1.han,
	jgunthorpe, thomas.abraham, arnd, devicetree-discuss, rob.herring,
	kgene.kim, bhelgaas, linux-arm-kernel, thomas.petazzoni, monstr,
	linux-kernel, suren.reddy, Andrew Murray
In-Reply-To: <1366107508-12672-4-git-send-email-Andrew.Murray@arm.com>

On Tue, 16 Apr 2013 11:18:28 +0100, Andrew Murray <Andrew.Murray@arm.com> wrote:
> This patch converts the pci_load_of_ranges function to use the new common
> of_pci_range_parser.
> 
> Signed-off-by: Andrew Murray <Andrew.Murray@arm.com>
> Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> Reviewed-by: Rob Herring <rob.herring@calxeda.com>

Looks okay to me, and thank you very much for doing the legwork to merge
the common code.

Reviewed-by: Grant Likely <grant.likely@secretlab.ca>

^ permalink raw reply

* Re: [PATCH v7 2/3] of/pci: Provide support for parsing PCI DT ranges property
From: Grant Likely @ 2013-04-18 13:44 UTC (permalink / raw)
  To: Andrew Murray, linux-mips, linuxppc-dev
  Cc: siva.kallam, linux-pci, linus.walleij, thierry.reding,
	Liviu.Dudau, paulus, linux-samsung-soc, linux, jg1.han,
	jgunthorpe, thomas.abraham, arnd, devicetree-discuss, rob.herring,
	kgene.kim, bhelgaas, linux-arm-kernel, thomas.petazzoni, monstr,
	linux-kernel, suren.reddy, Andrew Murray
In-Reply-To: <1366107508-12672-3-git-send-email-Andrew.Murray@arm.com>

On Tue, 16 Apr 2013 11:18:27 +0100, Andrew Murray <Andrew.Murray@arm.com> wrote:
> This patch factors out common implementation patterns to reduce overall kernel
> code and provide a means for host bridge drivers to directly obtain struct
> resources from the DT's ranges property without relying on architecture specific
> DT handling. This will make it easier to write archiecture independent host bridge
> drivers and mitigate against further duplication of DT parsing code.
> 
> This patch can be used in the following way:
> 
> 	struct of_pci_range_parser parser;
> 	struct of_pci_range range;
> 
> 	if (of_pci_range_parser(&parser, np))
> 		; //no ranges property
> 
> 	for_each_of_pci_range(&parser, &range) {
> 
> 		/*
> 			directly access properties of the address range, e.g.:
> 			range.pci_space, range.pci_addr, range.cpu_addr,
> 			range.size, range.flags
> 
> 			alternatively obtain a struct resource, e.g.:
> 			struct resource res;
> 			of_pci_range_to_resource(&range, np, &res);
> 		*/
> 	}
> 
> Additionally the implementation takes care of adjacent ranges and merges them
> into a single range (as was the case with powerpc and microblaze).
> 
> Signed-off-by: Andrew Murray <Andrew.Murray@arm.com>
> Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Reviewed-by: Rob Herring <rob.herring@calxeda.com>
> Tested-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Tested-by: Linus Walleij <linus.walleij@linaro.org>

Acked-by: Grant Likely <grant.likely@secretlab.ca>

But comments below...

> ---
>  drivers/of/address.c       |   67 ++++++++++++++++++++++++++
>  drivers/of/of_pci.c        |  113 ++++++++++++++++----------------------------
>  include/linux/of_address.h |   46 ++++++++++++++++++
>  3 files changed, 154 insertions(+), 72 deletions(-)
> 
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 04da786..6eec70c 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -227,6 +227,73 @@ int of_pci_address_to_resource(struct device_node *dev, int bar,
>  	return __of_address_to_resource(dev, addrp, size, flags, NULL, r);
>  }
>  EXPORT_SYMBOL_GPL(of_pci_address_to_resource);
> +
> +int of_pci_range_parser(struct of_pci_range_parser *parser,
> +			struct device_node *node)
> +{
> +	const int na = 3, ns = 2;
> +	int rlen;
> +
> +	parser->node = node;
> +	parser->pna = of_n_addr_cells(node);
> +	parser->np = parser->pna + na + ns;
> +
> +	parser->range = of_get_property(node, "ranges", &rlen);
> +	if (parser->range == NULL)
> +		return -ENOENT;
> +
> +	parser->end = parser->range + rlen / sizeof(__be32);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_pci_range_parser);

"of_pci_range_parser_init" would be a clearer name

> +struct of_pci_range *of_pci_process_ranges(struct of_pci_range_parser *parser,
> +						struct of_pci_range *range)

Similarly, "of_pci_range_parser_one" would be more consistent.

> +{
> +	const int na = 3, ns = 2;
> +
> +	if (!range)
> +		return NULL;
> +
> +	if (!parser->range || parser->range + parser->np > parser->end)
> +		return NULL;
> +
> +	range->pci_space = parser->range[0];
> +	range->flags = of_bus_pci_get_flags(parser->range);
> +	range->pci_addr = of_read_number(parser->range + 1, ns);
> +	range->cpu_addr = of_translate_address(parser->node,
> +				parser->range + na);
> +	range->size = of_read_number(parser->range + parser->pna + na, ns);
> +
> +	parser->range += parser->np;
> +
> +	/* Now consume following elements while they are contiguous */
> +	while (parser->range + parser->np <= parser->end) {
> +		u32 flags, pci_space;
> +		u64 pci_addr, cpu_addr, size;
> +
> +		pci_space = be32_to_cpup(parser->range);
> +		flags = of_bus_pci_get_flags(parser->range);
> +		pci_addr = of_read_number(parser->range + 1, ns);
> +		cpu_addr = of_translate_address(parser->node,
> +				parser->range + na);
> +		size = of_read_number(parser->range + parser->pna + na, ns);
> +
> +		if (flags != range->flags)
> +			break;
> +		if (pci_addr != range->pci_addr + range->size ||
> +		    cpu_addr != range->cpu_addr + range->size)
> +			break;
> +
> +		range->size += size;
> +		parser->range += parser->np;
> +	}
> +
> +	return range;
> +}
> +EXPORT_SYMBOL_GPL(of_pci_process_ranges);
> +
>  #endif /* CONFIG_PCI */
>  
>  /*
> diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
> index 1626172..e5ab604 100644
> --- a/drivers/of/of_pci.c
> +++ b/drivers/of/of_pci.c
> @@ -2,6 +2,7 @@
>  #include <linux/export.h>
>  #include <linux/of.h>
>  #include <linux/of_pci.h>
> +#include <linux/of_address.h>
>  #include <asm/prom.h>
>  
>  #if defined(CONFIG_PPC32) || defined(CONFIG_PPC64) || defined(CONFIG_MICROBLAZE)
> @@ -82,67 +83,43 @@ EXPORT_SYMBOL_GPL(of_pci_find_child_device);
>  void pci_process_bridge_OF_ranges(struct pci_controller *hose,
>  				  struct device_node *dev, int primary)
>  {
> -	const u32 *ranges;
> -	int rlen;
> -	int pna = of_n_addr_cells(dev);
> -	int np = pna + 5;
>  	int memno = 0, isa_hole = -1;
> -	u32 pci_space;
> -	unsigned long long pci_addr, cpu_addr, pci_next, cpu_next, size;
>  	unsigned long long isa_mb = 0;
>  	struct resource *res;
> +	struct of_pci_range range;
> +	struct of_pci_range_parser parser;
> +	u32 res_type;
>  
>  	pr_info("PCI host bridge %s %s ranges:\n",
>  	       dev->full_name, primary ? "(primary)" : "");
>  
> -	/* Get ranges property */
> -	ranges = of_get_property(dev, "ranges", &rlen);
> -	if (ranges == NULL)
> +	/* Check for ranges property */
> +	if (of_pci_range_parser(&parser, dev))
>  		return;
>  
> -	/* Parse it */
>  	pr_debug("Parsing ranges property...\n");
> -	while ((rlen -= np * 4) >= 0) {
> +	for_each_of_pci_range(&parser, &range) {
>  		/* Read next ranges element */
> -		pci_space = ranges[0];
> -		pci_addr = of_read_number(ranges + 1, 2);
> -		cpu_addr = of_translate_address(dev, ranges + 3);
> -		size = of_read_number(ranges + pna + 3, 2);

Tip: the diff on this function would be a whole lot simpler if the
above locals were kept, but updated from the ranges structure. Not at
all a big problem, but it is something that makes changes like this
easier to review. The removal of the locals could also be split into a
separate patch to end up with the same result.

> -
> -		pr_debug("pci_space: 0x%08x pci_addr:0x%016llx ",
> -				pci_space, pci_addr);
> -		pr_debug("cpu_addr:0x%016llx size:0x%016llx\n",
> -					cpu_addr, size);
> -
> -		ranges += np;
> +		pr_debug("pci_space: 0x%08x pci_addr: 0x%016llx ",
> +				range.pci_space, range.pci_addr);
> +		pr_debug("cpu_addr: 0x%016llx size: 0x%016llx\n",
> +				range.cpu_addr, range.size);

Nit: the patch changed whitespace on the pr_debug() statements, so even
though the first line of each is identical, they look different in the
patch.

>  
>  		/* If we failed translation or got a zero-sized region
>  		 * (some FW try to feed us with non sensical zero sized regions
>  		 * such as power3 which look like some kind of attempt
>  		 * at exposing the VGA memory hole)
>  		 */
> -		if (cpu_addr == OF_BAD_ADDR || size == 0)
> +		if (range.cpu_addr == OF_BAD_ADDR || range.size == 0)
>  			continue;

Can this also be rolled into the parsing iterator?

>  
> -		/* Now consume following elements while they are contiguous */
> -		for (; rlen >= np * sizeof(u32);
> -		     ranges += np, rlen -= np * 4) {
> -			if (ranges[0] != pci_space)
> -				break;
> -			pci_next = of_read_number(ranges + 1, 2);
> -			cpu_next = of_translate_address(dev, ranges + 3);
> -			if (pci_next != pci_addr + size ||
> -			    cpu_next != cpu_addr + size)
> -				break;
> -			size += of_read_number(ranges + pna + 3, 2);
> -		}
> -
>  		/* Act based on address space type */
>  		res = NULL;
> -		switch ((pci_space >> 24) & 0x3) {
> -		case 1:		/* PCI IO space */
> +		res_type = range.flags & IORESOURCE_TYPE_BITS;
> +		if (res_type == IORESOURCE_IO) {

Why the change from switch() to an if/else if sequence?

But those are mostly nitpicks. If this is deferred to v3.10 then I would
suggest fixing them up and posting for another round of review.

g.

^ permalink raw reply

* Re: [PATCH v7 3/3] of/pci: mips: convert to common of_pci_range_parser
From: Jason Cooper @ 2013-04-18 13:09 UTC (permalink / raw)
  To: Andrew Murray
  Cc: linux-mips@linux-mips.org, siva.kallam@samsung.com,
	linux-pci@vger.kernel.org, Linus Walleij, Thierry Reding,
	Liviu Dudau, Paul Mackerras, linux-samsung-soc,
	Russell King - ARM Linux, Jingoo Han, Jason Gunthorpe,
	Thomas Abraham, Arnd Bergmann,
	devicetree-discuss@lists.ozlabs.org, rob.herring@calxeda.com,
	Kukjin Kim, bhelgaas@google.com,
	linux-arm-kernel@lists.infradead.org, Thomas Petazzoni,
	Michal Simek, linux-kernel@vger.kernel.org,
	suren.reddy@samsung.com, linuxppc-dev@lists.ozlabs.org list
In-Reply-To: <20130418125910.GA17128@arm.com>

On Thu, Apr 18, 2013 at 01:59:10PM +0100, Andrew Murray wrote:
> On Wed, Apr 17, 2013 at 04:42:48PM +0100, Linus Walleij wrote:
> > On Tue, Apr 16, 2013 at 12:18 PM, Andrew Murray <Andrew.Murray@arm.com> wrote:
> > 
> > > This patch converts the pci_load_of_ranges function to use the new common
> > > of_pci_range_parser.
> > >
> > > Signed-off-by: Andrew Murray <Andrew.Murray@arm.com>
> > > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> > > Reviewed-by: Rob Herring <rob.herring@calxeda.com>
> > 
> > Tested-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Jason - you may not have seen this, but here (Linus Walleij) is another Tested-by
> to add to this patch in your tree (if you can).

Thanks, I saw it.  Unfortunately, the PR was already sent, and the branch
is now pulled into arm-soc.

thx,

Jason.

^ permalink raw reply

* Re: [PATCH v7 1/3] of/pci: Unify pci_process_bridge_OF_ranges from Microblaze and PowerPC
From: Jason Cooper @ 2013-04-18 13:06 UTC (permalink / raw)
  To: Grant Likely
  Cc: linux-mips@linux-mips.org, siva.kallam@samsung.com,
	linux-pci@vger.kernel.org, linus.walleij@linaro.org,
	thierry.reding@avionic-design.de, Liviu Dudau, Gregory CLEMENT,
	paulus@samba.org, linux-samsung-soc@vger.kernel.org,
	linux@arm.linux.org.uk, jg1.han@samsung.com,
	jgunthorpe@obsidianresearch.com, thomas.abraham@linaro.org,
	arnd@arndb.de, devicetree-discuss@lists.ozlabs.org,
	rob.herring@calxeda.com, kgene.kim@samsung.com,
	bh elgaas@g oogle.com, linux-arm-kernel@lists.infradead.org,
	thomas.petazzoni@free-electrons.com, monstr@monstr.eu,
	linux-kernel@vger.kernel.org, Andrew Lunn,
	suren.reddy@samsung.com, Olof Johansson, Andrew Murray,
	linuxppc-dev@lists.ozlabs.org
In-Reply-To: <20130418124832.D2C733E118C@localhost>

On Thu, Apr 18, 2013 at 01:48:32PM +0100, Grant Likely wrote:
> On Wed, 17 Apr 2013 12:22:23 -0400, Jason Cooper <jason@lakedaemon.net> wrote:
> > On Wed, Apr 17, 2013 at 05:17:48PM +0100, Grant Likely wrote:
> > > On Wed, Apr 17, 2013 at 5:10 PM, Jason Cooper <jason@lakedaemon.net> wrote:
> > > > On Wed, Apr 17, 2013 at 05:00:15PM +0100, Grant Likely wrote:
> > > >> On Tue, 16 Apr 2013 11:30:06 +0100, Andrew Murray <andrew.murray@arm.com> wrote:
> > > >> > On Tue, Apr 16, 2013 at 11:18:26AM +0100, Andrew Murray wrote:
> > > >> > > The pci_process_bridge_OF_ranges function, used to parse the "ranges"
> > > >> > > property of a PCI host device, is found in both Microblaze and PowerPC
> > > >> > > architectures. These implementations are nearly identical. This patch
> > > >> > > moves this common code to a common place.
> > > >> > >
> > > >> > > Signed-off-by: Andrew Murray <Andrew.Murray@arm.com>
> > > >> > > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> > > >> > > Reviewed-by: Rob Herring <rob.herring@calxeda.com>
> > > >> > > Tested-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> > > >> > > Tested-by: Linus Walleij <linus.walleij@linaro.org>
> > > >> > > Acked-by: Michal Simek <monstr@monstr.eu>
> > > >> > > ---
> > > >> > >  arch/microblaze/include/asm/pci-bridge.h |    5 +-
> > > >> > >  arch/microblaze/pci/pci-common.c         |  192 ----------------------------
> > > >> > >  arch/powerpc/include/asm/pci-bridge.h    |    5 +-
> > > >> > >  arch/powerpc/kernel/pci-common.c         |  192 ----------------------------
> > > >> >
> > > >> > Is there anyone on linuxppc-dev/linux-mips that can help test this patchset?
> > > >> >
> > > >> > I've tested that it builds on powerpc with a variety of configs (some which
> > > >> > include fsl_pci.c implementation). Though I don't have hardware to verify that
> > > >> > it works.
> > > >> >
> > > >> > I haven't tested this builds or runs on MIPS.
> > > >> >
> > > >> > You shouldn't see any difference in behaviour or new warnings and PCI devices
> > > >> > should continue to operate as before.
> > > >>
> > > >> I've got through a line-by-line comparison between powerpc, microblaze,
> > > >> and then new code. The differences are purely cosmetic, so I have
> > > >> absolutely no concerns about this patch. I've applied it to my tree.
> > > >
> > > > oops.  Due to the number of dependencies the mvebu-pcie series has (this
> > > > being one of them, we (arm-soc/mvebu) asked if we could take this
> > > > through our tree.  Rob Herring agreed to this several days ago.  Is this
> > > > a problem for you?
> > > >
> > > > It would truly (dogs and cats living together) upset the apple cart for
> > > > us at this stage to pipe these through a different tree...
> > > 
> > > Not a problem at all. I'll drop it.
> > 
> > Great!  Thanks.
> 
> You can add my Acked-by: Grant Likely <grant.likely@linaro.org> to the
> first patch. I've not reviewed out the second or third patches yet.
> 
> None of this appears to be in linux-next yet. I've boot tested on
> PowerPC, but that isn't the same as an ack by the PowerPC maintainers.
> It is probably too late for the whole now since we're after -rc7.
> However, if you ask me to, I have absolutely no problem putting the
> first patch into my tree for v3.10 merging. I went over the patch
> line-by-line and am convinced that it is functionally identical.
> 
> Let me know if you need me to do this.

Thanks for the offer, Olof just pulled the branch last night (my PRs
were a little late trying to sort all this out), so it should be in
shortly.

FYI:

  65ee348 of/pci: Unify pci_process_bridge_OF_ranges from Microblaze and PowerPC

is in arm-soc/mvebu/drivers as well as arm-soc/next/drivers (and
arm-soc/for-next if you want to test merging everything...)

thx,

Jason.

^ permalink raw reply

* Re: [PATCH v7 3/3] of/pci: mips: convert to common of_pci_range_parser
From: Andrew Murray @ 2013-04-18 12:59 UTC (permalink / raw)
  To: Linus Walleij
  Cc: linux-mips@linux-mips.org, siva.kallam@samsung.com,
	linux-pci@vger.kernel.org, Thierry Reding, Liviu Dudau,
	Paul Mackerras, linux-samsung-soc, Russell King - ARM Linux,
	Jingoo Han, Jason Gunthorpe, Thomas Abraham, jason, Arnd Bergmann,
	devicetree-discuss@lists.ozlabs.org, rob.herring@calxeda.com,
	Kukjin Kim, bhelgaas@google.com,
	linux-arm-kernel@lists.infradead.org, Thomas Petazzoni,
	Michal Simek, linux-kernel@vger.kernel.org,
	suren.reddy@samsung.com, linuxppc-dev@lists.ozlabs.org list
In-Reply-To: <CACRpkdbBaT1OKr5t8HW4+8y_wSDmGxmewAyVMekx8S-K9s3K8Q@mail.gmail.com>

On Wed, Apr 17, 2013 at 04:42:48PM +0100, Linus Walleij wrote:
> On Tue, Apr 16, 2013 at 12:18 PM, Andrew Murray <Andrew.Murray@arm.com> wrote:
> 
> > This patch converts the pci_load_of_ranges function to use the new common
> > of_pci_range_parser.
> >
> > Signed-off-by: Andrew Murray <Andrew.Murray@arm.com>
> > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> > Reviewed-by: Rob Herring <rob.herring@calxeda.com>
> 
> Tested-by: Linus Walleij <linus.walleij@linaro.org>

Jason - you may not have seen this, but here (Linus Walleij) is another Tested-by
to add to this patch in your tree (if you can).

Andrew Murray

^ permalink raw reply

* Re: [PATCH v7 1/3] of/pci: Unify pci_process_bridge_OF_ranges from Microblaze and PowerPC
From: Grant Likely @ 2013-04-18 12:48 UTC (permalink / raw)
  To: Jason Cooper
  Cc: linux-mips@linux-mips.org, siva.kallam@samsung.com,
	linux-pci@vger.kernel.org, linus.walleij@linaro.org,
	thierry.reding@avionic-design.de, Liviu Dudau, Gregory CLEMENT,
	paulus@samba.org, linux-samsung-soc@vger.kernel.org,
	linux@arm.linux.org.uk, jg1.han@samsung.com,
	jgunthorpe@obsidianresearch.com, thomas.abraham@linaro.org,
	arnd@arndb.de, devicetree-discuss@lists.ozlabs.org,
	rob.herring@calxeda.com, kgene.kim@samsung.com,
	bhelgaas@g oogle.com, linux-arm-kernel@lists.infradead.org,
	thomas.petazzoni@free-electrons.com, monstr@monstr.eu,
	linux-kernel@vger.kernel.org, Andrew Lunn,
	suren.reddy@samsung.com, Olof Johansson, Andrew Murray,
	linuxppc-dev@lists.ozlabs.org
In-Reply-To: <20130417162223.GC27197@titan.lakedaemon.net>

On Wed, 17 Apr 2013 12:22:23 -0400, Jason Cooper <jason@lakedaemon.net> wrote:
> On Wed, Apr 17, 2013 at 05:17:48PM +0100, Grant Likely wrote:
> > On Wed, Apr 17, 2013 at 5:10 PM, Jason Cooper <jason@lakedaemon.net> wrote:
> > > On Wed, Apr 17, 2013 at 05:00:15PM +0100, Grant Likely wrote:
> > >> On Tue, 16 Apr 2013 11:30:06 +0100, Andrew Murray <andrew.murray@arm.com> wrote:
> > >> > On Tue, Apr 16, 2013 at 11:18:26AM +0100, Andrew Murray wrote:
> > >> > > The pci_process_bridge_OF_ranges function, used to parse the "ranges"
> > >> > > property of a PCI host device, is found in both Microblaze and PowerPC
> > >> > > architectures. These implementations are nearly identical. This patch
> > >> > > moves this common code to a common place.
> > >> > >
> > >> > > Signed-off-by: Andrew Murray <Andrew.Murray@arm.com>
> > >> > > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> > >> > > Reviewed-by: Rob Herring <rob.herring@calxeda.com>
> > >> > > Tested-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> > >> > > Tested-by: Linus Walleij <linus.walleij@linaro.org>
> > >> > > Acked-by: Michal Simek <monstr@monstr.eu>
> > >> > > ---
> > >> > >  arch/microblaze/include/asm/pci-bridge.h |    5 +-
> > >> > >  arch/microblaze/pci/pci-common.c         |  192 ----------------------------
> > >> > >  arch/powerpc/include/asm/pci-bridge.h    |    5 +-
> > >> > >  arch/powerpc/kernel/pci-common.c         |  192 ----------------------------
> > >> >
> > >> > Is there anyone on linuxppc-dev/linux-mips that can help test this patchset?
> > >> >
> > >> > I've tested that it builds on powerpc with a variety of configs (some which
> > >> > include fsl_pci.c implementation). Though I don't have hardware to verify that
> > >> > it works.
> > >> >
> > >> > I haven't tested this builds or runs on MIPS.
> > >> >
> > >> > You shouldn't see any difference in behaviour or new warnings and PCI devices
> > >> > should continue to operate as before.
> > >>
> > >> I've got through a line-by-line comparison between powerpc, microblaze,
> > >> and then new code. The differences are purely cosmetic, so I have
> > >> absolutely no concerns about this patch. I've applied it to my tree.
> > >
> > > oops.  Due to the number of dependencies the mvebu-pcie series has (this
> > > being one of them, we (arm-soc/mvebu) asked if we could take this
> > > through our tree.  Rob Herring agreed to this several days ago.  Is this
> > > a problem for you?
> > >
> > > It would truly (dogs and cats living together) upset the apple cart for
> > > us at this stage to pipe these through a different tree...
> > 
> > Not a problem at all. I'll drop it.
> 
> Great!  Thanks.

You can add my Acked-by: Grant Likely <grant.likely@linaro.org> to the
first patch. I've not reviewed out the second or third patches yet.

None of this appears to be in linux-next yet. I've boot tested on
PowerPC, but that isn't the same as an ack by the PowerPC maintainers.
It is probably too late for the whole now since we're after -rc7.
However, if you ask me to, I have absolutely no problem putting the
first patch into my tree for v3.10 merging. I went over the patch
line-by-line and am convinced that it is functionally identical.

Let me know if you need me to do this.

g.

^ permalink raw reply

* [PATCH V3 0/5] powerpc, perf: BHRB based branch stack enablement on POWER8
From: Anshuman Khandual @ 2013-04-18 12:26 UTC (permalink / raw)
  To: linuxppc-dev, linux-kernel; +Cc: mikey

                Branch History Rolling Buffer (BHRB) is a new PMU feaure in IBM
POWER8 processor which records the branch instructions inside the execution
pipeline. This patchset enables the basic functionality of the feature through
generic perf branch stack sampling framework.

Sample output
-------------
$./perf record -b top
$./perf report

Overhead  Command  Source Shared Object                           Source Symbol  Target Shared Object                        Target Symbol
# ........  .......  ....................  ......................................  ....................  ...................................
#

     7.82%      top  libc-2.11.2.so        [k] _IO_vfscanf                         libc-2.11.2.so        [k] _IO_vfscanf
     6.17%      top  libc-2.11.2.so        [k] _IO_vfscanf                         [unknown]             [k] 00000000
     2.37%      top  [unknown]             [k] 0xf7aafb30                          [unknown]             [k] 00000000
     1.80%      top  [unknown]             [k] 0x0fe07978                          libc-2.11.2.so        [k] _IO_vfscanf
     1.60%      top  libc-2.11.2.so        [k] _IO_vfscanf                         [kernel.kallsyms]     [k] .do_task_stat
     1.20%      top  [kernel.kallsyms]     [k] .do_task_stat                       [kernel.kallsyms]     [k] .do_task_stat
     1.02%      top  libc-2.11.2.so        [k] vfprintf                            libc-2.11.2.so        [k] vfprintf
     0.92%      top  top                   [k] _init                               [unknown]             [k] 0x0fe037f4

Changes in V2
--------------
- Added copyright messages to the newly created files
- Modified couple of commit messages

Changes in V3
-------------
- Incorporated review comments from Segher https://lkml.org/lkml/2013/4/16/350
- Worked on a solution for review comment from Michael Ellerman https://lkml.org/lkml/2013/4/17/548
	- Could not move updated cpu_hw_events structure from core-book3s.c file into perf_event_server.h
          Because perf_event_server.h is pulled in first inside linux/perf_event.h before the definition of
          perf_branch_entry structure. Thats the reason why perf_branch_entry definition is not available
          inside perf_event_server.h where we define the array inside cpu_hw_events structure.

	- Finally have pulled in the code from perf_event_bhrb.c into core-book3s.c

- Improved documentation for the patchset


Anshuman Khandual (5):
  powerpc, perf: Add new BHRB related instructions for POWER8
  powerpc, perf: Add basic assembly code to read BHRB entries on POWER8
  powerpc, perf: Add new BHRB related generic functions, data and flags
  powerpc, perf: Define BHRB generic functions, data and flags for POWER8
  powerpc, perf: Enable branch stack sampling framework

 arch/powerpc/include/asm/perf_event_server.h |   7 ++
 arch/powerpc/include/asm/ppc-opcode.h        |   7 ++
 arch/powerpc/perf/Makefile                   |   2 +-
 arch/powerpc/perf/bhrb.S                     |  44 +++++++
 arch/powerpc/perf/core-book3s.c              | 167 ++++++++++++++++++++++++++-
 arch/powerpc/perf/power8-pmu.c               |  57 ++++++++-
 6 files changed, 279 insertions(+), 5 deletions(-)
 create mode 100644 arch/powerpc/perf/bhrb.S

-- 
1.7.11.7

^ permalink raw reply

* [PATCH V3 2/5] powerpc, perf: Add basic assembly code to read BHRB entries on POWER8
From: Anshuman Khandual @ 2013-04-18 12:26 UTC (permalink / raw)
  To: linuxppc-dev, linux-kernel; +Cc: mikey
In-Reply-To: <1366287976-3900-1-git-send-email-khandual@linux.vnet.ibm.com>

This patch adds the basic assembly code to read BHRB buffer. BHRB entries
are valid only after a PMU interrupt has happened (when MMCR0[PMAO]=1)
and BHRB has been freezed. BHRB read should not be attempted when it is
still enabled (MMCR0[PMAE]=1) and getting updated, as this can produce
non-deterministic results.

Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
---
 arch/powerpc/perf/Makefile |  2 +-
 arch/powerpc/perf/bhrb.S   | 44 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 45 insertions(+), 1 deletion(-)
 create mode 100644 arch/powerpc/perf/bhrb.S

diff --git a/arch/powerpc/perf/Makefile b/arch/powerpc/perf/Makefile
index 472db18..510fae1 100644
--- a/arch/powerpc/perf/Makefile
+++ b/arch/powerpc/perf/Makefile
@@ -2,7 +2,7 @@ subdir-ccflags-$(CONFIG_PPC_WERROR) := -Werror
 
 obj-$(CONFIG_PERF_EVENTS)	+= callchain.o
 
-obj-$(CONFIG_PPC_PERF_CTRS)	+= core-book3s.o
+obj-$(CONFIG_PPC_PERF_CTRS)	+= core-book3s.o bhrb.o
 obj64-$(CONFIG_PPC_PERF_CTRS)	+= power4-pmu.o ppc970-pmu.o power5-pmu.o \
 				   power5+-pmu.o power6-pmu.o power7-pmu.o \
 				   power8-pmu.o
diff --git a/arch/powerpc/perf/bhrb.S b/arch/powerpc/perf/bhrb.S
new file mode 100644
index 0000000..d85f9a5
--- /dev/null
+++ b/arch/powerpc/perf/bhrb.S
@@ -0,0 +1,44 @@
+/*
+ * Basic assembly code to read BHRB entries
+ *
+ * Copyright 2013 Anshuman Khandual, IBM Corporation.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+#include <asm/ppc_asm.h>
+#include <asm/ppc-opcode.h>
+
+	.text
+
+.balign 8
+
+/* r3 = n  (where n = [0-31])
+ * The maximum number of BHRB entries supported with PPC_MFBHRBE instruction
+ * is 1024. We have limited number of table entries here as POWER8 implements
+ * 32 BHRB entries.
+ */
+
+/* .global read_bhrb */
+_GLOBAL(read_bhrb)
+	cmpldi	r3,31
+	bgt	1f
+	ld	r4,bhrb_table@got(r2)
+	sldi	r3,r3,3
+	add	r3,r4,r3
+	mtctr	r3
+	bctr
+1:	li	r3,0
+	blr
+
+#define MFBHRB_TABLE1(n) PPC_MFBHRBE(R3,n); blr
+#define MFBHRB_TABLE2(n) MFBHRB_TABLE1(n); MFBHRB_TABLE1(n+1)
+#define MFBHRB_TABLE4(n) MFBHRB_TABLE2(n); MFBHRB_TABLE2(n+2)
+#define MFBHRB_TABLE8(n) MFBHRB_TABLE4(n); MFBHRB_TABLE4(n+4)
+#define MFBHRB_TABLE16(n) MFBHRB_TABLE8(n); MFBHRB_TABLE8(n+8)
+#define MFBHRB_TABLE32(n) MFBHRB_TABLE16(n); MFBHRB_TABLE16(n+16)
+
+bhrb_table:
+	MFBHRB_TABLE32(0)
-- 
1.7.11.7

^ permalink raw reply related

* [PATCH V3 5/5] powerpc, perf: Enable branch stack sampling framework
From: Anshuman Khandual @ 2013-04-18 12:26 UTC (permalink / raw)
  To: linuxppc-dev, linux-kernel; +Cc: mikey
In-Reply-To: <1366287976-3900-1-git-send-email-khandual@linux.vnet.ibm.com>

Provides basic enablement for perf branch stack sampling framework on
POWER8 processor based platforms. Adds new BHRB related elements into
cpu_hw_event structure to represent current BHRB config, BHRB filter
configuration, manage context and to hold output BHRB buffer during
PMU interrupt before passing to the user space. This also enables
processing of BHRB data and converts them into generic perf branch
stack data format.

Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/perf_event_server.h |   1 +
 arch/powerpc/perf/core-book3s.c              | 167 ++++++++++++++++++++++++++-
 2 files changed, 165 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/include/asm/perf_event_server.h b/arch/powerpc/include/asm/perf_event_server.h
index 3f0c15c..f265049 100644
--- a/arch/powerpc/include/asm/perf_event_server.h
+++ b/arch/powerpc/include/asm/perf_event_server.h
@@ -73,6 +73,7 @@ extern int register_power_pmu(struct power_pmu *);
 struct pt_regs;
 extern unsigned long perf_misc_flags(struct pt_regs *regs);
 extern unsigned long perf_instruction_pointer(struct pt_regs *regs);
+extern unsigned long int read_bhrb(int n);
 
 /*
  * Only override the default definitions in include/linux/perf_event.h
diff --git a/arch/powerpc/perf/core-book3s.c b/arch/powerpc/perf/core-book3s.c
index 4ac6e64..c627843 100644
--- a/arch/powerpc/perf/core-book3s.c
+++ b/arch/powerpc/perf/core-book3s.c
@@ -19,6 +19,11 @@
 #include <asm/firmware.h>
 #include <asm/ptrace.h>
 
+#define BHRB_MAX_ENTRIES	32
+#define BHRB_TARGET		0x0000000000000002
+#define BHRB_PREDICTION		0x0000000000000001
+#define BHRB_EA			0xFFFFFFFFFFFFFFFC
+
 struct cpu_hw_events {
 	int n_events;
 	int n_percpu;
@@ -38,7 +43,15 @@ struct cpu_hw_events {
 
 	unsigned int group_flag;
 	int n_txn_start;
+
+	/* BHRB bits */
+	u64				bhrb_filter;	/* BHRB HW branch filter */
+	int				bhrb_users;
+	void				*bhrb_context;
+	struct	perf_branch_stack	bhrb_stack;
+	struct	perf_branch_entry	bhrb_entries[BHRB_MAX_ENTRIES];
 };
+
 DEFINE_PER_CPU(struct cpu_hw_events, cpu_hw_events);
 
 struct power_pmu *ppmu;
@@ -858,6 +871,9 @@ static void power_pmu_enable(struct pmu *pmu)
 	}
 
  out:
+	if (cpuhw->bhrb_users)
+		ppmu->config_bhrb(cpuhw->bhrb_filter);
+
 	local_irq_restore(flags);
 }
 
@@ -888,6 +904,47 @@ static int collect_events(struct perf_event *group, int max_count,
 	return n;
 }
 
+/* Reset all possible BHRB entries */
+static void power_pmu_bhrb_reset(void)
+{
+	asm volatile(PPC_CLRBHRB);
+}
+
+void power_pmu_bhrb_enable(struct perf_event *event)
+{
+	struct cpu_hw_events *cpuhw = &__get_cpu_var(cpu_hw_events);
+
+	if (!ppmu->bhrb_nr)
+		return;
+
+	/* Clear BHRB if we changed task context to avoid data leaks */
+	if (event->ctx->task && cpuhw->bhrb_context != event->ctx) {
+		power_pmu_bhrb_reset();
+		cpuhw->bhrb_context = event->ctx;
+	}
+	cpuhw->bhrb_users++;
+}
+
+void power_pmu_bhrb_disable(struct perf_event *event)
+{
+	struct cpu_hw_events *cpuhw = &__get_cpu_var(cpu_hw_events);
+
+	if (!ppmu->bhrb_nr)
+		return;
+
+	cpuhw->bhrb_users--;
+	WARN_ON_ONCE(cpuhw->bhrb_users < 0);
+
+	if (!cpuhw->disabled && !cpuhw->bhrb_users) {
+		/* BHRB cannot be turned off when other
+		 * events are active on the PMU.
+		 */
+
+		/* avoid stale pointer */
+		cpuhw->bhrb_context = NULL;
+	}
+}
+
 /*
  * Add a event to the PMU.
  * If all events are not already frozen, then we disable and
@@ -947,6 +1004,9 @@ nocheck:
 
 	ret = 0;
  out:
+	if (has_branch_stack(event))
+		power_pmu_bhrb_enable(event);
+
 	perf_pmu_enable(event->pmu);
 	local_irq_restore(flags);
 	return ret;
@@ -999,6 +1059,9 @@ static void power_pmu_del(struct perf_event *event, int ef_flags)
 		cpuhw->mmcr[0] &= ~(MMCR0_PMXE | MMCR0_FCECE);
 	}
 
+	if (has_branch_stack(event))
+		power_pmu_bhrb_disable(event);
+
 	perf_pmu_enable(event->pmu);
 	local_irq_restore(flags);
 }
@@ -1117,6 +1180,15 @@ int power_pmu_commit_txn(struct pmu *pmu)
 	return 0;
 }
 
+/* Called from ctxsw to prevent one process's branch entries to
+ * mingle with the other process's entries during context switch.
+ */
+void power_pmu_flush_branch_stack(void)
+{
+	if (ppmu->bhrb_nr)
+		power_pmu_bhrb_reset();
+}
+
 /*
  * Return 1 if we might be able to put event on a limited PMC,
  * or 0 if not.
@@ -1231,9 +1303,11 @@ static int power_pmu_event_init(struct perf_event *event)
 	if (!ppmu)
 		return -ENOENT;
 
-	/* does not support taken branch sampling */
-	if (has_branch_stack(event))
-		return -EOPNOTSUPP;
+	if (has_branch_stack(event)) {
+	        /* PMU has BHRB enabled */
+		if (!(ppmu->flags & PPMU_BHRB))
+			return -EOPNOTSUPP;
+	}
 
 	switch (event->attr.type) {
 	case PERF_TYPE_HARDWARE:
@@ -1314,6 +1388,15 @@ static int power_pmu_event_init(struct perf_event *event)
 
 	cpuhw = &get_cpu_var(cpu_hw_events);
 	err = power_check_constraints(cpuhw, events, cflags, n + 1);
+
+	if (has_branch_stack(event)) {
+		cpuhw->bhrb_filter = ppmu->bhrb_filter_map(
+					event->attr.branch_sample_type);
+
+		if(cpuhw->bhrb_filter == -1)
+			return -EOPNOTSUPP;
+	}
+
 	put_cpu_var(cpu_hw_events);
 	if (err)
 		return -EINVAL;
@@ -1372,8 +1455,79 @@ struct pmu power_pmu = {
 	.cancel_txn	= power_pmu_cancel_txn,
 	.commit_txn	= power_pmu_commit_txn,
 	.event_idx	= power_pmu_event_idx,
+	.flush_branch_stack = power_pmu_flush_branch_stack,
 };
 
+/* Processing BHRB entries */
+void power_pmu_bhrb_read(struct cpu_hw_events *cpuhw)
+{
+	u64 val;
+	u64 addr;
+	int r_index, u_index, target, pred;
+
+	r_index = 0;
+	u_index = 0;
+	while (r_index < ppmu->bhrb_nr) {
+		/* Assembly read function */
+		val = read_bhrb(r_index);
+
+		/* Terminal marker: End of valid BHRB entries */
+		if (val == 0) {
+			break;
+		} else {
+			/* BHRB field break up */
+			addr = val & BHRB_EA;
+			pred = val & BHRB_PREDICTION;
+			target = val & BHRB_TARGET;
+
+			/* Probable Missed entry: Not applicable for POWER8 */
+			if ((addr == 0) && (target == 0) && (pred == 1)) {
+				r_index++;
+				continue;
+			}
+
+			/* Real Missed entry: Power8 based missed entry */
+			if ((addr == 0) && (target == 1) && (pred == 1)) {
+				r_index++;
+				continue;
+			}
+
+			/* Reserved condition: Not a valid entry  */
+			if ((addr == 0) && (target == 1) && (pred == 0)) {
+				r_index++;
+				continue;
+			}
+
+			/* Is a target address */
+			if (val & BHRB_TARGET) {
+				/* First address cannot be a target address */
+				if (r_index == 0) {
+					r_index++;
+					continue;
+				}
+
+				/* Update target address for the previous entry */
+				cpuhw->bhrb_entries[u_index - 1].to = addr;
+				cpuhw->bhrb_entries[u_index - 1].mispred = pred;
+				cpuhw->bhrb_entries[u_index - 1].predicted = ~pred;
+
+				/* Dont increment u_index */
+				r_index++;
+			} else {
+				/* Update address, flags for current entry */
+				cpuhw->bhrb_entries[u_index].from = addr;
+				cpuhw->bhrb_entries[u_index].mispred = pred;
+				cpuhw->bhrb_entries[u_index].predicted = ~pred;
+
+				/* Successfully popullated one entry */
+				u_index++;
+				r_index++;
+			}
+		}
+	}
+	cpuhw->bhrb_stack.nr = u_index;
+	return;
+}
 
 /*
  * A counter has overflowed; update its count and record
@@ -1433,6 +1587,13 @@ static void record_and_restart(struct perf_event *event, unsigned long val,
 		if (event->attr.sample_type & PERF_SAMPLE_ADDR)
 			perf_get_data_addr(regs, &data.addr);
 
+		if (event->attr.sample_type & PERF_SAMPLE_BRANCH_STACK) {
+			struct cpu_hw_events *cpuhw;
+			cpuhw = &__get_cpu_var(cpu_hw_events);
+			power_pmu_bhrb_read(cpuhw);
+			data.br_stack = &cpuhw->bhrb_stack;
+		}
+
 		if (perf_event_overflow(event, &data, regs))
 			power_pmu_stop(event, 0);
 	}
-- 
1.7.11.7

^ permalink raw reply related

* [PATCH V3 1/5] powerpc, perf: Add new BHRB related instructions for POWER8
From: Anshuman Khandual @ 2013-04-18 12:26 UTC (permalink / raw)
  To: linuxppc-dev, linux-kernel; +Cc: mikey
In-Reply-To: <1366287976-3900-1-git-send-email-khandual@linux.vnet.ibm.com>

This patch adds new POWER8 instruction encoding for reading
the BHRB buffer entries and also clearing it. Encoding for
"clrbhrb" instruction is straight forward. But "mfbhrbe"
encoding involves reading a certain index of BHRB buffer
into a particular GPR register.

Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/ppc-opcode.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/powerpc/include/asm/ppc-opcode.h b/arch/powerpc/include/asm/ppc-opcode.h
index 8752bc8..93ae5a1 100644
--- a/arch/powerpc/include/asm/ppc-opcode.h
+++ b/arch/powerpc/include/asm/ppc-opcode.h
@@ -82,6 +82,7 @@
 #define	__REGA0_R31	31
 
 /* sorted alphabetically */
+#define PPC_INST_BHRBE			0x7c00025c
 #define PPC_INST_DCBA			0x7c0005ec
 #define PPC_INST_DCBA_MASK		0xfc0007fe
 #define PPC_INST_DCBAL			0x7c2005ec
@@ -297,6 +298,12 @@
 #define PPC_NAP			stringify_in_c(.long PPC_INST_NAP)
 #define PPC_SLEEP		stringify_in_c(.long PPC_INST_SLEEP)
 
+/* BHRB instructions */
+#define PPC_CLRBHRB		stringify_in_c(.long 0x7c00035c)
+#define PPC_MFBHRBE(r, n)	stringify_in_c(.long PPC_INST_BHRBE | \
+						__PPC_RS(r) | \
+							(((n) & 0x1f) << 11))
+
 /* Transactional memory instructions */
 #define TRECHKPT		stringify_in_c(.long PPC_INST_TRECHKPT)
 #define TRECLAIM(r)		stringify_in_c(.long PPC_INST_TRECLAIM \
-- 
1.7.11.7

^ permalink raw reply related

* [PATCH V3 3/5] powerpc, perf: Add new BHRB related generic functions, data and flags
From: Anshuman Khandual @ 2013-04-18 12:26 UTC (permalink / raw)
  To: linuxppc-dev, linux-kernel; +Cc: mikey
In-Reply-To: <1366287976-3900-1-git-send-email-khandual@linux.vnet.ibm.com>

This patch adds couple of generic functions to power_pmu structure
which would configure the BHRB and it's filters. It also adds
representation of the number of BHRB entries present on the PMU.
A new PMU flag PPMU_BHRB would indicate presence of BHRB feature.

Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/perf_event_server.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/powerpc/include/asm/perf_event_server.h b/arch/powerpc/include/asm/perf_event_server.h
index 57b42da..3f0c15c 100644
--- a/arch/powerpc/include/asm/perf_event_server.h
+++ b/arch/powerpc/include/asm/perf_event_server.h
@@ -33,6 +33,8 @@ struct power_pmu {
 				unsigned long *valp);
 	int		(*get_alternatives)(u64 event_id, unsigned int flags,
 				u64 alt[]);
+	u64             (*bhrb_filter_map)(u64 branch_sample_type);
+	void            (*config_bhrb)(u64 pmu_bhrb_filter);
 	void		(*disable_pmc)(unsigned int pmc, unsigned long mmcr[]);
 	int		(*limited_pmc_event)(u64 event_id);
 	u32		flags;
@@ -42,6 +44,9 @@ struct power_pmu {
 	int		(*cache_events)[PERF_COUNT_HW_CACHE_MAX]
 			       [PERF_COUNT_HW_CACHE_OP_MAX]
 			       [PERF_COUNT_HW_CACHE_RESULT_MAX];
+
+	/* BHRB entries in the PMU */
+	int		bhrb_nr;
 };
 
 /*
@@ -54,6 +59,7 @@ struct power_pmu {
 #define PPMU_SIAR_VALID		0x00000010 /* Processor has SIAR Valid bit */
 #define PPMU_HAS_SSLOT		0x00000020 /* Has sampled slot in MMCRA */
 #define PPMU_HAS_SIER		0x00000040 /* Has SIER */
+#define PPMU_BHRB		0x00000080 /* has BHRB feature enabled */
 
 /*
  * Values for flags to get_alternatives()
-- 
1.7.11.7

^ permalink raw reply related

* [PATCH V3 4/5] powerpc, perf: Define BHRB generic functions, data and flags for POWER8
From: Anshuman Khandual @ 2013-04-18 12:26 UTC (permalink / raw)
  To: linuxppc-dev, linux-kernel; +Cc: mikey
In-Reply-To: <1366287976-3900-1-git-send-email-khandual@linux.vnet.ibm.com>

This patch populates BHRB specific data for power_pmu structure. It
also implements POWER8 specific BHRB filter and configuration functions.

Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
---
 arch/powerpc/perf/power8-pmu.c | 57 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 56 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/perf/power8-pmu.c b/arch/powerpc/perf/power8-pmu.c
index 106ae0b..153408c 100644
--- a/arch/powerpc/perf/power8-pmu.c
+++ b/arch/powerpc/perf/power8-pmu.c
@@ -109,6 +109,16 @@
 #define EVENT_IS_MARKED		(EVENT_MARKED_MASK << EVENT_MARKED_SHIFT)
 #define EVENT_PSEL_MASK		0xff	/* PMCxSEL value */
 
+/* MMCRA IFM bits - POWER8 */
+#define	POWER8_MMCRA_IFM1		0x0000000040000000UL
+#define	POWER8_MMCRA_IFM2		0x0000000080000000UL
+#define	POWER8_MMCRA_IFM3		0x00000000C0000000UL
+
+#define ONLY_PLM \
+	(PERF_SAMPLE_BRANCH_USER        |\
+	 PERF_SAMPLE_BRANCH_KERNEL      |\
+	 PERF_SAMPLE_BRANCH_HV)
+
 /*
  * Layout of constraint bits:
  *
@@ -428,6 +438,48 @@ static int power8_generic_events[] = {
 	[PERF_COUNT_HW_BRANCH_MISSES] =			PM_BR_MPRED_CMPL,
 };
 
+static u64 power8_bhrb_filter_map(u64 branch_sample_type)
+{
+	u64 pmu_bhrb_filter = 0;
+	u64 br_privilege = branch_sample_type & ONLY_PLM;
+
+	/* BHRB and regular PMU events share the same prvillege state
+	 * filter configuration. BHRB is always recorded along with a
+	 * regular PMU event. So privilege state filter criteria for BHRB
+	 * and the companion PMU events has to be the same. As a default
+	 * "perf record" tool sets all privillege bits ON when no filter
+	 * criteria is provided in the command line. So as along as all
+	 * privillege bits are ON or they are OFF, we are good to go.
+	 */
+	if ((br_privilege != 7) && (br_privilege != 0))
+		return -1;
+
+	/* No branch filter requested */
+	if (branch_sample_type & PERF_SAMPLE_BRANCH_ANY)
+		return pmu_bhrb_filter;
+
+	/* Invalid branch filter options - HW does not support */
+	if (branch_sample_type & PERF_SAMPLE_BRANCH_ANY_RETURN)
+		return -1;
+
+	if (branch_sample_type & PERF_SAMPLE_BRANCH_IND_CALL)
+		return -1;
+
+	if (branch_sample_type & PERF_SAMPLE_BRANCH_ANY_CALL) {
+		pmu_bhrb_filter |= POWER8_MMCRA_IFM1;
+		return pmu_bhrb_filter;
+	}
+
+	/* Every thing else is unsupported */
+	return -1;
+}
+
+static void power8_config_bhrb(u64 pmu_bhrb_filter)
+{
+	/* Enable BHRB filter in PMU */
+	mtspr(SPRN_MMCRA, (mfspr(SPRN_MMCRA) | pmu_bhrb_filter));
+}
+
 static struct power_pmu power8_pmu = {
 	.name			= "POWER8",
 	.n_counter		= 6,
@@ -435,12 +487,15 @@ static struct power_pmu power8_pmu = {
 	.add_fields		= POWER8_ADD_FIELDS,
 	.test_adder		= POWER8_TEST_ADDER,
 	.compute_mmcr		= power8_compute_mmcr,
+	.config_bhrb		= power8_config_bhrb,
+	.bhrb_filter_map	= power8_bhrb_filter_map,
 	.get_constraint		= power8_get_constraint,
 	.disable_pmc		= power8_disable_pmc,
-	.flags			= PPMU_HAS_SSLOT | PPMU_HAS_SIER,
+	.flags			= PPMU_HAS_SSLOT | PPMU_HAS_SIER | PPMU_BHRB,
 	.n_generic		= ARRAY_SIZE(power8_generic_events),
 	.generic_events		= power8_generic_events,
 	.attr_groups		= power8_pmu_attr_groups,
+	.bhrb_nr		= 32,
 };
 
 static int __init init_power8_pmu(void)
-- 
1.7.11.7

^ permalink raw reply related

* linux-next: manual merge of the ppc-temp tree with the metag tree
From: Stephen Rothwell @ 2013-04-18  7:49 UTC (permalink / raw)
  To: Michael Ellerman
  Cc: Paul Bolle, James Hogan, linux-kernel, linux-next, ppc-dev

[-- Attachment #1: Type: text/plain, Size: 892 bytes --]

Hi Michael,

Today's linux-next merge of the ppc-temp tree got a conflict in
arch/metag/mm/Kconfig between commit 2b8660ed3bfe ("memblock: Kill
ARCH_POPULATES_NODE_MAP once more") from the metag tree and commit
45b02f8d94e3 ("memblock: kill "config MAX_ACTIVE_REGIONS"") from the
ppc-temp tree.

I fixed it up (see below) and can carry the fix as necessary (no action
is required).

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

diff --cc arch/metag/mm/Kconfig
index 794f26a,ccf2576..0000000
--- a/arch/metag/mm/Kconfig
+++ b/arch/metag/mm/Kconfig
@@@ -93,11 -93,9 +93,6 @@@ config ARCH_SPARSEMEM_ENABL
  config ARCH_SPARSEMEM_DEFAULT
  	def_bool y
  
- config MAX_ACTIVE_REGIONS
- 	int
- 	default "2" if SPARSEMEM
- 	default "1"
- 
 -config ARCH_POPULATES_NODE_MAP
 -	def_bool y
 -
  config ARCH_SELECT_MEMORY_MODEL
  	def_bool y
  

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply

* Re: [PATCH] powerpc/mm: Use tlbiel for 4KB pages
From: Benjamin Herrenschmidt @ 2013-04-18  6:33 UTC (permalink / raw)
  To: Gavin Shan; +Cc: linuxppc-dev
In-Reply-To: <1366263235-7787-1-git-send-email-shangw@linux.vnet.ibm.com>

On Thu, 2013-04-18 at 13:33 +0800, Gavin Shan wrote:
> The firmware should have supplied correct information for the kernel
> to figure out the page size array, which is traced by mmu_psize_defs[].
> Otherwise, the kernel will fail back to use solely 4KB page size
> and copy mmu_psize_defaults_old[] over to mmu_psize_defs[]. However,
> the "tlbiel" isn't enabled in mmu_psize_defaults_old[].

Did you actually find a problem here ? There is a reason we don't enable
tlbiel in there ... those old CPUs do not support it.

Ben.

> Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
> ---
>  arch/powerpc/mm/hash_utils_64.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/powerpc/mm/hash_utils_64.c b/arch/powerpc/mm/hash_utils_64.c
> index 2d4a20c..8675ae5 100644
> --- a/arch/powerpc/mm/hash_utils_64.c
> +++ b/arch/powerpc/mm/hash_utils_64.c
> @@ -130,7 +130,7 @@ static struct mmu_psize_def mmu_psize_defaults_old[] = {
>  		.sllp	= 0,
>  		.penc	= 0,
>  		.avpnm	= 0,
> -		.tlbiel = 0,
> +		.tlbiel = 1,
>  	},
>  };
>  

^ permalink raw reply

* Re: questions around Book III-E and branch trace
From: Benjamin Herrenschmidt @ 2013-04-18  6:28 UTC (permalink / raw)
  To: Chris Friesen; +Cc: Paul Mackerras, linuxppc-dev
In-Reply-To: <516EED8D.3040304@genband.com>

On Wed, 2013-04-17 at 12:44 -0600, Chris Friesen wrote:
> Hi,
> 
> I'm trying to wrap my head around how linux handles branch tracing on 
> Book III-E.  I think I understand how we set MSR[DE] and DBCR0[IDM|BT], 
> and how we handle fixing things up if an instruction being traced causes 
> an exception.

That code is a mess and it wouldn't surprise me if it was broken...

At this point, the people who care the most about it are FSL, so
that's where you have the most chance to find a satisfactory answer.

Cheers,
Ben.

> I have a few questions though:
> 
> 1) Does user_enable_block_step() have a bug in it?  The current code has
> 
> task->thread.dbcr0 = DBCR0_IDM | DBCR0_BT;
> 
> Should that be as follows (to match the singel-step case)?
> 
> task->thread.dbcr0 |= DBCR0_IDM | DBCR0_BT;
> 
> 
> 2) Why doesn't DBCR0_ACTIVE_EVENTS include DBCR0_BT?
> 
> 
> 3) In sys_debug_setcontext() why does SIG_DBG_BRANCH_TRACING return 
> -EINVAL if CONFIG_PPC_ADV_DEBUG_REGS is set?  Would it not be possible 
> to use DBCR0_BT?
> 
> Thanks,
> Chris
> 
> 

^ permalink raw reply

* Re: [PATCH] [RFC] powerpc: Add VDSO version of time
From: Michael Ellerman @ 2013-04-18  5:58 UTC (permalink / raw)
  To: Adhemerval Zanella; +Cc: linuxppc-dev
In-Reply-To: <5162CEBE.1020607@linux.vnet.ibm.com>

On Mon, Apr 08, 2013 at 11:05:50AM -0300, Adhemerval Zanella wrote:
> On 04/05/2013 03:21 AM, Michael Ellerman wrote:
> > On Tue, Mar 19, 2013 at 04:55:31PM -0300, Adhemerval Zanella wrote:
> >> Hi all,
> >>
> >> This patch implement the time syscall as vDSO. I have a glibc patch
> >> to use it as IFUNC (as latest gettimeofday patch). Below the perf
> >> numbers:
> >>
> >> Baseline PPC32: 380 nsec
> >> Baseline PPC64: 352 nsec
> >> vdso PPC32:      20 nsec
> >> vdso PPC64:      20 nsec
> >>
> >> I focused on 64 bit kernel, do I need to provide a scheme for 32 bits
> >> as well?
> > You did provide a 32-bit implementation. I take it you haven't tested
> > that though? Can you test it?
> 
> Hi,
> 
> I didn't build a 32 bit kernel, but I tested 32 bits binaries (that uses
> the VDSO32 implantation) without any issue. Performance gains are similar.

OK. Please send an updated version of the patch which a changelog
describing the testing you've done, and add your Signed-off-by to the
patch.

cheers

^ permalink raw reply

* [PATCH] powerpc/mm: Use tlbiel for 4KB pages
From: Gavin Shan @ 2013-04-18  5:33 UTC (permalink / raw)
  To: linuxppc-dev; +Cc: Gavin Shan

The firmware should have supplied correct information for the kernel
to figure out the page size array, which is traced by mmu_psize_defs[].
Otherwise, the kernel will fail back to use solely 4KB page size
and copy mmu_psize_defaults_old[] over to mmu_psize_defs[]. However,
the "tlbiel" isn't enabled in mmu_psize_defaults_old[].

Signed-off-by: Gavin Shan <shangw@linux.vnet.ibm.com>
---
 arch/powerpc/mm/hash_utils_64.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/mm/hash_utils_64.c b/arch/powerpc/mm/hash_utils_64.c
index 2d4a20c..8675ae5 100644
--- a/arch/powerpc/mm/hash_utils_64.c
+++ b/arch/powerpc/mm/hash_utils_64.c
@@ -130,7 +130,7 @@ static struct mmu_psize_def mmu_psize_defaults_old[] = {
 		.sllp	= 0,
 		.penc	= 0,
 		.avpnm	= 0,
-		.tlbiel = 0,
+		.tlbiel = 1,
 	},
 };
 
-- 
1.7.5.4

^ permalink raw reply related

* [PATCH v2] Erroneous double irq_eoi() on CPM IRQ in MPC8xx
From: Christophe Leroy @ 2013-04-18  5:26 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, Paul Mackerras, Vitaly Bordug,
	Marcelo Tosatti, Thomas Gleixner
  Cc: linuxppc-dev, linux-kernel

irq_eoi() is already called by generic_handle_irq() so 
it shall not be called a again

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>

Index: linux/arch/powerpc/platforms/8xx/m8xx_setup.c
===================================================================
--- linux/arch/powerpc/platforms/8xx/m8xx_setup.c	(revision 4802)
+++ linux/arch/powerpc/platforms/8xx/m8xx_setup.c	(working copy)
@@ -218,19 +218,12 @@
 
 static void cpm_cascade(unsigned int irq, struct irq_desc *desc)
 {
-	struct irq_chip *chip;
-	int cascade_irq;
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	int cascade_irq = cpm_get_irq();
 
-	if ((cascade_irq = cpm_get_irq()) >= 0) {
-		struct irq_desc *cdesc = irq_to_desc(cascade_irq);
-
+	if (cascade_irq >= 0)
 		generic_handle_irq(cascade_irq);
 
-		chip = irq_desc_get_chip(cdesc);
-		chip->irq_eoi(&cdesc->irq_data);
-	}
-
-	chip = irq_desc_get_chip(desc);
 	chip->irq_eoi(&desc->irq_data);
 }
 

^ permalink raw reply

* [Suggestion] PowerPC: kernel: memory access violation when rtas_data_buf contents are more than 1026
From: Chen Gang @ 2013-04-18  4:45 UTC (permalink / raw)
  To: Benjamin Herrenschmidt, paulus@samba.org, Al Viro
  Cc: sfr@canb.auug.org.au, linuxppc-dev@lists.ozlabs.org,
	linux-kernel@vger.kernel.org

Hello Maintainers:


in arch/powerpc/kernel/lparcfg.c, parse_system_parameter_string()

  need set '\0' for 'local_buffer'.

  the reason is:
    SPLPAR_MAXLENGTH is 1026, RTAS_DATA_BUF_SIZE is 4096
    the contents of rtas_data_buf may truncated in memcpy (line 301).

    if contents are truncated.
      the splpar_strlen is more than 1026 (line 321)
      the while loop checking will not find the end of buffer (line 326)
      it will cause memory access violation.


  I find it by reading code, so please help check.

  thanks.

gchen.

-------------------------related fix patch--------------------------------------

diff --git a/arch/powerpc/kernel/lparcfg.c b/arch/powerpc/kernel/lparcfg.c
index 801a757..d92f387 100644
--- a/arch/powerpc/kernel/lparcfg.c
+++ b/arch/powerpc/kernel/lparcfg.c
@@ -299,6 +299,7 @@ static void parse_system_parameter_string(struct seq_file *m)
 				__pa(rtas_data_buf),
 				RTAS_DATA_BUF_SIZE);
 	memcpy(local_buffer, rtas_data_buf, SPLPAR_MAXLENGTH);
+	local_buffer[SPLPAR_MAXLENGTH - 1] = '\0';
 	spin_unlock(&rtas_data_buf_lock);
 
 	if (call_status != 0) {



-------------------------related source code------------------------------------


283 static void parse_system_parameter_string(struct seq_file *m)
284 {
285         int call_status;
286 
287         unsigned char *local_buffer = kmalloc(SPLPAR_MAXLENGTH, GFP_KERNEL);
288         if (!local_buffer) {
289                 printk(KERN_ERR "%s %s kmalloc failure at line %d\n",
290                        __FILE__, __func__, __LINE__);
291                 return;
292         }
293 
294         spin_lock(&rtas_data_buf_lock);
295         memset(rtas_data_buf, 0, SPLPAR_MAXLENGTH);
296         call_status = rtas_call(rtas_token("ibm,get-system-parameter"), 3, 1,
297                                 NULL,
298                                 SPLPAR_CHARACTERISTICS_TOKEN,
299                                 __pa(rtas_data_buf),
300                                 RTAS_DATA_BUF_SIZE);
301         memcpy(local_buffer, rtas_data_buf, SPLPAR_MAXLENGTH);
302         spin_unlock(&rtas_data_buf_lock);
303         
304         if (call_status != 0) {
305                 printk(KERN_INFO
306                        "%s %s Error calling get-system-parameter (0x%x)\n",
307                        __FILE__, __func__, call_status);
308         } else {       
309                 int splpar_strlen;
310                 int idx, w_idx;
311                 char *workbuffer = kzalloc(SPLPAR_MAXLENGTH, GFP_KERNEL);
312                 if (!workbuffer) { 
313                         printk(KERN_ERR "%s %s kmalloc failure at line %d\n",
314                                __FILE__, __func__, __LINE__);
315                         kfree(local_buffer);
316                         return;
317                 }       
318 #ifdef LPARCFG_DEBUG
319                 printk(KERN_INFO "success calling get-system-parameter\n");
320 #endif
321                 splpar_strlen = local_buffer[0] * 256 + local_buffer[1];
322                 local_buffer += 2;      /* step over strlen value */
323 
324                 w_idx = 0;
325                 idx = 0;
326                 while ((*local_buffer) && (idx < splpar_strlen)) {
327                         workbuffer[w_idx++] = local_buffer[idx++];
328                         if ((local_buffer[idx] == ',')
329                             || (local_buffer[idx] == '\0')) {
330                                 workbuffer[w_idx] = '\0';
331                                 if (w_idx) {
332                                         /* avoid the empty string */
333                                         seq_printf(m, "%s\n", workbuffer);
334                                 }
335                                 memset(workbuffer, 0, SPLPAR_MAXLENGTH);
336                                 idx++;  /* skip the comma */
337                                 w_idx = 0;
338                         } else if (local_buffer[idx] == '=') {
339                                 /* code here to replace workbuffer contents
340                                    with different keyword strings */
341                                 if (0 == strcmp(workbuffer, "MaxEntCap")) {
342                                         strcpy(workbuffer,
343                                                "partition_max_entitled_capacity");
344                                         w_idx = strlen(workbuffer);
345                                 }
346                                 if (0 == strcmp(workbuffer, "MaxPlatProcs")) {
347                                         strcpy(workbuffer,
348                                                "system_potential_processors");
349                                         w_idx = strlen(workbuffer);
350                                 }
351                         }
352                 }
353                 kfree(workbuffer);
354                 local_buffer -= 2;      /* back up over strlen value */
355         }
356         kfree(local_buffer);
357 }

^ permalink raw reply related

* Re: [PATCH] powerpc: Add HWCAP2 aux entry
From: Michael Neuling @ 2013-04-18  3:41 UTC (permalink / raw)
  To: akpm
  Cc: vda.linux, Nishanth Aravamudan, Steve Munroe, linux-kernel,
	paulus, viro, Ryan Arnold, linuxppc-dev
In-Reply-To: <31546.1366255991@ale.ozlabs.ibm.com>

akpm,

If you're happy with this, is it something you can take in your tree?

Mikey

Michael Neuling <mikey@neuling.org> wrote:
> We are currently out of free bits in AT_HWCAP. With POWER8, we have
> several hardware features that we need to advertise. 
> 
> Tested on POWER and x86.
> 
> Signed-off-by: Michael Neuling <michael@neuling.org>
> Signed-off-by: Nishanth Aravamudan <nacc@linux.vnet.ibm.com>
> ---
> 
> > Wouldn't it be safer to not emit AT_HWCAP2 unless it is defined by the arch?
> > 
> > That way the change would only impact powerpc.
> 
> Should be addressed with this version.
> 
> Mikey
> 
> diff --git a/arch/powerpc/include/asm/cputable.h b/arch/powerpc/include/asm/cputable.h
> index fb3245e..ccadad6 100644
> --- a/arch/powerpc/include/asm/cputable.h
> +++ b/arch/powerpc/include/asm/cputable.h
> @@ -52,6 +52,7 @@ struct cpu_spec {
>  	char		*cpu_name;
>  	unsigned long	cpu_features;		/* Kernel features */
>  	unsigned int	cpu_user_features;	/* Userland features */
> +	unsigned int	cpu_user_features2;	/* Userland features v2 */
>  	unsigned int	mmu_features;		/* MMU features */
>  
>  	/* cache line sizes */
> diff --git a/arch/powerpc/include/asm/elf.h b/arch/powerpc/include/asm/elf.h
> index ac9790f..cc0655a 100644
> --- a/arch/powerpc/include/asm/elf.h
> +++ b/arch/powerpc/include/asm/elf.h
> @@ -61,6 +61,7 @@ typedef elf_vrregset_t elf_fpxregset_t;
>     instruction set this cpu supports.  This could be done in userspace,
>     but it's not easy, and we've already done it here.  */
>  # define ELF_HWCAP	(cur_cpu_spec->cpu_user_features)
> +# define ELF_HWCAP2	(cur_cpu_spec->cpu_user_features2)
>  
>  /* This yields a string that ld.so will use to load implementation
>     specific libraries for optimization.  This is more specific in
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 3939829..1f8b5d5 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -240,6 +240,9 @@ create_elf_tables(struct linux_binprm *bprm, struct elfhdr *exec,
>  	NEW_AUX_ENT(AT_EGID, from_kgid_munged(cred->user_ns, cred->egid));
>   	NEW_AUX_ENT(AT_SECURE, security_bprm_secureexec(bprm));
>  	NEW_AUX_ENT(AT_RANDOM, (elf_addr_t)(unsigned long)u_rand_bytes);
> +#ifdef ELF_HWCAP2
> +	NEW_AUX_ENT(AT_HWCAP2, ELF_HWCAP2);
> +#endif
>  	NEW_AUX_ENT(AT_EXECFN, bprm->exec);
>  	if (k_platform) {
>  		NEW_AUX_ENT(AT_PLATFORM,
> diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
> index 9c13e02..bf2381d 100644
> --- a/fs/binfmt_elf_fdpic.c
> +++ b/fs/binfmt_elf_fdpic.c
> @@ -483,7 +483,6 @@ static int create_elf_fdpic_tables(struct linux_binprm *bprm,
>  	size_t platform_len = 0, len;
>  	char *k_platform, *k_base_platform;
>  	char __user *u_platform, *u_base_platform, *p;
> -	long hwcap;
>  	int loop;
>  	int nr;	/* reset for each csp adjustment */
>  
> @@ -502,8 +501,6 @@ static int create_elf_fdpic_tables(struct linux_binprm *bprm,
>  		return -EFAULT;
>  #endif
>  
> -	hwcap = ELF_HWCAP;
> -
>  	/*
>  	 * If this architecture has a platform capability string, copy it
>  	 * to userspace.  In some cases (Sparc), this info is impossible
> @@ -617,7 +614,10 @@ static int create_elf_fdpic_tables(struct linux_binprm *bprm,
>  
>  	nr = 0;
>  	csp -= DLINFO_ITEMS * 2 * sizeof(unsigned long);
> -	NEW_AUX_ENT(AT_HWCAP,	hwcap);
> +	NEW_AUX_ENT(AT_HWCAP,	ELF_HWCAP);
> +#ifdef ELF_HWCAP2
> +	NEW_AUX_ENT(AT_HWCAP2,	ELF_HWCAP2);
> +#endif
>  	NEW_AUX_ENT(AT_PAGESZ,	PAGE_SIZE);
>  	NEW_AUX_ENT(AT_CLKTCK,	CLOCKS_PER_SEC);
>  	NEW_AUX_ENT(AT_PHDR,	exec_params->ph_addr);
> diff --git a/include/uapi/linux/auxvec.h b/include/uapi/linux/auxvec.h
> index 61594d5..835c065 100644
> --- a/include/uapi/linux/auxvec.h
> +++ b/include/uapi/linux/auxvec.h
> @@ -28,6 +28,7 @@
>  #define AT_BASE_PLATFORM 24	/* string identifying real platform, may
>  				 * differ from AT_PLATFORM. */
>  #define AT_RANDOM 25	/* address of 16 random bytes */
> +#define AT_HWCAP2 26	/* extension of AT_HWCAP */
>  
>  #define AT_EXECFN  31	/* filename of program */
>  
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
> 

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox