Re: [PATCH V5 3/3] PCI: Mask and unmask hotplug interrupts during reset

[okaya]

On 2018-07-03 06:52, p...@codeaurora.org wrote:

On 2018-07-03 14:04, Lukas Wunner wrote:

On Mon, Jul 02, 2018 at 06:52:47PM -0400, Sinan Kaya wrote:
If a bridge supports hotplug and observes a PCIe fatal error, the 
following

events happen:

1. AER driver removes the devices from PCI tree on fatal error
2. AER driver brings down the link by issuing a secondary bus reset 
waits

for the link to come up.
3. Hotplug driver observes a link down interrupt
4. Hotplug driver tries to remove the devices waiting for the rescan 
lock
but devices are already removed by the AER driver and AER driver is 
waiting

for the link to come back up.
5. AER driver tries to re-enumerate devices after polling for the 
link

state to go up.
6. Hotplug driver obtains the lock and tries to remove the devices 
again.


If a bridge is a hotplug capable bridge, mask hotplug interrupts 
before the

reset and unmask afterwards.


Would it work for you if you just amended the AER driver to skip
removal and re-enumeration of devices if the port is a hotplug bridge?
Just check for is_hotplug_bridge in struct pci_dev.



I tend to agree with you Lukas.

on this line I already have follow up patches
although I am waiting for Bjorn to review some patch-series before 
that.

[PATCH v2 0/6] Fix issues and cleanup for ERR_FATAL and ERR_NONFATAL

It doesn't look to me a an entirely a race condition since its guarded
by pci_lock_rescan_remove())
I observed that both hotplug and aer/dpc comes out of it in a quiet 
sane state.




To add more detail on when this issue happens.

This problem is more visible on root ports with MSI-x capability or with 
multiple MSI interrupt numbers.


AFAIK, QDT root ports are single shared MSI interrupt only. Therefore, 
you won't see this issue.


As you can see in the code, rescan lock is held for the entire fatal 
error handling path.



My thinking is: Disabling hotplug interrupts during ERR_FATAL,
is something little away from natural course of link_down event
handling, which is handled by pciehp more maturely.
so it would be just easy not to take any action e.g. removal and
re-enumeration of devices from ERR_FATAL handling point of view.



I think it is more unnatural to fragment code flow and allow two drivers 
to do the same thing in parallel or create inter-driver dependency.


I got the idea from pci_reset_slot() function which is already masking 
hotplug interrupts when called by external entries during secondary bus 
reset. We just didn't handle the same for fatal error cases.

Re: [PATCH V5 3/3] PCI: Mask and unmask hotplug interrupts during reset

[okaya]

On 2018-07-03 06:52, p...@codeaurora.org wrote:

On 2018-07-03 14:04, Lukas Wunner wrote:

On Mon, Jul 02, 2018 at 06:52:47PM -0400, Sinan Kaya wrote:
If a bridge supports hotplug and observes a PCIe fatal error, the 
following

events happen:

1. AER driver removes the devices from PCI tree on fatal error
2. AER driver brings down the link by issuing a secondary bus reset 
waits

for the link to come up.
3. Hotplug driver observes a link down interrupt
4. Hotplug driver tries to remove the devices waiting for the rescan 
lock
but devices are already removed by the AER driver and AER driver is 
waiting

for the link to come back up.
5. AER driver tries to re-enumerate devices after polling for the 
link

state to go up.
6. Hotplug driver obtains the lock and tries to remove the devices 
again.


If a bridge is a hotplug capable bridge, mask hotplug interrupts 
before the

reset and unmask afterwards.


Would it work for you if you just amended the AER driver to skip
removal and re-enumeration of devices if the port is a hotplug bridge?
Just check for is_hotplug_bridge in struct pci_dev.



I tend to agree with you Lukas.

on this line I already have follow up patches
although I am waiting for Bjorn to review some patch-series before 
that.

[PATCH v2 0/6] Fix issues and cleanup for ERR_FATAL and ERR_NONFATAL

It doesn't look to me a an entirely a race condition since its guarded
by pci_lock_rescan_remove())
I observed that both hotplug and aer/dpc comes out of it in a quiet 
sane state.




To add more detail on when this issue happens.

This problem is more visible on root ports with MSI-x capability or with 
multiple MSI interrupt numbers.


AFAIK, QDT root ports are single shared MSI interrupt only. Therefore, 
you won't see this issue.


As you can see in the code, rescan lock is held for the entire fatal 
error handling path.



My thinking is: Disabling hotplug interrupts during ERR_FATAL,
is something little away from natural course of link_down event
handling, which is handled by pciehp more maturely.
so it would be just easy not to take any action e.g. removal and
re-enumeration of devices from ERR_FATAL handling point of view.



I think it is more unnatural to fragment code flow and allow two drivers 
to do the same thing in parallel or create inter-driver dependency.


I got the idea from pci_reset_slot() function which is already masking 
hotplug interrupts when called by external entries during secondary bus 
reset. We just didn't handle the same for fatal error cases.

Re: [PATCH V5 3/3] PCI: Mask and unmask hotplug interrupts during reset

[okaya]

On 2018-07-03 04:34, Lukas Wunner wrote:

On Mon, Jul 02, 2018 at 06:52:47PM -0400, Sinan Kaya wrote:
If a bridge supports hotplug and observes a PCIe fatal error, the 
following

events happen:

1. AER driver removes the devices from PCI tree on fatal error
2. AER driver brings down the link by issuing a secondary bus reset 
waits

for the link to come up.
3. Hotplug driver observes a link down interrupt
4. Hotplug driver tries to remove the devices waiting for the rescan 
lock
but devices are already removed by the AER driver and AER driver is 
waiting

for the link to come back up.
5. AER driver tries to re-enumerate devices after polling for the link
state to go up.
6. Hotplug driver obtains the lock and tries to remove the devices 
again.


If a bridge is a hotplug capable bridge, mask hotplug interrupts 
before the

reset and unmask afterwards.


Would it work for you if you just amended the AER driver to skip
removal and re-enumeration of devices if the port is a hotplug bridge?
Just check for is_hotplug_bridge in struct pci_dev.


The reason why we want to remove devices before secondary bus reset is 
to quiesce pcie bus traffic before issuing a reset.


Skipping this step might cause transactions to be lost in the middle of 
the reset as there will be active traffic flowing and drivers will 
suddenly start reading ffs.


I don't think we can skip this step.




That would seem like a much simpler solution, given that it is known
that the link will flap on reset, causing the hotplug driver to remove
and re-enumerate devices.  That would also cover cases where hotplug is
handled by a different driver than pciehp, or by the platform firmware.

Thanks,

Lukas

Re: [PATCH V5 3/3] PCI: Mask and unmask hotplug interrupts during reset

[okaya]

On 2018-07-03 04:34, Lukas Wunner wrote:

On Mon, Jul 02, 2018 at 06:52:47PM -0400, Sinan Kaya wrote:
If a bridge supports hotplug and observes a PCIe fatal error, the 
following

events happen:

1. AER driver removes the devices from PCI tree on fatal error
2. AER driver brings down the link by issuing a secondary bus reset 
waits

for the link to come up.
3. Hotplug driver observes a link down interrupt
4. Hotplug driver tries to remove the devices waiting for the rescan 
lock
but devices are already removed by the AER driver and AER driver is 
waiting

for the link to come back up.
5. AER driver tries to re-enumerate devices after polling for the link
state to go up.
6. Hotplug driver obtains the lock and tries to remove the devices 
again.


If a bridge is a hotplug capable bridge, mask hotplug interrupts 
before the

reset and unmask afterwards.


Would it work for you if you just amended the AER driver to skip
removal and re-enumeration of devices if the port is a hotplug bridge?
Just check for is_hotplug_bridge in struct pci_dev.


The reason why we want to remove devices before secondary bus reset is 
to quiesce pcie bus traffic before issuing a reset.


Skipping this step might cause transactions to be lost in the middle of 
the reset as there will be active traffic flowing and drivers will 
suddenly start reading ffs.


I don't think we can skip this step.




That would seem like a much simpler solution, given that it is known
that the link will flap on reset, causing the hotplug driver to remove
and re-enumerate devices.  That would also cover cases where hotplug is
handled by a different driver than pciehp, or by the platform firmware.

Thanks,

Lukas

Re: [PATCH V4 7/7] PCI: Handle link reset via hotplug if supported

[okaya]

On 2018-07-01 13:14, Lukas Wunner wrote:

On Thu, Jun 28, 2018 at 03:31:05PM -0400, Sinan Kaya wrote:

+static pci_ers_result_t pciehp_reset_link(struct pci_dev *pdev)
+{
+   struct pcie_device *pciedev;
+   struct controller *ctrl;
+   struct device *devhp;
+   struct slot *slot;
+   int rc;
+
+   devhp = pcie_port_find_device(pdev, PCIE_PORT_SERVICE_HP);
+   pciedev = to_pcie_device(devhp);
+   ctrl = get_service_data(pciedev);
+   slot = ctrl->slot;
+
+   rc = reset_slot(slot->hotplug_slot, 0);
+
+   return !rc ? PCI_ERS_RESULT_RECOVERED : PCI_ERS_RESULT_DISCONNECT;
+}


This looks like a bit of a detour.  There's a "struct pci_slot *slot"
pointer in struct pci_dev.  Any reason not to simply call:

rc = reset_slot(pdev->slot->hotplug_slot)


pdev here is the bridge. Slot pointers are only valid for children 
objects.





Lukas

Re: [PATCH V4 7/7] PCI: Handle link reset via hotplug if supported

[okaya]

On 2018-07-01 13:14, Lukas Wunner wrote:

On Thu, Jun 28, 2018 at 03:31:05PM -0400, Sinan Kaya wrote:

+static pci_ers_result_t pciehp_reset_link(struct pci_dev *pdev)
+{
+   struct pcie_device *pciedev;
+   struct controller *ctrl;
+   struct device *devhp;
+   struct slot *slot;
+   int rc;
+
+   devhp = pcie_port_find_device(pdev, PCIE_PORT_SERVICE_HP);
+   pciedev = to_pcie_device(devhp);
+   ctrl = get_service_data(pciedev);
+   slot = ctrl->slot;
+
+   rc = reset_slot(slot->hotplug_slot, 0);
+
+   return !rc ? PCI_ERS_RESULT_RECOVERED : PCI_ERS_RESULT_DISCONNECT;
+}


This looks like a bit of a detour.  There's a "struct pci_slot *slot"
pointer in struct pci_dev.  Any reason not to simply call:

rc = reset_slot(pdev->slot->hotplug_slot)


pdev here is the bridge. Slot pointers are only valid for children 
objects.





Lukas

Re: ACPI support in common clock framework

[okaya]

On 2018-06-14 06:58, Srinath Mannam wrote:

Thank you Andy,

Regards,
Srinath.

On Wed, Jun 13, 2018 at 1:43 PM, Andy Shevchenko
 wrote:

+Cc: Rafael, ACPI ML

On Wed, Jun 13, 2018 at 7:14 AM, Srinath Mannam
 wrote:

Hi Michael, Stephen,

We are adding ACPI support in our Linux based platform.
At present our clock hierarchy using common clock framework through 
DTS.
Now we required ACPI support in common clock framework to upgrade our 
platform.


For example, clk_get API called in many drivers to get clock device 
is

tightly coupled with DT framework.

Please let us know, if anybody in Open Source community have plans to
add ACPI support for common clock framework.
If not please suggest us alternative method to use common clock
framework in ACPI use case.


Are you hooking up clk apis to PS0/PS3 calls?

Clk api didn't play nice with acpi until now.



Thanks in advance.

Regards,
Srinath.




--
With Best Regards,
Andy Shevchenko

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" 
in

the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: ACPI support in common clock framework

[okaya]

On 2018-06-14 06:58, Srinath Mannam wrote:

Thank you Andy,

Regards,
Srinath.

On Wed, Jun 13, 2018 at 1:43 PM, Andy Shevchenko
 wrote:

+Cc: Rafael, ACPI ML

On Wed, Jun 13, 2018 at 7:14 AM, Srinath Mannam
 wrote:

Hi Michael, Stephen,

We are adding ACPI support in our Linux based platform.
At present our clock hierarchy using common clock framework through 
DTS.
Now we required ACPI support in common clock framework to upgrade our 
platform.


For example, clk_get API called in many drivers to get clock device 
is

tightly coupled with DT framework.

Please let us know, if anybody in Open Source community have plans to
add ACPI support for common clock framework.
If not please suggest us alternative method to use common clock
framework in ACPI use case.


Are you hooking up clk apis to PS0/PS3 calls?

Clk api didn't play nice with acpi until now.



Thanks in advance.

Regards,
Srinath.




--
With Best Regards,
Andy Shevchenko

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" 
in

the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Requirement to get BAR pci_bus_address in user space

[okaya]

On 2018-06-14 06:29, Srinath Mannam wrote:

++ Alex Williamson, kvm,

Hi Christoph,

Thank you for quick reply.

If we want to add this in vfio then I think we need to do the same in
uio case also.

As I mentioned in previous mail, in the current implementation
resource information (address and size) is gathering from resource
named file created in /sys directory.
So I expect it would be better to have similar method as existing in 
sysfs.




Can you give some info on why you need the actual bar address value?



Regards,
Srinath.

On Thu, Jun 14, 2018 at 3:50 PM, Christoph Hellwig  wrote:

The only safe way to use PCI(e) devices in userspace is through vfio.
I think that is where you need to take your inquiries.

Re: Requirement to get BAR pci_bus_address in user space

[okaya]

On 2018-06-14 06:29, Srinath Mannam wrote:

++ Alex Williamson, kvm,

Hi Christoph,

Thank you for quick reply.

If we want to add this in vfio then I think we need to do the same in
uio case also.

As I mentioned in previous mail, in the current implementation
resource information (address and size) is gathering from resource
named file created in /sys directory.
So I expect it would be better to have similar method as existing in 
sysfs.




Can you give some info on why you need the actual bar address value?



Regards,
Srinath.

On Thu, Jun 14, 2018 at 3:50 PM, Christoph Hellwig  wrote:

The only safe way to use PCI(e) devices in userspace is through vfio.
I think that is where you need to take your inquiries.

Re: [PATCH] staging: rts5208: add check on NULL before dereference

[okaya]

On 2018-06-09 15:34, Andy Shevchenko wrote:

On Sat, Jun 9, 2018 at 7:58 PM,   wrote:

On 2018-06-09 12:38, Anton Vasilyev wrote:


If rtsx_probe fails to allocate dev->chip, then NULL pointer
dereference occurs at rtsx_release_resources().

Patch adds checks chip on NULL before its dereference at
rtsx_release_resources and passing with dereference inside
rtsx_release_chip.

Found by Linux Driver Verification project (linuxtesting.org).



I think you should bail out if dev->chip is null rather than adding
conditiinals.


I'm wondering if it's false positive. At which circumstances that may 
happen?


Only if dev->chip allocation fails. Code tries to cleanup prior 
resources by calling clean_everything() function which ends up in 
rtsx_release_resources()

Re: [PATCH] staging: rts5208: add check on NULL before dereference

[okaya]

On 2018-06-09 15:34, Andy Shevchenko wrote:

On Sat, Jun 9, 2018 at 7:58 PM,   wrote:

On 2018-06-09 12:38, Anton Vasilyev wrote:


If rtsx_probe fails to allocate dev->chip, then NULL pointer
dereference occurs at rtsx_release_resources().

Patch adds checks chip on NULL before its dereference at
rtsx_release_resources and passing with dereference inside
rtsx_release_chip.

Found by Linux Driver Verification project (linuxtesting.org).



I think you should bail out if dev->chip is null rather than adding
conditiinals.


I'm wondering if it's false positive. At which circumstances that may 
happen?


Only if dev->chip allocation fails. Code tries to cleanup prior 
resources by calling clean_everything() function which ends up in 
rtsx_release_resources()

Re: [PATCH] staging: rts5208: add check on NULL before dereference

[okaya]

On 2018-06-09 12:38, Anton Vasilyev wrote:

If rtsx_probe fails to allocate dev->chip, then NULL pointer
dereference occurs at rtsx_release_resources().

Patch adds checks chip on NULL before its dereference at
rtsx_release_resources and passing with dereference inside
rtsx_release_chip.

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Anton Vasilyev 
---
 drivers/staging/rts5208/rtsx.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/rts5208/rtsx.c 
b/drivers/staging/rts5208/rtsx.c

index 70e0b8623110..952dd0d580cf 100644
--- a/drivers/staging/rts5208/rtsx.c
+++ b/drivers/staging/rts5208/rtsx.c
@@ -623,12 +623,13 @@ static void rtsx_release_resources(struct 
rtsx_dev *dev)




I think you should bail out if dev->chip is null rather than adding 
conditiinals.




if (dev->irq > 0)
free_irq(dev->irq, (void *)dev);
-   if (dev->chip->msi_en)
+   if (dev->chip && dev->chip->msi_en)
pci_disable_msi(dev->pci);
if (dev->remap_addr)
iounmap(dev->remap_addr);
+   if (dev->chip)
+   rtsx_release_chip(dev->chip);

-   rtsx_release_chip(dev->chip);
kfree(dev->chip);
 }

Re: [PATCH] staging: rts5208: add check on NULL before dereference

[okaya]

On 2018-06-09 12:38, Anton Vasilyev wrote:

If rtsx_probe fails to allocate dev->chip, then NULL pointer
dereference occurs at rtsx_release_resources().

Patch adds checks chip on NULL before its dereference at
rtsx_release_resources and passing with dereference inside
rtsx_release_chip.

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Anton Vasilyev 
---
 drivers/staging/rts5208/rtsx.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/rts5208/rtsx.c 
b/drivers/staging/rts5208/rtsx.c

index 70e0b8623110..952dd0d580cf 100644
--- a/drivers/staging/rts5208/rtsx.c
+++ b/drivers/staging/rts5208/rtsx.c
@@ -623,12 +623,13 @@ static void rtsx_release_resources(struct 
rtsx_dev *dev)




I think you should bail out if dev->chip is null rather than adding 
conditiinals.




if (dev->irq > 0)
free_irq(dev->irq, (void *)dev);
-   if (dev->chip->msi_en)
+   if (dev->chip && dev->chip->msi_en)
pci_disable_msi(dev->pci);
if (dev->remap_addr)
iounmap(dev->remap_addr);
+   if (dev->chip)
+   rtsx_release_chip(dev->chip);

-   rtsx_release_chip(dev->chip);
kfree(dev->chip);
 }

Re: [PATCH NEXT 6/6] PCI/PORTDRV: Remove ERR_FATAL handling from pcie_portdrv_slot_reset()

[okaya]

On 2018-06-08 00:57, p...@codeaurora.org wrote:

On 2018-06-08 03:04, Bjorn Helgaas wrote:

On Thu, Jun 07, 2018 at 07:18:03PM +0530, p...@codeaurora.org wrote:

On 2018-06-07 11:30, Oza Pawandeep wrote:
> We are handling ERR_FATAL by resetting the Link in software,skipping the
> driver pci_error_handlers callbacks, removing the devices from the PCI
> subsystem, and re-enumerating, as a result of that, no more calling
> pcie_portdrv_slot_reset in ERR_FATAL case.
>
> Signed-off-by: Oza Pawandeep 
>
> diff --git a/drivers/pci/pcie/portdrv_pci.c
> b/drivers/pci/pcie/portdrv_pci.c
> index 973f1b8..92f5d330 100644
> --- a/drivers/pci/pcie/portdrv_pci.c
> +++ b/drivers/pci/pcie/portdrv_pci.c
> @@ -42,17 +42,6 @@ __setup("pcie_ports=", pcie_port_setup);
>
>  /* global data */
>
> -static int pcie_portdrv_restore_config(struct pci_dev *dev)
> -{
> -  int retval;
> -
> -  retval = pci_enable_device(dev);
> -  if (retval)
> -  return retval;
> -  pci_set_master(dev);
> -  return 0;
> -}
> -
>  #ifdef CONFIG_PM
>  static int pcie_port_runtime_suspend(struct device *dev)
>  {
> @@ -162,14 +151,6 @@ static pci_ers_result_t
> pcie_portdrv_mmio_enabled(struct pci_dev *dev)
>
>  static pci_ers_result_t pcie_portdrv_slot_reset(struct pci_dev *dev)
>  {
> -  /* If fatal, restore cfg space for possible link reset at upstream */
> -  if (dev->error_state == pci_channel_io_frozen) {
> -  dev->state_saved = true;
> -  pci_restore_state(dev);
> -  pcie_portdrv_restore_config(dev);
> -  pci_enable_pcie_error_reporting(dev);
> -  }
> -
>return PCI_ERS_RESULT_RECOVERED;
>  }


Hi Bjorn,

the above patch removes ERR_FATAL handling from 
pcie_portdrv_slot_reset()

because now we are handling ERR_FATAL differently than before.

I tried to dig into pcie_portdrv_slot_reset() handling for ERR_FATAL 
case

where it
restores the config space, enable device, set master and enable error
reporting
and as far as I understand this is being done for upstream link 
(bridges

etc..)

why was it done at the first point (I checked the commit description, 
but

could not really get it)
and do we need to handle the same thing in ERR_FATAL now ?


You mean 4bf3392e0bf5 ("PCI-Express AER implemetation: pcie_portdrv
error handler"), which added pcie_portdrv_slot_reset()?  I agree, that
commit log has no useful information.  I don't know any of the history
behind it.



Yes Bjorn thats right.
I am trying to understand it but no clue.
since it is restoring the stuffs in ERR_FATAL case, why would PCIe
bridge loose all the settings ?  [config space, aer bits, master,
device enable etc..)
Max we do is link_reset in ERR_FATAL case, and Secondary bus reset
should affect downstream components (not upstream)


Our first generation controller had this problem. There could be others 
too.

Re: [PATCH NEXT 6/6] PCI/PORTDRV: Remove ERR_FATAL handling from pcie_portdrv_slot_reset()

[okaya]

On 2018-06-08 00:57, p...@codeaurora.org wrote:

On 2018-06-08 03:04, Bjorn Helgaas wrote:

On Thu, Jun 07, 2018 at 07:18:03PM +0530, p...@codeaurora.org wrote:

On 2018-06-07 11:30, Oza Pawandeep wrote:
> We are handling ERR_FATAL by resetting the Link in software,skipping the
> driver pci_error_handlers callbacks, removing the devices from the PCI
> subsystem, and re-enumerating, as a result of that, no more calling
> pcie_portdrv_slot_reset in ERR_FATAL case.
>
> Signed-off-by: Oza Pawandeep 
>
> diff --git a/drivers/pci/pcie/portdrv_pci.c
> b/drivers/pci/pcie/portdrv_pci.c
> index 973f1b8..92f5d330 100644
> --- a/drivers/pci/pcie/portdrv_pci.c
> +++ b/drivers/pci/pcie/portdrv_pci.c
> @@ -42,17 +42,6 @@ __setup("pcie_ports=", pcie_port_setup);
>
>  /* global data */
>
> -static int pcie_portdrv_restore_config(struct pci_dev *dev)
> -{
> -  int retval;
> -
> -  retval = pci_enable_device(dev);
> -  if (retval)
> -  return retval;
> -  pci_set_master(dev);
> -  return 0;
> -}
> -
>  #ifdef CONFIG_PM
>  static int pcie_port_runtime_suspend(struct device *dev)
>  {
> @@ -162,14 +151,6 @@ static pci_ers_result_t
> pcie_portdrv_mmio_enabled(struct pci_dev *dev)
>
>  static pci_ers_result_t pcie_portdrv_slot_reset(struct pci_dev *dev)
>  {
> -  /* If fatal, restore cfg space for possible link reset at upstream */
> -  if (dev->error_state == pci_channel_io_frozen) {
> -  dev->state_saved = true;
> -  pci_restore_state(dev);
> -  pcie_portdrv_restore_config(dev);
> -  pci_enable_pcie_error_reporting(dev);
> -  }
> -
>return PCI_ERS_RESULT_RECOVERED;
>  }


Hi Bjorn,

the above patch removes ERR_FATAL handling from 
pcie_portdrv_slot_reset()

because now we are handling ERR_FATAL differently than before.

I tried to dig into pcie_portdrv_slot_reset() handling for ERR_FATAL 
case

where it
restores the config space, enable device, set master and enable error
reporting
and as far as I understand this is being done for upstream link 
(bridges

etc..)

why was it done at the first point (I checked the commit description, 
but

could not really get it)
and do we need to handle the same thing in ERR_FATAL now ?


You mean 4bf3392e0bf5 ("PCI-Express AER implemetation: pcie_portdrv
error handler"), which added pcie_portdrv_slot_reset()?  I agree, that
commit log has no useful information.  I don't know any of the history
behind it.



Yes Bjorn thats right.
I am trying to understand it but no clue.
since it is restoring the stuffs in ERR_FATAL case, why would PCIe
bridge loose all the settings ?  [config space, aer bits, master,
device enable etc..)
Max we do is link_reset in ERR_FATAL case, and Secondary bus reset
should affect downstream components (not upstream)


Our first generation controller had this problem. There could be others 
too.

Re: [PATCH V4] PCI: move early dump functionality from x86 arch into the common code

[okaya]

On 2018-06-05 05:12, Andy Shevchenko wrote:
On Tue, Jun 5, 2018 at 5:16 AM, Sinan Kaya  
wrote:
Move early dump functionality into common code so that it is available 
for
all archtiectures. No need to carry arch specific reads around as the 
read
hooks are already initialized by the time pci_setup_device() is 
getting

called during scan.



Makes sense.

Reviewed-by: Andy Shevchenko 

One style comment below, though.

If you wait a bit, I perhaps would be able to test on x86.


Sure, no rush. This is a nice to have feature with no urgency.




Signed-off-by: Sinan Kaya 
---
 Documentation/admin-guide/kernel-parameters.txt |  2 +-
 arch/x86/include/asm/pci-direct.h   |  4 ---
 arch/x86/kernel/setup.c |  5 ---
 arch/x86/pci/common.c   |  4 ---
 arch/x86/pci/early.c| 44 
-

 drivers/pci/pci.c   |  5 +++
 drivers/pci/pci.h   |  1 +
 drivers/pci/probe.c | 19 +++
 8 files changed, 26 insertions(+), 58 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt 
b/Documentation/admin-guide/kernel-parameters.txt

index e490902..e64f1d8 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2995,7 +2995,7 @@
See also Documentation/blockdev/paride.txt.

pci=option[,option...]  [PCI] various PCI subsystem options:
-   earlydump   [X86] dump PCI config space before the 
kernel
+   earlydump   dump PCI config space before the 
kernel

changes anything
off [X86] don't probe for the PCI bus
bios[X86-32] force use of PCI BIOS, don't 
access
diff --git a/arch/x86/include/asm/pci-direct.h 
b/arch/x86/include/asm/pci-direct.h

index e1084f7..94597a3 100644
--- a/arch/x86/include/asm/pci-direct.h
+++ b/arch/x86/include/asm/pci-direct.h
@@ -15,8 +15,4 @@ extern void write_pci_config_byte(u8 bus, u8 slot, 
u8 func, u8 offset, u8 val);
 extern void write_pci_config_16(u8 bus, u8 slot, u8 func, u8 offset, 
u16 val);


 extern int early_pci_allowed(void);
-
-extern unsigned int pci_early_dump_regs;
-extern void early_dump_pci_device(u8 bus, u8 slot, u8 func);
-extern void early_dump_pci_devices(void);
 #endif /* _ASM_X86_PCI_DIRECT_H */
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 2f86d88..480f250 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -991,11 +991,6 @@ void __init setup_arch(char **cmdline_p)
setup_clear_cpu_cap(X86_FEATURE_APIC);
}

-#ifdef CONFIG_PCI
-   if (pci_early_dump_regs)
-   early_dump_pci_devices();
-#endif
-
e820__reserve_setup_data();
e820__finish_early_params();

diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
index 563049c..d4ec117 100644
--- a/arch/x86/pci/common.c
+++ b/arch/x86/pci/common.c
@@ -22,7 +22,6 @@
 unsigned int pci_probe = PCI_PROBE_BIOS | PCI_PROBE_CONF1 | 
PCI_PROBE_CONF2 |

PCI_PROBE_MMCONF;

-unsigned int pci_early_dump_regs;
 static int pci_bf_sort;
 int pci_routeirq;
 int noioapicquirk;
@@ -599,9 +598,6 @@ char *__init pcibios_setup(char *str)
pci_probe |= PCI_BIG_ROOT_WINDOW;
return NULL;
 #endif
-   } else if (!strcmp(str, "earlydump")) {
-   pci_early_dump_regs = 1;
-   return NULL;
} else if (!strcmp(str, "routeirq")) {
pci_routeirq = 1;
return NULL;
diff --git a/arch/x86/pci/early.c b/arch/x86/pci/early.c
index e5f753c..f5fc953 100644
--- a/arch/x86/pci/early.c
+++ b/arch/x86/pci/early.c
@@ -57,47 +57,3 @@ int early_pci_allowed(void)
PCI_PROBE_CONF1;
 }

-void early_dump_pci_device(u8 bus, u8 slot, u8 func)
-{
-   u32 value[256 / 4];
-   int i;
-
-   pr_info("pci :%02x:%02x.%d config space:\n", bus, slot, 
func);

-
-   for (i = 0; i < 256; i += 4)
-   value[i / 4] = read_pci_config(bus, slot, func, i);
-
-   print_hex_dump(KERN_INFO, "", DUMP_PREFIX_OFFSET, 16, 1, 
value, 256, false);

-}
-
-void early_dump_pci_devices(void)
-{
-   unsigned bus, slot, func;
-
-   if (!early_pci_allowed())
-   return;
-
-   for (bus = 0; bus < 256; bus++) {
-   for (slot = 0; slot < 32; slot++) {
-   for (func = 0; func < 8; func++) {
-   u32 class;
-   u8 type;
-
-   class = read_pci_config(bus, slot, 
func,
-   
PCI_CLASS_REVISION);

-   if (class == 0x)
-   continue;
-
-   

Re: [PATCH V4] PCI: move early dump functionality from x86 arch into the common code

[okaya]

On 2018-06-05 05:12, Andy Shevchenko wrote:
On Tue, Jun 5, 2018 at 5:16 AM, Sinan Kaya  
wrote:
Move early dump functionality into common code so that it is available 
for
all archtiectures. No need to carry arch specific reads around as the 
read
hooks are already initialized by the time pci_setup_device() is 
getting

called during scan.



Makes sense.

Reviewed-by: Andy Shevchenko 

One style comment below, though.

If you wait a bit, I perhaps would be able to test on x86.


Sure, no rush. This is a nice to have feature with no urgency.




Signed-off-by: Sinan Kaya 
---
 Documentation/admin-guide/kernel-parameters.txt |  2 +-
 arch/x86/include/asm/pci-direct.h   |  4 ---
 arch/x86/kernel/setup.c |  5 ---
 arch/x86/pci/common.c   |  4 ---
 arch/x86/pci/early.c| 44 
-

 drivers/pci/pci.c   |  5 +++
 drivers/pci/pci.h   |  1 +
 drivers/pci/probe.c | 19 +++
 8 files changed, 26 insertions(+), 58 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt 
b/Documentation/admin-guide/kernel-parameters.txt

index e490902..e64f1d8 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2995,7 +2995,7 @@
See also Documentation/blockdev/paride.txt.

pci=option[,option...]  [PCI] various PCI subsystem options:
-   earlydump   [X86] dump PCI config space before the 
kernel
+   earlydump   dump PCI config space before the 
kernel

changes anything
off [X86] don't probe for the PCI bus
bios[X86-32] force use of PCI BIOS, don't 
access
diff --git a/arch/x86/include/asm/pci-direct.h 
b/arch/x86/include/asm/pci-direct.h

index e1084f7..94597a3 100644
--- a/arch/x86/include/asm/pci-direct.h
+++ b/arch/x86/include/asm/pci-direct.h
@@ -15,8 +15,4 @@ extern void write_pci_config_byte(u8 bus, u8 slot, 
u8 func, u8 offset, u8 val);
 extern void write_pci_config_16(u8 bus, u8 slot, u8 func, u8 offset, 
u16 val);


 extern int early_pci_allowed(void);
-
-extern unsigned int pci_early_dump_regs;
-extern void early_dump_pci_device(u8 bus, u8 slot, u8 func);
-extern void early_dump_pci_devices(void);
 #endif /* _ASM_X86_PCI_DIRECT_H */
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 2f86d88..480f250 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -991,11 +991,6 @@ void __init setup_arch(char **cmdline_p)
setup_clear_cpu_cap(X86_FEATURE_APIC);
}

-#ifdef CONFIG_PCI
-   if (pci_early_dump_regs)
-   early_dump_pci_devices();
-#endif
-
e820__reserve_setup_data();
e820__finish_early_params();

diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
index 563049c..d4ec117 100644
--- a/arch/x86/pci/common.c
+++ b/arch/x86/pci/common.c
@@ -22,7 +22,6 @@
 unsigned int pci_probe = PCI_PROBE_BIOS | PCI_PROBE_CONF1 | 
PCI_PROBE_CONF2 |

PCI_PROBE_MMCONF;

-unsigned int pci_early_dump_regs;
 static int pci_bf_sort;
 int pci_routeirq;
 int noioapicquirk;
@@ -599,9 +598,6 @@ char *__init pcibios_setup(char *str)
pci_probe |= PCI_BIG_ROOT_WINDOW;
return NULL;
 #endif
-   } else if (!strcmp(str, "earlydump")) {
-   pci_early_dump_regs = 1;
-   return NULL;
} else if (!strcmp(str, "routeirq")) {
pci_routeirq = 1;
return NULL;
diff --git a/arch/x86/pci/early.c b/arch/x86/pci/early.c
index e5f753c..f5fc953 100644
--- a/arch/x86/pci/early.c
+++ b/arch/x86/pci/early.c
@@ -57,47 +57,3 @@ int early_pci_allowed(void)
PCI_PROBE_CONF1;
 }

-void early_dump_pci_device(u8 bus, u8 slot, u8 func)
-{
-   u32 value[256 / 4];
-   int i;
-
-   pr_info("pci :%02x:%02x.%d config space:\n", bus, slot, 
func);

-
-   for (i = 0; i < 256; i += 4)
-   value[i / 4] = read_pci_config(bus, slot, func, i);
-
-   print_hex_dump(KERN_INFO, "", DUMP_PREFIX_OFFSET, 16, 1, 
value, 256, false);

-}
-
-void early_dump_pci_devices(void)
-{
-   unsigned bus, slot, func;
-
-   if (!early_pci_allowed())
-   return;
-
-   for (bus = 0; bus < 256; bus++) {
-   for (slot = 0; slot < 32; slot++) {
-   for (func = 0; func < 8; func++) {
-   u32 class;
-   u8 type;
-
-   class = read_pci_config(bus, slot, 
func,
-   
PCI_CLASS_REVISION);

-   if (class == 0x)
-   continue;
-
-   

Re: [PATCH V2] PCI/portdrv: do not disable device on reboot/shutdown

[okaya]

On 2018-05-25 15:10, Bjorn Helgaas wrote:

On Fri, May 25, 2018 at 09:30:59AM -0400, Sinan Kaya wrote:

On 5/24/2018 2:35 PM, Bjorn Helgaas wrote:
> That sounds like a reasonable idea, and it is definitely another can
> of worms.  I looked briefly at some of the .shutdown() cases:

should we throw it into 4.18 and see what happens?


It wouldn't solve this particular problem because hpsa *does* have a
.shutdown() method.  The problem is that it doesn't work -- it's
supposed to stop DMA and interrupts but it apparently doesn't.

I think we need to fix hpsa first.



Absolutely, the othet patch is a parallel issue.

Re: [PATCH V2] PCI/portdrv: do not disable device on reboot/shutdown

[okaya]

On 2018-05-25 15:10, Bjorn Helgaas wrote:

On Fri, May 25, 2018 at 09:30:59AM -0400, Sinan Kaya wrote:

On 5/24/2018 2:35 PM, Bjorn Helgaas wrote:
> That sounds like a reasonable idea, and it is definitely another can
> of worms.  I looked briefly at some of the .shutdown() cases:

should we throw it into 4.18 and see what happens?


It wouldn't solve this particular problem because hpsa *does* have a
.shutdown() method.  The problem is that it doesn't work -- it's
supposed to stop DMA and interrupts but it apparently doesn't.

I think we need to fix hpsa first.



Absolutely, the othet patch is a parallel issue.

Re: [PATCH V2] PCI/portdrv: do not disable device on reboot/shutdown

[okaya]

On 2018-05-24 09:07, Bjorn Helgaas wrote:

On Thu, May 24, 2018 at 07:43:05AM -0400, Sinan Kaya wrote:

On 5/23/2018 6:57 PM, Sinan Kaya wrote:
>> The crash seems to indicate that the hpsa device attempted a DMA after
>> we cleared the Root Port's PCI_COMMAND_MASTER, which means
>> hpsa_shutdown() didn't stop DMA from the device (it looks like *most*
>> shutdown methods don't disable device DMA, so it's in good company).
> All drivers are expected to shutdown DMA and interrupts in their shutdown()
> routines. They can skip removing threads, data structures etc. but DMA and
> interrupt disabling are required. This is the difference between shutdown()
> and remove() callbacks.

I found this note yesterday to see why we are not disabling the
devices in the PCI core itself.

pci_device_remove()

/*
 * We would love to complain here if pci_dev->is_enabled is set, that
 * the driver should have called pci_disable_device(), but the
 * unfortunate fact is there are too many odd BIOS and bridge setups
 * that don't like drivers doing that all of the time.
	 * Oh well, we can dream of sane hardware when we sleep, no matter 
how

 * horrible the crap we have to deal with is when we are awake...
 */

Ryan, can you discard the previous patch and test this one instead?
remove() path in hpsa driver seems to call pci_disable_device() via

hpsa_remove_one()
hpsa_free_pci_init()

but nothing on the shutdown path.

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 4ed3d26..3823f04 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -8651,6 +8651,7 @@ static void hpsa_shutdown(struct pci_dev *pdev)
h->access.set_intr_mask(h, HPSA_INTR_OFF);
hpsa_free_irqs(h);  /* init_one 4 */
hpsa_disable_interrupt_mode(h); /* pci_init 2 */
+   pci_disable_device(h->pdev);
 }


I suspect this will make things "work" (if the device can't attempt
DMA, no Unsupported Request error will occur).

But I'm concerned that the reason for the DMA might that hpsa is
transferring buffers from system memory to the hpsa device, and if we
arbitrarily terminate those transfers with pci_disable_device(), we
may leave the hpsa device in an inconsistent state, e.g., with a dirty
filesystem.

But we really need guidance from an hpsa expert.  I don't know the
filesystem/SCSI/hpsa details.


Agreed,

We can drop shutdown and use the remove callback. Remove is supposed to 
do a safe cleanup.




Bjorn

Re: [PATCH V2] PCI/portdrv: do not disable device on reboot/shutdown

[okaya]

On 2018-05-24 09:07, Bjorn Helgaas wrote:

On Thu, May 24, 2018 at 07:43:05AM -0400, Sinan Kaya wrote:

On 5/23/2018 6:57 PM, Sinan Kaya wrote:
>> The crash seems to indicate that the hpsa device attempted a DMA after
>> we cleared the Root Port's PCI_COMMAND_MASTER, which means
>> hpsa_shutdown() didn't stop DMA from the device (it looks like *most*
>> shutdown methods don't disable device DMA, so it's in good company).
> All drivers are expected to shutdown DMA and interrupts in their shutdown()
> routines. They can skip removing threads, data structures etc. but DMA and
> interrupt disabling are required. This is the difference between shutdown()
> and remove() callbacks.

I found this note yesterday to see why we are not disabling the
devices in the PCI core itself.

pci_device_remove()

/*
 * We would love to complain here if pci_dev->is_enabled is set, that
 * the driver should have called pci_disable_device(), but the
 * unfortunate fact is there are too many odd BIOS and bridge setups
 * that don't like drivers doing that all of the time.
	 * Oh well, we can dream of sane hardware when we sleep, no matter 
how

 * horrible the crap we have to deal with is when we are awake...
 */

Ryan, can you discard the previous patch and test this one instead?
remove() path in hpsa driver seems to call pci_disable_device() via

hpsa_remove_one()
hpsa_free_pci_init()

but nothing on the shutdown path.

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 4ed3d26..3823f04 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -8651,6 +8651,7 @@ static void hpsa_shutdown(struct pci_dev *pdev)
h->access.set_intr_mask(h, HPSA_INTR_OFF);
hpsa_free_irqs(h);  /* init_one 4 */
hpsa_disable_interrupt_mode(h); /* pci_init 2 */
+   pci_disable_device(h->pdev);
 }


I suspect this will make things "work" (if the device can't attempt
DMA, no Unsupported Request error will occur).

But I'm concerned that the reason for the DMA might that hpsa is
transferring buffers from system memory to the hpsa device, and if we
arbitrarily terminate those transfers with pci_disable_device(), we
may leave the hpsa device in an inconsistent state, e.g., with a dirty
filesystem.

But we really need guidance from an hpsa expert.  I don't know the
filesystem/SCSI/hpsa details.


Agreed,

We can drop shutdown and use the remove callback. Remove is supposed to 
do a safe cleanup.




Bjorn

Re: [PATCH v15 3/9] PCI/AER: Handle ERR_FATAL with removal and re-enumeration of devices

[okaya]

On 2018-05-10 14:10, Bjorn Helgaas wrote:

On Thu, May 10, 2018 at 12:31:16PM +0530, p...@codeaurora.org wrote:

On 2018-05-10 04:51, Bjorn Helgaas wrote:
> On Wed, May 09, 2018 at 06:44:53PM +0530, p...@codeaurora.org wrote:
> > On 2018-05-09 18:37, Bjorn Helgaas wrote:
> > > On Tue, May 08, 2018 at 06:53:30PM -0500, Bjorn Helgaas wrote:
> > > > On Thu, May 03, 2018 at 01:03:52AM -0400, Oza Pawandeep wrote:
> > > > > This patch alters the behavior of handling of ERR_FATAL, where removal
> > > > > of devices is initiated, followed by reset link, followed by
> > > > > re-enumeration.
> > > > >
> > > > > So the errors are handled in a different way as follows:
> > > > > ERR_NONFATAL => call driver recovery entry points
> > > > > ERR_FATAL=> remove and re-enumerate
> > > > >
> > > > > please refer to Documentation/PCI/pci-error-recovery.txt for more 
details.
> > > > >
> > > > > Signed-off-by: Oza Pawandeep 
> > > > >
> > > > > diff --git a/drivers/pci/pcie/aer/aerdrv.c 
b/drivers/pci/pcie/aer/aerdrv.c
> > > > > index 779b387..206f590 100644
> > > > > --- a/drivers/pci/pcie/aer/aerdrv.c
> > > > > +++ b/drivers/pci/pcie/aer/aerdrv.c
> > > > > @@ -330,6 +330,13 @@ static pci_ers_result_t aer_root_reset(struct 
pci_dev *dev)
> > > > > reg32 |= ROOT_PORT_INTR_ON_MESG_MASK;
> > > > > pci_write_config_dword(dev, pos + PCI_ERR_ROOT_COMMAND, 
reg32);
> > > > >
> > > > > +   /*
> > > > > +* This function is called only on ERR_FATAL now, and since
> > > > > +* the pci_report_resume is called only in ERR_NONFATAL case,
> > > > > +* the clearing part has to be taken care here.
> > > > > +*/
> > > > > +   aer_error_resume(dev);
> > > >
> > > > I don't understand this part.  Previously the ERR_FATAL path looked
> > > > like
> > > > this:
> > > >
> > > >   do_recovery
> > > > reset_link
> > > >   driver->reset_link
> > > > aer_root_reset
> > > >   pci_reset_bridge_secondary_bus# <-- reset
> > > > broadcast_error_message(..., report_resume)
> > > >   pci_walk_bus(..., report_resume, ...)
> > > > report_resume
> > > >   if (cb == report_resume)
> > > > pci_cleanup_aer_uncorrect_error_status
> > > >   pci_write_config_dword(PCI_ERR_UNCOR_STATUS)  # <-- clear
> > > > status
> > > >
> > > > After this patch, it will look like this:
> > > >
> > > >   do_recovery
> > > > do_fatal_recovery
> > > >   pci_cleanup_aer_uncorrect_error_status
> > > > pci_write_config_dword(PCI_ERR_UNCOR_STATUS)# <-- clear
> > > > status
> > > >   reset_link
> > > > driver->reset_link
> > > >   aer_root_reset
> > > > pci_reset_bridge_secondary_bus  # <-- reset
> > > > aer_error_resume
> > > >   pcie_capability_write_word(PCI_EXP_DEVSTA)#
> > > > <-- clear more
> > > >   pci_write_config_dword(PCI_ERR_UNCOR_STATUS)  #
> > > > <-- clear status
> > > >
> > > > So if I'm understanding correctly, the new path clears the status too
> > > > early, then clears it again (plus clearing DEVSTA, which we didn't do
> > > > before) later.
> > > >
> > > > I would think we would want to leave aer_root_reset() alone, and
> > > > just move
> > > > the pci_cleanup_aer_uncorrect_error_status() in do_fatal_recovery()
> > > > down so
> > > > it happens after we call reset_link().  That way the reset/clear
> > > > sequence
> > > > would be the same as it was before.
> > >
> > > I've been fiddling with this a bit myself and will post the results to
> > > see
> > > what you think.
> >
> >
> > ok so you are suggesting to move
> > pci_cleanup_aer_uncorrect_error_status down
> > which I can do.
> >
> > And not to call aer_error_resume, because you think its clearing the
> > status
> > again.
> >
> > following code: calls aer_error_resume.
> > pci_broadcast_error_message()
> >  /*
> >  * If the error is reported by an end point, we
> > think this
> >  * error is related to the upstream link of the end
> > point.
> >  */
> > if (state == pci_channel_io_normal)
> > /*
> >  * the error is non fatal so the bus is ok,
> > just
> > invoke
> >  * the callback for the function that logged
> > the
> > error.
> >  */
> > cb(dev, _data);
> > else
> > pci_walk_bus(dev->bus, cb, _data);
>
> Holy crap, I thought this could not possibly get any more complicated,
> but you're right; we do actually call aer_error_resume() today via an
> extremely convoluted path:
>
>   do_recovery(pci_dev)
> broadcast_error_message(..., error_detected, ...)
> if (AER_FATAL)
>   reset_link(pci_dev)
> udev = BRIDGE ? pci_dev : pci_dev->bus->self
> driver->reset_link(udev)
>   aer_root_reset(udev)

Re: [PATCH v15 3/9] PCI/AER: Handle ERR_FATAL with removal and re-enumeration of devices

[okaya]

On 2018-05-10 14:10, Bjorn Helgaas wrote:

On Thu, May 10, 2018 at 12:31:16PM +0530, p...@codeaurora.org wrote:

On 2018-05-10 04:51, Bjorn Helgaas wrote:
> On Wed, May 09, 2018 at 06:44:53PM +0530, p...@codeaurora.org wrote:
> > On 2018-05-09 18:37, Bjorn Helgaas wrote:
> > > On Tue, May 08, 2018 at 06:53:30PM -0500, Bjorn Helgaas wrote:
> > > > On Thu, May 03, 2018 at 01:03:52AM -0400, Oza Pawandeep wrote:
> > > > > This patch alters the behavior of handling of ERR_FATAL, where removal
> > > > > of devices is initiated, followed by reset link, followed by
> > > > > re-enumeration.
> > > > >
> > > > > So the errors are handled in a different way as follows:
> > > > > ERR_NONFATAL => call driver recovery entry points
> > > > > ERR_FATAL=> remove and re-enumerate
> > > > >
> > > > > please refer to Documentation/PCI/pci-error-recovery.txt for more 
details.
> > > > >
> > > > > Signed-off-by: Oza Pawandeep 
> > > > >
> > > > > diff --git a/drivers/pci/pcie/aer/aerdrv.c 
b/drivers/pci/pcie/aer/aerdrv.c
> > > > > index 779b387..206f590 100644
> > > > > --- a/drivers/pci/pcie/aer/aerdrv.c
> > > > > +++ b/drivers/pci/pcie/aer/aerdrv.c
> > > > > @@ -330,6 +330,13 @@ static pci_ers_result_t aer_root_reset(struct 
pci_dev *dev)
> > > > > reg32 |= ROOT_PORT_INTR_ON_MESG_MASK;
> > > > > pci_write_config_dword(dev, pos + PCI_ERR_ROOT_COMMAND, 
reg32);
> > > > >
> > > > > +   /*
> > > > > +* This function is called only on ERR_FATAL now, and since
> > > > > +* the pci_report_resume is called only in ERR_NONFATAL case,
> > > > > +* the clearing part has to be taken care here.
> > > > > +*/
> > > > > +   aer_error_resume(dev);
> > > >
> > > > I don't understand this part.  Previously the ERR_FATAL path looked
> > > > like
> > > > this:
> > > >
> > > >   do_recovery
> > > > reset_link
> > > >   driver->reset_link
> > > > aer_root_reset
> > > >   pci_reset_bridge_secondary_bus# <-- reset
> > > > broadcast_error_message(..., report_resume)
> > > >   pci_walk_bus(..., report_resume, ...)
> > > > report_resume
> > > >   if (cb == report_resume)
> > > > pci_cleanup_aer_uncorrect_error_status
> > > >   pci_write_config_dword(PCI_ERR_UNCOR_STATUS)  # <-- clear
> > > > status
> > > >
> > > > After this patch, it will look like this:
> > > >
> > > >   do_recovery
> > > > do_fatal_recovery
> > > >   pci_cleanup_aer_uncorrect_error_status
> > > > pci_write_config_dword(PCI_ERR_UNCOR_STATUS)# <-- clear
> > > > status
> > > >   reset_link
> > > > driver->reset_link
> > > >   aer_root_reset
> > > > pci_reset_bridge_secondary_bus  # <-- reset
> > > > aer_error_resume
> > > >   pcie_capability_write_word(PCI_EXP_DEVSTA)#
> > > > <-- clear more
> > > >   pci_write_config_dword(PCI_ERR_UNCOR_STATUS)  #
> > > > <-- clear status
> > > >
> > > > So if I'm understanding correctly, the new path clears the status too
> > > > early, then clears it again (plus clearing DEVSTA, which we didn't do
> > > > before) later.
> > > >
> > > > I would think we would want to leave aer_root_reset() alone, and
> > > > just move
> > > > the pci_cleanup_aer_uncorrect_error_status() in do_fatal_recovery()
> > > > down so
> > > > it happens after we call reset_link().  That way the reset/clear
> > > > sequence
> > > > would be the same as it was before.
> > >
> > > I've been fiddling with this a bit myself and will post the results to
> > > see
> > > what you think.
> >
> >
> > ok so you are suggesting to move
> > pci_cleanup_aer_uncorrect_error_status down
> > which I can do.
> >
> > And not to call aer_error_resume, because you think its clearing the
> > status
> > again.
> >
> > following code: calls aer_error_resume.
> > pci_broadcast_error_message()
> >  /*
> >  * If the error is reported by an end point, we
> > think this
> >  * error is related to the upstream link of the end
> > point.
> >  */
> > if (state == pci_channel_io_normal)
> > /*
> >  * the error is non fatal so the bus is ok,
> > just
> > invoke
> >  * the callback for the function that logged
> > the
> > error.
> >  */
> > cb(dev, _data);
> > else
> > pci_walk_bus(dev->bus, cb, _data);
>
> Holy crap, I thought this could not possibly get any more complicated,
> but you're right; we do actually call aer_error_resume() today via an
> extremely convoluted path:
>
>   do_recovery(pci_dev)
> broadcast_error_message(..., error_detected, ...)
> if (AER_FATAL)
>   reset_link(pci_dev)
> udev = BRIDGE ? pci_dev : pci_dev->bus->self
> driver->reset_link(udev)
>   aer_root_reset(udev)
> if 

Re: [PATCH] pci/aspm: Remove CONFIG_PCIEASPM_DEBUG

[okaya]

On 2018-05-09 00:01, Rajat Jain wrote:

Currently, the linux kernel disables ASPM when a device is
removed from the kernel. But it is not enabled again when
a new device is added on that slot even if it was originally
enabled (by the BIOS) when the system booted up (assuming
POLICY_DEFAULT).

This was earlier discussed here:
https://www.spinics.net/lists/linux-pci/msg60212.html

And some suggestions from Bjorn here:
https://www.spinics.net/lists/linux-pci/msg60541.html

This patch picks up one of the suggestion, to remove the
CONFIG_PCIEASPM_DEBUG and thus make the code always
avilable. This provides control to userspace to control
ASPM on a per slot / device basis using sysfs interface.

Signed-off-by: Rajat Jain 


Reviewed-by: Sinan Kaya 



---
 drivers/pci/pci.h| 5 -
 drivers/pci/pcie/Kconfig | 8 
 drivers/pci/pcie/aspm.c  | 2 --
 3 files changed, 15 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 023f7cf25bff..383d92a6b0fb 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -365,13 +365,8 @@ static inline void
pcie_aspm_pm_state_change(struct pci_dev *pdev) { }
 static inline void pcie_aspm_powersave_config_link(struct pci_dev 
*pdev) { }

 #endif

-#ifdef CONFIG_PCIEASPM_DEBUG
 void pcie_aspm_create_sysfs_dev_files(struct pci_dev *pdev);
 void pcie_aspm_remove_sysfs_dev_files(struct pci_dev *pdev);
-#else
-static inline void pcie_aspm_create_sysfs_dev_files(struct pci_dev 
*pdev) { }
-static inline void pcie_aspm_remove_sysfs_dev_files(struct pci_dev 
*pdev) { }

-#endif

 #ifdef CONFIG_PCIE_PTM
 void pci_ptm_init(struct pci_dev *dev);
diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig
index b12e28b3d8f9..089b9f559d88 100644
--- a/drivers/pci/pcie/Kconfig
+++ b/drivers/pci/pcie/Kconfig
@@ -46,14 +46,6 @@ config PCIEASPM

  When in doubt, say Y.

-config PCIEASPM_DEBUG
-   bool "Debug PCI Express ASPM"
-   depends on PCIEASPM
-   default n
-   help
- This enables PCI Express ASPM debug support. It will add per-device
- interface to control ASPM.
-
 choice
prompt "Default ASPM policy"
default PCIEASPM_DEFAULT
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index c687c817b47d..8ffc13d42baa 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1161,7 +1161,6 @@ static int pcie_aspm_get_policy(char *buffer,
const struct kernel_param *kp)
 module_param_call(policy, pcie_aspm_set_policy, pcie_aspm_get_policy,
NULL, 0644);

-#ifdef CONFIG_PCIEASPM_DEBUG
 static ssize_t link_state_show(struct device *dev,
struct device_attribute *attr,
char *buf)
@@ -1264,7 +1263,6 @@ void pcie_aspm_remove_sysfs_dev_files(struct
pci_dev *pdev)
sysfs_remove_file_from_group(>dev.kobj,
_attr_clk_ctl.attr, power_group);
 }
-#endif

 static int __init pcie_aspm_disable(char *str)
 {

Re: [PATCH] pci/aspm: Remove CONFIG_PCIEASPM_DEBUG

[okaya]

On 2018-05-09 00:01, Rajat Jain wrote:

Currently, the linux kernel disables ASPM when a device is
removed from the kernel. But it is not enabled again when
a new device is added on that slot even if it was originally
enabled (by the BIOS) when the system booted up (assuming
POLICY_DEFAULT).

This was earlier discussed here:
https://www.spinics.net/lists/linux-pci/msg60212.html

And some suggestions from Bjorn here:
https://www.spinics.net/lists/linux-pci/msg60541.html

This patch picks up one of the suggestion, to remove the
CONFIG_PCIEASPM_DEBUG and thus make the code always
avilable. This provides control to userspace to control
ASPM on a per slot / device basis using sysfs interface.

Signed-off-by: Rajat Jain 


Reviewed-by: Sinan Kaya 



---
 drivers/pci/pci.h| 5 -
 drivers/pci/pcie/Kconfig | 8 
 drivers/pci/pcie/aspm.c  | 2 --
 3 files changed, 15 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 023f7cf25bff..383d92a6b0fb 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -365,13 +365,8 @@ static inline void
pcie_aspm_pm_state_change(struct pci_dev *pdev) { }
 static inline void pcie_aspm_powersave_config_link(struct pci_dev 
*pdev) { }

 #endif

-#ifdef CONFIG_PCIEASPM_DEBUG
 void pcie_aspm_create_sysfs_dev_files(struct pci_dev *pdev);
 void pcie_aspm_remove_sysfs_dev_files(struct pci_dev *pdev);
-#else
-static inline void pcie_aspm_create_sysfs_dev_files(struct pci_dev 
*pdev) { }
-static inline void pcie_aspm_remove_sysfs_dev_files(struct pci_dev 
*pdev) { }

-#endif

 #ifdef CONFIG_PCIE_PTM
 void pci_ptm_init(struct pci_dev *dev);
diff --git a/drivers/pci/pcie/Kconfig b/drivers/pci/pcie/Kconfig
index b12e28b3d8f9..089b9f559d88 100644
--- a/drivers/pci/pcie/Kconfig
+++ b/drivers/pci/pcie/Kconfig
@@ -46,14 +46,6 @@ config PCIEASPM

  When in doubt, say Y.

-config PCIEASPM_DEBUG
-   bool "Debug PCI Express ASPM"
-   depends on PCIEASPM
-   default n
-   help
- This enables PCI Express ASPM debug support. It will add per-device
- interface to control ASPM.
-
 choice
prompt "Default ASPM policy"
default PCIEASPM_DEFAULT
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index c687c817b47d..8ffc13d42baa 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1161,7 +1161,6 @@ static int pcie_aspm_get_policy(char *buffer,
const struct kernel_param *kp)
 module_param_call(policy, pcie_aspm_set_policy, pcie_aspm_get_policy,
NULL, 0644);

-#ifdef CONFIG_PCIEASPM_DEBUG
 static ssize_t link_state_show(struct device *dev,
struct device_attribute *attr,
char *buf)
@@ -1264,7 +1263,6 @@ void pcie_aspm_remove_sysfs_dev_files(struct
pci_dev *pdev)
sysfs_remove_file_from_group(>dev.kobj,
_attr_clk_ctl.attr, power_group);
 }
-#endif

 static int __init pcie_aspm_disable(char *str)
 {

Re: [PATCH] PCI: pciehp: Add quirk for QDF2400 Command Completed erratum

[okaya]

On 2018-05-07 22:35, Bjorn Helgaas wrote:

On Sun, May 06, 2018 at 06:30:53AM -0400, Sinan Kaya wrote:

The QDF2400 controller does not set the Command Completed bit unless
writes to the Slot Command register change "Control" bits.  Command
Completed is never set for writes that only change software 
notification

"Enable" bits.  This results in timeouts like this:

pciehp :00:00.0:pcie004: Timeout on hotplug command 0x1038

Cc: sta...@vger.kernel.org
Signed-off-by: Sinan Kaya 


Since there's no bisection benefit for keeping these separate, I folded
this into the original quirk and added Mika's reviewed-by.

I also added the following ID patch and used PCI_VENDOR_ID_QCOM:


Thanks for the clean up.



commit 333c8c1216c1e7ead6af7b3d667b43eb425b5034
Author: Bjorn Helgaas 
Date:   Mon May 7 15:52:55 2018 -0500

PCI: Add Qualcomm vendor ID

Add the Qualcomm vendor ID to pci_ids.h and use it in quirks.

Signed-off-by: Bjorn Helgaas 

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 2990ad1e7c99..e7bf44515fd6 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -4361,8 +4361,8 @@ static const struct pci_dev_acs_enabled {
{ PCI_VENDOR_ID_INTEL, 0x15b7, pci_quirk_mf_endpoint_acs },
{ PCI_VENDOR_ID_INTEL, 0x15b8, pci_quirk_mf_endpoint_acs },
/* QCOM QDF2xxx root ports */
-   { 0x17cb, 0x400, pci_quirk_qcom_rp_acs },
-   { 0x17cb, 0x401, pci_quirk_qcom_rp_acs },
+   { PCI_VENDOR_ID_QCOM, 0x0400, pci_quirk_qcom_rp_acs },
+   { PCI_VENDOR_ID_QCOM, 0x0401, pci_quirk_qcom_rp_acs },
/* Intel PCH root ports */
{ PCI_VENDOR_ID_INTEL, PCI_ANY_ID, pci_quirk_intel_pch_acs },
{ PCI_VENDOR_ID_INTEL, PCI_ANY_ID, pci_quirk_intel_spt_pch_acs },
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index cc608fc55334..883cb7bf78aa 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -2387,6 +2387,8 @@

 #define PCI_VENDOR_ID_LENOVO   0x17aa

+#define PCI_VENDOR_ID_QCOM 0x17cb
+
 #define PCI_VENDOR_ID_CDNS 0x17cd

 #define PCI_VENDOR_ID_ARECA0x17d3


---
 drivers/pci/hotplug/pciehp_hpc.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c 
b/drivers/pci/hotplug/pciehp_hpc.c

index e70eba5..974a8f1 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -914,3 +914,9 @@ static void quirk_cmd_compl(struct pci_dev *pdev)
 }
 DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_VENDOR_ID_INTEL, PCI_ANY_ID,
  PCI_CLASS_BRIDGE_PCI, 8, quirk_cmd_compl);
+
+DECLARE_PCI_FIXUP_CLASS_EARLY(0x17cb, 0x400,
+ PCI_CLASS_BRIDGE_PCI, 8, quirk_cmd_compl);
+
+DECLARE_PCI_FIXUP_CLASS_EARLY(0x17cb, 0x401,
+ PCI_CLASS_BRIDGE_PCI, 8, quirk_cmd_compl);
--
2.7.4


___
linux-arm-kernel mailing list
linux-arm-ker...@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PCI: pciehp: Add quirk for QDF2400 Command Completed erratum

[okaya]

On 2018-05-07 22:35, Bjorn Helgaas wrote:

On Sun, May 06, 2018 at 06:30:53AM -0400, Sinan Kaya wrote:

The QDF2400 controller does not set the Command Completed bit unless
writes to the Slot Command register change "Control" bits.  Command
Completed is never set for writes that only change software 
notification

"Enable" bits.  This results in timeouts like this:

pciehp :00:00.0:pcie004: Timeout on hotplug command 0x1038

Cc: sta...@vger.kernel.org
Signed-off-by: Sinan Kaya 


Since there's no bisection benefit for keeping these separate, I folded
this into the original quirk and added Mika's reviewed-by.

I also added the following ID patch and used PCI_VENDOR_ID_QCOM:


Thanks for the clean up.



commit 333c8c1216c1e7ead6af7b3d667b43eb425b5034
Author: Bjorn Helgaas 
Date:   Mon May 7 15:52:55 2018 -0500

PCI: Add Qualcomm vendor ID

Add the Qualcomm vendor ID to pci_ids.h and use it in quirks.

Signed-off-by: Bjorn Helgaas 

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 2990ad1e7c99..e7bf44515fd6 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -4361,8 +4361,8 @@ static const struct pci_dev_acs_enabled {
{ PCI_VENDOR_ID_INTEL, 0x15b7, pci_quirk_mf_endpoint_acs },
{ PCI_VENDOR_ID_INTEL, 0x15b8, pci_quirk_mf_endpoint_acs },
/* QCOM QDF2xxx root ports */
-   { 0x17cb, 0x400, pci_quirk_qcom_rp_acs },
-   { 0x17cb, 0x401, pci_quirk_qcom_rp_acs },
+   { PCI_VENDOR_ID_QCOM, 0x0400, pci_quirk_qcom_rp_acs },
+   { PCI_VENDOR_ID_QCOM, 0x0401, pci_quirk_qcom_rp_acs },
/* Intel PCH root ports */
{ PCI_VENDOR_ID_INTEL, PCI_ANY_ID, pci_quirk_intel_pch_acs },
{ PCI_VENDOR_ID_INTEL, PCI_ANY_ID, pci_quirk_intel_spt_pch_acs },
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index cc608fc55334..883cb7bf78aa 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -2387,6 +2387,8 @@

 #define PCI_VENDOR_ID_LENOVO   0x17aa

+#define PCI_VENDOR_ID_QCOM 0x17cb
+
 #define PCI_VENDOR_ID_CDNS 0x17cd

 #define PCI_VENDOR_ID_ARECA0x17d3


---
 drivers/pci/hotplug/pciehp_hpc.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c 
b/drivers/pci/hotplug/pciehp_hpc.c

index e70eba5..974a8f1 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -914,3 +914,9 @@ static void quirk_cmd_compl(struct pci_dev *pdev)
 }
 DECLARE_PCI_FIXUP_CLASS_EARLY(PCI_VENDOR_ID_INTEL, PCI_ANY_ID,
  PCI_CLASS_BRIDGE_PCI, 8, quirk_cmd_compl);
+
+DECLARE_PCI_FIXUP_CLASS_EARLY(0x17cb, 0x400,
+ PCI_CLASS_BRIDGE_PCI, 8, quirk_cmd_compl);
+
+DECLARE_PCI_FIXUP_CLASS_EARLY(0x17cb, 0x401,
+ PCI_CLASS_BRIDGE_PCI, 8, quirk_cmd_compl);
--
2.7.4


___
linux-arm-kernel mailing list
linux-arm-ker...@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: pciehp 0000:00:1c.0:pcie004: Timeout on hotplug command 0x1038 (issued 65284 msec ago)

[okaya]

On 2018-05-04 14:33, Bjorn Helgaas wrote:

On Fri, May 04, 2018 at 07:37:40AM +0100, ok...@codeaurora.org wrote:

On 2018-05-04 03:45, Bjorn Helgaas wrote:
> On Thu, May 03, 2018 at 10:49:24AM +0200, Paul Menzel wrote:
> > On 04/27/18 21:22, Bjorn Helgaas wrote:
> > > [+cc Lukas, Sinan]
> >
> > > On Thu, Apr 26, 2018 at 12:17:53PM +0200, Paul Menzel wrote:
> >
> > > > On the Lenovo X60t, during resume from ACPI suspend and during 
shutdown, the
> > > > message below is shown in the logs.
> > > >
> > > >  pciehp :00:1c.0:pcie004: Timeout on hotplug command 0x1038 
(issued
> > > > 65284 msec ago)
> > >
> > > This is an Intel root port:
> > >
> > >00:1c.0 PCI bridge: Intel Corporation NM10/ICH7 Family PCI Express 
Port 1 (rev 02) (prog-if 00 [Normal decode])
> > >
> > > and probably has the CF118 erratum (see
> > > 
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=3461a068661c
> > > for details).  I bet if you changed "msecs" in pcie_wait_cmd() to 3
> > > you'd see a 30 second delay during shutdown because we write a command to
> > > tell the port not to generate any more hotplug interrupts, and we wait for
> > > that command to complete, but the port never tells us it has completed.
> > >
> > > Lukas reported a similar issue in
> > > https://lkml.kernel.org/r/20180112104929.ga10...@wunner.de, which we sort
> > > of worked around by assuming that Thunderbolt controllers never support
> > > that "command complete" interrupt (see
> > > 
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=493fb50e958c)
> > >
> > > Sinan mooted the idea of using a "no-wait" path of sending the "don't
> > > generate hotplug interrupts" command.  I think we should work on this
> > > idea a little more.  If we're shutting down the whole system, I can't
> > > believe there's much value in *anything* we do in the pciehp_remove()
> > > path.
> > >
> > > Maybe we should just get rid of pciehp_remove() (and probably
> > > pcie_port_remove_service() and the other service driver remove methods)
> > > completely.  That dates from when the service drivers could be modules 
that
> > > could be potentially unloaded, but unloading them hasn't been possible for
> > > years.
> > >
> > > As far as the resume path, my guess is that in pciehp_resume(), we
> > > write a command to enable interrupts, then it looks like we get a
> > > PCI_EXP_SLTSTA_DLLSC "Link Up" interrupt, and apparently we issue
> > > another command.  Not sure exactly what's going on here.
>
> > Thank you for the quick reply and sorry for only being able to test
> > it now.
> > Please find the relevant bits from the ACPI S3 suspend “action”
> > below. The
> > full log is attached.
>
> No problem.  I think we need to bite the bullet and just do a quirk
> for the Intel erratum.  I tried to avoid it by waiting for command
> completion lazily, but I think that ended up being unnecessarily
> clever and it didn't even solve the whole problem.
>
> Can you try the patch below?  I think it should solve the problem
> you're seeing.
> ...



> +  /* Assume all Intel controllers have erratum CF118 */
> +  if (pdev->vendor == PCI_VENDOR_ID_INTEL)
> +  ctrl->cc_erratum = 1;
> +

Can we build a table like quirks.c?

Qdf2400 root ports have the same problem. I will do a follow up patch 
once

this finds its way in.


Yes, definitely.  I intended to do that but got a little lazy.  What
do you think about the following?  Paul, if you haven't tested the
first patch, can you try this one instead?  The logic is pretty much
the same.



Yes, this works for me.



3461a068661c ("PCI: pciehp: Wait for hotplug command completion
lazily") mentions AMD and Nvidia devices with the same issue, but
unfortunately doesn't include any specifics.


commit b0d6f2230e12c85ae3b65a854a53c67c7c1f6406
Author: Bjorn Helgaas 
Date:   Thu May 3 18:39:38 2018 -0500

PCI: pciehp: Add quirk for Intel Command Completed erratum

The Intel CF118 erratum means the controller does not set the 
Command
Completed bit unless writes to the Slot Command register change 
"Control"
bits.  Command Completed is never set for writes that only change 
software

notification "Enable" bits.  This results in timeouts like this:

  pciehp :00:1c.0:pcie004: Timeout on hotplug command 0x1038
(issued 65284 msec ago)

When this erratum is present, avoid these timeouts by marking 
commands

"completed" immediately unless they change the "Control" bits.

Here's the text of the erratum from the Intel document:

  CF118PCIe Slot Status Register Command Completed bit not 
always
   updated on any configuration write to the Slot 
Control

   Register

  Problem: For PCIe root ports (devices 0 - 10) supporting 
hot-plug,
   the Slot Status Register (offset AAh) Command 
Completed
   (bit[4]) status is updated under the following 
condition:
  

Re: pciehp 0000:00:1c.0:pcie004: Timeout on hotplug command 0x1038 (issued 65284 msec ago)

[okaya]

On 2018-05-04 14:33, Bjorn Helgaas wrote:

On Fri, May 04, 2018 at 07:37:40AM +0100, ok...@codeaurora.org wrote:

On 2018-05-04 03:45, Bjorn Helgaas wrote:
> On Thu, May 03, 2018 at 10:49:24AM +0200, Paul Menzel wrote:
> > On 04/27/18 21:22, Bjorn Helgaas wrote:
> > > [+cc Lukas, Sinan]
> >
> > > On Thu, Apr 26, 2018 at 12:17:53PM +0200, Paul Menzel wrote:
> >
> > > > On the Lenovo X60t, during resume from ACPI suspend and during 
shutdown, the
> > > > message below is shown in the logs.
> > > >
> > > >  pciehp :00:1c.0:pcie004: Timeout on hotplug command 0x1038 
(issued
> > > > 65284 msec ago)
> > >
> > > This is an Intel root port:
> > >
> > >00:1c.0 PCI bridge: Intel Corporation NM10/ICH7 Family PCI Express 
Port 1 (rev 02) (prog-if 00 [Normal decode])
> > >
> > > and probably has the CF118 erratum (see
> > > 
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=3461a068661c
> > > for details).  I bet if you changed "msecs" in pcie_wait_cmd() to 3
> > > you'd see a 30 second delay during shutdown because we write a command to
> > > tell the port not to generate any more hotplug interrupts, and we wait for
> > > that command to complete, but the port never tells us it has completed.
> > >
> > > Lukas reported a similar issue in
> > > https://lkml.kernel.org/r/20180112104929.ga10...@wunner.de, which we sort
> > > of worked around by assuming that Thunderbolt controllers never support
> > > that "command complete" interrupt (see
> > > 
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=493fb50e958c)
> > >
> > > Sinan mooted the idea of using a "no-wait" path of sending the "don't
> > > generate hotplug interrupts" command.  I think we should work on this
> > > idea a little more.  If we're shutting down the whole system, I can't
> > > believe there's much value in *anything* we do in the pciehp_remove()
> > > path.
> > >
> > > Maybe we should just get rid of pciehp_remove() (and probably
> > > pcie_port_remove_service() and the other service driver remove methods)
> > > completely.  That dates from when the service drivers could be modules 
that
> > > could be potentially unloaded, but unloading them hasn't been possible for
> > > years.
> > >
> > > As far as the resume path, my guess is that in pciehp_resume(), we
> > > write a command to enable interrupts, then it looks like we get a
> > > PCI_EXP_SLTSTA_DLLSC "Link Up" interrupt, and apparently we issue
> > > another command.  Not sure exactly what's going on here.
>
> > Thank you for the quick reply and sorry for only being able to test
> > it now.
> > Please find the relevant bits from the ACPI S3 suspend “action”
> > below. The
> > full log is attached.
>
> No problem.  I think we need to bite the bullet and just do a quirk
> for the Intel erratum.  I tried to avoid it by waiting for command
> completion lazily, but I think that ended up being unnecessarily
> clever and it didn't even solve the whole problem.
>
> Can you try the patch below?  I think it should solve the problem
> you're seeing.
> ...



> +  /* Assume all Intel controllers have erratum CF118 */
> +  if (pdev->vendor == PCI_VENDOR_ID_INTEL)
> +  ctrl->cc_erratum = 1;
> +

Can we build a table like quirks.c?

Qdf2400 root ports have the same problem. I will do a follow up patch 
once

this finds its way in.


Yes, definitely.  I intended to do that but got a little lazy.  What
do you think about the following?  Paul, if you haven't tested the
first patch, can you try this one instead?  The logic is pretty much
the same.



Yes, this works for me.



3461a068661c ("PCI: pciehp: Wait for hotplug command completion
lazily") mentions AMD and Nvidia devices with the same issue, but
unfortunately doesn't include any specifics.


commit b0d6f2230e12c85ae3b65a854a53c67c7c1f6406
Author: Bjorn Helgaas 
Date:   Thu May 3 18:39:38 2018 -0500

PCI: pciehp: Add quirk for Intel Command Completed erratum

The Intel CF118 erratum means the controller does not set the 
Command
Completed bit unless writes to the Slot Command register change 
"Control"
bits.  Command Completed is never set for writes that only change 
software

notification "Enable" bits.  This results in timeouts like this:

  pciehp :00:1c.0:pcie004: Timeout on hotplug command 0x1038
(issued 65284 msec ago)

When this erratum is present, avoid these timeouts by marking 
commands

"completed" immediately unless they change the "Control" bits.

Here's the text of the erratum from the Intel document:

  CF118PCIe Slot Status Register Command Completed bit not 
always
   updated on any configuration write to the Slot 
Control

   Register

  Problem: For PCIe root ports (devices 0 - 10) supporting 
hot-plug,
   the Slot Status Register (offset AAh) Command 
Completed
   (bit[4]) status is updated under the following 
condition:
   IOH 

Re: pciehp 0000:00:1c.0:pcie004: Timeout on hotplug command 0x1038 (issued 65284 msec ago)

[okaya]

On 2018-05-04 03:45, Bjorn Helgaas wrote:

On Thu, May 03, 2018 at 10:49:24AM +0200, Paul Menzel wrote:

On 04/27/18 21:22, Bjorn Helgaas wrote:
> [+cc Lukas, Sinan]

> On Thu, Apr 26, 2018 at 12:17:53PM +0200, Paul Menzel wrote:

> > On the Lenovo X60t, during resume from ACPI suspend and during shutdown, the
> > message below is shown in the logs.
> >
> >  pciehp :00:1c.0:pcie004: Timeout on hotplug command 0x1038 (issued
> > 65284 msec ago)
>
> This is an Intel root port:
>
>00:1c.0 PCI bridge: Intel Corporation NM10/ICH7 Family PCI Express Port 1 
(rev 02) (prog-if 00 [Normal decode])
>
> and probably has the CF118 erratum (see
> 
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=3461a068661c
> for details).  I bet if you changed "msecs" in pcie_wait_cmd() to 3
> you'd see a 30 second delay during shutdown because we write a command to
> tell the port not to generate any more hotplug interrupts, and we wait for
> that command to complete, but the port never tells us it has completed.
>
> Lukas reported a similar issue in
> https://lkml.kernel.org/r/20180112104929.ga10...@wunner.de, which we sort
> of worked around by assuming that Thunderbolt controllers never support
> that "command complete" interrupt (see
> 
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=493fb50e958c)
>
> Sinan mooted the idea of using a "no-wait" path of sending the "don't
> generate hotplug interrupts" command.  I think we should work on this
> idea a little more.  If we're shutting down the whole system, I can't
> believe there's much value in *anything* we do in the pciehp_remove()
> path.
>
> Maybe we should just get rid of pciehp_remove() (and probably
> pcie_port_remove_service() and the other service driver remove methods)
> completely.  That dates from when the service drivers could be modules that
> could be potentially unloaded, but unloading them hasn't been possible for
> years.
>
> As far as the resume path, my guess is that in pciehp_resume(), we
> write a command to enable interrupts, then it looks like we get a
> PCI_EXP_SLTSTA_DLLSC "Link Up" interrupt, and apparently we issue
> another command.  Not sure exactly what's going on here.


Thank you for the quick reply and sorry for only being able to test it 
now.
Please find the relevant bits from the ACPI S3 suspend “action” below. 
The

full log is attached.


No problem.  I think we need to bite the bullet and just do a quirk
for the Intel erratum.  I tried to avoid it by waiting for command
completion lazily, but I think that ended up being unnecessarily
clever and it didn't even solve the whole problem.

Can you try the patch below?  I think it should solve the problem
you're seeing.


commit ec48a1e0b91ce68903c8ea4dce659d4fdf17ad06
Author: Bjorn Helgaas 
Date:   Thu May 3 18:39:38 2018 -0500

PCI: pciehp: Add quirk for Intel Command Completed erratum

The Intel CF118 erratum means the controller does not set the 
Command
Completed bit unless writes to the Slot Command register change 
"Control"
bits.  Command Completed is never set for writes that only change 
software

notification "Enable" bits.  This results in timeouts like this:

  pciehp :00:1c.0:pcie004: Timeout on hotplug command 0x1038
(issued 65284 msec ago)

When this erratum is present, avoid these timeouts by marking 
commands

"completed" immediately unless they change the "Control" bits.

Here's the text of the erratum from the Intel document:

  CF118PCIe Slot Status Register Command Completed bit not 
always
   updated on any configuration write to the Slot 
Control

   Register

  Problem: For PCIe root ports (devices 0 - 10) supporting 
hot-plug,
   the Slot Status Register (offset AAh) Command 
Completed
   (bit[4]) status is updated under the following 
condition:
   IOH will set Command Completed bit after delivering 
the new
   commands written in the Slot Controller register 
(offset
   A8h) to VPP. The IOH detects new commands written in 
Slot
   Control register by checking the change of value for 
Power
   Controller Control (bit[10]), Power Indicator 
Control
   (bits[9:8]), Attention Indicator Control 
(bits[7:6]), or
   Electromechanical Interlock Control (bit[11]) 
fields. Any
   other configuration writes to the Slot Control 
register
   without changing the values of these fields will not 
cause

   Command Completed bit to be set.

   The PCIe Base Specification Revision 2.0 or later 
describes
   the “Slot Control Register” in section 7.8.10, as 
follows
   (Reference section 7.8.10, Slot Control Register, 
Offset
   18h). In hot-plug capable 

Re: pciehp 0000:00:1c.0:pcie004: Timeout on hotplug command 0x1038 (issued 65284 msec ago)

[okaya]

On 2018-05-04 03:45, Bjorn Helgaas wrote:

On Thu, May 03, 2018 at 10:49:24AM +0200, Paul Menzel wrote:

On 04/27/18 21:22, Bjorn Helgaas wrote:
> [+cc Lukas, Sinan]

> On Thu, Apr 26, 2018 at 12:17:53PM +0200, Paul Menzel wrote:

> > On the Lenovo X60t, during resume from ACPI suspend and during shutdown, the
> > message below is shown in the logs.
> >
> >  pciehp :00:1c.0:pcie004: Timeout on hotplug command 0x1038 (issued
> > 65284 msec ago)
>
> This is an Intel root port:
>
>00:1c.0 PCI bridge: Intel Corporation NM10/ICH7 Family PCI Express Port 1 
(rev 02) (prog-if 00 [Normal decode])
>
> and probably has the CF118 erratum (see
> 
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=3461a068661c
> for details).  I bet if you changed "msecs" in pcie_wait_cmd() to 3
> you'd see a 30 second delay during shutdown because we write a command to
> tell the port not to generate any more hotplug interrupts, and we wait for
> that command to complete, but the port never tells us it has completed.
>
> Lukas reported a similar issue in
> https://lkml.kernel.org/r/20180112104929.ga10...@wunner.de, which we sort
> of worked around by assuming that Thunderbolt controllers never support
> that "command complete" interrupt (see
> 
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=493fb50e958c)
>
> Sinan mooted the idea of using a "no-wait" path of sending the "don't
> generate hotplug interrupts" command.  I think we should work on this
> idea a little more.  If we're shutting down the whole system, I can't
> believe there's much value in *anything* we do in the pciehp_remove()
> path.
>
> Maybe we should just get rid of pciehp_remove() (and probably
> pcie_port_remove_service() and the other service driver remove methods)
> completely.  That dates from when the service drivers could be modules that
> could be potentially unloaded, but unloading them hasn't been possible for
> years.
>
> As far as the resume path, my guess is that in pciehp_resume(), we
> write a command to enable interrupts, then it looks like we get a
> PCI_EXP_SLTSTA_DLLSC "Link Up" interrupt, and apparently we issue
> another command.  Not sure exactly what's going on here.


Thank you for the quick reply and sorry for only being able to test it 
now.
Please find the relevant bits from the ACPI S3 suspend “action” below. 
The

full log is attached.


No problem.  I think we need to bite the bullet and just do a quirk
for the Intel erratum.  I tried to avoid it by waiting for command
completion lazily, but I think that ended up being unnecessarily
clever and it didn't even solve the whole problem.

Can you try the patch below?  I think it should solve the problem
you're seeing.


commit ec48a1e0b91ce68903c8ea4dce659d4fdf17ad06
Author: Bjorn Helgaas 
Date:   Thu May 3 18:39:38 2018 -0500

PCI: pciehp: Add quirk for Intel Command Completed erratum

The Intel CF118 erratum means the controller does not set the 
Command
Completed bit unless writes to the Slot Command register change 
"Control"
bits.  Command Completed is never set for writes that only change 
software

notification "Enable" bits.  This results in timeouts like this:

  pciehp :00:1c.0:pcie004: Timeout on hotplug command 0x1038
(issued 65284 msec ago)

When this erratum is present, avoid these timeouts by marking 
commands

"completed" immediately unless they change the "Control" bits.

Here's the text of the erratum from the Intel document:

  CF118PCIe Slot Status Register Command Completed bit not 
always
   updated on any configuration write to the Slot 
Control

   Register

  Problem: For PCIe root ports (devices 0 - 10) supporting 
hot-plug,
   the Slot Status Register (offset AAh) Command 
Completed
   (bit[4]) status is updated under the following 
condition:
   IOH will set Command Completed bit after delivering 
the new
   commands written in the Slot Controller register 
(offset
   A8h) to VPP. The IOH detects new commands written in 
Slot
   Control register by checking the change of value for 
Power
   Controller Control (bit[10]), Power Indicator 
Control
   (bits[9:8]), Attention Indicator Control 
(bits[7:6]), or
   Electromechanical Interlock Control (bit[11]) 
fields. Any
   other configuration writes to the Slot Control 
register
   without changing the values of these fields will not 
cause

   Command Completed bit to be set.

   The PCIe Base Specification Revision 2.0 or later 
describes
   the “Slot Control Register” in section 7.8.10, as 
follows
   (Reference section 7.8.10, Slot Control Register, 
Offset
   18h). In hot-plug capable Downstream Ports, a 

Re: pciehp 0000:00:1c.0:pcie004: Timeout on hotplug command 0x1038 (issued 65284 msec ago)

[okaya]

On 2018-04-27 21:18, Dave Young wrote:

On 04/28/18 at 08:56am, Dave Young wrote:

On 04/27/18 at 04:12pm, Bjorn Helgaas wrote:
> [+cc Eric, Vivek, kexec list]
>
> On Fri, Apr 27, 2018 at 03:34:30PM -0400, Sinan Kaya wrote:
> > On 4/27/2018 3:22 PM, Bjorn Helgaas wrote:
> > > Sinan mooted the idea of using a "no-wait" path of sending the "don't
> > > generate hotplug interrupts" command.  I think we should work on this
> > > idea a little more.  If we're shutting down the whole system, I can't
> > > believe there's much value in *anything* we do in the pciehp_remove()
> > > path.
> > >
> > > Maybe we should just get rid of pciehp_remove() (and probably
> > > pcie_port_remove_service() and the other service driver remove methods)
> > > completely.  That dates from when the service drivers could be modules 
that


Hmm, if it is the remove() method then kexec does not use it.  kexec 
use

the shutdown() method instead.  I missed this details when I replied.


Portdrv hooks up remove handler to shutdown. That's why remove is 
getting called.





> > > could be potentially unloaded, but unloading them hasn't been possible for
> > > years.
> >
> > Shutdown path is also used for kexec. Leaving hotplug interrupts
> > pending is dangerous for the newly loaded kernel as it leaves
> > spurious interrupts during the new kernel boot.
> >
> > I think we should always disable the hotplug interrupt on shutdown.
> > We might think of not waiting for command-completion as a
> > middle-ground or go to polling path instead of interrupts all the
> > time.
>
> Ah, I forgot about the kexec path.  The kexec path is used for
> crashdump, too, so ideally the newly-loaded kernel would defend itself
> when possible so it doesn't depend on the original kernel doing things
> correctly.

It is true for kdump.  But kexec needs device shutdown.

>
> Seems like this question of whether to do things in the original
> kernel or the kexec-ed kernel comes up periodically, but I can never
> remember a definitive answer.  My initial reaction is that it'd be
> nice if we didn't have to do *any* shutdown in the original kernel,
> but I'm sure there are reasons that's not practical.

Devices sometimes assume it is in a good state initialized in firmware 
boot
phase, so we need a shutdown in 1st kernel so that kexec kernel can 
boot

correctly for those devices.  For kdump since kernel already panicked
and it is not reliable so we do as less as we can in the 1st kernel
crash path, but there are some special handling for kdump in various 
drivers
to reset the devices in 2nd kernel, eg. when it see "reset_devices" 
kernel parameter.


>
> I copied Eric (kexec maintainer) and Vivek (contact listed in
> Documentation/kdump/kdump.txt) in case they have suggestions or would
> consider some sort of Documentation/ update.
>
> Bjorn
>
> ___
> kexec mailing list
> ke...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

Thanks
Dave

___
kexec mailing list
ke...@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: pciehp 0000:00:1c.0:pcie004: Timeout on hotplug command 0x1038 (issued 65284 msec ago)

[okaya]

On 2018-04-27 21:18, Dave Young wrote:

On 04/28/18 at 08:56am, Dave Young wrote:

On 04/27/18 at 04:12pm, Bjorn Helgaas wrote:
> [+cc Eric, Vivek, kexec list]
>
> On Fri, Apr 27, 2018 at 03:34:30PM -0400, Sinan Kaya wrote:
> > On 4/27/2018 3:22 PM, Bjorn Helgaas wrote:
> > > Sinan mooted the idea of using a "no-wait" path of sending the "don't
> > > generate hotplug interrupts" command.  I think we should work on this
> > > idea a little more.  If we're shutting down the whole system, I can't
> > > believe there's much value in *anything* we do in the pciehp_remove()
> > > path.
> > >
> > > Maybe we should just get rid of pciehp_remove() (and probably
> > > pcie_port_remove_service() and the other service driver remove methods)
> > > completely.  That dates from when the service drivers could be modules 
that


Hmm, if it is the remove() method then kexec does not use it.  kexec 
use

the shutdown() method instead.  I missed this details when I replied.


Portdrv hooks up remove handler to shutdown. That's why remove is 
getting called.





> > > could be potentially unloaded, but unloading them hasn't been possible for
> > > years.
> >
> > Shutdown path is also used for kexec. Leaving hotplug interrupts
> > pending is dangerous for the newly loaded kernel as it leaves
> > spurious interrupts during the new kernel boot.
> >
> > I think we should always disable the hotplug interrupt on shutdown.
> > We might think of not waiting for command-completion as a
> > middle-ground or go to polling path instead of interrupts all the
> > time.
>
> Ah, I forgot about the kexec path.  The kexec path is used for
> crashdump, too, so ideally the newly-loaded kernel would defend itself
> when possible so it doesn't depend on the original kernel doing things
> correctly.

It is true for kdump.  But kexec needs device shutdown.

>
> Seems like this question of whether to do things in the original
> kernel or the kexec-ed kernel comes up periodically, but I can never
> remember a definitive answer.  My initial reaction is that it'd be
> nice if we didn't have to do *any* shutdown in the original kernel,
> but I'm sure there are reasons that's not practical.

Devices sometimes assume it is in a good state initialized in firmware 
boot
phase, so we need a shutdown in 1st kernel so that kexec kernel can 
boot

correctly for those devices.  For kdump since kernel already panicked
and it is not reliable so we do as less as we can in the 1st kernel
crash path, but there are some special handling for kdump in various 
drivers
to reset the devices in 2nd kernel, eg. when it see "reset_devices" 
kernel parameter.


>
> I copied Eric (kexec maintainer) and Vivek (contact listed in
> Documentation/kdump/kdump.txt) in case they have suggestions or would
> consider some sort of Documentation/ update.
>
> Bjorn
>
> ___
> kexec mailing list
> ke...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

Thanks
Dave

___
kexec mailing list
ke...@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] dmaengine: dmatest: Remove use of VLAs

[okaya]

On 2018-04-09 19:14, Laura Abbott wrote:

On 04/09/2018 03:48 PM, Sinan Kaya wrote:

On 4/9/2018 5:06 PM, Laura Abbott wrote:

+   /* dst_cnt can't be more than u8 */
+   dma_addr_t dma_pq[255];


This is 2k stack space on 64 bit architectures. Isn't that a lot?



Depends on your definition of 'a lot'. My assumption was that
since this was a test module there would be some willingness
to be a bit more generous. The problem is the array size is
based off of the parameters passed in, although oddly enough
it's based off of the minimum of two variables. If you have
a suggestion for a tighter bound we can use that. Another
option is to just switch to allocating the array with kmalloc.
That might be reasonable here since there's other setup
that happens before the test starts.


I think allocation is a better choice.



Thanks,
Laura

--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] dmaengine: dmatest: Remove use of VLAs

[okaya]

On 2018-04-09 19:14, Laura Abbott wrote:

On 04/09/2018 03:48 PM, Sinan Kaya wrote:

On 4/9/2018 5:06 PM, Laura Abbott wrote:

+   /* dst_cnt can't be more than u8 */
+   dma_addr_t dma_pq[255];


This is 2k stack space on 64 bit architectures. Isn't that a lot?



Depends on your definition of 'a lot'. My assumption was that
since this was a test module there would be some willingness
to be a bit more generous. The problem is the array size is
based off of the parameters passed in, although oddly enough
it's based off of the minimum of two variables. If you have
a suggestion for a tighter bound we can use that. Another
option is to just switch to allocating the array with kmalloc.
That might be reasonable here since there's other setup
that happens before the test starts.


I think allocation is a better choice.



Thanks,
Laura

--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/5] io: define several IO & PIO barrier types for the asm-generic version

[okaya]

On 2018-04-06 06:19, Arnd Bergmann wrote:
On Thu, Apr 5, 2018 at 3:09 PM, Sinan Kaya  
wrote:
Getting ready to harden readX()/writeX() and inX()/outX() semantics 
for the

generic implementation.

Defining two set of macros as __io_br() and __io_ar() to indicate 
actions

to be taken before and after MMIO read.

Defining two set of macros as __io_bw() and __io_aw() to indicate 
actions

to be taken before and after MMIO write.

Defining two set of macros as __io_pbw() and __io_paw() to indicate 
actions

to be taken before and after Port IO write.

Defining two set of macros as __io_pbr() and __io_par() to indicate 
actions

to be taken before and after Port IO read.

If rmb() is available for the architecture, prefer rmb() as the 
default

implementation of __io_ar()/__io_par().

If wmb() is available for the architecture, prefer wmb() as the 
default

implementation of __io_bw()/__io_pbw().

Signed-off-by: Sinan Kaya 


I've applied the series to my asm-generic tree now, I will give it a 
few days

in linux-next to see if any obvious regressions happen, and then send
a pull request.

Checking the list of architectures that are affected by this, I see
h8300, microblaze, nios2, openrisc, s390, sparc, um, unicore32,
and xtensa, all of which use asm-generic/io.h without overriding
the default readl/writel.

I would guess that at least s390 doesn't need the barriers
(maintainers on Cc now), but there may be others that want to
override the new barriers with weaker ones where an MMIO
access is guaranteed to serialize against DMA, or where
a specialized barrier for this case exists.

Looking over the asm-generic implementation once more now,
I wonder if we should change the relaxed accessors to not have
any barriers (back to the version before your series) rather than
defaulting them to having the same barriers as the regular
readl/writel.


I can do a follow up patch. You want to map them to raw api without any 
barriers as before. Right?





 Arnd

Re: [PATCH v4 1/5] io: define several IO & PIO barrier types for the asm-generic version

[okaya]

On 2018-04-06 06:19, Arnd Bergmann wrote:
On Thu, Apr 5, 2018 at 3:09 PM, Sinan Kaya  
wrote:
Getting ready to harden readX()/writeX() and inX()/outX() semantics 
for the

generic implementation.

Defining two set of macros as __io_br() and __io_ar() to indicate 
actions

to be taken before and after MMIO read.

Defining two set of macros as __io_bw() and __io_aw() to indicate 
actions

to be taken before and after MMIO write.

Defining two set of macros as __io_pbw() and __io_paw() to indicate 
actions

to be taken before and after Port IO write.

Defining two set of macros as __io_pbr() and __io_par() to indicate 
actions

to be taken before and after Port IO read.

If rmb() is available for the architecture, prefer rmb() as the 
default

implementation of __io_ar()/__io_par().

If wmb() is available for the architecture, prefer wmb() as the 
default

implementation of __io_bw()/__io_pbw().

Signed-off-by: Sinan Kaya 


I've applied the series to my asm-generic tree now, I will give it a 
few days

in linux-next to see if any obvious regressions happen, and then send
a pull request.

Checking the list of architectures that are affected by this, I see
h8300, microblaze, nios2, openrisc, s390, sparc, um, unicore32,
and xtensa, all of which use asm-generic/io.h without overriding
the default readl/writel.

I would guess that at least s390 doesn't need the barriers
(maintainers on Cc now), but there may be others that want to
override the new barriers with weaker ones where an MMIO
access is guaranteed to serialize against DMA, or where
a specialized barrier for this case exists.

Looking over the asm-generic implementation once more now,
I wonder if we should change the relaxed accessors to not have
any barriers (back to the version before your series) rather than
defaulting them to having the same barriers as the regular
readl/writel.


I can do a follow up patch. You want to map them to raw api without any 
barriers as before. Right?





 Arnd

Re: [PATCH v3 1/5] io: define several IO & PIO barrier types for the asm-generic version

[okaya]

On 2018-04-05 03:00, Arnd Bergmann wrote:
On Thu, Apr 5, 2018 at 1:58 AM, Sinan Kaya  
wrote:


Looks good, but I'd change the comments to ones that document exactly
what those barriers are for:


+#ifndef __io_ar
+#ifdef rmb
+/* prefer rmb() as the default implementation of __io_ar() if 
supported */

+#define __io_ar()  rmb()


/*
 * prevent prefetching of coherent DMA data ahead of a dma-complete */


+#ifndef __io_bw
+#ifdef wmb
+/* prefer wmb() as the default implementation of __io_bw() if 
supported */

+#define __io_bw()  wmb()
+#else


/* flush writes to coherent DMA data before possibly triggering a DMA 
read */



+#ifndef __io_aw
+#define __io_aw()  barrier()
+#endif


/* serialize device access against a spin_unlock, usually handled there 
*/




I will add these and post the next version.

The other four patches look perfect already.  What's the timing we need 
for

these patches? Are they 4.18 material, or do we need them in 4.17 and
stable kernels to work around known bugs?


I was hoping to get all arch stuff in for 4.17.

Driver developers started removing redundant wmb().



  Arnd

Re: [PATCH v3 1/5] io: define several IO & PIO barrier types for the asm-generic version

[okaya]

On 2018-04-05 03:00, Arnd Bergmann wrote:
On Thu, Apr 5, 2018 at 1:58 AM, Sinan Kaya  
wrote:


Looks good, but I'd change the comments to ones that document exactly
what those barriers are for:


+#ifndef __io_ar
+#ifdef rmb
+/* prefer rmb() as the default implementation of __io_ar() if 
supported */

+#define __io_ar()  rmb()


/*
 * prevent prefetching of coherent DMA data ahead of a dma-complete */


+#ifndef __io_bw
+#ifdef wmb
+/* prefer wmb() as the default implementation of __io_bw() if 
supported */

+#define __io_bw()  wmb()
+#else


/* flush writes to coherent DMA data before possibly triggering a DMA 
read */



+#ifndef __io_aw
+#define __io_aw()  barrier()
+#endif


/* serialize device access against a spin_unlock, usually handled there 
*/




I will add these and post the next version.

The other four patches look perfect already.  What's the timing we need 
for

these patches? Are they 4.18 material, or do we need them in 4.17 and
stable kernels to work around known bugs?


I was hoping to get all arch stuff in for 4.17.

Driver developers started removing redundant wmb().



  Arnd

Re: [PATCH v1] PCI/DPC: Rename from pcie-dpc.c to dpc.c.

[okaya]

On 2018-03-31 18:34, Bjorn Helgaas wrote:

From: Bjorn Helgaas 

Rename from pcie-dpc.c to dpc.c.  The path 
"drivers/pci/pcie/pcie-dpc.c"

has more occurrences of "pci" than necessary.



Makes perfect sense.


Signed-off-by: Bjorn Helgaas 
---
 drivers/pci/pcie/Makefile   |2
 drivers/pci/pcie/dpc.c  |  306 
+++
 drivers/pci/pcie/pcie-dpc.c |  306 
---

 3 files changed, 307 insertions(+), 307 deletions(-)
 create mode 100644 drivers/pci/pcie/dpc.c
 delete mode 100644 drivers/pci/pcie/pcie-dpc.c

diff --git a/drivers/pci/pcie/Makefile b/drivers/pci/pcie/Makefile
index 223e4c34c29a..88d49500e134 100644
--- a/drivers/pci/pcie/Makefile
+++ b/drivers/pci/pcie/Makefile
@@ -16,5 +16,5 @@ obj-$(CONFIG_PCIEAER) += aer/

 obj-$(CONFIG_PCIE_PME) += pme.o

-obj-$(CONFIG_PCIE_DPC) += pcie-dpc.o
+obj-$(CONFIG_PCIE_DPC) += dpc.o
 obj-$(CONFIG_PCIE_PTM) += ptm.o
diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
new file mode 100644
index ..38e40c6c576f
--- /dev/null
+++ b/drivers/pci/pcie/dpc.c
@@ -0,0 +1,306 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PCI Express Downstream Port Containment services driver
+ * Author: Keith Busch 
+ *
+ * Copyright (C) 2016 Intel Corp.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include "../pci.h"
+#include "aer/aerdrv.h"
+
+struct dpc_dev {
+   struct pcie_device  *dev;
+   struct work_struct  work;
+   u16 cap_pos;
+   boolrp_extensions;
+   u32 rp_pio_status;
+   u8  rp_log_size;
+};
+
+static const char * const rp_pio_error_string[] = {
+	"Configuration Request received UR Completion",	 /* Bit Position 0  
*/
+	"Configuration Request received CA Completion",	 /* Bit Position 1  
*/

+   "Configuration Request Completion Timeout",/* Bit Position 2  */
+   NULL,
+   NULL,
+   NULL,
+   NULL,
+   NULL,
+   "I/O Request received UR Completion",  /* Bit Position 8  */
+   "I/O Request received CA Completion",  /* Bit Position 9  */
+   "I/O Request Completion Timeout",  /* Bit Position 10 */
+   NULL,
+   NULL,
+   NULL,
+   NULL,
+   NULL,
+   "Memory Request received UR Completion",   /* Bit Position 16 */
+   "Memory Request received CA Completion",   /* Bit Position 17 */
+   "Memory Request Completion Timeout",   /* Bit Position 18 */
+};
+
+static int dpc_wait_rp_inactive(struct dpc_dev *dpc)
+{
+   unsigned long timeout = jiffies + HZ;
+   struct pci_dev *pdev = dpc->dev->port;
+   struct device *dev = >dev->device;
+   u16 cap = dpc->cap_pos, status;
+
+   pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, );
+   while (status & PCI_EXP_DPC_RP_BUSY &&
+   !time_after(jiffies, timeout)) {
+   msleep(10);
+   pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, );
+   }
+   if (status & PCI_EXP_DPC_RP_BUSY) {
+   dev_warn(dev, "DPC root port still busy\n");
+   return -EBUSY;
+   }
+   return 0;
+}
+
+static void dpc_wait_link_inactive(struct dpc_dev *dpc)
+{
+   unsigned long timeout = jiffies + HZ;
+   struct pci_dev *pdev = dpc->dev->port;
+   struct device *dev = >dev->device;
+   u16 lnk_status;
+
+   pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, _status);
+   while (lnk_status & PCI_EXP_LNKSTA_DLLLA &&
+   !time_after(jiffies, timeout)) {
+   msleep(10);
+   pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, _status);
+   }
+   if (lnk_status & PCI_EXP_LNKSTA_DLLLA)
+   dev_warn(dev, "Link state not disabled for DPC event\n");
+}
+
+static void dpc_work(struct work_struct *work)
+{
+   struct dpc_dev *dpc = container_of(work, struct dpc_dev, work);
+   struct pci_dev *dev, *temp, *pdev = dpc->dev->port;
+   struct pci_bus *parent = pdev->subordinate;
+   u16 cap = dpc->cap_pos, ctl;
+
+   pci_lock_rescan_remove();
+   list_for_each_entry_safe_reverse(dev, temp, >devices,
+bus_list) {
+   pci_dev_get(dev);
+   pci_dev_set_disconnected(dev, NULL);
+   if (pci_has_subordinate(dev))
+   pci_walk_bus(dev->subordinate,
+pci_dev_set_disconnected, NULL);
+   pci_stop_and_remove_bus_device(dev);
+   pci_dev_put(dev);
+   }
+   pci_unlock_rescan_remove();
+
+   dpc_wait_link_inactive(dpc);
+   if (dpc->rp_extensions && dpc_wait_rp_inactive(dpc))
+   return;
+   if (dpc->rp_extensions && dpc->rp_pio_status) {
+  

Re: [PATCH v1] PCI/DPC: Rename from pcie-dpc.c to dpc.c.

[okaya]

On 2018-03-31 18:34, Bjorn Helgaas wrote:

From: Bjorn Helgaas 

Rename from pcie-dpc.c to dpc.c.  The path 
"drivers/pci/pcie/pcie-dpc.c"

has more occurrences of "pci" than necessary.



Makes perfect sense.


Signed-off-by: Bjorn Helgaas 
---
 drivers/pci/pcie/Makefile   |2
 drivers/pci/pcie/dpc.c  |  306 
+++
 drivers/pci/pcie/pcie-dpc.c |  306 
---

 3 files changed, 307 insertions(+), 307 deletions(-)
 create mode 100644 drivers/pci/pcie/dpc.c
 delete mode 100644 drivers/pci/pcie/pcie-dpc.c

diff --git a/drivers/pci/pcie/Makefile b/drivers/pci/pcie/Makefile
index 223e4c34c29a..88d49500e134 100644
--- a/drivers/pci/pcie/Makefile
+++ b/drivers/pci/pcie/Makefile
@@ -16,5 +16,5 @@ obj-$(CONFIG_PCIEAER) += aer/

 obj-$(CONFIG_PCIE_PME) += pme.o

-obj-$(CONFIG_PCIE_DPC) += pcie-dpc.o
+obj-$(CONFIG_PCIE_DPC) += dpc.o
 obj-$(CONFIG_PCIE_PTM) += ptm.o
diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
new file mode 100644
index ..38e40c6c576f
--- /dev/null
+++ b/drivers/pci/pcie/dpc.c
@@ -0,0 +1,306 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * PCI Express Downstream Port Containment services driver
+ * Author: Keith Busch 
+ *
+ * Copyright (C) 2016 Intel Corp.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include "../pci.h"
+#include "aer/aerdrv.h"
+
+struct dpc_dev {
+   struct pcie_device  *dev;
+   struct work_struct  work;
+   u16 cap_pos;
+   boolrp_extensions;
+   u32 rp_pio_status;
+   u8  rp_log_size;
+};
+
+static const char * const rp_pio_error_string[] = {
+	"Configuration Request received UR Completion",	 /* Bit Position 0  
*/
+	"Configuration Request received CA Completion",	 /* Bit Position 1  
*/

+   "Configuration Request Completion Timeout",/* Bit Position 2  */
+   NULL,
+   NULL,
+   NULL,
+   NULL,
+   NULL,
+   "I/O Request received UR Completion",  /* Bit Position 8  */
+   "I/O Request received CA Completion",  /* Bit Position 9  */
+   "I/O Request Completion Timeout",  /* Bit Position 10 */
+   NULL,
+   NULL,
+   NULL,
+   NULL,
+   NULL,
+   "Memory Request received UR Completion",   /* Bit Position 16 */
+   "Memory Request received CA Completion",   /* Bit Position 17 */
+   "Memory Request Completion Timeout",   /* Bit Position 18 */
+};
+
+static int dpc_wait_rp_inactive(struct dpc_dev *dpc)
+{
+   unsigned long timeout = jiffies + HZ;
+   struct pci_dev *pdev = dpc->dev->port;
+   struct device *dev = >dev->device;
+   u16 cap = dpc->cap_pos, status;
+
+   pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, );
+   while (status & PCI_EXP_DPC_RP_BUSY &&
+   !time_after(jiffies, timeout)) {
+   msleep(10);
+   pci_read_config_word(pdev, cap + PCI_EXP_DPC_STATUS, );
+   }
+   if (status & PCI_EXP_DPC_RP_BUSY) {
+   dev_warn(dev, "DPC root port still busy\n");
+   return -EBUSY;
+   }
+   return 0;
+}
+
+static void dpc_wait_link_inactive(struct dpc_dev *dpc)
+{
+   unsigned long timeout = jiffies + HZ;
+   struct pci_dev *pdev = dpc->dev->port;
+   struct device *dev = >dev->device;
+   u16 lnk_status;
+
+   pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, _status);
+   while (lnk_status & PCI_EXP_LNKSTA_DLLLA &&
+   !time_after(jiffies, timeout)) {
+   msleep(10);
+   pcie_capability_read_word(pdev, PCI_EXP_LNKSTA, _status);
+   }
+   if (lnk_status & PCI_EXP_LNKSTA_DLLLA)
+   dev_warn(dev, "Link state not disabled for DPC event\n");
+}
+
+static void dpc_work(struct work_struct *work)
+{
+   struct dpc_dev *dpc = container_of(work, struct dpc_dev, work);
+   struct pci_dev *dev, *temp, *pdev = dpc->dev->port;
+   struct pci_bus *parent = pdev->subordinate;
+   u16 cap = dpc->cap_pos, ctl;
+
+   pci_lock_rescan_remove();
+   list_for_each_entry_safe_reverse(dev, temp, >devices,
+bus_list) {
+   pci_dev_get(dev);
+   pci_dev_set_disconnected(dev, NULL);
+   if (pci_has_subordinate(dev))
+   pci_walk_bus(dev->subordinate,
+pci_dev_set_disconnected, NULL);
+   pci_stop_and_remove_bus_device(dev);
+   pci_dev_put(dev);
+   }
+   pci_unlock_rescan_remove();
+
+   dpc_wait_link_inactive(dpc);
+   if (dpc->rp_extensions && dpc_wait_rp_inactive(dpc))
+   return;
+   if (dpc->rp_extensions && dpc->rp_pio_status) {
+   pci_write_config_dword(pdev, cap + PCI_EXP_DPC_RP_PIO_STATUS,
+ 

Re: [PATCH v4 17/17] net: ena: Eliminate duplicate barriers on weakly-ordered archs

[okaya]

On 2018-03-25 08:06, Belgazal, Netanel wrote:

I think you should either add a parameter to
ena_com_write_sq_doorbell() or add ena_com_write_sq_doorbell_rel().
Right now, you have unused function.


That is true. I got rid of ena_com_write_sq_doorbell_rel.



On 3/20/18, 4:43 AM, "Sinan Kaya"  wrote:

Code includes barrier() followed by writel(). writel() already has 
a

barrier
on some architectures like arm64.

This ends up CPU observing two barriers back to back before 
executing the

register write.

Create a new wrapper function with relaxed write operator. Use the 
new

wrapper when a write is following a barrier().

Since code already has an explicit barrier call, changing writel() 
to

writel_relaxed().

Signed-off-by: Sinan Kaya 
---
 drivers/net/ethernet/amazon/ena/ena_com.c |  6 --
 drivers/net/ethernet/amazon/ena/ena_eth_com.h | 22 
--

 drivers/net/ethernet/amazon/ena/ena_netdev.c  |  4 ++--
 3 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c
b/drivers/net/ethernet/amazon/ena/ena_com.c
index bf2de52..b6e628f 100644
--- a/drivers/net/ethernet/amazon/ena/ena_com.c
+++ b/drivers/net/ethernet/amazon/ena/ena_com.c
@@ -631,7 +631,8 @@ static u32 ena_com_reg_bar_read32(struct
ena_com_dev *ena_dev, u16 offset)
 */
wmb();

-	writel(mmio_read_reg, ena_dev->reg_bar + 
ENA_REGS_MMIO_REG_READ_OFF);

+   writel_relaxed(mmio_read_reg,
+  ena_dev->reg_bar + ENA_REGS_MMIO_REG_READ_OFF);

for (i = 0; i < timeout; i++) {
if (read_resp->req_id == mmio_read->seq_num)
@@ -1826,7 +1827,8 @@ void ena_com_aenq_intr_handler(struct
ena_com_dev *dev, void *data)

 	/* write the aenq doorbell after all AENQ descriptors were read 
*/

mb();
-	writel((u32)aenq->head, dev->reg_bar + 
ENA_REGS_AENQ_HEAD_DB_OFF);

+   writel_relaxed((u32)aenq->head,
+  dev->reg_bar + ENA_REGS_AENQ_HEAD_DB_OFF);
 }

 int ena_com_dev_reset(struct ena_com_dev *ena_dev,
diff --git a/drivers/net/ethernet/amazon/ena/ena_eth_com.h
b/drivers/net/ethernet/amazon/ena/ena_eth_com.h
index 2f76572..09ef7cd 100644
--- a/drivers/net/ethernet/amazon/ena/ena_eth_com.h
+++ b/drivers/net/ethernet/amazon/ena/ena_eth_com.h
@@ -107,7 +107,8 @@ static inline int
ena_com_sq_empty_space(struct ena_com_io_sq *io_sq)
return io_sq->q_depth - 1 - cnt;
 }

-static inline int ena_com_write_sq_doorbell(struct ena_com_io_sq 
*io_sq)
+static inline int ena_com_write_sq_doorbell(struct ena_com_io_sq 
*io_sq,

+   bool relaxed)
 {
u16 tail;

@@ -116,7 +117,24 @@ static inline int
ena_com_write_sq_doorbell(struct ena_com_io_sq *io_sq)
 	pr_debug("write submission queue doorbell for queue: %d tail: 
%d\n",

 io_sq->qid, tail);

-   writel(tail, io_sq->db_addr);
+   if (relaxed)
+   writel_relaxed(tail, io_sq->db_addr);
+   else
+   writel(tail, io_sq->db_addr);
+
+   return 0;
+}
+
+static inline int ena_com_write_sq_doorbell_rel(struct
ena_com_io_sq *io_sq)
+{
+   u16 tail;
+
+   tail = io_sq->tail;
+
+	pr_debug("write submission queue doorbell for queue: %d tail: 
%d\n",

+io_sq->qid, tail);
+
+   writel_relaxed(tail, io_sq->db_addr);

return 0;
 }
diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c
b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index 6975150..0530201 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -556,7 +556,7 @@ static int ena_refill_rx_bufs(struct ena_ring
*rx_ring, u32 num)
 * issue a doorbell
 */
wmb();
-   ena_com_write_sq_doorbell(rx_ring->ena_com_io_sq);
+   ena_com_write_sq_doorbell(rx_ring->ena_com_io_sq, true);
}

rx_ring->next_to_use = next_to_use;
@@ -2151,7 +2151,7 @@ static netdev_tx_t ena_start_xmit(struct
sk_buff *skb, struct net_device *dev)

if (netif_xmit_stopped(txq) || !skb->xmit_more) {
/* trigger the dma engine */
-   ena_com_write_sq_doorbell(tx_ring->ena_com_io_sq);
+   ena_com_write_sq_doorbell(tx_ring->ena_com_io_sq, false);
u64_stats_update_begin(_ring->syncp);
tx_ring->tx_stats.doorbells++;
u64_stats_update_end(_ring->syncp);
--
2.7.4

Re: [PATCH v4 17/17] net: ena: Eliminate duplicate barriers on weakly-ordered archs

[okaya]

On 2018-03-25 08:06, Belgazal, Netanel wrote:

I think you should either add a parameter to
ena_com_write_sq_doorbell() or add ena_com_write_sq_doorbell_rel().
Right now, you have unused function.


That is true. I got rid of ena_com_write_sq_doorbell_rel.



On 3/20/18, 4:43 AM, "Sinan Kaya"  wrote:

Code includes barrier() followed by writel(). writel() already has 
a

barrier
on some architectures like arm64.

This ends up CPU observing two barriers back to back before 
executing the

register write.

Create a new wrapper function with relaxed write operator. Use the 
new

wrapper when a write is following a barrier().

Since code already has an explicit barrier call, changing writel() 
to

writel_relaxed().

Signed-off-by: Sinan Kaya 
---
 drivers/net/ethernet/amazon/ena/ena_com.c |  6 --
 drivers/net/ethernet/amazon/ena/ena_eth_com.h | 22 
--

 drivers/net/ethernet/amazon/ena/ena_netdev.c  |  4 ++--
 3 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c
b/drivers/net/ethernet/amazon/ena/ena_com.c
index bf2de52..b6e628f 100644
--- a/drivers/net/ethernet/amazon/ena/ena_com.c
+++ b/drivers/net/ethernet/amazon/ena/ena_com.c
@@ -631,7 +631,8 @@ static u32 ena_com_reg_bar_read32(struct
ena_com_dev *ena_dev, u16 offset)
 */
wmb();

-	writel(mmio_read_reg, ena_dev->reg_bar + 
ENA_REGS_MMIO_REG_READ_OFF);

+   writel_relaxed(mmio_read_reg,
+  ena_dev->reg_bar + ENA_REGS_MMIO_REG_READ_OFF);

for (i = 0; i < timeout; i++) {
if (read_resp->req_id == mmio_read->seq_num)
@@ -1826,7 +1827,8 @@ void ena_com_aenq_intr_handler(struct
ena_com_dev *dev, void *data)

 	/* write the aenq doorbell after all AENQ descriptors were read 
*/

mb();
-	writel((u32)aenq->head, dev->reg_bar + 
ENA_REGS_AENQ_HEAD_DB_OFF);

+   writel_relaxed((u32)aenq->head,
+  dev->reg_bar + ENA_REGS_AENQ_HEAD_DB_OFF);
 }

 int ena_com_dev_reset(struct ena_com_dev *ena_dev,
diff --git a/drivers/net/ethernet/amazon/ena/ena_eth_com.h
b/drivers/net/ethernet/amazon/ena/ena_eth_com.h
index 2f76572..09ef7cd 100644
--- a/drivers/net/ethernet/amazon/ena/ena_eth_com.h
+++ b/drivers/net/ethernet/amazon/ena/ena_eth_com.h
@@ -107,7 +107,8 @@ static inline int
ena_com_sq_empty_space(struct ena_com_io_sq *io_sq)
return io_sq->q_depth - 1 - cnt;
 }

-static inline int ena_com_write_sq_doorbell(struct ena_com_io_sq 
*io_sq)
+static inline int ena_com_write_sq_doorbell(struct ena_com_io_sq 
*io_sq,

+   bool relaxed)
 {
u16 tail;

@@ -116,7 +117,24 @@ static inline int
ena_com_write_sq_doorbell(struct ena_com_io_sq *io_sq)
 	pr_debug("write submission queue doorbell for queue: %d tail: 
%d\n",

 io_sq->qid, tail);

-   writel(tail, io_sq->db_addr);
+   if (relaxed)
+   writel_relaxed(tail, io_sq->db_addr);
+   else
+   writel(tail, io_sq->db_addr);
+
+   return 0;
+}
+
+static inline int ena_com_write_sq_doorbell_rel(struct
ena_com_io_sq *io_sq)
+{
+   u16 tail;
+
+   tail = io_sq->tail;
+
+	pr_debug("write submission queue doorbell for queue: %d tail: 
%d\n",

+io_sq->qid, tail);
+
+   writel_relaxed(tail, io_sq->db_addr);

return 0;
 }
diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c
b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index 6975150..0530201 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -556,7 +556,7 @@ static int ena_refill_rx_bufs(struct ena_ring
*rx_ring, u32 num)
 * issue a doorbell
 */
wmb();
-   ena_com_write_sq_doorbell(rx_ring->ena_com_io_sq);
+   ena_com_write_sq_doorbell(rx_ring->ena_com_io_sq, true);
}

rx_ring->next_to_use = next_to_use;
@@ -2151,7 +2151,7 @@ static netdev_tx_t ena_start_xmit(struct
sk_buff *skb, struct net_device *dev)

if (netif_xmit_stopped(txq) || !skb->xmit_more) {
/* trigger the dma engine */
-   ena_com_write_sq_doorbell(tx_ring->ena_com_io_sq);
+   ena_com_write_sq_doorbell(tx_ring->ena_com_io_sq, false);
u64_stats_update_begin(_ring->syncp);
tx_ring->tx_stats.doorbells++;
u64_stats_update_end(_ring->syncp);
--
2.7.4

Re: [PATCH v4 4/6] infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs

[okaya]

On 2018-03-22 08:24, ok...@codeaurora.org wrote:

On 2018-03-22 02:44, kbuild test robot wrote:

Hi Sinan,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.16-rc6 next-20180321]
[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/Sinan-Kaya/ib-Eliminate-duplicate-barriers-on-weakly-ordered-archs/20180321-091659
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 7.2.0
reproduce:
wget
https://raw.githubusercontent.com/intel/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=xtensa



Jason,

Can you remove the writeq change if it is too late for me to fix?

This is an infrastructural issue on xtensa arch.

Probably, it won't get fixed today.


AFAIS, even writeq won't compile on this arch. I started questioning 
this build test.





Sinan



All errors (new ones prefixed by >>):

   In file included from drivers/infiniband/hw/cxgb4/iw_cxgb4.h:73:0,
from drivers/infiniband/hw/cxgb4/device.c:40:
   drivers/infiniband/hw/cxgb4/t4.h: In function 'pio_copy':
drivers/infiniband/hw/cxgb4/t4.h:460:3: error: implicit declaration 
of function 'writeq_relaxed'; did you mean 'writel_relaxed'? 
[-Werror=implicit-function-declaration]

  writeq_relaxed(*src, dst);
  ^~
  writel_relaxed
   cc1: some warnings being treated as errors

vim +460 drivers/infiniband/hw/cxgb4/t4.h

   450
   451	/* This function copies 64 byte coalesced work request to 
memory

   452   * mapped BAR2 space. For coalesced WRs, the SGE fetches data
   453   * from the FIFO instead of from Host.
   454   */
   455  static inline void pio_copy(u64 __iomem *dst, u64 *src)
   456  {
   457  int count = 8;
   458
   459  while (count) {
 > 460   writeq_relaxed(*src, dst);
   461  src++;
   462  dst++;
   463  count--;
   464  }
   465  }
   466

---
0-DAY kernel test infrastructureOpen Source Technology 
Center
https://lists.01.org/pipermail/kbuild-all   Intel 
Corporation

Re: [PATCH v4 4/6] infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs

[okaya]

On 2018-03-22 08:24, ok...@codeaurora.org wrote:

On 2018-03-22 02:44, kbuild test robot wrote:

Hi Sinan,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.16-rc6 next-20180321]
[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/Sinan-Kaya/ib-Eliminate-duplicate-barriers-on-weakly-ordered-archs/20180321-091659
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 7.2.0
reproduce:
wget
https://raw.githubusercontent.com/intel/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=xtensa



Jason,

Can you remove the writeq change if it is too late for me to fix?

This is an infrastructural issue on xtensa arch.

Probably, it won't get fixed today.


AFAIS, even writeq won't compile on this arch. I started questioning 
this build test.





Sinan



All errors (new ones prefixed by >>):

   In file included from drivers/infiniband/hw/cxgb4/iw_cxgb4.h:73:0,
from drivers/infiniband/hw/cxgb4/device.c:40:
   drivers/infiniband/hw/cxgb4/t4.h: In function 'pio_copy':
drivers/infiniband/hw/cxgb4/t4.h:460:3: error: implicit declaration 
of function 'writeq_relaxed'; did you mean 'writel_relaxed'? 
[-Werror=implicit-function-declaration]

  writeq_relaxed(*src, dst);
  ^~
  writel_relaxed
   cc1: some warnings being treated as errors

vim +460 drivers/infiniband/hw/cxgb4/t4.h

   450
   451	/* This function copies 64 byte coalesced work request to 
memory

   452   * mapped BAR2 space. For coalesced WRs, the SGE fetches data
   453   * from the FIFO instead of from Host.
   454   */
   455  static inline void pio_copy(u64 __iomem *dst, u64 *src)
   456  {
   457  int count = 8;
   458
   459  while (count) {
 > 460   writeq_relaxed(*src, dst);
   461  src++;
   462  dst++;
   463  count--;
   464  }
   465  }
   466

---
0-DAY kernel test infrastructureOpen Source Technology 
Center
https://lists.01.org/pipermail/kbuild-all   Intel 
Corporation

Re: [PATCH v4 4/6] infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs

[okaya]

On 2018-03-22 02:44, kbuild test robot wrote:

Hi Sinan,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.16-rc6 next-20180321]
[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/Sinan-Kaya/ib-Eliminate-duplicate-barriers-on-weakly-ordered-archs/20180321-091659
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 7.2.0
reproduce:
wget
https://raw.githubusercontent.com/intel/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=xtensa



Jason,

Can you remove the writeq change if it is too late for me to fix?

This is an infrastructural issue on xtensa arch.

Probably, it won't get fixed today.

Sinan



All errors (new ones prefixed by >>):

   In file included from drivers/infiniband/hw/cxgb4/iw_cxgb4.h:73:0,
from drivers/infiniband/hw/cxgb4/device.c:40:
   drivers/infiniband/hw/cxgb4/t4.h: In function 'pio_copy':
drivers/infiniband/hw/cxgb4/t4.h:460:3: error: implicit declaration 
of function 'writeq_relaxed'; did you mean 'writel_relaxed'? 
[-Werror=implicit-function-declaration]

  writeq_relaxed(*src, dst);
  ^~
  writel_relaxed
   cc1: some warnings being treated as errors

vim +460 drivers/infiniband/hw/cxgb4/t4.h

   450
   451  /* This function copies 64 byte coalesced work request to memory
   452   * mapped BAR2 space. For coalesced WRs, the SGE fetches data
   453   * from the FIFO instead of from Host.
   454   */
   455  static inline void pio_copy(u64 __iomem *dst, u64 *src)
   456  {
   457  int count = 8;
   458
   459  while (count) {
 > 460   writeq_relaxed(*src, dst);
   461  src++;
   462  dst++;
   463  count--;
   464  }
   465  }
   466

---
0-DAY kernel test infrastructureOpen Source Technology 
Center
https://lists.01.org/pipermail/kbuild-all   Intel 
Corporation

Re: [PATCH v4 4/6] infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs

[okaya]

On 2018-03-22 02:44, kbuild test robot wrote:

Hi Sinan,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.16-rc6 next-20180321]
[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/Sinan-Kaya/ib-Eliminate-duplicate-barriers-on-weakly-ordered-archs/20180321-091659
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 7.2.0
reproduce:
wget
https://raw.githubusercontent.com/intel/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=xtensa



Jason,

Can you remove the writeq change if it is too late for me to fix?

This is an infrastructural issue on xtensa arch.

Probably, it won't get fixed today.

Sinan



All errors (new ones prefixed by >>):

   In file included from drivers/infiniband/hw/cxgb4/iw_cxgb4.h:73:0,
from drivers/infiniband/hw/cxgb4/device.c:40:
   drivers/infiniband/hw/cxgb4/t4.h: In function 'pio_copy':
drivers/infiniband/hw/cxgb4/t4.h:460:3: error: implicit declaration 
of function 'writeq_relaxed'; did you mean 'writel_relaxed'? 
[-Werror=implicit-function-declaration]

  writeq_relaxed(*src, dst);
  ^~
  writel_relaxed
   cc1: some warnings being treated as errors

vim +460 drivers/infiniband/hw/cxgb4/t4.h

   450
   451  /* This function copies 64 byte coalesced work request to memory
   452   * mapped BAR2 space. For coalesced WRs, the SGE fetches data
   453   * from the FIFO instead of from Host.
   454   */
   455  static inline void pio_copy(u64 __iomem *dst, u64 *src)
   456  {
   457  int count = 8;
   458
   459  while (count) {
 > 460   writeq_relaxed(*src, dst);
   461  src++;
   462  dst++;
   463  count--;
   464  }
   465  }
   466

---
0-DAY kernel test infrastructureOpen Source Technology 
Center
https://lists.01.org/pipermail/kbuild-all   Intel 
Corporation

Re: [PATCH v4 12/17] net: cxgb4/cxgb4vf: Eliminate duplicate barriers on weakly-ordered archs

[okaya]

On 2018-03-21 19:03, Casey Leedom wrote:
[[ Appologies for the DUPLICATE email.  I forgot to tell my Mail Agent 
to

   use Plain Text. -- Casey ]]

  I feel very uncomfortable with these proposed changes.  Our team is 
right

in the middle of trying to tease our way through the various platform
implementations of writel(), writel_relaxed(), __raw_writel(), etc. in 
order

to support x86, PowerPC, ARM, etc. with a single code base.  This is
complicated by the somewhat ... "fuzzily defined" semantics and varying
platform implementations of all of these APIs.  (And note that I'm just
picking writel() as an example.)

  Additionally, many of the changes aren't even in fast paths and are 
thus

unneeded for performance.

  Please don't make these changes.  We're trying to get this all sussed 
out.




I was also given the feedback to look at performance critical path only. 
I am in the process of revisiting the patches.


If you can point me to the ones that are important, I can try to limit 
the changes to those only.


If your team wants to do it, I can drop this patch as well.

I think the semantics of write API is clear. What was actually 
implemented is another story.


I can share a few of my findings.

A portable driver needs to do this.

descriptor update in mem
wmb ()
writel_relaxed ()
mmiowb ()

Using __raw_write() is wrong as it can get reordered.

Using wmb()+writel() is also wrong for performance reasons.

If something is unclear, please ask.

Re: [PATCH v4 12/17] net: cxgb4/cxgb4vf: Eliminate duplicate barriers on weakly-ordered archs

[okaya]

On 2018-03-21 19:03, Casey Leedom wrote:
[[ Appologies for the DUPLICATE email.  I forgot to tell my Mail Agent 
to

   use Plain Text. -- Casey ]]

  I feel very uncomfortable with these proposed changes.  Our team is 
right

in the middle of trying to tease our way through the various platform
implementations of writel(), writel_relaxed(), __raw_writel(), etc. in 
order

to support x86, PowerPC, ARM, etc. with a single code base.  This is
complicated by the somewhat ... "fuzzily defined" semantics and varying
platform implementations of all of these APIs.  (And note that I'm just
picking writel() as an example.)

  Additionally, many of the changes aren't even in fast paths and are 
thus

unneeded for performance.

  Please don't make these changes.  We're trying to get this all sussed 
out.




I was also given the feedback to look at performance critical path only. 
I am in the process of revisiting the patches.


If you can point me to the ones that are important, I can try to limit 
the changes to those only.


If your team wants to do it, I can drop this patch as well.

I think the semantics of write API is clear. What was actually 
implemented is another story.


I can share a few of my findings.

A portable driver needs to do this.

descriptor update in mem
wmb ()
writel_relaxed ()
mmiowb ()

Using __raw_write() is wrong as it can get reordered.

Using wmb()+writel() is also wrong for performance reasons.

If something is unclear, please ask.

Re: [PATCH REPOST v4 5/7] ixgbevf: keep writel() closer to wmb()

[okaya]

On 2018-03-21 17:54, David Miller wrote:

From: Jeff Kirsher 
Date: Wed, 21 Mar 2018 14:48:08 -0700


On Wed, 2018-03-21 at 14:56 -0400, Sinan Kaya wrote:

Remove ixgbevf_write_tail() in favor of moving writel() close to
wmb().

Signed-off-by: Sinan Kaya 
Reviewed-by: Alexander Duyck 
---
 drivers/net/ethernet/intel/ixgbevf/ixgbevf.h  | 5 -
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 4 ++--
 2 files changed, 2 insertions(+), 7 deletions(-)


This patch fails to compile because there is a call to
ixgbevf_write_tail() which you missed cleaning up.


For a change with delicate side effects, it doesn't create much
confidence if the code does not even compile.

Sinan, please put more care into the changes you are making.


I think the issue is the tree that code is getting tested has 
undelivered code as Alex mentioned.


I was using linux-next 4.16 rc4 for testing.

I will rebase to Jeff's tree.



Thank you.

Re: [PATCH REPOST v4 5/7] ixgbevf: keep writel() closer to wmb()

[okaya]

On 2018-03-21 17:54, David Miller wrote:

From: Jeff Kirsher 
Date: Wed, 21 Mar 2018 14:48:08 -0700


On Wed, 2018-03-21 at 14:56 -0400, Sinan Kaya wrote:

Remove ixgbevf_write_tail() in favor of moving writel() close to
wmb().

Signed-off-by: Sinan Kaya 
Reviewed-by: Alexander Duyck 
---
 drivers/net/ethernet/intel/ixgbevf/ixgbevf.h  | 5 -
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 4 ++--
 2 files changed, 2 insertions(+), 7 deletions(-)


This patch fails to compile because there is a call to
ixgbevf_write_tail() which you missed cleaning up.


For a change with delicate side effects, it doesn't create much
confidence if the code does not even compile.

Sinan, please put more care into the changes you are making.


I think the issue is the tree that code is getting tested has 
undelivered code as Alex mentioned.


I was using linux-next 4.16 rc4 for testing.

I will rebase to Jeff's tree.



Thank you.

Re: [PATCH REPOST v4 5/7] ixgbevf: keep writel() closer to wmb()

[okaya]

On 2018-03-21 17:48, Jeff Kirsher wrote:

On Wed, 2018-03-21 at 14:56 -0400, Sinan Kaya wrote:

Remove ixgbevf_write_tail() in favor of moving writel() close to
wmb().

Signed-off-by: Sinan Kaya 
Reviewed-by: Alexander Duyck 
---
 drivers/net/ethernet/intel/ixgbevf/ixgbevf.h  | 5 -
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 4 ++--
 2 files changed, 2 insertions(+), 7 deletions(-)


This patch fails to compile because there is a call to
ixgbevf_write_tail() which you missed cleaning up.


Hah, I did a compile test but maybe I missed something. I will get v6 of 
this patch only and leave the rest of the series as it is.

Re: [PATCH REPOST v4 5/7] ixgbevf: keep writel() closer to wmb()

[okaya]

On 2018-03-21 17:48, Jeff Kirsher wrote:

On Wed, 2018-03-21 at 14:56 -0400, Sinan Kaya wrote:

Remove ixgbevf_write_tail() in favor of moving writel() close to
wmb().

Signed-off-by: Sinan Kaya 
Reviewed-by: Alexander Duyck 
---
 drivers/net/ethernet/intel/ixgbevf/ixgbevf.h  | 5 -
 drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 4 ++--
 2 files changed, 2 insertions(+), 7 deletions(-)


This patch fails to compile because there is a call to
ixgbevf_write_tail() which you missed cleaning up.


Hah, I did a compile test but maybe I missed something. I will get v6 of 
this patch only and leave the rest of the series as it is.

Re: [PATCH v4 3/7] scsi: dpt_i2o: eliminate duplicate barriers on weakly-ordered archs

[okaya]

On 2018-03-20 06:23, Christoph Hellwig wrote:
This is a basically unmaintained driver for historic hardware, and not 
the

right place for micro-optimizations.


Sure, I can drop this on the next version.

Re: [PATCH v4 3/7] scsi: dpt_i2o: eliminate duplicate barriers on weakly-ordered archs

[okaya]

On 2018-03-20 06:23, Christoph Hellwig wrote:
This is a basically unmaintained driver for historic hardware, and not 
the

right place for micro-optimizations.


Sure, I can drop this on the next version.

Re: [PATCH 7/7] ixgbevf: eliminate duplicate barriers on weakly-ordered archs

[okaya]

On 2018-03-14 01:08, Timur Tabi wrote:

On 3/13/18 10:20 PM, Sinan Kaya wrote:
+/* Assumes caller has executed a write barrier to order memory and 
device

+ * requests.
+ */
  static inline void ixgbevf_write_tail(struct ixgbevf_ring *ring, u32 
value)

  {
-   writel(value, ring->tail);
+   writel_relaxed(value, ring->tail);
  }


Why not put the wmb() in this function, or just get rid of the wmb()
in the rest of the file and keep this as writel?  That way, you can
avoid the comment and the risk that comes with it.



Sure, both solutions will work. I want to see what the maintainer 
prefers. I can repost accordingly.

Re: [PATCH 7/7] ixgbevf: eliminate duplicate barriers on weakly-ordered archs

[okaya]

On 2018-03-14 01:08, Timur Tabi wrote:

On 3/13/18 10:20 PM, Sinan Kaya wrote:
+/* Assumes caller has executed a write barrier to order memory and 
device

+ * requests.
+ */
  static inline void ixgbevf_write_tail(struct ixgbevf_ring *ring, u32 
value)

  {
-   writel(value, ring->tail);
+   writel_relaxed(value, ring->tail);
  }


Why not put the wmb() in this function, or just get rid of the wmb()
in the rest of the file and keep this as writel?  That way, you can
avoid the comment and the risk that comes with it.



Sure, both solutions will work. I want to see what the maintainer 
prefers. I can repost accordingly.

Re: [PATCH 3/7] RDMA/qedr: eliminate duplicate barriers on weakly-ordered archs

[okaya]

On 2018-03-14 00:12, Jason Gunthorpe wrote:

On Tue, Mar 13, 2018 at 11:20:24PM -0400, Sinan Kaya wrote:

Code includes wmb() followed by writel() in multiple places. writel()
already has a barrier on some architectures like arm64.

This ends up CPU observing two barriers back to back before executing 
the

register write.

Since code already has an explicit barrier call, changing writel() to
writel_relaxed().

Signed-off-by: Sinan Kaya 
 drivers/infiniband/hw/qedr/verbs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


Sure matches my understanding of writel_relaxed

This is part of a series, should we take just this patch through the
rdma tree? If not:

Acked-by: Jason Gunthorpe 


Feel free to take pieces.




Thanks,
Jason

Re: [PATCH 3/7] RDMA/qedr: eliminate duplicate barriers on weakly-ordered archs

[okaya]

On 2018-03-14 00:12, Jason Gunthorpe wrote:

On Tue, Mar 13, 2018 at 11:20:24PM -0400, Sinan Kaya wrote:

Code includes wmb() followed by writel() in multiple places. writel()
already has a barrier on some architectures like arm64.

This ends up CPU observing two barriers back to back before executing 
the

register write.

Since code already has an explicit barrier call, changing writel() to
writel_relaxed().

Signed-off-by: Sinan Kaya 
 drivers/infiniband/hw/qedr/verbs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


Sure matches my understanding of writel_relaxed

This is part of a series, should we take just this patch through the
rdma tree? If not:

Acked-by: Jason Gunthorpe 


Feel free to take pieces.




Thanks,
Jason

Re: [PATCH] ACPI/PCI: pci_link: reduce verbosity when IRQ is enabled

[okaya]

On 2018-02-04 04:03, Rafael J. Wysocki wrote:
On Thu, Feb 1, 2018 at 8:32 AM, Rafael J. Wysocki  
wrote:
On Mon, Jan 29, 2018 at 8:01 PM, Sinan Kaya  
wrote:

Rafael,

On 1/16/2018 4:49 PM, Bjorn Helgaas wrote:

On Tue, Jan 16, 2018 at 01:53:00PM -0500, Sinan Kaya wrote:

Correcting linux-pci email.

On 1/16/2018 1:51 PM, Sinan Kaya wrote:
When ACPI Link object is enabled, the message is printed with a 
warning
prefix. Some test tools are capturing warning and test error types 
as

errors. Let's reduce the verbosity of success case.

Signed-off-by: Sinan Kaya 

Acked-by: Bjorn Helgaas 

Looks like this was a result of 4d9391557b68 ("ACPI: add missing 
KERN_*
constants to printks"), which I think added the wrong level in this 
case.




Any chance of merging this for 4.16?


There is.


This one is already there in the Linus' tree AFAICS.


You are right. I see it there. I have not seen an applied email. I 
thought it was still outstanding.



--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" 
in

the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ACPI/PCI: pci_link: reduce verbosity when IRQ is enabled

[okaya]

On 2018-02-04 04:03, Rafael J. Wysocki wrote:
On Thu, Feb 1, 2018 at 8:32 AM, Rafael J. Wysocki  
wrote:
On Mon, Jan 29, 2018 at 8:01 PM, Sinan Kaya  
wrote:

Rafael,

On 1/16/2018 4:49 PM, Bjorn Helgaas wrote:

On Tue, Jan 16, 2018 at 01:53:00PM -0500, Sinan Kaya wrote:

Correcting linux-pci email.

On 1/16/2018 1:51 PM, Sinan Kaya wrote:
When ACPI Link object is enabled, the message is printed with a 
warning
prefix. Some test tools are capturing warning and test error types 
as

errors. Let's reduce the verbosity of success case.

Signed-off-by: Sinan Kaya 

Acked-by: Bjorn Helgaas 

Looks like this was a result of 4d9391557b68 ("ACPI: add missing 
KERN_*
constants to printks"), which I think added the wrong level in this 
case.




Any chance of merging this for 4.16?


There is.


This one is already there in the Linus' tree AFAICS.


You are right. I see it there. I have not seen an applied email. I 
thought it was still outstanding.



--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" 
in

the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] nvme: Acknowledge completion queue on each iteration

[okaya]

On 2017-07-17 18:56, Keith Busch wrote:

On Mon, Jul 17, 2017 at 06:46:11PM -0400, Sinan Kaya wrote:

Hi Keith,

On 7/17/2017 6:45 PM, Keith Busch wrote:
> On Mon, Jul 17, 2017 at 06:36:23PM -0400, Sinan Kaya wrote:
>> Code is moving the completion queue doorbell after processing all completed
>> events and sending callbacks to the block layer on each iteration.
>>
>> This is causing a performance drop when a lot of jobs are queued towards
>> the HW. Move the completion queue doorbell on each loop instead and allow new
>> jobs to be queued by the HW.
>
> That doesn't make sense. Aggregating doorbell writes should be much more
> efficient for high depth workloads.
>

Problem is that code is throttling the HW as HW cannot queue more 
completions until

SW get a chance to clear it.

As an example:

for each in N
(
blk_layer()
)
ring door bell

HW cannot queue new job until N x blk_layer operations are processed 
and queue
element ownership is passed to the HW after the loop. HW is just 
sitting idle

there if no queue entries are available.


If no completion queue entries are available, then there can't possibly
be any submission queue entries for the HW to work on either.


Maybe, I need to understand the design better. I was curious why 
completion and submission queues were protected by a single lock causing 
lock contention.


I was treating each queue independently. I have seen slightly better 
performance by an early doorbell. That was my explanation.

Re: [PATCH] nvme: Acknowledge completion queue on each iteration

[okaya]

On 2017-07-17 18:56, Keith Busch wrote:

On Mon, Jul 17, 2017 at 06:46:11PM -0400, Sinan Kaya wrote:

Hi Keith,

On 7/17/2017 6:45 PM, Keith Busch wrote:
> On Mon, Jul 17, 2017 at 06:36:23PM -0400, Sinan Kaya wrote:
>> Code is moving the completion queue doorbell after processing all completed
>> events and sending callbacks to the block layer on each iteration.
>>
>> This is causing a performance drop when a lot of jobs are queued towards
>> the HW. Move the completion queue doorbell on each loop instead and allow new
>> jobs to be queued by the HW.
>
> That doesn't make sense. Aggregating doorbell writes should be much more
> efficient for high depth workloads.
>

Problem is that code is throttling the HW as HW cannot queue more 
completions until

SW get a chance to clear it.

As an example:

for each in N
(
blk_layer()
)
ring door bell

HW cannot queue new job until N x blk_layer operations are processed 
and queue
element ownership is passed to the HW after the loop. HW is just 
sitting idle

there if no queue entries are available.


If no completion queue entries are available, then there can't possibly
be any submission queue entries for the HW to work on either.


Maybe, I need to understand the design better. I was curious why 
completion and submission queues were protected by a single lock causing 
lock contention.


I was treating each queue independently. I have seen slightly better 
performance by an early doorbell. That was my explanation.

Re: [RFC 1/8] Introduce Peer-to-Peer memory (p2pmem) device

[okaya]

On 2017-03-31 21:57, Logan Gunthorpe wrote:

On 31/03/17 05:51 PM, Sinan Kaya wrote:
You can put a restriction with DMI/SMBIOS such that all devices from 
2016

work else they belong to blacklist.


How do you get a manufacturing date for a given device within the
kernel? Is this actually something generically available?

Logan


Smbios calls are used all over the place in kernel for introducing new 
functionality while maintaining backwards compatibility.


See drivers/pci and drivers/acpi directory.

Re: [RFC 1/8] Introduce Peer-to-Peer memory (p2pmem) device

[okaya]

On 2017-03-31 21:57, Logan Gunthorpe wrote:

On 31/03/17 05:51 PM, Sinan Kaya wrote:
You can put a restriction with DMI/SMBIOS such that all devices from 
2016

work else they belong to blacklist.


How do you get a manufacturing date for a given device within the
kernel? Is this actually something generically available?

Logan


Smbios calls are used all over the place in kernel for introducing new 
functionality while maintaining backwards compatibility.


See drivers/pci and drivers/acpi directory.

Re: [PATCH V2] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[okaya]

On 2017-03-03 04:43, Patel, Mayurkumar wrote:

Hi Kaya



Hi Mayurkumar

On 3/2/2017 11:05 AM, Patel, Mayurkumar wrote:


Hi Bjorn,

On 2/28/2017 1:57 PM, Patel, Mayurkumar wrote:
I was trying to figure out when to use saved values vs. the values 
in

registers by looking at the enable_cnt.
enable_cnt is 0 during boot on my system.
enable_cnt for the root port on my system is set to 1 for "root 
port" already without saving

any ASPM related settings.




What would be the best way to figure out when to save power-on 
values from

the registers?



I can upload the diffs) because enable_cnt in pci_enable_device() can 
be triggered

from multiple places at boot time so it might not be safe to use it?


Go ahead and share your diff. It doesn't hurt to look at other 
alternatives.




So basically, I introduced a new atomic variable to save the
aspm_policy for the first time.
Below is my diff.


Thanks.

Ok, i will incorporate your change and add your signoff, then repost the 
next version after testing next week. I am OoO until next week.





diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 3dd8bcb..c8e1e3a 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -338,8 +338,9 @@ static void pcie_aspm_check_latency(struct pci_dev
*endpoint)
}
 }

-static void pcie_aspm_cap_init(struct pcie_link_state *link, int 
blacklist)

+static void pcie_aspm_cap_init(struct pci_dev *pdev, int blacklist)
 {
+   struct pcie_link_state *link = pdev->link_state;
struct pci_dev *child, *parent = link->pdev;
struct pci_bus *linkbus = parent->subordinate;
struct aspm_register_info upreg, dwreg;
@@ -397,8 +398,21 @@ static void pcie_aspm_cap_init(struct
pcie_link_state *link, int blacklist)
link->latency_up.l1 = 
calc_l1_latency(upreg.latency_encoding_l1);
link->latency_dw.l1 = 
calc_l1_latency(dwreg.latency_encoding_l1);


-   /* Save default state */
-   link->aspm_default = link->aspm_enabled;
+   /*
+* Save default state from FW when enabling ASPM for the first 
time
+* during boot by looking at the calculated link->aspm_enabled 
bits

+* above and aspm_enable_cnt will be zero.
+*
+* If this path is getting called for the second/third time
+* (aspm_enable_cnt will be non-zero). Assume that the current 
state
+* of the ASPM registers may not necessarily match what FW 
asked us to

+* do as in the case of hotplug insertion/removal.
+*/
+   if (atomic_inc_return(>aspm_enable_cnt) == 1)
+   pdev->aspm_default = link->aspm_default = 
link->aspm_enabled;

+   else
+   link->aspm_default = pdev->aspm_default;
+

/* Setup initial capable state. Will be updated later */
link->aspm_capable = link->aspm_support;
@@ -606,7 +620,7 @@ void pcie_aspm_init_link_state(struct pci_dev 
*pdev)

 * upstream links also because capable state of them can be
 * update through pcie_aspm_cap_init().
 */
-   pcie_aspm_cap_init(link, blacklist);
+  pcie_aspm_cap_init(pdev, blacklist);

/* Setup initial Clock PM state */
pcie_clkpm_cap_init(link, blacklist);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index e2d1a12..aa7bd7e 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -321,6 +321,8 @@ struct pci_dev {

 #ifdef CONFIG_PCIEASPM
 struct pcie_link_state  *link_state;/* ASPM link state */
+   unsigned intaspm_default;   /* ASPM policy set by 
BIOS */
+   atomic_taspm_enable_cnt;/* ASPM policy 
initialization */

Re: [PATCH V2] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT

[okaya]

On 2017-03-03 04:43, Patel, Mayurkumar wrote:

Hi Kaya



Hi Mayurkumar

On 3/2/2017 11:05 AM, Patel, Mayurkumar wrote:


Hi Bjorn,

On 2/28/2017 1:57 PM, Patel, Mayurkumar wrote:
I was trying to figure out when to use saved values vs. the values 
in

registers by looking at the enable_cnt.
enable_cnt is 0 during boot on my system.
enable_cnt for the root port on my system is set to 1 for "root 
port" already without saving

any ASPM related settings.




What would be the best way to figure out when to save power-on 
values from

the registers?



I can upload the diffs) because enable_cnt in pci_enable_device() can 
be triggered

from multiple places at boot time so it might not be safe to use it?


Go ahead and share your diff. It doesn't hurt to look at other 
alternatives.




So basically, I introduced a new atomic variable to save the
aspm_policy for the first time.
Below is my diff.


Thanks.

Ok, i will incorporate your change and add your signoff, then repost the 
next version after testing next week. I am OoO until next week.





diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 3dd8bcb..c8e1e3a 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -338,8 +338,9 @@ static void pcie_aspm_check_latency(struct pci_dev
*endpoint)
}
 }

-static void pcie_aspm_cap_init(struct pcie_link_state *link, int 
blacklist)

+static void pcie_aspm_cap_init(struct pci_dev *pdev, int blacklist)
 {
+   struct pcie_link_state *link = pdev->link_state;
struct pci_dev *child, *parent = link->pdev;
struct pci_bus *linkbus = parent->subordinate;
struct aspm_register_info upreg, dwreg;
@@ -397,8 +398,21 @@ static void pcie_aspm_cap_init(struct
pcie_link_state *link, int blacklist)
link->latency_up.l1 = 
calc_l1_latency(upreg.latency_encoding_l1);
link->latency_dw.l1 = 
calc_l1_latency(dwreg.latency_encoding_l1);


-   /* Save default state */
-   link->aspm_default = link->aspm_enabled;
+   /*
+* Save default state from FW when enabling ASPM for the first 
time
+* during boot by looking at the calculated link->aspm_enabled 
bits

+* above and aspm_enable_cnt will be zero.
+*
+* If this path is getting called for the second/third time
+* (aspm_enable_cnt will be non-zero). Assume that the current 
state
+* of the ASPM registers may not necessarily match what FW 
asked us to

+* do as in the case of hotplug insertion/removal.
+*/
+   if (atomic_inc_return(>aspm_enable_cnt) == 1)
+   pdev->aspm_default = link->aspm_default = 
link->aspm_enabled;

+   else
+   link->aspm_default = pdev->aspm_default;
+

/* Setup initial capable state. Will be updated later */
link->aspm_capable = link->aspm_support;
@@ -606,7 +620,7 @@ void pcie_aspm_init_link_state(struct pci_dev 
*pdev)

 * upstream links also because capable state of them can be
 * update through pcie_aspm_cap_init().
 */
-   pcie_aspm_cap_init(link, blacklist);
+  pcie_aspm_cap_init(pdev, blacklist);

/* Setup initial Clock PM state */
pcie_clkpm_cap_init(link, blacklist);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index e2d1a12..aa7bd7e 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -321,6 +321,8 @@ struct pci_dev {

 #ifdef CONFIG_PCIEASPM
 struct pcie_link_state  *link_state;/* ASPM link state */
+   unsigned intaspm_default;   /* ASPM policy set by 
BIOS */
+   atomic_taspm_enable_cnt;/* ASPM policy 
initialization */

Re: [PATCH 2/2] iommu: add warning when sharing groups

[okaya]

On 2017-02-14 18:51, Alex Williamson wrote:

On Tue, 14 Feb 2017 16:25:22 -0500
Sinan Kaya  wrote:


The ACS requirement has been obscured in the current code and is only
known by certain individuals who happen to read the code. Print out a
warning with ACS path failure when ACS requirement is not met.

Signed-off-by: Sinan Kaya 
---
 drivers/iommu/iommu.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index dbe7f65..049ee0a 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -811,6 +811,9 @@ struct iommu_group *pci_device_group(struct device 
*dev)

if (IS_ERR(group))
return NULL;

+   if (pci_is_root_bus(bus))
+		dev_warn_once(>dev, "using shared group due to ACS path 
failure\n");

+
return group;
 }



The premise here is flawed.  An IOMMU group based at the root bus
doesn't necessarily imply a lack of ACS.  There are devices on root
buses, integrated endpoints and root ports.  Naturally an IOMMU group
for these devices needs to be based at the root bus.  Additionally,
there can be IOMMU groups developed around a lack of ACS that don't
intersect with the root bus.  Since this is a warn_once, the false
positives for root bus devices are going to be enumerated first.  On an
Intel system there's typically a device as 00.0 that will always be
pointlessly listed first.  Also, it's not clear that grouping devices
together is always wrong, as Robin pointed out in the EHCI/OHCI
example.  Lack of ACS on downtream ports is likely to cause problems,
especially if that downstream port exposes a slot.  Maybe that would be
a good place to start.  Also, what is someone supposed to do when they
see this error?  If we can hope they'll look for the error in the code
(unlikely) a big comment with useful external links might be
necessary.  Based on how easily vendors ignore kernel warnings, I'm
dubious there's any value to this path though.  Thanks,


Maybe, a better solution would be to add some sentences into vfio.txt 
documentation.


I'm ready to drop this patch. I just don't want ACS requirement to be 
hidden between the source code.


Would you be willing to do that?

I know I read all pci and vfio documents in the past. I could have 
captured this requirement if it was there.




Alex

Re: [PATCH 2/2] iommu: add warning when sharing groups

[okaya]

On 2017-02-14 18:51, Alex Williamson wrote:

On Tue, 14 Feb 2017 16:25:22 -0500
Sinan Kaya  wrote:


The ACS requirement has been obscured in the current code and is only
known by certain individuals who happen to read the code. Print out a
warning with ACS path failure when ACS requirement is not met.

Signed-off-by: Sinan Kaya 
---
 drivers/iommu/iommu.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index dbe7f65..049ee0a 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -811,6 +811,9 @@ struct iommu_group *pci_device_group(struct device 
*dev)

if (IS_ERR(group))
return NULL;

+   if (pci_is_root_bus(bus))
+		dev_warn_once(>dev, "using shared group due to ACS path 
failure\n");

+
return group;
 }



The premise here is flawed.  An IOMMU group based at the root bus
doesn't necessarily imply a lack of ACS.  There are devices on root
buses, integrated endpoints and root ports.  Naturally an IOMMU group
for these devices needs to be based at the root bus.  Additionally,
there can be IOMMU groups developed around a lack of ACS that don't
intersect with the root bus.  Since this is a warn_once, the false
positives for root bus devices are going to be enumerated first.  On an
Intel system there's typically a device as 00.0 that will always be
pointlessly listed first.  Also, it's not clear that grouping devices
together is always wrong, as Robin pointed out in the EHCI/OHCI
example.  Lack of ACS on downtream ports is likely to cause problems,
especially if that downstream port exposes a slot.  Maybe that would be
a good place to start.  Also, what is someone supposed to do when they
see this error?  If we can hope they'll look for the error in the code
(unlikely) a big comment with useful external links might be
necessary.  Based on how easily vendors ignore kernel warnings, I'm
dubious there's any value to this path though.  Thanks,


Maybe, a better solution would be to add some sentences into vfio.txt 
documentation.


I'm ready to drop this patch. I just don't want ACS requirement to be 
hidden between the source code.


Would you be willing to do that?

I know I read all pci and vfio documents in the past. I could have 
captured this requirement if it was there.




Alex

Re: [PATCH] ACPI/IORT: Fix iort_node_get_id() mapping entries indexing

[okaya]

On 2017-01-09 01:34, Hanjun Guo wrote:

Hi Sinan,

On 2017/1/8 5:09, Sinan Kaya wrote:

On 1/5/2017 1:29 PM, Lorenzo Pieralisi wrote:

Commit 618f535a6062 ("ACPI/IORT: Add single mapping function")
introduced a function (iort_node_get_id()) to retrieve ids for IORT
named components.

iort_node_get_id() takes an index as input to refer to a specific
mapping entry in the mapping array to retrieve the id at a specific
index provided the index is below the total mapping count; currently 
the
index is used to retrieve the mapping value from the correct entry 
but

not to dereference the correct entry while retrieving the mapping
output_reference (ie IORT parent pointer), which consequently always
resolves to the output_reference of the first entry in the mapping
array.

Update the map array entry pointer computation in iort_node_get_id() 
to

take into account the index value, fixing the issue.

Fixes: 618f535a6062 ("ACPI/IORT: Add single mapping function")
Reported-by: Hanjun Guo 
Signed-off-by: Lorenzo Pieralisi 
Cc: Hanjun Guo 
Cc: Sinan Kaya 
Cc: Tomasz Nowicki 
Cc: Nate Watterson 
Cc: "Rafael J. Wysocki" 
---
 drivers/acpi/arm64/iort.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index e0d2e6e..ba156c5 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -333,7 +333,7 @@ struct acpi_iort_node *iort_node_get_id(struct 
acpi_iort_node *node,

return NULL;

map = ACPI_ADD_PTR(struct acpi_iort_id_mapping, node,
-  node->mapping_offset);
+  node->mapping_offset + index * sizeof(*map));


What does this give us that the previous code didn't do?


Fir example, if you have multi mappings ids under platform device:

|-|
|  SMMU  2|<---
|-|   |
  |
  |
|-|   |
|  SMMU 1 |<  |
|-||  |
   |  |
   |  |
|-||  |
|  platform   ||  |
|  device ||  |
|-||  |
| stream id   ||  |
| 1   ||  |
| parent--||  |
|-|   |
|  stream id  |   |
|  2  |   |
|  parent-|---|
|-|

For now, we just use the first entry in the mapping entry to get
the parent, and always point to the same parent, as above, we will
always map to SMMU 1 even if you connect to different SMMUs. (Although
we may don't have such device topology yet)


I see. This wasn't obvious from the commit message. Thanks for taking 
time to explain it.


I think the commit message needs to include some of your description.






You are using map as a pointer and returning the offset of the first 
map entry above

and then accessing the map at the indexed offset with map[index]

The new code is using map as a plain pointer, calculating the pointer 
location with ACPI_ADD_PTR
instead and then collecting the output parameter with 
map->output_base.




/* Firmware bug! */
if (!map->output_reference) {
@@ -348,10 +348,10 @@ struct acpi_iort_node *iort_node_get_id(struct 
acpi_iort_node *node,

if (!(IORT_TYPE_MASK(parent->type) & type_mask))
return NULL;

-   if (map[index].flags & ACPI_IORT_ID_SINGLE_MAPPING) {
+   if (map->flags & ACPI_IORT_ID_SINGLE_MAPPING) {
if (node->type == ACPI_IORT_NODE_NAMED_COMPONENT ||
node->type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX) {
-   *id_out = map[index].output_base;
+   *id_out = map->output_base;


You are claiming that the existing code is collecting the output 
parameter from the first mapping.

I don't see this happening above.

What am I missing?


It's not about the output id but it's about the parent returned
by this function, it always return the first entry's parent in the
mapping entry.


Ok, I was judt looking at the patch. I didn't realize ww are changing 
the return value.


This could have been mentioned.







return parent;
}
}



If we are just doing a housekeeping, this is fine. I couldn't see an 
actual bug getting fixed.


Although we may don't have such use cases for now, but I think we
need to prepare for it, it worth a bugfix I think :)

Thanks
Hanjun

Re: [PATCH] ACPI/IORT: Fix iort_node_get_id() mapping entries indexing

[okaya]

On 2017-01-09 01:34, Hanjun Guo wrote:

Hi Sinan,

On 2017/1/8 5:09, Sinan Kaya wrote:

On 1/5/2017 1:29 PM, Lorenzo Pieralisi wrote:

Commit 618f535a6062 ("ACPI/IORT: Add single mapping function")
introduced a function (iort_node_get_id()) to retrieve ids for IORT
named components.

iort_node_get_id() takes an index as input to refer to a specific
mapping entry in the mapping array to retrieve the id at a specific
index provided the index is below the total mapping count; currently 
the
index is used to retrieve the mapping value from the correct entry 
but

not to dereference the correct entry while retrieving the mapping
output_reference (ie IORT parent pointer), which consequently always
resolves to the output_reference of the first entry in the mapping
array.

Update the map array entry pointer computation in iort_node_get_id() 
to

take into account the index value, fixing the issue.

Fixes: 618f535a6062 ("ACPI/IORT: Add single mapping function")
Reported-by: Hanjun Guo 
Signed-off-by: Lorenzo Pieralisi 
Cc: Hanjun Guo 
Cc: Sinan Kaya 
Cc: Tomasz Nowicki 
Cc: Nate Watterson 
Cc: "Rafael J. Wysocki" 
---
 drivers/acpi/arm64/iort.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index e0d2e6e..ba156c5 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -333,7 +333,7 @@ struct acpi_iort_node *iort_node_get_id(struct 
acpi_iort_node *node,

return NULL;

map = ACPI_ADD_PTR(struct acpi_iort_id_mapping, node,
-  node->mapping_offset);
+  node->mapping_offset + index * sizeof(*map));


What does this give us that the previous code didn't do?


Fir example, if you have multi mappings ids under platform device:

|-|
|  SMMU  2|<---
|-|   |
  |
  |
|-|   |
|  SMMU 1 |<  |
|-||  |
   |  |
   |  |
|-||  |
|  platform   ||  |
|  device ||  |
|-||  |
| stream id   ||  |
| 1   ||  |
| parent--||  |
|-|   |
|  stream id  |   |
|  2  |   |
|  parent-|---|
|-|

For now, we just use the first entry in the mapping entry to get
the parent, and always point to the same parent, as above, we will
always map to SMMU 1 even if you connect to different SMMUs. (Although
we may don't have such device topology yet)


I see. This wasn't obvious from the commit message. Thanks for taking 
time to explain it.


I think the commit message needs to include some of your description.






You are using map as a pointer and returning the offset of the first 
map entry above

and then accessing the map at the indexed offset with map[index]

The new code is using map as a plain pointer, calculating the pointer 
location with ACPI_ADD_PTR
instead and then collecting the output parameter with 
map->output_base.




/* Firmware bug! */
if (!map->output_reference) {
@@ -348,10 +348,10 @@ struct acpi_iort_node *iort_node_get_id(struct 
acpi_iort_node *node,

if (!(IORT_TYPE_MASK(parent->type) & type_mask))
return NULL;

-   if (map[index].flags & ACPI_IORT_ID_SINGLE_MAPPING) {
+   if (map->flags & ACPI_IORT_ID_SINGLE_MAPPING) {
if (node->type == ACPI_IORT_NODE_NAMED_COMPONENT ||
node->type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX) {
-   *id_out = map[index].output_base;
+   *id_out = map->output_base;


You are claiming that the existing code is collecting the output 
parameter from the first mapping.

I don't see this happening above.

What am I missing?


It's not about the output id but it's about the parent returned
by this function, it always return the first entry's parent in the
mapping entry.


Ok, I was judt looking at the patch. I didn't realize ww are changing 
the return value.


This could have been mentioned.







return parent;
}
}



If we are just doing a housekeeping, this is fine. I couldn't see an 
actual bug getting fixed.


Although we may don't have such use cases for now, but I think we
need to prepare for it, it worth a bugfix I think :)

Thanks
Hanjun

Re: [PATCH V7 4/4] dmaengine: qcom_hidma: add MSI support for interrupts

[okaya]

On 2016-10-24 03:30, Andy Shevchenko wrote:
On Mon, Oct 24, 2016 at 5:55 AM, Sinan Kaya  
wrote:

On 10/21/2016 12:11 PM, Andy Shevchenko wrote:

+static void hidma_free_msis(struct hidma_dev *dmadev)
> +{
> +#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN

Perhaps one #ifdef and two definitions of functions?


I don't think it will make a difference. I'll have to move
#ifdef around the caller of hidma_free_msis instead which
I think is uglier.

The hidma_write_msi_msg function gets called only when
CONFIG_GENERIC_MSI_IRQ_DOMAIN is defined. If I don't put
this around the function definition, I get unused function
warning from the compiler. This is the reason why preprocessor
definition is outside of the function definition.


I am talking about something like below:

#ifdef UGLY_DEFINE
myfunc_a()
{
}

myfunc_b()
{
}
#else
static inline myfunc_a() {}
static inline myfunc_b() {}
#endif


There is another way as well, namely use of IS_ENABLED(), IS_BUILTIN()
macros (I don't remember how exactly second one is spelt).



This was my initial approach. I was asked to remove the stub functions. 
So, I did it.

Re: [PATCH V7 4/4] dmaengine: qcom_hidma: add MSI support for interrupts

[okaya]

On 2016-10-24 03:30, Andy Shevchenko wrote:
On Mon, Oct 24, 2016 at 5:55 AM, Sinan Kaya  
wrote:

On 10/21/2016 12:11 PM, Andy Shevchenko wrote:

+static void hidma_free_msis(struct hidma_dev *dmadev)
> +{
> +#ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN

Perhaps one #ifdef and two definitions of functions?


I don't think it will make a difference. I'll have to move
#ifdef around the caller of hidma_free_msis instead which
I think is uglier.

The hidma_write_msi_msg function gets called only when
CONFIG_GENERIC_MSI_IRQ_DOMAIN is defined. If I don't put
this around the function definition, I get unused function
warning from the compiler. This is the reason why preprocessor
definition is outside of the function definition.


I am talking about something like below:

#ifdef UGLY_DEFINE
myfunc_a()
{
}

myfunc_b()
{
}
#else
static inline myfunc_a() {}
static inline myfunc_b() {}
#endif


There is another way as well, namely use of IS_ENABLED(), IS_BUILTIN()
macros (I don't remember how exactly second one is spelt).



This was my initial approach. I was asked to remove the stub functions. 
So, I did it.

Re: Why MSI is limited to 32 interrupts maximum

[okaya]

On 2016-10-15 06:30, Bharat Kumar Gogada wrote:

On Sat, 15 Oct 2016 12:37:55 +
Bharat Kumar Gogada  wrote:

Hi Bharat,

> Can anyone tell why MSI interrupts are limited to maximum 32
> interrupts, even though we have 16bit message data register ?

That's the very definition of the original PCI MSI: Up to 32 
consecutive interrupts

per function, and a single doorbell address.
MSI-X lifts that restriction and offers up to 2048 interrupts that do 
not have to
be contiguous can target individual doorbells. Assuming your MSI 
controller
advertises MSI-X support and that your devices do support it as well, 
you'll be

able to enjoy those.



Thanks Marc, so then only 5 bits of message data register will be
used, so what is the purpose rest of the bits ?



Data field is used to indicate which interrupt number is used. It does 
not need to start at 0.



You have 32 consecutive interrupts starting from some arbitrary base.



>
> Regards,
> Bharat
>
>
> This email and any attachments are intended for the sole use of the
> named recipient(s) and contain(s) confidential information that may be
> proprietary, privileged or copyrighted under applicable law. If you
> are not the intended recipient, do not read, copy, or forward this
> email message or any attachments. Delete this email message and any
> attachments immediately.
>

Please fix your email not to carry such disclaimer (or use a private 
email address
if you can't work around your company policy). Posting confidential 
information

on a public mailing list feels a bit silly.

Thanks,

M.
--
Without deviation from the norm, progress is not possible.

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Why MSI is limited to 32 interrupts maximum

[okaya]

On 2016-10-15 06:30, Bharat Kumar Gogada wrote:

On Sat, 15 Oct 2016 12:37:55 +
Bharat Kumar Gogada  wrote:

Hi Bharat,

> Can anyone tell why MSI interrupts are limited to maximum 32
> interrupts, even though we have 16bit message data register ?

That's the very definition of the original PCI MSI: Up to 32 
consecutive interrupts

per function, and a single doorbell address.
MSI-X lifts that restriction and offers up to 2048 interrupts that do 
not have to
be contiguous can target individual doorbells. Assuming your MSI 
controller
advertises MSI-X support and that your devices do support it as well, 
you'll be

able to enjoy those.



Thanks Marc, so then only 5 bits of message data register will be
used, so what is the purpose rest of the bits ?



Data field is used to indicate which interrupt number is used. It does 
not need to start at 0.



You have 32 consecutive interrupts starting from some arbitrary base.



>
> Regards,
> Bharat
>
>
> This email and any attachments are intended for the sole use of the
> named recipient(s) and contain(s) confidential information that may be
> proprietary, privileged or copyrighted under applicable law. If you
> are not the intended recipient, do not read, copy, or forward this
> email message or any attachments. Delete this email message and any
> attachments immediately.
>

Please fix your email not to carry such disclaimer (or use a private 
email address
if you can't work around your company policy). Posting confidential 
information

on a public mailing list feels a bit silly.

Thanks,

M.
--
Without deviation from the norm, progress is not possible.

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: 4.7 regression: ACPI: No IRQ available for PCI Interrupt Link [LNKD]. Try pci=noacpi or acpi=off

[okaya]

On 2016-09-30 02:44, Ondrej Zary wrote:

On Friday 30 September 2016, Sinan Kaya wrote:

On 9/29/2016 2:00 PM, Ondrej Zary wrote:
>> The previous two patches were in the right direction.
>>
>> > Can we also get the same output from 4.6 kernel with the attached
>> > patch for the same machine you sent these?
>
> Here it is.
>
>> > Something about SCI still doesn't feel right.
>> >
>> > The IRQ assignment fails if the penalty is greater than
>> > PIRQ_PENALTY_ISA_ALWAYS. This will happen if BIOS tells us to use an
>> > IRQ and same IRQ is in use by the SCI.

Thanks, I reverted penalize_sci function and dropped patch #1. Can you 
try

this again?


It seems to work, at least on one machine.


Ok, that comfirms my suspicion. We are  having trouble detecting sci 
interrupt  type and we end up penalizing the wrong value.


Can you try your other machines too?

I need to do some research now.

Re: 4.7 regression: ACPI: No IRQ available for PCI Interrupt Link [LNKD]. Try pci=noacpi or acpi=off

[okaya]

On 2016-09-30 02:44, Ondrej Zary wrote:

On Friday 30 September 2016, Sinan Kaya wrote:

On 9/29/2016 2:00 PM, Ondrej Zary wrote:
>> The previous two patches were in the right direction.
>>
>> > Can we also get the same output from 4.6 kernel with the attached
>> > patch for the same machine you sent these?
>
> Here it is.
>
>> > Something about SCI still doesn't feel right.
>> >
>> > The IRQ assignment fails if the penalty is greater than
>> > PIRQ_PENALTY_ISA_ALWAYS. This will happen if BIOS tells us to use an
>> > IRQ and same IRQ is in use by the SCI.

Thanks, I reverted penalize_sci function and dropped patch #1. Can you 
try

this again?


It seems to work, at least on one machine.


Ok, that comfirms my suspicion. We are  having trouble detecting sci 
interrupt  type and we end up penalizing the wrong value.


Can you try your other machines too?

I need to do some research now.

Re: 4.7 regression: ACPI: No IRQ available for PCI Interrupt Link [LNKD]. Try pci=noacpi or acpi=off

[okaya]

On 2016-09-29 05:10, Ondrej Zary wrote:

On Thursday 29 September 2016, Sinan Kaya wrote:

On 9/28/2016 3:23 PM, Ondrej Zary wrote:
> On Wednesday 28 September 2016 20:22:40 Sinan Kaya wrote:
>> On 9/28/2016 1:02 PM, Ondrej Zary wrote:
 Thanks, It sounds like you have more than one machine with similar

> problems. Can you collect the log from the other machines with
> 4.8-rc8?
>
> and also a boot log with 4.6 kernel where things are working?
>>>
>>> The attached logs are from another machine:
>>>
>>> dmesg-bad-debug.txt: 4.8-rc8 with your debug patch - bad
>>>
>>> dmesg-reverted.txt: 4.8-rc8 with patches (as per Rafael's suggestion)
>>> reverted - good
>>>
>>> dmesg-3.6.txt: 4.6 (Debian kernel) - good
>>
>> I think I see a race condition for the SCI interrupt. I need another
>> dump from 4.8-rc8 with the attached patch to confirm. Let's remove the
>> previous one and apply this one.
>
> dmesg-reverted.txt: 4.8-rc8 w/patches reverted (good)
> $ head /proc/interrupts
>CPU0
>   0:   8531XT-PIC  timer
>   1:  9XT-PIC  i8042
>   2:  0XT-PIC  cascade
>   8:  1XT-PIC  rtc0
>  11:713XT-PIC  acpi, uhci_hcd:usb1, uhci_hcd:usb2, nvkm, eth0
>  12:161XT-PIC  i8042
>  14:   4042XT-PIC  pata_via
>  15:  0XT-PIC  pata_via
> NMI:  0   Non-maskable interrupts
>
> dmesg-bad-debug.txt: 4.8-rc8 (bad)
> $ head /proc/interrupts
>CPU0
>   0:   8027XT-PIC  timer
>   1:286XT-PIC  i8042
>   2:  0XT-PIC  cascade
>   8:  1XT-PIC  rtc0
>  10:  0XT-PIC  uhci_hcd:usb1, uhci_hcd:usb2
>  11:  0XT-PIC  acpi, nvkm, eth0
>  12:161XT-PIC  i8042
>  14:   4069XT-PIC  pata_via
>  15:  0XT-PIC  pata_via
>
> (I'm moving between different machines through the day - these logs are
> from different machine than the last ones).

Can you try these patches on your machines please?


It doesn't even boot :(


Ok, since I have not seen the full boot log I am guessing that isa api 
gets called before the link objects are initialized.



Can you appply the first three only (0001, 0002 and 0003) to see if it 
makes a difference?




ACPI: No IRQ available for PCI Interrupt Link [LNKD]. Try pci=noacpi or 
acpi=off

BUG: unable to handle kernel paging request at e3382f64
IP: [] acpi_irq_get_penalty+0x69/0xa5
*pde = 
Oops:  [#1] SMP
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.8.0-rc8+ #114
Hardware name: System Manufacturer System Name/A7VL133, BIOS ASUS
A7VL133-VM ACPI BIOS Revision 1008 06/10/2002
task: c7094000 task.stack: c7098000
EIP: 0060:[] EFLAGS: 00210202 CPU:  0
EIP is at acpi_irq_get_penalty+0x69/0xa5
...
Call Trace:
acpi_isa_irq_available
acpi_pci_irq_enable
pcibios_enable_device
do_pci_enable_device
quirk_usb_early_handoff
pci_get_subsys
pci_fixup_device
pci_apply_final_quirks
pci_proc_init
do_one_initcall
parse_args
kernel_init_freeable
kernel_init_freeable
ret_from_kernel_thread
rest_init

Re: 4.7 regression: ACPI: No IRQ available for PCI Interrupt Link [LNKD]. Try pci=noacpi or acpi=off

[okaya]

On 2016-09-29 05:10, Ondrej Zary wrote:

On Thursday 29 September 2016, Sinan Kaya wrote:

On 9/28/2016 3:23 PM, Ondrej Zary wrote:
> On Wednesday 28 September 2016 20:22:40 Sinan Kaya wrote:
>> On 9/28/2016 1:02 PM, Ondrej Zary wrote:
 Thanks, It sounds like you have more than one machine with similar

> problems. Can you collect the log from the other machines with
> 4.8-rc8?
>
> and also a boot log with 4.6 kernel where things are working?
>>>
>>> The attached logs are from another machine:
>>>
>>> dmesg-bad-debug.txt: 4.8-rc8 with your debug patch - bad
>>>
>>> dmesg-reverted.txt: 4.8-rc8 with patches (as per Rafael's suggestion)
>>> reverted - good
>>>
>>> dmesg-3.6.txt: 4.6 (Debian kernel) - good
>>
>> I think I see a race condition for the SCI interrupt. I need another
>> dump from 4.8-rc8 with the attached patch to confirm. Let's remove the
>> previous one and apply this one.
>
> dmesg-reverted.txt: 4.8-rc8 w/patches reverted (good)
> $ head /proc/interrupts
>CPU0
>   0:   8531XT-PIC  timer
>   1:  9XT-PIC  i8042
>   2:  0XT-PIC  cascade
>   8:  1XT-PIC  rtc0
>  11:713XT-PIC  acpi, uhci_hcd:usb1, uhci_hcd:usb2, nvkm, eth0
>  12:161XT-PIC  i8042
>  14:   4042XT-PIC  pata_via
>  15:  0XT-PIC  pata_via
> NMI:  0   Non-maskable interrupts
>
> dmesg-bad-debug.txt: 4.8-rc8 (bad)
> $ head /proc/interrupts
>CPU0
>   0:   8027XT-PIC  timer
>   1:286XT-PIC  i8042
>   2:  0XT-PIC  cascade
>   8:  1XT-PIC  rtc0
>  10:  0XT-PIC  uhci_hcd:usb1, uhci_hcd:usb2
>  11:  0XT-PIC  acpi, nvkm, eth0
>  12:161XT-PIC  i8042
>  14:   4069XT-PIC  pata_via
>  15:  0XT-PIC  pata_via
>
> (I'm moving between different machines through the day - these logs are
> from different machine than the last ones).

Can you try these patches on your machines please?


It doesn't even boot :(


Ok, since I have not seen the full boot log I am guessing that isa api 
gets called before the link objects are initialized.



Can you appply the first three only (0001, 0002 and 0003) to see if it 
makes a difference?




ACPI: No IRQ available for PCI Interrupt Link [LNKD]. Try pci=noacpi or 
acpi=off

BUG: unable to handle kernel paging request at e3382f64
IP: [] acpi_irq_get_penalty+0x69/0xa5
*pde = 
Oops:  [#1] SMP
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.8.0-rc8+ #114
Hardware name: System Manufacturer System Name/A7VL133, BIOS ASUS
A7VL133-VM ACPI BIOS Revision 1008 06/10/2002
task: c7094000 task.stack: c7098000
EIP: 0060:[] EFLAGS: 00210202 CPU:  0
EIP is at acpi_irq_get_penalty+0x69/0xa5
...
Call Trace:
acpi_isa_irq_available
acpi_pci_irq_enable
pcibios_enable_device
do_pci_enable_device
quirk_usb_early_handoff
pci_get_subsys
pci_fixup_device
pci_apply_final_quirks
pci_proc_init
do_one_initcall
parse_args
kernel_init_freeable
kernel_init_freeable
ret_from_kernel_thread
rest_init

Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the callback

[okaya]

On 2016-08-19 01:52, Vinod Koul wrote:

On Thu, Aug 18, 2016 at 11:48:52PM -0400, Sinan Kaya wrote:

On 8/18/2016 11:42 PM, Vinod Koul wrote:
> On Thu, Aug 18, 2016 at 11:26:28PM -0400, Sinan Kaya wrote:
>> On 8/18/2016 10:48 PM, Vinod Koul wrote:
 Keep a size limited list with error cookies and flush them in terminate 
all?
>>> I think so, terminate_all anyway cleans up the channel. Btw what is the
>>> behaviour on error? Do you terminate or somthing else?
>>>
>>
>> On error, I flush all outstanding transactions with an error code and I reset
>> the channel. After the reset, the DMA channel is functional again. The client
>> doesn't need to shutdown anything.
>
> You mean from the client context or driver?
>

The client doesn't need to call device_free_chan_resources and 
device_terminate_all
to be specific. Client can certainly call these if it needs to but it 
is not

required to recover the channel.


You didn't answer my question!

On error you said you flush, so who does that?


This is done by the driver in interrupt context when an error interrupt 
is received. All transactions are posted and hw is reset.




After the reset in error condition, the client can continue issuing 
new requests

with tx_submit and device_issue_pending as usual.

Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the callback

[okaya]

On 2016-08-19 01:52, Vinod Koul wrote:

On Thu, Aug 18, 2016 at 11:48:52PM -0400, Sinan Kaya wrote:

On 8/18/2016 11:42 PM, Vinod Koul wrote:
> On Thu, Aug 18, 2016 at 11:26:28PM -0400, Sinan Kaya wrote:
>> On 8/18/2016 10:48 PM, Vinod Koul wrote:
 Keep a size limited list with error cookies and flush them in terminate 
all?
>>> I think so, terminate_all anyway cleans up the channel. Btw what is the
>>> behaviour on error? Do you terminate or somthing else?
>>>
>>
>> On error, I flush all outstanding transactions with an error code and I reset
>> the channel. After the reset, the DMA channel is functional again. The client
>> doesn't need to shutdown anything.
>
> You mean from the client context or driver?
>

The client doesn't need to call device_free_chan_resources and 
device_terminate_all
to be specific. Client can certainly call these if it needs to but it 
is not

required to recover the channel.


You didn't answer my question!

On error you said you flush, so who does that?


This is done by the driver in interrupt context when an error interrupt 
is received. All transactions are posted and hw is reset.




After the reset in error condition, the client can continue issuing 
new requests

with tx_submit and device_issue_pending as usual.

Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the callback

[okaya]

On 2016-08-08 04:51, Vinod Koul wrote:


This patch is needed to fix a race condition as the commit message 
describes.
The callback is called before returning the descriptor back to free 
pool.


If the client calls free resources, the descriptor that was not 
returned to free pool gets lost due

to race condition.


Hmmm, if you have txn's pending and client wants to free up, shouldn't
the pending txn's be cleaned up? Sound like a different bug to me..

So if I submit 5 txn's and now want to freeup, will you still leak
descriptors? Doesn't sound as right behaviour to me.



If free is called from the callback, current code will leak the current 
descriptor where free was called. It will release the other 4.


Because of ordering problem, descriptor is not in the active, pending or 
free pool.


I check pending txn by looking at active and queued lists when free is 
called.


After the callback, I put the descriptor back to free pool. At this 
moment, it is already too late.



I'll refactor the code after Dave's change for passing the error code 
while calling the

callback. That will be a different patch anyhow.


Yes the error reporting is different

Re: [PATCH] dmaengine: qcom_hidma: release the descriptor before the callback

[okaya]

On 2016-08-08 04:51, Vinod Koul wrote:


This patch is needed to fix a race condition as the commit message 
describes.
The callback is called before returning the descriptor back to free 
pool.


If the client calls free resources, the descriptor that was not 
returned to free pool gets lost due

to race condition.


Hmmm, if you have txn's pending and client wants to free up, shouldn't
the pending txn's be cleaned up? Sound like a different bug to me..

So if I submit 5 txn's and now want to freeup, will you still leak
descriptors? Doesn't sound as right behaviour to me.



If free is called from the callback, current code will leak the current 
descriptor where free was called. It will release the other 4.


Because of ordering problem, descriptor is not in the active, pending or 
free pool.


I check pending txn by looking at active and queued lists when free is 
called.


After the callback, I put the descriptor back to free pool. At this 
moment, it is already too late.



I'll refactor the code after Dave's change for passing the error code 
while calling the

callback. That will be a different patch anyhow.


Yes the error reporting is different

Re: [PATCH 10/10] dmaengine: qcom_hidma: add MSI support for interrupts

[okaya]

On 2016-08-08 04:14, Vinod Koul wrote:

On Thu, Aug 04, 2016 at 09:59:43AM -0400, Sinan Kaya wrote:

On 8/4/2016 8:46 AM, Vinod Koul wrote:
>> > Yes, I do have a new device ID for platforms with MSI capability.
>> >
>> > Which new binding are you referring to?
> If you have "QCOM8062" why do you need DT to tell hidma-1.1 ?

Unfortunately, DT cannot do a binding with the ACPI names. Similarly, 
ACPI

cannot do a binding with the DT name.

The structure of binding name is also subject to different kind of 
rules

for DT and ACPI.

This driver supports both device tree and ACPI. That's why, two 
different

names are required.


Hmmm, wasn't the who get_property stuff supposed to make properties 
work on
both ACPi & DT. I am not sure though about the current state of affairs 
on

that.



Get property works. It is able to abstract device driver properties. A 
driver doesn't need to know whether it is coming from acpi dsd or of.


However, no such mechanism exists for driver names due to nature of 
different naming requirements.


Of has its own match table and acpi has its own. Acpi also has 
ridiculous 8 character name limitation.

Re: [PATCH 10/10] dmaengine: qcom_hidma: add MSI support for interrupts

[okaya]

On 2016-08-08 04:14, Vinod Koul wrote:

On Thu, Aug 04, 2016 at 09:59:43AM -0400, Sinan Kaya wrote:

On 8/4/2016 8:46 AM, Vinod Koul wrote:
>> > Yes, I do have a new device ID for platforms with MSI capability.
>> >
>> > Which new binding are you referring to?
> If you have "QCOM8062" why do you need DT to tell hidma-1.1 ?

Unfortunately, DT cannot do a binding with the ACPI names. Similarly, 
ACPI

cannot do a binding with the DT name.

The structure of binding name is also subject to different kind of 
rules

for DT and ACPI.

This driver supports both device tree and ACPI. That's why, two 
different

names are required.


Hmmm, wasn't the who get_property stuff supposed to make properties 
work on
both ACPi & DT. I am not sure though about the current state of affairs 
on

that.



Get property works. It is able to abstract device driver properties. A 
driver doesn't need to know whether it is coming from acpi dsd or of.


However, no such mechanism exists for driver names due to nature of 
different naming requirements.


Of has its own match table and acpi has its own. Acpi also has 
ridiculous 8 character name limitation.

Re: [PATCH V10 4/9] vfio: platform: add support for ACPI probe

[okaya]

On 2016-07-18 20:00, Alex Williamson wrote:

On Mon, 18 Jul 2016 19:09:22 -0400
Sinan Kaya  wrote:


The code is using the compatible DT string to associate a reset driver
with the actual device itself. The compatible string does not exist on
ACPI based systems. HID is the unique identifier for a device driver
instead.

Signed-off-by: Sinan Kaya 
---
 drivers/vfio/platform/vfio_platform_common.c  | 69 
+--

 drivers/vfio/platform/vfio_platform_private.h |  1 +
 2 files changed, 65 insertions(+), 5 deletions(-)

diff --git a/drivers/vfio/platform/vfio_platform_common.c 
b/drivers/vfio/platform/vfio_platform_common.c

index 6be92c3..a5299f6 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -13,6 +13,7 @@
  */

 #include 
+#include 
 #include 
 #include 
 #include 
@@ -49,6 +50,32 @@ static vfio_platform_reset_fn_t 
vfio_platform_lookup_reset(const char *compat,

return reset_fn;
 }

+static int vfio_platform_acpi_probe(struct vfio_platform_device 
*vdev,

+   struct device *dev)
+{
+   struct acpi_device *adev;
+
+   if (acpi_disabled)
+   return -EPERM;
+
+   adev = ACPI_COMPANION(dev);


I didn't necessarily have a problem with this being set in the
declaration.


I think this is better. If ACPI is disabled, it is dangerous to call an 
ACPI API.






+   if (!adev) {
+   pr_err("VFIO: ACPI companion device not found for %s\n",
+   vdev->name);
+   return -ENODEV;
+   }
+
+#ifdef CONFIG_ACPI
+   vdev->acpihid = acpi_device_hid(adev);
+   if (!vdev->acpihid) {
+   pr_err("VFIO: cannot find ACPI HID for %s\n",
+  vdev->name);
+   return -EINVAL;
+   }
+#endif
+   return WARN_ON(!vdev->acpihid) ? -ENOENT : 0;


?!?!  The point was that that entire if{} branch is unnecessary.  The
WARN_ON handles the (impossible) case of !vdev->acpihid.  We just need:

#ifdef CONFIG_ACPI
vdev->acpihid = acpi_device_hid(adev);
#endif
return WARN_ON(!vdev->acpihid) ? -ENOENT : 0;



OK, got it now. I thought you were trying to get rid of #else


nit, might make sense to replace EPERM with ENOENT and use EINVAL here.



Sure, will take carr of it.

Anything else I need to take care of?


+}
+
 static bool vfio_platform_has_reset(struct vfio_platform_device 
*vdev)

 {
return vdev->of_reset ? true : false;
@@ -547,6 +574,37 @@ static const struct vfio_device_ops 
vfio_platform_ops = {

.mmap   = vfio_platform_mmap,
 };

+int vfio_platform_of_probe(struct vfio_platform_device *vdev,
+  struct device *dev)
+{
+   int ret;
+
+   ret = device_property_read_string(dev, "compatible",
+ >compat);
+   if (ret)
+   pr_err("VFIO: cannot retrieve compat for %s\n",
+   vdev->name);
+
+   return ret;
+}
+
+/*
+ * There can be two kernel build combinations. One build where
+ * ACPI is not selected in Kconfig and another one with the ACPI 
Kconfig.

+ *
+ * In the first case, vfio_platform_acpi_probe will return since
+ * acpi_disabled is 1. DT user will not see any kind of messages from
+ * ACPI.
+ *
+ * In the second case, both DT and ACPI is compiled in but the system 
is

+ * booting with any of these combinations.
+ *
+ * If the firmware is DT type, then acpi_disabled is 1. The ACPI 
probe routine

+ * terminates immediately without any messages.
+ *
+ * If the firmware is ACPI type, then acpi_disabled is 0. All other 
checks are

+ * valid checks. We cannot claim that this system is DT.
+ */
 int vfio_platform_probe_common(struct vfio_platform_device *vdev,
   struct device *dev)
 {
@@ -556,11 +614,12 @@ int vfio_platform_probe_common(struct 
vfio_platform_device *vdev,

if (!vdev)
return -EINVAL;

-   ret = device_property_read_string(dev, "compatible", >compat);
-   if (ret) {
-   pr_err("VFIO: cannot retrieve compat for %s\n", vdev->name);
-   return -EINVAL;
-   }
+   ret = vfio_platform_acpi_probe(vdev, dev);
+   if (ret)
+   ret = vfio_platform_of_probe(vdev, dev);
+
+   if (ret)
+   return ret;

vdev->device = dev;

diff --git a/drivers/vfio/platform/vfio_platform_private.h 
b/drivers/vfio/platform/vfio_platform_private.h

index 71ed7d1..ba9e4f8 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -58,6 +58,7 @@ struct vfio_platform_device {
struct mutexigate;
struct module   *parent_module;
const char  *compat;
+   const char  *acpihid;
struct module   

Re: [PATCH V10 4/9] vfio: platform: add support for ACPI probe

[okaya]

On 2016-07-18 20:00, Alex Williamson wrote:

On Mon, 18 Jul 2016 19:09:22 -0400
Sinan Kaya  wrote:


The code is using the compatible DT string to associate a reset driver
with the actual device itself. The compatible string does not exist on
ACPI based systems. HID is the unique identifier for a device driver
instead.

Signed-off-by: Sinan Kaya 
---
 drivers/vfio/platform/vfio_platform_common.c  | 69 
+--

 drivers/vfio/platform/vfio_platform_private.h |  1 +
 2 files changed, 65 insertions(+), 5 deletions(-)

diff --git a/drivers/vfio/platform/vfio_platform_common.c 
b/drivers/vfio/platform/vfio_platform_common.c

index 6be92c3..a5299f6 100644
--- a/drivers/vfio/platform/vfio_platform_common.c
+++ b/drivers/vfio/platform/vfio_platform_common.c
@@ -13,6 +13,7 @@
  */

 #include 
+#include 
 #include 
 #include 
 #include 
@@ -49,6 +50,32 @@ static vfio_platform_reset_fn_t 
vfio_platform_lookup_reset(const char *compat,

return reset_fn;
 }

+static int vfio_platform_acpi_probe(struct vfio_platform_device 
*vdev,

+   struct device *dev)
+{
+   struct acpi_device *adev;
+
+   if (acpi_disabled)
+   return -EPERM;
+
+   adev = ACPI_COMPANION(dev);


I didn't necessarily have a problem with this being set in the
declaration.


I think this is better. If ACPI is disabled, it is dangerous to call an 
ACPI API.






+   if (!adev) {
+   pr_err("VFIO: ACPI companion device not found for %s\n",
+   vdev->name);
+   return -ENODEV;
+   }
+
+#ifdef CONFIG_ACPI
+   vdev->acpihid = acpi_device_hid(adev);
+   if (!vdev->acpihid) {
+   pr_err("VFIO: cannot find ACPI HID for %s\n",
+  vdev->name);
+   return -EINVAL;
+   }
+#endif
+   return WARN_ON(!vdev->acpihid) ? -ENOENT : 0;


?!?!  The point was that that entire if{} branch is unnecessary.  The
WARN_ON handles the (impossible) case of !vdev->acpihid.  We just need:

#ifdef CONFIG_ACPI
vdev->acpihid = acpi_device_hid(adev);
#endif
return WARN_ON(!vdev->acpihid) ? -ENOENT : 0;



OK, got it now. I thought you were trying to get rid of #else


nit, might make sense to replace EPERM with ENOENT and use EINVAL here.



Sure, will take carr of it.

Anything else I need to take care of?


+}
+
 static bool vfio_platform_has_reset(struct vfio_platform_device 
*vdev)

 {
return vdev->of_reset ? true : false;
@@ -547,6 +574,37 @@ static const struct vfio_device_ops 
vfio_platform_ops = {

.mmap   = vfio_platform_mmap,
 };

+int vfio_platform_of_probe(struct vfio_platform_device *vdev,
+  struct device *dev)
+{
+   int ret;
+
+   ret = device_property_read_string(dev, "compatible",
+ >compat);
+   if (ret)
+   pr_err("VFIO: cannot retrieve compat for %s\n",
+   vdev->name);
+
+   return ret;
+}
+
+/*
+ * There can be two kernel build combinations. One build where
+ * ACPI is not selected in Kconfig and another one with the ACPI 
Kconfig.

+ *
+ * In the first case, vfio_platform_acpi_probe will return since
+ * acpi_disabled is 1. DT user will not see any kind of messages from
+ * ACPI.
+ *
+ * In the second case, both DT and ACPI is compiled in but the system 
is

+ * booting with any of these combinations.
+ *
+ * If the firmware is DT type, then acpi_disabled is 1. The ACPI 
probe routine

+ * terminates immediately without any messages.
+ *
+ * If the firmware is ACPI type, then acpi_disabled is 0. All other 
checks are

+ * valid checks. We cannot claim that this system is DT.
+ */
 int vfio_platform_probe_common(struct vfio_platform_device *vdev,
   struct device *dev)
 {
@@ -556,11 +614,12 @@ int vfio_platform_probe_common(struct 
vfio_platform_device *vdev,

if (!vdev)
return -EINVAL;

-   ret = device_property_read_string(dev, "compatible", >compat);
-   if (ret) {
-   pr_err("VFIO: cannot retrieve compat for %s\n", vdev->name);
-   return -EINVAL;
-   }
+   ret = vfio_platform_acpi_probe(vdev, dev);
+   if (ret)
+   ret = vfio_platform_of_probe(vdev, dev);
+
+   if (ret)
+   return ret;

vdev->device = dev;

diff --git a/drivers/vfio/platform/vfio_platform_private.h 
b/drivers/vfio/platform/vfio_platform_private.h

index 71ed7d1..ba9e4f8 100644
--- a/drivers/vfio/platform/vfio_platform_private.h
+++ b/drivers/vfio/platform/vfio_platform_private.h
@@ -58,6 +58,7 @@ struct vfio_platform_device {
struct mutexigate;
struct module   *parent_module;
const char  *compat;
+   const char  *acpihid;
struct module   *reset_module;
struct device   

Re: [GIT PULL] ACPI fixes for v4.7-rc7

[okaya]

On 2016-07-10 14:00, Thorsten Leemhuis wrote:

Hi Rafael!

On 08.07.2016 01:43, Rafael J. Wysocki wrote:


Please pull from

 git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
 acpi-4.7-rc7

to receive ACPI fixes for v4.7-rc7 with top-most commit
[…]
All of these fix recent regressions in ACPICA, in the ACPI PCI IRQ
management code and in the ACPI AML debugger.


FYI, it seems these changes lead to a new regression. Quoting
https://bugzilla.kernel.org/show_bug.cgi?id=121701

"""

Kernel fails to boot with this commit. With the commit reverted,
everything is fine. I cannot bisect any further - git is failing with
"you are trying to use to much memory"(?!)

This is a Dell Precision 5510 with the latest (1.2.10) BIOS applied.

Let me know what additional information I can provide."""

"""


Can you attach the boot log to the bugzilla?

Unfortunately, this code turned out to be the most fragile code. I fixed 
one issue and it looks like it broke yours.





Cheers, Thorsten
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [GIT PULL] ACPI fixes for v4.7-rc7

[okaya]

On 2016-07-10 14:00, Thorsten Leemhuis wrote:

Hi Rafael!

On 08.07.2016 01:43, Rafael J. Wysocki wrote:


Please pull from

 git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
 acpi-4.7-rc7

to receive ACPI fixes for v4.7-rc7 with top-most commit
[…]
All of these fix recent regressions in ACPICA, in the ACPI PCI IRQ
management code and in the ACPI AML debugger.


FYI, it seems these changes lead to a new regression. Quoting
https://bugzilla.kernel.org/show_bug.cgi?id=121701

"""

Kernel fails to boot with this commit. With the commit reverted,
everything is fine. I cannot bisect any further - git is failing with
"you are trying to use to much memory"(?!)

This is a Dell Precision 5510 with the latest (1.2.10) BIOS applied.

Let me know what additional information I can provide."""

"""


Can you attach the boot log to the bugzilla?

Unfortunately, this code turned out to be the most fragile code. I fixed 
one issue and it looks like it broke yours.





Cheers, Thorsten
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: kernel-4.7 bug in Intel sound and/or ACPI

[okaya]

On 2016-06-30 05:30, Wim Osterholt wrote:

On Wed, Jun 29, 2016 at 04:34:14AM -0400, Sinan Kaya wrote:

>

I posted this [PATCH V2 0/4] ACPI,PCI,IRQ: correct ISA penalty 
calculation


Can you test this and give me a tested-by?



Kernel-4.7-rc5 plus this patch works like a charm on my Inspiron 4100.
Dmesg output is quite similar to the one from kernel-4.6 .

Tested-by: Wim Osterholt. 



Thank you.





--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: kernel-4.7 bug in Intel sound and/or ACPI

[okaya]

On 2016-06-30 05:30, Wim Osterholt wrote:

On Wed, Jun 29, 2016 at 04:34:14AM -0400, Sinan Kaya wrote:

>

I posted this [PATCH V2 0/4] ACPI,PCI,IRQ: correct ISA penalty 
calculation


Can you test this and give me a tested-by?



Kernel-4.7-rc5 plus this patch works like a charm on my Inspiron 4100.
Dmesg output is quite similar to the one from kernel-4.6 .

Tested-by: Wim Osterholt. 



Thank you.





--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: kernel-4.7 bug in Intel sound and/or ACPI

[okaya]

On 2016-06-27 16:04, Wim Osterholt wrote:

On Mon, Jun 27, 2016 at 04:22:18AM -0400, ok...@codeaurora.org wrote:

> However, an earlier try on my Inspiron 510m did not work.
> I'll do a clean retry later today, just to make sure.


Ok, let me know. I can post a clean patch series. I was trying to get
you something to test before posting the official version.


The 510m just finished compiling and now it works fine too.
Thanks.



Nice, I will post the official version and let you know.




Regards, Wim.


- w...@djo.tudelft.nl -

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: kernel-4.7 bug in Intel sound and/or ACPI

[okaya]

On 2016-06-27 16:04, Wim Osterholt wrote:

On Mon, Jun 27, 2016 at 04:22:18AM -0400, ok...@codeaurora.org wrote:

> However, an earlier try on my Inspiron 510m did not work.
> I'll do a clean retry later today, just to make sure.


Ok, let me know. I can post a clean patch series. I was trying to get
you something to test before posting the official version.


The 510m just finished compiling and now it works fine too.
Thanks.



Nice, I will post the official version and let you know.




Regards, Wim.


- w...@djo.tudelft.nl -

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: kernel-4.7 bug in Intel sound and/or ACPI

[okaya]

On 2016-06-27 02:27, Wim Osterholt wrote:

On Sat, Jun 25, 2016 at 04:51:03AM -0400, ok...@codeaurora.org wrote:

On 2016-06-24 21:39, Wim Osterholt wrote:

Please apply the patches on top of clean 4.7-rc4 tree and apply them 
in

order with

git am 0001...
git am 0002...


It doesn't work that way.
Beginners problems with git.
Tried all kinds of things, including a new git clone to no avail.
Took a long time to discover that in the above example these were the 
names

of the 'attachments'.
It didn't work. Error 'could not find out the kind of patch' or some 
such.
Took much longer to find out that in that mail and in the saved 
attachments

there were lines beginning with '>From' that were the culprit.

Still not found out how to make patch work without errors.
Anyway, by doing it with more manual intervention om a stock 
kernel-4.7-rc4

on my (very slow) Inspiron 4100 it seems to work like before. Hooray.
However, an earlier try on my Inspiron 510m did not work.
I'll do a clean retry later today, just to make sure.



Ok, let me know. I can post a clean patch series. I was trying to get 
you something to test before posting the official version.




regards, Wim.


- w...@djo.tudelft.nl -

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: kernel-4.7 bug in Intel sound and/or ACPI

[okaya]

On 2016-06-27 02:27, Wim Osterholt wrote:

On Sat, Jun 25, 2016 at 04:51:03AM -0400, ok...@codeaurora.org wrote:

On 2016-06-24 21:39, Wim Osterholt wrote:

Please apply the patches on top of clean 4.7-rc4 tree and apply them 
in

order with

git am 0001...
git am 0002...


It doesn't work that way.
Beginners problems with git.
Tried all kinds of things, including a new git clone to no avail.
Took a long time to discover that in the above example these were the 
names

of the 'attachments'.
It didn't work. Error 'could not find out the kind of patch' or some 
such.
Took much longer to find out that in that mail and in the saved 
attachments

there were lines beginning with '>From' that were the culprit.

Still not found out how to make patch work without errors.
Anyway, by doing it with more manual intervention om a stock 
kernel-4.7-rc4

on my (very slow) Inspiron 4100 it seems to work like before. Hooray.
However, an earlier try on my Inspiron 510m did not work.
I'll do a clean retry later today, just to make sure.



Ok, let me know. I can post a clean patch series. I was trying to get 
you something to test before posting the official version.




regards, Wim.


- w...@djo.tudelft.nl -

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: kernel-4.7 bug in Intel sound and/or ACPI

[okaya]

On 2016-06-24 21:39, Wim Osterholt wrote:

On Fri, Jun 24, 2016 at 02:09:15AM -0400, Sinan Kaya wrote:


Can you give it a try?


Whell, I tried to no avail.

Wether it is on 4.6 or 4.7, with or without your previous patch,
I keep getting rejected hunks.
For example, here the line to be deleted is nowhere to be found:


diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
index 714ba4d..c2f22c9 100644
--- a/drivers/acpi/pci_link.c
+++ b/drivers/acpi/pci_link.c
@@ -497,7 +497,7 @@ static int acpi_irq_get_penalty(int irq)
int penalty = 0;

if (irq < ACPI_MAX_ISA_IRQS)
-   penalty += acpi_isa_irq_penalty[irq];
+   return acpi_isa_irq_penalty[irq];

/*
* Penalize IRQ used by ACPI SCI. If ACPI SCI pin attributes conflict



Regards, Wim.


- w...@djo.tudelft.nl -


Please apply the patches on top of clean 4.7-rc4 tree and apply them in 
order with


git am 0001...
git am 0002...

Re: kernel-4.7 bug in Intel sound and/or ACPI

[okaya]

On 2016-06-24 21:39, Wim Osterholt wrote:

On Fri, Jun 24, 2016 at 02:09:15AM -0400, Sinan Kaya wrote:


Can you give it a try?


Whell, I tried to no avail.

Wether it is on 4.6 or 4.7, with or without your previous patch,
I keep getting rejected hunks.
For example, here the line to be deleted is nowhere to be found:


diff --git a/drivers/acpi/pci_link.c b/drivers/acpi/pci_link.c
index 714ba4d..c2f22c9 100644
--- a/drivers/acpi/pci_link.c
+++ b/drivers/acpi/pci_link.c
@@ -497,7 +497,7 @@ static int acpi_irq_get_penalty(int irq)
int penalty = 0;

if (irq < ACPI_MAX_ISA_IRQS)
-   penalty += acpi_isa_irq_penalty[irq];
+   return acpi_isa_irq_penalty[irq];

/*
* Penalize IRQ used by ACPI SCI. If ACPI SCI pin attributes conflict



Regards, Wim.


- w...@djo.tudelft.nl -


Please apply the patches on top of clean 4.7-rc4 tree and apply them in 
order with


git am 0001...
git am 0002...

Re: kernel-4.7 bug in Intel sound and/or ACPI

[okaya]

On 2016-06-21 18:13, Wim Osterholt wrote:

On Tue, Jun 21, 2016 at 09:40:10AM -0400, Sinan Kaya wrote:


Thanks, It was a guess with no proof.

Let's undo the change above and start adding some print statements to 
collect

data from your system.

Can you add this to the end of acpi_irq_get_penalty function and then 
send

the output?

pr_info("%s:%d irq = %d penalty = %d\n", __func__, __LINE__, irq,
penalty);



This produced some 60 lines extra. Too much to include here.
The entire dmesg file is here:
http://webserver.djo.tudelft.nl/dmesg474+printpenalty


Thanks, let's go back to 4.6 and add a very similar printf to every 
single place where the array is modified and also right before the 
enabled message.


I am trying to find a system with similar characteristics for debug






Regards, Wim.


- w...@djo.tudelft.nl -

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: kernel-4.7 bug in Intel sound and/or ACPI

[okaya]

On 2016-06-21 18:13, Wim Osterholt wrote:

On Tue, Jun 21, 2016 at 09:40:10AM -0400, Sinan Kaya wrote:


Thanks, It was a guess with no proof.

Let's undo the change above and start adding some print statements to 
collect

data from your system.

Can you add this to the end of acpi_irq_get_penalty function and then 
send

the output?

pr_info("%s:%d irq = %d penalty = %d\n", __func__, __LINE__, irq,
penalty);



This produced some 60 lines extra. Too much to include here.
The entire dmesg file is here:
http://webserver.djo.tudelft.nl/dmesg474+printpenalty


Thanks, let's go back to 4.6 and add a very similar printf to every 
single place where the array is modified and also right before the 
enabled message.


I am trying to find a system with similar characteristics for debug






Regards, Wim.


- w...@djo.tudelft.nl -

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html