| Message ID | 20230403025230.25035-3-chunfeng.yun@mediatek.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/7] usb: mtu3: give back request when rx error happens | expand |
Il 03/04/23 04:52, Chunfeng Yun ha scritto: > When handle qmu transfer irq, it will unlock @mtu->lock before give back > request, if another thread hanlde disconnect event at the same time, and > try to disable ep, it may lock @mtu->lock and free qmu ring, then qmu > irq hanlder may get a NULL gpd, avoid the KE by checking gpd's value before KE == Kernel Error? I think you wanted to say KP == Kernel Panic instead. Also, s/hanlder/handler/g. > handling it. > > e.g. > qmu done irq on cpu0 thread running on cpu1 > > qmu_done_tx() > handle gpd [0] > mtu3_requ_complete() mtu3_gadget_ep_disable() > unlock @mtu->lock > give back request lock @mtu->lock > mtu3_ep_disable() > mtu3_gpd_ring_free() > unlock @mtu->lock > lock @mtu->lock > get next gpd [1] > > [1]: goto [0] to handle next gpd, and next gpd may be NULL. > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> This is a fix and needs a Fixes tag. Regards, Angelo
On Mon, 2023-04-03 at 14:31 +0200, AngeloGioacchino Del Regno wrote: > External email : Please do not click links or open attachments until > you have verified the sender or the content. > > > Il 03/04/23 04:52, Chunfeng Yun ha scritto: > > When handle qmu transfer irq, it will unlock @mtu->lock before give > > back > > request, if another thread hanlde disconnect event at the same > > time, and > > try to disable ep, it may lock @mtu->lock and free qmu ring, then > > qmu > > irq hanlder may get a NULL gpd, avoid the KE by checking gpd's > > value before > > KE == Kernel Error? I think you wanted to say KP == Kernel Panic > instead. > > Also, s/hanlder/handler/g. Ok, will modify it > > > handling it. > > > > e.g. > > qmu done irq on cpu0 thread running on cpu1 > > > > qmu_done_tx() > > handle gpd [0] > > mtu3_requ_complete() mtu3_gadget_ep_disable() > > unlock @mtu->lock > > give back request lock @mtu->lock > > mtu3_ep_disable() > > mtu3_gpd_ring_free() > > unlock @mtu->lock > > lock @mtu->lock > > get next gpd [1] > > > > [1]: goto [0] to handle next gpd, and next gpd may be NULL. > > > > > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> > > This is a fix and needs a Fixes tag. I usually add Fixes tag when the issue introduced by a patch except the original one when the driver applied. Thanks a lot > > Regards, > Angelo > >
Il 07/04/23 09:59, Chunfeng Yun (云春峰) ha scritto: > On Mon, 2023-04-03 at 14:31 +0200, AngeloGioacchino Del Regno wrote: >> External email : Please do not click links or open attachments until >> you have verified the sender or the content. >> >> >> Il 03/04/23 04:52, Chunfeng Yun ha scritto: >>> When handle qmu transfer irq, it will unlock @mtu->lock before give >>> back >>> request, if another thread hanlde disconnect event at the same >>> time, and >>> try to disable ep, it may lock @mtu->lock and free qmu ring, then >>> qmu >>> irq hanlder may get a NULL gpd, avoid the KE by checking gpd's >>> value before >> >> KE == Kernel Error? I think you wanted to say KP == Kernel Panic >> instead. >> >> Also, s/hanlder/handler/g. > Ok, will modify it >> >>> handling it. >>> >>> e.g. >>> qmu done irq on cpu0 thread running on cpu1 >>> >>> qmu_done_tx() >>> handle gpd [0] >>> mtu3_requ_complete() mtu3_gadget_ep_disable() >>> unlock @mtu->lock >>> give back request lock @mtu->lock >>> mtu3_ep_disable() >>> mtu3_gpd_ring_free() >>> unlock @mtu->lock >>> lock @mtu->lock >>> get next gpd [1] >>> >>> [1]: goto [0] to handle next gpd, and next gpd may be NULL. >> >> > >> > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> >> >> This is a fix and needs a Fixes tag. > I usually add Fixes tag when the issue introduced by a patch except the > original one when the driver applied. > If this patch is a fix for the "original driver", you shall still add a Fixes tag which advertises that this fixes the first commit, so the driver was broken from the very beginning. Thanks, Angelo.
diff --git a/drivers/usb/mtu3/mtu3_qmu.c b/drivers/usb/mtu3/mtu3_qmu.c index 66639f602a9d..3146a112141f 100644 --- a/drivers/usb/mtu3/mtu3_qmu.c +++ b/drivers/usb/mtu3/mtu3_qmu.c @@ -210,6 +210,7 @@ static struct qmu_gpd *advance_enq_gpd(struct mtu3_gpd_ring *ring) return ring->enqueue; } +/* @dequeue may be NULL if ring is unallocated or freed */ static struct qmu_gpd *advance_deq_gpd(struct mtu3_gpd_ring *ring) { if (ring->dequeue < ring->end) @@ -524,7 +525,7 @@ static void qmu_done_tx(struct mtu3 *mtu, u8 epnum) dev_dbg(mtu->dev, "%s EP%d, last=%p, current=%p, enq=%p\n", __func__, epnum, gpd, gpd_current, ring->enqueue); - while (gpd != gpd_current && !GET_GPD_HWO(gpd)) { + while (gpd && gpd != gpd_current && !GET_GPD_HWO(gpd)) { mreq = next_request(mep); @@ -563,7 +564,7 @@ static void qmu_done_rx(struct mtu3 *mtu, u8 epnum) dev_dbg(mtu->dev, "%s EP%d, last=%p, current=%p, enq=%p\n", __func__, epnum, gpd, gpd_current, ring->enqueue); - while (gpd != gpd_current && !GET_GPD_HWO(gpd)) { + while (gpd && gpd != gpd_current && !GET_GPD_HWO(gpd)) { mreq = next_request(mep);
When handle qmu transfer irq, it will unlock @mtu->lock before give back request, if another thread hanlde disconnect event at the same time, and try to disable ep, it may lock @mtu->lock and free qmu ring, then qmu irq hanlder may get a NULL gpd, avoid the KE by checking gpd's value before handling it. e.g. qmu done irq on cpu0 thread running on cpu1 qmu_done_tx() handle gpd [0] mtu3_requ_complete() mtu3_gadget_ep_disable() unlock @mtu->lock give back request lock @mtu->lock mtu3_ep_disable() mtu3_gpd_ring_free() unlock @mtu->lock lock @mtu->lock get next gpd [1] [1]: goto [0] to handle next gpd, and next gpd may be NULL. Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com> --- drivers/usb/mtu3/mtu3_qmu.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)