On Thu, 01 Aug 2024, "Saarinen, Jani" <jani.saarinen@xxxxxxxxx> wrote: > These is also this made by Luca https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11813 > > @Nikula, Jani , ok to merge. Already tested at trybot https://patchwork.freedesktop.org/series/136776/ Acked-by: Jani Nikula <jani.nikula@xxxxxxxxx> The full IGT results aren't in for the trybot submission though. > >> -----Original Message----- >> From: Coelho, Luciano <luciano.coelho@xxxxxxxxx> >> Sent: Thursday, 1 August 2024 11.23 >> To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx >> Cc: Saarinen, Jani <jani.saarinen@xxxxxxxxx> >> Subject: [core-for-CI PATCH] scsi: sd: Move sd_read_cpr() out of the q- >> >limits_lock region >> >> From: Shin'ichiro Kawasaki <shinichiro.kawasaki@xxxxxxx> >> >> Commit 804e498e0496 ("sd: convert to the atomic queue limits API") >> introduced pairs of function calls to queue_limits_start_update() and >> queue_limits_commit_update(). These two functions lock and unlock >> q->limits_lock. In sd_revalidate_disk(), sd_read_cpr() is called after >> queue_limits_start_update() call and before >> queue_limits_commit_update() call. sd_read_cpr() locks q->sysfs_dir_lock >> and &q->sysfs_lock. Then new lock dependencies were created between >> q->limits_lock, q->sysfs_dir_lock and q->sysfs_lock, as follows: >> >> sd_revalidate_disk >> queue_limits_start_update >> mutex_lock(&q->limits_lock) >> sd_read_cpr >> disk_set_independent_access_ranges >> mutex_lock(&q->sysfs_dir_lock) >> mutex_lock(&q->sysfs_lock) >> mutex_unlock(&q->sysfs_lock) >> mutex_unlock(&q->sysfs_dir_lock) >> queue_limits_commit_update >> mutex_unlock(&q->limits_lock) >> >> However, the three locks already had reversed dependencies in other places. >> Then the new dependencies triggered the lockdep WARN "possible circular >> locking dependency detected" [1]. This WARN was observed by running the >> blktests test case srp/002. >> >> To avoid the WARN, move the sd_read_cpr() call in sd_revalidate_disk() after >> the queue_limits_commit_update() call. In other words, move the >> sd_read_cpr() call out of the q->limits_lock region. >> >> [1] https://lore.kernel.org/linux- >> scsi/vlmv53ni3ltwxplig5qnw4xsl2h6ccxijfbqzekx76vxoim5a5@dekv7q3es3tx/ >> >> Fixes: 804e498e0496 ("sd: convert to the atomic queue limits API") >> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@xxxxxxx> >> Signed-off-by: Luca Coelho <luciano.coelho@xxxxxxxxx> >> --- >> drivers/scsi/sd.c | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index >> adeaa8ab9951..08cbe3815006 100644 >> --- a/drivers/scsi/sd.c >> +++ b/drivers/scsi/sd.c >> @@ -3753,7 +3753,6 @@ static int sd_revalidate_disk(struct gendisk *disk) >> sd_read_block_limits_ext(sdkp); >> sd_read_block_characteristics(sdkp, &lim); >> sd_zbc_read_zones(sdkp, &lim, buffer); >> - sd_read_cpr(sdkp); >> } >> >> sd_print_capacity(sdkp, old_capacity); @@ -3808,6 +3807,14 >> @@ static int sd_revalidate_disk(struct gendisk *disk) >> if (err) >> return err; >> >> + /* >> + * Query concurrent positioning ranges after >> + * queue_limits_commit_update() unlocked q->limits_lock to avoid >> + * deadlock with q->sysfs_dir_lock and q->sysfs_lock. >> + */ >> + if (sdkp->media_present && scsi_device_supports_vpd(sdp)) >> + sd_read_cpr(sdkp); >> + >> /* >> * For a zoned drive, revalidating the zones can be done only once >> * the gendisk capacity is set. So if this fails, set back the gendisk >> -- >> 2.39.2 > -- Jani Nikula, Intel
