* [PATCH] mmc: core: Use GFP_NOIO in ACMD22
@ 2024-10-14 11:44 Avri Altman
2024-10-15 9:44 ` Adrian Hunter
0 siblings, 1 reply; 5+ messages in thread
From: Avri Altman @ 2024-10-14 11:44 UTC (permalink / raw)
To: Ulf Hansson, linux-mmc; +Cc: Adrian Hunter, Avri Altman, stable
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;
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] mmc: core: Use GFP_NOIO in ACMD22
2024-10-14 11:44 [PATCH] mmc: core: Use GFP_NOIO in ACMD22 Avri Altman
@ 2024-10-15 9:44 ` Adrian Hunter
2024-10-16 14:44 ` Ulf Hansson
0 siblings, 1 reply; 5+ messages in thread
From: Adrian Hunter @ 2024-10-15 9:44 UTC (permalink / raw)
To: Avri Altman, Ulf Hansson, linux-mmc; +Cc: stable
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;
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] mmc: core: Use GFP_NOIO in ACMD22
2024-10-15 9:44 ` Adrian Hunter
@ 2024-10-16 14:44 ` Ulf Hansson
2024-10-16 15:20 ` Avri Altman
0 siblings, 1 reply; 5+ messages in thread
From: Ulf Hansson @ 2024-10-16 14:44 UTC (permalink / raw)
To: Adrian Hunter; +Cc: Avri Altman, linux-mmc, stable
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
^ permalink raw reply [flat|nested] 5+ messages in thread* RE: [PATCH] mmc: core: Use GFP_NOIO in ACMD22
2024-10-16 14:44 ` Ulf Hansson
@ 2024-10-16 15:20 ` Avri Altman
2024-10-17 9:13 ` Ulf Hansson
0 siblings, 1 reply; 5+ messages in thread
From: Avri Altman @ 2024-10-16 15:20 UTC (permalink / raw)
To: Ulf Hansson, Adrian Hunter
Cc: linux-mmc@vger.kernel.org, stable@vger.kernel.org
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] mmc: core: Use GFP_NOIO in ACMD22
2024-10-16 15:20 ` Avri Altman
@ 2024-10-17 9:13 ` Ulf Hansson
0 siblings, 0 replies; 5+ messages in thread
From: Ulf Hansson @ 2024-10-17 9:13 UTC (permalink / raw)
To: Avri Altman
Cc: Adrian Hunter, linux-mmc@vger.kernel.org, stable@vger.kernel.org
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-10-17 9:13 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-14 11:44 [PATCH] mmc: core: Use GFP_NOIO in ACMD22 Avri Altman
2024-10-15 9:44 ` Adrian Hunter
2024-10-16 14:44 ` Ulf Hansson
2024-10-16 15:20 ` Avri Altman
2024-10-17 9:13 ` Ulf Hansson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox