[PATCH v1 1/1] xen/xenbus: Use *-y instead of *-objs in Makefile

[Andy Shevchenko]

*-objs suffix is reserved rather for (user-space) host programs while
usually *-y suffix is used for kernel drivers (although *-objs works
for that purpose for now).

Let's correct the old usages of *-objs in Makefiles.

Signed-off-by: Andy Shevchenko 
---

Note, the original approach is weirdest from the existing.
Only a few drivers use this (-objs-y) one most likely by mistake.

 drivers/xen/xenbus/Makefile | 18 --
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/xen/xenbus/Makefile b/drivers/xen/xenbus/Makefile
index 0c7532110815..b0d69602214e 100644
--- a/drivers/xen/xenbus/Makefile
+++ b/drivers/xen/xenbus/Makefile
@@ -1,15 +1,13 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-y  += xenbus.o
+
+xenbus-y := xenbus_client.o
+xenbus-y += xenbus_comms.o
+xenbus-y += xenbus_xs.o
+xenbus-y += xenbus_probe.o
+
+xenbus-$(CONFIG_XEN_BACKEND) += xenbus_probe_backend.o
+
 obj-y  += xenbus_dev_frontend.o
-
-xenbus-objs =
-xenbus-objs += xenbus_client.o
-xenbus-objs += xenbus_comms.o
-xenbus-objs += xenbus_xs.o
-xenbus-objs += xenbus_probe.o
-
-xenbus-be-objs-$(CONFIG_XEN_BACKEND) += xenbus_probe_backend.o
-xenbus-objs += $(xenbus-be-objs-y)
-
 obj-$(CONFIG_XEN_BACKEND) += xenbus_dev_backend.o
 obj-$(CONFIG_XEN_XENBUS_FRONTEND) += xenbus_probe_frontend.o
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH 1/2] x86/xen/pvh: Set up percpu for stack canary in 32-bit kernel entry

[Andy Shevchenko]

On Mon, Oct 23, 2023 at 12:10 PM Hou Wenlong
 wrote:
>
> In a 32-bit SMP kernel, the stack canary is a percpu variable accessed
> as %fs:__stack_chk_guard. However, the ABI for PVH entry does not
> specify the %fs register state. It currently works because the initial
> %fs register is 0x10 for QEMU, which is the same as $PVH_DS_SEL.

> %However, for added safety, the percpu should be set up explicitly
> %before calling xen_prepare_pvh(), which accesses the stack canary.

Stray leading % in two lines above.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 00/24] ALSA: Generic PCM copy ops using sockptr_t

[Andy Shevchenko]

On Mon, Aug 07, 2023 at 05:22:18PM +0200, Takashi Iwai wrote:
> On Tue, 01 Aug 2023 19:51:39 +0200, Andy Shevchenko wrote:
> > On Tue, Aug 01, 2023 at 02:54:45PM +0200, Takashi Iwai wrote:

...

> I rather wonder why it can't be simple strncpy().

This is obvious. To avoid compiler warning about 0 (possible) truncation.

...

> > Taking all remarks into account I would rather go with sockptr.h being
> > untouched for now, just a big
> > 
> > /* DO NOT USE, it's obsolete, use uniptr.h instead! */
> > 
> > to be added.
> 
> Possibly that's a safer choice.  But the biggest question is whether
> we want a generic type or not.  Let's try to ask it first.
> 
> Interestingly, this file doesn't belong to any subsystem in
> MAINTAINERS, so I'm not sure who to be Cc'ed.  Chirstoph as the
> original author and net dev, maybe.

Yes, but actually it's fine to just copy and change sockptr.h to say
"that's blablabla for net subsystem" (maybe this is implied by the name?).
In that case we just introduce our copy and can do whatever modifications
we want (see previous reply).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 00/24] ALSA: Generic PCM copy ops using sockptr_t

[Andy Shevchenko]

On Tue, Aug 01, 2023 at 02:54:45PM +0200, Takashi Iwai wrote:
> On Mon, 31 Jul 2023 21:40:20 +0200,
> Mark Brown wrote:
> > On Mon, Jul 31, 2023 at 09:30:29PM +0200, Takashi Iwai wrote:
> > > Mark Brown wrote:
> > 
> > > > It really feels like we ought to rename, or add an alias for, the type
> > > > if we're going to start using it more widely - it's not helping to make
> > > > the code clearer.
> > 
> > > That was my very first impression, too, but I changed my mind after
> > > seeing the already used code.  An alias might work, either typedef or
> > > define genptr_t or such as sockptr_t.  But we'll need to copy the
> > > bunch of helper functions, too...
> > 
> > I would predict that if the type becomes more widely used that'll happen
> > eventually and the longer it's left the more work it'll be.
> 
> That's true.  The question is how more widely it'll be used, then.
> 
> Is something like below what you had in mind, too?

I agree with your proposal (uniptr_t also works for me), but see below.

...

> +#include 
> +#include 

But let make the list of the headers right this time.

It seems to me that

err.h
minmax.h // maybe not, see a remark at the bottom
string.h
types.h

are missing.

More below.

...

> + void *p = kmalloc_track_caller(len, GFP_USER | __GFP_NOWARN);
> +
> + if (!p)
> + return ERR_PTR(-ENOMEM);

This can use cleanup.h.

> + if (copy_from_uniptr(p, src, len)) {
> + kfree(p);
> + return ERR_PTR(-EFAULT);
> + }
> + return p;
> +}
> +
> +static inline void *memdup_uniptr_nul(uniptr_t src, size_t len)
> +{
> + char *p = kmalloc_track_caller(len + 1, GFP_KERNEL);

Ditto.

> + if (!p)
> + return ERR_PTR(-ENOMEM);
> + if (copy_from_uniptr(p, src, len)) {
> + kfree(p);
> + return ERR_PTR(-EFAULT);
> + }
> + p[len] = '\0';
> + return p;
> +}

...

> +static inline long strncpy_from_uniptr(char *dst, uniptr_t src, size_t count)
> +{
> + if (uniptr_is_kernel(src)) {
> + size_t len = min(strnlen(src.kernel, count - 1) + 1, count);

I didn't get why do we need min()? To check the count == 0 case?

Wouldn't

size_t len;

len = strnlen(src.kernel, count);
if (len == 0)
return 0;

/* Copy a trailing NUL if found */
if (len < count)
len++;

be a good equivalent?

> + memcpy(dst, src.kernel, len);
> + return len;
> + }
> + return strncpy_from_user(dst, src.user, count);
> +}

...

> +static inline int check_zeroed_uniptr(uniptr_t src, size_t offset, size_t 
> size)
> +{
> + if (!uniptr_is_kernel(src))

Why not to align all the functions to use same conditional (either always
positive or negative)?

> + return check_zeroed_user(src.user + offset, size);
> + return memchr_inv(src.kernel + offset, 0, size) == NULL;
> +}

...

Taking all remarks into account I would rather go with sockptr.h being
untouched for now, just a big

/* DO NOT USE, it's obsolete, use uniptr.h instead! */

to be added.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 00/24] ALSA: Generic PCM copy ops using sockptr_t

[Andy Shevchenko]

On Mon, Jul 31, 2023 at 09:30:29PM +0200, Takashi Iwai wrote:
> On Mon, 31 Jul 2023 19:20:54 +0200,
> Mark Brown wrote:
> > 
> > On Mon, Jul 31, 2023 at 05:46:54PM +0200, Takashi Iwai wrote:
> > 
> > > this is a patch set to clean up the PCM copy ops using sockptr_t as a
> > > "universal" pointer, inspired by the recent patch from Andy
> > > Shevchenko:
> > >   
> > > https://lore.kernel.org/r/20230721100146.67293-1-andriy.shevche...@linux.intel.com
> > 
> > > Even though it sounds a bit weird, sockptr_t is a generic type that is
> > > used already in wide ranges, and it can fit our purpose, too.  With
> > > sockptr_t, the former split of copy_user and copy_kernel PCM ops can
> > > be unified again gracefully.
> > 
> > It really feels like we ought to rename, or add an alias for, the type
> > if we're going to start using it more widely - it's not helping to make
> > the code clearer.
> 
> That was my very first impression, too, but I changed my mind after
> seeing the already used code.  An alias might work, either typedef or
> define genptr_t or such as sockptr_t.  But we'll need to copy the
> bunch of helper functions, too...

Maybe we should define a genptr_t (in genptr.h) and convert sockptr infra to
use it (in sockptr.h)? This will leave network and other existing users to
convert to it step-by-step.

Another approach is to simply copy sockptr.h to genptr.h with changed naming
scheme and add a deprecation note to the former.

Thank you, Takashi, for doing this!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 0/4] Make sscanf() stricter

[Andy Shevchenko]

On Sat, Jun 10, 2023 at 04:40:40PM -0400, Demi Marie Obenour wrote:
> Roger Pau Monné suggested making xenbus_scanf() stricter instead of
> using a custom parser.  Christoph Hellwig asked why the normal vsscanf()
> cannot be made stricter.  Richard Weinberger mentioned Linus Torvalds’s
> suggestion of using ! to allow overflow.

As Rasmus articulated, NAK w.o. test cases being added to all parts where your
changes touch.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v8 0/7] Add pci_dev_for_each_resource() helper and update users

[Andy Shevchenko]

On Wed, May 31, 2023 at 04:30:28PM -0500, Bjorn Helgaas wrote:
> On Wed, May 31, 2023 at 08:48:35PM +0200, Jonas Gorski wrote:

...

> > Looking at the code I understand where coverity is coming from:
> > 
> > #define __pci_dev_for_each_res0(dev, res, ...) \
> >for (unsigned int __b = 0;  \
> > res = pci_resource_n(dev, __b), __b < PCI_NUM_RESOURCES;   \
> > __b++)
> > 
> >  res will be assigned before __b is checked for being less than
> > PCI_NUM_RESOURCES, making it point to behind the array at the end of
> > the last loop iteration.
> > 
> > Rewriting the test expression as
> > 
> > __b < PCI_NUM_RESOURCES && (res = pci_resource_n(dev, __b));
> > 
> > should avoid the (coverity) warning by making use of lazy evaluation.
> > 
> > It probably makes the code slightly less performant as res will now be
> > checked for being not NULL (which will always be true), but I doubt it
> > will be significant (or in any hot paths).
> 
> Thanks a lot for looking into this!  I think you're right, and I think
> the rewritten expression is more logical as well.  Do you want to post
> a patch for it?

Gimme some time, I was on a long leave and now it's a pile to handle.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v8 0/7] Add pci_dev_for_each_resource() helper and update users

[Andy Shevchenko]

On Thu, Jun 01, 2023 at 07:25:46PM +0300, Andy Shevchenko wrote:
> On Wed, May 31, 2023 at 08:48:35PM +0200, Jonas Gorski wrote:
> > On Tue, 30 May 2023 at 23:34, Bjorn Helgaas  wrote:
> > > On Fri, May 12, 2023 at 02:48:51PM -0500, Bjorn Helgaas wrote:

...

> > > Where are we at?  Are we going to ignore this because some Coverity
> > > reports are false positives?
> > 
> > Looking at the code I understand where coverity is coming from:
> > 
> > #define __pci_dev_for_each_res0(dev, res, ...) \
> >for (unsigned int __b = 0;  \
> > res = pci_resource_n(dev, __b), __b < PCI_NUM_RESOURCES;   \
> > __b++)
> > 
> >  res will be assigned before __b is checked for being less than
> > PCI_NUM_RESOURCES, making it point to behind the array at the end of
> > the last loop iteration.
> 
> Which is fine and you stumbled over the same mistake I made, that's why the
> documentation has been added to describe why the heck this macro is written
> the way it's written.
> 
> Coverity sucks.
> 
> > Rewriting the test expression as
> > 
> > __b < PCI_NUM_RESOURCES && (res = pci_resource_n(dev, __b));
> > 
> > should avoid the (coverity) warning by making use of lazy evaluation.
> 
> Obviously NAK.
> 
> > It probably makes the code slightly less performant as res will now be
> > checked for being not NULL (which will always be true), but I doubt it
> > will be significant (or in any hot paths).

Oh my god, I mistakenly read this as bus macro, sorry for my rant,
it's simply wrong.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v8 0/7] Add pci_dev_for_each_resource() helper and update users

[Andy Shevchenko]

On Wed, May 31, 2023 at 08:48:35PM +0200, Jonas Gorski wrote:
> On Tue, 30 May 2023 at 23:34, Bjorn Helgaas  wrote:
> > On Fri, May 12, 2023 at 02:48:51PM -0500, Bjorn Helgaas wrote:
> > > On Fri, May 12, 2023 at 01:56:29PM +0300, Andy Shevchenko wrote:
> > > > On Tue, May 09, 2023 at 01:21:22PM -0500, Bjorn Helgaas wrote:
> > > > > On Tue, Apr 04, 2023 at 11:11:01AM -0500, Bjorn Helgaas wrote:
> > > > > > On Thu, Mar 30, 2023 at 07:24:27PM +0300, Andy Shevchenko wrote:
> > > > > > > Provide two new helper macros to iterate over PCI device 
> > > > > > > resources and
> > > > > > > convert users.
> > > > >
> > > > > > Applied 2-7 to pci/resource for v6.4, thanks, I really like this!
> > > > >
> > > > > This is 09cc90063240 ("PCI: Introduce pci_dev_for_each_resource()")
> > > > > upstream now.
> > > > >
> > > > > Coverity complains about each use,
> > > >
> > > > It needs more clarification here. Use of reduced variant of the
> > > > macro or all of them? If the former one, then I can speculate that
> > > > Coverity (famous for false positives) simply doesn't understand `for
> > > > (type var; var ...)` code.
> > >
> > > True, Coverity finds false positives.  It flagged every use in
> > > drivers/pci and drivers/pnp.  It didn't mention the arch/alpha, arm,
> > > mips, powerpc, sh, or sparc uses, but I think it just didn't look at
> > > those.
> > >
> > > It flagged both:
> > >
> > >   pbus_size_iopci_dev_for_each_resource(dev, r)
> > >   pbus_size_mem   pci_dev_for_each_resource(dev, r, i)
> > >
> > > Here's a spreadsheet with a few more details (unfortunately I don't
> > > know how to make it dump the actual line numbers or analysis like I
> > > pasted below, so "pci_dev_for_each_resource" doesn't appear).  These
> > > are mostly in the "Drivers-PCI" component.
> > >
> > > https://docs.google.com/spreadsheets/d/1ohOJwxqXXoDUA0gwopgk-z-6ArLvhN7AZn4mIlDkHhQ/edit?usp=sharing
> > >
> > > These particular reports are in the "High Impact Outstanding" tab.
> >
> > Where are we at?  Are we going to ignore this because some Coverity
> > reports are false positives?
> 
> Looking at the code I understand where coverity is coming from:
> 
> #define __pci_dev_for_each_res0(dev, res, ...) \
>for (unsigned int __b = 0;  \
> res = pci_resource_n(dev, __b), __b < PCI_NUM_RESOURCES;   \
> __b++)
> 
>  res will be assigned before __b is checked for being less than
> PCI_NUM_RESOURCES, making it point to behind the array at the end of
> the last loop iteration.

Which is fine and you stumbled over the same mistake I made, that's why the
documentation has been added to describe why the heck this macro is written
the way it's written.

Coverity sucks.

> Rewriting the test expression as
> 
> __b < PCI_NUM_RESOURCES && (res = pci_resource_n(dev, __b));
> 
> should avoid the (coverity) warning by making use of lazy evaluation.

Obviously NAK.

> It probably makes the code slightly less performant as res will now be
> checked for being not NULL (which will always be true), but I doubt it
> will be significant (or in any hot paths).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 00/20] x86: address -Wmissing-prototype warnings

[Andy Shevchenko]

On Fri, May 19, 2023 at 12:56 AM Dave Hansen  wrote:
> On 5/16/23 12:35, Arnd Bergmann wrote:

> I picked up the ones that were blatantly obvious, but left out 03, 04,
> 10, 12 and 19 for the moment.

Btw, there is series that went unnoticed

https://lore.kernel.org/all/2029110017.48510-1-andriy.shevche...@linux.intel.com/

I dunno why.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v8 0/7] Add pci_dev_for_each_resource() helper and update users

[Andy Shevchenko]

On Tue, May 09, 2023 at 01:21:22PM -0500, Bjorn Helgaas wrote:
> On Tue, Apr 04, 2023 at 11:11:01AM -0500, Bjorn Helgaas wrote:
> > On Thu, Mar 30, 2023 at 07:24:27PM +0300, Andy Shevchenko wrote:
> > > Provide two new helper macros to iterate over PCI device resources and
> > > convert users.
> 
> > Applied 2-7 to pci/resource for v6.4, thanks, I really like this!
> 
> This is 09cc90063240 ("PCI: Introduce pci_dev_for_each_resource()")
> upstream now.
> 
> Coverity complains about each use,

It needs more clarification here. Use of reduced variant of the macro or all of
them? If the former one, then I can speculate that Coverity (famous for false
positives) simply doesn't understand `for (type var; var ...)` code.

>   sample below from
> drivers/pci/vgaarb.c.  I didn't investigate at all, so it might be a
> false positive; just FYI.
> 
> 1. Condition screen_info.capabilities & (2U /* 1 << 1 */), taking 
> true branch.
>   556if (screen_info.capabilities & VIDEO_CAPABILITY_64BIT_BASE)
>   557base |= (u64)screen_info.ext_lfb_base << 32;
>   558
>   559limit = base + size;
>   560
>   561/* Does firmware framebuffer belong to us? */
> 2. Condition __b < PCI_NUM_RESOURCES, taking true branch.
> 3. Condition (r = >resource[__b]) , (__b < PCI_NUM_RESOURCES), 
> taking true branch.
> 6. Condition __b < PCI_NUM_RESOURCES, taking true branch.
> 7. cond_at_most: Checking __b < PCI_NUM_RESOURCES implies that __b 
> may be up to 16 on the true branch.
> 8. Condition (r = >resource[__b]) , (__b < PCI_NUM_RESOURCES), 
> taking true branch.
> 11. incr: Incrementing __b. The value of __b may now be up to 17.
> 12. alias: Assigning: r = >resource[__b]. r may now point to as 
> high as element 17 of pdev->resource (which consists of 17 64-byte elements).
> 13. Condition __b < PCI_NUM_RESOURCES, taking true branch.
> 14. Condition (r = >resource[__b]) , (__b < PCI_NUM_RESOURCES), 
> taking true branch.
>   562pci_dev_for_each_resource(pdev, r) {
> 4. Condition resource_type(r) != 512, taking true branch.
> 9. Condition resource_type(r) != 512, taking true branch.
> 
>   CID 1529911 (#1 of 1): Out-of-bounds read (OVERRUN)
>   15. overrun-local: Overrunning array of 1088 bytes at byte offset 1088 by 
> dereferencing pointer r. [show details]
>   563if (resource_type(r) != IORESOURCE_MEM)
> 5. Continuing loop.
> 10. Continuing loop.
>   564continue;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v8 0/7] Add pci_dev_for_each_resource() helper and update users

[Andy Shevchenko]

On Wed, Apr 05, 2023 at 03:18:32PM -0500, Bjorn Helgaas wrote:
> On Wed, Apr 05, 2023 at 11:28:27AM +0300, Andy Shevchenko wrote:
> > On Tue, Apr 04, 2023 at 11:11:01AM -0500, Bjorn Helgaas wrote:
> > > On Thu, Mar 30, 2023 at 07:24:27PM +0300, Andy Shevchenko wrote:

...

> > > I omitted
> > > 
> > >   [1/7] kernel.h: Split out COUNT_ARGS() and CONCATENATE()"
> > > 
> > > only because it's not essential to this series and has only a trivial
> > > one-line impact on include/linux/pci.h.
> > 
> > I'm not sure I understood what exactly "essentiality" means to you, but
> > I included that because it makes the split which can be used later by
> > others and not including kernel.h in the header is the objective I want
> > to achieve. Without this patch the achievement is going to be deferred.
> > Yet, this, as you have noticed, allows to compile and use the macros in
> > the rest of the patches.
> 
> I haven't followed the kernel.h splitting, and I try to avoid
> incidental changes outside of the files I maintain, so I just wanted
> to keep this series purely PCI and avoid any possible objections to a
> new include file or discussion about how it should be done.

Okay, fair enough :-) Thank you for elaboration, I will send the new version of
patch 7 separately.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v8 5/7] PCI: Allow pci_bus_for_each_resource() to take less arguments

[Andy Shevchenko]

On Thu, Mar 30, 2023 at 07:24:32PM +0300, Andy Shevchenko wrote:
> Refactor pci_bus_for_each_resource() in the same way as it's done in
> pci_dev_for_each_resource() case. This will allow to hide iterator
> inside the loop, where it's not used otherwise.
> 
> No functional changes intended.

Bjorn, this has wrong author in your tree:

https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=resource=46dbad19a59e0dd8f1e7065e5281345797fbb365

Or did I misinterpret something?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v8 7/7] pcmcia: Convert to use less arguments in pci_bus_for_each_resource()

[Andy Shevchenko]

On Thu, Mar 30, 2023 at 07:24:34PM +0300, Andy Shevchenko wrote:

...

> @@ -960,12 +960,9 @@ static int nonstatic_autoadd_resources(struct 
> pcmcia_socket *s)
>*/
>   if (s->cb_dev->bus->number == 0)
>   return -EINVAL;
> -
> - for (i = 0; i < PCI_BRIDGE_RESOURCE_NUM; i++) {
> - res = s->cb_dev->bus->resource[i];
> -#else
> - pci_bus_for_each_resource(s->cb_dev->bus, res, i) {
>  #endif
> +
> + pci_bus_for_each_resource(s->cb_dev->bus, res) {
>   if (!res)
>   continue;

As pointed out in the reply to Bjorn's email this hunk needs to be revisited,
since I wrote the documentation for the above call I have started understanding
the deal behind this special treatment for X86 case.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v8 0/7] Add pci_dev_for_each_resource() helper and update users

[Andy Shevchenko]

On Tue, Apr 04, 2023 at 11:11:01AM -0500, Bjorn Helgaas wrote:
> On Thu, Mar 30, 2023 at 07:24:27PM +0300, Andy Shevchenko wrote:
> > Provide two new helper macros to iterate over PCI device resources and
> > convert users.
> > 
> > Looking at it, refactor existing pci_bus_for_each_resource() and convert
> > users accordingly.
> > 
> > Note, the amount of lines grew due to the documentation update.
> > 
> > Changelog v8:
> > - fixed issue with pci_bus_for_each_resource() macro (LKP)
> > - due to above added a new patch to document how it works
> > - moved the last patch to be #2 (Philippe)
> > - added tags (Philippe)
> > 
> > Changelog v7:
> > - made both macros to share same name (Bjorn)
> 
> I didn't actually request the same name for both; I would have had no
> idea how to even do that :)
> 
> v6 had:
> 
>   pci_dev_for_each_resource_p(dev, res)
>   pci_dev_for_each_resource(dev, res, i)
> 
> and I suggested:
> 
>   pci_dev_for_each_resource(dev, res)
>   pci_dev_for_each_resource_idx(dev, res, i)
> 
> because that pattern is used elsewhere.

Ah, sorry I misinterpreted your suggestion (I thought that at the end of
the day you wanted the macro to be less intrusive, so we change less code,
that's why I interpreted it the way described in the Changelog).

> But you figured out how to do
> it, and having one name is even better, so thanks for that extra work!

You are welcome!

> > - split out the pci_resource_n() conversion (Bjorn)
> > 
> > Changelog v6:
> > - dropped unused variable in PPC code (LKP)
> > 
> > Changelog v5:
> > - renamed loop variable to minimize the clash (Keith)
> > - addressed smatch warning (Dan)
> > - addressed 0-day bot findings (LKP)
> > 
> > Changelog v4:
> > - rebased on top of v6.3-rc1
> > - added tag (Krzysztof)
> > 
> > Changelog v3:
> > - rebased on top of v2 by Mika, see above
> > - added tag to pcmcia patch (Dominik)
> > 
> > Changelog v2:
> > - refactor to have two macros
> > - refactor existing pci_bus_for_each_resource() in the same way and
> >   convert users
> > 
> > Andy Shevchenko (6):
> >   kernel.h: Split out COUNT_ARGS() and CONCATENATE()
> >   PCI: Introduce pci_resource_n()
> >   PCI: Document pci_bus_for_each_resource() to avoid confusion
> >   PCI: Allow pci_bus_for_each_resource() to take less arguments
> >   EISA: Convert to use less arguments in pci_bus_for_each_resource()
> >   pcmcia: Convert to use less arguments in pci_bus_for_each_resource()

...

> Applied 2-7 to pci/resource for v6.4, thanks, I really like this!

Btw, can you actually drop patch 7, please?
After I have updated the documentation I have realised that why the first
chunk is invalid. It needs mode careful check and rework.

> I omitted
> 
>   [1/7] kernel.h: Split out COUNT_ARGS() and CONCATENATE()"
> 
> only because it's not essential to this series and has only a trivial
> one-line impact on include/linux/pci.h.

I'm not sure I understood what exactly "essentiality" means to you, but
I included that because it makes the split which can be used later by
others and not including kernel.h in the header is the objective I want
to achieve. Without this patch the achievement is going to be deferred.
Yet, this, as you have noticed, allows to compile and use the macros in
the rest of the patches.

P.S. Thank you for the review and application of the rest!

-- 
With Best Regards,
Andy Shevchenko

[PATCH v8 5/7] PCI: Allow pci_bus_for_each_resource() to take less arguments

[Andy Shevchenko]

Refactor pci_bus_for_each_resource() in the same way as it's done in
pci_dev_for_each_resource() case. This will allow to hide iterator
inside the loop, where it's not used otherwise.

No functional changes intended.

Signed-off-by: Andy Shevchenko 
Reviewed-by: Krzysztof Wilczyński 
Reviewed-by: Philippe Mathieu-Daudé 
---
 drivers/pci/bus.c  |  7 +++
 drivers/pci/hotplug/shpchp_sysfs.c |  8 
 drivers/pci/pci.c  |  3 +--
 drivers/pci/probe.c|  2 +-
 drivers/pci/setup-bus.c| 10 --
 include/linux/pci.h| 24 +++-
 6 files changed, 32 insertions(+), 22 deletions(-)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 549c4bd5caec..5bc81cc0a2de 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -182,13 +182,13 @@ static int pci_bus_alloc_from_region(struct pci_bus *bus, 
struct resource *res,
void *alignf_data,
struct pci_bus_region *region)
 {
-   int i, ret;
struct resource *r, avail;
resource_size_t max;
+   int ret;
 
type_mask |= IORESOURCE_TYPE_BITS;
 
-   pci_bus_for_each_resource(bus, r, i) {
+   pci_bus_for_each_resource(bus, r) {
resource_size_t min_used = min;
 
if (!r)
@@ -289,9 +289,8 @@ bool pci_bus_clip_resource(struct pci_dev *dev, int idx)
struct resource *res = >resource[idx];
struct resource orig_res = *res;
struct resource *r;
-   int i;
 
-   pci_bus_for_each_resource(bus, r, i) {
+   pci_bus_for_each_resource(bus, r) {
resource_size_t start, end;
 
if (!r)
diff --git a/drivers/pci/hotplug/shpchp_sysfs.c 
b/drivers/pci/hotplug/shpchp_sysfs.c
index 64beed7a26be..01d47a42da04 100644
--- a/drivers/pci/hotplug/shpchp_sysfs.c
+++ b/drivers/pci/hotplug/shpchp_sysfs.c
@@ -24,16 +24,16 @@
 static ssize_t show_ctrl(struct device *dev, struct device_attribute *attr, 
char *buf)
 {
struct pci_dev *pdev;
-   int index, busnr;
struct resource *res;
struct pci_bus *bus;
size_t len = 0;
+   int busnr;
 
pdev = to_pci_dev(dev);
bus = pdev->subordinate;
 
len += sysfs_emit_at(buf, len, "Free resources: memory\n");
-   pci_bus_for_each_resource(bus, res, index) {
+   pci_bus_for_each_resource(bus, res) {
if (res && (res->flags & IORESOURCE_MEM) &&
!(res->flags & IORESOURCE_PREFETCH)) {
len += sysfs_emit_at(buf, len,
@@ -43,7 +43,7 @@ static ssize_t show_ctrl(struct device *dev, struct 
device_attribute *attr, char
}
}
len += sysfs_emit_at(buf, len, "Free resources: prefetchable memory\n");
-   pci_bus_for_each_resource(bus, res, index) {
+   pci_bus_for_each_resource(bus, res) {
if (res && (res->flags & IORESOURCE_MEM) &&
   (res->flags & IORESOURCE_PREFETCH)) {
len += sysfs_emit_at(buf, len,
@@ -53,7 +53,7 @@ static ssize_t show_ctrl(struct device *dev, struct 
device_attribute *attr, char
}
}
len += sysfs_emit_at(buf, len, "Free resources: IO\n");
-   pci_bus_for_each_resource(bus, res, index) {
+   pci_bus_for_each_resource(bus, res) {
if (res && (res->flags & IORESOURCE_IO)) {
len += sysfs_emit_at(buf, len,
 "start = %8.8llx, length = 
%8.8llx\n",
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 45c3bb039f21..585bb3988ddf 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -779,9 +779,8 @@ struct resource *pci_find_parent_resource(const struct 
pci_dev *dev,
 {
const struct pci_bus *bus = dev->bus;
struct resource *r;
-   int i;
 
-   pci_bus_for_each_resource(bus, r, i) {
+   pci_bus_for_each_resource(bus, r) {
if (!r)
continue;
if (resource_contains(r, res)) {
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index a3f68b6ba6ac..f8191750f6b7 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -533,7 +533,7 @@ void pci_read_bridge_bases(struct pci_bus *child)
pci_read_bridge_mmio_pref(child);
 
if (dev->transparent) {
-   pci_bus_for_each_resource(child->parent, res, i) {
+   pci_bus_for_each_resource(child->parent, res) {
if (res && res->flags) {
pci_bus_add_resource(child, res,
 PCI_SUBTRACTIVE_DECODE);
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 027b985dd1ee..fdeb121e9175 100644
--- a/

[PATCH v8 7/7] pcmcia: Convert to use less arguments in pci_bus_for_each_resource()

[Andy Shevchenko]

The pci_bus_for_each_resource() can hide the iterator loop since
it may be not used otherwise. With this, we may drop that iterator
variable definition.

Signed-off-by: Andy Shevchenko 
Reviewed-by: Krzysztof Wilczyński 
Acked-by: Dominik Brodowski 
---
 drivers/pcmcia/rsrc_nonstatic.c | 9 +++--
 drivers/pcmcia/yenta_socket.c   | 3 +--
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/pcmcia/rsrc_nonstatic.c b/drivers/pcmcia/rsrc_nonstatic.c
index ad1141fddb4c..96264ebee46a 100644
--- a/drivers/pcmcia/rsrc_nonstatic.c
+++ b/drivers/pcmcia/rsrc_nonstatic.c
@@ -934,7 +934,7 @@ static int adjust_io(struct pcmcia_socket *s, unsigned int 
action, unsigned long
 static int nonstatic_autoadd_resources(struct pcmcia_socket *s)
 {
struct resource *res;
-   int i, done = 0;
+   int done = 0;
 
if (!s->cb_dev || !s->cb_dev->bus)
return -ENODEV;
@@ -960,12 +960,9 @@ static int nonstatic_autoadd_resources(struct 
pcmcia_socket *s)
 */
if (s->cb_dev->bus->number == 0)
return -EINVAL;
-
-   for (i = 0; i < PCI_BRIDGE_RESOURCE_NUM; i++) {
-   res = s->cb_dev->bus->resource[i];
-#else
-   pci_bus_for_each_resource(s->cb_dev->bus, res, i) {
 #endif
+
+   pci_bus_for_each_resource(s->cb_dev->bus, res) {
if (!res)
continue;
 
diff --git a/drivers/pcmcia/yenta_socket.c b/drivers/pcmcia/yenta_socket.c
index 1365eaa20ff4..fd18ab571ce8 100644
--- a/drivers/pcmcia/yenta_socket.c
+++ b/drivers/pcmcia/yenta_socket.c
@@ -673,9 +673,8 @@ static int yenta_search_res(struct yenta_socket *socket, 
struct resource *res,
u32 min)
 {
struct resource *root;
-   int i;
 
-   pci_bus_for_each_resource(socket->dev->bus, root, i) {
+   pci_bus_for_each_resource(socket->dev->bus, root) {
if (!root)
continue;
 
-- 
2.40.0.1.gaa8946217a0b

[PATCH v8 4/7] PCI: Document pci_bus_for_each_resource() to avoid confusion

[Andy Shevchenko]

There might be a confusion with the implementation of the
pci_bus_for_each_resources() due to side effect of Logical
OR. Document entire macro and explain how it works and why
the conditional needs to be like that.

Signed-off-by: Andy Shevchenko 
---
 include/linux/pci.h | 20 
 1 file changed, 20 insertions(+)

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 5cacd9e4c8cd..e3b3af606280 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1446,6 +1446,26 @@ int devm_request_pci_bus_resources(struct device *dev,
 /* Temporary until new and working PCI SBR API in place */
 int pci_bridge_secondary_bus_reset(struct pci_dev *dev);
 
+/**
+ * pci_bus_for_each_resource - iterate over PCI bus resources
+ * @bus: the PCI bus
+ * @res: a varible to keep a pointer to the current resource
+ * @i: a variable to keep the index of the current resource
+ *
+ * Iterate over PCI bus resources. The first part is to go over PCI bus
+ * resource array, which has at most the %PCI_BRIDGE_RESOURCE_NUM entries.
+ * After that continue with the separate list of the additional resources,
+ * if not empty. That's why the Logical OR is being used.
+ *
+ * Possible usage:
+ *
+ * struct pci_bus *bus = ...;
+ * struct resource *res;
+ * unsigned int i;
+ *
+ * pci_bus_for_each_resource(bus, res, i)
+ * pr_info("PCI bus resource[%u]: %pR\n", i, res);
+ */
 #define pci_bus_for_each_resource(bus, res, i) \
for (i = 0; \
(res = pci_bus_resource_n(bus, i)) || i < PCI_BRIDGE_RESOURCE_NUM; \
-- 
2.40.0.1.gaa8946217a0b

[PATCH v8 6/7] EISA: Convert to use less arguments in pci_bus_for_each_resource()

[Andy Shevchenko]

The pci_bus_for_each_resource() can hide the iterator loop since
it may be not used otherwise. With this, we may drop that iterator
variable definition.

Signed-off-by: Andy Shevchenko 
Reviewed-by: Krzysztof Wilczyński 
Reviewed-by: Philippe Mathieu-Daudé 
---
 drivers/eisa/pci_eisa.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/eisa/pci_eisa.c b/drivers/eisa/pci_eisa.c
index 930c2332c3c4..8173e60bb808 100644
--- a/drivers/eisa/pci_eisa.c
+++ b/drivers/eisa/pci_eisa.c
@@ -20,8 +20,8 @@ static struct eisa_root_device pci_eisa_root;
 
 static int __init pci_eisa_init(struct pci_dev *pdev)
 {
-   int rc, i;
struct resource *res, *bus_res = NULL;
+   int rc;
 
if ((rc = pci_enable_device (pdev))) {
dev_err(>dev, "Could not enable device\n");
@@ -38,7 +38,7 @@ static int __init pci_eisa_init(struct pci_dev *pdev)
 * eisa_root_register() can only deal with a single io port resource,
*  so we use the first valid io port resource.
 */
-   pci_bus_for_each_resource(pdev->bus, res, i)
+   pci_bus_for_each_resource(pdev->bus, res)
if (res && (res->flags & IORESOURCE_IO)) {
bus_res = res;
break;
-- 
2.40.0.1.gaa8946217a0b

[PATCH v8 3/7] PCI: Introduce pci_dev_for_each_resource()

[Andy Shevchenko]

From: Mika Westerberg 

Instead of open-coding it everywhere introduce a tiny helper that can be
used to iterate over each resource of a PCI device, and convert the most
obvious users into it.

While at it drop doubled empty line before pdev_sort_resources().

No functional changes intended.

Suggested-by: Andy Shevchenko 
Signed-off-by: Mika Westerberg 
Signed-off-by: Andy Shevchenko 
Reviewed-by: Krzysztof Wilczyński 
---
 .clang-format |  1 +
 arch/alpha/kernel/pci.c   |  5 ++--
 arch/arm/kernel/bios32.c  | 16 ++---
 arch/arm/mach-dove/pcie.c | 10 
 arch/arm/mach-mv78xx0/pcie.c  | 10 
 arch/arm/mach-orion5x/pci.c   | 10 
 arch/mips/pci/ops-bcm63xx.c   |  8 +++
 arch/mips/pci/pci-legacy.c|  3 +--
 arch/powerpc/kernel/pci-common.c  | 21 
 arch/powerpc/platforms/4xx/pci.c  |  8 +++
 arch/powerpc/platforms/52xx/mpc52xx_pci.c |  5 ++--
 arch/powerpc/platforms/pseries/pci.c  | 16 ++---
 arch/sh/drivers/pci/pcie-sh7786.c | 10 
 arch/sparc/kernel/leon_pci.c  |  5 ++--
 arch/sparc/kernel/pci.c   | 10 
 arch/sparc/kernel/pcic.c  |  5 ++--
 drivers/pci/remove.c  |  5 ++--
 drivers/pci/setup-bus.c   | 27 -
 drivers/pci/setup-res.c   |  4 +---
 drivers/pci/vgaarb.c  | 17 -
 drivers/pci/xen-pcifront.c|  4 +---
 drivers/pnp/quirks.c  | 29 ---
 include/linux/pci.h   | 15 
 23 files changed, 112 insertions(+), 132 deletions(-)

diff --git a/.clang-format b/.clang-format
index d988e9fa9b26..2048b0296d76 100644
--- a/.clang-format
+++ b/.clang-format
@@ -520,6 +520,7 @@ ForEachMacros:
   - 'of_property_for_each_string'
   - 'of_property_for_each_u32'
   - 'pci_bus_for_each_resource'
+  - 'pci_dev_for_each_resource'
   - 'pci_doe_for_each_off'
   - 'pcl_for_each_chunk'
   - 'pcl_for_each_segment'
diff --git a/arch/alpha/kernel/pci.c b/arch/alpha/kernel/pci.c
index 64fbfb0763b2..4458eb7f44f0 100644
--- a/arch/alpha/kernel/pci.c
+++ b/arch/alpha/kernel/pci.c
@@ -288,11 +288,10 @@ pcibios_claim_one_bus(struct pci_bus *b)
struct pci_bus *child_bus;
 
list_for_each_entry(dev, >devices, bus_list) {
+   struct resource *r;
int i;
 
-   for (i = 0; i < PCI_NUM_RESOURCES; i++) {
-   struct resource *r = >resource[i];
-
+   pci_dev_for_each_resource(dev, r, i) {
if (r->parent || !r->start || !r->flags)
continue;
if (pci_has_flag(PCI_PROBE_ONLY) ||
diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index e7ef2b5bea9c..d334c7fb672b 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -142,15 +142,15 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_WINBOND2, 
PCI_DEVICE_ID_WINBOND2_89C940F,
  */
 static void pci_fixup_dec21285(struct pci_dev *dev)
 {
-   int i;
-
if (dev->devfn == 0) {
+   struct resource *r;
+
dev->class &= 0xff;
dev->class |= PCI_CLASS_BRIDGE_HOST << 8;
-   for (i = 0; i < PCI_NUM_RESOURCES; i++) {
-   dev->resource[i].start = 0;
-   dev->resource[i].end   = 0;
-   dev->resource[i].flags = 0;
+   pci_dev_for_each_resource(dev, r) {
+   r->start = 0;
+   r->end = 0;
+   r->flags = 0;
}
}
 }
@@ -162,13 +162,11 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_DEC, 
PCI_DEVICE_ID_DEC_21285, pci_fixup_d
 static void pci_fixup_ide_bases(struct pci_dev *dev)
 {
struct resource *r;
-   int i;
 
if ((dev->class >> 8) != PCI_CLASS_STORAGE_IDE)
return;
 
-   for (i = 0; i < PCI_NUM_RESOURCES; i++) {
-   r = dev->resource + i;
+   pci_dev_for_each_resource(dev, r) {
if ((r->start & ~0x80) == 0x374) {
r->start |= 2;
r->end = r->start;
diff --git a/arch/arm/mach-dove/pcie.c b/arch/arm/mach-dove/pcie.c
index 754ca381f600..3044b7e03890 100644
--- a/arch/arm/mach-dove/pcie.c
+++ b/arch/arm/mach-dove/pcie.c
@@ -142,14 +142,14 @@ static struct pci_ops pcie_ops = {
 static void rc_pci_fixup(struct pci_dev *dev)
 {
if (dev->bus->parent == NULL && dev->devfn == 0) {
-   int i;
+   struct resource *r;
 
dev->class &= 0xff;
dev->class |= PCI_CLASS_BRIDGE_HOST &l

[PATCH v8 1/7] kernel.h: Split out COUNT_ARGS() and CONCATENATE()

[Andy Shevchenko]

kernel.h is being used as a dump for all kinds of stuff for a long time.
The COUNT_ARGS() and CONCATENATE() macros may be used in some places
without need of the full kernel.h dependency train with it.

Here is the attempt on cleaning it up by splitting out these macros().

Signed-off-by: Andy Shevchenko 
---
 include/linux/args.h   | 13 +
 include/linux/kernel.h |  8 +---
 2 files changed, 14 insertions(+), 7 deletions(-)
 create mode 100644 include/linux/args.h

diff --git a/include/linux/args.h b/include/linux/args.h
new file mode 100644
index ..16ef6fad8add
--- /dev/null
+++ b/include/linux/args.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _LINUX_ARGS_H
+#define _LINUX_ARGS_H
+
+/* This counts to 12. Any more, it will return 13th argument. */
+#define __COUNT_ARGS(_0, _1, _2, _3, _4, _5, _6, _7, _8, _9, _10, _11, _12, 
_n, X...) _n
+#define COUNT_ARGS(X...) __COUNT_ARGS(, ##X, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 
2, 1, 0)
+
+#define __CONCAT(a, b) a ## b
+#define CONCATENATE(a, b) __CONCAT(a, b)
+
+#endif /* _LINUX_ARGS_H */
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 0d91e0af0125..fa675d50d7b7 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -13,6 +13,7 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -457,13 +458,6 @@ ftrace_vprintk(const char *fmt, va_list ap)
 static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
 #endif /* CONFIG_TRACING */
 
-/* This counts to 12. Any more, it will return 13th argument. */
-#define __COUNT_ARGS(_0, _1, _2, _3, _4, _5, _6, _7, _8, _9, _10, _11, _12, 
_n, X...) _n
-#define COUNT_ARGS(X...) __COUNT_ARGS(, ##X, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 
2, 1, 0)
-
-#define __CONCAT(a, b) a ## b
-#define CONCATENATE(a, b) __CONCAT(a, b)
-
 /* Rebuild everything on CONFIG_FTRACE_MCOUNT_RECORD */
 #ifdef CONFIG_FTRACE_MCOUNT_RECORD
 # define REBUILD_DUE_TO_FTRACE_MCOUNT_RECORD
-- 
2.40.0.1.gaa8946217a0b

[PATCH v8 2/7] PCI: Introduce pci_resource_n()

[Andy Shevchenko]

Introduce pci_resource_n() and replace open-coded implementations of it
in pci.h.

Signed-off-by: Andy Shevchenko 
Reviewed-by: Philippe Mathieu-Daudé 
---
 include/linux/pci.h | 15 +++
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/include/linux/pci.h b/include/linux/pci.h
index b50e5c79f7e3..aeaa95455d4c 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1995,14 +1995,13 @@ int pci_iobar_pfn(struct pci_dev *pdev, int bar, struct 
vm_area_struct *vma);
  * These helpers provide future and backwards compatibility
  * for accessing popular PCI BAR info
  */
-#define pci_resource_start(dev, bar)   ((dev)->resource[(bar)].start)
-#define pci_resource_end(dev, bar) ((dev)->resource[(bar)].end)
-#define pci_resource_flags(dev, bar)   ((dev)->resource[(bar)].flags)
-#define pci_resource_len(dev,bar) \
-   ((pci_resource_end((dev), (bar)) == 0) ? 0 :\
-   \
-(pci_resource_end((dev), (bar)) -  \
- pci_resource_start((dev), (bar)) + 1))
+#define pci_resource_n(dev, bar)   (&(dev)->resource[(bar)])
+#define pci_resource_start(dev, bar)   (pci_resource_n(dev, bar)->start)
+#define pci_resource_end(dev, bar) (pci_resource_n(dev, bar)->end)
+#define pci_resource_flags(dev, bar)   (pci_resource_n(dev, bar)->flags)
+#define pci_resource_len(dev,bar)  \
+   (pci_resource_end((dev), (bar)) ?   \
+resource_size(pci_resource_n((dev), (bar))) : 0)
 
 /*
  * Similar to the helpers above, these manipulate per-pci_dev
-- 
2.40.0.1.gaa8946217a0b

[PATCH v8 0/7] Add pci_dev_for_each_resource() helper and update users

[Andy Shevchenko]

Provide two new helper macros to iterate over PCI device resources and
convert users.

Looking at it, refactor existing pci_bus_for_each_resource() and convert
users accordingly.

Note, the amount of lines grew due to the documentation update.

Changelog v8:
- fixed issue with pci_bus_for_each_resource() macro (LKP)
- due to above added a new patch to document how it works
- moved the last patch to be #2 (Philippe)
- added tags (Philippe)

Changelog v7:
- made both macros to share same name (Bjorn)
- split out the pci_resource_n() conversion (Bjorn)

Changelog v6:
- dropped unused variable in PPC code (LKP)

Changelog v5:
- renamed loop variable to minimize the clash (Keith)
- addressed smatch warning (Dan)
- addressed 0-day bot findings (LKP)

Changelog v4:
- rebased on top of v6.3-rc1
- added tag (Krzysztof)

Changelog v3:
- rebased on top of v2 by Mika, see above
- added tag to pcmcia patch (Dominik)

Changelog v2:
- refactor to have two macros
- refactor existing pci_bus_for_each_resource() in the same way and
  convert users

Andy Shevchenko (6):
  kernel.h: Split out COUNT_ARGS() and CONCATENATE()
  PCI: Introduce pci_resource_n()
  PCI: Document pci_bus_for_each_resource() to avoid confusion
  PCI: Allow pci_bus_for_each_resource() to take less arguments
  EISA: Convert to use less arguments in pci_bus_for_each_resource()
  pcmcia: Convert to use less arguments in pci_bus_for_each_resource()

Mika Westerberg (1):
  PCI: Introduce pci_dev_for_each_resource()

 .clang-format |  1 +
 arch/alpha/kernel/pci.c   |  5 +-
 arch/arm/kernel/bios32.c  | 16 +++--
 arch/arm/mach-dove/pcie.c | 10 ++--
 arch/arm/mach-mv78xx0/pcie.c  | 10 ++--
 arch/arm/mach-orion5x/pci.c   | 10 ++--
 arch/mips/pci/ops-bcm63xx.c   |  8 +--
 arch/mips/pci/pci-legacy.c|  3 +-
 arch/powerpc/kernel/pci-common.c  | 21 +++
 arch/powerpc/platforms/4xx/pci.c  |  8 +--
 arch/powerpc/platforms/52xx/mpc52xx_pci.c |  5 +-
 arch/powerpc/platforms/pseries/pci.c  | 16 ++---
 arch/sh/drivers/pci/pcie-sh7786.c | 10 ++--
 arch/sparc/kernel/leon_pci.c  |  5 +-
 arch/sparc/kernel/pci.c   | 10 ++--
 arch/sparc/kernel/pcic.c  |  5 +-
 drivers/eisa/pci_eisa.c   |  4 +-
 drivers/pci/bus.c |  7 +--
 drivers/pci/hotplug/shpchp_sysfs.c|  8 +--
 drivers/pci/pci.c |  3 +-
 drivers/pci/probe.c   |  2 +-
 drivers/pci/remove.c  |  5 +-
 drivers/pci/setup-bus.c   | 37 +---
 drivers/pci/setup-res.c   |  4 +-
 drivers/pci/vgaarb.c  | 17 ++
 drivers/pci/xen-pcifront.c|  4 +-
 drivers/pcmcia/rsrc_nonstatic.c   |  9 +--
 drivers/pcmcia/yenta_socket.c |  3 +-
 drivers/pnp/quirks.c  | 29 -
 include/linux/args.h  | 13 
 include/linux/kernel.h|  8 +--
 include/linux/pci.h   | 72 +++
 32 files changed, 190 insertions(+), 178 deletions(-)
 create mode 100644 include/linux/args.h

-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v7 3/6] PCI: Allow pci_bus_for_each_resource() to take less arguments

[Andy Shevchenko]

On Thu, Mar 30, 2023 at 09:24:21PM +0800, kernel test robot wrote:
> 
> Greeting,
> 
> FYI, we noticed various errors such like
> "i40e: probe of :3d:00.0 failed with error -12"
> due to commit (built with gcc-11):
> 
> commit: d23d5938fd7ced817d6aa1ff86cd671ebbaebfc2 ("[PATCH v7 3/6] PCI: Allow 
> pci_bus_for_each_resource() to take less arguments")
> url: 
> https://github.com/intel-lab-lkp/linux/commits/Andy-Shevchenko/kernel-h-Split-out-COUNT_ARGS-and-CONCATENATE/20230324-013857
> base: https://git.kernel.org/cgit/linux/kernel/git/pci/pci.git next
> patch link: 
> https://lore.kernel.org/all/20230323173610.60442-4-andriy.shevche...@linux.intel.com/
> patch subject: [PATCH v7 3/6] PCI: Allow pci_bus_for_each_resource() to take 
> less arguments
> 
> in testcase: boot
> 
> on test machine: 96 threads 2 sockets Intel(R) Xeon(R) Gold 6252 CPU @ 
> 2.10GHz (Cascade Lake) with 512G memory
> 
> caused below changes (please refer to attached dmesg/kmsg for entire 
> log/backtrace):
> 
> 
> If you fix the issue, kindly add following tag
> | Reported-by: kernel test robot 
> | Link: 
> https://lore.kernel.org/oe-lkp/202303302009.55848372-oliver.s...@intel.com

Thanks, that is useful test!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 6/6] PCI: Make use of pci_resource_n()

[Andy Shevchenko]

On Fri, Mar 24, 2023 at 10:08:39AM +0100, Philippe Mathieu-Daudé wrote:
> On 23/3/23 18:36, Andy Shevchenko wrote:
> > Replace open-coded implementations of pci_resource_n() in pci.h.

...

> >   #define pci_resource_n(dev, bar)  (&(dev)->resource[(bar)])
> > -#define pci_resource_start(dev, bar)   ((dev)->resource[(bar)].start)
> > -#define pci_resource_end(dev, bar) ((dev)->resource[(bar)].end)
> > -#define pci_resource_flags(dev, bar)   ((dev)->resource[(bar)].flags)
> > -#define pci_resource_len(dev,bar) \
> > -   ((pci_resource_end((dev), (bar)) == 0) ? 0 :\
> > -   \
> > -(pci_resource_end((dev), (bar)) -  \
> > - pci_resource_start((dev), (bar)) + 1))
> > +#define pci_resource_start(dev, bar)   (pci_resource_n(dev, 
> > bar)->start)
> > +#define pci_resource_end(dev, bar) (pci_resource_n(dev, bar)->end)
> > +#define pci_resource_flags(dev, bar)   (pci_resource_n(dev, 
> > bar)->flags)
> > +#define pci_resource_len(dev,bar)  \
> > +   (pci_resource_end((dev), (bar)) ?   \
> > +resource_size(pci_resource_n((dev), (bar))) : 0)
> 
> Seems (to me) more logical to have this patch as "PCI: Introduce
> pci_resource_n()" ordered before your patch #2 "PCI: Introduce
> pci_dev_for_each_resource()".

Either way works for me. Bjorn, what do you like?

> Here as #6 or as #2:
> Reviewed-by: Philippe Mathieu-Daudé 

Thank you!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 4/6] EISA: Convert to use less arguments in pci_bus_for_each_resource()

[Andy Shevchenko]

On Fri, Mar 24, 2023 at 10:02:15AM +0100, Philippe Mathieu-Daudé wrote:
> On 23/3/23 18:36, Andy Shevchenko wrote:
> > The pci_bus_for_each_resource() can hide the iterator loop since
> > it may be not used otherwise. With this, we may drop that iterator
> > variable definition.
> > 
> > Signed-off-by: Andy Shevchenko 
> > Reviewed-by: Krzysztof Wilczyński 
> > ---
> >   drivers/eisa/pci_eisa.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/eisa/pci_eisa.c b/drivers/eisa/pci_eisa.c
> 
> Since this is *PCI* EISA, could be squashed into previous patch.

I believe it would be better to have them separated.
But if maintainers want to squash, I can do that.

> Reviewed-by: Philippe Mathieu-Daudé 

Thank you!


-- 
With Best Regards,
Andy Shevchenko

[PATCH v7 3/6] PCI: Allow pci_bus_for_each_resource() to take less arguments

[Andy Shevchenko]

Refactor pci_bus_for_each_resource() in the same way as it's done in
pci_dev_for_each_resource() case. This will allow to hide iterator
inside the loop, where it's not used otherwise.

No functional changes intended.

Signed-off-by: Andy Shevchenko 
Reviewed-by: Krzysztof Wilczyński 
---
 drivers/pci/bus.c  |  7 +++
 drivers/pci/hotplug/shpchp_sysfs.c |  8 
 drivers/pci/pci.c  |  3 +--
 drivers/pci/probe.c|  2 +-
 drivers/pci/setup-bus.c| 10 --
 include/linux/pci.h| 17 +
 6 files changed, 26 insertions(+), 21 deletions(-)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 549c4bd5caec..5bc81cc0a2de 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -182,13 +182,13 @@ static int pci_bus_alloc_from_region(struct pci_bus *bus, 
struct resource *res,
void *alignf_data,
struct pci_bus_region *region)
 {
-   int i, ret;
struct resource *r, avail;
resource_size_t max;
+   int ret;
 
type_mask |= IORESOURCE_TYPE_BITS;
 
-   pci_bus_for_each_resource(bus, r, i) {
+   pci_bus_for_each_resource(bus, r) {
resource_size_t min_used = min;
 
if (!r)
@@ -289,9 +289,8 @@ bool pci_bus_clip_resource(struct pci_dev *dev, int idx)
struct resource *res = >resource[idx];
struct resource orig_res = *res;
struct resource *r;
-   int i;
 
-   pci_bus_for_each_resource(bus, r, i) {
+   pci_bus_for_each_resource(bus, r) {
resource_size_t start, end;
 
if (!r)
diff --git a/drivers/pci/hotplug/shpchp_sysfs.c 
b/drivers/pci/hotplug/shpchp_sysfs.c
index 64beed7a26be..01d47a42da04 100644
--- a/drivers/pci/hotplug/shpchp_sysfs.c
+++ b/drivers/pci/hotplug/shpchp_sysfs.c
@@ -24,16 +24,16 @@
 static ssize_t show_ctrl(struct device *dev, struct device_attribute *attr, 
char *buf)
 {
struct pci_dev *pdev;
-   int index, busnr;
struct resource *res;
struct pci_bus *bus;
size_t len = 0;
+   int busnr;
 
pdev = to_pci_dev(dev);
bus = pdev->subordinate;
 
len += sysfs_emit_at(buf, len, "Free resources: memory\n");
-   pci_bus_for_each_resource(bus, res, index) {
+   pci_bus_for_each_resource(bus, res) {
if (res && (res->flags & IORESOURCE_MEM) &&
!(res->flags & IORESOURCE_PREFETCH)) {
len += sysfs_emit_at(buf, len,
@@ -43,7 +43,7 @@ static ssize_t show_ctrl(struct device *dev, struct 
device_attribute *attr, char
}
}
len += sysfs_emit_at(buf, len, "Free resources: prefetchable memory\n");
-   pci_bus_for_each_resource(bus, res, index) {
+   pci_bus_for_each_resource(bus, res) {
if (res && (res->flags & IORESOURCE_MEM) &&
   (res->flags & IORESOURCE_PREFETCH)) {
len += sysfs_emit_at(buf, len,
@@ -53,7 +53,7 @@ static ssize_t show_ctrl(struct device *dev, struct 
device_attribute *attr, char
}
}
len += sysfs_emit_at(buf, len, "Free resources: IO\n");
-   pci_bus_for_each_resource(bus, res, index) {
+   pci_bus_for_each_resource(bus, res) {
if (res && (res->flags & IORESOURCE_IO)) {
len += sysfs_emit_at(buf, len,
 "start = %8.8llx, length = 
%8.8llx\n",
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 7a67611dc5f4..99299f1299c4 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -779,9 +779,8 @@ struct resource *pci_find_parent_resource(const struct 
pci_dev *dev,
 {
const struct pci_bus *bus = dev->bus;
struct resource *r;
-   int i;
 
-   pci_bus_for_each_resource(bus, r, i) {
+   pci_bus_for_each_resource(bus, r) {
if (!r)
continue;
if (resource_contains(r, res)) {
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index a3f68b6ba6ac..f8191750f6b7 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -533,7 +533,7 @@ void pci_read_bridge_bases(struct pci_bus *child)
pci_read_bridge_mmio_pref(child);
 
if (dev->transparent) {
-   pci_bus_for_each_resource(child->parent, res, i) {
+   pci_bus_for_each_resource(child->parent, res) {
if (res && res->flags) {
pci_bus_add_resource(child, res,
 PCI_SUBTRACTIVE_DECODE);
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 027b985dd1ee..fdeb121e9175 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -770,9 +770,8

[PATCH v7 5/6] pcmcia: Convert to use less arguments in pci_bus_for_each_resource()

[Andy Shevchenko]

The pci_bus_for_each_resource() can hide the iterator loop since
it may be not used otherwise. With this, we may drop that iterator
variable definition.

Signed-off-by: Andy Shevchenko 
Reviewed-by: Krzysztof Wilczyński 
Acked-by: Dominik Brodowski 
---
 drivers/pcmcia/rsrc_nonstatic.c | 9 +++--
 drivers/pcmcia/yenta_socket.c   | 3 +--
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/pcmcia/rsrc_nonstatic.c b/drivers/pcmcia/rsrc_nonstatic.c
index ad1141fddb4c..96264ebee46a 100644
--- a/drivers/pcmcia/rsrc_nonstatic.c
+++ b/drivers/pcmcia/rsrc_nonstatic.c
@@ -934,7 +934,7 @@ static int adjust_io(struct pcmcia_socket *s, unsigned int 
action, unsigned long
 static int nonstatic_autoadd_resources(struct pcmcia_socket *s)
 {
struct resource *res;
-   int i, done = 0;
+   int done = 0;
 
if (!s->cb_dev || !s->cb_dev->bus)
return -ENODEV;
@@ -960,12 +960,9 @@ static int nonstatic_autoadd_resources(struct 
pcmcia_socket *s)
 */
if (s->cb_dev->bus->number == 0)
return -EINVAL;
-
-   for (i = 0; i < PCI_BRIDGE_RESOURCE_NUM; i++) {
-   res = s->cb_dev->bus->resource[i];
-#else
-   pci_bus_for_each_resource(s->cb_dev->bus, res, i) {
 #endif
+
+   pci_bus_for_each_resource(s->cb_dev->bus, res) {
if (!res)
continue;
 
diff --git a/drivers/pcmcia/yenta_socket.c b/drivers/pcmcia/yenta_socket.c
index 1365eaa20ff4..fd18ab571ce8 100644
--- a/drivers/pcmcia/yenta_socket.c
+++ b/drivers/pcmcia/yenta_socket.c
@@ -673,9 +673,8 @@ static int yenta_search_res(struct yenta_socket *socket, 
struct resource *res,
u32 min)
 {
struct resource *root;
-   int i;
 
-   pci_bus_for_each_resource(socket->dev->bus, root, i) {
+   pci_bus_for_each_resource(socket->dev->bus, root) {
if (!root)
continue;
 
-- 
2.40.0.1.gaa8946217a0b

[PATCH v7 6/6] PCI: Make use of pci_resource_n()

[Andy Shevchenko]

Replace open-coded implementations of pci_resource_n() in pci.h.

Signed-off-by: Andy Shevchenko 
---
 include/linux/pci.h | 14 ++
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 70a4684d5f26..9539cf63fe5e 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -2006,14 +2006,12 @@ int pci_iobar_pfn(struct pci_dev *pdev, int bar, struct 
vm_area_struct *vma);
  * for accessing popular PCI BAR info
  */
 #define pci_resource_n(dev, bar)   (&(dev)->resource[(bar)])
-#define pci_resource_start(dev, bar)   ((dev)->resource[(bar)].start)
-#define pci_resource_end(dev, bar) ((dev)->resource[(bar)].end)
-#define pci_resource_flags(dev, bar)   ((dev)->resource[(bar)].flags)
-#define pci_resource_len(dev,bar) \
-   ((pci_resource_end((dev), (bar)) == 0) ? 0 :\
-   \
-(pci_resource_end((dev), (bar)) -  \
- pci_resource_start((dev), (bar)) + 1))
+#define pci_resource_start(dev, bar)   (pci_resource_n(dev, bar)->start)
+#define pci_resource_end(dev, bar) (pci_resource_n(dev, bar)->end)
+#define pci_resource_flags(dev, bar)   (pci_resource_n(dev, bar)->flags)
+#define pci_resource_len(dev,bar)  \
+   (pci_resource_end((dev), (bar)) ?   \
+resource_size(pci_resource_n((dev), (bar))) : 0)
 
 #define __pci_dev_for_each_resource_0(dev, res, ...)   \
for (unsigned int __b = 0;  \
-- 
2.40.0.1.gaa8946217a0b

[PATCH v7 0/6] Add pci_dev_for_each_resource() helper and update users

[Andy Shevchenko]

Provide two new helper macros to iterate over PCI device resources and
convert users.

Looking at it, refactor existing pci_bus_for_each_resource() and convert
users accordingly.

Changelog v7:
- made both macros to share same name (Bjorn)
- split out the pci_resource_n() conversion (Bjorn)

Changelog v6:
- dropped unused variable in PPC code (LKP)

Changelog v5:
- renamed loop variable to minimize the clash (Keith)
- addressed smatch warning (Dan)
- addressed 0-day bot findings (LKP)

Changelog v4:
- rebased on top of v6.3-rc1
- added tag (Krzysztof)

Changelog v3:
- rebased on top of v2 by Mika, see above
- added tag to pcmcia patch (Dominik)

Changelog v2:
- refactor to have two macros
- refactor existing pci_bus_for_each_resource() in the same way and
  convert users

Andy Shevchenko (5):
  kernel.h: Split out COUNT_ARGS() and CONCATENATE()
  PCI: Allow pci_bus_for_each_resource() to take less arguments
  EISA: Convert to use less arguments in pci_bus_for_each_resource()
  pcmcia: Convert to use less arguments in pci_bus_for_each_resource()
  PCI: Make use of pci_resource_n()

Mika Westerberg (1):
  PCI: Introduce pci_dev_for_each_resource()

 .clang-format |  1 +
 arch/alpha/kernel/pci.c   |  5 +--
 arch/arm/kernel/bios32.c  | 16 
 arch/arm/mach-dove/pcie.c | 10 ++---
 arch/arm/mach-mv78xx0/pcie.c  | 10 ++---
 arch/arm/mach-orion5x/pci.c   | 10 ++---
 arch/mips/pci/ops-bcm63xx.c   |  8 ++--
 arch/mips/pci/pci-legacy.c|  3 +-
 arch/powerpc/kernel/pci-common.c  | 21 +-
 arch/powerpc/platforms/4xx/pci.c  |  8 ++--
 arch/powerpc/platforms/52xx/mpc52xx_pci.c |  5 +--
 arch/powerpc/platforms/pseries/pci.c  | 16 
 arch/sh/drivers/pci/pcie-sh7786.c | 10 ++---
 arch/sparc/kernel/leon_pci.c  |  5 +--
 arch/sparc/kernel/pci.c   | 10 ++---
 arch/sparc/kernel/pcic.c  |  5 +--
 drivers/eisa/pci_eisa.c   |  4 +-
 drivers/pci/bus.c |  7 ++--
 drivers/pci/hotplug/shpchp_sysfs.c|  8 ++--
 drivers/pci/pci.c |  3 +-
 drivers/pci/probe.c   |  2 +-
 drivers/pci/remove.c  |  5 +--
 drivers/pci/setup-bus.c   | 37 +++---
 drivers/pci/setup-res.c   |  4 +-
 drivers/pci/vgaarb.c  | 17 +++-
 drivers/pci/xen-pcifront.c|  4 +-
 drivers/pcmcia/rsrc_nonstatic.c   |  9 ++---
 drivers/pcmcia/yenta_socket.c |  3 +-
 drivers/pnp/quirks.c  | 29 +-
 include/linux/args.h  | 13 +++
 include/linux/kernel.h|  8 +---
 include/linux/pci.h   | 47 +--
 32 files changed, 165 insertions(+), 178 deletions(-)
 create mode 100644 include/linux/args.h

-- 
2.40.0.1.gaa8946217a0b

[PATCH v7 1/6] kernel.h: Split out COUNT_ARGS() and CONCATENATE()

[Andy Shevchenko]

kernel.h is being used as a dump for all kinds of stuff for a long time.
The COUNT_ARGS() and CONCATENATE() macros may be used in some places
without need of the full kernel.h dependency train with it.

Here is the attempt on cleaning it up by splitting out these macros().

Signed-off-by: Andy Shevchenko 
---
 include/linux/args.h   | 13 +
 include/linux/kernel.h |  8 +---
 2 files changed, 14 insertions(+), 7 deletions(-)
 create mode 100644 include/linux/args.h

diff --git a/include/linux/args.h b/include/linux/args.h
new file mode 100644
index ..16ef6fad8add
--- /dev/null
+++ b/include/linux/args.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef _LINUX_ARGS_H
+#define _LINUX_ARGS_H
+
+/* This counts to 12. Any more, it will return 13th argument. */
+#define __COUNT_ARGS(_0, _1, _2, _3, _4, _5, _6, _7, _8, _9, _10, _11, _12, 
_n, X...) _n
+#define COUNT_ARGS(X...) __COUNT_ARGS(, ##X, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 
2, 1, 0)
+
+#define __CONCAT(a, b) a ## b
+#define CONCATENATE(a, b) __CONCAT(a, b)
+
+#endif /* _LINUX_ARGS_H */
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 40bce7495af8..c049d3394425 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -13,6 +13,7 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -484,13 +485,6 @@ ftrace_vprintk(const char *fmt, va_list ap)
 static inline void ftrace_dump(enum ftrace_dump_mode oops_dump_mode) { }
 #endif /* CONFIG_TRACING */
 
-/* This counts to 12. Any more, it will return 13th argument. */
-#define __COUNT_ARGS(_0, _1, _2, _3, _4, _5, _6, _7, _8, _9, _10, _11, _12, 
_n, X...) _n
-#define COUNT_ARGS(X...) __COUNT_ARGS(, ##X, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 
2, 1, 0)
-
-#define __CONCAT(a, b) a ## b
-#define CONCATENATE(a, b) __CONCAT(a, b)
-
 /* Rebuild everything on CONFIG_FTRACE_MCOUNT_RECORD */
 #ifdef CONFIG_FTRACE_MCOUNT_RECORD
 # define REBUILD_DUE_TO_FTRACE_MCOUNT_RECORD
-- 
2.40.0.1.gaa8946217a0b

[PATCH v7 4/6] EISA: Convert to use less arguments in pci_bus_for_each_resource()

[Andy Shevchenko]

The pci_bus_for_each_resource() can hide the iterator loop since
it may be not used otherwise. With this, we may drop that iterator
variable definition.

Signed-off-by: Andy Shevchenko 
Reviewed-by: Krzysztof Wilczyński 
---
 drivers/eisa/pci_eisa.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/eisa/pci_eisa.c b/drivers/eisa/pci_eisa.c
index 930c2332c3c4..8173e60bb808 100644
--- a/drivers/eisa/pci_eisa.c
+++ b/drivers/eisa/pci_eisa.c
@@ -20,8 +20,8 @@ static struct eisa_root_device pci_eisa_root;
 
 static int __init pci_eisa_init(struct pci_dev *pdev)
 {
-   int rc, i;
struct resource *res, *bus_res = NULL;
+   int rc;
 
if ((rc = pci_enable_device (pdev))) {
dev_err(>dev, "Could not enable device\n");
@@ -38,7 +38,7 @@ static int __init pci_eisa_init(struct pci_dev *pdev)
 * eisa_root_register() can only deal with a single io port resource,
*  so we use the first valid io port resource.
 */
-   pci_bus_for_each_resource(pdev->bus, res, i)
+   pci_bus_for_each_resource(pdev->bus, res)
if (res && (res->flags & IORESOURCE_IO)) {
bus_res = res;
break;
-- 
2.40.0.1.gaa8946217a0b

[PATCH v7 2/6] PCI: Introduce pci_dev_for_each_resource()

[Andy Shevchenko]

From: Mika Westerberg 

Instead of open-coding it everywhere introduce a tiny helper that can be
used to iterate over each resource of a PCI device, and convert the most
obvious users into it.

While at it drop doubled empty line before pdev_sort_resources().

No functional changes intended.

Suggested-by: Andy Shevchenko 
Signed-off-by: Mika Westerberg 
Signed-off-by: Andy Shevchenko 
Reviewed-by: Krzysztof Wilczyński 
---
 .clang-format |  1 +
 arch/alpha/kernel/pci.c   |  5 ++--
 arch/arm/kernel/bios32.c  | 16 ++---
 arch/arm/mach-dove/pcie.c | 10 
 arch/arm/mach-mv78xx0/pcie.c  | 10 
 arch/arm/mach-orion5x/pci.c   | 10 
 arch/mips/pci/ops-bcm63xx.c   |  8 +++
 arch/mips/pci/pci-legacy.c|  3 +--
 arch/powerpc/kernel/pci-common.c  | 21 
 arch/powerpc/platforms/4xx/pci.c  |  8 +++
 arch/powerpc/platforms/52xx/mpc52xx_pci.c |  5 ++--
 arch/powerpc/platforms/pseries/pci.c  | 16 ++---
 arch/sh/drivers/pci/pcie-sh7786.c | 10 
 arch/sparc/kernel/leon_pci.c  |  5 ++--
 arch/sparc/kernel/pci.c   | 10 
 arch/sparc/kernel/pcic.c  |  5 ++--
 drivers/pci/remove.c  |  5 ++--
 drivers/pci/setup-bus.c   | 27 -
 drivers/pci/setup-res.c   |  4 +---
 drivers/pci/vgaarb.c  | 17 -
 drivers/pci/xen-pcifront.c|  4 +---
 drivers/pnp/quirks.c  | 29 ---
 include/linux/pci.h   | 16 +
 23 files changed, 113 insertions(+), 132 deletions(-)

diff --git a/.clang-format b/.clang-format
index d988e9fa9b26..2048b0296d76 100644
--- a/.clang-format
+++ b/.clang-format
@@ -520,6 +520,7 @@ ForEachMacros:
   - 'of_property_for_each_string'
   - 'of_property_for_each_u32'
   - 'pci_bus_for_each_resource'
+  - 'pci_dev_for_each_resource'
   - 'pci_doe_for_each_off'
   - 'pcl_for_each_chunk'
   - 'pcl_for_each_segment'
diff --git a/arch/alpha/kernel/pci.c b/arch/alpha/kernel/pci.c
index 64fbfb0763b2..4458eb7f44f0 100644
--- a/arch/alpha/kernel/pci.c
+++ b/arch/alpha/kernel/pci.c
@@ -288,11 +288,10 @@ pcibios_claim_one_bus(struct pci_bus *b)
struct pci_bus *child_bus;
 
list_for_each_entry(dev, >devices, bus_list) {
+   struct resource *r;
int i;
 
-   for (i = 0; i < PCI_NUM_RESOURCES; i++) {
-   struct resource *r = >resource[i];
-
+   pci_dev_for_each_resource(dev, r, i) {
if (r->parent || !r->start || !r->flags)
continue;
if (pci_has_flag(PCI_PROBE_ONLY) ||
diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index e7ef2b5bea9c..d334c7fb672b 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -142,15 +142,15 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_WINBOND2, 
PCI_DEVICE_ID_WINBOND2_89C940F,
  */
 static void pci_fixup_dec21285(struct pci_dev *dev)
 {
-   int i;
-
if (dev->devfn == 0) {
+   struct resource *r;
+
dev->class &= 0xff;
dev->class |= PCI_CLASS_BRIDGE_HOST << 8;
-   for (i = 0; i < PCI_NUM_RESOURCES; i++) {
-   dev->resource[i].start = 0;
-   dev->resource[i].end   = 0;
-   dev->resource[i].flags = 0;
+   pci_dev_for_each_resource(dev, r) {
+   r->start = 0;
+   r->end = 0;
+   r->flags = 0;
}
}
 }
@@ -162,13 +162,11 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_DEC, 
PCI_DEVICE_ID_DEC_21285, pci_fixup_d
 static void pci_fixup_ide_bases(struct pci_dev *dev)
 {
struct resource *r;
-   int i;
 
if ((dev->class >> 8) != PCI_CLASS_STORAGE_IDE)
return;
 
-   for (i = 0; i < PCI_NUM_RESOURCES; i++) {
-   r = dev->resource + i;
+   pci_dev_for_each_resource(dev, r) {
if ((r->start & ~0x80) == 0x374) {
r->start |= 2;
r->end = r->start;
diff --git a/arch/arm/mach-dove/pcie.c b/arch/arm/mach-dove/pcie.c
index 754ca381f600..3044b7e03890 100644
--- a/arch/arm/mach-dove/pcie.c
+++ b/arch/arm/mach-dove/pcie.c
@@ -142,14 +142,14 @@ static struct pci_ops pcie_ops = {
 static void rc_pci_fixup(struct pci_dev *dev)
 {
if (dev->bus->parent == NULL && dev->devfn == 0) {
-   int i;
+   struct resource *r;
 
dev->class &= 0xff;
dev->class |= PCI_CLASS_BRIDGE_HOST &l

Re: [PATCH v6 1/4] PCI: Introduce pci_dev_for_each_resource()

[Andy Shevchenko]

On Thu, Mar 23, 2023 at 10:02:38AM -0500, Bjorn Helgaas wrote:
> On Thu, Mar 23, 2023 at 04:30:01PM +0200, Andy Shevchenko wrote:

...

> I poked around looking for similar patterns elsewhere with:
> 
>   git grep "#define.*for_each_.*_p("
>   git grep "#define.*for_each_.*_idx("
> 
> I didn't find any other "_p" iterators and just a few "_idx" ones, so
> my hope is to follow what little precedent there is, as well as
> converge on the basic "*_for_each_resource()" iterators and remove the
> "_idx()" versions over time by doing things like the
> pci_claim_resource() change.

The p is heavily used in the byte order conversion helpers.

> What do you think?  If it seems like excessive churn, we can do it
> as-is and still try to reduce the use of the index variable over time.

I think _p has a precedent as well. But I can think about it a bit, maybe
we can come up with something smarter.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 1/4] PCI: Introduce pci_dev_for_each_resource()

[Andy Shevchenko]

On Wed, Mar 22, 2023 at 02:28:04PM -0500, Bjorn Helgaas wrote:
> On Mon, Mar 20, 2023 at 03:16:30PM +0200, Andy Shevchenko wrote:

...

> > +   pci_dev_for_each_resource_p(dev, r) {
> > /* zap the 2nd function of the winbond chip */
> > -   if (dev->resource[i].flags & IORESOURCE_IO
> > -   && dev->bus->number == 0 && dev->devfn == 0x81)
> > -   dev->resource[i].flags &= ~IORESOURCE_IO;
> > -   if (dev->resource[i].start == 0 && dev->resource[i].end) {
> > -   dev->resource[i].flags = 0;
> > -   dev->resource[i].end = 0;
> > +   if (dev->bus->number == 0 && dev->devfn == 0x81 &&
> > +   r->flags & IORESOURCE_IO)
> 
> This is a nice literal conversion, but it's kind of lame to test
> bus->number and devfn *inside* the loop here, since they can't change
> inside the loop.

Hmm... why are you asking me, even if I may agree on that? It's
in the original code and out of scope of this series.

> > +   r->flags &= ~IORESOURCE_IO;
> > +   if (r->start == 0 && r->end) {
> > +   r->flags = 0;
> > +   r->end = 0;
> > }
> > }

...

> >  #define pci_resource_len(dev,bar) \
> > ((pci_resource_end((dev), (bar)) == 0) ? 0 :\
> > \
> > -(pci_resource_end((dev), (bar)) -  \
> > - pci_resource_start((dev), (bar)) + 1))
> > +resource_size(pci_resource_n((dev), (bar
> 
> I like this change, but it's unrelated to pci_dev_for_each_resource()
> and unmentioned in the commit log.

And as you rightfully noticed this either. I can split it to a separate one.

...

> > +#define __pci_dev_for_each_resource(dev, res, __i, vartype)
> > \
> > +   for (vartype __i = 0;   \
> > +res = pci_resource_n(dev, __i), __i < PCI_NUM_RESOURCES;   \
> > +__i++)
> > +
> > +#define pci_dev_for_each_resource(dev, res, i) 
> > \
> > +   __pci_dev_for_each_resource(dev, res, i, )
> > +
> > +#define pci_dev_for_each_resource_p(dev, res)  
> > \
> > +   __pci_dev_for_each_resource(dev, res, __i, unsigned int)
> 
> This series converts many cases to drop the iterator variable ("i"),
> which is fantastic.
> 
> Several of the remaining places need the iterator variable only to
> call pci_claim_resource(), which could be converted to take a "struct
> resource *" directly without much trouble.
> 
> We don't have to do that pci_claim_resource() conversion now,

Exactly, it's definitely should be separate change.

> but
> since we're converging on the "(dev, res)" style, I think we should
> reverse the names so we have something like:
> 
>   pci_dev_for_each_resource(dev, res)
>   pci_dev_for_each_resource_idx(dev, res, i)

Wouldn't it be more churn, including pci_bus_for_each_resource() correction?

...

> Not sure __pci_dev_for_each_resource() is worthwhile since it only
> avoids repeating that single "for" statement, and passing in "vartype"
> (sometimes empty to implicitly avoid the declaration) is a little
> complicated to read.  I think it'd be easier to read like this:

No objections here.

>   #define pci_dev_for_each_resource(dev, res)  \
> for (unsigned int __i = 0; \
>  res = pci_resource_n(dev, __i), __i < PCI_NUM_RESOURCES;  \
>  __i++)
> 
>   #define pci_dev_for_each_resource_idx(dev, res, idx) \
> for (idx = 0;  \
>  res = pci_resource_n(dev, idx), idx < PCI_NUM_RESOURCES;  \
>  idx++)

-- 
With Best Regards,
Andy Shevchenko

[PATCH v6 0/4] Add pci_dev_for_each_resource() helper and update users

[Andy Shevchenko]

Provide two new helper macros to iterate over PCI device resources and
convert users.

Looking at it, refactor existing pci_bus_for_each_resource() and convert
users accordingly.

Changelog v6:
- dropped unused variable in PPC code (LKP)

Changelog v5:
- renamed loop variable to minimize the clash (Keith)
- addressed smatch warning (Dan)
- addressed 0-day bot findings (LKP)

Changelog v4:
- rebased on top of v6.3-rc1
- added tag (Krzysztof)

Changelog v3:
- rebased on top of v2 by Mika, see above
- added tag to pcmcia patch (Dominik)

Changelog v2:
- refactor to have two macros
- refactor existing pci_bus_for_each_resource() in the same way and
  convert users

Andy Shevchenko (3):
  PCI: Split pci_bus_for_each_resource_p() out of
pci_bus_for_each_resource()
  EISA: Convert to use pci_bus_for_each_resource_p()
  pcmcia: Convert to use pci_bus_for_each_resource_p()

Mika Westerberg (1):
  PCI: Introduce pci_dev_for_each_resource()

 .clang-format |  3 ++
 arch/alpha/kernel/pci.c   |  5 ++-
 arch/arm/kernel/bios32.c  | 16 +-
 arch/arm/mach-dove/pcie.c | 10 +++---
 arch/arm/mach-mv78xx0/pcie.c  | 10 +++---
 arch/arm/mach-orion5x/pci.c   | 10 +++---
 arch/mips/pci/ops-bcm63xx.c   |  8 ++---
 arch/mips/pci/pci-legacy.c|  3 +-
 arch/powerpc/kernel/pci-common.c  | 21 +++--
 arch/powerpc/platforms/4xx/pci.c  |  8 ++---
 arch/powerpc/platforms/52xx/mpc52xx_pci.c |  5 ++-
 arch/powerpc/platforms/pseries/pci.c  | 16 +-
 arch/sh/drivers/pci/pcie-sh7786.c | 10 +++---
 arch/sparc/kernel/leon_pci.c  |  5 ++-
 arch/sparc/kernel/pci.c   | 10 +++---
 arch/sparc/kernel/pcic.c  |  5 ++-
 drivers/eisa/pci_eisa.c   |  4 +--
 drivers/pci/bus.c |  7 ++---
 drivers/pci/hotplug/shpchp_sysfs.c|  8 ++---
 drivers/pci/pci.c |  5 ++-
 drivers/pci/probe.c   |  2 +-
 drivers/pci/remove.c  |  5 ++-
 drivers/pci/setup-bus.c   | 37 +--
 drivers/pci/setup-res.c   |  4 +--
 drivers/pci/vgaarb.c  | 17 +++
 drivers/pci/xen-pcifront.c|  4 +--
 drivers/pcmcia/rsrc_nonstatic.c   |  9 ++
 drivers/pcmcia/yenta_socket.c |  3 +-
 drivers/pnp/quirks.c  | 29 ++
 include/linux/pci.h   | 29 ++
 30 files changed, 142 insertions(+), 166 deletions(-)

-- 
2.39.2

[PATCH v6 3/4] EISA: Convert to use pci_bus_for_each_resource_p()

[Andy Shevchenko]

The pci_bus_for_each_resource_p() hides the iterator loop since
it may be not used otherwise. With this, we may drop that iterator
variable definition.

Signed-off-by: Andy Shevchenko 
Reviewed-by: Krzysztof Wilczyński 
---
 drivers/eisa/pci_eisa.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/eisa/pci_eisa.c b/drivers/eisa/pci_eisa.c
index 930c2332c3c4..907b86384396 100644
--- a/drivers/eisa/pci_eisa.c
+++ b/drivers/eisa/pci_eisa.c
@@ -20,8 +20,8 @@ static struct eisa_root_device pci_eisa_root;
 
 static int __init pci_eisa_init(struct pci_dev *pdev)
 {
-   int rc, i;
struct resource *res, *bus_res = NULL;
+   int rc;
 
if ((rc = pci_enable_device (pdev))) {
dev_err(>dev, "Could not enable device\n");
@@ -38,7 +38,7 @@ static int __init pci_eisa_init(struct pci_dev *pdev)
 * eisa_root_register() can only deal with a single io port resource,
*  so we use the first valid io port resource.
 */
-   pci_bus_for_each_resource(pdev->bus, res, i)
+   pci_bus_for_each_resource_p(pdev->bus, res)
if (res && (res->flags & IORESOURCE_IO)) {
bus_res = res;
break;
-- 
2.39.2

[PATCH v6 4/4] pcmcia: Convert to use pci_bus_for_each_resource_p()

[Andy Shevchenko]

The pci_bus_for_each_resource_p() hides the iterator loop since
it may be not used otherwise. With this, we may drop that iterator
variable definition.

Signed-off-by: Andy Shevchenko 
Reviewed-by: Krzysztof Wilczyński 
Acked-by: Dominik Brodowski 
---
 drivers/pcmcia/rsrc_nonstatic.c | 9 +++--
 drivers/pcmcia/yenta_socket.c   | 3 +--
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/pcmcia/rsrc_nonstatic.c b/drivers/pcmcia/rsrc_nonstatic.c
index ad1141fddb4c..9d92d4bb6239 100644
--- a/drivers/pcmcia/rsrc_nonstatic.c
+++ b/drivers/pcmcia/rsrc_nonstatic.c
@@ -934,7 +934,7 @@ static int adjust_io(struct pcmcia_socket *s, unsigned int 
action, unsigned long
 static int nonstatic_autoadd_resources(struct pcmcia_socket *s)
 {
struct resource *res;
-   int i, done = 0;
+   int done = 0;
 
if (!s->cb_dev || !s->cb_dev->bus)
return -ENODEV;
@@ -960,12 +960,9 @@ static int nonstatic_autoadd_resources(struct 
pcmcia_socket *s)
 */
if (s->cb_dev->bus->number == 0)
return -EINVAL;
-
-   for (i = 0; i < PCI_BRIDGE_RESOURCE_NUM; i++) {
-   res = s->cb_dev->bus->resource[i];
-#else
-   pci_bus_for_each_resource(s->cb_dev->bus, res, i) {
 #endif
+
+   pci_bus_for_each_resource_p(s->cb_dev->bus, res) {
if (!res)
continue;
 
diff --git a/drivers/pcmcia/yenta_socket.c b/drivers/pcmcia/yenta_socket.c
index 1365eaa20ff4..2e5bdf3db0ba 100644
--- a/drivers/pcmcia/yenta_socket.c
+++ b/drivers/pcmcia/yenta_socket.c
@@ -673,9 +673,8 @@ static int yenta_search_res(struct yenta_socket *socket, 
struct resource *res,
u32 min)
 {
struct resource *root;
-   int i;
 
-   pci_bus_for_each_resource(socket->dev->bus, root, i) {
+   pci_bus_for_each_resource_p(socket->dev->bus, root) {
if (!root)
continue;
 
-- 
2.39.2

[PATCH v6 2/4] PCI: Split pci_bus_for_each_resource_p() out of pci_bus_for_each_resource()

[Andy Shevchenko]

Refactor pci_bus_for_each_resource() in the same way as it's done in
pci_dev_for_each_resource() case. This will allow to hide iterator
inside the loop, where it's not used otherwise.

No functional changes intended.

Signed-off-by: Andy Shevchenko 
Reviewed-by: Krzysztof Wilczyński 
---
 .clang-format  |  1 +
 drivers/pci/bus.c  |  7 +++
 drivers/pci/hotplug/shpchp_sysfs.c |  8 
 drivers/pci/pci.c  |  5 ++---
 drivers/pci/probe.c|  2 +-
 drivers/pci/setup-bus.c| 10 --
 include/linux/pci.h| 14 ++
 7 files changed, 25 insertions(+), 22 deletions(-)

diff --git a/.clang-format b/.clang-format
index 266abb843654..81c9f055086f 100644
--- a/.clang-format
+++ b/.clang-format
@@ -520,6 +520,7 @@ ForEachMacros:
   - 'of_property_for_each_string'
   - 'of_property_for_each_u32'
   - 'pci_bus_for_each_resource'
+  - 'pci_bus_for_each_resource_p'
   - 'pci_dev_for_each_resource'
   - 'pci_dev_for_each_resource_p'
   - 'pci_doe_for_each_off'
diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 549c4bd5caec..b0789d332d36 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -182,13 +182,13 @@ static int pci_bus_alloc_from_region(struct pci_bus *bus, 
struct resource *res,
void *alignf_data,
struct pci_bus_region *region)
 {
-   int i, ret;
struct resource *r, avail;
resource_size_t max;
+   int ret;
 
type_mask |= IORESOURCE_TYPE_BITS;
 
-   pci_bus_for_each_resource(bus, r, i) {
+   pci_bus_for_each_resource_p(bus, r) {
resource_size_t min_used = min;
 
if (!r)
@@ -289,9 +289,8 @@ bool pci_bus_clip_resource(struct pci_dev *dev, int idx)
struct resource *res = >resource[idx];
struct resource orig_res = *res;
struct resource *r;
-   int i;
 
-   pci_bus_for_each_resource(bus, r, i) {
+   pci_bus_for_each_resource_p(bus, r) {
resource_size_t start, end;
 
if (!r)
diff --git a/drivers/pci/hotplug/shpchp_sysfs.c 
b/drivers/pci/hotplug/shpchp_sysfs.c
index 64beed7a26be..ff04f0c5e7c3 100644
--- a/drivers/pci/hotplug/shpchp_sysfs.c
+++ b/drivers/pci/hotplug/shpchp_sysfs.c
@@ -24,16 +24,16 @@
 static ssize_t show_ctrl(struct device *dev, struct device_attribute *attr, 
char *buf)
 {
struct pci_dev *pdev;
-   int index, busnr;
struct resource *res;
struct pci_bus *bus;
size_t len = 0;
+   int busnr;
 
pdev = to_pci_dev(dev);
bus = pdev->subordinate;
 
len += sysfs_emit_at(buf, len, "Free resources: memory\n");
-   pci_bus_for_each_resource(bus, res, index) {
+   pci_bus_for_each_resource_p(bus, res) {
if (res && (res->flags & IORESOURCE_MEM) &&
!(res->flags & IORESOURCE_PREFETCH)) {
len += sysfs_emit_at(buf, len,
@@ -43,7 +43,7 @@ static ssize_t show_ctrl(struct device *dev, struct 
device_attribute *attr, char
}
}
len += sysfs_emit_at(buf, len, "Free resources: prefetchable memory\n");
-   pci_bus_for_each_resource(bus, res, index) {
+   pci_bus_for_each_resource_p(bus, res) {
if (res && (res->flags & IORESOURCE_MEM) &&
   (res->flags & IORESOURCE_PREFETCH)) {
len += sysfs_emit_at(buf, len,
@@ -53,7 +53,7 @@ static ssize_t show_ctrl(struct device *dev, struct 
device_attribute *attr, char
}
}
len += sysfs_emit_at(buf, len, "Free resources: IO\n");
-   pci_bus_for_each_resource(bus, res, index) {
+   pci_bus_for_each_resource_p(bus, res) {
if (res && (res->flags & IORESOURCE_IO)) {
len += sysfs_emit_at(buf, len,
 "start = %8.8llx, length = 
%8.8llx\n",
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 7a67611dc5f4..2f8915ab41ef 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -779,9 +779,8 @@ struct resource *pci_find_parent_resource(const struct 
pci_dev *dev,
 {
const struct pci_bus *bus = dev->bus;
struct resource *r;
-   int i;
 
-   pci_bus_for_each_resource(bus, r, i) {
+   pci_bus_for_each_resource_p(bus, r) {
if (!r)
continue;
if (resource_contains(r, res)) {
@@ -799,7 +798,7 @@ struct resource *pci_find_parent_resource(const struct 
pci_dev *dev,
 * be both a positively-decoded aperture and a
 * subtractively-decoded region that contain the BAR.
 * We want the positively-decoded one, so this depends
-* on pci_bus_for_each_resource() 

[PATCH v6 1/4] PCI: Introduce pci_dev_for_each_resource()

[Andy Shevchenko]

From: Mika Westerberg 

Instead of open-coding it everywhere introduce a tiny helper that can be
used to iterate over each resource of a PCI device, and convert the most
obvious users into it.

While at it drop doubled empty line before pdev_sort_resources().

No functional changes intended.

Suggested-by: Andy Shevchenko 
Signed-off-by: Mika Westerberg 
Signed-off-by: Andy Shevchenko 
Reviewed-by: Krzysztof Wilczyński 
---
 .clang-format |  2 ++
 arch/alpha/kernel/pci.c   |  5 ++--
 arch/arm/kernel/bios32.c  | 16 ++---
 arch/arm/mach-dove/pcie.c | 10 
 arch/arm/mach-mv78xx0/pcie.c  | 10 
 arch/arm/mach-orion5x/pci.c   | 10 
 arch/mips/pci/ops-bcm63xx.c   |  8 +++
 arch/mips/pci/pci-legacy.c|  3 +--
 arch/powerpc/kernel/pci-common.c  | 21 
 arch/powerpc/platforms/4xx/pci.c  |  8 +++
 arch/powerpc/platforms/52xx/mpc52xx_pci.c |  5 ++--
 arch/powerpc/platforms/pseries/pci.c  | 16 ++---
 arch/sh/drivers/pci/pcie-sh7786.c | 10 
 arch/sparc/kernel/leon_pci.c  |  5 ++--
 arch/sparc/kernel/pci.c   | 10 
 arch/sparc/kernel/pcic.c  |  5 ++--
 drivers/pci/remove.c  |  5 ++--
 drivers/pci/setup-bus.c   | 27 -
 drivers/pci/setup-res.c   |  4 +---
 drivers/pci/vgaarb.c  | 17 -
 drivers/pci/xen-pcifront.c|  4 +---
 drivers/pnp/quirks.c  | 29 ---
 include/linux/pci.h   | 15 ++--
 23 files changed, 111 insertions(+), 134 deletions(-)

diff --git a/.clang-format b/.clang-format
index d988e9fa9b26..266abb843654 100644
--- a/.clang-format
+++ b/.clang-format
@@ -520,6 +520,8 @@ ForEachMacros:
   - 'of_property_for_each_string'
   - 'of_property_for_each_u32'
   - 'pci_bus_for_each_resource'
+  - 'pci_dev_for_each_resource'
+  - 'pci_dev_for_each_resource_p'
   - 'pci_doe_for_each_off'
   - 'pcl_for_each_chunk'
   - 'pcl_for_each_segment'
diff --git a/arch/alpha/kernel/pci.c b/arch/alpha/kernel/pci.c
index 64fbfb0763b2..4458eb7f44f0 100644
--- a/arch/alpha/kernel/pci.c
+++ b/arch/alpha/kernel/pci.c
@@ -288,11 +288,10 @@ pcibios_claim_one_bus(struct pci_bus *b)
struct pci_bus *child_bus;
 
list_for_each_entry(dev, >devices, bus_list) {
+   struct resource *r;
int i;
 
-   for (i = 0; i < PCI_NUM_RESOURCES; i++) {
-   struct resource *r = >resource[i];
-
+   pci_dev_for_each_resource(dev, r, i) {
if (r->parent || !r->start || !r->flags)
continue;
if (pci_has_flag(PCI_PROBE_ONLY) ||
diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index e7ef2b5bea9c..5254734b23e6 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -142,15 +142,15 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_WINBOND2, 
PCI_DEVICE_ID_WINBOND2_89C940F,
  */
 static void pci_fixup_dec21285(struct pci_dev *dev)
 {
-   int i;
-
if (dev->devfn == 0) {
+   struct resource *r;
+
dev->class &= 0xff;
dev->class |= PCI_CLASS_BRIDGE_HOST << 8;
-   for (i = 0; i < PCI_NUM_RESOURCES; i++) {
-   dev->resource[i].start = 0;
-   dev->resource[i].end   = 0;
-   dev->resource[i].flags = 0;
+   pci_dev_for_each_resource_p(dev, r) {
+   r->start = 0;
+   r->end = 0;
+   r->flags = 0;
}
}
 }
@@ -162,13 +162,11 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_DEC, 
PCI_DEVICE_ID_DEC_21285, pci_fixup_d
 static void pci_fixup_ide_bases(struct pci_dev *dev)
 {
struct resource *r;
-   int i;
 
if ((dev->class >> 8) != PCI_CLASS_STORAGE_IDE)
return;
 
-   for (i = 0; i < PCI_NUM_RESOURCES; i++) {
-   r = dev->resource + i;
+   pci_dev_for_each_resource_p(dev, r) {
if ((r->start & ~0x80) == 0x374) {
r->start |= 2;
r->end = r->start;
diff --git a/arch/arm/mach-dove/pcie.c b/arch/arm/mach-dove/pcie.c
index 754ca381f600..58cecd79a204 100644
--- a/arch/arm/mach-dove/pcie.c
+++ b/arch/arm/mach-dove/pcie.c
@@ -142,14 +142,14 @@ static struct pci_ops pcie_ops = {
 static void rc_pci_fixup(struct pci_dev *dev)
 {
if (dev->bus->parent == NULL && dev->devfn == 0) {
-   int i;
+   struct resource *r;
 
dev->class &= 0xff;
dev->class |= P

[PATCH v5 1/4] PCI: Introduce pci_dev_for_each_resource()

[Andy Shevchenko]

From: Mika Westerberg 

Instead of open-coding it everywhere introduce a tiny helper that can be
used to iterate over each resource of a PCI device, and convert the most
obvious users into it.

While at it drop doubled empty line before pdev_sort_resources().

No functional changes intended.

Suggested-by: Andy Shevchenko 
Signed-off-by: Mika Westerberg 
Signed-off-by: Andy Shevchenko 
Reviewed-by: Krzysztof Wilczyński 
---
 .clang-format |  2 ++
 arch/alpha/kernel/pci.c   |  5 ++--
 arch/arm/kernel/bios32.c  | 16 ++---
 arch/arm/mach-dove/pcie.c | 10 
 arch/arm/mach-mv78xx0/pcie.c  | 10 
 arch/arm/mach-orion5x/pci.c   | 10 
 arch/mips/pci/ops-bcm63xx.c   |  8 +++
 arch/mips/pci/pci-legacy.c|  3 +--
 arch/powerpc/kernel/pci-common.c  | 21 
 arch/powerpc/platforms/4xx/pci.c  |  8 +++
 arch/powerpc/platforms/52xx/mpc52xx_pci.c |  4 ++--
 arch/powerpc/platforms/pseries/pci.c  | 16 ++---
 arch/sh/drivers/pci/pcie-sh7786.c | 10 
 arch/sparc/kernel/leon_pci.c  |  5 ++--
 arch/sparc/kernel/pci.c   | 10 
 arch/sparc/kernel/pcic.c  |  5 ++--
 drivers/pci/remove.c  |  5 ++--
 drivers/pci/setup-bus.c   | 27 -
 drivers/pci/setup-res.c   |  4 +---
 drivers/pci/vgaarb.c  | 17 -
 drivers/pci/xen-pcifront.c|  4 +---
 drivers/pnp/quirks.c  | 29 ---
 include/linux/pci.h   | 15 ++--
 23 files changed, 111 insertions(+), 133 deletions(-)

diff --git a/.clang-format b/.clang-format
index d988e9fa9b26..266abb843654 100644
--- a/.clang-format
+++ b/.clang-format
@@ -520,6 +520,8 @@ ForEachMacros:
   - 'of_property_for_each_string'
   - 'of_property_for_each_u32'
   - 'pci_bus_for_each_resource'
+  - 'pci_dev_for_each_resource'
+  - 'pci_dev_for_each_resource_p'
   - 'pci_doe_for_each_off'
   - 'pcl_for_each_chunk'
   - 'pcl_for_each_segment'
diff --git a/arch/alpha/kernel/pci.c b/arch/alpha/kernel/pci.c
index 64fbfb0763b2..4458eb7f44f0 100644
--- a/arch/alpha/kernel/pci.c
+++ b/arch/alpha/kernel/pci.c
@@ -288,11 +288,10 @@ pcibios_claim_one_bus(struct pci_bus *b)
struct pci_bus *child_bus;
 
list_for_each_entry(dev, >devices, bus_list) {
+   struct resource *r;
int i;
 
-   for (i = 0; i < PCI_NUM_RESOURCES; i++) {
-   struct resource *r = >resource[i];
-
+   pci_dev_for_each_resource(dev, r, i) {
if (r->parent || !r->start || !r->flags)
continue;
if (pci_has_flag(PCI_PROBE_ONLY) ||
diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index e7ef2b5bea9c..5254734b23e6 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -142,15 +142,15 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_WINBOND2, 
PCI_DEVICE_ID_WINBOND2_89C940F,
  */
 static void pci_fixup_dec21285(struct pci_dev *dev)
 {
-   int i;
-
if (dev->devfn == 0) {
+   struct resource *r;
+
dev->class &= 0xff;
dev->class |= PCI_CLASS_BRIDGE_HOST << 8;
-   for (i = 0; i < PCI_NUM_RESOURCES; i++) {
-   dev->resource[i].start = 0;
-   dev->resource[i].end   = 0;
-   dev->resource[i].flags = 0;
+   pci_dev_for_each_resource_p(dev, r) {
+   r->start = 0;
+   r->end = 0;
+   r->flags = 0;
}
}
 }
@@ -162,13 +162,11 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_DEC, 
PCI_DEVICE_ID_DEC_21285, pci_fixup_d
 static void pci_fixup_ide_bases(struct pci_dev *dev)
 {
struct resource *r;
-   int i;
 
if ((dev->class >> 8) != PCI_CLASS_STORAGE_IDE)
return;
 
-   for (i = 0; i < PCI_NUM_RESOURCES; i++) {
-   r = dev->resource + i;
+   pci_dev_for_each_resource_p(dev, r) {
if ((r->start & ~0x80) == 0x374) {
r->start |= 2;
r->end = r->start;
diff --git a/arch/arm/mach-dove/pcie.c b/arch/arm/mach-dove/pcie.c
index 754ca381f600..58cecd79a204 100644
--- a/arch/arm/mach-dove/pcie.c
+++ b/arch/arm/mach-dove/pcie.c
@@ -142,14 +142,14 @@ static struct pci_ops pcie_ops = {
 static void rc_pci_fixup(struct pci_dev *dev)
 {
if (dev->bus->parent == NULL && dev->devfn == 0) {
-   int i;
+   struct resource *r;
 
dev->class &= 0xff;
dev->class |= P

[PATCH v5 0/4] PCI: Add pci_dev_for_each_resource() helper and update users

[Andy Shevchenko]

Provide two new helper macros to iterate over PCI device resources and
convert users.

Looking at it, refactor existing pci_bus_for_each_resource() and convert
users accordingly.

Changelog v5:
- renamed loop variable to minimize the clash (Keith)
- addressed smatch warning (Dan)
- addressed 0-day bot findings (LKP)

Changelog v4:
- rebased on top of v6.3-rc1
- added tag (Krzysztof)

Changelog v3:
- rebased on top of v2 by Mika, see above
- added tag to pcmcia patch (Dominik)

Changelog v2:
- refactor to have two macros
- refactor existing pci_bus_for_each_resource() in the same way and
  convert users

Andy Shevchenko (3):
  PCI: Split pci_bus_for_each_resource_p() out of
pci_bus_for_each_resource()
  EISA: Convert to use pci_bus_for_each_resource_p()
  pcmcia: Convert to use pci_bus_for_each_resource_p()

Mika Westerberg (1):
  PCI: Introduce pci_dev_for_each_resource()

 .clang-format |  3 ++
 arch/alpha/kernel/pci.c   |  5 ++-
 arch/arm/kernel/bios32.c  | 16 +-
 arch/arm/mach-dove/pcie.c | 10 +++---
 arch/arm/mach-mv78xx0/pcie.c  | 10 +++---
 arch/arm/mach-orion5x/pci.c   | 10 +++---
 arch/mips/pci/ops-bcm63xx.c   |  8 ++---
 arch/mips/pci/pci-legacy.c|  3 +-
 arch/powerpc/kernel/pci-common.c  | 21 +++--
 arch/powerpc/platforms/4xx/pci.c  |  8 ++---
 arch/powerpc/platforms/52xx/mpc52xx_pci.c |  4 +--
 arch/powerpc/platforms/pseries/pci.c  | 16 +-
 arch/sh/drivers/pci/pcie-sh7786.c | 10 +++---
 arch/sparc/kernel/leon_pci.c  |  5 ++-
 arch/sparc/kernel/pci.c   | 10 +++---
 arch/sparc/kernel/pcic.c  |  5 ++-
 drivers/eisa/pci_eisa.c   |  4 +--
 drivers/pci/bus.c |  7 ++---
 drivers/pci/hotplug/shpchp_sysfs.c|  8 ++---
 drivers/pci/pci.c |  5 ++-
 drivers/pci/probe.c   |  2 +-
 drivers/pci/remove.c  |  5 ++-
 drivers/pci/setup-bus.c   | 37 +--
 drivers/pci/setup-res.c   |  4 +--
 drivers/pci/vgaarb.c  | 17 +++
 drivers/pci/xen-pcifront.c|  4 +--
 drivers/pcmcia/rsrc_nonstatic.c   |  9 ++
 drivers/pcmcia/yenta_socket.c |  3 +-
 drivers/pnp/quirks.c  | 29 ++
 include/linux/pci.h   | 29 ++
 30 files changed, 142 insertions(+), 165 deletions(-)

-- 
2.39.2

[PATCH v5 4/4] pcmcia: Convert to use pci_bus_for_each_resource_p()

[Andy Shevchenko]

The pci_bus_for_each_resource_p() hides the iterator loop since
it may be not used otherwise. With this, we may drop that iterator
variable definition.

Signed-off-by: Andy Shevchenko 
Reviewed-by: Krzysztof Wilczyński 
Acked-by: Dominik Brodowski 
---
 drivers/pcmcia/rsrc_nonstatic.c | 9 +++--
 drivers/pcmcia/yenta_socket.c   | 3 +--
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/pcmcia/rsrc_nonstatic.c b/drivers/pcmcia/rsrc_nonstatic.c
index ad1141fddb4c..9d92d4bb6239 100644
--- a/drivers/pcmcia/rsrc_nonstatic.c
+++ b/drivers/pcmcia/rsrc_nonstatic.c
@@ -934,7 +934,7 @@ static int adjust_io(struct pcmcia_socket *s, unsigned int 
action, unsigned long
 static int nonstatic_autoadd_resources(struct pcmcia_socket *s)
 {
struct resource *res;
-   int i, done = 0;
+   int done = 0;
 
if (!s->cb_dev || !s->cb_dev->bus)
return -ENODEV;
@@ -960,12 +960,9 @@ static int nonstatic_autoadd_resources(struct 
pcmcia_socket *s)
 */
if (s->cb_dev->bus->number == 0)
return -EINVAL;
-
-   for (i = 0; i < PCI_BRIDGE_RESOURCE_NUM; i++) {
-   res = s->cb_dev->bus->resource[i];
-#else
-   pci_bus_for_each_resource(s->cb_dev->bus, res, i) {
 #endif
+
+   pci_bus_for_each_resource_p(s->cb_dev->bus, res) {
if (!res)
continue;
 
diff --git a/drivers/pcmcia/yenta_socket.c b/drivers/pcmcia/yenta_socket.c
index 1365eaa20ff4..2e5bdf3db0ba 100644
--- a/drivers/pcmcia/yenta_socket.c
+++ b/drivers/pcmcia/yenta_socket.c
@@ -673,9 +673,8 @@ static int yenta_search_res(struct yenta_socket *socket, 
struct resource *res,
u32 min)
 {
struct resource *root;
-   int i;
 
-   pci_bus_for_each_resource(socket->dev->bus, root, i) {
+   pci_bus_for_each_resource_p(socket->dev->bus, root) {
if (!root)
continue;
 
-- 
2.39.2

[PATCH v5 3/4] EISA: Convert to use pci_bus_for_each_resource_p()

[Andy Shevchenko]

The pci_bus_for_each_resource_p() hides the iterator loop since
it may be not used otherwise. With this, we may drop that iterator
variable definition.

Signed-off-by: Andy Shevchenko 
Reviewed-by: Krzysztof Wilczyński 
---
 drivers/eisa/pci_eisa.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/eisa/pci_eisa.c b/drivers/eisa/pci_eisa.c
index 930c2332c3c4..907b86384396 100644
--- a/drivers/eisa/pci_eisa.c
+++ b/drivers/eisa/pci_eisa.c
@@ -20,8 +20,8 @@ static struct eisa_root_device pci_eisa_root;
 
 static int __init pci_eisa_init(struct pci_dev *pdev)
 {
-   int rc, i;
struct resource *res, *bus_res = NULL;
+   int rc;
 
if ((rc = pci_enable_device (pdev))) {
dev_err(>dev, "Could not enable device\n");
@@ -38,7 +38,7 @@ static int __init pci_eisa_init(struct pci_dev *pdev)
 * eisa_root_register() can only deal with a single io port resource,
*  so we use the first valid io port resource.
 */
-   pci_bus_for_each_resource(pdev->bus, res, i)
+   pci_bus_for_each_resource_p(pdev->bus, res)
if (res && (res->flags & IORESOURCE_IO)) {
bus_res = res;
break;
-- 
2.39.2

[PATCH v5 2/4] PCI: Split pci_bus_for_each_resource_p() out of pci_bus_for_each_resource()

[Andy Shevchenko]

Refactor pci_bus_for_each_resource() in the same way as it's done in
pci_dev_for_each_resource() case. This will allow to hide iterator
inside the loop, where it's not used otherwise.

No functional changes intended.

Signed-off-by: Andy Shevchenko 
Reviewed-by: Krzysztof Wilczyński 
---
 .clang-format  |  1 +
 drivers/pci/bus.c  |  7 +++
 drivers/pci/hotplug/shpchp_sysfs.c |  8 
 drivers/pci/pci.c  |  5 ++---
 drivers/pci/probe.c|  2 +-
 drivers/pci/setup-bus.c| 10 --
 include/linux/pci.h| 14 ++
 7 files changed, 25 insertions(+), 22 deletions(-)

diff --git a/.clang-format b/.clang-format
index 266abb843654..81c9f055086f 100644
--- a/.clang-format
+++ b/.clang-format
@@ -520,6 +520,7 @@ ForEachMacros:
   - 'of_property_for_each_string'
   - 'of_property_for_each_u32'
   - 'pci_bus_for_each_resource'
+  - 'pci_bus_for_each_resource_p'
   - 'pci_dev_for_each_resource'
   - 'pci_dev_for_each_resource_p'
   - 'pci_doe_for_each_off'
diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 549c4bd5caec..b0789d332d36 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -182,13 +182,13 @@ static int pci_bus_alloc_from_region(struct pci_bus *bus, 
struct resource *res,
void *alignf_data,
struct pci_bus_region *region)
 {
-   int i, ret;
struct resource *r, avail;
resource_size_t max;
+   int ret;
 
type_mask |= IORESOURCE_TYPE_BITS;
 
-   pci_bus_for_each_resource(bus, r, i) {
+   pci_bus_for_each_resource_p(bus, r) {
resource_size_t min_used = min;
 
if (!r)
@@ -289,9 +289,8 @@ bool pci_bus_clip_resource(struct pci_dev *dev, int idx)
struct resource *res = >resource[idx];
struct resource orig_res = *res;
struct resource *r;
-   int i;
 
-   pci_bus_for_each_resource(bus, r, i) {
+   pci_bus_for_each_resource_p(bus, r) {
resource_size_t start, end;
 
if (!r)
diff --git a/drivers/pci/hotplug/shpchp_sysfs.c 
b/drivers/pci/hotplug/shpchp_sysfs.c
index 64beed7a26be..ff04f0c5e7c3 100644
--- a/drivers/pci/hotplug/shpchp_sysfs.c
+++ b/drivers/pci/hotplug/shpchp_sysfs.c
@@ -24,16 +24,16 @@
 static ssize_t show_ctrl(struct device *dev, struct device_attribute *attr, 
char *buf)
 {
struct pci_dev *pdev;
-   int index, busnr;
struct resource *res;
struct pci_bus *bus;
size_t len = 0;
+   int busnr;
 
pdev = to_pci_dev(dev);
bus = pdev->subordinate;
 
len += sysfs_emit_at(buf, len, "Free resources: memory\n");
-   pci_bus_for_each_resource(bus, res, index) {
+   pci_bus_for_each_resource_p(bus, res) {
if (res && (res->flags & IORESOURCE_MEM) &&
!(res->flags & IORESOURCE_PREFETCH)) {
len += sysfs_emit_at(buf, len,
@@ -43,7 +43,7 @@ static ssize_t show_ctrl(struct device *dev, struct 
device_attribute *attr, char
}
}
len += sysfs_emit_at(buf, len, "Free resources: prefetchable memory\n");
-   pci_bus_for_each_resource(bus, res, index) {
+   pci_bus_for_each_resource_p(bus, res) {
if (res && (res->flags & IORESOURCE_MEM) &&
   (res->flags & IORESOURCE_PREFETCH)) {
len += sysfs_emit_at(buf, len,
@@ -53,7 +53,7 @@ static ssize_t show_ctrl(struct device *dev, struct 
device_attribute *attr, char
}
}
len += sysfs_emit_at(buf, len, "Free resources: IO\n");
-   pci_bus_for_each_resource(bus, res, index) {
+   pci_bus_for_each_resource_p(bus, res) {
if (res && (res->flags & IORESOURCE_IO)) {
len += sysfs_emit_at(buf, len,
 "start = %8.8llx, length = 
%8.8llx\n",
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 7a67611dc5f4..2f8915ab41ef 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -779,9 +779,8 @@ struct resource *pci_find_parent_resource(const struct 
pci_dev *dev,
 {
const struct pci_bus *bus = dev->bus;
struct resource *r;
-   int i;
 
-   pci_bus_for_each_resource(bus, r, i) {
+   pci_bus_for_each_resource_p(bus, r) {
if (!r)
continue;
if (resource_contains(r, res)) {
@@ -799,7 +798,7 @@ struct resource *pci_find_parent_resource(const struct 
pci_dev *dev,
 * be both a positively-decoded aperture and a
 * subtractively-decoded region that contain the BAR.
 * We want the positively-decoded one, so this depends
-* on pci_bus_for_each_resource() 

Re: [PATCH v4 1/4] PCI: Introduce pci_dev_for_each_resource()

[Andy Shevchenko]

On Sat, Mar 11, 2023 at 04:54:32PM +0300, Dan Carpenter wrote:

> 059b4a086017fb Mika Westerberg 2023-03-10  246
> unsigned long type = resource_type(r);
> 999ed65ad12e37 Rene Herman 2008-07-25  247  
> 059b4a086017fb Mika Westerberg 2023-03-10 @248if 
> (type != IORESOURCE_IO || type != IORESOURCE_MEM ||
>   
> ^^
> This || needs to be &&.  This loop will always hit the continue path
> without doing anything.
> 
> 059b4a086017fb Mika Westerberg 2023-03-10  249
> resource_size(r) == 0)
> 0509ad5e1a7d92 Bjorn Helgaas   2008-03-11  250
> continue;

Thanks, I'll fix in v5.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 1/4] PCI: Introduce pci_dev_for_each_resource()

[Andy Shevchenko]

On Fri, Mar 10, 2023 at 03:15:38PM -0700, Keith Busch wrote:
> On Fri, Mar 10, 2023 at 07:14:13PM +0200, Andy Shevchenko wrote:

...

> > +#define pci_dev_for_each_resource_p(dev, res)  
> > \
> > +   __pci_dev_for_each_resource(dev, res, i, unsigned int)
> 
> It looks dangerous to have a macro declare a variable when starting a new
> scope. How do you know the name 'i' won't clash with something defined above?

I'll rename. Thank you.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v4 2/4] PCI: Split pci_bus_for_each_resource_p() out of pci_bus_for_each_resource()

[Andy Shevchenko]

Refactor pci_bus_for_each_resource() in the same way as it's done in
pci_dev_for_each_resource() case. This will allow to hide iterator
inside the loop, where it's not used otherwise.

No functional changes intended.

Signed-off-by: Andy Shevchenko 
Reviewed-by: Krzysztof Wilczyński 
---
 .clang-format  |  1 +
 drivers/pci/bus.c  |  7 +++
 drivers/pci/hotplug/shpchp_sysfs.c |  8 
 drivers/pci/pci.c  |  5 ++---
 drivers/pci/probe.c|  2 +-
 drivers/pci/setup-bus.c| 10 --
 include/linux/pci.h| 14 ++
 7 files changed, 25 insertions(+), 22 deletions(-)

diff --git a/.clang-format b/.clang-format
index 266abb843654..81c9f055086f 100644
--- a/.clang-format
+++ b/.clang-format
@@ -520,6 +520,7 @@ ForEachMacros:
   - 'of_property_for_each_string'
   - 'of_property_for_each_u32'
   - 'pci_bus_for_each_resource'
+  - 'pci_bus_for_each_resource_p'
   - 'pci_dev_for_each_resource'
   - 'pci_dev_for_each_resource_p'
   - 'pci_doe_for_each_off'
diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 83ae838ceb5f..b48bcc965a42 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -161,13 +161,13 @@ static int pci_bus_alloc_from_region(struct pci_bus *bus, 
struct resource *res,
void *alignf_data,
struct pci_bus_region *region)
 {
-   int i, ret;
struct resource *r, avail;
resource_size_t max;
+   int ret;
 
type_mask |= IORESOURCE_TYPE_BITS;
 
-   pci_bus_for_each_resource(bus, r, i) {
+   pci_bus_for_each_resource_p(bus, r) {
resource_size_t min_used = min;
 
if (!r)
@@ -268,9 +268,8 @@ bool pci_bus_clip_resource(struct pci_dev *dev, int idx)
struct resource *res = >resource[idx];
struct resource orig_res = *res;
struct resource *r;
-   int i;
 
-   pci_bus_for_each_resource(bus, r, i) {
+   pci_bus_for_each_resource_p(bus, r) {
resource_size_t start, end;
 
if (!r)
diff --git a/drivers/pci/hotplug/shpchp_sysfs.c 
b/drivers/pci/hotplug/shpchp_sysfs.c
index 64beed7a26be..ff04f0c5e7c3 100644
--- a/drivers/pci/hotplug/shpchp_sysfs.c
+++ b/drivers/pci/hotplug/shpchp_sysfs.c
@@ -24,16 +24,16 @@
 static ssize_t show_ctrl(struct device *dev, struct device_attribute *attr, 
char *buf)
 {
struct pci_dev *pdev;
-   int index, busnr;
struct resource *res;
struct pci_bus *bus;
size_t len = 0;
+   int busnr;
 
pdev = to_pci_dev(dev);
bus = pdev->subordinate;
 
len += sysfs_emit_at(buf, len, "Free resources: memory\n");
-   pci_bus_for_each_resource(bus, res, index) {
+   pci_bus_for_each_resource_p(bus, res) {
if (res && (res->flags & IORESOURCE_MEM) &&
!(res->flags & IORESOURCE_PREFETCH)) {
len += sysfs_emit_at(buf, len,
@@ -43,7 +43,7 @@ static ssize_t show_ctrl(struct device *dev, struct 
device_attribute *attr, char
}
}
len += sysfs_emit_at(buf, len, "Free resources: prefetchable memory\n");
-   pci_bus_for_each_resource(bus, res, index) {
+   pci_bus_for_each_resource_p(bus, res) {
if (res && (res->flags & IORESOURCE_MEM) &&
   (res->flags & IORESOURCE_PREFETCH)) {
len += sysfs_emit_at(buf, len,
@@ -53,7 +53,7 @@ static ssize_t show_ctrl(struct device *dev, struct 
device_attribute *attr, char
}
}
len += sysfs_emit_at(buf, len, "Free resources: IO\n");
-   pci_bus_for_each_resource(bus, res, index) {
+   pci_bus_for_each_resource_p(bus, res) {
if (res && (res->flags & IORESOURCE_IO)) {
len += sysfs_emit_at(buf, len,
 "start = %8.8llx, length = 
%8.8llx\n",
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 7a67611dc5f4..2f8915ab41ef 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -779,9 +779,8 @@ struct resource *pci_find_parent_resource(const struct 
pci_dev *dev,
 {
const struct pci_bus *bus = dev->bus;
struct resource *r;
-   int i;
 
-   pci_bus_for_each_resource(bus, r, i) {
+   pci_bus_for_each_resource_p(bus, r) {
if (!r)
continue;
if (resource_contains(r, res)) {
@@ -799,7 +798,7 @@ struct resource *pci_find_parent_resource(const struct 
pci_dev *dev,
 * be both a positively-decoded aperture and a
 * subtractively-decoded region that contain the BAR.
 * We want the positively-decoded one, so this depends
-* on pci_bus_for_each_resource() 

[PATCH v4 1/4] PCI: Introduce pci_dev_for_each_resource()

[Andy Shevchenko]

From: Mika Westerberg 

Instead of open-coding it everywhere introduce a tiny helper that can be
used to iterate over each resource of a PCI device, and convert the most
obvious users into it.

While at it drop doubled empty line before pdev_sort_resources().

No functional changes intended.

Suggested-by: Andy Shevchenko 
Signed-off-by: Mika Westerberg 
Signed-off-by: Andy Shevchenko 
Reviewed-by: Krzysztof Wilczyński 
---
 .clang-format |  2 ++
 arch/alpha/kernel/pci.c   |  5 ++--
 arch/arm/kernel/bios32.c  | 16 ++---
 arch/arm/mach-dove/pcie.c | 10 
 arch/arm/mach-mv78xx0/pcie.c  | 10 
 arch/arm/mach-orion5x/pci.c   | 10 
 arch/mips/pci/ops-bcm63xx.c   |  8 +++
 arch/mips/pci/pci-legacy.c|  3 +--
 arch/powerpc/kernel/pci-common.c  | 21 
 arch/powerpc/platforms/4xx/pci.c  |  8 +++
 arch/powerpc/platforms/52xx/mpc52xx_pci.c |  4 ++--
 arch/powerpc/platforms/pseries/pci.c  | 16 ++---
 arch/sh/drivers/pci/pcie-sh7786.c | 10 
 arch/sparc/kernel/leon_pci.c  |  5 ++--
 arch/sparc/kernel/pci.c   | 10 
 arch/sparc/kernel/pcic.c  |  5 ++--
 drivers/pci/remove.c  |  5 ++--
 drivers/pci/setup-bus.c   | 27 -
 drivers/pci/setup-res.c   |  4 +---
 drivers/pci/vgaarb.c  | 17 -
 drivers/pci/xen-pcifront.c|  4 +---
 drivers/pnp/quirks.c  | 29 ---
 include/linux/pci.h   | 12 ++
 23 files changed, 110 insertions(+), 131 deletions(-)

diff --git a/.clang-format b/.clang-format
index d988e9fa9b26..266abb843654 100644
--- a/.clang-format
+++ b/.clang-format
@@ -520,6 +520,8 @@ ForEachMacros:
   - 'of_property_for_each_string'
   - 'of_property_for_each_u32'
   - 'pci_bus_for_each_resource'
+  - 'pci_dev_for_each_resource'
+  - 'pci_dev_for_each_resource_p'
   - 'pci_doe_for_each_off'
   - 'pcl_for_each_chunk'
   - 'pcl_for_each_segment'
diff --git a/arch/alpha/kernel/pci.c b/arch/alpha/kernel/pci.c
index 64fbfb0763b2..4458eb7f44f0 100644
--- a/arch/alpha/kernel/pci.c
+++ b/arch/alpha/kernel/pci.c
@@ -288,11 +288,10 @@ pcibios_claim_one_bus(struct pci_bus *b)
struct pci_bus *child_bus;
 
list_for_each_entry(dev, >devices, bus_list) {
+   struct resource *r;
int i;
 
-   for (i = 0; i < PCI_NUM_RESOURCES; i++) {
-   struct resource *r = >resource[i];
-
+   pci_dev_for_each_resource(dev, r, i) {
if (r->parent || !r->start || !r->flags)
continue;
if (pci_has_flag(PCI_PROBE_ONLY) ||
diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index e7ef2b5bea9c..5254734b23e6 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -142,15 +142,15 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_WINBOND2, 
PCI_DEVICE_ID_WINBOND2_89C940F,
  */
 static void pci_fixup_dec21285(struct pci_dev *dev)
 {
-   int i;
-
if (dev->devfn == 0) {
+   struct resource *r;
+
dev->class &= 0xff;
dev->class |= PCI_CLASS_BRIDGE_HOST << 8;
-   for (i = 0; i < PCI_NUM_RESOURCES; i++) {
-   dev->resource[i].start = 0;
-   dev->resource[i].end   = 0;
-   dev->resource[i].flags = 0;
+   pci_dev_for_each_resource_p(dev, r) {
+   r->start = 0;
+   r->end = 0;
+   r->flags = 0;
}
}
 }
@@ -162,13 +162,11 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_DEC, 
PCI_DEVICE_ID_DEC_21285, pci_fixup_d
 static void pci_fixup_ide_bases(struct pci_dev *dev)
 {
struct resource *r;
-   int i;
 
if ((dev->class >> 8) != PCI_CLASS_STORAGE_IDE)
return;
 
-   for (i = 0; i < PCI_NUM_RESOURCES; i++) {
-   r = dev->resource + i;
+   pci_dev_for_each_resource_p(dev, r) {
if ((r->start & ~0x80) == 0x374) {
r->start |= 2;
r->end = r->start;
diff --git a/arch/arm/mach-dove/pcie.c b/arch/arm/mach-dove/pcie.c
index 754ca381f600..58cecd79a204 100644
--- a/arch/arm/mach-dove/pcie.c
+++ b/arch/arm/mach-dove/pcie.c
@@ -142,14 +142,14 @@ static struct pci_ops pcie_ops = {
 static void rc_pci_fixup(struct pci_dev *dev)
 {
if (dev->bus->parent == NULL && dev->devfn == 0) {
-   int i;
+   struct resource *r;
 
dev->class &= 0xff;
dev->class |= P

[PATCH v4 3/4] EISA: Convert to use pci_bus_for_each_resource_p()

[Andy Shevchenko]

The pci_bus_for_each_resource_p() hides the iterator loop since
it may be not used otherwise. With this, we may drop that iterator
variable definition.

Signed-off-by: Andy Shevchenko 
Reviewed-by: Krzysztof Wilczyński 
---
 drivers/eisa/pci_eisa.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/eisa/pci_eisa.c b/drivers/eisa/pci_eisa.c
index 930c2332c3c4..907b86384396 100644
--- a/drivers/eisa/pci_eisa.c
+++ b/drivers/eisa/pci_eisa.c
@@ -20,8 +20,8 @@ static struct eisa_root_device pci_eisa_root;
 
 static int __init pci_eisa_init(struct pci_dev *pdev)
 {
-   int rc, i;
struct resource *res, *bus_res = NULL;
+   int rc;
 
if ((rc = pci_enable_device (pdev))) {
dev_err(>dev, "Could not enable device\n");
@@ -38,7 +38,7 @@ static int __init pci_eisa_init(struct pci_dev *pdev)
 * eisa_root_register() can only deal with a single io port resource,
*  so we use the first valid io port resource.
 */
-   pci_bus_for_each_resource(pdev->bus, res, i)
+   pci_bus_for_each_resource_p(pdev->bus, res)
if (res && (res->flags & IORESOURCE_IO)) {
bus_res = res;
break;
-- 
2.39.1

[PATCH v4 0/4] PCI: Add pci_dev_for_each_resource() helper and update users

[Andy Shevchenko]

Provide two new helper macros to iterate over PCI device resources and
convert users.

Looking at it, refactor existing pci_bus_for_each_resource() and convert
users accordingly.

Changelog v4:
- rebased on top of v6.3-rc1
- added tag (Krzysztof)

Changelog v3:
- rebased on top of v2 by Mika, see above
- added tag to pcmcia patch (Dominik)

Changelog v2:
- refactor to have two macros
- refactor existing pci_bus_for_each_resource() in the same way and
  convert users

Andy Shevchenko (3):
  PCI: Split pci_bus_for_each_resource_p() out of
pci_bus_for_each_resource()
  EISA: Convert to use pci_bus_for_each_resource_p()
  pcmcia: Convert to use pci_bus_for_each_resource_p()

Mika Westerberg (1):
  PCI: Introduce pci_dev_for_each_resource()

 .clang-format |  3 ++
 arch/alpha/kernel/pci.c   |  5 ++-
 arch/arm/kernel/bios32.c  | 16 +-
 arch/arm/mach-dove/pcie.c | 10 +++---
 arch/arm/mach-mv78xx0/pcie.c  | 10 +++---
 arch/arm/mach-orion5x/pci.c   | 10 +++---
 arch/mips/pci/ops-bcm63xx.c   |  8 ++---
 arch/mips/pci/pci-legacy.c|  3 +-
 arch/powerpc/kernel/pci-common.c  | 21 +++--
 arch/powerpc/platforms/4xx/pci.c  |  8 ++---
 arch/powerpc/platforms/52xx/mpc52xx_pci.c |  4 +--
 arch/powerpc/platforms/pseries/pci.c  | 16 +-
 arch/sh/drivers/pci/pcie-sh7786.c | 10 +++---
 arch/sparc/kernel/leon_pci.c  |  5 ++-
 arch/sparc/kernel/pci.c   | 10 +++---
 arch/sparc/kernel/pcic.c  |  5 ++-
 drivers/eisa/pci_eisa.c   |  4 +--
 drivers/pci/bus.c |  7 ++---
 drivers/pci/hotplug/shpchp_sysfs.c|  8 ++---
 drivers/pci/pci.c |  5 ++-
 drivers/pci/probe.c   |  2 +-
 drivers/pci/remove.c  |  5 ++-
 drivers/pci/setup-bus.c   | 37 +--
 drivers/pci/setup-res.c   |  4 +--
 drivers/pci/vgaarb.c  | 17 +++
 drivers/pci/xen-pcifront.c|  4 +--
 drivers/pcmcia/rsrc_nonstatic.c   |  9 ++
 drivers/pcmcia/yenta_socket.c |  3 +-
 drivers/pnp/quirks.c  | 29 ++
 include/linux/pci.h   | 26 +---
 30 files changed, 141 insertions(+), 163 deletions(-)

-- 
2.39.1

[PATCH v4 4/4] pcmcia: Convert to use pci_bus_for_each_resource_p()

[Andy Shevchenko]

The pci_bus_for_each_resource_p() hides the iterator loop since
it may be not used otherwise. With this, we may drop that iterator
variable definition.

Signed-off-by: Andy Shevchenko 
Reviewed-by: Krzysztof Wilczyński 
Acked-by: Dominik Brodowski 
---
 drivers/pcmcia/rsrc_nonstatic.c | 9 +++--
 drivers/pcmcia/yenta_socket.c   | 3 +--
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/pcmcia/rsrc_nonstatic.c b/drivers/pcmcia/rsrc_nonstatic.c
index ad1141fddb4c..9d92d4bb6239 100644
--- a/drivers/pcmcia/rsrc_nonstatic.c
+++ b/drivers/pcmcia/rsrc_nonstatic.c
@@ -934,7 +934,7 @@ static int adjust_io(struct pcmcia_socket *s, unsigned int 
action, unsigned long
 static int nonstatic_autoadd_resources(struct pcmcia_socket *s)
 {
struct resource *res;
-   int i, done = 0;
+   int done = 0;
 
if (!s->cb_dev || !s->cb_dev->bus)
return -ENODEV;
@@ -960,12 +960,9 @@ static int nonstatic_autoadd_resources(struct 
pcmcia_socket *s)
 */
if (s->cb_dev->bus->number == 0)
return -EINVAL;
-
-   for (i = 0; i < PCI_BRIDGE_RESOURCE_NUM; i++) {
-   res = s->cb_dev->bus->resource[i];
-#else
-   pci_bus_for_each_resource(s->cb_dev->bus, res, i) {
 #endif
+
+   pci_bus_for_each_resource_p(s->cb_dev->bus, res) {
if (!res)
continue;
 
diff --git a/drivers/pcmcia/yenta_socket.c b/drivers/pcmcia/yenta_socket.c
index 1365eaa20ff4..2e5bdf3db0ba 100644
--- a/drivers/pcmcia/yenta_socket.c
+++ b/drivers/pcmcia/yenta_socket.c
@@ -673,9 +673,8 @@ static int yenta_search_res(struct yenta_socket *socket, 
struct resource *res,
u32 min)
 {
struct resource *root;
-   int i;
 
-   pci_bus_for_each_resource(socket->dev->bus, root, i) {
+   pci_bus_for_each_resource_p(socket->dev->bus, root) {
if (!root)
continue;
 
-- 
2.39.1

[PATCH v3 1/4] PCI: Introduce pci_dev_for_each_resource()

[Andy Shevchenko]

From: Mika Westerberg 

Instead of open-coding it everywhere introduce a tiny helper that can be
used to iterate over each resource of a PCI device, and convert the most
obvious users into it.

While at it drop doubled empty line before pdev_sort_resources().

No functional changes intended.

Suggested-by: Andy Shevchenko 
Signed-off-by: Mika Westerberg 
Signed-off-by: Andy Shevchenko 
---
 .clang-format|  2 ++
 arch/alpha/kernel/pci.c  |  5 ++---
 arch/arm/kernel/bios32.c | 16 +++-
 arch/mips/pci/pci-legacy.c   |  3 +--
 arch/powerpc/kernel/pci-common.c |  5 ++---
 arch/sparc/kernel/leon_pci.c |  5 ++---
 arch/sparc/kernel/pci.c  | 10 --
 arch/sparc/kernel/pcic.c |  5 ++---
 drivers/pci/remove.c |  5 ++---
 drivers/pci/setup-bus.c  | 26 ++
 drivers/pci/setup-res.c  |  4 +---
 drivers/pci/xen-pcifront.c   |  4 +---
 include/linux/pci.h  | 11 +++
 13 files changed, 47 insertions(+), 54 deletions(-)

diff --git a/.clang-format b/.clang-format
index f98481a53ea8..08d579fea6cf 100644
--- a/.clang-format
+++ b/.clang-format
@@ -520,6 +520,8 @@ ForEachMacros:
   - 'of_property_for_each_string'
   - 'of_property_for_each_u32'
   - 'pci_bus_for_each_resource'
+  - 'pci_dev_for_each_resource'
+  - 'pci_dev_for_each_resource_p'
   - 'pci_doe_for_each_off'
   - 'pcl_for_each_chunk'
   - 'pcl_for_each_segment'
diff --git a/arch/alpha/kernel/pci.c b/arch/alpha/kernel/pci.c
index 64fbfb0763b2..4458eb7f44f0 100644
--- a/arch/alpha/kernel/pci.c
+++ b/arch/alpha/kernel/pci.c
@@ -288,11 +288,10 @@ pcibios_claim_one_bus(struct pci_bus *b)
struct pci_bus *child_bus;
 
list_for_each_entry(dev, >devices, bus_list) {
+   struct resource *r;
int i;
 
-   for (i = 0; i < PCI_NUM_RESOURCES; i++) {
-   struct resource *r = >resource[i];
-
+   pci_dev_for_each_resource(dev, r, i) {
if (r->parent || !r->start || !r->flags)
continue;
if (pci_has_flag(PCI_PROBE_ONLY) ||
diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index e7ef2b5bea9c..5254734b23e6 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -142,15 +142,15 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_WINBOND2, 
PCI_DEVICE_ID_WINBOND2_89C940F,
  */
 static void pci_fixup_dec21285(struct pci_dev *dev)
 {
-   int i;
-
if (dev->devfn == 0) {
+   struct resource *r;
+
dev->class &= 0xff;
dev->class |= PCI_CLASS_BRIDGE_HOST << 8;
-   for (i = 0; i < PCI_NUM_RESOURCES; i++) {
-   dev->resource[i].start = 0;
-   dev->resource[i].end   = 0;
-   dev->resource[i].flags = 0;
+   pci_dev_for_each_resource_p(dev, r) {
+   r->start = 0;
+   r->end = 0;
+   r->flags = 0;
}
}
 }
@@ -162,13 +162,11 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_DEC, 
PCI_DEVICE_ID_DEC_21285, pci_fixup_d
 static void pci_fixup_ide_bases(struct pci_dev *dev)
 {
struct resource *r;
-   int i;
 
if ((dev->class >> 8) != PCI_CLASS_STORAGE_IDE)
return;
 
-   for (i = 0; i < PCI_NUM_RESOURCES; i++) {
-   r = dev->resource + i;
+   pci_dev_for_each_resource_p(dev, r) {
if ((r->start & ~0x80) == 0x374) {
r->start |= 2;
r->end = r->start;
diff --git a/arch/mips/pci/pci-legacy.c b/arch/mips/pci/pci-legacy.c
index 468722c8a5c6..ec2567f8efd8 100644
--- a/arch/mips/pci/pci-legacy.c
+++ b/arch/mips/pci/pci-legacy.c
@@ -249,12 +249,11 @@ static int pcibios_enable_resources(struct pci_dev *dev, 
int mask)
 
pci_read_config_word(dev, PCI_COMMAND, );
old_cmd = cmd;
-   for (idx = 0; idx < PCI_NUM_RESOURCES; idx++) {
+   pci_dev_for_each_resource(dev, r, idx) {
/* Only set up the requested stuff */
if (!(mask & (1<resource[idx];
if (!(r->flags & (IORESOURCE_IO | IORESOURCE_MEM)))
continue;
if ((idx == PCI_ROM_RESOURCE) &&
diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index d67cf79bf5d0..8ddcfa6bcb50 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -1452,11 +1452,10 @@ void pcibios_claim_one_bus(struct pci_bus *bus)
struct pci_bus *child_bus;
 
list_for_each_entry(dev, >devices, bus_list) {
+   struct resource *r;
int i;
 
-   for (i = 0; i < PCI_NUM_RESOURCES;

[PATCH v3 0/4] PCI: Add pci_dev_for_each_resource() helper and

[Andy Shevchenko]

Provide two new helper macros to iterate over PCI device resources and
convert users.

Looking at it, refactor existing pci_bus_for_each_resource() and convert
users accordingly.

This applies on top of this patch Mika sent out earlier:
https://lore.kernel.org/r/20221114115953.40236-1-mika.westerb...@linux.intel.com

Changelog v3:
- rebased on top of v2 by Mika, see above
- added tag to pcmcia patch (Dominik)

Changelog v2:
- refactor to have two macros
- refactor existing pci_bus_for_each_resource() in the same way and
  convert users

Andy Shevchenko (3):
  PCI: Split pci_bus_for_each_resource_p() out of
pci_bus_for_each_resource()
  EISA: Convert to use pci_bus_for_each_resource_p()
  pcmcia: Convert to use pci_bus_for_each_resource_p()

Mika Westerberg (1):
  PCI: Introduce pci_dev_for_each_resource()

 .clang-format  |  3 +++
 arch/alpha/kernel/pci.c|  5 ++---
 arch/arm/kernel/bios32.c   | 16 ++---
 arch/mips/pci/pci-legacy.c |  3 +--
 arch/powerpc/kernel/pci-common.c   |  5 ++---
 arch/sparc/kernel/leon_pci.c   |  5 ++---
 arch/sparc/kernel/pci.c| 10 -
 arch/sparc/kernel/pcic.c   |  5 ++---
 drivers/eisa/pci_eisa.c|  4 ++--
 drivers/pci/bus.c  |  7 +++---
 drivers/pci/hotplug/shpchp_sysfs.c |  8 +++
 drivers/pci/pci.c  |  5 ++---
 drivers/pci/probe.c|  2 +-
 drivers/pci/remove.c   |  5 ++---
 drivers/pci/setup-bus.c| 36 --
 drivers/pci/setup-res.c|  4 +---
 drivers/pci/xen-pcifront.c |  4 +---
 drivers/pcmcia/rsrc_nonstatic.c|  9 +++-
 drivers/pcmcia/yenta_socket.c  |  3 +--
 include/linux/pci.h| 25 +
 20 files changed, 78 insertions(+), 86 deletions(-)

-- 
2.35.1

[PATCH v3 3/4] EISA: Convert to use pci_bus_for_each_resource_p()

[Andy Shevchenko]

The pci_bus_for_each_resource_p() hides the iterator loop since
it may be not used otherwise. With this, we may drop that iterator
variable definition.

Signed-off-by: Andy Shevchenko 
---
 drivers/eisa/pci_eisa.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/eisa/pci_eisa.c b/drivers/eisa/pci_eisa.c
index 930c2332c3c4..907b86384396 100644
--- a/drivers/eisa/pci_eisa.c
+++ b/drivers/eisa/pci_eisa.c
@@ -20,8 +20,8 @@ static struct eisa_root_device pci_eisa_root;
 
 static int __init pci_eisa_init(struct pci_dev *pdev)
 {
-   int rc, i;
struct resource *res, *bus_res = NULL;
+   int rc;
 
if ((rc = pci_enable_device (pdev))) {
dev_err(>dev, "Could not enable device\n");
@@ -38,7 +38,7 @@ static int __init pci_eisa_init(struct pci_dev *pdev)
 * eisa_root_register() can only deal with a single io port resource,
*  so we use the first valid io port resource.
 */
-   pci_bus_for_each_resource(pdev->bus, res, i)
+   pci_bus_for_each_resource_p(pdev->bus, res)
if (res && (res->flags & IORESOURCE_IO)) {
bus_res = res;
break;
-- 
2.35.1

[PATCH v3 2/4] PCI: Split pci_bus_for_each_resource_p() out of pci_bus_for_each_resource()

[Andy Shevchenko]

Refactor pci_bus_for_each_resource() in the same way as it's done in
pci_dev_for_each_resource() case. This will allow to hide iterator
inside the loop, where it's not used otherwise.

No functional changes intended.

Signed-off-by: Andy Shevchenko 
---
 .clang-format  |  1 +
 drivers/pci/bus.c  |  7 +++
 drivers/pci/hotplug/shpchp_sysfs.c |  8 
 drivers/pci/pci.c  |  5 ++---
 drivers/pci/probe.c|  2 +-
 drivers/pci/setup-bus.c| 10 --
 include/linux/pci.h| 14 ++
 7 files changed, 25 insertions(+), 22 deletions(-)

diff --git a/.clang-format b/.clang-format
index 08d579fea6cf..b61fd8791346 100644
--- a/.clang-format
+++ b/.clang-format
@@ -520,6 +520,7 @@ ForEachMacros:
   - 'of_property_for_each_string'
   - 'of_property_for_each_u32'
   - 'pci_bus_for_each_resource'
+  - 'pci_bus_for_each_resource_p'
   - 'pci_dev_for_each_resource'
   - 'pci_dev_for_each_resource_p'
   - 'pci_doe_for_each_off'
diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 3cef835b375f..fc8e9c11c5f2 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -161,13 +161,13 @@ static int pci_bus_alloc_from_region(struct pci_bus *bus, 
struct resource *res,
void *alignf_data,
struct pci_bus_region *region)
 {
-   int i, ret;
struct resource *r, avail;
resource_size_t max;
+   int ret;
 
type_mask |= IORESOURCE_TYPE_BITS;
 
-   pci_bus_for_each_resource(bus, r, i) {
+   pci_bus_for_each_resource_p(bus, r) {
resource_size_t min_used = min;
 
if (!r)
@@ -264,9 +264,8 @@ bool pci_bus_clip_resource(struct pci_dev *dev, int idx)
struct resource *res = >resource[idx];
struct resource orig_res = *res;
struct resource *r;
-   int i;
 
-   pci_bus_for_each_resource(bus, r, i) {
+   pci_bus_for_each_resource_p(bus, r) {
resource_size_t start, end;
 
if (!r)
diff --git a/drivers/pci/hotplug/shpchp_sysfs.c 
b/drivers/pci/hotplug/shpchp_sysfs.c
index 64beed7a26be..ff04f0c5e7c3 100644
--- a/drivers/pci/hotplug/shpchp_sysfs.c
+++ b/drivers/pci/hotplug/shpchp_sysfs.c
@@ -24,16 +24,16 @@
 static ssize_t show_ctrl(struct device *dev, struct device_attribute *attr, 
char *buf)
 {
struct pci_dev *pdev;
-   int index, busnr;
struct resource *res;
struct pci_bus *bus;
size_t len = 0;
+   int busnr;
 
pdev = to_pci_dev(dev);
bus = pdev->subordinate;
 
len += sysfs_emit_at(buf, len, "Free resources: memory\n");
-   pci_bus_for_each_resource(bus, res, index) {
+   pci_bus_for_each_resource_p(bus, res) {
if (res && (res->flags & IORESOURCE_MEM) &&
!(res->flags & IORESOURCE_PREFETCH)) {
len += sysfs_emit_at(buf, len,
@@ -43,7 +43,7 @@ static ssize_t show_ctrl(struct device *dev, struct 
device_attribute *attr, char
}
}
len += sysfs_emit_at(buf, len, "Free resources: prefetchable memory\n");
-   pci_bus_for_each_resource(bus, res, index) {
+   pci_bus_for_each_resource_p(bus, res) {
if (res && (res->flags & IORESOURCE_MEM) &&
   (res->flags & IORESOURCE_PREFETCH)) {
len += sysfs_emit_at(buf, len,
@@ -53,7 +53,7 @@ static ssize_t show_ctrl(struct device *dev, struct 
device_attribute *attr, char
}
}
len += sysfs_emit_at(buf, len, "Free resources: IO\n");
-   pci_bus_for_each_resource(bus, res, index) {
+   pci_bus_for_each_resource_p(bus, res) {
if (res && (res->flags & IORESOURCE_IO)) {
len += sysfs_emit_at(buf, len,
 "start = %8.8llx, length = 
%8.8llx\n",
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 9f3cc829dfee..9cb9939f0e79 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -782,9 +782,8 @@ struct resource *pci_find_parent_resource(const struct 
pci_dev *dev,
 {
const struct pci_bus *bus = dev->bus;
struct resource *r;
-   int i;
 
-   pci_bus_for_each_resource(bus, r, i) {
+   pci_bus_for_each_resource_p(bus, r) {
if (!r)
continue;
if (resource_contains(r, res)) {
@@ -802,7 +801,7 @@ struct resource *pci_find_parent_resource(const struct 
pci_dev *dev,
 * be both a positively-decoded aperture and a
 * subtractively-decoded region that contain the BAR.
 * We want the positively-decoded one, so this depends
-* on pci_bus_for_each_resource() 

[PATCH v3 4/4] pcmcia: Convert to use pci_bus_for_each_resource_p()

[Andy Shevchenko]

The pci_bus_for_each_resource_p() hides the iterator loop since
it may be not used otherwise. With this, we may drop that iterator
variable definition.

Signed-off-by: Andy Shevchenko 
Acked-by: Dominik Brodowski 
---
 drivers/pcmcia/rsrc_nonstatic.c | 9 +++--
 drivers/pcmcia/yenta_socket.c   | 3 +--
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/pcmcia/rsrc_nonstatic.c b/drivers/pcmcia/rsrc_nonstatic.c
index ad1141fddb4c..9d92d4bb6239 100644
--- a/drivers/pcmcia/rsrc_nonstatic.c
+++ b/drivers/pcmcia/rsrc_nonstatic.c
@@ -934,7 +934,7 @@ static int adjust_io(struct pcmcia_socket *s, unsigned int 
action, unsigned long
 static int nonstatic_autoadd_resources(struct pcmcia_socket *s)
 {
struct resource *res;
-   int i, done = 0;
+   int done = 0;
 
if (!s->cb_dev || !s->cb_dev->bus)
return -ENODEV;
@@ -960,12 +960,9 @@ static int nonstatic_autoadd_resources(struct 
pcmcia_socket *s)
 */
if (s->cb_dev->bus->number == 0)
return -EINVAL;
-
-   for (i = 0; i < PCI_BRIDGE_RESOURCE_NUM; i++) {
-   res = s->cb_dev->bus->resource[i];
-#else
-   pci_bus_for_each_resource(s->cb_dev->bus, res, i) {
 #endif
+
+   pci_bus_for_each_resource_p(s->cb_dev->bus, res) {
if (!res)
continue;
 
diff --git a/drivers/pcmcia/yenta_socket.c b/drivers/pcmcia/yenta_socket.c
index 3966a6ceb1ac..b200f2b99a7a 100644
--- a/drivers/pcmcia/yenta_socket.c
+++ b/drivers/pcmcia/yenta_socket.c
@@ -673,9 +673,8 @@ static int yenta_search_res(struct yenta_socket *socket, 
struct resource *res,
u32 min)
 {
struct resource *root;
-   int i;
 
-   pci_bus_for_each_resource(socket->dev->bus, root, i) {
+   pci_bus_for_each_resource_p(socket->dev->bus, root) {
if (!root)
continue;
 
-- 
2.35.1

Re: [PATCH v3] platform/x86: don't unconditionally attach Intel PMC when virtualized

[Andy Shevchenko]

On Thu, Nov 10, 2022 at 05:31:44PM +0100, Roger Pau Monne wrote:
> The current logic in the Intel PMC driver will forcefully attach it
> when detecting any CPU on the intel_pmc_core_platform_ids array,
> even if the matching ACPI device is not present.
> 
> There's no checking in pmc_core_probe() to assert that the PMC device
> is present, and hence on virtualized environments the PMC device
> probes successfully, even if the underlying registers are not present.
> Previous to 21ae43570940 the driver would check for the presence of a
> specific PCI device, and that prevented the driver from attaching when
> running virtualized.
> 
> Fix by only forcefully attaching the PMC device when not running
> virtualized.  Note that virtualized platforms can still get the device
> to load if the appropriate ACPI device is present on the tables
> provided to the VM.
> 
> Make an exception for the Xen initial domain, which does have full
> hardware access, and hence can attach to the PMC if present.

Reviewed-by: Andy Shevchenko 

> Fixes: 21ae43570940 ('platform/x86: intel_pmc_core: Substitute PCI with CPUID 
> enumeration')
> Signed-off-by: Roger Pau Monné 
> Acked-by: David E. Box 
> ---
> Changes since v2:
>  - Don't split condition line.
> 
> Changes since v1:
>  - Use cpu_feature_enabled() instead of boot_cpu_has().
> ---
>  drivers/platform/x86/intel/pmc/pltdrv.c | 9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel/pmc/pltdrv.c 
> b/drivers/platform/x86/intel/pmc/pltdrv.c
> index 15ca8afdd973..ddfba38c2104 100644
> --- a/drivers/platform/x86/intel/pmc/pltdrv.c
> +++ b/drivers/platform/x86/intel/pmc/pltdrv.c
> @@ -18,6 +18,8 @@
>  #include 
>  #include 
>  
> +#include 
> +
>  static void intel_pmc_core_release(struct device *dev)
>  {
>   kfree(dev);
> @@ -53,6 +55,13 @@ static int __init pmc_core_platform_init(void)
>   if (acpi_dev_present("INT33A1", NULL, -1))
>   return -ENODEV;
>  
> + /*
> +  * Skip forcefully attaching the device for VMs. Make an exception for
> +  * Xen dom0, which does have full hardware access.
> +  */
> + if (cpu_feature_enabled(X86_FEATURE_HYPERVISOR) && 
> !xen_initial_domain())
> + return -ENODEV;
> +
>   if (!x86_match_cpu(intel_pmc_core_platform_ids))
>   return -ENODEV;
>  
> -- 
> 2.37.3
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2] platform/x86: don't unconditionally attach Intel PMC when virtualized

[Andy Shevchenko]

On Thu, Nov 10, 2022 at 02:33:35PM +0100, Roger Pau Monne wrote:
> The current logic in the Intel PMC driver will forcefully attach it
> when detecting any CPU on the intel_pmc_core_platform_ids array,
> even if the matching ACPI device is not present.
> 
> There's no checking in pmc_core_probe() to assert that the PMC device
> is present, and hence on virtualized environments the PMC device
> probes successfully, even if the underlying registers are not present.
> Previous to 21ae43570940 the driver would check for the presence of a
> specific PCI device, and that prevented the driver from attaching when
> running virtualized.
> 
> Fix by only forcefully attaching the PMC device when not running
> virtualized.  Note that virtualized platforms can still get the device
> to load if the appropriate ACPI device is present on the tables
> provided to the VM.
> 
> Make an exception for the Xen initial domain, which does have full
> hardware access, and hence can attach to the PMC if present.
> 
> Fixes: 21ae43570940 ('platform/x86: intel_pmc_core: Substitute PCI with CPUID 
> enumeration')
> Signed-off-by: Roger Pau Monné 
> Acked-by: David E. Box 

> Cc: Rajneesh Bhardwaj 
> Cc: David E Box 
> Cc: Hans de Goede 
> Cc: Mark Gross 
> Cc: Andy Shevchenko 
> Cc: Srinivas Pandruvada 
> Cc: Juergen Gross 
> Cc: platform-driver-...@vger.kernel.org
> Cc: xen-devel@lists.xenproject.org

You may use --cc to the sending tool, instead of polluting a commit message
with that. Moreover, the Cc list will be archived on lore.kernel.org anyway,
in case you really need it to be recorded.

...

> + if (cpu_feature_enabled(X86_FEATURE_HYPERVISOR) &&
> + !xen_initial_domain())

One line? It's 81 character only and we have no strong 80 here, IIRC.

> + return -ENODEV;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/4] pcmcia: Convert to use pci_bus_for_each_resource_p()

[Andy Shevchenko]

On Thu, Nov 03, 2022 at 07:38:07PM +0100, Dominik Brodowski wrote:
> Am Fri, Nov 04, 2022 at 03:29:44AM +0900 schrieb Krzysztof Wilczyński:

...

> > That said, Dominik is the maintainer of PCMCIA driver, so his is the last
> > word, so to speak. :)
> > 
> > > Considering this is done, can you issue your conditional tag so I will
> > > incorporate it in v3?
> > 
> > No need, really.  Again, unless Dominik thinks otherwise.
> 
> Ah, thanks for the correction. Then v2 is perfectly fine.

I'm fine with either, thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/4] pcmcia: Convert to use pci_bus_for_each_resource_p()

[Andy Shevchenko]

On Fri, Nov 04, 2022 at 03:29:44AM +0900, Krzysztof Wilczyński wrote:

> > > > -
> > > > -   for (i = 0; i < PCI_BRIDGE_RESOURCE_NUM; i++) {
> > > > -   res = s->cb_dev->bus->resource[i];
> > > > -#else
> > > > -   pci_bus_for_each_resource(s->cb_dev->bus, res, i) {
> > > >  #endif
> > > > +
> > > > +   pci_bus_for_each_resource_p(s->cb_dev->bus, res) {
> > > > if (!res)
> > > > continue;
> > > 
> > > Doesn't this remove the proper iterator for X86? Even if that is the right
> > > thing to do, it needs an explict explanation.
> > 
> > I dunno what was in 2010, but reading code now I have found no differences 
> > in
> > the logic on how resources are being iterated in these two pieces of code.
> 
> This code is over a decade old (13 years old to be precise) and there was
> something odd between Bjorn's and Jesse's patches, as per:
> 
>   89a74ecccd1f ("PCI: add pci_bus_for_each_resource(), remove direct 
> bus->resource[] refs")
>   cf26e8dc4194 ("pcmcia: do not autoadd root PCI bus resources")

Yeah, thanks for pointing out to the other patch from the same 2010 year.
It seems the code was completely identical that time, now it uses more
sophisticated way of getting bus resources, but it's kept the same for
the resources under PCI_BRIDGE_RESOURCE_NUM threshold.

> > But fine, I will add a line to a commit message about this change.
> 
> I wouldn't, personally.  The change you are proposing is self-explanatory
> and somewhat in-line with what is there already - unless I am also reading
> the current implementation wrong.

But it wouldn't be harmful either.

> That said, Dominik is the maintainer of PCMCIA driver, so his is the last
> word, so to speak. :)
> 
> > Considering this is done, can you issue your conditional tag so I will
> > incorporate it in v3?
> 
> No need, really.  Again, unless Dominik thinks otherwise.

I think that what is wanted to have to get his tag.

Thanks for review, both of you, guys!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/4] pcmcia: Convert to use pci_bus_for_each_resource_p()

[Andy Shevchenko]

On Thu, Nov 03, 2022 at 06:25:45PM +0100, Dominik Brodowski wrote:
> Am Thu, Nov 03, 2022 at 07:12:45PM +0200 schrieb Andy Shevchenko:
> > On Thu, Nov 03, 2022 at 06:03:24PM +0100, Dominik Brodowski wrote:

...

> > Considering this is done, can you issue your conditional tag so I will
> > incorporate it in v3?
> 
> Certainly, feel free to add
> 
>   Acked-by: Dominik Brodowski 

Thank you for the review!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/4] pcmcia: Convert to use pci_bus_for_each_resource_p()

[Andy Shevchenko]

On Thu, Nov 03, 2022 at 06:03:24PM +0100, Dominik Brodowski wrote:
> Am Thu, Nov 03, 2022 at 06:46:44PM +0200 schrieb Andy Shevchenko:

...

> > -
> > -   for (i = 0; i < PCI_BRIDGE_RESOURCE_NUM; i++) {
> > -   res = s->cb_dev->bus->resource[i];
> > -#else
> > -   pci_bus_for_each_resource(s->cb_dev->bus, res, i) {
> >  #endif
> > +
> > +   pci_bus_for_each_resource_p(s->cb_dev->bus, res) {
> > if (!res)
> > continue;
> 
> Doesn't this remove the proper iterator for X86? Even if that is the right
> thing to do, it needs an explict explanation.

I dunno what was in 2010, but reading code now I have found no differences in
the logic on how resources are being iterated in these two pieces of code.

But fine, I will add a line to a commit message about this change.

Considering this is done, can you issue your conditional tag so I will
incorporate it in v3?

-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 4/4] pcmcia: Convert to use pci_bus_for_each_resource_p()

[Andy Shevchenko]

The pci_bus_for_each_resource_p() hides the iterator loop since
it may be not used otherwise. With this, we may drop that iterator
variable definition.

Signed-off-by: Andy Shevchenko 
---
 drivers/pcmcia/rsrc_nonstatic.c | 9 +++--
 drivers/pcmcia/yenta_socket.c   | 3 +--
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/pcmcia/rsrc_nonstatic.c b/drivers/pcmcia/rsrc_nonstatic.c
index ad1141fddb4c..9d92d4bb6239 100644
--- a/drivers/pcmcia/rsrc_nonstatic.c
+++ b/drivers/pcmcia/rsrc_nonstatic.c
@@ -934,7 +934,7 @@ static int adjust_io(struct pcmcia_socket *s, unsigned int 
action, unsigned long
 static int nonstatic_autoadd_resources(struct pcmcia_socket *s)
 {
struct resource *res;
-   int i, done = 0;
+   int done = 0;
 
if (!s->cb_dev || !s->cb_dev->bus)
return -ENODEV;
@@ -960,12 +960,9 @@ static int nonstatic_autoadd_resources(struct 
pcmcia_socket *s)
 */
if (s->cb_dev->bus->number == 0)
return -EINVAL;
-
-   for (i = 0; i < PCI_BRIDGE_RESOURCE_NUM; i++) {
-   res = s->cb_dev->bus->resource[i];
-#else
-   pci_bus_for_each_resource(s->cb_dev->bus, res, i) {
 #endif
+
+   pci_bus_for_each_resource_p(s->cb_dev->bus, res) {
if (!res)
continue;
 
diff --git a/drivers/pcmcia/yenta_socket.c b/drivers/pcmcia/yenta_socket.c
index 3966a6ceb1ac..b200f2b99a7a 100644
--- a/drivers/pcmcia/yenta_socket.c
+++ b/drivers/pcmcia/yenta_socket.c
@@ -673,9 +673,8 @@ static int yenta_search_res(struct yenta_socket *socket, 
struct resource *res,
u32 min)
 {
struct resource *root;
-   int i;
 
-   pci_bus_for_each_resource(socket->dev->bus, root, i) {
+   pci_bus_for_each_resource_p(socket->dev->bus, root) {
if (!root)
continue;
 
-- 
2.35.1

[PATCH v2 3/4] EISA: Convert to use pci_bus_for_each_resource_p()

[Andy Shevchenko]

The pci_bus_for_each_resource_p() hides the iterator loop since
it may be not used otherwise. With this, we may drop that iterator
variable definition.

Signed-off-by: Andy Shevchenko 
---
 drivers/eisa/pci_eisa.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/eisa/pci_eisa.c b/drivers/eisa/pci_eisa.c
index 930c2332c3c4..907b86384396 100644
--- a/drivers/eisa/pci_eisa.c
+++ b/drivers/eisa/pci_eisa.c
@@ -20,8 +20,8 @@ static struct eisa_root_device pci_eisa_root;
 
 static int __init pci_eisa_init(struct pci_dev *pdev)
 {
-   int rc, i;
struct resource *res, *bus_res = NULL;
+   int rc;
 
if ((rc = pci_enable_device (pdev))) {
dev_err(>dev, "Could not enable device\n");
@@ -38,7 +38,7 @@ static int __init pci_eisa_init(struct pci_dev *pdev)
 * eisa_root_register() can only deal with a single io port resource,
*  so we use the first valid io port resource.
 */
-   pci_bus_for_each_resource(pdev->bus, res, i)
+   pci_bus_for_each_resource_p(pdev->bus, res)
if (res && (res->flags & IORESOURCE_IO)) {
bus_res = res;
break;
-- 
2.35.1

[PATCH v2 0/4] PCI: Add pci_dev_for_each_resource() helper and refactor bus one

[Andy Shevchenko]

Provide two new helper macros to iterate over PCI device resources and
convert users.

Looking at it, refactor existing pci_bus_for_each_resource() and convert
users accordingly.

This applies on top of this patch Mika sent out earlier:
https://lore.kernel.org/linux-pci/20221103103254.30497-1-mika.westerb...@linux.intel.com/

Changelog v2:
- refactor to have two macros
- refactor existing pci_bus_for_each_resource() in the same way and
  convert users

Andy Shevchenko (3):
  PCI: Split pci_bus_for_each_resource_p() out of
pci_bus_for_each_resource()
  EISA: Convert to use pci_bus_for_each_resource_p()
  pcmcia: Convert to use pci_bus_for_each_resource_p()

Mika Westerberg (1):
  PCI: Introduce pci_dev_for_each_resource()

 .clang-format  |  3 +++
 arch/alpha/kernel/pci.c|  5 ++---
 arch/arm/kernel/bios32.c   | 16 ++---
 arch/mips/pci/pci-legacy.c |  3 +--
 arch/powerpc/kernel/pci-common.c   |  5 ++---
 arch/sparc/kernel/leon_pci.c   |  5 ++---
 arch/sparc/kernel/pci.c| 10 -
 arch/sparc/kernel/pcic.c   |  5 ++---
 drivers/eisa/pci_eisa.c|  4 ++--
 drivers/pci/bus.c  |  7 +++---
 drivers/pci/hotplug/shpchp_sysfs.c |  8 +++
 drivers/pci/pci.c  |  5 ++---
 drivers/pci/probe.c|  2 +-
 drivers/pci/remove.c   |  5 ++---
 drivers/pci/setup-bus.c| 36 --
 drivers/pci/setup-res.c|  4 +---
 drivers/pci/xen-pcifront.c |  4 +---
 drivers/pcmcia/rsrc_nonstatic.c|  9 +++-
 drivers/pcmcia/yenta_socket.c  |  3 +--
 include/linux/pci.h| 25 +
 20 files changed, 78 insertions(+), 86 deletions(-)

-- 
2.35.1

[PATCH v2 2/4] PCI: Split pci_bus_for_each_resource_p() out of pci_bus_for_each_resource()

[Andy Shevchenko]

Refactor pci_bus_for_each_resource() in the same way as it's done in
pci_dev_for_each_resource() case. This will allow to hide iterator
inside the loop, where it's not used otherwise.

No functional changes intended.

Signed-off-by: Andy Shevchenko 
---
 .clang-format  |  1 +
 drivers/pci/bus.c  |  7 +++
 drivers/pci/hotplug/shpchp_sysfs.c |  8 
 drivers/pci/pci.c  |  5 ++---
 drivers/pci/probe.c|  2 +-
 drivers/pci/setup-bus.c| 10 --
 include/linux/pci.h| 14 ++
 7 files changed, 25 insertions(+), 22 deletions(-)

diff --git a/.clang-format b/.clang-format
index 08d579fea6cf..b61fd8791346 100644
--- a/.clang-format
+++ b/.clang-format
@@ -520,6 +520,7 @@ ForEachMacros:
   - 'of_property_for_each_string'
   - 'of_property_for_each_u32'
   - 'pci_bus_for_each_resource'
+  - 'pci_bus_for_each_resource_p'
   - 'pci_dev_for_each_resource'
   - 'pci_dev_for_each_resource_p'
   - 'pci_doe_for_each_off'
diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 3cef835b375f..fc8e9c11c5f2 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -161,13 +161,13 @@ static int pci_bus_alloc_from_region(struct pci_bus *bus, 
struct resource *res,
void *alignf_data,
struct pci_bus_region *region)
 {
-   int i, ret;
struct resource *r, avail;
resource_size_t max;
+   int ret;
 
type_mask |= IORESOURCE_TYPE_BITS;
 
-   pci_bus_for_each_resource(bus, r, i) {
+   pci_bus_for_each_resource_p(bus, r) {
resource_size_t min_used = min;
 
if (!r)
@@ -264,9 +264,8 @@ bool pci_bus_clip_resource(struct pci_dev *dev, int idx)
struct resource *res = >resource[idx];
struct resource orig_res = *res;
struct resource *r;
-   int i;
 
-   pci_bus_for_each_resource(bus, r, i) {
+   pci_bus_for_each_resource_p(bus, r) {
resource_size_t start, end;
 
if (!r)
diff --git a/drivers/pci/hotplug/shpchp_sysfs.c 
b/drivers/pci/hotplug/shpchp_sysfs.c
index 64beed7a26be..ff04f0c5e7c3 100644
--- a/drivers/pci/hotplug/shpchp_sysfs.c
+++ b/drivers/pci/hotplug/shpchp_sysfs.c
@@ -24,16 +24,16 @@
 static ssize_t show_ctrl(struct device *dev, struct device_attribute *attr, 
char *buf)
 {
struct pci_dev *pdev;
-   int index, busnr;
struct resource *res;
struct pci_bus *bus;
size_t len = 0;
+   int busnr;
 
pdev = to_pci_dev(dev);
bus = pdev->subordinate;
 
len += sysfs_emit_at(buf, len, "Free resources: memory\n");
-   pci_bus_for_each_resource(bus, res, index) {
+   pci_bus_for_each_resource_p(bus, res) {
if (res && (res->flags & IORESOURCE_MEM) &&
!(res->flags & IORESOURCE_PREFETCH)) {
len += sysfs_emit_at(buf, len,
@@ -43,7 +43,7 @@ static ssize_t show_ctrl(struct device *dev, struct 
device_attribute *attr, char
}
}
len += sysfs_emit_at(buf, len, "Free resources: prefetchable memory\n");
-   pci_bus_for_each_resource(bus, res, index) {
+   pci_bus_for_each_resource_p(bus, res) {
if (res && (res->flags & IORESOURCE_MEM) &&
   (res->flags & IORESOURCE_PREFETCH)) {
len += sysfs_emit_at(buf, len,
@@ -53,7 +53,7 @@ static ssize_t show_ctrl(struct device *dev, struct 
device_attribute *attr, char
}
}
len += sysfs_emit_at(buf, len, "Free resources: IO\n");
-   pci_bus_for_each_resource(bus, res, index) {
+   pci_bus_for_each_resource_p(bus, res) {
if (res && (res->flags & IORESOURCE_IO)) {
len += sysfs_emit_at(buf, len,
 "start = %8.8llx, length = 
%8.8llx\n",
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 2127aba3550b..ff5b34337dab 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -782,9 +782,8 @@ struct resource *pci_find_parent_resource(const struct 
pci_dev *dev,
 {
const struct pci_bus *bus = dev->bus;
struct resource *r;
-   int i;
 
-   pci_bus_for_each_resource(bus, r, i) {
+   pci_bus_for_each_resource_p(bus, r) {
if (!r)
continue;
if (resource_contains(r, res)) {
@@ -802,7 +801,7 @@ struct resource *pci_find_parent_resource(const struct 
pci_dev *dev,
 * be both a positively-decoded aperture and a
 * subtractively-decoded region that contain the BAR.
 * We want the positively-decoded one, so this depends
-* on pci_bus_for_each_resource() 

[PATCH v2 1/4] PCI: Introduce pci_dev_for_each_resource()

[Andy Shevchenko]

From: Mika Westerberg 

Instead of open-coding it everywhere introduce a tiny helper that can be
used to iterate over each resource of a PCI device, and convert the most
obvious users into it.

While at it drop doubled empty line before pdev_sort_resources().

No functional changes intended.

Suggested-by: Andy Shevchenko 
Signed-off-by: Mika Westerberg 
Signed-off-by: Andy Shevchenko 
---
 .clang-format|  2 ++
 arch/alpha/kernel/pci.c  |  5 ++---
 arch/arm/kernel/bios32.c | 16 +++-
 arch/mips/pci/pci-legacy.c   |  3 +--
 arch/powerpc/kernel/pci-common.c |  5 ++---
 arch/sparc/kernel/leon_pci.c |  5 ++---
 arch/sparc/kernel/pci.c  | 10 --
 arch/sparc/kernel/pcic.c |  5 ++---
 drivers/pci/remove.c |  5 ++---
 drivers/pci/setup-bus.c  | 26 ++
 drivers/pci/setup-res.c  |  4 +---
 drivers/pci/xen-pcifront.c   |  4 +---
 include/linux/pci.h  | 11 +++
 13 files changed, 47 insertions(+), 54 deletions(-)

diff --git a/.clang-format b/.clang-format
index f98481a53ea8..08d579fea6cf 100644
--- a/.clang-format
+++ b/.clang-format
@@ -520,6 +520,8 @@ ForEachMacros:
   - 'of_property_for_each_string'
   - 'of_property_for_each_u32'
   - 'pci_bus_for_each_resource'
+  - 'pci_dev_for_each_resource'
+  - 'pci_dev_for_each_resource_p'
   - 'pci_doe_for_each_off'
   - 'pcl_for_each_chunk'
   - 'pcl_for_each_segment'
diff --git a/arch/alpha/kernel/pci.c b/arch/alpha/kernel/pci.c
index 64fbfb0763b2..4458eb7f44f0 100644
--- a/arch/alpha/kernel/pci.c
+++ b/arch/alpha/kernel/pci.c
@@ -288,11 +288,10 @@ pcibios_claim_one_bus(struct pci_bus *b)
struct pci_bus *child_bus;
 
list_for_each_entry(dev, >devices, bus_list) {
+   struct resource *r;
int i;
 
-   for (i = 0; i < PCI_NUM_RESOURCES; i++) {
-   struct resource *r = >resource[i];
-
+   pci_dev_for_each_resource(dev, r, i) {
if (r->parent || !r->start || !r->flags)
continue;
if (pci_has_flag(PCI_PROBE_ONLY) ||
diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index e7ef2b5bea9c..5254734b23e6 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -142,15 +142,15 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_WINBOND2, 
PCI_DEVICE_ID_WINBOND2_89C940F,
  */
 static void pci_fixup_dec21285(struct pci_dev *dev)
 {
-   int i;
-
if (dev->devfn == 0) {
+   struct resource *r;
+
dev->class &= 0xff;
dev->class |= PCI_CLASS_BRIDGE_HOST << 8;
-   for (i = 0; i < PCI_NUM_RESOURCES; i++) {
-   dev->resource[i].start = 0;
-   dev->resource[i].end   = 0;
-   dev->resource[i].flags = 0;
+   pci_dev_for_each_resource_p(dev, r) {
+   r->start = 0;
+   r->end = 0;
+   r->flags = 0;
}
}
 }
@@ -162,13 +162,11 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_DEC, 
PCI_DEVICE_ID_DEC_21285, pci_fixup_d
 static void pci_fixup_ide_bases(struct pci_dev *dev)
 {
struct resource *r;
-   int i;
 
if ((dev->class >> 8) != PCI_CLASS_STORAGE_IDE)
return;
 
-   for (i = 0; i < PCI_NUM_RESOURCES; i++) {
-   r = dev->resource + i;
+   pci_dev_for_each_resource_p(dev, r) {
if ((r->start & ~0x80) == 0x374) {
r->start |= 2;
r->end = r->start;
diff --git a/arch/mips/pci/pci-legacy.c b/arch/mips/pci/pci-legacy.c
index 468722c8a5c6..ec2567f8efd8 100644
--- a/arch/mips/pci/pci-legacy.c
+++ b/arch/mips/pci/pci-legacy.c
@@ -249,12 +249,11 @@ static int pcibios_enable_resources(struct pci_dev *dev, 
int mask)
 
pci_read_config_word(dev, PCI_COMMAND, );
old_cmd = cmd;
-   for (idx = 0; idx < PCI_NUM_RESOURCES; idx++) {
+   pci_dev_for_each_resource(dev, r, idx) {
/* Only set up the requested stuff */
if (!(mask & (1<resource[idx];
if (!(r->flags & (IORESOURCE_IO | IORESOURCE_MEM)))
continue;
if ((idx == PCI_ROM_RESOURCE) &&
diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
index d67cf79bf5d0..8ddcfa6bcb50 100644
--- a/arch/powerpc/kernel/pci-common.c
+++ b/arch/powerpc/kernel/pci-common.c
@@ -1452,11 +1452,10 @@ void pcibios_claim_one_bus(struct pci_bus *bus)
struct pci_bus *child_bus;
 
list_for_each_entry(dev, >devices, bus_list) {
+   struct resource *r;
int i;
 
-   for (i = 0; i < PCI_NUM_RESOURCES;

Re: [PATCH v3 7/8] genirq: Return a const cpumask from irq_data_get_affinity_mask

[Andy Shevchenko]

On Fri, Jul 1, 2022 at 10:01 PM Samuel Holland  wrote:
>
> Now that the irq_data_update_affinity helper exists, enforce its use
> by returning a a const cpumask from irq_data_get_affinity_mask.
>
> Since the previous commit already updated places that needed to call
> irq_data_update_affinity, this commit updates the remaining code that
> either did not modify the cpumask or immediately passed the modified
> mask to irq_set_affinity.

When we refer to functions, we use parentheses, e.g. func().

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 03/45] notifier: Add atomic/blocking_notifier_has_unique_priority()

[Andy Shevchenko]

On Thu, Oct 28, 2021 at 12:16:33AM +0300, Dmitry Osipenko wrote:
> Add atomic/blocking_notifier_has_unique_priority() helpers which return
> true if given handler has unique priority.

...

> +/**
> + *   atomic_notifier_has_unique_priority - Checks whether notifier's 
> priority is unique
> + *   @nh: Pointer to head of the atomic notifier chain
> + *   @n: Entry in notifier chain to check
> + *
> + *   Checks whether there is another notifier in the chain with the same 
> priority.
> + *   Must be called in process context.
> + *
> + *   Returns true if priority is unique, false otherwise.

Why this indentation?

> + */
> +bool atomic_notifier_has_unique_priority(struct atomic_notifier_head *nh,
> + struct notifier_block *n)
> +{
> + struct notifier_block **nl = >head;
> + unsigned long flags;
> + bool ret = true;
> +
> + spin_lock_irqsave(>lock, flags);
> +
> + while ((*nl) != NULL && (*nl)->priority >= n->priority) {

' != NULL' is redundant.

> + if ((*nl)->priority == n->priority && (*nl) != n) {
> + ret = false;
> + break;
> + }
> +
> + nl = &((*nl)->next);
> + }
> +
> + spin_unlock_irqrestore(>lock, flags);
> +
> + return ret;
> +}

...

> + /*
> +  * This code gets used during boot-up, when task switching is
> +  * not yet working and interrupts must remain disabled.  At

One space is enough.

> +  * such times we must not call down_write().
> +  */

> + while ((*nl) != NULL && (*nl)->priority >= n->priority) {

' != NULL' is not needed.

> + if ((*nl)->priority == n->priority && (*nl) != n) {
> + ret = false;
> + break;
> + }
> +
> + nl = &((*nl)->next);
> + }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 00/11] PCI: Drop duplicated tracking of a pci_dev's bound driver

[Andy Shevchenko]

On Fri, Oct 15, 2021 at 7:46 PM Bjorn Helgaas  wrote:
> On Wed, Oct 13, 2021 at 04:23:09PM +0300, Andy Shevchenko wrote:

...

> so compared to Uwe's v6, I restored that section to the original code.
> My goal here was to make the patch as simple and easy to review as
> possible.

Thanks for elaboration. I have got it.

...

> You're right, this didn't make much sense in that patch.  I moved the
> line join to the previous patch, which unindented this section and
> made space for this to fit on one line.  Here's the revised commit:
>
>   
> https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/commit/?id=34ab316d7287

Side remark: default without break or return is error prone (okay, to
some extent). Perhaps adding the return statement there will make
things robust and clean.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 00/11] PCI: Drop duplicated tracking of a pci_dev's bound driver

[Andy Shevchenko]

On Wed, Oct 13, 2021 at 06:33:56AM -0500, Bjorn Helgaas wrote:
> On Wed, Oct 13, 2021 at 12:26:42PM +0300, Andy Shevchenko wrote:
> > On Wed, Oct 13, 2021 at 2:33 AM Bjorn Helgaas  wrote:
> > > On Mon, Oct 04, 2021 at 02:59:24PM +0200, Uwe Kleine-König wrote:

...

> > It's a bit unusual. Other to_*_dev() are not NULL-aware IIRC.
> 
> It is a little unusual.  I only found three of 77 that are NULL-aware:
> 
>   to_moxtet_driver()
>   to_siox_driver()
>   to_spi_driver()
> 
> It seems worthwhile to me because it makes the patch and the resulting
> code significantly cleaner.

I'm not objecting the change, just a remark.

...

> > > +   for (id = drv ? drv->id_table : NULL; id && id->vendor; id++)
> > > +   if (id->vendor == vendor && id->device == device)
> > 
> > > +   break;
> > 
> > return true;
> > 
> > > return id && id->vendor;
> > 
> > return false;
> 
> Good cleanup for a follow-up patch, but doesn't seem directly related
> to the objective here.

True. Maybe you can bake one while not forgotten?

...

> > > +   return drv && drv->resume ?
> > > +   drv->resume(pci_dev) : 
> > > pci_pm_reenable_device(pci_dev);
> > 
> > One line?
> 
> I don't think I touched that line.

Then why they are both in + section?

...

> > > +   struct pci_driver *drv = to_pci_driver(dev->dev.driver);
> > > const struct pci_error_handlers *err_handler =
> > > -   dev->dev.driver ? 
> > > to_pci_driver(dev->dev.driver)->err_handler : NULL;
> > > +   drv ? drv->err_handler : NULL;
> > 
> > Isn't dev->driver == to_pci_driver(dev->dev.driver)?
> 
> Yes, I think so, but not sure what you're getting at here, can you
> elaborate?

Getting pointer from another pointer seems waste of resources, why we
can't simply

struct pci_driver *drv = dev->driver;

?

...

> > Stray change? Or is it in a separate patch in your tree?
> 
> Could be skipped.  The string now fits on one line so I combined it to
> make it more greppable.

This is inconsistency in your changes, in one case you are objecting of
doing something close to the changed lines, in the other you are doing
unrelated change.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 00/11] PCI: Drop duplicated tracking of a pci_dev's bound driver

[Andy Shevchenko]

On Wed, Oct 13, 2021 at 2:33 AM Bjorn Helgaas  wrote:
> On Mon, Oct 04, 2021 at 02:59:24PM +0200, Uwe Kleine-König wrote:

> I split some of the bigger patches apart so they only touched one
> driver or subsystem at a time.  I also updated to_pci_driver() so it
> returns NULL when given NULL, which makes some of the validations
> quite a bit simpler, especially in the PM code in pci-driver.c.

It's a bit unusual. Other to_*_dev() are not NULL-aware IIRC.

Below are some comments as well.

...

>  static bool match_id(struct pci_dev *pdev, unsigned short vendor, unsigned 
> short device)
>  {
> +   struct pci_driver *drv = to_pci_driver(pdev->dev.driver);
> const struct pci_device_id *id;
>
> if (pdev->vendor == vendor && pdev->device == device)
> return true;

> +   for (id = drv ? drv->id_table : NULL; id && id->vendor; id++)
> +   if (id->vendor == vendor && id->device == device)

> +   break;

return true;

> return id && id->vendor;

return false;

>  }

...

> +   afu_result = err_handler->error_detected(afu_dev,
> +state);

One line?

...

> device_lock(_dev->dev);
> -   if (vf_dev->dev.driver) {
> +   if (to_pci_driver(vf_dev->dev.driver)) {

Hmm...

...

> +   if (!pci_dev->state_saved && pci_dev->current_state != PCI_D0

> +   && pci_dev->current_state != PCI_UNKNOWN) {

Can we keep && on the previous line?

> +   pci_WARN_ONCE(pci_dev, pci_dev->current_state != prev,
> + "PCI PM: Device state not saved by 
> %pS\n",
> + drv->suspend);
> }

...

> +   return drv && drv->resume ?
> +   drv->resume(pci_dev) : 
> pci_pm_reenable_device(pci_dev);

One line?

...

> +   struct pci_driver *drv = to_pci_driver(dev->dev.driver);
> const struct pci_error_handlers *err_handler =
> -   dev->dev.driver ? 
> to_pci_driver(dev->dev.driver)->err_handler : NULL;
> +   drv ? drv->err_handler : NULL;

Isn't dev->driver == to_pci_driver(dev->dev.driver)?

...

> +   struct pci_driver *drv = to_pci_driver(dev->dev.driver);
> const struct pci_error_handlers *err_handler =
> -   dev->dev.driver ? 
> to_pci_driver(dev->dev.driver)->err_handler : NULL;
> +   drv ? drv->err_handler : NULL;

Ditto.

...

> device_lock(>dev);
> +   pdrv = to_pci_driver(dev->dev.driver);
> if (!pci_dev_set_io_state(dev, state) ||
> -   !dev->dev.driver ||
> -   !(pdrv = to_pci_driver(dev->dev.driver))->err_handler ||

> +   !pdrv ||
> +   !pdrv->err_handler ||

One line now?

> !pdrv->err_handler->error_detected) {

Or this and the previous line?

...

> +   pdrv = to_pci_driver(dev->dev.driver);
> +   if (!pdrv ||
> +   !pdrv->err_handler ||
> !pdrv->err_handler->mmio_enabled)
> goto out;

Ditto.

...

> +   pdrv = to_pci_driver(dev->dev.driver);
> +   if (!pdrv ||
> +   !pdrv->err_handler ||
> !pdrv->err_handler->slot_reset)
> goto out;

Ditto.

...

> if (!pci_dev_set_io_state(dev, pci_channel_io_normal) ||
> -   !dev->dev.driver ||
> -   !(pdrv = to_pci_driver(dev->dev.driver))->err_handler ||
> +   !pdrv ||
> +   !pdrv->err_handler ||
> !pdrv->err_handler->resume)
> goto out;

Ditto.

> -   result = PCI_ERS_RESULT_NONE;
>
> pcidev = pci_get_domain_bus_and_slot(domain, bus, devfn);
> if (!pcidev || !pcidev->dev.driver) {
> dev_err(>xdev->dev, "device or AER driver is NULL\n");
> pci_dev_put(pcidev);
> -   return result;
> +   return PCI_ERS_RESULT_NONE;
> }
> pdrv = to_pci_driver(pcidev->dev.driver);

What about splitting the conditional to two with clear error message
in each and use pci_err() in the second one?

...

> default:
> dev_err(>xdev->dev,
> -   "bad request in aer recovery "
> -   "operation!\n");
> +   "bad request in AER recovery operation!\n");

Stray change? Or is it in a separate patch in your tree?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v15 10/12] swiotlb: Add restricted DMA pool initialization

[Andy Shevchenko]

On Thu, Jun 24, 2021 at 6:59 PM Claire Chang  wrote:
>
> Add the initialization function to create restricted DMA pools from
> matching reserved-memory nodes.
>
> Regardless of swiotlb setting, the restricted DMA pool is preferred if
> available.
>
> The restricted DMA pools provide a basic level of protection against the
> DMA overwriting buffer contents at unexpected times. However, to protect
> against general data leakage and system memory corruption, the system
> needs to provide a way to lock down the memory access, e.g., MPU.





> +static int rmem_swiotlb_device_init(struct reserved_mem *rmem,
> +   struct device *dev)
> +{
> +   struct io_tlb_mem *mem = rmem->priv;
> +   unsigned long nslabs = rmem->size >> IO_TLB_SHIFT;
> +
> +   /*
> +* Since multiple devices can share the same pool, the private data,
> +* io_tlb_mem struct, will be initialized by the first device attached
> +* to it.
> +*/

> +   if (!mem) {

Can it be rather

if (mem)
  goto out_assign;

or so?

> +   mem = kzalloc(struct_size(mem, slots, nslabs), GFP_KERNEL);
> +   if (!mem)
> +   return -ENOMEM;
> +
> +   set_memory_decrypted((unsigned long)phys_to_virt(rmem->base),
> +rmem->size >> PAGE_SHIFT);

Below you are using a macro from pfn.h, but not here, I think it's PFN_DOWN().

> +   swiotlb_init_io_tlb_mem(mem, rmem->base, nslabs, false);
> +   mem->force_bounce = true;
> +   mem->for_alloc = true;
> +
> +   rmem->priv = mem;
> +
> +   if (IS_ENABLED(CONFIG_DEBUG_FS)) {
> +   mem->debugfs =
> +   debugfs_create_dir(rmem->name, debugfs_dir);
> +   swiotlb_create_debugfs_files(mem);
> +   }
> +   }
> +
> +   dev->dma_io_tlb_mem = mem;
> +
> +   return 0;
> +}
> +
> +static void rmem_swiotlb_device_release(struct reserved_mem *rmem,
> +   struct device *dev)
> +{
> +   dev->dma_io_tlb_mem = io_tlb_default_mem;
> +}
> +
> +static const struct reserved_mem_ops rmem_swiotlb_ops = {
> +   .device_init = rmem_swiotlb_device_init,
> +   .device_release = rmem_swiotlb_device_release,
> +};
> +
> +static int __init rmem_swiotlb_setup(struct reserved_mem *rmem)
> +{
> +   unsigned long node = rmem->fdt_node;
> +
> +   if (of_get_flat_dt_prop(node, "reusable", NULL) ||
> +   of_get_flat_dt_prop(node, "linux,cma-default", NULL) ||
> +   of_get_flat_dt_prop(node, "linux,dma-default", NULL) ||
> +   of_get_flat_dt_prop(node, "no-map", NULL))
> +   return -EINVAL;
> +
> +   if (PageHighMem(pfn_to_page(PHYS_PFN(rmem->base {
> +   pr_err("Restricted DMA pool must be accessible within the 
> linear mapping.");
> +   return -EINVAL;
> +   }
> +
> +   rmem->ops = _swiotlb_ops;
> +   pr_info("Reserved memory: created restricted DMA pool at %pa, size 
> %ld MiB\n",
> +   >base, (unsigned long)rmem->size / SZ_1M);

Oh là là, besides explicit casting that I believe can be avoided, %ld
!= unsigned long. Can you check the printk-formats.rst document?

> +   return 0;
> +}
> +
> +RESERVEDMEM_OF_DECLARE(dma, "restricted-dma-pool", rmem_swiotlb_setup);
>  #endif /* CONFIG_DMA_RESTRICTED_POOL */

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/6] PCI: Adapt all code locations to not use struct pci_dev::driver directly

[Andy Shevchenko]

On Tue, Aug 03, 2021 at 12:01:49PM +0200, Uwe Kleine-König wrote:
> This prepares removing the driver member of struct pci_dev which holds the
> same information than struct pci_dev::dev->driver.

...

> + struct pci_driver *pdrv;

Missed blank line here and everywhere else. I don't remember if it's a
checkpatch who complains on this.

> + return (pdev && (pdrv = pci_driver_of_dev(pdev))) ? pdrv->name : 
> "";

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/6] PCI: Provide wrapper to access a pci_dev's bound driver

[Andy Shevchenko]

On Tue, Aug 03, 2021 at 12:01:48PM +0200, Uwe Kleine-König wrote:
> Which driver a device is bound to is available twice: In struct
> pci_dev::dev->driver and in struct pci_dev::driver. To get rid of the
> duplication introduce a wrapper to access struct pci_dev's driver
> member. Once all users are converted the wrapper can be changed to
> calculate the driver using pci_dev::dev->driver.

...

>  #define  to_pci_driver(drv) container_of(drv, struct pci_driver, driver)
> +#define pci_driver_of_dev(pdev) ((pdev)->driver)

Seems like above is (mis)using TAB instead of space after #define. Not sure if
it's good to have them different.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 0/5] PCI: Drop duplicated tracking of a pci_dev's bound driver

[Andy Shevchenko]

On Thu, Jul 29, 2021 at 10:37:35PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> struct pci_dev tracks the bound pci driver twice. This series is about
> removing this duplication.
> 
> The first two patches are just cleanups. The third patch introduces a
> wrapper that abstracts access to struct pci_dev->driver. In the next
> patch (hopefully) all users are converted to use the new wrapper and
> finally the fifth patch removes the duplication.
> 
> Note this series is only build tested (allmodconfig on several
> architectures).
> 
> I'm open to restructure this series if this simplifies things. E.g. the
> use of the new wrapper in drivers/pci could be squashed into the patch
> introducing the wrapper. Patch 4 could be split by maintainer tree or
> squashed into patch 3 completely.

I see only patch 4 and this cover letter...

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 1/1] kernel.h: Split out panic and oops helpers

[Andy Shevchenko]

kernel.h is being used as a dump for all kinds of stuff for a long time.
Here is the attempt to start cleaning it up by splitting out panic and
oops helpers.

There are several purposes of doing this:
- dropping dependency in bug.h
- dropping a loop by moving out panic_notifier.h
- unload kernel.h from something which has its own domain

At the same time convert users tree-wide to use new headers, although
for the time being include new header back to kernel.h to avoid twisted
indirected includes for existing users.

Signed-off-by: Andy Shevchenko 
Reviewed-by: Bjorn Andersson 
Acked-by: Mike Rapoport 
Acked-by: Corey Minyard 
Acked-by: Christian Brauner 
Acked-by: Arnd Bergmann 
Acked-by: Kees Cook 
Acked-by: Wei Liu 
Acked-by: Rasmus Villemoes 
Co-developed-by: Andrew Morton 
Signed-off-by: Andrew Morton 
Acked-by: Sebastian Reichel 
Acked-by: Luis Chamberlain 
Acked-by: Stephen Boyd 
Acked-by: Thomas Bogendoerfer 
Acked-by: Helge Deller  # parisc
---
v3: rebased on top of v5.13-rc1, collected a few more tags

Note WRT Andrew's SoB tag above: I have added it since part of the cases
I took from him. Andrew, feel free to amend or tell me how you want me
to do.

 arch/alpha/kernel/setup.c |  2 +-
 arch/arm64/kernel/setup.c |  1 +
 arch/mips/kernel/relocate.c   |  1 +
 arch/mips/sgi-ip22/ip22-reset.c   |  1 +
 arch/mips/sgi-ip32/ip32-reset.c   |  1 +
 arch/parisc/kernel/pdc_chassis.c  |  1 +
 arch/powerpc/kernel/setup-common.c|  1 +
 arch/s390/kernel/ipl.c|  1 +
 arch/sparc/kernel/sstate.c|  1 +
 arch/um/drivers/mconsole_kern.c   |  1 +
 arch/um/kernel/um_arch.c  |  1 +
 arch/x86/include/asm/desc.h   |  1 +
 arch/x86/kernel/cpu/mshyperv.c|  1 +
 arch/x86/kernel/setup.c   |  1 +
 arch/x86/purgatory/purgatory.c|  2 +
 arch/x86/xen/enlighten.c  |  1 +
 arch/xtensa/platforms/iss/setup.c |  1 +
 drivers/bus/brcmstb_gisb.c|  1 +
 drivers/char/ipmi/ipmi_msghandler.c   |  1 +
 drivers/clk/analogbits/wrpll-cln28hpc.c   |  4 +
 drivers/edac/altera_edac.c|  1 +
 drivers/firmware/google/gsmi.c|  1 +
 drivers/hv/vmbus_drv.c|  1 +
 .../hwtracing/coresight/coresight-cpu-debug.c |  1 +
 drivers/leds/trigger/ledtrig-activity.c   |  1 +
 drivers/leds/trigger/ledtrig-heartbeat.c  |  1 +
 drivers/leds/trigger/ledtrig-panic.c  |  1 +
 drivers/misc/bcm-vk/bcm_vk_dev.c  |  1 +
 drivers/misc/ibmasm/heartbeat.c   |  1 +
 drivers/misc/pvpanic/pvpanic.c|  1 +
 drivers/net/ipa/ipa_smp2p.c   |  1 +
 drivers/parisc/power.c|  1 +
 drivers/power/reset/ltc2952-poweroff.c|  1 +
 drivers/remoteproc/remoteproc_core.c  |  1 +
 drivers/s390/char/con3215.c   |  1 +
 drivers/s390/char/con3270.c   |  1 +
 drivers/s390/char/sclp.c  |  1 +
 drivers/s390/char/sclp_con.c  |  1 +
 drivers/s390/char/sclp_vt220.c|  1 +
 drivers/s390/char/zcore.c |  1 +
 drivers/soc/bcm/brcmstb/pm/pm-arm.c   |  1 +
 drivers/staging/olpc_dcon/olpc_dcon.c |  1 +
 drivers/video/fbdev/hyperv_fb.c   |  1 +
 include/asm-generic/bug.h |  3 +-
 include/linux/kernel.h| 84 +---
 include/linux/panic.h | 98 +++
 include/linux/panic_notifier.h| 12 +++
 kernel/hung_task.c|  1 +
 kernel/kexec_core.c   |  1 +
 kernel/panic.c|  1 +
 kernel/rcu/tree.c |  2 +
 kernel/sysctl.c   |  1 +
 kernel/trace/trace.c  |  1 +
 53 files changed, 167 insertions(+), 85 deletions(-)
 create mode 100644 include/linux/panic.h
 create mode 100644 include/linux/panic_notifier.h

diff --git a/arch/alpha/kernel/setup.c b/arch/alpha/kernel/setup.c
index 03dda3beb3bd..5d1296534682 100644
--- a/arch/alpha/kernel/setup.c
+++ b/arch/alpha/kernel/setup.c
@@ -28,6 +28,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -46,7 +47,6 @@
 #include 
 #include 
 
-extern struct atomic_notifier_head panic_notifier_list;
 static int alpha_panic_event(struct notifier_block *, unsigned long, void *);
 static struct notifier_block alpha_panic_block = {
alpha_panic_event,
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index 61845c0821d9..787bc0f601b3 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -23,6 +23,7 @@
 #include 
 #include 
 #include 
+#include 
 #include

[PATCH v2 1/1] kernel.h: Split out panic and oops helpers

[Andy Shevchenko]

kernel.h is being used as a dump for all kinds of stuff for a long time.
Here is the attempt to start cleaning it up by splitting out panic and
oops helpers.

There are several purposes of doing this:
- dropping dependency in bug.h
- dropping a loop by moving out panic_notifier.h
- unload kernel.h from something which has its own domain

At the same time convert users tree-wide to use new headers, although
for the time being include new header back to kernel.h to avoid twisted
indirected includes for existing users.

Signed-off-by: Andy Shevchenko 
Reviewed-by: Bjorn Andersson 
Acked-by: Mike Rapoport 
Acked-by: Corey Minyard 
Acked-by: Christian Brauner 
Acked-by: Arnd Bergmann 
Acked-by: Kees Cook 
Acked-by: Wei Liu 
Acked-by: Rasmus Villemoes 
Signed-off-by: Andrew Morton 
---
v2:
 - fixed all errors with allmodconfig on x86_64 (Andrew)
 - checked with allyesconfig on x86_64
 - additionally grepped source code for panic notifier list usage
   and converted all users
 - elaborated commit message (Luis)
 - collected given tags (incl. Andrew's SoB, see below)

I added Andrew's SoB since part of the fixes I took from him. Andrew,
feel free to amend or tell me how you want me to do.

 arch/alpha/kernel/setup.c |  2 +-
 arch/arm64/kernel/setup.c |  1 +
 arch/mips/kernel/relocate.c   |  1 +
 arch/mips/sgi-ip22/ip22-reset.c   |  1 +
 arch/mips/sgi-ip32/ip32-reset.c   |  1 +
 arch/parisc/kernel/pdc_chassis.c  |  1 +
 arch/powerpc/kernel/setup-common.c|  1 +
 arch/s390/kernel/ipl.c|  1 +
 arch/sparc/kernel/sstate.c|  1 +
 arch/um/drivers/mconsole_kern.c   |  1 +
 arch/um/kernel/um_arch.c  |  1 +
 arch/x86/include/asm/desc.h   |  1 +
 arch/x86/kernel/cpu/mshyperv.c|  1 +
 arch/x86/kernel/setup.c   |  1 +
 arch/x86/purgatory/purgatory.c|  2 +
 arch/x86/xen/enlighten.c  |  1 +
 arch/xtensa/platforms/iss/setup.c |  1 +
 drivers/bus/brcmstb_gisb.c|  1 +
 drivers/char/ipmi/ipmi_msghandler.c   |  1 +
 drivers/clk/analogbits/wrpll-cln28hpc.c   |  4 +
 drivers/edac/altera_edac.c|  1 +
 drivers/firmware/google/gsmi.c|  1 +
 drivers/hv/vmbus_drv.c|  1 +
 .../hwtracing/coresight/coresight-cpu-debug.c |  1 +
 drivers/leds/trigger/ledtrig-activity.c   |  1 +
 drivers/leds/trigger/ledtrig-heartbeat.c  |  1 +
 drivers/leds/trigger/ledtrig-panic.c  |  1 +
 drivers/misc/bcm-vk/bcm_vk_dev.c  |  1 +
 drivers/misc/ibmasm/heartbeat.c   |  1 +
 drivers/misc/pvpanic/pvpanic.c|  1 +
 drivers/net/ipa/ipa_smp2p.c   |  1 +
 drivers/parisc/power.c|  1 +
 drivers/power/reset/ltc2952-poweroff.c|  1 +
 drivers/remoteproc/remoteproc_core.c  |  1 +
 drivers/s390/char/con3215.c   |  1 +
 drivers/s390/char/con3270.c   |  1 +
 drivers/s390/char/sclp.c  |  1 +
 drivers/s390/char/sclp_con.c  |  1 +
 drivers/s390/char/sclp_vt220.c|  1 +
 drivers/s390/char/zcore.c |  1 +
 drivers/soc/bcm/brcmstb/pm/pm-arm.c   |  1 +
 drivers/staging/olpc_dcon/olpc_dcon.c |  1 +
 drivers/video/fbdev/hyperv_fb.c   |  1 +
 include/asm-generic/bug.h |  3 +-
 include/linux/kernel.h| 84 +---
 include/linux/panic.h | 98 +++
 include/linux/panic_notifier.h| 12 +++
 kernel/hung_task.c|  1 +
 kernel/kexec_core.c   |  1 +
 kernel/panic.c|  1 +
 kernel/rcu/tree.c |  2 +
 kernel/sysctl.c   |  1 +
 kernel/trace/trace.c  |  1 +
 53 files changed, 167 insertions(+), 85 deletions(-)
 create mode 100644 include/linux/panic.h
 create mode 100644 include/linux/panic_notifier.h

diff --git a/arch/alpha/kernel/setup.c b/arch/alpha/kernel/setup.c
index 03dda3beb3bd..5d1296534682 100644
--- a/arch/alpha/kernel/setup.c
+++ b/arch/alpha/kernel/setup.c
@@ -28,6 +28,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -46,7 +47,6 @@
 #include 
 #include 
 
-extern struct atomic_notifier_head panic_notifier_list;
 static int alpha_panic_event(struct notifier_block *, unsigned long, void *);
 static struct notifier_block alpha_panic_block = {
alpha_panic_event,
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
index 61845c0821d9..787bc0f601b3 100644
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -23,6 +23,7 @@
 #include 
 #include 
 #include

Re: [PATCH v3] intel/pinctrl: check REVID register value for device presence

[Andy Shevchenko]

On Thu, Mar 25, 2021 at 10:09:47AM +0100, Roger Pau Monne wrote:
> Use the value read from the REVID register in order to check for the
> presence of the device. A read of all ones is treated as if the device
> is not present, and hence probing is ended.
> 
> This fixes an issue when running as a Xen PVH dom0, where the ACPI
> DSDT table is provided unmodified to dom0 and hence contains the
> pinctrl devices, but the MMIO region(s) containing the device
> registers might not be mapped in the guest physical memory map if such
> region(s) are not exposed on a PCI device BAR or marked as reserved in
> the host memory map.

Applied for fixes, thanks!

> 91d898e51e60 ('pinctrl: intel: Convert capability list to features')
> Suggested-by: Andy Shevchenko 
> Signed-off-by: Roger Pau Monné 
> ---
> Changes since v2:
>  - Return ENODEV.
>  - Adjust code comment.
> 
> Changes since v1:
>  - New in this version.
> ---
> Cc: Mika Westerberg 
> Cc: Andy Shevchenko 
> Cc: Linus Walleij 
> Cc: linux-g...@vger.kernel.org
> ---
>  drivers/pinctrl/intel/pinctrl-intel.c | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c 
> b/drivers/pinctrl/intel/pinctrl-intel.c
> index 8085782cd8f9..9fc5bba514ea 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -1491,8 +1491,13 @@ static int intel_pinctrl_probe(struct platform_device 
> *pdev,
>   if (IS_ERR(regs))
>   return PTR_ERR(regs);
>  
> - /* Determine community features based on the revision */
> + /*
> +  * Determine community features based on the revision.
> +  * A value of all ones means the device is not present.
> +  */
>   value = readl(regs + REVID);
> + if (value == ~0u)
> + return -ENODEV;
>   if (((value & REVID_MASK) >> REVID_SHIFT) >= 0x94) {
>   community->features |= PINCTRL_FEATURE_DEBOUNCE;
>   community->features |= PINCTRL_FEATURE_1K_PD;
> -- 
> 2.30.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RESEND] intel/pinctrl: check capability offset is between MMIO region

[Andy Shevchenko]

On Thu, Mar 25, 2021 at 09:46:46AM +0100, Roger Pau Monné wrote:
> On Wed, Mar 24, 2021 at 06:57:12PM +0200, Andy Shevchenko wrote:
> > On Wed, Mar 24, 2021 at 04:13:59PM +0100, Roger Pau Monné wrote:
> > > On Wed, Mar 24, 2021 at 04:22:44PM +0200, Andy Shevchenko wrote:
> > > > On Wed, Mar 24, 2021 at 02:55:15PM +0100, Roger Pau Monné wrote:
> > > > > On Wed, Mar 24, 2021 at 02:58:07PM +0200, Andy Shevchenko wrote:

...

> > > > Unfortunately it does not expose PCI configuration space.
> > > 
> > > Are those regions supposed to be marked as reserved in the memory map,
> > > or that's left to the discretion of the hardware vendor?
> > 
> > I didn't get. The OS doesn't see them and an internal backbone simply drops 
> > any
> > IO access to that region.
> 
> I'm not sure I understand the above reply. My question was whether the
> MMIO regions used by the pinctrl device (as fetched from the ACPI DSDT
> table) are supposed belong to regions marked as RESERVED in the
> firmware memory map (ie: either the e820 or the EFI one).

I don't actually know. I guess it should be done in order to have ACPI device
a possibility to claim the resource.

> > > > > Doing something like pci_device_is_present would require a register
> > > > > that we know will never return ~0 unless the device is not present. As
> > > > > said above, maybe we could use REVID to that end?
> > > > 
> > > > Yes, that's good, see above.
> > > > 
> > > > WRT capabilities, if we crash we will see the report immediately on the
> > > > hardware which has such an issue. (It's quite unlikely we will ever 
> > > > have one,
> > > > that's why I consider it's not critical)
> > > 
> > > I would rather prefer to not crash, because I think the kernel should
> > > only resort to crashing when there's no alternative, and here it's
> > > perfectly fine to just print an error message and don't load the
> > > driver.
> > 
> > Are we speaking about real hardware that has an issue? I eagerly want to 
> > know
> > what is that beast.
> 
> OK, I'm not going to resend this anymore. I'm happy with just getting
> the first patch in.
> 
> I think you trust the hardware more that I would do, and I also think
> the check added here is very minimal an unintrusive and serves as a
> way to sanitize the data fetched from the hardware in order to prevent
> a kernel page fault if such data turns out to be wrong.
> 
> Taking a reactive approach of requiring a broken piece of hardware to
> exist in order to sanitize a fetched value seems too risky. I could
> add a WARN_ON or similar if you want some kind of splat that's very
> noticeable when this goes wrong but that doesn't end up in a fatal
> kernel page fault.

You found the issue anyway as long as you had a crash, so current code already
proved that it does it work perfectly.

Since I know what hardware this driver is for, I can assure you, that it will
be quite unlikely to have wrong data in the capability register. The data sheet
is crystal clear about the register's contents: on real hardware it must be
present and be set to a sane value.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] intel/pinctrl: check capability offset is between MMIO region

[Andy Shevchenko]

On Wed, Mar 24, 2021 at 04:43:12PM +0100, Roger Pau Monne wrote:
> When parsing the capability list make sure the offset is between the
> MMIO region mapped in 'regs', or else the kernel hits a page fault.
> 
> Adding the check is harmless, and prevents buggy or broken systems
> from crashing the kernel if the capability linked list is somehow
> broken.

I don't think we need a dead code in the kernel. If you have a hardware to show
this issue, I eagerly want to know this!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] intel/pinctrl: check REVID register value for device presence

[Andy Shevchenko]

On Wed, Mar 24, 2021 at 04:43:11PM +0100, Roger Pau Monne wrote:

Thanks for a fix! My comments below.

> Use the value read from the REVID register in order to check for the
> presence of the device. A read of all ones is treated as if the device
> is not present, and hence probing is ended.
> 
> This fixes an issue when running as a Xen PVH dom0, where the ACPI
> DSDT table is provided unmodified to dom0 and hence contains the
> pinctrl devices, but the MMIO region(s) containing the device
> registers might not be mapped in the guest physical memory map if such
> region(s) are not exposed on a PCI device BAR or marked as reserved in
> the host memory map.

Any particular point that we can use in the Fixes tag?

...

> Suggested-by: Andy Shevchenko 

Hmm... was it that address I have used? In any case I think my @linux.intel.com
is better.

...

>   /* Determine community features based on the revision */
>   value = readl(regs + REVID);
> + if (value == ~0u)
> + return -ENODATA;

I think -ENODEV is more appropriate here.
Also comment above should be adjusted to explain this check.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RESEND] intel/pinctrl: check capability offset is between MMIO region

[Andy Shevchenko]

On Wed, Mar 24, 2021 at 04:13:59PM +0100, Roger Pau Monné wrote:
> On Wed, Mar 24, 2021 at 04:22:44PM +0200, Andy Shevchenko wrote:
> > On Wed, Mar 24, 2021 at 02:55:15PM +0100, Roger Pau Monné wrote:
> > > On Wed, Mar 24, 2021 at 02:58:07PM +0200, Andy Shevchenko wrote:
> > > > On Wed, Mar 24, 2021 at 01:31:18PM +0100, Roger Pau Monne wrote:
> > > > Moreover, it seems you are bailing out and basically denying driver to 
> > > > load.
> > > > This does look that capability is simply the first register that blows 
> > > > the setup.
> > > > I think you have to fix something into Xen to avoid loading these 
> > > > drivers or
> > > > check with something like pci_device_is_present() approach.
> > > 
> > > Is there a backing PCI device BAR for those MMIO regions that the
> > > pinctrl driver is trying to access? AFAICT those regions are only
> > > reported in the ACPI DSDT table on the _CRS method of the object (at
> > > least on my system).
> > 
> > Unfortunately it does not expose PCI configuration space.
> 
> Are those regions supposed to be marked as reserved in the memory map,
> or that's left to the discretion of the hardware vendor?

I didn't get. The OS doesn't see them and an internal backbone simply drops any
IO access to that region.

> > > Doing something like pci_device_is_present would require a register
> > > that we know will never return ~0 unless the device is not present. As
> > > said above, maybe we could use REVID to that end?
> > 
> > Yes, that's good, see above.
> > 
> > WRT capabilities, if we crash we will see the report immediately on the
> > hardware which has such an issue. (It's quite unlikely we will ever have 
> > one,
> > that's why I consider it's not critical)
> 
> I would rather prefer to not crash, because I think the kernel should
> only resort to crashing when there's no alternative, and here it's
> perfectly fine to just print an error message and don't load the
> driver.

Are we speaking about real hardware that has an issue? I eagerly want to know
what is that beast.

> IMO I would rather boot without pinctrl than get a panic if
> it turns out pinctrl capabilities list is somehow corrupted.

Again, do you have a hardware that does this?

> It's a
> long shot, but the check added in order to prevent this scenario is
> minimal.

> In any case I will send a new version with the REVID check and this
> current patch.

Okay, let's continue there, but I'm pessimistic about accepting this patch.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RESEND] intel/pinctrl: check capability offset is between MMIO region

[Andy Shevchenko]

On Wed, Mar 24, 2021 at 02:55:15PM +0100, Roger Pau Monné wrote:
> On Wed, Mar 24, 2021 at 02:58:07PM +0200, Andy Shevchenko wrote:
> > On Wed, Mar 24, 2021 at 01:31:18PM +0100, Roger Pau Monne wrote:

...

> What could be done is check whether reading REVID returns ~0 and exit
> at that point, if ~0 will never be a valid value returned by that
> register. I think that should be a separate patch however.

Sounds good to me.

> > Moreover, it seems you are bailing out and basically denying driver to load.
> > This does look that capability is simply the first register that blows the 
> > setup.
> > I think you have to fix something into Xen to avoid loading these drivers or
> > check with something like pci_device_is_present() approach.
> 
> Is there a backing PCI device BAR for those MMIO regions that the
> pinctrl driver is trying to access? AFAICT those regions are only
> reported in the ACPI DSDT table on the _CRS method of the object (at
> least on my system).

Unfortunately it does not expose PCI configuration space.

> Doing something like pci_device_is_present would require a register
> that we know will never return ~0 unless the device is not present. As
> said above, maybe we could use REVID to that end?

Yes, that's good, see above.

WRT capabilities, if we crash we will see the report immediately on the
hardware which has such an issue. (It's quite unlikely we will ever have one,
that's why I consider it's not critical)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RESEND] intel/pinctrl: check capability offset is between MMIO region

[Andy Shevchenko]

On Wed, Mar 24, 2021 at 01:31:18PM +0100, Roger Pau Monne wrote:
> When parsing the capability list make sure the offset is between the
> MMIO region mapped in 'regs', or else the kernel hits a page fault.
> 
> This fault has been seen when running as a Xen PVH dom0, which doesn't
> have the MMIO regions mapped into the domain physical memory map,
> despite having the device reported in the ACPI DSDT table. This
> results in reporting a capability offset of 0x (because the kernel
> is accessing unpopulated memory), and such offset is outside of the
> mapped region.
> 
> Adding the check is harmless, and prevents buggy or broken systems
> from crashing the kernel if the MMIO region is not properly reported.

Thanks for the report.

Looking into the code I would like rather see the explicit comparison to 0x
or ~0 against entire register b/c it's (one of) standard way of devices to tell
that something is not supported.

Moreover, it seems you are bailing out and basically denying driver to load.
This does look that capability is simply the first register that blows the 
setup.
I think you have to fix something into Xen to avoid loading these drivers or
check with something like pci_device_is_present() approach.

> Fixes: 91d898e51e60 ('pinctrl: intel: Convert capability list to features')
> Signed-off-by: Roger Pau Monné 
> ---
> Cc: Mika Westerberg 
> Cc: Andy Shevchenko 
> Cc: Linus Walleij 
> Cc: linux-g...@vger.kernel.org
> ---
> Resend because I've missed adding the maintainers, sorry for the spam.

I have a script to make it easier: 
https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh

-- 
With Best Regards,
Andy Shevchenko

Re: [patch 03/30] genirq: Move irq_set_lockdep_class() to core

[Andy Shevchenko]

On Sat, Dec 12, 2020 at 12:07 AM Thomas Gleixner  wrote:
>
> On Fri, Dec 11 2020 at 22:08, Thomas Gleixner wrote:
>
> > On Fri, Dec 11 2020 at 19:53, Andy Shevchenko wrote:
> >
> >> On Thu, Dec 10, 2020 at 10:14 PM Thomas Gleixner  
> >> wrote:
> >>>
> >>> irq_set_lockdep_class() is used from modules and requires irq_to_desc() to
> >>> be exported. Move it into the core code which lifts another requirement 
> >>> for
> >>> the export.
> >>
> >> ...
> >>
> >>> +   if (IS_ENABLED(CONFIG_LOCKDEP))
> >>> +   __irq_set_lockdep_class(irq, lock_class, request_class);
> >
> > You are right. Let me fix that.
>
> No. I have to correct myself. You're wrong.
>
> The inline is evaluated in the compilation units which include that
> header and because the function declaration is unconditional it is
> happy.
>
> Now the optimizer stage makes the whole thing a NOOP if CONFIG_LOCKDEP=n
> and thereby drops the reference to the function which makes it not
> required for linking.
>
> So in the file where the function is implemented:
>
> #ifdef CONFIG_LOCKDEP
> void __irq_set_lockdep_class()
> {
> }
> #endif
>
> The whole block is either discarded because CONFIG_LOCKDEP is not
> defined or compile if it is defined which makes it available for the
> linker.
>
> And in the latter case the optimizer keeps the call in the inline (it
> optimizes the condition away because it's always true).
>
> So in both cases the compiler and the linker are happy and everything
> works as expected.
>
> It would fail if the header file had the following:
>
> #ifdef CONFIG_LOCKDEP
> void __irq_set_lockdep_class();
> #endif
>
> Because then it would complain about the missing function prototype when
> it evaluates the inline.

I understand that (that's why I put "if even no warning") and what I'm
talking about is the purpose of IS_ENABLED(). It's usually good for
compile testing !CONFIG_FOO cases. But here it seems inconsistent.

The pattern I usually see in the cases like this is

 #ifdef CONFIG_LOCKDEP
 void __irq_set_lockdep_class();
 #else
 static inline void ... {}
 #endif

and call it directly in the caller.

It's not a big deal, so up to you.

-- 
With Best Regards,
Andy Shevchenko

Re: [patch 16/30] mfd: ab8500-debugfs: Remove the racy fiddling with irq_desc

[Andy Shevchenko]

On Thu, Dec 10, 2020 at 9:57 PM Thomas Gleixner  wrote:
>
> First of all drivers have absolutely no business to dig into the internals
> of an irq descriptor. That's core code and subject to change. All of this
> information is readily available to /proc/interrupts in a safe and race
> free way.
>
> Remove the inspection code which is a blatant violation of subsystem
> boundaries and racy against concurrent modifications of the interrupt
> descriptor.
>
> Print the irq line instead so the information can be looked up in a sane
> way in /proc/interrupts.

...

> -   seq_printf(s, "%3i:  %6i %4i",
> +   seq_printf(s, "%3i:  %6i %4i %4i\n",

Seems different specifiers, I think the intention was something like
   seq_printf(s, "%3i:  %4i %6i %4i\n",

>line,
> +  line + irq_first,
>num_interrupts[line],
>    num_wake_interrupts[line]);


-- 
With Best Regards,
Andy Shevchenko

Re: [patch 03/30] genirq: Move irq_set_lockdep_class() to core

[Andy Shevchenko]

On Thu, Dec 10, 2020 at 10:14 PM Thomas Gleixner  wrote:
>
> irq_set_lockdep_class() is used from modules and requires irq_to_desc() to
> be exported. Move it into the core code which lifts another requirement for
> the export.

...

> +   if (IS_ENABLED(CONFIG_LOCKDEP))
> +   __irq_set_lockdep_class(irq, lock_class, request_class);

Maybe I missed something, but even if the compiler does not warn the
use of if IS_ENABLED() with complimentary #ifdef seems inconsistent.

> +#ifdef CONFIG_LOCKDEP
...
> +EXPORT_SYMBOL_GPL(irq_set_lockdep_class);
> +#endif


-- 
With Best Regards,
Andy Shevchenko

Re: [SPECIFICATION RFC] The firmware and bootloader log specification

[Andy Shevchenko]

On Sat, Nov 14, 2020 at 2:01 AM Daniel Kiper  wrote:

...

> The log specification should be as much as possible platform agnostic
> and self contained. The final version of this spec should be merged into
> existing specifications, e.g. UEFI, ACPI, Multiboot2, or be a standalone
> spec, e.g. as a part of OASIS Standards. The former seems better but is
> not perfect too...

With all respect... https://xkcd.com/927/


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 000/141] Fix fall-through warnings for Clang

[Andy Shevchenko]

On Mon, Nov 23, 2020 at 10:39 PM James Bottomley
 wrote:
> On Mon, 2020-11-23 at 19:56 +0100, Miguel Ojeda wrote:
> > On Mon, Nov 23, 2020 at 4:58 PM James Bottomley
> >  wrote:

...

> > But if we do the math, for an author, at even 1 minute per line
> > change and assuming nothing can be automated at all, it would take 1
> > month of work. For maintainers, a couple of trivial lines is noise
> > compared to many other patches.
>
> So you think a one line patch should take one minute to produce ... I
> really don't think that's grounded in reality.  I suppose a one line
> patch only takes a minute to merge with b4 if no-one reviews or tests
> it, but that's not really desirable.

In my practice most of the one line patches were either to fix or to
introduce quite interesting issues.
1 minute is 2-3 orders less than usually needed for such patches.
That's why I don't like churn produced by people who often even didn't
compile their useful contributions.

-- 
With Best Regards,
Andy Shevchenko

Re: [Xen-devel] [PATCH 0/2] Remove support for deprecated %pf and %pF in vsprintf

[Andy Shevchenko]

On Mon, Mar 25, 2019 at 05:13:00PM +0200, Sakari Ailus wrote:

> All other invalid pointer conversion specifiers currently result into a
> warning only. I see that as an orthogonal change to this set. I found
> another issue in checkpatch.pl that may require some discussion; would you
> be ok with addressing this in another set?

If it looks better that way, I have no objection.

-- 
With Best Regards,
Andy Shevchenko



___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 0/2] Remove support for deprecated %pf and %pF in vsprintf

[Andy Shevchenko]

On Sun, Mar 24, 2019 at 11:10:08PM +0200, Sakari Ailus wrote:
> On Fri, Mar 22, 2019 at 07:05:50PM +0200, Andy Shevchenko wrote:
> > On Fri, Mar 22, 2019 at 03:53:50PM +0200, Sakari Ailus wrote:
> > 
> > > Porting a patch
> > > forward should have no issues either as checkpatch.pl has been complaining
> > > of the use of %pf and %pF for a while now.
> > 
> > And that's exactly the reason why I think instead of removing warning on
> > checkpatch, it makes sense to convert to an error for a while. People are
> > tending read documentation on internet and thus might have outdated one. And
> > yes, the compiler doesn't tell a thing about it.
> > 
> > P.S. Though, if majority of people will tell that I'm wrong, then it's okay 
> > to
> > remove.
> 
> I wonder if you wrote this before seeing my other patchset.

Yes, I wrote it before seeing another series.

> What I think could be done is to warn of plain %pf (without following "w")
> in checkpatch.pl, and %pf that is not followed by "w" in the kernel.
> Although we didn't have such checks to begin with. The case is still a
> little bit different as %pf used to be a valid conversion specifier whereas
> %pO likely has never existed.
> 
> So, how about adding such checks in the other set? I can retain %p[fF] check
> here, too, if you like.

Consistency tells me that the warning->error transformation in checkpatch.pl
belongs this series.


-- 
With Best Regards,
Andy Shevchenko



___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 0/2] Remove support for deprecated %pf and %pF in vsprintf

[Andy Shevchenko]

On Fri, Mar 22, 2019 at 03:53:50PM +0200, Sakari Ailus wrote:

> Porting a patch
> forward should have no issues either as checkpatch.pl has been complaining
> of the use of %pf and %pF for a while now.

And that's exactly the reason why I think instead of removing warning on
checkpatch, it makes sense to convert to an error for a while. People are
tending read documentation on internet and thus might have outdated one. And
yes, the compiler doesn't tell a thing about it.

P.S. Though, if majority of people will tell that I'm wrong, then it's okay to
remove.

-- 
With Best Regards,
Andy Shevchenko



___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v2] xen/ACPI: Switch to bitmap_zalloc()

[Andy Shevchenko]

Switch to bitmap_zalloc() to show clearly what we are allocating.
Besides that it returns pointer of bitmap type instead of opaque void *.

Signed-off-by: Andy Shevchenko 
---
- added one more missed conversion
 drivers/xen/xen-acpi-processor.c | 22 +++---
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/xen/xen-acpi-processor.c b/drivers/xen/xen-acpi-processor.c
index fbb9137c7d02..98e35644fda7 100644
--- a/drivers/xen/xen-acpi-processor.c
+++ b/drivers/xen/xen-acpi-processor.c
@@ -410,21 +410,21 @@ static int check_acpi_ids(struct acpi_processor 
*pr_backup)
/* All online CPUs have been processed at this stage. Now verify
 * whether in fact "online CPUs" == physical CPUs.
 */
-   acpi_id_present = kcalloc(BITS_TO_LONGS(nr_acpi_bits), sizeof(unsigned 
long), GFP_KERNEL);
+   acpi_id_present = bitmap_zalloc(nr_acpi_bits, GFP_KERNEL);
if (!acpi_id_present)
return -ENOMEM;
 
-   acpi_id_cst_present = kcalloc(BITS_TO_LONGS(nr_acpi_bits), 
sizeof(unsigned long), GFP_KERNEL);
+   acpi_id_cst_present = bitmap_zalloc(nr_acpi_bits, GFP_KERNEL);
if (!acpi_id_cst_present) {
-   kfree(acpi_id_present);
+   bitmap_free(acpi_id_present);
return -ENOMEM;
}
 
acpi_psd = kcalloc(nr_acpi_bits, sizeof(struct acpi_psd_package),
   GFP_KERNEL);
if (!acpi_psd) {
-   kfree(acpi_id_present);
-   kfree(acpi_id_cst_present);
+   bitmap_free(acpi_id_present);
+   bitmap_free(acpi_id_cst_present);
return -ENOMEM;
}
 
@@ -533,14 +533,14 @@ static int __init xen_acpi_processor_init(void)
return -ENODEV;
 
nr_acpi_bits = get_max_acpi_id() + 1;
-   acpi_ids_done = kcalloc(BITS_TO_LONGS(nr_acpi_bits), sizeof(unsigned 
long), GFP_KERNEL);
+   acpi_ids_done = bitmap_zalloc(nr_acpi_bits, GFP_KERNEL);
if (!acpi_ids_done)
return -ENOMEM;
 
acpi_perf_data = alloc_percpu(struct acpi_processor_performance);
if (!acpi_perf_data) {
pr_debug("Memory allocation error for acpi_perf_data\n");
-   kfree(acpi_ids_done);
+   bitmap_free(acpi_ids_done);
return -ENOMEM;
}
for_each_possible_cpu(i) {
@@ -584,7 +584,7 @@ static int __init xen_acpi_processor_init(void)
 err_out:
/* Freeing a NULL pointer is OK: alloc_percpu zeroes. */
free_acpi_perf_data();
-   kfree(acpi_ids_done);
+   bitmap_free(acpi_ids_done);
return rc;
 }
 static void __exit xen_acpi_processor_exit(void)
@@ -592,9 +592,9 @@ static void __exit xen_acpi_processor_exit(void)
int i;
 
unregister_syscore_ops(_syscore_ops);
-   kfree(acpi_ids_done);
-   kfree(acpi_id_present);
-   kfree(acpi_id_cst_present);
+   bitmap_free(acpi_ids_done);
+   bitmap_free(acpi_id_present);
+   bitmap_free(acpi_id_cst_present);
kfree(acpi_psd);
for_each_possible_cpu(i)
acpi_processor_unregister_performance(i);
-- 
2.20.1


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v1] xen/ACPI: Switch to bitmap_zalloc()

[Andy Shevchenko]

Switch to bitmap_zalloc() to show clearly what we are allocating.
Besides that it returns pointer of bitmap type instead of opaque void *.

Signed-off-by: Andy Shevchenko 
---
 drivers/xen/xen-acpi-processor.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/xen/xen-acpi-processor.c b/drivers/xen/xen-acpi-processor.c
index fbb9137c7d02..a6089851628f 100644
--- a/drivers/xen/xen-acpi-processor.c
+++ b/drivers/xen/xen-acpi-processor.c
@@ -410,21 +410,21 @@ static int check_acpi_ids(struct acpi_processor 
*pr_backup)
/* All online CPUs have been processed at this stage. Now verify
 * whether in fact "online CPUs" == physical CPUs.
 */
-   acpi_id_present = kcalloc(BITS_TO_LONGS(nr_acpi_bits), sizeof(unsigned 
long), GFP_KERNEL);
+   acpi_id_present = bitmap_zalloc(nr_acpi_bits, GFP_KERNEL);
if (!acpi_id_present)
return -ENOMEM;
 
-   acpi_id_cst_present = kcalloc(BITS_TO_LONGS(nr_acpi_bits), 
sizeof(unsigned long), GFP_KERNEL);
+   acpi_id_cst_present = bitmap_zalloc(nr_acpi_bits, GFP_KERNEL);
if (!acpi_id_cst_present) {
-   kfree(acpi_id_present);
+   bitmap_free(acpi_id_present);
return -ENOMEM;
}
 
acpi_psd = kcalloc(nr_acpi_bits, sizeof(struct acpi_psd_package),
   GFP_KERNEL);
if (!acpi_psd) {
-   kfree(acpi_id_present);
-   kfree(acpi_id_cst_present);
+   bitmap_free(acpi_id_present);
+   bitmap_free(acpi_id_cst_present);
return -ENOMEM;
}
 
@@ -593,8 +593,8 @@ static void __exit xen_acpi_processor_exit(void)
 
unregister_syscore_ops(_syscore_ops);
kfree(acpi_ids_done);
-   kfree(acpi_id_present);
-   kfree(acpi_id_cst_present);
+   bitmap_free(acpi_id_present);
+   bitmap_free(acpi_id_cst_present);
kfree(acpi_psd);
for_each_possible_cpu(i)
acpi_processor_unregister_performance(i);
-- 
2.20.1


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 0/3] xen: re-enable booting as Xen PVH guest

[Andy Shevchenko]

On Mon, Feb 19, 2018 at 12:27 PM, Rafael J. Wysocki
<rafael.j.wyso...@intel.com> wrote:
> On 2/19/2018 11:09 AM, Juergen Gross wrote:
>>
>> The Xen PVH boot protocol passes vital information to the kernel via
>> a start_info block. One of the data transferred is the physical address
>> of the RSDP table.
>>
>> Unfortunately PVH support in the kernel didn't use that passed address
>> for RSDP, but relied on the legacy mechanism searching for the RSDP in
>> low memory. After a recent change in Xen putting the RSDP to a higher
>> address booting as PVH guest is now failing.
>>
>> This small series repairs that by passing the RSDP address from the
>> start_info block to ACPI handling.
>>
>> Changes in V3:
>> - instead of using a weak function add a function pointer to x86_init
>>for obtaining the RSDP address
>>
>> Juergen Gross (3):
>>acpi: introduce acpi_arch_get_root_pointer() for getting rsdp address
>>x86/acpi: add a new x86_init_acpi structure to x86_init_ops
>>x86/xen: add pvh specific rsdp address retrieval function
>>
>>   arch/x86/include/asm/acpi.h |  7 +++
>>   arch/x86/include/asm/x86_init.h |  9 +
>>   arch/x86/kernel/x86_init.c  |  5 +
>>   arch/x86/xen/enlighten_pvh.c| 14 +++---
>>   drivers/acpi/osl.c  |  5 -
>>   include/linux/acpi.h|  7 +++
>>   6 files changed, 43 insertions(+), 4 deletions(-)
>>
> The series is fine by me:
>
> Acked-by: Rafael J. Wysocki <rafael.j.wyso...@intel.com>

Don't know much about Xen, though the ACPI / x86 stubs are exactly
what I'm wanting!
Thanks.

Reviewed-by: Andy Shevchenko <andy.shevche...@gmail.com>

-- 
With Best Regards,
Andy Shevchenko

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 1/2] x86/acpi: add retrieval function for rsdp address

[Andy Shevchenko]

On Thu, Feb 1, 2018 at 9:57 AM, Rafael J. Wysocki <raf...@kernel.org> wrote:
> On Wed, Jan 31, 2018 at 4:43 PM, Andy Shevchenko
> <andy.shevche...@gmail.com> wrote:
>> On Mon, Jan 29, 2018 at 5:02 AM, Rafael J. Wysocki <raf...@kernel.org> wrote:
>>> On Sun, Jan 28, 2018 at 4:04 PM, Andy Shevchenko
>>> <andy.shevche...@gmail.com> wrote:

>> Instead of declaring function as __weak, establish a new struct for
>> ACPI related stubs and incorporate it into x86_init.
>>
>> That is my proposal. I think I would go this way in my case where I
>> need to treat differently ACPI HW reduced initialization of legacy
>> devices.
>
> IOW you'd like to have a set of ACPI init callbacks that could be
> defined by an arch, right?

Correct!

And since there is another potential user (Xen) for this approach I
consider it a good chance to be chosen.
Though I have no idea if Xen can do things differently.

-- 
With Best Regards,
Andy Shevchenko

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 1/2] x86/acpi: add retrieval function for rsdp address

[Andy Shevchenko]

On Mon, Jan 29, 2018 at 5:02 AM, Rafael J. Wysocki <raf...@kernel.org> wrote:
> On Sun, Jan 28, 2018 at 4:04 PM, Andy Shevchenko
> <andy.shevche...@gmail.com> wrote:
>> On Fri, Jan 26, 2018 at 8:21 PM, Juergen Gross <jgr...@suse.com> wrote:
>>> On 26/01/18 19:08, Andy Shevchenko wrote:
>>>> On Thu, Jan 25, 2018 at 4:36 PM, Juergen Gross <jgr...@suse.com> wrote:

>>>> The problem with weak functions that we can't have more than one
>>>> implementation per kernel while we would like to built several code
>>>> paths.
>>>>
>>>> I have stumbled on the similar stuff and realize that.
>>>>
>>>> Perhaps, one of the solution is to have an additional struct under
>>>> x86_init to alternate ACPI related stuff.
>>>
>>> I think we can go that route when another user of that interface is
>>> appearing.
>>
>> Why not to establish the struct? At least this route I would like to
>> go with [1].
>>
>> [1]: https://lkml.org/lkml/2018/1/17/834
>
> Maybe I'm a bit slow today, but care to explain what exactly you mean?

Instead of declaring function as __weak, establish a new struct for
ACPI related stubs and incorporate it into x86_init.

That is my proposal. I think I would go this way in my case where I
need to treat differently ACPI HW reduced initialization of legacy
devices.

-- 
With Best Regards,
Andy Shevchenko

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v2 1/2] x86/acpi: add retrieval function for rsdp address

[Andy Shevchenko]

On Mon, Jan 29, 2018 at 5:01 AM, Rafael J. Wysocki <raf...@kernel.org> wrote:
> On Fri, Jan 26, 2018 at 7:08 PM, Andy Shevchenko
> <andy.shevche...@gmail.com> wrote:

>> I have stumbled on the similar stuff and realize that.
>>
>> Perhaps, one of the solution is to have an additional struct under
>> x86_init to alternate ACPI related stuff.
>
> I'm not sure if that really is a problem in this particular case.
>
> Why would you want to use different RSDP retrieval functions for one arch?

I was not clear. I'm talking about approach struct x86_init vs. __weak function.
In my case it has nothing to do with RDSP, but with ACPI stubs.

-- 
With Best Regards,
Andy Shevchenko

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel