Re: [PATCH] PCI: Mediatek: Use resource_size function on resource object

Re: [PATCH] PCI: Mediatek: Use resource_size function on resource object

[Lorenzo Pieralisi]

On Wed, Jan 02, 2019 at 02:03:53PM +0800, honghui.zhang@mediatek.com wrote:
> From: Honghui Zhang <honghui.zhang@mediatek.com>
> 
> 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 <honghui.zhang@mediatek.com>
> ---
>  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
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PCI: Mediatek: Use resource_size function on resource object

[Bjorn Helgaas]

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, honghui.zhang@mediatek.com wrote:
> From: Honghui Zhang <honghui.zhang@mediatek.com>
> 
> 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 <honghui.zhang@mediatek.com>
> ---
>  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
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PCI: Mediatek: Use resource_size function on resource object

[Bjorn Helgaas]

On Wed, Jan 02, 2019 at 02:03:53PM +0800, honghui.zhang@mediatek.com wrote:
> From: Honghui Zhang <honghui.zhang@mediatek.com>
> 
> 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 <honghui.zhang@mediatek.com>
> ---
>  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
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PCI: Mediatek: Use resource_size function on resource object

[Bjorn Helgaas]

On Wed, Jan 30, 2019 at 12:33:47PM +0000, Lorenzo Pieralisi wrote:
> On Wed, Jan 02, 2019 at 02:03:53PM +0800, honghui.zhang@mediatek.com wrote:
> > From: Honghui Zhang <honghui.zhang@mediatek.com>
> > 
> > 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 <honghui.zhang@mediatek.com>
> > ---
> >  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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PCI: Mediatek: Use resource_size function on resource object

[Bjorn Helgaas]

On Wed, Jan 02, 2019 at 02:03:53PM +0800, honghui.zhang@mediatek.com wrote:
> From: Honghui Zhang <honghui.zhang@mediatek.com>
> 
> 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 <honghui.zhang@mediatek.com>
> ---
>  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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PCI: Mediatek: Use resource_size function on resource object

[Lorenzo Pieralisi]

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, honghui.zhang@mediatek.com wrote:
> > > From: Honghui Zhang <honghui.zhang@mediatek.com>
> > > 
> > > 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 <honghui.zhang@mediatek.com>
> > > ---
> > >  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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PCI: Mediatek: Use resource_size function on resource object

[Honghui Zhang]

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, honghui.zhang@mediatek.com wrote:
> > > > From: Honghui Zhang <honghui.zhang@mediatek.com>
> > > > 
> > > > 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 <honghui.zhang@mediatek.com>
> > > > ---
> > > >  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



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [SPAM]Re: [PATCH] PCI: Mediatek: Use resource_size function on resource object

[Honghui Zhang]

On Wed, 2019-01-30 at 10:03 -0600, Bjorn Helgaas wrote:
> On Wed, Jan 02, 2019 at 02:03:53PM +0800, honghui.zhang@mediatek.com wrote:
> > From: Honghui Zhang <honghui.zhang@mediatek.com>
> > 
> > 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 <honghui.zhang@mediatek.com>
> > ---
> >  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)".
> 
HW design point it out that the window size is 2^x, the original code
set x as fls(0xffffffff), which is equal to 31.

The HW design possible values for PCIE_AXI_WINDOW0 size would be 12 to
36, which means that the PCIE2AXI will translate the 2^12 ~ 2^36 window
size. Which means that EP could access memory address from 0 to
2^12~2^36.

Since most of our HW(like MT2712 and MT7622) are all have 4GB DRAM size,
the proper size values should be 32 to enable EP DMA access all the DRAM
in RC. But since all the DRAM start from 0x4000_0000, So the value for
the size should be 33 to fit all the EP DMA memory access request.

I will send another patch to fix this. 

Thanks for your kindly review.

> Bjorn



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PCI: Mediatek: Use resource_size function on resource object

[Honghui Zhang]

On Wed, 2019-01-30 at 09:49 -0600, Bjorn Helgaas wrote:
> On Wed, Jan 02, 2019 at 02:03:53PM +0800, honghui.zhang@mediatek.com wrote:
> > From: Honghui Zhang <honghui.zhang@mediatek.com>
> > 
> > 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 <honghui.zhang@mediatek.com>
> > ---
> >  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
> > 



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] PCI: Mediatek: Use resource_size function on resource object

[Honghui Zhang]

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, honghui.zhang@mediatek.com wrote:
> > > From: Honghui Zhang <honghui.zhang@mediatek.com>
> > > 
> > > 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 <honghui.zhang@mediatek.com>
> > > ---
> > >  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
> > > 
> 
> 



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel