| Message ID | 1546409033-20412-1-git-send-email-honghui.zhang@mediatek.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: Mediatek: Use resource_size function on resource object | expand |
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 >
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 >
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 >
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
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
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
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
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 > >
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 > > > > >
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);