Subject: Re: CVS commit: src/sys
To: Yorick Hardy <yhardy@uj.ac.za>
From: Jachym Holecek <freza@dspfpga.com>
List: tech-kern
Date: 02/23/2007 18:39:11
Hello,

a couple of minor nits:

# Yorick Hardy 2007-02-23:
> --- share/man/man9/bus_dma.9.orig	2007-02-23 16:21:58.000000000 +0200
> +++ share/man/man9/bus_dma.9	2007-02-23 16:33:57.000000000 +0200
> @@ -777,6 +784,53 @@
>  .Pp
>  Returns -1 to indicate failure.
>  Otherwise, returns an opaque value to be interpreted by the device pager.
> +.It Fn bus_dmatag_subregion "tag" "min_addr" "max_addr" "newtag" "flags"
> +Creates a new bus_dma_tag_t in
> +.Fa newtag
> +from
> +.Fa tag
> +with the added constraint that data will only be transferred to
> +memory addresses in the range (subregion)

Aren't they bus addresses, really? On a system with IOMMU, the subregion
constraint translates to address constraint on the _bus_ side of the
translation, memory address isn't really restricted.

Unless I've confused myself with terminology again :-).

> +.Fa min_addr
> +to
> +.Fa max_addr.
> +The arguments are as follows:
> +.Bl -tag -width nsegs -compact
> +.It Fa tag
> +This is the bus_dma_tag_t passed down from the parent driver via
> +.Fa \*[Lt]bus\*[Gt]_attach_args .
> +.It Fa min_addr
> +The low address constraint for DMA transfers.
> +This address is included.

I should've asked earlier how much practical value the lower constraint
has... No problem keeping it, just curious -- I can't imagine how would
one need to FUBAR a device to make it unable to transfer below certain
address.

> +.It Fa max_addr
> +The high address constraint for DMA transfers.
> +This address is not included.

Maybe call it "limit" then (normally, I'd expect "maximum" is included
in the range)?

> [...]
> +.It Fn bus_dmatag_destroy "tag"
> +Frees any resources associated with
> +.Fa tag
> +when the tag is no longer needed.
> +.Fn bus_dmatag_destroy
> +should always be called when a 
> +.Fa tag
> +created by
> +.Fn bus_dmatag_subregion
> +is no longer needed.
> +If given valid arguments,
> +.Fn bus_dmatag_destroy
> +always succeeds.

It's void, so can't really fail nor succeed. Maybe just say it assumes
valid argument (which means "behaviour undefined for invalid value")?

Thanks for doing this, BTW.

	-- Jachym