Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] drm/appletbdrm: memory leak of suspend_state on unbind - [Medium] drm/appletbdrm: missing reset_resume handler breaks reset recovery --
commit c3fac1e92efd5498a32a50fb4e9a862ae7b24e77 Author: Vasilij Markin <[email protected]> drm/appletbdrm: Add suspend and resume support The driver has no USB power management callbacks, so the USB core unbinds it before system suspend and binds it again on resume. The DRM device is unregistered and registered again on every suspend and resume cycle, userspace driving the Touch Bar through it (such as tiny-dfr) loses its device, and the Touch Bar stays dark after resume. > Implement suspend and resume with drm_mode_config_helper_suspend() and > drm_mode_config_helper_resume(), so the device stays registered and the > last committed state is restored on resume. If the device was reset > while suspended, repeat the probe-time handshake first. [Severity: Medium] Will the device actually stay registered if it is reset while suspended? According to the Linux USB core logic, if a device is reset during suspend and the driver lacks a .reset_resume callback, the USB core sets needs_binding = 1, which forcefully unbinds and rebinds the driver. This fallback unbinds the DRM device, destroying the userspace connection and defeating the stated goal for the reset scenario. > diff --git a/drivers/gpu/drm/tiny/appletbdrm.c > b/drivers/gpu/drm/tiny/appletbdrm.c > index 4297d9c1125e4..919ca14181af3 100644 > --- a/drivers/gpu/drm/tiny/appletbdrm.c > +++ b/drivers/gpu/drm/tiny/appletbdrm.c [ ... ] > @@ -821,6 +822,20 @@ static void appletbdrm_shutdown(struct usb_interface > *intf) > drm_atomic_helper_shutdown(&adev->drm); > } > > +static int appletbdrm_suspend(struct usb_interface *intf, pm_message_t > message) > +{ > + struct appletbdrm_device *adev = usb_get_intfdata(intf); > + > + return drm_mode_config_helper_suspend(&adev->drm); [Severity: Medium] Does this code leak adev->drm.mode_config.suspend_state if the device is unbound while suspended? drm_mode_config_helper_suspend() allocates an atomic state and stores it in adev->drm.mode_config.suspend_state. If the device is reset during suspend, the USB core forcefully unbinds the driver because it lacks a reset_resume handler. The unbind path calls appletbdrm_disconnect() and tears down the DRM device. Neither appletbdrm_disconnect() nor the DRM core cleanup routines free suspend_state. Does this cause a memory leak of the atomic commit object and all its duplicated plane/CRTC states on every suspend/reset cycle? > +} > + > +static int appletbdrm_resume(struct usb_interface *intf) [ ... ] > @@ -832,6 +847,8 @@ static struct usb_driver appletbdrm_usb_driver = { > .probe = appletbdrm_probe, > .disconnect = appletbdrm_disconnect, > .shutdown = appletbdrm_shutdown, > + .suspend = appletbdrm_suspend, > + .resume = appletbdrm_resume, > .id_table = appletbdrm_usb_id_table, > }; [Severity: Medium] Is the .reset_resume callback missing here? The commit message states: "If the device was reset while suspended, repeat the probe-time handshake first" to ensure the device stays registered. However, intentionally omitting the .reset_resume callback causes the USB core to unbind the driver on reset instead of restoring the DRM state. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
