Re: [EXT] Re: [PATCH v2] PCI: endpoint: pci-epf-{,v}ntb: fix a check for no epc alignment constraint

[Manivannan Sadhasivam]

[Added Kishon's new email address and removed the old one]

On Thu, Oct 27, 2022 at 03:34:11PM +0000, Frank Li wrote:
> 
> 
> > -----Original Message-----
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> > Sent: Thursday, October 27, 2022 10:12 AM
> > To: Frank Li <frank.li@xxxxxxx>
> > Cc: Shunsuke Mie <mie@xxxxxxxxxx>; Jon Mason <jdmason@xxxxxxxx>; Dave
> > Jiang <dave.jiang@xxxxxxxxx>; Allen Hubbe <allenbh@xxxxxxxxx>; Kishon
> > Vijay Abraham I <kishon@xxxxxx>; Lorenzo Pieralisi <lpieralisi@xxxxxxxxxx>;
> > Krzysztof Wilczyński <kw@xxxxxxxxx>; Bjorn Helgaas
> > <bhelgaas@xxxxxxxxxx>; ntb@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx;
> > linux-kernel@xxxxxxxxxxxxxxx
> > Subject: Re: [EXT] Re: [PATCH v2] PCI: endpoint: pci-epf-{,v}ntb: fix a check for
> > no epc alignment constraint
> > 
> > Caution: EXT Email
> > 
> > On Thu, Oct 27, 2022 at 02:35:56PM +0000, Frank Li wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Shunsuke Mie <mie@xxxxxxxxxx>
> > > > Sent: Wednesday, October 26, 2022 8:43 PM
> > > > To: Frank Li <frank.li@xxxxxxx>
> > > > Cc: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>; Jon
> > > > Mason <jdmason@xxxxxxxx>; Dave Jiang <dave.jiang@xxxxxxxxx>; Allen
> > > > Hubbe <allenbh@xxxxxxxxx>; Kishon Vijay Abraham I <kishon@xxxxxx>;
> > > > Lorenzo Pieralisi <lpieralisi@xxxxxxxxxx>; Krzysztof Wilczyński
> > > > <kw@xxxxxxxxx>; Bjorn Helgaas <bhelgaas@xxxxxxxxxx>;
> > > > ntb@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; linux-
> > kernel@xxxxxxxxxxxxxxx
> > > > Subject: Re: [EXT] Re: [PATCH v2] PCI: endpoint: pci-epf-{,v}ntb: fix a check
> > for
> > > > no epc alignment constraint
> > > >
> > > > Caution: EXT Email
> > > >
> > > > Hi Frank,
> > > >
> > > > 2022年10月26日(水) 1:07 Frank Li <frank.li@xxxxxxx>:
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> > > > > > Sent: Tuesday, October 25, 2022 9:22 AM
> > > > > > To: Shunsuke Mie <mie@xxxxxxxxxx>
> > > > > > Cc: Jon Mason <jdmason@xxxxxxxx>; Dave Jiang
> > <dave.jiang@xxxxxxxxx>;
> > > > > > Allen Hubbe <allenbh@xxxxxxxxx>; Kishon Vijay Abraham I
> > > > > > <kishon@xxxxxx>; Lorenzo Pieralisi <lpieralisi@xxxxxxxxxx>; Krzysztof
> > > > > > Wilczyński <kw@xxxxxxxxx>; Bjorn Helgaas <bhelgaas@xxxxxxxxxx>;
> > > > > > ntb@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; linux-
> > > > kernel@xxxxxxxxxxxxxxx
> > > > > > Subject: [EXT] Re: [PATCH v2] PCI: endpoint: pci-epf-{,v}ntb: fix a check
> > for
> > > > no
> > > > > > epc alignment constraint
> > > > > >
> > > > > > Caution: EXT Email
> > > > > >
> > > > > > On Thu, Sep 22, 2022 at 06:23:57PM +0900, Shunsuke Mie wrote:
> > > > > > > Some PCI endpoint controllers have no alignment constraints, and
> > the
> > > > > > > epc_features->align becomes 0. In this case, IS_ALIGNED() in
> > > > >
> > > > > [Frank Li] why not set epc_features->align 1
> > > > > no alignment constraints should mean align to byte.
> > > > It is one of the solutions too I think. But in that case,  we need to
> > > > write epc_features->align = 1 to all epc drivers, dwc, qcom, rcar,
> > > > tegra, and etc.
> > > >
> > > > I think that my change is better.
> > >
> > > I think it should be based on what original term defined.
> > > It should be fixed at where make mistake.
> > >
> > 
> > 1byte is the default alignment that drivers can assume, why do you want
> > drivers
> > to set them explicitly when they do not want any special alignment?
> 
> What's definition of not alignment by align variable?
> Using both 0 and 1 as no alignment is not good enough. 
> 
> I grep whole kernel driver directory, not one use 
> 	If (align && IS_ALIGNED(x, align))  statement.    

I can see multiple hits:

lib/ubsan.c
drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
drivers/gpu/drm/i915/i915_vma.c
drivers/nvdimm/pfn_devs.c
drivers/misc/pci_endpoint_test.c

But in most of the places, the alignment is guaranteed to be set by the client
drivers because they might be read from the hardware register or fixed for an
IP. But in this case, I don't think we should _force_ the drivers to set
alignment to 1 (default) if they don't really care about it.

Thanks,
Mani

> 
> There are a common convention, align is 2^n
> 
> 
> > 
> > I think this patch is fine.
> > 
> > Thanks,
> > Mani
> > 
> > > Are there other place use align == 0 means no alignment in kernel?
> > >
> > > >
> > > > > > > epf_ntb_config_spad_bar_alloc() doesn't work well. Check for this
> > > > before
> > > > > > > IS_ALIGNED().
> > > > > > >
> > > > > > > Signed-off-by: Shunsuke Mie <mie@xxxxxxxxxx>
> > > > > >
> > > > > > Reviewed-by: Manivannan Sadhasivam
> > > > > > <manivannan.sadhasivam@xxxxxxxxxx>
> > > > > >
> > > > > > Thanks,
> > > > > > Mani
> > > > > >
> > > > > > > ---
> > > > > > > Changes in v2:
> > > > > > > * Fix the commit message in phrasings and words.
> > > > > > > ---
> > > > > > > ---
> > > > > > >  drivers/pci/endpoint/functions/pci-epf-ntb.c  | 2 +-
> > > > > > >  drivers/pci/endpoint/functions/pci-epf-vntb.c | 2 +-
> > > > > > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/pci/endpoint/functions/pci-epf-ntb.c
> > > > > > b/drivers/pci/endpoint/functions/pci-epf-ntb.c
> > > > > > > index 9a00448c7e61..f74155ee8d72 100644
> > > > > > > --- a/drivers/pci/endpoint/functions/pci-epf-ntb.c
> > > > > > > +++ b/drivers/pci/endpoint/functions/pci-epf-ntb.c
> > > > > > > @@ -1021,7 +1021,7 @@ static int
> > > > epf_ntb_config_spad_bar_alloc(struct
> > > > > > epf_ntb *ntb,
> > > > > > >       peer_size = peer_epc_features->bar_fixed_size[peer_barno];
> > > > > > >
> > > > > > >       /* Check if epc_features is populated incorrectly */
> > > > > > > -     if ((!IS_ALIGNED(size, align)))
> > > > > > > +     if (align && (!IS_ALIGNED(size, align)))
> > > > > > >               return -EINVAL;
> > > > > > >
> > > > > > >       spad_count = ntb->spad_count;
> > > > > > > diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > > > > > b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > > > > > > index 0ea85e1d292e..5e346c0a0f05 100644
> > > > > > > --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > > > > > > +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
> > > > > > > @@ -418,7 +418,7 @@ static int
> > epf_ntb_config_spad_bar_alloc(struct
> > > > > > epf_ntb *ntb)
> > > > > > >       size = epc_features->bar_fixed_size[barno];
> > > > > > >       align = epc_features->align;
> > > > > > >
> > > > > > > -     if ((!IS_ALIGNED(size, align)))
> > > > > > > +     if (align && !IS_ALIGNED(size, align))
> > > > > > >               return -EINVAL;
> > > > > > >
> > > > > > >       spad_count = ntb->spad_count;
> > > > > > > --
> > > > > > > 2.17.1
> > > > > > >
> > > > > >
> > > > > > --
> > > > > > மணிவண்ணன் சதாசிவம்
> > > >
> > > > Best,
> > > > Shunsuke
> > 
> > --
> > மணிவண்ணன் சதாசிவம்

-- 
மணிவண்ணன் சதாசிவம்