Re: [PATCH v5 3/4] PCI/DPC: Unify and plumb error handling into DPC

[poza]

On 2018-01-19 10:14, Sinan Kaya wrote:

On 1/18/2018 11:23 PM, p...@codeaurora.org wrote:

On 2018-01-18 23:33, Sinan Kaya wrote:

On 1/18/2018 1:00 PM, p...@codeaurora.org wrote:
I think you would put into include/linux/pci.h only if there is an 
external
use of constant outside of drivers/pci directory. Otherwise, you 
should keep
the setting inside one of the header files in drivers/pci 
directory.


I don't see any other subsystem caring about DPC_FATAL definition.


ok so you are suggesting to move only DPC_FATAL ? so then AER can 
stay where it is.


Now that both AER and DPC handling is getting unified, I think it 
makes sense to
keep all error codes (AER+DPC) together in drivers/pci/pci.h rather 
than having

them split in aer.h and dpc.h.

Otherwise, how would we avoid having a new error type defined with 
the

existing values.


I agree, its is just that drivers/acpi/apet/ghes.c has to do
#include ../../pci/pci.h


That's bad. I was just thinking about the DPC error code only. I didn't 
realize

AER error codes are being referenced from ghes.c.



but thats okay I think.  let me move error codes to drivers/pci/pci.h.


It is better if error codes move to include/linux/pci.h and keep them 
together.




The problem with moving them to include/linux/pci.h, it falls into 
global scope, besides

they have to be renamed to/prefixed with PCI_ERR_xxx

the use of AER_FATAL, DPC_FATAL etc.. is very limited in entire linux. 
and likely to be so.

I think moving them to drivers/pci/pci.h would be more restricted/local

let me make patch-set based on that, and see how it looks like. we can 
arrive at some consensus then.




Regards,
Oza.

Re: [PATCH v5 3/4] PCI/DPC: Unify and plumb error handling into DPC

[poza]

On 2018-01-19 10:14, Sinan Kaya wrote:

On 1/18/2018 11:23 PM, p...@codeaurora.org wrote:

On 2018-01-18 23:33, Sinan Kaya wrote:

On 1/18/2018 1:00 PM, p...@codeaurora.org wrote:
I think you would put into include/linux/pci.h only if there is an 
external
use of constant outside of drivers/pci directory. Otherwise, you 
should keep
the setting inside one of the header files in drivers/pci 
directory.


I don't see any other subsystem caring about DPC_FATAL definition.


ok so you are suggesting to move only DPC_FATAL ? so then AER can 
stay where it is.


Now that both AER and DPC handling is getting unified, I think it 
makes sense to
keep all error codes (AER+DPC) together in drivers/pci/pci.h rather 
than having

them split in aer.h and dpc.h.

Otherwise, how would we avoid having a new error type defined with 
the

existing values.


I agree, its is just that drivers/acpi/apet/ghes.c has to do
#include ../../pci/pci.h


That's bad. I was just thinking about the DPC error code only. I didn't 
realize

AER error codes are being referenced from ghes.c.



but thats okay I think.  let me move error codes to drivers/pci/pci.h.


It is better if error codes move to include/linux/pci.h and keep them 
together.




The problem with moving them to include/linux/pci.h, it falls into 
global scope, besides

they have to be renamed to/prefixed with PCI_ERR_xxx

the use of AER_FATAL, DPC_FATAL etc.. is very limited in entire linux. 
and likely to be so.

I think moving them to drivers/pci/pci.h would be more restricted/local

let me make patch-set based on that, and see how it looks like. we can 
arrive at some consensus then.




Regards,
Oza.

Re: [PATCH v5 3/4] PCI/DPC: Unify and plumb error handling into DPC

[Sinan Kaya]

On 1/18/2018 11:23 PM, p...@codeaurora.org wrote:
> On 2018-01-18 23:33, Sinan Kaya wrote:
>> On 1/18/2018 1:00 PM, p...@codeaurora.org wrote:
 I think you would put into include/linux/pci.h only if there is an external
 use of constant outside of drivers/pci directory. Otherwise, you should 
 keep
 the setting inside one of the header files in drivers/pci directory.

 I don't see any other subsystem caring about DPC_FATAL definition.
>>>
>>> ok so you are suggesting to move only DPC_FATAL ? so then AER can stay 
>>> where it is.
>>
>> Now that both AER and DPC handling is getting unified, I think it makes 
>> sense to
>> keep all error codes (AER+DPC) together in drivers/pci/pci.h rather than 
>> having
>> them split in aer.h and dpc.h.
>>
>> Otherwise, how would we avoid having a new error type defined with the
>> existing values.
> 
> I agree, its is just that drivers/acpi/apet/ghes.c has to do
> #include ../../pci/pci.h

That's bad. I was just thinking about the DPC error code only. I didn't realize
AER error codes are being referenced from ghes.c.

> 
> but thats okay I think.  let me move error codes to drivers/pci/pci.h.

It is better if error codes move to include/linux/pci.h and keep them together.

> 
> Regards,
> Oza.
> 


-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.

Re: [PATCH v5 3/4] PCI/DPC: Unify and plumb error handling into DPC

[Sinan Kaya]

On 1/18/2018 11:23 PM, p...@codeaurora.org wrote:
> On 2018-01-18 23:33, Sinan Kaya wrote:
>> On 1/18/2018 1:00 PM, p...@codeaurora.org wrote:
 I think you would put into include/linux/pci.h only if there is an external
 use of constant outside of drivers/pci directory. Otherwise, you should 
 keep
 the setting inside one of the header files in drivers/pci directory.

 I don't see any other subsystem caring about DPC_FATAL definition.
>>>
>>> ok so you are suggesting to move only DPC_FATAL ? so then AER can stay 
>>> where it is.
>>
>> Now that both AER and DPC handling is getting unified, I think it makes 
>> sense to
>> keep all error codes (AER+DPC) together in drivers/pci/pci.h rather than 
>> having
>> them split in aer.h and dpc.h.
>>
>> Otherwise, how would we avoid having a new error type defined with the
>> existing values.
> 
> I agree, its is just that drivers/acpi/apet/ghes.c has to do
> #include ../../pci/pci.h

That's bad. I was just thinking about the DPC error code only. I didn't realize
AER error codes are being referenced from ghes.c.

> 
> but thats okay I think.  let me move error codes to drivers/pci/pci.h.

It is better if error codes move to include/linux/pci.h and keep them together.

> 
> Regards,
> Oza.
> 


-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.

Re: [PATCH v5 3/4] PCI/DPC: Unify and plumb error handling into DPC

[poza]

On 2018-01-18 23:33, Sinan Kaya wrote:

On 1/18/2018 1:00 PM, p...@codeaurora.org wrote:
I think you would put into include/linux/pci.h only if there is an 
external
use of constant outside of drivers/pci directory. Otherwise, you 
should keep

the setting inside one of the header files in drivers/pci directory.

I don't see any other subsystem caring about DPC_FATAL definition.


ok so you are suggesting to move only DPC_FATAL ? so then AER can stay 
where it is.


Now that both AER and DPC handling is getting unified, I think it makes 
sense to
keep all error codes (AER+DPC) together in drivers/pci/pci.h rather 
than having

them split in aer.h and dpc.h.

Otherwise, how would we avoid having a new error type defined with the
existing values.


I agree, its is just that drivers/acpi/apet/ghes.c has to do
#include ../../pci/pci.h

but thats okay I think.  let me move error codes to drivers/pci/pci.h.

Regards,
Oza.

Re: [PATCH v5 3/4] PCI/DPC: Unify and plumb error handling into DPC

[poza]

On 2018-01-18 23:33, Sinan Kaya wrote:

On 1/18/2018 1:00 PM, p...@codeaurora.org wrote:
I think you would put into include/linux/pci.h only if there is an 
external
use of constant outside of drivers/pci directory. Otherwise, you 
should keep

the setting inside one of the header files in drivers/pci directory.

I don't see any other subsystem caring about DPC_FATAL definition.


ok so you are suggesting to move only DPC_FATAL ? so then AER can stay 
where it is.


Now that both AER and DPC handling is getting unified, I think it makes 
sense to
keep all error codes (AER+DPC) together in drivers/pci/pci.h rather 
than having

them split in aer.h and dpc.h.

Otherwise, how would we avoid having a new error type defined with the
existing values.


I agree, its is just that drivers/acpi/apet/ghes.c has to do
#include ../../pci/pci.h

but thats okay I think.  let me move error codes to drivers/pci/pci.h.

Regards,
Oza.

Re: [PATCH v5 3/4] PCI/DPC: Unify and plumb error handling into DPC

[Sinan Kaya]

On 1/18/2018 1:00 PM, p...@codeaurora.org wrote:
>> I think you would put into include/linux/pci.h only if there is an external
>> use of constant outside of drivers/pci directory. Otherwise, you should keep
>> the setting inside one of the header files in drivers/pci directory.
>>
>> I don't see any other subsystem caring about DPC_FATAL definition.
> 
> ok so you are suggesting to move only DPC_FATAL ? so then AER can stay where 
> it is.

Now that both AER and DPC handling is getting unified, I think it makes sense to
keep all error codes (AER+DPC) together in drivers/pci/pci.h rather than having
them split in aer.h and dpc.h.

Otherwise, how would we avoid having a new error type defined with the existing 
values.



-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.

Re: [PATCH v5 3/4] PCI/DPC: Unify and plumb error handling into DPC

[Sinan Kaya]

On 1/18/2018 1:00 PM, p...@codeaurora.org wrote:
>> I think you would put into include/linux/pci.h only if there is an external
>> use of constant outside of drivers/pci directory. Otherwise, you should keep
>> the setting inside one of the header files in drivers/pci directory.
>>
>> I don't see any other subsystem caring about DPC_FATAL definition.
> 
> ok so you are suggesting to move only DPC_FATAL ? so then AER can stay where 
> it is.

Now that both AER and DPC handling is getting unified, I think it makes sense to
keep all error codes (AER+DPC) together in drivers/pci/pci.h rather than having
them split in aer.h and dpc.h.

Otherwise, how would we avoid having a new error type defined with the existing 
values.



-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.

Re: [PATCH v5 3/4] PCI/DPC: Unify and plumb error handling into DPC

[poza]

On 2018-01-18 22:01, Sinan Kaya wrote:

On 1/18/2018 12:57 AM, p...@codeaurora.org wrote:

On 2018-01-18 10:47, p...@codeaurora.org wrote:

On 2018-01-17 22:16, Sinan Kaya wrote:

On 1/17/2018 5:37 AM, Oza Pawandeep wrote:

+++ b/include/linux/dpc.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _DPC_H_
+#define _DPC_H_
+
+#define DPC_FATAL    4
+
+#endif //_DPC_H_
+


can you keep this in drivers/pci.h and get rid of this file?


I thought about this, but if I keep it in drivers/pci.h,
then AER's defines have to be in that as well. (for unification)

and then all the dependent files who are using AER_FATAL such as
drivers/acpi/apei/ghees.c
have to go on including this drivers file which is odd way of doing 
it.


So I am not very sure about thissince AER_FATAL are in aer.h, I
have made dpc.h


Regards,
Oza.


Should I be doing in next patch-set series ?



I think you would put into include/linux/pci.h only if there is an 
external
use of constant outside of drivers/pci directory. Otherwise, you should 
keep

the setting inside one of the header files in drivers/pci directory.

I don't see any other subsystem caring about DPC_FATAL definition.


ok so you are suggesting to move only DPC_FATAL ? so then AER can stay 
where it is.

Re: [PATCH v5 3/4] PCI/DPC: Unify and plumb error handling into DPC

[poza]

On 2018-01-18 22:01, Sinan Kaya wrote:

On 1/18/2018 12:57 AM, p...@codeaurora.org wrote:

On 2018-01-18 10:47, p...@codeaurora.org wrote:

On 2018-01-17 22:16, Sinan Kaya wrote:

On 1/17/2018 5:37 AM, Oza Pawandeep wrote:

+++ b/include/linux/dpc.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _DPC_H_
+#define _DPC_H_
+
+#define DPC_FATAL    4
+
+#endif //_DPC_H_
+


can you keep this in drivers/pci.h and get rid of this file?


I thought about this, but if I keep it in drivers/pci.h,
then AER's defines have to be in that as well. (for unification)

and then all the dependent files who are using AER_FATAL such as
drivers/acpi/apei/ghees.c
have to go on including this drivers file which is odd way of doing 
it.


So I am not very sure about thissince AER_FATAL are in aer.h, I
have made dpc.h


Regards,
Oza.


Should I be doing in next patch-set series ?



I think you would put into include/linux/pci.h only if there is an 
external
use of constant outside of drivers/pci directory. Otherwise, you should 
keep

the setting inside one of the header files in drivers/pci directory.

I don't see any other subsystem caring about DPC_FATAL definition.


ok so you are suggesting to move only DPC_FATAL ? so then AER can stay 
where it is.

Re: [PATCH v5 3/4] PCI/DPC: Unify and plumb error handling into DPC

[Sinan Kaya]

On 1/18/2018 12:57 AM, p...@codeaurora.org wrote:
> On 2018-01-18 10:47, p...@codeaurora.org wrote:
>> On 2018-01-17 22:16, Sinan Kaya wrote:
>>> On 1/17/2018 5:37 AM, Oza Pawandeep wrote:
 +++ b/include/linux/dpc.h
 @@ -0,0 +1,9 @@
 +/* SPDX-License-Identifier: GPL-2.0 */
 +
 +#ifndef _DPC_H_
 +#define _DPC_H_
 +
 +#define DPC_FATAL    4
 +
 +#endif //_DPC_H_
 +
>>>
>>> can you keep this in drivers/pci.h and get rid of this file?
>>
>> I thought about this, but if I keep it in drivers/pci.h,
>> then AER's defines have to be in that as well. (for unification)
>>
>> and then all the dependent files who are using AER_FATAL such as
>> drivers/acpi/apei/ghees.c
>> have to go on including this drivers file which is odd way of doing it.
>>
>> So I am not very sure about thissince AER_FATAL are in aer.h, I
>> have made dpc.h
>>
>>
>> Regards,
>> Oza.
> 
> Should I be doing in next patch-set series ?
> 

I think you would put into include/linux/pci.h only if there is an external
use of constant outside of drivers/pci directory. Otherwise, you should keep
the setting inside one of the header files in drivers/pci directory.

I don't see any other subsystem caring about DPC_FATAL definition.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.

Re: [PATCH v5 3/4] PCI/DPC: Unify and plumb error handling into DPC

[Sinan Kaya]

On 1/18/2018 12:57 AM, p...@codeaurora.org wrote:
> On 2018-01-18 10:47, p...@codeaurora.org wrote:
>> On 2018-01-17 22:16, Sinan Kaya wrote:
>>> On 1/17/2018 5:37 AM, Oza Pawandeep wrote:
 +++ b/include/linux/dpc.h
 @@ -0,0 +1,9 @@
 +/* SPDX-License-Identifier: GPL-2.0 */
 +
 +#ifndef _DPC_H_
 +#define _DPC_H_
 +
 +#define DPC_FATAL    4
 +
 +#endif //_DPC_H_
 +
>>>
>>> can you keep this in drivers/pci.h and get rid of this file?
>>
>> I thought about this, but if I keep it in drivers/pci.h,
>> then AER's defines have to be in that as well. (for unification)
>>
>> and then all the dependent files who are using AER_FATAL such as
>> drivers/acpi/apei/ghees.c
>> have to go on including this drivers file which is odd way of doing it.
>>
>> So I am not very sure about thissince AER_FATAL are in aer.h, I
>> have made dpc.h
>>
>>
>> Regards,
>> Oza.
> 
> Should I be doing in next patch-set series ?
> 

I think you would put into include/linux/pci.h only if there is an external
use of constant outside of drivers/pci directory. Otherwise, you should keep
the setting inside one of the header files in drivers/pci directory.

I don't see any other subsystem caring about DPC_FATAL definition.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.

Re: [PATCH v5 3/4] PCI/DPC: Unify and plumb error handling into DPC

[poza]

On 2018-01-18 10:52, p...@codeaurora.org wrote:

On 2018-01-17 22:15, Sinan Kaya wrote:

On 1/17/2018 5:37 AM, Oza Pawandeep wrote:

+   driver = pci_find_dpc_service(udev);
+#endif
 #if IS_ENABLED(CONFIG_PCIEAER)
-   /* Use the aer driver of the component firstly */
-   driver = pci_find_aer_service(udev);


I think we need a pci_find_service function that unifies these two.


Right now, find_xxx_service are in their respective file and exporting 
it.

which makes sense no less than having generic function.

If I have to change pci_find_service(, int service_name) then it
has to be somewhere in generic file.
probably portdrv_core.c

either way I am fine but just thinking out if its really required.

Regards,
Oza.


Should I be doing in next patch-set series ?

Regards,
Oza.

Re: [PATCH v5 3/4] PCI/DPC: Unify and plumb error handling into DPC

[poza]

On 2018-01-18 10:52, p...@codeaurora.org wrote:

On 2018-01-17 22:15, Sinan Kaya wrote:

On 1/17/2018 5:37 AM, Oza Pawandeep wrote:

+   driver = pci_find_dpc_service(udev);
+#endif
 #if IS_ENABLED(CONFIG_PCIEAER)
-   /* Use the aer driver of the component firstly */
-   driver = pci_find_aer_service(udev);


I think we need a pci_find_service function that unifies these two.


Right now, find_xxx_service are in their respective file and exporting 
it.

which makes sense no less than having generic function.

If I have to change pci_find_service(, int service_name) then it
has to be somewhere in generic file.
probably portdrv_core.c

either way I am fine but just thinking out if its really required.

Regards,
Oza.


Should I be doing in next patch-set series ?

Regards,
Oza.

Re: [PATCH v5 3/4] PCI/DPC: Unify and plumb error handling into DPC

[poza]

On 2018-01-18 10:47, p...@codeaurora.org wrote:

On 2018-01-17 22:16, Sinan Kaya wrote:

On 1/17/2018 5:37 AM, Oza Pawandeep wrote:

+++ b/include/linux/dpc.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _DPC_H_
+#define _DPC_H_
+
+#define DPC_FATAL  4
+
+#endif //_DPC_H_
+


can you keep this in drivers/pci.h and get rid of this file?


I thought about this, but if I keep it in drivers/pci.h,
then AER's defines have to be in that as well. (for unification)

and then all the dependent files who are using AER_FATAL such as
drivers/acpi/apei/ghees.c
have to go on including this drivers file which is odd way of doing it.

So I am not very sure about thissince AER_FATAL are in aer.h, I
have made dpc.h


Regards,
Oza.


Should I be doing in next patch-set series ?

Re: [PATCH v5 3/4] PCI/DPC: Unify and plumb error handling into DPC

[poza]

On 2018-01-18 10:47, p...@codeaurora.org wrote:

On 2018-01-17 22:16, Sinan Kaya wrote:

On 1/17/2018 5:37 AM, Oza Pawandeep wrote:

+++ b/include/linux/dpc.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _DPC_H_
+#define _DPC_H_
+
+#define DPC_FATAL  4
+
+#endif //_DPC_H_
+


can you keep this in drivers/pci.h and get rid of this file?


I thought about this, but if I keep it in drivers/pci.h,
then AER's defines have to be in that as well. (for unification)

and then all the dependent files who are using AER_FATAL such as
drivers/acpi/apei/ghees.c
have to go on including this drivers file which is odd way of doing it.

So I am not very sure about thissince AER_FATAL are in aer.h, I
have made dpc.h


Regards,
Oza.


Should I be doing in next patch-set series ?

Re: [PATCH v5 3/4] PCI/DPC: Unify and plumb error handling into DPC

[poza]

On 2018-01-17 22:15, Sinan Kaya wrote:

On 1/17/2018 5:37 AM, Oza Pawandeep wrote:

+   driver = pci_find_dpc_service(udev);
+#endif
 #if IS_ENABLED(CONFIG_PCIEAER)
-   /* Use the aer driver of the component firstly */
-   driver = pci_find_aer_service(udev);


I think we need a pci_find_service function that unifies these two.


Right now, find_xxx_service are in their respective file and exporting 
it.

which makes sense no less than having generic function.

If I have to change pci_find_service(, int service_name) then it has 
to be somewhere in generic file.

probably portdrv_core.c

either way I am fine but just thinking out if its really required.

Regards,
Oza.

Re: [PATCH v5 3/4] PCI/DPC: Unify and plumb error handling into DPC

[poza]

On 2018-01-17 22:15, Sinan Kaya wrote:

On 1/17/2018 5:37 AM, Oza Pawandeep wrote:

+   driver = pci_find_dpc_service(udev);
+#endif
 #if IS_ENABLED(CONFIG_PCIEAER)
-   /* Use the aer driver of the component firstly */
-   driver = pci_find_aer_service(udev);


I think we need a pci_find_service function that unifies these two.


Right now, find_xxx_service are in their respective file and exporting 
it.

which makes sense no less than having generic function.

If I have to change pci_find_service(, int service_name) then it has 
to be somewhere in generic file.

probably portdrv_core.c

either way I am fine but just thinking out if its really required.

Regards,
Oza.

Re: [PATCH v5 3/4] PCI/DPC: Unify and plumb error handling into DPC

[poza]

On 2018-01-17 22:16, Sinan Kaya wrote:

On 1/17/2018 5:37 AM, Oza Pawandeep wrote:

+++ b/include/linux/dpc.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _DPC_H_
+#define _DPC_H_
+
+#define DPC_FATAL  4
+
+#endif //_DPC_H_
+


can you keep this in drivers/pci.h and get rid of this file?


I thought about this, but if I keep it in drivers/pci.h,
then AER's defines have to be in that as well. (for unification)

and then all the dependent files who are using AER_FATAL such as 
drivers/acpi/apei/ghees.c

have to go on including this drivers file which is odd way of doing it.

So I am not very sure about thissince AER_FATAL are in aer.h, I have 
made dpc.h



Regards,
Oza.

Re: [PATCH v5 3/4] PCI/DPC: Unify and plumb error handling into DPC

[poza]

On 2018-01-17 22:16, Sinan Kaya wrote:

On 1/17/2018 5:37 AM, Oza Pawandeep wrote:

+++ b/include/linux/dpc.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _DPC_H_
+#define _DPC_H_
+
+#define DPC_FATAL  4
+
+#endif //_DPC_H_
+


can you keep this in drivers/pci.h and get rid of this file?


I thought about this, but if I keep it in drivers/pci.h,
then AER's defines have to be in that as well. (for unification)

and then all the dependent files who are using AER_FATAL such as 
drivers/acpi/apei/ghees.c

have to go on including this drivers file which is odd way of doing it.

So I am not very sure about thissince AER_FATAL are in aer.h, I have 
made dpc.h



Regards,
Oza.

Re: [PATCH v5 3/4] PCI/DPC: Unify and plumb error handling into DPC

[Sinan Kaya]

On 1/17/2018 5:37 AM, Oza Pawandeep wrote:
> +++ b/include/linux/dpc.h
> @@ -0,0 +1,9 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef _DPC_H_
> +#define _DPC_H_
> +
> +#define DPC_FATAL4
> +
> +#endif //_DPC_H_
> +

can you keep this in drivers/pci.h and get rid of this file?

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.

Re: [PATCH v5 3/4] PCI/DPC: Unify and plumb error handling into DPC

[Sinan Kaya]

On 1/17/2018 5:37 AM, Oza Pawandeep wrote:
> +++ b/include/linux/dpc.h
> @@ -0,0 +1,9 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef _DPC_H_
> +#define _DPC_H_
> +
> +#define DPC_FATAL4
> +
> +#endif //_DPC_H_
> +

can you keep this in drivers/pci.h and get rid of this file?

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.

Re: [PATCH v5 3/4] PCI/DPC: Unify and plumb error handling into DPC

[Sinan Kaya]

On 1/17/2018 5:37 AM, Oza Pawandeep wrote:
> + driver = pci_find_dpc_service(udev);
> +#endif
>  #if IS_ENABLED(CONFIG_PCIEAER)
> - /* Use the aer driver of the component firstly */
> - driver = pci_find_aer_service(udev);

I think we need a pci_find_service function that unifies these two.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.

Re: [PATCH v5 3/4] PCI/DPC: Unify and plumb error handling into DPC

[Sinan Kaya]

On 1/17/2018 5:37 AM, Oza Pawandeep wrote:
> + driver = pci_find_dpc_service(udev);
> +#endif
>  #if IS_ENABLED(CONFIG_PCIEAER)
> - /* Use the aer driver of the component firstly */
> - driver = pci_find_aer_service(udev);

I think we need a pci_find_service function that unifies these two.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.

[PATCH v5 3/4] PCI/DPC: Unify and plumb error handling into DPC

[Oza Pawandeep]

Current DPC driver does not do recovery, e.g. calling end-point's driver's
callbacks, which sanitize the sw.

DPC driver implements link_reset callback, and calls pci_do_recovery.

Signed-off-by: Oza Pawandeep 

diff --git a/drivers/pci/pcie/pcie-dpc.c b/drivers/pci/pcie/pcie-dpc.c
index 2d976a6..ed916765 100644
--- a/drivers/pci/pcie/pcie-dpc.c
+++ b/drivers/pci/pcie/pcie-dpc.c
@@ -13,8 +13,12 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include "../pci.h"
+#include "portdrv.h"
+
+static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev);
 
 struct rp_pio_header_log_regs {
u32 dw0;
@@ -67,6 +71,60 @@ struct dpc_dev {
"Memory Request Completion Timeout", /* Bit Position 18 */
 };
 
+static int find_dpc_dev_iter(struct device *device, void *data)
+{
+   struct pcie_port_service_driver *service_driver;
+   struct device **dev;
+
+   dev = (struct device **) data;
+
+   if (device->bus == _port_bus_type && device->driver) {
+   service_driver = to_service_driver(device->driver);
+   if (service_driver->service == PCIE_PORT_SERVICE_DPC) {
+   *dev = device;
+   return 1;
+   }
+   }
+
+   return 0;
+}
+
+static struct device *pci_find_dpc_dev(struct pci_dev *pdev)
+{
+   struct device *dev = NULL;
+
+   device_for_each_child(>dev, , find_dpc_dev_iter);
+
+   return dev;
+}
+
+static int find_dpc_service_iter(struct device *device, void *data)
+{
+   struct pcie_port_service_driver *service_driver, **drv;
+
+   drv = (struct pcie_port_service_driver **) data;
+
+   if (device->bus == _port_bus_type && device->driver) {
+   service_driver = to_service_driver(device->driver);
+   if (service_driver->service == PCIE_PORT_SERVICE_DPC) {
+   *drv = service_driver;
+   return 1;
+   }
+   }
+
+   return 0;
+}
+
+struct pcie_port_service_driver *pci_find_dpc_service(struct pci_dev *dev)
+{
+   struct pcie_port_service_driver *drv = NULL;
+
+   device_for_each_child(>dev, , find_dpc_service_iter);
+
+   return drv;
+}
+EXPORT_SYMBOL(pci_find_dpc_service);
+
 static int dpc_wait_rp_inactive(struct dpc_dev *dpc)
 {
unsigned long timeout = jiffies + HZ;
@@ -104,11 +162,23 @@ static void dpc_wait_link_inactive(struct dpc_dev *dpc)
dev_warn(dev, "Link state not disabled for DPC event\n");
 }
 
-static void interrupt_event_handler(struct work_struct *work)
+/**
+ * dpc_reset_link - reset link DPC  routine
+ * @dev: pointer to Root Port's pci_dev data structure
+ *
+ * Invoked by Port Bus driver when performing link reset at Root Port.
+ */
+static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
 {
-   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;
+   struct pci_dev *dev, *temp;
+   struct dpc_dev *dpc;
+   struct pcie_device *pciedev;
+   struct device *devdpc;
+
+   devdpc = pci_find_dpc_dev(pdev);
+   pciedev = to_pcie_device(devdpc);
+   dpc = get_service_data(pciedev);
 
pci_lock_rescan_remove();
list_for_each_entry_safe_reverse(dev, temp, >devices,
@@ -125,7 +195,7 @@ static void interrupt_event_handler(struct work_struct 
*work)
 
dpc_wait_link_inactive(dpc);
if (dpc->rp && dpc_wait_rp_inactive(dpc))
-   return;
+   return PCI_ERS_RESULT_DISCONNECT;
if (dpc->rp && dpc->rp_pio_status) {
pci_write_config_dword(pdev,
  dpc->cap_pos + PCI_EXP_DPC_RP_PIO_STATUS,
@@ -135,6 +205,17 @@ static void interrupt_event_handler(struct work_struct 
*work)
 
pci_write_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_STATUS,
PCI_EXP_DPC_STATUS_TRIGGER | PCI_EXP_DPC_STATUS_INTERRUPT);
+
+   return PCI_ERS_RESULT_RECOVERED;
+}
+
+static void interrupt_event_handler(struct work_struct *work)
+{
+   struct dpc_dev *dpc = container_of(work, struct dpc_dev, work);
+   struct pci_dev *pdev = dpc->dev->port;
+
+   /* From DPC point of view error is always FATAL. */
+   pci_do_recovery(pdev, DPC_FATAL);
 }
 
 static void dpc_rp_pio_print_tlp_header(struct device *dev,
@@ -339,6 +420,7 @@ static void dpc_remove(struct pcie_device *dev)
.service= PCIE_PORT_SERVICE_DPC,
.probe  = dpc_probe,
.remove = dpc_remove,
+   .reset_link = dpc_reset_link,
 };
 
 static int __init dpc_service_init(void)
diff --git a/drivers/pci/pcie/pcie-err.c b/drivers/pci/pcie/pcie-err.c
index a532fe0..8ce1de1 100644
--- a/drivers/pci/pcie/pcie-err.c
+++ b/drivers/pci/pcie/pcie-err.c
@@ -17,9 +17,12 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include "portdrv.h"
 
+static 

[PATCH v5 3/4] PCI/DPC: Unify and plumb error handling into DPC

[Oza Pawandeep]

Current DPC driver does not do recovery, e.g. calling end-point's driver's
callbacks, which sanitize the sw.

DPC driver implements link_reset callback, and calls pci_do_recovery.

Signed-off-by: Oza Pawandeep 

diff --git a/drivers/pci/pcie/pcie-dpc.c b/drivers/pci/pcie/pcie-dpc.c
index 2d976a6..ed916765 100644
--- a/drivers/pci/pcie/pcie-dpc.c
+++ b/drivers/pci/pcie/pcie-dpc.c
@@ -13,8 +13,12 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include "../pci.h"
+#include "portdrv.h"
+
+static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev);
 
 struct rp_pio_header_log_regs {
u32 dw0;
@@ -67,6 +71,60 @@ struct dpc_dev {
"Memory Request Completion Timeout", /* Bit Position 18 */
 };
 
+static int find_dpc_dev_iter(struct device *device, void *data)
+{
+   struct pcie_port_service_driver *service_driver;
+   struct device **dev;
+
+   dev = (struct device **) data;
+
+   if (device->bus == _port_bus_type && device->driver) {
+   service_driver = to_service_driver(device->driver);
+   if (service_driver->service == PCIE_PORT_SERVICE_DPC) {
+   *dev = device;
+   return 1;
+   }
+   }
+
+   return 0;
+}
+
+static struct device *pci_find_dpc_dev(struct pci_dev *pdev)
+{
+   struct device *dev = NULL;
+
+   device_for_each_child(>dev, , find_dpc_dev_iter);
+
+   return dev;
+}
+
+static int find_dpc_service_iter(struct device *device, void *data)
+{
+   struct pcie_port_service_driver *service_driver, **drv;
+
+   drv = (struct pcie_port_service_driver **) data;
+
+   if (device->bus == _port_bus_type && device->driver) {
+   service_driver = to_service_driver(device->driver);
+   if (service_driver->service == PCIE_PORT_SERVICE_DPC) {
+   *drv = service_driver;
+   return 1;
+   }
+   }
+
+   return 0;
+}
+
+struct pcie_port_service_driver *pci_find_dpc_service(struct pci_dev *dev)
+{
+   struct pcie_port_service_driver *drv = NULL;
+
+   device_for_each_child(>dev, , find_dpc_service_iter);
+
+   return drv;
+}
+EXPORT_SYMBOL(pci_find_dpc_service);
+
 static int dpc_wait_rp_inactive(struct dpc_dev *dpc)
 {
unsigned long timeout = jiffies + HZ;
@@ -104,11 +162,23 @@ static void dpc_wait_link_inactive(struct dpc_dev *dpc)
dev_warn(dev, "Link state not disabled for DPC event\n");
 }
 
-static void interrupt_event_handler(struct work_struct *work)
+/**
+ * dpc_reset_link - reset link DPC  routine
+ * @dev: pointer to Root Port's pci_dev data structure
+ *
+ * Invoked by Port Bus driver when performing link reset at Root Port.
+ */
+static pci_ers_result_t dpc_reset_link(struct pci_dev *pdev)
 {
-   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;
+   struct pci_dev *dev, *temp;
+   struct dpc_dev *dpc;
+   struct pcie_device *pciedev;
+   struct device *devdpc;
+
+   devdpc = pci_find_dpc_dev(pdev);
+   pciedev = to_pcie_device(devdpc);
+   dpc = get_service_data(pciedev);
 
pci_lock_rescan_remove();
list_for_each_entry_safe_reverse(dev, temp, >devices,
@@ -125,7 +195,7 @@ static void interrupt_event_handler(struct work_struct 
*work)
 
dpc_wait_link_inactive(dpc);
if (dpc->rp && dpc_wait_rp_inactive(dpc))
-   return;
+   return PCI_ERS_RESULT_DISCONNECT;
if (dpc->rp && dpc->rp_pio_status) {
pci_write_config_dword(pdev,
  dpc->cap_pos + PCI_EXP_DPC_RP_PIO_STATUS,
@@ -135,6 +205,17 @@ static void interrupt_event_handler(struct work_struct 
*work)
 
pci_write_config_word(pdev, dpc->cap_pos + PCI_EXP_DPC_STATUS,
PCI_EXP_DPC_STATUS_TRIGGER | PCI_EXP_DPC_STATUS_INTERRUPT);
+
+   return PCI_ERS_RESULT_RECOVERED;
+}
+
+static void interrupt_event_handler(struct work_struct *work)
+{
+   struct dpc_dev *dpc = container_of(work, struct dpc_dev, work);
+   struct pci_dev *pdev = dpc->dev->port;
+
+   /* From DPC point of view error is always FATAL. */
+   pci_do_recovery(pdev, DPC_FATAL);
 }
 
 static void dpc_rp_pio_print_tlp_header(struct device *dev,
@@ -339,6 +420,7 @@ static void dpc_remove(struct pcie_device *dev)
.service= PCIE_PORT_SERVICE_DPC,
.probe  = dpc_probe,
.remove = dpc_remove,
+   .reset_link = dpc_reset_link,
 };
 
 static int __init dpc_service_init(void)
diff --git a/drivers/pci/pcie/pcie-err.c b/drivers/pci/pcie/pcie-err.c
index a532fe0..8ce1de1 100644
--- a/drivers/pci/pcie/pcie-err.c
+++ b/drivers/pci/pcie/pcie-err.c
@@ -17,9 +17,12 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include "portdrv.h"
 
+static