Re: [PATCH v6 01/17] powerpc/vas: Define macros, register fields and structures

[Michael Ellerman]

Sukadev Bhattiprolu  writes:

> Define macros for the VAS hardware registers and bit-fields as well
> as couple of data structures needed by the VAS driver.
>
> Signed-off-by: Sukadev Bhattiprolu 
> ---
> Changelog[v6]
>   - Add some fields for FTW windows
>
> Changelog[v4]
>   - [Michael Neuling] Move VAS code to arch/powerpc; Reorg vas.h and
> vas-internal.h to kernel and uapi versions; rather than creating
> separate properties for window context/address entries in device
> tree, combine them into "reg" properties; drop ->hwirq and irq_port
> fields from vas_window as they are only needed with user space
> windows.
>   - Drop the error check for CONFIG_PPC_4K_PAGES. Instead in a
> follow-on patch add a "depends on CONFIG_PPC_64K_PAGES".
>
> Changelog[v3]
>   - Rename winctx->pid to winctx->pidr to reflect that its a value
> from the PID register (SPRN_PID), not the linux process id.
>   - Make it easier to split header into kernel/user parts
>   - To keep user interface simple, use macros rather than enum for
> the threshold-control modes.
>   - Add a pid field to struct vas_window - needed for user space
> send windows.
>
> Changelog[v2]
>   - Add an overview of VAS in vas-internal.h
>   - Get window context parameters from device tree and drop
> unnecessary macros.
> ---
>  arch/powerpc/include/asm/vas.h   |  35 
>  arch/powerpc/include/uapi/asm/vas.h  |  25 +++

I thought we weren't exposing VAS to userspace yet?

If we are then we need to get things straight WRT copy/paste abort.

cheers

Re: [PATCH] powerpc/perf: double unlock bug in imc_common_cpuhp_mem_free()

[Madhavan Srinivasan]

On Monday 14 August 2017 09:00 AM, Michael Ellerman wrote:

Dan Carpenter  writes:


There is a typo so we call unlock instead of lock.

Fixes: 885dcd709ba9 ("powerpc/perf: Add nest IMC PMU support")
Signed-off-by: Dan Carpenter 
---
I also don't understand how the _imc_refc[node_id].lock works.  Why
can't we use ref->lock everywhere?  They seem equivalent, and my static
checker complains if we call the same lock different names.

That looks like a bug to me, ie. we should always use ref.


ok. will send a fix.

Thanks
Maddy



Maddy?

cheers


diff --git a/arch/powerpc/perf/imc-pmu.c b/arch/powerpc/perf/imc-pmu.c
index 46cd912af060..52017f6eafd9 100644
--- a/arch/powerpc/perf/imc-pmu.c
+++ b/arch/powerpc/perf/imc-pmu.c
@@ -1124,7 +1124,7 @@ static void cleanup_all_thread_imc_memory(void)
  static void imc_common_cpuhp_mem_free(struct imc_pmu *pmu_ptr)
  {
if (pmu_ptr->domain == IMC_DOMAIN_NEST) {
-   mutex_unlock(_init_lock);
+   mutex_lock(_init_lock);
if (nest_pmus == 1) {

cpuhp_remove_state(CPUHP_AP_PERF_POWERPC_NEST_IMC_ONLINE);
kfree(nest_imc_refc);

Re: [PATCH] powerpc/perf: double unlock bug in imc_common_cpuhp_mem_free()

[Madhavan Srinivasan]

On Saturday 12 August 2017 01:35 AM, Dan Carpenter wrote:

There is a typo so we call unlock instead of lock.


Reviewed-by: Madhavan Srinivasan 

nest_imc_refc used to maintain list of perf sessions thats using the
nest units currently. This is needed in turning off nest engine microcode
when not in use.

Yes will send a patch to fix ref->lock change.

Thanks for fix

Maddy



Fixes: 885dcd709ba9 ("powerpc/perf: Add nest IMC PMU support")
Signed-off-by: Dan Carpenter 
---
I also don't understand how the _imc_refc[node_id].lock works.  Why
can't we use ref->lock everywhere?  They seem equivalent, and my static
checker complains if we call the same lock different names.

diff --git a/arch/powerpc/perf/imc-pmu.c b/arch/powerpc/perf/imc-pmu.c
index 46cd912af060..52017f6eafd9 100644
--- a/arch/powerpc/perf/imc-pmu.c
+++ b/arch/powerpc/perf/imc-pmu.c
@@ -1124,7 +1124,7 @@ static void cleanup_all_thread_imc_memory(void)
  static void imc_common_cpuhp_mem_free(struct imc_pmu *pmu_ptr)
  {
if (pmu_ptr->domain == IMC_DOMAIN_NEST) {
-   mutex_unlock(_init_lock);
+   mutex_lock(_init_lock);
if (nest_pmus == 1) {

cpuhp_remove_state(CPUHP_AP_PERF_POWERPC_NEST_IMC_ONLINE);
kfree(nest_imc_refc);

Re: [PATCH] powerpc/perf: double unlock bug in imc_common_cpuhp_mem_free()

[Michael Ellerman]

Dan Carpenter  writes:

> There is a typo so we call unlock instead of lock.
>
> Fixes: 885dcd709ba9 ("powerpc/perf: Add nest IMC PMU support")
> Signed-off-by: Dan Carpenter 
> ---
> I also don't understand how the _imc_refc[node_id].lock works.  Why
> can't we use ref->lock everywhere?  They seem equivalent, and my static
> checker complains if we call the same lock different names.

That looks like a bug to me, ie. we should always use ref.

Maddy?

cheers

> diff --git a/arch/powerpc/perf/imc-pmu.c b/arch/powerpc/perf/imc-pmu.c
> index 46cd912af060..52017f6eafd9 100644
> --- a/arch/powerpc/perf/imc-pmu.c
> +++ b/arch/powerpc/perf/imc-pmu.c
> @@ -1124,7 +1124,7 @@ static void cleanup_all_thread_imc_memory(void)
>  static void imc_common_cpuhp_mem_free(struct imc_pmu *pmu_ptr)
>  {
>   if (pmu_ptr->domain == IMC_DOMAIN_NEST) {
> - mutex_unlock(_init_lock);
> + mutex_lock(_init_lock);
>   if (nest_pmus == 1) {
>   
> cpuhp_remove_state(CPUHP_AP_PERF_POWERPC_NEST_IMC_ONLINE);
>   kfree(nest_imc_refc);

Re: [PATCH 1/3] powerpc: simplify and fix VGA default device behaviour

[Daniel Axtens]

Hi Bjorn,

Thanks for reading the patch and providing some feedback.

>> This will work as intended if there is only one device, but if
>> there are multiple devices, we may override the device the VGA
>> arbiter picked.
>
> This quirk only runs on VGA class devices.  If there's more than one
> VGA device in the system, and we assume that firmware only enables
> PCI_COMMAND_IO or PCI_COMMAND_MEMORY on "the one initialized by
> firmware", which seems reasonable to me, I think the existing code
> does match the commit message.
>
> We set the first VGA device we find to be the default.  Then, if we
> find another VGA device that's enabled, we make *it* the default
> instead.
>
>> Instead, set a device as default if there is no default device AND
>> this device decodes.
>> 
>> This will not change behaviour on single-headed systems.
>
> If there is no enabled VGA device on the system, your new code means
> there will be no default VGA device.

Ah. Right you are.

>
> It's not clear from this changelog what problem this patch solves.
> Maybe it's the "some displays not being picked up by the VGA arbiter"
> you mentioned, but there's not enough detail to connect it with the
> patch, especially since the patch means we'll set the default device
> in fewer cases than we did before.
>
> With the patch, we only set the default if we find an enabled VGA
> device.  Previously we also set the default if we found a VGA device
> that had not been enabled.

My overall problem is not having default devices on some
machines. Initially, to solve this, I wanted to make this code
generic. To do that I wanted to make sure it played nice with the vga
arbiter, so not overriding default devices willy-nilly was a
requirement. Evidently, I was overly keen and restricted its behaviour
too much.

Regardless, in this current approach I don't make this powerpc code
generic after all, so this patch is unnecessary. I will remove it for
v2, which I will post after further testing.

I document the effects of the new approach for powerpc in more detail in
patch 3 of this series. If powerpc wants to keep the existing approach
we can arrange for that too; it's a simple matter of ifdefs.

Regards,
Daniel
>
>> Cc: Brian King 
>> Signed-off-by: Daniel Axtens 
>> 
>> ---
>> 
>> Tested in TCG (the card provided by qemu doesn't automatically
>> register with vgaarb, so the relevant code path has been tested)
>> but I would appreciate any tests on real hardware.
>> 
>> Informal benh ack: https://patchwork.kernel.org/patch/9850235/
>> ---
>>  arch/powerpc/kernel/pci-common.c | 5 -
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>> 
>> diff --git a/arch/powerpc/kernel/pci-common.c 
>> b/arch/powerpc/kernel/pci-common.c
>> index 341a7469cab8..c95fdda3a2dc 100644
>> --- a/arch/powerpc/kernel/pci-common.c
>> +++ b/arch/powerpc/kernel/pci-common.c
>> @@ -1746,8 +1746,11 @@ static void fixup_vga(struct pci_dev *pdev)
>>  {
>>  u16 cmd;
>>  
>> +if (vga_default_device())
>> +return;
>> +
>>  pci_read_config_word(pdev, PCI_COMMAND, );
>> -if ((cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) || 
>> !vga_default_device())
>> +if (cmd & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY))
>>  vga_set_default_device(pdev);
>>  
>>  }
>> -- 
>> 2.11.0
>> 

Re: [PATCH 2/2] powerpc/powernv/npu: Don't explicitly flush nmmu tlb

[kbuild test robot]

Hi Alistair,

[auto build test ERROR on powerpc/next]
[also build test ERROR on v4.13-rc4 next-20170811]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Alistair-Popple/powerpc-powernv-npu-Move-tlb-flush-before-launching-ATSD/20170813-211752
base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-defconfig (attached as .config)
compiler: powerpc64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget 
https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=powerpc 

All errors (new ones prefixed by >>):

   arch/powerpc/platforms/powernv/npu-dma.c: In function 
'pnv_npu2_init_context':
>> arch/powerpc/platforms/powernv/npu-dma.c:746:3: error: implicit declaration 
>> of function 'mm_context_set_global_tlbi' 
>> [-Werror=implicit-function-declaration]
  mm_context_set_global_tlbi(>context);
  ^~
   cc1: all warnings being treated as errors

vim +/mm_context_set_global_tlbi +746 arch/powerpc/platforms/powernv/npu-dma.c

   652  
   653  /*
   654   * Call into OPAL to setup the nmmu context for the current task in
   655   * the NPU. This must be called to setup the context tables before the
   656   * GPU issues ATRs. pdev should be a pointed to PCIe GPU device.
   657   *
   658   * A release callback should be registered to allow a device driver to
   659   * be notified that it should not launch any new translation requests
   660   * as the final TLB invalidate is about to occur.
   661   *
   662   * Returns an error if there no contexts are currently available or a
   663   * npu_context which should be passed to pnv_npu2_handle_fault().
   664   *
   665   * mmap_sem must be held in write mode.
   666   */
   667  struct npu_context *pnv_npu2_init_context(struct pci_dev *gpdev,
   668  unsigned long flags,
   669  struct npu_context *(*cb)(struct npu_context *, 
void *),
   670  void *priv)
   671  {
   672  int rc;
   673  u32 nvlink_index;
   674  struct device_node *nvlink_dn;
   675  struct mm_struct *mm = current->mm;
   676  struct pnv_phb *nphb;
   677  struct npu *npu;
   678  struct npu_context *npu_context;
   679  
   680  /*
   681   * At present we don't support GPUs connected to multiple NPUs 
and I'm
   682   * not sure the hardware does either.
   683   */
   684  struct pci_dev *npdev = pnv_pci_get_npu_dev(gpdev, 0);
   685  
   686  if (!firmware_has_feature(FW_FEATURE_OPAL))
   687  return ERR_PTR(-ENODEV);
   688  
   689  if (!npdev)
   690  /* No nvlink associated with this GPU device */
   691  return ERR_PTR(-ENODEV);
   692  
   693  if (!mm || mm->context.id == 0) {
   694  /*
   695   * Kernel thread contexts are not supported and context 
id 0 is
   696   * reserved on the GPU.
   697   */
   698  return ERR_PTR(-EINVAL);
   699  }
   700  
   701  nphb = pci_bus_to_host(npdev->bus)->private_data;
   702  npu = >npu;
   703  
   704  /*
   705   * Setup the NPU context table for a particular GPU. These need 
to be
   706   * per-GPU as we need the tables to filter ATSDs when there are 
no
   707   * active contexts on a particular GPU.
   708   */
   709  rc = opal_npu_init_context(nphb->opal_id, mm->context.id, flags,
   710  PCI_DEVID(gpdev->bus->number, 
gpdev->devfn));
   711  if (rc < 0)
   712  return ERR_PTR(-ENOSPC);
   713  
   714  /*
   715   * We store the npu pci device so we can more easily get at the
   716   * associated npus.
   717   */
   718  npu_context = mm->context.npu_context;
   719  if (!npu_context) {
   720  npu_context = kzalloc(sizeof(struct npu_context), 
GFP_KERNEL);
   721  if (!npu_context)
   722  return ERR_PTR(-ENOMEM);
   723  
   724  mm->context.npu_context = npu_context;
   725  npu_context->mm = mm;
   726  npu_context->mn.ops = _nmmu_notifier_ops;
   727  __mmu_notifier_register(_context->mn, mm);
   728  kref_init(_context->kref);
   729  } else {
   730  kref_get(_context->kref);
   731  }
   732  
   733  npu_context->release_cb = cb;
   734  

qustion about eeh_add_virt_device

[Julia Lawall]

Hello,

At the suggestion of Christoph Hellwig, I am working on inlining the
functions stored in the err_handler field of a pci_driver structure into
the pci_driver structure itself.  A number of functions in the file
arch/powerpc/kernel/eeh_driver.c have code like:

if (!driver->err_handler ||
!driver->err_handler->error_detected) {
eeh_pcid_put(dev);
return NULL;
}

This I would just convert to:

if (!driver->error_detected) {
eeh_pcid_put(dev);
return NULL;
}

But I am not sure what is best to do about eeh_add_virt_device, which
contains:

if (driver->err_handler)
return NULL;

Should I try to find a subfield of the err_handler that is guaranteed to
be there if anything is there?  Or could the test just be dropped, leaving
a direct return NULL?

thanks,
julia

Re: [PATCH] powerpc: store the intended structure

[Julia Lawall]

On Sun, 13 Aug 2017, Joe Perches wrote:

> On Sun, 2017-08-13 at 15:24 +0200, Julia Lawall wrote:
> > Normally the values in the resource field and the argument to ARRAY_SIZE
> > in the num_resources are the same.  In this case, the value in the reousrce
> > field is the same as the one in the previous platform_device structure, and
> > appears to be a copy-paste error.  Replace the value in the resource field
> > with the argument to the local call to ARRAY_SIZE.
>
> found by a script or eyeballs?

A script that was looking for something else.  But I wrote a script for
this specific issue and this was the only match.  I am currently checking
in a more general way.

julia

Re: [PATCH] powerpc: store the intended structure

[Joe Perches]

On Sun, 2017-08-13 at 15:24 +0200, Julia Lawall wrote:
> Normally the values in the resource field and the argument to ARRAY_SIZE
> in the num_resources are the same.  In this case, the value in the reousrce
> field is the same as the one in the previous platform_device structure, and
> appears to be a copy-paste error.  Replace the value in the resource field
> with the argument to the local call to ARRAY_SIZE.

found by a script or eyeballs?

[PATCH] powerpc: store the intended structure

[Julia Lawall]

Normally the values in the resource field and the argument to ARRAY_SIZE
in the num_resources are the same.  In this case, the value in the reousrce
field is the same as the one in the previous platform_device structure, and
appears to be a copy-paste error.  Replace the value in the resource field
with the argument to the local call to ARRAY_SIZE.

Signed-off-by: Julia Lawall 

---
 arch/powerpc/platforms/chrp/pegasos_eth.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/platforms/chrp/pegasos_eth.c 
b/arch/powerpc/platforms/chrp/pegasos_eth.c
index 2b4dc6a..1976071 100644
--- a/arch/powerpc/platforms/chrp/pegasos_eth.c
+++ b/arch/powerpc/platforms/chrp/pegasos_eth.c
@@ -63,7 +63,7 @@
.name   = "orion-mdio",
.id = -1,
.num_resources  = ARRAY_SIZE(mv643xx_eth_mvmdio_resources),
-   .resource   = mv643xx_eth_shared_resources,
+   .resource   = mv643xx_eth_mvmdio_resources,
 };
 
 static struct resource mv643xx_eth_port1_resources[] = {

[PATCH 05/11] serial: uuc_uart: constify uart_ops structures

[Julia Lawall]

These uart_ops structures are only stored in the ops field of a
uart_port structure and this fields is const, so the uart_ops
structures can also be const.

Done with the help of Coccinelle.

Signed-off-by: Julia Lawall 

---
 drivers/tty/serial/ucc_uart.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/ucc_uart.c b/drivers/tty/serial/ucc_uart.c
index 481eb29..55b7027 100644
--- a/drivers/tty/serial/ucc_uart.c
+++ b/drivers/tty/serial/ucc_uart.c
@@ -1085,7 +1085,7 @@ static int qe_uart_verify_port(struct uart_port *port,
  *
  * Details on these functions can be found in Documentation/serial/driver
  */
-static struct uart_ops qe_uart_pops = {
+static const struct uart_ops qe_uart_pops = {
.tx_empty   = qe_uart_tx_empty,
.set_mctrl  = qe_uart_set_mctrl,
.get_mctrl  = qe_uart_get_mctrl,

[PATCH 00/11] constify uart_ops structures

[Julia Lawall]

These uart_ops structures are only stored in the ops field of a
uart_port structure and this fields is const, so the uart_ops
structures can also be const.

Done with the help of Coccinelle.

---

 drivers/tty/serial/21285.c  |2 +-
 drivers/tty/serial/apbuart.c|2 +-
 drivers/tty/serial/cpm_uart/cpm_uart_core.c |2 +-
 drivers/tty/serial/m32r_sio.c   |2 +-
 drivers/tty/serial/meson_uart.c |2 +-
 drivers/tty/serial/mpc52xx_uart.c   |2 +-
 drivers/tty/serial/mux.c|2 +-
 drivers/tty/serial/owl-uart.c   |2 +-
 drivers/tty/serial/sunsab.c |2 +-
 drivers/tty/serial/sunsu.c  |2 +-
 drivers/tty/serial/ucc_uart.c   |2 +-
 11 files changed, 11 insertions(+), 11 deletions(-)