linux-kernel - RE: [PATCH v2 1/3] scsi: ufs: core: Remove redundant host

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] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <DM6PR04MB65750DBFE520C0DDE075146EFC4D2@DM6PR04MB6575.namprd04.prod.outlook.com>
Date: Wed, 23 Oct 2024 06:47:51 +0000
From: Avri Altman <Avri.Altman@....com>
To: Bart Van Assche <bvanassche@....org>, "Martin K . Petersen"
	<martin.petersen@...cle.com>
CC: "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2 1/3] scsi: ufs: core: Remove redundant host_lock calls
 around UTMRLDBR.

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