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

From: Scott Wood <[email-masked]>
Date: Mon, 22 Oct 2012 18:53:02 -0500

On 10/22/2012 04:18:07 PM, Tabi Timur-B04825 wrote:
> On Wed, Oct 17, 2012 at 12:32 PM, Varun Sethi =20
> <[email-masked]> wrote:
> > +}
> > +
> > +static unsigned long pamu_get_fspi_and_allocate(u32 subwin_cnt)
> > +{
>=20
> subwin_cnt should probably be an unsigned int.
>=20
> 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 =3D gen_pool_alloc(spaace_pool, subwin_cnt * =20
> sizeof(paace_t));
> > +       if (!spaace_addr)
> > +               return ULONG_MAX;
>=20
> What's wrong with returning 0 on error?

0 is a valid spaact offset

> > +
> > +       return (spaace_addr - (unsigned long)spaact) / =20
> (sizeof(paace_t));
>=20
> 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 =20
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
>=20
> 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)
>=20
> There should be a better way to define this.  ~(PAMU_PAGE_SIZE-1) =20
> 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 =20
pgsize_bitmap
will need to change as well.

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

Break up that nesting with some subfunctions.

> > +       while (!list_empty(&dma_domain->devices)) {
> > +               info =3D list_entry(dma_domain->devices.next,
> > +                       struct device_domain_info, link);
> > +               remove_domain_ref(info, dma_domain->subwin_cnt);
> > +       }
>=20
> 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 =20
> *dma_domain, int enable)
>=20
> bool enable
>=20
> Finally, please CC: me on all IOMMU and PAMU patches you post =20
> upstream.

Me too.

-Scott=