| Message ID | 20181004160517.12491-1-peron.clem@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: block: avoid multiblock reads for the last sector in SPI mode | expand |
On Thu, 4 Oct 2018 at 18:05, Clément Péron <peron.clem@gmail.com> wrote: > > From: Chris Boot <bootc@bootc.net> > > I was trying to get a Micro-SD card working over SPI today when I ran > into an issue which made the card unusable. The card was probed > correctly, and the msdos partition table was read, but then some command > was sent that made the card go into a bad state and stay that way. Only > a re-probe would get the card unstuck and only until just after the > partition tables were read. > > After some digging I ran into a post [1] on the spi-devel-general mailing > list where someone had exactly the same issue as me, but back in 2007. > There was a patch attached which, after some cleanup, fixes the issue > completely for me. > > I have tried this with 3 different Micro-SD cards, all of which suffered > from the problem before the patch and none of which fail after the > patch. > > The error shows up as lots of: > mmcblk0: error -38 sending status comand, retrying > mmcblk0: error -38 sending status comand, retrying > mmcblk0: error -38 sending status comand, aborting > > 1 : http://permalink.gmane.org/gmane.linux.kernel.spi.devel/516 > > Signed-off-by: Chris Boot <bootc@bootc.net> > Signed-off-by: Clément Péron <peron.clem@gmail.com> Hi, I got issue with some SD Cards over SPI. After some debugging I understand that there is an issue with reading latest sector in multiblock mode. Googling and found this patch from 2007 https://lore.kernel.org/patchwork/patch/305060/ I test it on my Kernel 4.9 and was fixing my issue ! If someone could have a look at it, Thanks, Clément > --- > drivers/mmc/core/block.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c > index a0b9102c4c6e..7bf94ed96506 100644 > --- a/drivers/mmc/core/block.c > +++ b/drivers/mmc/core/block.c > @@ -1370,6 +1370,16 @@ static void mmc_blk_data_prep(struct mmc_queue *mq, struct mmc_queue_req *mqrq, > brq->data.blocks = card->host->max_blk_count; > > if (brq->data.blocks > 1) { > + /* > + * Some SD cards in SPI mode return a CRC error or even lock up > + * completely when trying to read the last block using a > + * multiblock read command. > + */ > + if (mmc_host_is_spi(card->host) && (rq_data_dir(req) == READ) && > + (blk_rq_pos(req) + blk_rq_sectors(req) == > + get_capacity(md->disk))) > + brq->data.blocks--; > + > /* > * After a read error, we redo the request one sector > * at a time in order to accurately determine which > -- > 2.17.1 >
On 4 October 2018 at 18:08, Clément Péron <peron.clem@gmail.com> wrote: > On Thu, 4 Oct 2018 at 18:05, Clément Péron <peron.clem@gmail.com> wrote: >> >> From: Chris Boot <bootc@bootc.net> >> >> I was trying to get a Micro-SD card working over SPI today when I ran >> into an issue which made the card unusable. The card was probed >> correctly, and the msdos partition table was read, but then some command >> was sent that made the card go into a bad state and stay that way. Only >> a re-probe would get the card unstuck and only until just after the >> partition tables were read. >> >> After some digging I ran into a post [1] on the spi-devel-general mailing >> list where someone had exactly the same issue as me, but back in 2007. >> There was a patch attached which, after some cleanup, fixes the issue >> completely for me. >> >> I have tried this with 3 different Micro-SD cards, all of which suffered >> from the problem before the patch and none of which fail after the >> patch. >> >> The error shows up as lots of: >> mmcblk0: error -38 sending status comand, retrying >> mmcblk0: error -38 sending status comand, retrying >> mmcblk0: error -38 sending status comand, aborting >> >> 1 : http://permalink.gmane.org/gmane.linux.kernel.spi.devel/516 >> >> Signed-off-by: Chris Boot <bootc@bootc.net> >> Signed-off-by: Clément Péron <peron.clem@gmail.com> > > Hi, > > I got issue with some SD Cards over SPI. > After some debugging I understand that there is an issue with reading > latest sector in multiblock mode. > Googling and found this patch from 2007 > https://lore.kernel.org/patchwork/patch/305060/ > I test it on my Kernel 4.9 and was fixing my issue ! > > If someone could have a look at it, We are lacking a driver maintainer for "drivers/mmc/host/mmc_spi.c" and unfortunate me personally, also has limited knowledge around the SPI case. However, from the problem description and by looking at the mmc spi driver code (which certainly would need a little love), I get the feeling that this isn't really a card specific problem. The point is, instead of adding a hack in the mmc core, for something that probably should be fixed in the host driver, doesn't really make sense in the long run. Of course I might be wrong, but in such case we should use a so called mmc/sd card quirk instead. Another option to workaround the problem, is to implement the ->multi_io_quirk host ops for mmc_spi. In that way you can for multi block reads, return 1 as the required block size - thus telling the mmc core to use single block read instead. Of course that would then effect all multi block reads for mmc spi, then likely degrade performance/throughput. What do you think? [...] Kind regards Uffe
Hi Ulf, > > We are lacking a driver maintainer for "drivers/mmc/host/mmc_spi.c" > and unfortunate me personally, also has limited knowledge around the > SPI case. > > However, from the problem description and by looking at the mmc spi > driver code (which certainly would need a little love), I get the > feeling that this isn't really a card specific problem. The point is, > instead of adding a hack in the mmc core, for something that probably > should be fixed in the host driver, doesn't really make sense in the > long run. Of course I might be wrong, but in such case we should use a > so called mmc/sd card quirk instead. > > Another option to workaround the problem, is to implement the > ->multi_io_quirk host ops for mmc_spi. In that way you can for multi > block reads, return 1 as the required block size - thus telling the > mmc core to use single block read instead. Of course that would then > effect all multi block reads for mmc spi, then likely degrade > performance/throughput. > > What do you think? In one way I agree, this fix should be done in the host or use a quirk, multi_io_quirk seems to be here for that kind of issue. But i'm really not a fan of completely disable all the MULTI_READ cmd, the impact of the performance won't be negligible. If the multi_io_quirk prototype can be modified to know if the last block will be read, fine for me. In another way there is already lots of trick done in the core block for SD Card over SPI look how many time we call "mmc_host_is_spi". And having this here instead of hacking the mmc_spi is surely more proper. Thanks, Clement > > [...] > > Kind regards > Uffe
On 6 October 2018 at 12:00, Clément Péron <peron.clem@gmail.com> wrote: > Hi Ulf, > >> >> We are lacking a driver maintainer for "drivers/mmc/host/mmc_spi.c" >> and unfortunate me personally, also has limited knowledge around the >> SPI case. >> >> However, from the problem description and by looking at the mmc spi >> driver code (which certainly would need a little love), I get the >> feeling that this isn't really a card specific problem. The point is, >> instead of adding a hack in the mmc core, for something that probably >> should be fixed in the host driver, doesn't really make sense in the >> long run. Of course I might be wrong, but in such case we should use a >> so called mmc/sd card quirk instead. >> >> Another option to workaround the problem, is to implement the >> ->multi_io_quirk host ops for mmc_spi. In that way you can for multi >> block reads, return 1 as the required block size - thus telling the >> mmc core to use single block read instead. Of course that would then >> effect all multi block reads for mmc spi, then likely degrade >> performance/throughput. >> >> What do you think? > > In one way I agree, this fix should be done in the host or use a > quirk, multi_io_quirk seems to be here for that kind of issue. > But i'm really not a fan of completely disable all the MULTI_READ cmd, > the impact of the performance won't be negligible. I buy that, I don't like it either. > If the multi_io_quirk prototype can be modified to know if the last > block will be read, fine for me. > In another way there is already lots of trick done in the core block > for SD Card over SPI look how many time we call "mmc_host_is_spi". > And having this here instead of hacking the mmc_spi is surely more proper. Fair enough, another check for "mmc_host_is_spi" wont make any difference. However, would you mind re-writing the changelog? The current version doesn't really state what the problem is and nor what the change actually does. Moreover, the old gmane link doesn't work, so let's just drop it. Kind regards Uffe
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index a0b9102c4c6e..7bf94ed96506 100644 --- a/drivers/mmc/core/block.c +++ b/drivers/mmc/core/block.c @@ -1370,6 +1370,16 @@ static void mmc_blk_data_prep(struct mmc_queue *mq, struct mmc_queue_req *mqrq, brq->data.blocks = card->host->max_blk_count; if (brq->data.blocks > 1) { + /* + * Some SD cards in SPI mode return a CRC error or even lock up + * completely when trying to read the last block using a + * multiblock read command. + */ + if (mmc_host_is_spi(card->host) && (rq_data_dir(req) == READ) && + (blk_rq_pos(req) + blk_rq_sectors(req) == + get_capacity(md->disk))) + brq->data.blocks--; + /* * After a read error, we redo the request one sector * at a time in order to accurately determine which