| Message ID | 20220519074351.829774-1-william.xuanziyang@huawei.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v3] net: wwan: t7xx: fix GFP_KERNEL usage in spin_lock context | expand |
On Thu, 19 May 2022 at 09:26, Ziyang Xuan <william.xuanziyang@huawei.com> wrote: > > t7xx_cldma_clear_rxq() call t7xx_cldma_alloc_and_map_skb() in spin_lock > context, But __dev_alloc_skb() in t7xx_cldma_alloc_and_map_skb() uses > GFP_KERNEL, that will introduce scheduling factor in spin_lock context. > > Because t7xx_cldma_clear_rxq() is called after stopping CLDMA, so we can > remove the spin_lock from t7xx_cldma_clear_rxq(). > > Fixes: 39d439047f1d ("net: wwan: t7xx: Add control DMA interface") > Suggested-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> > Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> Reviewed-by: Loic Poulain <loic.poulain@linaro.org> > --- > drivers/net/wwan/t7xx/t7xx_hif_cldma.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > --- > v3: > - Add Suggested-by and simplify comments > v2: > - Remove spin_lock instead of using GFP_ATOMIC > > diff --git a/drivers/net/wwan/t7xx/t7xx_hif_cldma.c b/drivers/net/wwan/t7xx/t7xx_hif_cldma.c > index 46066dcd2607..3a46a5bea24f 100644 > --- a/drivers/net/wwan/t7xx/t7xx_hif_cldma.c > +++ b/drivers/net/wwan/t7xx/t7xx_hif_cldma.c > @@ -782,10 +782,11 @@ static int t7xx_cldma_clear_rxq(struct cldma_ctrl *md_ctrl, int qnum) > struct cldma_queue *rxq = &md_ctrl->rxq[qnum]; > struct cldma_request *req; > struct cldma_gpd *gpd; > - unsigned long flags; > int ret = 0; > > - spin_lock_irqsave(&rxq->ring_lock, flags); > + /* CLDMA has been stopped. There is not any CLDMA IRQ, holding > + * ring_lock is not needed. > + */ > t7xx_cldma_q_reset(rxq); > list_for_each_entry(req, &rxq->tr_ring->gpd_ring, entry) { > gpd = req->gpd; > @@ -808,7 +809,6 @@ static int t7xx_cldma_clear_rxq(struct cldma_ctrl *md_ctrl, int qnum) > > t7xx_cldma_gpd_set_data_ptr(req->gpd, req->mapped_buff); > } > - spin_unlock_irqrestore(&rxq->ring_lock, flags); > > return ret; > } > -- > 2.25.1 >
On Thu, 19 May 2022 09:29:12 +0200 Loic Poulain wrote: > On Thu, 19 May 2022 at 09:26, Ziyang Xuan <william.xuanziyang@huawei.com> wrote: > > > > t7xx_cldma_clear_rxq() call t7xx_cldma_alloc_and_map_skb() in spin_lock > > context, But __dev_alloc_skb() in t7xx_cldma_alloc_and_map_skb() uses > > GFP_KERNEL, that will introduce scheduling factor in spin_lock context. > > > > Because t7xx_cldma_clear_rxq() is called after stopping CLDMA, so we can > > remove the spin_lock from t7xx_cldma_clear_rxq(). > > > > Fixes: 39d439047f1d ("net: wwan: t7xx: Add control DMA interface") > > Suggested-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> > > Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> > > Reviewed-by: Loic Poulain <loic.poulain@linaro.org> Wait, you reviewed two different fixes for the same issue? Please say something when that happens I thought both are needed :/
On Fri, 20 May 2022 17:25:56 -0700 Jakub Kicinski wrote: > On Thu, 19 May 2022 09:29:12 +0200 Loic Poulain wrote: > > On Thu, 19 May 2022 at 09:26, Ziyang Xuan <william.xuanziyang@huawei.com> wrote: > > > > > > t7xx_cldma_clear_rxq() call t7xx_cldma_alloc_and_map_skb() in spin_lock > > > context, But __dev_alloc_skb() in t7xx_cldma_alloc_and_map_skb() uses > > > GFP_KERNEL, that will introduce scheduling factor in spin_lock context. > > > > > > Because t7xx_cldma_clear_rxq() is called after stopping CLDMA, so we can > > > remove the spin_lock from t7xx_cldma_clear_rxq(). > > > > > > Fixes: 39d439047f1d ("net: wwan: t7xx: Add control DMA interface") > > > Suggested-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> > > > Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> > > > > Reviewed-by: Loic Poulain <loic.poulain@linaro.org> > > Wait, you reviewed two different fixes for the same issue? > Please say something when that happens I thought both are needed :/ FWIW I pushed out the other one before I realized (they both apply without conflicts so I thought they fixed different issues) If this one is preferred please respin and squash a revert of 9ee152ee3ee3 into it.
Le sam. 21 mai 2022 à 03:01, Jakub Kicinski <kuba@kernel.org> a écrit : > > On Fri, 20 May 2022 17:25:56 -0700 Jakub Kicinski wrote: > > On Thu, 19 May 2022 09:29:12 +0200 Loic Poulain wrote: > > > On Thu, 19 May 2022 at 09:26, Ziyang Xuan <william.xuanziyang@huawei.com> wrote: > > > > > > > > t7xx_cldma_clear_rxq() call t7xx_cldma_alloc_and_map_skb() in spin_lock > > > > context, But __dev_alloc_skb() in t7xx_cldma_alloc_and_map_skb() uses > > > > GFP_KERNEL, that will introduce scheduling factor in spin_lock context. > > > > > > > > Because t7xx_cldma_clear_rxq() is called after stopping CLDMA, so we can > > > > remove the spin_lock from t7xx_cldma_clear_rxq(). > > > > > > > > Fixes: 39d439047f1d ("net: wwan: t7xx: Add control DMA interface") > > > > Suggested-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> > > > > Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> > > > > > > Reviewed-by: Loic Poulain <loic.poulain@linaro.org> > > > > Wait, you reviewed two different fixes for the same issue? > > Please say something when that happens I thought both are needed :/ Right, I've actually overlooked that the other patch has only one atomic user, which becomes useless with this change. > > > FWIW I pushed out the other one before I realized (they both apply > without conflicts so I thought they fixed different issues) > If this one is preferred please respin and squash a revert of > > > 9ee152ee3ee3 into it. Yes this one is preferred, I'll respin it. Sorry for this. Thanks, Loic
diff --git a/drivers/net/wwan/t7xx/t7xx_hif_cldma.c b/drivers/net/wwan/t7xx/t7xx_hif_cldma.c index 46066dcd2607..3a46a5bea24f 100644 --- a/drivers/net/wwan/t7xx/t7xx_hif_cldma.c +++ b/drivers/net/wwan/t7xx/t7xx_hif_cldma.c @@ -782,10 +782,11 @@ static int t7xx_cldma_clear_rxq(struct cldma_ctrl *md_ctrl, int qnum) struct cldma_queue *rxq = &md_ctrl->rxq[qnum]; struct cldma_request *req; struct cldma_gpd *gpd; - unsigned long flags; int ret = 0; - spin_lock_irqsave(&rxq->ring_lock, flags); + /* CLDMA has been stopped. There is not any CLDMA IRQ, holding + * ring_lock is not needed. + */ t7xx_cldma_q_reset(rxq); list_for_each_entry(req, &rxq->tr_ring->gpd_ring, entry) { gpd = req->gpd; @@ -808,7 +809,6 @@ static int t7xx_cldma_clear_rxq(struct cldma_ctrl *md_ctrl, int qnum) t7xx_cldma_gpd_set_data_ptr(req->gpd, req->mapped_buff); } - spin_unlock_irqrestore(&rxq->ring_lock, flags); return ret; }
t7xx_cldma_clear_rxq() call t7xx_cldma_alloc_and_map_skb() in spin_lock context, But __dev_alloc_skb() in t7xx_cldma_alloc_and_map_skb() uses GFP_KERNEL, that will introduce scheduling factor in spin_lock context. Because t7xx_cldma_clear_rxq() is called after stopping CLDMA, so we can remove the spin_lock from t7xx_cldma_clear_rxq(). Fixes: 39d439047f1d ("net: wwan: t7xx: Add control DMA interface") Suggested-by: Ricardo Martinez <ricardo.martinez@linux.intel.com> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> --- drivers/net/wwan/t7xx/t7xx_hif_cldma.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) --- v3: - Add Suggested-by and simplify comments v2: - Remove spin_lock instead of using GFP_ATOMIC