| Message ID | 20211125090028.786832-9-tudor.ambarus@microchip.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dmaengine: at_xdmac: Various fixes | expand |
On 25-11-21, 11:00, Tudor Ambarus wrote: > So that we don't use the same desc over and over again. Please use full para in the changelog and not a continuation of the patch title! and why is wrong with using same desc over and over? Any benefits of not doing so? > > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> > --- > drivers/dma/at_xdmac.c | 23 ++++++++++++++--------- > 1 file changed, 14 insertions(+), 9 deletions(-) > > diff --git a/drivers/dma/at_xdmac.c b/drivers/dma/at_xdmac.c > index 2cc9af222681..8804a86a9bcc 100644 > --- a/drivers/dma/at_xdmac.c > +++ b/drivers/dma/at_xdmac.c > @@ -729,7 +729,8 @@ at_xdmac_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl, > if (!desc) { > dev_err(chan2dev(chan), "can't get descriptor\n"); > if (first) > - list_splice_init(&first->descs_list, &atchan->free_descs_list); > + list_splice_tail_init(&first->descs_list, > + &atchan->free_descs_list); > goto spin_unlock; > } > > @@ -817,7 +818,8 @@ at_xdmac_prep_dma_cyclic(struct dma_chan *chan, dma_addr_t buf_addr, > if (!desc) { > dev_err(chan2dev(chan), "can't get descriptor\n"); > if (first) > - list_splice_init(&first->descs_list, &atchan->free_descs_list); > + list_splice_tail_init(&first->descs_list, > + &atchan->free_descs_list); > spin_unlock_irqrestore(&atchan->lock, irqflags); > return NULL; > } > @@ -1051,8 +1053,8 @@ at_xdmac_prep_interleaved(struct dma_chan *chan, > src_addr, dst_addr, > xt, chunk); > if (!desc) { > - list_splice_init(&first->descs_list, > - &atchan->free_descs_list); > + list_splice_tail_init(&first->descs_list, > + &atchan->free_descs_list); > return NULL; > } > > @@ -1132,7 +1134,8 @@ at_xdmac_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest, dma_addr_t src, > if (!desc) { > dev_err(chan2dev(chan), "can't get descriptor\n"); > if (first) > - list_splice_init(&first->descs_list, &atchan->free_descs_list); > + list_splice_tail_init(&first->descs_list, > + &atchan->free_descs_list); > return NULL; > } > > @@ -1308,8 +1311,8 @@ at_xdmac_prep_dma_memset_sg(struct dma_chan *chan, struct scatterlist *sgl, > sg_dma_len(sg), > value); > if (!desc && first) > - list_splice_init(&first->descs_list, > - &atchan->free_descs_list); > + list_splice_tail_init(&first->descs_list, > + &atchan->free_descs_list); > > if (!first) > first = desc; > @@ -1701,7 +1704,8 @@ static void at_xdmac_tasklet(struct tasklet_struct *t) > > spin_lock_irq(&atchan->lock); > /* Move the xfer descriptors into the free descriptors list. */ > - list_splice_init(&desc->descs_list, &atchan->free_descs_list); > + list_splice_tail_init(&desc->descs_list, > + &atchan->free_descs_list); > at_xdmac_advance_work(atchan); > spin_unlock_irq(&atchan->lock); > } > @@ -1850,7 +1854,8 @@ static int at_xdmac_device_terminate_all(struct dma_chan *chan) > /* Cancel all pending transfers. */ > list_for_each_entry_safe(desc, _desc, &atchan->xfers_list, xfer_node) { > list_del(&desc->xfer_node); > - list_splice_init(&desc->descs_list, &atchan->free_descs_list); > + list_splice_tail_init(&desc->descs_list, > + &atchan->free_descs_list); > } > > clear_bit(AT_XDMAC_CHAN_IS_PAUSED, &atchan->status); > -- > 2.25.1
Hi, Vinod, On 12/13/21 10:07 AM, Vinod Koul wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 25-11-21, 11:00, Tudor Ambarus wrote: >> So that we don't use the same desc over and over again. > > Please use full para in the changelog and not a continuation of the > patch title! Ok, will add a better commit description. Here and in other patches where your comment applies. > > and why is wrong with using same desc over and over? Any benefits of not > doing so? Not wrong, but if we move the free desc to the tail of the list, then the sequence of descriptors is more track-able in case of debug. You would know which descriptor should come next and you could easier catch concurrency over descriptors for example. I saw virt-dma uses list_splice_tail_init() as well, I found it a good idea, so I thought to follow the core driver. Cheers, ta
On 13-12-21, 08:51, Tudor.Ambarus@microchip.com wrote: > Hi, Vinod, > > On 12/13/21 10:07 AM, Vinod Koul wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > On 25-11-21, 11:00, Tudor Ambarus wrote: > >> So that we don't use the same desc over and over again. > > > > Please use full para in the changelog and not a continuation of the > > patch title! > > Ok, will add a better commit description. Here and in other patches where > your comment applies. Great! > > > > and why is wrong with using same desc over and over? Any benefits of not > > doing so? > > Not wrong, but if we move the free desc to the tail of the list, then the > sequence of descriptors is more track-able in case of debug. You would > know which descriptor should come next and you could easier catch > concurrency over descriptors for example. I saw virt-dma uses > list_splice_tail_init() as well, I found it a good idea, so I thought to > follow the core driver. Okay, I would be good to add this motivation in the change log. I am sure after few you would also wonder why you did this change :)
On 13-12-21, 14:29, Vinod Koul wrote: > On 13-12-21, 08:51, Tudor.Ambarus@microchip.com wrote: > > Hi, Vinod, > > > > On 12/13/21 10:07 AM, Vinod Koul wrote: > > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > > > On 25-11-21, 11:00, Tudor Ambarus wrote: > > >> So that we don't use the same desc over and over again. > > > > > > Please use full para in the changelog and not a continuation of the > > > patch title! > > > > Ok, will add a better commit description. Here and in other patches where > > your comment applies. > > Great! > > > > > > > and why is wrong with using same desc over and over? Any benefits of not > > > doing so? > > > > Not wrong, but if we move the free desc to the tail of the list, then the > > sequence of descriptors is more track-able in case of debug. You would > > know which descriptor should come next and you could easier catch > > concurrency over descriptors for example. I saw virt-dma uses > > list_splice_tail_init() as well, I found it a good idea, so I thought to > > follow the core driver. > > Okay, I would be good to add this motivation in the change log. I am > sure after few you would also wonder why you did this change :) Also, pls submit serial patches to Greg separately. I guess he saw the title and overlooked those...
On 12/13/21 11:00 AM, Vinod Koul wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 13-12-21, 14:29, Vinod Koul wrote: >> On 13-12-21, 08:51, Tudor.Ambarus@microchip.com wrote: >>> Hi, Vinod, >>> >>> On 12/13/21 10:07 AM, Vinod Koul wrote: >>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>>> >>>> On 25-11-21, 11:00, Tudor Ambarus wrote: >>>>> So that we don't use the same desc over and over again. >>>> >>>> Please use full para in the changelog and not a continuation of the >>>> patch title! >>> >>> Ok, will add a better commit description. Here and in other patches where >>> your comment applies. >> >> Great! >> >>>> >>>> and why is wrong with using same desc over and over? Any benefits of not >>>> doing so? >>> >>> Not wrong, but if we move the free desc to the tail of the list, then the >>> sequence of descriptors is more track-able in case of debug. You would >>> know which descriptor should come next and you could easier catch >>> concurrency over descriptors for example. I saw virt-dma uses >>> list_splice_tail_init() as well, I found it a good idea, so I thought to >>> follow the core driver. >> >> Okay, I would be good to add this motivation in the change log. I am >> sure after few you would also wonder why you did this change :) Sure. > > Also, pls submit serial patches to Greg separately. I guess he saw the > title and overlooked those... I received a private message from Greg informing me that he applied the patches to tty-next and that they will be merged during the merge window. So I'll drop the tty patches in v3. Cheers, ta
On 12/13/21 11:22 AM, Tudor Ambarus wrote: > On 12/13/21 11:00 AM, Vinod Koul wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >> >> On 13-12-21, 14:29, Vinod Koul wrote: >>> On 13-12-21, 08:51, Tudor.Ambarus@microchip.com wrote: >>>> Hi, Vinod, >>>> >>>> On 12/13/21 10:07 AM, Vinod Koul wrote: >>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>>>> >>>>> On 25-11-21, 11:00, Tudor Ambarus wrote: >>>>>> So that we don't use the same desc over and over again. >>>>> >>>>> Please use full para in the changelog and not a continuation of the >>>>> patch title! >>>> >>>> Ok, will add a better commit description. Here and in other patches where >>>> your comment applies. >>> >>> Great! >>> >>>>> >>>>> and why is wrong with using same desc over and over? Any benefits of not >>>>> doing so? >>>> >>>> Not wrong, but if we move the free desc to the tail of the list, then the >>>> sequence of descriptors is more track-able in case of debug. You would >>>> know which descriptor should come next and you could easier catch >>>> concurrency over descriptors for example. I saw virt-dma uses >>>> list_splice_tail_init() as well, I found it a good idea, so I thought to >>>> follow the core driver. >>> >>> Okay, I would be good to add this motivation in the change log. I am >>> sure after few you would also wonder why you did this change :) > > Sure. > >> >> Also, pls submit serial patches to Greg separately. I guess he saw the >> title and overlooked those... > > I received a private message from Greg informing me that he applied the for clarity: Greg applied just the 2 tty patches, not the entire series. > patches to tty-next and that they will be merged during the merge window. > So I'll drop the tty patches in v3. v3 will follow and it will contain just the at_xdmac patches.
diff --git a/drivers/dma/at_xdmac.c b/drivers/dma/at_xdmac.c index 2cc9af222681..8804a86a9bcc 100644 --- a/drivers/dma/at_xdmac.c +++ b/drivers/dma/at_xdmac.c @@ -729,7 +729,8 @@ at_xdmac_prep_slave_sg(struct dma_chan *chan, struct scatterlist *sgl, if (!desc) { dev_err(chan2dev(chan), "can't get descriptor\n"); if (first) - list_splice_init(&first->descs_list, &atchan->free_descs_list); + list_splice_tail_init(&first->descs_list, + &atchan->free_descs_list); goto spin_unlock; } @@ -817,7 +818,8 @@ at_xdmac_prep_dma_cyclic(struct dma_chan *chan, dma_addr_t buf_addr, if (!desc) { dev_err(chan2dev(chan), "can't get descriptor\n"); if (first) - list_splice_init(&first->descs_list, &atchan->free_descs_list); + list_splice_tail_init(&first->descs_list, + &atchan->free_descs_list); spin_unlock_irqrestore(&atchan->lock, irqflags); return NULL; } @@ -1051,8 +1053,8 @@ at_xdmac_prep_interleaved(struct dma_chan *chan, src_addr, dst_addr, xt, chunk); if (!desc) { - list_splice_init(&first->descs_list, - &atchan->free_descs_list); + list_splice_tail_init(&first->descs_list, + &atchan->free_descs_list); return NULL; } @@ -1132,7 +1134,8 @@ at_xdmac_prep_dma_memcpy(struct dma_chan *chan, dma_addr_t dest, dma_addr_t src, if (!desc) { dev_err(chan2dev(chan), "can't get descriptor\n"); if (first) - list_splice_init(&first->descs_list, &atchan->free_descs_list); + list_splice_tail_init(&first->descs_list, + &atchan->free_descs_list); return NULL; } @@ -1308,8 +1311,8 @@ at_xdmac_prep_dma_memset_sg(struct dma_chan *chan, struct scatterlist *sgl, sg_dma_len(sg), value); if (!desc && first) - list_splice_init(&first->descs_list, - &atchan->free_descs_list); + list_splice_tail_init(&first->descs_list, + &atchan->free_descs_list); if (!first) first = desc; @@ -1701,7 +1704,8 @@ static void at_xdmac_tasklet(struct tasklet_struct *t) spin_lock_irq(&atchan->lock); /* Move the xfer descriptors into the free descriptors list. */ - list_splice_init(&desc->descs_list, &atchan->free_descs_list); + list_splice_tail_init(&desc->descs_list, + &atchan->free_descs_list); at_xdmac_advance_work(atchan); spin_unlock_irq(&atchan->lock); } @@ -1850,7 +1854,8 @@ static int at_xdmac_device_terminate_all(struct dma_chan *chan) /* Cancel all pending transfers. */ list_for_each_entry_safe(desc, _desc, &atchan->xfers_list, xfer_node) { list_del(&desc->xfer_node); - list_splice_init(&desc->descs_list, &atchan->free_descs_list); + list_splice_tail_init(&desc->descs_list, + &atchan->free_descs_list); } clear_bit(AT_XDMAC_CHAN_IS_PAUSED, &atchan->status);
So that we don't use the same desc over and over again. Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com> --- drivers/dma/at_xdmac.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)