While reviewing the SDUC series, Adrian made a comment concerning the memory allocation code in mmc_sd_num_wr_blocks() - see [1]. Prevent memory allocations from triggering I/O operations while ACMD22 is in progress.
[1] https://www.spinics.net/lists/linux-mmc/msg82199.html
Suggested-by: Adrian Hunter adrian.hunter@intel.com Signed-off-by: Avri Altman avri.altman@wdc.com Cc: stable@vger.kernel.org --- drivers/mmc/core/block.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index 04f3165cf9ae..042b0147d47e 100644 --- a/drivers/mmc/core/block.c +++ b/drivers/mmc/core/block.c @@ -995,6 +995,8 @@ static int mmc_sd_num_wr_blocks(struct mmc_card *card, u32 *written_blocks) u32 result; __be32 *blocks; u8 resp_sz = mmc_card_ult_capacity(card) ? 8 : 4; + unsigned int noio_flag; + struct mmc_request mrq = {}; struct mmc_command cmd = {}; struct mmc_data data = {}; @@ -1018,9 +1020,13 @@ static int mmc_sd_num_wr_blocks(struct mmc_card *card, u32 *written_blocks) mrq.cmd = &cmd; mrq.data = &data;
+ noio_flag = memalloc_noio_save(); + blocks = kmalloc(resp_sz, GFP_KERNEL); - if (!blocks) + if (!blocks) { + memalloc_noio_restore(noio_flag); return -ENOMEM; + }
sg_init_one(&sg, blocks, resp_sz);
@@ -1041,6 +1047,8 @@ static int mmc_sd_num_wr_blocks(struct mmc_card *card, u32 *written_blocks) } kfree(blocks);
+ memalloc_noio_restore(noio_flag); + if (cmd.error || data.error) return -EIO;
On 14/10/24 14:44, Avri Altman wrote:
While reviewing the SDUC series, Adrian made a comment concerning the memory allocation code in mmc_sd_num_wr_blocks() - see [1]. Prevent memory allocations from triggering I/O operations while ACMD22 is in progress.
[1] https://www.spinics.net/lists/linux-mmc/msg82199.html
Suggested-by: Adrian Hunter adrian.hunter@intel.com Signed-off-by: Avri Altman avri.altman@wdc.com Cc: stable@vger.kernel.org
drivers/mmc/core/block.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index 04f3165cf9ae..042b0147d47e 100644 --- a/drivers/mmc/core/block.c +++ b/drivers/mmc/core/block.c @@ -995,6 +995,8 @@ static int mmc_sd_num_wr_blocks(struct mmc_card *card, u32 *written_blocks) u32 result; __be32 *blocks; u8 resp_sz = mmc_card_ult_capacity(card) ? 8 : 4;
- unsigned int noio_flag;
- struct mmc_request mrq = {}; struct mmc_command cmd = {}; struct mmc_data data = {};
@@ -1018,9 +1020,13 @@ static int mmc_sd_num_wr_blocks(struct mmc_card *card, u32 *written_blocks) mrq.cmd = &cmd; mrq.data = &data;
- noio_flag = memalloc_noio_save();
- blocks = kmalloc(resp_sz, GFP_KERNEL);
Could have memalloc_noio_restore() here:
memalloc_noio_restore(noio_flag);
but I feel maybe adding something like:
u64 __aligned(8) tiny_io_buf;
to either struct mmc_card or struct mmc_host is better? Ulf, any thoughts?
- if (!blocks)
- if (!blocks) {
memalloc_noio_restore(noio_flag);- }
sg_init_one(&sg, blocks, resp_sz); @@ -1041,6 +1047,8 @@ static int mmc_sd_num_wr_blocks(struct mmc_card *card, u32 *written_blocks) } kfree(blocks);
- memalloc_noio_restore(noio_flag);
- if (cmd.error || data.error) return -EIO;
On Tue, 15 Oct 2024 at 11:44, Adrian Hunter adrian.hunter@intel.com wrote:
On 14/10/24 14:44, Avri Altman wrote:
While reviewing the SDUC series, Adrian made a comment concerning the memory allocation code in mmc_sd_num_wr_blocks() - see [1]. Prevent memory allocations from triggering I/O operations while ACMD22 is in progress.
[1] https://www.spinics.net/lists/linux-mmc/msg82199.html
Suggested-by: Adrian Hunter adrian.hunter@intel.com Signed-off-by: Avri Altman avri.altman@wdc.com Cc: stable@vger.kernel.org
drivers/mmc/core/block.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index 04f3165cf9ae..042b0147d47e 100644 --- a/drivers/mmc/core/block.c +++ b/drivers/mmc/core/block.c @@ -995,6 +995,8 @@ static int mmc_sd_num_wr_blocks(struct mmc_card *card, u32 *written_blocks) u32 result; __be32 *blocks; u8 resp_sz = mmc_card_ult_capacity(card) ? 8 : 4;
unsigned int noio_flag;struct mmc_request mrq = {}; struct mmc_command cmd = {}; struct mmc_data data = {};@@ -1018,9 +1020,13 @@ static int mmc_sd_num_wr_blocks(struct mmc_card *card, u32 *written_blocks) mrq.cmd = &cmd; mrq.data = &data;
noio_flag = memalloc_noio_save();blocks = kmalloc(resp_sz, GFP_KERNEL);Could have memalloc_noio_restore() here:
memalloc_noio_restore(noio_flag);but I feel maybe adding something like:
u64 __aligned(8) tiny_io_buf;to either struct mmc_card or struct mmc_host is better? Ulf, any thoughts?
I have no strong opinion.
A third option could be to allocate the buffer dynamically in the struct mmc_card when probing the mmc block device driver, based on mmc_card_sd() returning true.
if (mmc_card_sd()) card->io_buf = devm_kmalloc(&card->dev, 4, GFP_KERNEL);
Kind regards Uffe
On Tue, 15 Oct 2024 at 11:44, Adrian Hunter adrian.hunter@intel.com wrote:
On 14/10/24 14:44, Avri Altman wrote:
While reviewing the SDUC series, Adrian made a comment concerning the memory allocation code in mmc_sd_num_wr_blocks() - see [1]. Prevent memory allocations from triggering I/O operations while ACMD22 is in progress.
[1] https://www.spinics.net/lists/linux-mmc/msg82199.html
Suggested-by: Adrian Hunter adrian.hunter@intel.com Signed-off-by: Avri Altman avri.altman@wdc.com Cc: stable@vger.kernel.org
drivers/mmc/core/block.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index 04f3165cf9ae..042b0147d47e 100644 --- a/drivers/mmc/core/block.c +++ b/drivers/mmc/core/block.c @@ -995,6 +995,8 @@ static int mmc_sd_num_wr_blocks(struct
mmc_card *card, u32 *written_blocks)
u32 result; __be32 *blocks; u8 resp_sz = mmc_card_ult_capacity(card) ? 8 : 4;
unsigned int noio_flag;struct mmc_request mrq = {}; struct mmc_command cmd = {}; struct mmc_data data = {};@@ -1018,9 +1020,13 @@ static int mmc_sd_num_wr_blocks(struct
mmc_card *card, u32 *written_blocks)
mrq.cmd = &cmd; mrq.data = &data;
noio_flag = memalloc_noio_save();blocks = kmalloc(resp_sz, GFP_KERNEL);Could have memalloc_noio_restore() here:
memalloc_noio_restore(noio_flag);but I feel maybe adding something like:
u64 __aligned(8) tiny_io_buf;to either struct mmc_card or struct mmc_host is better? Ulf, any thoughts?
I have no strong opinion.
Then I would vote to stay with Adrian's original NOIO suggestion, because: 1) My testing shows that mmc_sd_num_wr_blocks() is hardly being hit, and 2) that allocation is within the write timeout anyway
So unless you want it otherwise, will remove the redundant macro call and re-spin.
Thanks, Avri
A third option could be to allocate the buffer dynamically in the struct mmc_card when probing the mmc block device driver, based on mmc_card_sd() returning true.
if (mmc_card_sd()) card->io_buf = devm_kmalloc(&card->dev, 4, GFP_KERNEL);
Kind regards Uffe
On Wed, 16 Oct 2024 at 17:21, Avri Altman Avri.Altman@wdc.com wrote:
On Tue, 15 Oct 2024 at 11:44, Adrian Hunter adrian.hunter@intel.com wrote:
On 14/10/24 14:44, Avri Altman wrote:
While reviewing the SDUC series, Adrian made a comment concerning the memory allocation code in mmc_sd_num_wr_blocks() - see [1]. Prevent memory allocations from triggering I/O operations while ACMD22 is in progress.
[1] https://www.spinics.net/lists/linux-mmc/msg82199.html
Suggested-by: Adrian Hunter adrian.hunter@intel.com Signed-off-by: Avri Altman avri.altman@wdc.com Cc: stable@vger.kernel.org
drivers/mmc/core/block.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index 04f3165cf9ae..042b0147d47e 100644 --- a/drivers/mmc/core/block.c +++ b/drivers/mmc/core/block.c @@ -995,6 +995,8 @@ static int mmc_sd_num_wr_blocks(struct
mmc_card *card, u32 *written_blocks)
u32 result; __be32 *blocks; u8 resp_sz = mmc_card_ult_capacity(card) ? 8 : 4;
unsigned int noio_flag;struct mmc_request mrq = {}; struct mmc_command cmd = {}; struct mmc_data data = {};@@ -1018,9 +1020,13 @@ static int mmc_sd_num_wr_blocks(struct
mmc_card *card, u32 *written_blocks)
mrq.cmd = &cmd; mrq.data = &data;
noio_flag = memalloc_noio_save();blocks = kmalloc(resp_sz, GFP_KERNEL);Could have memalloc_noio_restore() here:
memalloc_noio_restore(noio_flag);but I feel maybe adding something like:
u64 __aligned(8) tiny_io_buf;to either struct mmc_card or struct mmc_host is better? Ulf, any thoughts?
I have no strong opinion.
Then I would vote to stay with Adrian's original NOIO suggestion, because:
- My testing shows that mmc_sd_num_wr_blocks() is hardly being hit, and
- that allocation is within the write timeout anyway
So unless you want it otherwise, will remove the redundant macro call and re-spin.
Sounds good to me!
[...]
Kind regards Uffe
linux-stable-mirror@lists.linaro.org
-
Adrian Hunter -
Avri Altman -
Avri Altman
-
Ulf Hansson