Re: [PATCH v3 00/11] sysctl: treewide: constify ctl_table argument of sysctl handlers

2024-04-25 Thread Thomas Weißschuh
Hi Joel,

On 2024-04-25 13:04:12+, Joel Granados wrote:
> On Wed, Apr 24, 2024 at 08:12:34PM -0700, Jakub Kicinski wrote:
> > On Tue, 23 Apr 2024 09:54:35 +0200 Thomas Weißschuh wrote:
> > > The series was split from my larger series sysctl-const series [0].
> > > It only focusses on the proc_handlers but is an important step to be
> > > able to move all static definitions of ctl_table into .rodata.
> > 
> > Split this per subsystem, please.
> It is tricky to do that because it changes the first argument (ctl*) to
> const in the proc_handler function type defined in sysclt.h:
> "
> -typedef int proc_handler(struct ctl_table *ctl, int write, void *buffer,
> +typedef int proc_handler(const struct ctl_table *ctl, int write, void 
> *buffer,
> size_t *lenp, loff_t *ppos);
> "
> This means that all the proc_handlers need to change at the same time.
> 
> However, there is an alternative way to do this that allows chunking. We
> first define the proc_handler as a void pointer (casting it where it is
> being used) [1]. Then we could do the constification by subsystem (like
> Jakub proposes). Finally we can "revert the void pointer change so we
> don't have one size fit all pointer as our proc_handler [2].
> 
> Here are some comments about the alternative:
> 1. We would need to make the first argument const in all the derived
>proc_handlers [3] 
> 2. There would be no undefined behavior for two reasons:
>2.1. There is no case where we change the first argument. We know
> this because there are no compile errors after we make it const.
>2.2. We would always go from non-const to const. This is the case
> because all the stuff that is unchanged in non-const.
> 3. If the idea sticks, it should go into mainline as one patchset. I
>would not like to have a void* proc_handler in a kernel release.
> 4. I think this is a "win/win" solution were the constification goes
>through and it is divided in such a way that it is reviewable.
> 
> I would really like to hear what ppl think about this "heretic"
> alternative. @Thomas, @Luis, @Kees @Jakub?

Thanks for that alternative, I'm not a big fan though.

Besides the wonky syntax, Control Flow Integrity should trap on
this construct. Functions are called through different pointers than
their actual types which is exactly what CFI is meant to prevent.

Maybe people find it easier to review when using
"--word-diff" and/or "-U0" with git diff/show.
There is really nothing going an besides adding a few "const"s.

But if the consensus prefers this solution, I'll be happy to adopt it.

> [1] 
> https://git.kernel.org/pub/scm/linux/kernel/git/joel.granados/linux.git/commit/?h=jag/constfy_treewide_alternative=4a383503b1ea650d4e12c1f5838974e879f5aa6f
> [2] 
> https://git.kernel.org/pub/scm/linux/kernel/git/joel.granados/linux.git/commit/?h=jag/constfy_treewide_alternative=a3be65973d27ec2933b9e81e1bec60be3a9b460d
> [3] proc_dostring, proc_dobool, proc_dointvec


Thomas


Re: [PATCH v13 25/35] KVM: selftests: Convert lib's mem regions to KVM_SET_USER_MEMORY_REGION2

2024-04-25 Thread Shuah Khan

On 4/25/24 09:09, Sean Christopherson wrote:

On Thu, Apr 25, 2024, Shuah Khan wrote:

On 4/25/24 08:12, Dan Carpenter wrote:

On Fri, Oct 27, 2023 at 11:22:07AM -0700, Sean Christopherson wrote:

Use KVM_SET_USER_MEMORY_REGION2 throughout KVM's selftests library so that
support for guest private memory can be added without needing an entirely
separate set of helpers.

Note, this obviously makes selftests backwards-incompatible with older KVM

^^

versions from this point forward.

^

Is there a way we could disable the tests on older kernels instead of
making them fail?  Check uname or something?  There is probably a
standard way to do this...  It's these tests which fail.


They shouldn't fail - the tests should be skipped on older kernels.


Ah, that makes sense.  Except for a few outliers that aren't all that 
interesting,
all KVM selftests create memslots, so I'm tempted to just make it a hard 
requirement
to spare us headache, e.g.

diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c 
b/tools/testing/selftests/kvm/lib/kvm_util.c
index b2262b5fad9e..4b2038b1f11f 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -2306,6 +2306,9 @@ void __attribute((constructor)) kvm_selftest_init(void)
 /* Tell stdout not to buffer its content. */
 setbuf(stdout, NULL);
  
+   __TEST_REQUIRE(kvm_has_cap(KVM_CAP_USER_MEMORY2),

+  "KVM selftests from v6.8+ require 
KVM_SET_USER_MEMORY_REGION2");
+
 kvm_selftest_arch_init();
  }

--

but it's also easy enough to be more precise and skip only those that actually
create memslots.


This is approach is what is recommended in kselfest document. Rubn as many tests
as possible and skip the ones that can't be run due to unmet dependencies.



diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c 
b/tools/testing/selftests/kvm/lib/kvm_util.c
index b2262b5fad9e..b21152adf448 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -944,6 +944,9 @@ int __vm_set_user_memory_region2(struct kvm_vm *vm, 
uint32_t slot, uint32_t flag
 .guest_memfd_offset = guest_memfd_offset,
 };
  
+   __TEST_REQUIRE(kvm_has_cap(KVM_CAP_USER_MEMORY2),

+  "KVM selftests from v6.8+ require 
KVM_SET_USER_MEMORY_REGION2");
+
 return ioctl(vm->fd, KVM_SET_USER_MEMORY_REGION2, );
  }
  
@@ -970,6 +973,9 @@ void vm_mem_add(struct kvm_vm *vm, enum vm_mem_backing_src_type src_type,

 size_t mem_size = npages * vm->page_size;
 size_t alignment;
  
+   __TEST_REQUIRE(kvm_has_cap(KVM_CAP_USER_MEMORY2),

+  "KVM selftests from v6.8+ require 
KVM_SET_USER_MEMORY_REGION2");
+
 TEST_ASSERT(vm_adjust_num_guest_pages(vm->mode, npages) == npages,
 "Number of guest pages is not compatible with the host. "
 "Try npages=%d", vm_adjust_num_guest_pages(vm->mode, npages));
--


thanks,
-- Shuah


Re: [PATCH v13 25/35] KVM: selftests: Convert lib's mem regions to KVM_SET_USER_MEMORY_REGION2

2024-04-25 Thread Sean Christopherson
On Thu, Apr 25, 2024, Shuah Khan wrote:
> On 4/25/24 08:12, Dan Carpenter wrote:
> > On Fri, Oct 27, 2023 at 11:22:07AM -0700, Sean Christopherson wrote:
> > > Use KVM_SET_USER_MEMORY_REGION2 throughout KVM's selftests library so that
> > > support for guest private memory can be added without needing an entirely
> > > separate set of helpers.
> > > 
> > > Note, this obviously makes selftests backwards-incompatible with older KVM
> >
> > ^^
> > > versions from this point forward.
> >^
> > 
> > Is there a way we could disable the tests on older kernels instead of
> > making them fail?  Check uname or something?  There is probably a
> > standard way to do this...  It's these tests which fail.
> 
> They shouldn't fail - the tests should be skipped on older kernels.

Ah, that makes sense.  Except for a few outliers that aren't all that 
interesting,
all KVM selftests create memslots, so I'm tempted to just make it a hard 
requirement
to spare us headache, e.g.

diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c 
b/tools/testing/selftests/kvm/lib/kvm_util.c
index b2262b5fad9e..4b2038b1f11f 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -2306,6 +2306,9 @@ void __attribute((constructor)) kvm_selftest_init(void)
/* Tell stdout not to buffer its content. */
setbuf(stdout, NULL);
 
+   __TEST_REQUIRE(kvm_has_cap(KVM_CAP_USER_MEMORY2),
+  "KVM selftests from v6.8+ require 
KVM_SET_USER_MEMORY_REGION2");
+
kvm_selftest_arch_init();
 }

--

but it's also easy enough to be more precise and skip only those that actually
create memslots.

diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c 
b/tools/testing/selftests/kvm/lib/kvm_util.c
index b2262b5fad9e..b21152adf448 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -944,6 +944,9 @@ int __vm_set_user_memory_region2(struct kvm_vm *vm, 
uint32_t slot, uint32_t flag
.guest_memfd_offset = guest_memfd_offset,
};
 
+   __TEST_REQUIRE(kvm_has_cap(KVM_CAP_USER_MEMORY2),
+  "KVM selftests from v6.8+ require 
KVM_SET_USER_MEMORY_REGION2");
+
return ioctl(vm->fd, KVM_SET_USER_MEMORY_REGION2, );
 }
 
@@ -970,6 +973,9 @@ void vm_mem_add(struct kvm_vm *vm, enum 
vm_mem_backing_src_type src_type,
size_t mem_size = npages * vm->page_size;
size_t alignment;
 
+   __TEST_REQUIRE(kvm_has_cap(KVM_CAP_USER_MEMORY2),
+  "KVM selftests from v6.8+ require 
KVM_SET_USER_MEMORY_REGION2");
+
TEST_ASSERT(vm_adjust_num_guest_pages(vm->mode, npages) == npages,
"Number of guest pages is not compatible with the host. "
"Try npages=%d", vm_adjust_num_guest_pages(vm->mode, npages));
--


Re: [PATCH v13 25/35] KVM: selftests: Convert lib's mem regions to KVM_SET_USER_MEMORY_REGION2

2024-04-25 Thread Shuah Khan

On 4/25/24 08:12, Dan Carpenter wrote:

On Fri, Oct 27, 2023 at 11:22:07AM -0700, Sean Christopherson wrote:

Use KVM_SET_USER_MEMORY_REGION2 throughout KVM's selftests library so that
support for guest private memory can be added without needing an entirely
separate set of helpers.

Note, this obviously makes selftests backwards-incompatible with older KVM

   ^^

versions from this point forward.

   ^

Is there a way we could disable the tests on older kernels instead of
making them fail?  Check uname or something?  There is probably a
standard way to do this...  It's these tests which fail.


They shouldn't fail - the tests should be skipped on older kernels.
If it is absolutely necessary to dd uname to check kernel version,
refer to zram/zram_lib.sh for an example.

thanks,
-- Shuah


Re: [PATCH v13 25/35] KVM: selftests: Convert lib's mem regions to KVM_SET_USER_MEMORY_REGION2

2024-04-25 Thread Dan Carpenter
On Fri, Oct 27, 2023 at 11:22:07AM -0700, Sean Christopherson wrote:
> Use KVM_SET_USER_MEMORY_REGION2 throughout KVM's selftests library so that
> support for guest private memory can be added without needing an entirely
> separate set of helpers.
> 
> Note, this obviously makes selftests backwards-incompatible with older KVM
  ^^
> versions from this point forward.
  ^

Is there a way we could disable the tests on older kernels instead of
making them fail?  Check uname or something?  There is probably a
standard way to do this...  It's these tests which fail.

 kvm_aarch32_id_regs
 kvm_access_tracking_perf_test
 kvm_arch_timer
 kvm_debug-exceptions
 kvm_demand_paging_test
 kvm_dirty_log_perf_test
 kvm_dirty_log_test
 kvm_guest_print_test
 kvm_hypercalls
 kvm_kvm_page_table_test
 kvm_memslot_modification_stress_test
 kvm_memslot_perf_test
 kvm_page_fault_test
 kvm_psci_test
 kvm_rseq_test
 kvm_smccc_filter
 kvm_steal_time
 kvm_vgic_init
 kvm_vgic_irq
 kvm_vpmu_counter_access

regards,
dan carpenter



Re: [PATCH v5 RESEND] arch/powerpc/kvm: Add support for reading VPA counters for pseries guests

2024-04-25 Thread Naveen N Rao
On Wed, Apr 24, 2024 at 11:08:38AM +0530, Gautam Menghani wrote:
> On Mon, Apr 22, 2024 at 09:15:02PM +0530, Naveen N Rao wrote:
> > On Tue, Apr 02, 2024 at 12:36:54PM +0530, Gautam Menghani wrote:
> > >  static int kvmhv_vcpu_entry_nestedv2(struct kvm_vcpu *vcpu, u64 
> > >  time_limit,
> > >unsigned long lpcr, u64 *tb)
> > >  {
> > > @@ -4130,6 +4161,11 @@ static int kvmhv_vcpu_entry_nestedv2(struct 
> > > kvm_vcpu *vcpu, u64 time_limit,
> > >   kvmppc_gse_put_u64(io->vcpu_run_input, KVMPPC_GSID_LPCR, lpcr);
> > >  
> > >   accumulate_time(vcpu, >arch.in_guest);
> > > +
> > > + /* Enable the guest host context switch time tracking */
> > > + if (unlikely(trace_kvmppc_vcpu_exit_cs_time_enabled()))
> > > + kvmhv_set_l2_accumul(1);
> > > +
> > >   rc = plpar_guest_run_vcpu(0, vcpu->kvm->arch.lpid, vcpu->vcpu_id,
> > > , );
> > >  
> > > @@ -4156,6 +4192,10 @@ static int kvmhv_vcpu_entry_nestedv2(struct 
> > > kvm_vcpu *vcpu, u64 time_limit,
> > >  
> > >   timer_rearm_host_dec(*tb);
> > >  
> > > + /* Record context switch and guest_run_time data */
> > > + if (kvmhv_get_l2_accumul())
> > > + do_trace_nested_cs_time(vcpu);
> > > +
> > >   return trap;
> > >  }
> > 
> > I'm assuming the counters in VPA are cumulative, since you are zero'ing 
> > them out on exit. If so, I think a better way to implement this is to 
> > use TRACE_EVENT_FN() and provide tracepoint registration and 
> > unregistration functions. You can then enable the counters once during 
> > registration and avoid repeated writes to the VPA area. With that, you 
> > also won't need to do anything before vcpu entry. If you maintain 
> > previous values, you can calculate the delta and emit the trace on vcpu 
> > exit. The values in VPA area can then serve as the cumulative values.
> > 
> 
> This approach will have a problem. The context switch times are reported
> in the L1 LPAR's CPU's VPA area. Consider the following scenario:
> 
> 1. L1 has 2 cpus, and L2 has 1 cpu
> 2. L2 runs on L1's cpu0 for a few seconds, and the counter values go to
> 1 million
> 3. We are maintaining a copy of values of VPA in separate variables, so
> those variables also have 1 million.
> 4. Now if L2's vcpu is migrated to another L1 cpu, that L1 cpu's VPA
> counters will start from 0, so if we try to get delta value, we will end
> up doing 0 - 1 million, which would be wrong.

I'm assuming you mean migrating the task. If we maintain the previous 
readings in paca, it should work I think.

> 
> The aggregation logic in this patch works as we zero out the VPA after
> every switch, and maintain aggregation in a vcpu->arch

Are the cumulative values of the VPA counters of no significance? We 
lose those with this approach. Not sure if we care.


- Naveen



Re: [PATCH v3 00/11] sysctl: treewide: constify ctl_table argument of sysctl handlers

2024-04-25 Thread Joel Granados
On Wed, Apr 24, 2024 at 08:12:34PM -0700, Jakub Kicinski wrote:
> On Tue, 23 Apr 2024 09:54:35 +0200 Thomas Weißschuh wrote:
> > The series was split from my larger series sysctl-const series [0].
> > It only focusses on the proc_handlers but is an important step to be
> > able to move all static definitions of ctl_table into .rodata.
> 
> Split this per subsystem, please.
It is tricky to do that because it changes the first argument (ctl*) to
const in the proc_handler function type defined in sysclt.h:
"
-typedef int proc_handler(struct ctl_table *ctl, int write, void *buffer,
+typedef int proc_handler(const struct ctl_table *ctl, int write, void *buffer,
size_t *lenp, loff_t *ppos);
"
This means that all the proc_handlers need to change at the same time.

However, there is an alternative way to do this that allows chunking. We
first define the proc_handler as a void pointer (casting it where it is
being used) [1]. Then we could do the constification by subsystem (like
Jakub proposes). Finally we can "revert the void pointer change so we
don't have one size fit all pointer as our proc_handler [2].

Here are some comments about the alternative:
1. We would need to make the first argument const in all the derived
   proc_handlers [3] 
2. There would be no undefined behavior for two reasons:
   2.1. There is no case where we change the first argument. We know
this because there are no compile errors after we make it const.
   2.2. We would always go from non-const to const. This is the case
because all the stuff that is unchanged in non-const.
3. If the idea sticks, it should go into mainline as one patchset. I
   would not like to have a void* proc_handler in a kernel release.
4. I think this is a "win/win" solution were the constification goes
   through and it is divided in such a way that it is reviewable.

I would really like to hear what ppl think about this "heretic"
alternative. @Thomas, @Luis, @Kees @Jakub?

Best

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/joel.granados/linux.git/commit/?h=jag/constfy_treewide_alternative=4a383503b1ea650d4e12c1f5838974e879f5aa6f
[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/joel.granados/linux.git/commit/?h=jag/constfy_treewide_alternative=a3be65973d27ec2933b9e81e1bec60be3a9b460d
[3] proc_dostring, proc_dobool, proc_dointvec

--

Joel Granados


signature.asc
Description: PGP signature


Re: [PATCH v8 2/3] PCI/AER: Disable AER service on suspend

2024-04-25 Thread Kai-Heng Feng
On Fri, Apr 19, 2024 at 4:35 AM Bjorn Helgaas  wrote:
>
> On Tue, Apr 16, 2024 at 12:32:24PM +0800, Kai-Heng Feng wrote:
> > When the power rail gets cut off, the hardware can create some electric
> > noise on the link that triggers AER. If IRQ is shared between AER with
> > PME, such AER noise will cause a spurious wakeup on system suspend.
> >
> > When the power rail gets back, the firmware of the device resets itself
> > and can create unexpected behavior like sending PTM messages. For this
> > case, the driver will always be too late to toggle off features should
> > be disabled.
> >
> > As Per PCIe Base Spec 5.0, section 5.2, titled "Link State Power
> > Management", TLP and DLLP transmission are disabled for a Link in L2/L3
> > Ready (D3hot), L2 (D3cold with aux power) and L3 (D3cold) states. So if
> > the power will be turned off during suspend process, disable AER service
> > and re-enable it during the resume process. This should not affect the
> > basic functionality.
> >
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=209149
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=216295
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=218090
> > Signed-off-by: Kai-Heng Feng 
>
> Thanks for reviving this series.  I tried follow the history about
> this, but there are at least two series that were very similar and I
> can't put it all together.
>
> > ---
> > v8:
> >  - Add more bug reports.
> >
> > v7:
> >  - Wording
> >  - Disable AER completely (again) if power will be turned off
> >
> > v6:
> > v5:
> >  - Wording.
> >
> > v4:
> > v3:
> >  - No change.
> >
> > v2:
> >  - Only disable AER IRQ.
> >  - No more check on PME IRQ#.
> >  - Use helper.
> >
> >  drivers/pci/pcie/aer.c | 25 +
> >  1 file changed, 25 insertions(+)
> >
> > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> > index ac6293c24976..bea7818c2d1b 100644
> > --- a/drivers/pci/pcie/aer.c
> > +++ b/drivers/pci/pcie/aer.c
> > @@ -28,6 +28,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -1497,6 +1498,28 @@ static int aer_probe(struct pcie_device *dev)
> >   return 0;
> >  }
> >
> > +static int aer_suspend(struct pcie_device *dev)
> > +{
> > + struct aer_rpc *rpc = get_service_data(dev);
> > + struct pci_dev *pdev = rpc->rpd;
> > +
> > + if (pci_ancestor_pr3_present(pdev) || pm_suspend_via_firmware())
> > + aer_disable_rootport(rpc);
>
> Why do we check pci_ancestor_pr3_present(pdev) and
> pm_suspend_via_firmware()?  I'm getting pretty convinced that we need
> to disable AER interrupts on suspend in general.  I think it will be
> better if we do that consistently on all platforms, not special cases
> based on details of how we suspend.

Sure. Will change in next revision.

>
> Also, why do we use aer_disable_rootport() instead of just
> aer_disable_irq()?  I think it's the interrupt that causes issues on
> suspend.  I see that there *were* some versions that used
> aer_disable_irq(), but I can't find the reason it changed.

Interrupt can cause system wakeup, if it's shared with PME.

The reason why aer_disable_rootport() is used over aer_disable_irq()
is that when the latter is used the error still gets logged during
sleep cycle. Once the pcieport driver resumes, it invokes
aer_root_reset() to reset the hierarchy, while the hierarchy hasn't
resumed yet.

So use aer_disable_rootport() to prevent such issue from happening.

Kai-Heng

>
> > +
> > + return 0;
> > +}
> > +
> > +static int aer_resume(struct pcie_device *dev)
> > +{
> > + struct aer_rpc *rpc = get_service_data(dev);
> > + struct pci_dev *pdev = rpc->rpd;
> > +
> > + if (pci_ancestor_pr3_present(pdev) || pm_resume_via_firmware())
> > + aer_enable_rootport(rpc);
> > +
> > + return 0;
> > +}
> > +
> >  /**
> >   * aer_root_reset - reset Root Port hierarchy, RCEC, or RCiEP
> >   * @dev: pointer to Root Port, RCEC, or RCiEP
> > @@ -1561,6 +1584,8 @@ static struct pcie_port_service_driver aerdriver = {
> >   .service= PCIE_PORT_SERVICE_AER,
> >
> >   .probe  = aer_probe,
> > + .suspend= aer_suspend,
> > + .resume = aer_resume,
> >   .remove = aer_remove,
> >  };
> >
> > --
> > 2.34.1
> >


Re: [PATCH v3 00/11] sysctl: treewide: constify ctl_table argument of sysctl handlers

2024-04-25 Thread Thomas Weißschuh
On 2024-04-24 20:12:34+, Jakub Kicinski wrote:
> On Tue, 23 Apr 2024 09:54:35 +0200 Thomas Weißschuh wrote:
> > The series was split from my larger series sysctl-const series [0].
> > It only focusses on the proc_handlers but is an important step to be
> > able to move all static definitions of ctl_table into .rodata.
> 
> Split this per subsystem, please.

Unfortunately this would introduce an enormous amount of code churn.

The function prototypes for each callback have to stay consistent.
So a another callback member ("proc_handler_new") is needed and users
would be migrated to it gradually.

But then *all* definitions of "struct ctl_table" throughout the tree need to
be touched.
In contrast, the proposed series only needs to change the handler
implementations, not their usage sites.

There are many, many more usage sites than handler implementations.

Especially, as the majority of sysctl tables use the standard handlers
(proc_dostring, proc_dobool, ...) and are not affected by the proposed
aproach at all.

And then we would have introduced a new handler name "proc_handler_new"
and maybe have to do the whole thing again to rename it back to
the original and well-known "proc_handler".


Of course if somebody has a better aproach, I'm all ears.


Thomas


Re: [PATCH v8 1/3] PCI: Add helper to check if any of ancestor device support D3cold

2024-04-25 Thread Kai-Heng Feng
On Thu, Apr 18, 2024 at 9:15 AM Kuppuswamy Sathyanarayanan
 wrote:
>
>
> On 4/15/24 9:32 PM, Kai-Heng Feng wrote:
> > In addition to nearest upstream bridge, driver may want to know if the
> > entire hierarchy can be powered off to perform different action.
> >
> > So walk higher up the hierarchy to find out if any device has valid
> > _PR3.
> >
> > The user will be introduced in next patch.
> >
> > Signed-off-by: Kai-Heng Feng 
> > ---
>
> Since it has been a while, I was not sure what this series is about.
>
> IMO, it is better to include a cover letter with the summary of your
> changes.

OK, will do in next revision.

>
>
> > v8:
> >  - No change.
> >
> >  drivers/pci/pci.c   | 16 
> >  include/linux/pci.h |  2 ++
> >  2 files changed, 18 insertions(+)
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index e5f243dd4288..7a5662f116b8 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -6225,6 +6225,22 @@ bool pci_pr3_present(struct pci_dev *pdev)
> >   acpi_has_method(adev->handle, "_PR3");
> >  }
> >  EXPORT_SYMBOL_GPL(pci_pr3_present);
> > +
> > +bool pci_ancestor_pr3_present(struct pci_dev *pdev)
> > +{
> > + struct pci_dev *parent = pdev;
> > +
> > + if (acpi_disabled)
> > + return false;
> > +
> > + while ((parent = pci_upstream_bridge(parent))) {
> > + if (pci_pr3_present(pdev))
>
> I think it should be "parent" here?

Thanks for catching this.

But this patch will be dropped in next version for better simplicity.

Kai-Heng

>
> > + return true;
> > + }
> > +
> > + return false;
> > +}
> > +EXPORT_SYMBOL_GPL(pci_ancestor_pr3_present);
> >  #endif
> >
> >  /**
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 16493426a04f..cd71ebfd0f89 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -2620,10 +2620,12 @@ struct irq_domain 
> > *pci_host_bridge_acpi_msi_domain(struct pci_bus *bus);
> >  void
> >  pci_msi_register_fwnode_provider(struct fwnode_handle *(*fn)(struct device 
> > *));
> >  bool pci_pr3_present(struct pci_dev *pdev);
> > +bool pci_ancestor_pr3_present(struct pci_dev *pdev);
> >  #else
> >  static inline struct irq_domain *
> >  pci_host_bridge_acpi_msi_domain(struct pci_bus *bus) { return NULL; }
> >  static inline bool pci_pr3_present(struct pci_dev *pdev) { return false; }
> > +static inline bool pci_ancestor_pr3_present(struct pci_dev *pdev) { return 
> > false; }
> >  #endif
> >
> >  #ifdef CONFIG_EEH
>
> --
> Sathyanarayanan Kuppuswamy
> Linux Kernel Developer
>