Re: [PATCH] soc: qcom: pmic_glink_altmode: Use common error handling code in pmic_glink_altmode_probe()
On Thu, Feb 29, 2024 at 12:26:49AM +0100, Konrad Dybcio wrote: > > > On 2/28/24 19:05, Markus Elfring wrote: > > From: Markus Elfring > > Date: Wed, 28 Feb 2024 18:45:13 +0100 > > > > Add a jump target so that a bit of exception handling can be better reused > > at the end of this function implementation. > > > > This issue was transformed by using the Coccinelle software. > > > > Signed-off-by: Markus Elfring > > --- > > (+CC Peter) > > Hmm.. this looks very similar to the problem that __free solves > with .. > > I know no better, but wouldn't the same mechanism, down to the > usage of DEFINE_FREE work fine for _put-like functions? Jonathan Cameron has created something like this: https://lore.kernel.org/all/20240225142714.286440-1-ji...@kernel.org/ It hasn't been merged yet and it would need a bit of adjusting for this use case but it's basically what you want. regards, dan carpenter
[bug report] drm/msm/dpu: reserve cdm blocks for writeback in case of YUV output
Hello Abhinav Kumar,
This is a semi-automatic email about new static checker warnings.
The patch 8b45a26f2ba9: "drm/msm/dpu: reserve cdm blocks for
writeback in case of YUV output" from Dec 12, 2023, leads to the
following Smatch complaint:
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c:2084
dpu_encoder_helper_phys_cleanup()
warn: variable dereferenced before check 'phys_enc->hw_pp' (see line 2075)
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
2074 /* reset the merge 3D HW block */
2075 if (phys_enc->hw_pp->merge_3d) {
^^^
The existing code assumed that phys_enc->hw_pp is vald.
2076
phys_enc->hw_pp->merge_3d->ops.setup_3d_mode(phys_enc->hw_pp->merge_3d,
2077 BLEND_3D_NONE);
2078 if (phys_enc->hw_ctl->ops.update_pending_flush_merge_3d)
2079
phys_enc->hw_ctl->ops.update_pending_flush_merge_3d(ctl,
2080 phys_enc->hw_pp->merge_3d->idx);
2081 }
2082
2083 if (phys_enc->hw_cdm) {
2084 if (phys_enc->hw_cdm->ops.bind_pingpong_blk &&
phys_enc->hw_pp)
^^^
But the patch assumes it can be NULL.
2085
phys_enc->hw_cdm->ops.bind_pingpong_blk(phys_enc->hw_cdm,
2086
PINGPONG_NONE);
regards,
dan carpenter
[Freedreno] [PATCH] drm/msm/dp: Fix platform_get_irq() check
The platform_get_irq() function returns negative error codes. It never
returns zero. Fix the check accordingly.
Fixes: 82c2a5751227 ("drm/msm/dp: tie dp_display_irq_handler() with dp driver")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/msm/dp/dp_display.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 61b7103498a7..d80cb3d14c6b 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1166,9 +1166,9 @@ static int dp_display_request_irq(struct
dp_display_private *dp)
struct platform_device *pdev = dp->dp_display.pdev;
dp->irq = platform_get_irq(pdev, 0);
- if (!dp->irq) {
+ if (dp->irq < 0) {
DRM_ERROR("failed to get irq\n");
- return -EINVAL;
+ return dp->irq;
}
rc = devm_request_irq(>dev, dp->irq, dp_display_irq_handler,
--
2.42.0
Re: [Freedreno] [PATCH v2] drm/msm: remove unnecessary NULL check
On Thu, Nov 16, 2023 at 01:05:52PM -0800, Abhinav Kumar wrote:
>
>
> On 11/1/2023 12:23 PM, Abhinav Kumar wrote:
> >
> >
> > On 10/13/2023 1:25 AM, Dan Carpenter wrote:
> > > This NULL check was required when it was added, but we shuffled the code
> > > around and now it's not.? The inconsistent NULL checking triggers a
> > > Smatch warning:
> > >
> > > drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c:847 mdp5_init() warn:
> > > variable dereferenced before check 'mdp5_kms' (see line 782)
> > >
> > > Fixes: 1f50db2f3e1e ("drm/msm/mdp5: move resource allocation to the
> > > _probe function"
>
> A small error here. Its missing the closing brace for the Fixes tag.
> Checkpatch cries without it.
>
Sorry. I must have accidentally deleted it after I ran checkpatch.
> I have fixed it while applying.
Thanks!
regards,
dan carpenter
Re: [Freedreno] [PATCH] drm/msm: remove unnecessary NULL check
On Fri, Oct 13, 2023 at 10:01:49AM +0200, Uwe Kleine-König wrote:
> Hello,
>
> On Fri, Oct 13, 2023 at 10:17:08AM +0300, Dan Carpenter wrote:
> > This NULL check was required when it was added, but we shuffled the code
> > around in commit 1f50db2f3e1e ("drm/msm/mdp5: move resource allocation
> > to the _probe function") and now it's not. The inconsistent NULL
> > checking triggers a Smatch warning:
> >
> > drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c:847 mdp5_init() warn:
> > variable dereferenced before check 'mdp5_kms' (see line 782)
> >
> > Signed-off-by: Dan Carpenter
>
> LGTM
>
> Reviewed-by: Uwe Kleine-König
>
> This patch opportunity is valid since commit 1f50db2f3e1e
> ("drm/msm/mdp5: move resource allocation to the _probe function") but
> applies to older trees (where it introduces a bug).
> On one hand it's not really a fix, but maybe still add a Fixes: line to
> ensure it's not backported to older stables? Hmm, I don't know.
Sure. Being extra safe is good.
regards,
dan carpenter
[Freedreno] [PATCH v2] drm/msm: remove unnecessary NULL check
This NULL check was required when it was added, but we shuffled the code
around and now it's not. The inconsistent NULL checking triggers a
Smatch warning:
drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c:847 mdp5_init() warn:
variable dereferenced before check 'mdp5_kms' (see line 782)
Fixes: 1f50db2f3e1e ("drm/msm/mdp5: move resource allocation to the _probe
function"
Signed-off-by: Dan Carpenter
---
v2: Added a Fixes tag. It's not really a bug fix and so adding the
fixes tag is slightly unfair but it should prevent this patch from
accidentally getting backported before the refactoring and causing an
issue.
Btw, fixes tags are often unfair like this. People look at fixes tags
and think, "the fix introduced a bug" but actually it's really common
that the fix was just not complete. But from a backporting perspective
it makes sense to tie them together.
Plus everyone introduces bugs. If you're not introducing bugs, then
you're probably not writing a lot of code.
drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index 11d9fc2c6bf5..ec933d597e20 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -844,8 +844,7 @@ static int mdp5_init(struct platform_device *pdev, struct
drm_device *dev)
return 0;
fail:
- if (mdp5_kms)
- mdp5_destroy(mdp5_kms);
+ mdp5_destroy(mdp5_kms);
return ret;
}
--
2.39.2
[Freedreno] [PATCH] drm/msm: remove unnecessary NULL check
This NULL check was required when it was added, but we shuffled the code
around in commit 1f50db2f3e1e ("drm/msm/mdp5: move resource allocation
to the _probe function") and now it's not. The inconsistent NULL
checking triggers a Smatch warning:
drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c:847 mdp5_init() warn:
variable dereferenced before check 'mdp5_kms' (see line 782)
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index 11d9fc2c6bf5..ec933d597e20 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -844,8 +844,7 @@ static int mdp5_init(struct platform_device *pdev, struct
drm_device *dev)
return 0;
fail:
- if (mdp5_kms)
- mdp5_destroy(mdp5_kms);
+ mdp5_destroy(mdp5_kms);
return ret;
}
--
2.39.2
Re: [Freedreno] [bug report] drm/msm/a6xx: Send ACD state to QMP at GMU resume
On Thu, Oct 12, 2023 at 06:33:20PM +0200, Konrad Dybcio wrote: > @@ -1810,8 +1816,8 @@ int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct > device_node *node) > > return 0; > > - if (!IS_ERR_OR_NULL(gmu->qmp)) > - qmp_put(gmu->qmp); > +remove_device_link: > + device_link_del(link); > This patch looks good to me but there is something happening here which I don't understand. This is not related to you patch, but where does the device_link_del() happen in the remove() function? regards, dan carpenter
[Freedreno] [bug report] drm/msm/dpu: shift IRQ indices by 1
Hello Dmitry Baryshkov,
The patch f35fe63fcf14: "drm/msm/dpu: shift IRQ indices by 1" from
Aug 2, 2023 (linux-next), leads to the following Smatch static
checker warning:
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c:371
dpu_encoder_helper_wait_for_irq()
warn: unsigned 'irq_idx' is never less than zero.
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
349 int dpu_encoder_helper_wait_for_irq(struct dpu_encoder_phys *phys_enc,
350 unsigned int irq_idx,
The patch makes this unsigned
351 void (*func)(void *arg),
352 struct dpu_encoder_wait_info *wait_info)
353 {
354 u32 irq_status;
355 int ret;
356
357 if (!wait_info) {
358 DPU_ERROR("invalid params\n");
359 return -EINVAL;
360 }
361 /* note: do master / slave checking outside */
362
363 /* return EWOULDBLOCK since we know the wait isn't necessary */
364 if (phys_enc->enable_state == DPU_ENC_DISABLED) {
365 DRM_ERROR("encoder is disabled id=%u, callback=%ps,
IRQ=[%d, %d]\n",
366 DRMID(phys_enc->parent), func,
367 DPU_IRQ_REG(irq_idx), DPU_IRQ_BIT(irq_idx));
368 return -EWOULDBLOCK;
369 }
370
--> 371 if (irq_idx < 0) {
^^^
It looks like this can be safely removed
372 DRM_DEBUG_KMS("skip irq wait id=%u, callback=%ps\n",
373 DRMID(phys_enc->parent), func);
374 return 0;
375 }
376
regards,
dan carpenter
[Freedreno] [bug report] drm/msm/a6xx: Send ACD state to QMP at GMU resume
Hello Konrad Dybcio,
The patch 88a0997f2f94: "drm/msm/a6xx: Send ACD state to QMP at GMU
resume" from Sep 25, 2023 (linux-next), leads to the following Smatch
static checker warning:
drivers/gpu/drm/msm/adreno/a6xx_gmu.c:1813 a6xx_gmu_init()
warn: ignoring unreachable code.
drivers/gpu/drm/msm/adreno/a6xx_gmu.c
1774 gmu->cxpd = dev_pm_domain_attach_by_name(gmu->dev, "cx");
1775 if (IS_ERR(gmu->cxpd)) {
1776 ret = PTR_ERR(gmu->cxpd);
1777 goto err_mmio;
1778 }
1779
1780 if (!device_link_add(gmu->dev, gmu->cxpd,
1781 DL_FLAG_PM_RUNTIME)) {
1782 ret = -ENODEV;
1783 goto detach_cxpd;
1784 }
1785
1786 gmu->qmp = qmp_get(gmu->dev);
1787 if (IS_ERR(gmu->qmp) && adreno_is_a7xx(adreno_gpu))
1788 return PTR_ERR(gmu->qmp);
This should be a goto something_undo_device_link_add;
1789
1790 init_completion(>pd_gate);
1791 complete_all(>pd_gate);
1792 gmu->pd_nb.notifier_call = cxpd_notifier_cb;
1793
1794 /*
1795 * Get a link to the GX power domain to reset the GPU in case
of GMU
1796 * crash
1797 */
1798 gmu->gxpd = dev_pm_domain_attach_by_name(gmu->dev, "gx");
1799
1800 /* Get the power levels for the GMU and GPU */
1801 a6xx_gmu_pwrlevels_probe(gmu);
1802
1803 /* Set up the HFI queues */
1804 a6xx_hfi_init(gmu);
1805
1806 /* Initialize RPMh */
1807 a6xx_gmu_rpmh_init(gmu);
1808
1809 gmu->initialized = true;
1810
1811 return 0;
1812
--> 1813 if (!IS_ERR_OR_NULL(gmu->qmp))
1814 qmp_put(gmu->qmp);
Unreachable. But we don't need to undo the qmp_get() in this funciton
because that's the last failure path. Do we need to add a qmp_put() to
a6xx_gmu_remove()? At first glance that looks like the undo function
for a6xx_gmu_init().
1815
1816 detach_cxpd:
1817 dev_pm_domain_detach(gmu->cxpd, false);
1818
1819 err_mmio:
1820 iounmap(gmu->mmio);
1821 if (platform_get_resource_byname(pdev, IORESOURCE_MEM, "rscc"))
1822 iounmap(gmu->rscc);
1823 free_irq(gmu->gmu_irq, gmu);
1824 free_irq(gmu->hfi_irq, gmu);
1825
1826 err_memory:
1827 a6xx_gmu_memory_free(gmu);
1828 err_put_device:
1829 /* Drop reference taken in of_find_device_by_node */
1830 put_device(gmu->dev);
1831
1832 return ret;
1833 }
regards,
dan carpenter
[Freedreno] [PATCH] drm/msm/dsi: fix irq_of_parse_and_map() error checking
The irq_of_parse_and_map() function returns zero on error. It
never returns negative error codes. Fix the check.
Fixes: a689554ba6ed ("drm/msm: Initial add DSI connector support")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/msm/dsi/dsi_host.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 5d9ec27c89d3..13da53737a6a 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -1894,10 +1894,9 @@ int msm_dsi_host_init(struct msm_dsi *msm_dsi)
}
msm_host->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
- if (msm_host->irq < 0) {
- ret = msm_host->irq;
- dev_err(>dev, "failed to get irq: %d\n", ret);
- return ret;
+ if (!msm_host->irq) {
+ dev_err(>dev, "failed to get irq\n");
+ return -EINVAL;
}
/* do not autoenable, will be enabled later */
--
2.39.2
Re: [Freedreno] [PATCH v3 1/3] drm/display: add transparent bridge helper
Hi Dmitry,
kernel test robot noticed the following build warnings:
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url:
https://github.com/intel-lab-lkp/linux/commits/Dmitry-Baryshkov/drm-display-add-transparent-bridge-helper/20230802-091932
base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:
https://lore.kernel.org/r/20230802011845.4176631-2-dmitry.baryshkov%40linaro.org
patch subject: [PATCH v3 1/3] drm/display: add transparent bridge helper
config: x86_64-randconfig-m001-20230808
(https://download.01.org/0day-ci/archive/20230809/202308090559.rmlh2dl6-...@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce:
(https://download.01.org/0day-ci/archive/20230809/202308090559.rmlh2dl6-...@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot
| Reported-by: Dan Carpenter
| Closes: https://lore.kernel.org/r/202308090559.rmlh2dl6-...@intel.com/
smatch warnings:
drivers/gpu/drm/display/drm_simple_bridge.c:41 drm_simple_bridge_register()
warn: possible memory leak of 'adev'
vim +/adev +41 drivers/gpu/drm/display/drm_simple_bridge.c
abf701043719cd Dmitry Baryshkov 2023-08-02 30 int
drm_simple_bridge_register(struct device *parent)
abf701043719cd Dmitry Baryshkov 2023-08-02 31 {
abf701043719cd Dmitry Baryshkov 2023-08-02 32 struct auxiliary_device
*adev;
abf701043719cd Dmitry Baryshkov 2023-08-02 33 int ret;
abf701043719cd Dmitry Baryshkov 2023-08-02 34
abf701043719cd Dmitry Baryshkov 2023-08-02 35 adev =
kzalloc(sizeof(*adev), GFP_KERNEL);
abf701043719cd Dmitry Baryshkov 2023-08-02 36 if (!adev)
abf701043719cd Dmitry Baryshkov 2023-08-02 37 return -ENOMEM;
abf701043719cd Dmitry Baryshkov 2023-08-02 38
abf701043719cd Dmitry Baryshkov 2023-08-02 39 ret =
ida_alloc(_bridge_ida, GFP_KERNEL);
abf701043719cd Dmitry Baryshkov 2023-08-02 40 if (ret < 0)
abf701043719cd Dmitry Baryshkov 2023-08-02 @41 return ret;
kfree(adev);
abf701043719cd Dmitry Baryshkov 2023-08-02 42
abf701043719cd Dmitry Baryshkov 2023-08-02 43 adev->id = ret;
abf701043719cd Dmitry Baryshkov 2023-08-02 44 adev->name =
"simple_bridge";
abf701043719cd Dmitry Baryshkov 2023-08-02 45 adev->dev.parent =
parent;
abf701043719cd Dmitry Baryshkov 2023-08-02 46 adev->dev.of_node =
parent->of_node;
abf701043719cd Dmitry Baryshkov 2023-08-02 47 adev->dev.release =
drm_simple_bridge_release;
abf701043719cd Dmitry Baryshkov 2023-08-02 48
abf701043719cd Dmitry Baryshkov 2023-08-02 49 ret =
auxiliary_device_init(adev);
abf701043719cd Dmitry Baryshkov 2023-08-02 50 if (ret) {
abf701043719cd Dmitry Baryshkov 2023-08-02 51 kfree(adev);
This needs to ida_free(_bridge_ida, adev->id) as well. There is
a smatch check for this (check_unwind.c) but I guess I plan to re-write
it a bit before I turn that on.
abf701043719cd Dmitry Baryshkov 2023-08-02 52 return ret;
abf701043719cd Dmitry Baryshkov 2023-08-02 53 }
abf701043719cd Dmitry Baryshkov 2023-08-02 54
abf701043719cd Dmitry Baryshkov 2023-08-02 55 ret =
auxiliary_device_add(adev);
abf701043719cd Dmitry Baryshkov 2023-08-02 56 if (ret) {
abf701043719cd Dmitry Baryshkov 2023-08-02 57
auxiliary_device_uninit(adev);
abf701043719cd Dmitry Baryshkov 2023-08-02 58 return ret;
abf701043719cd Dmitry Baryshkov 2023-08-02 59 }
abf701043719cd Dmitry Baryshkov 2023-08-02 60
abf701043719cd Dmitry Baryshkov 2023-08-02 61 return
devm_add_action_or_reset(parent, drm_simple_bridge_unregister_adev, adev);
abf701043719cd Dmitry Baryshkov 2023-08-02 62 }
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [Freedreno] [PATCH v3 4/9] PM / QoS: Decouple request alloc from dev_pm_qos_mtx
Hi Rob,
kernel test robot noticed the following build warnings:
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url:
https://github.com/intel-lab-lkp/linux/commits/Rob-Clark/PM-devfreq-Drop-unneed-locking-to-appease-lockdep/20230804-060505
base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:
https://lore.kernel.org/r/20230803220202.78036-5-robdclark%40gmail.com
patch subject: [PATCH v3 4/9] PM / QoS: Decouple request alloc from
dev_pm_qos_mtx
config: i386-randconfig-m021-20230730
(https://download.01.org/0day-ci/archive/20230804/202308041725.npwswh0l-...@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce:
(https://download.01.org/0day-ci/archive/20230804/202308041725.npwswh0l-...@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot
| Reported-by: Dan Carpenter
| Closes: https://lore.kernel.org/r/202308041725.npwswh0l-...@intel.com/
smatch warnings:
drivers/base/power/qos.c:973 dev_pm_qos_update_user_latency_tolerance() warn:
possible memory leak of 'req'
vim +/req +973 drivers/base/power/qos.c
2d984ad132a87c Rafael J. Wysocki 2014-02-11 923 int
dev_pm_qos_update_user_latency_tolerance(struct device *dev, s32 val)
2d984ad132a87c Rafael J. Wysocki 2014-02-11 924 {
b5ac35ff4296f7 Rob Clark 2023-08-03 925struct
dev_pm_qos_request *req = NULL;
2d984ad132a87c Rafael J. Wysocki 2014-02-11 926int ret;
2d984ad132a87c Rafael J. Wysocki 2014-02-11 927
211fb32e3a0547 Rob Clark 2023-08-03 928ret =
dev_pm_qos_constraints_ensure_allocated(dev);
211fb32e3a0547 Rob Clark 2023-08-03 929if (ret)
211fb32e3a0547 Rob Clark 2023-08-03 930return ret;
211fb32e3a0547 Rob Clark 2023-08-03 931
b5ac35ff4296f7 Rob Clark 2023-08-03 932if
(!dev->power.qos->latency_tolerance_req)
b5ac35ff4296f7 Rob Clark 2023-08-03 933req =
kzalloc(sizeof(*req), GFP_KERNEL);
b5ac35ff4296f7 Rob Clark 2023-08-03 934
2d984ad132a87c Rafael J. Wysocki 2014-02-11 935
mutex_lock(_pm_qos_mtx);
2d984ad132a87c Rafael J. Wysocki 2014-02-11 936
211fb32e3a0547 Rob Clark 2023-08-03 937if
(!dev->power.qos->latency_tolerance_req) {
2d984ad132a87c Rafael J. Wysocki 2014-02-11 938if (val < 0) {
80a6f7c79b7822 Andrew Lutomirski 2016-11-29 939if (val
== PM_QOS_LATENCY_TOLERANCE_NO_CONSTRAINT)
80a6f7c79b7822 Andrew Lutomirski 2016-11-29 940
ret = 0;
80a6f7c79b7822 Andrew Lutomirski 2016-11-29 941else
2d984ad132a87c Rafael J. Wysocki 2014-02-11 942
ret = -EINVAL;
2d984ad132a87c Rafael J. Wysocki 2014-02-11 943goto
out;
kfree(req);?
2d984ad132a87c Rafael J. Wysocki 2014-02-11 944}
2d984ad132a87c Rafael J. Wysocki 2014-02-11 945if (!req) {
2d984ad132a87c Rafael J. Wysocki 2014-02-11 946ret =
-ENOMEM;
2d984ad132a87c Rafael J. Wysocki 2014-02-11 947goto
out;
2d984ad132a87c Rafael J. Wysocki 2014-02-11 948}
2d984ad132a87c Rafael J. Wysocki 2014-02-11 949ret =
__dev_pm_qos_add_request(dev, req, DEV_PM_QOS_LATENCY_TOLERANCE, val);
2d984ad132a87c Rafael J. Wysocki 2014-02-11 950if (ret < 0) {
2d984ad132a87c Rafael J. Wysocki 2014-02-11 951
kfree(req);
2d984ad132a87c Rafael J. Wysocki 2014-02-11 952goto
out;
2d984ad132a87c Rafael J. Wysocki 2014-02-11 953}
2d984ad132a87c Rafael J. Wysocki 2014-02-11 954
dev->power.qos->latency_tolerance_req = req;
2d984ad132a87c Rafael J. Wysocki 2014-02-11 955} else {
b5ac35ff4296f7 Rob Clark 2023-08-03 956/*
b5ac35ff4296f7 Rob Clark 2023-08-03 957 * If we raced
with another thread to allocate the request,
b5ac35ff4296f7 Rob Clark 2023-08-03 958 * simply free
the redundant allocation and move on.
b5ac35ff4296f7 Rob Clark 2023-08-03 959 */
b5ac35ff4296f7 Rob Clark 2023-08-03 960if (req)
b5ac35ff4296f7 Rob Clark 2023-08-03 961
kfree(req);
b5ac35ff4296f7 Rob Clark 2023-08-03 962
2d984ad132a87c Rafael J. Wysocki 2014-02-11 963if (val < 0) {
2d984ad132a87c Rafael J. Wysocki 2014-02-11 964
__dev_pm_qos_drop_user_request(dev, DEV_PM_QOS_LATENCY_TOLERANCE);
2d984ad132a87c Rafael J. Wysocki 2014-02-11 965ret = 0;
2d984ad132a87c Rafael J. Wysocki 2014-02-11 966} else {
2d984ad132a87c Rafael
[Freedreno] [bug report] drm/msm/dpu: drop separate dpu_core_perf_tune overrides
Hello Dmitry Baryshkov,
The patch 6a4bc73915af: "drm/msm/dpu: drop separate
dpu_core_perf_tune overrides" from Jul 30, 2023 (linux-next), leads
to the following Smatch static checker warning:
drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c:295
_dpu_core_perf_get_core_clk_rate()
error: uninitialized symbol 'clk_rate'.
drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
280 static u64 _dpu_core_perf_get_core_clk_rate(struct dpu_kms *kms)
281 {
282 u64 clk_rate;
283 struct drm_crtc *crtc;
284 struct dpu_crtc_state *dpu_cstate;
285
286 if (kms->perf.perf_tune.mode == DPU_PERF_MODE_FIXED)
287 return kms->perf.fix_core_clk_rate;
288
289 if (kms->perf.perf_tune.mode == DPU_PERF_MODE_MINIMUM)
290 return kms->perf.max_core_clk_rate;
291
292 drm_for_each_crtc(crtc, kms->dev) {
293 if (crtc->enabled) {
294 dpu_cstate = to_dpu_crtc_state(crtc->state);
--> 295 clk_rate =
max(dpu_cstate->new_perf.core_clk_rate,
296 clk_rate);
Never initialized
297 }
298 }
299
300 return clk_rate;
301 }
regards,
dan carpenter
[Freedreno] [bug report] drm/msm/a5xx: really check for A510 in a5xx_gpu_init
Hello Dmitry Baryshkov,
This is a semi-automatic email about new static checker warnings.
The patch 736a93273656: "drm/msm/a5xx: really check for A510 in
a5xx_gpu_init" from Apr 9, 2023, leads to the following Smatch
complaint:
drivers/gpu/drm/msm/adreno/a5xx_gpu.c:1753 a5xx_gpu_init()
warn: variable dereferenced before check 'pdev' (see line 1746)
drivers/gpu/drm/msm/adreno/a5xx_gpu.c
1745 struct platform_device *pdev = priv->gpu_pdev;
1746 struct adreno_platform_config *config = pdev->dev.platform_data;
^^^
The patch adds an unchecked dereference
1747 struct a5xx_gpu *a5xx_gpu = NULL;
1748 struct adreno_gpu *adreno_gpu;
1749 struct msm_gpu *gpu;
1750 unsigned int nr_rings;
1751 int ret;
1752
1753 if (!pdev) {
^
But the existing code assumes it can be NULL. Do we really need this
check?
1754 DRM_DEV_ERROR(dev->dev, "No A5XX device is defined\n");
1755 return ERR_PTR(-ENXIO);
regards,
dan carpenter
Re: [Freedreno] [PATCH] drm/msm/dpu: tidy up some error checking
On Thu, Jun 08, 2023 at 02:26:14AM +0300, Dmitry Baryshkov wrote:
> On 07/06/2023 17:42, Dan Carpenter wrote:
> > On Tue, Jun 06, 2023 at 10:23:46PM +0200, Marijn Suijten wrote:
> > > > @@ -359,8 +359,8 @@ static void dpu_encoder_phys_cmd_tearcheck_config(
> > > > * frequency divided by the no. of rows (lines) in the LCDpanel.
> > > > */
> > > > vsync_hz = dpu_kms_get_clk_rate(dpu_kms, "vsync");
> > > > - if (vsync_hz <= 0) {
> > > > - DPU_DEBUG_CMDENC(cmd_enc, "invalid - vsync_hz %u\n",
> > > > + if (!vsync_hz) {
> > > > + DPU_DEBUG_CMDENC(cmd_enc, "invalid - vsync_hz %lu\n",
> > > > vsync_hz);
> > >
> > > Nit: no need to print the value here, you know it's zero. Could be
> > > clarified to just "no vsync clock".
> > >
> >
> > Yeah. That's obviously not useful. Sorry, I will resend.
>
> I'll fix while applying. Seems easier.
Thanks!
regards,
dan carpenter
[Freedreno] [PATCH v2] drm/msm/dpu: tidy up some error checking
The "vsync_hz" variable is unsigned int so it can't be less
than zero. The dpu_kms_get_clk_rate() function used to return a u64
but I previously changed it to return an unsigned long and zero on
error so it matches clk_get_rate().
Change the "vsync_hz" type to unsigned long as well and change the
error checking to check for zero instead of negatives. This change
does not affect runtime at all beyond a minor adjustment to the debug
output.
Suggested-by: Marijn Suijten
Signed-off-by: Dan Carpenter
Reviewed-by: Marijn Suijten
---
v2: update the debug output
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index 4f8c9187f76d..27a823c72c06 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -323,8 +323,8 @@ static void dpu_encoder_phys_cmd_tearcheck_config(
to_dpu_encoder_phys_cmd(phys_enc);
struct dpu_hw_tear_check tc_cfg = { 0 };
struct drm_display_mode *mode;
+ unsigned long vsync_hz;
bool tc_enable = true;
- u32 vsync_hz;
struct dpu_kms *dpu_kms;
if (phys_enc->has_intf_te) {
@@ -359,9 +359,8 @@ static void dpu_encoder_phys_cmd_tearcheck_config(
* frequency divided by the no. of rows (lines) in the LCDpanel.
*/
vsync_hz = dpu_kms_get_clk_rate(dpu_kms, "vsync");
- if (vsync_hz <= 0) {
- DPU_DEBUG_CMDENC(cmd_enc, "invalid - vsync_hz %u\n",
-vsync_hz);
+ if (!vsync_hz) {
+ DPU_DEBUG_CMDENC(cmd_enc, "no vsync clock\n");
return;
}
@@ -381,7 +380,7 @@ static void dpu_encoder_phys_cmd_tearcheck_config(
tc_cfg.rd_ptr_irq = mode->vdisplay + 1;
DPU_DEBUG_CMDENC(cmd_enc,
- "tc vsync_clk_speed_hz %u vtotal %u vrefresh %u\n",
+ "tc vsync_clk_speed_hz %lu vtotal %u vrefresh %u\n",
vsync_hz, mode->vtotal, drm_mode_vrefresh(mode));
DPU_DEBUG_CMDENC(cmd_enc,
"tc enable %u start_pos %u rd_ptr_irq %u\n",
--
2.39.2
Re: [Freedreno] [PATCH] drm/msm/dpu: tidy up some error checking
On Tue, Jun 06, 2023 at 10:23:46PM +0200, Marijn Suijten wrote:
> > @@ -359,8 +359,8 @@ static void dpu_encoder_phys_cmd_tearcheck_config(
> > * frequency divided by the no. of rows (lines) in the LCDpanel.
> > */
> > vsync_hz = dpu_kms_get_clk_rate(dpu_kms, "vsync");
> > - if (vsync_hz <= 0) {
> > - DPU_DEBUG_CMDENC(cmd_enc, "invalid - vsync_hz %u\n",
> > + if (!vsync_hz) {
> > + DPU_DEBUG_CMDENC(cmd_enc, "invalid - vsync_hz %lu\n",
> > vsync_hz);
>
> Nit: no need to print the value here, you know it's zero. Could be
> clarified to just "no vsync clock".
>
Yeah. That's obviously not useful. Sorry, I will resend.
regards,
dan carpenter
[Freedreno] [PATCH] drm/msm/dpu: tidy up some error checking
The "vsync_hz" variable is unsigned int so it can't be less
than zero. The dpu_kms_get_clk_rate() function used to return a u64
but I previously changed it to return an unsigned long and zero on
error so it matches clk_get_rate().
Change the "vsync_hz" type to unsigned long as well and change the
error checking to check for zero instead of negatives. This change
does not affect runtime at all. It's just a clean up.
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
index d8ed85a238af..6aecaba14e7e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c
@@ -324,7 +324,7 @@ static void dpu_encoder_phys_cmd_tearcheck_config(
struct dpu_hw_tear_check tc_cfg = { 0 };
struct drm_display_mode *mode;
bool tc_enable = true;
- u32 vsync_hz;
+ unsigned long vsync_hz;
struct dpu_kms *dpu_kms;
if (phys_enc->has_intf_te) {
@@ -359,8 +359,8 @@ static void dpu_encoder_phys_cmd_tearcheck_config(
* frequency divided by the no. of rows (lines) in the LCDpanel.
*/
vsync_hz = dpu_kms_get_clk_rate(dpu_kms, "vsync");
- if (vsync_hz <= 0) {
- DPU_DEBUG_CMDENC(cmd_enc, "invalid - vsync_hz %u\n",
+ if (!vsync_hz) {
+ DPU_DEBUG_CMDENC(cmd_enc, "invalid - vsync_hz %lu\n",
vsync_hz);
return;
}
@@ -381,7 +381,7 @@ static void dpu_encoder_phys_cmd_tearcheck_config(
tc_cfg.rd_ptr_irq = mode->vdisplay + 1;
DPU_DEBUG_CMDENC(cmd_enc,
- "tc vsync_clk_speed_hz %u vtotal %u vrefresh %u\n",
+ "tc vsync_clk_speed_hz %lu vtotal %u vrefresh %u\n",
vsync_hz, mode->vtotal, drm_mode_vrefresh(mode));
DPU_DEBUG_CMDENC(cmd_enc,
"tc enable %u start_pos %u rd_ptr_irq %u\n",
--
2.39.2
[Freedreno] [PATCH v2] drm/msm/dpu: clean up dpu_kms_get_clk_rate() returns
Static analysis tools complain about the -EINVAL error code being
stored in an unsigned variable. Let's change this to match
the clk_get_rate() function which is type unsigned long and returns
zero on error.
Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
Signed-off-by: Dan Carpenter
---
v2: In v1 I change the type to int which was wrong. This is a different
approach. CC the freedreno list this time too.
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 4 ++--
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 0e7a68714e9e..25e6a15eaf9f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -979,13 +979,13 @@ static int _dpu_kms_mmu_init(struct dpu_kms *dpu_kms)
return 0;
}
-u64 dpu_kms_get_clk_rate(struct dpu_kms *dpu_kms, char *clock_name)
+unsigned long dpu_kms_get_clk_rate(struct dpu_kms *dpu_kms, char *clock_name)
{
struct clk *clk;
clk = msm_clk_bulk_get_clock(dpu_kms->clocks, dpu_kms->num_clocks,
clock_name);
if (!clk)
- return -EINVAL;
+ return 0;
return clk_get_rate(clk);
}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
index aca39a4689f4..961918e5a5b3 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h
@@ -201,6 +201,6 @@ void dpu_disable_vblank(struct msm_kms *kms, struct
drm_crtc *crtc);
*
* Return: current clock rate
*/
-u64 dpu_kms_get_clk_rate(struct dpu_kms *dpu_kms, char *clock_name);
+unsigned long dpu_kms_get_clk_rate(struct dpu_kms *dpu_kms, char *clock_name);
#endif /* __dpu_kms_H__ */
--
2.39.2
[Freedreno] [bug report] drm/msm/dpu: add HDMI output support
Hello Dmitry Baryshkov,
The patch ea767bb1752f: "drm/msm/dpu: add HDMI output support" from
Apr 15, 2023, leads to the following Smatch static checker warning:
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c:647 _dpu_kms_initialize_hdmi()
error: uninitialized symbol 'i'.
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
620 static int _dpu_kms_initialize_hdmi(struct drm_device *dev,
621 struct msm_drm_private *priv,
622 struct dpu_kms *dpu_kms)
623 {
624 struct drm_encoder *encoder = NULL;
625 struct msm_display_info info;
626 int rc;
627 int i;
628
629 if (!priv->hdmi)
630 return 0;
631
632 encoder = dpu_encoder_init(dev, DRM_MODE_ENCODER_TMDS);
633 if (IS_ERR(encoder)) {
634 DPU_ERROR("encoder init failed for HDMI display\n");
635 return PTR_ERR(encoder);
636 }
637
638 memset(, 0, sizeof(info));
639 rc = msm_hdmi_modeset_init(priv->hdmi, dev, encoder);
640 if (rc) {
641 DPU_ERROR("modeset_init failed for DP, rc = %d\n", rc);
642 drm_encoder_cleanup(encoder);
643 return rc;
644 }
645
646 info.num_of_h_tiles = 1;
--> 647 info.h_tile_instance[0] = i;
^
Uninitialized.
648 info.intf_type = INTF_HDMI;
649 rc = dpu_encoder_setup(dev, encoder, );
650 if (rc) {
651 DPU_ERROR("failed to setup DPU encoder %d: rc:%d\n",
652 encoder->base.id, rc);
653 return rc;
654 }
655
656 return 0;
657 }
regards,
dan carpenter
Re: [Freedreno] [PATCH v2 18/23] PM / QoS: Decouple request alloc from dev_pm_qos_mtx
Hi Rob,
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url:
https://github.com/intel-lab-lkp/linux/commits/Rob-Clark/drm-msm-Pre-allocate-hw_fence/20230320-224826
base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:
https://lore.kernel.org/r/20230320144356.803762-19-robdclark%40gmail.com
patch subject: [PATCH v2 18/23] PM / QoS: Decouple request alloc from
dev_pm_qos_mtx
config: arm64-randconfig-m041-20230319
(https://download.01.org/0day-ci/archive/20230321/202303211207.mucst3ck-...@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 12.1.0
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot
| Reported-by: Dan Carpenter
| Link: https://lore.kernel.org/r/202303211207.mucst3ck-...@intel.com/
smatch warnings:
drivers/base/power/qos.c:947 dev_pm_qos_update_user_latency_tolerance() error:
uninitialized symbol 'req'.
drivers/base/power/qos.c:975 dev_pm_qos_update_user_latency_tolerance() warn:
possible memory leak of 'req'
vim +/req +947 drivers/base/power/qos.c
2d984ad132a87c Rafael J. Wysocki 2014-02-11 923 int
dev_pm_qos_update_user_latency_tolerance(struct device *dev, s32 val)
2d984ad132a87c Rafael J. Wysocki 2014-02-11 924 {
2d7e4629d7265d Rob Clark 2023-03-20 925struct
dev_pm_qos_request *req = NULL;
2d984ad132a87c Rafael J. Wysocki 2014-02-11 926int ret;
2d984ad132a87c Rafael J. Wysocki 2014-02-11 927
00dd582e52a535 Rob Clark 2023-03-20 928ret =
dev_pm_qos_constraints_ensure_allocated(dev);
00dd582e52a535 Rob Clark 2023-03-20 929if (ret)
00dd582e52a535 Rob Clark 2023-03-20 930return ret;
00dd582e52a535 Rob Clark 2023-03-20 931
2d7e4629d7265d Rob Clark 2023-03-20 932if
(!dev->power.qos->latency_tolerance_req)
2d7e4629d7265d Rob Clark 2023-03-20 933req =
kzalloc(sizeof(*req), GFP_KERNEL);
2d7e4629d7265d Rob Clark 2023-03-20 934
2d984ad132a87c Rafael J. Wysocki 2014-02-11 935
mutex_lock(_pm_qos_mtx);
2d984ad132a87c Rafael J. Wysocki 2014-02-11 936
00dd582e52a535 Rob Clark 2023-03-20 937if
(!dev->power.qos->latency_tolerance_req) {
2d984ad132a87c Rafael J. Wysocki 2014-02-11 938struct
dev_pm_qos_request *req;
This "req" shadows the ealier req.
2d984ad132a87c Rafael J. Wysocki 2014-02-11 939
2d984ad132a87c Rafael J. Wysocki 2014-02-11 940if (val < 0) {
80a6f7c79b7822 Andrew Lutomirski 2016-11-29 941if (val
== PM_QOS_LATENCY_TOLERANCE_NO_CONSTRAINT)
80a6f7c79b7822 Andrew Lutomirski 2016-11-29 942
ret = 0;
80a6f7c79b7822 Andrew Lutomirski 2016-11-29 943else
2d984ad132a87c Rafael J. Wysocki 2014-02-11 944
ret = -EINVAL;
2d984ad132a87c Rafael J. Wysocki 2014-02-11 945goto
out;
2d984ad132a87c Rafael J. Wysocki 2014-02-11 946}
2d984ad132a87c Rafael J. Wysocki 2014-02-11 @947if (!req) {
So it leads to an unintialized variable and a leak.
2d984ad132a87c Rafael J. Wysocki 2014-02-11 948ret =
-ENOMEM;
2d984ad132a87c Rafael J. Wysocki 2014-02-11 949goto
out;
2d984ad132a87c Rafael J. Wysocki 2014-02-11 950}
2d984ad132a87c Rafael J. Wysocki 2014-02-11 951ret =
__dev_pm_qos_add_request(dev, req, DEV_PM_QOS_LATENCY_TOLERANCE, val);
2d984ad132a87c Rafael J. Wysocki 2014-02-11 952if (ret < 0) {
2d984ad132a87c Rafael J. Wysocki 2014-02-11 953
kfree(req);
2d984ad132a87c Rafael J. Wysocki 2014-02-11 954goto
out;
2d984ad132a87c Rafael J. Wysocki 2014-02-11 955}
2d984ad132a87c Rafael J. Wysocki 2014-02-11 956
dev->power.qos->latency_tolerance_req = req;
2d984ad132a87c Rafael J. Wysocki 2014-02-11 957} else {
2d7e4629d7265d Rob Clark 2023-03-20 958/*
2d7e4629d7265d Rob Clark 2023-03-20 959 * If we raced
with another thread to allocate the request,
2d7e4629d7265d Rob Clark 2023-03-20 960 * simply free
the redundant allocation and move on.
2d7e4629d7265d Rob Clark 2023-03-20 961 */
2d7e4629d7265d Rob Clark 2023-03-20 962if (req)
2d7e4629d7265d Rob Clark 2023-03-20 963
kfree(req);
2d7e4629d7265d Rob Clark 2023-03-20 964
2d984ad132a87c Rafael J. Wysocki 2014-02-11 965if (val < 0) {
2d984ad132a87c Rafael J. Wysocki 2014-02-11 966
__dev_pm_qos_drop_user_request(dev, DEV_PM_QOS_LATENCY_TOLERANCE);
2d984ad132a87c Rafael J. Wysocki 2014-02-11 967ret = 0;
2d984ad132a
Re: [Freedreno] [PATCH] drm/msm: another fix for the headless Adreno GPU
Hi Dmitry,
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url:
https://github.com/intel-lab-lkp/linux/commits/Dmitry-Baryshkov/drm-msm-another-fix-for-the-headless-Adreno-GPU/20221231-103022
base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:
https://lore.kernel.org/r/20221231022937.286491-1-dmitry.baryshkov%40linaro.org
patch subject: [PATCH] drm/msm: another fix for the headless Adreno GPU
config: loongarch-randconfig-m031-20230101
compiler: loongarch64-linux-gcc (GCC) 12.1.0
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot
| Reported-by: Dan Carpenter
smatch warnings:
drivers/gpu/drm/msm/msm_atomic.c:194 msm_atomic_commit_tail() error:
uninitialized symbol 'async'.
vim +/async +194 drivers/gpu/drm/msm/msm_atomic.c
d4d2c60497cfc5 Rob Clark2019-08-29 181
d14659f5de7d28 Sean Paul2018-02-28 182 void
msm_atomic_commit_tail(struct drm_atomic_state *state)
cf3a7e4ce08e68 Rob Clark2014-11-08 183 {
cf3a7e4ce08e68 Rob Clark2014-11-08 184 struct drm_device *dev
= state->dev;
0b776d457b9476 Rob Clark2015-01-30 185 struct msm_drm_private
*priv = dev->dev_private;
0b776d457b9476 Rob Clark2015-01-30 186 struct msm_kms *kms =
priv->kms;
2d99ced787e3d0 Rob Clark2019-08-29 187 struct drm_crtc
*async_crtc = NULL;
d4d2c60497cfc5 Rob Clark2019-08-29 188 unsigned crtc_mask =
get_crtc_mask(state);
91a514e50f1157 Dmitry Baryshkov 2022-12-31 189 bool async;
^^^
91a514e50f1157 Dmitry Baryshkov 2022-12-31 190
91a514e50f1157 Dmitry Baryshkov 2022-12-31 191 if (!kms)
91a514e50f1157 Dmitry Baryshkov 2022-12-31 192 return;
0b776d457b9476 Rob Clark2015-01-30 193
d934a712c5e6a3 Rob Clark2019-08-29 @194
trace_msm_atomic_commit_tail_start(async, crtc_mask);
^
Unitialized.
--
0-DAY CI Kernel Test Service
https://01.org/lkp
Re: [Freedreno] [PATCH 1/4] drm/msm/adreno: Fix null ptr access in adreno_gpu_cleanup()
^^^
[QUESTION #2] Passing an error pointer here will trigger a stack trace
so this is bug. The drivers->aspace pointer is allocted in
msm_gpu_create_private_address_space()
drivers/gpu/drm/msm/msm_gpu.c
824 struct msm_gem_address_space *
825 msm_gpu_create_private_address_space(struct msm_gpu *gpu, struct
task_struct *task)
826 {
827 struct msm_gem_address_space *aspace = NULL;
828 if (!gpu)
829 return NULL;
830
831 /*
832 * If the target doesn't support private address spaces then
return
833 * the global one
834 */
835 if (gpu->funcs->create_private_address_space) {
836 aspace = gpu->funcs->create_private_address_space(gpu);
837 if (!IS_ERR(aspace))
838 aspace->pid = get_pid(task_pid(task));
839 }
840
841 if (IS_ERR_OR_NULL(aspace))
^^
[QUESTION #3] This check seems reversed. It should be if *NOT* is error
or null.
842 aspace = msm_gem_address_space_get(gpu->aspace);
843
844 return aspace;
845 }
regards,
dan carpenter
[Freedreno] [PATCH v2] drm/msm/hdmi: remove unnecessary NULL check
This code was recently refactored in commit and now the "hdmi" pointer
can't be NULL. Checking for NULL leads to a Smatch warning:
drivers/gpu/drm/msm/hdmi/hdmi.c:141 msm_hdmi_init()
warn: variable dereferenced before check 'hdmi' (see line 119)
Fixes: 69a88d8633ec ("drm/msm/hdmi: move resource allocation to probe function")
Signed-off-by: Dan Carpenter
---
v2: Add a Fixes tag. Re-work the commit message.
drivers/gpu/drm/msm/hdmi/hdmi.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 7001fabd0977..4d3fdc806bef 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -138,8 +138,7 @@ static int msm_hdmi_init(struct hdmi *hdmi)
return 0;
fail:
- if (hdmi)
- msm_hdmi_destroy(hdmi);
+ msm_hdmi_destroy(hdmi);
return ret;
}
--
2.35.1
[Freedreno] [PATCH] drm/msm/hdmi: remove unnecessary NULL check
This code was refactored in commit 69a88d8633ec ("drm/msm/hdmi: move
resource allocation to probe function") and now the "hdmi" pointer can't
be NULL. Checking causes a Smatch warning:
drivers/gpu/drm/msm/hdmi/hdmi.c:141 msm_hdmi_init()
warn: variable dereferenced before check 'hdmi' (see line 119)
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/msm/hdmi/hdmi.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 7001fabd0977..4d3fdc806bef 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -138,8 +138,7 @@ static int msm_hdmi_init(struct hdmi *hdmi)
return 0;
fail:
- if (hdmi)
- msm_hdmi_destroy(hdmi);
+ msm_hdmi_destroy(hdmi);
return ret;
}
--
2.35.1
Re: [Freedreno] [PATCH] msm/adreno: fix repeated words in comments
On Fri, Aug 26, 2022 at 12:45:09PM +0300, Dmitry Baryshkov wrote: > On 24/07/2022 10:36, wangjianli wrote: > > Delete the redundant word 'in'. > > Could you please: > - adjust the commit subject to follow the rest of commit messages, > - drop the extra whitespace at the beginning of the commit message, > - add a correct Fixes tag. This doesn't fix a bug so the fixes tag is inappropriate. regards, dan carpenter
[Freedreno] [PATCH] drm/msm: return an error pointer in msm_gem_prime_get_sg_table()
The msm_gem_prime_get_sg_table() needs to return error pointers on
error. This is called from drm_gem_map_dma_buf() and returning a
NULL will lead to a crash in that function.
Fixes: ac45146733b0 ("drm/msm: fix msm_gem_prime_get_sg_table()")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/msm/msm_gem_prime.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/msm_gem_prime.c
b/drivers/gpu/drm/msm/msm_gem_prime.c
index e8f1b7a2ca9c..94ab705e9b8a 100644
--- a/drivers/gpu/drm/msm/msm_gem_prime.c
+++ b/drivers/gpu/drm/msm/msm_gem_prime.c
@@ -17,7 +17,7 @@ struct sg_table *msm_gem_prime_get_sg_table(struct
drm_gem_object *obj)
int npages = obj->size >> PAGE_SHIFT;
if (WARN_ON(!msm_obj->pages)) /* should have already pinned! */
- return NULL;
+ return ERR_PTR(-ENOMEM);
return drm_prime_pages_to_sg(obj->dev, msm_obj->pages, npages);
}
--
2.35.1
Re: [Freedreno] [PATCH 3/3] drm/msm: Add a way to override processes comm/cmdline
On Thu, Mar 17, 2022 at 08:03:59AM -0700, Rob Clark wrote:
> > > diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> > > index 4ec62b601adc..68f3f8ade76d 100644
> > > --- a/drivers/gpu/drm/msm/msm_gpu.c
> > > +++ b/drivers/gpu/drm/msm/msm_gpu.c
> > > @@ -364,14 +364,21 @@ static void retire_submits(struct msm_gpu *gpu);
> > >
> > > static void get_comm_cmdline(struct msm_gem_submit *submit, char **comm,
> > > char **cmd)
> > > {
> > > + struct msm_file_private *ctx = submit->queue->ctx;
> > > struct task_struct *task;
> > >
> > > + *comm = kstrdup(ctx->comm, GFP_KERNEL);
> > > + *cmd = kstrdup(ctx->cmdline, GFP_KERNEL);
> > > +
> > > task = get_pid_task(submit->pid, PIDTYPE_PID);
> > > if (!task)
> > > return;
> > >
> > > - *comm = kstrdup(task->comm, GFP_KERNEL);
> > > - *cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL);
> > > + if (!*comm)
> > > + *comm = kstrdup(task->comm, GFP_KERNEL);
> >
> > What?
> >
> > If the first allocation failed, then this one is going to fail as well.
> > Just return -ENOMEM. Or maybe this is meant to be checking for an empty
> > string?
>
> fwiw, if ctx->comm is NULL, the kstrdup() will return NULL, so this
> isn't intended to deal with OoM, but the case that comm and/or cmdline
> is not overridden.
Ah, I should have thought about that. Thanks!
regards,
dan carpenter
Re: [Freedreno] [PATCH 3/3] drm/msm: Add a way to override processes comm/cmdline
On Wed, Mar 16, 2022 at 05:29:45PM -0700, Rob Clark wrote:
> switch (param) {
> + case MSM_PARAM_COMM:
> + case MSM_PARAM_CMDLINE: {
> + char *str, **paramp;
> +
> + str = kmalloc(len + 1, GFP_KERNEL);
if (!str)
return -ENOMEM;
> + if (copy_from_user(str, u64_to_user_ptr(value), len)) {
> + kfree(str);
> + return -EFAULT;
> + }
> +
> + /* Ensure string is null terminated: */
> + str[len] = '\0';
> +
> + if (param == MSM_PARAM_COMM) {
> + paramp = >comm;
> + } else {
> + paramp = >cmdline;
> + }
> +
> + kfree(*paramp);
> + *paramp = str;
> +
> + return 0;
> + }
> case MSM_PARAM_SYSPROF:
> if (!capable(CAP_SYS_ADMIN))
> return -EPERM;
> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> index 4ec62b601adc..68f3f8ade76d 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.c
> +++ b/drivers/gpu/drm/msm/msm_gpu.c
> @@ -364,14 +364,21 @@ static void retire_submits(struct msm_gpu *gpu);
>
> static void get_comm_cmdline(struct msm_gem_submit *submit, char **comm,
> char **cmd)
> {
> + struct msm_file_private *ctx = submit->queue->ctx;
> struct task_struct *task;
>
> + *comm = kstrdup(ctx->comm, GFP_KERNEL);
> + *cmd = kstrdup(ctx->cmdline, GFP_KERNEL);
> +
> task = get_pid_task(submit->pid, PIDTYPE_PID);
> if (!task)
> return;
>
> - *comm = kstrdup(task->comm, GFP_KERNEL);
> - *cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL);
> + if (!*comm)
> + *comm = kstrdup(task->comm, GFP_KERNEL);
What?
If the first allocation failed, then this one is going to fail as well.
Just return -ENOMEM. Or maybe this is meant to be checking for an empty
string?
> +
> + if (!*cmd)
> + *cmd = kstrdup_quotable_cmdline(task, GFP_KERNEL);
Same.
>
> put_task_struct(task);
> }
regards,
dan carpenter
[Freedreno] [PATCH] drm/msm/adreno: fix cast in adreno_get_param()
These casts need to happen before the shift. The only time it would
matter would be if "rev.core" is >= 128. In that case the sign bit
would be extended and we do not want that.
Fixes: afab9d91d872 ("drm/msm/adreno: Expose speedbin to userspace")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/msm/adreno/adreno_gpu.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index 15c8997b7251..f7b3f6d266a9 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -244,10 +244,10 @@ int adreno_get_param(struct msm_gpu *gpu, struct
msm_file_private *ctx,
*value = !adreno_is_a650_family(adreno_gpu) ? 0x10 : 0;
return 0;
case MSM_PARAM_CHIP_ID:
- *value = (uint64_t) adreno_gpu->rev.patchid |
- (uint64_t) (adreno_gpu->rev.minor << 8) |
- (uint64_t) (adreno_gpu->rev.major << 16) |
- (uint64_t) (adreno_gpu->rev.core << 24);
+ *value = (uint64_t)adreno_gpu->rev.patchid |
+((uint64_t)adreno_gpu->rev.minor << 8) |
+((uint64_t)adreno_gpu->rev.major << 16) |
+((uint64_t)adreno_gpu->rev.core << 24);
if (!adreno_gpu->info->revn)
*value |= ((uint64_t) adreno_gpu->speedbin) << 32;
return 0;
--
2.20.1
[Freedreno] [PATCH] drm/msm/dp: Fix double free on error in msm_dp_bridge_init()
The "dp_bridge" pointer is allocated with devm_kzalloc() so it will be
freed automatically. Kfreeing it here will only lead to a double free.
Fixes: 8a3b4c17f863 ("drm/msm/dp: employ bridge mechanism for display enable
and disable")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/msm/dp/dp_drm.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index 188e77c59885..d4d360d19eba 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -243,7 +243,6 @@ struct drm_bridge *msm_dp_bridge_init(struct msm_dp
*dp_display, struct drm_devi
rc = drm_bridge_attach(encoder, bridge, NULL,
DRM_BRIDGE_ATTACH_NO_CONNECTOR);
if (rc) {
DRM_ERROR("failed to attach bridge, rc=%d\n", rc);
- kfree(dp_bridge);
return ERR_PTR(rc);
}
--
2.20.1
[Freedreno] [PATCH 2/2] drm/msm: uninitialized variable in msm_gem_import()
The msm_gem_new_impl() function cleans up after itself so there is no
need to call drm_gem_object_put(). Conceptually, it does not make sense
to call a kref_put() function until after the reference counting has
been initialized which happens immediately after this call in the
drm_gem_(private_)object_init() functions.
In the msm_gem_import() function the "obj" pointer is uninitialized, so
it will lead to a crash.
Fixes: 05b849111c07 ("drm/msm: prime support")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/msm/msm_gem.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index 49185d524be3..0e491cd21c53 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -1167,7 +1167,7 @@ struct drm_gem_object *msm_gem_new(struct drm_device
*dev, uint32_t size, uint32
ret = msm_gem_new_impl(dev, size, flags, );
if (ret)
- goto fail;
+ return ERR_PTR(ret);
msm_obj = to_msm_bo(obj);
@@ -1251,7 +1251,7 @@ struct drm_gem_object *msm_gem_import(struct drm_device
*dev,
ret = msm_gem_new_impl(dev, size, MSM_BO_WC, );
if (ret)
- goto fail;
+ return ERR_PTR(ret);
drm_gem_private_object_init(dev, obj, size);
--
2.20.1
[Freedreno] [PATCH 1/2] drm/msm: fix potential NULL dereference in cleanup
The "msm_obj->node" list needs to be initialized earlier so that the
list_del() in msm_gem_free_object() doesn't experience a NULL pointer
dereference.
Fixes: 6ed0897cd800 ("drm/msm: Fix debugfs deadlock")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/msm/msm_gem.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index 40a9863f5951..49185d524be3 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -1132,6 +1132,7 @@ static int msm_gem_new_impl(struct drm_device *dev,
msm_obj->flags = flags;
msm_obj->madv = MSM_MADV_WILLNEED;
+ INIT_LIST_HEAD(_obj->node);
INIT_LIST_HEAD(_obj->vmas);
*obj = _obj->base;
--
2.20.1
[Freedreno] [bug report] drm/msm/a6xx: Fix llcc configuration for a660 gpu
Hello Akhil P Oommen,
The patch a6f24383f6c0: "drm/msm/a6xx: Fix llcc configuration for
a660 gpu" from Jul 30, 2021, leads to the following Smatch static
checker warning:
drivers/gpu/drm/msm/adreno/a6xx_gpu.c:1480 a6xx_llc_activate()
error: uninitialized symbol 'gpu_scid'.
drivers/gpu/drm/msm/adreno/a6xx_gpu.c
1423 static void a6xx_llc_activate(struct a6xx_gpu *a6xx_gpu)
1424 {
1425 struct adreno_gpu *adreno_gpu = _gpu->base;
1426 struct msm_gpu *gpu = _gpu->base;
1427 u32 gpu_scid, cntl1_regval = 0;
1428
1429 if (IS_ERR(a6xx_gpu->llc_mmio))
1430 return;
1431
1432 if (!llcc_slice_activate(a6xx_gpu->llc_slice)) {
1433 gpu_scid = llcc_get_slice_id(a6xx_gpu->llc_slice);
1434
1435 gpu_scid &= 0x1f;
1436 cntl1_regval = (gpu_scid << 0) | (gpu_scid << 5) |
(gpu_scid << 10) |
1437(gpu_scid << 15) | (gpu_scid << 20);
1438 }
gpu_scid not initialized on the else path.
1439
1440 /*
1441 * For targets with a MMU500, activate the slice but don't
program the
1442 * register. The XBL will take care of that.
1443 */
1444 if (!llcc_slice_activate(a6xx_gpu->htw_llc_slice)) {
1445 if (!a6xx_gpu->have_mmu500) {
1446 u32 gpuhtw_scid =
llcc_get_slice_id(a6xx_gpu->htw_llc_slice);
1447
1448 gpuhtw_scid &= 0x1f;
1449 cntl1_regval |= FIELD_PREP(GENMASK(29, 25),
gpuhtw_scid);
1450 }
1451 }
1452
1453 if (!cntl1_regval)
1454 return;
1455
1456 /*
1457 * Program the slice IDs for the various GPU blocks and GPU MMU
1458 * pagetables
1459 */
1460 if (!a6xx_gpu->have_mmu500) {
1461 a6xx_llc_write(a6xx_gpu,
1462 REG_A6XX_CX_MISC_SYSTEM_CACHE_CNTL_1,
cntl1_regval);
1463
1464 /*
1465 * Program cacheability overrides to not allocate cache
1466 * lines on a write miss
1467 */
1468 a6xx_llc_rmw(a6xx_gpu,
1469 REG_A6XX_CX_MISC_SYSTEM_CACHE_CNTL_0, 0xF,
0x03);
1470 return;
1471 }
1472
1473 gpu_rmw(gpu, REG_A6XX_GBIF_SCACHE_CNTL1, GENMASK(24, 0),
cntl1_regval);
1474
1475 /* On A660, the SCID programming for UCHE traffic is done in
1476 * A6XX_GBIF_SCACHE_CNTL0[14:10]
1477 */
1478 if (adreno_is_a660_family(adreno_gpu))
1479 gpu_rmw(gpu, REG_A6XX_GBIF_SCACHE_CNTL0, (0x1f << 10) |
--> 1480 (1 << 8), (gpu_scid << 10) | (1 << 8));
^^
Used here.
1481 }
regards,
dan carpenter
[Freedreno] [PATCH] drm/msm: unlock on error in get_sched_entity()
Add a missing unlock on the error path if drm_sched_entity_init() fails.
Fixes: 68002469e571 ("drm/msm: One sched entity per process per priority")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/msm/msm_submitqueue.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c
b/drivers/gpu/drm/msm/msm_submitqueue.c
index b8621c6e0554..7cb158bcbcf6 100644
--- a/drivers/gpu/drm/msm/msm_submitqueue.c
+++ b/drivers/gpu/drm/msm/msm_submitqueue.c
@@ -101,6 +101,7 @@ get_sched_entity(struct msm_file_private *ctx, struct
msm_ringbuffer *ring,
ret = drm_sched_entity_init(entity, sched_prio, , 1,
NULL);
if (ret) {
+ mutex_unlock(_lock);
kfree(entity);
return ERR_PTR(ret);
}
--
2.20.1
Re: [Freedreno] [PATCH] drm/msm/disp: fix endian bug in debugfs code
On Tue, Oct 05, 2021 at 02:31:12AM +0300, Dmitry Baryshkov wrote:
> On 04/10/2021 16:47, Dan Carpenter wrote:
> > The "vbif->features" is type unsigned long but the debugfs file
> > is treating it as a u32 type. This will work in little endian
> > systems, but the correct thing is to change the debugfs to use
> > an unsigned long.
> >
> > Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
> > Signed-off-by: Dan Carpenter
> > ---
> > You might wonder why this code has so many casts. It's required because
> > this data is const. Which is fine because the file is read only.
> >
> > drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
> > index 21d20373eb8b..e645a886e3c6 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
> > @@ -305,8 +305,8 @@ void dpu_debugfs_vbif_init(struct dpu_kms *dpu_kms,
> > struct dentry *debugfs_root)
> > debugfs_vbif = debugfs_create_dir(vbif_name, entry);
> > - debugfs_create_u32("features", 0600, debugfs_vbif,
> > - (u32 *)>features);
> > + debugfs_create_ulong("features", 0600, debugfs_vbif,
> > +(unsigned long *)>features);
>
> As you are converting this to the ulong file, could you please also remove
> the now-unnecessary type cast?
I wanted to remove all the casting but they are required because of the
const.
regards,
dan carpenter
[Freedreno] [PATCH] drm/msm: Fix potential Oops in a6xx_gmu_rpmh_init()
There are two problems here:
1) The "seqptr" is used uninitalized when we free it at the end.
2) The a6xx_gmu_get_mmio() function returns error pointers. It never
returns true.
Fixes: 64245fc55172 ("drm/msm/a6xx: use AOP-initialized PDC for a650")
Fixes: f8fc924e088e ("drm/msm/a6xx: Fix PDC register overlap")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index a7c58018959f..3bd6e579ea89 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -512,11 +512,11 @@ static void a6xx_gmu_rpmh_init(struct a6xx_gmu *gmu)
struct adreno_gpu *adreno_gpu = _gpu->base;
struct platform_device *pdev = to_platform_device(gmu->dev);
void __iomem *pdcptr = a6xx_gmu_get_mmio(pdev, "gmu_pdc");
- void __iomem *seqptr;
+ void __iomem *seqptr = NULL;
uint32_t pdc_address_offset;
bool pdc_in_aop = false;
- if (!pdcptr)
+ if (IS_ERR(pdcptr))
goto err;
if (adreno_is_a650(adreno_gpu) || adreno_is_a660_family(adreno_gpu))
@@ -528,7 +528,7 @@ static void a6xx_gmu_rpmh_init(struct a6xx_gmu *gmu)
if (!pdc_in_aop) {
seqptr = a6xx_gmu_get_mmio(pdev, "gmu_pdc_seq");
- if (!seqptr)
+ if (IS_ERR(seqptr))
goto err;
}
--
2.20.1
[Freedreno] [PATCH] drm/msm/disp: fix endian bug in debugfs code
The "vbif->features" is type unsigned long but the debugfs file
is treating it as a u32 type. This will work in little endian
systems, but the correct thing is to change the debugfs to use
an unsigned long.
Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
Signed-off-by: Dan Carpenter
---
You might wonder why this code has so many casts. It's required because
this data is const. Which is fine because the file is read only.
drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
index 21d20373eb8b..e645a886e3c6 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_vbif.c
@@ -305,8 +305,8 @@ void dpu_debugfs_vbif_init(struct dpu_kms *dpu_kms, struct
dentry *debugfs_root)
debugfs_vbif = debugfs_create_dir(vbif_name, entry);
- debugfs_create_u32("features", 0600, debugfs_vbif,
- (u32 *)>features);
+ debugfs_create_ulong("features", 0600, debugfs_vbif,
+(unsigned long *)>features);
debugfs_create_u32("xin_halt_timeout", 0400, debugfs_vbif,
(u32 *)>xin_halt_timeout);
--
2.20.1
[Freedreno] [bug report] drm/msm: Add SDM845 DPU support
Hello MSM Devs,
The patch 25fdd5933e4c: "drm/msm: Add SDM845 DPU support" from Jun
27, 2018, leads to the following Smatch static checker warnings:
drivers/gpu/drm/msm/msm_gpu.c:301 msm_gpu_crashstate_capture() error: potential
null dereference 'state->bos'. (kcalloc returns null)
drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c:177
msm_disp_snapshot_add_block() error: potential null dereference 'new_blk'.
(kzalloc returns null)
drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c:96 mdp5_plane_reset() error:
potential null dereference 'mdp5_state'. (kzalloc returns null)
drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c:98 mdp5_plane_reset() error:
potential null dereference 'mdp5_state'. (kzalloc returns null)
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c:963 dpu_crtc_atomic_check() error:
potential null dereference 'pstates'. (kzalloc returns null)
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c:1009 dpu_crtc_atomic_check() error:
potential null dereference 'pstates'. (kzalloc returns null)
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c:1075 dpu_crtc_atomic_check() error:
potential null dereference 'pstates'. (kzalloc returns null)
drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c:214 dpu_core_irq_preinstall()
error: potential null dereference 'dpu_kms->irq_obj.irq_cb_tbl'. (kcalloc
returns null)
drivers/gpu/drm/msm/disp/dpu1/dpu_core_irq.c:215 dpu_core_irq_preinstall()
error: potential null dereference 'dpu_kms->irq_obj.irq_counts'. (kcalloc
returns null)
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
901 static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
902 struct drm_atomic_state *state)
903 {
904 struct drm_crtc_state *crtc_state =
drm_atomic_get_new_crtc_state(state,
905
crtc);
906 struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
907 struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc_state);
908 struct plane_state *pstates;
909
910 const struct drm_plane_state *pstate;
911 struct drm_plane *plane;
912 struct drm_display_mode *mode;
913
914 int cnt = 0, rc = 0, mixer_width = 0, i, z_pos;
915
916 struct dpu_multirect_plane_states multirect_plane[DPU_STAGE_MAX
* 2];
917 int multirect_count = 0;
918 const struct drm_plane_state *pipe_staged[SSPP_MAX];
919 int left_zpos_cnt = 0, right_zpos_cnt = 0;
920 struct drm_rect crtc_rect = { 0 };
921
922 pstates = kzalloc(sizeof(*pstates) * DPU_STAGE_MAX * 4,
GFP_KERNEL);
^
There are a bunch of allocations with no checks for NULL
923
924 if (!crtc_state->enable || !crtc_state->active) {
925 DRM_DEBUG_ATOMIC("crtc%d -> enable %d, active %d, skip
atomic_check\n",
926 crtc->base.id, crtc_state->enable,
927 crtc_state->active);
928 memset(>new_perf, 0, sizeof(cstate->new_perf));
929 goto end;
930 }
931
932 mode = _state->adjusted_mode;
933 DRM_DEBUG_ATOMIC("%s: check\n", dpu_crtc->name);
934
935 /* force a full mode set if active state changed */
936 if (crtc_state->active_changed)
937 crtc_state->mode_changed = true;
938
939 memset(pipe_staged, 0, sizeof(pipe_staged));
940
941 if (cstate->num_mixers) {
942 mixer_width = mode->hdisplay / cstate->num_mixers;
943
944 _dpu_crtc_setup_lm_bounds(crtc, crtc_state);
945 }
946
947 crtc_rect.x2 = mode->hdisplay;
948 crtc_rect.y2 = mode->vdisplay;
949
950 /* get plane state for all drm planes associated with crtc
state */
951 drm_atomic_crtc_state_for_each_plane_state(plane, pstate,
crtc_state) {
952 struct drm_rect dst, clip = crtc_rect;
953
954 if (IS_ERR_OR_NULL(pstate)) {
955 rc = PTR_ERR(pstate);
956 DPU_ERROR("%s: failed to get plane%d state,
%d\n",
957 dpu_crtc->name, plane->base.id,
rc);
958 goto end;
959 }
960 if (cnt >= DPU_STAGE_MAX * 4)
961 continue;
962
--> 963 pstates[cnt].dpu_pstate = to_dpu_plane_state(pstate);
964 pstates[cnt].drm_pstate = pstate;
965 pstates[cnt].stage = pstate->normalized_zpos;
966 pstates[cnt].pipe_id = dpu_plane_pipe(plane);
967
regards,
dan carpenter
[Freedreno] [PATCH] drm/msm: potential error pointer dereference in init()
The msm_iommu_new() returns error pointers on failure so check for that
to avoid an Oops.
Fixes: ccac7ce373c1 ("drm/msm: Refactor address space initialization")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index ae48f41821cf..ad247c06e198 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -908,6 +908,10 @@ static int _dpu_kms_mmu_init(struct dpu_kms *dpu_kms)
return 0;
mmu = msm_iommu_new(dpu_kms->dev->dev, domain);
+ if (IS_ERR(mmu)) {
+ iommu_domain_free(domain);
+ return PTR_ERR(mmu);
+ }
aspace = msm_gem_address_space_create(mmu, "dpu1",
0x1000, 0x1 - 0x1000);
--
2.20.1
Re: [Freedreno] [PATCH 3/3] drm/msm/dsi: fix signedness bug in msm_dsi_host_cmd_rx()
On Sat, Oct 02, 2021 at 01:59:56AM +0300, Dmitry Baryshkov wrote:
> On 01/10/2021 15:36, Dan Carpenter wrote:
> > The "msg->tx_len" variable is type size_t so if dsi_cmds2buf_tx()
> > returns a negative error code that it type promoted to a high positive
> > value and treat as a success. The second problem with this code is
> > that it can return meaningless positive values on error.
>
> It looks to me that this piece of code is not fully correct at all.
> dsi_cmds2bus_tx would return the size of DSI packet, not the size of the DSI
> buffer.
Ugh... I misread what you were saying. I was thinking I could just
check for negatives. This sounds like struct_size() thing?
>
> Could you please be more specific, which 'meaningless positive values' were
> you receiving?
>
Returning any positive values at this point is a bug. It's supposed to
return the number of bytes that were recieved.
And there is another bug as well:
drivers/gpu/drm/msm/dsi/dsi_host.c
1370 static int dsi_cmds2buf_tx(struct msm_dsi_host *msm_host,
1371 const struct mipi_dsi_msg *msg)
1372 {
1373 int len, ret;
1374 int bllp_len = msm_host->mode->hdisplay *
1375 dsi_get_bpp(msm_host->format) / 8;
1376
1377 len = dsi_cmd_dma_add(msm_host, msg);
1378 if (!len) {
The dsi_cmd_dma_add() returns negative error codes so this check should
be "if (len <= 0) {".
1379 pr_err("%s: failed to add cmd type = 0x%x\n",
1380 __func__, msg->type);
1381 return -EINVAL;
1382 }
1383
I'm not sure about the size of "the DSI packet" Could you handle this
one and give me a Reported-by tag? That's probably simpler than another
back and forth on email.
regards,
dan carpenter
Re: [Freedreno] [PATCH 3/3] drm/msm/dsi: fix signedness bug in msm_dsi_host_cmd_rx()
On Sat, Oct 02, 2021 at 01:59:56AM +0300, Dmitry Baryshkov wrote: > On 01/10/2021 15:36, Dan Carpenter wrote: > > The "msg->tx_len" variable is type size_t so if dsi_cmds2buf_tx() > > returns a negative error code that it type promoted to a high positive > > value and treat as a success. The second problem with this code is > > that it can return meaningless positive values on error. > > It looks to me that this piece of code is not fully correct at all. > dsi_cmds2bus_tx would return the size of DSI packet, not the size of the DSI > buffer. > > Could you please be more specific, which 'meaningless positive values' were > you receiving? > Sorry, I misread the code. I thought it returned negatives or the number of bytes copied. (This is from static analysis btw). Anyway, returning only negatives is a much better way. I will fix this patch and resend. regards, dan carpenter
[Freedreno] [PATCH] drm/msm/a3xx: fix error handling in a3xx_gpu_init()
These error paths returned 1 on failure, instead of a negative error
code. This would lead to an Oops in the caller. A second problem is
that the check for "if (ret != -ENODATA)" did not work because "ret" was
set to 1.
Fixes: 5785dd7a8ef0 ("drm/msm: Fix duplicate gpu node in icc summary")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/msm/adreno/a3xx_gpu.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
index 4534633fe7cd..8fb847c174ff 100644
--- a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
@@ -571,13 +571,14 @@ struct msm_gpu *a3xx_gpu_init(struct drm_device *dev)
}
icc_path = devm_of_icc_get(>dev, "gfx-mem");
- ret = IS_ERR(icc_path);
- if (ret)
+ if (IS_ERR(icc_path)) {
+ ret = PTR_ERR(icc_path);
goto fail;
+ }
ocmem_icc_path = devm_of_icc_get(>dev, "ocmem");
- ret = IS_ERR(ocmem_icc_path);
- if (ret) {
+ if (IS_ERR(ocmem_icc_path)) {
+ ret = PTR_ERR(ocmem_icc_path);
/* allow -ENODATA, ocmem icc is optional */
if (ret != -ENODATA)
goto fail;
--
2.20.1
[Freedreno] [PATCH] drm/msm/a4xx: fix error handling in a4xx_gpu_init()
This code returns 1 on error instead of a negative error. It leads to
an Oops in the caller. A second problem is that the check for
"if (ret != -ENODATA)" cannot be true because "ret" is set to 1.
Fixes: 5785dd7a8ef0 ("drm/msm: Fix duplicate gpu node in icc summary")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/msm/adreno/a4xx_gpu.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
index 82bebb40234d..a96ee79cc5e0 100644
--- a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
@@ -699,13 +699,14 @@ struct msm_gpu *a4xx_gpu_init(struct drm_device *dev)
}
icc_path = devm_of_icc_get(>dev, "gfx-mem");
- ret = IS_ERR(icc_path);
- if (ret)
+ if (IS_ERR(icc_path)) {
+ ret = PTR_ERR(icc_path);
goto fail;
+ }
ocmem_icc_path = devm_of_icc_get(>dev, "ocmem");
- ret = IS_ERR(ocmem_icc_path);
- if (ret) {
+ if (IS_ERR(ocmem_icc_path)) {
+ ret = PTR_ERR(ocmem_icc_path);
/* allow -ENODATA, ocmem icc is optional */
if (ret != -ENODATA)
goto fail;
--
2.20.1
[Freedreno] [PATCH 3/3] drm/msm/dsi: fix signedness bug in msm_dsi_host_cmd_rx()
The "msg->tx_len" variable is type size_t so if dsi_cmds2buf_tx()
returns a negative error code that it type promoted to a high positive
value and treat as a success. The second problem with this code is
that it can return meaningless positive values on error.
Fixes: a689554ba6ed ("drm/msm: Initial add DSI connector support")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/msm/dsi/dsi_host.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
b/drivers/gpu/drm/msm/dsi/dsi_host.c
index c86b5090fae6..42073a562072 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -2133,8 +2133,10 @@ int msm_dsi_host_cmd_rx(struct mipi_dsi_host *host,
}
ret = dsi_cmds2buf_tx(msm_host, msg);
- if (ret < msg->tx_len) {
+ if (ret < 0 || ret < msg->tx_len) {
pr_err("%s: Read cmd Tx failed, %d\n", __func__, ret);
+ if (ret >= 0)
+ ret = -EIO;
return ret;
}
--
2.20.1
[Freedreno] [PATCH 2/3] drm/msm/dsi: fix off by one in dsi_bus_clk_enable error handling
This disables a lock which wasn't enabled and it does not disable
the first lock in the array.
Fixes: 6e0eb52eba9e ("drm/msm/dsi: Parse bus clocks from a list")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/msm/dsi/dsi_host.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
b/drivers/gpu/drm/msm/dsi/dsi_host.c
index e269df285136..c86b5090fae6 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -451,7 +451,7 @@ static int dsi_bus_clk_enable(struct msm_dsi_host *msm_host)
return 0;
err:
- for (; i > 0; i--)
+ while (--i >= 0)
clk_disable_unprepare(msm_host->bus_clks[i]);
return ret;
--
2.20.1
[Freedreno] [PATCH 1/3] drm/msm/dsi: Fix an error code in msm_dsi_modeset_init()
Return an error code if msm_dsi_manager_validate_current_config().
Don't return success.
Fixes: 8b03ad30e314 ("drm/msm/dsi: Use one connector for dual DSI mode")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/msm/dsi/dsi.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c
index 614dc7f26f2c..75ae3008b68f 100644
--- a/drivers/gpu/drm/msm/dsi/dsi.c
+++ b/drivers/gpu/drm/msm/dsi/dsi.c
@@ -215,8 +215,10 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct
drm_device *dev,
goto fail;
}
- if (!msm_dsi_manager_validate_current_config(msm_dsi->id))
+ if (!msm_dsi_manager_validate_current_config(msm_dsi->id)) {
+ ret = -EINVAL;
goto fail;
+ }
msm_dsi->encoder = encoder;
--
2.20.1
[Freedreno] [kbuild] Re: [PATCH v2 04/13] drm/hdcp: Expand HDCP helper library for enable/disable/check
Hi Sean,
url:
https://github.com/0day-ci/linux/commits/Sean-Paul/drm-hdcp-Pull-HDCP-auth-exchange-check-into-helpers/20210916-044145
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-randconfig-m001-20210916 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
Reported-by: Dan Carpenter
New smatch warnings:
drivers/gpu/drm/drm_hdcp.c:1208 drm_hdcp_helper_enable_hdcp() error:
uninitialized symbol 'check_link_interval'.
Old smatch warnings:
drivers/gpu/drm/drm_hdcp.c:514 drm_hdcp_atomic_check() warn: inconsistent
indenting
vim +/check_link_interval +1208 drivers/gpu/drm/drm_hdcp.c
cbc5065be3a652f Sean Paul 2021-09-15 1127 static int
drm_hdcp_helper_enable_hdcp(struct drm_hdcp_helper_data *data,
cbc5065be3a652f Sean Paul 2021-09-15 1128
struct drm_atomic_state *state,
cbc5065be3a652f Sean Paul 2021-09-15 1129
struct mutex *driver_mutex)
cbc5065be3a652f Sean Paul 2021-09-15 1130 {
cbc5065be3a652f Sean Paul 2021-09-15 1131 struct drm_connector *connector
= data->connector;
cbc5065be3a652f Sean Paul 2021-09-15 1132 struct drm_connector_state
*conn_state;
cbc5065be3a652f Sean Paul 2021-09-15 1133 struct drm_device *dev =
connector->dev;
cbc5065be3a652f Sean Paul 2021-09-15 1134 unsigned long
check_link_interval;
^^^
cbc5065be3a652f Sean Paul 2021-09-15 1135 bool capable;
cbc5065be3a652f Sean Paul 2021-09-15 1136 int ret = 0;
cbc5065be3a652f Sean Paul 2021-09-15 1137
cbc5065be3a652f Sean Paul 2021-09-15 1138 conn_state =
drm_atomic_get_new_connector_state(state, connector);
cbc5065be3a652f Sean Paul 2021-09-15 1139
cbc5065be3a652f Sean Paul 2021-09-15 1140 mutex_lock(>mutex);
cbc5065be3a652f Sean Paul 2021-09-15 1141
cbc5065be3a652f Sean Paul 2021-09-15 1142 if (data->value ==
DRM_MODE_CONTENT_PROTECTION_ENABLED) {
cbc5065be3a652f Sean Paul 2021-09-15 1143
drm_hdcp_update_value(data, DRM_MODE_CONTENT_PROTECTION_ENABLED,
cbc5065be3a652f Sean Paul 2021-09-15 1144
true);
cbc5065be3a652f Sean Paul 2021-09-15 1145 goto out_data_mutex;
cbc5065be3a652f Sean Paul 2021-09-15 1146 }
cbc5065be3a652f Sean Paul 2021-09-15 1147
cbc5065be3a652f Sean Paul 2021-09-15 1148 drm_WARN_ON(dev,
data->driver_mutex != NULL);
cbc5065be3a652f Sean Paul 2021-09-15 1149 data->driver_mutex =
driver_mutex;
cbc5065be3a652f Sean Paul 2021-09-15 1150
cbc5065be3a652f Sean Paul 2021-09-15 1151
drm_hdcp_helper_driver_lock(data);
cbc5065be3a652f Sean Paul 2021-09-15 1152
cbc5065be3a652f Sean Paul 2021-09-15 1153 if (data->funcs->setup) {
cbc5065be3a652f Sean Paul 2021-09-15 1154 ret =
data->funcs->setup(connector, state);
cbc5065be3a652f Sean Paul 2021-09-15 1155 if (ret) {
cbc5065be3a652f Sean Paul 2021-09-15 1156 drm_err(dev,
"Failed to setup HDCP %d\n", ret);
cbc5065be3a652f Sean Paul 2021-09-15 1157 goto out;
cbc5065be3a652f Sean Paul 2021-09-15 1158 }
cbc5065be3a652f Sean Paul 2021-09-15 1159 }
cbc5065be3a652f Sean Paul 2021-09-15 1160
cbc5065be3a652f Sean Paul 2021-09-15 1161 if
(!data->funcs->are_keys_valid ||
cbc5065be3a652f Sean Paul 2021-09-15 1162
!data->funcs->are_keys_valid(connector)) {
cbc5065be3a652f Sean Paul 2021-09-15 1163 if
(data->funcs->load_keys) {
cbc5065be3a652f Sean Paul 2021-09-15 1164 ret =
data->funcs->load_keys(connector);
cbc5065be3a652f Sean Paul 2021-09-15 1165 if (ret) {
cbc5065be3a652f Sean Paul 2021-09-15 1166
drm_err(dev, "Failed to load HDCP keys %d\n", ret);
cbc5065be3a652f Sean Paul 2021-09-15 1167 goto
out;
cbc5065be3a652f Sean Paul 2021-09-15 1168 }
cbc5065be3a652f Sean Paul 2021-09-15 1169 }
cbc5065be3a652f Sean Paul 2021-09-15 1170 }
cbc5065be3a652f Sean Paul 2021-09-15 1171
cbc5065be3a652f Sean Paul 2021-09-15 1172 /*
cbc5065be3a652f Sean Paul 2021-09-15 1173 * Considering that HDCP2.2 is
more secure than HDCP1.4, If the setup
cbc5065be3a652f Sean Paul 2021-09-15 1174 * is capable of HDCP2.2, it is
preferred to use HDCP2.2.
cbc5065be3a652f Sean Paul 2021-09-15 1175 */
cbc5065be3a652f Sean Paul 2021-09-15 1176 ret =
data->funcs->hdcp2_capable(connector, );
cbc5065be3a652f Sean Paul 2021-09-15 1177 if (ret) {
cbc5065be3a652f Sean Paul 2021-09-15 1178 drm_err(dev, "HDCP 2.x
capability check failed %d\n", ret);
c
Re: [Freedreno] [PATCH] drm/msm/dp: Fix warnings reported by kbot in DP driver
On Thu, Mar 04, 2021 at 10:55:58PM -0800, Stephen Boyd wrote:
> > @@ -368,44 +368,21 @@ static int dp_debug_init(struct dp_debug *dp_debug,
> > struct drm_minor *minor)
> > int rc = 0;
> > struct dp_debug_private *debug = container_of(dp_debug,
> > struct dp_debug_private, dp_debug);
> > - struct dentry *file;
> > - struct dentry *test_active;
> > - struct dentry *test_data, *test_type;
> >
> > - file = debugfs_create_file("dp_debug", 0444, minor->debugfs_root,
> > + debugfs_create_file("dp_debug", 0444, minor->debugfs_root,
> > debug, _debug_fops);
> > - if (IS_ERR_OR_NULL(file)) {
> > - rc = PTR_ERR(file);
> > - DRM_ERROR("[%s] debugfs create file failed, rc=%d\n",
> > - DEBUG_NAME, rc);
> > - }
> >
> > - test_active = debugfs_create_file("msm_dp_test_active", 0444,
> > + debugfs_create_file("msm_dp_test_active", 0444,
> > minor->debugfs_root,
> > debug, _active_fops);
> > - if (IS_ERR_OR_NULL(test_active)) {
> > - rc = PTR_ERR(test_active);
> > - DRM_ERROR("[%s] debugfs test_active failed, rc=%d\n",
> > - DEBUG_NAME, rc);
> > - }
> >
> > - test_data = debugfs_create_file("msm_dp_test_data", 0444,
> > + debugfs_create_file("msm_dp_test_data", 0444,
> > minor->debugfs_root,
> > debug, _test_data_fops);
> > - if (IS_ERR_OR_NULL(test_data)) {
> > - rc = PTR_ERR(test_data);
> > - DRM_ERROR("[%s] debugfs test_data failed, rc=%d\n",
> > - DEBUG_NAME, rc);
> > - }
> >
> > - test_type = debugfs_create_file("msm_dp_test_type", 0444,
> > + debugfs_create_file("msm_dp_test_type", 0444,
> > minor->debugfs_root,
> > debug, _test_type_fops);
> > - if (IS_ERR_OR_NULL(test_type)) {
> > - rc = PTR_ERR(test_type);
> > - DRM_ERROR("[%s] debugfs test_type failed, rc=%d\n",
> > - DEBUG_NAME, rc);
> > - }
>
> Debugfs failures.
[ Update. I misunderstood what you were saying, and initially thought
you were critiquing the patch instead of the commit message. The
patch looks okay. Probably a lot of maintainers would prefer it
broken multiple chunks with one patch per class of warning. But I
already wrote this email and I love the sound of my own voice so I'm
sending it. - dan ]
The Smatch warning for this was that the error handling was slightly
off because debugfs_create_file() doesn't return NULL these days. But
really these functions are not supposed to be error checked in the
normal case.
If you do a `git grep -w debugfs_create_file` there are 1472 callers
and only 192 check. This is partly because Greg went through and did a
mass delete of error handling.
The way that debugfs works is if you fail to create a directory then
the debugfs_create_file will check if the root is an error pointer. So
passing it "handles" errors itself.
The one time where I've seen that checking for errors is essential is
if they driver dereferences the "test_data" dentry itself. That's
pretty uncommon.
[ So probably the commit message for this chunk should be:
Delete unnecessary debugfs error handling
Debugfs functions are not supposed to be checked in the normal case
so delete this code. Also it silences a Smatch warning that we're
checking for NULL when these functions only return error pointers. ]
regards,
dan carpenter
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [kbuild] Re: [PATCH 7/8] drm/msm/dpu: add SM8150 to hw catalog
Hi Jonathan,
Thank you for the patch! Perhaps something to improve:
url:
https://github.com/0day-ci/linux/commits/Jonathan-Marek/Initial-SM8150-and-SM8250-DPU-bringup/20200526-112725
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
9cb1fd0efd195590b828b9b865421ad345a4a145
config: arm64-randconfig-s031-20200527 (attached as .config)
compiler: aarch64-linux-gcc (GCC) 9.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.1-240-gf0fe1cd9-dirty
# save the attached .config to linux build tree
make W=1 C=1 ARCH=arm64 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot
sparse warnings: (new ones prefixed by >>)
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c:98:26: sparse: sparse:
undefined identifier 'DPU_SSPP_SMART_DMA_V2_5'
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c:96:10: sparse: sparse:
Initializer entry defined twice
>> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c:104:10: sparse: also
>> defined here
#
https://github.com/0day-ci/linux/commit/6a90302321af832ea18679e1b29b0301caade709
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 6a90302321af832ea18679e1b29b0301caade709
vim +104 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
7bdc0c4b812602f Kalyan Thota 2019-11-25 92
6a90302321af832 Jonathan Marek 2020-05-25 93 static const struct dpu_caps
sm8150_dpu_caps = {
6a90302321af832 Jonathan Marek 2020-05-25 94 .max_mixer_width =
DEFAULT_DPU_OUTPUT_LINE_WIDTH,
6a90302321af832 Jonathan Marek 2020-05-25 95 .max_mixer_blendstages
= 0xb,
6a90302321af832 Jonathan Marek 2020-05-25 96 .max_linewidth = 4096,
^^
6a90302321af832 Jonathan Marek 2020-05-25 97 .qseed_type =
DPU_SSPP_SCALER_QSEED3,
6a90302321af832 Jonathan Marek 2020-05-25 98 .smart_dma_rev =
DPU_SSPP_SMART_DMA_V2_5,
6a90302321af832 Jonathan Marek 2020-05-25 99 .ubwc_version =
DPU_HW_UBWC_VER_30,
6a90302321af832 Jonathan Marek 2020-05-25 100 .has_src_split = true,
6a90302321af832 Jonathan Marek 2020-05-25 101 .has_dim_layer = true,
6a90302321af832 Jonathan Marek 2020-05-25 102 .has_idle_pc = true,
6a90302321af832 Jonathan Marek 2020-05-25 103 .has_3d_merge = true,
6a90302321af832 Jonathan Marek 2020-05-25 @104 .max_linewidth = 4096,
^^
Delete duplicate code.
6a90302321af832 Jonathan Marek 2020-05-25 105 .pixel_ram_size =
DEFAULT_PIXEL_RAM_SIZE,
6a90302321af832 Jonathan Marek 2020-05-25 106 .max_hdeci_exp =
MAX_HORZ_DECIMATION,
6a90302321af832 Jonathan Marek 2020-05-25 107 .max_vdeci_exp =
MAX_VERT_DECIMATION,
6a90302321af832 Jonathan Marek 2020-05-25 108 };
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
___
kbuild mailing list -- kbu...@lists.01.org
To unsubscribe send an email to kbuild-le...@lists.01.org
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH] drm/msm: fix an error code in the ioctl
The copy_to/from_user() functions return the number of bytes remaining
to be copied but we should return -EFAULT to the user.
Fixes: f05c83e77460 ("drm/msm: add uapi to get/set debug name")
Signed-off-by: Dan Carpenter
---
If I were reviewing this patch, I would be suspicous that we don't
return immediately after the first copy_from_user() fails but I'm fairly
sure that is the correct behavior.
drivers/gpu/drm/msm/msm_drv.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index b871e2e98129..1d4426cb260d 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -851,8 +851,9 @@ static int msm_ioctl_gem_info(struct drm_device *dev, void
*data,
ret = -EINVAL;
break;
}
- ret = copy_from_user(msm_obj->name,
- u64_to_user_ptr(args->value), args->len);
+ if (copy_from_user(msm_obj->name, u64_to_user_ptr(args->value),
+ args->len))
+ ret = -EFAULT;
msm_obj->name[args->len] = '\0';
for (i = 0; i < args->len; i++) {
if (!isprint(msm_obj->name[i])) {
@@ -868,8 +869,9 @@ static int msm_ioctl_gem_info(struct drm_device *dev, void
*data,
}
args->len = strlen(msm_obj->name);
if (args->value) {
- ret = copy_to_user(u64_to_user_ptr(args->value),
- msm_obj->name, args->len);
+ if (copy_to_user(u64_to_user_ptr(args->value),
+msm_obj->name, args->len))
+ ret = -EFAULT;
}
break;
}
--
2.17.1
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH] drm/msm/gpu: Fix a couple memory leaks in debugfs
The msm_gpu_open() function should free "show_priv" on error or it
causes static checker warnings.
Fixes: 4f776f4511c7 ("drm/msm/gpu: Convert the GPU show function to use the GPU
state")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/msm/msm_debugfs.c | 15 +++
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_debugfs.c
b/drivers/gpu/drm/msm/msm_debugfs.c
index f0da0d3c8a80..d756436c1fcd 100644
--- a/drivers/gpu/drm/msm/msm_debugfs.c
+++ b/drivers/gpu/drm/msm/msm_debugfs.c
@@ -84,7 +84,7 @@ static int msm_gpu_open(struct inode *inode, struct file
*file)
ret = mutex_lock_interruptible(>struct_mutex);
if (ret)
- return ret;
+ goto free_priv;
pm_runtime_get_sync(>pdev->dev);
show_priv->state = gpu->funcs->gpu_state_get(gpu);
@@ -94,13 +94,20 @@ static int msm_gpu_open(struct inode *inode, struct file
*file)
if (IS_ERR(show_priv->state)) {
ret = PTR_ERR(show_priv->state);
- kfree(show_priv);
- return ret;
+ goto free_priv;
}
show_priv->dev = dev;
- return single_open(file, msm_gpu_show, show_priv);
+ ret = single_open(file, msm_gpu_show, show_priv);
+ if (ret)
+ goto free_priv;
+
+ return 0;
+
+free_priv:
+ kfree(show_priv);
+ return ret;
}
static const struct file_operations msm_gpu_fops = {
--
2.18.0
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [bug report] drm/msm: Add SDM845 DPU support
Hello Jeykumar Sankaran,
This is a semi-automatic email about new static checker warnings.
The patch 25fdd5933e4c: "drm/msm: Add SDM845 DPU support" from Jun
27, 2018, leads to the following Smatch complaint:
./drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c:2464
dpu_encoder_wait_for_event()
warn: variable dereferenced before check 'phys' (see line 2456)
./drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
2455 case MSM_ENC_VBLANK:
2456 fn_wait = phys->ops.wait_for_vblank;
We always dereference "phys"
2457 break;
2458 default:
2459 DPU_ERROR_ENC(dpu_enc, "unknown wait event
%d\n",
2460 event);
2461 return -EINVAL;
^^
or we return
2462 };
2463
2464 if (phys && fn_wait) {
This check is too late.
2465 DPU_ATRACE_BEGIN("wait_for_completion_event");
2466 ret = fn_wait(phys);
regards,
dan carpenter
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [bug report] drm/msm: add a5xx specific debugfs
Hello Rob Clark,
This is a semi-automatic email about new static checker warnings.
The patch 024ad8df763f: "drm/msm: add a5xx specific debugfs" from Dec
13, 2017, leads to the following Smatch complaint:
./drivers/gpu/drm/msm/adreno/a5xx_debugfs.c:166 a5xx_debugfs_init()
warn: variable dereferenced before check 'minor' (see line 162)
./drivers/gpu/drm/msm/adreno/a5xx_debugfs.c
161 {
162 struct drm_device *dev = minor->dev;
^^
Dereference
163 struct dentry *ent;
164 int ret;
165
166 if (!minor)
^^
Check
167 return 0;
168
regards,
dan carpenter
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH] drm/msm: unlock on error in msm_gem_get_iova()
We recently added locking to this function but there was a direct return
that was overlooked where we need to unlock.
Fixes: 0e08270a1f01 ("drm/msm: Separate locking of buffer resources from
struct_mutex")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index 65f35544c1ec..065d933df2c3 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -383,8 +383,10 @@ int msm_gem_get_iova(struct drm_gem_object *obj,
struct page **pages;
vma = add_vma(obj, aspace);
- if (IS_ERR(vma))
- return PTR_ERR(vma);
+ if (IS_ERR(vma)) {
+ ret = PTR_ERR(vma);
+ goto unlock;
+ }
pages = get_pages(obj);
if (IS_ERR(pages)) {
@@ -405,7 +407,7 @@ int msm_gem_get_iova(struct drm_gem_object *obj,
fail:
del_vma(vma);
-
+unlock:
mutex_unlock(_obj->lock);
return ret;
}
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [PATCH] drm/msm: fix an integer overflow test
We recently added an integer overflow check but it needs an additional
tweak to work properly on 32 bit systems.
The problem is that we're doing the right hand side of the assignment as
type unsigned long so the max it will have an integer overflow instead
of being larger than SIZE_MAX. That means the "sz > SIZE_MAX" condition
is never true even on 32 bit systems. We need to first cast it to u64
and then do the math.
Fixes: 4a630fadbb29 ("drm/msm: Fix potential buffer overflow issue")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c
b/drivers/gpu/drm/msm/msm_gem_submit.c
index 6bfca7470141..8095658e8cb4 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -34,8 +34,8 @@ static struct msm_gem_submit *submit_create(struct drm_device
*dev,
struct msm_gpu *gpu, uint32_t nr_bos, uint32_t nr_cmds)
{
struct msm_gem_submit *submit;
- uint64_t sz = sizeof(*submit) + (nr_bos * sizeof(submit->bos[0])) +
- (nr_cmds * sizeof(submit->cmd[0]));
+ uint64_t sz = sizeof(*submit) + ((u64)nr_bos * sizeof(submit->bos[0])) +
+ ((u64)nr_cmds * sizeof(submit->cmd[0]));
if (sz > SIZE_MAX)
return NULL;
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [patch] drm/msm/dsi: free first element on error
On Thu, Feb 16, 2017 at 01:27:47PM +0200, Jani Nikula wrote:
> On Thu, 16 Feb 2017, Dan Carpenter <dan.carpen...@oracle.com> wrote:
> > We want to free msm_host->bus_clks[0] so the > should be >=.
> >
> > Fixes: 6e0eb52eba9e ("drm/msm/dsi: Parse bus clocks from a list")
> > Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
> >
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
> > b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > index 1fc07ce24686..239e79b39a45 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > @@ -437,7 +437,7 @@ static int dsi_bus_clk_enable(struct msm_dsi_host
> > *msm_host)
> >
> > return 0;
> > err:
> > - for (; i > 0; i--)
> > + for (; i >= 0; i--)
> > clk_disable_unprepare(msm_host->bus_clks[i]);
>
> By the looks of it this is also wrong. I didn't look at the functions,
> but you probably don't want to unprepare something where prepare failed,
> i.e. you want to -1 both the start and end offsets. Perhaps the right
> fix is
>
> while (i--)
> clk_disable_unprepare(msm_host->bus_clks[i]);
>
> which also seems to be widely used on error paths.
>
Ah yeah. You're right. I'll resend.
regards,
dan carpenter
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [patch] drm/msm/dsi: free first element on error
We want to free msm_host->bus_clks[0] so the > should be >=.
Fixes: 6e0eb52eba9e ("drm/msm/dsi: Parse bus clocks from a list")
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
b/drivers/gpu/drm/msm/dsi/dsi_host.c
index 1fc07ce24686..239e79b39a45 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -437,7 +437,7 @@ static int dsi_bus_clk_enable(struct msm_dsi_host *msm_host)
return 0;
err:
- for (; i > 0; i--)
+ for (; i >= 0; i--)
clk_disable_unprepare(msm_host->bus_clks[i]);
return ret;
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
[Freedreno] [patch] drm/msm: return -EFAULT if copy_from_user() fails
copy_from_user_inatomic() is actually a local function that returns
-EFAULT or positive values on error. Otherwise copy_from_user() returns
the number of bytes remaining to be copied. We want to return -EFAULT
here.
I removed an unlikely() because we just did a copy_from_user()
so I don't think it can possibly make a difference.
Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>
---
Not compiled.
diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c
b/drivers/gpu/drm/msm/msm_gem_submit.c
index 4896765..1172fe7 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -95,13 +95,13 @@ static int submit_lookup_objects(struct msm_gem_submit
*submit,
*/
submit->bos[i].flags = 0;
- ret = copy_from_user_inatomic(_bo, userptr,
sizeof(submit_bo));
- if (unlikely(ret)) {
+ if (copy_from_user_inatomic(_bo, userptr,
sizeof(submit_bo))) {
pagefault_enable();
spin_unlock(>table_lock);
- ret = copy_from_user(_bo, userptr,
sizeof(submit_bo));
- if (ret)
+ if (copy_from_user(_bo, userptr,
sizeof(submit_bo))) {
+ ret = -EFAULT;
goto out;
+ }
spin_lock(>table_lock);
pagefault_disable();
}
@@ -317,9 +317,10 @@ static int submit_reloc(struct msm_gem_submit *submit,
struct msm_gem_object *ob
uint64_t iova;
bool valid;
- ret = copy_from_user(_reloc, userptr,
sizeof(submit_reloc));
- if (ret)
+ if (copy_from_user(_reloc, userptr,
sizeof(submit_reloc))) {
+ ret = -EFAULT;
goto out;
+ }
if (submit_reloc.submit_offset % 4) {
DRM_ERROR("non-aligned reloc offset: %u\n",
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
