| Message ID | 20220922092357.123732-1-mie@igel.co.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | [v2] PCI: endpoint: pci-epf-{,v}ntb: fix a check for no epc alignment constraint | expand |
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 <mie@igel.co.jp> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> 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 <manivannan.sadhasivam@linaro.org> > Sent: Tuesday, October 25, 2022 9:22 AM > To: Shunsuke Mie <mie@igel.co.jp> > Cc: Jon Mason <jdmason@kudzu.us>; Dave Jiang <dave.jiang@intel.com>; > Allen Hubbe <allenbh@gmail.com>; Kishon Vijay Abraham I > <kishon@ti.com>; Lorenzo Pieralisi <lpieralisi@kernel.org>; Krzysztof > Wilczyński <kw@linux.com>; Bjorn Helgaas <bhelgaas@google.com>; > ntb@lists.linux.dev; linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org > 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 <mie@igel.co.jp> > > Reviewed-by: Manivannan Sadhasivam > <manivannan.sadhasivam@linaro.org> > > 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 <frank.li@nxp.com>: > > > > > -----Original Message----- > > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > Sent: Tuesday, October 25, 2022 9:22 AM > > To: Shunsuke Mie <mie@igel.co.jp> > > Cc: Jon Mason <jdmason@kudzu.us>; Dave Jiang <dave.jiang@intel.com>; > > Allen Hubbe <allenbh@gmail.com>; Kishon Vijay Abraham I > > <kishon@ti.com>; Lorenzo Pieralisi <lpieralisi@kernel.org>; Krzysztof > > Wilczyński <kw@linux.com>; Bjorn Helgaas <bhelgaas@google.com>; > > ntb@lists.linux.dev; linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org > > 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 <mie@igel.co.jp> > > > > Reviewed-by: Manivannan Sadhasivam > > <manivannan.sadhasivam@linaro.org> > > > > 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 <mie@igel.co.jp> > Sent: Wednesday, October 26, 2022 8:43 PM > To: Frank Li <frank.li@nxp.com> > Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>; Jon > Mason <jdmason@kudzu.us>; Dave Jiang <dave.jiang@intel.com>; Allen > Hubbe <allenbh@gmail.com>; Kishon Vijay Abraham I <kishon@ti.com>; > Lorenzo Pieralisi <lpieralisi@kernel.org>; Krzysztof Wilczyński > <kw@linux.com>; Bjorn Helgaas <bhelgaas@google.com>; > ntb@lists.linux.dev; linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org > 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@nxp.com>: > > > > > > > > > -----Original Message----- > > > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > Sent: Tuesday, October 25, 2022 9:22 AM > > > To: Shunsuke Mie <mie@igel.co.jp> > > > Cc: Jon Mason <jdmason@kudzu.us>; Dave Jiang <dave.jiang@intel.com>; > > > Allen Hubbe <allenbh@gmail.com>; Kishon Vijay Abraham I > > > <kishon@ti.com>; Lorenzo Pieralisi <lpieralisi@kernel.org>; Krzysztof > > > Wilczyński <kw@linux.com>; Bjorn Helgaas <bhelgaas@google.com>; > > > ntb@lists.linux.dev; linux-pci@vger.kernel.org; linux- > kernel@vger.kernel.org > > > 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 <mie@igel.co.jp> > > > > > > Reviewed-by: Manivannan Sadhasivam > > > <manivannan.sadhasivam@linaro.org> > > > > > > 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 <mie@igel.co.jp> > > Sent: Wednesday, October 26, 2022 8:43 PM > > To: Frank Li <frank.li@nxp.com> > > Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>; Jon > > Mason <jdmason@kudzu.us>; Dave Jiang <dave.jiang@intel.com>; Allen > > Hubbe <allenbh@gmail.com>; Kishon Vijay Abraham I <kishon@ti.com>; > > Lorenzo Pieralisi <lpieralisi@kernel.org>; Krzysztof Wilczyński > > <kw@linux.com>; Bjorn Helgaas <bhelgaas@google.com>; > > ntb@lists.linux.dev; linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org > > 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@nxp.com>: > > > > > > > > > > > > > -----Original Message----- > > > > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > Sent: Tuesday, October 25, 2022 9:22 AM > > > > To: Shunsuke Mie <mie@igel.co.jp> > > > > Cc: Jon Mason <jdmason@kudzu.us>; Dave Jiang <dave.jiang@intel.com>; > > > > Allen Hubbe <allenbh@gmail.com>; Kishon Vijay Abraham I > > > > <kishon@ti.com>; Lorenzo Pieralisi <lpieralisi@kernel.org>; Krzysztof > > > > Wilczyński <kw@linux.com>; Bjorn Helgaas <bhelgaas@google.com>; > > > > ntb@lists.linux.dev; linux-pci@vger.kernel.org; linux- > > kernel@vger.kernel.org > > > > 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 <mie@igel.co.jp> > > > > > > > > Reviewed-by: Manivannan Sadhasivam > > > > <manivannan.sadhasivam@linaro.org> > > > > > > > > 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 <manivannan.sadhasivam@linaro.org> > Sent: Thursday, October 27, 2022 10:12 AM > To: Frank Li <frank.li@nxp.com> > Cc: Shunsuke Mie <mie@igel.co.jp>; Jon Mason <jdmason@kudzu.us>; Dave > Jiang <dave.jiang@intel.com>; Allen Hubbe <allenbh@gmail.com>; Kishon > Vijay Abraham I <kishon@ti.com>; Lorenzo Pieralisi <lpieralisi@kernel.org>; > Krzysztof Wilczyński <kw@linux.com>; Bjorn Helgaas > <bhelgaas@google.com>; ntb@lists.linux.dev; linux-pci@vger.kernel.org; > linux-kernel@vger.kernel.org > 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@igel.co.jp> > > > Sent: Wednesday, October 26, 2022 8:43 PM > > > To: Frank Li <frank.li@nxp.com> > > > Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>; Jon > > > Mason <jdmason@kudzu.us>; Dave Jiang <dave.jiang@intel.com>; Allen > > > Hubbe <allenbh@gmail.com>; Kishon Vijay Abraham I <kishon@ti.com>; > > > Lorenzo Pieralisi <lpieralisi@kernel.org>; Krzysztof Wilczyński > > > <kw@linux.com>; Bjorn Helgaas <bhelgaas@google.com>; > > > ntb@lists.linux.dev; linux-pci@vger.kernel.org; linux- > kernel@vger.kernel.org > > > 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@nxp.com>: > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > > Sent: Tuesday, October 25, 2022 9:22 AM > > > > > To: Shunsuke Mie <mie@igel.co.jp> > > > > > Cc: Jon Mason <jdmason@kudzu.us>; Dave Jiang > <dave.jiang@intel.com>; > > > > > Allen Hubbe <allenbh@gmail.com>; Kishon Vijay Abraham I > > > > > <kishon@ti.com>; Lorenzo Pieralisi <lpieralisi@kernel.org>; Krzysztof > > > > > Wilczyński <kw@linux.com>; Bjorn Helgaas <bhelgaas@google.com>; > > > > > ntb@lists.linux.dev; linux-pci@vger.kernel.org; linux- > > > kernel@vger.kernel.org > > > > > 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 <mie@igel.co.jp> > > > > > > > > > > Reviewed-by: Manivannan Sadhasivam > > > > > <manivannan.sadhasivam@linaro.org> > > > > > > > > > > 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 <manivannan.sadhasivam@linaro.org> > > Sent: Thursday, October 27, 2022 10:12 AM > > To: Frank Li <frank.li@nxp.com> > > Cc: Shunsuke Mie <mie@igel.co.jp>; Jon Mason <jdmason@kudzu.us>; Dave > > Jiang <dave.jiang@intel.com>; Allen Hubbe <allenbh@gmail.com>; Kishon > > Vijay Abraham I <kishon@ti.com>; Lorenzo Pieralisi <lpieralisi@kernel.org>; > > Krzysztof Wilczyński <kw@linux.com>; Bjorn Helgaas > > <bhelgaas@google.com>; ntb@lists.linux.dev; linux-pci@vger.kernel.org; > > linux-kernel@vger.kernel.org > > 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@igel.co.jp> > > > > Sent: Wednesday, October 26, 2022 8:43 PM > > > > To: Frank Li <frank.li@nxp.com> > > > > Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>; Jon > > > > Mason <jdmason@kudzu.us>; Dave Jiang <dave.jiang@intel.com>; Allen > > > > Hubbe <allenbh@gmail.com>; Kishon Vijay Abraham I <kishon@ti.com>; > > > > Lorenzo Pieralisi <lpieralisi@kernel.org>; Krzysztof Wilczyński > > > > <kw@linux.com>; Bjorn Helgaas <bhelgaas@google.com>; > > > > ntb@lists.linux.dev; linux-pci@vger.kernel.org; linux- > > kernel@vger.kernel.org > > > > 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@nxp.com>: > > > > > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > > > Sent: Tuesday, October 25, 2022 9:22 AM > > > > > > To: Shunsuke Mie <mie@igel.co.jp> > > > > > > Cc: Jon Mason <jdmason@kudzu.us>; Dave Jiang > > <dave.jiang@intel.com>; > > > > > > Allen Hubbe <allenbh@gmail.com>; Kishon Vijay Abraham I > > > > > > <kishon@ti.com>; Lorenzo Pieralisi <lpieralisi@kernel.org>; Krzysztof > > > > > > Wilczyński <kw@linux.com>; Bjorn Helgaas <bhelgaas@google.com>; > > > > > > ntb@lists.linux.dev; linux-pci@vger.kernel.org; linux- > > > > kernel@vger.kernel.org > > > > > > 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@igel.co.jp> > > > > > > > > > > > > Reviewed-by: Manivannan Sadhasivam > > > > > > <manivannan.sadhasivam@linaro.org> > > > > > > > > > > > > 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 <manivannan.sadhasivam@linaro.org> > Sent: Thursday, October 27, 2022 11:10 AM > To: Frank Li <frank.li@nxp.com> > Cc: Shunsuke Mie <mie@igel.co.jp>; Jon Mason <jdmason@kudzu.us>; Dave > Jiang <dave.jiang@intel.com>; Allen Hubbe <allenbh@gmail.com>; > kishon@kernel.org; Lorenzo Pieralisi <lpieralisi@kernel.org>; Krzysztof > Wilczyński <kw@linux.com>; Bjorn Helgaas <bhelgaas@google.com>; > ntb@lists.linux.dev; linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org > 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 <manivannan.sadhasivam@linaro.org> > > > Sent: Thursday, October 27, 2022 10:12 AM > > > To: Frank Li <frank.li@nxp.com> > > > Cc: Shunsuke Mie <mie@igel.co.jp>; Jon Mason <jdmason@kudzu.us>; > Dave > > > Jiang <dave.jiang@intel.com>; Allen Hubbe <allenbh@gmail.com>; > Kishon > > > Vijay Abraham I <kishon@ti.com>; Lorenzo Pieralisi > <lpieralisi@kernel.org>; > > > Krzysztof Wilczyński <kw@linux.com>; Bjorn Helgaas > > > <bhelgaas@google.com>; ntb@lists.linux.dev; linux-pci@vger.kernel.org; > > > linux-kernel@vger.kernel.org > > > 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@igel.co.jp> > > > > > Sent: Wednesday, October 26, 2022 8:43 PM > > > > > To: Frank Li <frank.li@nxp.com> > > > > > Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>; > Jon > > > > > Mason <jdmason@kudzu.us>; Dave Jiang <dave.jiang@intel.com>; > Allen > > > > > Hubbe <allenbh@gmail.com>; Kishon Vijay Abraham I > <kishon@ti.com>; > > > > > Lorenzo Pieralisi <lpieralisi@kernel.org>; Krzysztof Wilczyński > > > > > <kw@linux.com>; Bjorn Helgaas <bhelgaas@google.com>; > > > > > ntb@lists.linux.dev; linux-pci@vger.kernel.org; linux- > > > kernel@vger.kernel.org > > > > > 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@nxp.com>: > > > > > > > > > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > > From: Manivannan Sadhasivam > <manivannan.sadhasivam@linaro.org> > > > > > > > Sent: Tuesday, October 25, 2022 9:22 AM > > > > > > > To: Shunsuke Mie <mie@igel.co.jp> > > > > > > > Cc: Jon Mason <jdmason@kudzu.us>; Dave Jiang > > > <dave.jiang@intel.com>; > > > > > > > Allen Hubbe <allenbh@gmail.com>; Kishon Vijay Abraham I > > > > > > > <kishon@ti.com>; Lorenzo Pieralisi <lpieralisi@kernel.org>; > Krzysztof > > > > > > > Wilczyński <kw@linux.com>; Bjorn Helgaas > <bhelgaas@google.com>; > > > > > > > ntb@lists.linux.dev; linux-pci@vger.kernel.org; linux- > > > > > kernel@vger.kernel.org > > > > > > > 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 <mie@igel.co.jp> > > > > > > > > > > > > > > Reviewed-by: Manivannan Sadhasivam > > > > > > > <manivannan.sadhasivam@linaro.org> > > > > > > > > > > > > > > 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 <frank.li@nxp.com>: > > > > > -----Original Message----- > > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > Sent: Thursday, October 27, 2022 11:10 AM > > To: Frank Li <frank.li@nxp.com> > > Cc: Shunsuke Mie <mie@igel.co.jp>; Jon Mason <jdmason@kudzu.us>; Dave > > Jiang <dave.jiang@intel.com>; Allen Hubbe <allenbh@gmail.com>; > > kishon@kernel.org; Lorenzo Pieralisi <lpieralisi@kernel.org>; Krzysztof > > Wilczyński <kw@linux.com>; Bjorn Helgaas <bhelgaas@google.com>; > > ntb@lists.linux.dev; linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org > > 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 <manivannan.sadhasivam@linaro.org> > > > > Sent: Thursday, October 27, 2022 10:12 AM > > > > To: Frank Li <frank.li@nxp.com> > > > > Cc: Shunsuke Mie <mie@igel.co.jp>; Jon Mason <jdmason@kudzu.us>; > > Dave > > > > Jiang <dave.jiang@intel.com>; Allen Hubbe <allenbh@gmail.com>; > > Kishon > > > > Vijay Abraham I <kishon@ti.com>; Lorenzo Pieralisi > > <lpieralisi@kernel.org>; > > > > Krzysztof Wilczyński <kw@linux.com>; Bjorn Helgaas > > > > <bhelgaas@google.com>; ntb@lists.linux.dev; linux-pci@vger.kernel.org; > > > > linux-kernel@vger.kernel.org > > > > 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@igel.co.jp> > > > > > > Sent: Wednesday, October 26, 2022 8:43 PM > > > > > > To: Frank Li <frank.li@nxp.com> > > > > > > Cc: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>; > > Jon > > > > > > Mason <jdmason@kudzu.us>; Dave Jiang <dave.jiang@intel.com>; > > Allen > > > > > > Hubbe <allenbh@gmail.com>; Kishon Vijay Abraham I > > <kishon@ti.com>; > > > > > > Lorenzo Pieralisi <lpieralisi@kernel.org>; Krzysztof Wilczyński > > > > > > <kw@linux.com>; Bjorn Helgaas <bhelgaas@google.com>; > > > > > > ntb@lists.linux.dev; linux-pci@vger.kernel.org; linux- > > > > kernel@vger.kernel.org > > > > > > 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@nxp.com>: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -----Original Message----- > > > > > > > > From: Manivannan Sadhasivam > > <manivannan.sadhasivam@linaro.org> > > > > > > > > Sent: Tuesday, October 25, 2022 9:22 AM > > > > > > > > To: Shunsuke Mie <mie@igel.co.jp> > > > > > > > > Cc: Jon Mason <jdmason@kudzu.us>; Dave Jiang > > > > <dave.jiang@intel.com>; > > > > > > > > Allen Hubbe <allenbh@gmail.com>; Kishon Vijay Abraham I > > > > > > > > <kishon@ti.com>; Lorenzo Pieralisi <lpieralisi@kernel.org>; > > Krzysztof > > > > > > > > Wilczyński <kw@linux.com>; Bjorn Helgaas > > <bhelgaas@google.com>; > > > > > > > > ntb@lists.linux.dev; linux-pci@vger.kernel.org; linux- > > > > > > kernel@vger.kernel.org > > > > > > > > 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 <mie@igel.co.jp> > > > > > > > > > > > > > > > > Reviewed-by: Manivannan Sadhasivam > > > > > > > > <manivannan.sadhasivam@linaro.org> > > > > > > > > > > > > > > > > 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
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;
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 <mie@igel.co.jp> --- 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(-)