mbox series

[v2,0/3] Untie the host lock entanglement - part 1

Message ID 20241022074319.512127-1-avri.altman@wdc.com
Headers show
Series Untie the host lock entanglement - part 1 | expand

Message

Avri Altman Oct. 22, 2024, 7:43 a.m. UTC 
While trying to simplify the ufs core driver with the guard() macro [1],
Bart made note of the abuse of the scsi host lock in the ufs driver.
Indeed, the host lock is deeply entangled in various flows across the
driver, as if it was some occasional default synchronization mean.

Here is the first part of defusing it, remove some of those calls around
host registers accesses, which needs no protection.

Doing this in phases seems like a reasonable approach, given the myriad
use of the host lock.


Changes compared to v1:
 - get rid of redundant locking (Bart)
 - leave out the HCE register

[1] https://lore.kernel.org/linux-scsi/0b031b8f-c07c-42ef-af93-7336439d3c37@acm.org/

Avri Altman (3):
  scsi: ufs: core: Remove redundant host_lock calls around UTMRLDBR.
  scsi: ufs: core: Remove redundant host_lock calls around UTMRLCLR
  scsi: ufs: core: Remove redundant host_lock calls around UTRLCLR.

 drivers/ufs/core/ufshcd.c | 26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)

Comments

Avri Altman Oct. 23, 2024, 6:47 a.m. UTC | #1 
> On 10/22/24 12:43 AM, Avri Altman wrote:
> > @@ -6877,13 +6874,13 @@ static irqreturn_t ufshcd_check_errors(struct
> ufs_hba *hba, u32 intr_status)
> >    */
> >   static irqreturn_t ufshcd_tmc_handler(struct ufs_hba *hba)
> >   {
> > -     unsigned long flags, pending, issued;
> > +     unsigned long flags;
> > +     unsigned long pending = ufshcd_readl(hba,
> REG_UTP_TASK_REQ_DOOR_BELL);
> > +     unsigned long issued = hba->outstanding_tasks & ~pending;
> >       irqreturn_t ret = IRQ_NONE;
> >       int tag;
> >
> >       spin_lock_irqsave(hba->host->host_lock, flags);
> > -     pending = ufshcd_readl(hba, REG_UTP_TASK_REQ_DOOR_BELL);
> > -     issued = hba->outstanding_tasks & ~pending;
> 
> Please keep the 'pending' and 'issued' assignments in the function body.
> Initializing variables in the declaration block is fine but adding code in the
> declaration block that has side effects is a bit controversial.
Done.

> 
> >       for_each_set_bit(tag, &issued, hba->nutmrs) {
> >               struct request *req = hba->tmf_rqs[tag];
> >               struct completion *c = req->end_io_data;
> 
> Would it be sufficient to hold the SCSI host lock around the
> hba->outstanding_tasks read only? I don't think that the
> for_each_set_bit() loop needs to be protected with the SCSI host lock.
That may cause concurrent access to tmf_rqs?
So better withdraw from changing ufshcd_tmc_handler() and just leave the whole function as it is?

Thanks,
Avri
> 
> Otherwise this patch looks good to me.
> 
> Thanks,
> 
> Bart.
Bart Van Assche Oct. 23, 2024, 7:44 p.m. UTC | #2 
On 10/22/24 11:47 PM, Avri Altman wrote:
>> On 10/22/24 12:43 AM, Avri Altman wrote:
>>>        for_each_set_bit(tag, &issued, hba->nutmrs) {
>>>                struct request *req = hba->tmf_rqs[tag];
>>>                struct completion *c = req->end_io_data;
>>
>> Would it be sufficient to hold the SCSI host lock around the
>> hba->outstanding_tasks read only? I don't think that the
>> for_each_set_bit() loop needs to be protected with the SCSI host lock.
 >
> That may cause concurrent access to tmf_rqs?

Right, the host_lock serializes hba->tmf_rqs[] accesses. Without having
analyzed whether or not removing locking from around the hba->tmf_rqs[]
accesses, let's keep this locking.

> So better withdraw from changing ufshcd_tmc_handler() and just leave
> the whole function as it is?
That sounds good to me.

Thanks,

Bart.