Hi all,
Today's linux-next merge of the dmaengine tree got a conflict in:
drivers/dma/idxd/submit.c
between commit:
8affd8a4b5ce3 ("dmaengine: idxd: fix missed completion on abort path")
from the dmaengine-fixes tree and commit:
5d78abb6fbc97 ("dmaengine: idxd: rework descriptor free path on failure")
from the dmaengine tree.
I fixed it up (see below) and can carry the fix as necessary. This
is now fixed as far as linux-next is concerned, but any non trivial
conflicts should be mentioned to your upstream maintainer when your tree
is submitted for merging. You may also want to consider cooperating
with the maintainer of the conflicting tree to minimise any particularly
complex conflicts.
diff --cc drivers/dma/idxd/submit.c
index 83452fbbb168b,569815a84e95b..0000000000000
--- a/drivers/dma/idxd/submit.c
+++ b/drivers/dma/idxd/submit.c
@@@ -134,20 -120,32 +125,43 @@@ static void llist_abort_desc(struct idx
spin_unlock(&ie->list_lock);
if (found)
- complete_desc(found, IDXD_COMPLETE_ABORT);
+ idxd_dma_complete_txd(found, IDXD_COMPLETE_ABORT, false);
+
+ /*
- * complete_desc() will return desc to allocator and the desc can be
- * acquired by a different process and the desc->list can be modified.
- * Delete desc from list so the list trasversing does not get corrupted
- * by the other process.
++ * completing the descriptor will return desc to allocator and
++ * the desc can be acquired by a different process and the
++ * desc->list can be modified. Delete desc from list so the
++ * list trasversing does not get corrupted by the other process.
+ */
+ list_for_each_entry_safe(d, t, &flist, list) {
+ list_del_init(&d->list);
- complete_desc(d, IDXD_COMPLETE_NORMAL);
++ idxd_dma_complete_txd(d, IDXD_COMPLETE_NORMAL, false);
+ }
}
+ /*
+ * ENQCMDS typically fail when the WQ is inactive or busy. On host submission, the driver
+ * has better control of number of descriptors being submitted to a shared wq by limiting
+ * the number of driver allocated descriptors to the wq size. However, when the swq is
+ * exported to a guest kernel, it may be shared with multiple guest kernels. This means
+ * the likelihood of getting busy returned on the swq when submitting goes significantly up.
+ * Having a tunable retry mechanism allows the driver to keep trying for a bit before giving
+ * up. The sysfs knob can be tuned by the system administrator.
+ */
+ int idxd_enqcmds(struct idxd_wq *wq, void __iomem *portal, const void *desc)
+ {
+ int rc, retries = 0;
+
+ do {
+ rc = enqcmds(portal, desc);
+ if (rc == 0)
+ break;
+ cpu_relax();
+ } while (retries++ < wq->enqcmds_retries);
+
+ return rc;
+ }
+
int idxd_submit_desc(struct idxd_wq *wq, struct idxd_desc *desc)
{
struct idxd_device *idxd = wq->idxd;
On Wed, Dec 15, 2021 at 10:53 AM <[email protected]> wrote:
> Today's linux-next merge of the dmaengine tree got a conflict in:
>
> drivers/dma/idxd/submit.c
>
> between commit:
>
> 8affd8a4b5ce3 ("dmaengine: idxd: fix missed completion on abort path")
>
> from the dmaengine-fixes tree and commit:
>
> 5d78abb6fbc97 ("dmaengine: idxd: rework descriptor free path on failure")
>
> from the dmaengine tree.
>
> I fixed it up (see below) and can carry the fix as necessary. This
> is now fixed as far as linux-next is concerned, but any non trivial
> conflicts should be mentioned to your upstream maintainer when your tree
> is submitted for merging. You may also want to consider cooperating
> with the maintainer of the conflicting tree to minimise any particularly
> complex conflicts.
>
> diff --cc drivers/dma/idxd/submit.c
> index 83452fbbb168b,569815a84e95b..0000000000000
> --- a/drivers/dma/idxd/submit.c
> +++ b/drivers/dma/idxd/submit.c
> @@@ -134,20 -120,32 +125,43 @@@ static void llist_abort_desc(struct idx
> spin_unlock(&ie->list_lock);
>
> if (found)
> - complete_desc(found, IDXD_COMPLETE_ABORT);
> + idxd_dma_complete_txd(found, IDXD_COMPLETE_ABORT, false);
> +
> + /*
> - * complete_desc() will return desc to allocator and the desc can be
> - * acquired by a different process and the desc->list can be modified.
> - * Delete desc from list so the list trasversing does not get corrupted
> - * by the other process.
> ++ * completing the descriptor will return desc to allocator and
> ++ * the desc can be acquired by a different process and the
> ++ * desc->list can be modified. Delete desc from list so the
> ++ * list trasversing does not get corrupted by the other process.
traversing
> + */
> + list_for_each_entry_safe(d, t, &flist, list) {
> + list_del_init(&d->list);
> - complete_desc(d, IDXD_COMPLETE_NORMAL);
> ++ idxd_dma_complete_txd(d, IDXD_COMPLETE_NORMAL, false);
Is "false" correct here?
> + }
> }
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
> On Dec 28, 2021, at 2:09 AM, Geert Uytterhoeven <[email protected]> wrote:
>
> On Wed, Dec 15, 2021 at 10:53 AM <[email protected]> wrote:
>> Today's linux-next merge of the dmaengine tree got a conflict in:
>>
>> drivers/dma/idxd/submit.c
>>
>> between commit:
>>
>> 8affd8a4b5ce3 ("dmaengine: idxd: fix missed completion on abort path")
>>
>> from the dmaengine-fixes tree and commit:
>>
>> 5d78abb6fbc97 ("dmaengine: idxd: rework descriptor free path on failure")
>>
>> from the dmaengine tree.
>>
>> I fixed it up (see below) and can carry the fix as necessary. This
>> is now fixed as far as linux-next is concerned, but any non trivial
>> conflicts should be mentioned to your upstream maintainer when your tree
>> is submitted for merging. You may also want to consider cooperating
>> with the maintainer of the conflicting tree to minimise any particularly
>> complex conflicts.
>>
>> diff --cc drivers/dma/idxd/submit.c
>> index 83452fbbb168b,569815a84e95b..0000000000000
>> --- a/drivers/dma/idxd/submit.c
>> +++ b/drivers/dma/idxd/submit.c
>> @@@ -134,20 -120,32 +125,43 @@@ static void llist_abort_desc(struct idx
>> spin_unlock(&ie->list_lock);
>>
>> if (found)
>> - complete_desc(found, IDXD_COMPLETE_ABORT);
>> + idxd_dma_complete_txd(found, IDXD_COMPLETE_ABORT, false);
>> +
>> + /*
>> - * complete_desc() will return desc to allocator and the desc can be
>> - * acquired by a different process and the desc->list can be modified.
>> - * Delete desc from list so the list trasversing does not get corrupted
>> - * by the other process.
>> ++ * completing the descriptor will return desc to allocator and
>> ++ * the desc can be acquired by a different process and the
>> ++ * desc->list can be modified. Delete desc from list so the
>> ++ * list trasversing does not get corrupted by the other process.
>
> traversing
>
>> + */
>> + list_for_each_entry_safe(d, t, &flist, list) {
>> + list_del_init(&d->list);
>> - complete_desc(d, IDXD_COMPLETE_NORMAL);
>> ++ idxd_dma_complete_txd(d, IDXD_COMPLETE_NORMAL, false);
>
> Is "false" correct here?
Yes that’s correct. Thanks.
>
>> + }
>> }
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
On 12/28/2021 2:09 AM, Geert Uytterhoeven wrote:
> On Wed, Dec 15, 2021 at 10:53 AM <[email protected]> wrote:
>> Today's linux-next merge of the dmaengine tree got a conflict in:
>>
>> drivers/dma/idxd/submit.c
>>
>> between commit:
>>
>> 8affd8a4b5ce3 ("dmaengine: idxd: fix missed completion on abort path")
>>
>> from the dmaengine-fixes tree and commit:
>>
>> 5d78abb6fbc97 ("dmaengine: idxd: rework descriptor free path on failure")
>>
>> from the dmaengine tree.
>>
>> I fixed it up (see below) and can carry the fix as necessary. This
>> is now fixed as far as linux-next is concerned, but any non trivial
>> conflicts should be mentioned to your upstream maintainer when your tree
>> is submitted for merging. You may also want to consider cooperating
>> with the maintainer of the conflicting tree to minimise any particularly
>> complex conflicts.
>>
>> diff --cc drivers/dma/idxd/submit.c
>> index 83452fbbb168b,569815a84e95b..0000000000000
>> --- a/drivers/dma/idxd/submit.c
>> +++ b/drivers/dma/idxd/submit.c
>> @@@ -134,20 -120,32 +125,43 @@@ static void llist_abort_desc(struct idx
>> spin_unlock(&ie->list_lock);
>>
>> if (found)
>> - complete_desc(found, IDXD_COMPLETE_ABORT);
>> + idxd_dma_complete_txd(found, IDXD_COMPLETE_ABORT, false);
>> +
>> + /*
>> - * complete_desc() will return desc to allocator and the desc can be
>> - * acquired by a different process and the desc->list can be modified.
>> - * Delete desc from list so the list trasversing does not get corrupted
>> - * by the other process.
>> ++ * completing the descriptor will return desc to allocator and
>> ++ * the desc can be acquired by a different process and the
>> ++ * desc->list can be modified. Delete desc from list so the
>> ++ * list trasversing does not get corrupted by the other process.
> traversing
>
>> + */
>> + list_for_each_entry_safe(d, t, &flist, list) {
>> + list_del_init(&d->list);
>> - complete_desc(d, IDXD_COMPLETE_NORMAL);
>> ++ idxd_dma_complete_txd(d, IDXD_COMPLETE_NORMAL, false);
> Is "false" correct here?
Hi Geert, took a closer look today. I believe it should be 'true' here
since this is a normal completion that needs to release the descriptors.
Sorry about the previous incorrect response.
>
>> + }
>> }
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
Hi Dave,
On Tue, 4 Jan 2022 14:41:00 -0700 Dave Jiang <[email protected]> wrote:
>
> >> + */
> >> + list_for_each_entry_safe(d, t, &flist, list) {
> >> + list_del_init(&d->list);
> >> - complete_desc(d, IDXD_COMPLETE_NORMAL);
> >> ++ idxd_dma_complete_txd(d, IDXD_COMPLETE_NORMAL, false);
> > Is "false" correct here?
>
> Hi Geert, took a closer look today. I believe it should be 'true'
> here since this is a normal completion that needs to release the
> descriptors. Sorry about the previous incorrect response.
I have updated my resolution from today. Thanks for the feedback.
--
Cheers,
Stephen Rothwell
On 05-01-22, 10:04, Stephen Rothwell wrote:
> Hi Dave,
>
> On Tue, 4 Jan 2022 14:41:00 -0700 Dave Jiang <[email protected]> wrote:
> >
> > >> + */
> > >> + list_for_each_entry_safe(d, t, &flist, list) {
> > >> + list_del_init(&d->list);
> > >> - complete_desc(d, IDXD_COMPLETE_NORMAL);
> > >> ++ idxd_dma_complete_txd(d, IDXD_COMPLETE_NORMAL, false);
> > > Is "false" correct here?
> >
> > Hi Geert, took a closer look today. I believe it should be 'true'
> > here since this is a normal completion that needs to release the
> > descriptors. Sorry about the previous incorrect response.
>
> I have updated my resolution from today. Thanks for the feedback.
I have merged fixes into next as well, so this should not be required
tomorrow.
Dave pls test..
--
~Vinod
On 1/5/2022 12:19 AM, Vinod Koul wrote:
> On 05-01-22, 10:04, Stephen Rothwell wrote:
>> Hi Dave,
>>
>> On Tue, 4 Jan 2022 14:41:00 -0700 Dave Jiang <[email protected]> wrote:
>>>>> + */
>>>>> + list_for_each_entry_safe(d, t, &flist, list) {
>>>>> + list_del_init(&d->list);
>>>>> - complete_desc(d, IDXD_COMPLETE_NORMAL);
>>>>> ++ idxd_dma_complete_txd(d, IDXD_COMPLETE_NORMAL, false);
>>>> Is "false" correct here?
>>> Hi Geert, took a closer look today. I believe it should be 'true'
>>> here since this is a normal completion that needs to release the
>>> descriptors. Sorry about the previous incorrect response.
>> I have updated my resolution from today. Thanks for the feedback.
> I have merged fixes into next as well, so this should not be required
> tomorrow.
>
> Dave pls test..
Tested. Looks good. Thanks.
On 12-04-22, 12:42, Stephen Rothwell wrote:
> Hi all,
>
> Today's linux-next merge of the dmaengine tree got a conflict in:
>
> drivers/dma/mediatek/mtk-hsdma.c
>
> between commit:
>
> 5cfde5b82f05 ("dmaengine: mediatek: mtk-hsdma: Use platform_get_irq() to get the interrupt")
>
> from the dmaengine-fixes tree and commit:
>
> bb40bb695ec8 ("dmaengine: mediatek: mtk-hsdma: Use platform_get_irq() to get the interrupt")
>
> from the dmaengine tree.
Thanks, this should not be in fixes, I have fixed that up.
>
> I fixed it up (I arbitrarily used the former) and can carry the fix as
> necessary. This is now fixed as far as linux-next is concerned, but any
> non trivial conflicts should be mentioned to your upstream maintainer
> when your tree is submitted for merging. You may also want to consider
> cooperating with the maintainer of the conflicting tree to minimise any
> particularly complex conflicts.
>
> --
> Cheers,
> Stephen Rothwell
--
~Vinod