diff mbox series

[2/6,v3] mmc: debugfs: No blocklayer = no card status and extcsd

Message ID 20170608085403.11795-3-linus.walleij@linaro.org
State New
Headers show
Series None | expand

Commit Message

Linus Walleij June 8, 2017, 8:53 a.m. UTC 
If we don't have the block layer enabled, we do not present card
status and extcsd in the debugfs.

Debugfs is not ABI, and maintaining files of no relevance for
non-block devices comes at a high maintenance cost if we shall
support it with the block layer compiled out.

The expected number of debugfs users utilizing these two
debugfs files is already low as there is an ioctl() to get the
same information using the mmc-tools, and of these few users
the expected number of people using it on SDIO or combo cards
are expected to be zero.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

---
ChangeLog v2->v3:
- No changes just resending
---
 drivers/mmc/core/debugfs.c | 39 ++++++++++++++++++++++++++++++---------
 1 file changed, 30 insertions(+), 9 deletions(-)

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

Ulf Hansson June 16, 2017, 9:32 a.m. UTC | #1 
On 8 June 2017 at 10:53, Linus Walleij <linus.walleij@linaro.org> wrote:
> If we don't have the block layer enabled, we do not present card

> status and extcsd in the debugfs.

>

> Debugfs is not ABI, and maintaining files of no relevance for

> non-block devices comes at a high maintenance cost if we shall

> support it with the block layer compiled out.


Well, as a matter of fact the code is still there after these changes,
just in a different form.

I would rather set the arguments to why these changes are good, that
we want to minimize the number of users of the bif fat mmc lock, but
also to deal with mmc request through the regular I/O path as to avoid
starvation.

>

> The expected number of debugfs users utilizing these two

> debugfs files is already low as there is an ioctl() to get the

> same information using the mmc-tools, and of these few users

> the expected number of people using it on SDIO or combo cards

> are expected to be zero.


Yeah. I can have been thinking a bit more about this. So, perhaps this
is good first step, to not entirely remove the current debugfs nodes
for cards.

However, from maintenance point of view, this series doesn't really
make me more happy, rather the opposite. Some more comments below.

>

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> ---

> ChangeLog v2->v3:

> - No changes just resending

> ---

>  drivers/mmc/core/debugfs.c | 39 ++++++++++++++++++++++++++++++---------

>  1 file changed, 30 insertions(+), 9 deletions(-)

>

> diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c

> index a1fba5732d66..b176932b8092 100644

> --- a/drivers/mmc/core/debugfs.c

> +++ b/drivers/mmc/core/debugfs.c

> @@ -281,6 +281,8 @@ void mmc_remove_host_debugfs(struct mmc_host *host)

>         debugfs_remove_recursive(host->debugfs_root);

>  }

>

> +#if IS_ENABLED(CONFIG_MMC_BLOCK)

> +

>  static int mmc_dbg_card_status_get(void *data, u64 *val)

>  {

>         struct mmc_card *card = data;

> @@ -360,6 +362,32 @@ static const struct file_operations mmc_dbg_ext_csd_fops = {

>         .llseek         = default_llseek,

>  };

>

> +static int mmc_add_block_debugfs(struct mmc_card *card, struct dentry *root)

> +{

> +       if (mmc_card_mmc(card) || mmc_card_sd(card)) {

> +               if (!debugfs_create_file("status", S_IRUSR, root, card,

> +                                        &mmc_dbg_card_status_fops))

> +                       return -EIO;

> +       }

> +

> +       if (mmc_card_mmc(card)) {

> +               if (!debugfs_create_file("ext_csd", S_IRUSR, root, card,

> +                                        &mmc_dbg_ext_csd_fops))

> +                       return -EIO;

> +       }

> +

> +       return 0;

> +}

> +

> +#else /* !IS_ENABLED(CONFIG_MMC_BLOCK) */

> +

> +static int mmc_add_block_debugfs(struct mmc_card *card, struct dentry *root)

> +{


So does this really work as expected?

Currently one can build the mmc block as a separate module. If that
module is inserted after the mmc core module attempts to register the
debugfs nodes, we would end up in this stub function, right?

In other words, those system that for some odd reason decide to insmod
the mmc block module at a later point, won't be given the debugfs
nodes?

> +       return 0;

> +}

> +

> +#endif

> +

>  void mmc_add_card_debugfs(struct mmc_card *card)

>  {

>         struct mmc_host *host = card->host;

> @@ -382,15 +410,8 @@ void mmc_add_card_debugfs(struct mmc_card *card)

>         if (!debugfs_create_x32("state", S_IRUSR, root, &card->state))

>                 goto err;

>

> -       if (mmc_card_mmc(card) || mmc_card_sd(card))

> -               if (!debugfs_create_file("status", S_IRUSR, root, card,

> -                                       &mmc_dbg_card_status_fops))

> -                       goto err;

> -

> -       if (mmc_card_mmc(card))

> -               if (!debugfs_create_file("ext_csd", S_IRUSR, root, card,

> -                                       &mmc_dbg_ext_csd_fops))

> -                       goto err;

> +       if (mmc_add_block_debugfs(card, root))

> +               goto err;

>

>         return;

>

> --

> 2.9.4

>


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
Linus Walleij June 19, 2017, 9:25 a.m. UTC | #2 
On Fri, Jun 16, 2017 at 11:32 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 8 June 2017 at 10:53, Linus Walleij <linus.walleij@linaro.org> wrote:


>> +#else /* !IS_ENABLED(CONFIG_MMC_BLOCK) */

>> +

>> +static int mmc_add_block_debugfs(struct mmc_card *card, struct dentry *root)

>> +{

>

> So does this really work as expected?

>

> Currently one can build the mmc block as a separate module. If that

> module is inserted after the mmc core module attempts to register the

> debugfs nodes, we would end up in this stub function, right?


Not really. IS_ENABLED() is a compile-time flag. The stub is not even
compiled in if the block layer is selected as module.

I.e. if CONFIG_MMC_BLOCK is "y" or "m" then the real function gets
compiled in. If it is "n" the stub gets compiled in.

> In other words, those system that for some odd reason decide to insmod

> the mmc block module at a later point, won't be given the debugfs

> nodes?


This is not really related, since I have all the debugfs files for the
block layer as static functions in this patch, i.e. if the block layer
is inserted, the debugfs files come up.

On a broader note, AFAIK it is already impossible to insert the block
module after the core module (if it was selected as "m" at compile time),
as the block module must be inserted first, or the core module cannot
resolve the symbols it needs from the block module.

insmod will fail, and modprobe will load the required modules first,
so it orders the block layer to load before the core so that the symbols
are resolved.

Yours,
Linus Walleij
--
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 June 19, 2017, 10:04 a.m. UTC | #3 
On 19 June 2017 at 11:25, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Jun 16, 2017 at 11:32 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:

>> On 8 June 2017 at 10:53, Linus Walleij <linus.walleij@linaro.org> wrote:

>

>>> +#else /* !IS_ENABLED(CONFIG_MMC_BLOCK) */

>>> +

>>> +static int mmc_add_block_debugfs(struct mmc_card *card, struct dentry *root)

>>> +{

>>

>> So does this really work as expected?

>>

>> Currently one can build the mmc block as a separate module. If that

>> module is inserted after the mmc core module attempts to register the

>> debugfs nodes, we would end up in this stub function, right?

>

> Not really. IS_ENABLED() is a compile-time flag. The stub is not even

> compiled in if the block layer is selected as module.

>

> I.e. if CONFIG_MMC_BLOCK is "y" or "m" then the real function gets

> compiled in. If it is "n" the stub gets compiled in.


Okay.

>

>> In other words, those system that for some odd reason decide to insmod

>> the mmc block module at a later point, won't be given the debugfs

>> nodes?

>

> This is not really related, since I have all the debugfs files for the

> block layer as static functions in this patch, i.e. if the block layer

> is inserted, the debugfs files come up.


If that would be the case, the I am fine. But, I am not sure. See comment below.

>

> On a broader note, AFAIK it is already impossible to insert the block

> module after the core module (if it was selected as "m" at compile time),

> as the block module must be inserted first, or the core module cannot

> resolve the symbols it needs from the block module.


No, this is wrong. It's actually the opposite.

Currently there are no dependency from the core module on the block
module. However, the block module uses exported functions from the
core module, so the dependency is the opposite.

This is exactly what I was trying to say, we must not create a
circular dependency of the modules.

To solve this, you must move the entire creation of the debugfs nodes
to be done when the block device driver probes, in other words from
mmc_blk_probe().

>

> insmod will fail, and modprobe will load the required modules first,

> so it orders the block layer to load before the core so that the symbols

> are resolved.

>

> Yours,

> Linus Walleij


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
Linus Walleij June 19, 2017, 11:14 a.m. UTC | #4 
On Mon, Jun 19, 2017 at 12:04 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 19 June 2017 at 11:25, Linus Walleij <linus.walleij@linaro.org> wrote:


>> This is not really related, since I have all the debugfs files for the

>> block layer as static functions in this patch, i.e. if the block layer

>> is inserted, the debugfs files come up.

>

> If that would be the case, the I am fine. But, I am not sure. See comment below.

>

>> On a broader note, AFAIK it is already impossible to insert the block

>> module after the core module (if it was selected as "m" at compile time),

>> as the block module must be inserted first, or the core module cannot

>> resolve the symbols it needs from the block module.

>

> No, this is wrong. It's actually the opposite.

>

> Currently there are no dependency from the core module on the block

> module. However, the block module uses exported functions from the

> core module, so the dependency is the opposite.


Aha, I see how you mean.

> This is exactly what I was trying to say, we must not create a

> circular dependency of the modules.


I think modprobe resolves circular dependencies by inserting
both modules at the same time, actually. insmod will not work.

> To solve this, you must move the entire creation of the debugfs nodes

> to be done when the block device driver probes, in other words from

> mmc_blk_probe().


Yes and this is done in patch 6/6.

If you think it is dangerous to have this as an intermediary step, we
can just squash the patches.

Yours,
Linus Walleij
--
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 June 19, 2017, 11:19 a.m. UTC | #5 
On 19 June 2017 at 13:14, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, Jun 19, 2017 at 12:04 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:

>> On 19 June 2017 at 11:25, Linus Walleij <linus.walleij@linaro.org> wrote:

>

>>> This is not really related, since I have all the debugfs files for the

>>> block layer as static functions in this patch, i.e. if the block layer

>>> is inserted, the debugfs files come up.

>>

>> If that would be the case, the I am fine. But, I am not sure. See comment below.

>>

>>> On a broader note, AFAIK it is already impossible to insert the block

>>> module after the core module (if it was selected as "m" at compile time),

>>> as the block module must be inserted first, or the core module cannot

>>> resolve the symbols it needs from the block module.

>>

>> No, this is wrong. It's actually the opposite.

>>

>> Currently there are no dependency from the core module on the block

>> module. However, the block module uses exported functions from the

>> core module, so the dependency is the opposite.

>

> Aha, I see how you mean.

>

>> This is exactly what I was trying to say, we must not create a

>> circular dependency of the modules.

>

> I think modprobe resolves circular dependencies by inserting

> both modules at the same time, actually. insmod will not work.

>

>> To solve this, you must move the entire creation of the debugfs nodes

>> to be done when the block device driver probes, in other words from

>> mmc_blk_probe().

>

> Yes and this is done in patch 6/6.

>

> If you think it is dangerous to have this as an intermediary step, we

> can just squash the patches.


Yes, please do that!

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
diff mbox series

Patch

diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
index a1fba5732d66..b176932b8092 100644
--- a/drivers/mmc/core/debugfs.c
+++ b/drivers/mmc/core/debugfs.c
@@ -281,6 +281,8 @@  void mmc_remove_host_debugfs(struct mmc_host *host)
 	debugfs_remove_recursive(host->debugfs_root);
 }
 
+#if IS_ENABLED(CONFIG_MMC_BLOCK)
+
 static int mmc_dbg_card_status_get(void *data, u64 *val)
 {
 	struct mmc_card	*card = data;
@@ -360,6 +362,32 @@  static const struct file_operations mmc_dbg_ext_csd_fops = {
 	.llseek		= default_llseek,
 };
 
+static int mmc_add_block_debugfs(struct mmc_card *card, struct dentry *root)
+{
+	if (mmc_card_mmc(card) || mmc_card_sd(card)) {
+		if (!debugfs_create_file("status", S_IRUSR, root, card,
+					 &mmc_dbg_card_status_fops))
+			return -EIO;
+	}
+
+	if (mmc_card_mmc(card)) {
+		if (!debugfs_create_file("ext_csd", S_IRUSR, root, card,
+					 &mmc_dbg_ext_csd_fops))
+			return -EIO;
+	}
+
+	return 0;
+}
+
+#else /* !IS_ENABLED(CONFIG_MMC_BLOCK) */
+
+static int mmc_add_block_debugfs(struct mmc_card *card, struct dentry *root)
+{
+	return 0;
+}
+
+#endif
+
 void mmc_add_card_debugfs(struct mmc_card *card)
 {
 	struct mmc_host	*host = card->host;
@@ -382,15 +410,8 @@  void mmc_add_card_debugfs(struct mmc_card *card)
 	if (!debugfs_create_x32("state", S_IRUSR, root, &card->state))
 		goto err;
 
-	if (mmc_card_mmc(card) || mmc_card_sd(card))
-		if (!debugfs_create_file("status", S_IRUSR, root, card,
-					&mmc_dbg_card_status_fops))
-			goto err;
-
-	if (mmc_card_mmc(card))
-		if (!debugfs_create_file("ext_csd", S_IRUSR, root, card,
-					&mmc_dbg_ext_csd_fops))
-			goto err;
+	if (mmc_add_block_debugfs(card, root))
+		goto err;
 
 	return;