[5/6] mmc: debugfs: Move card status retrieveal into the block layer

Message ID 20170519133732.27470-6-linus.walleij@linaro.org
State New
Headers show
Series
  • More MMC block core refactorings
Related show

Commit Message

Linus Walleij May 19, 2017, 1:37 p.m.
The debugfs file "status" (in e.g. /debug/mmc3/mmc3:0001) is
only available if and only if the card used is an (e)MMC or
SD card, not for SDIO, as can be seen from this guard in
mmc_add_card_debugfs();

if (mmc_card_mmc(card) || mmc_card_sd(card))
  (...create debugfs "status" entry...)

Further this debugfs entry suffers from all the same starvation
issues as the other userspace things, under e.g. a heavy
dd operation.

It is therefore logical to move this over to the block layer
when it is enabled, using the new custom requests and issue
it using the block request queue.

This makes this debugfs card access land under the request
queue host lock instead of orthogonally taking the lock.

Tested during heavy dd load by cat:in the status file. We
add IS_ENABLED() guards and keep the code snippet just
issueing the card status as a static inline in the header
so we can still have card status working when the block
layer is compiled out.

Keeping two copies of mmc_dbg_card_status_get() around
seems to be a necessary evil to be able to have the MMC/SD
stack working with the block layer disabled: under these
circumstances, the code must simply take another path.

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

---
 drivers/mmc/core/block.c   | 28 ++++++++++++++++++++++++++++
 drivers/mmc/core/block.h   | 37 +++++++++++++++++++++++++++++++++++++
 drivers/mmc/core/debugfs.c | 15 ++-------------
 drivers/mmc/core/queue.h   |  2 ++
 4 files changed, 69 insertions(+), 13 deletions(-)

-- 
2.9.3

--
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 May 22, 2017, 7:42 a.m. | #1
On 19 May 2017 at 15:37, Linus Walleij <linus.walleij@linaro.org> wrote:
> The debugfs file "status" (in e.g. /debug/mmc3/mmc3:0001) is

> only available if and only if the card used is an (e)MMC or

> SD card, not for SDIO, as can be seen from this guard in

> mmc_add_card_debugfs();

>

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

>   (...create debugfs "status" entry...)

>

> Further this debugfs entry suffers from all the same starvation

> issues as the other userspace things, under e.g. a heavy

> dd operation.

>

> It is therefore logical to move this over to the block layer

> when it is enabled, using the new custom requests and issue

> it using the block request queue.

>

> This makes this debugfs card access land under the request

> queue host lock instead of orthogonally taking the lock.

>

> Tested during heavy dd load by cat:in the status file. We

> add IS_ENABLED() guards and keep the code snippet just

> issueing the card status as a static inline in the header

> so we can still have card status working when the block

> layer is compiled out.


Seems like a bit of re-factoring/cleanup could help here as
preparation step. I just posted a patch [1] cleaning up how the mmc
block layer fetches the card's status.

Perhaps that could at least simplify a bit for $subject patch,
especially since it also makes mmc_send_status() being exported.

>

> Keeping two copies of mmc_dbg_card_status_get() around

> seems to be a necessary evil to be able to have the MMC/SD

> stack working with the block layer disabled: under these

> circumstances, the code must simply take another path.

>

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

> ---

>  drivers/mmc/core/block.c   | 28 ++++++++++++++++++++++++++++

>  drivers/mmc/core/block.h   | 37 +++++++++++++++++++++++++++++++++++++

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

>  drivers/mmc/core/queue.h   |  2 ++

>  4 files changed, 69 insertions(+), 13 deletions(-)

>

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

> index 52635120a0a5..8858798d1349 100644

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

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

> @@ -1191,6 +1191,7 @@ static void mmc_blk_issue_drv_op(struct mmc_queue *mq, struct request *req)

>         struct mmc_queue_req *mq_rq;

>         struct mmc_card *card = mq->card;

>         struct mmc_blk_data *md = mq->blkdata;

> +       u32 status;

>         int ret;

>         int i;

>

> @@ -1219,6 +1220,11 @@ static void mmc_blk_issue_drv_op(struct mmc_queue *mq, struct request *req)

>                         card->ext_csd.boot_ro_lock |=

>                                 EXT_CSD_BOOT_WP_B_PWR_WP_EN;

>                 break;

> +       case MMC_DRV_OP_GET_CARD_STATUS:

> +               ret = mmc_send_status(card, &status);

> +               if (!ret)

> +                       ret = status;

> +               break;

>         default:

>                 pr_err("%s: unknown driver specific operation\n",

>                        md->disk->disk_name);

> @@ -1968,6 +1974,28 @@ void mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)

>                 mmc_put_card(card);

>  }

>

> +/* Called from debugfs for MMC/SD cards */

> +int mmc_blk_card_status_get(struct mmc_card *card, u64 *val)

> +{

> +       struct mmc_blk_data *md = dev_get_drvdata(&card->dev);

> +       struct mmc_queue *mq = &md->queue;

> +       struct request *req;

> +       int ret;

> +

> +       /* Ask the block layer about the card status */

> +       req = blk_get_request(mq->queue, REQ_OP_DRV_IN, __GFP_RECLAIM);

> +       req_to_mmc_queue_req(req)->drv_op = MMC_DRV_OP_GET_CARD_STATUS;

> +       blk_execute_rq(mq->queue, NULL, req, 0);

> +       ret = req_to_mmc_queue_req(req)->drv_op_result;

> +       if (ret >= 0) {

> +               *val = ret;

> +               ret = 0;

> +       }

> +

> +       return ret;

> +}

> +EXPORT_SYMBOL(mmc_blk_card_status_get);

> +

>  static inline int mmc_blk_readonly(struct mmc_card *card)

>  {

>         return mmc_card_readonly(card) ||

> diff --git a/drivers/mmc/core/block.h b/drivers/mmc/core/block.h

> index 860ca7c8df86..1e26755a864b 100644

> --- a/drivers/mmc/core/block.h

> +++ b/drivers/mmc/core/block.h

> @@ -1,9 +1,46 @@

>  #ifndef _MMC_CORE_BLOCK_H

>  #define _MMC_CORE_BLOCK_H

>

> +#include <linux/kconfig.h>

> +#include "core.h"

> +#include "mmc_ops.h"

> +

>  struct mmc_queue;

>  struct request;

>

> +#if IS_ENABLED(CONFIG_MMC_BLOCK)


What wrong with a regular ifdefs stubs? Why do you need IS_ENABLED()?

> +

>  void mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req);


I don't get what mmc_blk_issue_rq() has to do with this change? Could
you please explain?

> +int mmc_blk_card_status_get(struct mmc_card *card, u64 *val);

> +

> +#else

> +

> +/*

> + * Small stub functions to be used when the block layer is not

> + * enabled, e.g. for pure SDIO without MMC/SD configurations.

> + */

> +

> +static inline void mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)

> +{

> +       return;

> +}

> +

> +static inline int mmc_blk_card_status_get(struct mmc_card *card, u64 *val)

> +{

> +       u32 status;

> +       int ret;

> +

> +       mmc_get_card(card);

> +

> +       ret = mmc_send_status(card, &status);

> +       if (!ret)

> +               *val = status;

> +

> +       mmc_put_card(card);

> +

> +       return ret;

> +}

> +

> +#endif /* IS_ENABLED(CONFIG_MMC_BLOCK) */

>

>  #endif

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

> index a1fba5732d66..ce5b921c7d96 100644

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

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

> @@ -19,6 +19,7 @@

>  #include <linux/mmc/card.h>

>  #include <linux/mmc/host.h>

>

> +#include "block.h"

>  #include "core.h"

>  #include "card.h"

>  #include "host.h"

> @@ -283,19 +284,7 @@ void mmc_remove_host_debugfs(struct mmc_host *host)

>

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

>  {

> -       struct mmc_card *card = data;

> -       u32             status;

> -       int             ret;

> -

> -       mmc_get_card(card);

> -

> -       ret = mmc_send_status(data, &status);

> -       if (!ret)

> -               *val = status;

> -

> -       mmc_put_card(card);

> -

> -       return ret;

> +       return mmc_blk_card_status_get(data, val);

>  }

>  DEFINE_SIMPLE_ATTRIBUTE(mmc_dbg_card_status_fops, mmc_dbg_card_status_get,

>                 NULL, "%08llx\n");

> diff --git a/drivers/mmc/core/queue.h b/drivers/mmc/core/queue.h

> index 361b46408e0f..2b39717453a5 100644

> --- a/drivers/mmc/core/queue.h

> +++ b/drivers/mmc/core/queue.h

> @@ -36,10 +36,12 @@ struct mmc_blk_request {

>   * enum mmc_drv_op - enumerates the operations in the mmc_queue_req

>   * @MMC_DRV_OP_IOCTL: ioctl operation

>   * @MMC_DRV_OP_BOOT_WP: write protect boot partitions

> + * @MMC_DRV_OP_GET_CARD_STATUS: get card status

>   */

>  enum mmc_drv_op {

>         MMC_DRV_OP_IOCTL,

>         MMC_DRV_OP_BOOT_WP,

> +       MMC_DRV_OP_GET_CARD_STATUS,

>  };

>

>  struct mmc_queue_req {

> --

> 2.9.3

>


Hmm, this thing seems a bit upside-down.

Currently it's possible to build the mmc block device driver as a
module. In cases like this, accessing the card debugs node to request
the card's status, would trigger a call to mmc_blk_card_status_get().
How would that work when the mmc block device driver isn't loaded and
probed?

It seems like the life cycle of the card debugfs node is now being
controlled as when the mmc block device driver has been successfully
probed. We need to deal with that somehow.

Kind regards
Uffe

[1]
https://patchwork.kernel.org/patch/9739663/
--
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 May 23, 2017, 9:49 a.m. | #2
On Mon, May 22, 2017 at 9:42 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 19 May 2017 at 15:37, Linus Walleij <linus.walleij@linaro.org> wrote:


>> The debugfs file "status" (in e.g. /debug/mmc3/mmc3:0001) is

>> only available if and only if the card used is an (e)MMC or

>> SD card, not for SDIO, as can be seen from this guard in

>> mmc_add_card_debugfs();

>>

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

>>   (...create debugfs "status" entry...)

>>

>> Further this debugfs entry suffers from all the same starvation

>> issues as the other userspace things, under e.g. a heavy

>> dd operation.

>>

>> It is therefore logical to move this over to the block layer

>> when it is enabled, using the new custom requests and issue

>> it using the block request queue.

>>

>> This makes this debugfs card access land under the request

>> queue host lock instead of orthogonally taking the lock.

>>

>> Tested during heavy dd load by cat:in the status file. We

>> add IS_ENABLED() guards and keep the code snippet just

>> issueing the card status as a static inline in the header

>> so we can still have card status working when the block

>> layer is compiled out.

>

> Seems like a bit of re-factoring/cleanup could help here as

> preparation step. I just posted a patch [1] cleaning up how the mmc

> block layer fetches the card's status.

>

> Perhaps that could at least simplify a bit for $subject patch,

> especially since it also makes mmc_send_status() being exported.


OK if you merge your stuff I can iterate my patches on top,
no problem.

>> +#if IS_ENABLED(CONFIG_MMC_BLOCK)

>

> What wrong with a regular ifdefs stubs? Why do you need IS_ENABLED()?


This is because the entire block layer can be compiled as a
module.

In that case CONFIG_MMC_BLOCK contains 'm' instead of 'y'
which confusingly does not evaluate to true in the preprocessor
(it assumes it is a misspelled 'n' I guess).

And then the autobuilders wreak havoc.

And that is why the IS_ENABLED() defines exist in the first
place IIUC.

I'm all for making CONFIG_MMC_BLOCK into a bool... but
don't know how people (Intel laptops) feel about that extra
code in their kernel at all times.

>>  void mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req);

>

> I don't get what mmc_blk_issue_rq() has to do with this change? Could

> you please explain?


If we start doing stubs we should stub everything IMO, but if you
prefer I can make that a separate patch. Seems a bit overdone
though.

> Hmm, this thing seems a bit upside-down.

>

> Currently it's possible to build the mmc block device driver as a

> module. In cases like this, accessing the card debugs node to request

> the card's status, would trigger a call to mmc_blk_card_status_get().

> How would that work when the mmc block device driver isn't loaded and

> probed?


Module symbol resolution and driver loading is always necessary,
it is no different for e.g. a wifi driver using the 802.11 library.
It is definately possible to shoot oneself in the foot, but I think
udev & friends usually load things in the right order?

> It seems like the life cycle of the card debugfs node is now being

> controlled as when the mmc block device driver has been successfully

> probed. We need to deal with that somehow.


Only for these two files but yes.

ext_csd is a bit dubious as it is only available on storage devices
(eMMC) that can not be SD, SDIO or combo cards, and we could make
it only appear if using the block layer.

The card status however we need to keep if people want it.

We *COULD* consider just thrashing these debugfs files. It is not
technically ABI and I wonder who is actually using them.

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
Christoph Hellwig May 23, 2017, 9:52 a.m. | #3
On Tue, May 23, 2017 at 11:49:48AM +0200, Linus Walleij wrote:
> In that case CONFIG_MMC_BLOCK contains 'm' instead of 'y'

> which confusingly does not evaluate to true in the preprocessor

> (it assumes it is a misspelled 'n' I guess).

> 

> And then the autobuilders wreak havoc.

> 

> And that is why the IS_ENABLED() defines exist in the first

> place IIUC.

> 

> I'm all for making CONFIG_MMC_BLOCK into a bool... but

> don't know how people (Intel laptops) feel about that extra

> code in their kernel at all times.


All the time?  At least only if any mmc/sd host driver is loaded,
right?  I don't think those additional 20k fatten the cow
(as we say in German).
--
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 May 23, 2017, 10:17 a.m. | #4
On Tue, May 23, 2017 at 11:49 AM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> On Mon, May 22, 2017 at 9:42 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:

>> On 19 May 2017 at 15:37, Linus Walleij <linus.walleij@linaro.org> wrote:

>

>>> The debugfs file "status" (in e.g. /debug/mmc3/mmc3:0001) is

>>> only available if and only if the card used is an (e)MMC or

>>> SD card, not for SDIO, as can be seen from this guard in

>>> mmc_add_card_debugfs();

>>>

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

>>>   (...create debugfs "status" entry...)

>>>

>>> Further this debugfs entry suffers from all the same starvation

>>> issues as the other userspace things, under e.g. a heavy

>>> dd operation.

>>>

>>> It is therefore logical to move this over to the block layer

>>> when it is enabled, using the new custom requests and issue

>>> it using the block request queue.

>>>

>>> This makes this debugfs card access land under the request

>>> queue host lock instead of orthogonally taking the lock.

>>>

>>> Tested during heavy dd load by cat:in the status file. We

>>> add IS_ENABLED() guards and keep the code snippet just

>>> issueing the card status as a static inline in the header

>>> so we can still have card status working when the block

>>> layer is compiled out.

>>

>> Seems like a bit of re-factoring/cleanup could help here as

>> preparation step. I just posted a patch [1] cleaning up how the mmc

>> block layer fetches the card's status.

>>

>> Perhaps that could at least simplify a bit for $subject patch,

>> especially since it also makes mmc_send_status() being exported.

>

> OK if you merge your stuff I can iterate my patches on top,

> no problem.

>

>>> +#if IS_ENABLED(CONFIG_MMC_BLOCK)

>>

>> What wrong with a regular ifdefs stubs? Why do you need IS_ENABLED()?

>

> This is because the entire block layer can be compiled as a

> module.


The block layer cannot be a module, but the MMC layer can.

> I'm all for making CONFIG_MMC_BLOCK into a bool... but

> don't know how people (Intel laptops) feel about that extra

> code in their kernel at all times.


I think CONFIG_MMC should remain tristate. However, we could
consider making CONFIG_MMC_BLOCK a 'bool' symbol and
linking it into the mmc-core module when enabled.

>> It seems like the life cycle of the card debugfs node is now being

>> controlled as when the mmc block device driver has been successfully

>> probed. We need to deal with that somehow.

>

> Only for these two files but yes.

>

> ext_csd is a bit dubious as it is only available on storage devices

> (eMMC) that can not be SD, SDIO or combo cards, and we could make

> it only appear if using the block layer.

>

> The card status however we need to keep if people want it.

>

> We *COULD* consider just thrashing these debugfs files. It is not

> technically ABI and I wonder who is actually using them.


I think the ext_csd is extremely useful on storage devices to have
exposed to user space in some form. There is a comment saying
that we don't store it in RAM at the moment as it is rarely used,
but we could change that, as it's only 512 bytes per eMMC device
and there is rarely more than one of them. IIRC, the ext_csd is
entirely readonly and doesn't change during runtime, so we
gain nothing by issuing the commands every time someone reads
the debugfs file.

       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 May 23, 2017, 11:04 a.m. | #5
[...]

>> Seems like a bit of re-factoring/cleanup could help here as

>> preparation step. I just posted a patch [1] cleaning up how the mmc

>> block layer fetches the card's status.

>>

>> Perhaps that could at least simplify a bit for $subject patch,

>> especially since it also makes mmc_send_status() being exported.

>

> OK if you merge your stuff I can iterate my patches on top,

> no problem.


Done!

>

>>> +#if IS_ENABLED(CONFIG_MMC_BLOCK)

>>

>> What wrong with a regular ifdefs stubs? Why do you need IS_ENABLED()?

>

> This is because the entire block layer can be compiled as a

> module.

>

> In that case CONFIG_MMC_BLOCK contains 'm' instead of 'y'

> which confusingly does not evaluate to true in the preprocessor

> (it assumes it is a misspelled 'n' I guess).

>

> And then the autobuilders wreak havoc.

>

> And that is why the IS_ENABLED() defines exist in the first

> place IIUC.


Thanks for explanation! It makes more sense to me now!

>

> I'm all for making CONFIG_MMC_BLOCK into a bool... but

> don't know how people (Intel laptops) feel about that extra

> code in their kernel at all times.

>

>>>  void mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req);

>>

>> I don't get what mmc_blk_issue_rq() has to do with this change? Could

>> you please explain?

>

> If we start doing stubs we should stub everything IMO, but if you

> prefer I can make that a separate patch. Seems a bit overdone

> though.

>

>> Hmm, this thing seems a bit upside-down.

>>

>> Currently it's possible to build the mmc block device driver as a

>> module. In cases like this, accessing the card debugs node to request

>> the card's status, would trigger a call to mmc_blk_card_status_get().

>> How would that work when the mmc block device driver isn't loaded and

>> probed?

>

> Module symbol resolution and driver loading is always necessary,

> it is no different for e.g. a wifi driver using the 802.11 library.

> It is definately possible to shoot oneself in the foot, but I think

> udev & friends usually load things in the right order?


My my point is that *if* the card debugfs nodes shows up to the user -
they should work (no matter if the mmc block device has been probed or
not).

Can we guarantee that, is this approach?

>

>> It seems like the life cycle of the card debugfs node is now being

>> controlled as when the mmc block device driver has been successfully

>> probed. We need to deal with that somehow.

>

> Only for these two files but yes.

>

> ext_csd is a bit dubious as it is only available on storage devices

> (eMMC) that can not be SD, SDIO or combo cards, and we could make

> it only appear if using the block layer.


Yes, that's a nice option.

>

> The card status however we need to keep if people want it.


Again, this is for debug purpose. I don't see an issue moving also
this one within CONFIG_MMC_BLOCK.

>

> We *COULD* consider just thrashing these debugfs files. It is not

> technically ABI and I wonder who is actually using them.


Right.

According to Avri, apparently some userspace tools measuring
performance. I would be interested to know a bit more about those
tools.

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
Ulf Hansson May 23, 2017, 11:22 a.m. | #6
[...]

>>>> +#if IS_ENABLED(CONFIG_MMC_BLOCK)

>>>

>>> What wrong with a regular ifdefs stubs? Why do you need IS_ENABLED()?

>>

>> This is because the entire block layer can be compiled as a

>> module.

>

> The block layer cannot be a module, but the MMC layer can.

>

>> I'm all for making CONFIG_MMC_BLOCK into a bool... but

>> don't know how people (Intel laptops) feel about that extra

>> code in their kernel at all times.

>

> I think CONFIG_MMC should remain tristate. However, we could

> consider making CONFIG_MMC_BLOCK a 'bool' symbol and

> linking it into the mmc-core module when enabled.


That is a good idea, no matter what. We should do the same thing for
CONFIG_MMC_TEST. There are really no need to have to separate modules,
one is more than enough.

However, I don't think that by itself, solves the changed life-cycle
issue of the card debugfs node.

Some more protection is needed, to make sure the mmc block device is
loaded/probed before calling mmc_blk_card_status_get().

>

>>> It seems like the life cycle of the card debugfs node is now being

>>> controlled as when the mmc block device driver has been successfully

>>> probed. We need to deal with that somehow.

>>

>> Only for these two files but yes.

>>

>> ext_csd is a bit dubious as it is only available on storage devices

>> (eMMC) that can not be SD, SDIO or combo cards, and we could make

>> it only appear if using the block layer.

>>

>> The card status however we need to keep if people want it.

>>

>> We *COULD* consider just thrashing these debugfs files. It is not

>> technically ABI and I wonder who is actually using them.

>

> I think the ext_csd is extremely useful on storage devices to have

> exposed to user space in some form. There is a comment saying

> that we don't store it in RAM at the moment as it is rarely used,

> but we could change that, as it's only 512 bytes per eMMC device

> and there is rarely more than one of them. IIRC, the ext_csd is

> entirely readonly and doesn't change during runtime, so we

> gain nothing by issuing the commands every time someone reads

> the debugfs file.


First, we have mmc utils (going via mmc ioctl), which purpose are
exactly for these cases. As a matter of fact it does a better job,
since it even present the data in a more human readable format.

Second, ext csd gets updated very often to control/change some
behavior of the device. You must be mixing this up with some of the
other card registers.

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
Arnd Bergmann May 23, 2017, 11:29 a.m. | #7
On Tue, May 23, 2017 at 1:22 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> [...]

>

>>>>> +#if IS_ENABLED(CONFIG_MMC_BLOCK)

>>>>

>>>> What wrong with a regular ifdefs stubs? Why do you need IS_ENABLED()?

>>>

>>> This is because the entire block layer can be compiled as a

>>> module.

>>

>> The block layer cannot be a module, but the MMC layer can.

>>

>>> I'm all for making CONFIG_MMC_BLOCK into a bool... but

>>> don't know how people (Intel laptops) feel about that extra

>>> code in their kernel at all times.

>>

>> I think CONFIG_MMC should remain tristate. However, we could

>> consider making CONFIG_MMC_BLOCK a 'bool' symbol and

>> linking it into the mmc-core module when enabled.

>

> That is a good idea, no matter what. We should do the same thing for

> CONFIG_MMC_TEST. There are really no need to have to separate modules,

> one is more than enough.


I think for the test module it conceptually makes more sense to
be separate, though I can see that this requires to be careful
with the debugfs file lifetime rules.

>>>> It seems like the life cycle of the card debugfs node is now being

>>>> controlled as when the mmc block device driver has been successfully

>>>> probed. We need to deal with that somehow.

>>>

>>> Only for these two files but yes.

>>>

>>> ext_csd is a bit dubious as it is only available on storage devices

>>> (eMMC) that can not be SD, SDIO or combo cards, and we could make

>>> it only appear if using the block layer.

>>>

>>> The card status however we need to keep if people want it.

>>>

>>> We *COULD* consider just thrashing these debugfs files. It is not

>>> technically ABI and I wonder who is actually using them.

>>

>> I think the ext_csd is extremely useful on storage devices to have

>> exposed to user space in some form. There is a comment saying

>> that we don't store it in RAM at the moment as it is rarely used,

>> but we could change that, as it's only 512 bytes per eMMC device

>> and there is rarely more than one of them. IIRC, the ext_csd is

>> entirely readonly and doesn't change during runtime, so we

>> gain nothing by issuing the commands every time someone reads

>> the debugfs file.

>

> First, we have mmc utils (going via mmc ioctl), which purpose are

> exactly for these cases. As a matter of fact it does a better job,

> since it even present the data in a more human readable format.

>

> Second, ext csd gets updated very often to control/change some

> behavior of the device. You must be mixing this up with some of the

> other card registers.


Ok, it's been a while since I looked at the details, thanks for the
clarification.

     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

Patch

diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 52635120a0a5..8858798d1349 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -1191,6 +1191,7 @@  static void mmc_blk_issue_drv_op(struct mmc_queue *mq, struct request *req)
 	struct mmc_queue_req *mq_rq;
 	struct mmc_card *card = mq->card;
 	struct mmc_blk_data *md = mq->blkdata;
+	u32 status;
 	int ret;
 	int i;
 
@@ -1219,6 +1220,11 @@  static void mmc_blk_issue_drv_op(struct mmc_queue *mq, struct request *req)
 			card->ext_csd.boot_ro_lock |=
 				EXT_CSD_BOOT_WP_B_PWR_WP_EN;
 		break;
+	case MMC_DRV_OP_GET_CARD_STATUS:
+		ret = mmc_send_status(card, &status);
+		if (!ret)
+			ret = status;
+		break;
 	default:
 		pr_err("%s: unknown driver specific operation\n",
 		       md->disk->disk_name);
@@ -1968,6 +1974,28 @@  void mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
 		mmc_put_card(card);
 }
 
+/* Called from debugfs for MMC/SD cards */
+int mmc_blk_card_status_get(struct mmc_card *card, u64 *val)
+{
+	struct mmc_blk_data *md = dev_get_drvdata(&card->dev);
+	struct mmc_queue *mq = &md->queue;
+	struct request *req;
+	int ret;
+
+	/* Ask the block layer about the card status */
+	req = blk_get_request(mq->queue, REQ_OP_DRV_IN, __GFP_RECLAIM);
+	req_to_mmc_queue_req(req)->drv_op = MMC_DRV_OP_GET_CARD_STATUS;
+	blk_execute_rq(mq->queue, NULL, req, 0);
+	ret = req_to_mmc_queue_req(req)->drv_op_result;
+	if (ret >= 0) {
+		*val = ret;
+		ret = 0;
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL(mmc_blk_card_status_get);
+
 static inline int mmc_blk_readonly(struct mmc_card *card)
 {
 	return mmc_card_readonly(card) ||
diff --git a/drivers/mmc/core/block.h b/drivers/mmc/core/block.h
index 860ca7c8df86..1e26755a864b 100644
--- a/drivers/mmc/core/block.h
+++ b/drivers/mmc/core/block.h
@@ -1,9 +1,46 @@ 
 #ifndef _MMC_CORE_BLOCK_H
 #define _MMC_CORE_BLOCK_H
 
+#include <linux/kconfig.h>
+#include "core.h"
+#include "mmc_ops.h"
+
 struct mmc_queue;
 struct request;
 
+#if IS_ENABLED(CONFIG_MMC_BLOCK)
+
 void mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req);
+int mmc_blk_card_status_get(struct mmc_card *card, u64 *val);
+
+#else
+
+/*
+ * Small stub functions to be used when the block layer is not
+ * enabled, e.g. for pure SDIO without MMC/SD configurations.
+ */
+
+static inline void mmc_blk_issue_rq(struct mmc_queue *mq, struct request *req)
+{
+	return;
+}
+
+static inline int mmc_blk_card_status_get(struct mmc_card *card, u64 *val)
+{
+	u32 status;
+	int ret;
+
+	mmc_get_card(card);
+
+	ret = mmc_send_status(card, &status);
+	if (!ret)
+		*val = status;
+
+	mmc_put_card(card);
+
+	return ret;
+}
+
+#endif /* IS_ENABLED(CONFIG_MMC_BLOCK) */
 
 #endif
diff --git a/drivers/mmc/core/debugfs.c b/drivers/mmc/core/debugfs.c
index a1fba5732d66..ce5b921c7d96 100644
--- a/drivers/mmc/core/debugfs.c
+++ b/drivers/mmc/core/debugfs.c
@@ -19,6 +19,7 @@ 
 #include <linux/mmc/card.h>
 #include <linux/mmc/host.h>
 
+#include "block.h"
 #include "core.h"
 #include "card.h"
 #include "host.h"
@@ -283,19 +284,7 @@  void mmc_remove_host_debugfs(struct mmc_host *host)
 
 static int mmc_dbg_card_status_get(void *data, u64 *val)
 {
-	struct mmc_card	*card = data;
-	u32		status;
-	int		ret;
-
-	mmc_get_card(card);
-
-	ret = mmc_send_status(data, &status);
-	if (!ret)
-		*val = status;
-
-	mmc_put_card(card);
-
-	return ret;
+	return mmc_blk_card_status_get(data, val);
 }
 DEFINE_SIMPLE_ATTRIBUTE(mmc_dbg_card_status_fops, mmc_dbg_card_status_get,
 		NULL, "%08llx\n");
diff --git a/drivers/mmc/core/queue.h b/drivers/mmc/core/queue.h
index 361b46408e0f..2b39717453a5 100644
--- a/drivers/mmc/core/queue.h
+++ b/drivers/mmc/core/queue.h
@@ -36,10 +36,12 @@  struct mmc_blk_request {
  * enum mmc_drv_op - enumerates the operations in the mmc_queue_req
  * @MMC_DRV_OP_IOCTL: ioctl operation
  * @MMC_DRV_OP_BOOT_WP: write protect boot partitions
+ * @MMC_DRV_OP_GET_CARD_STATUS: get card status
  */
 enum mmc_drv_op {
 	MMC_DRV_OP_IOCTL,
 	MMC_DRV_OP_BOOT_WP,
+	MMC_DRV_OP_GET_CARD_STATUS,
 };
 
 struct mmc_queue_req {