| Message ID | c63d5ec5298e5ec7bbc6bde3ec71b25027998778.1642162459.git.michal.simek@xilinx.com |
|---|---|
| State | Superseded |
| Delegated to: | Michal Simek |
| Headers | show |
| Series | [1/2] lmb: Fix lmb property's defination under struct lmb | expand |
Hi, On 1/14/22 1:14 PM, Michal Simek wrote: > From: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com> > > Under struct lmb {} the lmb property's should be defined only if > CONFIG_LMB_MEMORY_REGIONS is defined. > > Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com> > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > --- > > include/lmb.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/lmb.h b/include/lmb.h > index ab277ca80004..1476d78c2823 100644 > --- a/include/lmb.h > +++ b/include/lmb.h > @@ -68,7 +68,7 @@ struct lmb_region { > struct lmb { > struct lmb_region memory; > struct lmb_region reserved; > -#if !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) > +#if IS_ENABLED(CONFIG_LMB_MEMORY_REGIONS) > struct lmb_property memory_regions[CONFIG_LMB_MEMORY_REGIONS]; > struct lmb_property reserved_regions[CONFIG_LMB_RESERVED_REGIONS]; > #endif I think this patch don't change the exiting code because in lib/Kconfig we have also the dependancy with LMB_USE_MAX_REGIONS: config LMB_MEMORY_REGIONS int "Number of memory regions in lmb lib" depends on LMB && !LMB_USE_MAX_REGIONS => memory_regions and reserved_regions are needed in struc lmb only if CONFIG_LMB_USE_MAX_REGIONS is not defined else it is defined in "struct lmb_region" under the SAME compilation flag struct lmb_region { unsigned long cnt; unsigned long max; #if IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) struct lmb_property region[CONFIG_LMB_MAX_REGIONS]; #else struct lmb_property *region; #endif }; with .region = pointer to 'memory_regions[]' or 'reserved_regions[]' in "struct lmb" I think it is more clear to have the compilation flag in "struct lmb_region" and in "struct lmb_region". but I have no objection to change it. PS: I introduce this flag to keep the previous behavior and previous struct size on other platform when I push the commit 6d66502bc741 ("lmb: Add 2 config to define the max number of regions") Regards Patrick
On 1/17/22 17:23, Patrick DELAUNAY wrote: > Hi, > > On 1/14/22 1:14 PM, Michal Simek wrote: >> From: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com> >> >> Under struct lmb {} the lmb property's should be defined only if >> CONFIG_LMB_MEMORY_REGIONS is defined. >> >> Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com> >> Signed-off-by: Michal Simek <michal.simek@xilinx.com> >> --- >> >> include/lmb.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/include/lmb.h b/include/lmb.h >> index ab277ca80004..1476d78c2823 100644 >> --- a/include/lmb.h >> +++ b/include/lmb.h >> @@ -68,7 +68,7 @@ struct lmb_region { >> struct lmb { >> struct lmb_region memory; >> struct lmb_region reserved; >> -#if !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) >> +#if IS_ENABLED(CONFIG_LMB_MEMORY_REGIONS) >> struct lmb_property memory_regions[CONFIG_LMB_MEMORY_REGIONS]; >> struct lmb_property reserved_regions[CONFIG_LMB_RESERVED_REGIONS]; >> #endif > > > I think this patch don't change the exiting code > > because in lib/Kconfig we have also the dependancy with LMB_USE_MAX_REGIONS: > > config LMB_MEMORY_REGIONS > int "Number of memory regions in lmb lib" > depends on LMB && !LMB_USE_MAX_REGIONS > > => memory_regions and reserved_regions are needed in struc lmb > only if CONFIG_LMB_USE_MAX_REGIONS is not defined > else it is defined in "struct lmb_region" under the SAME compilation flag > > > struct lmb_region { > unsigned long cnt; > unsigned long max; > #if IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) > struct lmb_property region[CONFIG_LMB_MAX_REGIONS]; > #else > struct lmb_property *region; > #endif > }; > > with .region = pointer to 'memory_regions[]' or 'reserved_regions[]' in "struct > lmb" > > I think it is more clear to have the compilation flag in "struct lmb_region" and > in "struct lmb_region". > > but I have no objection to change it. > > > PS: I introduce this flag to keep the previous behavior and previous struct size > on other platform > > when I push the commit 6d66502bc741 ("lmb: Add 2 config to define the max > number of regions") > When I run make xilinx_zynqmp_virt_defconfig and then disable LMB I am getting compilation failure just simply because of CONFIG_LMB_USE_MAX_REGIONS is disabled but because there is #if !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) which is simply #if 1 but CONFIG_LMB_MEMORY_REGIONS is not defined. That's why patch is just correcting that if statement to be aligned with macro which should be defined. Thanks, Michal
On 1/17/22 17:23, Patrick DELAUNAY wrote: > Hi, > > On 1/14/22 1:14 PM, Michal Simek wrote: >> From: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com> >> >> Under struct lmb {} the lmb property's should be defined only if >> CONFIG_LMB_MEMORY_REGIONS is defined. >> >> Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com> >> Signed-off-by: Michal Simek <michal.simek@xilinx.com> >> --- >> >> include/lmb.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/include/lmb.h b/include/lmb.h >> index ab277ca80004..1476d78c2823 100644 >> --- a/include/lmb.h >> +++ b/include/lmb.h >> @@ -68,7 +68,7 @@ struct lmb_region { >> struct lmb { >> struct lmb_region memory; >> struct lmb_region reserved; >> -#if !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) >> +#if IS_ENABLED(CONFIG_LMB_MEMORY_REGIONS) >> struct lmb_property memory_regions[CONFIG_LMB_MEMORY_REGIONS]; >> struct lmb_property reserved_regions[CONFIG_LMB_RESERVED_REGIONS]; >> #endif > > > I think this patch don't change the exiting code > > because in lib/Kconfig we have also the dependancy with > LMB_USE_MAX_REGIONS: > > config LMB_MEMORY_REGIONS > int "Number of memory regions in lmb lib" > depends on LMB && !LMB_USE_MAX_REGIONS > > => memory_regions and reserved_regions are needed in struc lmb > only if CONFIG_LMB_USE_MAX_REGIONS is not defined > else it is defined in "struct lmb_region" under the SAME compilation > flag > > > struct lmb_region { > unsigned long cnt; > unsigned long max; > #if IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) > struct lmb_property region[CONFIG_LMB_MAX_REGIONS]; > #else > struct lmb_property *region; > #endif > }; > > with .region = pointer to 'memory_regions[]' or 'reserved_regions[]' in > "struct lmb" > > I think it is more clear to have the compilation flag in "struct > lmb_region" and in "struct lmb_region". > > but I have no objection to change it. > > > PS: I introduce this flag to keep the previous behavior and previous > struct size on other platform > > when I push the commit 6d66502bc741 ("lmb: Add 2 config to define > the max number of regions") > > Regards > > Patrick > > Should we really use #ifdef for these structures to save some bytes on the stack. Isn't it preferable to move the code from using '#ifdef CONFIG' to 'if IS_ENABLED(CONFIG'? Best regards Heinrich
pá 14. 1. 2022 v 13:14 odesílatel Michal Simek <michal.simek@xilinx.com> napsal: > > From: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com> > > Under struct lmb {} the lmb property's should be defined only if > CONFIG_LMB_MEMORY_REGIONS is defined. > > Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com> > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > --- > > include/lmb.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/lmb.h b/include/lmb.h > index ab277ca80004..1476d78c2823 100644 > --- a/include/lmb.h > +++ b/include/lmb.h > @@ -68,7 +68,7 @@ struct lmb_region { > struct lmb { > struct lmb_region memory; > struct lmb_region reserved; > -#if !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) > +#if IS_ENABLED(CONFIG_LMB_MEMORY_REGIONS) > struct lmb_property memory_regions[CONFIG_LMB_MEMORY_REGIONS]; > struct lmb_property reserved_regions[CONFIG_LMB_RESERVED_REGIONS]; > #endif > -- > 2.34.1 > applied. M
po 7. 2. 2022 v 10:40 odesílatel Michal Simek <monstr@monstr.eu> napsal: > > pá 14. 1. 2022 v 13:14 odesílatel Michal Simek <michal.simek@xilinx.com> napsal: > > > > From: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com> > > > > Under struct lmb {} the lmb property's should be defined only if > > CONFIG_LMB_MEMORY_REGIONS is defined. > > > > Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma@xilinx.com> > > Signed-off-by: Michal Simek <michal.simek@xilinx.com> > > --- > > > > include/lmb.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/include/lmb.h b/include/lmb.h > > index ab277ca80004..1476d78c2823 100644 > > --- a/include/lmb.h > > +++ b/include/lmb.h > > @@ -68,7 +68,7 @@ struct lmb_region { > > struct lmb { > > struct lmb_region memory; > > struct lmb_region reserved; > > -#if !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) > > +#if IS_ENABLED(CONFIG_LMB_MEMORY_REGIONS) > > struct lmb_property memory_regions[CONFIG_LMB_MEMORY_REGIONS]; > > struct lmb_property reserved_regions[CONFIG_LMB_RESERVED_REGIONS]; > > #endif > > -- > > 2.34.1 > > > > applied. > M ci loop found an issue that's why I have sent v2 and remove these patches from my queue. M
diff --git a/include/lmb.h b/include/lmb.h index ab277ca80004..1476d78c2823 100644 --- a/include/lmb.h +++ b/include/lmb.h @@ -68,7 +68,7 @@ struct lmb_region { struct lmb { struct lmb_region memory; struct lmb_region reserved; -#if !IS_ENABLED(CONFIG_LMB_USE_MAX_REGIONS) +#if IS_ENABLED(CONFIG_LMB_MEMORY_REGIONS) struct lmb_property memory_regions[CONFIG_LMB_MEMORY_REGIONS]; struct lmb_property reserved_regions[CONFIG_LMB_RESERVED_REGIONS]; #endif