| Message ID | 20211209031424.17842-2-chunfeng.yun@mediatek.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/3] usb: mtu3: fix interval value for intr and isoc | expand |
On Thu, Dec 09, 2021 at 11:14:23AM +0800, Chunfeng Yun wrote: > There is a seldom issue that the controller access invalid address > and trigger devapc or emimpu violation. That is due to memory access > is out of order and cause gpd data is not correct. > Make sure GPD is fully written before giving it to HW by setting its > HWO. > > Fixes: 48e0d3735aa5 ("usb: mtu3: supports new QMU format") > Cc: stable@vger.kernel.org > Reported-by: Eddie Hung <eddie.hung@mediatek.com> > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> > --- > drivers/usb/mtu3/mtu3_qmu.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/mtu3/mtu3_qmu.c b/drivers/usb/mtu3/mtu3_qmu.c > index 3f414f91b589..34bb5ac67efe 100644 > --- a/drivers/usb/mtu3/mtu3_qmu.c > +++ b/drivers/usb/mtu3/mtu3_qmu.c > @@ -273,6 +273,8 @@ static int mtu3_prepare_tx_gpd(struct mtu3_ep *mep, struct mtu3_request *mreq) > gpd->dw3_info |= cpu_to_le32(GPD_EXT_FLAG_ZLP); > } > > + /* make sure GPD is fully written before giving it to HW */ > + mb(); So this means you are using mmio for this structure? If so, shouldn't you be using normal io memory read/write calls as well and not just "raw" pointers like this: > gpd->dw0_info |= cpu_to_le32(GPD_FLAGS_IOC | GPD_FLAGS_HWO); Are you sure this is ok? Sprinkling around mb() calls is almost never the correct solution. If you need to ensure that a write succeeds, shouldn't you do a read from it afterward? Many busses require this, doesn't yours? > > mreq->gpd = gpd; > @@ -306,6 +308,8 @@ static int mtu3_prepare_rx_gpd(struct mtu3_ep *mep, struct mtu3_request *mreq) > gpd->next_gpd = cpu_to_le32(lower_32_bits(enq_dma)); > ext_addr |= GPD_EXT_NGP(mtu, upper_32_bits(enq_dma)); > gpd->dw3_info = cpu_to_le32(ext_addr); > + /* make sure GPD is fully written before giving it to HW */ > + mb(); Again, mb(); does not ensure that memory-mapped i/o actually hits the HW. Or if it does on your platform, how? mb() is a compiler barrier, not a memory write to a bus barrier. Please read Documentation/memory-barriers.txt for more details. thanks, greg k-h
On Mon, 2021-12-13 at 15:18 +0100, Greg Kroah-Hartman wrote: > On Thu, Dec 09, 2021 at 11:14:23AM +0800, Chunfeng Yun wrote: > > There is a seldom issue that the controller access invalid address > > and trigger devapc or emimpu violation. That is due to memory > > access > > is out of order and cause gpd data is not correct. > > Make sure GPD is fully written before giving it to HW by setting > > its > > HWO. > > > > Fixes: 48e0d3735aa5 ("usb: mtu3: supports new QMU format") > > Cc: stable@vger.kernel.org > > Reported-by: Eddie Hung <eddie.hung@mediatek.com> > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> > > --- > > drivers/usb/mtu3/mtu3_qmu.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/usb/mtu3/mtu3_qmu.c > > b/drivers/usb/mtu3/mtu3_qmu.c > > index 3f414f91b589..34bb5ac67efe 100644 > > --- a/drivers/usb/mtu3/mtu3_qmu.c > > +++ b/drivers/usb/mtu3/mtu3_qmu.c > > @@ -273,6 +273,8 @@ static int mtu3_prepare_tx_gpd(struct mtu3_ep > > *mep, struct mtu3_request *mreq) > > gpd->dw3_info |= cpu_to_le32(GPD_EXT_FLAG_ZLP); > > } > > > > + /* make sure GPD is fully written before giving it to HW */ > > + mb(); > > So this means you are using mmio for this structure? No, it's a noncached memory. > If so, shouldn't > you be using normal io memory read/write calls as well and not just > "raw" pointers like this: > > > gpd->dw0_info |= cpu_to_le32(GPD_FLAGS_IOC | GPD_FLAGS_HWO); > > Are you sure this is ok? > > Sprinkling around mb() calls is almost never the correct solution. > > If you need to ensure that a write succeeds, shouldn't you do a read > from it afterward? Many busses require this, doesn't yours? It works for register access. Here is noncache memory access, add mb(), just want to prohibite both the compiler and CPU from reordering read/writes. > > > > > > > mreq->gpd = gpd; > > @@ -306,6 +308,8 @@ static int mtu3_prepare_rx_gpd(struct mtu3_ep > > *mep, struct mtu3_request *mreq) > > gpd->next_gpd = cpu_to_le32(lower_32_bits(enq_dma)); > > ext_addr |= GPD_EXT_NGP(mtu, upper_32_bits(enq_dma)); > > gpd->dw3_info = cpu_to_le32(ext_addr); > > + /* make sure GPD is fully written before giving it to HW */ > > + mb(); > > Again, mb(); does not ensure that memory-mapped i/o actually hits the > HW. Or if it does on your platform, how? Maybe the comment is misleading, I'll change it, here just want to prevent reordering of compiler and cpu. > > mb() is a compiler barrier, not a memory write to a bus > barrier. Please > read Documentation/memory-barriers.txt for more details. Ok, I'll do. Thanks a lot > > thanks, > > greg k-h
diff --git a/drivers/usb/mtu3/mtu3_qmu.c b/drivers/usb/mtu3/mtu3_qmu.c index 3f414f91b589..34bb5ac67efe 100644 --- a/drivers/usb/mtu3/mtu3_qmu.c +++ b/drivers/usb/mtu3/mtu3_qmu.c @@ -273,6 +273,8 @@ static int mtu3_prepare_tx_gpd(struct mtu3_ep *mep, struct mtu3_request *mreq) gpd->dw3_info |= cpu_to_le32(GPD_EXT_FLAG_ZLP); } + /* make sure GPD is fully written before giving it to HW */ + mb(); gpd->dw0_info |= cpu_to_le32(GPD_FLAGS_IOC | GPD_FLAGS_HWO); mreq->gpd = gpd; @@ -306,6 +308,8 @@ static int mtu3_prepare_rx_gpd(struct mtu3_ep *mep, struct mtu3_request *mreq) gpd->next_gpd = cpu_to_le32(lower_32_bits(enq_dma)); ext_addr |= GPD_EXT_NGP(mtu, upper_32_bits(enq_dma)); gpd->dw3_info = cpu_to_le32(ext_addr); + /* make sure GPD is fully written before giving it to HW */ + mb(); gpd->dw0_info |= cpu_to_le32(GPD_FLAGS_IOC | GPD_FLAGS_HWO); mreq->gpd = gpd; @@ -445,7 +449,8 @@ static void qmu_tx_zlp_error_handler(struct mtu3 *mtu, u8 epnum) return; } mtu3_setbits(mbase, MU3D_EP_TXCR0(mep->epnum), TX_TXPKTRDY); - + /* make sure GPD is fully written before giving it to HW */ + mb(); /* by pass the current GDP */ gpd_current->dw0_info |= cpu_to_le32(GPD_FLAGS_BPS | GPD_FLAGS_HWO);
There is a seldom issue that the controller access invalid address and trigger devapc or emimpu violation. That is due to memory access is out of order and cause gpd data is not correct. Make sure GPD is fully written before giving it to HW by setting its HWO. Fixes: 48e0d3735aa5 ("usb: mtu3: supports new QMU format") Cc: stable@vger.kernel.org Reported-by: Eddie Hung <eddie.hung@mediatek.com> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> --- drivers/usb/mtu3/mtu3_qmu.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)