RFC: Fix DMA channel and memory leak in vc4

[boatbodger]

Adding a non-Raspberry DSI display. Even with panel driver and DTB apparently correct, startup was frequently failling with a dmesg entry:
[ 4.690030] [drm:vc4_dsi_bind [vc4]] ERROR Failed to get DMA channel: mask:0001 chan:-19

The vc4/panel startup 'pas a doble' means vc4 requests EPROBE_DEFER before the panel is ready, sometimes several times.

It was found that each iteration was requesting a new DMA channel, and the system was running out.
It was also noticed that small amounts of memory were being allocated each time.
The attached patch seems to fix the second memory leak and the DMA leak.
The patch file contains a proposal for a third patch to fix the first memory leak, but when applied, breaks something, resulting in a kernel oops.
The patch as offered also includes additional debugging messages which could be removed for production.
I have tested this patch in conjunction with the very recent fixes from pelwell which also address a DMA leak.
PS: this is the first time I've done a pull request, so please forgive me if the etiquette isn't quite right...

[pelwell]

Thank you for trying to improve this code, but I think your general approach is the wrong one. If you follow the pattern I outline in the line comments you will probably find that it doesn't leak and doesn't crash.

If you have changes to submit you have two options:

1. Append a followup patch to your forked branch and it will appear here. The commits can be squashed into one at merge time. This is helpful if the patch is extensive, the changes are small and the end result will be a single commit.
2. Fix up your commits locally to look how you want them to, then force push (git push -f) to your forked branch and the whole PR will be rewritten.

[boatbodger]

I have reworked the patch on the basis of releasing any required resources if it is necessary to error out of the bind. For most memory allocs that had already been ensured by using managed variants, but there was a one which wasn't.
As no managed calls seem to exist for DMA channels, the code has been modified to ensure its explicit release on error exits.

[pelwell]

That's looking much better from a functionality point of view. The remaining changes I would like are essentially cosmetic/stylistic:

• Don't make unnecessary whitespace changes.
• The added DRM_INFO is unnecessary/undesirable in production code.
• You only ever call the release_return function in situations where the DMA channel has been claimed, so the release doesn't need to be conditional.
• Despite the old "goto considered harmful" article, using goto to branch to common error handling cleanup code at the end of a function is standard practice in Linux drivers. In the case of multiple resources which may need to be released, multiple labels are used:

        ....

        if (!claim_resource_1()) {
                ret = -EMUMBLE;
                goto err_exit;
        }

        ptr = claim_resource_2();
        if (IS_ERR(ptr)) {
                ret = PTR_ERR(ptr);
                goto err_release_1;
        }

        ret = claim_resource_3();
        if (ret)
                goto err_release_2;
...

        return 0;

err_release_2:
        free_resource_2(ptr);
err_release_1:
        free_resource_1();
err_exit:
        return ret;
}

• Although Linux is fond of using helper functions, trusting the compiler to inline what it needs to, now that the conditionality and the error-code passing is removed release_return is just dma_release_channel.

[boatbodger]

Thanks for the helpful feedback, which I believe I've reflected in this new P/R.
Happy to take on board any further points you may have, and thank you for your patience.

[boatbodger]

@pelwell I wonder if you had a chance to review the changes I made following your feedback? (I may not quite have the hang of the pull request process - I'm hoping my most recent 'commit' would have 'refreshed' the pull request for you, but I'm not totally sure...)

[pelwell]

Sorry for the delayed response - I'm temporarily reassigned and unable to test the latest patch, but it looks OK so will probably get merged early next week.

[pelwell]

[ Sorry for the abnormally slow responses - the normally slow responses should be in effect now. ]

That looks good (with a minor whitespace change). Do you have a "Signed-off-by: " line to add? If you reply with one I can add it when squashing down to a single commit.

[boatbodger]

Do you have a "Signed-off-by: " line to add? If you reply with one I can add it when squashing down to a single commit.
Signed-off-by: Chris G Miller chris@creative-electronics.net

[lategoodbye]

Would be nice to get this upstream