* [PATCH] mmc: core: don't initialize block size from ext_csd if not present
@ 2021-01-12 21:09 Peter Collingbourne
2021-01-12 23:29 ` Damien Le Moal
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Peter Collingbourne @ 2021-01-12 21:09 UTC (permalink / raw)
To: Adrian Hunter, Christoph Hellwig, Linus Walleij, Ulf Hansson
Cc: Peter Collingbourne, linux-mmc, Damien Le Moal
If extended CSD was not available, the eMMC driver would incorrectly
set the block size to 0, as the data_sector_size field of ext_csd
was never initialized. This issue was exposed by commit 817046ecddbc
("block: Align max_hw_sectors to logical blocksize") which caused
max_sectors and max_hw_sectors to be set to 0 after setting the block
size to 0, resulting in a kernel panic in bio_split when attempting
to read from the device. Fix it by only reading the block size from
ext_csd if it is available.
Fixes: 817046ecddbc ("block: Align max_hw_sectors to logical blocksize")
Signed-off-by: Peter Collingbourne <pcc@google.com>
Link: https://linux-review.googlesource.com/id/If244d178da4d86b52034459438fec295b02d6e60
---
drivers/mmc/core/queue.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c
index de7cb0369c30..735cdbf1145c 100644
--- a/drivers/mmc/core/queue.c
+++ b/drivers/mmc/core/queue.c
@@ -20,6 +20,7 @@
#include "core.h"
#include "card.h"
#include "host.h"
+#include "mmc_ops.h"
#define MMC_DMA_MAP_MERGE_SEGMENTS 512
@@ -384,7 +385,7 @@ static void mmc_setup_queue(struct mmc_queue *mq, struct mmc_card *card)
"merging was advertised but not possible");
blk_queue_max_segments(mq->queue, mmc_get_max_segments(host));
- if (mmc_card_mmc(card))
+ if (mmc_card_mmc(card) && mmc_can_ext_csd(card))
block_size = card->ext_csd.data_sector_size;
blk_queue_logical_block_size(mq->queue, block_size);
--
2.30.0.284.gd98b1dd5eaa7-goog
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] mmc: core: don't initialize block size from ext_csd if not present
2021-01-12 21:09 [PATCH] mmc: core: don't initialize block size from ext_csd if not present Peter Collingbourne
@ 2021-01-12 23:29 ` Damien Le Moal
2021-01-13 9:16 ` kernel test robot
2021-01-13 10:43 ` Adrian Hunter
2 siblings, 0 replies; 8+ messages in thread
From: Damien Le Moal @ 2021-01-12 23:29 UTC (permalink / raw)
To: Peter Collingbourne, Adrian Hunter, Christoph Hellwig,
Linus Walleij, Ulf Hansson
Cc: linux-mmc
On 2021/01/13 6:09, Peter Collingbourne wrote:
> If extended CSD was not available, the eMMC driver would incorrectly
> set the block size to 0, as the data_sector_size field of ext_csd
> was never initialized. This issue was exposed by commit 817046ecddbc
> ("block: Align max_hw_sectors to logical blocksize") which caused
> max_sectors and max_hw_sectors to be set to 0 after setting the block
> size to 0, resulting in a kernel panic in bio_split when attempting
> to read from the device. Fix it by only reading the block size from
> ext_csd if it is available.
>
> Fixes: 817046ecddbc ("block: Align max_hw_sectors to logical blocksize")
> Signed-off-by: Peter Collingbourne <pcc@google.com>
> Link: https://linux-review.googlesource.com/id/If244d178da4d86b52034459438fec295b02d6e60
> ---
> drivers/mmc/core/queue.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c
> index de7cb0369c30..735cdbf1145c 100644
> --- a/drivers/mmc/core/queue.c
> +++ b/drivers/mmc/core/queue.c
> @@ -20,6 +20,7 @@
> #include "core.h"
> #include "card.h"
> #include "host.h"
> +#include "mmc_ops.h"
>
> #define MMC_DMA_MAP_MERGE_SEGMENTS 512
>
> @@ -384,7 +385,7 @@ static void mmc_setup_queue(struct mmc_queue *mq, struct mmc_card *card)
> "merging was advertised but not possible");
> blk_queue_max_segments(mq->queue, mmc_get_max_segments(host));
>
> - if (mmc_card_mmc(card))
> + if (mmc_card_mmc(card) && mmc_can_ext_csd(card))
> block_size = card->ext_csd.data_sector_size;
>
> blk_queue_logical_block_size(mq->queue, block_size);
>
This looks good to me. I wonder though if we should add a WARN() in
blk_queue_logical_block_size() if it is passed a 0 block size to more easily
catch this kind of bug.
Reviewed-by: Damien Le Moal <damien.lemoal@wdc.com>
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] mmc: core: don't initialize block size from ext_csd if not present
2021-01-12 21:09 [PATCH] mmc: core: don't initialize block size from ext_csd if not present Peter Collingbourne
@ 2021-01-13 9:16 ` kernel test robot
2021-01-13 9:16 ` kernel test robot
2021-01-13 10:43 ` Adrian Hunter
2 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2021-01-13 9:16 UTC (permalink / raw)
To: Peter Collingbourne, Adrian Hunter, Christoph Hellwig,
Linus Walleij, Ulf Hansson
Cc: kbuild-all, Peter Collingbourne, linux-mmc, Damien Le Moal
[-- Attachment #1: Type: text/plain, Size: 1563 bytes --]
Hi Peter,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on hch-configfs/for-next v5.11-rc3 next-20210113]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Peter-Collingbourne/mmc-core-don-t-initialize-block-size-from-ext_csd-if-not-present/20210113-054414
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git e609571b5ffa3528bf85292de1ceaddac342bc1c
config: x86_64-rhel (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
# https://github.com/0day-ci/linux/commit/562a77dc6c88601c5129c0bbb9141119494320c7
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Peter-Collingbourne/mmc-core-don-t-initialize-block-size-from-ext_csd-if-not-present/20210113-054414
git checkout 562a77dc6c88601c5129c0bbb9141119494320c7
# save the attached .config to linux build tree
make W=1 ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>, old ones prefixed by <<):
>> ERROR: modpost: "mmc_can_ext_csd" [drivers/mmc/core/mmc_block.ko] undefined!
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 45904 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] mmc: core: don't initialize block size from ext_csd if not present
@ 2021-01-13 9:16 ` kernel test robot
0 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2021-01-13 9:16 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 1597 bytes --]
Hi Peter,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on hch-configfs/for-next v5.11-rc3 next-20210113]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Peter-Collingbourne/mmc-core-don-t-initialize-block-size-from-ext_csd-if-not-present/20210113-054414
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git e609571b5ffa3528bf85292de1ceaddac342bc1c
config: x86_64-rhel (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
# https://github.com/0day-ci/linux/commit/562a77dc6c88601c5129c0bbb9141119494320c7
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Peter-Collingbourne/mmc-core-don-t-initialize-block-size-from-ext_csd-if-not-present/20210113-054414
git checkout 562a77dc6c88601c5129c0bbb9141119494320c7
# save the attached .config to linux build tree
make W=1 ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>, old ones prefixed by <<):
>> ERROR: modpost: "mmc_can_ext_csd" [drivers/mmc/core/mmc_block.ko] undefined!
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 45904 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] mmc: core: don't initialize block size from ext_csd if not present
2021-01-12 21:09 [PATCH] mmc: core: don't initialize block size from ext_csd if not present Peter Collingbourne
2021-01-12 23:29 ` Damien Le Moal
2021-01-13 9:16 ` kernel test robot
@ 2021-01-13 10:43 ` Adrian Hunter
2021-01-13 19:46 ` Peter Collingbourne
2 siblings, 1 reply; 8+ messages in thread
From: Adrian Hunter @ 2021-01-13 10:43 UTC (permalink / raw)
To: Peter Collingbourne, Christoph Hellwig, Linus Walleij, Ulf Hansson
Cc: linux-mmc, Damien Le Moal
On 12/01/21 11:09 pm, Peter Collingbourne wrote:
> If extended CSD was not available, the eMMC driver would incorrectly
> set the block size to 0, as the data_sector_size field of ext_csd
> was never initialized. This issue was exposed by commit 817046ecddbc
> ("block: Align max_hw_sectors to logical blocksize") which caused
> max_sectors and max_hw_sectors to be set to 0 after setting the block
> size to 0, resulting in a kernel panic in bio_split when attempting
> to read from the device. Fix it by only reading the block size from
> ext_csd if it is available.
>
> Fixes: 817046ecddbc ("block: Align max_hw_sectors to logical blocksize")
I would go with the original commit i.e.
Fixes: a5075eb94837 ("mmc: block: Allow disabling 512B sector size emulation")
> Signed-off-by: Peter Collingbourne <pcc@google.com>
> Link: https://linux-review.googlesource.com/id/If244d178da4d86b52034459438fec295b02d6e60
> ---
> drivers/mmc/core/queue.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c
> index de7cb0369c30..735cdbf1145c 100644
> --- a/drivers/mmc/core/queue.c
> +++ b/drivers/mmc/core/queue.c
> @@ -20,6 +20,7 @@
> #include "core.h"
> #include "card.h"
> #include "host.h"
> +#include "mmc_ops.h"
>
> #define MMC_DMA_MAP_MERGE_SEGMENTS 512
>
> @@ -384,7 +385,7 @@ static void mmc_setup_queue(struct mmc_queue *mq, struct mmc_card *card)
> "merging was advertised but not possible");
> blk_queue_max_segments(mq->queue, mmc_get_max_segments(host));
>
> - if (mmc_card_mmc(card))
> + if (mmc_card_mmc(card) && mmc_can_ext_csd(card))
> block_size = card->ext_csd.data_sector_size;
Might as well be:
if (mmc_card_mmc(card) && card->ext_csd.data_sector_size)
block_size = card->ext_csd.data_sector_size;
>
> blk_queue_logical_block_size(mq->queue, block_size);
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] mmc: core: don't initialize block size from ext_csd if not present
2021-01-13 10:43 ` Adrian Hunter
@ 2021-01-13 19:46 ` Peter Collingbourne
2021-01-14 6:10 ` Adrian Hunter
0 siblings, 1 reply; 8+ messages in thread
From: Peter Collingbourne @ 2021-01-13 19:46 UTC (permalink / raw)
To: Adrian Hunter
Cc: Christoph Hellwig, Linus Walleij, Ulf Hansson, linux-mmc, Damien Le Moal
On Wed, Jan 13, 2021 at 2:43 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 12/01/21 11:09 pm, Peter Collingbourne wrote:
> > If extended CSD was not available, the eMMC driver would incorrectly
> > set the block size to 0, as the data_sector_size field of ext_csd
> > was never initialized. This issue was exposed by commit 817046ecddbc
> > ("block: Align max_hw_sectors to logical blocksize") which caused
> > max_sectors and max_hw_sectors to be set to 0 after setting the block
> > size to 0, resulting in a kernel panic in bio_split when attempting
> > to read from the device. Fix it by only reading the block size from
> > ext_csd if it is available.
> >
> > Fixes: 817046ecddbc ("block: Align max_hw_sectors to logical blocksize")
>
> I would go with the original commit i.e.
>
> Fixes: a5075eb94837 ("mmc: block: Allow disabling 512B sector size emulation")
Sure, makes sense.
> > Signed-off-by: Peter Collingbourne <pcc@google.com>
> > Link: https://linux-review.googlesource.com/id/If244d178da4d86b52034459438fec295b02d6e60
> > ---
> > drivers/mmc/core/queue.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c
> > index de7cb0369c30..735cdbf1145c 100644
> > --- a/drivers/mmc/core/queue.c
> > +++ b/drivers/mmc/core/queue.c
> > @@ -20,6 +20,7 @@
> > #include "core.h"
> > #include "card.h"
> > #include "host.h"
> > +#include "mmc_ops.h"
> >
> > #define MMC_DMA_MAP_MERGE_SEGMENTS 512
> >
> > @@ -384,7 +385,7 @@ static void mmc_setup_queue(struct mmc_queue *mq, struct mmc_card *card)
> > "merging was advertised but not possible");
> > blk_queue_max_segments(mq->queue, mmc_get_max_segments(host));
> >
> > - if (mmc_card_mmc(card))
> > + if (mmc_card_mmc(card) && mmc_can_ext_csd(card))
> > block_size = card->ext_csd.data_sector_size;
>
> Might as well be:
>
> if (mmc_card_mmc(card) && card->ext_csd.data_sector_size)
> block_size = card->ext_csd.data_sector_size;
Can we rely on this data structure to be zero initialized? I suppose
so, provided that it was allocated with mmc_alloc_card which uses
kzalloc. But it isn't entirely obvious and I figure it may be a little
better to be explicit in our intent here. But either way works for me.
Peter
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] mmc: core: don't initialize block size from ext_csd if not present
2021-01-13 19:46 ` Peter Collingbourne
@ 2021-01-14 6:10 ` Adrian Hunter
2021-01-14 20:14 ` Peter Collingbourne
0 siblings, 1 reply; 8+ messages in thread
From: Adrian Hunter @ 2021-01-14 6:10 UTC (permalink / raw)
To: Peter Collingbourne
Cc: Christoph Hellwig, Linus Walleij, Ulf Hansson, linux-mmc, Damien Le Moal
On 13/01/21 9:46 pm, Peter Collingbourne wrote:
> On Wed, Jan 13, 2021 at 2:43 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>
>> On 12/01/21 11:09 pm, Peter Collingbourne wrote:
>>> If extended CSD was not available, the eMMC driver would incorrectly
>>> set the block size to 0, as the data_sector_size field of ext_csd
>>> was never initialized. This issue was exposed by commit 817046ecddbc
>>> ("block: Align max_hw_sectors to logical blocksize") which caused
>>> max_sectors and max_hw_sectors to be set to 0 after setting the block
>>> size to 0, resulting in a kernel panic in bio_split when attempting
>>> to read from the device. Fix it by only reading the block size from
>>> ext_csd if it is available.
>>>
>>> Fixes: 817046ecddbc ("block: Align max_hw_sectors to logical blocksize")
>>
>> I would go with the original commit i.e.
>>
>> Fixes: a5075eb94837 ("mmc: block: Allow disabling 512B sector size emulation")
>
> Sure, makes sense.
>
>>> Signed-off-by: Peter Collingbourne <pcc@google.com>
>>> Link: https://linux-review.googlesource.com/id/If244d178da4d86b52034459438fec295b02d6e60
>>> ---
>>> drivers/mmc/core/queue.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c
>>> index de7cb0369c30..735cdbf1145c 100644
>>> --- a/drivers/mmc/core/queue.c
>>> +++ b/drivers/mmc/core/queue.c
>>> @@ -20,6 +20,7 @@
>>> #include "core.h"
>>> #include "card.h"
>>> #include "host.h"
>>> +#include "mmc_ops.h"
>>>
>>> #define MMC_DMA_MAP_MERGE_SEGMENTS 512
>>>
>>> @@ -384,7 +385,7 @@ static void mmc_setup_queue(struct mmc_queue *mq, struct mmc_card *card)
>>> "merging was advertised but not possible");
>>> blk_queue_max_segments(mq->queue, mmc_get_max_segments(host));
>>>
>>> - if (mmc_card_mmc(card))
>>> + if (mmc_card_mmc(card) && mmc_can_ext_csd(card))
>>> block_size = card->ext_csd.data_sector_size;
>>
>> Might as well be:
>>
>> if (mmc_card_mmc(card) && card->ext_csd.data_sector_size)
>> block_size = card->ext_csd.data_sector_size;
>
> Can we rely on this data structure to be zero initialized? I suppose
> so, provided that it was allocated with mmc_alloc_card which uses
> kzalloc. But it isn't entirely obvious and I figure it may be a little
> better to be explicit in our intent here. But either way works for me.
The only valid values are 512 and 4096, so you could add WARN_ON(block_size
!= 512 && block_size != 4096) if you want.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] mmc: core: don't initialize block size from ext_csd if not present
2021-01-14 6:10 ` Adrian Hunter
@ 2021-01-14 20:14 ` Peter Collingbourne
0 siblings, 0 replies; 8+ messages in thread
From: Peter Collingbourne @ 2021-01-14 20:14 UTC (permalink / raw)
To: Adrian Hunter
Cc: Christoph Hellwig, Linus Walleij, Ulf Hansson, linux-mmc, Damien Le Moal
On Wed, Jan 13, 2021 at 10:11 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 13/01/21 9:46 pm, Peter Collingbourne wrote:
> > On Wed, Jan 13, 2021 at 2:43 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>
> >> On 12/01/21 11:09 pm, Peter Collingbourne wrote:
> >>> If extended CSD was not available, the eMMC driver would incorrectly
> >>> set the block size to 0, as the data_sector_size field of ext_csd
> >>> was never initialized. This issue was exposed by commit 817046ecddbc
> >>> ("block: Align max_hw_sectors to logical blocksize") which caused
> >>> max_sectors and max_hw_sectors to be set to 0 after setting the block
> >>> size to 0, resulting in a kernel panic in bio_split when attempting
> >>> to read from the device. Fix it by only reading the block size from
> >>> ext_csd if it is available.
> >>>
> >>> Fixes: 817046ecddbc ("block: Align max_hw_sectors to logical blocksize")
> >>
> >> I would go with the original commit i.e.
> >>
> >> Fixes: a5075eb94837 ("mmc: block: Allow disabling 512B sector size emulation")
> >
> > Sure, makes sense.
> >
> >>> Signed-off-by: Peter Collingbourne <pcc@google.com>
> >>> Link: https://linux-review.googlesource.com/id/If244d178da4d86b52034459438fec295b02d6e60
> >>> ---
> >>> drivers/mmc/core/queue.c | 3 ++-
> >>> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/mmc/core/queue.c b/drivers/mmc/core/queue.c
> >>> index de7cb0369c30..735cdbf1145c 100644
> >>> --- a/drivers/mmc/core/queue.c
> >>> +++ b/drivers/mmc/core/queue.c
> >>> @@ -20,6 +20,7 @@
> >>> #include "core.h"
> >>> #include "card.h"
> >>> #include "host.h"
> >>> +#include "mmc_ops.h"
> >>>
> >>> #define MMC_DMA_MAP_MERGE_SEGMENTS 512
> >>>
> >>> @@ -384,7 +385,7 @@ static void mmc_setup_queue(struct mmc_queue *mq, struct mmc_card *card)
> >>> "merging was advertised but not possible");
> >>> blk_queue_max_segments(mq->queue, mmc_get_max_segments(host));
> >>>
> >>> - if (mmc_card_mmc(card))
> >>> + if (mmc_card_mmc(card) && mmc_can_ext_csd(card))
> >>> block_size = card->ext_csd.data_sector_size;
> >>
> >> Might as well be:
> >>
> >> if (mmc_card_mmc(card) && card->ext_csd.data_sector_size)
> >> block_size = card->ext_csd.data_sector_size;
> >
> > Can we rely on this data structure to be zero initialized? I suppose
> > so, provided that it was allocated with mmc_alloc_card which uses
> > kzalloc. But it isn't entirely obvious and I figure it may be a little
> > better to be explicit in our intent here. But either way works for me.
>
> The only valid values are 512 and 4096, so you could add WARN_ON(block_size
> != 512 && block_size != 4096) if you want.
Okay, I did that in v2.
Peter
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-01-14 20:15 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-12 21:09 [PATCH] mmc: core: don't initialize block size from ext_csd if not present Peter Collingbourne
2021-01-12 23:29 ` Damien Le Moal
2021-01-13 9:16 ` kernel test robot
2021-01-13 9:16 ` kernel test robot
2021-01-13 10:43 ` Adrian Hunter
2021-01-13 19:46 ` Peter Collingbourne
2021-01-14 6:10 ` Adrian Hunter
2021-01-14 20:14 ` Peter Collingbourne
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.