linux-kernel - RE: [PATCH v3 1/2] scsi: ufs: core: Introduce a new clock

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 for Android: free password hash cracker in your pocket

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

Message-ID:
 <DM6PR04MB6575A35B9478BB93DB426BFFFC272@DM6PR04MB6575.namprd04.prod.outlook.com>
Date: Mon, 18 Nov 2024 07:00:31 +0000
From: Avri Altman <Avri.Altman@....com>
To: Bean Huo <huobean@...il.com>, "Martin K . Petersen"
	<martin.petersen@...cle.com>, "beanhuo@...ron.com" <beanhuo@...ron.com>
CC: "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Bart Van
 Assche <bvanassche@....org>
Subject: RE: [PATCH v3 1/2] scsi: ufs: core: Introduce a new clock_gating lock


> On Tue, 2024-11-05 at 13:25 +0200, Avri Altman wrote:
> > -       spin_lock_irqsave(hba->host->host_lock, flags);
> > -       /*
> > -        * In case you are here to cancel this work the gating state
> > -        * would be marked as REQ_CLKS_ON. In this case save time by
> > -        * skipping the gating work and exit after changing the clock
> > -        * state to CLKS_ON.
> > -        */
> > -       if (hba->clk_gating.is_suspended ||
> > -               (hba->clk_gating.state != REQ_CLKS_OFF)) {
> > -               hba->clk_gating.state = CLKS_ON;
> > -               trace_ufshcd_clk_gating(dev_name(hba->dev),
> > -                                       hba->clk_gating.state);
> > -               goto rel_lock;
> > +       scoped_guard(spinlock_irqsave, &hba->clk_gating.lock)
> > +       {
> > +               /*
> > +                * In case you are here to cancel this work the
> > gating state
> > +                * would be marked as REQ_CLKS_ON. In this case save
> > time by
> > +                * skipping the gating work and exit after changing
> > the clock
> > +                * state to CLKS_ON.
> > +                */
> > +               if (hba->clk_gating.is_suspended ||
> > +                   hba->clk_gating.state != REQ_CLKS_OFF) {
> > +                       hba->clk_gating.state = CLKS_ON;
> > +                       trace_ufshcd_clk_gating(dev_name(hba->dev),
> > +                                               hba-
> > >clk_gating.state);
> > +                       return;
> > +               }
> > +               if (ufshcd_is_ufs_dev_busy(hba) ||
> > +                   hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL)
> > +                       return;
> >         }
> 
> I'm wondering if it would be safe to replace host_lock with gating.lock or
> scaling.lock. For instance, in above context, ufshcd_state needs to be checked,
> but it's currently serialized by host_lock.
Hi, thank you for your feedback.
Yeah - I think you have a valid point.
I will remove the state check out of the scope of the clk_gating.lock,
and restore it under the host lock.

Thanks,
Avri


> 
> King regards,
> Bean