linux-kernel - Re: [PATCH v1 5/6] scsi: ufs: Let host_sem cover the entire system suspend/resume

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 17 May 2021 08:05:17 -0700
From:   Bart Van Assche <bvanassche@....org>
To:     Can Guo <cang@...eaurora.org>
Cc:     asutoshd@...eaurora.org, nguyenb@...eaurora.org,
        hongwus@...eaurora.org, ziqichen@...eaurora.org,
        linux-scsi@...r.kernel.org, kernel-team@...roid.com,
        Alim Akhtar <alim.akhtar@...sung.com>,
        Avri Altman <avri.altman@....com>,
        "James E.J. Bottomley" <jejb@...ux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        Stanley Chu <stanley.chu@...iatek.com>,
        Bean Huo <beanhuo@...ron.com>,
        Jaegeuk Kim <jaegeuk@...nel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1 5/6] scsi: ufs: Let host_sem cover the entire system
 suspend/resume

On 5/16/21 8:22 PM, Can Guo wrote:
> Hi Bart,
> 
> On 2021-05-14 11:55, Bart Van Assche wrote:
>> On 5/12/21 10:55 PM, Can Guo wrote:
>>> UFS error handling now is doing more than just re-probing, but also
>>> sending
>>> scsi cmds, e.g., for clearing UACs, and recovering runtime PM error,
>>> which
>>> may change runtime status of scsi devices. To protect system
>>> suspend/resume
>>> from being disturbed by error handling, move the host_sem from wl pm ops
>>> to ufshcd_suspend_prepare() and ufshcd_resume_complete().
>>
>> In ufshcd.h I found the following:
>>
>>  * @host_sem: semaphore used to serialize concurrent contexts
>>
>> That's the wrong way to use a synchronization object. A synchronization
>> object must protect data instead of code. Does host_sem perhaps need to
>> be split into multiple synchronization objects?
> 
> Thanks for the comments. These contexts are changing critical data and
> registers, so the sem is used to protect data actually, just like the
> scaling_lock protecting scaling and cmd transations.

But where is the documentation that explains which data members are
protected by hba->host_sem and which data members are protected by
hba->host->host_lock? Was the host_lock protection perhaps introduced
before scsi-mq was introduced? Before scsi-mq acquiring the host_lock
was sufficient to serialize against ufshcd_queuecommand() but that is
not sufficient when using scsi-mq.

I want to verify whether locking is used correctly in the UFS driver but
without documentation of which synchronization object protects which
data members that is not possible.

Thanks,

Bart.