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