Some PCI endpoint controllers have no alignment constraints, and the
epc_features->align becomes 0. In this case, IS_ALIGNED() in
epf_ntb_config_spad_bar_alloc() doesn't work well. Check for this before
IS_ALIGNED().
Signed-off-by: Shunsuke Mie <[email protected]>
---
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
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
> epf_ntb_config_spad_bar_alloc() doesn't work well. Check for this before
> IS_ALIGNED().
>
> Signed-off-by: Shunsuke Mie <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
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
>
--
மணிவண்ணன் சதாசிவம்
> -----Original Message-----
> From: Manivannan Sadhasivam <[email protected]>
> Sent: Tuesday, October 25, 2022 9:22 AM
> To: Shunsuke Mie <[email protected]>
> Cc: Jon Mason <[email protected]>; Dave Jiang <[email protected]>;
> Allen Hubbe <[email protected]>; Kishon Vijay Abraham I
> <[email protected]>; Lorenzo Pieralisi <[email protected]>; Krzysztof
> Wilczyński <[email protected]>; Bjorn Helgaas <[email protected]>;
> [email protected]; [email protected]; [email protected]
> 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.
> > epf_ntb_config_spad_bar_alloc() doesn't work well. Check for this before
> > IS_ALIGNED().
> >
> > Signed-off-by: Shunsuke Mie <[email protected]>
>
> Reviewed-by: Manivannan Sadhasivam
> <[email protected]>
>
> 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
> >
>
> --
> மணிவண்ணன் சதாசிவம்
Hi Frank,
2022年10月26日(水) 1:07 Frank Li <[email protected]>:
>
>
>
> > -----Original Message-----
> > From: Manivannan Sadhasivam <[email protected]>
> > Sent: Tuesday, October 25, 2022 9:22 AM
> > To: Shunsuke Mie <[email protected]>
> > Cc: Jon Mason <[email protected]>; Dave Jiang <[email protected]>;
> > Allen Hubbe <[email protected]>; Kishon Vijay Abraham I
> > <[email protected]>; Lorenzo Pieralisi <[email protected]>; Krzysztof
> > Wilczyński <[email protected]>; Bjorn Helgaas <[email protected]>;
> > [email protected]; [email protected]; [email protected]
> > 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.
> > > epf_ntb_config_spad_bar_alloc() doesn't work well. Check for this before
> > > IS_ALIGNED().
> > >
> > > Signed-off-by: Shunsuke Mie <[email protected]>
> >
> > Reviewed-by: Manivannan Sadhasivam
> > <[email protected]>
> >
> > 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
> -----Original Message-----
> From: Shunsuke Mie <[email protected]>
> Sent: Wednesday, October 26, 2022 8:43 PM
> To: Frank Li <[email protected]>
> Cc: Manivannan Sadhasivam <[email protected]>; Jon
> Mason <[email protected]>; Dave Jiang <[email protected]>; Allen
> Hubbe <[email protected]>; Kishon Vijay Abraham I <[email protected]>;
> Lorenzo Pieralisi <[email protected]>; Krzysztof Wilczyński
> <[email protected]>; Bjorn Helgaas <[email protected]>;
> [email protected]; [email protected]; [email protected]
> 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 <[email protected]>:
> >
> >
> >
> > > -----Original Message-----
> > > From: Manivannan Sadhasivam <[email protected]>
> > > Sent: Tuesday, October 25, 2022 9:22 AM
> > > To: Shunsuke Mie <[email protected]>
> > > Cc: Jon Mason <[email protected]>; Dave Jiang <[email protected]>;
> > > Allen Hubbe <[email protected]>; Kishon Vijay Abraham I
> > > <[email protected]>; Lorenzo Pieralisi <[email protected]>; Krzysztof
> > > Wilczyński <[email protected]>; Bjorn Helgaas <[email protected]>;
> > > [email protected]; [email protected]; linux-
> [email protected]
> > > 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.
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 <[email protected]>
> > >
> > > Reviewed-by: Manivannan Sadhasivam
> > > <[email protected]>
> > >
> > > 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
On Thu, Oct 27, 2022 at 02:35:56PM +0000, Frank Li wrote:
>
>
> > -----Original Message-----
> > From: Shunsuke Mie <[email protected]>
> > Sent: Wednesday, October 26, 2022 8:43 PM
> > To: Frank Li <[email protected]>
> > Cc: Manivannan Sadhasivam <[email protected]>; Jon
> > Mason <[email protected]>; Dave Jiang <[email protected]>; Allen
> > Hubbe <[email protected]>; Kishon Vijay Abraham I <[email protected]>;
> > Lorenzo Pieralisi <[email protected]>; Krzysztof Wilczyński
> > <[email protected]>; Bjorn Helgaas <[email protected]>;
> > [email protected]; [email protected]; [email protected]
> > 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 <[email protected]>:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Manivannan Sadhasivam <[email protected]>
> > > > Sent: Tuesday, October 25, 2022 9:22 AM
> > > > To: Shunsuke Mie <[email protected]>
> > > > Cc: Jon Mason <[email protected]>; Dave Jiang <[email protected]>;
> > > > Allen Hubbe <[email protected]>; Kishon Vijay Abraham I
> > > > <[email protected]>; Lorenzo Pieralisi <[email protected]>; Krzysztof
> > > > Wilczyński <[email protected]>; Bjorn Helgaas <[email protected]>;
> > > > [email protected]; [email protected]; linux-
> > [email protected]
> > > > 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?
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 <[email protected]>
> > > >
> > > > Reviewed-by: Manivannan Sadhasivam
> > > > <[email protected]>
> > > >
> > > > 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
--
மணிவண்ணன் சதாசிவம்
> -----Original Message-----
> From: Manivannan Sadhasivam <[email protected]>
> Sent: Thursday, October 27, 2022 10:12 AM
> To: Frank Li <[email protected]>
> Cc: Shunsuke Mie <[email protected]>; Jon Mason <[email protected]>; Dave
> Jiang <[email protected]>; Allen Hubbe <[email protected]>; Kishon
> Vijay Abraham I <[email protected]>; Lorenzo Pieralisi <[email protected]>;
> Krzysztof Wilczyński <[email protected]>; Bjorn Helgaas
> <[email protected]>; [email protected]; [email protected];
> [email protected]
> 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 <[email protected]>
> > > Sent: Wednesday, October 26, 2022 8:43 PM
> > > To: Frank Li <[email protected]>
> > > Cc: Manivannan Sadhasivam <[email protected]>; Jon
> > > Mason <[email protected]>; Dave Jiang <[email protected]>; Allen
> > > Hubbe <[email protected]>; Kishon Vijay Abraham I <[email protected]>;
> > > Lorenzo Pieralisi <[email protected]>; Krzysztof Wilczyński
> > > <[email protected]>; Bjorn Helgaas <[email protected]>;
> > > [email protected]; [email protected]; linux-
> [email protected]
> > > 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 <[email protected]>:
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Manivannan Sadhasivam <[email protected]>
> > > > > Sent: Tuesday, October 25, 2022 9:22 AM
> > > > > To: Shunsuke Mie <[email protected]>
> > > > > Cc: Jon Mason <[email protected]>; Dave Jiang
> <[email protected]>;
> > > > > Allen Hubbe <[email protected]>; Kishon Vijay Abraham I
> > > > > <[email protected]>; Lorenzo Pieralisi <[email protected]>; Krzysztof
> > > > > Wilczyński <[email protected]>; Bjorn Helgaas <[email protected]>;
> > > > > [email protected]; [email protected]; linux-
> > > [email protected]
> > > > > 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.
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 <[email protected]>
> > > > >
> > > > > Reviewed-by: Manivannan Sadhasivam
> > > > > <[email protected]>
> > > > >
> > > > > 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
>
> --
> மணிவண்ணன் சதாசிவம்
[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 <[email protected]>
> > Sent: Thursday, October 27, 2022 10:12 AM
> > To: Frank Li <[email protected]>
> > Cc: Shunsuke Mie <[email protected]>; Jon Mason <[email protected]>; Dave
> > Jiang <[email protected]>; Allen Hubbe <[email protected]>; Kishon
> > Vijay Abraham I <[email protected]>; Lorenzo Pieralisi <[email protected]>;
> > Krzysztof Wilczyński <[email protected]>; Bjorn Helgaas
> > <[email protected]>; [email protected]; [email protected];
> > [email protected]
> > 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 <[email protected]>
> > > > Sent: Wednesday, October 26, 2022 8:43 PM
> > > > To: Frank Li <[email protected]>
> > > > Cc: Manivannan Sadhasivam <[email protected]>; Jon
> > > > Mason <[email protected]>; Dave Jiang <[email protected]>; Allen
> > > > Hubbe <[email protected]>; Kishon Vijay Abraham I <[email protected]>;
> > > > Lorenzo Pieralisi <[email protected]>; Krzysztof Wilczyński
> > > > <[email protected]>; Bjorn Helgaas <[email protected]>;
> > > > [email protected]; [email protected]; linux-
> > [email protected]
> > > > 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 <[email protected]>:
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Manivannan Sadhasivam <[email protected]>
> > > > > > Sent: Tuesday, October 25, 2022 9:22 AM
> > > > > > To: Shunsuke Mie <[email protected]>
> > > > > > Cc: Jon Mason <[email protected]>; Dave Jiang
> > <[email protected]>;
> > > > > > Allen Hubbe <[email protected]>; Kishon Vijay Abraham I
> > > > > > <[email protected]>; Lorenzo Pieralisi <[email protected]>; Krzysztof
> > > > > > Wilczyński <[email protected]>; Bjorn Helgaas <[email protected]>;
> > > > > > [email protected]; [email protected]; linux-
> > > > [email protected]
> > > > > > 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 <[email protected]>
> > > > > >
> > > > > > Reviewed-by: Manivannan Sadhasivam
> > > > > > <[email protected]>
> > > > > >
> > > > > > 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
> >
> > --
> > மணிவண்ணன் சதாசிவம்
--
மணிவண்ணன் சதாசிவம்
> -----Original Message-----
> From: Manivannan Sadhasivam <[email protected]>
> Sent: Thursday, October 27, 2022 11:10 AM
> To: Frank Li <[email protected]>
> Cc: Shunsuke Mie <[email protected]>; Jon Mason <[email protected]>; Dave
> Jiang <[email protected]>; Allen Hubbe <[email protected]>;
> [email protected]; Lorenzo Pieralisi <[email protected]>; Krzysztof
> Wilczyński <[email protected]>; Bjorn Helgaas <[email protected]>;
> [email protected]; [email protected]; [email protected]
> Subject: Re: [EXT] Re: [PATCH v2] PCI: endpoint: pci-epf-{,v}ntb: fix a check for
> no epc alignment constraint
>
> Caution: EXT Email
>
> [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 <[email protected]>
> > > Sent: Thursday, October 27, 2022 10:12 AM
> > > To: Frank Li <[email protected]>
> > > Cc: Shunsuke Mie <[email protected]>; Jon Mason <[email protected]>;
> Dave
> > > Jiang <[email protected]>; Allen Hubbe <[email protected]>;
> Kishon
> > > Vijay Abraham I <[email protected]>; Lorenzo Pieralisi
> <[email protected]>;
> > > Krzysztof Wilczyński <[email protected]>; Bjorn Helgaas
> > > <[email protected]>; [email protected]; [email protected];
> > > [email protected]
> > > 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 <[email protected]>
> > > > > Sent: Wednesday, October 26, 2022 8:43 PM
> > > > > To: Frank Li <[email protected]>
> > > > > Cc: Manivannan Sadhasivam <[email protected]>;
> Jon
> > > > > Mason <[email protected]>; Dave Jiang <[email protected]>;
> Allen
> > > > > Hubbe <[email protected]>; Kishon Vijay Abraham I
> <[email protected]>;
> > > > > Lorenzo Pieralisi <[email protected]>; Krzysztof Wilczyński
> > > > > <[email protected]>; Bjorn Helgaas <[email protected]>;
> > > > > [email protected]; [email protected]; linux-
> > > [email protected]
> > > > > 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 <[email protected]>:
> > > > > >
> > > > > >
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Manivannan Sadhasivam
> <[email protected]>
> > > > > > > Sent: Tuesday, October 25, 2022 9:22 AM
> > > > > > > To: Shunsuke Mie <[email protected]>
> > > > > > > Cc: Jon Mason <[email protected]>; Dave Jiang
> > > <[email protected]>;
> > > > > > > Allen Hubbe <[email protected]>; Kishon Vijay Abraham I
> > > > > > > <[email protected]>; Lorenzo Pieralisi <[email protected]>;
> Krzysztof
> > > > > > > Wilczyński <[email protected]>; Bjorn Helgaas
> <[email protected]>;
> > > > > > > [email protected]; [email protected]; linux-
> > > > > [email protected]
> > > > > > > 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.
I keep my opinion. I think EP controller have not reported correct data.
Hardware register also can be set 0 as no alignment means.
It broken "align" conversion.
If most people prefer this way, I suggest change api document at
Include/linux/pci-epc.h to explicitly said 0 is validate option.
>
> 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 <[email protected]>
> > > > > > >
> > > > > > > Reviewed-by: Manivannan Sadhasivam
> > > > > > > <[email protected]>
> > > > > > >
> > > > > > > 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
> > >
> > > --
> > > மணிவண்ணன் சதாசிவம்
>
> --
> மணிவண்ணன் சதாசிவம்
2022年10月28日(金) 1:25 Frank Li <[email protected]>:
>
>
>
> > -----Original Message-----
> > From: Manivannan Sadhasivam <[email protected]>
> > Sent: Thursday, October 27, 2022 11:10 AM
> > To: Frank Li <[email protected]>
> > Cc: Shunsuke Mie <[email protected]>; Jon Mason <[email protected]>; Dave
> > Jiang <[email protected]>; Allen Hubbe <[email protected]>;
> > [email protected]; Lorenzo Pieralisi <[email protected]>; Krzysztof
> > Wilczyński <[email protected]>; Bjorn Helgaas <[email protected]>;
> > [email protected]; [email protected]; [email protected]
> > Subject: Re: [EXT] Re: [PATCH v2] PCI: endpoint: pci-epf-{,v}ntb: fix a check for
> > no epc alignment constraint
> >
> > Caution: EXT Email
> >
> > [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 <[email protected]>
> > > > Sent: Thursday, October 27, 2022 10:12 AM
> > > > To: Frank Li <[email protected]>
> > > > Cc: Shunsuke Mie <[email protected]>; Jon Mason <[email protected]>;
> > Dave
> > > > Jiang <[email protected]>; Allen Hubbe <[email protected]>;
> > Kishon
> > > > Vijay Abraham I <[email protected]>; Lorenzo Pieralisi
> > <[email protected]>;
> > > > Krzysztof Wilczyński <[email protected]>; Bjorn Helgaas
> > > > <[email protected]>; [email protected]; [email protected];
> > > > [email protected]
> > > > 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 <[email protected]>
> > > > > > Sent: Wednesday, October 26, 2022 8:43 PM
> > > > > > To: Frank Li <[email protected]>
> > > > > > Cc: Manivannan Sadhasivam <[email protected]>;
> > Jon
> > > > > > Mason <[email protected]>; Dave Jiang <[email protected]>;
> > Allen
> > > > > > Hubbe <[email protected]>; Kishon Vijay Abraham I
> > <[email protected]>;
> > > > > > Lorenzo Pieralisi <[email protected]>; Krzysztof Wilczyński
> > > > > > <[email protected]>; Bjorn Helgaas <[email protected]>;
> > > > > > [email protected]; [email protected]; linux-
> > > > [email protected]
> > > > > > 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 <[email protected]>:
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > > -----Original Message-----
> > > > > > > > From: Manivannan Sadhasivam
> > <[email protected]>
> > > > > > > > Sent: Tuesday, October 25, 2022 9:22 AM
> > > > > > > > To: Shunsuke Mie <[email protected]>
> > > > > > > > Cc: Jon Mason <[email protected]>; Dave Jiang
> > > > <[email protected]>;
> > > > > > > > Allen Hubbe <[email protected]>; Kishon Vijay Abraham I
> > > > > > > > <[email protected]>; Lorenzo Pieralisi <[email protected]>;
> > Krzysztof
> > > > > > > > Wilczyński <[email protected]>; Bjorn Helgaas
> > <[email protected]>;
> > > > > > > > [email protected]; [email protected]; linux-
> > > > > > [email protected]
> > > > > > > > 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.
>
> I keep my opinion. I think EP controller have not reported correct data.
> Hardware register also can be set 0 as no alignment means.
> It broken "align" conversion.
It is certainly true. In addition, I don't think it is a good design
for the framework to
require the same check for all ep function drivers. like this patch.
But I can't be determined, so I'm going to look into other
drivers/subsystems to find a
common and better way.
> If most people prefer this way, I suggest change api document at
> Include/linux/pci-epc.h to explicitly said 0 is validate option.
Yes. If we take the way, I'll add the explanation.
> >
> > 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 <[email protected]>
> > > > > > > >
> > > > > > > > Reviewed-by: Manivannan Sadhasivam
> > > > > > > > <[email protected]>
> > > > > > > >
> > > > > > > > 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
> > > >
> > > > --
> > > > மணிவண்ணன் சதாசிவம்
> >
> > --
> > மணிவண்ணன் சதாசிவம்
Best,
Shunsuke