Messages in this thread | | | Subject | Re: [PATCH 1/3] scsi: ufs: Add "wb_on" sysfs node to control WB on/off | From | "Asutosh Das (asd)" <> | Date | Wed, 2 Dec 2020 09:58:29 -0800 |
| |
On 12/2/2020 8:20 AM, Bean Huo wrote: > On Mon, 2020-11-30 at 15:19 -0800, Asutosh Das (asd) wrote: >>> + return -EINVAL; >>> + >>> + pm_runtime_get_sync(hba->dev); >>> + res = ufshcd_wb_ctrl(hba, wb_enable); >> >> Say, a platform supports clock-scaling and this bit is toggled. >> The control goes into ufshcd_wb_ctrl for both this sysfs and >> clock-scaling contexts. The clock-scaling context passes all checks >> and >> blocks on waiting for this wb control to be disabled and then tries >> to >> enable wb when it's already disabled. Perhaps that's a race there? > > Hi Asutosh > Appreciate your review. > There is only inconsistent problem between clock-scaling and sysfs, > since hba->dev_cmd.lock can garantee there is only one can change > fWriteBoosterEn. But this is only happening on user willfully wants to > control WB through sysfs even they know the platform supports clock- > scaling. > > Since this is for the platform which doesn't support clock-scaling, I > think based on your comments, it should be acceptable for you like > this:
Or a synchronization primitive b/w the 2 contexts would work just as well. However, I don't have an issue if the user is denied toggling of wb anyway. LGTM.
> > > +static ssize_t wb_on_store(struct device *dev, struct device_attribute > *attr, > + const char *buf, size_t count) > +{ > + struct ufs_hba *hba = dev_get_drvdata(dev); > + unsigned int wb_enable; > + ssize_t res; > + > + if (ufshcd_is_clkscaling_supported(hba)) { > + dev_err(dev, "supports dynamic clk scaling, control WB > + through sysfs is not allowed!"); > + return -EOPNOTSUPP; > + } > + if (!ufshcd_is_wb_allowed(hba)) > + return -EOPNOTSUPP; > + > + if (kstrtouint(buf, 0, &wb_enable)) > + return -EINVAL; > + > + if (wb_enable != 0 && wb_enable != 1) > + return -EINVAL; > + > + pm_runtime_get_sync(hba->dev); > + res = ufshcd_wb_ctrl(hba, wb_enable); > + pm_runtime_put_sync(hba->dev); > + > + return res < 0 ? res : count; > +} > > thanks, > Bean > >
-- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, Linux Foundation Collaborative Project
| |