RE: [PATCH v2 1/3] scsi: ufs: core: Remove redundant host_lock calls around UTMRLDBR.

[Avri Altman]

> 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.