| Message ID | 1644046760-83345-1-git-send-email-kwmad.kim@samsung.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v1] scsi: ufs: remove clk_scaling_lock when clkscaling isn't supported. | expand |
> clk_scaling_lock is to prevent from running clkscaling related operations > with others which might be affected by the operations concurrently. > I think it looks hardware specific. > If the feature isn't supported, I think there is no reasonto prevent from ^^^ reason to > running other functions, such as ufshcd_queuecommand and It is no longer used in queuecommand since 5675c381ea51 and 8d077ede48c1 > ufshcd_exec_dev_cmd, concurrently. > > So I add a condition at some points protecting with clk_scaling_lock. But you still need a way to serialize device management commands. Thanks, Avri > > Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com> > --- > drivers/scsi/ufs/ufshcd.c | 21 ++++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index 460d2b4..8471c90 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -2980,7 +2980,8 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba, > /* Protects use of hba->reserved_slot. */ > lockdep_assert_held(&hba->dev_cmd.lock); > > - down_read(&hba->clk_scaling_lock); > + if (ufshcd_is_clkscaling_supported(hba)) > + down_read(&hba->clk_scaling_lock); > > lrbp = &hba->lrb[tag]; > WARN_ON(lrbp->cmd); > @@ -2998,7 +2999,8 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba, > (struct utp_upiu_req *)lrbp->ucd_rsp_ptr); > > out: > - up_read(&hba->clk_scaling_lock); > + if (ufshcd_is_clkscaling_supported(hba)) > + up_read(&hba->clk_scaling_lock); > return err; > } > > @@ -6014,7 +6016,8 @@ static void ufshcd_err_handling_prepare(struct > ufs_hba *hba) > if (ufshcd_is_clkscaling_supported(hba) && > hba->clk_scaling.is_enabled) > ufshcd_suspend_clkscaling(hba); > - ufshcd_clk_scaling_allow(hba, false); > + if (ufshcd_is_clkscaling_supported(hba)) > + ufshcd_clk_scaling_allow(hba, false); > } > ufshcd_scsi_block_requests(hba); > /* Drain ufshcd_queuecommand() */ > @@ -6247,7 +6250,8 @@ static void ufshcd_err_handler(struct work_struct > *work) > * Hold the scaling lock just in case dev cmds > * are sent via bsg and/or sysfs. > */ > - down_write(&hba->clk_scaling_lock); > + if (ufshcd_is_clkscaling_supported(hba)) > + down_write(&hba->clk_scaling_lock); > hba->force_pmc = true; > pmc_err = ufshcd_config_pwr_mode(hba, &(hba->pwr_info)); > if (pmc_err) { > @@ -6257,7 +6261,8 @@ static void ufshcd_err_handler(struct work_struct > *work) > } > hba->force_pmc = false; > ufshcd_print_pwr_info(hba); > - up_write(&hba->clk_scaling_lock); > + if (ufshcd_is_clkscaling_supported(hba)) > + up_write(&hba->clk_scaling_lock); > spin_lock_irqsave(hba->host->host_lock, flags); > } > > @@ -6753,7 +6758,8 @@ static int ufshcd_issue_devman_upiu_cmd(struct > ufs_hba *hba, > /* Protects use of hba->reserved_slot. */ > lockdep_assert_held(&hba->dev_cmd.lock); > > - down_read(&hba->clk_scaling_lock); > + if (ufshcd_is_clkscaling_supported(hba)) > + down_read(&hba->clk_scaling_lock); > > lrbp = &hba->lrb[tag]; > WARN_ON(lrbp->cmd); > @@ -6822,7 +6828,8 @@ static int ufshcd_issue_devman_upiu_cmd(struct > ufs_hba *hba, > ufshcd_add_query_upiu_trace(hba, err ? UFS_QUERY_ERR : > UFS_QUERY_COMP, > (struct utp_upiu_req *)lrbp->ucd_rsp_ptr); > > - up_read(&hba->clk_scaling_lock); > + if (ufshcd_is_clkscaling_supported(hba)) > + up_read(&hba->clk_scaling_lock); > return err; > } > > -- > 2.7.4
> > I think it looks hardware specific. > > If the feature isn't supported, I think there is no reasonto prevent > > from > ^^^ > reason to > > > running other functions, such as ufshcd_queuecommand and > It is no longer used in queuecommand since 5675c381ea51 and 8d077ede48c1 Yeah, you're right. It's just an example. I just want to tell that the lock also protects things that are not related with clk scaling directly. > > > ufshcd_exec_dev_cmd, concurrently. > > > > So I add a condition at some points protecting with clk_scaling_lock. > But you still need a way to serialize device management commands. > > Thanks, > Avri The dev cmd execution period is protected by mutex. And actual ringing a doorbell is protected by spin lock. Is there another reason to need clk_scaling_lock even with it? Thanks. Kiwoong Kim
On 11/02/2022 04:15, Kiwoong Kim wrote: >>> I think it looks hardware specific. >>> If the feature isn't supported, I think there is no reasonto prevent >>> from >> ^^^ >> reason to >> >>> running other functions, such as ufshcd_queuecommand and >> It is no longer used in queuecommand since 5675c381ea51 and 8d077ede48c1 > > Yeah, you're right. It's just an example. I just want to tell that the lock also protects things that are not related with clk scaling directly. > >> >>> ufshcd_exec_dev_cmd, concurrently. >>> >>> So I add a condition at some points protecting with clk_scaling_lock. >> But you still need a way to serialize device management commands. >> >> Thanks, >> Avri > > The dev cmd execution period is protected by mutex. > And actual ringing a doorbell is protected by spin lock. > > Is there another reason to need clk_scaling_lock even with it? > The error handler really should have exclusive access. One of the places you change does explain that: * Hold the scaling lock just in case dev cmds * are sent via bsg and/or sysfs. */ - down_write(&hba->clk_scaling_lock); + if (ufshcd_is_clkscaling_supported(hba)) + down_write(&hba->clk_scaling_lock);
> > > I think it looks hardware specific. > > > If the feature isn't supported, I think there is no reasonto prevent > > > from > > > > ^^^ reason to > > > > > running other functions, such as ufshcd_queuecommand and > > It is no longer used in queuecommand since 5675c381ea51 and > > 8d077ede48c1 > > Yeah, you're right. It's just an example. I just want to tell that the lock also > protects things that are not related with clk scaling directly. OK. > > > > > > ufshcd_exec_dev_cmd, concurrently. > > > > > > So I add a condition at some points protecting with clk_scaling_lock. > > But you still need a way to serialize device management commands. > > > > Thanks, > > Avri > > The dev cmd execution period is protected by mutex. > And actual ringing a doorbell is protected by spin lock. > > Is there another reason to need clk_scaling_lock even with it? Right. Acked-by: Avri Altman <avri.altman@wdc.com> > > Thanks. > Kiwoong Kim >
> The error handler really should have exclusive access. One of the places > you change does explain that: > > * Hold the scaling lock just in case dev cmds > * are sent via bsg and/or sysfs. > */ > - down_write(&hba->clk_scaling_lock); > + if (ufshcd_is_clkscaling_supported(hba)) > + down_write(&hba->clk_scaling_lock); Yeah.., I saw the comment but didn't get why. Is there anyone who knows why it's necessary for all SoCs? At lease, I know there is no reason to forbid concurrent executions of dev cmd and power mode change. If there's nothing, how about adding a quick to ignore it? Thanks. Kiwoong Kim
On 12/02/2022 06:44, Kiwoong Kim wrote: >> The error handler really should have exclusive access. One of the places >> you change does explain that: >> >> * Hold the scaling lock just in case dev cmds >> * are sent via bsg and/or sysfs. >> */ >> - down_write(&hba->clk_scaling_lock); >> + if (ufshcd_is_clkscaling_supported(hba)) >> + down_write(&hba->clk_scaling_lock); > > > Yeah.., I saw the comment but didn't get why. > > Is there anyone who knows why it's necessary for all SoCs? > At lease, I know there is no reason to forbid concurrent executions of dev cmd and power mode change. > > If there's nothing, how about adding a quick to ignore it? Is it worth it? The error handler really should have exclusive access. Have you considered, for example, races of ufshcd_reset_and restore() and dev commands, tm commands, UIC commands. I suspect more locking is needed not less.
On 2/4/22 23:39, Kiwoong Kim wrote: > clk_scaling_lock is to prevent from running clkscaling related operations > with others which might be affected by the operations concurrently. > I think it looks hardware specific. > If the feature isn't supported, I think there is no reasonto prevent from > running other functions, such as ufshcd_queuecommand and > ufshcd_exec_dev_cmd, concurrently. > > So I add a condition at some points protecting with clk_scaling_lock. > > Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com> > --- > drivers/scsi/ufs/ufshcd.c | 21 ++++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index 460d2b4..8471c90 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -2980,7 +2980,8 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba, > /* Protects use of hba->reserved_slot. */ > lockdep_assert_held(&hba->dev_cmd.lock); > > - down_read(&hba->clk_scaling_lock); > + if (ufshcd_is_clkscaling_supported(hba)) > + down_read(&hba->clk_scaling_lock); > > lrbp = &hba->lrb[tag]; > WARN_ON(lrbp->cmd); I don't like this patch at all. This patch makes testing the UFS driver more complicated without having any clear benefit. Additionally, adding if-statements in front of locking makes static source code analysis harder and is an anti-pattern. Please don't do this. Bart.
> > - down_read(&hba->clk_scaling_lock); > > + if (ufshcd_is_clkscaling_supported(hba)) > > + down_read(&hba->clk_scaling_lock); > > > > lrbp = &hba->lrb[tag]; > > WARN_ON(lrbp->cmd); > > I don't like this patch at all. This patch makes testing the UFS driver > more complicated without having any clear benefit. Additionally, adding > if-statements in front of locking makes static source code analysis harder > and is an anti-pattern. Please don't do this. > > Bart. The benefit that I think is not blocking dev cmd during submitting a scsi cmd. Rather, I don't understand why this lock is required if a SoC doesn't support clk scaling. The period of ringing doorbells has been already protected by spin lock. Thanks. Kiwoong Kim
On Sat, 2022-02-12 at 13:44 +0900, Kiwoong Kim wrote: > > The error handler really should have exclusive access. One of the > > places > > you change does explain that: > > > > * Hold the scaling lock just in case dev cmds > > * are sent via bsg and/or sysfs. > > */ > > - down_write(&hba->clk_scaling_lock); > > + if (ufshcd_is_clkscaling_supported(hba)) > > + down_write(&hba->clk_scaling_lock); > > Yeah.., I saw the comment but didn't get why. > > Is there anyone who knows why it's necessary for all SoCs? > At lease, I know there is no reason to forbid concurrent executions > of dev cmd and power mode change. > > If there's nothing, how about adding a quick to ignore it? > > Thanks. > Kiwoong Kim > The name of clk_scaling_lock has explained everything, for the platform which doesn't support load-based clk scaling, doesn't need to hold this lock. Acked-by: Bean Huo <beanhuo@micron.com>
On 2/15/22 03:00, Bean Huo wrote: > The name of clk_scaling_lock has explained everything, for the platform > which doesn't support load-based clk scaling, doesn't need to hold this > lock. Hi Bean, Have you noticed Adrian's reply? I agree with Adrian that this patch breaks the UFS error handler. Bart.
On 2/14/22 22:03, Kiwoong Kim wrote:
> The benefit that I think is not blocking dev cmd during submitting a scsi cmd.
Hi Kiwoong,
Both ufshcd_exec_dev_cmd() and previous versions of
ufshcd_queuecommand() obtain a reader lock on the clock scaling lock so
concurrent submission of both command types is allowed. I'm not aware of
any version of the UFS driver that serializes device commands against
SCSI commands.
Additionally, please take a look at commit 8d077ede48c1 ("scsi: ufs:
Optimize the command queueing code"). That patch removes the clock
scaling lock from ufshcd_queuecommand().
Thanks,
Bart.
> The name of clk_scaling_lock has explained everything, for the platform > which doesn't support load-based clk scaling, doesn't need to hold this > lock. > > Acked-by: Bean Huo <beanhuo@micron.com> Okay, thank you for your information. I still have no idea of why the lock is required for ufshcd_exec_dev_cmd from the UFS specification w/o any featuring.
> On 2/14/22 22:03, Kiwoong Kim wrote: > > The benefit that I think is not blocking dev cmd during submitting a > scsi cmd. > > Hi Kiwoong, > > Both ufshcd_exec_dev_cmd() and previous versions of > ufshcd_queuecommand() obtain a reader lock on the clock scaling lock so > concurrent submission of both command types is allowed. I'm not aware of > any version of the UFS driver that serializes device commands against SCSI > commands. > > Additionally, please take a look at commit 8d077ede48c1 ("scsi: ufs: > Optimize the command queueing code"). That patch removes the clock scaling > lock from ufshcd_queuecommand(). > > Thanks, > > Bart. Okay, I got it.
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index 460d2b4..8471c90 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -2980,7 +2980,8 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba, /* Protects use of hba->reserved_slot. */ lockdep_assert_held(&hba->dev_cmd.lock); - down_read(&hba->clk_scaling_lock); + if (ufshcd_is_clkscaling_supported(hba)) + down_read(&hba->clk_scaling_lock); lrbp = &hba->lrb[tag]; WARN_ON(lrbp->cmd); @@ -2998,7 +2999,8 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba, (struct utp_upiu_req *)lrbp->ucd_rsp_ptr); out: - up_read(&hba->clk_scaling_lock); + if (ufshcd_is_clkscaling_supported(hba)) + up_read(&hba->clk_scaling_lock); return err; } @@ -6014,7 +6016,8 @@ static void ufshcd_err_handling_prepare(struct ufs_hba *hba) if (ufshcd_is_clkscaling_supported(hba) && hba->clk_scaling.is_enabled) ufshcd_suspend_clkscaling(hba); - ufshcd_clk_scaling_allow(hba, false); + if (ufshcd_is_clkscaling_supported(hba)) + ufshcd_clk_scaling_allow(hba, false); } ufshcd_scsi_block_requests(hba); /* Drain ufshcd_queuecommand() */ @@ -6247,7 +6250,8 @@ static void ufshcd_err_handler(struct work_struct *work) * Hold the scaling lock just in case dev cmds * are sent via bsg and/or sysfs. */ - down_write(&hba->clk_scaling_lock); + if (ufshcd_is_clkscaling_supported(hba)) + down_write(&hba->clk_scaling_lock); hba->force_pmc = true; pmc_err = ufshcd_config_pwr_mode(hba, &(hba->pwr_info)); if (pmc_err) { @@ -6257,7 +6261,8 @@ static void ufshcd_err_handler(struct work_struct *work) } hba->force_pmc = false; ufshcd_print_pwr_info(hba); - up_write(&hba->clk_scaling_lock); + if (ufshcd_is_clkscaling_supported(hba)) + up_write(&hba->clk_scaling_lock); spin_lock_irqsave(hba->host->host_lock, flags); } @@ -6753,7 +6758,8 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba, /* Protects use of hba->reserved_slot. */ lockdep_assert_held(&hba->dev_cmd.lock); - down_read(&hba->clk_scaling_lock); + if (ufshcd_is_clkscaling_supported(hba)) + down_read(&hba->clk_scaling_lock); lrbp = &hba->lrb[tag]; WARN_ON(lrbp->cmd); @@ -6822,7 +6828,8 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba, ufshcd_add_query_upiu_trace(hba, err ? UFS_QUERY_ERR : UFS_QUERY_COMP, (struct utp_upiu_req *)lrbp->ucd_rsp_ptr); - up_read(&hba->clk_scaling_lock); + if (ufshcd_is_clkscaling_supported(hba)) + up_read(&hba->clk_scaling_lock); return err; }
clk_scaling_lock is to prevent from running clkscaling related operations with others which might be affected by the operations concurrently. I think it looks hardware specific. If the feature isn't supported, I think there is no reasonto prevent from running other functions, such as ufshcd_queuecommand and ufshcd_exec_dev_cmd, concurrently. So I add a condition at some points protecting with clk_scaling_lock. Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com> --- drivers/scsi/ufs/ufshcd.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)