Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1703993 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v2] mmc: mmci_qcom_dml: include mmci_qcom_dml.h and fix #ifdef Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-08-04 15:40 +0200
    Re: [PATCH v2] mmc: mmci_qcom_dml: include mmci_qcom_dml.h and fix #ifdef Arnd Bergmann <arnd@arndb.de> - 2017-08-05 00:20 +0200
      Re: [PATCH v2] mmc: mmci_qcom_dml: include mmci_qcom_dml.h and fix #ifdef Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-08-06 05:00 +0200
        Re: [PATCH v2] mmc: mmci_qcom_dml: include mmci_qcom_dml.h and fix #ifdef Arnd Bergmann <arnd@arndb.de> - 2017-08-06 11:40 +0200
          Re: [PATCH v2] mmc: mmci_qcom_dml: include mmci_qcom_dml.h and fix #ifdef Ulf Hansson <ulf.hansson@linaro.org> - 2017-08-06 12:20 +0200
      Re: [PATCH v2] mmc: mmci_qcom_dml: include mmci_qcom_dml.h and fix  #ifdef Srinivas Kandagatla <srinivas.kandagatla@linaro.org> - 2017-08-06 12:30 +0200
        Re: [PATCH v2] mmc: mmci_qcom_dml: include mmci_qcom_dml.h and fix #ifdef Arnd Bergmann <arnd@arndb.de> - 2017-08-06 22:50 +0200

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

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

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

[toc] | [next] | [standalone]

#1704252

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

[toc] | [prev] | [next] | [standalone]

#1704736

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

[toc] | [prev] | [next] | [standalone]

#1704800

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

[toc] | [prev] | [next] | [standalone]

#1704806

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

[toc] | [prev] | [next] | [standalone]

#1704808 — Re: [PATCH v2] mmc: mmci_qcom_dml: include mmci_qcom_dml.h and fix #ifdef

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

diff --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)
  {
@@ -175,3 +176,4 @@ int dml_hw_init(struct mmci_host *host, struct 
device_node *np)

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


thanks,
srini

[toc] | [prev] | [next] | [standalone]

#1704968

On Sun, Aug 6, 2017 at 12:26 PM, Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
>
> 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.

I would say making dml a 'bool' option is not ok unless you also change
the Makefile to link it into the same module as the base mmci driver.

The alternative is to export the symbols and then always build the
dml code as a module when the mmci driver is a module, as below.

Or even just move all the code into the base driver, and call it only
using an if(IS_ENABLED()) check for the bool symbol.

       Arnd

diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index 8c46766c000c..5c996cba9f25 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -3,7 +3,9 @@
 #

 obj-$(CONFIG_MMC_ARMMMCI) += mmci.o
-obj-$(CONFIG_MMC_QCOM_DML) += mmci_qcom_dml.o
+ifdef CONFIG_MMC_QCOM_DML
+obj-$(CONFIG_MMC_ARMMMCI) += mmci_qcom_dml.o
+endif
 obj-$(CONFIG_MMC_PXA) += pxamci.o
 obj-$(CONFIG_MMC_MXC) += mxcmmc.o
 obj-$(CONFIG_MMC_MXS) += mxs-mmc.o

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web