diff mbox

[v2] mmc: mmci_qcom_dml: include mmci_qcom_dml.h and fix #ifdef

Message ID 1501853664-10752-1-git-send-email-yamada.masahiro@socionext.com
State New
Headers show

Commit Message

Masahiro Yamada Aug. 4, 2017, 1:34 p.m. UTC 
Include mmci_qcom_dml.h from mmci_qcom_dml.c to fix the following
sparse warnings:

  CHECK   drivers/mmc/host/mmci_qcom_dml.c
drivers/mmc/host/mmci_qcom_dml.c:57:6: warning: symbol 'dml_start_xfer' was not declared. Should it be static?
drivers/mmc/host/mmci_qcom_dml.c:122:5: warning: symbol 'dml_hw_init' was not declared. Should it be static?

Fixing them causes redefintion of dml_start_xfer error, revealing another
problem in the header.  #ifdef CONFIG_MMC_QCOM_DML is wrong because this
driver is tristate.  (CONFIG_MMC_QCOM_DML_MODULE is defined when it is
built as a module)

Since dml_hw_init() is called from mmci.c, IS_REACHABLE() is needed to
cater to all the combinations.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

---

Changes in v2:
  - Fix error reported by kbuild test robot
   (this patch is intended to replace commit 64f0aacb in linux-mmc/fixes

 drivers/mmc/host/mmci_qcom_dml.c | 1 +
 drivers/mmc/host/mmci_qcom_dml.h | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Arnd Bergmann Aug. 4, 2017, 10:10 p.m. UTC | #1 
On Fri, Aug 4, 2017 at 3:34 PM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> Include mmci_qcom_dml.h from mmci_qcom_dml.c to fix the following

> sparse warnings:

>

>   CHECK   drivers/mmc/host/mmci_qcom_dml.c

> drivers/mmc/host/mmci_qcom_dml.c:57:6: warning: symbol 'dml_start_xfer' was not declared. Should it be static?

> drivers/mmc/host/mmci_qcom_dml.c:122:5: warning: symbol 'dml_hw_init' was not declared. Should it be static?

>

> Fixing them causes redefintion of dml_start_xfer error, revealing another

> problem in the header.  #ifdef CONFIG_MMC_QCOM_DML is wrong because this

> driver is tristate.  (CONFIG_MMC_QCOM_DML_MODULE is defined when it is

> built as a module)

>

> Since dml_hw_init() is called from mmci.c, IS_REACHABLE() is needed to

> cater to all the combinations.

>

> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>


I think this is still not a good solution, it will just turn a link error into
an unusable system at runtime when the DML code is a loadable module
but not used. Also, the symbols are not exported, so it won't work when both
are built as modules.
How about linking the DML code into the mmci module and making that
Kconfig option a 'bool'?

Another small problem I found is the use of writel_relaxed() that might
be unsafe here and is not explained anywhere.

Finally, I can't find any record of the 9cb15142d0e3 ("mmc: mmci:
Add qcom dml support to the driver.") patch in the archives of the relevant
mailing lists I'm subscribed to (linux-mmc, linux-kernel or linux-arm-kernel).

Was it posted there recently? Was Russell on Cc on the patch?
The only record I find using google is for the patch from 2014, in
https://patchwork.kernel.org/patch/4638221/ Ulf said he queued the
patch for linux-3.18.

      Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Masahiro Yamada Aug. 6, 2017, 2:58 a.m. UTC | #2 
Hi Arnd,



2017-08-05 7:10 GMT+09:00 Arnd Bergmann <arnd@arndb.de>:
> On Fri, Aug 4, 2017 at 3:34 PM, Masahiro Yamada

> <yamada.masahiro@socionext.com> wrote:

>> Include mmci_qcom_dml.h from mmci_qcom_dml.c to fix the following

>> sparse warnings:

>>

>>   CHECK   drivers/mmc/host/mmci_qcom_dml.c

>> drivers/mmc/host/mmci_qcom_dml.c:57:6: warning: symbol 'dml_start_xfer' was not declared. Should it be static?

>> drivers/mmc/host/mmci_qcom_dml.c:122:5: warning: symbol 'dml_hw_init' was not declared. Should it be static?

>>

>> Fixing them causes redefintion of dml_start_xfer error, revealing another

>> problem in the header.  #ifdef CONFIG_MMC_QCOM_DML is wrong because this

>> driver is tristate.  (CONFIG_MMC_QCOM_DML_MODULE is defined when it is

>> built as a module)

>>

>> Since dml_hw_init() is called from mmci.c, IS_REACHABLE() is needed to

>> cater to all the combinations.

>>

>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

>

> I think this is still not a good solution, it will just turn a link error into

> an unusable system at runtime when the DML code is a loadable module

> but not used. Also, the symbols are not exported, so it won't work when both

> are built as modules.

> How about linking the DML code into the mmci module and making that

> Kconfig option a 'bool'?



You are right.
My patch does not solve the root of the problem.

It turned out not so trivial as I had first expected.

I'd like somebody else to take care of it
because I am not familiar with this driver.

(My first motivation was clean-up sparse reports,
and I thought it was easy to fix it, but it was not.)




-- 
Best Regards
Masahiro Yamada
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Aug. 6, 2017, 9:32 a.m. UTC | #3 
On Sun, Aug 6, 2017 at 4:58 AM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> 2017-08-05 7:10 GMT+09:00 Arnd Bergmann <arnd@arndb.de>:

>> On Fri, Aug 4, 2017 at 3:34 PM, Masahiro Yamada

>> <yamada.masahiro@socionext.com> wrote:

>>> Include mmci_qcom_dml.h from mmci_qcom_dml.c to fix the following

>>> sparse warnings:

>>>

>>>   CHECK   drivers/mmc/host/mmci_qcom_dml.c

>>> drivers/mmc/host/mmci_qcom_dml.c:57:6: warning: symbol 'dml_start_xfer' was not declared. Should it be static?

>>> drivers/mmc/host/mmci_qcom_dml.c:122:5: warning: symbol 'dml_hw_init' was not declared. Should it be static?

>>>

>>> Fixing them causes redefintion of dml_start_xfer error, revealing another

>>> problem in the header.  #ifdef CONFIG_MMC_QCOM_DML is wrong because this

>>> driver is tristate.  (CONFIG_MMC_QCOM_DML_MODULE is defined when it is

>>> built as a module)

>>>

>>> Since dml_hw_init() is called from mmci.c, IS_REACHABLE() is needed to

>>> cater to all the combinations.

>>>

>>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

>>

>> I think this is still not a good solution, it will just turn a link error into

>> an unusable system at runtime when the DML code is a loadable module

>> but not used. Also, the symbols are not exported, so it won't work when both

>> are built as modules.

>> How about linking the DML code into the mmci module and making that

>> Kconfig option a 'bool'?

>

>

> You are right.

> My patch does not solve the root of the problem.

>

> It turned out not so trivial as I had first expected.

>

> I'd like somebody else to take care of it

> because I am not familiar with this driver.


Maybe it's best if Ulf just reverts the broken commit, and then we start
over with reviewing the patch properly.

         Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson Aug. 6, 2017, 10:10 a.m. UTC | #4 
On 6 August 2017 at 11:32, Arnd Bergmann <arnd@arndb.de> wrote:
> On Sun, Aug 6, 2017 at 4:58 AM, Masahiro Yamada

> <yamada.masahiro@socionext.com> wrote:

>> 2017-08-05 7:10 GMT+09:00 Arnd Bergmann <arnd@arndb.de>:

>>> On Fri, Aug 4, 2017 at 3:34 PM, Masahiro Yamada

>>> <yamada.masahiro@socionext.com> wrote:

>>>> Include mmci_qcom_dml.h from mmci_qcom_dml.c to fix the following

>>>> sparse warnings:

>>>>

>>>>   CHECK   drivers/mmc/host/mmci_qcom_dml.c

>>>> drivers/mmc/host/mmci_qcom_dml.c:57:6: warning: symbol 'dml_start_xfer' was not declared. Should it be static?

>>>> drivers/mmc/host/mmci_qcom_dml.c:122:5: warning: symbol 'dml_hw_init' was not declared. Should it be static?

>>>>

>>>> Fixing them causes redefintion of dml_start_xfer error, revealing another

>>>> problem in the header.  #ifdef CONFIG_MMC_QCOM_DML is wrong because this

>>>> driver is tristate.  (CONFIG_MMC_QCOM_DML_MODULE is defined when it is

>>>> built as a module)

>>>>

>>>> Since dml_hw_init() is called from mmci.c, IS_REACHABLE() is needed to

>>>> cater to all the combinations.

>>>>

>>>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

>>>

>>> I think this is still not a good solution, it will just turn a link error into

>>> an unusable system at runtime when the DML code is a loadable module

>>> but not used. Also, the symbols are not exported, so it won't work when both

>>> are built as modules.

>>> How about linking the DML code into the mmci module and making that

>>> Kconfig option a 'bool'?

>>

>>

>> You are right.

>> My patch does not solve the root of the problem.

>>

>> It turned out not so trivial as I had first expected.

>>

>> I'd like somebody else to take care of it

>> because I am not familiar with this driver.

>

> Maybe it's best if Ulf just reverts the broken commit, and then we start

> over with reviewing the patch properly.


The broken commit is reverted, however I will just drop both the bad
commit and its revert next time I rebase my next branch.

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Srinivas Kandagatla Aug. 6, 2017, 10:26 a.m. UTC | #5 
On 04/08/17 23:10, Arnd Bergmann wrote:
> How about linking the DML code into the mmci module and making that

> Kconfig option a 'bool'?



Yes, I think making this bool and exporting the two symbols should fix 
this. It does not make sense to make dml helpers a module anyway.

If it sounds okay, I can send a proper patch to fix this.

------------------------>cut<-----------------------------------

  {
@@ -175,3 +176,4 @@ int dml_hw_init(struct mmci_host *host, struct 
device_node *np)

         return 0;
------------------------>cut<-----------------------------------


thanks,
srini
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.htmldiff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 5755b69..3345384 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -15,7 +15,7 @@ config MMC_ARMMMCI
           If unsure, say N.

  config MMC_QCOM_DML
-       tristate "Qualcomm Data Mover for SD Card Controller"
+       bool "Qualcomm Data Mover for SD Card Controller"
         depends on MMC_ARMMMCI && QCOM_BAM_DMA
         default y
         help
diff --git a/drivers/mmc/host/mmci_qcom_dml.c 
b/drivers/mmc/host/mmci_qcom_dml.c
index 00750c9..e7d9c74 100644
--- a/drivers/mmc/host/mmci_qcom_dml.c
+++ b/drivers/mmc/host/mmci_qcom_dml.c
@@ -97,6 +97,7 @@ void dml_start_xfer(struct mmci_host *host, struct 
mmc_data *data)
         /* make sure the dml is configured before dma is triggered */
         wmb();
  }
+EXPORT_SYMBOL_GPL(dml_start_xfer);

  static int of_get_dml_pipe_index(struct device_node *np, const char *name)
kernel test robot Aug. 6, 2017, 11:12 a.m. UTC | #6 
Hi Masahiro,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.13-rc3 next-20170804]
[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/Masahiro-Yamada/mmc-mmci_qcom_dml-include-mmci_qcom_dml-h-and-fix-ifdef/20170805-173714
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All errors (new ones prefixed by >>):

>> ERROR: "dml_hw_init" [drivers/mmc/host/mmci.ko] undefined!

>> ERROR: "dml_start_xfer" [drivers/mmc/host/mmci.ko] undefined!


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

Patch

diff --git a/drivers/mmc/host/mmci_qcom_dml.c b/drivers/mmc/host/mmci_qcom_dml.c
index 00750c9d3514..95de699853d2 100644
--- a/drivers/mmc/host/mmci_qcom_dml.c
+++ b/drivers/mmc/host/mmci_qcom_dml.c
@@ -18,6 +18,7 @@ 
 #include <linux/mmc/host.h>
 #include <linux/mmc/card.h>
 #include "mmci.h"
+#include "mmci_qcom_dml.h"
 
 /* Registers */
 #define DML_CONFIG			0x00
diff --git a/drivers/mmc/host/mmci_qcom_dml.h b/drivers/mmc/host/mmci_qcom_dml.h
index 6e405d09d534..d5e88f102ba3 100644
--- a/drivers/mmc/host/mmci_qcom_dml.h
+++ b/drivers/mmc/host/mmci_qcom_dml.h
@@ -15,7 +15,7 @@ 
 #ifndef __MMC_QCOM_DML_H__
 #define __MMC_QCOM_DML_H__
 
-#ifdef CONFIG_MMC_QCOM_DML
+#if IS_REACHABLE(CONFIG_MMC_QCOM_DML)
 int dml_hw_init(struct mmci_host *host, struct device_node *np);
 void dml_start_xfer(struct mmci_host *host, struct mmc_data *data);
 #else