[PATCH v1 1/4] ACPI / bus: Remove checks in acpi_get_match_data()

[PATCH v1 1/4] 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, if pure OF driver calls this function, it's weird to have ACPI
companion without ACPI ID in this case.

Fixes: 80212a162329 ("ACPI / bus: Introduce acpi_get_match_data() function")
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 f87ed3be779a..b271eb16341d 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -789,12 +789,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 v1 2/4] 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.

Fixes: 80212a162329 ("ACPI / bus: Introduce acpi_get_match_data() function")
Fixes: 146b4dbb0eef ("ACPI: properties: Implement get_match_data() callback")
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 b271eb16341d..da29c103f76a 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -785,7 +785,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;
 
@@ -795,7 +795,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 v1 3/4] dmaengine: hidma: Constify returned by device_get_match_data() value

[Andy Shevchenko]

The value under the hood is constant in any case.
It makes sense to show this explicitly.

Fixes: 95fbfb7aa28d ("dmaengine: qcom_hidma: Add support for the new revision")
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/dma/qcom/hidma.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/qcom/hidma.c b/drivers/dma/qcom/hidma.c
index 963cc5228d05..24e163cefb7c 100644
--- a/drivers/dma/qcom/hidma.c
+++ b/drivers/dma/qcom/hidma.c
@@ -743,9 +743,9 @@ static int hidma_request_msi(struct hidma_dev *dmadev,
 
 static bool hidma_test_capability(struct device *dev, enum hidma_cap test_cap)
 {
-	enum hidma_cap cap;
+	const enum hidma_cap cap;
 
-	cap = (enum hidma_cap) device_get_match_data(dev);
+	cap = (const enum hidma_cap) device_get_match_data(dev);
 	return cap ? ((cap & test_cap) > 0) : 0;
 }
 
-- 
2.15.1

[PATCH v1 4/4] device property: Constify device_get_match_data()

[Andy Shevchenko]

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

Fixes: 80212a162329 ("ACPI / bus: Introduce acpi_get_match_data() function")
Fixes: b283f157611f ("device property: Introduce a common API to fetch device match data")
Fixes: 146b4dbb0eef ("ACPI: properties: Implement get_match_data() callback")
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 +-
 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 da29c103f76a..f4bba5b03843 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -785,7 +785,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;
 
@@ -793,7 +793,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 v1 1/4] ACPI / bus: Remove checks in acpi_get_match_data()

[Sinan Kaya]

On 1/31/2018 4:29 PM, Andy Shevchenko wrote:
> 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, if pure OF driver calls this function, it's weird to have ACPI
> companion without ACPI ID in this case.

We talked about this during review. 

of_match_device() does all the checking for the OF part. ACPI doesn't have
any checks.

-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v1 1/4] ACPI / bus: Remove checks in acpi_get_match_data()

[Rafael J. Wysocki]

On Wed, Jan 31, 2018 at 11:17 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
> On 1/31/2018 4:29 PM, Andy Shevchenko wrote:
>> 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, if pure OF driver calls this function, it's weird to have ACPI
>> companion without ACPI ID in this case.
>
> We talked about this during review.
>
> of_match_device() does all the checking for the OF part. ACPI doesn't have
> any checks.

Yeah, this patch is just plain incorrect AFAICS.

Re: [PATCH v1 1/4] ACPI / bus: Remove checks in acpi_get_match_data()

[Andy Shevchenko]

On Thu, 2018-02-01 at 08:27 +0100, Rafael J. Wysocki wrote:
> On Wed, Jan 31, 2018 at 11:17 PM, Sinan Kaya <okaya@codeaurora.org>
> wrote:
> > On 1/31/2018 4:29 PM, Andy Shevchenko wrote:
> > > 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, if pure OF driver calls this function, it's weird to have
> > > ACPI
> > > companion without ACPI ID in this case.
> > 
> > We talked about this during review.
> > 
> > of_match_device() does all the checking for the OF part. ACPI
> > doesn't have
> > any checks.
> 
> Yeah, this patch is just plain incorrect AFAICS.

I don't see how check dev->driver is implemented on OF side then


of_device_get_match_data() which is called by
of_fwnode_device_get_match_data() has dereferenced dev->driver w/o any
check.

I can't agree that the patch is plain incorrect, if I didn't miss
anything.

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

Re: [PATCH v1 1/4] ACPI / bus: Remove checks in acpi_get_match_data()

[okaya]

On 2018-02-01 07:24, Andy Shevchenko wrote:
> On Thu, 2018-02-01 at 08:27 +0100, Rafael J. Wysocki wrote:
>> On Wed, Jan 31, 2018 at 11:17 PM, Sinan Kaya <okaya@codeaurora.org>
>> wrote:
>> > On 1/31/2018 4:29 PM, Andy Shevchenko wrote:
>> > > 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, if pure OF driver calls this function, it's weird to have
>> > > ACPI
>> > > companion without ACPI ID in this case.
>> >
>> > We talked about this during review.
>> >
>> > of_match_device() does all the checking for the OF part. ACPI
>> > doesn't have
>> > any checks.
>> 
>> Yeah, this patch is just plain incorrect AFAICS.
> 
> I don't see how check dev->driver is implemented on OF side then
> 
> 
> of_device_get_match_data() which is called by
> of_fwnode_device_get_match_data() has dereferenced dev->driver w/o any
> check.
> 
> I can't agree that the patch is plain incorrect, if I didn't miss
> anything.


Sorry, i should have been more specific. I was talkimg about match_data 
not driver.

I agree that driver check is redundant.

Re: [PATCH v1 1/4] ACPI / bus: Remove checks in acpi_get_match_data()

[Rafael J. Wysocki]

On Thursday, February 1, 2018 1:24:53 PM CET Andy Shevchenko wrote:
> On Thu, 2018-02-01 at 08:27 +0100, Rafael J. Wysocki wrote:
> > On Wed, Jan 31, 2018 at 11:17 PM, Sinan Kaya <okaya@codeaurora.org>
> > wrote:
> > > On 1/31/2018 4:29 PM, Andy Shevchenko wrote:
> > > > 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, if pure OF driver calls this function, it's weird to have
> > > > ACPI
> > > > companion without ACPI ID in this case.
> > > 
> > > We talked about this during review.
> > > 
> > > of_match_device() does all the checking for the OF part. ACPI
> > > doesn't have
> > > any checks.
> > 
> > Yeah, this patch is just plain incorrect AFAICS.
> 
> I don't see how check dev->driver is implemented on OF side then
> 
> 
> of_device_get_match_data() which is called by
> of_fwnode_device_get_match_data() has dereferenced dev->driver w/o any
> check.
> 
> I can't agree that the patch is plain incorrect, if I didn't miss
> anything.

OK, you're right, sorry.

Re: [PATCH v1 3/4] dmaengine: hidma: Constify returned by device_get_match_data() value

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1343 bytes --]

Hi Andy,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on next-20180202]
[cannot apply to pm/linux-next v4.15]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Andy-Shevchenko/ACPI-bus-Remove-checks-in-acpi_get_match_data/20180204-000348
config: x86_64-randconfig-x014-201805 (attached as .config)
compiler: gcc-7 (Debian 7.2.0-12) 7.2.1 20171025
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/dma/qcom/hidma.c: In function 'hidma_test_capability':
>> drivers/dma/qcom/hidma.c:748:6: error: assignment of read-only variable 'cap'
     cap = (const enum hidma_cap) device_get_match_data(dev);
         ^

vim +/cap +748 drivers/dma/qcom/hidma.c

   743	
   744	static bool hidma_test_capability(struct device *dev, enum hidma_cap test_cap)
   745	{
   746		const enum hidma_cap cap;
   747	
 > 748		cap = (const enum hidma_cap) device_get_match_data(dev);
   749		return cap ? ((cap & test_cap) > 0) : 0;
   750	}
   751	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27490 bytes --]