Re: [PATCH v4 07/11] powerpc/pseries/iommu: Reorganize iommu_table_setparms*() with new helper

From: Alexey Kardashevskiy
Date: Wed Jul 14 2021 - 04:32:32 EST




On 13/07/2021 14:47, Leonardo Brás wrote:
Hello Alexey,

On Fri, 2021-06-18 at 19:26 -0300, Leonardo Brás wrote:

+                                        unsigned long liobn,
unsigned long win_addr,
+                                        unsigned long
window_size,
unsigned long page_shift,
+                                        unsigned long base,
struct
iommu_table_ops *table_ops)


iommu_table_setparms() rather than passing 0 around.

The same comment about "liobn" - set it in
iommu_table_setparms_lpar().
The reviewer will see what field atters in what situation imho.


The idea here was to keep all tbl parameters setting in
_iommu_table_setparms (or iommu_table_setparms_common).

I understand the idea that each one of those is optional in the other
case, but should we keep whatever value is present in the other
variable (not zeroing the other variable), or do someting like:

tbl->it_index = 0;
tbl->it_base = basep;
(in iommu_table_setparms)

tbl->it_index = liobn;
tbl->it_base = 0;
(in iommu_table_setparms_lpar)


This one is supposed to be a question, but I missed the question mark.
Sorry about that.

Ah ok :)

I would like to get your opinion in this :)

Besides making the "base" parameter a pointer, I really do not have strong preference, just make it not hurting eyes of a reader, that's all :)

imho in general, rather than answering 5 weeks later, it is more productive to address whatever comments were made, add comments (in the code or commit logs) why you are sticking to your initial approach, rebase and repost the whole thing. Thanks,



--
Alexey