Use local wq in order to avoid flush_scheduled_work() usage.
Signed-off-by: Tetsuo Handa penguin-kernel@I-love.SAKURA.ne.jp --- Please see commit c4f135d643823a86 ("workqueue: Wrap flush_workqueue() using a macro") for background.
This is a blind conversion, and is only compile tested.
.../drm/bridge/cadence/cdns-mhdp8546-core.c | 30 ++++++++++++++++--- .../drm/bridge/cadence/cdns-mhdp8546-core.h | 2 ++ .../drm/bridge/cadence/cdns-mhdp8546-hdcp.c | 16 +++++----- 3 files changed, 36 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c index 67f0f444b4e8..1d7c6af66217 100644 --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c @@ -53,6 +53,8 @@ #include "cdns-mhdp8546-hdcp.h" #include "cdns-mhdp8546-j721e.h"
+struct workqueue_struct *cadence_mhdp_wq; + static int cdns_mhdp_mailbox_read(struct cdns_mhdp_device *mhdp) { int ret, empty; @@ -2049,7 +2051,7 @@ static void cdns_mhdp_atomic_enable(struct drm_bridge *bridge, out: mutex_unlock(&mhdp->link_mutex); if (ret < 0) - schedule_work(&mhdp->modeset_retry_work); + queue_work(cadence_mhdp_wq, &mhdp->modeset_retry_work); }
static void cdns_mhdp_atomic_disable(struct drm_bridge *bridge, @@ -2373,7 +2375,7 @@ static irqreturn_t cdns_mhdp_irq_handler(int irq, void *data) spin_unlock(&mhdp->start_lock);
if (bridge_attached && (sw_ev0 & CDNS_DPTX_HPD)) { - schedule_work(&mhdp->hpd_work); + queue_work(cadence_mhdp_wq, &mhdp->hpd_work); }
if (sw_ev0 & ~CDNS_DPTX_HPD) { @@ -2413,7 +2415,7 @@ static void cdns_mhdp_hpd_work(struct work_struct *work) ret = cdns_mhdp_update_link_status(mhdp); if (mhdp->connector.dev) { if (ret < 0) - schedule_work(&mhdp->modeset_retry_work); + queue_work(cadence_mhdp_wq, &mhdp->modeset_retry_work); else drm_kms_helper_hotplug_event(mhdp->bridge.dev); } else { @@ -2632,7 +2634,27 @@ static struct platform_driver mhdp_driver = { .probe = cdns_mhdp_probe, .remove = cdns_mhdp_remove, }; -module_platform_driver(mhdp_driver); + +static int __init mhdp_init(void) +{ + int ret; + + cadence_mhdp_wq = alloc_workqueue("cadence_mhdp_wq", 0, 0); + if (!cadence_mhdp_wq) + return -ENOMEM; + ret = platform_driver_register(&mhdp_driver); + if (ret) + destroy_workqueue(cadence_mhdp_wq); + return ret; +} +module_init(mhdp_init); + +static void __exit mhdp_exit(void) +{ + platform_driver_unregister(&mhdp_driver); + destroy_workqueue(cadence_mhdp_wq); +} +module_exit(mhdp_exit);
MODULE_FIRMWARE(FW_NAME);
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h index bedddd510d17..e6c475612480 100644 --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h @@ -419,4 +419,6 @@ struct cdns_mhdp_device {
u32 cdns_mhdp_wait_for_sw_event(struct cdns_mhdp_device *mhdp, uint32_t event);
+extern struct workqueue_struct *cadence_mhdp_wq; + #endif diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c index 946212a95598..09f9e3d42f11 100644 --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c @@ -449,14 +449,14 @@ static int cdns_mhdp_hdcp_check_link(struct cdns_mhdp_device *mhdp) ret = _cdns_mhdp_hdcp_disable(mhdp); if (ret) { mhdp->hdcp.value = DRM_MODE_CONTENT_PROTECTION_DESIRED; - schedule_work(&mhdp->hdcp.prop_work); + queue_work(cadence_mhdp_wq, &mhdp->hdcp.prop_work); goto out; }
ret = _cdns_mhdp_hdcp_enable(mhdp, mhdp->hdcp.hdcp_content_type); if (ret) { mhdp->hdcp.value = DRM_MODE_CONTENT_PROTECTION_DESIRED; - schedule_work(&mhdp->hdcp.prop_work); + queue_work(cadence_mhdp_wq, &mhdp->hdcp.prop_work); } out: mutex_unlock(&mhdp->hdcp.mutex); @@ -474,8 +474,8 @@ static void cdns_mhdp_hdcp_check_work(struct work_struct *work) hdcp);
if (!cdns_mhdp_hdcp_check_link(mhdp)) - schedule_delayed_work(&hdcp->check_work, - DRM_HDCP_CHECK_PERIOD_MS); + queue_delayed_work(cadence_mhdp_wq, &hdcp->check_work, + DRM_HDCP_CHECK_PERIOD_MS); }
static void cdns_mhdp_hdcp_prop_work(struct work_struct *work) @@ -538,9 +538,9 @@ int cdns_mhdp_hdcp_enable(struct cdns_mhdp_device *mhdp, u8 content_type)
mhdp->hdcp.hdcp_content_type = content_type; mhdp->hdcp.value = DRM_MODE_CONTENT_PROTECTION_ENABLED; - schedule_work(&mhdp->hdcp.prop_work); - schedule_delayed_work(&mhdp->hdcp.check_work, - DRM_HDCP_CHECK_PERIOD_MS); + queue_work(cadence_mhdp_wq, &mhdp->hdcp.prop_work); + queue_delayed_work(cadence_mhdp_wq, &mhdp->hdcp.check_work, + DRM_HDCP_CHECK_PERIOD_MS); out: mutex_unlock(&mhdp->hdcp.mutex); return ret; @@ -553,7 +553,7 @@ int cdns_mhdp_hdcp_disable(struct cdns_mhdp_device *mhdp) mutex_lock(&mhdp->hdcp.mutex); if (mhdp->hdcp.value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) { mhdp->hdcp.value = DRM_MODE_CONTENT_PROTECTION_UNDESIRED; - schedule_work(&mhdp->hdcp.prop_work); + queue_work(cadence_mhdp_wq, &mhdp->hdcp.prop_work); ret = _cdns_mhdp_hdcp_disable(mhdp); } mutex_unlock(&mhdp->hdcp.mutex);
Use local wq in order to avoid flush_scheduled_work() usage.
Signed-off-by: Tetsuo Handa penguin-kernel@I-love.SAKURA.ne.jp --- Changes in v2: Replace flush_scheduled_work() with flush_workqueue().
Please see commit c4f135d643823a86 ("workqueue: Wrap flush_workqueue() using a macro") for background.
This is a blind conversion, and is only compile tested.
.../drm/bridge/cadence/cdns-mhdp8546-core.c | 32 ++++++++++++++++--- .../drm/bridge/cadence/cdns-mhdp8546-core.h | 2 ++ .../drm/bridge/cadence/cdns-mhdp8546-hdcp.c | 16 +++++----- 3 files changed, 37 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c index 67f0f444b4e8..f29c9484d4bb 100644 --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c @@ -53,6 +53,8 @@ #include "cdns-mhdp8546-hdcp.h" #include "cdns-mhdp8546-j721e.h"
+struct workqueue_struct *cadence_mhdp_wq; + static int cdns_mhdp_mailbox_read(struct cdns_mhdp_device *mhdp) { int ret, empty; @@ -2049,7 +2051,7 @@ static void cdns_mhdp_atomic_enable(struct drm_bridge *bridge, out: mutex_unlock(&mhdp->link_mutex); if (ret < 0) - schedule_work(&mhdp->modeset_retry_work); + queue_work(cadence_mhdp_wq, &mhdp->modeset_retry_work); }
static void cdns_mhdp_atomic_disable(struct drm_bridge *bridge, @@ -2373,7 +2375,7 @@ static irqreturn_t cdns_mhdp_irq_handler(int irq, void *data) spin_unlock(&mhdp->start_lock);
if (bridge_attached && (sw_ev0 & CDNS_DPTX_HPD)) { - schedule_work(&mhdp->hpd_work); + queue_work(cadence_mhdp_wq, &mhdp->hpd_work); }
if (sw_ev0 & ~CDNS_DPTX_HPD) { @@ -2413,7 +2415,7 @@ static void cdns_mhdp_hpd_work(struct work_struct *work) ret = cdns_mhdp_update_link_status(mhdp); if (mhdp->connector.dev) { if (ret < 0) - schedule_work(&mhdp->modeset_retry_work); + queue_work(cadence_mhdp_wq, &mhdp->modeset_retry_work); else drm_kms_helper_hotplug_event(mhdp->bridge.dev); } else { @@ -2603,7 +2605,7 @@ static int cdns_mhdp_remove(struct platform_device *pdev) pm_runtime_disable(&pdev->dev);
cancel_work_sync(&mhdp->modeset_retry_work); - flush_scheduled_work(); + flush_workqueue(cadence_mhdp_wq);
clk_disable_unprepare(mhdp->clk);
@@ -2632,7 +2634,27 @@ static struct platform_driver mhdp_driver = { .probe = cdns_mhdp_probe, .remove = cdns_mhdp_remove, }; -module_platform_driver(mhdp_driver); + +static int __init mhdp_init(void) +{ + int ret; + + cadence_mhdp_wq = alloc_workqueue("cadence_mhdp_wq", 0, 0); + if (!cadence_mhdp_wq) + return -ENOMEM; + ret = platform_driver_register(&mhdp_driver); + if (ret) + destroy_workqueue(cadence_mhdp_wq); + return ret; +} +module_init(mhdp_init); + +static void __exit mhdp_exit(void) +{ + platform_driver_unregister(&mhdp_driver); + destroy_workqueue(cadence_mhdp_wq); +} +module_exit(mhdp_exit);
MODULE_FIRMWARE(FW_NAME);
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h index bedddd510d17..e6c475612480 100644 --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h @@ -419,4 +419,6 @@ struct cdns_mhdp_device {
u32 cdns_mhdp_wait_for_sw_event(struct cdns_mhdp_device *mhdp, uint32_t event);
+extern struct workqueue_struct *cadence_mhdp_wq; + #endif diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c index 946212a95598..09f9e3d42f11 100644 --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-hdcp.c @@ -449,14 +449,14 @@ static int cdns_mhdp_hdcp_check_link(struct cdns_mhdp_device *mhdp) ret = _cdns_mhdp_hdcp_disable(mhdp); if (ret) { mhdp->hdcp.value = DRM_MODE_CONTENT_PROTECTION_DESIRED; - schedule_work(&mhdp->hdcp.prop_work); + queue_work(cadence_mhdp_wq, &mhdp->hdcp.prop_work); goto out; }
ret = _cdns_mhdp_hdcp_enable(mhdp, mhdp->hdcp.hdcp_content_type); if (ret) { mhdp->hdcp.value = DRM_MODE_CONTENT_PROTECTION_DESIRED; - schedule_work(&mhdp->hdcp.prop_work); + queue_work(cadence_mhdp_wq, &mhdp->hdcp.prop_work); } out: mutex_unlock(&mhdp->hdcp.mutex); @@ -474,8 +474,8 @@ static void cdns_mhdp_hdcp_check_work(struct work_struct *work) hdcp);
if (!cdns_mhdp_hdcp_check_link(mhdp)) - schedule_delayed_work(&hdcp->check_work, - DRM_HDCP_CHECK_PERIOD_MS); + queue_delayed_work(cadence_mhdp_wq, &hdcp->check_work, + DRM_HDCP_CHECK_PERIOD_MS); }
static void cdns_mhdp_hdcp_prop_work(struct work_struct *work) @@ -538,9 +538,9 @@ int cdns_mhdp_hdcp_enable(struct cdns_mhdp_device *mhdp, u8 content_type)
mhdp->hdcp.hdcp_content_type = content_type; mhdp->hdcp.value = DRM_MODE_CONTENT_PROTECTION_ENABLED; - schedule_work(&mhdp->hdcp.prop_work); - schedule_delayed_work(&mhdp->hdcp.check_work, - DRM_HDCP_CHECK_PERIOD_MS); + queue_work(cadence_mhdp_wq, &mhdp->hdcp.prop_work); + queue_delayed_work(cadence_mhdp_wq, &mhdp->hdcp.check_work, + DRM_HDCP_CHECK_PERIOD_MS); out: mutex_unlock(&mhdp->hdcp.mutex); return ret; @@ -553,7 +553,7 @@ int cdns_mhdp_hdcp_disable(struct cdns_mhdp_device *mhdp) mutex_lock(&mhdp->hdcp.mutex); if (mhdp->hdcp.value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) { mhdp->hdcp.value = DRM_MODE_CONTENT_PROTECTION_UNDESIRED; - schedule_work(&mhdp->hdcp.prop_work); + queue_work(cadence_mhdp_wq, &mhdp->hdcp.prop_work); ret = _cdns_mhdp_hdcp_disable(mhdp); } mutex_unlock(&mhdp->hdcp.mutex);
On 2022/06/10 23:35, Tetsuo Handa wrote:
Use local wq in order to avoid flush_scheduled_work() usage.
Signed-off-by: Tetsuo Handa penguin-kernel@I-love.SAKURA.ne.jp
Changes in v2: Replace flush_scheduled_work() with flush_workqueue().
Please see commit c4f135d643823a86 ("workqueue: Wrap flush_workqueue() using a macro") for background.
This is a blind conversion, and is only compile tested.
.../drm/bridge/cadence/cdns-mhdp8546-core.c | 32 ++++++++++++++++--- .../drm/bridge/cadence/cdns-mhdp8546-core.h | 2 ++ .../drm/bridge/cadence/cdns-mhdp8546-hdcp.c | 16 +++++----- 3 files changed, 37 insertions(+), 13 deletions(-)
I'm thinking about flush_work() version, and I got confused.
Since cdns-mhdp8546 driver uses 4 works
mhdp->modeset_retry_work mhdp->hpd_work mhdp->hdcp.check_work mhdp->hdcp.prop_work
I assume that flush_scheduled_work() in cdns_mhdp_remove() needs to wait for only these 4 works. And since mhdp->modeset_retry_work already uses cancel_work_sync(), flush_scheduled_work() would need to wait for only 3 works. Therefore, I guess that the flush_work() version would look something like
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c index 67f0f444b4e8..04b21752ab3f 100644 --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c @@ -2603,7 +2603,11 @@ static int cdns_mhdp_remove(struct platform_device *pdev) pm_runtime_disable(&pdev->dev);
cancel_work_sync(&mhdp->modeset_retry_work); - flush_scheduled_work(); + flush_work(&mhdp->hpd_work); + if (mhdp->hdcp_supported) { + cancel_delayed_work_sync(&mhdp->hdcp.check_work); + flush_work(&mhdp->hdcp.prop_work); + }
clk_disable_unprepare(mhdp->clk);
but I came to wonder whether mhdp->hdcp.check_work should be flushed or cancelled.
While flush_scheduled_work() waits for completion of works which were already queued to system_wq, mhdp->modeset_retry_work and mhdp->hdcp.check_work are delayed works. That is, work won't be queued to system_wq unless timeout expires.
Current code will wait for mhdp->hdcp.check_work only if timeout already expired. If timeout is not expired yet, flush_scheduled_work() will fail to cancel mhdp->hdcp.check_work, and cdns_mhdp_hdcp_check_work() which is triggered by mhdp->hdcp.check_work will schedule hdcp->check_work, which is too late for flush_scheduled_work() to wait for completion of cdns_mhdp_hdcp_prop_work().
Thus, how do we want to handle this race window?
flush_delayed_work(&mhdp->hdcp.check_work) followed by flush_work(&mhdp->hdcp.prop_work) (i.e. flush as much as possible) ?
cancel_delayed_work_sync(&mhdp->hdcp.check_work) followed by cancel_work_sync(&mhdp->hdcp.prop_work) (i.e. cancel as much as possible) ?
do nothing (i.e. no need to flush or cancel mhdp->hdcp.check_work and mhdp->hdcp.prop_work) ?
dri-devel@lists.freedesktop.org