[PATCH v1 3/7] resource: Introduce resource_union() for overlapping resources

[PATCH v1 3/7] resource: Introduce resource_union() for overlapping resources

[Andy Shevchenko]

Some already present users may utilize resource_union() helper.
Provide it for them and for wider use in the future.

Deliberately avoid min()/max() macro as they are still parts of
kernel.h which is quite a burden to be included here in order
to avoid circular dependencies.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: linux-pci@vger.kernel.org
---
 include/linux/ioport.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index 0193987b9968..c98df0ec7422 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -232,6 +232,16 @@ static inline bool resource_overlaps(struct resource *r1, struct resource *r2)
        return (r1->start <= r2->end && r1->end >= r2->start);
 }
 
+static inline bool
+resource_union(struct resource *r1, struct resource *r2, struct resource *r)
+{
+	if (!resource_overlaps(r1, r2))
+		return false;
+	r->start = r2->start < r1->start ? r2->start : r1->start;
+	r->end = r2->end > r1->end ? r2->end : r1->end;
+	return true;
+}
+
 /* Convenience shorthand with allocation */
 #define request_region(start,n,name)		__request_region(&ioport_resource, (start), (n), (name), 0)
 #define request_muxed_region(start,n,name)	__request_region(&ioport_resource, (start), (n), (name), IORESOURCE_MUXED)
-- 
2.28.0

Re: [PATCH v1 3/7] resource: Introduce resource_union() for overlapping resources

[Rafael J. Wysocki]

On Thu, Aug 13, 2020 at 7:57 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> Some already present users may utilize resource_union() helper.
> Provide it for them and for wider use in the future.
>
> Deliberately avoid min()/max() macro as they are still parts of
> kernel.h which is quite a burden to be included here in order
> to avoid circular dependencies.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: linux-pci@vger.kernel.org
> ---
>  include/linux/ioport.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/include/linux/ioport.h b/include/linux/ioport.h
> index 0193987b9968..c98df0ec7422 100644
> --- a/include/linux/ioport.h
> +++ b/include/linux/ioport.h
> @@ -232,6 +232,16 @@ static inline bool resource_overlaps(struct resource *r1, struct resource *r2)
>         return (r1->start <= r2->end && r1->end >= r2->start);
>  }
>
> +static inline bool
> +resource_union(struct resource *r1, struct resource *r2, struct resource *r)
> +{
> +       if (!resource_overlaps(r1, r2))
> +               return false;

I tend to add empty lines after return statements like this to make
them more clearly visible.

> +       r->start = r2->start < r1->start ? r2->start : r1->start;
> +       r->end = r2->end > r1->end ? r2->end : r1->end;

Well, what about using min() and max() here?

> +       return true;
> +}
> +
>  /* Convenience shorthand with allocation */
>  #define request_region(start,n,name)           __request_region(&ioport_resource, (start), (n), (name), 0)
>  #define request_muxed_region(start,n,name)     __request_region(&ioport_resource, (start), (n), (name), IORESOURCE_MUXED)
> --
> 2.28.0
>

Re: [PATCH v1 3/7] resource: Introduce resource_union() for overlapping resources

[Andy Shevchenko]

On Fri, Aug 14, 2020 at 05:23:07PM +0200, Rafael J. Wysocki wrote:
> On Thu, Aug 13, 2020 at 7:57 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > Some already present users may utilize resource_union() helper.
> > Provide it for them and for wider use in the future.
> >
> > Deliberately avoid min()/max() macro as they are still parts of
> > kernel.h which is quite a burden to be included here in order
> > to avoid circular dependencies.

...

> > +       if (!resource_overlaps(r1, r2))
> > +               return false;
> 
> I tend to add empty lines after return statements like this to make
> them more clearly visible.

Okay!

> > +       r->start = r2->start < r1->start ? r2->start : r1->start;
> > +       r->end = r2->end > r1->end ? r2->end : r1->end;
> 
> Well, what about using min() and max() here?

I devoted one paragraph in the commit message to answer this. The kernel.h
(which I'm planning to split at some point) is a monster which brings more pain
than solves here. Note, this is a header file and it's quite clean from
dependencies perspective.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 3/7] resource: Introduce resource_union() for overlapping resources

[Rafael J. Wysocki]

On Fri, Aug 14, 2020 at 5:37 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Aug 14, 2020 at 05:23:07PM +0200, Rafael J. Wysocki wrote:
> > On Thu, Aug 13, 2020 at 7:57 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > Some already present users may utilize resource_union() helper.
> > > Provide it for them and for wider use in the future.
> > >
> > > Deliberately avoid min()/max() macro as they are still parts of
> > > kernel.h which is quite a burden to be included here in order
> > > to avoid circular dependencies.
>
> ...
>
> > > +       if (!resource_overlaps(r1, r2))
> > > +               return false;
> >
> > I tend to add empty lines after return statements like this to make
> > them more clearly visible.
>
> Okay!
>
> > > +       r->start = r2->start < r1->start ? r2->start : r1->start;
> > > +       r->end = r2->end > r1->end ? r2->end : r1->end;
> >
> > Well, what about using min() and max() here?
>
> I devoted one paragraph in the commit message to answer this. The kernel.h
> (which I'm planning to split at some point) is a monster which brings more pain
> than solves here. Note, this is a header file and it's quite clean from
> dependencies perspective.

But this is code duplication (even if really small) and it is not
entirely clean too.

Maybe move the definitions of min() and max() to a separate header file?

Re: [PATCH v1 3/7] resource: Introduce resource_union() for overlapping resources

[Andy Shevchenko]

On Fri, Aug 14, 2020 at 06:09:53PM +0200, Rafael J. Wysocki wrote:
> On Fri, Aug 14, 2020 at 5:37 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Fri, Aug 14, 2020 at 05:23:07PM +0200, Rafael J. Wysocki wrote:
> > > On Thu, Aug 13, 2020 at 7:57 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:

...

> > > Well, what about using min() and max() here?
> >
> > I devoted one paragraph in the commit message to answer this. The kernel.h
> > (which I'm planning to split at some point) is a monster which brings more pain
> > than solves here. Note, this is a header file and it's quite clean from
> > dependencies perspective.
> 
> But this is code duplication (even if really small) and it is not
> entirely clean too.
> 
> Maybe move the definitions of min() and max() to a separate header file?

That is the plan in the kernel.h splitting project. But do you want me to do it
here? I can try to bring that patch into this series.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 3/7] resource: Introduce resource_union() for overlapping resources

[Rafael J. Wysocki]

On Fri, Aug 14, 2020 at 6:23 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Fri, Aug 14, 2020 at 06:09:53PM +0200, Rafael J. Wysocki wrote:
> > On Fri, Aug 14, 2020 at 5:37 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Fri, Aug 14, 2020 at 05:23:07PM +0200, Rafael J. Wysocki wrote:
> > > > On Thu, Aug 13, 2020 at 7:57 PM Andy Shevchenko
> > > > <andriy.shevchenko@linux.intel.com> wrote:
>
> ...
>
> > > > Well, what about using min() and max() here?
> > >
> > > I devoted one paragraph in the commit message to answer this. The kernel.h
> > > (which I'm planning to split at some point) is a monster which brings more pain
> > > than solves here. Note, this is a header file and it's quite clean from
> > > dependencies perspective.
> >
> > But this is code duplication (even if really small) and it is not
> > entirely clean too.
> >
> > Maybe move the definitions of min() and max() to a separate header file?
>
> That is the plan in the kernel.h splitting project. But do you want me to do it
> here? I can try to bring that patch into this series.

Well, ostensibly the purpose of this series is to reduce code
duplication, but if it adds code duplication, that kind of defeats the
purpose IMO.

Re: [PATCH v1 3/7] resource: Introduce resource_union() for overlapping resources

[Andy Shevchenko]

On Fri, Aug 14, 2020 at 07:17:18PM +0200, Rafael J. Wysocki wrote:
> On Fri, Aug 14, 2020 at 6:23 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Fri, Aug 14, 2020 at 06:09:53PM +0200, Rafael J. Wysocki wrote:
> > > On Fri, Aug 14, 2020 at 5:37 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Fri, Aug 14, 2020 at 05:23:07PM +0200, Rafael J. Wysocki wrote:
> > > > > On Thu, Aug 13, 2020 at 7:57 PM Andy Shevchenko
> > > > > <andriy.shevchenko@linux.intel.com> wrote:
> >
> > ...
> >
> > > > > Well, what about using min() and max() here?
> > > >
> > > > I devoted one paragraph in the commit message to answer this. The kernel.h
> > > > (which I'm planning to split at some point) is a monster which brings more pain
> > > > than solves here. Note, this is a header file and it's quite clean from
> > > > dependencies perspective.
> > >
> > > But this is code duplication (even if really small) and it is not
> > > entirely clean too.
> > >
> > > Maybe move the definitions of min() and max() to a separate header file?
> >
> > That is the plan in the kernel.h splitting project. But do you want me to do it
> > here? I can try to bring that patch into this series.
> 
> Well, ostensibly the purpose of this series is to reduce code
> duplication, but if it adds code duplication, that kind of defeats the
> purpose IMO.

Okay, I will append minmax.h split in v2.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 5/7] PCI/ACPI: Replace open coded variant of resource_union()

[Andy Shevchenko]

Since we have resource_union() helper, let's utilize it here.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: linux-pci@vger.kernel.org
---
 drivers/acpi/pci_root.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index f90e841c59f5..2a6a741896de 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -724,9 +724,7 @@ static void acpi_pci_root_validate_resources(struct device *dev,
 			 * our resources no longer match the ACPI _CRS, but
 			 * the kernel resource tree doesn't allow overlaps.
 			 */
-			if (resource_overlaps(res1, res2)) {
-				res2->start = min(res1->start, res2->start);
-				res2->end = max(res1->end, res2->end);
+			if (resource_union(res1, res2, res2)) {
 				dev_info(dev, "host bridge window expanded to %pR; %pR ignored\n",
 					 res2, res1);
 				free = true;
-- 
2.28.0

Re: [PATCH v1 5/7] PCI/ACPI: Replace open coded variant of resource_union()

[Rafael J. Wysocki]

On Thu, Aug 13, 2020 at 7:57 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> Since we have resource_union() helper, let's utilize it here.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: linux-pci@vger.kernel.org

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/acpi/pci_root.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index f90e841c59f5..2a6a741896de 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -724,9 +724,7 @@ static void acpi_pci_root_validate_resources(struct device *dev,
>                          * our resources no longer match the ACPI _CRS, but
>                          * the kernel resource tree doesn't allow overlaps.
>                          */
> -                       if (resource_overlaps(res1, res2)) {
> -                               res2->start = min(res1->start, res2->start);
> -                               res2->end = max(res1->end, res2->end);
> +                       if (resource_union(res1, res2, res2)) {
>                                 dev_info(dev, "host bridge window expanded to %pR; %pR ignored\n",
>                                          res2, res1);
>                                 free = true;
> --
> 2.28.0
>

[PATCH v1 6/7] PCI/ACPI: Fix description of @handle for acpi_is_root_bridge()

[Andy Shevchenko]

Fix description of handle parameter in documentation of acpi_is_root_bridge().
Otherwise we get the following warning:

  CHECK   drivers/acpi/pci_root.c
  drivers/acpi/pci_root.c:71: warning: Function parameter or member 'handle' not described in 'acpi_is_root_bridge'

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: linux-pci@vger.kernel.org
---
 drivers/acpi/pci_root.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index 2a6a741896de..f723679954d7 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -62,7 +62,7 @@ static DEFINE_MUTEX(osc_lock);
 
 /**
  * acpi_is_root_bridge - determine whether an ACPI CA node is a PCI root bridge
- * @handle - the ACPI CA node in question.
+ * @handle: the ACPI CA node in question.
  *
  * Note: we could make this API take a struct acpi_device * instead, but
  * for now, it's more convenient to operate on an acpi_handle.
-- 
2.28.0

Re: [PATCH v1 6/7] PCI/ACPI: Fix description of @handle for acpi_is_root_bridge()

[Rafael J. Wysocki]

On Thu, Aug 13, 2020 at 7:57 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> Fix description of handle parameter in documentation of acpi_is_root_bridge().
> Otherwise we get the following warning:
>
>   CHECK   drivers/acpi/pci_root.c
>   drivers/acpi/pci_root.c:71: warning: Function parameter or member 'handle' not described in 'acpi_is_root_bridge'

I'm not sure why this patch doesn't go by itself.  It appears to be
immediately applicable.

I'll apply it next week if Bjorn doesn't object.

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: linux-pci@vger.kernel.org
> ---
>  drivers/acpi/pci_root.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index 2a6a741896de..f723679954d7 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -62,7 +62,7 @@ static DEFINE_MUTEX(osc_lock);
>
>  /**
>   * acpi_is_root_bridge - determine whether an ACPI CA node is a PCI root bridge
> - * @handle - the ACPI CA node in question.
> + * @handle: the ACPI CA node in question.
>   *
>   * Note: we could make this API take a struct acpi_device * instead, but
>   * for now, it's more convenient to operate on an acpi_handle.
> --
> 2.28.0
>

Re: [PATCH v1 6/7] PCI/ACPI: Fix description of @handle for acpi_is_root_bridge()

[Andy Shevchenko]

On Fri, Aug 14, 2020 at 05:11:55PM +0200, Rafael J. Wysocki wrote:
> On Thu, Aug 13, 2020 at 7:57 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > Fix description of handle parameter in documentation of acpi_is_root_bridge().
> > Otherwise we get the following warning:
> >
> >   CHECK   drivers/acpi/pci_root.c
> >   drivers/acpi/pci_root.c:71: warning: Function parameter or member 'handle' not described in 'acpi_is_root_bridge'
> 
> I'm not sure why this patch doesn't go by itself.  It appears to be
> immediately applicable.

Just appears when I working on the rest of the series. Can be standalone if it
makes more sense.

> I'll apply it next week if Bjorn doesn't object.

At least fine with me, thanks!

> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: linux-pci@vger.kernel.org
> > ---
> >  drivers/acpi/pci_root.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> > index 2a6a741896de..f723679954d7 100644
> > --- a/drivers/acpi/pci_root.c
> > +++ b/drivers/acpi/pci_root.c
> > @@ -62,7 +62,7 @@ static DEFINE_MUTEX(osc_lock);
> >
> >  /**
> >   * acpi_is_root_bridge - determine whether an ACPI CA node is a PCI root bridge
> > - * @handle - the ACPI CA node in question.
> > + * @handle: the ACPI CA node in question.
> >   *
> >   * Note: we could make this API take a struct acpi_device * instead, but
> >   * for now, it's more convenient to operate on an acpi_handle.
> > --
> > 2.28.0
> >

-- 
With Best Regards,
Andy Shevchenko