Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [Medium] The commit message inaccurately claims the patch uses 'encoder atomic helpers directly', but no atomic helpers are attached. Pre-existing issues: - [High] The return value of `drm_encoder_init()` is ignored, which can lead to a NULL pointer dereference and kernel panic if initialization fails. - [High] The sleeping function `msleep()` is called within the `zynqmp_dpsub_crtc_atomic_enable()` callback, which executes in an atomic context path. -- --- Patch [1]: [PATCH 1/2] drm/xlnx/zynqmp_dpsub: remove dependency on DRM simple helpers --- commit 3da90995bfda87eb7ff24a962cee94ae68627efd Author: Diogo Silva <[email protected]> drm/xlnx/zynqmp_dpsub: remove dependency on DRM simple helpers Simple KMS helper are deprecated since they only add an intermediate layer between drivers and the atomic modesetting. This patch removes the drm_simple_encoder_init() helper usage in the xlnx zynqmp_dpsub driver by open coding it and using the encoder atomic helpers directly. This is a step to eventually get rid of this simple KMS helper, once all drivers that use it have been converted. > This patch removes the drm_simple_encoder_init() helper usage in the > xlnx zynqmp_dpsub driver by open coding it and using the encoder atomic > helpers directly. [Severity: Medium] The commit message states that the patch uses "encoder atomic helpers directly". Is this description accurate? The patch only defines a basic drm_encoder_funcs structure for cleanup and does not attach any atomic helpers (e.g., via drm_encoder_helper_add()). > diff --git a/drivers/gpu/drm/xlnx/zynqmp_kms.c > b/drivers/gpu/drm/xlnx/zynqmp_kms.c > index d5f9224505655..698ac692abce9 100644 > --- a/drivers/gpu/drm/xlnx/zynqmp_kms.c > +++ b/drivers/gpu/drm/xlnx/zynqmp_kms.c [ ... ] > @@ -436,7 +439,8 @@ static int zynqmp_dpsub_kms_init(struct zynqmp_dpsub > *dpsub) > > /* Create the encoder and attach the bridge. */ > encoder->possible_crtcs |= drm_crtc_mask(&dpsub->drm->crtc); > - drm_simple_encoder_init(&dpsub->drm->dev, encoder, > DRM_MODE_ENCODER_NONE); > + drm_encoder_init(&dpsub->drm->dev, encoder, &zynqmp_dpsub_encoder_funcs, > + DRM_MODE_ENCODER_NONE, NULL); [Severity: High] This is a pre-existing issue, but the return value of drm_encoder_init() is ignored here in zynqmp_dpsub_kms_init(). If drm_encoder_init() fails, it returns an error code without fully initializing the encoder structure, including its bridge_chain_mutex and bridge_chain list head. > > ret = drm_bridge_attach(encoder, dpsub->bridge, NULL, > DRM_BRIDGE_ATTACH_NO_CONNECTOR); Because the return value is not checked, execution blindly continues to drm_bridge_attach(), which will attempt to lock the uninitialized mutex and call list_add() on the uninitialized list head. Can this lead to a NULL pointer dereference and kernel panic? While reviewing this file, I noticed another potential issue in zynqmp_dpsub_crtc_atomic_enable(): [Severity: High] This isn't a bug introduced by this patch, but does this code sleep in an atomic context path? drivers/gpu/drm/xlnx/zynqmp_kms.c:zynqmp_dpsub_crtc_atomic_enable() { ... msleep(3 * 1000 / vrefresh); ... } Calling sleeping functions like msleep() from CRTC atomic enable callbacks can lead to kernel warnings and potential deadlocks. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
