* [bug report] perf: xgene: Add APM X-Gene SoC Performance Monitoring Unit driver
@ 2016-10-12 11:32 Dan Carpenter
2016-10-12 14:55 ` Mason
2016-10-12 15:23 ` Mark Rutland
0 siblings, 2 replies; 7+ messages in thread
From: Dan Carpenter @ 2016-10-12 11:32 UTC (permalink / raw)
To: linux-arm-kernel
Hello Tai Nguyen,
The patch 832c927d119b: "perf: xgene: Add APM X-Gene SoC Performance
Monitoring Unit driver" from Jul 15, 2016, leads to the following
static checker warning:
drivers/perf/xgene_pmu.c:1014 acpi_get_pmu_hw_inf()
warn: '&res' isn't an ERR_PTR
drivers/perf/xgene_pmu.c
992 static struct
993 xgene_pmu_dev_ctx *acpi_get_pmu_hw_inf(struct xgene_pmu *xgene_pmu,
994 struct acpi_device *adev, u32 type)
995 {
996 struct device *dev = xgene_pmu->dev;
997 struct list_head resource_list;
998 struct xgene_pmu_dev_ctx *ctx;
999 const union acpi_object *obj;
1000 struct hw_pmu_info *inf;
1001 void __iomem *dev_csr;
1002 struct resource res;
1003 int enable_bit;
1004 int rc;
1005
1006 ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
1007 if (!ctx)
1008 return NULL;
1009
1010 INIT_LIST_HEAD(&resource_list);
1011 rc = acpi_dev_get_resources(adev, &resource_list,
1012 acpi_pmu_dev_add_resource, &res);
1013 acpi_dev_free_resource_list(&resource_list);
1014 if (rc < 0 || IS_ERR(&res)) {
^^^^
Obviously this is a stack address and not an error pointer. I'm not
sure what was intended here.
1015 dev_err(dev, "PMU type %d: No resource address found\n", type);
1016 goto err;
1017 }
1018
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread* [bug report] perf: xgene: Add APM X-Gene SoC Performance Monitoring Unit driver
2016-10-12 11:32 [bug report] perf: xgene: Add APM X-Gene SoC Performance Monitoring Unit driver Dan Carpenter
@ 2016-10-12 14:55 ` Mason
2016-10-12 15:23 ` Mark Rutland
1 sibling, 0 replies; 7+ messages in thread
From: Mason @ 2016-10-12 14:55 UTC (permalink / raw)
To: linux-arm-kernel
On 12/10/2016 13:32, Dan Carpenter wrote:
> Hello Tai Nguyen,
>
> The patch 832c927d119b: "perf: xgene: Add APM X-Gene SoC Performance
> Monitoring Unit driver" from Jul 15, 2016, leads to the following
> static checker warning:
>
> drivers/perf/xgene_pmu.c:1014 acpi_get_pmu_hw_inf()
> warn: '&res' isn't an ERR_PTR
>
> drivers/perf/xgene_pmu.c
> 992 static struct
> 993 xgene_pmu_dev_ctx *acpi_get_pmu_hw_inf(struct xgene_pmu *xgene_pmu,
> 994 struct acpi_device *adev, u32 type)
> 995 {
> 996 struct device *dev = xgene_pmu->dev;
> 997 struct list_head resource_list;
> 998 struct xgene_pmu_dev_ctx *ctx;
> 999 const union acpi_object *obj;
> 1000 struct hw_pmu_info *inf;
> 1001 void __iomem *dev_csr;
> 1002 struct resource res;
> 1003 int enable_bit;
> 1004 int rc;
> 1005
> 1006 ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> 1007 if (!ctx)
> 1008 return NULL;
> 1009
> 1010 INIT_LIST_HEAD(&resource_list);
> 1011 rc = acpi_dev_get_resources(adev, &resource_list,
> 1012 acpi_pmu_dev_add_resource, &res);
> 1013 acpi_dev_free_resource_list(&resource_list);
> 1014 if (rc < 0 || IS_ERR(&res)) {
> ^^^^
> Obviously this is a stack address and not an error pointer. I'm not
> sure what was intended here.
Reminds me of 287980e49ffc0f6d911601e7e352a812ed27768e
("remove lots of IS_ERR_VALUE abuses")
Regards.
^ permalink raw reply [flat|nested] 7+ messages in thread* [bug report] perf: xgene: Add APM X-Gene SoC Performance Monitoring Unit driver
2016-10-12 11:32 [bug report] perf: xgene: Add APM X-Gene SoC Performance Monitoring Unit driver Dan Carpenter
2016-10-12 14:55 ` Mason
@ 2016-10-12 15:23 ` Mark Rutland
2016-10-12 15:28 ` Tai Tri Nguyen
1 sibling, 1 reply; 7+ messages in thread
From: Mark Rutland @ 2016-10-12 15:23 UTC (permalink / raw)
To: linux-arm-kernel
Hi Dan,
Thanks for the report.
On Wed, Oct 12, 2016 at 02:32:29PM +0300, Dan Carpenter wrote:
> The patch 832c927d119b: "perf: xgene: Add APM X-Gene SoC Performance
> Monitoring Unit driver" from Jul 15, 2016, leads to the following
> static checker warning:
>
> drivers/perf/xgene_pmu.c:1014 acpi_get_pmu_hw_inf()
> warn: '&res' isn't an ERR_PTR
>
> drivers/perf/xgene_pmu.c
> 992 static struct
> 993 xgene_pmu_dev_ctx *acpi_get_pmu_hw_inf(struct xgene_pmu *xgene_pmu,
> 994 struct acpi_device *adev, u32 type)
> 995 {
> 996 struct device *dev = xgene_pmu->dev;
> 997 struct list_head resource_list;
> 998 struct xgene_pmu_dev_ctx *ctx;
> 999 const union acpi_object *obj;
> 1000 struct hw_pmu_info *inf;
> 1001 void __iomem *dev_csr;
> 1002 struct resource res;
> 1003 int enable_bit;
> 1004 int rc;
> 1005
> 1006 ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> 1007 if (!ctx)
> 1008 return NULL;
> 1009
> 1010 INIT_LIST_HEAD(&resource_list);
> 1011 rc = acpi_dev_get_resources(adev, &resource_list,
> 1012 acpi_pmu_dev_add_resource, &res);
> 1013 acpi_dev_free_resource_list(&resource_list);
> 1014 if (rc < 0 || IS_ERR(&res)) {
> ^^^^
> Obviously this is a stack address and not an error pointer. I'm not
> sure what was intended here.
Urrgh; I should have caught that in review. It's been this way since v1 [1], so
I can't reverse-engineer the intent. However, I don't believe that it's
necessary to check anything other than the rc value here.
This should always evaluate as false, and shouldn't result in a functional
problem, but it is completely bogus.
Tai, can you plase send a patch deleting The IS_ERR() check here?
Thanks,
Mark.
[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-March/419173.html
^ permalink raw reply [flat|nested] 7+ messages in thread* [bug report] perf: xgene: Add APM X-Gene SoC Performance Monitoring Unit driver
2016-10-12 15:23 ` Mark Rutland
@ 2016-10-12 15:28 ` Tai Tri Nguyen
0 siblings, 0 replies; 7+ messages in thread
From: Tai Tri Nguyen @ 2016-10-12 15:28 UTC (permalink / raw)
To: linux-arm-kernel
Hi Mark,
Sure I'm working on it.
Regards,
Tai
On Wed, Oct 12, 2016 at 8:23 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> Hi Dan,
>
> Thanks for the report.
>
> On Wed, Oct 12, 2016 at 02:32:29PM +0300, Dan Carpenter wrote:
>> The patch 832c927d119b: "perf: xgene: Add APM X-Gene SoC Performance
>> Monitoring Unit driver" from Jul 15, 2016, leads to the following
>> static checker warning:
>>
>> drivers/perf/xgene_pmu.c:1014 acpi_get_pmu_hw_inf()
>> warn: '&res' isn't an ERR_PTR
>>
>> drivers/perf/xgene_pmu.c
>> 992 static struct
>> 993 xgene_pmu_dev_ctx *acpi_get_pmu_hw_inf(struct xgene_pmu *xgene_pmu,
>> 994 struct acpi_device *adev, u32 type)
>> 995 {
>> 996 struct device *dev = xgene_pmu->dev;
>> 997 struct list_head resource_list;
>> 998 struct xgene_pmu_dev_ctx *ctx;
>> 999 const union acpi_object *obj;
>> 1000 struct hw_pmu_info *inf;
>> 1001 void __iomem *dev_csr;
>> 1002 struct resource res;
>> 1003 int enable_bit;
>> 1004 int rc;
>> 1005
>> 1006 ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
>> 1007 if (!ctx)
>> 1008 return NULL;
>> 1009
>> 1010 INIT_LIST_HEAD(&resource_list);
>> 1011 rc = acpi_dev_get_resources(adev, &resource_list,
>> 1012 acpi_pmu_dev_add_resource, &res);
>> 1013 acpi_dev_free_resource_list(&resource_list);
>> 1014 if (rc < 0 || IS_ERR(&res)) {
>> ^^^^
>> Obviously this is a stack address and not an error pointer. I'm not
>> sure what was intended here.
>
> Urrgh; I should have caught that in review. It's been this way since v1 [1], so
> I can't reverse-engineer the intent. However, I don't believe that it's
> necessary to check anything other than the rc value here.
>
> This should always evaluate as false, and shouldn't result in a functional
> problem, but it is completely bogus.
>
> Tai, can you plase send a patch deleting The IS_ERR() check here?
>
> Thanks,
> Mark.
>
> [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-March/419173.html
--
Tai
^ permalink raw reply [flat|nested] 7+ messages in thread
* [bug report] perf: xgene: Add APM X-Gene SoC Performance Monitoring Unit driver
@ 2017-07-11 11:43 Dan Carpenter
2017-07-11 22:48 ` Tai Tri Nguyen
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2017-07-11 11:43 UTC (permalink / raw)
To: linux-arm-kernel
Hello Tai Nguyen,
The patch 832c927d119b: "perf: xgene: Add APM X-Gene SoC Performance
Monitoring Unit driver" from Jul 15, 2016, leads to the following
static checker warning:
drivers/perf/xgene_pmu.c:1922 xgene_pmu_probe()
warn: 'xgene_pmu->pcppmu_csr' is an error pointer or valid
drivers/perf/xgene_pmu.c
1851 if (version < 0)
1852 return -ENODEV;
^^^^^^^^^^^^^^
Good. Direct returns are the way to go if possible.
1853
1854 if (version == PCP_PMU_V3)
1855 xgene_pmu->ops = &xgene_pmu_v3_ops;
1856 else
1857 xgene_pmu->ops = &xgene_pmu_ops;
1858
1859 INIT_LIST_HEAD(&xgene_pmu->l3cpmus);
1860 INIT_LIST_HEAD(&xgene_pmu->iobpmus);
1861 INIT_LIST_HEAD(&xgene_pmu->mcbpmus);
1862 INIT_LIST_HEAD(&xgene_pmu->mcpmus);
1863
1864 xgene_pmu->version = version;
1865 dev_info(&pdev->dev, "X-Gene PMU version %d\n", xgene_pmu->version);
1866
1867 res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
1868 xgene_pmu->pcppmu_csr = devm_ioremap_resource(&pdev->dev, res);
1869 if (IS_ERR(xgene_pmu->pcppmu_csr)) {
1870 dev_err(&pdev->dev, "ioremap failed for PCP PMU resource\n");
1871 rc = PTR_ERR(xgene_pmu->pcppmu_csr);
1872 goto err;
^^^^^^^^
If we hit this goto then the kernel will crash.
1873 }
[ snip ]
1916 /* Enable interrupt */
1917 xgene_pmu->ops->unmask_int(xgene_pmu);
1918
1919 return 0;
1920
1921 err:
1922 if (xgene_pmu->pcppmu_csr)
1923 devm_iounmap(&pdev->dev, xgene_pmu->pcppmu_csr);
1924 devm_kfree(&pdev->dev, xgene_pmu);
Can't we remove all this cleanup since it's devm_ managed resources?
1925
1926 return rc;
1927 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread* [bug report] perf: xgene: Add APM X-Gene SoC Performance Monitoring Unit driver
2017-07-11 11:43 Dan Carpenter
@ 2017-07-11 22:48 ` Tai Tri Nguyen
2017-07-12 9:34 ` Mark Rutland
0 siblings, 1 reply; 7+ messages in thread
From: Tai Tri Nguyen @ 2017-07-11 22:48 UTC (permalink / raw)
To: linux-arm-kernel
Hi Dan,
Thanks for reporting the bug.
On Tue, Jul 11, 2017 at 4:43 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> Hello Tai Nguyen,
>
> The patch 832c927d119b: "perf: xgene: Add APM X-Gene SoC Performance
> Monitoring Unit driver" from Jul 15, 2016, leads to the following
> static checker warning:
>
> drivers/perf/xgene_pmu.c:1922 xgene_pmu_probe()
> warn: 'xgene_pmu->pcppmu_csr' is an error pointer or valid
>
> drivers/perf/xgene_pmu.c
> 1851 if (version < 0)
> 1852 return -ENODEV;
> ^^^^^^^^^^^^^^
> Good. Direct returns are the way to go if possible.
>
> 1853
> 1854 if (version == PCP_PMU_V3)
> 1855 xgene_pmu->ops = &xgene_pmu_v3_ops;
> 1856 else
> 1857 xgene_pmu->ops = &xgene_pmu_ops;
> 1858
> 1859 INIT_LIST_HEAD(&xgene_pmu->l3cpmus);
> 1860 INIT_LIST_HEAD(&xgene_pmu->iobpmus);
> 1861 INIT_LIST_HEAD(&xgene_pmu->mcbpmus);
> 1862 INIT_LIST_HEAD(&xgene_pmu->mcpmus);
> 1863
> 1864 xgene_pmu->version = version;
> 1865 dev_info(&pdev->dev, "X-Gene PMU version %d\n", xgene_pmu->version);
> 1866
> 1867 res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> 1868 xgene_pmu->pcppmu_csr = devm_ioremap_resource(&pdev->dev, res);
> 1869 if (IS_ERR(xgene_pmu->pcppmu_csr)) {
> 1870 dev_err(&pdev->dev, "ioremap failed for PCP PMU resource\n");
> 1871 rc = PTR_ERR(xgene_pmu->pcppmu_csr);
> 1872 goto err;
> ^^^^^^^^
> If we hit this goto then the kernel will crash.
>
> 1873 }
>
> [ snip ]
>
> 1916 /* Enable interrupt */
> 1917 xgene_pmu->ops->unmask_int(xgene_pmu);
> 1918
> 1919 return 0;
> 1920
> 1921 err:
> 1922 if (xgene_pmu->pcppmu_csr)
> 1923 devm_iounmap(&pdev->dev, xgene_pmu->pcppmu_csr);
> 1924 devm_kfree(&pdev->dev, xgene_pmu);
>
> Can't we remove all this cleanup since it's devm_ managed resources?
Yes, you are right.
We can remove all this cleanup and also others because these managed
resources are automatically freed/unmapped on driver detach.
Hi Mark,
If you agree, I can post a patch to fix the issue.
[...]
Regards,
--
Tai
^ permalink raw reply [flat|nested] 7+ messages in thread* [bug report] perf: xgene: Add APM X-Gene SoC Performance Monitoring Unit driver
2017-07-11 22:48 ` Tai Tri Nguyen
@ 2017-07-12 9:34 ` Mark Rutland
0 siblings, 0 replies; 7+ messages in thread
From: Mark Rutland @ 2017-07-12 9:34 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
On Tue, Jul 11, 2017 at 03:48:06PM -0700, Tai Tri Nguyen wrote:
> On Tue, Jul 11, 2017 at 4:43 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > The patch 832c927d119b: "perf: xgene: Add APM X-Gene SoC Performance
> > Monitoring Unit driver" from Jul 15, 2016, leads to the following
> > static checker warning:
> >
> > drivers/perf/xgene_pmu.c:1922 xgene_pmu_probe()
> > warn: 'xgene_pmu->pcppmu_csr' is an error pointer or valid
> > 1869 if (IS_ERR(xgene_pmu->pcppmu_csr)) {
> > 1870 dev_err(&pdev->dev, "ioremap failed for PCP PMU resource\n");
> > 1871 rc = PTR_ERR(xgene_pmu->pcppmu_csr);
> > 1872 goto err;
> > ^^^^^^^^
> > If we hit this goto then the kernel will crash.
> >
> > 1873 }
> >
> > [ snip ]
> >
> > 1916 /* Enable interrupt */
> > 1917 xgene_pmu->ops->unmask_int(xgene_pmu);
> > 1918
> > 1919 return 0;
> > 1920
> > 1921 err:
> > 1922 if (xgene_pmu->pcppmu_csr)
> > 1923 devm_iounmap(&pdev->dev, xgene_pmu->pcppmu_csr);
> > 1924 devm_kfree(&pdev->dev, xgene_pmu);
> >
> > Can't we remove all this cleanup since it's devm_ managed resources?
>
> Yes, you are right.
> We can remove all this cleanup and also others because these managed
> resources are automatically freed/unmapped on driver detach.
>
> Hi Mark,
>
> If you agree, I can post a patch to fix the issue.
Yes please.
Mark.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-07-12 9:34 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-10-12 11:32 [bug report] perf: xgene: Add APM X-Gene SoC Performance Monitoring Unit driver Dan Carpenter
2016-10-12 14:55 ` Mason
2016-10-12 15:23 ` Mark Rutland
2016-10-12 15:28 ` Tai Tri Nguyen
-- strict thread matches above, loose matches on Subject: below --
2017-07-11 11:43 Dan Carpenter
2017-07-11 22:48 ` Tai Tri Nguyen
2017-07-12 9:34 ` Mark Rutland
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox