From: Honghui Zhang <[email protected]>
drivers/pci/pcie-mediatek.c:720:13-16: WARNING: Suspicious code. resource_size is maybe missing with mem
Generated by: scripts/coccinelle/api/resource_size.cocci
Signed-off-by: Honghui Zhang <[email protected]>
---
drivers/pci/controller/pcie-mediatek.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index e307166..0168376 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -654,7 +654,6 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
struct resource *mem = &pcie->mem;
const struct mtk_pcie_soc *soc = port->pcie->soc;
u32 val;
- size_t size;
int err;
/* MT7622 platforms need to enable LTSSM and ASPM from PCIe subsys */
@@ -706,8 +705,7 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
mtk_pcie_enable_msi(port);
/* Set AHB to PCIe translation windows */
- size = mem->end - mem->start;
- val = lower_32_bits(mem->start) | AHB2PCIE_SIZE(fls(size));
+ val = lower_32_bits(mem->start) | AHB2PCIE_SIZE(fls(resource_size(mem)));
writel(val, port->base + PCIE_AHB_TRANS_BASE0_L);
val = upper_32_bits(mem->start);
--
2.6.4
On Wed, Jan 02, 2019 at 02:03:53PM +0800, [email protected] wrote:
> From: Honghui Zhang <[email protected]>
>
> drivers/pci/pcie-mediatek.c:720:13-16: WARNING: Suspicious code. resource_size is maybe missing with mem
>
> Generated by: scripts/coccinelle/api/resource_size.cocci
>
> Signed-off-by: Honghui Zhang <[email protected]>
> ---
> drivers/pci/controller/pcie-mediatek.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> index e307166..0168376 100644
> --- a/drivers/pci/controller/pcie-mediatek.c
> +++ b/drivers/pci/controller/pcie-mediatek.c
> @@ -654,7 +654,6 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
> struct resource *mem = &pcie->mem;
> const struct mtk_pcie_soc *soc = port->pcie->soc;
> u32 val;
> - size_t size;
> int err;
>
> /* MT7622 platforms need to enable LTSSM and ASPM from PCIe subsys */
> @@ -706,8 +705,7 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
> mtk_pcie_enable_msi(port);
>
> /* Set AHB to PCIe translation windows */
> - size = mem->end - mem->start;
> - val = lower_32_bits(mem->start) | AHB2PCIE_SIZE(fls(size));
> + val = lower_32_bits(mem->start) | AHB2PCIE_SIZE(fls(resource_size(mem)));
checkpatch warns on this line, please make sure patches pass it before
posting them.
I will fix it up myself but please pay attention next time.
Thanks,
Lorenzo
> writel(val, port->base + PCIE_AHB_TRANS_BASE0_L);
>
> val = upper_32_bits(mem->start);
> --
> 2.6.4
>
Please pay attention to the changelog conventions, e.g., next time use
"PCI: mediatek: ..." so yours matches "git log --oneline
drivers/pci/controller/pcie-mediatek.c"
On Wed, Jan 02, 2019 at 02:03:53PM +0800, [email protected] wrote:
> From: Honghui Zhang <[email protected]>
>
> drivers/pci/pcie-mediatek.c:720:13-16: WARNING: Suspicious code. resource_size is maybe missing with mem
>
> Generated by: scripts/coccinelle/api/resource_size.cocci
This is not a changelog; it's only a restatement of the warning. Next
time please include a description of the change. It's OK if it
repeats the subject.
> Signed-off-by: Honghui Zhang <[email protected]>
> ---
> drivers/pci/controller/pcie-mediatek.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> index e307166..0168376 100644
> --- a/drivers/pci/controller/pcie-mediatek.c
> +++ b/drivers/pci/controller/pcie-mediatek.c
> @@ -654,7 +654,6 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
> struct resource *mem = &pcie->mem;
> const struct mtk_pcie_soc *soc = port->pcie->soc;
> u32 val;
> - size_t size;
> int err;
>
> /* MT7622 platforms need to enable LTSSM and ASPM from PCIe subsys */
> @@ -706,8 +705,7 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
> mtk_pcie_enable_msi(port);
>
> /* Set AHB to PCIe translation windows */
> - size = mem->end - mem->start;
> - val = lower_32_bits(mem->start) | AHB2PCIE_SIZE(fls(size));
> + val = lower_32_bits(mem->start) | AHB2PCIE_SIZE(fls(resource_size(mem)));
> writel(val, port->base + PCIE_AHB_TRANS_BASE0_L);
>
> val = upper_32_bits(mem->start);
> --
> 2.6.4
>
On Wed, Jan 02, 2019 at 02:03:53PM +0800, [email protected] wrote:
> From: Honghui Zhang <[email protected]>
>
> drivers/pci/pcie-mediatek.c:720:13-16: WARNING: Suspicious code. resource_size is maybe missing with mem
>
> Generated by: scripts/coccinelle/api/resource_size.cocci
>
> Signed-off-by: Honghui Zhang <[email protected]>
> ---
> drivers/pci/controller/pcie-mediatek.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> index e307166..0168376 100644
> --- a/drivers/pci/controller/pcie-mediatek.c
> +++ b/drivers/pci/controller/pcie-mediatek.c
> @@ -654,7 +654,6 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
> struct resource *mem = &pcie->mem;
> const struct mtk_pcie_soc *soc = port->pcie->soc;
> u32 val;
> - size_t size;
> int err;
>
> /* MT7622 platforms need to enable LTSSM and ASPM from PCIe subsys */
> @@ -706,8 +705,7 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
> mtk_pcie_enable_msi(port);
>
> /* Set AHB to PCIe translation windows */
> - size = mem->end - mem->start;
> - val = lower_32_bits(mem->start) | AHB2PCIE_SIZE(fls(size));
> + val = lower_32_bits(mem->start) | AHB2PCIE_SIZE(fls(resource_size(mem)));
This is actually a fairly interesting change because it effectively
changes this:
fls(mem->end - mem->start)
to this:
fls(mem->end - mem->start + 1)
And mem->end is the last valid address, so it changes something like
this:
fls(0xffff) # == 15
to this:
fls(0x10000) # == 16
So while this *looks* like a trivial warning fix, it likely fixes an
important bug, and it's worth pointing out what that bug is in the
changelog.
> writel(val, port->base + PCIE_AHB_TRANS_BASE0_L);
>
> val = upper_32_bits(mem->start);
> --
> 2.6.4
>
On Wed, Jan 30, 2019 at 12:33:47PM +0000, Lorenzo Pieralisi wrote:
> On Wed, Jan 02, 2019 at 02:03:53PM +0800, [email protected] wrote:
> > From: Honghui Zhang <[email protected]>
> >
> > drivers/pci/pcie-mediatek.c:720:13-16: WARNING: Suspicious code. resource_size is maybe missing with mem
> >
> > Generated by: scripts/coccinelle/api/resource_size.cocci
> >
> > Signed-off-by: Honghui Zhang <[email protected]>
> > ---
> > drivers/pci/controller/pcie-mediatek.c | 4 +---
> > 1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> > index e307166..0168376 100644
> > --- a/drivers/pci/controller/pcie-mediatek.c
> > +++ b/drivers/pci/controller/pcie-mediatek.c
> > @@ -654,7 +654,6 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
> > struct resource *mem = &pcie->mem;
> > const struct mtk_pcie_soc *soc = port->pcie->soc;
> > u32 val;
> > - size_t size;
> > int err;
> >
> > /* MT7622 platforms need to enable LTSSM and ASPM from PCIe subsys */
> > @@ -706,8 +705,7 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
> > mtk_pcie_enable_msi(port);
> >
> > /* Set AHB to PCIe translation windows */
> > - size = mem->end - mem->start;
> > - val = lower_32_bits(mem->start) | AHB2PCIE_SIZE(fls(size));
> > + val = lower_32_bits(mem->start) | AHB2PCIE_SIZE(fls(resource_size(mem)));
>
> checkpatch warns on this line, please make sure patches pass it before
> posting them.
I didn't actually run checkpatch myself, so I don't know why it
complained.
The patch you merged moves "mem_size = resource_size(mem)" higher up,
away from the previous location and its use, which makes it a little
harder to read.
Bjorn
On Wed, Jan 02, 2019 at 02:03:53PM +0800, [email protected] wrote:
> From: Honghui Zhang <[email protected]>
>
> drivers/pci/pcie-mediatek.c:720:13-16: WARNING: Suspicious code. resource_size is maybe missing with mem
>
> Generated by: scripts/coccinelle/api/resource_size.cocci
>
> Signed-off-by: Honghui Zhang <[email protected]>
> ---
> drivers/pci/controller/pcie-mediatek.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> index e307166..0168376 100644
> --- a/drivers/pci/controller/pcie-mediatek.c
> +++ b/drivers/pci/controller/pcie-mediatek.c
> @@ -654,7 +654,6 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
> struct resource *mem = &pcie->mem;
> const struct mtk_pcie_soc *soc = port->pcie->soc;
> u32 val;
> - size_t size;
> int err;
>
> /* MT7622 platforms need to enable LTSSM and ASPM from PCIe subsys */
> @@ -706,8 +705,7 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
> mtk_pcie_enable_msi(port);
>
> /* Set AHB to PCIe translation windows */
> - size = mem->end - mem->start;
> - val = lower_32_bits(mem->start) | AHB2PCIE_SIZE(fls(size));
> + val = lower_32_bits(mem->start) | AHB2PCIE_SIZE(fls(resource_size(mem)));
> writel(val, port->base + PCIE_AHB_TRANS_BASE0_L);
>
> val = upper_32_bits(mem->start);
Unrelated to this patch, but just below this:
/* Set PCIe to AXI translation memory space.*/
val = fls(0xffffffff) | WIN_ENABLE;
writel(val, port->base + PCIE_AXI_WINDOW0);
Can you double-check the use of "fls(0xffffffff)"? That expression is
a constant and I think evaluates to 31 (0x1f), i.e.,
val = 0x1f | WIN_ENABLE;
I don't know the hardware, so this might be correct, but
"fls(0xffffffff)" looks funny because I think it's the same as
"fls(0x80000000)".
Bjorn
On Wed, Jan 30, 2019 at 09:58:53AM -0600, Bjorn Helgaas wrote:
> On Wed, Jan 30, 2019 at 12:33:47PM +0000, Lorenzo Pieralisi wrote:
> > On Wed, Jan 02, 2019 at 02:03:53PM +0800, [email protected] wrote:
> > > From: Honghui Zhang <[email protected]>
> > >
> > > drivers/pci/pcie-mediatek.c:720:13-16: WARNING: Suspicious code. resource_size is maybe missing with mem
> > >
> > > Generated by: scripts/coccinelle/api/resource_size.cocci
> > >
> > > Signed-off-by: Honghui Zhang <[email protected]>
> > > ---
> > > drivers/pci/controller/pcie-mediatek.c | 4 +---
> > > 1 file changed, 1 insertion(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> > > index e307166..0168376 100644
> > > --- a/drivers/pci/controller/pcie-mediatek.c
> > > +++ b/drivers/pci/controller/pcie-mediatek.c
> > > @@ -654,7 +654,6 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
> > > struct resource *mem = &pcie->mem;
> > > const struct mtk_pcie_soc *soc = port->pcie->soc;
> > > u32 val;
> > > - size_t size;
> > > int err;
> > >
> > > /* MT7622 platforms need to enable LTSSM and ASPM from PCIe subsys */
> > > @@ -706,8 +705,7 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
> > > mtk_pcie_enable_msi(port);
> > >
> > > /* Set AHB to PCIe translation windows */
> > > - size = mem->end - mem->start;
> > > - val = lower_32_bits(mem->start) | AHB2PCIE_SIZE(fls(size));
> > > + val = lower_32_bits(mem->start) | AHB2PCIE_SIZE(fls(resource_size(mem)));
> >
> > checkpatch warns on this line, please make sure patches pass it before
> > posting them.
>
> I didn't actually run checkpatch myself, so I don't know why it
> complained.
WARNING: line over 80 characters
#35: FILE: drivers/pci/controller/pcie-mediatek.c:708:
+ val = lower_32_bits(mem->start) | AHB2PCIE_SIZE(fls(resource_size(mem)));
I do run it as a sanity check.
> The patch you merged moves "mem_size = resource_size(mem)" higher up,
> away from the previous location and its use, which makes it a little
> harder to read.
That's because it was how the original code (which as you pointed out
is likely buggy) was written.
Anyway patch dropped waiting for a v2 consistent with your review -
apologies for missing some key review points.
Lorenzo
On Wed, 2019-01-30 at 16:31 +0000, Lorenzo Pieralisi wrote:
> On Wed, Jan 30, 2019 at 09:58:53AM -0600, Bjorn Helgaas wrote:
> > On Wed, Jan 30, 2019 at 12:33:47PM +0000, Lorenzo Pieralisi wrote:
> > > On Wed, Jan 02, 2019 at 02:03:53PM +0800, [email protected] wrote:
> > > > From: Honghui Zhang <[email protected]>
> > > >
> > > > drivers/pci/pcie-mediatek.c:720:13-16: WARNING: Suspicious code. resource_size is maybe missing with mem
> > > >
> > > > Generated by: scripts/coccinelle/api/resource_size.cocci
> > > >
> > > > Signed-off-by: Honghui Zhang <[email protected]>
> > > > ---
> > > > drivers/pci/controller/pcie-mediatek.c | 4 +---
> > > > 1 file changed, 1 insertion(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> > > > index e307166..0168376 100644
> > > > --- a/drivers/pci/controller/pcie-mediatek.c
> > > > +++ b/drivers/pci/controller/pcie-mediatek.c
> > > > @@ -654,7 +654,6 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
> > > > struct resource *mem = &pcie->mem;
> > > > const struct mtk_pcie_soc *soc = port->pcie->soc;
> > > > u32 val;
> > > > - size_t size;
> > > > int err;
> > > >
> > > > /* MT7622 platforms need to enable LTSSM and ASPM from PCIe subsys */
> > > > @@ -706,8 +705,7 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
> > > > mtk_pcie_enable_msi(port);
> > > >
> > > > /* Set AHB to PCIe translation windows */
> > > > - size = mem->end - mem->start;
> > > > - val = lower_32_bits(mem->start) | AHB2PCIE_SIZE(fls(size));
> > > > + val = lower_32_bits(mem->start) | AHB2PCIE_SIZE(fls(resource_size(mem)));
> > >
> > > checkpatch warns on this line, please make sure patches pass it before
> > > posting them.
> >
> > I didn't actually run checkpatch myself, so I don't know why it
> > complained.
>
> WARNING: line over 80 characters
> #35: FILE: drivers/pci/controller/pcie-mediatek.c:708:
> + val = lower_32_bits(mem->start) | AHB2PCIE_SIZE(fls(resource_size(mem)));
>
> I do run it as a sanity check.
>
> > The patch you merged moves "mem_size = resource_size(mem)" higher up,
> > away from the previous location and its use, which makes it a little
> > harder to read.
>
> That's because it was how the original code (which as you pointed out
> is likely buggy) was written.
>
> Anyway patch dropped waiting for a v2 consistent with your review -
> apologies for missing some key review points.
>
Thanks for your comments, I will send a v2 and fix all the issue you
pointed out.
> Lorenzo
On Wed, 2019-01-30 at 09:49 -0600, Bjorn Helgaas wrote:
> On Wed, Jan 02, 2019 at 02:03:53PM +0800, [email protected] wrote:
> > From: Honghui Zhang <[email protected]>
> >
> > drivers/pci/pcie-mediatek.c:720:13-16: WARNING: Suspicious code. resource_size is maybe missing with mem
> >
> > Generated by: scripts/coccinelle/api/resource_size.cocci
> >
> > Signed-off-by: Honghui Zhang <[email protected]>
> > ---
> > drivers/pci/controller/pcie-mediatek.c | 4 +---
> > 1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> > index e307166..0168376 100644
> > --- a/drivers/pci/controller/pcie-mediatek.c
> > +++ b/drivers/pci/controller/pcie-mediatek.c
> > @@ -654,7 +654,6 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
> > struct resource *mem = &pcie->mem;
> > const struct mtk_pcie_soc *soc = port->pcie->soc;
> > u32 val;
> > - size_t size;
> > int err;
> >
> > /* MT7622 platforms need to enable LTSSM and ASPM from PCIe subsys */
> > @@ -706,8 +705,7 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
> > mtk_pcie_enable_msi(port);
> >
> > /* Set AHB to PCIe translation windows */
> > - size = mem->end - mem->start;
> > - val = lower_32_bits(mem->start) | AHB2PCIE_SIZE(fls(size));
> > + val = lower_32_bits(mem->start) | AHB2PCIE_SIZE(fls(resource_size(mem)));
>
> This is actually a fairly interesting change because it effectively
> changes this:
>
> fls(mem->end - mem->start)
>
> to this:
>
> fls(mem->end - mem->start + 1)
>
> And mem->end is the last valid address, so it changes something like
> this:
>
> fls(0xffff) # == 15
>
> to this:
>
> fls(0x10000) # == 16
>
> So while this *looks* like a trivial warning fix, it likely fixes an
> important bug, and it's worth pointing out what that bug is in the
> changelog.
>
> > writel(val, port->base + PCIE_AHB_TRANS_BASE0_L);
> >
> > val = upper_32_bits(mem->start);
This size will set the MMIO size, which means that the RC will translate
the MMIO access from mem->start to mem->end.
The real MMIO size is specified in devicetree, which is 0x1000_0000 for
both mt2712 and mt7622.
This change make the size from fls(0x1000_0000) to fls(0x1000_0001), not
really change the values.
I will update the commit message and add the information mentioned
above.
Thanks for your kindly review.
> > --
> > 2.6.4
> >
On Thu, 2019-01-31 at 11:03 +0800, Honghui Zhang wrote:
> On Wed, 2019-01-30 at 09:49 -0600, Bjorn Helgaas wrote:
> > On Wed, Jan 02, 2019 at 02:03:53PM +0800, [email protected] wrote:
> > > From: Honghui Zhang <[email protected]>
> > >
> > > drivers/pci/pcie-mediatek.c:720:13-16: WARNING: Suspicious code. resource_size is maybe missing with mem
> > >
> > > Generated by: scripts/coccinelle/api/resource_size.cocci
> > >
> > > Signed-off-by: Honghui Zhang <[email protected]>
> > > ---
> > > drivers/pci/controller/pcie-mediatek.c | 4 +---
> > > 1 file changed, 1 insertion(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> > > index e307166..0168376 100644
> > > --- a/drivers/pci/controller/pcie-mediatek.c
> > > +++ b/drivers/pci/controller/pcie-mediatek.c
> > > @@ -654,7 +654,6 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
> > > struct resource *mem = &pcie->mem;
> > > const struct mtk_pcie_soc *soc = port->pcie->soc;
> > > u32 val;
> > > - size_t size;
> > > int err;
> > >
> > > /* MT7622 platforms need to enable LTSSM and ASPM from PCIe subsys */
> > > @@ -706,8 +705,7 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
> > > mtk_pcie_enable_msi(port);
> > >
> > > /* Set AHB to PCIe translation windows */
> > > - size = mem->end - mem->start;
> > > - val = lower_32_bits(mem->start) | AHB2PCIE_SIZE(fls(size));
> > > + val = lower_32_bits(mem->start) | AHB2PCIE_SIZE(fls(resource_size(mem)));
> >
> > This is actually a fairly interesting change because it effectively
> > changes this:
> >
> > fls(mem->end - mem->start)
> >
> > to this:
> >
> > fls(mem->end - mem->start + 1)
> >
> > And mem->end is the last valid address, so it changes something like
> > this:
> >
> > fls(0xffff) # == 15
> >
> > to this:
> >
> > fls(0x10000) # == 16
> >
> > So while this *looks* like a trivial warning fix, it likely fixes an
> > important bug, and it's worth pointing out what that bug is in the
> > changelog.
> >
> > > writel(val, port->base + PCIE_AHB_TRANS_BASE0_L);
> > >
> > > val = upper_32_bits(mem->start);
>
> This size will set the MMIO size, which means that the RC will translate
> the MMIO access from mem->start to mem->end.
> The real MMIO size is specified in devicetree, which is 0x1000_0000 for
> both mt2712 and mt7622.
>
> This change make the size from fls(0x1000_0000) to fls(0x1000_0001), not
> really change the values.
>
> I will update the commit message and add the information mentioned
> above.
>
> Thanks for your kindly review.
I was wrong, after take a look at the resource parser function, that it
will initialize the res->end as res->start + res->size - 1.
But this change is still Ok since it will change the MMIO from
fls(0xfff_ffff) to fls(0x1000_0000), this just enlarge the MMIO
translate window size. The HW assigned MMIO is 0x1000_0000, but original
code set this translate window to fls(0xfff_ffff) = 0x800_0000 is fine
in most case since all the EP device we connect never asked a MMIO
window bigger than 0x800_0000. (As a matter of fact, most EP device will
only ask for 4MB MMIO window size.)
I will put this in the commit message.
thanks.
>
> > > --
> > > 2.6.4
> > >
>
>