| Message ID | 20170608085403.11795-3-linus.walleij@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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 --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;
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(-)