| Message ID | 20241014114458.360538-1-avri.altman@wdc.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mmc: core: Use GFP_NOIO in ACMD22 | expand |
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); > 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 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: > 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. Sounds good to me! [...] Kind regards Uffe
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;
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(-)