Simplify busdma tag creation

[scottl]

Summary

Introduce the concept of busdma tag templates. A template can be allocated
off the stack, initialized to default values, and then filled in with
driver-specific values, all without having to worry about the numerous
other fields in the tag. The resulting template is then passed into
busdma and and the normal opaque tag object created.

One question is whether the logic for using the optional parent tag as
a template should be combined into this logic.

[imp]

Conceptually, this looks good. I can't tell for sure if all the args are identical to before, but my hasty review says yes.

sys/x86/x86/busdma_machdep.c

248 ↗

(On Diff #65919)

Mixing different kinds of pointers may not be well defined. You may need to do this on two different lines.

[scottl]

Switch to a typedef for the template. Be type-correct with
NULL field assignments.

[kib]

This is fine as simpler KPI, but it cannot provide the stable KBI in its current form. I am not sure if this is important.

Also, _simple does not sound descriptive. Might be use _from_template. And yes, I think it would be nice to have a KPI of the form where you could clone the existing tag, only overriding some parameters that are need to be. This KPI is very close.

sys/x86/x86/busdma_machdep.c

248 ↗

(On Diff #65925)

Could we start deprecating filter functions ? They are unusable anyway, and I propose that for this KPI the filter was always NULL. I.e., remove it from the template, and pass NULL to bus_dma_tag_create() in bus_dma_tag_simple().

259 ↗

(On Diff #65925)

You are inconsistent with blank lines: bu_dma_tag_init() does not add blank line after '{'.

[scottl]

I already changed the names in an upcoming revision; I agree that simple wasn't a good name. For the purposes of cloning an existing tag, what I'd propose is to have a function, bus_dma_template_clone(*template, *dmat) that serializes the opaque fields of the tag back into a template, then lets you optionally modify the template, and then turn it into a new tag with bus_dma_template_tag(*template, *tag). I'll code that up and submit it in the next patch.

sys/x86/x86/busdma_machdep.c

248 ↗

(On Diff #65925)

One of the biggest mistakes that I feel I've made with the CAM and busdma APIs in the past was in changing the argument signatures of existing functions. I don't want to repeat that mistake and redefine a bus bus_dma_tag_create(). But I also agree with you that filters are useless and the filter arguments are just noise. This new template API is my attempt to essentially deprecate filters as well as the old API signature without actually changing it. I suppose that the filter arguments can also be removed from the template, though, as a signal that filters are no longer a first-class part of the API.

259 ↗

(On Diff #65925)

I believe that it is part of style(9) to have a blank line when no local variables are declared.

static void
usage(void)
{
        /* Optional blank line goes here. */

Optionally, insert a blank line at the beginning of functions with no
local variables.  Older versions of this style document required the
blank line convention, so it is widely used in existing code.

[scottl]

I also have an large update to bus_dma.9 that I'll add to the review.

[kib]

sys/x86/x86/busdma_machdep.c

248 ↗

(On Diff #65925)

I am not proposing to change any functions' signatures. I propose to drop filtfunc/filtfuncarg from the bus_dma_template_t structure, nobody would use them anyway. Pass NULL for bus_dma_tag_create() for filter when called from bus_dma_tag_simple() (or whatever you rename it).

259 ↗

(On Diff #65925)

My point is that the patch is not consistent. You added blank line there, but not in bus_dma_tag_init().

From some time, the blank line is optional.

[scottl]

Change function names for better consistency. Add bus_dma_template_clone().
Update the man page.

[kib]

share/man/man9/bus_dma.9

92 ↗

(On Diff #65932)

There is .Fo/.Fa/.Fc macros to avoid too long lines for function declarations in mdoc.

sys/x86/x86/busdma_machdep.c

264 ↗

(On Diff #65932)

Space is needed before '('.

Does it make sense to check for NULLs ? Function call cannot have any useful interpretation of the user intent when either of args are NULL.

277 ↗

(On Diff #65932)

Same.