lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 11 Jul 2018 10:57:11 +0200 From: Ludovic BARRE <ludovic.barre@...com> To: Ulf Hansson <ulf.hansson@...aro.org> CC: Rob Herring <robh+dt@...nel.org>, Maxime Coquelin <mcoquelin.stm32@...il.com>, Alexandre Torgue <alexandre.torgue@...com>, Gerald Baeza <gerald.baeza@...com>, Linux ARM <linux-arm-kernel@...ts.infradead.org>, Linux Kernel Mailing List <linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>, "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org> Subject: Re: [PATCH 13/19] mmc: mmci: send stop cmd if a data command fail hi Ulf thanks for review, comments below On 07/04/2018 03:37 PM, Ulf Hansson wrote: > On 12 June 2018 at 15:14, Ludovic Barre <ludovic.Barre@...com> wrote: >> From: Ludovic Barre <ludovic.barre@...com> >> >> The mmc framework follows the requirement of SD_Specification: >> the STOP_TRANSMISSION is sent on multiple write/read commands >> and the stop command (alone), not needed on other ADTC commands. >> >> But, some variants require a stop command "STOP_TRANSMISION" to clear >> the DPSM "Data Path State Machine" if an error happens on command or data >> step. If it's not done the next data command will freeze hardware block. >> Needed to support the STM32 sdmmc variant. >> >> Signed-off-by: Ludovic Barre <ludovic.barre@...com> >> --- >> drivers/mmc/host/mmci.c | 36 +++++++++++++++++++++++++++++++----- >> drivers/mmc/host/mmci.h | 3 +++ >> 2 files changed, 34 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c >> index 87724e1..9af7db8 100644 >> --- a/drivers/mmc/host/mmci.c >> +++ b/drivers/mmc/host/mmci.c >> @@ -24,6 +24,7 @@ >> #include <linux/mmc/pm.h> >> #include <linux/mmc/host.h> >> #include <linux/mmc/card.h> >> +#include <linux/mmc/mmc.h> >> #include <linux/mmc/slot-gpio.h> >> #include <linux/amba/bus.h> >> #include <linux/clk.h> >> @@ -522,11 +523,28 @@ static void mmci_set_mask1(struct mmci_host *host, unsigned int mask) >> host->mask1_reg = mask; >> } >> >> -static void mmci_stop_data(struct mmci_host *host) >> +static int mmci_stop_data(struct mmci_host *host) > > Let's leave this as void function and instead add a check in > mmci_start_command(), to see if the command is a > MMC_STOP_TRANSMISSION. If so, then add host->variant->cmdreg_stop to > the register that is written in it. In fact: the cmdreg_stop bit (which abort and reinit the DPSM) should have been activated only in error case. But I see a mistake in this piece of code => cmdreg_stop bit is set for all MMC_STOP_TRANSMISSION. So, I read closely the sdmmc datasheet and it seems possible to set cmdreg_stop bit for all MMC_STOP_TRANSMISSION (error or not). So, I will try your proposition to set cmdreg_stop bit in mmci_start_command() if the command is MMC_STOP_TRANSMISSION. > > And actually, I think this change alone should be a separate patch > coming before $subject patch in the series, as it addresses the first > problem of two. > >> { >> + struct mmc_command *stop = &host->stop_abort; >> + struct mmc_data *data = host->data; >> + unsigned int cmd = 0; >> + >> mmci_write_datactrlreg(host, 0); >> mmci_set_mask1(host, 0); >> host->data = NULL; >> + >> + if (host->variant->cmdreg_stop) { >> + cmd |= host->variant->cmdreg_stop; >> + if (!data->stop) { >> + memset(stop, 0, sizeof(struct mmc_command)); >> + stop->opcode = MMC_STOP_TRANSMISSION; >> + stop->arg = 0; >> + stop->flags = MMC_RSP_R1B | MMC_CMD_AC; >> + data->stop = stop; > > I don't understand why you want the host->stop_abort being present in > the mmci host struct? Can we get rid of that? > > Anyway, re-using the data->stop pointer seems reasonable. In fact mrq->data->stop could be not referenced (NULL). for data command, the stop command is allocated in mmc_blk_request struct mmc_blk_request { struct mmc_command stop; }; struct mmc_data { struct mmc_command *stop; }; struct mmc_request { struct mmc_command *stop; }; in mmc_blk_rw_rq_prep function: if data.blocks > 1 brq->mrq.stop = &brq->stop; else brq->mrq.stop = NULL; in mmc_mrq_prep function: if (mrq->stop) { mrq->data->stop = mrq->stop; Update about this topic: This last week I found a new specific need for sdmmc variant. On response type MMC_RSP_BUSY (example mmc_switch command cmd6, AC command without data): SDMMC needs to set MMCIDATATIMER to define busy_timeout. If timeout expires Datatimeout and DPSM flags occur. So a MMC_STOP_TRANSMISSION with cmdreg_stop bit is required to abort and reinitialize the DPSM. So, I think we could keep "host->stop_abort" which may be used in cmd or data requests. The stop_abort command is always the same, so stop_abort command could be initialized only while probe function (under variant->cmdreg_stop). What do you think about it ? > >> + } >> + } >> + >> + return cmd; >> } >> >> static void mmci_init_sg(struct mmci_host *host, struct mmc_data *data) >> @@ -703,6 +721,7 @@ mmci_data_irq(struct mmci_host *host, struct mmc_data *data, >> unsigned int status) >> { >> unsigned int status_err; >> + unsigned int cmd_reg = 0; >> >> /* Make sure we have data to handle */ >> if (!data) >> @@ -761,7 +780,7 @@ mmci_data_irq(struct mmci_host *host, struct mmc_data *data, >> if (status & MCI_DATAEND || data->error) { >> mmci_dma_finalize(host, data); >> >> - mmci_stop_data(host); >> + cmd_reg = mmci_stop_data(host); >> >> if (!data->error) >> /* The error clause is handled above, success! */ >> @@ -770,7 +789,7 @@ mmci_data_irq(struct mmci_host *host, struct mmc_data *data, >> if (!data->stop || host->mrq->sbc) { >> mmci_request_end(host, data->mrq); >> } else { >> - mmci_start_command(host, data->stop, 0); >> + mmci_start_command(host, data->stop, cmd_reg); >> } >> } >> } >> @@ -780,6 +799,8 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd, >> unsigned int status) >> { >> void __iomem *base = host->base; >> + struct mmc_data *data = host->data; >> + unsigned int cmd_reg = 0; >> bool sbc; >> >> if (!cmd) >> @@ -865,11 +886,16 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd, >> } >> >> if ((!sbc && !cmd->data) || cmd->error) { >> - if (host->data) { >> + if (data) { >> /* Terminate the DMA transfer */ >> mmci_dma_error(host); >> >> - mmci_stop_data(host); >> + cmd_reg = mmci_stop_data(host); >> + >> + if (data->stop) { > > This means a change in behavior for !host->variant->cmdreg_stop > variants, because the mmc core may have assigned data->stop. Did you > test this path for any of the old variants? I just tested on: -stm32f7 => close to pl180 variant. -stm32h7 and mp1 => sdmmc variant. -I like try to snowball but I've not yet find this board. -no arm or qcom boards that seem ok for the both (f7/mp1). > > The change as such anyway seems like a good idea. If we had data, we > may play safe and send a stop command to let the card to try to close > down the transfer. On the other hand, we don't want this to come as > side-effect, but rather it deserves is own standalone patch. > > I guess there are two options going forward. > 1) Split the patch and see if this works for other variants first and > the add your changes on top for the new ST variant. > 2) Make this change only to affect the new ST variant. At first time, I prefer not to have a side effect on other variant. This part must be rework, like I note above in "Update about this topic", the stop transmission with cmdreg_stop bit could be send on command without data (like cmd6: mmc_switch) > > I am fine with both. > >> + mmci_start_command(host, data->stop, cmd_reg); >> + return; >> + } >> } >> mmci_request_end(host, host->mrq); >> } else if (sbc) { >> diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h >> index 2d7e901..4b4cd1d 100644 >> --- a/drivers/mmc/host/mmci.h >> +++ b/drivers/mmc/host/mmci.h >> @@ -243,6 +243,7 @@ struct mmci_host; >> * @cmdreg_lrsp_crc: enable value for long response with crc >> * @cmdreg_srsp_crc: enable value for short response with crc >> * @cmdreg_srsp: enable value for short response without crc >> + * @cmdreg_stop: enable value for stop and abort transmission >> * @datalength_bits: number of bits in the MMCIDATALENGTH register >> * @fifosize: number of bytes that can be written when MMCI_TXFIFOEMPTY >> * is asserted (likewise for RX) >> @@ -298,6 +299,7 @@ struct variant_data { >> unsigned int cmdreg_lrsp_crc; >> unsigned int cmdreg_srsp_crc; >> unsigned int cmdreg_srsp; >> + unsigned int cmdreg_stop; >> unsigned int datalength_bits; >> unsigned int fifosize; >> unsigned int fifohalfsize; >> @@ -343,6 +345,7 @@ struct mmci_host { >> struct mmc_request *mrq; >> struct mmc_command *cmd; >> struct mmc_data *data; >> + struct mmc_command stop_abort; >> struct mmc_host *mmc; >> struct clk *clk; >> bool singleirq; >> -- >> 2.7.4 >> > > Kind regards > Uffe >
Powered by blists - more mailing lists