[PATCH v1 1/7] resource: Simplify region_intersects() by reducing conditionals

[PATCH v1 1/7] resource: Simplify region_intersects() by reducing conditionals

[Andy Shevchenko]

Now we have for 'other' and 'type' variables

other	type	return
  0	  0	REGION_DISJOINT
  0	  x	REGION_INTERSECTS
  x	  0	REGION_DISJOINT
  x	  x	REGION_MIXED

Obviously it's easier to check 'type' for 0 first instead of
currently checked 'other'.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 kernel/resource.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/kernel/resource.c b/kernel/resource.c
index 841737bbda9e..70575a61bf20 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -554,13 +554,10 @@ int region_intersects(resource_size_t start, size_t size, unsigned long flags,
 	}
 	read_unlock(&resource_lock);
 
-	if (other == 0)
-		return type ? REGION_INTERSECTS : REGION_DISJOINT;
+	if (type == 0)
+		return REGION_DISJOINT;
 
-	if (type)
-		return REGION_MIXED;
-
-	return REGION_DISJOINT;
+	return (other == 0) ? REGION_INTERSECTS : REGION_MIXED;
 }
 EXPORT_SYMBOL_GPL(region_intersects);
 
-- 
2.28.0

[PATCH v1 2/7] resource: Group resource_overlaps() with other inline helpers

[Andy Shevchenko]

For better maintenance group resource_overlaps() with other inline helpers.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 include/linux/ioport.h | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index 6c2b06fe8beb..0193987b9968 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -226,6 +226,11 @@ static inline bool resource_contains(struct resource *r1, struct resource *r2)
 	return r1->start <= r2->start && r1->end >= r2->end;
 }
 
+/* True if any part of r1 overlaps r2 */
+static inline bool resource_overlaps(struct resource *r1, struct resource *r2)
+{
+       return (r1->start <= r2->end && r1->end >= r2->start);
+}
 
 /* Convenience shorthand with allocation */
 #define request_region(start,n,name)		__request_region(&ioport_resource, (start), (n), (name), 0)
@@ -291,12 +296,6 @@ extern int
 walk_iomem_res_desc(unsigned long desc, unsigned long flags, u64 start, u64 end,
 		    void *arg, int (*func)(struct resource *, void *));
 
-/* True if any part of r1 overlaps r2 */
-static inline bool resource_overlaps(struct resource *r1, struct resource *r2)
-{
-       return (r1->start <= r2->end && r1->end >= r2->start);
-}
-
 struct resource *devm_request_free_mem_region(struct device *dev,
 		struct resource *base, unsigned long size);
 struct resource *request_free_mem_region(struct resource *base,
-- 
2.28.0

[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

[PATCH v1 4/7] resource: Introduce resource_intersection() for overlapping resources

[Andy Shevchenko]

There will be at least one user that can utilize new helper.
Provide the helper for future user and for wider use.

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>
---
 include/linux/ioport.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index c98df0ec7422..1d5ab2e7f9eb 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_intersection(struct resource *r1, struct resource *r2, struct resource *r)
+{
+	if (!resource_overlaps(r1, r2))
+		return false;
+	r->start = r1->start > r2->start ? r1->start : r2->start;
+	r->end = r1->end < r2->end ? r1->end : r2->end;
+	return true;
+}
+
 static inline bool
 resource_union(struct resource *r1, struct resource *r2, struct resource *r)
 {
-- 
2.28.0

[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

[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

[PATCH v1 7/7] ACPI: watchdog: 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: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/acpi/acpi_watchdog.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/acpi/acpi_watchdog.c b/drivers/acpi/acpi_watchdog.c
index 5c1e9ea43123..ca28183f4d13 100644
--- a/drivers/acpi/acpi_watchdog.c
+++ b/drivers/acpi/acpi_watchdog.c
@@ -151,11 +151,7 @@ void __init acpi_watchdog_init(void)
 		found = false;
 		resource_list_for_each_entry(rentry, &resource_list) {
 			if (rentry->res->flags == res.flags &&
-			    resource_overlaps(rentry->res, &res)) {
-				if (res.start < rentry->res->start)
-					rentry->res->start = res.start;
-				if (res.end > rentry->res->end)
-					rentry->res->end = res.end;
+			    resource_union(rentry->res, &res, rentry->res)) {
 				found = true;
 				break;
 			}
-- 
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 1/7] resource: Simplify region_intersects() by reducing conditionals

[Rafael J. Wysocki]

On Thu, Aug 13, 2020 at 7:57 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> Now we have for 'other' and 'type' variables
>
> other   type    return
>   0       0     REGION_DISJOINT
>   0       x     REGION_INTERSECTS
>   x       0     REGION_DISJOINT
>   x       x     REGION_MIXED
>
> Obviously it's easier to check 'type' for 0 first instead of
> currently checked 'other'.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  kernel/resource.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/resource.c b/kernel/resource.c
> index 841737bbda9e..70575a61bf20 100644
> --- a/kernel/resource.c
> +++ b/kernel/resource.c
> @@ -554,13 +554,10 @@ int region_intersects(resource_size_t start, size_t size, unsigned long flags,
>         }
>         read_unlock(&resource_lock);
>
> -       if (other == 0)
> -               return type ? REGION_INTERSECTS : REGION_DISJOINT;
> +       if (type == 0)
> +               return REGION_DISJOINT;
>
> -       if (type)
> -               return REGION_MIXED;
> -
> -       return REGION_DISJOINT;
> +       return (other == 0) ? REGION_INTERSECTS : REGION_MIXED;

The parens are not needed here.

Also I would do

if (other == 0)
       REGION_INTERSECTS;

return REGION_MIXED;

>  }
>  EXPORT_SYMBOL_GPL(region_intersects);
>
> --
> 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

Re: [PATCH v1 2/7] resource: Group resource_overlaps() with other inline helpers

[Rafael J. Wysocki]

On Thu, Aug 13, 2020 at 7:57 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> For better maintenance group resource_overlaps() with other inline helpers.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  include/linux/ioport.h | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/ioport.h b/include/linux/ioport.h
> index 6c2b06fe8beb..0193987b9968 100644
> --- a/include/linux/ioport.h
> +++ b/include/linux/ioport.h
> @@ -226,6 +226,11 @@ static inline bool resource_contains(struct resource *r1, struct resource *r2)
>         return r1->start <= r2->start && r1->end >= r2->end;
>  }
>
> +/* True if any part of r1 overlaps r2 */
> +static inline bool resource_overlaps(struct resource *r1, struct resource *r2)
> +{
> +       return (r1->start <= r2->end && r1->end >= r2->start);

The redundant parens can be dropped while at it.

> +}
>
>  /* Convenience shorthand with allocation */
>  #define request_region(start,n,name)           __request_region(&ioport_resource, (start), (n), (name), 0)
> @@ -291,12 +296,6 @@ extern int
>  walk_iomem_res_desc(unsigned long desc, unsigned long flags, u64 start, u64 end,
>                     void *arg, int (*func)(struct resource *, void *));
>
> -/* True if any part of r1 overlaps r2 */
> -static inline bool resource_overlaps(struct resource *r1, struct resource *r2)
> -{
> -       return (r1->start <= r2->end && r1->end >= r2->start);
> -}
> -
>  struct resource *devm_request_free_mem_region(struct device *dev,
>                 struct resource *base, unsigned long size);
>  struct resource *request_free_mem_region(struct resource *base,
> --
> 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 4/7] resource: Introduce resource_intersection() for overlapping resources

[Rafael J. Wysocki]

On Thu, Aug 13, 2020 at 7:57 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> There will be at least one user that can utilize new helper.
> Provide the helper for future user and for wider use.
>
> 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>
> ---
>  include/linux/ioport.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/include/linux/ioport.h b/include/linux/ioport.h
> index c98df0ec7422..1d5ab2e7f9eb 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_intersection(struct resource *r1, struct resource *r2, struct resource *r)
> +{
> +       if (!resource_overlaps(r1, r2))
> +               return false;
> +       r->start = r1->start > r2->start ? r1->start : r2->start;
> +       r->end = r1->end < r2->end ? r1->end : r2->end;
> +       return true;
> +}

I have the same comments as for patch [3/7].

> +
>  static inline bool
>  resource_union(struct resource *r1, struct resource *r2, struct resource *r)
>  {
> --
> 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
>

Re: [PATCH v1 7/7] ACPI: watchdog: 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: Mika Westerberg <mika.westerberg@linux.intel.com>

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

> ---
>  drivers/acpi/acpi_watchdog.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/acpi/acpi_watchdog.c b/drivers/acpi/acpi_watchdog.c
> index 5c1e9ea43123..ca28183f4d13 100644
> --- a/drivers/acpi/acpi_watchdog.c
> +++ b/drivers/acpi/acpi_watchdog.c
> @@ -151,11 +151,7 @@ void __init acpi_watchdog_init(void)
>                 found = false;
>                 resource_list_for_each_entry(rentry, &resource_list) {
>                         if (rentry->res->flags == res.flags &&
> -                           resource_overlaps(rentry->res, &res)) {
> -                               if (res.start < rentry->res->start)
> -                                       rentry->res->start = res.start;
> -                               if (res.end > rentry->res->end)
> -                                       rentry->res->end = res.end;
> +                           resource_union(rentry->res, &res, rentry->res)) {
>                                 found = true;
>                                 break;
>                         }
> --
> 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 2/7] resource: Group resource_overlaps() with other inline helpers

[Andy Shevchenko]

On Fri, Aug 14, 2020 at 05:18:33PM +0200, Rafael J. Wysocki wrote:
> On Thu, Aug 13, 2020 at 7:57 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > For better maintenance group resource_overlaps() with other inline helpers.

...

> > +/* True if any part of r1 overlaps r2 */
> > +static inline bool resource_overlaps(struct resource *r1, struct resource *r2)
> > +{
> > +       return (r1->start <= r2->end && r1->end >= r2->start);
> 
> The redundant parens can be dropped while at it.

Okay, will do!

> > +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 4/7] resource: Introduce resource_intersection() for overlapping resources

[Andy Shevchenko]

On Fri, Aug 14, 2020 at 05:24:36PM +0200, Rafael J. Wysocki wrote:
> On Thu, Aug 13, 2020 at 7:57 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > There will be at least one user that can utilize new helper.
> > Provide the helper for future user and for wider use.
> >
> > 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.

...

> I have the same comments as for patch [3/7].

Same answers as there :-)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/7] resource: Simplify region_intersects() by reducing conditionals

[Andy Shevchenko]

On Fri, Aug 14, 2020 at 05:17:22PM +0200, Rafael J. Wysocki wrote:
> On Thu, Aug 13, 2020 at 7:57 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > Now we have for 'other' and 'type' variables
> >
> > other   type    return
> >   0       0     REGION_DISJOINT
> >   0       x     REGION_INTERSECTS
> >   x       0     REGION_DISJOINT
> >   x       x     REGION_MIXED
> >
> > Obviously it's easier to check 'type' for 0 first instead of
> > currently checked 'other'.

...

> > +       return (other == 0) ? REGION_INTERSECTS : REGION_MIXED;
> 
> The parens are not needed here.

Right.

> Also I would do
> 
> if (other == 0)
>        REGION_INTERSECTS;
> 
> return REGION_MIXED;

Works for me, I'll update in v2.

-- 
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