linux-kernel - Re: [PATCH] scsi: ufs: wb: Add Manual Flush sysfs and cleanup toggle functions

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: <CAM36TBt_2wPjbnXsQPZLgf18FckdC9qbj9ErFgDLZ-0xUQ_Oig@mail.gmail.com>
Date:   Sun, 24 Apr 2022 03:40:03 +0900
From:   jin young choi <ychoijy@...il.com>
To:     Bart Van Assche <bvanassche@....org>
Cc:     j-young.choi@...sung.com, ALIM AKHTAR <alim.akhtar@...sung.com>,
        "avri.altman@....com" <avri.altman@....com>,
        "jejb@...ux.ibm.com" <jejb@...ux.ibm.com>,
        "martin.petersen@...cle.com" <martin.petersen@...cle.com>,
        "beanhuo@...ron.com" <beanhuo@...ron.com>,
        Daejun Park <daejun7.park@...sung.com>,
        "adrian.hunter@...el.com" <adrian.hunter@...el.com>,
        "cang@...eaurora.org" <cang@...eaurora.org>,
        "asutoshd@...eaurora.org" <asutoshd@...eaurora.org>,
        "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] scsi: ufs: wb: Add Manual Flush sysfs and cleanup toggle functions

>
> On 4/22/22 05:14, Jinyoung CHOI wrote:
> > There is the following quirk to bypass "WB Manual Flush" in Write
> > Booster.
> >
> >    - UFSHCD_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL
> >
> > If this quirk is not set, there is no knob that can controll "WB Manual Flush".
> >
> >       There are three flags that control Write Booster Feature.
> >               1. WB ON/OFF
> >               2. WB Hibern Flush ON/OFF
> >               3. WB Flush ON/OFF
> >
> >       The sysfs that controls the WB was implemented. (1)
> >
> >       In the case of "Hibern Flush", it is always good to turn on.
> >       Control may not be required. (2)
> >
> >       Finally, "Manual flush" may be determined that it can affect
> >       performance or power consumption.
> >       So the sysfs that controls this may be necessary. (3)
> >
> > In addition, toggle functions for controlling the above flags are cleaned.
>
> Please make all sentences in the patch description start at the left margin.
>

OK. I'll fix it. :)

> > diff --git a/drivers/scsi/ufs/ufs-sysfs.c b/drivers/scsi/ufs/ufs-sysfs.c
> > index 5c405ff7b6ea..6bbb56152708 100644
> > --- a/drivers/scsi/ufs/ufs-sysfs.c
> > +++ b/drivers/scsi/ufs/ufs-sysfs.c
> > @@ -229,7 +229,7 @@ static ssize_t wb_on_store(struct device *dev, struct device_attribute *attr,
> >                * If the platform supports UFSHCD_CAP_CLK_SCALING, turn WB
> >                * on/off will be done while clock scaling up/down.
> >                */
> > -             dev_warn(dev, "To control WB through wb_on is not allowed!\n");
> > +             dev_warn(dev, "To control Write Booster is not allowed!\n");
> >               return -EOPNOTSUPP;
> >       }
>
> The new error message is grammatically incorrect. Please fix.
>

OK. I'll fix it. :)

> > +     if (!ufshcd_is_wb_flush_allowed(hba)) {
> > +             dev_warn(dev, "To control WB Flush is not allowed!\n");
>
> Same issue for the above error message.
>
> > +static DEVICE_ATTR_RW(wb_flush_on);
>
> "wb_flush_enabled" is probably a better name than "wb_flush_on".
> Additionally, the "wb_flush_en" is closer to the terminology used in the
> UFS specification (fWriteBoosterBufferFlushEn).
>

'wb_on' sysfs already existed. So I named it in the same format. (_on)
I'll change both. (_on -> _enable)

>  > diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
>  > index 4a00c24a3209..6c85f512f82f 100644
>  > --- a/drivers/scsi/ufs/ufs.h
>  > +++ b/drivers/scsi/ufs/ufs.h
>  > @@ -611,7 +611,7 @@ struct ufs_dev_info {
>  >
>  >      /* UFS WB related flags */
>  >      bool    wb_enabled;
>  > -    bool    wb_buf_flush_enabled;
>  > +    bool    wb_flush_enabled;
>  >      u8      wb_dedicated_lu;
>  >      u8      wb_buffer_type;
>
> Adding a variable with the name "wb_flush_enabled" next to a variable with
> the name "wb_buf_flush_enabled" is confusing. Please chose better names and
> add comments.
>

Hmm... it would be better not to modify the variable name.
I'll put it back

> > -static int __ufshcd_wb_toggle(struct ufs_hba *hba, bool set, enum flag_idn idn)
> > +static int __ufshcd_wb_toggle(struct ufs_hba *hba, const char *knob,
> > +                           bool set, enum flag_idn idn)
> >   {
> > +     int ret;
> >       u8 index;
> >       enum query_opcode opcode = set ? UPIU_QUERY_OPCODE_SET_FLAG :
> > -                                UPIU_QUERY_OPCODE_CLEAR_FLAG;
> > +             UPIU_QUERY_OPCODE_CLEAR_FLAG;
> > +
> > +     if (!ufshcd_is_wb_allowed(hba))
> > +             return -EPERM;
> >
> >       index = ufshcd_wb_get_query_index(hba);
> > -     return ufshcd_query_flag_retry(hba, opcode, idn, index, NULL);
> > +
> > +     ret = ufshcd_query_flag_retry(hba, opcode, idn, index, NULL);
> > +     if (ret) {
> > +             dev_err(hba->dev, "%s: %s %s failed %d\n",
> > +                     __func__, knob, set ? "enable" : "disable", ret);
> > +             return ret;
> > +     }
> > +
> > +     dev_dbg(hba->dev, "%s: %s %s\n",
> > +              __func__, knob, set ? "enabled" : "disabled");
> > +
> > +     return ret;
> >   }
>
> Please leave out the dev_dbg() message and move the dev_err() message to
> the callers of __ufshcd_wb_toggle() such that the 'knob' argument does not
> have to be added to __ufshcd_wb_toggle().
>
> Thanks,
>
> Bart.

OK. I got it.
Regarding this review, I wrote a comment on avri's comment.

Thanks,
Jinyoung