| 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
| ||
|
Message-ID: <f8b5540d-d797-60d6-2351-8867b6b2b444@st.com>
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