On Wed, Jul 26, 2023 at 06:30:15PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Jul 26, 2023 at 08:02:24AM +0300, Serge Semin wrote:
> > On Mon, Jul 24, 2023 at 01:15:56PM +0530, Manivannan Sadhasivam wrote:
> > > On Fri, Jul 21, 2023 at 04:44:36PM +0900, Yoshihiro Shimoda wrote:
> > > > The __dw_pcie_prog_outbound_atu() currently has 6 arguments.
> > > > To support INTx IRQs in the future, it requires an additional 2
> > > > arguments. For improved code readability, introduce the struct
> > > > dw_pcie_ob_atu_cfg and update the arguments of
> > > > dw_pcie_prog_outbound_atu().
> > > >
> > > > Consequently, remove __dw_pcie_prog_outbound_atu() and
> > > > dw_pcie_prog_ep_outbound_atu() because there is no longer
> > > > a need.
> > > >
> > > > No behavior changes.
> > > >
> > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
> > >
> > > One nit below. With that,
> > >
> > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> > >
> > > > Reviewed-by: Serge Semin <fancer.lancer@xxxxxxxxx>
> > > > ---
> > > > .../pci/controller/dwc/pcie-designware-ep.c | 21 +++++---
> > > > .../pci/controller/dwc/pcie-designware-host.c | 52 +++++++++++++------
> > > > drivers/pci/controller/dwc/pcie-designware.c | 49 ++++++-----------
> > > > drivers/pci/controller/dwc/pcie-designware.h | 15 ++++--
> > > > 4 files changed, 77 insertions(+), 60 deletions(-)
> > > >
> > >
> > > [...]
> > >
> > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > > > index 3c06e025c905..85de0d8346fa 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > > > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > > > @@ -288,6 +288,15 @@ enum dw_pcie_core_rst {
> > > > DW_PCIE_NUM_CORE_RSTS
> > > > };
> > > >
> > > > +struct dw_pcie_ob_atu_cfg {
> > > > + int index;
> > > > + int type;
> > > > + u8 func_no;
> > > > + u64 cpu_addr;
> > > > + u64 pci_addr;
> > > > + u64 size;
> > >
> >
> > > Reorder the members in below order to avoid holes:
> > >
> > > u64
> > > int
> > > u8
> >
> > One more time. Your suggestion won't prevent the compiler from adding
> > the pads. (If by "holes" you meant the padding. Otherwise please
> > elaborate what you meant?).
>
> Struct padding is often referred as struct holes. So yes, I'm referring the
> same.
>
> > The structure will have the same size of
> > 40 bytes in both cases. So your suggestion will just worsen the
> > structure readability from having a more natural parameters order (MW
> > index, type, function, and then the mapping parameters) to a redundant
> > type-based order.
> >
>
> This is a common comment I provide for all structures. Even though the current
> result (reordering) doesn't save any space, when the structure grows big (who
> knows), we often see more holes/padding being inserted by the compiler if the
> members are not ordered in the descending order w.r.t their size.
>
> I agree that it makes more clear if the members are grouped based on their
> function etc... but for large structures this would often add more padding/hole.
This structure will never be big enough to be considered for such
strange optimization. Moreover practicality almost always beats some
theoretical considerations. In this case there is no any reason to
reorder the fields as you say.
Speaking in general I very much doubt that saving a few bytes of
memory can be considered as a better option than having a more
readable structure especially these days. Moreover for all these years
I never met anybody asking to set the descending order of
the members or maintaining such limitation in the commonly used kernel
structures. What is normally done:
1. Move an embedded object to the head of the structure for the
container_of-macro optimization.
2. Group up the commonly used fields to optimize the system cache
utilization.
3. Logical grouping the members, which naturally may lead to the more
optimal cache utilization.
4. Move a field to a certain place of the structure to fill in the
pads.
Even if the "descending alignment" requirement minimizes the number of
the pads it isn't the only possible way to do so in the particular
cases and it looks too harsh to be blindly applied all the time. If a
few bytes is so important why not do the same for instance for the
local variables too? They are also normally size-aligned in the stack
memory, which is much more precious in kernel.
Anyway in this case changing the fields order is absolutely redundant.
Even a provided afterwards update doesn't cause the structure size
change. So for the sake of readability it's better to leave its fields
ordered as is.
-Serge(y)
>
> - Mani
>
> > -Serge(y)
> >
> > >
> > > - Mani
> > >
> > > > +};
> > > > +
> > > > struct dw_pcie_host_ops {
> > > > int (*host_init)(struct dw_pcie_rp *pp);
> > > > void (*host_deinit)(struct dw_pcie_rp *pp);
> > > > @@ -416,10 +425,8 @@ void dw_pcie_write_dbi2(struct dw_pcie *pci, u32 reg, size_t size, u32 val);
> > > > int dw_pcie_link_up(struct dw_pcie *pci);
> > > > void dw_pcie_upconfig_setup(struct dw_pcie *pci);
> > > > int dw_pcie_wait_for_link(struct dw_pcie *pci);
> > > > -int dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
> > > > - u64 cpu_addr, u64 pci_addr, u64 size);
> > > > -int dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> > > > - int type, u64 cpu_addr, u64 pci_addr, u64 size);
> > > > +int dw_pcie_prog_outbound_atu(struct dw_pcie *pci,
> > > > + const struct dw_pcie_ob_atu_cfg *atu);
> > > > int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int type,
> > > > u64 cpu_addr, u64 pci_addr, u64 size);
> > > > int dw_pcie_prog_ep_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> > > > --
> > > > 2.25.1
> > > >
> > >
> > > --
> > > மணிவண்ணன் சதாசிவம்
>
> --
> மணிவண்ணன் சதாசிவம்
