[PATCH] drm/amdgpu: delete unnecessary check
The "ret" variable is zero. No need to check.
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
index a037e8fba29f..4d50fb039509 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
@@ -2807,7 +2807,7 @@ static void amdgpu_ras_do_page_retirement(struct
work_struct *work)
static void amdgpu_ras_poison_creation_handler(struct amdgpu_device *adev,
uint32_t timeout_ms)
{
- int ret = 0;
+ int ret;
struct ras_ecc_log_info *ecc_log;
struct ras_query_if info;
uint32_t timeout = timeout_ms;
@@ -2836,8 +2836,7 @@ static void amdgpu_ras_poison_creation_handler(struct
amdgpu_device *adev,
return;
}
- if (!ret)
- schedule_delayed_work(>page_retirement_dwork, 0);
+ schedule_delayed_work(>page_retirement_dwork, 0);
}
static int amdgpu_ras_poison_consumption_handler(struct amdgpu_device *adev,
--
2.43.0
[PATCH] drm/amd/pm: Fix error code in vega10_hwmgr_backend_init()
Return -EINVAL on error instead of success. Also on the success path,
return a literal zero instead of "return result;"
Fixes: e098bc9612c2 ("drm/amd/pm: optimize the power related source code
layout")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_hwmgr.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_hwmgr.c
b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_hwmgr.c
index 37c915d7723c..9b9f8615070a 100644
--- a/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_hwmgr.c
+++ b/drivers/gpu/drm/amd/pm/powerplay/hwmgr/vega10_hwmgr.c
@@ -924,7 +924,7 @@ static int vega10_hwmgr_backend_init(struct pp_hwmgr *hwmgr)
data->total_active_cus = adev->gfx.cu_info.number;
if (!hwmgr->not_vf)
- return result;
+ return -EINVAL;
/* Setup default Overdrive Fan control settings */
data->odn_fan_table.target_fan_speed =
@@ -947,7 +947,7 @@ static int vega10_hwmgr_backend_init(struct pp_hwmgr *hwmgr)
"Mem Channel Index Exceeded maximum!",
return -EINVAL);
- return result;
+ return 0;
}
static int vega10_init_sclk_threshold(struct pp_hwmgr *hwmgr)
--
2.43.0
Re: [PATCH] staging: fb_tinylcd Alignment to open parenthesis
On Fri, May 03, 2024 at 04:20:24PM +, Ashok Kumar wrote: > > Is there a list of exceptions to the checkpatch information that we can > ignore in general. > For Greg's subsystems ignore the warning about extra parentheses. You can search on lore for if a patch has been patch has been sent before. Otherwise ignore checkpatch if it tells you to do something that makes the code less readable. regards, dan carpenter
Re: [PATCH] staging: fb_tinylcd Alignment to open parenthesis
On Thu, May 02, 2024 at 06:52:16PM -0700, Ashok Kumar wrote: > Corrected coding style CHECK: Alignment should match open parenthesis > The original author was aligned it deliberately to improve readability. Just ignore checkpatch on this. regards, dan carpenter
Re: [PATCH] staging: fbtft:fixes unnecessary parentheses
On Sun, Apr 28, 2024 at 08:19:08PM +0800, pipishuo wrote: > This patch fixes the checks reported by checkpatch.pl > for unnecessary parentheses > > Signed-off-by: pipishuo <1289151...@qq.com> > --- > drivers/staging/fbtft/fb_ili9320.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/staging/fbtft/fb_ili9320.c > b/drivers/staging/fbtft/fb_ili9320.c > index 0be7c2d51548..045275a9bc0b 100644 > --- a/drivers/staging/fbtft/fb_ili9320.c > +++ b/drivers/staging/fbtft/fb_ili9320.c > @@ -37,7 +37,8 @@ static int init_display(struct fbtft_par *par) > devcode = read_devicecode(par); > fbtft_par_dbg(DEBUG_INIT_DISPLAY, par, "Device code: 0x%04X\n", > devcode); > - if ((devcode != 0x) && (devcode != 0x9320)) > + if (devcode != 0x && > + devcode != 0x9320) Nah, just leave it. It's a personal preference for Greg. https://lore.kernel.org/all/?q=init_display%20devcode regards, dan carpenter
[PATCH] drm/amdgpu: Fix signedness bug in sdma_v4_0_process_trap_irq()
The "instance" variable needs to be signed for the error handling to work.
Fixes: b34ddc71267a ("drm/amdgpu: add error handle to avoid out-of-bounds")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
index 101038395c3b..772604feb6ac 100644
--- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
@@ -2017,7 +2017,7 @@ static int sdma_v4_0_process_trap_irq(struct
amdgpu_device *adev,
struct amdgpu_irq_src *source,
struct amdgpu_iv_entry *entry)
{
- uint32_t instance;
+ int instance;
DRM_DEBUG("IH: SDMA trap\n");
instance = sdma_v4_0_irq_id_to_seq(entry->client_id);
--
2.43.0
[PATCH] drm/amd/display: re-indent dpp401_dscl_program_isharp()
Smatch complains because some lines are indented more than they should
be. I went a bit crazy re-indenting this. ;)
The comments were not useful except as a marker of things which are left
to implement so I deleted most of them except for the TODO.
I introduced a "data" pointer so that I could replace
"scl_data->dscl_prog_data." with just "data->" and shorten the lines a
bit. It's more readable without the line breaks.
I also tried to align it so you can see what is changing on each line.
Signed-off-by: Dan Carpenter
---
.../display/dc/dpp/dcn401/dcn401_dpp_dscl.c | 93 ++-
1 file changed, 30 insertions(+), 63 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/dc/dpp/dcn401/dcn401_dpp_dscl.c
b/drivers/gpu/drm/amd/display/dc/dpp/dcn401/dcn401_dpp_dscl.c
index c20376083441..696ccf96b847 100644
--- a/drivers/gpu/drm/amd/display/dc/dpp/dcn401/dcn401_dpp_dscl.c
+++ b/drivers/gpu/drm/amd/display/dc/dpp/dcn401/dcn401_dpp_dscl.c
@@ -779,75 +779,42 @@ static void dpp401_dscl_program_isharp(struct dpp
*dpp_base,
const struct scaler_data *scl_data)
{
struct dcn401_dpp *dpp = TO_DCN401_DPP(dpp_base);
+ const struct dscl_prog_data *data;
if (memcmp(>scl_data, scl_data, sizeof(*scl_data)) == 0)
return;
PERF_TRACE();
dpp->scl_data = *scl_data;
- // ISHARP_EN
- REG_SET(ISHARP_MODE, 0,
- ISHARP_EN, scl_data->dscl_prog_data.isharp_en);
- // ISHARP_NOISEDET_EN
- REG_SET(ISHARP_MODE, 0,
- ISHARP_NOISEDET_EN,
scl_data->dscl_prog_data.isharp_noise_det.enable);
- // ISHARP_NOISEDET_MODE
- REG_SET(ISHARP_MODE, 0,
- ISHARP_NOISEDET_MODE,
scl_data->dscl_prog_data.isharp_noise_det.mode);
- // ISHARP_NOISEDET_UTHRE
- REG_SET(ISHARP_NOISEDET_THRESHOLD, 0,
- ISHARP_NOISEDET_UTHRE,
scl_data->dscl_prog_data.isharp_noise_det.uthreshold);
- // ISHARP_NOISEDET_DTHRE
- REG_SET(ISHARP_NOISEDET_THRESHOLD, 0,
- ISHARP_NOISEDET_DTHRE,
scl_data->dscl_prog_data.isharp_noise_det.dthreshold);
- REG_SET(ISHARP_MODE, 0,
- ISHARP_NOISEDET_MODE,
scl_data->dscl_prog_data.isharp_noise_det.mode);
- // ISHARP_NOISEDET_UTHRE
- REG_SET(ISHARP_NOISEDET_THRESHOLD, 0,
- ISHARP_NOISEDET_UTHRE,
scl_data->dscl_prog_data.isharp_noise_det.uthreshold);
- // ISHARP_NOISEDET_DTHRE
- REG_SET(ISHARP_NOISEDET_THRESHOLD, 0,
- ISHARP_NOISEDET_DTHRE,
scl_data->dscl_prog_data.isharp_noise_det.dthreshold);
- // ISHARP_NOISEDET_PWL_START_IN
- REG_SET(ISHARP_NOISE_GAIN_PWL, 0,
- ISHARP_NOISEDET_PWL_START_IN,
scl_data->dscl_prog_data.isharp_noise_det.pwl_start_in);
- // ISHARP_NOISEDET_PWL_END_IN
- REG_SET(ISHARP_NOISE_GAIN_PWL, 0,
- ISHARP_NOISEDET_PWL_END_IN,
scl_data->dscl_prog_data.isharp_noise_det.pwl_end_in);
- // ISHARP_NOISEDET_PWL_SLOPE
- REG_SET(ISHARP_NOISE_GAIN_PWL, 0,
- ISHARP_NOISEDET_PWL_SLOPE,
scl_data->dscl_prog_data.isharp_noise_det.pwl_slope);
- // ISHARP_LBA_MODE
- REG_SET(ISHARP_MODE, 0,
- ISHARP_LBA_MODE,
scl_data->dscl_prog_data.isharp_lba.mode);
- // TODO: ISHARP_LBA: IN_SEG, BASE_SEG, SLOPE_SEG
- // ISHARP_FMT_MODE
- REG_SET(ISHARP_MODE, 0,
- ISHARP_FMT_MODE,
scl_data->dscl_prog_data.isharp_fmt.mode);
- // ISHARP_FMT_NORM
- REG_SET(ISHARP_MODE, 0,
- ISHARP_FMT_NORM,
scl_data->dscl_prog_data.isharp_fmt.norm);
- // ISHARP_DELTA_LUT
- dpp401_dscl_set_isharp_filter(dpp,
scl_data->dscl_prog_data.isharp_delta);
- // ISHARP_NLDELTA_SCLIP_EN_P
- REG_SET(ISHARP_NLDELTA_SOFT_CLIP, 0,
- ISHARP_NLDELTA_SCLIP_EN_P,
scl_data->dscl_prog_data.isharp_nldelta_sclip.enable_p);
- // ISHARP_NLDELTA_SCLIP_PIVOT_P
- REG_SET(ISHARP_NLDELTA_SOFT_CLIP, 0,
- ISHARP_NLDELTA_SCLIP_PIVOT_P,
scl_data->dscl_prog_data.isharp_nldelta_sclip.pivot_p);
- // ISHARP_NLDELTA_SCLIP_SLOPE_P
- REG_SET(ISHARP_NLDELTA_SOFT_CLIP, 0,
- ISHARP_NLDELTA_SCLIP_SLOPE_P,
scl_data->dscl_prog_data.isharp_nldelta_sclip.slope_p);
- // ISHARP_NLDELTA_SCLIP_EN_N
- REG_SET(I
[PATCH v2] drm/amd/display: re-indent dc_power_down_on_boot()
These lines are indented too far. Clean the whitespace.
Signed-off-by: Dan Carpenter
---
v2: Delete another blank line (checkpatch.pl --strict).
drivers/gpu/drm/amd/display/dc/core/dc.c | 8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c
b/drivers/gpu/drm/amd/display/dc/core/dc.c
index 3e16041bf4f9..5a0835f884a8 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
@@ -5192,11 +5192,9 @@ void dc_interrupt_ack(struct dc *dc, enum dc_irq_source
src)
void dc_power_down_on_boot(struct dc *dc)
{
if (dc->ctx->dce_environment != DCE_ENV_VIRTUAL_HW &&
- dc->hwss.power_down_on_boot) {
-
- if (dc->caps.ips_support)
- dc_exit_ips_for_hw_access(dc);
-
+ dc->hwss.power_down_on_boot) {
+ if (dc->caps.ips_support)
+ dc_exit_ips_for_hw_access(dc);
dc->hwss.power_down_on_boot(dc);
}
}
--
2.43.0
Re: [PATCH] drm/amd/display: re-indent dc_power_down_on_boot()
On Wed, Apr 24, 2024 at 03:33:11PM +0200, Christian König wrote:
> Am 24.04.24 um 15:20 schrieb Dan Carpenter:
> > On Wed, Apr 24, 2024 at 03:11:08PM +0200, Christian König wrote:
> > > Am 24.04.24 um 13:41 schrieb Dan Carpenter:
> > > > These lines are indented too far. Clean the whitespace.
> > > >
> > > > Signed-off-by: Dan Carpenter
> > > > ---
> > > >drivers/gpu/drm/amd/display/dc/core/dc.c | 7 +++
> > > >1 file changed, 3 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c
> > > > b/drivers/gpu/drm/amd/display/dc/core/dc.c
> > > > index 8eefba757da4..f64d7229eb6c 100644
> > > > --- a/drivers/gpu/drm/amd/display/dc/core/dc.c
> > > > +++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
> > > > @@ -5043,11 +5043,10 @@ void dc_interrupt_ack(struct dc *dc, enum
> > > > dc_irq_source src)
> > > >void dc_power_down_on_boot(struct dc *dc)
> > > >{
> > > > if (dc->ctx->dce_environment != DCE_ENV_VIRTUAL_HW &&
> > > > - dc->hwss.power_down_on_boot) {
> > > > -
> > > > - if (dc->caps.ips_support)
> > > > - dc_exit_ips_for_hw_access(dc);
> > > > + dc->hwss.power_down_on_boot) {
> > > > + if (dc->caps.ips_support)
> > > > + dc_exit_ips_for_hw_access(dc);
> > > Well while at it can't the two ifs be merged here?
> > >
> > > (I don't know this code to well, but it looks like it).
> > >
> > I'm sorry, I don't see what you're saying.
>
> The indentation was so messed up that I though the call to
> power_down_on_boot() was after both ifs, but it is still inside the first.
>
> So your patch is actually right, sorry for the noise.
Okay, but let me send a v2 anyway to delete the extra blank line.
regards,
dan carpenter
Re: [PATCH] drm/amd/display: re-indent dc_power_down_on_boot()
On Wed, Apr 24, 2024 at 03:11:08PM +0200, Christian König wrote:
> Am 24.04.24 um 13:41 schrieb Dan Carpenter:
> > These lines are indented too far. Clean the whitespace.
> >
> > Signed-off-by: Dan Carpenter
> > ---
> > drivers/gpu/drm/amd/display/dc/core/dc.c | 7 +++
> > 1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c
> > b/drivers/gpu/drm/amd/display/dc/core/dc.c
> > index 8eefba757da4..f64d7229eb6c 100644
> > --- a/drivers/gpu/drm/amd/display/dc/core/dc.c
> > +++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
> > @@ -5043,11 +5043,10 @@ void dc_interrupt_ack(struct dc *dc, enum
> > dc_irq_source src)
> > void dc_power_down_on_boot(struct dc *dc)
> > {
> > if (dc->ctx->dce_environment != DCE_ENV_VIRTUAL_HW &&
> > - dc->hwss.power_down_on_boot) {
> > -
> > - if (dc->caps.ips_support)
> > - dc_exit_ips_for_hw_access(dc);
> > + dc->hwss.power_down_on_boot) {
> > + if (dc->caps.ips_support)
> > + dc_exit_ips_for_hw_access(dc);
>
> Well while at it can't the two ifs be merged here?
>
> (I don't know this code to well, but it looks like it).
>
I'm sorry, I don't see what you're saying.
I probably should have deleted the other blank line as well, though.
It introduces a checkpatch.pl --strict warning.
regards,
dan carpenter
[PATCH] drm/amd/display: re-indent dc_power_down_on_boot()
These lines are indented too far. Clean the whitespace.
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/amd/display/dc/core/dc.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c
b/drivers/gpu/drm/amd/display/dc/core/dc.c
index 8eefba757da4..f64d7229eb6c 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
@@ -5043,11 +5043,10 @@ void dc_interrupt_ack(struct dc *dc, enum dc_irq_source
src)
void dc_power_down_on_boot(struct dc *dc)
{
if (dc->ctx->dce_environment != DCE_ENV_VIRTUAL_HW &&
- dc->hwss.power_down_on_boot) {
-
- if (dc->caps.ips_support)
- dc_exit_ips_for_hw_access(dc);
+ dc->hwss.power_down_on_boot) {
+ if (dc->caps.ips_support)
+ dc_exit_ips_for_hw_access(dc);
dc->hwss.power_down_on_boot(dc);
}
}
--
2.43.0
Re: [PATCH] staging:fbtft:fb_ili9320 Removed redundant Parentheses
0) Fix your From address. 1) Look at how other people write subjects. git log --oneline drivers/staging/fbtft/fb_ili9320.c On Fri, Apr 19, 2024 at 11:04:21AM -0700, A wrote: > >From 51e98164e314a2d1d834d2a9baea21a9823650bb Mon Sep 17 00:00:00 2001 > From: Ashok Kumar > Date: Fri, 19 Apr 2024 10:32:48 -0700 > Subject: [PATCH] staging:fbtft:fb_ili9320 Removed redundant > Parentheses 2) This should not be part of the email. > > Adhere to Linux kernel coding style. > Reported by checkpatch > > CHECK: Unnecessary parentheses around 'devcode != 0x' > + if ((devcode != 0x) && (devcode != 0x9320)) > 3) Just leave this as-is. Ignore checkpatch in this case. Greg likes parens. > Signed-off-by: Ashok Kumar > --- > drivers/staging/fbtft/fb_ili9320.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/fbtft/fb_ili9320.c > b/drivers/staging/fbtft/fb_ili9320.c > index 0be7c2d51548..409b54cc562e 100644 > --- a/drivers/staging/fbtft/fb_ili9320.c > +++ b/drivers/staging/fbtft/fb_ili9320.c > @@ -37,7 +37,7 @@ static int init_display(struct fbtft_par *par) > devcode = read_devicecode(par); > fbtft_par_dbg(DEBUG_INIT_DISPLAY, par, "Device code: > 0x%04X\n", 4) This patch is corrupt and will not apply. Read the first two paragraphs of Documentation/process/email-clients.rst. regards, dan carpenter
[bug report] drm/nouveau/disp/r535: initial support
Hello Nouveau Devs,
Commit 9e999023 ("drm/nouveau/disp/r535: initial support") from
Sep 19, 2023 (linux-next), leads to the following Smatch static
checker warning:
drivers/gpu/drm/nouveau/nvkm/engine/disp/r535.c:1482 r535_disp_oneinit() error:
potential NULL/IS_ERR bug 'ctrl'
drivers/gpu/drm/nouveau/nvkm/engine/disp/r535.c:1582 r535_disp_oneinit() error:
potential NULL/IS_ERR bug 'ctrl'
drivers/gpu/drm/nouveau/nvkm/engine/disp/r535.c:1596 r535_disp_oneinit() error:
potential NULL/IS_ERR bug 'ctrl'
drivers/gpu/drm/nouveau/nvkm/engine/disp/r535.c:1625 r535_disp_oneinit() error:
potential NULL/IS_ERR bug 'ctrl'
drivers/gpu/drm/nouveau/nvkm/engine/fifo/r535.c:519 r535_fifo_ectx_size()
error: potential NULL/IS_ERR bug 'ctrl'
drivers/gpu/drm/nouveau/nvkm/engine/fifo/r535.c:556 r535_fifo_runl_ctor()
error: potential NULL/IS_ERR bug 'ctrl'
drivers/gpu/drm/nouveau/nvkm/engine/fifo/r535.c:631 r535_fifo_runl_ctor()
error: potential NULL/IS_ERR bug 'ctrl'
drivers/gpu/drm/nouveau/nvkm/engine/disp/r535.c
1432 static int
1433 r535_disp_oneinit(struct nvkm_disp *disp)
1434 {
1435 struct nvkm_device *device = disp->engine.subdev.device;
1436 struct nvkm_gsp *gsp = device->gsp;
1437 NV2080_CTRL_INTERNAL_DISPLAY_WRITE_INST_MEM_PARAMS *ctrl;
1438 int ret, i;
1439
1440 /* RAMIN. */
1441 ret = nvkm_gpuobj_new(device, 0x1, 0x1, false, NULL,
>inst);
1442 if (ret)
1443 return ret;
1444
1445 if (WARN_ON(nvkm_memory_target(disp->inst->memory) !=
NVKM_MEM_TARGET_VRAM))
1446 return -EINVAL;
1447
1448 ctrl = nvkm_gsp_rm_ctrl_get(>internal.device.subdevice,
1449
NV2080_CTRL_CMD_INTERNAL_DISPLAY_WRITE_INST_MEM,
1450 sizeof(*ctrl));
1451 if (IS_ERR(ctrl))
1452 return PTR_ERR(ctrl);
1453
1454 ctrl->instMemPhysAddr = nvkm_memory_addr(disp->inst->memory);
1455 ctrl->instMemSize = nvkm_memory_size(disp->inst->memory);
1456 ctrl->instMemAddrSpace = ADDR_FBMEM;
1457 ctrl->instMemCpuCacheAttr = NV_MEMORY_WRITECOMBINED;
1458
1459 ret = nvkm_gsp_rm_ctrl_wr(>internal.device.subdevice,
ctrl);
1460 if (ret)
1461 return ret;
1462
1463 /* OBJs. */
1464 ret = nvkm_gsp_client_device_ctor(gsp, >rm.client,
>rm.device);
1465 if (ret)
1466 return ret;
1467
1468 ret = nvkm_gsp_rm_alloc(>rm.device.object, 0x0073,
NV04_DISPLAY_COMMON, 0,
1469 >rm.objcom);
1470 if (ret)
1471 return ret;
1472
1473 {
1474 NV2080_CTRL_INTERNAL_DISPLAY_GET_STATIC_INFO_PARAMS
*ctrl;
1475
1476 ctrl =
nvkm_gsp_rm_ctrl_rd(>internal.device.subdevice,
1477
NV2080_CTRL_CMD_INTERNAL_DISPLAY_GET_STATIC_INFO,
1478sizeof(*ctrl));
1479 if (IS_ERR(ctrl))
The problem here is that r535_gsp_rpc_push() returns a mix of error
pointers and NULL. I've written a blog about how mixing error pointers
and NULL normally works and I think there is some kind of similar logic
here but I forget what it is...
https://staticthinking.wordpress.com/2022/08/01/mixing-error-pointers-and-null/
1480 return PTR_ERR(ctrl);
1481
--> 1482 disp->wndw.mask = ctrl->windowPresentMask;
^^
Potential NULL dereference.
1483 disp->wndw.nr = fls(disp->wndw.mask);
1484 nvkm_gsp_rm_ctrl_done(>internal.device.subdevice,
ctrl);
1485 }
1486
1487 /* */
1488 {
1489 #if defined(CONFIG_ACPI) && defined(CONFIG_X86)
1490 NV2080_CTRL_INTERNAL_INIT_BRIGHTC_STATE_LOAD_PARAMS
*ctrl;
1491 struct nvkm_gsp_object *subdevice =
>rm.client.gsp->internal.device.subdevice;
1492
1493 ctrl = nvkm_gsp_rm_ctrl_get(subdevice,
1494
NV2080_CTRL_CMD_INTERNAL_INIT_BRIGHTC_STATE_LOAD,
1495 sizeof(*ctrl));
1496 if (IS_ERR(ctrl))
regards,
dan carpenter
Re: [PATCH] treewide: Fix common grammar mistake "the the"
On Thu, Apr 11, 2024 at 05:04:40PM +0200, Thorsten Blum wrote:
> Use `find . -type f -exec sed -i 's/\/the/g' {} +` to find all
> occurrences of "the the" and replace them with a single "the".
>
> Changes only comments and documentation - no code changes.
>
> Signed-off-by: Thorsten Blum
> ---
It's tricky to know which tree a patch like this would go through. We
used to have a trivial tree for this stuff but I guess that didn't work.
It's possible that it could go through linux-doc, but probably it has to
go as a set of patches through each of the trees in the CC list.
regards,
dan carpenter
[bug report] drm: bridge: cdns-mhdp8546: Fix possible null pointer dereference
Hello Aleksandr Mishin,
Commit 935a92a1c400 ("drm: bridge: cdns-mhdp8546: Fix possible null
pointer dereference") from Apr 8, 2024 (linux-next), leads to the
following Smatch static checker warning:
drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c:2074
cdns_mhdp_atomic_enable()
warn: inconsistent returns '>link_mutex'.
drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
1986 static void cdns_mhdp_atomic_enable(struct drm_bridge *bridge,
1987 struct drm_bridge_state
*bridge_state)
1988 {
1989 struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
1990 struct drm_atomic_state *state = bridge_state->base.state;
1991 struct cdns_mhdp_bridge_state *mhdp_state;
1992 struct drm_crtc_state *crtc_state;
1993 struct drm_connector *connector;
1994 struct drm_connector_state *conn_state;
1995 struct drm_bridge_state *new_state;
1996 const struct drm_display_mode *mode;
1997 u32 resp;
1998 int ret;
1999
2000 dev_dbg(mhdp->dev, "bridge enable\n");
2001
2002 mutex_lock(>link_mutex);
^^
Holding a lock
2003
2004 if (mhdp->plugged && !mhdp->link_up) {
2005 ret = cdns_mhdp_link_up(mhdp);
2006 if (ret < 0)
2007 goto out;
2008 }
2009
2010 if (mhdp->info && mhdp->info->ops && mhdp->info->ops->enable)
2011 mhdp->info->ops->enable(mhdp);
2012
2013 /* Enable VIF clock for stream 0 */
2014 ret = cdns_mhdp_reg_read(mhdp, CDNS_DPTX_CAR, );
2015 if (ret < 0) {
2016 dev_err(mhdp->dev, "Failed to read CDNS_DPTX_CAR
%d\n", ret);
2017 goto out;
2018 }
2019
2020 cdns_mhdp_reg_write(mhdp, CDNS_DPTX_CAR,
2021 resp | CDNS_VIF_CLK_EN |
CDNS_VIF_CLK_RSTN);
2022
2023 connector = drm_atomic_get_new_connector_for_encoder(state,
2024
bridge->encoder);
2025 if (WARN_ON(!connector))
2026 goto out;
2027
2028 conn_state = drm_atomic_get_new_connector_state(state,
connector);
2029 if (WARN_ON(!conn_state))
2030 goto out;
2031
2032 if (mhdp->hdcp_supported &&
2033 mhdp->hw_state == MHDP_HW_READY &&
2034 conn_state->content_protection ==
2035 DRM_MODE_CONTENT_PROTECTION_DESIRED) {
2036 mutex_unlock(>link_mutex);
2037 cdns_mhdp_hdcp_enable(mhdp,
conn_state->hdcp_content_type);
2038 mutex_lock(>link_mutex);
2039 }
2040
2041 crtc_state = drm_atomic_get_new_crtc_state(state,
conn_state->crtc);
2042 if (WARN_ON(!crtc_state))
2043 goto out;
2044
2045 mode = _state->adjusted_mode;
2046
2047 new_state = drm_atomic_get_new_bridge_state(state, bridge);
2048 if (WARN_ON(!new_state))
2049 goto out;
2050
2051 if (!cdns_mhdp_bandwidth_ok(mhdp, mode, mhdp->link.num_lanes,
2052 mhdp->link.rate)) {
2053 ret = -EINVAL;
2054 goto out;
2055 }
2056
2057 cdns_mhdp_sst_enable(mhdp, mode);
2058
2059 mhdp_state = to_cdns_mhdp_bridge_state(new_state);
2060
2061 mhdp_state->current_mode = drm_mode_duplicate(bridge->dev,
mode);
2062 if (!mhdp_state->current_mode)
2063 return;
^^^
Needs to unlock before returning.
2064
2065 drm_mode_set_name(mhdp_state->current_mode);
2066
2067 dev_dbg(mhdp->dev, "%s: Enabling mode %s\n", __func__,
mode->name);
2068
2069 mhdp->bridge_enabled = true;
2070
2071 out:
2072 mutex_unlock(>link_mutex);
2073 if (ret < 0)
--> 2074 schedule_work(>modeset_retry_work);
2075 }
regards,
dan carpenter
Re: [bug report] drm/panthor: Add the scheduler logical block
On Wed, Apr 10, 2024 at 03:11:52PM +0100, Steven Price wrote:
> On 08/04/2024 08:35, Dan Carpenter wrote:
> > Hello Boris Brezillon,
> >
> > Commit de8548813824 ("drm/panthor: Add the scheduler logical block")
> > from Feb 29, 2024 (linux-next), leads to the following Smatch static
> > checker warning:
> >
> > drivers/gpu/drm/panthor/panthor_sched.c:1153
> > csg_slot_sync_state_locked()
> > error: uninitialized symbol 'new_state'.
> >
> > drivers/gpu/drm/panthor/panthor_sched.c
> > 1123 static void
> > 1124 csg_slot_sync_state_locked(struct panthor_device *ptdev, u32
> > csg_id)
> > 1125 {
> > 1126 struct panthor_csg_slot *csg_slot =
> > >scheduler->csg_slots[csg_id];
> > 1127 struct panthor_fw_csg_iface *csg_iface;
> > 1128 struct panthor_group *group;
> > 1129 enum panthor_group_state new_state, old_state;
> > 1130
> > 1131 lockdep_assert_held(>scheduler->lock);
> > 1132
> > 1133 csg_iface = panthor_fw_get_csg_iface(ptdev, csg_id);
> > 1134 group = csg_slot->group;
> > 1135
> > 1136 if (!group)
> > 1137 return;
> > 1138
> > 1139 old_state = group->state;
> > 1140 switch (csg_iface->output->ack & CSG_STATE_MASK) {
> > ^^
> > This mask is 0x7. Should it be 0x3? That would silence the static
> > checker warning.
>
> The mask is correct - it's effectively a hardware register (well it's
> read/written by the firmware on the hardware). States 4-7 are officially
> "reserved" and Should Not Happen™!
>
> I guess a "default:" case with a WARN_ON() would be the right solution.
>
> Steve
A WARN_ON() won't silence the warning. Plus WARN_ON() is not free in
terms of memory usage. And they're kind of controversial these days to
be honest.
One solution would be to just ignore the static checker warning. These
are a one time thing, and if people have questions in the future, they
can just search lore for this thread.
regards,
dan carpenter
[PATCH] drm/panthor: clean up some types in panthor_sched_suspend()
These variables should be u32 instead of u64 because they're only
storing u32 values. Also static checkers complain when we do:
suspended_slots &= ~upd_ctx.timedout_mask;
In this code "suspended_slots" is a u64 and "upd_ctx.timedout_mask". The
mask clears out the top 32 bits which would likely be a bug if anything
were stored there.
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/panthor/panthor_sched.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/panthor/panthor_sched.c
b/drivers/gpu/drm/panthor/panthor_sched.c
index d4bc652b34d5..b3a51a6de523 100644
--- a/drivers/gpu/drm/panthor/panthor_sched.c
+++ b/drivers/gpu/drm/panthor/panthor_sched.c
@@ -2546,7 +2546,7 @@ void panthor_sched_suspend(struct panthor_device *ptdev)
{
struct panthor_scheduler *sched = ptdev->scheduler;
struct panthor_csg_slots_upd_ctx upd_ctx;
- u64 suspended_slots, faulty_slots;
+ u32 suspended_slots, faulty_slots;
struct panthor_group *group;
u32 i;
--
2.43.0
[bug report] drm/panthor: Add the scheduler logical block
Hello Boris Brezillon,
Commit de8548813824 ("drm/panthor: Add the scheduler logical block")
from Feb 29, 2024 (linux-next), leads to the following Smatch static
checker warning:
drivers/gpu/drm/panthor/panthor_sched.c:1153
csg_slot_sync_state_locked()
error: uninitialized symbol 'new_state'.
drivers/gpu/drm/panthor/panthor_sched.c
1123 static void
1124 csg_slot_sync_state_locked(struct panthor_device *ptdev, u32 csg_id)
1125 {
1126 struct panthor_csg_slot *csg_slot =
>scheduler->csg_slots[csg_id];
1127 struct panthor_fw_csg_iface *csg_iface;
1128 struct panthor_group *group;
1129 enum panthor_group_state new_state, old_state;
1130
1131 lockdep_assert_held(>scheduler->lock);
1132
1133 csg_iface = panthor_fw_get_csg_iface(ptdev, csg_id);
1134 group = csg_slot->group;
1135
1136 if (!group)
1137 return;
1138
1139 old_state = group->state;
1140 switch (csg_iface->output->ack & CSG_STATE_MASK) {
^^
This mask is 0x7. Should it be 0x3? That would silence the static
checker warning.
1141 case CSG_STATE_START:
1142 case CSG_STATE_RESUME:
1143 new_state = PANTHOR_CS_GROUP_ACTIVE;
1144 break;
1145 case CSG_STATE_TERMINATE:
1146 new_state = PANTHOR_CS_GROUP_TERMINATED;
1147 break;
1148 case CSG_STATE_SUSPEND:
1149 new_state = PANTHOR_CS_GROUP_SUSPENDED;
1150 break;
1151 }
1152
--> 1153 if (old_state == new_state)
1154 return;
1155
1156 if (new_state == PANTHOR_CS_GROUP_SUSPENDED)
1157 csg_slot_sync_queues_state_locked(ptdev, csg_id);
1158
1159 if (old_state == PANTHOR_CS_GROUP_ACTIVE) {
1160 u32 i;
1161
1162 /* Reset the queue slots so we start from a clean
1163 * state when starting/resuming a new group on this
1164 * CSG slot. No wait needed here, and no ringbell
1165 * either, since the CS slot will only be re-used
1166 * on the next CSG start operation.
1167 */
1168 for (i = 0; i < group->queue_count; i++) {
1169 if (group->queues[i])
1170 cs_slot_reset_locked(ptdev, csg_id, i);
1171 }
1172 }
1173
1174 group->state = new_state;
1175 }
regards,
dan carpenter
Re: [PATCH 7/7] drm/udl: Remove struct udl_connector
Hi Thomas, 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/Thomas-Zimmermann/drm-edid-Implement-drm_probe_ddc-with-drm_edid_probe_custom/20240404-231057 base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next patch link: https://lore.kernel.org/r/20240404150857.5520-8-tzimmermann%40suse.de patch subject: [PATCH 7/7] drm/udl: Remove struct udl_connector config: parisc-randconfig-r071-20240405 (https://download.01.org/0day-ci/archive/20240405/202404051359.y6aguwfi-...@intel.com/config) compiler: hppa-linux-gcc (GCC) 13.2.0 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/202404051359.y6aguwfi-...@intel.com/ smatch warnings: drivers/gpu/drm/udl/udl_modeset.c:527 udl_modeset_init() warn: passing a valid pointer to 'PTR_ERR' vim +/PTR_ERR +527 drivers/gpu/drm/udl/udl_modeset.c 72d73dd3a95c7e8 Thomas Zimmermann 2022-10-06 518 encoder = >encoder; 72d73dd3a95c7e8 Thomas Zimmermann 2022-10-06 519 ret = drm_encoder_init(dev, encoder, _encoder_funcs, DRM_MODE_ENCODER_DAC, NULL); 72d73dd3a95c7e8 Thomas Zimmermann 2022-10-06 520 if (ret) 72d73dd3a95c7e8 Thomas Zimmermann 2022-10-06 521 return ret; 72d73dd3a95c7e8 Thomas Zimmermann 2022-10-06 522 encoder->possible_crtcs = drm_crtc_mask(crtc); 5320918b9a87865 Dave Airlie 2010-12-15 523 a80d9e00c8195dc Thomas Zimmermann 2024-04-04 524 connector = >connector; a80d9e00c8195dc Thomas Zimmermann 2024-04-04 525 ret = drm_connector_init(dev, connector, _connector_funcs, DRM_MODE_CONNECTOR_VGA); a80d9e00c8195dc Thomas Zimmermann 2024-04-04 526 if (ret) fe5b7c86d6068ac Daniel Vetter 2020-03-23 @527 return PTR_ERR(connector); ^^^ This needs to be updated to "return ret;" -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
[PATCH] drm: xlnx: db: fix a memory leak in probe
Free "dp" before returning.
Fixes: be318d01a903 ("drm: xlnx: dp: Reset DisplayPort IP")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/xlnx/zynqmp_dp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/xlnx/zynqmp_dp.c b/drivers/gpu/drm/xlnx/zynqmp_dp.c
index 5a40aa1d4283..8a15d18a65a6 100644
--- a/drivers/gpu/drm/xlnx/zynqmp_dp.c
+++ b/drivers/gpu/drm/xlnx/zynqmp_dp.c
@@ -1716,7 +1716,7 @@ int zynqmp_dp_probe(struct zynqmp_dpsub *dpsub)
ret = zynqmp_dp_reset(dp, true);
if (ret < 0)
- return ret;
+ goto err_free;
ret = zynqmp_dp_reset(dp, false);
if (ret < 0)
--
2.43.0
Re: [PATCH v5 02/10] PCI: Deprecate iomap-table functions
Hi Philipp,
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/Philipp-Stanner/PCI-Add-new-set-of-devres-functions/20240403-160932
base: https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link:
https://lore.kernel.org/r/20240403080712.13986-5-pstanner%40redhat.com
patch subject: [PATCH v5 02/10] PCI: Deprecate iomap-table functions
config: i386-randconfig-141-20240404
(https://download.01.org/0day-ci/archive/20240404/202404040920.qixhnemu-...@intel.com/config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
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/202404040920.qixhnemu-...@intel.com/
smatch warnings:
drivers/pci/devres.c:897 pcim_iomap_regions_request_all() error: we previously
assumed 'legacy_iomap_table' could be null (see line 894)
vim +/legacy_iomap_table +897 drivers/pci/devres.c
acc2364fe66106 Philipp Stanner 2024-01-31 865 int
pcim_iomap_regions_request_all(struct pci_dev *pdev, int mask,
acc2364fe66106 Philipp Stanner 2024-01-31 866
const char *name)
acc2364fe66106 Philipp Stanner 2024-01-31 867 {
34e90b966504f3 Philipp Stanner 2024-04-03 868 short bar;
34e90b966504f3 Philipp Stanner 2024-04-03 869 int ret;
34e90b966504f3 Philipp Stanner 2024-04-03 870 void __iomem
**legacy_iomap_table;
acc2364fe66106 Philipp Stanner 2024-01-31 871
34e90b966504f3 Philipp Stanner 2024-04-03 872 ret =
pcim_request_all_regions(pdev, name);
34e90b966504f3 Philipp Stanner 2024-04-03 873 if (ret != 0)
34e90b966504f3 Philipp Stanner 2024-04-03 874 return ret;
acc2364fe66106 Philipp Stanner 2024-01-31 875
34e90b966504f3 Philipp Stanner 2024-04-03 876 for (bar = 0; bar <
PCI_STD_NUM_BARS; bar++) {
34e90b966504f3 Philipp Stanner 2024-04-03 877 if
(!mask_contains_bar(mask, bar))
34e90b966504f3 Philipp Stanner 2024-04-03 878
continue;
34e90b966504f3 Philipp Stanner 2024-04-03 879 if
(!pcim_iomap(pdev, bar, 0))
34e90b966504f3 Philipp Stanner 2024-04-03 880 goto
err;
34e90b966504f3 Philipp Stanner 2024-04-03 881 }
34e90b966504f3 Philipp Stanner 2024-04-03 882
34e90b966504f3 Philipp Stanner 2024-04-03 883 return 0;
34e90b966504f3 Philipp Stanner 2024-04-03 884
34e90b966504f3 Philipp Stanner 2024-04-03 885 err:
34e90b966504f3 Philipp Stanner 2024-04-03 886 /*
34e90b966504f3 Philipp Stanner 2024-04-03 887 * Here it gets tricky:
pcim_iomap() above has most likely
34e90b966504f3 Philipp Stanner 2024-04-03 888 * failed because it
got an OOM when trying to create the
34e90b966504f3 Philipp Stanner 2024-04-03 889 * legacy-table.
34e90b966504f3 Philipp Stanner 2024-04-03 890 * We check here if
that has happened. If not, pcim_iomap()
34e90b966504f3 Philipp Stanner 2024-04-03 891 * must have failed
because of EINVAL.
34e90b966504f3 Philipp Stanner 2024-04-03 892 */
34e90b966504f3 Philipp Stanner 2024-04-03 893 legacy_iomap_table =
(void __iomem **)pcim_iomap_table(pdev);
34e90b966504f3 Philipp Stanner 2024-04-03 @894 ret =
legacy_iomap_table ? -EINVAL : -ENOMEM;
^^
Check for NULL
34e90b966504f3 Philipp Stanner 2024-04-03 895
34e90b966504f3 Philipp Stanner 2024-04-03 896 while (--bar >= 0)
34e90b966504f3 Philipp Stanner 2024-04-03 @897
pcim_iounmap(pdev, legacy_iomap_table[bar]);
^^
Unchecked dereference
34e90b966504f3 Philipp Stanner 2024-04-03 898
34e90b966504f3 Philipp Stanner 2024-04-03 899
pcim_release_all_regions(pdev);
34e90b966504f3 Philipp Stanner 2024-04-03 900
34e90b966504f3 Philipp Stanner 2024-04-03 901 return ret;
acc2364fe66106 Philipp Stanner 2024-01-31 902 }
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [PATCH v3] drm/panthor: Fix couple of NULL vs IS_ERR() bugs
On Tue, Apr 02, 2024 at 05:19:25PM +0200, Boris Brezillon wrote:
> On Tue, 2 Apr 2024 17:44:18 +0300
> Dan Carpenter wrote:
>
> > On Tue, Apr 02, 2024 at 04:38:38PM +0200, Boris Brezillon wrote:
> > > On Tue, 2 Apr 2024 07:14:11 -0700
> > > Harshit Mogalapalli wrote:
> > >
> > > > Currently panthor_vm_get_heap_pool() returns both ERR_PTR() and
> > > > NULL(when create is false and if there is no poool attached to the
> > >
> > >^ pool
> > >
> > > > VM)
> > > > - Change the function to return error pointers, when pool is
> > > > NULL return -ENOENT
> > > > - Also handle the callers to check for IS_ERR() on failure.
> > > >
> > > > Fixes: 4bdca1150792 ("drm/panthor: Add the driver frontend block")
> > >
> > > I would explain that the code was correct, but the documentation didn't
> > > match the function behavior, otherwise it feels a bit weird to have a
> > > Fixes tag here.
> >
> > The code wasn't correct, it returned a mix of error pointers and NULL.
>
> AFAICT, this is allowed, otherwise why would we have IS_ERR_OR_NULL().
Yep. I have written a blog about this:
https://staticthinking.wordpress.com/2022/08/01/mixing-error-pointers-and-null/
> The fact smatch can't see through panthor_vm_get_heap_pool() and detect
> that the return value is different for create=false/true doesn't mean
> the code was wrong. I'm certainly not saying this is a good thing to
> have a function that encodes the error case with two different kind of
> return value, but I wouldn't qualify it as a bug either. What's
> incorrect though, is the fact the documentation doesn't match the code.
>
> > So it needs a Fixes tag.
>
> I didn't say we should drop the Fixes tag, but the bug being fixed here
> is a mismatch between the doc and the implementation, the code itself
> was correct, and the behavior is actually unchanged with this patch
> applied, it's just done in a less confusing way.
Oh. Sorry, I haven't been following this thread closely and I misread
the code as well. You're right that the code works. In this case, I
would say actually that it does not need a Fixes tag because it's not
a bug. It's just a cleanup.
Sorry for the noise.
regards,
dan carpenter
Re: [PATCH v3] drm/panthor: Fix couple of NULL vs IS_ERR() bugs
On Tue, Apr 02, 2024 at 04:38:38PM +0200, Boris Brezillon wrote:
> On Tue, 2 Apr 2024 07:14:11 -0700
> Harshit Mogalapalli wrote:
>
> > Currently panthor_vm_get_heap_pool() returns both ERR_PTR() and
> > NULL(when create is false and if there is no poool attached to the
>
>^ pool
>
> > VM)
> > - Change the function to return error pointers, when pool is
> > NULL return -ENOENT
> > - Also handle the callers to check for IS_ERR() on failure.
> >
> > Fixes: 4bdca1150792 ("drm/panthor: Add the driver frontend block")
>
> I would explain that the code was correct, but the documentation didn't
> match the function behavior, otherwise it feels a bit weird to have a
> Fixes tag here.
The code wasn't correct, it returned a mix of error pointers and NULL.
So it needs a Fixes tag.
regards,
dan carpenter
[PATCH] drm/panthor: Fix a couple -ENOMEM error codes
These error paths forgot to set the error code to -ENOMEM.
Fixes: 647810ec2476 ("drm/panthor: Add the MMU/VM logical block")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/panthor/panthor_mmu.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c
b/drivers/gpu/drm/panthor/panthor_mmu.c
index fdd35249169f..a26b40aab261 100644
--- a/drivers/gpu/drm/panthor/panthor_mmu.c
+++ b/drivers/gpu/drm/panthor/panthor_mmu.c
@@ -1264,8 +1264,10 @@ static int panthor_vm_prepare_map_op_ctx(struct
panthor_vm_op_ctx *op_ctx,
op_ctx->rsvd_page_tables.pages = kcalloc(pt_count,
sizeof(*op_ctx->rsvd_page_tables.pages),
GFP_KERNEL);
- if (!op_ctx->rsvd_page_tables.pages)
+ if (!op_ctx->rsvd_page_tables.pages) {
+ ret = -ENOMEM;
goto err_cleanup;
+ }
ret = kmem_cache_alloc_bulk(pt_cache, GFP_KERNEL, pt_count,
op_ctx->rsvd_page_tables.pages);
@@ -1318,8 +1320,10 @@ static int panthor_vm_prepare_unmap_op_ctx(struct
panthor_vm_op_ctx *op_ctx,
op_ctx->rsvd_page_tables.pages = kcalloc(pt_count,
sizeof(*op_ctx->rsvd_page_tables.pages),
GFP_KERNEL);
- if (!op_ctx->rsvd_page_tables.pages)
+ if (!op_ctx->rsvd_page_tables.pages) {
+ ret = -ENOMEM;
goto err_cleanup;
+ }
ret = kmem_cache_alloc_bulk(pt_cache, GFP_KERNEL, pt_count,
op_ctx->rsvd_page_tables.pages);
--
2.43.0
[PATCH] drm/panthor: Fix off by one in panthor_fw_get_cs_iface()
The ->iface.streams[csg_slot][] array has MAX_CS_PER_CSG elements so
this > comparison needs to be >= to prevent an out of bounds access.
Fixes: 2718d91816ee ("drm/panthor: Add the FW logical block")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/panthor/panthor_fw.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/panthor/panthor_fw.c
b/drivers/gpu/drm/panthor/panthor_fw.c
index 33c87a59834e..181395e2859a 100644
--- a/drivers/gpu/drm/panthor/panthor_fw.c
+++ b/drivers/gpu/drm/panthor/panthor_fw.c
@@ -308,7 +308,7 @@ panthor_fw_get_csg_iface(struct panthor_device *ptdev, u32
csg_slot)
struct panthor_fw_cs_iface *
panthor_fw_get_cs_iface(struct panthor_device *ptdev, u32 csg_slot, u32
cs_slot)
{
- if (drm_WARN_ON(>base, csg_slot >= MAX_CSGS || cs_slot >
MAX_CS_PER_CSG))
+ if (drm_WARN_ON(>base, csg_slot >= MAX_CSGS || cs_slot >=
MAX_CS_PER_CSG))
return NULL;
return >fw->iface.streams[csg_slot][cs_slot];
--
2.43.0
[PATCH] drm/panthor: Fix error code in panthor_gpu_init()
This code accidentally returns zero/success on error because of a typo.
It should be "irq" instead of "ret". The other thing is that if
platform_get_irq_byname() were to return zero then the error code would
be cmplicated. Fortunately, it does not so we can just change <= to
< 0.
Fixes: 5cd894e258c4 ("drm/panthor: Add the GPU logical block")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/panthor/panthor_gpu.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/panthor/panthor_gpu.c
b/drivers/gpu/drm/panthor/panthor_gpu.c
index 0f7c962440d3..5251d8764e7d 100644
--- a/drivers/gpu/drm/panthor/panthor_gpu.c
+++ b/drivers/gpu/drm/panthor/panthor_gpu.c
@@ -211,8 +211,8 @@ int panthor_gpu_init(struct panthor_device *ptdev)
return ret;
irq = platform_get_irq_byname(to_platform_device(ptdev->base.dev),
"gpu");
- if (irq <= 0)
- return ret;
+ if (irq < 0)
+ return irq;
ret = panthor_request_gpu_irq(ptdev, >gpu->irq, irq,
GPU_INTERRUPTS_MASK);
if (ret)
--
2.43.0
Re: [PATCH 4/8] media: platform: ti: use for_each_endpoint_of_node()
On Mon, Mar 25, 2024 at 03:05:27AM +, Kuninori Morimoto wrote:
> We already have for_each_endpoint_of_node(), don't use
> of_graph_get_next_endpoint() directly. Replace it.
>
> Signed-off-by: Kuninori Morimoto
> ---
> drivers/media/platform/ti/am437x/am437x-vpfe.c | 8 +++-
> drivers/media/platform/ti/davinci/vpif_capture.c | 11 +--
> 2 files changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/media/platform/ti/am437x/am437x-vpfe.c
> b/drivers/media/platform/ti/am437x/am437x-vpfe.c
> index 77e12457d149..4f185a0d42b3 100644
> --- a/drivers/media/platform/ti/am437x/am437x-vpfe.c
> +++ b/drivers/media/platform/ti/am437x/am437x-vpfe.c
> @@ -2306,14 +2306,10 @@ vpfe_get_pdata(struct vpfe_device *vpfe)
> if (!pdata)
> return NULL;
>
> - for (i = 0; ; i++) {
> + for_each_endpoint_of_node(dev->of_node, endpoint) {
> struct v4l2_fwnode_endpoint bus_cfg = { .bus_type = 0 };
> struct device_node *rem;
>
> - endpoint = of_graph_get_next_endpoint(dev->of_node, endpoint);
> - if (!endpoint)
> - break;
> -
> sdinfo = >sub_devs[i];
^
"i" is uninitialized now.
Also in the initializer it has "struct device_node *endpoint = NULL;"
which is unnecessary now. And at the end it has:
of_node_put(endpoint);
return pdata;
Since endpoint is NULL this was always a pointless no-op but now it's
more obvious, so lets delete that.
regards,
dan carpenter
Re: [PATCH v5 08/11] devm-helpers: Add resource managed version of debugfs directory create function
On Sat, Mar 23, 2024 at 10:10:40PM +0100, Christophe JAILLET wrote:
> > static int pvt_ts_dbgfs_create(struct pvt_device *pvt, struct device *dev)
> > {
> > - pvt->dbgfs_dir = debugfs_create_dir(dev_name(dev), NULL);
> > + pvt->dbgfs_dir = devm_debugfs_create_dir(dev, dev_name(dev), NULL);
> > + if (IS_ERR(pvt->dbgfs_dir))
> > + return PTR_ERR(pvt->dbgfs_dir);
>
> Not sure if the test and error handling should be added here.
Yep. debugfs_create_dir() is not supposed to be checked here. It
breaks the driver if CONFIG_DEBUGFS is disabled. I have written a blog
about this:
https://staticthinking.wordpress.com/2023/07/24/debugfs-functions-are-not-supposed-to-be-checked/
regards,
dan carpenter
[PATCH] backlight: mp3309c: fix signedness bug in mp3309c_parse_fwnode()
The "num_levels" variable is used to store error codes from
device_property_count_u32() so it needs to be signed. This doesn't
cause an issue at runtime because devm_kcalloc() won't allocate negative
sizes. However, it's still worth fixing.
Fixes: b54c828bdba9 ("backlight: mp3309c: Make use of device properties")
Signed-off-by: Dan Carpenter
---
drivers/video/backlight/mp3309c.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/video/backlight/mp3309c.c
b/drivers/video/backlight/mp3309c.c
index c80a1481e742..4e98e60417d2 100644
--- a/drivers/video/backlight/mp3309c.c
+++ b/drivers/video/backlight/mp3309c.c
@@ -205,8 +205,9 @@ static int mp3309c_parse_fwnode(struct mp3309c_chip *chip,
struct mp3309c_platform_data *pdata)
{
int ret, i;
- unsigned int num_levels, tmp_value;
+ unsigned int tmp_value;
struct device *dev = chip->dev;
+ int num_levels;
if (!dev_fwnode(dev))
return dev_err_probe(dev, -ENODEV, "failed to get firmware
node\n");
--
2.43.0
[PATCH] drm/amd/display: delete unnecessary check in dcn35_set_long_vblank()
"timing" is "_ctx[i]->stream->timing" where ->timing is not the
first struct member of ->stream. So it's the address which points into
the middle of a struct. It can't be NULL so delete the NULL check.
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/amd/display/dc/hwss/dcn35/dcn35_hwseq.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/dc/hwss/dcn35/dcn35_hwseq.c
b/drivers/gpu/drm/amd/display/dc/hwss/dcn35/dcn35_hwseq.c
index 2e8ec58a16eb..87cfd9f1cec9 100644
--- a/drivers/gpu/drm/amd/display/dc/hwss/dcn35/dcn35_hwseq.c
+++ b/drivers/gpu/drm/amd/display/dc/hwss/dcn35/dcn35_hwseq.c
@@ -1411,10 +1411,7 @@ void dcn35_set_long_vblank(struct pipe_ctx **pipe_ctx,
if (pipe_ctx[i]->stream) {
struct dc_crtc_timing *timing =
_ctx[i]->stream->timing;
- if (timing)
- params.vertical_blank_start = timing->v_total -
timing->v_front_porch;
- else
- params.vertical_blank_start = 0;
+ params.vertical_blank_start = timing->v_total -
timing->v_front_porch;
if ((pipe_ctx[i]->stream_res.tg != NULL) &&
pipe_ctx[i]->stream_res.tg->funcs &&
pipe_ctx[i]->stream_res.tg->funcs->set_long_vtotal)
--
2.43.0
Re: [PATCH 3/6] backlight/omap1-bl: Replace FB_BLANK_ states with simple on/off
On Thu, Mar 14, 2024 at 09:16:15AM +0100, Thomas Zimmermann wrote: > Hi > > Am 13.03.24 um 19:00 schrieb Sam Ravnborg: > > Hi Thomas, > > > > On Wed, Mar 13, 2024 at 04:45:02PM +0100, Thomas Zimmermann wrote: > > > The backlight is on for fb_blank eq FB_BLANK_UNBLANK, or off for > > > any other value in fb_blank. But the field fb_blank in struct > > > backlight_properties is deprecated and should not be used any > > > longer. > > > > > > Replace the test for fb_blank in omap's backlight code with a > > > simple boolean parameter and push the test into the update_status > > > helper. Instead of reading fb_blank directly, decode the backlight > > > device's status with backlight_is_blank(). > > > > > > Signed-off-by: Thomas Zimmermann > > My biased opinion is that the approach in this patch is a little bit > > better: > > https://lore.kernel.org/lkml/20230107-sam-video-backlight-drop-fb_blank-v1-13-1bd9bafb3...@ravnborg.org/ > > > > I never came around resending this series it seems. > > Oh, that series has been around for over a year. I don't care about which > patches go in as long as they remove the dependency on . I saw > that Dan has already r-b'ed the current patchset, but if you prefer I'll > adopt yours. I hadn't seen Sam's patch. It's a little bit more daring, but it's really nice code and I trust him. regards, dan carpenter
Re: [PATCH 0/6] backlight: Remove struct backlight_properties.fb_blank
I was only going to comment on the staging bits but, heck, I reviewed the whole series. Reviewed-by: Dan Carpenter regards, dan carpenter
Re: [PATCH 1/6] auxdisplay/ht16k33: Replace use of fb_blank with backlight helper
On Wed, Mar 13, 2024 at 04:45:00PM +0100, Thomas Zimmermann wrote: > Replace the use of struct backlight_properties.fb_blank with a > call to backlight_get_brightness(). The helper implement the same > logic as the driver's function. If you end up resending there is a typo s/implement/implements/. regards, dan carpenter
Re: [PATCH 5/6] staging/fbtft: Remove reference to fb_blank
On Wed, Mar 13, 2024 at 04:45:04PM +0100, Thomas Zimmermann wrote: > The field fb_blank in struct backlight_properties is deprecated and > should not be used. Don't output its value in the driver's debug print. > > Signed-off-by: Thomas Zimmermann > --- > drivers/staging/fbtft/fb_ssd1351.c | 4 +--- > drivers/staging/fbtft/fbtft-core.c | 5 ++--- Reviewed-by: Dan Carpenter regards, dan carpenter
Re: [PATCH 1/6] auxdisplay/ht16k33: Replace use of fb_blank with backlight helper
On Wed, Mar 13, 2024 at 05:08:08PM +0100, Miguel Ojeda wrote: > > - int brightness = bl->props.brightness; > > + int brightness = backlight_get_brightness(bl); > > This can be `const` now (or even removed and have the call embedded below). > Is there an advantage to making this const? regards, dan carpenter
Re: [PATCH 00/14] Add support for suppressing warning backtraces
Thanks! Acked-by: Dan Carpenter regards, dan carpenter
[drm-intel:for-linux-next-gt 1/4] drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:2680 guc_context_policy_init_v70() warn: variable dereferenced before check 'ce' (see line 2663)
tree: git://anongit.freedesktop.org/drm-intel for-linux-next-gt
head: 7ad6a8fae597af7fae5193efc73276609337c360
commit: cec82816d0d018f178b9b7f88fe4bf80d66954e9 [1/4] drm/i915/guc: Use
context hints for GT frequency
config: i386-randconfig-141-20240309
(https://download.01.org/0day-ci/archive/20240310/202403101225.7ahejhzj-...@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
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/202403101225.7ahejhzj-...@intel.com/
New smatch warnings:
drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c:2680
guc_context_policy_init_v70() warn: variable dereferenced before check 'ce'
(see line 2663)
vim +/ce +2680 drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
774ce1510e6ccb Daniele Ceraolo Spurio 2022-07-18 2661 static int
guc_context_policy_init_v70(struct intel_context *ce, bool loop)
2584b3549f4c40 John Harrison 2022-04-12 2662 {
2584b3549f4c40 John Harrison 2022-04-12 @2663 struct
intel_engine_cs *engine = ce->engine;
^^
Unchecked dereference
3f2f20da79b208 Andi Shyti 2023-12-29 2664 struct
intel_guc *guc = gt_to_guc(engine->gt);
2584b3549f4c40 John Harrison 2022-04-12 2665 struct
context_policy policy;
2584b3549f4c40 John Harrison 2022-04-12 2666 u32
execution_quantum;
2584b3549f4c40 John Harrison 2022-04-12 2667 u32
preemption_timeout;
cec82816d0d018 Vinay Belgaumkar 2024-03-05 2668 u32
slpc_ctx_freq_req = 0;
2584b3549f4c40 John Harrison 2022-04-12 2669 unsigned long
flags;
2584b3549f4c40 John Harrison 2022-04-12 2670 int ret;
3a4bfa091c46e9 Rahul Kumar Singh 2021-07-26 2671
7935785240508c John Harrison 2021-07-26 2672 /* NB: For both
of these, zero means disabled. */
568944af44e753 John Harrison 2022-10-06 2673
GEM_BUG_ON(overflows_type(engine->props.timeslice_duration_ms * 1000,
568944af44e753 John Harrison 2022-10-06 2674
execution_quantum));
568944af44e753 John Harrison 2022-10-06 2675
GEM_BUG_ON(overflows_type(engine->props.preempt_timeout_ms * 1000,
568944af44e753 John Harrison 2022-10-06 2676
preemption_timeout));
2584b3549f4c40 John Harrison 2022-04-12 2677
execution_quantum = engine->props.timeslice_duration_ms * 1000;
2584b3549f4c40 John Harrison 2022-04-12 2678
preemption_timeout = engine->props.preempt_timeout_ms * 1000;
2584b3549f4c40 John Harrison 2022-04-12 2679
cec82816d0d018 Vinay Belgaumkar 2024-03-05 @2680 if (ce &&
(ce->flags & BIT(CONTEXT_LOW_LATENCY)))
^^
NULL check is too late.
cec82816d0d018 Vinay Belgaumkar 2024-03-05 2681
slpc_ctx_freq_req |= SLPC_CTX_FREQ_REQ_IS_COMPUTE;
cec82816d0d018 Vinay Belgaumkar 2024-03-05 2682
2584b3549f4c40 John Harrison 2022-04-12 2683
__guc_context_policy_start_klv(, ce->guc_id.id);
2584b3549f4c40 John Harrison 2022-04-12 2684
2584b3549f4c40 John Harrison 2022-04-12 2685
__guc_context_policy_add_priority(, ce->guc_state.prio);
2584b3549f4c40 John Harrison 2022-04-12 2686
__guc_context_policy_add_execution_quantum(, execution_quantum);
2584b3549f4c40 John Harrison 2022-04-12 2687
__guc_context_policy_add_preemption_timeout(, preemption_timeout);
cec82816d0d018 Vinay Belgaumkar 2024-03-05 2688
__guc_context_policy_add_slpc_ctx_freq_req(, slpc_ctx_freq_req);
2584b3549f4c40 John Harrison 2022-04-12 2689
2584b3549f4c40 John Harrison 2022-04-12 2690 if
(engine->flags & I915_ENGINE_WANT_FORCED_PREEMPTION)
2584b3549f4c40 John Harrison 2022-04-12 2691
__guc_context_policy_add_preempt_to_idle(, 1);
2584b3549f4c40 John Harrison 2022-04-12 2692
2584b3549f4c40 John Harrison 2022-04-12 2693 ret =
__guc_context_set_context_policies(guc, , loop);
2584b3549f4c40 John Harrison 2022-04-12 2694
2584b3549f4c40 John Harrison 2022-04-12 2695
spin_lock_irqsave(>guc_state.lock, flags);
6c82c75230b87d Daniele Ceraolo Spurio 2022-07-27 2696 if (ret != 0)
2584b3549f4c40 John Harrison 2022-04-12 2697
set_context_policy_required(ce);
2584b3549f4c40 John Harrison 2022-04-12 2698 else
2584b
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
Re: [PATCH] staging: fbtft: Fix "space prohibited before that close parenthesis ')'" error reported by checkpatch
On Wed, Feb 21, 2024 at 06:54:04PM +0900, sawara0...@gmail.com wrote: > From: Kyoji Ogasawara > > Since whitespace is prohibited before that close parenthesis ')' in a > conditional statements, remove it and fix checkpatch.pl error "space > prohibited before that > close parenthesis ')'". > > Signed-off-by: Kyoji Ogasawara This breaks the build. You could do a search for it. https://lore.kernel.org/all/?q=define_fbtft_write_reg regards, dan carpenter
Re: [PATCH v2] udmabuf: Fix a potential (and unlikely) access to unallocated memory
On Mon, Feb 19, 2024 at 06:59:02PM +0100, Christophe JAILLET wrote: > Le 19/02/2024 à 09:37, Dan Carpenter a écrit : > > On Sun, Feb 18, 2024 at 06:46:44PM +0100, Christophe JAILLET wrote: > > > If 'list_limit' is set to a very high value, 'lsize' computation could > > > overflow if 'head.count' is big enough. > > > > > > > The "list_limit" is set via module parameter so if you set that high > > enough to lead to an integer overflow then you kind of deserve what > > you get. > > > > This patch is nice for kernel hardening and making the code easier to > > read/audit but the real world security impact is negligible. > > Agreed. > > That is what I meant by "and unlikely". > Maybe the commit message could be more explicit if needed. > > Let me know if ok as-is or if I should try to re-word the description. No, it's fine. But in the future if there is an integer overflow then lets mention in the commit message who it affects or what the impact is. regards, dan carpenter
Re: [PATCH v2] udmabuf: Fix a potential (and unlikely) access to unallocated memory
On Sun, Feb 18, 2024 at 06:46:44PM +0100, Christophe JAILLET wrote: > If 'list_limit' is set to a very high value, 'lsize' computation could > overflow if 'head.count' is big enough. > The "list_limit" is set via module parameter so if you set that high enough to lead to an integer overflow then you kind of deserve what you get. This patch is nice for kernel hardening and making the code easier to read/audit but the real world security impact is negligible. regards, dan carpenter
Re: [PATCH 16/28] drm/i915/color: Create a transfer function color pipeline
Hi Uma,
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/Uma-Shankar/drm-color-pipeline-base-work/20240213-144544
base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:
https://lore.kernel.org/r/20240213064835.139464-17-uma.shankar%40intel.com
patch subject: [PATCH 16/28] drm/i915/color: Create a transfer function color
pipeline
config: i386-randconfig-141-20240217
(https://download.01.org/0day-ci/archive/20240218/202402180310.gmdixajx-...@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project
6009708b4367171ccdbf4b5905cb6a803753fe18)
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/202402180310.gmdixajx-...@intel.com/
New smatch warnings:
drivers/gpu/drm/i915/display/intel_color.c:3867 intel_plane_tf_pipeline_init()
error: 'colorop' dereferencing possible ERR_PTR()
vim +/colorop +3867 drivers/gpu/drm/i915/display/intel_color.c
5e1e0f87c9bcae Chaitanya Kumar Borah 2024-02-13 3852 int
intel_plane_tf_pipeline_init(struct drm_plane *plane, struct drm_prop_enum_list
*list)
5e1e0f87c9bcae Chaitanya Kumar Borah 2024-02-13 3853 {
5e1e0f87c9bcae Chaitanya Kumar Borah 2024-02-13 3854 struct intel_plane
*intel_plane = to_intel_plane(plane);
5e1e0f87c9bcae Chaitanya Kumar Borah 2024-02-13 3855 struct
intel_plane_colorop *colorop;
5e1e0f87c9bcae Chaitanya Kumar Borah 2024-02-13 3856 struct drm_device *dev
= plane->dev;
5e1e0f87c9bcae Chaitanya Kumar Borah 2024-02-13 3857 struct drm_i915_private
*i915 = to_i915(dev);
5e1e0f87c9bcae Chaitanya Kumar Borah 2024-02-13 3858 int ret;
5e1e0f87c9bcae Chaitanya Kumar Borah 2024-02-13 3859 struct drm_colorop
*prev_op;
5e1e0f87c9bcae Chaitanya Kumar Borah 2024-02-13 3860
5e1e0f87c9bcae Chaitanya Kumar Borah 2024-02-13 3861 colorop =
intel_plane_colorop_create(CB_PLANE_PRE_CSC_LUT);
No error checking
5e1e0f87c9bcae Chaitanya Kumar Borah 2024-02-13 3862
5e1e0f87c9bcae Chaitanya Kumar Borah 2024-02-13 3863 ret =
drm_colorop_init(dev, >base, plane, DRM_COLOROP_1D_LUT);
5e1e0f87c9bcae Chaitanya Kumar Borah 2024-02-13 3864 if (ret)
5e1e0f87c9bcae Chaitanya Kumar Borah 2024-02-13 3865 return ret;
5e1e0f87c9bcae Chaitanya Kumar Borah 2024-02-13 3866
5e1e0f87c9bcae Chaitanya Kumar Borah 2024-02-13 @3867 list->type =
colorop->base.base.id;
5e1e0f87c9bcae Chaitanya Kumar Borah 2024-02-13 3868 list->name =
kasprintf(GFP_KERNEL, "Color Pipeline %d", colorop->base.base.id);
5e1e0f87c9bcae Chaitanya Kumar Borah 2024-02-13 3869
5e1e0f87c9bcae Chaitanya Kumar Borah 2024-02-13 3870 /* TODO: handle
failures and clean up*/
5e1e0f87c9bcae Chaitanya Kumar Borah 2024-02-13 3871 prev_op =
>base;
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
[PATCH] drm/nouveau/mmu/r535: uninitialized variable in r535_bar_new_()
If gf100_bar_new_() fails then "bar" is not initialized.
Fixes: 5bf0257136a2 ("drm/nouveau/mmu/r535: initial support")
Signed-off-by: Dan Carpenter
---
It looks like this was intended to handle a failure from the "rm" func
but "rm" can't actually fail so it's easier to write the error handling
for the code as-is.
drivers/gpu/drm/nouveau/nvkm/subdev/bar/r535.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/bar/r535.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/bar/r535.c
index 4135690326f4..3a30bea30e36 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/bar/r535.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/bar/r535.c
@@ -168,12 +168,11 @@ r535_bar_new_(const struct nvkm_bar_func *hw, struct
nvkm_device *device,
rm->flush = r535_bar_flush;
ret = gf100_bar_new_(rm, device, type, inst, );
- *pbar = bar;
if (ret) {
- if (!bar)
- kfree(rm);
+ kfree(rm);
return ret;
}
+ *pbar = bar;
bar->flushBAR2PhysMode = ioremap(device->func->resource_addr(device,
3), PAGE_SIZE);
if (!bar->flushBAR2PhysMode)
--
2.43.0
[PATCH] drm/imx/dcss: fix resource size calculation
The resource is inclusive of the ->start and ->end addresses so this
calculation is not correct. It should be "res->end - res->start + 1".
Use the resource_size() to do the calculation.
Fixes: 90393c9b5408 ("drm/imx/dcss: request memory region")
Signed-off-by: Dan Carpenter
---
>From static analysis. Not tested.
drivers/gpu/drm/imx/dcss/dcss-dev.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/imx/dcss/dcss-dev.c
b/drivers/gpu/drm/imx/dcss/dcss-dev.c
index 597e9b7bd4bf..7fd0c4c14205 100644
--- a/drivers/gpu/drm/imx/dcss/dcss-dev.c
+++ b/drivers/gpu/drm/imx/dcss/dcss-dev.c
@@ -167,7 +167,6 @@ struct dcss_dev *dcss_dev_create(struct device *dev, bool
hdmi_output)
struct resource *res;
struct dcss_dev *dcss;
const struct dcss_type_data *devtype;
- resource_size_t res_len;
devtype = of_device_get_match_data(dev);
if (!devtype) {
@@ -181,8 +180,7 @@ struct dcss_dev *dcss_dev_create(struct device *dev, bool
hdmi_output)
return ERR_PTR(-EINVAL);
}
- res_len = res->end - res->start;
- if (!devm_request_mem_region(dev, res->start, res_len, "dcss")) {
+ if (!devm_request_mem_region(dev, res->start, resource_size(res),
"dcss")) {
dev_err(dev, "cannot request memory region\n");
return ERR_PTR(-EBUSY);
}
--
2.43.0
[PATCH] drm/amd/display: Fix && vs || typos
These ANDs should be ORs or it will lead to a NULL dereference.
Fixes: fb5a3d037082 ("drm/amd/display: Add NULL test for 'timing generator' in
'dcn21_set_pipe()'")
Fixes: 886571d217d7 ("drm/amd/display: Fix 'panel_cntl' could be null in
'dcn21_set_backlight_level()'")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/amd/display/dc/hwss/dcn21/dcn21_hwseq.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/dc/hwss/dcn21/dcn21_hwseq.c
b/drivers/gpu/drm/amd/display/dc/hwss/dcn21/dcn21_hwseq.c
index 5c7f380a84f9..7252f5f781f0 100644
--- a/drivers/gpu/drm/amd/display/dc/hwss/dcn21/dcn21_hwseq.c
+++ b/drivers/gpu/drm/amd/display/dc/hwss/dcn21/dcn21_hwseq.c
@@ -211,7 +211,7 @@ void dcn21_set_pipe(struct pipe_ctx *pipe_ctx)
struct dmcu *dmcu = pipe_ctx->stream->ctx->dc->res_pool->dmcu;
uint32_t otg_inst;
- if (!abm && !tg && !panel_cntl)
+ if (!abm || !tg || !panel_cntl)
return;
otg_inst = tg->inst;
@@ -245,7 +245,7 @@ bool dcn21_set_backlight_level(struct pipe_ctx *pipe_ctx,
struct panel_cntl *panel_cntl = pipe_ctx->stream->link->panel_cntl;
uint32_t otg_inst;
- if (!abm && !tg && !panel_cntl)
+ if (!abm || !tg || !panel_cntl)
return false;
otg_inst = tg->inst;
--
2.43.0
[bug report] drm/bridge: imx: add driver for HDMI TX Parallel Video Interface
Hello Lucas Stach,
This is a semi-automatic email about new static checker warnings.
drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c:81
imx8mp_hdmi_pvi_bridge_enable()
warn: variable dereferenced before check 'bridge_state' (see line 54)
drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c
53 {
54 struct drm_atomic_state *state = bridge_state->base.state;
^^
bridge_state is dereferenced here.
55 struct imx8mp_hdmi_pvi *pvi = to_imx8mp_hdmi_pvi(bridge);
56 struct drm_connector_state *conn_state;
57 const struct drm_display_mode *mode;
58 struct drm_crtc_state *crtc_state;
59 struct drm_connector *connector;
60 u32 bus_flags, val;
61
62 connector = drm_atomic_get_new_connector_for_encoder(state,
bridge->encoder);
63 conn_state = drm_atomic_get_new_connector_state(state,
connector);
64 crtc_state = drm_atomic_get_new_crtc_state(state,
conn_state->crtc);
65
66 if (WARN_ON(pm_runtime_resume_and_get(pvi->dev)))
67 return;
68
69 mode = _state->adjusted_mode;
70
71 val = FIELD_PREP(PVI_CTRL_MODE_MASK, PVI_CTRL_MODE_LCDIF) |
PVI_CTRL_EN;
72
73 if (mode->flags & DRM_MODE_FLAG_PVSYNC)
74 val |= PVI_CTRL_OP_VSYNC_POL | PVI_CTRL_INP_VSYNC_POL;
75
76 if (mode->flags & DRM_MODE_FLAG_PHSYNC)
77 val |= PVI_CTRL_OP_HSYNC_POL | PVI_CTRL_INP_HSYNC_POL;
78
79 if (pvi->next_bridge->timings)
80 bus_flags = pvi->next_bridge->timings->input_bus_flags;
81 else if (bridge_state)
This check for NULL is too late. Hopefully it can it be removed?
82 bus_flags = bridge_state->input_bus_cfg.flags;
83
regards,
dan carpenter
[PATCH] drm/imagination: Fix an IS_ERR vs NULL bug in pvr_context_create()
The pvr_vm_context_lookup() function returns NULL on error (not error
pointers). Update the check accordingly.
Fixes: d2d79d29bb98 ("drm/imagination: Implement context creation/destruction
ioctls")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/imagination/pvr_context.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/imagination/pvr_context.c
b/drivers/gpu/drm/imagination/pvr_context.c
index eded5e955cc0..e38d3578fc09 100644
--- a/drivers/gpu/drm/imagination/pvr_context.c
+++ b/drivers/gpu/drm/imagination/pvr_context.c
@@ -315,8 +315,8 @@ int pvr_context_create(struct pvr_file *pvr_file, struct
drm_pvr_ioctl_create_co
goto err_free_ctx;
ctx->vm_ctx = pvr_vm_context_lookup(pvr_file, args->vm_context_handle);
- if (IS_ERR(ctx->vm_ctx)) {
- err = PTR_ERR(ctx->vm_ctx);
+ if (!ctx->vm_ctx) {
+ err = -EINVAL;
goto err_free_ctx;
}
--
2.43.0
Re: [PATCH next 10/11] block: Use a boolean expression instead of max() on booleans
On Mon, Jan 29, 2024 at 09:22:40AM +, David Laight wrote: > From: Jani Nikula > > Sent: 29 January 2024 09:08 > > > > On Sun, 28 Jan 2024, David Laight wrote: > > > blk_stack_limits() contains: > > > t->zoned = max(t->zoned, b->zoned); > > > These are bool, so it is just a bitwise or. > > > > Should be a logical or, really. And || in code. > > Not really, bitwise is fine for bool (especially for 'or') > and generates better code. For | vs || the type doesn't make a difference... It makes a difference for AND. "0x1 & 0x10" vs "0x1 && 0x10". regards, dan carpenter
[PATCH] drm/i915/gvt: Fix uninitialized variable in handle_mmio()
This code prints the wrong variable in the warning message. It should
print "i" instead of "info->offset". On the first iteration "info" is
uninitialized leading to a crash and on subsequent iterations it prints
the previous offset instead of the current one.
Fixes: e0f74ed4634d ("i915/gvt: Separate the MMIO tracking table from GVT-g")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/i915/gvt/handlers.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gvt/handlers.c
b/drivers/gpu/drm/i915/gvt/handlers.c
index 90f6c1ece57d..efcb00472be2 100644
--- a/drivers/gpu/drm/i915/gvt/handlers.c
+++ b/drivers/gpu/drm/i915/gvt/handlers.c
@@ -2849,8 +2849,7 @@ static int handle_mmio(struct intel_gvt_mmio_table_iter
*iter, u32 offset,
for (i = start; i < end; i += 4) {
p = intel_gvt_find_mmio_info(gvt, i);
if (p) {
- WARN(1, "dup mmio definition offset %x\n",
- info->offset);
+ WARN(1, "dup mmio definition offset %x\n", i);
/* We return -EEXIST here to make GVT-g load fail.
* So duplicated MMIO can be found as soon as
--
2.43.0
[bug report] drm/amdkfd: Export DMABufs from KFD using GEM handles
Hello Felix Kuehling,
The patch 1819200166ce: "drm/amdkfd: Export DMABufs from KFD using
GEM handles" from Aug 24, 2023 (linux-next), leads to the following
Smatch static checker warning:
drivers/dma-buf/dma-buf.c:729 dma_buf_get()
warn: fd used after fd_install() 'fd'
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
809 static int kfd_mem_export_dmabuf(struct kgd_mem *mem)
810 {
811 if (!mem->dmabuf) {
812 struct amdgpu_device *bo_adev;
813 struct dma_buf *dmabuf;
814 int r, fd;
815
816 bo_adev = amdgpu_ttm_adev(mem->bo->tbo.bdev);
817 r = drm_gem_prime_handle_to_fd(_adev->ddev,
bo_adev->kfd.client.file,
818 mem->gem_handle,
819 mem->alloc_flags &
KFD_IOC_ALLOC_MEM_FLAGS_WRITABLE ?
820 DRM_RDWR : 0, );
^^^
The drm_gem_prime_handle_to_fd() function does an fd_install() and
returns the result as "fd".
821 if (r)
822 return r;
823 dmabuf = dma_buf_get(fd);
^^
Then we do another fget() inside dma_buf_get(). I'm not an expert,
but this looks wrong. We can't assume that the dmabuf here is the
same one from drm_gem_prime_handle_to_fd() because the user could
change it after the fd_install(). I suspect drm_gem_prime_handle_to_fd()
should pass the dmabuf back instead.
We had several CVEs similar to this such as CVE-2022-1998.
824 close_fd(fd);
825 if (WARN_ON_ONCE(IS_ERR(dmabuf)))
826 return PTR_ERR(dmabuf);
827 mem->dmabuf = dmabuf;
828 }
829
830 return 0;
831 }
regards,
dan carpenter
Re: [PATCH][next] drm/nouveau/fifo/gk104: remove redundant variable ret
Let's CC Felix on this one because he might know the answer.
All day long I spend looking at code like this:
net/core/dev.c:724 dev_fill_forward_path() info: returning a literal zero is
cleaner
net/core/dev.c:732 dev_fill_forward_path() info: returning a literal zero is
cleaner
net/core/dev.c
696 int dev_fill_forward_path(const struct net_device *dev, const u8 *daddr,
697struct net_device_path_stack *stack)
698 {
699 const struct net_device *last_dev;
700 struct net_device_path_ctx ctx = {
701 .dev= dev,
702 };
703 struct net_device_path *path;
704 int ret = 0;
705
706 memcpy(ctx.daddr, daddr, sizeof(ctx.daddr));
707 stack->num_paths = 0;
708 while (ctx.dev && ctx.dev->netdev_ops->ndo_fill_forward_path) {
709 last_dev = ctx.dev;
710 path = dev_fwd_path(stack);
711 if (!path)
712 return -1;
713
714 memset(path, 0, sizeof(struct net_device_path));
715 ret = ctx.dev->netdev_ops->ndo_fill_forward_path(,
path);
716 if (ret < 0)
This if condition might trick you into thinking that ->ndo_fill_forward_path()
can return non-zero positive numbers, but it can't. It returns zero on
success or negative error codes on failure. Smatch is doing cross
function analysis so we know this.
717 return -1;
718
719 if (WARN_ON_ONCE(last_dev == ctx.dev))
720 return -1;
721 }
722
723 if (!ctx.dev)
724 return ret;
Is this intentional or not? Who knows? If this were an obvious bug,
I could fix it right away but ambiguous stuff like this takes way more
time to deal with.
725
726 path = dev_fwd_path(stack);
727 if (!path)
728 return -1;
729 path->type = DEV_PATH_ETHERNET;
730 path->dev = ctx.dev;
731
732 return ret;
Obviously this is intentional, but if you were tricked by the checking
earlier then you might assume that ret is some positive value from the
last iteration through the loop. "return 0;" is so much clearer.
733 }
regards,
dan carpetner
Re: [PATCH][next] drm/nouveau/fifo/gk104: remove redundant variable ret
On Tue, Jan 23, 2024 at 12:04:23AM +0100, Danilo Krummrich wrote:
> On 1/16/24 13:31, Dan Carpenter wrote:
> > On Tue, Jan 16, 2024 at 11:16:09AM +, Colin Ian King wrote:
> > > The variable ret is being assigned a value but it isn't being
> > > read afterwards. The assignment is redundant and so ret can be
> > > removed.
> > >
> > > Cleans up clang scan build warning:
> > > warning: Although the value stored to 'ret' is used in the enclosing
> > > expression, the value is never actually read from 'ret'
> > > [deadcode.DeadStores]
> > >
> > > Signed-off-by: Colin Ian King
> > > ---
> > > drivers/gpu/drm/nouveau/nvif/fifo.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/nouveau/nvif/fifo.c
> > > b/drivers/gpu/drm/nouveau/nvif/fifo.c
> > > index a463289962b2..e96de14ce87e 100644
> > > --- a/drivers/gpu/drm/nouveau/nvif/fifo.c
> > > +++ b/drivers/gpu/drm/nouveau/nvif/fifo.c
> > > @@ -73,9 +73,9 @@ u64
> > > nvif_fifo_runlist(struct nvif_device *device, u64 engine)
> > > {
> > > u64 runm = 0;
> > > - int ret, i;
> > > + int i;
> > > - if ((ret = nvif_fifo_runlists(device)))
> > > + if (nvif_fifo_runlists(device))
> > > return runm;
> >
> > Could we return a literal zero here? Otherwise, I'm surprised this
> > doesn't trigger a static checker warning.
>
> Why do you think so? Conditionally, runm is used later on as well. I don't
> think the checker should complain about keeping the value single source.
>
> If you agree, want to offer your RB?
If you look at v6.7 then probably 300 patches were from static
analysis. The syzbot gets credit for 63 bugs and those bugs are more
important because those are real life bugs. But static analysis is a
huge in terms of just quantity.
One of the most common bugs that static checkers complain about is
missing error codes. It's a super common bug. Returning success
instead of failure almost always results in NULL dereference or a use
after free or some kind of crash. Fortunately, error paths seldom
affect real life users.
My published Smatch checks only complain about:
if (ret)
return ret;
if (failure)
return ret;
I have a different check that I haven't published but I wish that I
could which looks like:
if (!ret)
return ret;
Here is a bug that check found recently.
https://lore.kernel.org/all/9c81251b-bc87-4ca3-bb86-843dc85e5145@moroto.mountain/
I have a different unpublished check for every time ret is zero and we
do:
return ret;
But I only review those warnings for specific code. Perhaps, I could
make a warning for:
if (failure)
return ret;
I'm sure I tried this in the past and it had more false positives than
when we have an "if (ret) return ret;" like in the first example, but I
can't remember. I could experiment with that a bit...
To me, if "return ret;" and "return 0;" are the same, then "return 0;"
is obviously more clear and looks more intentional. When I was looking
at the code here, I had to consider the context. Especially when the
patch was dealing with the "ret" variable it seemed suspicous. But
"return 0;" is unamibuous.
I don't have a problem with this patch, it's correct. But I really do
think that "return 0;" is clearer than "return ret;"
regards,
dan carpenter
Re: [PATCH][next] drm/nouveau/fifo/gk104: remove redundant variable ret
On Tue, Jan 16, 2024 at 11:16:09AM +, Colin Ian King wrote:
> The variable ret is being assigned a value but it isn't being
> read afterwards. The assignment is redundant and so ret can be
> removed.
>
> Cleans up clang scan build warning:
> warning: Although the value stored to 'ret' is used in the enclosing
> expression, the value is never actually read from 'ret'
> [deadcode.DeadStores]
>
> Signed-off-by: Colin Ian King
> ---
> drivers/gpu/drm/nouveau/nvif/fifo.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nvif/fifo.c
> b/drivers/gpu/drm/nouveau/nvif/fifo.c
> index a463289962b2..e96de14ce87e 100644
> --- a/drivers/gpu/drm/nouveau/nvif/fifo.c
> +++ b/drivers/gpu/drm/nouveau/nvif/fifo.c
> @@ -73,9 +73,9 @@ u64
> nvif_fifo_runlist(struct nvif_device *device, u64 engine)
> {
> u64 runm = 0;
> - int ret, i;
> + int i;
>
> - if ((ret = nvif_fifo_runlists(device)))
> + if (nvif_fifo_runlists(device))
> return runm;
Could we return a literal zero here? Otherwise, I'm surprised this
doesn't trigger a static checker warning.
regards,
dan carpenter
[PATCH] drm/nouveau/disp/r535: fix error code in r535_dp_aux_xfer()
This code was shuffled around but the return wasn't updated. It should
return "ret" instead of "ctrl".
Fixes: 4ae3a20102b2 ("nouveau/gsp: don't free ctrl messages on errors")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/nouveau/nvkm/engine/disp/r535.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/disp/r535.c
b/drivers/gpu/drm/nouveau/nvkm/engine/disp/r535.c
index 6a0a4d3b8902..027867c2a8c5 100644
--- a/drivers/gpu/drm/nouveau/nvkm/engine/disp/r535.c
+++ b/drivers/gpu/drm/nouveau/nvkm/engine/disp/r535.c
@@ -1080,7 +1080,7 @@ r535_dp_aux_xfer(struct nvkm_outp *outp, u8 type, u32
addr, u8 *data, u8 *psize)
ret = nvkm_gsp_rm_ctrl_push(>rm.objcom, , sizeof(*ctrl));
if (ret) {
nvkm_gsp_rm_ctrl_done(>rm.objcom, ctrl);
- return PTR_ERR(ctrl);
+ return ret;
}
memcpy(data, ctrl->data, size);
--
2.43.0
[PATCH] drm/amdgpu: fix return value in aca_bank_hwip_is_matched()
The aca_bank_hwip_is_matched() function is type bool. This error path
return -EINVAL which is cast to true, but it should return false
instead.
Fixes: 22a4fa4709e3 ("drm/amdgpu: implement RAS ACA driver framework")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
index 6074a529caf7..1d3ae7c241e5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_aca.c
@@ -182,7 +182,7 @@ static bool aca_bank_hwip_is_matched(struct aca_bank *bank,
enum aca_hwip_type t
u64 ipid;
if (!bank || type == ACA_HWIP_TYPE_UNKNOW)
- return -EINVAL;
+ return false;
hwip = _hwid_mcatypes[type];
if (!hwip->hwid)
--
2.43.0
[bug report] backlight: hx8357: Convert to agnostic GPIO API
Hello Andy Shevchenko,
The patch 7d84a63a39b7: "backlight: hx8357: Convert to agnostic GPIO
API" from Dec 7, 2023 (linux-next), leads to the following Smatch
static checker warning:
drivers/video/backlight/hx8357.c:612 hx8357_probe()
error: potential NULL/IS_ERR bug 'lcd->im_pins'
drivers/video/backlight/hx8357.c
580 static int hx8357_probe(struct spi_device *spi)
581 {
582 struct device *dev = >dev;
583 struct lcd_device *lcdev;
584 struct hx8357_data *lcd;
585 const struct of_device_id *match;
586 int i, ret;
587
588 lcd = devm_kzalloc(>dev, sizeof(*lcd), GFP_KERNEL);
589 if (!lcd)
590 return -ENOMEM;
591
592 ret = spi_setup(spi);
593 if (ret < 0) {
594 dev_err(>dev, "SPI setup failed.\n");
595 return ret;
596 }
597
598 lcd->spi = spi;
599
600 match = of_match_device(hx8357_dt_ids, >dev);
601 if (!match || !match->data)
602 return -EINVAL;
603
604 lcd->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
605 if (IS_ERR(lcd->reset))
606 return dev_err_probe(dev, PTR_ERR(lcd->reset), "failed
to request reset GPIO\n");
607 gpiod_set_consumer_name(lcd->reset, "hx8357-reset");
608
609 lcd->im_pins = devm_gpiod_get_array_optional(dev, "im",
GPIOD_OUT_LOW);
610 if (IS_ERR(lcd->im_pins))
611 return dev_err_probe(dev, PTR_ERR(lcd->im_pins),
"failed to request im GPIOs\n");
--> 612 if (lcd->im_pins->ndescs < HX8357_NUM_IM_PINS)
I think _optional() functions can return NULL.
613 return dev_err_probe(dev, -EINVAL, "not enough im
GPIOs\n");
614
615 for (i = 0; i < HX8357_NUM_IM_PINS; i++)
616 gpiod_set_consumer_name(lcd->im_pins->desc[i],
"im_pins");
617
618 lcdev = devm_lcd_device_register(>dev, "mxsfb", >dev,
lcd,
619 _ops);
620 if (IS_ERR(lcdev)) {
621 ret = PTR_ERR(lcdev);
622 return ret;
623 }
624 spi_set_drvdata(spi, lcdev);
625
626 hx8357_lcd_reset(lcdev);
627
628 ret = ((int (*)(struct lcd_device *))match->data)(lcdev);
629 if (ret) {
630 dev_err(>dev, "Couldn't initialize panel\n");
631 return ret;
632 }
633
634 dev_info(>dev, "Panel probed\n");
635
636 return 0;
637 }
regards,
dan carpenter
[PATCH] drm/xe: clean up type of GUC_HXG_MSG_0_ORIGIN
The GUC_HXG_MSG_0_ORIGIN definition should be unsigned. Currently it is defined as INT_MIN. This doesn't cause a problem currently but it's still worth cleaning up. Signed-off-by: Dan Carpenter --- drivers/gpu/drm/xe/abi/guc_messages_abi.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/xe/abi/guc_messages_abi.h b/drivers/gpu/drm/xe/abi/guc_messages_abi.h index 3d199016cf88..c04606872e48 100644 --- a/drivers/gpu/drm/xe/abi/guc_messages_abi.h +++ b/drivers/gpu/drm/xe/abi/guc_messages_abi.h @@ -40,7 +40,7 @@ */ #define GUC_HXG_MSG_MIN_LEN1u -#define GUC_HXG_MSG_0_ORIGIN (0x1 << 31) +#define GUC_HXG_MSG_0_ORIGIN (0x1U << 31) #define GUC_HXG_ORIGIN_HOST 0u #define GUC_HXG_ORIGIN_GUC 1u #define GUC_HXG_MSG_0_TYPE (0x7 << 28) -- 2.42.0
[bug report] drm/amdgpu/vpe: enable vpe dpm
Hello Peyton Lee,
The patch 5f82a0c90cca: "drm/amdgpu/vpe: enable vpe dpm" from Dec 12,
2023 (linux-next), leads to the following Smatch static checker
warning:
drivers/gpu/drm/amd/amdgpu/amdgpu_vpe.c:62 vpe_u1_8_from_fraction() warn:
unsigned 'numerator' is never less than zero.
drivers/gpu/drm/amd/amdgpu/amdgpu_vpe.c:63 vpe_u1_8_from_fraction() warn:
unsigned 'denominator' is never less than zero.
drivers/gpu/drm/amd/amdgpu/amdgpu_vpe.c
60 static uint16_t vpe_u1_8_from_fraction(uint16_t numerator, uint16_t
denominator)
61 {
--> 62 bool arg1_negative = numerator < 0;
63 bool arg2_negative = denominator < 0;
uint16_t can't be negative.
64
65 uint16_t arg1_value = (uint16_t)(arg1_negative ? -numerator :
numerator);
66 uint16_t arg2_value = (uint16_t)(arg2_negative ? -denominator :
denominator);
67
68 uint16_t remainder;
69
regards,
dan carpenter
[PATCH] drm/xe/device: clean up on error in probe()
This error path should clean up before returning.
Smatch detected this bug:
drivers/gpu/drm/xe/xe_device.c:487 xe_device_probe() warn: missing unwind
goto?
Fixes: 4cb12b71923b ("drm/xe/xe2: Determine bios enablement for flat ccs on
igfx")
Signed-off-by: Dan Carpenter
---
Speeking of static analysis, someone should probably run Sparse on this
driver. There are some missing annotations.
drivers/gpu/drm/xe/xe_mmio.h:65:72: warning: incorrect type in argument 1
(different address spaces)
drivers/gpu/drm/xe/xe_mmio.h:65:72:expected void const volatile [noderef]
__iomem *addr
drivers/gpu/drm/xe/xe_mmio.h:65:72:got void *
drivers/gpu/drm/xe/xe_device.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
index d9ae77fe7382..b8d8da546670 100644
--- a/drivers/gpu/drm/xe/xe_device.c
+++ b/drivers/gpu/drm/xe/xe_device.c
@@ -484,7 +484,7 @@ int xe_device_probe(struct xe_device *xe)
err = xe_device_set_has_flat_ccs(xe);
if (err)
- return err;
+ goto err_irq_shutdown;
err = xe_mmio_probe_vram(xe);
if (err)
--
2.42.0
[bug report] drm/xe: Introduce a new DRM driver for Intel GPUs
Hello Matthew Brost,
The patch dd08ebf6c352: "drm/xe: Introduce a new DRM driver for Intel
GPUs" from Mar 30, 2023 (linux-next), leads to the following Smatch
static checker warning:
drivers/gpu/drm/xe/xe_bo.c:2246 xe_bo_dumb_create()
warn: potential integer overflow from user '((args->width * cpp)) +
(((64)) - 1)'
drivers/gpu/drm/xe/xe_bo.c
2234 int xe_bo_dumb_create(struct drm_file *file_priv,
2235 struct drm_device *dev,
2236 struct drm_mode_create_dumb *args)
2237 {
2238 struct xe_device *xe = to_xe_device(dev);
2239 struct xe_bo *bo;
2240 uint32_t handle;
2241 int cpp = DIV_ROUND_UP(args->bpp, 8);
2242 int err;
2243 u32 page_size = max_t(u32, PAGE_SIZE,
2244 xe->info.vram_flags & XE_VRAM_FLAGS_NEED64K ? SZ_64K :
SZ_4K);
2245
--> 2246 args->pitch = ALIGN(args->width * cpp, 64);
drm_mode_create_dumb() guarantees that "args->width * cpp" can't
overflow but if we pick "args->width * cpp" set to U32_MAX - 63 or above
then the ALIGN() can overflow to zero.
I should have picked INT_MAX as the limit in drm_mode_create_dumb()...
2247 args->size = ALIGN(mul_u32_u32(args->pitch, args->height),
2248page_size);
2249
2250 bo = xe_bo_create_user(xe, NULL, NULL, args->size,
2251DRM_XE_GEM_CPU_CACHING_WC,
2252ttm_bo_type_device,
2253
XE_BO_CREATE_VRAM_IF_DGFX(xe_device_get_root_tile(xe)) |
2254XE_BO_CREATE_USER_BIT |
XE_BO_SCANOUT_BIT |
2255XE_BO_NEEDS_CPU_ACCESS);
2256 if (IS_ERR(bo))
2257 return PTR_ERR(bo);
2258
2259 err = drm_gem_handle_create(file_priv, >ttm.base, );
2260 /* drop reference from allocate - handle holds it now */
2261 drm_gem_object_put(>ttm.base);
2262 if (!err)
2263 args->handle = handle;
2264 return err;
2265 }
regards,
dan carpenter
[bug report] drm/xe: shift wrapping in xe_gem_create_ioctl()
Hello Matthew Brost,
The patch dd08ebf6c352: "drm/xe: Introduce a new DRM driver for Intel
GPUs" from Mar 30, 2023 (linux-next), leads to the following Smatch
static checker warning:
drivers/gpu/drm/xe/xe_bo.c:1916 xe_gem_create_ioctl()
warn: potential integer overflow from user 'args->placement <<'
drivers/gpu/drm/xe/xe_bo.c
1869 int xe_gem_create_ioctl(struct drm_device *dev, void *data,
1870 struct drm_file *file)
1871 {
1872 struct xe_device *xe = to_xe_device(dev);
1873 struct xe_file *xef = to_xe_file(file);
1874 struct drm_xe_gem_create *args = data;
1875 struct xe_vm *vm = NULL;
1876 struct xe_bo *bo;
1877 unsigned int bo_flags;
1878 u32 handle;
1879 int err;
1880
1881 if (XE_IOCTL_DBG(xe, args->extensions) ||
1882 XE_IOCTL_DBG(xe, args->pad[0] || args->pad[1] ||
args->pad[2]) ||
1883 XE_IOCTL_DBG(xe, args->reserved[0] || args->reserved[1]))
1884 return -EINVAL;
1885
1886 /* at least one valid memory placement must be specified */
1887 if (XE_IOCTL_DBG(xe, (args->placement &
~xe->info.mem_region_mask) ||
1888 !args->placement))
1889 return -EINVAL;
1890
1891 if (XE_IOCTL_DBG(xe, args->flags &
1892 ~(DRM_XE_GEM_CREATE_FLAG_DEFER_BACKING |
1893DRM_XE_GEM_CREATE_FLAG_SCANOUT |
1894DRM_XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM)))
1895 return -EINVAL;
1896
1897 if (XE_IOCTL_DBG(xe, args->handle))
1898 return -EINVAL;
1899
1900 if (XE_IOCTL_DBG(xe, !args->size))
1901 return -EINVAL;
1902
1903 if (XE_IOCTL_DBG(xe, args->size > SIZE_MAX))
1904 return -EINVAL;
1905
1906 if (XE_IOCTL_DBG(xe, args->size & ~PAGE_MASK))
1907 return -EINVAL;
1908
1909 bo_flags = 0;
1910 if (args->flags & DRM_XE_GEM_CREATE_FLAG_DEFER_BACKING)
1911 bo_flags |= XE_BO_DEFER_BACKING;
1912
1913 if (args->flags & DRM_XE_GEM_CREATE_FLAG_SCANOUT)
1914 bo_flags |= XE_BO_SCANOUT_BIT;
1915
--> 1916 bo_flags |= args->placement << (ffs(XE_BO_CREATE_SYSTEM_BIT) -
1);
^^^
Potential shift wrapping.
1917
1918 if (args->flags & DRM_XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM) {
1919 if (XE_IOCTL_DBG(xe, !(bo_flags &
XE_BO_CREATE_VRAM_MASK)))
1920 return -EINVAL;
1921
1922 bo_flags |= XE_BO_NEEDS_CPU_ACCESS;
1923 }
1924
regards,
dan carpenter
[PATCH] drm/xe/selftests: Fix an error pointer dereference bug
Check if "bo" is an error pointer before calling xe_bo_lock() on it.
Fixes: d6abc18d6693 ("drm/xe/xe2: Modify xe_bo_test for system memory")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/xe/tests/xe_bo.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/xe/tests/xe_bo.c b/drivers/gpu/drm/xe/tests/xe_bo.c
index 412b2e7ce40c..3436fd9cf2b2 100644
--- a/drivers/gpu/drm/xe/tests/xe_bo.c
+++ b/drivers/gpu/drm/xe/tests/xe_bo.c
@@ -125,14 +125,13 @@ static void ccs_test_run_tile(struct xe_device *xe,
struct xe_tile *tile,
bo = xe_bo_create_user(xe, NULL, NULL, SZ_1M, DRM_XE_GEM_CPU_CACHING_WC,
ttm_bo_type_device, bo_flags);
-
- xe_bo_lock(bo, false);
-
if (IS_ERR(bo)) {
KUNIT_FAIL(test, "Failed to create bo.\n");
return;
}
+ xe_bo_lock(bo, false);
+
kunit_info(test, "Verifying that CCS data is cleared on creation.\n");
ret = ccs_test_migrate(tile, bo, false, 0ULL, 0xdeadbeefdeadbeefULL,
test);
--
2.42.0
[PATCH] drm/xe: unlock on error path in xe_vm_add_compute_exec_queue()
Drop the ">lock" before returning.
Fixes: 24f947d58fe5 ("drm/xe: Use DRM GPUVM helpers for external- and evicted
objects")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/xe/xe_vm.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
index 9180f2d2d71d..4aa7979fe6bf 100644
--- a/drivers/gpu/drm/xe/xe_vm.c
+++ b/drivers/gpu/drm/xe/xe_vm.c
@@ -332,13 +332,13 @@ int xe_vm_add_compute_exec_queue(struct xe_vm *vm, struct
xe_exec_queue *q)
down_write(>lock);
err = drm_gpuvm_exec_lock(_exec);
if (err)
- return err;
+ goto out_up_write;
pfence = xe_preempt_fence_create(q, q->compute.context,
++q->compute.seqno);
if (!pfence) {
err = -ENOMEM;
- goto out_unlock;
+ goto out_fini;
}
list_add(>compute.link, >preempt.exec_queues);
@@ -361,8 +361,9 @@ int xe_vm_add_compute_exec_queue(struct xe_vm *vm, struct
xe_exec_queue *q)
up_read(>userptr.notifier_lock);
-out_unlock:
+out_fini:
drm_exec_fini(exec);
+out_up_write:
up_write(>lock);
return err;
--
2.42.0
Re: [PATCH 1/3] drm: property: One function call less in drm_property_create() after error detection
On Wed, Jan 03, 2024 at 06:18:13PM +0100, Michel Dänzer wrote: > On 2024-01-03 17:24, Markus Elfring wrote: > > > >> Out of curiosity, what exactly did Coccinelle report? > > > > Some SmPL scripts from my own selection tend to point questionable > > implementation details out. > > That doesn't answer my question. > > Without seeing the actual Coccinelle report, I'll assume that it didn't > actually call for this change. This isn't one of the Coccinelle scripts which ship with the kernel, it's something that Markus wrote himself. regards, dan carpenter
[bug report] drm/xe: missing access_ok() in ioctl
Hello Matthew Brost,
The patch dd08ebf6c352: "drm/xe: Introduce a new DRM driver for Intel
GPUs" from Mar 30, 2023 (linux-next), leads to the following Smatch
static checker warning:
drivers/gpu/drm/xe/xe_exec.c:163 xe_exec_ioctl()
warn: calling '__copy_from_user()' without access_ok()
drivers/gpu/drm/xe/xe_exec.c
102 int xe_exec_ioctl(struct drm_device *dev, void *data, struct drm_file
*file)
103 {
104 struct xe_device *xe = to_xe_device(dev);
105 struct xe_file *xef = to_xe_file(file);
106 struct drm_xe_exec *args = data;
107 struct drm_xe_sync __user *syncs_user =
u64_to_user_ptr(args->syncs);
108 u64 __user *addresses_user = u64_to_user_ptr(args->address);
109 struct xe_exec_queue *q;
110 struct xe_sync_entry *syncs = NULL;
111 u64 addresses[XE_HW_ENGINE_MAX_INSTANCE];
112 struct drm_gpuvm_exec vm_exec = {.extra.fn = xe_exec_fn};
113 struct drm_exec *exec = _exec.exec;
114 u32 i, num_syncs = 0;
115 struct xe_sched_job *job;
116 struct dma_fence *rebind_fence;
117 struct xe_vm *vm;
118 bool write_locked;
119 ktime_t end = 0;
120 int err = 0;
121
122 if (XE_IOCTL_DBG(xe, args->extensions) ||
123 XE_IOCTL_DBG(xe, args->pad[0] || args->pad[1] ||
args->pad[2]) ||
124 XE_IOCTL_DBG(xe, args->reserved[0] || args->reserved[1]))
125 return -EINVAL;
126
127 q = xe_exec_queue_lookup(xef, args->exec_queue_id);
128 if (XE_IOCTL_DBG(xe, !q))
129 return -ENOENT;
130
131 if (XE_IOCTL_DBG(xe, q->flags & EXEC_QUEUE_FLAG_VM))
132 return -EINVAL;
133
134 if (XE_IOCTL_DBG(xe, args->num_batch_buffer &&
135 q->width != args->num_batch_buffer))
136 return -EINVAL;
137
138 if (XE_IOCTL_DBG(xe, q->flags & EXEC_QUEUE_FLAG_BANNED)) {
139 err = -ECANCELED;
140 goto err_exec_queue;
141 }
142
143 if (args->num_syncs) {
144 syncs = kcalloc(args->num_syncs, sizeof(*syncs),
GFP_KERNEL);
145 if (!syncs) {
146 err = -ENOMEM;
147 goto err_exec_queue;
148 }
149 }
150
151 vm = q->vm;
152
153 for (i = 0; i < args->num_syncs; i++) {
154 err = xe_sync_entry_parse(xe, xef, [num_syncs++],
155 _user[i],
SYNC_PARSE_FLAG_EXEC |
156 (xe_vm_in_lr_mode(vm) ?
157SYNC_PARSE_FLAG_LR_MODE :
0));
158 if (err)
159 goto err_syncs;
160 }
161
162 if (xe_exec_queue_is_parallel(q)) {
--> 163 err = __copy_from_user(addresses, addresses_user,
sizeof(u64) *
164q->width);
It's been a long time since I've ever reported one of these. The
difference between copy_from_user() and __copy_from_user() is that the
underscore version means we called
access_ok(addresses_user, sizeof(u64) * q->width) already. In olden
days if we were doing multiple copies then it was nice to avoid doing
duplicate checks on the fast path.
I don't think we have called access_ok() here. At one point Linus said
that if we don't call access_ok() in the same function then it should
just automatically considered a bug. We used to have a lot of issues
with this back in the day... I don't think anyone took Linus too
seriously on this though.
165 if (err) {
166 err = -EFAULT;
167 goto err_syncs;
168 }
169 }
170
171 retry:
172 if (!xe_vm_in_lr_mode(vm) && xe_vm_userptr_check_repin(vm)) {
173 err = down_write_killable(>lock);
174 write_locked = true;
175 } else {
regards,
dan carpenter
Re: [PATCH 08/11] nouveau/gsp: don't free ctrl messages on errors
On Thu, Jan 04, 2024 at 10:41:50AM +1000, Dave Airlie wrote: > On Thu, 4 Jan 2024 at 00:47, Dan Carpenter wrote: > > > > Hi Dave, > > > > 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/Dave-Airlie/nouveau-gsp-drop-some-acpi-related-debug/20231222-180432 > > base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next > > patch link: > > https://lore.kernel.org/r/20231222043308.3090089-9-airlied%40gmail.com > > patch subject: [PATCH 08/11] nouveau/gsp: don't free ctrl messages on errors > > config: powerpc-randconfig-r071-20231226 > > (https://download.01.org/0day-ci/archive/20231227/202312271917.55xudmdc-...@intel.com/config) > > compiler: clang version 18.0.0git (https://github.com/llvm/llvm-project > > d3ef86708241a3bee902615c190dead1638c4e09) > > > > 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/202312271917.55xudmdc-...@intel.com/ > > This is a false positive, I think the code is operating like I'd > expect, we maybe could restructure it to avoid this warning? > > The idea is you send an rpc msg, if there's a reply you get a reply, > if no reply you get NULL and if an error you get an error. > > So in the case you get an error or NULL you just want to return 0 for > the NULL as it's successful, and error otherwise. > > Would using PTR_ERR_OR_ZERO make smatch happy? (even if it's not > really what we want). Hm... You're using the API correctly. Linus has complained about this warning before but in new code over 90% of the warnings are correct. It's a high quality warning. I looked around for an explanation to see what the NULL meant but couldn't find it documented in the code. The NULL vs error pointer comes from a function pointer and it's not always clear where the documentation should be with a function pointer. So perhaps I missed it. Let's not use PTR_ERR_OR_ZERO. Perhaps I should introduce a PTR_ERR_OR_NULL() macro to silence this warning. But most of the code which does this correctly is in fs/ and they probably are like Linus and would be surprised to learn that people get it wrong... regards, dan carpenter
[bug report] drm/xe: Introduce a new DRM driver for Intel GPUs
Hello Matthew Brost, The patch dd08ebf6c352: "drm/xe: Introduce a new DRM driver for Intel GPUs" from Mar 30, 2023 (linux-next), leads to the following Smatch static checker warning: drivers/gpu/drm/xe/tests/xe_bo.c:298 evict_test_run_tile() error: double unlocked 'external->ttm.base.resv' (orig line 220) drivers/gpu/drm/xe/tests/xe_bo.c 294 295 xe_bo_lock(external, false); 296 xe_bo_unpin_external(external); 297 xe_bo_unlock(external); This is a false positive, but the API is ugly... xe_bo_lock() is an interruptible lock if the second argument is true. Why not just create a xe_bo_lock_interruptible()? This has several advantages: 1) More readable and you could delete the comments explaining how to use it because it's so obvious. 2) Less typing because you wouldn't have to write ", false" so much. 3) You could mark the _interruptible version as must check and the other version could be a void function. 4) Smatch could parse it automatically. I can write some custom code to parse the existing function but it's a small hassle for me. regards, dan carpenter
Re: [PATCH 08/11] nouveau/gsp: don't free ctrl messages on errors
Hi Dave,
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/Dave-Airlie/nouveau-gsp-drop-some-acpi-related-debug/20231222-180432
base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:
https://lore.kernel.org/r/20231222043308.3090089-9-airlied%40gmail.com
patch subject: [PATCH 08/11] nouveau/gsp: don't free ctrl messages on errors
config: powerpc-randconfig-r071-20231226
(https://download.01.org/0day-ci/archive/20231227/202312271917.55xudmdc-...@intel.com/config)
compiler: clang version 18.0.0git (https://github.com/llvm/llvm-project
d3ef86708241a3bee902615c190dead1638c4e09)
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/202312271917.55xudmdc-...@intel.com/
New smatch warnings:
drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c:659 r535_gsp_rpc_rm_ctrl_push()
warn: passing zero to 'PTR_ERR'
drivers/gpu/drm/nouveau/nvkm/engine/disp/r535.c:1063 r535_dp_aux_xfer() warn:
passing a valid pointer to 'PTR_ERR'
Old smatch warnings:
drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c:1887 nvkm_gsp_radix3_sg() error:
uninitialized symbol 'addr'.
vim +/PTR_ERR +659 drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
af265ee961627a Dave Airlie 2023-12-22 649 static int
af265ee961627a Dave Airlie 2023-12-22 650 r535_gsp_rpc_rm_ctrl_push(struct
nvkm_gsp_object *object, void **argv, u32 repc)
4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 651 {
af265ee961627a Dave Airlie 2023-12-22 652 rpc_gsp_rm_control_v03_00 *rpc
= container_of((*argv), typeof(*rpc), params);
4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 653 struct nvkm_gsp *gsp =
object->client->gsp;
af265ee961627a Dave Airlie 2023-12-22 654 int ret = 0;
4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 655
4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 656 rpc = nvkm_gsp_rpc_push(gsp,
rpc, true, repc);
af265ee961627a Dave Airlie 2023-12-22 657 if (IS_ERR_OR_NULL(rpc)) {
af265ee961627a Dave Airlie 2023-12-22 658 *argv = NULL;
af265ee961627a Dave Airlie 2023-12-22 @659 return PTR_ERR(rpc);
If nvkm_gsp_rpc_push() returns NULL (probably a failure) then this
returns PTR_ERR(NULL) which is zero/success.
af265ee961627a Dave Airlie 2023-12-22 660 }
4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 661
4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 662 if (rpc->status) {
af265ee961627a Dave Airlie 2023-12-22 663 ret =
r535_rpc_status_to_errno(rpc->status);
555bb9c29a45be Dave Airlie 2023-12-22 664 if (ret != -EAGAIN)
4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 665
nvkm_error(>subdev, "cli:0x%08x obj:0x%08x ctrl cmd:0x%08x failed:
0x%08x\n",
4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 666
object->client->object.handle, object->handle, rpc->cmd, rpc->status);
4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 667 }
4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 668
af265ee961627a Dave Airlie 2023-12-22 669 if (repc)
af265ee961627a Dave Airlie 2023-12-22 670 *argv = rpc->params;
af265ee961627a Dave Airlie 2023-12-22 671 else
4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 672 nvkm_gsp_rpc_done(gsp,
rpc);
4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 673
4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 674 return ret;
4cf2c83eb3a4c4 Ben Skeggs 2023-09-19 675 }
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [PATCH 2/2] drm/imagination: Fix error path in pvr_vm_create_context
Thanks so much. Reviewed-by: Dan Carpenter regards, dan carpenter
Re: [PATCH v4 06/10] drm/panel: Add Synaptics R63353 panel driver
Hi Dario,
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/Dario-Binacchi/drm-bridge-Fix-bridge-disable-logic/20231205-185455
base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:
https://lore.kernel.org/r/20231205105341.4100896-7-dario.binacchi%40amarulasolutions.com
patch subject: [PATCH v4 06/10] drm/panel: Add Synaptics R63353 panel driver
config: i386-randconfig-r071-20231206
(https://download.01.org/0day-ci/archive/20231207/202312070214.eyi9t4eq-...@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce:
(https://download.01.org/0day-ci/archive/20231207/202312070214.eyi9t4eq-...@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/202312070214.eyi9t4eq-...@intel.com/
smatch warnings:
drivers/gpu/drm/panel/panel-synaptics-r63353.c:132 r63353_panel_activate()
warn: missing unwind goto?
vim +132 drivers/gpu/drm/panel/panel-synaptics-r63353.c
64f91a53613aea Michael Trimarchi 2023-12-05 107 static int
r63353_panel_activate(struct r63353_panel *rpanel)
64f91a53613aea Michael Trimarchi 2023-12-05 108 {
64f91a53613aea Michael Trimarchi 2023-12-05 109struct mipi_dsi_device
*dsi = rpanel->dsi;
64f91a53613aea Michael Trimarchi 2023-12-05 110struct device *dev =
>dev;
64f91a53613aea Michael Trimarchi 2023-12-05 111int i, ret;
64f91a53613aea Michael Trimarchi 2023-12-05 112
64f91a53613aea Michael Trimarchi 2023-12-05 113ret =
mipi_dsi_dcs_soft_reset(dsi);
64f91a53613aea Michael Trimarchi 2023-12-05 114if (ret < 0) {
64f91a53613aea Michael Trimarchi 2023-12-05 115dev_err(dev,
"Failed to do Software Reset (%d)\n", ret);
64f91a53613aea Michael Trimarchi 2023-12-05 116goto fail;
64f91a53613aea Michael Trimarchi 2023-12-05 117}
64f91a53613aea Michael Trimarchi 2023-12-05 118
64f91a53613aea Michael Trimarchi 2023-12-05 119usleep_range(15000,
17000);
64f91a53613aea Michael Trimarchi 2023-12-05 120
64f91a53613aea Michael Trimarchi 2023-12-05 121ret =
mipi_dsi_dcs_enter_sleep_mode(dsi);
64f91a53613aea Michael Trimarchi 2023-12-05 122if (ret < 0) {
64f91a53613aea Michael Trimarchi 2023-12-05 123dev_err(dev,
"Failed to enter sleep mode (%d)\n", ret);
64f91a53613aea Michael Trimarchi 2023-12-05 124goto fail;
64f91a53613aea Michael Trimarchi 2023-12-05 125}
64f91a53613aea Michael Trimarchi 2023-12-05 126
64f91a53613aea Michael Trimarchi 2023-12-05 127for (i = 0; i <
rpanel->pdata->init_length; i++) {
64f91a53613aea Michael Trimarchi 2023-12-05 128const struct
r63353_instr *instr = >pdata->init[i];
64f91a53613aea Michael Trimarchi 2023-12-05 129
64f91a53613aea Michael Trimarchi 2023-12-05 130ret =
mipi_dsi_dcs_write_buffer(dsi, instr->data, instr->len);
64f91a53613aea Michael Trimarchi 2023-12-05 131if (ret < 0)
64f91a53613aea Michael Trimarchi 2023-12-05 @132return
ret;
goto fail?
64f91a53613aea Michael Trimarchi 2023-12-05 133}
64f91a53613aea Michael Trimarchi 2023-12-05 134
64f91a53613aea Michael Trimarchi 2023-12-05 135msleep(120);
64f91a53613aea Michael Trimarchi 2023-12-05 136
64f91a53613aea Michael Trimarchi 2023-12-05 137ret =
mipi_dsi_dcs_exit_sleep_mode(dsi);
64f91a53613aea Michael Trimarchi 2023-12-05 138if (ret < 0) {
64f91a53613aea Michael Trimarchi 2023-12-05 139dev_err(dev,
"Failed to exit sleep mode (%d)\n", ret);
64f91a53613aea Michael Trimarchi 2023-12-05 140goto fail;
64f91a53613aea Michael Trimarchi 2023-12-05 141}
64f91a53613aea Michael Trimarchi 2023-12-05 142
64f91a53613aea Michael Trimarchi 2023-12-05 143usleep_range(5000,
1);
64f91a53613aea Michael Trimarchi 2023-12-05 144
64f91a53613aea Michael Trimarchi 2023-12-05 145ret =
mipi_dsi_dcs_set_display_on(dsi);
64f91a53613aea Michael Trimarchi 2023-12-05 146if (ret < 0) {
64f91a53613aea Michael Trimarchi 2023-12-05 147dev_err(dev,
"Failed to set display ON (%d)\n", ret);
64f91a53613aea Michael Trimarchi 2023-12-05 148goto fail;
64f91a53613aea Michael Trimarchi 2023-12-05 149}
64f91a53613aea Michael Trimarchi 2023-12-05 150
64f91a53613aea Michael Trimarchi 2023-12-05 151return 0;
64f91a53613aea Michael Trimarchi 2023-12-05 152
64f91a53613aea Michael Trimarchi 2023-12-05 153 fail:
64f91a53613aea Michael Trimarchi 2023-12-05 154
gpiod_
[PATCH] drm/bridge: nxp-ptn3460: simplify some error checking
The i2c_master_send/recv() functions return negative error codes or
they return "len" on success. So the error handling here can be written
as just normal checks for "if (ret < 0) return ret;". No need to
complicate things.
Btw, in this code the "len" parameter can never be zero, but even if
it were, then I feel like this would still be the best way to write it.
Fixes: 914437992876 ("drm/bridge: nxp-ptn3460: fix i2c_master_send() error
checking")
Signed-off-by: Dan Carpenter
---
This is not really a bug fix but I added a Fixes tag because I don't
want people to pull my other commit without also applying this.
drivers/gpu/drm/bridge/nxp-ptn3460.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c
b/drivers/gpu/drm/bridge/nxp-ptn3460.c
index 9b7eb8c669c1..7c0076e49953 100644
--- a/drivers/gpu/drm/bridge/nxp-ptn3460.c
+++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c
@@ -54,15 +54,15 @@ static int ptn3460_read_bytes(struct ptn3460_bridge
*ptn_bridge, char addr,
int ret;
ret = i2c_master_send(ptn_bridge->client, , 1);
- if (ret <= 0) {
+ if (ret < 0) {
DRM_ERROR("Failed to send i2c command, ret=%d\n", ret);
- return ret ?: -EIO;
+ return ret;
}
ret = i2c_master_recv(ptn_bridge->client, buf, len);
- if (ret != len) {
+ if (ret < 0) {
DRM_ERROR("Failed to recv i2c data, ret=%d\n", ret);
- return ret < 0 ? ret : -EIO;
+ return ret;
}
return 0;
@@ -78,9 +78,9 @@ static int ptn3460_write_byte(struct ptn3460_bridge
*ptn_bridge, char addr,
buf[1] = val;
ret = i2c_master_send(ptn_bridge->client, buf, ARRAY_SIZE(buf));
- if (ret != ARRAY_SIZE(buf)) {
+ if (ret < 0) {
DRM_ERROR("Failed to send i2c command, ret=%d\n", ret);
- return ret < 0 ? ret : -EIO;
+ return ret;
}
return 0;
--
2.42.0
[PATCH] drm/imagination: Move dereference after NULL check in pvr_mmu_backing_page_init()
This code dereferences "page->pvr_dev" and then checked for NULL on the
next line. Re-order it to avoid a potential NULL pointer dereference.
Fixes: ff5f643de0bf ("drm/imagination: Add GEM and VM related code")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/imagination/pvr_mmu.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/imagination/pvr_mmu.c
b/drivers/gpu/drm/imagination/pvr_mmu.c
index c8562bfc0dcd..4fe70610ed94 100644
--- a/drivers/gpu/drm/imagination/pvr_mmu.c
+++ b/drivers/gpu/drm/imagination/pvr_mmu.c
@@ -316,12 +316,14 @@ pvr_mmu_backing_page_init(struct pvr_mmu_backing_page
*page,
static void
pvr_mmu_backing_page_fini(struct pvr_mmu_backing_page *page)
{
- struct device *dev = from_pvr_device(page->pvr_dev)->dev;
+ struct device *dev;
/* Do nothing if no allocation is present. */
if (!page->pvr_dev)
return;
+ dev = from_pvr_device(page->pvr_dev)->dev;
+
dma_unmap_page(dev, page->dma_addr, PVR_MMU_BACKING_PAGE_SIZE,
DMA_TO_DEVICE);
--
2.42.0
[bug report] drm/imagination: Implement MIPS firmware processor and MMU support
Hello Sarah Walker,
The patch 927f3e0253c1: "drm/imagination: Implement MIPS firmware
processor and MMU support" from Nov 22, 2023 (linux-next), leads to
the following Smatch static checker warning:
drivers/gpu/drm/imagination/pvr_vm_mips.c:204 pvr_vm_mips_map()
warn: 'pfn' can be negative (type promoted to high)
drivers/gpu/drm/imagination/pvr_vm_mips.c
144 int
145 pvr_vm_mips_map(struct pvr_device *pvr_dev, struct pvr_fw_object
*fw_obj)
146 {
147 struct pvr_fw_device *fw_dev = _dev->fw_dev;
148 struct pvr_fw_mips_data *mips_data =
fw_dev->processor_data.mips_data;
149 struct pvr_gem_object *pvr_obj = fw_obj->gem;
150 const u64 start = fw_obj->fw_mm_node.start;
151 const u64 size = fw_obj->fw_mm_node.size;
152 u64 end;
153 u32 cache_policy;
154 u32 pte_flags;
155 u32 start_pfn;
156 u32 end_pfn;
157 s32 pfn;
pfn is s32 but start_pfn is u32.
158 int err;
159
160 if (check_add_overflow(start, size - 1, ))
161 return -EINVAL;
162
163 if (start < ROGUE_FW_HEAP_BASE ||
164 start >= ROGUE_FW_HEAP_BASE + fw_dev->fw_heap_info.raw_size
||
165 end < ROGUE_FW_HEAP_BASE ||
166 end >= ROGUE_FW_HEAP_BASE + fw_dev->fw_heap_info.raw_size ||
167 (start & ROGUE_MIPSFW_PAGE_MASK_4K) ||
168 ((end + 1) & ROGUE_MIPSFW_PAGE_MASK_4K))
169 return -EINVAL;
170
171 start_pfn = (start & fw_dev->fw_heap_info.offset_mask) >>
ROGUE_MIPSFW_LOG2_PAGE_SIZE_4K;
Can start_pfn be zero?
172 end_pfn = (end & fw_dev->fw_heap_info.offset_mask) >>
ROGUE_MIPSFW_LOG2_PAGE_SIZE_4K;
173
174 if (pvr_obj->flags & PVR_BO_FW_FLAGS_DEVICE_UNCACHED)
175 cache_policy = ROGUE_MIPSFW_UNCACHED_CACHE_POLICY;
176 else
177 cache_policy = mips_data->cache_policy;
178
179 pte_flags = get_mips_pte_flags(true, true, cache_policy);
180
181 for (pfn = start_pfn; pfn <= end_pfn; pfn++) {
182 dma_addr_t dma_addr;
183 u32 pte;
184
185 err = pvr_fw_object_get_dma_addr(fw_obj,
186 (pfn - start_pfn) <<
187
ROGUE_MIPSFW_LOG2_PAGE_SIZE_4K,
188 _addr);
189 if (err)
190 goto err_unmap_pages;
191
192 pte = ((dma_addr >> ROGUE_MIPSFW_LOG2_PAGE_SIZE_4K)
193<< ROGUE_MIPSFW_ENTRYLO_PFN_SHIFT) &
mips_data->pfn_mask;
194 pte |= pte_flags;
195
196 WRITE_ONCE(mips_data->pt[pfn], pte);
197 }
198
199 pvr_mmu_flush_request_all(pvr_dev);
200
201 return 0;
202
203 err_unmap_pages:
--> 204 for (; pfn >= start_pfn; pfn--)
If start_pfn can be zero then this is an endless loop. I've seen this
code in other places as well. This loop is slightly off as well. It
should decrement pfn on the first iteration.
There are hack arounds for using unsigned iterators:
while (pfn-- > start_pfn)
WRITE_ONCE(mips_data->pt[pfn], 0);
Personally I would be tempted to make things signed... Here are some
links to various rants I have wrote:
https://staticthinking.wordpress.com/2022/06/01/unsigned-int-i-is-stupid/
https://staticthinking.wordpress.com/2023/07/25/wsign-compare-is-garbage/
You didn't ask for rants... No one ever asks for rants...
205 WRITE_ONCE(mips_data->pt[pfn], 0);
206
207 pvr_mmu_flush_request_all(pvr_dev);
208 WARN_ON(pvr_mmu_flush_exec(pvr_dev, true));
209
210 return err;
211 }
regards,
dan carpenter
[bug report] drm/imagination: Implement firmware infrastructure and META FW support
Hello Sarah Walker,
The patch cc1aeedb98ad: "drm/imagination: Implement firmware
infrastructure and META FW support" from Nov 22, 2023 (linux-next),
leads to the following Smatch static checker warning:
drivers/gpu/drm/imagination/pvr_vm.c:631 pvr_vm_create_context()
error: 'vm_ctx->mmu_ctx' dereferencing possible ERR_PTR()
drivers/gpu/drm/imagination/pvr_vm.c
597 vm_ctx = kzalloc(sizeof(*vm_ctx), GFP_KERNEL);
598 if (!vm_ctx)
599 return ERR_PTR(-ENOMEM);
600
601 drm_gem_private_object_init(_dev->base, _ctx->dummy_gem,
0);
602
603 vm_ctx->pvr_dev = pvr_dev;
604 kref_init(_ctx->ref_count);
605 mutex_init(_ctx->lock);
606
607 drm_gpuvm_init(_ctx->gpuvm_mgr,
608is_userspace_context ? "PowerVR-user-VM" :
"PowerVR-FW-VM",
6090, _dev->base, _ctx->dummy_gem,
6100, 1ULL << device_addr_bits, 0, 0,
_vm_gpuva_ops);
611
612 vm_ctx->mmu_ctx = pvr_mmu_context_create(pvr_dev);
613 err = PTR_ERR_OR_ZERO(_ctx->mmu_ctx);
^
s/&//.
The address is never an error pointer so this will always return 0.
Fixing this will silence the static checker but there are some other
issues as well.
614 if (err) {
615 vm_ctx->mmu_ctx = NULL;
Setting vm_ctx->mmu_ctx is not sufficient.
616 goto err_put_ctx;
617 }
618
619 if (is_userspace_context) {
620 err = pvr_fw_object_create(pvr_dev, sizeof(struct
rogue_fwif_fwmemcontext),
621
PVR_BO_FW_FLAGS_DEVICE_UNCACHED,
622fw_mem_context_init, vm_ctx,
_ctx->fw_mem_ctx_obj);
623
624 if (err)
625 goto err_page_table_destroy;
626 }
627
628 return vm_ctx;
629
630 err_page_table_destroy:
--> 631 pvr_mmu_context_destroy(vm_ctx->mmu_ctx);
This will lead to a double free. Delete.
632
633 err_put_ctx:
634 pvr_vm_context_put(vm_ctx);
The pvr_vm_context_put() function does:
kref_put(_ctx->ref_count, pvr_vm_context_release);
I really think that with kref free functions the way to do it is to
wait until the last possible momement when everything has been allocated
before we set up the kref release code. Here the pvr_vm_context_release()
will call:
pvr_mmu_context_destroy(vm_ctx->mmu_ctx);
We already did that on line 631 as mentioned so it's a double free. But
also imagine if vm_ctx->mmu_ctx is NULL, then it will lead to a NULL
dereference.
The pvr_vm_context_release() function has several WARN() functions that
trigger if not everything is set up. It's complicated to review. So I
kind of always think that people should manually kfree() things in their
allocation functions and then do a kref_init() at the end.
635
636 return ERR_PTR(err);
637 }
regards,
dan carpenter
[bug report] drm/imagination: Implement firmware infrastructure and META FW support
Hello Sarah Walker,
The patch cc1aeedb98ad: "drm/imagination: Implement firmware
infrastructure and META FW support" from Nov 22, 2023 (linux-next),
leads to the following Smatch static checker warning:
drivers/gpu/drm/imagination/pvr_fw_startstop.c:210 pvr_fw_stop() warn: odd mask
'0x & 0x'
drivers/gpu/drm/imagination/pvr_fw_startstop.c:213 pvr_fw_stop() warn: odd mask
'0x & 0x'
drivers/gpu/drm/imagination/pvr_fw_startstop.c:216 pvr_fw_stop() warn: odd mask
'0x & 0x'
drivers/gpu/drm/imagination/pvr_fw_startstop.c:219 pvr_fw_stop() warn: odd mask
'0x & 0x'
drivers/gpu/drm/imagination/pvr_fw_startstop.c
187 int
188 pvr_fw_stop(struct pvr_device *pvr_dev)
189 {
190 const u32 sidekick_idle_mask = ROGUE_CR_SIDEKICK_IDLE_MASKFULL &
191
~(ROGUE_CR_SIDEKICK_IDLE_GARTEN_EN |
192
ROGUE_CR_SIDEKICK_IDLE_SOCIF_EN |
193
ROGUE_CR_SIDEKICK_IDLE_HOSTIF_EN);
194 bool skip_garten_idle = false;
195 u32 reg_value;
196 int err;
197
198 /*
199 * Wait for Sidekick/Jones to signal IDLE except for the Garten
Wrapper.
200 * For cores with the LAYOUT_MARS feature, SIDEKICK would have
been
201 * powered down by the FW.
202 */
203 err = pvr_cr_poll_reg32(pvr_dev, ROGUE_CR_SIDEKICK_IDLE,
sidekick_idle_mask,
204 sidekick_idle_mask, POLL_TIMEOUT_USEC);
205 if (err)
206 return err;
207
208 /* Unset MTS DM association with threads. */
209 pvr_cr_write32(pvr_dev, ROGUE_CR_MTS_INTCTX_THREAD0_DM_ASSOC,
--> 210ROGUE_CR_MTS_INTCTX_THREAD0_DM_ASSOC_MASKFULL &
211
ROGUE_CR_MTS_INTCTX_THREAD0_DM_ASSOC_DM_ASSOC_CLRMSK);
What's the point of these masks? They don't overlap so they just equal
zero.
212 pvr_cr_write32(pvr_dev, ROGUE_CR_MTS_BGCTX_THREAD0_DM_ASSOC,
213ROGUE_CR_MTS_BGCTX_THREAD0_DM_ASSOC_MASKFULL &
214
ROGUE_CR_MTS_BGCTX_THREAD0_DM_ASSOC_DM_ASSOC_CLRMSK);
215 pvr_cr_write32(pvr_dev, ROGUE_CR_MTS_INTCTX_THREAD1_DM_ASSOC,
216ROGUE_CR_MTS_INTCTX_THREAD1_DM_ASSOC_MASKFULL &
217
ROGUE_CR_MTS_INTCTX_THREAD1_DM_ASSOC_DM_ASSOC_CLRMSK);
218 pvr_cr_write32(pvr_dev, ROGUE_CR_MTS_BGCTX_THREAD1_DM_ASSOC,
219ROGUE_CR_MTS_BGCTX_THREAD1_DM_ASSOC_MASKFULL &
220
ROGUE_CR_MTS_BGCTX_THREAD1_DM_ASSOC_DM_ASSOC_CLRMSK);
221
222 /* Extra Idle checks. */
223 err = pvr_cr_poll_reg32(pvr_dev, ROGUE_CR_BIF_STATUS_MMU, 0,
regards,
dan carpenter
[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: [PATCH] drm/bridge: nxp-ptn3460: fix i2c_master_send() error checking
On Tue, Dec 05, 2023 at 03:04:49PM +0100, Robert Foss wrote: > On Tue, Dec 5, 2023, 15:01 Dan Carpenter wrote: > > > On Tue, Dec 05, 2023 at 02:48:26PM +0100, Robert Foss wrote: > > > On Mon, 4 Dec 2023 15:29:00 +0300, Dan Carpenter wrote: > > > > The i2c_master_send/recv() functions return negative error codes or the > > > > number of bytes that were able to be sent/received. This code has > > > > two problems. 1) Instead of checking if all the bytes were sent or > > > > received, it checks that at least one byte was sent or received. > > > > 2) If there was a partial send/receive then we should return a negative > > > > error code but this code returns success. > > > > > > > > [...] > > > > > > Applied, thanks! > > > > > > [1/1] drm/bridge: nxp-ptn3460: fix i2c_master_send() error checking > > > https://cgit.freedesktop.org/drm/drm-misc/commit/?id=914437992876 > > > > > > > Wait. That was unexpected. Neil's review comments were correct. I was > > planning to send a v2 patch which was just a cleanup. > > > > Sorry Dan, I was too quick on the draw. Can you send a fixup and I'll apply > it too? > Sure. I will do that. regards, dan carpenter
Re: [PATCH] drm/bridge: nxp-ptn3460: fix i2c_master_send() error checking
On Tue, Dec 05, 2023 at 02:48:26PM +0100, Robert Foss wrote: > On Mon, 4 Dec 2023 15:29:00 +0300, Dan Carpenter wrote: > > The i2c_master_send/recv() functions return negative error codes or the > > number of bytes that were able to be sent/received. This code has > > two problems. 1) Instead of checking if all the bytes were sent or > > received, it checks that at least one byte was sent or received. > > 2) If there was a partial send/receive then we should return a negative > > error code but this code returns success. > > > > [...] > > Applied, thanks! > > [1/1] drm/bridge: nxp-ptn3460: fix i2c_master_send() error checking > https://cgit.freedesktop.org/drm/drm-misc/commit/?id=914437992876 > Wait. That was unexpected. Neil's review comments were correct. I was planning to send a v2 patch which was just a cleanup. regards, dan carpenter
Re: Kunit drm_test_check_plane_state: EXPECTATION FAILED at drivers/gpu/drm/tests/drm_plane_helper_test.c:123
On Tue, Dec 05, 2023 at 09:37:05AM +0100, Maxime Ripard wrote:
> Hi Naresh,
>
> Thanks for the report
>
> On Mon, Dec 04, 2023 at 11:05:36PM +0530, Naresh Kamboju wrote:
> > The Kunit drm_plane_helper failed on all devices running Linux next-20231204
> >
> > ## Test Regressions (compared to next-20231201)
> > * qemu-armv7, kunit and
> > * x86, kunit
> > - drm_test_check_invalid_plane_state_downscaling_invalid
> > - drm_test_check_invalid_plane_state_drm_plane_helper
> > - drm_test_check_invalid_plane_state_drm_test_check_invalid_plane_state
> > - drm_test_check_invalid_plane_state_positioning_invalid
> > - drm_test_check_invalid_plane_state_upscaling_invalid
> > - drm_test_check_plane_state_clipping_rotate_reflect
> > - drm_test_check_plane_state_clipping_simple
> > - drm_test_check_plane_state_downscaling
> > - drm_test_check_plane_state_drm_test_check_plane_state
> > - drm_test_check_plane_state_positioning_simple
> > - drm_test_check_plane_state_rounding1
> > - drm_test_check_plane_state_rounding2
> > - drm_test_check_plane_state_rounding3
> > - drm_test_check_plane_state_rounding4
> > - drm_test_check_plane_state_upscaling
>
> I found the source of failure to be f1e75da5364e ("drm/atomic: Loosen FB
> atomic checks").
>
> Fortunately for us, it's already been reverted yesterday for some
> unrelated reason, so it should be fixed in next-20231205 onward.
Sorry, that's a bummer that these patches were reverted. :( The whole
episode was a bit unfortunate...
Qualcom has been working on those patches for a year. They must not be
using kunit testing as part of their QC... It's some kind of
communication failure on our part.
Hopefully we can get this all sorted out and re-apply the patches soon.
regards,
dan carpenter
Re: [PATCH] drm/bridge: nxp-ptn3460: fix i2c_master_send() error checking
On Mon, Dec 04, 2023 at 02:53:05PM +0100, Neil Armstrong wrote:
> On 04/12/2023 13:29, Dan Carpenter wrote:
> > The i2c_master_send/recv() functions return negative error codes or the
> > number of bytes that were able to be sent/received. This code has
> > two problems. 1) Instead of checking if all the bytes were sent or
> > received, it checks that at least one byte was sent or received.
> > 2) If there was a partial send/receive then we should return a negative
> > error code but this code returns success.
> >
> > Fixes: a9fe713d7d45 ("drm/bridge: Add PTN3460 bridge driver")
> > Cc: sta...@vger.kernel.org
> > Signed-off-by: Dan Carpenter
> > ---
> > This is from static analysis and code review. It's always a concern
> > when you add stricter error handling that something will break.
> >
> > drivers/gpu/drm/bridge/nxp-ptn3460.c | 10 +-
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c
> > b/drivers/gpu/drm/bridge/nxp-ptn3460.c
> > index d81920227a8a..9b7eb8c669c1 100644
> > --- a/drivers/gpu/drm/bridge/nxp-ptn3460.c
> > +++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c
> > @@ -56,13 +56,13 @@ static int ptn3460_read_bytes(struct ptn3460_bridge
> > *ptn_bridge, char addr,
> > ret = i2c_master_send(ptn_bridge->client, , 1);
> > if (ret <= 0) {
> > DRM_ERROR("Failed to send i2c command, ret=%d\n", ret);
> > - return ret;
> > + return ret ?: -EIO;
> > }
> > ret = i2c_master_recv(ptn_bridge->client, buf, len);
> > - if (ret <= 0) {
> > + if (ret != len) {
>
> This is impossible, i2c_transfer_buffer_flags() returns len as-is if no
> error, so
> ret can only be negative or equal to len. The original code is right.
It works, but it's not "right". The <= 0 could be changed to < 0. The
"len" variable is EDID_LENGTH (128).
regards,
dan carpenter
[bug report] drm/amdgpu: Workaround to skip kiq ring test during ras gpu recovery
Hello Stanley.Yang,
The patch b1338a8e71ac: "drm/amdgpu: Workaround to skip kiq ring test
during ras gpu recovery" from Oct 17, 2023 (linux-next), leads to the
following Smatch static checker warning:
drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c:604 amdgpu_get_xgmi_hive()
warn: sleeping in atomic context
drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c
591 struct amdgpu_hive_info *amdgpu_get_xgmi_hive(struct amdgpu_device
*adev)
592 {
593 struct amdgpu_hive_info *hive = NULL;
594 int ret;
595
596 if (!adev->gmc.xgmi.hive_id)
597 return NULL;
598
599 if (adev->hive) {
600 kobject_get(>hive->kobj);
601 return adev->hive;
602 }
603
--> 604 mutex_lock(_mutex);
^^^
Shhh The mutexes are sleeping.
605
606 list_for_each_entry(hive, _hive_list, node) {
The caller is amdgpu_gfx_disable_kcq():
drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
516 spin_lock(>ring_lock);
^^^
Holding a spin lock.
517 if (amdgpu_ring_alloc(kiq_ring, kiq->pmf->unmap_queues_size *
518 adev->gfx.num_compute_rings)) {
519 spin_unlock(>ring_lock);
520 return -ENOMEM;
521 }
522
523 for (i = 0; i < adev->gfx.num_compute_rings; i++) {
524 j = i + xcc_id * adev->gfx.num_compute_rings;
525 kiq->pmf->kiq_unmap_queues(kiq_ring,
526 >gfx.compute_ring[j],
527 RESET_QUEUES, 0, 0);
528 }
529
530 /**
531 * This is workaround: only skip kiq_ring test
532 * during ras recovery in suspend stage for gfx9.4.3
533 */
534 hive = amdgpu_get_xgmi_hive(adev);
^^
Can't call a sleeping function when holding a spin_lock.
535 if (hive) {
536 hive_ras_recovery = atomic_read(>ras_recovery);
537 amdgpu_put_xgmi_hive(hive);
538 }
539
540 ras = amdgpu_ras_get_context(adev);
541 if ((amdgpu_ip_version(adev, GC_HWIP, 0) == IP_VERSION(9, 4,
3)) &&
542 ras && (atomic_read(>in_recovery) ||
hive_ras_recovery)) {
543 spin_unlock(>ring_lock);
544 return 0;
545 }
546
547 if (kiq_ring->sched.ready && !adev->job_hang)
548 r = amdgpu_ring_test_helper(kiq_ring);
549 spin_unlock(>ring_lock);
regards,
dan carpenter
[PATCH] drm/bridge: nxp-ptn3460: fix i2c_master_send() error checking
The i2c_master_send/recv() functions return negative error codes or the
number of bytes that were able to be sent/received. This code has
two problems. 1) Instead of checking if all the bytes were sent or
received, it checks that at least one byte was sent or received.
2) If there was a partial send/receive then we should return a negative
error code but this code returns success.
Fixes: a9fe713d7d45 ("drm/bridge: Add PTN3460 bridge driver")
Cc: sta...@vger.kernel.org
Signed-off-by: Dan Carpenter
---
This is from static analysis and code review. It's always a concern
when you add stricter error handling that something will break.
drivers/gpu/drm/bridge/nxp-ptn3460.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c
b/drivers/gpu/drm/bridge/nxp-ptn3460.c
index d81920227a8a..9b7eb8c669c1 100644
--- a/drivers/gpu/drm/bridge/nxp-ptn3460.c
+++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c
@@ -56,13 +56,13 @@ static int ptn3460_read_bytes(struct ptn3460_bridge
*ptn_bridge, char addr,
ret = i2c_master_send(ptn_bridge->client, , 1);
if (ret <= 0) {
DRM_ERROR("Failed to send i2c command, ret=%d\n", ret);
- return ret;
+ return ret ?: -EIO;
}
ret = i2c_master_recv(ptn_bridge->client, buf, len);
- if (ret <= 0) {
+ if (ret != len) {
DRM_ERROR("Failed to recv i2c data, ret=%d\n", ret);
- return ret;
+ return ret < 0 ? ret : -EIO;
}
return 0;
@@ -78,9 +78,9 @@ static int ptn3460_write_byte(struct ptn3460_bridge
*ptn_bridge, char addr,
buf[1] = val;
ret = i2c_master_send(ptn_bridge->client, buf, ARRAY_SIZE(buf));
- if (ret <= 0) {
+ if (ret != ARRAY_SIZE(buf)) {
DRM_ERROR("Failed to send i2c command, ret=%d\n", ret);
- return ret;
+ return ret < 0 ? ret : -EIO;
}
return 0;
--
2.42.0
[PATCH] drm/imagination: fix off by one in pvr_vm_mips_init() error handling
If the call to vmap() fails the "page_nr" is one element beyond the end
of the mips_data->pt_dma_addr[] and mips_data->pt_pages[] arrays.
The way that this is traditionally written is that we clean up the
partial loop iteration before the goto and then we can say
while (--i >= 0). At that point we know that all the elements thus
far are initialized so we don't need to have NULL checks.
Fixes: 927f3e0253c1 ("drm/imagination: Implement MIPS firmware processor and
MMU support")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/imagination/pvr_vm_mips.c | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/imagination/pvr_vm_mips.c
b/drivers/gpu/drm/imagination/pvr_vm_mips.c
index 7268cf6e630b..2bc7181a4c3e 100644
--- a/drivers/gpu/drm/imagination/pvr_vm_mips.c
+++ b/drivers/gpu/drm/imagination/pvr_vm_mips.c
@@ -57,6 +57,7 @@ pvr_vm_mips_init(struct pvr_device *pvr_dev)
PAGE_SIZE,
DMA_TO_DEVICE);
if (dma_mapping_error(dev, mips_data->pt_dma_addr[page_nr])) {
err = -ENOMEM;
+ __free_page(mips_data->pt_pages[page_nr]);
goto err_free_pages;
}
}
@@ -79,13 +80,11 @@ pvr_vm_mips_init(struct pvr_device *pvr_dev)
return 0;
err_free_pages:
- for (; page_nr >= 0; page_nr--) {
- if (mips_data->pt_dma_addr[page_nr])
- dma_unmap_page(from_pvr_device(pvr_dev)->dev,
- mips_data->pt_dma_addr[page_nr],
PAGE_SIZE, DMA_TO_DEVICE);
+ while (--page_nr >= 0) {
+ dma_unmap_page(from_pvr_device(pvr_dev)->dev,
+ mips_data->pt_dma_addr[page_nr], PAGE_SIZE,
DMA_TO_DEVICE);
- if (mips_data->pt_pages[page_nr])
- __free_page(mips_data->pt_pages[page_nr]);
+ __free_page(mips_data->pt_pages[page_nr]);
}
return err;
--
2.42.0
[bug report] drm/imagination: Implement firmware infrastructure and META FW support
Hello Sarah Walker, The patch cc1aeedb98ad: "drm/imagination: Implement firmware infrastructure and META FW support" from Nov 22, 2023 (linux-next), leads to the following Smatch static checker warning: drivers/gpu/drm/imagination/pvr_ccb.c:277 pvr_kccb_send_cmd_reserved_powered() warn: odd binop '0x0 & 0xf' drivers/gpu/drm/imagination/pvr_ccb.c 268 WRITE_ONCE(pvr_dev->kccb.rtn[old_write_offset], 269ROGUE_FWIF_KCCB_RTN_SLOT_NO_RESPONSE); 270 } 271 mb(); /* memory barrier */ 272 WRITE_ONCE(ctrl->write_offset, new_write_offset); 273 pvr_dev->kccb.reserved_count--; 274 275 /* Kick MTS */ 276 pvr_fw_mts_schedule(pvr_dev, --> 277 PVR_FWIF_DM_GP & ~ROGUE_CR_MTS_SCHEDULE_DM_CLRMSK); ^^ PVR_FWIF_DM_GP is zero. 278 279 out_unlock: 280 mutex_unlock(_ccb->lock); 281 } regards, dan carpenter
[PATCH 2/2] drm/imagination: Fix IS_ERR() vs NULL bug in pvr_request_firmware()
The pvr_build_firmware_filename() function returns NULL on error. It
doesn't return error pointers.
Fixes: f99f5f3ea7ef ("drm/imagination: Add GPU ID parsing and firmware loading")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/imagination/pvr_device.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/imagination/pvr_device.c
b/drivers/gpu/drm/imagination/pvr_device.c
index e1dcc4e42087..5389aea7ff21 100644
--- a/drivers/gpu/drm/imagination/pvr_device.c
+++ b/drivers/gpu/drm/imagination/pvr_device.c
@@ -286,8 +286,8 @@ pvr_request_firmware(struct pvr_device *pvr_dev)
filename = pvr_build_firmware_filename(pvr_dev, "powervr/rogue",
PVR_FW_VERSION_MAJOR);
- if (IS_ERR(filename))
- return PTR_ERR(filename);
+ if (!filename)
+ return -ENOMEM;
/*
* This function takes a copy of , meaning we can free our
--
2.42.0
[PATCH 1/2] drm/imagination: Fix error codes in pvr_device_clk_init()
There is a cut and paste error so this code returns the wrong variable.
Fixes: 1f88f017e649 ("drm/imagination: Get GPU resources")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/imagination/pvr_device.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/imagination/pvr_device.c
b/drivers/gpu/drm/imagination/pvr_device.c
index 8499becf4fbb..e1dcc4e42087 100644
--- a/drivers/gpu/drm/imagination/pvr_device.c
+++ b/drivers/gpu/drm/imagination/pvr_device.c
@@ -105,12 +105,12 @@ static int pvr_device_clk_init(struct pvr_device *pvr_dev)
sys_clk = devm_clk_get_optional(drm_dev->dev, "sys");
if (IS_ERR(sys_clk))
- return dev_err_probe(drm_dev->dev, PTR_ERR(core_clk),
+ return dev_err_probe(drm_dev->dev, PTR_ERR(sys_clk),
"failed to get sys clock\n");
mem_clk = devm_clk_get_optional(drm_dev->dev, "mem");
if (IS_ERR(mem_clk))
- return dev_err_probe(drm_dev->dev, PTR_ERR(core_clk),
+ return dev_err_probe(drm_dev->dev, PTR_ERR(mem_clk),
"failed to get mem clock\n");
pvr_dev->core_clk = core_clk;
--
2.42.0
Re: [bug report] backlight: mp3309c: Add support for MPS MP3309C
On Wed, Nov 29, 2023 at 11:12:29AM +, Flavio Suligoi wrote: > Hi Dan, > > Can I add the "Reported-by" tag, with your name, in my 2nd vers of > the commit to fix this bug? Yeah. Thanks! If the bug report is sent to a public mailing list then there is no need to ask. regards, dan carpenter
[bug report] backlight: mp3309c: Add support for MPS MP3309C
>default_brightness);
292 if (ret)
293 pdata->default_brightness = pdata->max_brightness;
294 if (pdata->default_brightness > pdata->max_brightness) {
295 dev_err(chip->dev,
296 "default brightness exceeds max brightness\n");
297 pdata->default_brightness = pdata->max_brightness;
298 }
299
300 /*
regards,
dan carpenter
[PATCH] nouveau/gsp/r535: remove a stray unlock in r535_gsp_rpc_send()
This unlock doesn't belong here and it leads to a double unlock in
the caller, r535_gsp_rpc_push().
Fixes: 176fdcbddfd2 ("drm/nouveau/gsp/r535: add support for booting GSP-RM")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
index dc44f5c7833f..818e5c73b7a6 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
@@ -365,10 +365,8 @@ r535_gsp_rpc_send(struct nvkm_gsp *gsp, void *argv, bool
wait, u32 repc)
}
ret = r535_gsp_cmdq_push(gsp, rpc);
- if (ret) {
- mutex_unlock(>cmdq.mutex);
+ if (ret)
return ERR_PTR(ret);
- }
if (wait) {
msg = r535_gsp_msg_recv(gsp, fn, repc);
--
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: [RFC] drm/tests: annotate intentional stack trace in drm_test_rect_calc_hscale()
On Mon, Nov 06, 2023 at 02:58:12PM +0100, mrip...@kernel.org wrote: > > But a similar thing is happening here where we have so many bogus > > warnings that we missed a real bug. > > IIRC, there was a similar discussion for lockdep issues. IMO, any > (unintended) warning should trigger a test failure. > > I guess that would require adding some intrumentation to __WARN somehow, > and also allowing tests to check whether a warning had been generated > during their execution for tests that want to trigger one. I think this is a good idea. I was looking at how lockdep prints warnings (see print_circular_bug_header()). It doesn't use WARN() it prints a bunch of pr_warn() statements and then a stack trace. We would have to have a increment the counter manually in that situation. I'm writing a script to parse a dmesg and collect Oopses. So now I know to look for WARN(), lockdep, and KASAN. What other bugs formats do we have? Probably someone like the syzbot devs have already has written a script like this? regards, dan carpenter
[PATCH] nouveau/gsp/r535: Fix a NULL vs error pointer bug
The r535_gsp_cmdq_get() function returns error pointers but this code
checks for NULL. Also we need to propagate the error pointer back to
the callers in r535_gsp_rpc_get(). Returning NULL will lead to a NULL
pointer dereference.
Fixes: 176fdcbddfd2 ("drm/nouveau/gsp/r535: add support for booting GSP-RM")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
index e31f9641114b..f8409e2f9fef 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
@@ -689,8 +689,8 @@ r535_gsp_rpc_get(struct nvkm_gsp *gsp, u32 fn, u32 argc)
struct nvfw_gsp_rpc *rpc;
rpc = r535_gsp_cmdq_get(gsp, ALIGN(sizeof(*rpc) + argc, sizeof(u64)));
- if (!rpc)
- return NULL;
+ if (IS_ERR(rpc))
+ return ERR_CAST(rpc);
rpc->header_version = 0x0300;
rpc->signature = ('C' << 24) | ('P' << 16) | ('R' << 8) | 'V';
--
2.39.2
[PATCH] nouveau/gsp/r535: uninitialized variable in r535_gsp_acpi_mux_id()
The if we hit the "continue" statement on the first iteration through
the loop then "handle_mux" needs to be set to NULL so we continue
looping.
Fixes: 176fdcbddfd2 ("drm/nouveau/gsp/r535: add support for booting GSP-RM")
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
index e31f9641114b..afa8e7377a76 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
@@ -1159,7 +1159,7 @@ static void
r535_gsp_acpi_mux_id(acpi_handle handle, u32 id, MUX_METHOD_DATA_ELEMENT *mode,
MUX_METHOD_DATA_ELEMENT *part)
{
- acpi_handle iter = NULL, handle_mux;
+ acpi_handle iter = NULL, handle_mux = NULL;
acpi_status status;
unsigned long long value;
--
2.42.0
[PATCH] drm/rect: only print the stack trace in drm_calc_scale() once
The kunit test triggers this stack trace deliberately. The
drm_rect_scale_cases[] test cases include a negative src and a negative
dst. This ends up spamming the logs and can obscure real warnings.
Just print the warning once.
Reported-by: Linux Kernel Functional Testing
Closes:
https://lore.kernel.org/all/CA+G9fYuA643RHHpPnz9Ww7rr3zV5a0y=7_uFcybBSL=qp_s...@mail.gmail.com/
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/drm_rect.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c
index 85c79a38c13a..a992d70b928b 100644
--- a/drivers/gpu/drm/drm_rect.c
+++ b/drivers/gpu/drm/drm_rect.c
@@ -135,7 +135,7 @@ static int drm_calc_scale(int src, int dst)
{
int scale = 0;
- if (WARN_ON(src < 0 || dst < 0))
+ if (WARN_ON_ONCE(src < 0 || dst < 0))
return -EINVAL;
if (dst == 0)
--
2.42.0
Re: [RFC] drm/tests: annotate intentional stack trace in drm_test_rect_calc_hscale()
Let me add Richard to the CC list. See lore for more details. https://lore.kernel.org/all/CA+G9fYuA643RHHpPnz9Ww7rr3zV5a0y=7_uFcybBSL=qp_s...@mail.gmail.com/ On Tue, Oct 31, 2023 at 09:57:48PM +0530, Naresh Kamboju wrote: > On Mon, 30 Oct 2023 at 14:33, Dan Carpenter wrote: > > > > We have started printing more and more intentional stack traces. Whether > > it's testing KASAN is able to detect use after frees or it's part of a > > kunit test. > > > > These stack traces can be problematic. They suddenly show up as a new > > failure. Now the test team has to contact the developers. A bunch of > > people have to investigate the bug. We finally decide that it's > > intentional so now the test team has to update their filter scripts to > > mark it as intentional. These filters are ad-hoc because there is no > > standard format for warnings. > > > > A better way would be to mark it as intentional from the start. > > > > Here, I have marked the beginning and the end of the trace. It's more > > tricky for things like lkdtm_FORTIFY_MEM_MEMBER() where the flow doesn't > > reach the end of the function. I guess I would print a different > > warning for stack traces that can't have a > > "Intentional warning finished\n" message at the end. > > > > I haven't actually tested this patch... Daniel, do you have a > > list of intentional stack traces we could annotate? > > [My two cents] > > I have been noticing following kernel warnings / BUGs Some are intentional and some are not. I had a similar thing happen to me last week where I had too many Smatch false positives in my devel code so I accidentally sent a patch with a stupid bug. I've since updated my QC process to run both the devel and released versions of Smatch. But a similar thing is happening here where we have so many bogus warnings that we missed a real bug. > These are starting happening from next-20231009. > I am not sure which are "Intentional warnings" or real regressions. > > [ 37.378220] BUG: KASAN: slab-out-of-bounds in kmalloc_oob_right+0xc4/0x300 > [ 37.645506] BUG: KASAN: slab-out-of-bounds in kmalloc_oob_right+0xec/0x300 > .. > [ 632.407425] BUG: KASAN: null-ptr-deref in kobject_namespace+0x3c/0xb0 > > > Logs: [Sorry for sharing long logs ] Not your fault. These long warnings are the issue at hand. > == > > [ cut here ] > [ 629.699281] WARNING: CPU: 0 PID: 2834 at > drivers/gpu/drm/drm_rect.c:138 drm_rect_calc_hscale+0xbc/0xe8 Deliberate. > [ 629.914458] WARNING: CPU: 5 PID: 2836 at > drivers/gpu/drm/drm_rect.c:138 drm_rect_calc_hscale+0xbc/0xe8 > [drm_kms_helper] Deliberate. > [ 630.172564] WARNING: CPU: 5 PID: 2846 at > drivers/gpu/drm/drm_rect.c:138 drm_rect_calc_vscale+0xbc/0xe8 > [drm_kms_helper] Deliberate. > [ cut here ] > [ 630.388003] WARNING: CPU: 3 PID: 2848 at > drivers/gpu/drm/drm_rect.c:138 drm_rect_calc_vscale+0xbc/0xe8 > [drm_kms_helper] Deliberate. > [ cut here ] > [ 631.679963] kobject: '(null)' (f512f33b): is not > initialized, yet kobject_get() is being called. Not deliberate. This seems like a straight forward bug to fix. Failing a kobject_get() seems like it would obviously lead to a refcounting underflow and a use after free so I suspect some of the other warnings that follow are caused by this issue. We should fix it first and see which warnings disappear. So testing the Linux Kernel Dump Test Module is always going to create warnings. So intentional warnings are a part of life. We should annotate them. But having too many warnings is bad and has caused this kobject_get() bug. We should delete the warning in drm_calc_scale() or make it a WARN_ONCE() and mark it as intentional in the kunit test. regards, dan carpenter
[RFC] drm/tests: annotate intentional stack trace in drm_test_rect_calc_hscale()
We have started printing more and more intentional stack traces. Whether
it's testing KASAN is able to detect use after frees or it's part of a
kunit test.
These stack traces can be problematic. They suddenly show up as a new
failure. Now the test team has to contact the developers. A bunch of
people have to investigate the bug. We finally decide that it's
intentional so now the test team has to update their filter scripts to
mark it as intentional. These filters are ad-hoc because there is no
standard format for warnings.
A better way would be to mark it as intentional from the start.
Here, I have marked the beginning and the end of the trace. It's more
tricky for things like lkdtm_FORTIFY_MEM_MEMBER() where the flow doesn't
reach the end of the function. I guess I would print a different
warning for stack traces that can't have a
"Intentional warning finished\n" message at the end.
I haven't actually tested this patch... Daniel, do you have a
list of intentional stack traces we could annotate?
Signed-off-by: Dan Carpenter
---
drivers/gpu/drm/tests/drm_rect_test.c | 2 ++
include/kunit/test.h | 3 +++
2 files changed, 5 insertions(+)
diff --git a/drivers/gpu/drm/tests/drm_rect_test.c
b/drivers/gpu/drm/tests/drm_rect_test.c
index 76332cd2ead8..367738254493 100644
--- a/drivers/gpu/drm/tests/drm_rect_test.c
+++ b/drivers/gpu/drm/tests/drm_rect_test.c
@@ -409,8 +409,10 @@ static void drm_test_rect_calc_hscale(struct kunit *test)
const struct drm_rect_scale_case *params = test->param_value;
int scaling_factor;
+ START_INTENTIONAL_WARNING();
scaling_factor = drm_rect_calc_hscale(>src, >dst,
params->min_range,
params->max_range);
+ END_INTENTIONAL_WARNING();
KUNIT_EXPECT_EQ(test, scaling_factor, params->expected_scaling_factor);
}
diff --git a/include/kunit/test.h b/include/kunit/test.h
index 20ed9f9275c9..1f01d4c81055 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -337,6 +337,9 @@ void __kunit_test_suites_exit(struct kunit_suite **suites,
int num_suites);
void kunit_exec_run_tests(struct kunit_suite_set *suite_set, bool builtin);
void kunit_exec_list_tests(struct kunit_suite_set *suite_set, bool
include_attr);
+#define START_INTENTIONAL_WARNING() pr_info("Triggering a stack trace\n")
+#define END_INTENTIONAL_WARNING() pr_info("Intentional warning finished\n")
+
#if IS_BUILTIN(CONFIG_KUNIT)
int kunit_run_all_tests(void);
#else
--
2.42.0
Re: [bug report] drm: Warn about negative sizes when calculating scale factor
On Fri, Oct 20, 2023 at 05:11:24PM +0300, Dan Carpenter wrote:
> On Fri, Oct 20, 2023 at 02:55:37PM +0300, Ville Syrjälä wrote:
> > On Fri, Oct 20, 2023 at 02:39:04PM +0300, Dan Carpenter wrote:
> > > On Wed, Oct 18, 2023 at 05:17:42PM +0300, Dan Carpenter wrote:
> > > > drivers/gpu/drm/drm_rect.c
> > > >134 static int drm_calc_scale(int src, int dst)
> > > >135 {
> > > >136 int scale = 0;
> > > >137
> > > >138 if (WARN_ON(src < 0 || dst < 0))
> > > > ^^^
> > > > These days, with automated fuzz testing, this WARN_ON() is problematic.
> > > > WARN() is considered a kernel bug, and pr_warn() is the hip new way to
> > > > alert the user about issues.
> > >
> > > Btw, a lot of people (Greg claims it's the majority of Linux users)
> > > these days have run kernels with panic on warn enabled so that's another
> > > reason to avoid using WARN_ON() for stuff that it known to be possible.
> >
> > This is not possible, unless there is a serious bug somewhere else.
>
> Ah. That's fine then. This is kunit which is deliberately triggering
> the WARN_ON(). The KASAN testing also deliberately triggers WARN_ON()s
> so it's a necessary thing.
>
> I just wonder if there is some way to mark these kinds of warnings as
> expected. Perhaps we could add a comment in the kunit test?
Is there some kind of tool which looks at a dmesg and can separate out
the intentional stack traces from the others?
regards,
dan carpenter
[PATCH 2/2] fbdev/imsttfb: fix a resource leak in probe
I've re-written the error handling but the bug is that if init_imstt()
fails we need to call iounmap(par->cmap_regs).
Fixes: c75f5a550610 ("fbdev: imsttfb: Fix use after free bug in imsttfb_probe")
Signed-off-by: Dan Carpenter
---
drivers/video/fbdev/imsttfb.c | 29 -
1 file changed, 16 insertions(+), 13 deletions(-)
diff --git a/drivers/video/fbdev/imsttfb.c b/drivers/video/fbdev/imsttfb.c
index acb943f85700..660499260f46 100644
--- a/drivers/video/fbdev/imsttfb.c
+++ b/drivers/video/fbdev/imsttfb.c
@@ -1496,8 +1496,8 @@ static int imsttfb_probe(struct pci_dev *pdev, const
struct pci_device_id *ent)
if (!request_mem_region(addr, size, "imsttfb")) {
printk(KERN_ERR "imsttfb: Can't reserve memory region\n");
- framebuffer_release(info);
- return -ENODEV;
+ ret = -ENODEV;
+ goto release_info;
}
switch (pdev->device) {
@@ -1514,36 +1514,39 @@ static int imsttfb_probe(struct pci_dev *pdev, const
struct pci_device_id *ent)
printk(KERN_INFO "imsttfb: Device 0x%x unknown, "
"contact maintainer.\n", pdev->device);
ret = -ENODEV;
- goto error;
+ goto release_mem_region;
}
info->fix.smem_start = addr;
info->screen_base = (__u8 *)ioremap(addr, par->ramdac == IBM ?
0x40 : 0x80);
if (!info->screen_base)
- goto error;
+ goto release_mem_region;
info->fix.mmio_start = addr + 0x80;
par->dc_regs = ioremap(addr + 0x80, 0x1000);
if (!par->dc_regs)
- goto error;
+ goto unmap_screen_base;
par->cmap_regs_phys = addr + 0x84;
par->cmap_regs = (__u8 *)ioremap(addr + 0x84, 0x1000);
if (!par->cmap_regs)
- goto error;
+ goto unmap_dc_regs;
info->pseudo_palette = par->palette;
ret = init_imstt(info);
if (ret)
- goto error;
+ goto unmap_cmap_regs;
pci_set_drvdata(pdev, info);
- return ret;
+ return 0;
-error:
- if (par->dc_regs)
- iounmap(par->dc_regs);
- if (info->screen_base)
- iounmap(info->screen_base);
+unmap_cmap_regs:
+ iounmap(par->cmap_regs);
+unmap_dc_regs:
+ iounmap(par->dc_regs);
+unmap_screen_base:
+ iounmap(info->screen_base);
+release_mem_region:
release_mem_region(addr, size);
+release_info:
framebuffer_release(info);
return ret;
}
--
2.42.0
[PATCH 1/2] fbdev/imsttfb: fix double free in probe()
The init_imstt() function calls framebuffer_release() on error and then
the probe() function calls it again. It should only be done in probe.
Fixes: 518ecb6a209f ("fbdev: imsttfb: Fix error path of imsttfb_probe()")
Signed-off-by: Dan Carpenter
---
drivers/video/fbdev/imsttfb.c | 6 +-
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/video/fbdev/imsttfb.c b/drivers/video/fbdev/imsttfb.c
index e7e03e920729..acb943f85700 100644
--- a/drivers/video/fbdev/imsttfb.c
+++ b/drivers/video/fbdev/imsttfb.c
@@ -1421,7 +1421,6 @@ static int init_imstt(struct fb_info *info)
if ((info->var.xres * info->var.yres) * (info->var.bits_per_pixel >> 3)
> info->fix.smem_len
|| !(compute_imstt_regvals(par, info->var.xres, info->var.yres))) {
printk("imsttfb: %ux%ux%u not supported\n", info->var.xres,
info->var.yres, info->var.bits_per_pixel);
- framebuffer_release(info);
return -ENODEV;
}
@@ -1453,14 +1452,11 @@ static int init_imstt(struct fb_info *info)
FBINFO_HWACCEL_FILLRECT |
FBINFO_HWACCEL_YPAN;
- if (fb_alloc_cmap(>cmap, 0, 0)) {
- framebuffer_release(info);
+ if (fb_alloc_cmap(>cmap, 0, 0))
return -ENODEV;
- }
if (register_framebuffer(info) < 0) {
fb_dealloc_cmap(>cmap);
- framebuffer_release(info);
return -ENODEV;
}
--
2.42.0
Re: [PATCH v2] vga_switcheroo: Fix impossible judgment condition
On Thu, Oct 26, 2023 at 05:18:04PM +0800, Su Hui wrote:
> 'id' is enum type like unsigned int, so it will never be less than zero.
> It's better to check VGA_SWITCHEROO_UNKNOWN_ID too.
>
> Fixes: 4aaf448fa975 ("vga_switcheroo: set audio client id according to bound
> GPU id")
> Signed-off-by: Su Hui
> ---
> v2:
> - add check of VGA_SWITCHEROO_UNKNOWN_ID(Dan's suggestion).
>
> By the way, all functions of 'get_client_id' will never return error code
> or VGA_SWITCHEROO_UNKNOWN_ID，should we remove this check or keep it for
> future.
>
Thanks!
Reviewed-by: Dan Carpenter
regards,
dan carpenter
