On 2022-02-25 03:45, Damien Le Moal wrote:
The return statement inside the sdeb_read_lock(), sdeb_read_unlock(), sdeb_write_lock() and sdeb_write_unlock() confuse sparse, leading to many warnings about unexpected unlocks in the resp_xxx() functions. Modify the lock/unlock functions using the __acquire() and __release() inline annotations for the sdebug_no_rwlock == true case to avoid these warnings.
I'm confused. The idea with sdebug_no_rwlock was that the application may know that the protection afforded by the driver's rwlock is not needed because locking is performed at a higher level (e.g. in the user space). Hence there is no need to use a read-write lock (or a full lock) in this driver to protect a read (say) against a co-incident write to the same memory region. So this was a performance enhancement. The proposed patch seems to be replacing a read-write lock with a full lock. That would be going in the opposite direction to what I intended. If this is the only solution, a better idea might be to drop the patch (in staging I think) that introduced the sdebug_no_rwlock option. The sdebug_no_rwlock option has been pretty thoroughly tested (for over a year) with memory to memory transfers (together with sgl to sgl additions to lib/scatterlist.h). Haven't seen any unexplained crashes that I could trace to this lack of locking. OTOH I haven't measured any improvement of the copy speed either, that may be because my tests are approaching the copy bandwidth of the ram. Does sparse understand guard variables (e.g. like 'bool lock_taken')? From what I've seen with sg3_utils Coverity doesn't, leading to many false reports. Doug Gilbert
Signed-off-by: Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx> --- drivers/scsi/scsi_debug.c | 68 +++++++++++++++++++++++++-------------- 1 file changed, 44 insertions(+), 24 deletions(-) diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c index 0d25b30922ef..f4e97f2224b2 100644 --- a/drivers/scsi/scsi_debug.c +++ b/drivers/scsi/scsi_debug.c @@ -3167,45 +3167,65 @@ static int prot_verify_read(struct scsi_cmnd *scp, sector_t start_sec, static inline void sdeb_read_lock(struct sdeb_store_info *sip) { - if (sdebug_no_rwlock) - return; - if (sip) - read_lock(&sip->macc_lck); - else - read_lock(&sdeb_fake_rw_lck); + if (sdebug_no_rwlock) { + if (sip) + __acquire(&sip->macc_lck); + else + __acquire(&sdeb_fake_rw_lck); + } else { + if (sip) + read_lock(&sip->macc_lck); + else + read_lock(&sdeb_fake_rw_lck); + } }static inline voidsdeb_read_unlock(struct sdeb_store_info *sip) { - if (sdebug_no_rwlock) - return; - if (sip) - read_unlock(&sip->macc_lck); - else - read_unlock(&sdeb_fake_rw_lck); + if (sdebug_no_rwlock) { + if (sip) + __release(&sip->macc_lck); + else + __release(&sdeb_fake_rw_lck); + } else { + if (sip) + read_unlock(&sip->macc_lck); + else + read_unlock(&sdeb_fake_rw_lck); + } }static inline voidsdeb_write_lock(struct sdeb_store_info *sip) { - if (sdebug_no_rwlock) - return; - if (sip) - write_lock(&sip->macc_lck); - else - write_lock(&sdeb_fake_rw_lck); + if (sdebug_no_rwlock) { + if (sip) + __acquire(&sip->macc_lck); + else + __acquire(&sdeb_fake_rw_lck); + } else { + if (sip) + write_lock(&sip->macc_lck); + else + write_lock(&sdeb_fake_rw_lck); + } }static inline voidsdeb_write_unlock(struct sdeb_store_info *sip) { - if (sdebug_no_rwlock) - return; - if (sip) - write_unlock(&sip->macc_lck); - else - write_unlock(&sdeb_fake_rw_lck); + if (sdebug_no_rwlock) { + if (sip) + __release(&sip->macc_lck); + else + __release(&sdeb_fake_rw_lck); + } else { + if (sip) + write_unlock(&sip->macc_lck); + else + write_unlock(&sdeb_fake_rw_lck); + } }static int resp_read_dt0(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
