* [PATCH 2/3] dw_mmc: Avoid adding the number of transmitted bytes twice
2013-03-12 10:53 [PATCH 1/3] dw_mmc: Don't loop when handling an interrupt Markos Chandras
@ 2013-03-12 10:53 ` Markos Chandras
2013-03-22 16:50 ` Chris Ball
2013-03-12 10:53 ` [PATCH 3/3] dw_mmc: Handle unaligned data submission correctly Markos Chandras
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Markos Chandras @ 2013-03-12 10:53 UTC (permalink / raw)
To: linux-mmc
Cc: linux-kernel, Markos Chandras, Seungwon Jeon, Jaehoon Chung,
Chris Ball
Previously, it was possible to add either 0 bytes or add nbytes
twice if we broke out of the outer loop and then carry on to the
"done" label. This is now fixed by adding the transferred bytes
right after the pull/pop operation
Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
Cc: Seungwon Jeon <tgih.jun@samsung.com>
Cc: Jaehoon Chung <jh80.chung@samsung.com>
Cc: Chris Ball <cjb@laptop.org>
---
The patch is based on Chris Ball's mmc-next branch
drivers/mmc/host/dw_mmc.c | 12 ++++--------
1 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 78c7251..4e7a5c8 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1446,7 +1446,7 @@ static void dw_mci_read_data_pio(struct dw_mci *host)
struct mmc_data *data = host->data;
int shift = host->data_shift;
u32 status;
- unsigned int nbytes = 0, len;
+ unsigned int len;
unsigned int remain, fcnt;
do {
@@ -1465,8 +1465,8 @@ static void dw_mci_read_data_pio(struct dw_mci *host)
if (!len)
break;
dw_mci_pull_data(host, (void *)(buf + offset), len);
+ data->bytes_xfered += len;
offset += len;
- nbytes += len;
remain -= len;
} while (remain);
@@ -1474,7 +1474,6 @@ static void dw_mci_read_data_pio(struct dw_mci *host)
status = mci_readl(host, MINTSTS);
mci_writel(host, RINTSTS, SDMMC_INT_RXDR);
} while (status & SDMMC_INT_RXDR); /*if the RXDR is ready read again*/
- data->bytes_xfered += nbytes;
if (!remain) {
if (!sg_miter_next(sg_miter))
@@ -1485,7 +1484,6 @@ static void dw_mci_read_data_pio(struct dw_mci *host)
return;
done:
- data->bytes_xfered += nbytes;
sg_miter_stop(sg_miter);
host->sg = NULL;
smp_wmb();
@@ -1500,7 +1498,7 @@ static void dw_mci_write_data_pio(struct dw_mci *host)
struct mmc_data *data = host->data;
int shift = host->data_shift;
u32 status;
- unsigned int nbytes = 0, len;
+ unsigned int len;
unsigned int fifo_depth = host->fifo_depth;
unsigned int remain, fcnt;
@@ -1521,8 +1519,8 @@ static void dw_mci_write_data_pio(struct dw_mci *host)
if (!len)
break;
host->push_data(host, (void *)(buf + offset), len);
+ data->bytes_xfered += len;
offset += len;
- nbytes += len;
remain -= len;
} while (remain);
@@ -1530,7 +1528,6 @@ static void dw_mci_write_data_pio(struct dw_mci *host)
status = mci_readl(host, MINTSTS);
mci_writel(host, RINTSTS, SDMMC_INT_TXDR);
} while (status & SDMMC_INT_TXDR); /* if TXDR write again */
- data->bytes_xfered += nbytes;
if (!remain) {
if (!sg_miter_next(sg_miter))
@@ -1541,7 +1538,6 @@ static void dw_mci_write_data_pio(struct dw_mci *host)
return;
done:
- data->bytes_xfered += nbytes;
sg_miter_stop(sg_miter);
host->sg = NULL;
smp_wmb();
--
1.7.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 2/3] dw_mmc: Avoid adding the number of transmitted bytes twice
2013-03-12 10:53 ` [PATCH 2/3] dw_mmc: Avoid adding the number of transmitted bytes twice Markos Chandras
@ 2013-03-22 16:50 ` Chris Ball
0 siblings, 0 replies; 10+ messages in thread
From: Chris Ball @ 2013-03-22 16:50 UTC (permalink / raw)
To: Markos Chandras; +Cc: linux-mmc, linux-kernel, Seungwon Jeon, Jaehoon Chung
Hi,
On Tue, Mar 12 2013, Markos Chandras wrote:
> Previously, it was possible to add either 0 bytes or add nbytes
> twice if we broke out of the outer loop and then carry on to the
> "done" label. This is now fixed by adding the transferred bytes
> right after the pull/pop operation
>
> Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
> Cc: Seungwon Jeon <tgih.jun@samsung.com>
> Cc: Jaehoon Chung <jh80.chung@samsung.com>
> Cc: Chris Ball <cjb@laptop.org>
Thanks, pushed to mmc-next for 3.10.
- Chris.
--
Chris Ball <cjb@laptop.org> <http://printf.net/>
One Laptop Per Child
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/3] dw_mmc: Handle unaligned data submission correctly
2013-03-12 10:53 [PATCH 1/3] dw_mmc: Don't loop when handling an interrupt Markos Chandras
2013-03-12 10:53 ` [PATCH 2/3] dw_mmc: Avoid adding the number of transmitted bytes twice Markos Chandras
@ 2013-03-12 10:53 ` Markos Chandras
2013-03-22 16:52 ` Chris Ball
2013-03-13 8:31 ` [PATCH 1/3] dw_mmc: Don't loop when handling an interrupt Jaehoon Chung
2013-03-13 14:22 ` Seungwon Jeon
3 siblings, 1 reply; 10+ messages in thread
From: Markos Chandras @ 2013-03-12 10:53 UTC (permalink / raw)
To: linux-mmc
Cc: linux-kernel, Markos Chandras, Seungwon Jeon, Jaehoon Chung,
Chris Ball
Commit f9c2a0dc42a6938ff2a80e55ca2bbd1d5581c72e
"mmc: dw_mmc: Fix PIO mode with support of highmem" introduced
a regression since v3.2 making the mmc_test hang on test #13
with a "Data starvation by host timeout" interrupt.
This is because, sg_mapping_iter is used to iterate through the
data which spans on multiple pages. The problem is detected on
unaligned data submission where the code previously checked for
!(sg_next(host->sg)) which is true because we only have a single
scatter/gather list which then expands to multiple pages.
Therefore, the driver incorrectly assumed that this was the last
list item and submitted unaligned data to the mmc device. This
overflowed the FIFO on the device before all the data were written
to it. The code was fixed to only submit unaligned data when we are
handling the last sg_miter item by checking whether we reached
the desired data length or not.
The patch was tested against mmc_test and all the tests passed.
Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
Cc: Seungwon Jeon <tgih.jun@samsung.com>
Cc: Jaehoon Chung <jh80.chung@samsung.com>
Cc: Chris Ball <cjb@laptop.org>
---
The patch is based on Chris Ball's mmc-next branch
drivers/mmc/host/dw_mmc.c | 33 ++++++++++++++++++++++++---------
1 files changed, 24 insertions(+), 9 deletions(-)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 4e7a5c8..d3f68d4 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -1191,12 +1191,15 @@ static void dw_mci_pull_final_bytes(struct dw_mci *host, void *buf, int cnt)
static void dw_mci_push_data16(struct dw_mci *host, void *buf, int cnt)
{
+ struct mmc_data *data = host->data;
+ int init_cnt = cnt;
+
/* try and push anything in the part_buf */
if (unlikely(host->part_buf_count)) {
int len = dw_mci_push_part_bytes(host, buf, cnt);
buf += len;
cnt -= len;
- if (!sg_next(host->sg) || host->part_buf_count == 2) {
+ if (host->part_buf_count == 2) {
mci_writew(host, DATA(host->data_offset),
host->part_buf16);
host->part_buf_count = 0;
@@ -1229,9 +1232,11 @@ static void dw_mci_push_data16(struct dw_mci *host, void *buf, int cnt)
/* put anything remaining in the part_buf */
if (cnt) {
dw_mci_set_part_bytes(host, buf, cnt);
- if (!sg_next(host->sg))
+ /* Push data if we have reached the expected data length */
+ if ((data->bytes_xfered + init_cnt) ==
+ (data->blksz * data->blocks))
mci_writew(host, DATA(host->data_offset),
- host->part_buf16);
+ host->part_buf16);
}
}
@@ -1269,12 +1274,15 @@ static void dw_mci_pull_data16(struct dw_mci *host, void *buf, int cnt)
static void dw_mci_push_data32(struct dw_mci *host, void *buf, int cnt)
{
+ struct mmc_data *data = host->data;
+ int init_cnt = cnt;
+
/* try and push anything in the part_buf */
if (unlikely(host->part_buf_count)) {
int len = dw_mci_push_part_bytes(host, buf, cnt);
buf += len;
cnt -= len;
- if (!sg_next(host->sg) || host->part_buf_count == 4) {
+ if (host->part_buf_count == 4) {
mci_writel(host, DATA(host->data_offset),
host->part_buf32);
host->part_buf_count = 0;
@@ -1307,9 +1315,11 @@ static void dw_mci_push_data32(struct dw_mci *host, void *buf, int cnt)
/* put anything remaining in the part_buf */
if (cnt) {
dw_mci_set_part_bytes(host, buf, cnt);
- if (!sg_next(host->sg))
+ /* Push data if we have reached the expected data length */
+ if ((data->bytes_xfered + init_cnt) ==
+ (data->blksz * data->blocks))
mci_writel(host, DATA(host->data_offset),
- host->part_buf32);
+ host->part_buf32);
}
}
@@ -1347,12 +1357,15 @@ static void dw_mci_pull_data32(struct dw_mci *host, void *buf, int cnt)
static void dw_mci_push_data64(struct dw_mci *host, void *buf, int cnt)
{
+ struct mmc_data *data = host->data;
+ int init_cnt = cnt;
+
/* try and push anything in the part_buf */
if (unlikely(host->part_buf_count)) {
int len = dw_mci_push_part_bytes(host, buf, cnt);
buf += len;
cnt -= len;
- if (!sg_next(host->sg) || host->part_buf_count == 8) {
+ if (host->part_buf_count == 8) {
mci_writew(host, DATA(host->data_offset),
host->part_buf);
host->part_buf_count = 0;
@@ -1385,9 +1398,11 @@ static void dw_mci_push_data64(struct dw_mci *host, void *buf, int cnt)
/* put anything remaining in the part_buf */
if (cnt) {
dw_mci_set_part_bytes(host, buf, cnt);
- if (!sg_next(host->sg))
+ /* Push data if we have reached the expected data length */
+ if ((data->bytes_xfered + init_cnt) ==
+ (data->blksz * data->blocks))
mci_writeq(host, DATA(host->data_offset),
- host->part_buf);
+ host->part_buf);
}
}
--
1.7.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 3/3] dw_mmc: Handle unaligned data submission correctly
2013-03-12 10:53 ` [PATCH 3/3] dw_mmc: Handle unaligned data submission correctly Markos Chandras
@ 2013-03-22 16:52 ` Chris Ball
0 siblings, 0 replies; 10+ messages in thread
From: Chris Ball @ 2013-03-22 16:52 UTC (permalink / raw)
To: Markos Chandras; +Cc: linux-mmc, linux-kernel, Seungwon Jeon, Jaehoon Chung
Hi,
On Tue, Mar 12 2013, Markos Chandras wrote:
> Commit f9c2a0dc42a6938ff2a80e55ca2bbd1d5581c72e
> "mmc: dw_mmc: Fix PIO mode with support of highmem" introduced
> a regression since v3.2 making the mmc_test hang on test #13
> with a "Data starvation by host timeout" interrupt.
>
> This is because, sg_mapping_iter is used to iterate through the
> data which spans on multiple pages. The problem is detected on
> unaligned data submission where the code previously checked for
> !(sg_next(host->sg)) which is true because we only have a single
> scatter/gather list which then expands to multiple pages.
> Therefore, the driver incorrectly assumed that this was the last
> list item and submitted unaligned data to the mmc device. This
> overflowed the FIFO on the device before all the data were written
> to it. The code was fixed to only submit unaligned data when we are
> handling the last sg_miter item by checking whether we reached
> the desired data length or not.
>
> The patch was tested against mmc_test and all the tests passed.
>
> Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
> Cc: Seungwon Jeon <tgih.jun@samsung.com>
> Cc: Jaehoon Chung <jh80.chung@samsung.com>
> Cc: Chris Ball <cjb@laptop.org>
Thanks, pushed to mmc-next for 3.10.
- Chris.
--
Chris Ball <cjb@laptop.org> <http://printf.net/>
One Laptop Per Child
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] dw_mmc: Don't loop when handling an interrupt
2013-03-12 10:53 [PATCH 1/3] dw_mmc: Don't loop when handling an interrupt Markos Chandras
2013-03-12 10:53 ` [PATCH 2/3] dw_mmc: Avoid adding the number of transmitted bytes twice Markos Chandras
2013-03-12 10:53 ` [PATCH 3/3] dw_mmc: Handle unaligned data submission correctly Markos Chandras
@ 2013-03-13 8:31 ` Jaehoon Chung
2013-03-13 14:22 ` Seungwon Jeon
3 siblings, 0 replies; 10+ messages in thread
From: Jaehoon Chung @ 2013-03-13 8:31 UTC (permalink / raw)
To: Markos Chandras
Cc: linux-mmc, linux-kernel, Seungwon Jeon, Jaehoon Chung, Chris Ball
Acked-by: Jaehoon Chung <jh80.chung@samsung.com>
On 03/12/2013 07:53 PM, Markos Chandras wrote:
> There is no reason to loop when handling an interrupt. The "if" clauses
> will handle all of them sequentially. This also eliminates the extra loop
> we used to take with no pending interrupts and we ended up breaking out
> of the while loop.
>
> Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
> Cc: Seungwon Jeon <tgih.jun@samsung.com>
> Cc: Jaehoon Chung <jh80.chung@samsung.com>
> Cc: Chris Ball <cjb@laptop.org>
> ---
> The patch is based on Chris Ball's mmc-next branch
>
> drivers/mmc/host/dw_mmc.c | 11 ++++-------
> 1 files changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 60063cc..78c7251 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -1563,11 +1563,11 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
> {
> struct dw_mci *host = dev_id;
> u32 pending;
> - unsigned int pass_count = 0;
> int i;
>
> - do {
> - pending = mci_readl(host, MINTSTS); /* read-only mask reg */
> + pending = mci_readl(host, MINTSTS); /* read-only mask reg */
> +
> + if (pending) {
>
> /*
> * DTO fix - version 2.10a and below, and only if internal DMA
> @@ -1579,9 +1579,6 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
> pending |= SDMMC_INT_DATA_OVER;
> }
>
> - if (!pending)
> - break;
> -
> if (pending & DW_MCI_CMD_ERROR_FLAGS) {
> mci_writel(host, RINTSTS, DW_MCI_CMD_ERROR_FLAGS);
> host->cmd_status = pending;
> @@ -1642,7 +1639,7 @@ static irqreturn_t dw_mci_interrupt(int irq, void *dev_id)
> }
> }
>
> - } while (pass_count++ < 5);
> + }
>
> #ifdef CONFIG_MMC_DW_IDMAC
> /* Handle DMA interrupts */
>
^ permalink raw reply [flat|nested] 10+ messages in thread* RE: [PATCH 1/3] dw_mmc: Don't loop when handling an interrupt
2013-03-12 10:53 [PATCH 1/3] dw_mmc: Don't loop when handling an interrupt Markos Chandras
` (2 preceding siblings ...)
2013-03-13 8:31 ` [PATCH 1/3] dw_mmc: Don't loop when handling an interrupt Jaehoon Chung
@ 2013-03-13 14:22 ` Seungwon Jeon
2013-03-13 14:26 ` Chris Ball
2013-03-13 14:26 ` Markos Chandras
3 siblings, 2 replies; 10+ messages in thread
From: Seungwon Jeon @ 2013-03-13 14:22 UTC (permalink / raw)
To: 'Markos Chandras', linux-mmc
Cc: linux-kernel, 'Jaehoon Chung', 'Chris Ball'
On Tuesday, March 12, 2013, Markos Chandras wrote:
> There is no reason to loop when handling an interrupt. The "if" clauses
> will handle all of them sequentially. This also eliminates the extra loop
> we used to take with no pending interrupts and we ended up breaking out
> of the while loop.
>
> Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
> Cc: Seungwon Jeon <tgih.jun@samsung.com>
Acked-by: Seungwon Jeon <tgih.jun@samsung.com>
You can add my ack to other patches.
However, could you change the subject prefix to 'mmc: dw_mmc' and resend?
Thanks,
Seungwon Jeon
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/3] dw_mmc: Don't loop when handling an interrupt
2013-03-13 14:22 ` Seungwon Jeon
@ 2013-03-13 14:26 ` Chris Ball
2013-03-13 14:27 ` Markos Chandras
2013-03-13 14:26 ` Markos Chandras
1 sibling, 1 reply; 10+ messages in thread
From: Chris Ball @ 2013-03-13 14:26 UTC (permalink / raw)
To: Seungwon Jeon
Cc: 'Markos Chandras', linux-mmc, linux-kernel,
'Jaehoon Chung'
Hi,
On Wed, Mar 13 2013, Seungwon Jeon wrote:
> On Tuesday, March 12, 2013, Markos Chandras wrote:
>> There is no reason to loop when handling an interrupt. The "if" clauses
>> will handle all of them sequentially. This also eliminates the extra loop
>> we used to take with no pending interrupts and we ended up breaking out
>> of the while loop.
>>
>> Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
>> Cc: Seungwon Jeon <tgih.jun@samsung.com>
>
> Acked-by: Seungwon Jeon <tgih.jun@samsung.com>
>
> You can add my ack to other patches.
> However, could you change the subject prefix to 'mmc: dw_mmc' and resend?
It's okay, I'll take care of that when applying this time. Thanks!
- Chris.
--
Chris Ball <cjb@laptop.org> <http://printf.net/>
One Laptop Per Child
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] dw_mmc: Don't loop when handling an interrupt
2013-03-13 14:26 ` Chris Ball
@ 2013-03-13 14:27 ` Markos Chandras
0 siblings, 0 replies; 10+ messages in thread
From: Markos Chandras @ 2013-03-13 14:27 UTC (permalink / raw)
To: Chris Ball
Cc: Seungwon Jeon, linux-mmc, linux-kernel, 'Jaehoon Chung'
On 03/13/2013 02:26 PM, Chris Ball wrote:
> Hi,
>
> On Wed, Mar 13 2013, Seungwon Jeon wrote:
>> On Tuesday, March 12, 2013, Markos Chandras wrote:
>>> There is no reason to loop when handling an interrupt. The "if" clauses
>>> will handle all of them sequentially. This also eliminates the extra loop
>>> we used to take with no pending interrupts and we ended up breaking out
>>> of the while loop.
>>>
>>> Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
>>> Cc: Seungwon Jeon <tgih.jun@samsung.com>
>>
>> Acked-by: Seungwon Jeon <tgih.jun@samsung.com>
>>
>> You can add my ack to other patches.
>> However, could you change the subject prefix to 'mmc: dw_mmc' and resend?
>
> It's okay, I'll take care of that when applying this time. Thanks!
>
> - Chris.
>
Hi Chris,
Ok thank you
Markos
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] dw_mmc: Don't loop when handling an interrupt
2013-03-13 14:22 ` Seungwon Jeon
2013-03-13 14:26 ` Chris Ball
@ 2013-03-13 14:26 ` Markos Chandras
1 sibling, 0 replies; 10+ messages in thread
From: Markos Chandras @ 2013-03-13 14:26 UTC (permalink / raw)
To: Seungwon Jeon
Cc: linux-mmc, linux-kernel, 'Jaehoon Chung',
'Chris Ball'
On 03/13/2013 02:22 PM, Seungwon Jeon wrote:
> On Tuesday, March 12, 2013, Markos Chandras wrote:
>> There is no reason to loop when handling an interrupt. The "if" clauses
>> will handle all of them sequentially. This also eliminates the extra loop
>> we used to take with no pending interrupts and we ended up breaking out
>> of the while loop.
>>
>> Signed-off-by: Markos Chandras <markos.chandras@imgtec.com>
>> Cc: Seungwon Jeon <tgih.jun@samsung.com>
>
> Acked-by: Seungwon Jeon <tgih.jun@samsung.com>
>
> You can add my ack to other patches.
> However, could you change the subject prefix to 'mmc: dw_mmc' and resend?
>
> Thanks,
> Seungwon Jeon
>
Hi Seungwon,
Thanks. Ok I will fix the title and send them again.
Markos
^ permalink raw reply [flat|nested] 10+ messages in thread