diff for duplicates of <1350949982.30970.11@snotra> diff --git a/a/1.txt b/N1/1.txt index fcb4133..2f1b1af 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -1,36 +1,36 @@ On 10/22/2012 04:18:07 PM, Tabi Timur-B04825 wrote: -> On Wed, Oct 17, 2012 at 12:32 PM, Varun Sethi +> On Wed, Oct 17, 2012 at 12:32 PM, Varun Sethi =20 > <Varun.Sethi@freescale.com> 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 = gen_pool_alloc(spaace_pool, subwin_cnt * +> > + 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) / +> > + 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 +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) @@ -39,7 +39,7 @@ 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 @@ -49,8 +49,8 @@ everywhere or dropping bits. > > + * to the IOMMU API. > > + */ > > +#define FSL_PAMU_PGSIZES (~0xFFFUL) -> -> There should be a better way to define this. ~(PAMU_PAGE_SIZE-1) +>=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 @@ -59,41 +59,41 @@ 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 +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 = dma_domain->subwin_cnt; +> > + u32 subwin_cnt =3D dma_domain->subwin_cnt; > > + unsigned long rpn; -> > + int ret = 0, i; +> > + int ret =3D 0, i; > > + > > + if (subwin_cnt) { -> > + struct dma_subwindow *sub_win_ptr = +> > + struct dma_subwindow *sub_win_ptr =3D > > + &dma_domain->sub_win_arr[0]; -> > + for (i = 0; i < subwin_cnt; i++) { +> > + for (i =3D 0; i < subwin_cnt; i++) { > > + if (sub_win_ptr[i].valid) { -> > + rpn = sub_win_ptr[i].paddr >> +> > + rpn =3D sub_win_ptr[i].paddr >> > > + PAMU_PAGE_SHIFT, > > + spin_lock(&iommu_lock); -> > + ret = pamu_config_spaace(liodn, +> > + 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) ? +> > + (i > 0) ? =20 > 1 : 0, -> > + +> > + =20 > sub_win_ptr[i].prot); > > + spin_unlock(&iommu_lock); > > + if (ret) { -> > + pr_err("PAMU SPAACE +> > + pr_err("PAMU SPAACE =20 > configuration failed for liodn %d\n", > > + liodn); > > + return ret; @@ -104,25 +104,25 @@ will need to change as well. Break up that nesting with some subfunctions. > > + while (!list_empty(&dma_domain->devices)) { -> > + info = list_entry(dma_domain->devices.next, +> > + 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 +> > +static int configure_domain_dma_state(struct fsl_dma_domain =20 > *dma_domain, int enable) -> +>=20 > bool enable -> -> Finally, please CC: me on all IOMMU and PAMU patches you post +>=20 +> Finally, please CC: me on all IOMMU and PAMU patches you post =20 > upstream. Me too. --Scott +-Scott= diff --git a/a/content_digest b/N1/content_digest index 4e56628..923025e 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -4,45 +4,45 @@ "Date\0Mon, 22 Oct 2012 18:53:02 -0500\0" "To\0Tabi Timur-B04825 <B04825@freescale.com>\0" "Cc\0Sethi Varun-B16395 <B16395@freescale.com>" - joerg.roedel@amd.com <joerg.roedel@amd.com> iommu@lists.linux-foundation.org <iommu@lists.linux-foundation.org> linuxppc-dev@lists.ozlabs.org <linuxppc-dev@lists.ozlabs.org> - " linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>\0" + linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org> + " joerg.roedel@amd.com <joerg.roedel@amd.com>\0" "\00:1\0" "b\0" "On 10/22/2012 04:18:07 PM, Tabi Timur-B04825 wrote:\n" - "> On Wed, Oct 17, 2012 at 12:32 PM, Varun Sethi \n" + "> On Wed, Oct 17, 2012 at 12:32 PM, Varun Sethi =20\n" "> <Varun.Sethi@freescale.com> wrote:\n" "> > +}\n" "> > +\n" "> > +static unsigned long pamu_get_fspi_and_allocate(u32 subwin_cnt)\n" "> > +{\n" - "> \n" + ">=20\n" "> subwin_cnt should probably be an unsigned int.\n" - "> \n" + ">=20\n" "> This function needs to be documented. What value is being returned?\n" "\n" "spaact offset (yes, this needs to be documented)\n" "\n" "> > + unsigned long spaace_addr;\n" "> > +\n" - "> > + spaace_addr = gen_pool_alloc(spaace_pool, subwin_cnt * \n" + "> > + spaace_addr =3D gen_pool_alloc(spaace_pool, subwin_cnt * =20\n" "> sizeof(paace_t));\n" "> > + if (!spaace_addr)\n" "> > + return ULONG_MAX;\n" - "> \n" + ">=20\n" "> What's wrong with returning 0 on error?\n" "\n" "0 is a valid spaact offset\n" "\n" "> > +\n" - "> > + return (spaace_addr - (unsigned long)spaact) / \n" + "> > + return (spaace_addr - (unsigned long)spaact) / =20\n" "> (sizeof(paace_t));\n" - "> \n" + ">=20\n" "> Is this supposed to be a virtual address? If so, then return void*\n" "> instead of an unsigned long.\n" "\n" - "It's not a virtual address. How often does subtraction followed by \n" + "It's not a virtual address. How often does subtraction followed by =20\n" "division result in a valid virtual address?\n" "\n" "> > +int pamu_update_paace_stash(int liodn, u32 subwin, u32 value)\n" @@ -51,7 +51,7 @@ "\n" "> > +#define PAMU_PAGE_SHIFT 12\n" "> > +#define PAMU_PAGE_SIZE 4096ULL\n" - "> \n" + ">=20\n" "> 4096ULL? Why not just 4096?\n" "\n" "This lets it be used in phys_addr_t expressions without needing casts\n" @@ -61,8 +61,8 @@ "> > + * to the IOMMU API.\n" "> > + */\n" "> > +#define FSL_PAMU_PGSIZES (~0xFFFUL)\n" - "> \n" - "> There should be a better way to define this. ~(PAMU_PAGE_SIZE-1) \n" + ">=20\n" + "> There should be a better way to define this. ~(PAMU_PAGE_SIZE-1) =20\n" "> maybe?\n" "\n" "Is it even true? We don't support IOMMU pages larger than the SoC can\n" @@ -71,41 +71,41 @@ "The (~0xFFFUL) version also discards some valid IOMMU page sizes on\n" "32-bit kernels. One use case for windows larger than the CPU virtual\n" "address space is creating one big identity-map window to effectively\n" - "disable translation. If we're to support that, the size of \n" + "disable translation. If we're to support that, the size of =20\n" "pgsize_bitmap\n" "will need to change as well.\n" "\n" "> > +static int map_liodn(int liodn, struct fsl_dma_domain *dma_domain)\n" "> > +{\n" - "> > + u32 subwin_cnt = dma_domain->subwin_cnt;\n" + "> > + u32 subwin_cnt =3D dma_domain->subwin_cnt;\n" "> > + unsigned long rpn;\n" - "> > + int ret = 0, i;\n" + "> > + int ret =3D 0, i;\n" "> > +\n" "> > + if (subwin_cnt) {\n" - "> > + struct dma_subwindow *sub_win_ptr =\n" + "> > + struct dma_subwindow *sub_win_ptr =3D\n" "> > + &dma_domain->sub_win_arr[0];\n" - "> > + for (i = 0; i < subwin_cnt; i++) {\n" + "> > + for (i =3D 0; i < subwin_cnt; i++) {\n" "> > + if (sub_win_ptr[i].valid) {\n" - "> > + rpn = sub_win_ptr[i].paddr >>\n" + "> > + rpn =3D sub_win_ptr[i].paddr >>\n" "> > + PAMU_PAGE_SHIFT,\n" "> > + spin_lock(&iommu_lock);\n" - "> > + ret = pamu_config_spaace(liodn, \n" + "> > + ret =3D pamu_config_spaace(liodn, =20\n" "> subwin_cnt, i,\n" - "> > + \n" + "> > + =20\n" "> sub_win_ptr[i].size,\n" "> > + -1,\n" "> > + rpn,\n" - "> > + \n" + "> > + =20\n" "> dma_domain->snoop_id,\n" - "> > + \n" + "> > + =20\n" "> dma_domain->stash_id,\n" - "> > + (i > 0) ? \n" + "> > + (i > 0) ? =20\n" "> 1 : 0,\n" - "> > + \n" + "> > + =20\n" "> sub_win_ptr[i].prot);\n" "> > + spin_unlock(&iommu_lock);\n" "> > + if (ret) {\n" - "> > + pr_err(\"PAMU SPAACE \n" + "> > + pr_err(\"PAMU SPAACE =20\n" "> configuration failed for liodn %d\\n\",\n" "> > + liodn);\n" "> > + return ret;\n" @@ -116,27 +116,27 @@ "Break up that nesting with some subfunctions.\n" "\n" "> > + while (!list_empty(&dma_domain->devices)) {\n" - "> > + info = list_entry(dma_domain->devices.next,\n" + "> > + info =3D list_entry(dma_domain->devices.next,\n" "> > + struct device_domain_info, link);\n" "> > + remove_domain_ref(info, dma_domain->subwin_cnt);\n" "> > + }\n" - "> \n" + ">=20\n" "> I wonder if you should use list_for_each_safe() instead.\n" "\n" "The above is simpler if you're destroying the entire list.\n" "\n" "> > +}\n" "> > +\n" - "> > +static int configure_domain_dma_state(struct fsl_dma_domain \n" + "> > +static int configure_domain_dma_state(struct fsl_dma_domain =20\n" "> *dma_domain, int enable)\n" - "> \n" + ">=20\n" "> bool enable\n" - "> \n" - "> Finally, please CC: me on all IOMMU and PAMU patches you post \n" + ">=20\n" + "> Finally, please CC: me on all IOMMU and PAMU patches you post =20\n" "> upstream.\n" "\n" "Me too.\n" "\n" - -Scott + -Scott= -a9cf65ce7a710b5c4738440683e096698a1481ee4826e335895b9108c1eebb90 +5df8d2d3cc4b08de1dffd621af5c7ccdfdd9498f471ff97e4bf7f35e2658418c
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.