Re: [PATCH 3/3 v3] iommu/fsl: Freescale PAMU driver and IOMMU API implementation.

Scott Wood
2012-10-22T23:53:09Z

On 10/22/2012 04:18:07 PM, Tabi Timur-B04825 wrote:
> On Wed, Oct 17, 2012 at 12:32 PM, Varun Sethi
> <Varun.Sethi@freescale.com> wrote:
> > +}
> > +
> > +static unsigned long pamu_get_fspi_and_allocate(u32 subwin_cnt)
> > +{
>
> subwin_cnt should probably be an unsigned int.
>
> This function needs to be documented. What value is being returned?

spaact offset (yes, this needs to be documented)

> > + unsigned long spaace_addr;
> > +
> > + spaace_addr = gen_pool_alloc(spaace_pool, subwin_cnt *
> sizeof(paace_t));
> > + if (!spaace_addr)
> > + return ULONG_MAX;
>
> What's wrong with returning 0 on error?

0 is a valid spaact offset

> > +
> > + return (spaace_addr - (unsigned long)spaact) /
> (sizeof(paace_t));
>
> Is this supposed to be a virtual address? If so, then return void*
> instead of an unsigned long.

It's not a virtual address. How often does subtraction followed by
division result in a valid virtual address?

> > +int pamu_update_paace_stash(int liodn, u32 subwin, u32 value)

Whitespace

> > +#define PAMU_PAGE_SHIFT 12
> > +#define PAMU_PAGE_SIZE 4096ULL
>
> 4096ULL? Why not just 4096?

This lets it be used in phys_addr_t expressions without needing casts
everywhere or dropping bits.

> > +/* This bitmap advertises the page sizes supported by PAMU hardware
> > + * to the IOMMU API.
> > + */
> > +#define FSL_PAMU_PGSIZES (~0xFFFUL)
>
> There should be a better way to define this. ~(PAMU_PAGE_SIZE-1)
> maybe?

Is it even true? We don't support IOMMU pages larger than the SoC can
address.

The (~0xFFFUL) version also discards some valid IOMMU page sizes on
32-bit kernels. One use case for windows larger than the CPU virtual
address space is creating one big identity-map window to effectively
disable translation. If we're to support that, the size of
pgsize_bitmap
will need to change as well.

> > +static int map_liodn(int liodn, struct fsl_dma_domain *dma_domain)
> > +{
> > + u32 subwin_cnt = dma_domain->subwin_cnt;
> > + unsigned long rpn;
> > + int ret = 0, i;
> > +
> > + if (subwin_cnt) {
> > + struct dma_subwindow *sub_win_ptr =
> > + &dma_domain->sub_win_arr[0];
> > + for (i = 0; i < subwin_cnt; i++) {
> > + if (sub_win_ptr[i].valid) {
> > + rpn = sub_win_ptr[i].paddr >>
> > + PAMU_PAGE_SHIFT,
> > + spin_lock(&iommu_lock);
> > + ret = pamu_config_spaace(liodn,
> subwin_cnt, i,
> > +
> sub_win_ptr[i].size,
> > + -1,
> > + rpn,
> > +
> dma_domain->snoop_id,
> > +
> dma_domain->stash_id,
> > + (i > 0) ?
> 1 : 0,
> > +
> sub_win_ptr[i].prot);
> > + spin_unlock(&iommu_lock);
> > + if (ret) {
> > + pr_err("PAMU SPAACE
> configuration failed for liodn %d\n",
> > + liodn);
> > + return ret;
> > + }
> > + }
> > + }

Break up that nesting with some subfunctions.

> > + while (!list_empty(&dma_domain->devices)) {
> > + info = list_entry(dma_domain->devices.next,
> > + struct device_domain_info, link);
> > + remove_domain_ref(info, dma_domain->subwin_cnt);
> > + }
>
> I wonder if you should use list_for_each_safe() instead.

The above is simpler if you're destroying the entire list.

> > +}
> > +
> > +static int configure_domain_dma_state(struct fsl_dma_domain
> *dma_domain, int enable)
>
> bool enable
>
> Finally, please CC: me on all IOMMU and PAMU patches you post
> upstream.

Me too.

-Scott