| Message ID | 1471317827-6049-1-git-send-email-shawn.lin@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Shawn, On 08/16/2016 12:23 PM, Shawn Lin wrote: > We could see an obvious race condition by test that > the former write operation by IDMAC aiming to clear > OWN bit reach right after the later configuration of > the same desc, which makes the IDMAC be in SUSPEND > state as the OWN bit was cleared by the asynchronous > write operation of IDMAC. The bug can be very easy > reproduced on RK3288 or similar when lowering the > bandwidth of bus and aggravating the Qos to make the > large numbers of IP fight for the priority. One possible > replaceable solution may be alloc dual buff for the > desc to avoid it but could still race each other > theoretically. It makes sense. But Sorry..i didn't understand what "when lowering the bandwidth of bus.." means.. According to TRM, we need to check whether OWN bit is set or not. But it seems that it's related with controlling PLDMND register. I need to check and read TRM in more detail. > > Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> > > --- > > drivers/mmc/host/dw_mmc.c | 34 ++++++++++++++++++++++++++++++++++ > 1 file changed, 34 insertions(+) > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > index 32380d5..7b01fab 100644 > --- a/drivers/mmc/host/dw_mmc.c > +++ b/drivers/mmc/host/dw_mmc.c > @@ -490,6 +490,23 @@ static void dw_mci_translate_sglist(struct dw_mci *host, struct mmc_data *data, > length -= desc_len; > > /* > + * OWN bit should be clear by IDMAC after > + * finishing transfer. Let's wait for the > + * asynchronous operation of IDMAC and cpu > + * to make sure that we do not rely on the > + * order of Qos of bus and architecture. > + * Otherwise we could see a race condition > + * here that the former write operation of > + * IDMAC(to clear the OWN bit) reach right > + * after the later new configuration of desc > + * which makes value of desc been covered > + * leading to DMA_SUSPEND state as IDMAC fecth > + * the wrong desc then. > + */ > + while ((readl(&desc->des0) & IDMAC_DES0_OWN)) > + ; Well, I don't like this style..because it has the potential risk for infinite loop. And Could you make the comment more simpler than now? > + > + /* > * Set the OWN bit and disable interrupts > * for this descriptor > */ > @@ -535,6 +552,23 @@ static void dw_mci_translate_sglist(struct dw_mci *host, struct mmc_data *data, > length -= desc_len; > > /* > + * OWN bit should be clear by IDMAC after > + * finishing transfer. Let's wait for the > + * asynchronous operation of IDMAC and cpu > + * to make sure that we do not rely on the > + * order of Qos of bus and architecture. > + * Otherwise we could see a race condition > + * here that the former write operation of > + * IDMAC(to clear the OWN bit) reach right > + * after the later new configuration of desc > + * which makes value of desc been covered > + * leading to DMA_SUSPEND state as IDMAC fecth > + * the wrong desc then. > + */ > + while ((readl(&desc->des0) & IDMAC_DES0_OWN)) > + ; Ditto. > + > + /* > * Set the OWN bit and disable interrupts > * for this descriptor > */ > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
在 2016/8/17 17:52, Jaehoon Chung 写道: > Hi Shawn, > > On 08/16/2016 12:23 PM, Shawn Lin wrote: >> We could see an obvious race condition by test that >> the former write operation by IDMAC aiming to clear >> OWN bit reach right after the later configuration of >> the same desc, which makes the IDMAC be in SUSPEND >> state as the OWN bit was cleared by the asynchronous >> write operation of IDMAC. The bug can be very easy >> reproduced on RK3288 or similar when lowering the >> bandwidth of bus and aggravating the Qos to make the >> large numbers of IP fight for the priority. One possible >> replaceable solution may be alloc dual buff for the >> desc to avoid it but could still race each other >> theoretically. > > It makes sense. > But Sorry..i didn't understand what "when lowering the bandwidth of bus.." means.. I will amend it. What I actually mean is to reduce the running rate of system bus without touching the rate of CPU. This makes the CPU run more fater than other masters like IDMAC. > > According to TRM, we need to check whether OWN bit is set or not. > But it seems that it's related with controlling PLDMND register. > I need to check and read TRM in more detail. Per the TRM, "The IDMAC engine fetches the descriptor and checks the OWN bit. If the OWN bit is not set, it means that the host owns the descriptor. In this case the DMA enters suspend state and asserts the Descriptor Unable interrupt in the IDSTS register (IDSTS64 register, in case of 64-bit address configuration). In such a case, the host needs to release the IDMAC by writing any value to the poll demand register. " So in this case, it should generate a INT for IDSTS(64) and release the IDMAC by writing to PLDMND, then we retry it until IDMAC owns the descriptor. It's quite a verbose behaviour and we should do more a lot for the tasklet to overcome this pain... It's sane to just verify the own bit from memory when preparing desc, and it doesn't cost performance from the test as it is really few to meet this case. > >> >> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> >> >> --- >> >> drivers/mmc/host/dw_mmc.c | 34 ++++++++++++++++++++++++++++++++++ >> 1 file changed, 34 insertions(+) >> >> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c >> index 32380d5..7b01fab 100644 >> --- a/drivers/mmc/host/dw_mmc.c >> +++ b/drivers/mmc/host/dw_mmc.c >> @@ -490,6 +490,23 @@ static void dw_mci_translate_sglist(struct dw_mci *host, struct mmc_data *data, >> length -= desc_len; >> >> /* >> + * OWN bit should be clear by IDMAC after >> + * finishing transfer. Let's wait for the >> + * asynchronous operation of IDMAC and cpu >> + * to make sure that we do not rely on the >> + * order of Qos of bus and architecture. >> + * Otherwise we could see a race condition >> + * here that the former write operation of >> + * IDMAC(to clear the OWN bit) reach right >> + * after the later new configuration of desc >> + * which makes value of desc been covered >> + * leading to DMA_SUSPEND state as IDMAC fecth >> + * the wrong desc then. >> + */ >> + while ((readl(&desc->des0) & IDMAC_DES0_OWN)) >> + ; > > Well, I don't like this style..because it has the potential risk for infinite loop. > And Could you make the comment more simpler than now? yes, I will remove the RFC prefix and fix these. > >> + >> + /* >> * Set the OWN bit and disable interrupts >> * for this descriptor >> */ >> @@ -535,6 +552,23 @@ static void dw_mci_translate_sglist(struct dw_mci *host, struct mmc_data *data, >> length -= desc_len; >> >> /* >> + * OWN bit should be clear by IDMAC after >> + * finishing transfer. Let's wait for the >> + * asynchronous operation of IDMAC and cpu >> + * to make sure that we do not rely on the >> + * order of Qos of bus and architecture. >> + * Otherwise we could see a race condition >> + * here that the former write operation of >> + * IDMAC(to clear the OWN bit) reach right >> + * after the later new configuration of desc >> + * which makes value of desc been covered >> + * leading to DMA_SUSPEND state as IDMAC fecth >> + * the wrong desc then. >> + */ >> + while ((readl(&desc->des0) & IDMAC_DES0_OWN)) >> + ; > > Ditto. > >> + >> + /* >> * Set the OWN bit and disable interrupts >> * for this descriptor >> */ >> > > > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip >
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c index 32380d5..7b01fab 100644 --- a/drivers/mmc/host/dw_mmc.c +++ b/drivers/mmc/host/dw_mmc.c @@ -490,6 +490,23 @@ static void dw_mci_translate_sglist(struct dw_mci *host, struct mmc_data *data, length -= desc_len; /* + * OWN bit should be clear by IDMAC after + * finishing transfer. Let's wait for the + * asynchronous operation of IDMAC and cpu + * to make sure that we do not rely on the + * order of Qos of bus and architecture. + * Otherwise we could see a race condition + * here that the former write operation of + * IDMAC(to clear the OWN bit) reach right + * after the later new configuration of desc + * which makes value of desc been covered + * leading to DMA_SUSPEND state as IDMAC fecth + * the wrong desc then. + */ + while ((readl(&desc->des0) & IDMAC_DES0_OWN)) + ; + + /* * Set the OWN bit and disable interrupts * for this descriptor */ @@ -535,6 +552,23 @@ static void dw_mci_translate_sglist(struct dw_mci *host, struct mmc_data *data, length -= desc_len; /* + * OWN bit should be clear by IDMAC after + * finishing transfer. Let's wait for the + * asynchronous operation of IDMAC and cpu + * to make sure that we do not rely on the + * order of Qos of bus and architecture. + * Otherwise we could see a race condition + * here that the former write operation of + * IDMAC(to clear the OWN bit) reach right + * after the later new configuration of desc + * which makes value of desc been covered + * leading to DMA_SUSPEND state as IDMAC fecth + * the wrong desc then. + */ + while ((readl(&desc->des0) & IDMAC_DES0_OWN)) + ; + + /* * Set the OWN bit and disable interrupts * for this descriptor */
We could see an obvious race condition by test that the former write operation by IDMAC aiming to clear OWN bit reach right after the later configuration of the same desc, which makes the IDMAC be in SUSPEND state as the OWN bit was cleared by the asynchronous write operation of IDMAC. The bug can be very easy reproduced on RK3288 or similar when lowering the bandwidth of bus and aggravating the Qos to make the large numbers of IP fight for the priority. One possible replaceable solution may be alloc dual buff for the desc to avoid it but could still race each other theoretically. Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com> --- drivers/mmc/host/dw_mmc.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)