[PATCH v3 1/5] ACPI / bus: Return error code from __acpi_match_device() in one case

[PATCH v3 1/5] ACPI / bus: Return error code from __acpi_match_device() in one case

[Andy Shevchenko]

Instead of playing tricks with last invalid entry,
return simple -ENODATA error code cast to pointer.

It would be good for future in case caller passes NULL pointer for
ID table. Moreover, caller can check the code to be sure what happened
inside callee.

Fixes: 2b9c698efa58 ("ACPI / scan: Take the PRP0001 position in the list of IDs into account")
Cc: Sinan Kaya <okaya@codeaurora.org>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Vinod Koul <vinod.koul@intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/acpi/bus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 58239abb0a72..761286e50067 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -769,7 +769,7 @@ static const struct acpi_device_id *__acpi_match_device(
 		 */
 		if (!strcmp(ACPI_DT_NAMESPACE_HID, hwid->id)
 		    && acpi_of_match_device(device, of_ids))
-			return id;
+			return ERR_PTR(-ENODATA);
 	}
 	return NULL;
 }
-- 
2.15.1

[PATCH v3 2/5] ACPI / bus: Do not traverse through non-existed device table

[Andy Shevchenko]

When __acpi_match_device() is called it would be possible to have
ACPI ID table a MULL pointer. To avoid potential dereference,
check for this before traverse.

While here, remove redundant 'else'.

Cc: Sinan Kaya <okaya@codeaurora.org>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Vinod Koul <vinod.koul@intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/acpi/bus.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 761286e50067..da7e9cf770a6 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -752,11 +752,13 @@ static const struct acpi_device_id *__acpi_match_device(
 
 	list_for_each_entry(hwid, &device->pnp.ids, list) {
 		/* First, check the ACPI/PNP IDs provided by the caller. */
-		for (id = ids; id->id[0] || id->cls; id++) {
-			if (id->id[0] && !strcmp((char *) id->id, hwid->id))
-				return id;
-			else if (id->cls && __acpi_match_device_cls(id, hwid))
-				return id;
+		if (ids) {
+			for (id = ids; id->id[0] || id->cls; id++) {
+				if (id->id[0] && !strcmp((char *)id->id, hwid->id))
+					return id;
+				if (id->cls && __acpi_match_device_cls(id, hwid))
+					return id;
+			}
 		}
 
 		/*
-- 
2.15.1

[PATCH v3 3/5] ACPI / bus: Remove checks in acpi_get_match_data()

[Andy Shevchenko]

As well as its sibling of_device_get_match_data() has no such checks,
no need to do it in acpi_get_match_data().

First of all, we are not supposed to call fwnode API like this without
driver attached.

Second, since __acpi_match_device() does check input parameter there is
no need to duplicate it outside.

And last but not least one, the API should still serve the cases when
ACPI device is enumerated via PRP0001. In such case driver has neither
ACPI table nor driver data there.

Cc: Sinan Kaya <okaya@codeaurora.org>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Vinod Koul <vinod.koul@intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/acpi/bus.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index da7e9cf770a6..1a7aa97a32f8 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -798,12 +798,6 @@ void *acpi_get_match_data(const struct device *dev)
 {
 	const struct acpi_device_id *match;
 
-	if (!dev->driver)
-		return NULL;
-
-	if (!dev->driver->acpi_match_table)
-		return NULL;
-
 	match = acpi_match_device(dev->driver->acpi_match_table, dev);
 	if (!match)
 		return NULL;
-- 
2.15.1

[PATCH v3 4/5] ACPI / bus: Rename acpi_get_match_data() to acpi_device_get_match_data()

[Andy Shevchenko]

Do the renaming to be consistent with its sibling, i.e.
of_device_get_match_data().

No functional change.

Cc: Sinan Kaya <okaya@codeaurora.org>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Vinod Koul <vinod.koul@intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/acpi/bus.c      | 4 ++--
 drivers/acpi/property.c | 2 +-
 include/linux/acpi.h    | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 1a7aa97a32f8..fd5407314f51 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -794,7 +794,7 @@ const struct acpi_device_id *acpi_match_device(const struct acpi_device_id *ids,
 }
 EXPORT_SYMBOL_GPL(acpi_match_device);
 
-void *acpi_get_match_data(const struct device *dev)
+void *acpi_device_get_match_data(const struct device *dev)
 {
 	const struct acpi_device_id *match;
 
@@ -804,7 +804,7 @@ void *acpi_get_match_data(const struct device *dev)
 
 	return (void *)match->driver_data;
 }
-EXPORT_SYMBOL_GPL(acpi_get_match_data);
+EXPORT_SYMBOL_GPL(acpi_device_get_match_data);
 
 int acpi_match_device_ids(struct acpi_device *device,
 			  const struct acpi_device_id *ids)
diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index 466d1503aba0..f9b5fa230a86 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -1275,7 +1275,7 @@ static void *
 acpi_fwnode_device_get_match_data(const struct fwnode_handle *fwnode,
 				  const struct device *dev)
 {
-	return acpi_get_match_data(dev);
+	return acpi_device_get_match_data(dev);
 }
 
 #define DECLARE_ACPI_FWNODE_OPS(ops) \
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 246845da3f84..d0cbbbd88e0e 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -587,7 +587,7 @@ extern int acpi_nvs_for_each_region(int (*func)(__u64, __u64, void *),
 const struct acpi_device_id *acpi_match_device(const struct acpi_device_id *ids,
 					       const struct device *dev);
 
-void *acpi_get_match_data(const struct device *dev);
+void *acpi_device_get_match_data(const struct device *dev);
 extern bool acpi_driver_match_device(struct device *dev,
 				     const struct device_driver *drv);
 int acpi_device_uevent_modalias(struct device *, struct kobj_uevent_env *);
@@ -766,7 +766,7 @@ static inline const struct acpi_device_id *acpi_match_device(
 	return NULL;
 }
 
-static inline void *acpi_get_match_data(const struct device *dev)
+static inline void *acpi_device_get_match_data(const struct device *dev)
 {
 	return NULL;
 }
-- 
2.15.1

[PATCH v3 5/5] device property: Constify device_get_match_data()

[Andy Shevchenko]

Constify device_get_match_data() as OF and ACPI variants return
constant value.

Cc: Sinan Kaya <okaya@codeaurora.org>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Vinod Koul <vinod.koul@intel.com>
Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/acpi/bus.c       | 4 ++--
 drivers/acpi/property.c  | 2 +-
 drivers/base/property.c  | 5 ++---
 drivers/of/property.c    | 4 ++--
 include/linux/acpi.h     | 4 ++--
 include/linux/fwnode.h   | 4 ++--
 include/linux/property.h | 2 +-
 7 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index fd5407314f51..ad9be9a21941 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -794,7 +794,7 @@ const struct acpi_device_id *acpi_match_device(const struct acpi_device_id *ids,
 }
 EXPORT_SYMBOL_GPL(acpi_match_device);
 
-void *acpi_device_get_match_data(const struct device *dev)
+const void *acpi_device_get_match_data(const struct device *dev)
 {
 	const struct acpi_device_id *match;
 
@@ -802,7 +802,7 @@ void *acpi_device_get_match_data(const struct device *dev)
 	if (!match)
 		return NULL;
 
-	return (void *)match->driver_data;
+	return (const void *)match->driver_data;
 }
 EXPORT_SYMBOL_GPL(acpi_device_get_match_data);
 
diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index f9b5fa230a86..5815356ea6ad 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -1271,7 +1271,7 @@ static int acpi_fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode,
 	return 0;
 }
 
-static void *
+static const void *
 acpi_fwnode_device_get_match_data(const struct fwnode_handle *fwnode,
 				  const struct device *dev)
 {
diff --git a/drivers/base/property.c b/drivers/base/property.c
index 302236281d83..8f205f6461ed 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -1410,9 +1410,8 @@ int fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode,
 }
 EXPORT_SYMBOL(fwnode_graph_parse_endpoint);
 
-void *device_get_match_data(struct device *dev)
+const void *device_get_match_data(struct device *dev)
 {
-	return fwnode_call_ptr_op(dev_fwnode(dev), device_get_match_data,
-				  dev);
+	return fwnode_call_ptr_op(dev_fwnode(dev), device_get_match_data, dev);
 }
 EXPORT_SYMBOL_GPL(device_get_match_data);
diff --git a/drivers/of/property.c b/drivers/of/property.c
index 36ed84e26d9c..f46828e3b082 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -977,11 +977,11 @@ static int of_fwnode_graph_parse_endpoint(const struct fwnode_handle *fwnode,
 	return 0;
 }
 
-static void *
+static const void *
 of_fwnode_device_get_match_data(const struct fwnode_handle *fwnode,
 				const struct device *dev)
 {
-	return (void *)of_device_get_match_data(dev);
+	return of_device_get_match_data(dev);
 }
 
 const struct fwnode_operations of_fwnode_ops = {
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index d0cbbbd88e0e..9a618204aba4 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -587,7 +587,7 @@ extern int acpi_nvs_for_each_region(int (*func)(__u64, __u64, void *),
 const struct acpi_device_id *acpi_match_device(const struct acpi_device_id *ids,
 					       const struct device *dev);
 
-void *acpi_device_get_match_data(const struct device *dev);
+const void *acpi_device_get_match_data(const struct device *dev);
 extern bool acpi_driver_match_device(struct device *dev,
 				     const struct device_driver *drv);
 int acpi_device_uevent_modalias(struct device *, struct kobj_uevent_env *);
@@ -766,7 +766,7 @@ static inline const struct acpi_device_id *acpi_match_device(
 	return NULL;
 }
 
-static inline void *acpi_device_get_match_data(const struct device *dev)
+static inline const void *acpi_device_get_match_data(const struct device *dev)
 {
 	return NULL;
 }
diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
index 4fa1a489efe4..4fe8f289b3f6 100644
--- a/include/linux/fwnode.h
+++ b/include/linux/fwnode.h
@@ -73,8 +73,8 @@ struct fwnode_operations {
 	struct fwnode_handle *(*get)(struct fwnode_handle *fwnode);
 	void (*put)(struct fwnode_handle *fwnode);
 	bool (*device_is_available)(const struct fwnode_handle *fwnode);
-	void *(*device_get_match_data)(const struct fwnode_handle *fwnode,
-				       const struct device *dev);
+	const void *(*device_get_match_data)(const struct fwnode_handle *fwnode,
+					     const struct device *dev);
 	bool (*property_present)(const struct fwnode_handle *fwnode,
 				 const char *propname);
 	int (*property_read_int_array)(const struct fwnode_handle *fwnode,
diff --git a/include/linux/property.h b/include/linux/property.h
index 769d372c1edf..2eea4b310fc2 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -283,7 +283,7 @@ bool device_dma_supported(struct device *dev);
 
 enum dev_dma_attr device_get_dma_attr(struct device *dev);
 
-void *device_get_match_data(struct device *dev);
+const void *device_get_match_data(struct device *dev);
 
 int device_get_phy_mode(struct device *dev);
 
-- 
2.15.1

Re: [PATCH v3 1/5] ACPI / bus: Return error code from __acpi_match_device() in one case

[Rafael J. Wysocki]

On Wed, Feb 7, 2018 at 3:56 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> Instead of playing tricks with last invalid entry,
> return simple -ENODATA error code cast to pointer.
>
> It would be good for future in case caller passes NULL pointer for
> ID table. Moreover, caller can check the code to be sure what happened
> inside callee.
>
> Fixes: 2b9c698efa58 ("ACPI / scan: Take the PRP0001 position in the list of IDs into account")

I still don't think the Fixes: tag here is valid (the code works as is
AFAICS), but I could drop it when applying the patch just fine. :-)

That said, see below.

> Cc: Sinan Kaya <okaya@codeaurora.org>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Vinod Koul <vinod.koul@intel.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/acpi/bus.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index 58239abb0a72..761286e50067 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -769,7 +769,7 @@ static const struct acpi_device_id *__acpi_match_device(
>                  */
>                 if (!strcmp(ACPI_DT_NAMESPACE_HID, hwid->id)
>                     && acpi_of_match_device(device, of_ids))
> -                       return id;
> +                       return ERR_PTR(-ENODATA);

Doesn't the comment above need to be updated?

Also the return value here means "success", so why is an error the right choice?

>         }
>         return NULL;
>  }
> --

Overall, this really looks like a preparation for a future patch, so
why not just say that straight away in the changelog?

Re: [PATCH v3 1/5] ACPI / bus: Return error code from __acpi_match_device() in one case

[Andy Shevchenko]

On Thu, 2018-02-08 at 16:14 +0100, Rafael J. Wysocki wrote:
> On Wed, Feb 7, 2018 at 3:56 PM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > Instead of playing tricks with last invalid entry,
> > return simple -ENODATA error code cast to pointer.
> > 
> > It would be good for future in case caller passes NULL pointer for
> > ID table. Moreover, caller can check the code to be sure what
> > happened
> > inside callee.
> > 
> > Fixes: 2b9c698efa58 ("ACPI / scan: Take the PRP0001 position in the
> > list of IDs into account")
> 
> I still don't think the Fixes: tag here is valid (the code works as is
> AFAICS), but I could drop it when applying the patch just fine. :-)

OK.

> >                 if (!strcmp(ACPI_DT_NAMESPACE_HID, hwid->id)
> >                     && acpi_of_match_device(device, of_ids))
> > -                       return id;
> > +                       return ERR_PTR(-ENODATA);
> 
> Doesn't the comment above need to be updated?

Will do.

> Also the return value here means "success", so why is an error the
> right choice?

Because we need to return something which is not NULL. Naturally feels
the error code, esp. ENODATA, is quite suitable. We indeed have no data
in this case, and it's not a NULL case (not found / not match) — we have
a match.


> Overall, this really looks like a preparation for a future patch, so
> why not just say that straight away in the changelog?

It's not _just_ a preparation, it mitigates the trick used in mentioned
by Fixes tag commit.

I would rather update comment here, and add explanation to the commit
message to be sure it covers tricks mitigation and preparation purposes.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v3 1/5] ACPI / bus: Return error code from __acpi_match_device() in one case

[Rafael J. Wysocki]

On Thu, Feb 8, 2018 at 4:14 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Wed, Feb 7, 2018 at 3:56 PM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
>> Instead of playing tricks with last invalid entry,
>> return simple -ENODATA error code cast to pointer.
>>
>> It would be good for future in case caller passes NULL pointer for
>> ID table. Moreover, caller can check the code to be sure what happened
>> inside callee.
>>
>> Fixes: 2b9c698efa58 ("ACPI / scan: Take the PRP0001 position in the list of IDs into account")
>
> I still don't think the Fixes: tag here is valid (the code works as is
> AFAICS), but I could drop it when applying the patch just fine. :-)
>
> That said, see below.
>
>> Cc: Sinan Kaya <okaya@codeaurora.org>
>> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
>> Cc: Vinod Koul <vinod.koul@intel.com>
>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> ---
>>  drivers/acpi/bus.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
>> index 58239abb0a72..761286e50067 100644
>> --- a/drivers/acpi/bus.c
>> +++ b/drivers/acpi/bus.c
>> @@ -769,7 +769,7 @@ static const struct acpi_device_id *__acpi_match_device(
>>                  */
>>                 if (!strcmp(ACPI_DT_NAMESPACE_HID, hwid->id)
>>                     && acpi_of_match_device(device, of_ids))
>> -                       return id;
>> +                       return ERR_PTR(-ENODATA);
>
> Doesn't the comment above need to be updated?
>
> Also the return value here means "success", so why is an error the right choice?

Moreover, if ids is NULL, then dereferencing it in the loop above
won't work already, so if we get here, there is at least one entry in
it.

Re: [PATCH v3 1/5] ACPI / bus: Return error code from __acpi_match_device() in one case

[Rafael J. Wysocki]

On Thu, Feb 8, 2018 at 4:44 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Thu, 2018-02-08 at 16:14 +0100, Rafael J. Wysocki wrote:
>> On Wed, Feb 7, 2018 at 3:56 PM, Andy Shevchenko
>> <andriy.shevchenko@linux.intel.com> wrote:
>> > Instead of playing tricks with last invalid entry,
>> > return simple -ENODATA error code cast to pointer.
>> >
>> > It would be good for future in case caller passes NULL pointer for
>> > ID table. Moreover, caller can check the code to be sure what
>> > happened
>> > inside callee.
>> >
>> > Fixes: 2b9c698efa58 ("ACPI / scan: Take the PRP0001 position in the
>> > list of IDs into account")
>>
>> I still don't think the Fixes: tag here is valid (the code works as is
>> AFAICS), but I could drop it when applying the patch just fine. :-)
>
> OK.
>
>> >                 if (!strcmp(ACPI_DT_NAMESPACE_HID, hwid->id)
>> >                     && acpi_of_match_device(device, of_ids))
>> > -                       return id;
>> > +                       return ERR_PTR(-ENODATA);
>>
>> Doesn't the comment above need to be updated?
>
> Will do.
>
>> Also the return value here means "success", so why is an error the
>> right choice?
>
> Because we need to return something which is not NULL. Naturally feels
> the error code, esp. ENODATA, is quite suitable. We indeed have no data
> in this case, and it's not a NULL case (not found / not match) — we have
> a match.

But this is an error code that means "success".  May I call it rather confusing?

>
>> Overall, this really looks like a preparation for a future patch, so
>> why not just say that straight away in the changelog?
>
> It's not _just_ a preparation, it mitigates the trick used in mentioned
> by Fixes tag commit.
>
> I would rather update comment here, and add explanation to the commit
> message to be sure it covers tricks mitigation and preparation purposes.

This is not mitigation, sorry.  It just replaces one possibly
confusing thing with another.

The code as is works as I said and this patch doesn't make it any
better as far as I'm concerned.

Re: [PATCH v3 1/5] ACPI / bus: Return error code from __acpi_match_device() in one case

[Andy Shevchenko]

On Thu, 2018-02-08 at 16:48 +0100, Rafael J. Wysocki wrote:
> On Thu, Feb 8, 2018 at 4:44 PM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, 2018-02-08 at 16:14 +0100, Rafael J. Wysocki wrote:
> > > 

> > 
> > > Also the return value here means "success", so why is an error the
> > > right choice?
> > 
> > Because we need to return something which is not NULL. Naturally
> > feels
> > the error code, esp. ENODATA, is quite suitable. We indeed have no
> > data
> > in this case, and it's not a NULL case (not found / not match) — we
> > have
> > a match.
> 
> But this is an error code that means "success".  May I call it rather
> confusing?

This function AFAICS does two things at once:
- matches device against ID
- returns matched ID entry in the table

Return value combines those two into actually ternary option:
- no match
- match with ID
- match without ID

> > > Overall, this really looks like a preparation for a future patch,
> > > so
> > > why not just say that straight away in the changelog?
> > 
> > It's not _just_ a preparation, it mitigates the trick used in
> > mentioned
> > by Fixes tag commit.
> > 
> > I would rather update comment here, and add explanation to the
> > commit
> > message to be sure it covers tricks mitigation and preparation
> > purposes.
> 
> This is not mitigation, sorry.  It just replaces one possibly
> confusing thing with another.

I would agree here...

> The code as is works as I said and this patch doesn't make it any
> better as far as I'm concerned.

...but not here. Instead of returning pointer to *something* (from
caller point of view), we explicitly tell caller what of the above
happened. We don't rely on the organization  of ID table or its life
time (though it's forever).

I can say that is *slightly* better. But agree that is not cleanest
solution I can come up with.

I'm all ears on other possibilities how to get rid of that trick.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v3 2/5] ACPI / bus: Do not traverse through non-existed device table

[Rafael J. Wysocki]

On Wed, Feb 7, 2018 at 3:56 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> When __acpi_match_device() is called it would be possible to have
> ACPI ID table a MULL pointer. To avoid potential dereference,
> check for this before traverse.
>
> While here, remove redundant 'else'.
>
> Cc: Sinan Kaya <okaya@codeaurora.org>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Vinod Koul <vinod.koul@intel.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/acpi/bus.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index 761286e50067..da7e9cf770a6 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -752,11 +752,13 @@ static const struct acpi_device_id *__acpi_match_device(
>
>         list_for_each_entry(hwid, &device->pnp.ids, list) {
>                 /* First, check the ACPI/PNP IDs provided by the caller. */
> -               for (id = ids; id->id[0] || id->cls; id++) {
> -                       if (id->id[0] && !strcmp((char *) id->id, hwid->id))
> -                               return id;
> -                       else if (id->cls && __acpi_match_device_cls(id, hwid))
> -                               return id;
> +               if (ids) {
> +                       for (id = ids; id->id[0] || id->cls; id++) {
> +                               if (id->id[0] && !strcmp((char *)id->id, hwid->id))
> +                                       return id;
> +                               if (id->cls && __acpi_match_device_cls(id, hwid))
> +                                       return id;
> +                       }
>                 }
>
>                 /*
> --

The return value below should be updated in *this* patch, because this
is what allows ids to be NULL in the first place.

And as far as I'm concerned you can do:

if (!strcmp(ACPI_DT_NAMESPACE_HID, hwid->id)
        return (const struct acpi_device_id
*)acpi_of_match_device(device, of_ids);

and update the comment accordingly.

Re: [PATCH v3 2/5] ACPI / bus: Do not traverse through non-existed device table

[Rafael J. Wysocki]

On Thu, Feb 8, 2018 at 4:59 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Wed, Feb 7, 2018 at 3:56 PM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
>> When __acpi_match_device() is called it would be possible to have
>> ACPI ID table a MULL pointer. To avoid potential dereference,
>> check for this before traverse.
>>
>> While here, remove redundant 'else'.
>>
>> Cc: Sinan Kaya <okaya@codeaurora.org>
>> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
>> Cc: Vinod Koul <vinod.koul@intel.com>
>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> ---
>>  drivers/acpi/bus.c | 12 +++++++-----
>>  1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
>> index 761286e50067..da7e9cf770a6 100644
>> --- a/drivers/acpi/bus.c
>> +++ b/drivers/acpi/bus.c
>> @@ -752,11 +752,13 @@ static const struct acpi_device_id *__acpi_match_device(
>>
>>         list_for_each_entry(hwid, &device->pnp.ids, list) {
>>                 /* First, check the ACPI/PNP IDs provided by the caller. */
>> -               for (id = ids; id->id[0] || id->cls; id++) {
>> -                       if (id->id[0] && !strcmp((char *) id->id, hwid->id))
>> -                               return id;
>> -                       else if (id->cls && __acpi_match_device_cls(id, hwid))
>> -                               return id;
>> +               if (ids) {
>> +                       for (id = ids; id->id[0] || id->cls; id++) {
>> +                               if (id->id[0] && !strcmp((char *)id->id, hwid->id))
>> +                                       return id;
>> +                               if (id->cls && __acpi_match_device_cls(id, hwid))
>> +                                       return id;
>> +                       }
>>                 }
>>
>>                 /*
>> --
>
> The return value below should be updated in *this* patch, because this
> is what allows ids to be NULL in the first place.
>
> And as far as I'm concerned you can do:
>
> if (!strcmp(ACPI_DT_NAMESPACE_HID, hwid->id)

Missing paren, sorry.

>         return (const struct acpi_device_id
> *)acpi_of_match_device(device, of_ids);
>
> and update the comment accordingly.

Re: [PATCH v3 1/5] ACPI / bus: Return error code from __acpi_match_device() in one case

[Rafael J. Wysocki]

On Thu, Feb 8, 2018 at 4:59 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Thu, 2018-02-08 at 16:48 +0100, Rafael J. Wysocki wrote:
>> On Thu, Feb 8, 2018 at 4:44 PM, Andy Shevchenko
>> <andriy.shevchenko@linux.intel.com> wrote:
>> > On Thu, 2018-02-08 at 16:14 +0100, Rafael J. Wysocki wrote:
>> > >
>
>> >
>> > > Also the return value here means "success", so why is an error the
>> > > right choice?
>> >
>> > Because we need to return something which is not NULL. Naturally
>> > feels
>> > the error code, esp. ENODATA, is quite suitable. We indeed have no
>> > data
>> > in this case, and it's not a NULL case (not found / not match) — we
>> > have
>> > a match.
>>
>> But this is an error code that means "success".  May I call it rather
>> confusing?
>
> This function AFAICS does two things at once:
> - matches device against ID
> - returns matched ID entry in the table
>
> Return value combines those two into actually ternary option:
> - no match
> - match with ID
> - match without ID

Right.  And the caller knows when to expect the third case which is
the whole point.

>> > > Overall, this really looks like a preparation for a future patch,
>> > > so
>> > > why not just say that straight away in the changelog?
>> >
>> > It's not _just_ a preparation, it mitigates the trick used in
>> > mentioned
>> > by Fixes tag commit.
>> >
>> > I would rather update comment here, and add explanation to the
>> > commit
>> > message to be sure it covers tricks mitigation and preparation
>> > purposes.
>>
>> This is not mitigation, sorry.  It just replaces one possibly
>> confusing thing with another.
>
> I would agree here...
>
>> The code as is works as I said and this patch doesn't make it any
>> better as far as I'm concerned.
>
> ...but not here. Instead of returning pointer to *something* (from
> caller point of view), we explicitly tell caller what of the above
> happened. We don't rely on the organization  of ID table or its life
> time (though it's forever).
>
> I can say that is *slightly* better. But agree that is not cleanest
> solution I can come up with.
>
> I'm all ears on other possibilities how to get rid of that trick.

Please see my reply to the second patch, then.

Re: [PATCH v3 2/5] ACPI / bus: Do not traverse through non-existed device table

[Andy Shevchenko]

On Thu, 2018-02-08 at 16:59 +0100, Rafael J. Wysocki wrote:
> On Wed, Feb 7, 2018 at 3:56 PM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > When __acpi_match_device() is called it would be possible to have
> > ACPI ID table a MULL pointer. To avoid potential dereference,
> > check for this before traverse.
> > 
> > While here, remove redundant 'else'.
> > 
> > 

> > +               if (ids) {
> > +                       for (id = ids; id->id[0] || id->cls; id++) {
> > +                               if (id->id[0] && !strcmp((char *)id-
> > >id, hwid->id))
> > +                                       return id;
> > +                               if (id->cls &&
> > __acpi_match_device_cls(id, hwid))
> > +                                       return id;
> > +                       }
> > 
> 

> The return value below should be updated in *this* patch, because this
> is what allows ids to be NULL in the first place.

OK, so I'll fold it into patch 1 then.

> And as far as I'm concerned you can do:
> 
> if (!strcmp(ACPI_DT_NAMESPACE_HID, hwid->id)
>         return (const struct acpi_device_id
> *)acpi_of_match_device(device, of_ids);
> 
> and update the comment accordingly.

But it's still a trick.

Okay, what comes to my mind (yes, not so simple, but cleaner I suppose)
is to define

struct acpi_of_device_id {
  struct acpi_device_id *acpi_id;
  struct of_device_id *of_id;
};

Add a new parameter to acpi_of_match_device(..., struct
acpi_of_device_id *id).


Update __acpi_match_device() in the similar way.

Update callers.

It looks to me much more cleaner.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v3 2/5] ACPI / bus: Do not traverse through non-existed device table

[Andy Shevchenko]

On Thu, 2018-02-08 at 16:59 +0100, Rafael J. Wysocki wrote:
> On Wed, Feb 7, 2018 at 3:56 PM, Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > 

> And as far as I'm concerned you can do:
> 
> if (!strcmp(ACPI_DT_NAMESPACE_HID, hwid->id)
>         return (const struct acpi_device_id
> *)acpi_of_match_device(device, of_ids);
> 
> and update the comment accordingly.

Would the following work for you?

--- 8< --- 8< --- 8< ---
>From 03013c0c1e487cda4c0d8dc5065ed2f0031095de Mon Sep 17 00:00:00 2001
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Date: Thu, 1 Feb 2018 21:52:14 +0200
Subject: [PATCH 1/1] ACPI / bus: Do not traverse through non-existed
device
 table

When __acpi_match_device() is called it would be possible to have
ACPI ID table a MULL pointer. To avoid potential dereference,
check for this before traverse.

This patch implies a bit of refactoring acpi_of_match_device() to return
pointer to OF ID when matched followed by refactoring
__acpi_match_device() to return either ACPI or OF ID when matches.

While here, remove redundant 'else'.

Cc: Sinan Kaya <okaya@codeaurora.org>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Vinod Koul <vinod.koul@intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/acpi/bus.c | 66 +++++++++++++++++++++++++++++++++------------
---------
 1 file changed, 41 insertions(+), 25 deletions(-)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 676c9788e1c8..33c5166e6028 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -660,13 +660,15 @@ struct acpi_device *acpi_companion_match(const
struct device *dev)
  * acpi_of_match_device - Match device object using the "compatible"
property.
  * @adev: ACPI device object to match.
  * @of_match_table: List of device IDs to match against.
+ * @of_id: OF ID if matched
  *
  * If @dev has an ACPI companion which has ACPI_DT_NAMESPACE_HID in its
list of
  * identifiers and a _DSD object with the "compatible" property, use
that
  * property to match against the given list of identifiers.
  */
 static bool acpi_of_match_device(struct acpi_device *adev,
-				 const struct of_device_id
*of_match_table)
+				 const struct of_device_id
*of_match_table,
+				 const struct of_device_id **of_id)
 {
 	const union acpi_object *of_compatible, *obj;
 	int i, nval;
@@ -690,8 +692,11 @@ static bool acpi_of_match_device(struct acpi_device
*adev,
 		const struct of_device_id *id;
 
 		for (id = of_match_table; id->compatible[0]; id++)
-			if (!strcasecmp(obj->string.pointer, id-
>compatible))
+			if (!strcasecmp(obj->string.pointer, id-
>compatible)) {
+				if (of_id)
+					*of_id = id;
 				return true;
+			}
 	}
 
 	return false;
@@ -762,10 +767,11 @@ static bool __acpi_match_device_cls(const struct
acpi_device_id *id,
 	return true;
 }
 
-static const struct acpi_device_id *__acpi_match_device(
-	struct acpi_device *device,
-	const struct acpi_device_id *ids,
-	const struct of_device_id *of_ids)
+static bool __acpi_match_device(struct acpi_device *device,
+				const struct acpi_device_id *acpi_ids,
+				const struct of_device_id *of_ids,
+				const struct acpi_device_id **acpi_id,
+				const struct of_device_id **of_id)
 {
 	const struct acpi_device_id *id;
 	struct acpi_hardware_id *hwid;
@@ -775,30 +781,35 @@ static const struct acpi_device_id
*__acpi_match_device(
 	 * driver for it.
 	 */
 	if (!device || !device->status.present)
-		return NULL;
+		return false;
 
 	list_for_each_entry(hwid, &device->pnp.ids, list) {
 		/* First, check the ACPI/PNP IDs provided by the
caller. */
-		for (id = ids; id->id[0] || id->cls; id++) {
-			if (id->id[0] && !strcmp((char *) id->id, hwid-
>id))
-				return id;
-			else if (id->cls && __acpi_match_device_cls(id,
hwid))
-				return id;
+		if (acpi_ids) {
+			bool found;
+
+			for (id = acpi_ids; id->id[0] || id->cls; id++)
{
+				found = false;
+				if (id->id[0] && !strcmp((char *)id-
>id, hwid->id))
+					found = true;
+				if (id->cls &&
__acpi_match_device_cls(id, hwid))
+					found = true;
+				if (found) {
+					if (acpi_id)
+						*acpi_id = id;
+					return true;
+				}
+			}
 		}
 
 		/*
 		 * Next, check ACPI_DT_NAMESPACE_HID and try to match
the
 		 * "compatible" property if found.
-		 *
-		 * The id returned by the below is not valid, but the
only
-		 * caller passing non-NULL of_ids here is only
interested in
-		 * whether or not the return value is NULL.
 		 */
-		if (!strcmp(ACPI_DT_NAMESPACE_HID, hwid->id)
-		    && acpi_of_match_device(device, of_ids))
-			return id;
+		if (!strcmp(ACPI_DT_NAMESPACE_HID, hwid->id))
+			return acpi_of_match_device(device, of_ids,
of_id);
 	}
-	return NULL;
+	return false;
 }
 
 /**
@@ -815,7 +826,10 @@ static const struct acpi_device_id
*__acpi_match_device(
 const struct acpi_device_id *acpi_match_device(const struct
acpi_device_id *ids,
 					       const struct device
*dev)
 {
-	return __acpi_match_device(acpi_companion_match(dev), ids,
NULL);
+	const struct acpi_device_id *id = NULL;
+
+	__acpi_match_device(acpi_companion_match(dev), ids, NULL, &id,
NULL);
+	return id;
 }
 EXPORT_SYMBOL_GPL(acpi_match_device);
 
@@ -840,7 +854,7 @@ EXPORT_SYMBOL_GPL(acpi_get_match_data);
 int acpi_match_device_ids(struct acpi_device *device,
 			  const struct acpi_device_id *ids)
 {
-	return __acpi_match_device(device, ids, NULL) ? 0 : -ENOENT;
+	return __acpi_match_device(device, ids, NULL, NULL, NULL) ? 0 :
-ENOENT;
 }
 EXPORT_SYMBOL(acpi_match_device_ids);
 
@@ -849,10 +863,12 @@ bool acpi_driver_match_device(struct device *dev,
 {
 	if (!drv->acpi_match_table)
 		return acpi_of_match_device(ACPI_COMPANION(dev),
-					    drv->of_match_table);
+					    drv->of_match_table,
+					    NULL);
 
-	return !!__acpi_match_device(acpi_companion_match(dev),
-				     drv->acpi_match_table, drv-
>of_match_table);
+	return __acpi_match_device(acpi_companion_match(dev),
+				   drv->acpi_match_table, drv-
>of_match_table,
+				   NULL, NULL);
 }
 EXPORT_SYMBOL_GPL(acpi_driver_match_device);
 
-- 
2.15.1



-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v3 2/5] ACPI / bus: Do not traverse through non-existed device table

[Rafael J. Wysocki]

On Thu, Feb 8, 2018 at 5:53 PM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Thu, 2018-02-08 at 16:59 +0100, Rafael J. Wysocki wrote:
>> On Wed, Feb 7, 2018 at 3:56 PM, Andy Shevchenko
>> <andriy.shevchenko@linux.intel.com> wrote:
>> >
>
>> And as far as I'm concerned you can do:
>>
>> if (!strcmp(ACPI_DT_NAMESPACE_HID, hwid->id)
>>         return (const struct acpi_device_id
>> *)acpi_of_match_device(device, of_ids);
>>
>> and update the comment accordingly.
>
> Would the following work for you?

Yes, it would.

> --- 8< --- 8< --- 8< ---
> From 03013c0c1e487cda4c0d8dc5065ed2f0031095de Mon Sep 17 00:00:00 2001
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Date: Thu, 1 Feb 2018 21:52:14 +0200
> Subject: [PATCH 1/1] ACPI / bus: Do not traverse through non-existed
> device
>  table
>
> When __acpi_match_device() is called it would be possible to have
> ACPI ID table a MULL pointer. To avoid potential dereference,
> check for this before traverse.
>
> This patch implies a bit of refactoring acpi_of_match_device() to return
> pointer to OF ID when matched followed by refactoring
> __acpi_match_device() to return either ACPI or OF ID when matches.
>
> While here, remove redundant 'else'.
>
> Cc: Sinan Kaya <okaya@codeaurora.org>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Vinod Koul <vinod.koul@intel.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/acpi/bus.c | 66 +++++++++++++++++++++++++++++++++------------
> ---------
>  1 file changed, 41 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index 676c9788e1c8..33c5166e6028 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -660,13 +660,15 @@ struct acpi_device *acpi_companion_match(const
> struct device *dev)
>   * acpi_of_match_device - Match device object using the "compatible"
> property.
>   * @adev: ACPI device object to match.
>   * @of_match_table: List of device IDs to match against.
> + * @of_id: OF ID if matched
>   *
>   * If @dev has an ACPI companion which has ACPI_DT_NAMESPACE_HID in its
> list of
>   * identifiers and a _DSD object with the "compatible" property, use
> that
>   * property to match against the given list of identifiers.
>   */
>  static bool acpi_of_match_device(struct acpi_device *adev,
> -                                const struct of_device_id
> *of_match_table)
> +                                const struct of_device_id
> *of_match_table,
> +                                const struct of_device_id **of_id)
>  {
>         const union acpi_object *of_compatible, *obj;
>         int i, nval;
> @@ -690,8 +692,11 @@ static bool acpi_of_match_device(struct acpi_device
> *adev,
>                 const struct of_device_id *id;
>
>                 for (id = of_match_table; id->compatible[0]; id++)
> -                       if (!strcasecmp(obj->string.pointer, id-
>>compatible))
> +                       if (!strcasecmp(obj->string.pointer, id-
>>compatible)) {
> +                               if (of_id)
> +                                       *of_id = id;
>                                 return true;
> +                       }
>         }
>
>         return false;
> @@ -762,10 +767,11 @@ static bool __acpi_match_device_cls(const struct
> acpi_device_id *id,
>         return true;
>  }
>
> -static const struct acpi_device_id *__acpi_match_device(
> -       struct acpi_device *device,
> -       const struct acpi_device_id *ids,
> -       const struct of_device_id *of_ids)
> +static bool __acpi_match_device(struct acpi_device *device,
> +                               const struct acpi_device_id *acpi_ids,
> +                               const struct of_device_id *of_ids,
> +                               const struct acpi_device_id **acpi_id,
> +                               const struct of_device_id **of_id)
>  {
>         const struct acpi_device_id *id;
>         struct acpi_hardware_id *hwid;
> @@ -775,30 +781,35 @@ static const struct acpi_device_id
> *__acpi_match_device(
>          * driver for it.
>          */
>         if (!device || !device->status.present)
> -               return NULL;
> +               return false;
>
>         list_for_each_entry(hwid, &device->pnp.ids, list) {
>                 /* First, check the ACPI/PNP IDs provided by the
> caller. */
> -               for (id = ids; id->id[0] || id->cls; id++) {
> -                       if (id->id[0] && !strcmp((char *) id->id, hwid-
>>id))
> -                               return id;
> -                       else if (id->cls && __acpi_match_device_cls(id,
> hwid))
> -                               return id;
> +               if (acpi_ids) {
> +                       bool found;
> +
> +                       for (id = acpi_ids; id->id[0] || id->cls; id++)
> {
> +                               found = false;
> +                               if (id->id[0] && !strcmp((char *)id-
>>id, hwid->id))
> +                                       found = true;
> +                               if (id->cls &&
> __acpi_match_device_cls(id, hwid))
> +                                       found = true;
> +                               if (found) {
> +                                       if (acpi_id)
> +                                               *acpi_id = id;
> +                                       return true;
> +                               }
> +                       }
>                 }
>
>                 /*
>                  * Next, check ACPI_DT_NAMESPACE_HID and try to match
> the
>                  * "compatible" property if found.
> -                *
> -                * The id returned by the below is not valid, but the
> only
> -                * caller passing non-NULL of_ids here is only
> interested in
> -                * whether or not the return value is NULL.
>                  */
> -               if (!strcmp(ACPI_DT_NAMESPACE_HID, hwid->id)
> -                   && acpi_of_match_device(device, of_ids))
> -                       return id;
> +               if (!strcmp(ACPI_DT_NAMESPACE_HID, hwid->id))
> +                       return acpi_of_match_device(device, of_ids,
> of_id);
>         }
> -       return NULL;
> +       return false;
>  }
>
>  /**
> @@ -815,7 +826,10 @@ static const struct acpi_device_id
> *__acpi_match_device(
>  const struct acpi_device_id *acpi_match_device(const struct
> acpi_device_id *ids,
>                                                const struct device
> *dev)
>  {
> -       return __acpi_match_device(acpi_companion_match(dev), ids,
> NULL);
> +       const struct acpi_device_id *id = NULL;
> +
> +       __acpi_match_device(acpi_companion_match(dev), ids, NULL, &id,
> NULL);
> +       return id;
>  }
>  EXPORT_SYMBOL_GPL(acpi_match_device);
>
> @@ -840,7 +854,7 @@ EXPORT_SYMBOL_GPL(acpi_get_match_data);
>  int acpi_match_device_ids(struct acpi_device *device,
>                           const struct acpi_device_id *ids)
>  {
> -       return __acpi_match_device(device, ids, NULL) ? 0 : -ENOENT;
> +       return __acpi_match_device(device, ids, NULL, NULL, NULL) ? 0 :
> -ENOENT;
>  }
>  EXPORT_SYMBOL(acpi_match_device_ids);
>
> @@ -849,10 +863,12 @@ bool acpi_driver_match_device(struct device *dev,
>  {
>         if (!drv->acpi_match_table)
>                 return acpi_of_match_device(ACPI_COMPANION(dev),
> -                                           drv->of_match_table);
> +                                           drv->of_match_table,
> +                                           NULL);
>
> -       return !!__acpi_match_device(acpi_companion_match(dev),
> -                                    drv->acpi_match_table, drv-
>>of_match_table);
> +       return __acpi_match_device(acpi_companion_match(dev),
> +                                  drv->acpi_match_table, drv-
>>of_match_table,
> +                                  NULL, NULL);
>  }
>  EXPORT_SYMBOL_GPL(acpi_driver_match_device);
>
> --
> 2.15.1
>
>
>
> --
> Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Intel Finland Oy