RE: [PATCH v6 1/9] rpmb: add Replay Protected Memory Block (RPMB) subsystem
>
> Signed-off-by: Tomas Winkler
> Signed-off-by: Alexander Usyskin
Tested-by: Avri Altman
- mmc - full functionality. One issue found that was fixed on V6: patch V6 2/9.
- ufs - read & read counter only. Testing is still wip.
> +static int rpmb_request_verify(struct rpmb_dev *rdev, struct rpmb_data
> +*rpmbd) {
Seems excessive - Isn't the standard should be enforced by the device?
Cheers,
Avri
RE: [PATCH 1/1] scsi: ufs: make sure all interrupts are processed
Hi,
Can you elaborate how this can even happen?
Isn't the interrupt aggregation capability should attend for those cases?
Thanks,
Avri
> -Original Message-
> From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-
> ow...@vger.kernel.org] On Behalf Of Asutosh Das
> Sent: Tuesday, January 30, 2018 6:54 AM
> To: subha...@codeaurora.org; c...@codeaurora.org;
> vivek.gau...@codeaurora.org; rna...@codeaurora.org;
> vinholika...@gmail.com; j...@linux.vnet.ibm.com;
> martin.peter...@oracle.com
> Cc: linux-s...@vger.kernel.org; Venkat Gopalakrishnan
> ; Asutosh Das ; open
> list
> Subject: [PATCH 1/1] scsi: ufs: make sure all interrupts are processed
>
> From: Venkat Gopalakrishnan
>
> As multiple requests are submitted to the ufs host controller in parallel
> there
> could be instances where the command completion interrupt arrives later for a
> request that is already processed earlier as the corresponding doorbell was
> cleared when handling the previous interrupt. Read the interrupt status in a
> loop after processing the received interrupt to catch such interrupts and
> handle
> it.
>
> Signed-off-by: Venkat Gopalakrishnan
> Signed-off-by: Asutosh Das
> ---
> drivers/scsi/ufs/ufshcd.c | 27 +++
> 1 file changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index
> 8af2af3..58d81de 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -5357,19 +5357,30 @@ static irqreturn_t ufshcd_intr(int irq, void *__hba)
> u32 intr_status, enabled_intr_status;
> irqreturn_t retval = IRQ_NONE;
> struct ufs_hba *hba = __hba;
> + int retries = hba->nutrs;
>
> spin_lock(hba->host->host_lock);
> intr_status = ufshcd_readl(hba, REG_INTERRUPT_STATUS);
> - enabled_intr_status =
> - intr_status & ufshcd_readl(hba, REG_INTERRUPT_ENABLE);
>
> - if (intr_status)
> - ufshcd_writel(hba, intr_status, REG_INTERRUPT_STATUS);
> + /*
> + * There could be max of hba->nutrs reqs in flight and in worst case
> + * if the reqs get finished 1 by 1 after the interrupt status is
> + * read, make sure we handle them by checking the interrupt status
> + * again in a loop until we process all of the reqs before returning.
> + */
> + do {
> + enabled_intr_status =
> + intr_status & ufshcd_readl(hba,
> REG_INTERRUPT_ENABLE);
> + if (intr_status)
> + ufshcd_writel(hba, intr_status,
> REG_INTERRUPT_STATUS);
> + if (enabled_intr_status) {
> + ufshcd_sl_intr(hba, enabled_intr_status);
> + retval = IRQ_HANDLED;
> + }
> +
> + intr_status = ufshcd_readl(hba, REG_INTERRUPT_STATUS);
> + } while (intr_status && --retries);
>
> - if (enabled_intr_status) {
> - ufshcd_sl_intr(hba, enabled_intr_status);
> - retval = IRQ_HANDLED;
> - }
> spin_unlock(hba->host->host_lock);
> return retval;
> }
> --
> Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
> Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux
> Foundation Collaborative Project.
RE: [PATCH v9] mmc: Export host capabilities to debugfs.
> -Original Message- > From: Andy Shevchenko [mailto:andriy.shevche...@linux.intel.com] > Sent: Monday, March 12, 2018 12:08 PM > To: Harish Jenny K N ; ulf.hans...@linaro.org; > linus.wall...@linaro.org; adrian.hun...@intel.com; shawn.lin@rock- > chips.com; Avri Altman > Cc: linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; > vladimir_zapols...@mentor.com > Subject: Re: [PATCH v9] mmc: Export host capabilities to debugfs. > > On Mon, 2018-03-12 at 10:01 +0530, Harish Jenny K N wrote: > > This patch exports the host capabilities to debugfs > > > > This idea of sharing host capabilities over debugfs came up from Abbas > > Raza Earlier discussions: > > https://lkml.org/lkml/2018/3/5/357 > > https://www.spinics.net/lists/linux-mmc/msg48219.html > > > > Address below minors and, FWIW, take mine > > Reviewed-by: Andy Shevchenko > > > + for_each_set_bit(bit, (const unsigned long *)&caps, > > BITS_PER_LONG) > > > + for_each_set_bit(bit, (const unsigned long *)&caps2, > > BITS_PER_LONG) > > Explicit casting is not needed anymore in both cases. Also maybe use sizeof(mmc_host_capabilities) instead of BITS_PER_LONG? > > -- > Andy Shevchenko > Intel Finland Oy
RE: [PATCH v9] mmc: Export host capabilities to debugfs.
> -Original Message- > From: Harish Jenny K N [mailto:harish_kand...@mentor.com] > Sent: Monday, March 12, 2018 1:17 PM > To: Avri Altman ; Andy Shevchenko > ; ulf.hans...@linaro.org; > linus.wall...@linaro.org; adrian.hun...@intel.com; shawn@rock-chips.com > Cc: linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; > vladimir_zapols...@mentor.com > Subject: Re: [PATCH v9] mmc: Export host capabilities to debugfs. > > > > On Monday 12 March 2018 04:15 PM, Avri Altman wrote: > > > >> -Original Message- > >> From: Andy Shevchenko [mailto:andriy.shevche...@linux.intel.com] > >> Sent: Monday, March 12, 2018 12:08 PM > >> To: Harish Jenny K N ; > >> ulf.hans...@linaro.org; linus.wall...@linaro.org; > >> adrian.hun...@intel.com; shawn.lin@rock- chips.com; Avri Altman > >> > >> Cc: linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; > >> vladimir_zapols...@mentor.com > >> Subject: Re: [PATCH v9] mmc: Export host capabilities to debugfs. > >> > >> On Mon, 2018-03-12 at 10:01 +0530, Harish Jenny K N wrote: > >>> This patch exports the host capabilities to debugfs > >>> > >>> This idea of sharing host capabilities over debugfs came up from > >>> Abbas Raza Earlier discussions: > >>> https://lkml.org/lkml/2018/3/5/357 > >>> https://www.spinics.net/lists/linux-mmc/msg48219.html > >>> > >> Address below minors and, FWIW, take mine > >> > >> Reviewed-by: Andy Shevchenko > >> > >>> + for_each_set_bit(bit, (const unsigned long *)&caps, > >>> BITS_PER_LONG) > >>> + for_each_set_bit(bit, (const unsigned long *)&caps2, > >>> BITS_PER_LONG) > >> Explicit casting is not needed anymore in both cases. > > Also maybe use sizeof(mmc_host_capabilities) instead of BITS_PER_LONG? > > You mean sizeof(caps) and not sizeof(mmc_host_capabilities) . Right ? meant ARRAY_SIZE(mmc_host_capabilities) Thanks, Avri > > > Thanks, > Harish Jenny K N
RE: [PATCH v6] mmc: Export host capabilities to debugfs.
> -Original Message-
> From: Harish Jenny K N [mailto:harish_kand...@mentor.com]
> Sent: Wednesday, March 07, 2018 7:38 AM
> To: ulf.hans...@linaro.org; linus.wall...@linaro.org;
> adrian.hun...@intel.com; shawn@rock-chips.com; Avri Altman
> ; andriy.shevche...@linux.intel.com
> Cc: linux-...@vger.kernel.org; linux-kernel@vger.kernel.org;
> harish_kand...@mentor.com; vladimir_zapols...@mentor.com
> Subject: [PATCH v6] mmc: Export host capabilities to debugfs.
>
> This patch exports the host capabilities to debugfs
>
> This idea of sharing host capabilities over debugfs came up from Abbas Raza
> Earlier discussions:
> https://lkml.org/lkml/2018/3/5/357
> https://www.spinics.net/lists/linux-mmc/msg48219.html
>
> Signed-off-by: Harish Jenny K N
> ---
>
>
> +static int mmc_caps_show(struct seq_file *s, void *unused) {
> + struct mmc_host *host = s->private;
> + u32 caps = host->caps;
> +
> + seq_puts(s, "\nMMC Host capabilities are:\n");
> + seq_puts(s,
> "=\n");
> + seq_printf(s, "Can the host do 4 bit transfers :\t%s\n",
> +((caps & MMC_CAP_4_BIT_DATA) ? "Yes" : "No"));
Maybe use a more compact form, and just call a macro with the applicable
(stringified) bit?
Thanks,
Avri
RE: [PATCH 5/8] bsg: refactor bsg_ioctl
> On Sun, Nov 11, 2018 at 02:32:08PM +0100, Christoph Hellwig wrote: > > Move all actual functionality into helpers, just leaving the dispatch > > in this function. > > > > Signed-off-by: Christoph Hellwig > > --- > > block/bsg.c | 158 > > 1 file changed, 72 insertions(+), 86 deletions(-) > > > > Looks fine to me. Did ran the same small test-tool I ran against Jens' > patches, nothing broke. > > Reviewed-by: Benjamin Block > Tested-by: Benjamin Block Tested-by: Avri Altman Tested the scsi pass-through (ufs-bsg) path - nothing is broken.
RE: [PATCH 6/8] bsg-lib: handle bidi requests without block layer help
> -Original Message- > From: linux-scsi-ow...@vger.kernel.org > On Behalf Of Christoph Hellwig > Sent: Sunday, November 11, 2018 3:32 PM > To: ax...@kernel.dk; martin.peter...@oracle.com; o...@electrozaur.com > Cc: Johannes Thumshirn ; Benjamin Block > ; linux-s...@vger.kernel.org; linux- > bl...@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: [PATCH 6/8] bsg-lib: handle bidi requests without block layer help > > We can just stash away the second request in struct bsg_job instead > of using the block layer req->next_rq field, allowing for the eventual > removal of the latter. > > Signed-off-by: Christoph Hellwig Tested-by: Avri Altman Regardless of the ongoing discussion with Benjamin - Tested the scsi pass-through (ufs-bsg) path - nothing is broken.
RE: [PATCH] scsi: ufs: revamp string descriptor reading
Hello Tomas, > > Define new a type: uc_string_id for easier string > handling and less casting. Reduce number or string > copies in price of a dynamic allocation. > > Signed-off-by: Tomas Winkler Tested-by: Avri Altman Just one nit - doesn't really matters. Cheers, Avri > --- > drivers/scsi/ufs/ufs-sysfs.c | 20 ++--- > drivers/scsi/ufs/ufs.h | 2 +- > drivers/scsi/ufs/ufshcd.c| 164 +-- > drivers/scsi/ufs/ufshcd.h| 9 +- > 4 files changed, 115 insertions(+), 80 deletions(-) > > ufs_fixup_device_setup(hba, &card); > + ufs_put_device_desc(&card); ufs_get_device_desc() and ufs_put_device_desc() actually serves the quirks setup. Make sense to call it from within so the logic is clear and in one place. Might also save ufs_put_device_desc().
RE: [PATCH 1/7] mmc-utils: interpret OPTIMAL_*_SIZE fields in extcsd
Looks fine. Thanks, Avri > eMMC 5.0 introduced OPTIMAL_READ_SIZE, OPTIMAL_WRITE_SIZE and > OPTIMAL > TRIM_UNIT_SIZE fields in the extcsd > > Interpret these fields when reading out the extcsd with human-readable > results > > Signed-off-by: James Nuss Reviewed-by: Avri Altman
RE: [PATCH 2/7] mmc-utils: treat FIRMWARE_VERSION as binary field instead of string
> +++ b/mmc_cmds.c
> @@ -1758,8 +1758,15 @@ int do_read_extcsd(int nargs, char **argv)
> }
>
> if (ext_csd_rev >= 7) {
> - printf("eMMC Firmware Version: %s\n",
> - (char*)&ext_csd[EXT_CSD_FIRMWARE_VERSION]);
> + printf("Firmware Version:
> 0x%02x%02x%02x%02x%02x%02x%02x%02x\n",
> + ext_csd[EXT_CSD_FIRMWARE_VERSION_7],
> + ext_csd[EXT_CSD_FIRMWARE_VERSION_6],
> + ext_csd[EXT_CSD_FIRMWARE_VERSION_5],
> + ext_csd[EXT_CSD_FIRMWARE_VERSION_4],
> + ext_csd[EXT_CSD_FIRMWARE_VERSION_3],
> + ext_csd[EXT_CSD_FIRMWARE_VERSION_2],
> + ext_csd[EXT_CSD_FIRMWARE_VERSION_1],
> + ext_csd[EXT_CSD_FIRMWARE_VERSION_0]);
ExtCSD[261:254] is an ASCII string, just add a terminating null.
Thanks,
Avri
RE: [PATCH 2/7] mmc-utils: treat FIRMWARE_VERSION as binary field instead of string
>
> On Wed, Oct 10, 2018 at 4:43 AM Avri Altman wrote:
> >
> >
> > > +++ b/mmc_cmds.c
> > > @@ -1758,8 +1758,15 @@ int do_read_extcsd(int nargs, char **argv)
> > > }
> > >
> > > if (ext_csd_rev >= 7) {
> > > - printf("eMMC Firmware Version: %s\n",
> > > - (char*)&ext_csd[EXT_CSD_FIRMWARE_VERSION]);
> > > + printf("Firmware Version:
> > > 0x%02x%02x%02x%02x%02x%02x%02x%02x\n",
> > > + ext_csd[EXT_CSD_FIRMWARE_VERSION_7],
> > > + ext_csd[EXT_CSD_FIRMWARE_VERSION_6],
> > > + ext_csd[EXT_CSD_FIRMWARE_VERSION_5],
> > > + ext_csd[EXT_CSD_FIRMWARE_VERSION_4],
> > > + ext_csd[EXT_CSD_FIRMWARE_VERSION_3],
> > > + ext_csd[EXT_CSD_FIRMWARE_VERSION_2],
> > > + ext_csd[EXT_CSD_FIRMWARE_VERSION_1],
> > > + ext_csd[EXT_CSD_FIRMWARE_VERSION_0]);
> > ExtCSD[261:254] is an ASCII string, just add a terminating null.
>
> Unfortunately I found two different manufacturers which put
> non-printable characters in this 8-byte field. So I don't think it can
> be treated as ASCII in all cases. Printing out the hex value seemed
> liked the most comprehensive solution.
NAK with prejudice.
This interfere with the output that we/our clients expects.
RE: [PATCH v3 1/2] mmc: block: Issue flush only if allowed
> On 20/04/21 8:53 am, Avri Altman wrote:
> > The cache may be flushed to the nonvolatile storage by writing to
> > FLUSH_CACHE byte (EXT_CSD byte [32]). When in command queueing mode,
> the
> > cache may be flushed by issuing a CMDQ_TASK_ DEV_MGMT (CMD48) with a
> > FLUSH_CACHE op-code. Either way, verify that The cache function is
> > turned ON before doing so.
> >
> > fixes: 1e8e55b67030 (mmc: block: Add CQE support)
> >
> > Reported-by: Brendan Peter
> > Tested-by: Brendan Peter
> > Signed-off-by: Avri Altman
> > ---
> > drivers/mmc/core/block.c | 7 +++
> > drivers/mmc/core/mmc.c | 2 +-
> > drivers/mmc/core/mmc_ops.h | 5 +
> > 3 files changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
> > index 8bfd4d95b386..5b6501fc9fb7 100644
> > --- a/drivers/mmc/core/block.c
> > +++ b/drivers/mmc/core/block.c
> > @@ -1476,6 +1476,11 @@ static int mmc_blk_cqe_issue_flush(struct
> mmc_queue *mq, struct request *req)
> > struct mmc_queue_req *mqrq = req_to_mmc_queue_req(req);
> > struct mmc_request *mrq = mmc_blk_cqe_prep_dcmd(mqrq, req);
> >
> > + if (mmc_card_mmc(mq->card) && !mmc_flush_allowed(mq->card)) {
> > + blk_mq_end_request(req, BLK_STS_OK);
> > + return -EPERM;
> > + }
> > +
> > mrq->cmd->opcode = MMC_SWITCH;
> > mrq->cmd->arg = (MMC_SWITCH_MODE_WRITE_BYTE << 24) |
> > (EXT_CSD_FLUSH_CACHE << 16) |
> > @@ -2226,6 +2231,8 @@ enum mmc_issued mmc_blk_mq_issue_rq(struct
> mmc_queue *mq, struct request *req)
> > switch (req_op(req)) {
> > case REQ_OP_FLUSH:
> > ret = mmc_blk_cqe_issue_flush(mq, req);
> > + if (ret == -EPERM)
> > + return MMC_REQ_FINISHED;
>
> Using an error code for this case seems a little fragile.
>
> How about something like:
>
> case REQ_OP_FLUSH:
> if (mmc_blk_cache_disabled(mq->card)) {
> blk_mq_end_request(req, BLK_STS_OK);
> return MMC_REQ_FINISHED;
> }
> ret = mmc_blk_cqe_issue_flush(mq, req);
>
>
> static bool mmc_blk_cache_disabled(struct mmc_card *card)
> {
> return mmc_card_mmc(mq->card) && !mmc_flush_allowed(card);
> }
Done.
Thanks,
Avri
[PATCH v4 0/2] Do not flush cache when it is disabled
v3 -> v4 - Attend Adrian's comments - Add Acked-by tag v2 -> v3: - rebase onto recent cache changes v1 -> v2: - Attend Adrian's comments Cache is a temporary storage space in an eMMC device. Volatile by nature, the cache should in typical case reduce the access time compared to an access to the main nonvolatile storage. The cache function can be turned ON and OFF. Once OFF, the host is not expected to issue a flush-cache command to the device. Avri Altman (2): mmc: block: Issue flush only if allowed mmc: block: Update ext_csd.cache_ctrl if it was written drivers/mmc/core/block.c | 21 + drivers/mmc/core/mmc.c | 2 +- drivers/mmc/core/mmc_ops.h | 5 + 3 files changed, 27 insertions(+), 1 deletion(-) -- 2.25.1
[PATCH v4 1/2] mmc: block: Issue flush only if allowed
The cache may be flushed to the nonvolatile storage by writing to
FLUSH_CACHE byte (EXT_CSD byte [32]). When in command queueing mode, the
cache may be flushed by issuing a CMDQ_TASK_ DEV_MGMT (CMD48) with a
FLUSH_CACHE op-code. Either way, verify that The cache function is
turned ON before doing so.
fixes: 1e8e55b67030 (mmc: block: Add CQE support)
Reported-by: Brendan Peter
Tested-by: Brendan Peter
Signed-off-by: Avri Altman
---
drivers/mmc/core/block.c | 9 +
drivers/mmc/core/mmc.c | 2 +-
drivers/mmc/core/mmc_ops.h | 5 +
3 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 8bfd4d95b386..24e1ecbdd510 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -2186,6 +2186,11 @@ static int mmc_blk_wait_for_idle(struct mmc_queue *mq,
struct mmc_host *host)
return mmc_blk_rw_wait(mq, NULL);
}
+static bool mmc_blk_cache_disabled(struct mmc_card *card)
+{
+ return mmc_card_mmc(card) && !mmc_flush_allowed(card);
+}
+
enum mmc_issued mmc_blk_mq_issue_rq(struct mmc_queue *mq, struct request *req)
{
struct mmc_blk_data *md = mq->blkdata;
@@ -2225,6 +2230,10 @@ enum mmc_issued mmc_blk_mq_issue_rq(struct mmc_queue
*mq, struct request *req)
case MMC_ISSUE_ASYNC:
switch (req_op(req)) {
case REQ_OP_FLUSH:
+ if (mmc_blk_cache_disabled(mq->card)) {
+ blk_mq_end_request(req, BLK_STS_OK);
+ return MMC_REQ_FINISHED;
+ }
ret = mmc_blk_cqe_issue_flush(mq, req);
break;
case REQ_OP_READ:
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index 9ad4aa537867..e3da62ffcb5e 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -2037,7 +2037,7 @@ static int _mmc_flush_cache(struct mmc_card *card)
{
int err = 0;
- if (card->ext_csd.cache_size > 0 && card->ext_csd.cache_ctrl & 1) {
+ if (mmc_flush_allowed(card)) {
err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
EXT_CSD_FLUSH_CACHE, 1,
CACHE_FLUSH_TIMEOUT_MS);
diff --git a/drivers/mmc/core/mmc_ops.h b/drivers/mmc/core/mmc_ops.h
index 5782fdf4e8e9..2682bf66708a 100644
--- a/drivers/mmc/core/mmc_ops.h
+++ b/drivers/mmc/core/mmc_ops.h
@@ -19,6 +19,11 @@ enum mmc_busy_cmd {
struct mmc_host;
struct mmc_card;
+static inline bool mmc_flush_allowed(struct mmc_card *card)
+{
+ return card->ext_csd.cache_size > 0 && card->ext_csd.cache_ctrl & 1;
+}
+
int mmc_select_card(struct mmc_card *card);
int mmc_deselect_cards(struct mmc_host *host);
int mmc_set_dsr(struct mmc_host *host);
--
2.25.1
[PATCH v4 2/2] mmc: block: Update ext_csd.cache_ctrl if it was written
The cache function can be turned ON and OFF by writing to the CACHE_CTRL
byte (EXT_CSD byte [33]). However, card->ext_csd.cache_ctrl is only
set on init if cache size > 0.
Fix that by explicitly setting ext_csd.cache_ctrl on ext-csd write.
Signed-off-by: Avri Altman
Acked-by: Adrian Hunter
---
drivers/mmc/core/block.c | 12
1 file changed, 12 insertions(+)
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 24e1ecbdd510..7e70f11e85e2 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -572,6 +572,18 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card,
struct mmc_blk_data *md,
main_md->part_curr = value & EXT_CSD_PART_CONFIG_ACC_MASK;
}
+ /*
+* Make sure to update CACHE_CTRL in case it was changed. The cache
+* will get turned back on if the card is re-initialized, e.g.
+* suspend/resume or hw reset in recovery.
+*/
+ if ((MMC_EXTRACT_INDEX_FROM_ARG(cmd.arg) == EXT_CSD_CACHE_CTRL) &&
+ (cmd.opcode == MMC_SWITCH)) {
+ u8 value = MMC_EXTRACT_VALUE_FROM_ARG(cmd.arg) & 1;
+
+ card->ext_csd.cache_ctrl = value;
+ }
+
/*
* According to the SD specs, some commands require a delay after
* issuing the command.
--
2.25.1
RE: [PATCH v32 4/4] scsi: ufs: Add HPB 2.0 support
> Hi,
> > if (dev_info->wspecversion >= UFS_DEV_HPB_SUPPORT_VERSION &&
> > (b_ufs_feature_sup & UFS_DEV_HPB_SUPPORT)) {
> > - dev_info->hpb_enabled = true;
> > + bool hpb_en = false;
> > +
> > ufshpb_get_dev_info(hba, desc_buf);
> > +
> > + if (!ufshpb_is_legacy(hba))
> > + err = ufshcd_query_flag_retry(hba,
> > +
> > UPIU_QUERY_OPCODE_READ_FLAG,
> > +
> > QUERY_FLAG_IDN_HPB_EN, 0,
> > + &hpb_en);
> > +
> > + if (ufshpb_is_legacy(hba) || (!err && hpb_en))
> > + dev_info->hpb_enabled = true;
> > }
> I think there is a confusion concerning fHPBEn flag.
> The spec say: "If host wants to enable HPB, host set the fHPBEn flag as ‘1’."
> And its default value is '0'.
> So upon successful init, we should set this flag and not read it.
After some further thinking, I wish to withdraw from my comment above.
The spec doesn't really say how this flag should be used, but provides the
"system"
With a mean to disable hpb.
So I tend to agree with your interpretation, that this flag should be
configured by the OEMs,
Along with all other hpb configurables, should they choose to enable it.
As it is of a persistent nature, we might even consider in the future,
to add some logic that will allow to use this flag to disable hpb.
Thanks,
Avri
>
> I wouldn't rush to fix it however, before we see what Martin/Greg are planning
> for this feature.
> Thanks,
> Avri
RE: [PATCH 4/4] scsi: ufs: make ufshcd_config_pwr_mode of non-static func
> -Original Message- > From: linux-scsi-ow...@vger.kernel.org > On Behalf Of Alim Akhtar > Sent: Sunday, May 06, 2018 1:14 PM > To: linux-s...@vger.kernel.org; linux-kernel@vger.kernel.org > Cc: j...@linux.vnet.ibm.com; martin.peter...@oracle.com; > vivek.gau...@codeaurora.org; subha...@codeaurora.org; > vinholika...@gmail.com; o...@lixom.net; alim.akh...@samsung.com > Subject: [PATCH 4/4] scsi: ufs: make ufshcd_config_pwr_mode of non-static > func > > This makes ufshcd_config_pwr_mode non-static so that other vendors like > exynos can use the same. > > Signed-off-by: Seungwon Jeon > Signed-off-by: Alim Akhtar Acked-by: Avri Altman Might be also useful exporting an API for all uic commands? Thanks, Avri
RE: [PATCH v1] mmc: core: Verify SD bus width
>
> The SD Physical Layer Spec says the following: Since the SD Memory Card
> shall support at least the two bus modes 1-bit or 4-bit width, then any SD
> Card shall set at least bits 0 and 2 (SD_BUS_WIDTH="0101").
>
> This change verifies the card has specified a bus width.
>
> verified it didn't mount.
>
> Signed-off-by: Raul E Rangel
Acked-by: Avri Altman
> ---
> AMD SDHC Device 7806 can get into a bad state after a card disconnect
> where anything transferred via the DATA lines will always result in a
> zero filled buffer. Currently the driver will continue without error if
> the HC is in this condition. A block device will be created, but reading
> from it will result in a zero buffer. This makes it seem like the SD
> device has been erased, when in actuality the data is never getting
> copied from the DATA lines to the data buffer.
>
> SCR is the first command in the SD initialization sequence that uses the
> DATA lines. By checking that the response was invalid, we can abort
> mounting the card.
>
> Here is an example of a bad trace: https://pastebin.com/TY2cF9n0
> Look for sd_scr and sd_ssr.
>
> drivers/mmc/core/sd.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> index 265e1aeeb9d8..f6481f8e9fe7 100644
> --- a/drivers/mmc/core/sd.c
> +++ b/drivers/mmc/core/sd.c
> @@ -205,6 +205,13 @@ static int mmc_decode_scr(struct mmc_card *card)
>
> scr->sda_vsn = UNSTUFF_BITS(resp, 56, 4);
> scr->bus_widths = UNSTUFF_BITS(resp, 48, 4);
> +
> + /* SD Spec says: any SD Card shall set at least bits 0 and 2 */
> + if (!scr->bus_widths) {
So why not testing that exact mask?
you have SD_SCR_BUS_WIDTH_1 and SD_SCR_BUS_WIDTH_4 already defined...
RE: [PATCH v1 1/4] mmc: core: Add trace event for SD SCR response
>
> On Mon, 15 Apr 2019 16:52:38 -0600
> Raul E Rangel wrote:
>
> > Example:
> > sd_scr: mmc0: version: 2, spec3: 1, width: 5, cmds: 0, raw: {0x2b58000,0x0}
> >
> > Signed-off-by: Raul E Rangel
> > ---
> >
> > drivers/mmc/core/sd.c | 4
> > include/trace/events/mmc.h | 42
> ++
> > 2 files changed, 46 insertions(+)
> >
> > diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> > index 265e1aeeb9d8..3bed7c4b6d75 100644
> > --- a/drivers/mmc/core/sd.c
> > +++ b/drivers/mmc/core/sd.c
> > @@ -21,6 +21,8 @@
> > #include
> > #include
> >
> > +#include
> > +
> > #include "core.h"
> > #include "card.h"
> > #include "host.h"
> > @@ -200,6 +202,7 @@ static int mmc_decode_scr(struct mmc_card *card)
> > if (scr_struct != 0) {
> > pr_err("%s: unrecognised SCR structure version %d\n",
> > mmc_hostname(card->host), scr_struct);
> > + trace_sd_scr(card, NULL);
> > return -EINVAL;
> > }
> >
> > @@ -221,6 +224,7 @@ static int mmc_decode_scr(struct mmc_card *card)
> >
> > if (scr->sda_spec3)
> > scr->cmds = UNSTUFF_BITS(resp, 32, 2);
> > + trace_sd_scr(card, scr);
> > return 0;
> > }
> >
> > diff --git a/include/trace/events/mmc.h b/include/trace/events/mmc.h
> > index 7b706ff21335..e45258e8a6cb 100644
> > --- a/include/trace/events/mmc.h
> > +++ b/include/trace/events/mmc.h
> > @@ -10,6 +10,48 @@
> > #include
> > #include
> >
> > +TRACE_EVENT(sd_scr,
As all 3 new trace events (csd, scr, and ssr) are pretty close,
You might want to find a common ground and declare an event class.
Thanks,
Avri
> > +
> > + TP_PROTO(struct mmc_card *card, struct sd_scr *scr),
> > +
> > + TP_ARGS(card, scr),
> > +
> > + TP_STRUCT__entry(
> > + __array(u32,raw,2)
> > + __field(unsigned char, sda_vsn)
> > + __field(unsigned char, sda_spec3)
> > + __field(unsigned char, bus_widths)
> > + __field(unsigned char, cmds)
> > + __string(name, mmc_hostname(card->host))
> > + ),
> > +
> > + TP_fast_assign(
> > + memcpy(__entry->raw, card->raw_scr, sizeof(card-
> >raw_scr));
>
> Having the memcpy() size be that of the source is dangerous. Not that
> the destination will every be smaller, but I would highly suggest
> either trying to see if "sizeof(__entry->raw)" works or at worse,
> sizeof(u32)*2.
>
> -- Steve
>
> > + if (scr) {
> > + __entry->sda_vsn = scr->sda_vsn;
> > + __entry->sda_spec3 = scr->sda_spec3;
> > + __entry->bus_widths = scr->bus_widths;
> > + __entry->cmds = scr->cmds;
> > + } else {
> > + __entry->sda_vsn = 0;
> > + __entry->sda_spec3 = 0;
> > + __entry->bus_widths = 0;
> > + __entry->cmds = 0;
> > + }
> > + __assign_str(name, mmc_hostname(card->host));
> > + ),
> > +
> > + TP_printk("%s: version: %d, spec3: %d, width: %d, cmds: %d, "
> > + "raw: %s",
> > + __get_str(name),
> > + __entry->sda_vsn,
> > + __entry->sda_spec3,
> > + __entry->bus_widths,
> > + __entry->cmds,
> > + __print_array(__entry->raw, 2, sizeof(u32))
> > + )
> > +);
> > +
> > TRACE_EVENT(mmc_request_start,
> >
> > TP_PROTO(struct mmc_host *host, struct mmc_request *mrq),
RE: [PATCH v2] mmc: core: Verify SD bus width
> > The SD Physical Layer Spec says the following: Since the SD Memory Card > shall support at least the two bus modes 1-bit or 4-bit width, then any SD > Card shall set at least bits 0 and 2 (SD_BUS_WIDTH="0101"). > > This change verifies the card has specified a bus width. > > AMD SDHC Device 7806 can get into a bad state after a card disconnect > where anything transferred via the DATA lines will always result in a > zero filled buffer. Currently the driver will continue without error if > the HC is in this condition. A block device will be created, but reading > from it will result in a zero buffer. This makes it seem like the SD > device has been erased, when in actuality the data is never getting > copied from the DATA lines to the data buffer. > > SCR is the first command in the SD initialization sequence that uses the > DATA lines. By checking that the response was invalid, we can abort > mounting the card. > > Acked-by: Avri Altman > > Signed-off-by: Raul E Rangel Reviewed-by: Avri Altman Thanks, Avri
RE: [PATCH v4 3/3] scsi: ufs-bsg: Allow reading descriptors
Martin, Any further comments? Thanks, Avri > -Original Message- > From: Evan Green > Sent: Monday, January 28, 2019 8:42 PM > To: Avri Altman > Cc: James E.J. Bottomley ; Martin K. Petersen > ; SCSI ; LKML > ; Avi Shchislowski ; > Alex Lemberg > Subject: Re: [PATCH v4 3/3] scsi: ufs-bsg: Allow reading descriptors > > On Sat, Jan 26, 2019 at 11:08 PM Avri Altman wrote: > > > > Add this functionality, placing the descriptor being read in the actual > > data buffer in the bio. > > > > That is, for both read and write descriptors query upiu, we are using > > the job's request_payload. This in turn, is mapped back in user land to > > the applicable sg_io_v4 xferp: dout_xferp for write descriptor, > > and din_xferp for read descriptor. > > > > Signed-off-by: Avri Altman > > Reviewed-by: Evan Green
RE: [PATCH 1/3] mmc: core: Calculate the discard arg only once
> On Sun, 3 Feb 2019 at 09:51, Avri Altman wrote:
> >
> > The discard arg is a read-only ext_csd parameter -
> > set it once on card init.
>
> I like the idea here. There is really no point checking this for every
> corresponding request, nice!
>
> However, the "discard arg" isn't specific to eMMC, as it's also used
> for SD cards. So, the change log is a bit miss-leading, I think. Can
> you please clarify this.
Done.
> >
> > + /* set discard_arg */
> > + if (mmc_can_discard(card))
> > + card->discard_arg = MMC_DISCARD_ARG;
> > + else if (mmc_can_trim(card))
> > + card->discard_arg = MMC_TRIM_ARG;
> > + else
> > + card->discard_arg = MMC_ERASE_ARG;
>
> You are assigning card->discard_arg only for (e)MMC, while I think you
> should do that also for SD.
>
> I guess it practice it doesn't matter, because MMC_ERASE_ARG is zero.
> Although, I would prefer to keep the code consistent, so I think you
> should assign card->discard_arg for for SD cards as well.
Done.
> > diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> > index de73778..447648b 100644
> > --- a/include/linux/mmc/card.h
> > +++ b/include/linux/mmc/card.h
> > @@ -308,6 +308,8 @@ struct mmc_card {
> > unsigned intnr_parts;
> >
> > unsigned intbouncesz; /* Bounce buffer size */
> > +
> > + unsigned intdiscard_arg;/* discard args */
>
> Please move this a couple of lines above, close to the other "erase"
> related variables.
Done.
>
> You may even consider to rename the arg to "erase_arg", but I have no
> strong opinion changing to that.
Done.
>
> > };
> >
> > static inline bool mmc_large_sector(struct mmc_card *card)
> > --
> > 1.9.1
> >
>
> Kind regards
> Uffe
[PATCH v2 1/3] mmc: core: Calculate the discard arg only once
In MMC, the discard arg is a read-only ext_csd parameter - set it once
on card init. To be consistent, do that for SD as well even though its
discard arg is always 0x0.
Signed-off-by: Avri Altman
---
drivers/mmc/core/block.c | 12 +++-
drivers/mmc/core/core.c | 4 ++--
drivers/mmc/core/mmc.c | 8
drivers/mmc/core/sd.c| 2 ++
include/linux/mmc/card.h | 1 +
include/linux/mmc/sd.h | 5 +
6 files changed, 21 insertions(+), 11 deletions(-)
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index b08fb91..131e80e 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -1124,7 +1124,7 @@ static void mmc_blk_issue_discard_rq(struct mmc_queue
*mq, struct request *req)
{
struct mmc_blk_data *md = mq->blkdata;
struct mmc_card *card = md->queue.card;
- unsigned int from, nr, arg;
+ unsigned int from, nr;
int err = 0, type = MMC_BLK_DISCARD;
blk_status_t status = BLK_STS_OK;
@@ -1136,24 +1136,18 @@ static void mmc_blk_issue_discard_rq(struct mmc_queue
*mq, struct request *req)
from = blk_rq_pos(req);
nr = blk_rq_sectors(req);
- if (mmc_can_discard(card))
- arg = MMC_DISCARD_ARG;
- else if (mmc_can_trim(card))
- arg = MMC_TRIM_ARG;
- else
- arg = MMC_ERASE_ARG;
do {
err = 0;
if (card->quirks & MMC_QUIRK_INAND_CMD38) {
err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
INAND_CMD38_ARG_EXT_CSD,
-arg == MMC_TRIM_ARG ?
+card->erase_arg == MMC_TRIM_ARG ?
INAND_CMD38_ARG_TRIM :
INAND_CMD38_ARG_ERASE,
0);
}
if (!err)
- err = mmc_erase(card, from, nr, arg);
+ err = mmc_erase(card, from, nr, card->erase_arg);
} while (err == -EIO && !mmc_blk_reset(md, card->host, type));
if (err)
status = BLK_STS_IOERR;
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 5bd58b9..de0f1a1 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2164,7 +2164,7 @@ static unsigned int mmc_align_erase_size(struct mmc_card
*card,
* @card: card to erase
* @from: first sector to erase
* @nr: number of sectors to erase
- * @arg: erase command argument (SD supports only %MMC_ERASE_ARG)
+ * @arg: erase command argument (SD supports only %SD_ERASE_ARG)
*
* Caller must claim host before calling this function.
*/
@@ -2181,7 +2181,7 @@ int mmc_erase(struct mmc_card *card, unsigned int from,
unsigned int nr,
if (!card->erase_size)
return -EOPNOTSUPP;
- if (mmc_card_sd(card) && arg != MMC_ERASE_ARG)
+ if (mmc_card_sd(card) && arg != SD_ERASE_ARG)
return -EOPNOTSUPP;
if ((arg & MMC_SECURE_ARGS) &&
diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
index da892a5..09c688f 100644
--- a/drivers/mmc/core/mmc.c
+++ b/drivers/mmc/core/mmc.c
@@ -1743,6 +1743,14 @@ static int mmc_init_card(struct mmc_host *host, u32 ocr,
card->ext_csd.power_off_notification = EXT_CSD_POWER_ON;
}
+ /* set erase_arg */
+ if (mmc_can_discard(card))
+ card->erase_arg = MMC_DISCARD_ARG;
+ else if (mmc_can_trim(card))
+ card->erase_arg = MMC_TRIM_ARG;
+ else
+ card->erase_arg = MMC_ERASE_ARG;
+
/*
* Select timing interface
*/
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index d0d9f90..bd48b28 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -271,6 +271,8 @@ static int mmc_read_ssr(struct mmc_card *card)
}
}
+ card->erase_arg = SD_ERASE_ARG;
+
return 0;
}
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index de73778..8f429b6 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -277,6 +277,7 @@ struct mmc_card {
unsigned interase_shift;/* if erase unit is power 2 */
unsigned intpref_erase; /* in sectors */
unsigned integ_boundary;/* don't cross erase-group
boundaries */
+ unsigned interase_arg; /* erase / trim / discard */
u8 erased_byte;/* value of erased bytes */
u32 raw_cid[4]; /* raw card CID */
diff --git a/include/linux/mmc/sd.h b/include/linux/mmc/sd.h
index 1ebcf9b..1a6d10f 100644
--- a/include/linux/mmc/sd.h
+++ b/include/linux/mmc/sd.h
@@ -91,4 +91,9 @@
#define SD_SWITCH_ACCESS_DEF 0
#d
[PATCH v2 2/3] mmc: core: Indicate SD specs higher than 4.0
SD specs version 4.x and 5.x have a dedicated slices in the SCR register.
Higher versions will rely on a combination of the existing fields.
Signed-off-by: Avri Altman
---
drivers/mmc/core/sd.c| 5 +
include/linux/mmc/card.h | 2 ++
2 files changed, 7 insertions(+)
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index bd48b28..c2db94d 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -209,6 +209,11 @@ static int mmc_decode_scr(struct mmc_card *card)
/* Check if Physical Layer Spec v3.0 is supported */
scr->sda_spec3 = UNSTUFF_BITS(resp, 47, 1);
+ if (scr->sda_spec3) {
+ scr->sda_spec4 = UNSTUFF_BITS(resp, 42, 1);
+ scr->sda_specx = UNSTUFF_BITS(resp, 38, 4);
+ }
+
if (UNSTUFF_BITS(resp, 55, 1))
card->erased_byte = 0xFF;
else
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index 8f429b6..d791813f 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -133,6 +133,8 @@ struct mmc_ext_csd {
struct sd_scr {
unsigned char sda_vsn;
unsigned char sda_spec3;
+ unsigned char sda_spec4;
+ unsigned char sda_specx;
unsigned char bus_widths;
#define SD_SCR_BUS_WIDTH_1 (1<<0)
#define SD_SCR_BUS_WIDTH_4 (1<<2)
--
1.9.1
[PATCH v2 0/3] mmc: core: Add SD Discard support
SD spec v5.1 adds discard support. The flows and commands matches those in eMMC, Which leaves to set the appropriate discard arg in CMD38 if DISCARD_SUPPORT (b313) is set in the SD_STATUS register. We set this arg on card init: not in mmc_init_erase as one might expect but arbitrarily once the card indicated its discard support. This is because unlike erase, it doesn't really involve any logic, and we want to avoid the unnecessary complication. V1->v2: In the first patch, assign the discard arg for SD cards as well to keep the code consistent. Rename "discard_arg" to "erase_arg", and elaborate the change log. Avri Altman (3): mmc: core: Calculate the discard arg only once mmc: core: Indicate SD specs higher than 4.0 mmc: core: Add discard support to sd drivers/mmc/core/block.c | 12 +++- drivers/mmc/core/core.c | 8 ++-- drivers/mmc/core/mmc.c | 8 drivers/mmc/core/sd.c| 15 +++ include/linux/mmc/card.h | 3 +++ include/linux/mmc/sd.h | 6 ++ 6 files changed, 41 insertions(+), 11 deletions(-) -- 1.9.1
[PATCH v2 3/3] mmc: core: Add discard support to sd
SD spec v5.1 adds discard support. The flows and commands are similar to
mmc, so just set the discard arg in CMD38.
Actually, there is no need to check for the spec version like we are
doing, as it is assured that the reserved bits in earlier versions are
null. Do that anyway to document the spec version that introduce it.
Signed-off-by: Avri Altman
---
drivers/mmc/core/core.c | 6 +-
drivers/mmc/core/sd.c | 10 +-
include/linux/mmc/sd.h | 1 +
3 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index de0f1a1..4d62f28 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2164,7 +2164,7 @@ static unsigned int mmc_align_erase_size(struct mmc_card
*card,
* @card: card to erase
* @from: first sector to erase
* @nr: number of sectors to erase
- * @arg: erase command argument (SD supports only %SD_ERASE_ARG)
+ * @arg: erase command argument
*
* Caller must claim host before calling this function.
*/
@@ -2181,6 +2181,9 @@ int mmc_erase(struct mmc_card *card, unsigned int from,
unsigned int nr,
if (!card->erase_size)
return -EOPNOTSUPP;
+ if (mmc_card_sd(card) && arg == SD_DISCARD_ARG)
+ goto skip_arg_testing;
+
if (mmc_card_sd(card) && arg != SD_ERASE_ARG)
return -EOPNOTSUPP;
@@ -2200,6 +2203,7 @@ int mmc_erase(struct mmc_card *card, unsigned int from,
unsigned int nr,
if (arg == MMC_ERASE_ARG)
nr = mmc_align_erase_size(card, &from, &to, nr);
+skip_arg_testing:
if (nr == 0)
return 0;
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index c2db94d..2b4fc22 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -231,6 +231,8 @@ static int mmc_read_ssr(struct mmc_card *card)
{
unsigned int au, es, et, eo;
__be32 *raw_ssr;
+ u32 resp[4] = {};
+ u8 discard_support;
int i;
if (!(card->csd.cmdclass & CCC_APP_SPEC)) {
@@ -276,7 +278,13 @@ static int mmc_read_ssr(struct mmc_card *card)
}
}
- card->erase_arg = SD_ERASE_ARG;
+ /*
+* starting SD5.1 discard is supported if DISCARD_SUPPORT (b313) is set
+*/
+ resp[3] = card->raw_ssr[6];
+ discard_support = UNSTUFF_BITS(resp, 313 - 288, 1);
+ card->erase_arg = (card->scr.sda_specx && discard_support) ?
+ SD_DISCARD_ARG : SD_ERASE_ARG;
return 0;
}
diff --git a/include/linux/mmc/sd.h b/include/linux/mmc/sd.h
index 1a6d10f..ec94a5a 100644
--- a/include/linux/mmc/sd.h
+++ b/include/linux/mmc/sd.h
@@ -95,5 +95,6 @@
* Erase/discard
*/
#define SD_ERASE_ARG 0x
+#define SD_DISCARD_ARG 0x0001
#endif /* LINUX_MMC_SD_H */
--
1.9.1
RE: [PATCH v3 6/8] scsi: ufs: qcom: Expose the reset controller for PHY
Hi, > On 06/02/19 12:29 AM, Evan Green wrote: > > Expose a reset controller that the phy will later use to control its > > own PHY reset in the UFS controller. This will enable the combining > > of PHY init functionality into a single function. > > > > Signed-off-by: Evan Green > > I'd like to get ACK from scsi/ufs/ MAINTAINER Vinayak for me merge it in PHY > tree. Looks like this series is qcom specific, and has less impact of the ufs core driver. > > + err = devm_reset_controller_register(dev, &host->rcdev); Just my 2 cents: Isn't this should be done somewhere in drivers/clk/qcom, Like its done for any other qcom board?
[PATCH v4 1/3] scsi: ufs-bsg: Change the calling convention for write descriptor
When we had a write descriptor query upiu, we appended the descriptor
right after the bsg request. This was fine as the bsg driver allows to
allocate whatever buffer we needed in its job request.
Still, the proper way to deliver payload, however small (we only write
config descriptors of 144 bytes), is by using the job request payload
data buffer.
So change this ABI now, while ufs-bsg is still new, and nobody is
actually using it.
Signed-off-by: Avri Altman
Reviewed-by: Evan Green
---
Documentation/scsi/ufs.txt | 6 ++
drivers/scsi/ufs/ufs_bsg.c | 47 +-
2 files changed, 32 insertions(+), 21 deletions(-)
diff --git a/Documentation/scsi/ufs.txt b/Documentation/scsi/ufs.txt
index 520b5b0..78fe7cb 100644
--- a/Documentation/scsi/ufs.txt
+++ b/Documentation/scsi/ufs.txt
@@ -147,6 +147,12 @@ send SG_IO with the applicable sg_io_v4:
io_hdr_v4.max_response_len = reply_len;
io_hdr_v4.request_len = request_len;
io_hdr_v4.request = (__u64)request_upiu;
+ if (dir == SG_DXFER_TO_DEV) {
+ io_hdr_v4.dout_xfer_len = (uint32_t)byte_cnt;
+ io_hdr_v4.dout_xferp = (uintptr_t)(__u64)buff;
+ }
+
+If you wish to write a descriptor, use the dout_xferp sg_io_v4.
UFS Specifications can be found at,
UFS - http://www.jedec.org/sites/default/files/docs/JESD220.pdf
diff --git a/drivers/scsi/ufs/ufs_bsg.c b/drivers/scsi/ufs/ufs_bsg.c
index 775bb4e..2fd0769 100644
--- a/drivers/scsi/ufs/ufs_bsg.c
+++ b/drivers/scsi/ufs/ufs_bsg.c
@@ -27,15 +27,11 @@ static int ufs_bsg_get_query_desc_size(struct ufs_hba *hba,
int *desc_len,
static int ufs_bsg_verify_query_size(struct ufs_hba *hba,
unsigned int request_len,
-unsigned int reply_len,
-int desc_len, enum query_opcode desc_op)
+unsigned int reply_len)
{
int min_req_len = sizeof(struct ufs_bsg_request);
int min_rsp_len = sizeof(struct ufs_bsg_reply);
- if (desc_op == UPIU_QUERY_OPCODE_WRITE_DESC)
- min_req_len += desc_len;
-
if (min_req_len > request_len || min_rsp_len > reply_len) {
dev_err(hba->dev, "not enough space assigned\n");
return -EINVAL;
@@ -44,14 +40,13 @@ static int ufs_bsg_verify_query_size(struct ufs_hba *hba,
return 0;
}
-static int ufs_bsg_verify_query_params(struct ufs_hba *hba,
- struct ufs_bsg_request *bsg_request,
- unsigned int request_len,
- unsigned int reply_len,
- uint8_t *desc_buff, int *desc_len,
- enum query_opcode desc_op)
+static int ufs_bsg_alloc_desc_buffer(struct ufs_hba *hba, struct bsg_job *job,
+uint8_t **desc_buff, int *desc_len,
+enum query_opcode desc_op)
{
+ struct ufs_bsg_request *bsg_request = job->request;
struct utp_upiu_query *qr;
+ u8 *descp;
if (desc_op == UPIU_QUERY_OPCODE_READ_DESC) {
dev_err(hba->dev, "unsupported opcode %d\n", desc_op);
@@ -67,11 +62,19 @@ static int ufs_bsg_verify_query_params(struct ufs_hba *hba,
return -EINVAL;
}
- if (ufs_bsg_verify_query_size(hba, request_len, reply_len, *desc_len,
- desc_op))
+ if (*desc_len > job->request_payload.payload_len) {
+ dev_err(hba->dev, "Illegal desc size\n");
return -EINVAL;
+ }
+
+ descp = kzalloc(*desc_len, GFP_KERNEL);
+ if (!descp)
+ return -ENOMEM;
- desc_buff = (uint8_t *)(bsg_request + 1);
+ sg_copy_to_buffer(job->request_payload.sg_list,
+ job->request_payload.sg_cnt, descp, *desc_len);
+
+ *desc_buff = descp;
out:
return 0;
@@ -91,7 +94,7 @@ static int ufs_bsg_request(struct bsg_job *job)
enum query_opcode desc_op = UPIU_QUERY_OPCODE_NOP;
int ret;
- ret = ufs_bsg_verify_query_size(hba, req_len, reply_len, 0, desc_op);
+ ret = ufs_bsg_verify_query_size(hba, req_len, reply_len);
if (ret)
goto out;
@@ -101,9 +104,8 @@ static int ufs_bsg_request(struct bsg_job *job)
switch (msgcode) {
case UPIU_TRANSACTION_QUERY_REQ:
desc_op = bsg_request->upiu_req.qr.opcode;
- ret = ufs_bsg_verify_query_params(hba, bsg_request, req_len,
- reply_len, desc_buff,
- &desc_len, desc_op);
+ ret = ufs_bsg_alloc_desc_buffer(hba, job, &desc_buff,
+
[PATCH v4 0/3] scsi: ufs-bsg: Add read descriptor
UFS Protocol Information Units (UPIU) are UFS packets that travel between the host and the device on the UniPro bus. Our previous series added the capability to send UPIUs to the ufs driver. It does not cover all the possible UPIU types - we are mainly focused on device management, provisioning, testing and validation, so it covers UPIUs that falls in that box. Our intension is to publish ufs-utils soon - an open source user space utility that relies on that infrastructure to perform those tasks. This short series is adding one last functionality needed by ufs-utils that was somehow left behind - allowing reading descriptors as well. V3->v4: Improve code readability in ufs-bsg: Allow reading descriptors Update Reviewed-by tag. V2->v3: Add a prep patch with write descriptor calling convention changes. Elaborate the commit log of ufs-bsg: Allow reading descriptors Add Reviewed-by tag. v1->v2: Withdraw from the attempt to change the reply buffer, instead place the descriptor being read in the actual data buffer in the bio. Avri Altman (3): scsi: ufs-bsg: Change the calling convention for write descriptor scsi: ufs: Allow reading descriptor via raw upiu scsi: ufs-bsg: Allow reading descriptors Documentation/scsi/ufs.txt | 11 drivers/scsi/ufs/ufs_bsg.c | 63 ++ drivers/scsi/ufs/ufshcd.c | 20 ++- 3 files changed, 61 insertions(+), 33 deletions(-) -- 1.9.1
[PATCH v4 3/3] scsi: ufs-bsg: Allow reading descriptors
Add this functionality, placing the descriptor being read in the actual
data buffer in the bio.
That is, for both read and write descriptors query upiu, we are using
the job's request_payload. This in turn, is mapped back in user land to
the applicable sg_io_v4 xferp: dout_xferp for write descriptor,
and din_xferp for read descriptor.
Signed-off-by: Avri Altman
---
Documentation/scsi/ufs.txt | 7 ++-
drivers/scsi/ufs/ufs_bsg.c | 20
2 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/Documentation/scsi/ufs.txt b/Documentation/scsi/ufs.txt
index 78fe7cb..1769f71 100644
--- a/Documentation/scsi/ufs.txt
+++ b/Documentation/scsi/ufs.txt
@@ -150,9 +150,14 @@ send SG_IO with the applicable sg_io_v4:
if (dir == SG_DXFER_TO_DEV) {
io_hdr_v4.dout_xfer_len = (uint32_t)byte_cnt;
io_hdr_v4.dout_xferp = (uintptr_t)(__u64)buff;
+ } else {
+ io_hdr_v4.din_xfer_len = (uint32_t)byte_cnt;
+ io_hdr_v4.din_xferp = (uintptr_t)(__u64)buff;
}
-If you wish to write a descriptor, use the dout_xferp sg_io_v4.
+If you wish to read or write a descriptor, use the appropriate xferp of
+sg_io_v4.
+
UFS Specifications can be found at,
UFS - http://www.jedec.org/sites/default/files/docs/JESD220.pdf
diff --git a/drivers/scsi/ufs/ufs_bsg.c b/drivers/scsi/ufs/ufs_bsg.c
index 2fd0769..869e71f 100644
--- a/drivers/scsi/ufs/ufs_bsg.c
+++ b/drivers/scsi/ufs/ufs_bsg.c
@@ -48,12 +48,8 @@ static int ufs_bsg_alloc_desc_buffer(struct ufs_hba *hba,
struct bsg_job *job,
struct utp_upiu_query *qr;
u8 *descp;
- if (desc_op == UPIU_QUERY_OPCODE_READ_DESC) {
- dev_err(hba->dev, "unsupported opcode %d\n", desc_op);
- return -ENOTSUPP;
- }
-
- if (desc_op != UPIU_QUERY_OPCODE_WRITE_DESC)
+ if (desc_op != UPIU_QUERY_OPCODE_WRITE_DESC &&
+ desc_op != UPIU_QUERY_OPCODE_READ_DESC)
goto out;
qr = &bsg_request->upiu_req.qr;
@@ -71,8 +67,10 @@ static int ufs_bsg_alloc_desc_buffer(struct ufs_hba *hba,
struct bsg_job *job,
if (!descp)
return -ENOMEM;
- sg_copy_to_buffer(job->request_payload.sg_list,
- job->request_payload.sg_cnt, descp, *desc_len);
+ if (desc_op == UPIU_QUERY_OPCODE_WRITE_DESC)
+ sg_copy_to_buffer(job->request_payload.sg_list,
+ job->request_payload.sg_cnt, descp,
+ *desc_len);
*desc_buff = descp;
@@ -140,6 +138,12 @@ static int ufs_bsg_request(struct bsg_job *job)
if (!desc_buff)
goto out;
+ if (desc_op == UPIU_QUERY_OPCODE_READ_DESC && desc_len)
+ bsg_reply->reply_payload_rcv_len =
+ sg_copy_from_buffer(job->request_payload.sg_list,
+ job->request_payload.sg_cnt,
+ desc_buff, desc_len);
+
kfree(desc_buff);
out:
--
1.9.1
[PATCH v4 2/3] scsi: ufs: Allow reading descriptor via raw upiu
Allow to read descriptors via raw upiu. This in fact was forbidden just
as a precaution, as ufs-bsg actually enforces which functionality is
supported.
Signed-off-by: Avri Altman
Reviewed-by: Evan Green
---
drivers/scsi/ufs/ufshcd.c | 20 ++--
1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 2ddf244..6b9ed21 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -5775,6 +5775,20 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba
*hba,
/* just copy the upiu response as it is */
memcpy(rsp_upiu, lrbp->ucd_rsp_ptr, sizeof(*rsp_upiu));
+ if (desc_buff && desc_op == UPIU_QUERY_OPCODE_READ_DESC) {
+ u8 *descp = (u8 *)lrbp->ucd_rsp_ptr + sizeof(*rsp_upiu);
+ u16 resp_len = be32_to_cpu(lrbp->ucd_rsp_ptr->header.dword_2) &
+ MASK_QUERY_DATA_SEG_LEN;
+
+ if (*buff_len >= resp_len) {
+ memcpy(desc_buff, descp, resp_len);
+ *buff_len = resp_len;
+ } else {
+ dev_warn(hba->dev, "rsp size is bigger than buffer");
+ *buff_len = 0;
+ err = -EINVAL;
+ }
+ }
ufshcd_put_dev_cmd_tag(hba, tag);
wake_up(&hba->dev_cmd.tag_wq);
@@ -5810,11 +5824,6 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba,
int ocs_value;
u8 tm_f = be32_to_cpu(req_upiu->header.dword_1) >> 16 & MASK_TM_FUNC;
- if (desc_buff && desc_op != UPIU_QUERY_OPCODE_WRITE_DESC) {
- err = -ENOTSUPP;
- goto out;
- }
-
switch (msgcode) {
case UPIU_TRANSACTION_NOP_OUT:
cmd_type = DEV_CMD_TYPE_NOP;
@@ -5855,7 +5864,6 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba,
break;
}
-out:
return err;
}
--
1.9.1
RE: [PATCH v2 3/3] mmc: core: Add discard support to sd
Ulf hi,
Thanks a lot for your comments.
>
> On Wed, 6 Feb 2019 at 12:29, Avri Altman wrote:
> >
> > SD spec v5.1 adds discard support. The flows and commands are similar to
> > mmc, so just set the discard arg in CMD38.
>
> So this means that we from now on, if the SD card supports discard, we
> are going to use it in favor of erase. This is consistent with how we
> treat eMMC, so I think it makes sense. Could you perhaps fold in some
> information about this in the changelog, to make this clear on what
> this change means.
Done.
>
> You may also want to explain a little bit what the difference is from
> the SD card storage point of view. Like after an erase, it either 1 or
> 0s on the media, while after a discard it can be whatever.
Done.
>
> >
> > Actually, there is no need to check for the spec version like we are
> > doing, as it is assured that the reserved bits in earlier versions are
> > null. Do that anyway to document the spec version that introduce it.
>
> The check is needed for other purposes, so please just drop this statement.
Done.
>
> >
> > Signed-off-by: Avri Altman
> > ---
> > drivers/mmc/core/core.c | 6 +-
> > drivers/mmc/core/sd.c | 10 +-
> > include/linux/mmc/sd.h | 1 +
> > 3 files changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> > index de0f1a1..4d62f28 100644
> > --- a/drivers/mmc/core/core.c
> > +++ b/drivers/mmc/core/core.c
> > @@ -2164,7 +2164,7 @@ static unsigned int mmc_align_erase_size(struct
> mmc_card *card,
> > * @card: card to erase
> > * @from: first sector to erase
> > * @nr: number of sectors to erase
> > - * @arg: erase command argument (SD supports only %SD_ERASE_ARG)
> > + * @arg: erase command argument
> > *
> > * Caller must claim host before calling this function.
> > */
> > @@ -2181,6 +2181,9 @@ int mmc_erase(struct mmc_card *card, unsigned
> int from, unsigned int nr,
> > if (!card->erase_size)
> > return -EOPNOTSUPP;
> >
> > + if (mmc_card_sd(card) && arg == SD_DISCARD_ARG)
> > + goto skip_arg_testing;
> > +
>
> This isn't consistent with the rest of the code path in this function,
> please adopt to that.
Done.
>
> > if (mmc_card_sd(card) && arg != SD_ERASE_ARG)
>
> Couldn't you add a check for !SD_DISCARD_ARG here instead?
Done.
>
> > return -EOPNOTSUPP;
> >
> > @@ -2200,6 +2203,7 @@ int mmc_erase(struct mmc_card *card, unsigned
> int from, unsigned int nr,
> > if (arg == MMC_ERASE_ARG)
> > nr = mmc_align_erase_size(card, &from, &to, nr);
> >
> > +skip_arg_testing:
> > if (nr == 0)
> > return 0;
> >
> > diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
> > index c2db94d..2b4fc22 100644
> > --- a/drivers/mmc/core/sd.c
> > +++ b/drivers/mmc/core/sd.c
> > @@ -231,6 +231,8 @@ static int mmc_read_ssr(struct mmc_card *card)
> > {
> > unsigned int au, es, et, eo;
> > __be32 *raw_ssr;
> > + u32 resp[4] = {};
> > + u8 discard_support;
> > int i;
> >
> > if (!(card->csd.cmdclass & CCC_APP_SPEC)) {
> > @@ -276,7 +278,13 @@ static int mmc_read_ssr(struct mmc_card *card)
> > }
> > }
> >
> > - card->erase_arg = SD_ERASE_ARG;
> > + /*
> > +* starting SD5.1 discard is supported if DISCARD_SUPPORT (b313) is
> set
> > +*/
> > + resp[3] = card->raw_ssr[6];
> > + discard_support = UNSTUFF_BITS(resp, 313 - 288, 1);
>
> Couldn't you just replace this with "discard_support =
> UNSTUFF_BITS(card->raw_ssr, 313 - 256, 1);" ?
SD status register is transmitted from 512bit onwards.
The 512th bit is the one transmitted first and so on.
So the 313th bit has to be checked at byte offset 0x18 in which both discard
and FULE were enabled.
Byte offset 0x18 means dword offset 6, and UNSTUFF_BITS accounts for 4 dwords
only.
Using UNSTUFF_BITS(card->raw_ssr,) you can only access bits 511..383, but
not 313.
>
> > + card->erase_arg = (card->scr.sda_specx && discard_support) ?
> > + SD_DISCARD_ARG : SD_ERASE_ARG;
> >
> > return 0;
> > }
> > diff --git a/include/linux/mmc/sd.h b/include/linux/mmc/sd.h
> > index 1a6d10f..ec94a5a 100644
> > --- a/include/linux/mmc/sd.h
> >
RE: [PATCH v5 1/2] scsi: ufs: Do not disable vccq in UFSHC driver
>
> Commit 60f0187031c0 ("disable vccq if it's not needed by UFS device")
> introduced a small power optimization as a driver quirk: ignore the
> vccq load specified in the UFSHC DT node when said host controller
> is connected to specific Flash chips (Samsung and Hynix currently).
>
> Unfortunately, this optimization breaks UFS on systems where vccq
> powers not only the Flash chip, but the host controller as well,
> such as APQ8098 MEDIABOX or MTP8998:
>
> [3.929877] ufshcd-qcom 1da4000.ufshc: ufshcd_query_attr: opcode 0x04
> for idn 13 failed, index 0, err = -11
> [5.433815] ufshcd-qcom 1da4000.ufshc: ufshcd_query_attr: opcode 0x04
> for idn 13 failed, index 0, err = -11
> [6.937771] ufshcd-qcom 1da4000.ufshc: ufshcd_query_attr: opcode 0x04
> for idn 13 failed, index 0, err = -11
> [6.937866] ufshcd-qcom 1da4000.ufshc: ufshcd_query_attr_retry: query
> attribute, idn 13, failed with error -11 after 3 retires
> [6.946412] ufshcd-qcom 1da4000.ufshc: ufshcd_disable_auto_bkops:
> failed to enable exception event -11
> [6.957972] ufshcd-qcom 1da4000.ufshc: dme-peer-get: attr-id 0x1587
> failed 3 retries
> [6.967181] ufshcd-qcom 1da4000.ufshc: dme-peer-get: attr-id 0x1586
> failed 3 retries
> [6.975025] ufshcd-qcom 1da4000.ufshc: ufshcd_get_max_pwr_mode:
> invalid max pwm tx gear read = 0
> [6.982755] ufshcd-qcom 1da4000.ufshc: ufshcd_probe_hba: Failed getting
> max supported power mode
> [8.505770] ufshcd-qcom 1da4000.ufshc: ufshcd_query_flag: Sending flag
> query for idn 3 failed, err = -11
> [ 10.009807] ufshcd-qcom 1da4000.ufshc: ufshcd_query_flag: Sending flag
> query for idn 3 failed, err = -11
> [ 11.513766] ufshcd-qcom 1da4000.ufshc: ufshcd_query_flag: Sending flag
> query for idn 3 failed, err = -11
> [ 11.513861] ufshcd-qcom 1da4000.ufshc: ufshcd_query_flag_retry: query
> attribute, opcode 5, idn 3, failed with error -11 after 3 retires
> [ 13.049807] ufshcd-qcom 1da4000.ufshc: __ufshcd_query_descriptor:
> opcode 0x01 for idn 8 failed, index 0, err = -11
> [ 14.553768] ufshcd-qcom 1da4000.ufshc: __ufshcd_query_descriptor:
> opcode 0x01 for idn 8 failed, index 0, err = -11
> [ 16.057767] ufshcd-qcom 1da4000.ufshc: __ufshcd_query_descriptor:
> opcode 0x01 for idn 8 failed, index 0, err = -11
> [ 16.057872] ufshcd-qcom 1da4000.ufshc: ufshcd_read_desc_param: Failed
> reading descriptor. desc_id 8, desc_index 0, param_offset 0, ret -11
> [ 16.067109] ufshcd-qcom 1da4000.ufshc: ufshcd_init_icc_levels: Failed
> reading power descriptor.len = 98 ret = -11
> [ 37.073787] ufshcd-qcom 1da4000.ufshc: link startup failed 1
>
> In my opinion, the rationale for the original patch is questionable.
> If neither the UFSHC, nor the Flash chip, require any load from vccq,
> then that power rail should simply not be specified at all in the DT.
>
> Working around that fact in the driver is detrimental, as evidenced
> by the failure to initialize the host controller on MSM8998.
>
> Revert the original patch, and clean up loose ends in the next patch.
>
> Relevant patches:
>
> 60f0187031c05e04cbadffb62f557d0ff3564490
> c58ab7aab71e2c783087115f0ce1623c2fdcf0b2
> 46c1cf706076500cdcde3445be97233793eec7f1
>
> Signed-off-by: Marc Gonzalez
Acked-by: Avri Altman
RE: [PATCH v5 2/2] scsi: ufs: Remove unused device quirks
> > The UFSHC driver defines a few quirks that are not used anywhere: > > UFS_DEVICE_QUIRK_BROKEN_LCC > UFS_DEVICE_NO_VCCQ > UFS_DEVICE_QUIRK_NO_LINK_OFF > UFS_DEVICE_NO_FASTAUTO > > Let's remove them. > > Signed-off-by: Marc Gonzalez Acked-by: Avri Altman
RE: [REPOST PATCH v4 0/3] scsi: ufs-bsg: Add read descriptor
Martin, Any further comments concerning this series? Thanks, Avri > > UFS Protocol Information Units (UPIU) are UFS packets that travel > between the host and the device on the UniPro bus. Our previous series > added the capability to send UPIUs to the ufs driver. It does not cover > all the possible UPIU types - we are mainly focused on device management, > provisioning, testing and validation, so it covers UPIUs that falls in > that box. > > Our intension is to publish ufs-utils soon - an open source user space > utility that relies on that infrastructure to perform those tasks. > This short series is adding one last functionality needed by ufs-utils > that was somehow left behind - allowing reading descriptors as well. > > Repost of v4 and adds tags already received. > While at it, it turns out that quite a few people are waiting for > ufs-utils, e.g. https://www.spinics.net/lists/linux-scsi/msg127807.html. > So I expect that the small, but vibrant community of ufs users, will try > to expand this infrastructure even further. > > V3->v4: > Improve code readability in ufs-bsg: Allow reading descriptors > Update Reviewed-by tag. > > V2->v3: > Add a prep patch with write descriptor calling convention changes. > Elaborate the commit log of ufs-bsg: Allow reading descriptors > Add Reviewed-by tag. > > v1->v2: > Withdraw from the attempt to change the reply buffer, instead place the > descriptor being read in the actual data buffer in the bio. > > Avri Altman (3): > scsi: ufs-bsg: Change the calling convention for write descriptor > scsi: ufs: Allow reading descriptor via raw upiu > scsi: ufs-bsg: Allow reading descriptors > > Documentation/scsi/ufs.txt | 11 > drivers/scsi/ufs/ufs_bsg.c | 63 ++-- > -- > drivers/scsi/ufs/ufshcd.c | 20 ++- > 3 files changed, 61 insertions(+), 33 deletions(-) > > -- > 1.9.1
[PATCH v3 0/2] mmc: core: Add SD Discard support
SD spec v5.1 adds discard support. The flows and commands matches those in eMMC, Which leaves to set the appropriate discard arg in CMD38 if DISCARD_SUPPORT (b313) is set in the SD_STATUS register. We set this arg on card init: not in mmc_init_erase as one might expect but arbitrarily once the card indicated its discard support. This is because unlike erase, it doesn't really involve any logic, and we want to avoid the unnecessary complication. V2->v3: The first 2 patches in the series got accepted. Elaborate the changelog of some of the differences between discard and erase. Clean some inconsistency in arg checking in mmc_erase(). Add a patch to set the timeout for sd discard. V1->v2: In the first patch, assign the discard arg for SD cards as well to keep the code consistent. Rename "discard_arg" to "erase_arg", and elaborate the change log. Avri Altman (2): mmc: core: Add discard support to sd mmc: core: Add sd discard timeout drivers/mmc/core/core.c | 15 +++ drivers/mmc/core/sd.c | 10 +- include/linux/mmc/sd.h | 1 + 3 files changed, 21 insertions(+), 5 deletions(-) -- 1.9.1
[PATCH v3 1/2] mmc: core: Add discard support to sd
SD spec v5.1 adds discard support. The flows and commands are similar to
mmc, so just set the discard arg in CMD38.
A host which supports DISCARD shall check if the DISCARD_SUPPORT (b313)
is set in the SD_STATUS register. If the card does not support discard,
the host shall not issue DISCARD command, but ERASE command instead.
Post the DISCARD operation, the card may de-allocate the discarded
blocks partially or completely. So the host mustn't make any assumptions
concerning the content of the discarded region. This is unlike ERASE
command, in which the region is guaranteed to contain either '0's or
'1's, depends on the content of DATA_STAT_AFTER_ERASE (b55) in the scr
register.
One more important difference compared to ERASE is the busy timeout
which we will address on the next patch.
Signed-off-by: Avri Altman
---
drivers/mmc/core/core.c | 8
drivers/mmc/core/sd.c | 10 +-
include/linux/mmc/sd.h | 1 +
3 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 0950edf..b7367ac 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1847,7 +1847,7 @@ static unsigned int mmc_align_erase_size(struct mmc_card
*card,
* @card: card to erase
* @from: first sector to erase
* @nr: number of sectors to erase
- * @arg: erase command argument (SD supports only %SD_ERASE_ARG)
+ * @arg: erase command argument
*
* Caller must claim host before calling this function.
*/
@@ -1864,14 +1864,14 @@ int mmc_erase(struct mmc_card *card, unsigned int from,
unsigned int nr,
if (!card->erase_size)
return -EOPNOTSUPP;
- if (mmc_card_sd(card) && arg != SD_ERASE_ARG)
+ if (mmc_card_sd(card) && arg != SD_ERASE_ARG && arg != SD_DISCARD_ARG)
return -EOPNOTSUPP;
- if ((arg & MMC_SECURE_ARGS) &&
+ if (mmc_card_mmc(card) && (arg & MMC_SECURE_ARGS) &&
!(card->ext_csd.sec_feature_support & EXT_CSD_SEC_ER_EN))
return -EOPNOTSUPP;
- if ((arg & MMC_TRIM_ARGS) &&
+ if (mmc_card_mmc(card) && (arg & MMC_TRIM_ARGS) &&
!(card->ext_csd.sec_feature_support & EXT_CSD_SEC_GB_CL_EN))
return -EOPNOTSUPP;
diff --git a/drivers/mmc/core/sd.c b/drivers/mmc/core/sd.c
index c2db94d..2b4fc22 100644
--- a/drivers/mmc/core/sd.c
+++ b/drivers/mmc/core/sd.c
@@ -231,6 +231,8 @@ static int mmc_read_ssr(struct mmc_card *card)
{
unsigned int au, es, et, eo;
__be32 *raw_ssr;
+ u32 resp[4] = {};
+ u8 discard_support;
int i;
if (!(card->csd.cmdclass & CCC_APP_SPEC)) {
@@ -276,7 +278,13 @@ static int mmc_read_ssr(struct mmc_card *card)
}
}
- card->erase_arg = SD_ERASE_ARG;
+ /*
+* starting SD5.1 discard is supported if DISCARD_SUPPORT (b313) is set
+*/
+ resp[3] = card->raw_ssr[6];
+ discard_support = UNSTUFF_BITS(resp, 313 - 288, 1);
+ card->erase_arg = (card->scr.sda_specx && discard_support) ?
+ SD_DISCARD_ARG : SD_ERASE_ARG;
return 0;
}
diff --git a/include/linux/mmc/sd.h b/include/linux/mmc/sd.h
index 1a6d10f..ec94a5a 100644
--- a/include/linux/mmc/sd.h
+++ b/include/linux/mmc/sd.h
@@ -95,5 +95,6 @@
* Erase/discard
*/
#define SD_ERASE_ARG 0x
+#define SD_DISCARD_ARG 0x0001
#endif /* LINUX_MMC_SD_H */
--
1.9.1
[PATCH v3 2/2] mmc: core: Add sd discard timeout
The busy timeout is 250msec per discard command.
Signed-off-by: Avri Altman
---
drivers/mmc/core/core.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index b7367ac..4979d4e 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -51,6 +51,7 @@
/* The max erase timeout, used when host->max_busy_timeout isn't specified */
#define MMC_ERASE_TIMEOUT_MS (60 * 1000) /* 60 s */
+#define SD_DISCARD_TIMEOUT_MS (250)
static const unsigned freqs[] = { 40, 30, 20, 10 };
@@ -1619,6 +1620,12 @@ static unsigned int mmc_sd_erase_timeout(struct mmc_card
*card,
{
unsigned int erase_timeout;
+ /* for DISCARD none of the below calculation applies.
+* the busy timeout is 250msec per discard command.
+*/
+ if (arg == SD_DISCARD_ARG)
+ return SD_DISCARD_TIMEOUT_MS;
+
if (card->ssr.erase_timeout) {
/* Erase timeout specified in SD Status Register (SSR) */
erase_timeout = card->ssr.erase_timeout * qty +
--
1.9.1
RE: [RFC PATCH v3 1/1] scsi: ufs: Enable power management for wlun
> + } else {
Is it possible to get here?
Scsi_scan_host is called only after successful add_wluns
> + /* device wlun is probed */
> + hba->luns_avail--;
> + }
> +}
> +
>
> /**
> @@ -7254,6 +7312,14 @@ static int ufshcd_scsi_add_wlus(struct ufs_hba
> *hba)
> goto out;
> }
> ufshcd_blk_pm_runtime_init(hba->sdev_ufs_device);
> + /*
> +* A pm_runtime_put_sync is invoked when this device enables
> blk_pm_runtime
> +* & would suspend the device-wlun upon timer expiry.
> +* But suspending device wlun _may_ put the ufs device in the
> pre-defined
> +* low power mode (SSU ). Probing of other luns may fail
> then.
> +* Don't allow this suspend until all the luns have been probed.
Maybe add one more sentence: see pm_runtime_mark_last_busy in ufshcd_setup_links
> - ufshcd_clear_ua_wluns(hba);
Are there any callers left to ufshcd_clear_ua_wluns?
Can it be removed?
> + if (hba->wlun_dev_clr_ua)
> + ufshcd_clear_ua_wlun(hba, UFS_UPIU_UFS_DEVICE_WLUN);
>
> cmd[4] = pwr_mode << 4;
RE: [PATCH v22 2/4] scsi: ufs: L2P map management for HPB read
> +/*
> + * This function will parse recommended active subregion information in
> sense
> + * data field of response UPIU with SAM_STAT_GOOD state.
> + */
> +void ufshpb_rsp_upiu(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
> +{
> + struct ufshpb_lu *hpb;
> + struct scsi_device *sdev;
> + struct utp_hpb_rsp *rsp_field = &lrbp->ucd_rsp_ptr->hr;
> + int data_seg_len;
> + bool found = false;
> +
> + __shost_for_each_device(sdev, hba->host) {
> + hpb = ufshpb_get_hpb_data(sdev);
> +
> + if (!hpb)
> + continue;
> +
> + if (rsp_field->lun == hpb->lun) {
> + found = true;
> + break;
This piece of code looks awkward, although it is probably working.
Why not just having a reference to the hpb luns, e.g. something like:
struct ufshpb_lu *hpb_luns[8] in struct ufs_hba.
Less elegant - but much more effective than iterating the scsi host on every
completion interrupt.
> + }
> + }
> +
> + if (!found)
> + return;
> +
> + if ((ufshpb_get_state(hpb) != HPB_PRESENT) &&
> + (ufshpb_get_state(hpb) != HPB_SUSPEND)) {
> + dev_notice(&hpb->sdev_ufs_lu->sdev_dev,
> + "%s: ufshpb state is not PRESENT/SUSPEND\n",
> + __func__);
> + return;
> + }
> +
> + data_seg_len = be32_to_cpu(lrbp->ucd_rsp_ptr->header.dword_2)
> + & MASK_RSP_UPIU_DATA_SEG_LEN;
> +
> + /* To flush remained rsp_list, we queue the map_work task */
> + if (!data_seg_len) {
data_seg_len should be 0x14
> + if (!ufshpb_is_general_lun(hpb->lun))
> + return;
> +
> + ufshpb_kick_map_work(hpb);
> + return;
> + }
> +
> + /* Check HPB_UPDATE_ALERT */
> + if (!(lrbp->ucd_rsp_ptr->header.dword_2 &
> + UPIU_HEADER_DWORD(0, 2, 0, 0)))
> + return;
> +
> + BUILD_BUG_ON(sizeof(struct utp_hpb_rsp) != UTP_HPB_RSP_SIZE);
> +
> + if (!ufshpb_is_hpb_rsp_valid(hba, lrbp, rsp_field))
> + return;
How about moving both the data_seg_len and alert bit checks into
ufshpb_is_hpb_rsp_valid,
And moving ufshpb_is_hpb_rsp_valid to the beginning of the function?
This way you would save redundant stuff if not a valid response.
Thanks,
Avri
RE: [PATCH v22 2/4] scsi: ufs: L2P map management for HPB read
> + if (!ufshpb_is_hpb_rsp_valid(hba, lrbp, rsp_field))
> + return;
> +
> + hpb->stats.rb_noti_cnt++;
> + switch (rsp_field->hpb_op) {
> + case HPB_RSP_NONE:
> + /* nothing to do */
> + break;
Maybe checks this too in ufshpb_is_hpb_rsp_valid
> + case HPB_RSP_REQ_REGION_UPDATE:
> + WARN_ON(data_seg_len != DEV_DATA_SEG_LEN);
> + ufshpb_rsp_req_region_update(hpb, rsp_field);
> + break;
> + case HPB_RSP_DEV_RESET:
> + dev_warn(&hpb->sdev_ufs_lu->sdev_dev,
> +"UFS device lost HPB information during PM.\n");
> + break;
> + default:
> + dev_notice(&hpb->sdev_ufs_lu->sdev_dev,
> + "hpb_op is not available: %d\n",
> + rsp_field->hpb_op);
> + break;
> + }
> +}
RE: [PATCH v22 3/4] scsi: ufs: Prepare HPB read for cached sub-region
> + err = ufshpb_fill_ppn_from_page(hpb, srgn->mctx, srgn_offset, 1,
> &ppn);
> + spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
> + if (unlikely(err < 0)) {
> + /*
> +* In this case, the region state is active,
> +* but the ppn table is not allocated.
> +* Make sure that ppn table must be allocated on
> +* active state.
> +*/
> + WARN_ON(true);
> + dev_err(hba->dev, "get ppn failed. err %d\n", err);
Maybe just pr_warn instead of risking crashing the machine over that?
> + return;
> + }
RE: RE: RE: [PATCH v22 2/4] scsi: ufs: L2P map management for HPB read
>
> > +/*
> > > + * This function will parse recommended active subregion information in
> > > sense
> > > + * data field of response UPIU with SAM_STAT_GOOD state.
> > > + */
> > > +void ufshpb_rsp_upiu(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
> > > +{
> > > + struct ufshpb_lu *hpb;
> > > + struct scsi_device *sdev;
> > > + struct utp_hpb_rsp *rsp_field = &lrbp->ucd_rsp_ptr->hr;
> > > + int data_seg_len;
> > > + bool found = false;
> > > +
> > > + __shost_for_each_device(sdev, hba->host) {
> > > + hpb = ufshpb_get_hpb_data(sdev);
> > > +
> > > + if (!hpb)
> > > + continue;
> > > +
> > > + if (rsp_field->lun == hpb->lun) {
> > > + found = true;
> > > + break;
> > This piece of code looks awkward, although it is probably working.
> > Why not just having a reference to the hpb luns, e.g. something like:
> > struct ufshpb_lu *hpb_luns[8] in struct ufs_hba.
> > Less elegant - but much more effective than iterating the scsi host on every
> completion interrupt.
>
> How about checking (cmd->lun == rsp->lun) before the iteration?
> Major case will be have same lun. And, it is hard to add struct ufshpb_lu
> *hpb_luns[128]
> in struct ufs_hba, because LUN can be upto 127.
Oh - yes, 8 is for write booster.
OK. Whatever you think.
However, I think there can be up to 32 regular luns,
meaning UFS_UPIU_MAX_UNIT_NUM_ID need to be fixed as well.
Thanks,
Avri
>
> Thanks,
> Daejun
RE: RE: [PATCH v22 3/4] scsi: ufs: Prepare HPB read for cached sub-region
>
>
> > > + err = ufshpb_fill_ppn_from_page(hpb, srgn->mctx, srgn_offset, 1,
> &ppn);
> > > + spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
> > > + if (unlikely(err < 0)) {
> > > + /*
> > > +* In this case, the region state is active,
> > > +* but the ppn table is not allocated.
> > > +* Make sure that ppn table must be allocated on
> > > +* active state.
> > > +*/
> > > + WARN_ON(true);
> > > + dev_err(hba->dev, "get ppn failed. err %d\n", err);
> > Maybe just pr_warn instead of risking crashing the machine over that?
>
> Why it crashing the machine? WARN_ON will just print kernel message.
I think that it can be configured, but I am not sure.
>
> Thanks,
> Daejun
RE: RE: [PATCH v22 3/4] scsi: ufs: Prepare HPB read for cached sub-region
> > > > + err = ufshpb_fill_ppn_from_page(hpb, srgn->mctx, srgn_offset, 1,
> > &ppn);
> > > > + spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
> > > > + if (unlikely(err < 0)) {
> > > > + /*
> > > > +* In this case, the region state is active,
> > > > +* but the ppn table is not allocated.
> > > > +* Make sure that ppn table must be allocated on
> > > > +* active state.
> > > > +*/
> > > > + WARN_ON(true);
> > > > + dev_err(hba->dev, "get ppn failed. err %d\n", err);
> > > Maybe just pr_warn instead of risking crashing the machine over that?
> >
> > Why it crashing the machine? WARN_ON will just print kernel message.
> I think that it can be configured, but I am not sure.
I think it can be configured via the parameter panic_on_warn
RE: [PATCH v22 4/4] scsi: ufs: Add HPB 2.0 support
> @@ -2656,7 +2656,12 @@ static int ufshcd_queuecommand(struct Scsi_Host
> *host, struct scsi_cmnd *cmd)
>
> lrbp->req_abort_skip = false;
>
> - ufshpb_prep(hba, lrbp);
> + err = ufshpb_prep(hba, lrbp);
> + if (err == -EAGAIN) {
> + lrbp->cmd = NULL;
> + ufshcd_release(hba);
> + goto out;
> + }
Did I miss-read it, or are you bailing out of wb failed e.g. because no tag is
available?
Why not continue with read10?
> + if (blk_insert_cloned_request(q, req) != BLK_STS_OK)
> + return -EAGAIN;
Why did you choose to use blk_insert_cloned_request and not e.g. the more
common blk_execute_rq_nowait?
> + hpb->stats.pre_req_cnt++;
> +
> + return 0;
> +}
> - ufshpb_set_hpb_read_to_upiu(hpb, lrbp, lpn, ppn, transfer_len);
> + if (ufshpb_is_required_wb(hpb, transfer_len)) {
> + err = ufshpb_issue_pre_req(hpb, cmd, &read_id);
> + if (err) {
> + unsigned long timeout;
> +
> + timeout = cmd->jiffies_at_alloc + msecs_to_jiffies(
> + hpb->params.requeue_timeout_ms);
> + if (time_before(jiffies, timeout))
> + return -EAGAIN;
Why requeue_timeout_ms needs to be a configurable parameter?
Why rq->timeout is not enough?
Thanks,
Avri
RE: [PATCH v22 3/4] scsi: ufs: Prepare HPB read for cached sub-region
> +static int ufshpb_fill_ppn_from_page(struct ufshpb_lu *hpb,
> +struct ufshpb_map_ctx *mctx, int pos,
> +int len, u64 *ppn_buf)
> +{
> + struct page *page;
> + int index, offset;
> + int copied;
> +
> + index = pos / (PAGE_SIZE / HPB_ENTRY_SIZE);
> + offset = pos % (PAGE_SIZE / HPB_ENTRY_SIZE);
Maybe cache hpb->entries_per_page in ufshpb_lu_parameter_init as well?
> +
> + if ((offset + len) <= (PAGE_SIZE / HPB_ENTRY_SIZE))
> + copied = len;
> + else
> + copied = (PAGE_SIZE / HPB_ENTRY_SIZE) - offset;
> +
> + page = mctx->m_page[index];
> + if (unlikely(!page)) {
> + dev_err(&hpb->sdev_ufs_lu->sdev_dev,
> + "error. cannot find page in mctx\n");
> + return -ENOMEM;
> + }
> +
> + memcpy(ppn_buf, page_address(page) + (offset * HPB_ENTRY_SIZE),
> + copied * HPB_ENTRY_SIZE);
> +
> + return copied;
> +}
RE: [PATCH v22 4/4] scsi: ufs: Add HPB 2.0 support
> + copied = ufshpb_fill_ppn_from_page(hpb, srgn->mctx, srgn_offset,
> + pre_req->wb.len - offset,
> + &addr[offset]);
> +
> + if (copied < 0)
> + goto mctx_error;
> +
> + offset += copied;
> + srgn_offset += offset;
This seems wrong.
How come the region offset is affected from the offset inside the pages?
> +
> + if (srgn_offset == hpb->entries_per_srgn) {
> + srgn_offset = 0;
> +
> + if (++srgn_idx == hpb->srgns_per_rgn) {
> + srgn_idx = 0;
> + rgn_idx++;
> + }
> + }
> +
> + if (offset < pre_req->wb.len)
> + goto next_offset;
If the 512k resides in a single subregion, and span across pages, fill_ppn
should take care of that.
If the 512k spans across subregion regions, than it spans across 2 subregions
at most,
and maybe you can use it.
RE: RE: [PATCH v22 4/4] scsi: ufs: Add HPB 2.0 support
>
> > > @@ -2656,7 +2656,12 @@ static int ufshcd_queuecommand(struct
> Scsi_Host
> > > *host, struct scsi_cmnd *cmd)
> > >
> > > lrbp->req_abort_skip = false;
> > >
> > > - ufshpb_prep(hba, lrbp);
> > > + err = ufshpb_prep(hba, lrbp);
> > > + if (err == -EAGAIN) {
> > > + lrbp->cmd = NULL;
> > > + ufshcd_release(hba);
> > > + goto out;
> > > + }
> > Did I miss-read it, or are you bailing out of wb failed e.g. because no tag
> > is
> available?
> > Why not continue with read10?
>
> We try to sending HPB read several times within the requeue_timeout_ms.
> Because it strategy has more benefit for overall performance in this
> situation that many requests are queueing.
This extra logic, IMO, should be optional. Default none.
And yes, in this case requeue_timeout should be a parameter for each OEM to
scale.
RE: RE: [PATCH v22 4/4] scsi: ufs: Add HPB 2.0 support
> >
> > > > @@ -2656,7 +2656,12 @@ static int ufshcd_queuecommand(struct
> > Scsi_Host
> > > > *host, struct scsi_cmnd *cmd)
> > > >
> > > > lrbp->req_abort_skip = false;
> > > >
> > > > - ufshpb_prep(hba, lrbp);
> > > > + err = ufshpb_prep(hba, lrbp);
> > > > + if (err == -EAGAIN) {
> > > > + lrbp->cmd = NULL;
> > > > + ufshcd_release(hba);
> > > > + goto out;
> > > > + }
> > > Did I miss-read it, or are you bailing out of wb failed e.g. because no
> > > tag is
> > available?
> > > Why not continue with read10?
> >
> > We try to sending HPB read several times within the requeue_timeout_ms.
> > Because it strategy has more benefit for overall performance in this
> > situation that many requests are queueing.
> This extra logic, IMO, should be optional. Default none.
> And yes, in this case requeue_timeout should be a parameter for each OEM to
> scale.
And either way, hpb internal flows should not cause dumping the command.
Worse case - continue as READ10
RE: [PATCH v24 4/4] scsi: ufs: Add HPB 2.0 support
> if (dev_info->wspecversion >= UFS_DEV_HPB_SUPPORT_VERSION &&
> (b_ufs_feature_sup & UFS_DEV_HPB_SUPPORT)) {
> - dev_info->hpb_enabled = true;
> + bool hpb_en = false;
> +
> ufshpb_get_dev_info(hba, desc_buf);
> +
> + err = ufshcd_query_flag_retry(hba,
> UPIU_QUERY_OPCODE_READ_FLAG,
> + QUERY_FLAG_IDN_HPB_EN, 0,
> &hpb_en);
> + if (ufshpb_is_legacy(hba) || (!err && hpb_en))
If is_legacy you shouldn't send fHPBen in the first place, not ignoring its
failure.
Also, using a Boolean is limiting you to HPB2.0 vs. HPB1.0.
What would you do in new flags/attributes/descriptors that HPB3.0 will
introduce?
> + dev_info->hpb_enabled = true;
> }
RE: [PATCH v24 4/4] scsi: ufs: Add HPB 2.0 support
> +static int ufshpb_issue_umap_all_req(struct ufshpb_lu *hpb)
Maybe ufshpb_issue_umap_all_req is just a wrapper for ufshpb_issue_umap_req?
e.g it calls ufshpb_issue_umap_req(hpb, int read_buferr_id = 0x3) ?
Then on host mode inactivation:
static int ufshpb_issue_umap_single_req(struct ufshpb_lu *hpb)
{
return ufshpb_issue_umap_req(hpb, 0x1);
}
> @@ -1691,6 +2180,7 @@ static void ufshpb_hpb_lu_prepared(struct ufs_hba
> *hba)
> ufshpb_set_state(hpb, HPB_PRESENT);
> if ((hpb->lu_pinned_end - hpb->lu_pinned_start) > 0)
> queue_work(ufshpb_wq, &hpb->map_work);
> + ufshpb_issue_umap_all_req(hpb);
If (!is_legacy)
RE: [PATCH v24 4/4] scsi: ufs: Add HPB 2.0 support
>
>
> > +static int ufshpb_issue_umap_all_req(struct ufshpb_lu *hpb)
> Maybe ufshpb_issue_umap_all_req is just a wrapper for
> ufshpb_issue_umap_req?
> e.g it calls ufshpb_issue_umap_req(hpb, int read_buferr_id = 0x3) ?
> Then on host mode inactivation:
> static int ufshpb_issue_umap_single_req(struct ufshpb_lu *hpb)
> {
> return ufshpb_issue_umap_req(hpb, 0x1);
> }
Better yet, ufshpb_execute_umap_req can get *rgn as an extra argument.
ufshpb_issue_umap_all_req will call it with NULL, while
ufshpb_issue_umap_single_req will call it with the rgn to inactivate.
Then, ufshpb_set_unmap_cmd takes the form:
static void ufshpb_set_unmap_cmd(unsigned char *cdb, struct ufshpb_region *rgn)
{
cdb[0] = UFSHPB_WRITE_BUFFER;
if (rgn) {
cdb[1] = UFSHPB_WRITE_BUFFER_INACT_SINGLE_ID;
put_unaligned_be16(rgn->rgn_idx, &cdb[2]);
} else {
cdb[1] = UFSHPB_WRITE_BUFFER_INACT_ALL_ID;
}
cdb[9] = 0x00;
}
Does it make sense?
Thanks,
Avri
RE: RE: RE: [PATCH v22 4/4] scsi: ufs: Add HPB 2.0 support
>
> > > > > @@ -2656,7 +2656,12 @@ static int ufshcd_queuecommand(struct
> > > > Scsi_Host
> > > > > > *host, struct scsi_cmnd *cmd)
> > > > > >
> > > > > > lrbp->req_abort_skip = false;
> > > > > >
> > > > > > - ufshpb_prep(hba, lrbp);
> > > > > > + err = ufshpb_prep(hba, lrbp);
> > > > > > + if (err == -EAGAIN) {
> > > > > > + lrbp->cmd = NULL;
> > > > > > + ufshcd_release(hba);
> > > > > > + goto out;
> > > > > > + }
> > > > > Did I miss-read it, or are you bailing out of wb failed e.g. because
> > > > > no tag
> is
> > > > available?
> > > > > Why not continue with read10?
> > > >
> > > > We try to sending HPB read several times within the
> requeue_timeout_ms.
> > > > Because it strategy has more benefit for overall performance in this
> > > > situation that many requests are queueing.
> > > This extra logic, IMO, should be optional. Default none.
> > > And yes, in this case requeue_timeout should be a parameter for each OEM
> to
> > > scale.
> > And either way, hpb internal flows should not cause dumping the command.
> > Worse case - continue as READ10
>
> However, this can improve the overall performance and should be used
> carefully. The problem can be solved by setting the default timeout
> ms to 0. And OEM can change it.
Yes. I was thinking that too.
Thanks,
Avri
>
> Thanks,
> Daejun
RE: [PATCH v19 3/3] scsi: ufs: Prepare HPB read for cached sub-region
> +static bool ufshpb_test_ppn_dirty(struct ufshpb_lu *hpb, int rgn_idx,
> + int srgn_idx, int srgn_offset, int cnt)
> +{
> + struct ufshpb_region *rgn;
> + struct ufshpb_subregion *srgn;
> + int bitmap_len = hpb->entries_per_srgn;
> + int bit_len;
> +
> +next_srgn:
> + rgn = hpb->rgn_tbl + rgn_idx;
> + srgn = rgn->srgn_tbl + srgn_idx;
> +
> + if (!ufshpb_is_valid_srgn(rgn, srgn))
> + return true;
The subregion is changing its state to issued, only in ufshpb_issue_map_req.
Although you already know that those physical addresses are no longer valid in
ufshpb_update_active_info.
Can we mark that this subregion is no longer valid earlier, so not to keep
sending faulty HPB-READ to that subregion?
Thanks,
Avri
RE: [PATCH V2 1/4] scsi: ufs: Add exception event tracepoint
> > Currently, exception event status can be read from wExceptionEventStatus > attribute (sysfs file attributes/exception_event_status under the UFS host > controller device directory). Polling that attribute to track UFS exception > events is impractical, so add a tracepoint to track exception events for > testing and debugging purposes. > > Note, by the time the exception event status is read, the exception event > may have cleared, so the value can be zero - see example below. > > Note also, only enabled exception events can be reported. A subsequent > patch adds the ability for users to enable selected exception events via > debugfs. > > Example with driver instrumented to enable all exception events: > > # echo 1 > > /sys/kernel/debug/tracing/events/ufs/ufshcd_exception_event/enable > > ... do some I/O ... > > # cat /sys/kernel/debug/tracing/trace > # tracer: nop > # > # entries-in-buffer/entries-written: 3/3 #P:5 > # > #_-=> irqs-off > # / _=> need-resched > # | / _---=> hardirq/softirq > # || / _--=> preempt-depth > # ||| / delay > # TASK-PID CPU# TIMESTAMP FUNCTION > # | | | | | >kworker/2:2-173 [002] 731.486419: ufshcd_exception_event: > :00:12.5: status 0x0 >kworker/2:2-173 [002] 732.608918: ufshcd_exception_event: > :00:12.5: status 0x4 >kworker/2:2-173 [002] 732.609312: ufshcd_exception_event: > :00:12.5: status 0x4 > > Signed-off-by: Adrian Hunter Reviewed-by: Avri Altman
[PATCH] scsi: ufs: Fix a duplicate dev quirk number
fixes: 2b2bfc8aa519 (scsi: ufs: Introduce a quirk to allow only
page-aligned sg entries)
Signed-off-by: Avri Altman
---
drivers/scsi/ufs/ufshcd.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index ee61f821f75d..18e56c1c1b30 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -570,7 +570,7 @@ enum ufshcd_quirks {
/*
* This quirk allows only sg entries aligned with page size.
*/
- UFSHCD_QUIRK_ALIGN_SG_WITH_PAGE_SIZE= 1 << 13,
+ UFSHCD_QUIRK_ALIGN_SG_WITH_PAGE_SIZE= 1 << 14,
};
enum ufshcd_caps {
--
2.25.1
RE: [PATCH v20 4/4] scsi: ufs: Add HPB 2.0 support
> > + /* for pre_req */ > + hpb->pre_req_min_tr_len = HPB_MULTI_CHUNK_LOW; This actually needs to be bMAX_DATA_SIZE_FOR_HPB_SINGLE_CMD. Also wasn't able to find any reference to fHPBen? Thanks, Avri
[PATCH v3 5/9] scsi: ufshpb: Region inactivation in host mode
I host mode, the host is expected to send HPB-WRITE-BUFFER with
buffer-id = 0x1 when it inactivates a region.
Use the map-requests pool as there is no point in assigning a
designated cache for umap-requests.
Signed-off-by: Avri Altman
---
drivers/scsi/ufs/ufshpb.c | 120 --
drivers/scsi/ufs/ufshpb.h | 5 +-
2 files changed, 105 insertions(+), 20 deletions(-)
diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index 9cb17c97cf93..3435a5507853 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -717,16 +717,15 @@ int ufshpb_prep(struct ufs_hba *hba, struct ufshcd_lrb
*lrbp)
return 0;
}
-static struct ufshpb_req *ufshpb_get_map_req(struct ufshpb_lu *hpb,
-struct ufshpb_subregion *srgn)
+static struct ufshpb_req *ufshpb_get_req(struct ufshpb_lu *hpb,
+int rgn_idx, enum req_opf dir)
{
- struct ufshpb_req *map_req;
+ struct ufshpb_req *rq;
struct request *req;
- struct bio *bio;
int retries = HPB_MAP_REQ_RETRIES;
- map_req = kmem_cache_alloc(hpb->map_req_cache, GFP_KERNEL);
- if (!map_req)
+ rq = kmem_cache_alloc(hpb->map_req_cache, GFP_KERNEL);
+ if (!rq)
return NULL;
retry:
@@ -738,36 +737,57 @@ static struct ufshpb_req *ufshpb_get_map_req(struct
ufshpb_lu *hpb,
goto retry;
}
+ req = blk_get_request(hpb->sdev_ufs_lu->request_queue,
+ dir, 0);
if (IS_ERR(req))
- goto free_map_req;
+ goto free_rq;
+
+ rq->hpb = hpb;
+ rq->req = req;
+ rq->rb.rgn_idx = rgn_idx;
+
+ return rq;
+
+free_rq:
+ kmem_cache_free(hpb->map_req_cache, rq);
+ return NULL;
+}
+
+static void ufshpb_put_req(struct ufshpb_lu *hpb, struct ufshpb_req *rq)
+{
+ blk_put_request(rq->req);
+ kmem_cache_free(hpb->map_req_cache, rq);
+}
+
+static struct ufshpb_req *ufshpb_get_map_req(struct ufshpb_lu *hpb,
+struct ufshpb_subregion *srgn)
+{
+ struct ufshpb_req *map_req;
+ struct bio *bio;
+
+ map_req = ufshpb_get_req(hpb, srgn->rgn_idx, REQ_OP_SCSI_IN);
+ if (!map_req)
+ return NULL;
bio = bio_alloc(GFP_KERNEL, hpb->pages_per_srgn);
if (!bio) {
- blk_put_request(req);
- goto free_map_req;
+ ufshpb_put_req(hpb, map_req);
+ return NULL;
}
- map_req->hpb = hpb;
- map_req->req = req;
map_req->bio = bio;
- map_req->rb.rgn_idx = srgn->rgn_idx;
map_req->rb.srgn_idx = srgn->srgn_idx;
map_req->rb.mctx = srgn->mctx;
return map_req;
-
-free_map_req:
- kmem_cache_free(hpb->map_req_cache, map_req);
- return NULL;
}
static void ufshpb_put_map_req(struct ufshpb_lu *hpb,
- struct ufshpb_req *map_req)
+ struct ufshpb_req *map_req)
{
bio_put(map_req->bio);
- blk_put_request(map_req->req);
- kmem_cache_free(hpb->map_req_cache, map_req);
+ ufshpb_put_req(hpb, map_req);
}
static int ufshpb_clear_dirty_bitmap(struct ufshpb_lu *hpb,
@@ -829,6 +849,13 @@ static void ufshpb_activate_subregion(struct ufshpb_lu
*hpb,
srgn->srgn_state = HPB_SRGN_VALID;
}
+static void ufshpb_umap_req_compl_fn(struct request *req, blk_status_t error)
+{
+ struct ufshpb_req *umap_req = (struct ufshpb_req *)req->end_io_data;
+
+ ufshpb_put_req(umap_req->hpb, umap_req);
+}
+
static void ufshpb_map_req_compl_fn(struct request *req, blk_status_t error)
{
struct ufshpb_req *map_req = (struct ufshpb_req *) req->end_io_data;
@@ -847,6 +874,14 @@ static void ufshpb_map_req_compl_fn(struct request *req,
blk_status_t error)
ufshpb_put_map_req(map_req->hpb, map_req);
}
+static void ufshpb_set_unmap_cmd(unsigned char *cdb, int rgn_idx)
+{
+ cdb[0] = UFSHPB_WRITE_BUFFER;
+ cdb[1] = UFSHPB_WRITE_BUFFER_ID;
+ put_unaligned_be16(rgn_idx, &cdb[2]);
+ cdb[9] = 0x00;
+}
+
static void ufshpb_set_read_buf_cmd(unsigned char *cdb, int rgn_idx,
int srgn_idx, int srgn_mem_size)
{
@@ -860,6 +895,27 @@ static void ufshpb_set_read_buf_cmd(unsigned char *cdb,
int rgn_idx,
cdb[9] = 0x00;
}
+static int ufshpb_execute_umap_req(struct ufshpb_lu *hpb,
+ struct ufshpb_req *umap_req)
+{
+ struct request_queue *q;
+ struct request *req;
+ struct scsi_request *rq;
+
+ q = hpb->sdev_ufs_lu->request_queue;
+ req = umap_req->req;
+ req->timeout = 0;
+ req->end_io_data = (void *)umap_req;
[PATCH v3 3/9] scsi: ufshpb: Add region's reads counter
In host control mode, reads are the major source of activation trials.
Keep track of those reads counters, for both active as well inactive
regions.
We reset the read counter upon write - we are only interested in "clean"
reads. less intuitive however, is that we also reset it upon region's
deactivation. Region deactivation is often due to the fact that
eviction took place: a region become active on the expense of another.
This is happening when the max-active-regions limit has crossed. If we
don’t reset the counter, we will trigger a lot of trashing of the HPB
database, since few reads (or even one) to the region that was
deactivated, will trigger a re-activation trial.
Keep those counters normalized, as we are using those reads as a
comparative score, to make various decisions.
If during consecutive normalizations an active region has exhaust its
reads - inactivate it.
Signed-off-by: Avri Altman
---
drivers/scsi/ufs/ufshpb.c | 108 --
drivers/scsi/ufs/ufshpb.h | 7 +++
2 files changed, 100 insertions(+), 15 deletions(-)
diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index e052260868ad..348185964c32 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -16,6 +16,8 @@
#include "ufshpb.h"
#include "../sd.h"
+#define ACTIVATION_THRESHOLD 4 /* 4 IOs */
+
/* memory management */
static struct kmem_cache *ufshpb_mctx_cache;
static mempool_t *ufshpb_mctx_pool;
@@ -570,6 +572,21 @@ static int ufshpb_issue_pre_req(struct ufshpb_lu *hpb,
struct scsi_cmnd *cmd,
return ret;
}
+static void ufshpb_update_active_info(struct ufshpb_lu *hpb, int rgn_idx,
+ int srgn_idx)
+{
+ struct ufshpb_region *rgn;
+ struct ufshpb_subregion *srgn;
+
+ rgn = hpb->rgn_tbl + rgn_idx;
+ srgn = rgn->srgn_tbl + srgn_idx;
+
+ list_del_init(&rgn->list_inact_rgn);
+
+ if (list_empty(&srgn->list_act_srgn))
+ list_add_tail(&srgn->list_act_srgn, &hpb->lh_act_srgn);
+}
+
/*
* This function will set up HPB read command using host-side L2P map data.
*/
@@ -616,12 +633,45 @@ int ufshpb_prep(struct ufs_hba *hba, struct ufshcd_lrb
*lrbp)
ufshpb_set_ppn_dirty(hpb, rgn_idx, srgn_idx, srgn_offset,
transfer_len);
spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
+
+ if (hpb->is_hcm) {
+ spin_lock_irqsave(&rgn->rgn_lock, flags);
+ rgn->reads = 0;
+ spin_unlock_irqrestore(&rgn->rgn_lock, flags);
+ }
+
return 0;
}
if (!ufshpb_is_support_chunk(hpb, transfer_len))
return 0;
+ if (hpb->is_hcm) {
+ bool activate = false;
+ /*
+* in host control mode, reads are the main source for
+* activation trials.
+*/
+ spin_lock_irqsave(&rgn->rgn_lock, flags);
+ rgn->reads++;
+ if (rgn->reads == ACTIVATION_THRESHOLD)
+ activate = true;
+ spin_unlock_irqrestore(&rgn->rgn_lock, flags);
+ if (activate) {
+ spin_lock_irqsave(&hpb->rsp_list_lock, flags);
+ ufshpb_update_active_info(hpb, rgn_idx, srgn_idx);
+ hpb->stats.rb_active_cnt++;
+ spin_unlock_irqrestore(&hpb->rsp_list_lock, flags);
+ dev_dbg(&hpb->sdev_ufs_lu->sdev_dev,
+ "activate region %d-%d\n", rgn_idx, srgn_idx);
+ }
+
+ /* keep those counters normalized */
+ if (rgn->reads > hpb->entries_per_srgn &&
+ !test_and_set_bit(WORK_PENDING, &hpb->work_data_bits))
+ schedule_work(&hpb->ufshpb_normalization_work);
+ }
+
spin_lock_irqsave(&hpb->rgn_state_lock, flags);
if (ufshpb_test_ppn_dirty(hpb, rgn_idx, srgn_idx, srgn_offset,
transfer_len)) {
@@ -738,21 +788,6 @@ static int ufshpb_clear_dirty_bitmap(struct ufshpb_lu *hpb,
return 0;
}
-static void ufshpb_update_active_info(struct ufshpb_lu *hpb, int rgn_idx,
- int srgn_idx)
-{
- struct ufshpb_region *rgn;
- struct ufshpb_subregion *srgn;
-
- rgn = hpb->rgn_tbl + rgn_idx;
- srgn = rgn->srgn_tbl + srgn_idx;
-
- list_del_init(&rgn->list_inact_rgn);
-
- if (list_empty(&srgn->list_act_srgn))
- list_add_tail(&srgn->list_act_srgn, &hpb->lh_act_srgn);
-}
-
static void ufshpb_update_inactive_info(
[PATCH v3 4/9] scsi: ufshpb: Make eviction depends on region's reads
In host mode, eviction is considered an extreme measure.
verify that the entering region has enough reads, and the exiting
region has much less reads.
Signed-off-by: Avri Altman
---
drivers/scsi/ufs/ufshpb.c | 20 +++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index 348185964c32..9cb17c97cf93 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -17,6 +17,7 @@
#include "../sd.h"
#define ACTIVATION_THRESHOLD 4 /* 4 IOs */
+#define EVICTION_THRESHOLD (ACTIVATION_THRESHOLD << 6) /* 256 IOs */
/* memory management */
static struct kmem_cache *ufshpb_mctx_cache;
@@ -995,6 +996,13 @@ static struct ufshpb_region *ufshpb_victim_lru_info(struct
ufshpb_lu *hpb)
if (ufshpb_check_srgns_issue_state(hpb, rgn))
continue;
+ /*
+* in host control mode, verify that the exiting region
+* has less reads
+*/
+ if (hpb->is_hcm && rgn->reads > (EVICTION_THRESHOLD >> 1))
+ continue;
+
victim_rgn = rgn;
break;
}
@@ -1150,7 +1158,7 @@ static int ufshpb_issue_map_req(struct ufshpb_lu *hpb,
static int ufshpb_add_region(struct ufshpb_lu *hpb, struct ufshpb_region *rgn)
{
- struct ufshpb_region *victim_rgn;
+ struct ufshpb_region *victim_rgn = NULL;
struct victim_select_info *lru_info = &hpb->lru_info;
unsigned long flags;
int ret = 0;
@@ -1178,6 +1186,16 @@ static int ufshpb_add_region(struct ufshpb_lu *hpb,
struct ufshpb_region *rgn)
* because the device could detect this region
* by not issuing HPB_READ
*/
+
+ /*
+* in host control mode, verify that the entering
+* region has enough reads
+*/
+ if (hpb->is_hcm && rgn->reads < EVICTION_THRESHOLD) {
+ ret = -EACCES;
+ goto out;
+ }
+
victim_rgn = ufshpb_victim_lru_info(hpb);
if (!victim_rgn) {
dev_warn(&hpb->sdev_ufs_lu->sdev_dev,
--
2.25.1
[PATCH v3 7/9] scsi: ufshpb: Add "Cold" regions timer
In order not to hang on to “cold” regions, we shall inactivate a
region that has no READ access for a predefined amount of time -
READ_TO_MS. For that purpose we shall monitor the active regions list,
polling it on every POLLING_INTERVAL_MS. On timeout expiry we shall add
the region to the "to-be-inactivated" list, unless it is clean and did
not exhaust its READ_TO_EXPIRIES - another parameter.
All this does not apply to pinned regions.
Signed-off-by: Avri Altman
---
drivers/scsi/ufs/ufshpb.c | 70 ++-
drivers/scsi/ufs/ufshpb.h | 7
2 files changed, 76 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index 15755f677097..9a1c525204f7 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -18,6 +18,9 @@
#define ACTIVATION_THRESHOLD 4 /* 4 IOs */
#define EVICTION_THRESHOLD (ACTIVATION_THRESHOLD << 6) /* 256 IOs */
+#define READ_TO_MS 1000
+#define READ_TO_EXPIRIES 100
+#define POLLING_INTERVAL_MS 200
/* memory management */
static struct kmem_cache *ufshpb_mctx_cache;
@@ -1029,12 +1032,64 @@ static int ufshpb_check_srgns_issue_state(struct
ufshpb_lu *hpb,
return 0;
}
+static void ufshpb_read_to_handler(struct work_struct *work)
+{
+ struct delayed_work *dwork = to_delayed_work(work);
+ struct ufshpb_lu *hpb;
+ struct victim_select_info *lru_info;
+ struct ufshpb_region *rgn;
+ unsigned long flags;
+ LIST_HEAD(expired_list);
+
+ hpb = container_of(dwork, struct ufshpb_lu, ufshpb_read_to_work);
+
+ if (test_and_set_bit(TIMEOUT_WORK_PENDING, &hpb->work_data_bits))
+ return;
+
+ spin_lock_irqsave(&hpb->rgn_state_lock, flags);
+
+ lru_info = &hpb->lru_info;
+
+ list_for_each_entry(rgn, &lru_info->lh_lru_rgn, list_lru_rgn) {
+ bool timedout = ktime_after(ktime_get(), rgn->read_timeout);
+
+ if (timedout) {
+ rgn->read_timeout_expiries--;
+ if (is_rgn_dirty(rgn) ||
+ rgn->read_timeout_expiries == 0)
+ list_add(&rgn->list_expired_rgn, &expired_list);
+ else
+ rgn->read_timeout = ktime_add_ms(ktime_get(),
+READ_TO_MS);
+ }
+ }
+
+ spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
+
+ list_for_each_entry(rgn, &expired_list, list_expired_rgn) {
+ list_del_init(&rgn->list_expired_rgn);
+ spin_lock_irqsave(&hpb->rsp_list_lock, flags);
+ ufshpb_update_inactive_info(hpb, rgn->rgn_idx);
+ hpb->stats.rb_inactive_cnt++;
+ spin_unlock_irqrestore(&hpb->rsp_list_lock, flags);
+ }
+
+ clear_bit(TIMEOUT_WORK_PENDING, &hpb->work_data_bits);
+
+ schedule_delayed_work(&hpb->ufshpb_read_to_work,
+ msecs_to_jiffies(POLLING_INTERVAL_MS));
+}
+
static void ufshpb_add_lru_info(struct victim_select_info *lru_info,
- struct ufshpb_region *rgn)
+ struct ufshpb_region *rgn)
{
rgn->rgn_state = HPB_RGN_ACTIVE;
list_add_tail(&rgn->list_lru_rgn, &lru_info->lh_lru_rgn);
atomic_inc(&lru_info->active_cnt);
+ if (rgn->hpb->is_hcm) {
+ rgn->read_timeout = ktime_add_ms(ktime_get(), READ_TO_MS);
+ rgn->read_timeout_expiries = READ_TO_EXPIRIES;
+ }
}
static void ufshpb_hit_lru_info(struct victim_select_info *lru_info,
@@ -1778,6 +1833,7 @@ static int ufshpb_alloc_region_tbl(struct ufs_hba *hba,
struct ufshpb_lu *hpb)
INIT_LIST_HEAD(&rgn->list_inact_rgn);
INIT_LIST_HEAD(&rgn->list_lru_rgn);
+ INIT_LIST_HEAD(&rgn->list_expired_rgn);
if (rgn_idx == hpb->rgns_per_lu - 1) {
srgn_cnt = ((hpb->srgns_per_lu - 1) %
@@ -1799,6 +1855,7 @@ static int ufshpb_alloc_region_tbl(struct ufs_hba *hba,
struct ufshpb_lu *hpb)
}
rgn->rgn_flags = 0;
+ rgn->hpb = hpb;
}
return 0;
@@ -2017,6 +2074,8 @@ static int ufshpb_lu_hpb_init(struct ufs_hba *hba, struct
ufshpb_lu *hpb)
ufshpb_normalization_work_handler);
INIT_WORK(&hpb->ufshpb_lun_reset_work,
ufshpb_reset_work_handler);
+ INIT_DELAYED_WORK(&hpb->ufshpb_read_to_work,
+ ufshpb_read_to_handler);
}
hpb->map_req_cache = kmem_cache_create("ufshpb_req_cache",
@@ -2051,6 +2110,10 @@ static int uf
[PATCH v3 9/9] scsi: ufshpb: Make host mode parameters configurable
We can make use of this commit, to elaborate some more of the host control mode logic, explaining what role play each and every variable. While at it, allow those parameters to be configurable. Signed-off-by: Avri Altman --- Documentation/ABI/testing/sysfs-driver-ufs | 68 ++ drivers/scsi/ufs/ufshpb.c | 254 +++-- drivers/scsi/ufs/ufshpb.h | 18 ++ 3 files changed, 326 insertions(+), 14 deletions(-) diff --git a/Documentation/ABI/testing/sysfs-driver-ufs b/Documentation/ABI/testing/sysfs-driver-ufs index 0017eaf89cbe..1d72201c4953 100644 --- a/Documentation/ABI/testing/sysfs-driver-ufs +++ b/Documentation/ABI/testing/sysfs-driver-ufs @@ -1322,3 +1322,71 @@ Description: This entry shows the maximum HPB data size for using single HPB === The file is read only. + +What: /sys/class/scsi_device/*/device/hpb_param_sysfs/activation_thld +Date: February 2021 +Contact: Avri Altman +Description: In host control mode, reads are the major source of activation + trials. once this threshold hs met, the region is added to the + "to-be-activated" list. Since we reset the read counter upon + write, this include sending a rb command updating the region + ppn as well. + +What: /sys/class/scsi_device/*/device/hpb_param_sysfs/normalization_factor +Date: February 2021 +Contact: Avri Altman +Description: In host control mode, We think of the regions as "buckets". + Those buckets are being filled with reads, and emptied on write. + We use entries_per_srgn - the amount of blocks in a subregion as + our bucket size. This applies because HPB1.0 only concern a + single-block reads. Once the bucket size is crossed, we trigger + a normalization work - not only to avoid overflow, but mainly + because we want to keep those counters normalized, as we are + using those reads as a comparative score, to make various decisions. + The normalization is dividing (shift right) the read counter by + the normalization_factor. If during consecutive normalizations + an active region has exhaust its reads - inactivate it. + +What: /sys/class/scsi_device/*/device/hpb_param_sysfs/eviction_thld_enter +Date: February 2021 +Contact: Avri Altman +Description: Region deactivation is often due to the fact that eviction took + place: a region become active on the expense of another. This is + happening when the max-active-regions limit has crossed. + In host mode, eviction is considered an extreme measure. We + want to verify that the entering region has enough reads, and + the exiting region has much less reads. eviction_thld_enter is + the min reads that a region must have in order to be considered + as a candidate to evict other region. + +What: /sys/class/scsi_device/*/device/hpb_param_sysfs/eviction_thld_exit +Date: February 2021 +Contact: Avri Altman +Description: same as above for the exiting region. A region is consider to + be a candidate to be evicted, only if it has less reads than + eviction_thld_exit. + +What: /sys/class/scsi_device/*/device/hpb_param_sysfs/read_timeout_ms +Date: February 2021 +Contact: Avri Altman +Description: In order not to hang on to “cold” regions, we shall inactivate + a region that has no READ access for a predefined amount of + time - read_timeout_ms. If read_timeout_ms has expired, and the + region is dirty - it is less likely that we can make any use of + HPB-READing it. So we inactivate it. Still, deactivation has + its overhead, and we may still benefit from HPB-READing this + region if it is clean - see read_timeout_expiries. + +What: /sys/class/scsi_device/*/device/hpb_param_sysfs/read_timeout_expiries +Date: February 2021 +Contact: Avri Altman +Description: if the region read timeout has expired, but the region is clean, + just re-wind its timer for another spin. Do that as long as it + is clean and did not exhaust its read_timeout_expiries threshold. + +What: /sys/class/scsi_device/*/device/hpb_param_sysfs/timeout_polling_interval_ms +Date: February 2021 +Contact: Avri Altman +Description: the frequency in which the delayed worker that checks the + read_timeouts is awaken. +>>>>>>> ef6bf08e666d... scsi: ufshpb: Make host mode parameters configurable diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c inde
[PATCH v3 8/9] scsi: ufshpb: Add support for host control mode
Support devices that report they are using host control mode.
Signed-off-by: Avri Altman
---
drivers/scsi/ufs/ufshpb.c | 6 --
1 file changed, 6 deletions(-)
diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index 9a1c525204f7..2c0f63a828ca 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -2523,12 +2523,6 @@ void ufshpb_get_dev_info(struct ufs_hba *hba, u8
*desc_buf)
u32 max_hpb_sigle_cmd = 0;
hpb_dev_info->control_mode = desc_buf[DEVICE_DESC_PARAM_HPB_CONTROL];
- if (hpb_dev_info->control_mode == HPB_HOST_CONTROL) {
- dev_err(hba->dev, "%s: host control mode is not supported.\n",
- __func__);
- hpb_dev_info->hpb_disabled = true;
- return;
- }
version = get_unaligned_be16(desc_buf + DEVICE_DESC_PARAM_HPB_VER);
if (version != HPB_SUPPORT_VERSION) {
--
2.25.1
[PATCH v3 6/9] scsi: ufshpb: Add hpb dev reset response
The spec does not define what is the host's recommended response when
the device send hpb dev reset response (oper 0x2).
We will update all active hpb regions: mark them and do that on the next
read.
Signed-off-by: Avri Altman
---
drivers/scsi/ufs/ufshpb.c | 40 ---
drivers/scsi/ufs/ufshpb.h | 3 +++
2 files changed, 40 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index 3435a5507853..15755f677097 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -658,7 +658,8 @@ int ufshpb_prep(struct ufs_hba *hba, struct ufshcd_lrb
*lrbp)
if (rgn->reads == ACTIVATION_THRESHOLD)
activate = true;
spin_unlock_irqrestore(&rgn->rgn_lock, flags);
- if (activate) {
+ if (activate ||
+ test_and_clear_bit(RGN_FLAG_UPDATE, &rgn->rgn_flags)) {
spin_lock_irqsave(&hpb->rsp_list_lock, flags);
ufshpb_update_active_info(hpb, rgn_idx, srgn_idx);
hpb->stats.rb_active_cnt++;
@@ -1446,6 +1447,11 @@ void ufshpb_rsp_upiu(struct ufs_hba *hba, struct
ufshcd_lrb *lrbp)
case HPB_RSP_DEV_RESET:
dev_warn(&hpb->sdev_ufs_lu->sdev_dev,
"UFS device lost HPB information during PM.\n");
+
+ if (hpb->is_hcm &&
+ !test_and_set_bit(RESET_PENDING, &hpb->work_data_bits))
+ schedule_work(&hpb->ufshpb_lun_reset_work);
+
break;
default:
dev_notice(&hpb->sdev_ufs_lu->sdev_dev,
@@ -1560,6 +1566,27 @@ static void ufshpb_run_inactive_region_list(struct
ufshpb_lu *hpb)
spin_unlock_irqrestore(&hpb->rsp_list_lock, flags);
}
+static void ufshpb_reset_work_handler(struct work_struct *work)
+{
+ struct ufshpb_lu *hpb;
+ struct victim_select_info *lru_info;
+ struct ufshpb_region *rgn;
+ unsigned long flags;
+
+ hpb = container_of(work, struct ufshpb_lu, ufshpb_lun_reset_work);
+
+ lru_info = &hpb->lru_info;
+
+ spin_lock_irqsave(&hpb->rgn_state_lock, flags);
+
+ list_for_each_entry(rgn, &lru_info->lh_lru_rgn, list_lru_rgn)
+ set_bit(RGN_FLAG_UPDATE, &rgn->rgn_flags);
+
+ spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
+
+ clear_bit(RESET_PENDING, &hpb->work_data_bits);
+}
+
static void ufshpb_normalization_work_handler(struct work_struct *work)
{
struct ufshpb_lu *hpb;
@@ -1770,6 +1797,8 @@ static int ufshpb_alloc_region_tbl(struct ufs_hba *hba,
struct ufshpb_lu *hpb)
} else {
rgn->rgn_state = HPB_RGN_INACTIVE;
}
+
+ rgn->rgn_flags = 0;
}
return 0;
@@ -1983,9 +2012,12 @@ static int ufshpb_lu_hpb_init(struct ufs_hba *hba,
struct ufshpb_lu *hpb)
INIT_LIST_HEAD(&hpb->list_hpb_lu);
INIT_WORK(&hpb->map_work, ufshpb_map_work_handler);
- if (hpb->is_hcm)
+ if (hpb->is_hcm) {
INIT_WORK(&hpb->ufshpb_normalization_work,
ufshpb_normalization_work_handler);
+ INIT_WORK(&hpb->ufshpb_lun_reset_work,
+ ufshpb_reset_work_handler);
+ }
hpb->map_req_cache = kmem_cache_create("ufshpb_req_cache",
sizeof(struct ufshpb_req), 0, 0, NULL);
@@ -2085,8 +2117,10 @@ static void ufshpb_discard_rsp_lists(struct ufshpb_lu
*hpb)
static void ufshpb_cancel_jobs(struct ufshpb_lu *hpb)
{
- if (hpb->is_hcm)
+ if (hpb->is_hcm) {
+ cancel_work_sync(&hpb->ufshpb_lun_reset_work);
cancel_work_sync(&hpb->ufshpb_normalization_work);
+ }
cancel_work_sync(&hpb->map_work);
}
diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
index 13ec49ba260d..a3b85d2df224 100644
--- a/drivers/scsi/ufs/ufshpb.h
+++ b/drivers/scsi/ufs/ufshpb.h
@@ -119,6 +119,7 @@ struct ufshpb_region {
struct list_head list_lru_rgn;
unsigned long rgn_flags;
#define RGN_FLAG_DIRTY 0
+#define RGN_FLAG_UPDATE 1
/* region reads - for host mode */
spinlock_t rgn_lock;
@@ -215,8 +216,10 @@ struct ufshpb_lu {
/* for selecting victim */
struct victim_select_info lru_info;
struct work_struct ufshpb_normalization_work;
+ struct work_struct ufshpb_lun_reset_work;
unsigned long work_data_bits;
#define WORK_PENDING 0
+#define RESET_PENDING 1
/* pinned region information */
u32 lu_pinned_start;
--
2.25.1
[PATCH v3 1/9] scsi: ufshpb: Cache HPB Control mode on init
We will use it later, when we'll need to differentiate between device
and host control modes.
Signed-off-by: Avri Altman
---
drivers/scsi/ufs/ufshcd.h | 2 ++
drivers/scsi/ufs/ufshpb.c | 8 +---
drivers/scsi/ufs/ufshpb.h | 2 ++
3 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 49bfebf366c7..1a8dcdaa357a 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -655,6 +655,7 @@ struct ufs_hba_variant_params {
* @slave_conf_cnt: counter to check all lu finished initialization
* @hpb_disabled: flag to check if HPB is disabled
* @max_hpb_single_cmd: maximum size of single HPB command
+ * @control_mode: either host or device
*/
struct ufshpb_dev_info {
int num_lu;
@@ -663,6 +664,7 @@ struct ufshpb_dev_info {
atomic_t slave_conf_cnt;
bool hpb_disabled;
int max_hpb_single_cmd;
+ u8 control_mode;
};
#endif
diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index e97a78400f68..f9140f3a4eed 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -1522,6 +1522,9 @@ static void ufshpb_lu_parameter_init(struct ufs_hba *hba,
% (hpb->srgn_mem_size / HPB_ENTRY_SIZE);
hpb->pages_per_srgn = DIV_ROUND_UP(hpb->srgn_mem_size, PAGE_SIZE);
+
+ if (hpb_dev_info->control_mode == HPB_HOST_CONTROL)
+ hpb->is_hcm = true;
}
static int ufshpb_alloc_region_tbl(struct ufs_hba *hba, struct ufshpb_lu *hpb)
@@ -2205,11 +2208,10 @@ void ufshpb_get_dev_info(struct ufs_hba *hba, u8
*desc_buf)
{
struct ufshpb_dev_info *hpb_dev_info = &hba->ufshpb_dev;
int version, ret;
- u8 hpb_mode;
u32 max_hpb_sigle_cmd = 0;
- hpb_mode = desc_buf[DEVICE_DESC_PARAM_HPB_CONTROL];
- if (hpb_mode == HPB_HOST_CONTROL) {
+ hpb_dev_info->control_mode = desc_buf[DEVICE_DESC_PARAM_HPB_CONTROL];
+ if (hpb_dev_info->control_mode == HPB_HOST_CONTROL) {
dev_err(hba->dev, "%s: host control mode is not supported.\n",
__func__);
hpb_dev_info->hpb_disabled = true;
diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
index eb8366d47d8a..f55a71b250eb 100644
--- a/drivers/scsi/ufs/ufshpb.h
+++ b/drivers/scsi/ufs/ufshpb.h
@@ -223,6 +223,8 @@ struct ufshpb_lu {
u32 entries_per_srgn_shift;
u32 pages_per_srgn;
+ bool is_hcm;
+
struct ufshpb_stats stats;
struct ufshpb_params params;
--
2.25.1
[PATCH v3 0/9] Add Host control mode to HPB
v2 -> v3: - Attend Greg's and Can's comments - rebase on Daejun's v21 v1 -> v2: - attend Greg's and Daejun's comments - add patch 9 making host mode parameters configurable - rebase on Daejun's v19 The HPB spec defines 2 control modes - device control mode and host control mode. In oppose to device control mode, in which the host obey to whatever recommendation received from the device - In host control mode, the host uses its own algorithms to decide which regions should be activated or inactivated. We kept the host managed heuristic simple and concise. Aside from adding a by-spec functionality, host control mode entails some further potential benefits: makes the hpb logic transparent and readable, while allow tuning / scaling its various parameters, and utilize system-wide info to optimize HPB potential. This series is based on Samsung's V19 device-control HPB2.0 driver, see msg-id: 20210218090853epcms2p8ccac0b5611dec22afd04ecc06e74498e@epcms2p8 in lore.kernel.org. The patches are also available in wdc ufs repo: https://github.com/westerndigitalcorporation/WDC-UFS-REPO/tree/hpb-v21 This version was tested on Galaxy S20, and Xiaomi Mi10 pro. Your meticulous review and testing is mostly welcome and appreciated. Thanks, Avri Avri Altman (9): scsi: ufshpb: Cache HPB Control mode on init scsi: ufshpb: Add host control mode support to rsp_upiu scsi: ufshpb: Add region's reads counter scsi: ufshpb: Make eviction depends on region's reads scsi: ufshpb: Region inactivation in host mode scsi: ufshpb: Add hpb dev reset response scsi: ufshpb: Add "Cold" regions timer scsi: ufshpb: Add support for host control mode scsi: ufshpb: Make host mode parameters configurable Documentation/ABI/testing/sysfs-driver-ufs | 68 +++ drivers/scsi/ufs/ufshcd.h | 2 + drivers/scsi/ufs/ufshpb.c | 622 +++-- drivers/scsi/ufs/ufshpb.h | 44 +- 4 files changed, 691 insertions(+), 45 deletions(-) -- 2.25.1
[PATCH v3 2/9] scsi: ufshpb: Add host control mode support to rsp_upiu
In device control mode, the device may recommend the host to either
activate or inactivate a region, and the host should follow. Meaning
those are not actually recommendations, but more of instructions.
On the contrary, in host control mode, the recommendation protocol is
slightly changed:
a) The device may only recommend the host to update a subregion of an
already-active region. And,
b) The device may *not* recommend to inactivate a region.
Furthermore, in host control mode, the host may choose not to follow any
of the device's recommendations. However, in case of a recommendation to
update an active and clean subregion, it is better to follow those
recommendation because otherwise the host has no other way to know that
some internal relocation took place.
Signed-off-by: Avri Altman
---
drivers/scsi/ufs/ufshpb.c | 34 +-
drivers/scsi/ufs/ufshpb.h | 2 ++
2 files changed, 35 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index f9140f3a4eed..e052260868ad 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -156,6 +156,8 @@ static void ufshpb_set_ppn_dirty(struct ufshpb_lu *hpb, int
rgn_idx,
else
set_bit_len = cnt;
+ set_bit(RGN_FLAG_DIRTY, &rgn->rgn_flags);
+
if (rgn->rgn_state != HPB_RGN_INACTIVE &&
srgn->srgn_state == HPB_SRGN_VALID)
bitmap_set(srgn->mctx->ppn_dirty, srgn_offset, set_bit_len);
@@ -222,6 +224,11 @@ static bool ufshpb_test_ppn_dirty(struct ufshpb_lu *hpb,
int rgn_idx,
return false;
}
+static inline bool is_rgn_dirty(struct ufshpb_region *rgn)
+{
+ return test_bit(RGN_FLAG_DIRTY, &rgn->rgn_flags);
+}
+
static int ufshpb_fill_ppn_from_page(struct ufshpb_lu *hpb,
struct ufshpb_map_ctx *mctx, int pos, int len,
u64 *ppn_buf)
@@ -715,6 +722,7 @@ static void ufshpb_put_map_req(struct ufshpb_lu *hpb,
static int ufshpb_clear_dirty_bitmap(struct ufshpb_lu *hpb,
struct ufshpb_subregion *srgn)
{
+ struct ufshpb_region *rgn;
u32 num_entries = hpb->entries_per_srgn;
WARN_ON(!srgn->mctx);
@@ -723,6 +731,10 @@ static int ufshpb_clear_dirty_bitmap(struct ufshpb_lu *hpb,
num_entries = hpb->last_srgn_entries;
bitmap_zero(srgn->mctx->ppn_dirty, num_entries);
+
+ rgn = hpb->rgn_tbl + srgn->rgn_idx;
+ clear_bit(RGN_FLAG_DIRTY, &rgn->rgn_flags);
+
return 0;
}
@@ -1171,6 +1183,18 @@ static void ufshpb_rsp_req_region_update(struct
ufshpb_lu *hpb,
srgn_i =
be16_to_cpu(rsp_field->hpb_active_field[i].active_srgn);
+ rgn = hpb->rgn_tbl + rgn_i;
+ if (hpb->is_hcm &&
+ (rgn->rgn_state != HPB_RGN_ACTIVE || is_rgn_dirty(rgn))) {
+ /*
+* in host control mode, subregion activation
+* recommendations are only allowed to active regions.
+* Also, ignore recommendations for dirty regions - the
+* host will make decisions concerning those by himself
+*/
+ continue;
+ }
+
dev_dbg(&hpb->sdev_ufs_lu->sdev_dev,
"activate(%d) region %d - %d\n", i, rgn_i, srgn_i);
@@ -1178,7 +1202,6 @@ static void ufshpb_rsp_req_region_update(struct ufshpb_lu
*hpb,
ufshpb_update_active_info(hpb, rgn_i, srgn_i);
spin_unlock(&hpb->rsp_list_lock);
- rgn = hpb->rgn_tbl + rgn_i;
srgn = rgn->srgn_tbl + srgn_i;
/* blocking HPB_READ */
@@ -1189,6 +1212,14 @@ static void ufshpb_rsp_req_region_update(struct
ufshpb_lu *hpb,
hpb->stats.rb_active_cnt++;
}
+ if (hpb->is_hcm) {
+ /*
+* in host control mode the device is not allowed to inactivate
+* regions
+*/
+ goto out;
+ }
+
for (i = 0; i < rsp_field->inactive_rgn_cnt; i++) {
rgn_i = be16_to_cpu(rsp_field->hpb_inactive_field[i]);
dev_dbg(&hpb->sdev_ufs_lu->sdev_dev,
@@ -1213,6 +1244,7 @@ static void ufshpb_rsp_req_region_update(struct ufshpb_lu
*hpb,
hpb->stats.rb_inactive_cnt++;
}
+out:
dev_dbg(&hpb->sdev_ufs_lu->sdev_dev, "Noti: #ACT %u #INACT %u\n",
rsp_field->active_rgn_cnt, rsp_field->inactive_rgn_cnt);
diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
index f55a71b250eb..8e6c2a5b80a0 100644
--- a/drivers/scsi/ufs/ufshpb.h
+++ b/drivers/scsi/ufs/ufshpb.h
@@ -115,6 +115,
RE: [PATCH v3 9/9] scsi: ufshpb: Make host mode parameters configurable
> > +What: > /sys/class/scsi_device/*/device/hpb_param_sysfs/timeout_polling_interval_ms > > +Date:February 2021 > > +Contact: Avri Altman > > +Description: the frequency in which the delayed worker that checks the > > + read_timeouts is awaken. > > +>>>>>>> ef6bf08e666d... scsi: ufshpb: Make host mode parameters > configurable > > Did you mean for this last line to be here? Thanks. Done.
RE: [PATCH] scsi: ufs: convert sysfs sprintf/snprintf family to sysfs_emit
> > Fix the following coccicheck warning: > > ./drivers/scsi/ufs/ufshcd.c:1838:8-16: WARNING: use scnprintf or > sprintf. > > ./drivers/scsi/ufs/ufshcd.c:1815:8-16: WARNING: use scnprintf or > sprintf. > > ./drivers/scsi/ufs/ufshcd.c:1525:8-16: WARNING: use scnprintf or > sprintf. > > Reported-by: Abaci Robot > Signed-off-by: Jiapeng Chong Reviewed-by: Avri Altman
RE: [PATCH 1/8] scsi: ufshpb: Cache HPB Control mode on init
> > > > +static enum UFSHPB_MODE ufshpb_mode; > > How are you allowed to have a single variable for a device-specific > thing? What happens when you have two controllers or disks or whatever > you are binding to here? How does this work at all? > > This should be per-device, right? Right. Done. Not being bickering, AFAIK, there aren't, nor will be in the foreseen future, any multi-ufs-hosts designs. There were some talks in the past about ufs cards, but this is officially off the table. Thanks, Avri
RE: [PATCH 2/8] scsi: ufshpb: Add host control mode support to rsp_upiu
> > > On Wed, Jan 27, 2021 at 05:12:11PM +0200, Avri Altman wrote: > > There are some limitations to activations / inactivations in host > > control mode - Add those. > > I can not understand this changelog text, please make it more > descriptive as I have no way to know how to review this patch :( Done.
RE: [PATCH 1/8] scsi: ufshpb: Cache HPB Control mode on init
> On Sun, Jan 31, 2021 at 07:08:00AM +0000, Avri Altman wrote: > > > > > > > > +static enum UFSHPB_MODE ufshpb_mode; > > > > > > How are you allowed to have a single variable for a device-specific > > > thing? What happens when you have two controllers or disks or whatever > > > you are binding to here? How does this work at all? > > > > > > This should be per-device, right? > > Right. Done. > > > > Not being bickering, AFAIK, there aren't, nor will be in the foreseen > > future, > any multi-ufs-hosts designs. > > Why not? What prevents someone from putting 2 PCI ufs host controllers > in a system tomorrow? > > > There were some talks in the past about ufs cards, but this is officially > > off > the table. > > Never say never :) > > Seriously, how can you somehow ensure that a random manufacturer will > not do this? Better let the platform vendors answer this. As for your comment - you are obviously right - I will fix this. Thanks, Avri
RE: [PATCH 3/8] scsi: ufshpb: Add region's reads counter
> >
> > + if (ufshpb_mode == HPB_HOST_CONTROL)
> > + reads = atomic64_inc_return(&rgn->reads);
> > +
> > if (!ufshpb_is_support_chunk(transfer_len))
> > return;
> >
> > + if (ufshpb_mode == HPB_HOST_CONTROL) {
> > + /*
> > + * in host control mode, reads are the main source for
> > + * activation trials.
> > + */
> > + if (reads == ACTIVATION_THRSHLD) {
Oops - this is a bug...
> > +
> > + /* region reads - for host mode */
> > + atomic64_t reads;
>
> Why do you need an atomic variable for this? What are you trying to
> "protect" here by flushing the cpus all the time? What protects this
> variable from changing right after you have read from it (hint, you do
> that above...)
>
> atomics are not race-free, use a real lock if you need that.
We are on the data path here - this is called from queuecommand.
The "reads" counter is being symmetrically read and written,
so adding a spin lock here might become a source for contention.
Also I am not worried about the exact value of this counter, except of one
place -
See above. Will fix it.
Thanks,
Avri
RE: [PATCH 7/8] scsi: ufshpb: Add "Cold" regions timer
> > + /* region "cold" timer - for host mode */ > > + ktime_t read_timeout; > > + atomic_t read_timeout_expiries; > > Why does this have to be an atomic when you have a lock to protect this > structure already taken? Done. You are right, it is protected by the hpb state lock. Will fix it. Thanks, Avri
RE: [PATCH V1 1/3] scsi: ufs: export api for use in vendor file
>
> Exporting functions ufshcd_set_dev_pwr_mode, ufshcd_disable_vreg
> and ufshcd_enable_vreg so that vendor drivers can make use of
> them in setting vendor specific regulator setting
> in vendor specific file.
As for ufshcd_{enable,disable}_vreg - maybe inline ufshcd_toggle_vreg and use
it instead?
>
> Signed-off-by: Nitin Rawat
> Signed-off-by: Bao D. Nguyen
> Signed-off-by: Veerabhadrarao Badiganti
> ---
> drivers/scsi/ufs/ufshcd.c | 9 ++---
> drivers/scsi/ufs/ufshcd.h | 4
> 2 files changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 9c691e4..000a03a 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -8091,7 +8091,7 @@ static int ufshcd_config_vreg(struct device *dev,
> return ret;
> }
>
> -static int ufshcd_enable_vreg(struct device *dev, struct ufs_vreg *vreg)
> +int ufshcd_enable_vreg(struct device *dev, struct ufs_vreg *vreg)
> {
> int ret = 0;
>
> @@ -8110,8 +8110,9 @@ static int ufshcd_enable_vreg(struct device *dev,
> struct ufs_vreg *vreg)
> out:
> return ret;
> }
> +EXPORT_SYMBOL(ufshcd_enable_vreg);
Why do you need to export it across the kernel?
Isn't making it non-static suffices?
Do you need it for a loadable module?
>
> -static int ufshcd_disable_vreg(struct device *dev, struct ufs_vreg *vreg)
> +int ufshcd_disable_vreg(struct device *dev, struct ufs_vreg *vreg)
> {
> int ret = 0;
>
> @@ -8131,6 +8132,7 @@ static int ufshcd_disable_vreg(struct device *dev,
> struct ufs_vreg *vreg)
> out:
> return ret;
> }
> +EXPORT_SYMBOL(ufshcd_disable_vreg);
>
> static int ufshcd_setup_vreg(struct ufs_hba *hba, bool on)
> {
> @@ -8455,7 +8457,7 @@ ufshcd_send_request_sense(struct ufs_hba *hba,
> struct scsi_device *sdp)
> * Returns 0 if requested power mode is set successfully
> * Returns non-zero if failed to set the requested power mode
> */
> -static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
> +int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
> enum ufs_dev_pwr_mode pwr_mode)
> {
> unsigned char cmd[6] = { START_STOP };
> @@ -8513,6 +8515,7 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba
> *hba,
> hba->host->eh_noresume = 0;
> return ret;
> }
> +EXPORT_SYMBOL(ufshcd_set_dev_pwr_mode);
>
> static int ufshcd_link_state_transition(struct ufs_hba *hba,
> enum uic_link_state req_link_state,
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index ee61f82..1410c95 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -997,6 +997,10 @@ extern int ufshcd_dme_get_attr(struct ufs_hba *hba,
> u32 attr_sel,
>u32 *mib_val, u8 peer);
> extern int ufshcd_config_pwr_mode(struct ufs_hba *hba,
> struct ufs_pa_layer_attr *desired_pwr_mode);
> +extern int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
> + enum ufs_dev_pwr_mode
> pwr_mode);
> +extern int ufshcd_enable_vreg(struct device *dev, struct ufs_vreg *vreg);
> +extern int ufshcd_disable_vreg(struct device *dev, struct ufs_vreg *vreg);
>
> /* UIC command interfaces for DME primitives */
> #define DME_LOCAL 0
> --
> 2.7.4
RE: [PATCH V1 0/3] scsi: ufs: Add a vops to configure VCC voltage level
> > UFS specification allows different VCC configurations for UFS devices, > for example, > (1)2.70V - 3.60V (For UFS 2.x devices) > (2)2.40V - 2.70V (For UFS 3.x devices) > For platforms supporting both ufs 2.x (2.7v-3.6v) and > ufs 3.x (2.4v-2.7v), the voltage requirements (VCC) is 2.4v-3.6v. > So to support this, we need to start the ufs device initialization with > the common VCC voltage(2.7v) and after reading the device descriptor we > need to switch to the correct range(vcc min and vcc max) of VCC voltage > as per UFS device type since 2.7v is the marginal voltage as per specs > for both type of devices. > > Once VCC regulator supply has been intialised to 2.7v and UFS device > type is read from device descriptor, we follows below steps to > change the VCC voltage values. > > 1. Set the device to SLEEP state. > 2. Disable the Vcc Regulator. > 3. Set the vcc voltage according to the device type and reenable >the regulator. > 4. Set the device mode back to ACTIVE. > > The above changes are done in vendor specific file by > adding a vops which will be needed for platform > supporting both ufs 2.x and ufs 3.x devices. The flow should be generic - isn't it? Why do you need the entire flow to be vendor-specific? Why not just the parameters vendor-specific? Thanks, Avri
RE: [PATCH 3/8] scsi: ufshpb: Add region's reads counter
> > +#define WORK_PENDING 0
> > +#define ACTIVATION_THRSHLD 4 /* 4 IOs */
> Rather than fixing it with macro, how about using sysfs and make it
> configurable?
Yes.
I will add a patch making all the logic configurable.
As all those are hpb-related parameters, I think module parameters are more
adequate.
>
> > @@ -306,12 +325,39 @@ void ufshpb_prep(struct ufs_hba *hba, struct
> ufshcd_lrb *lrbp)
> > ufshpb_set_ppn_dirty(hpb, rgn_idx, srgn_idx, srgn_offset,
> >transfer_len);
> > spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
> > +
> > + if (ufshpb_mode == HPB_HOST_CONTROL)
> > + atomic64_set(&rgn->reads, 0);
> > +
> > return;
> > }
> >
> > + if (ufshpb_mode == HPB_HOST_CONTROL)
> > + reads = atomic64_inc_return(&rgn->reads);
> > +
> > if (!ufshpb_is_support_chunk(transfer_len))
> > return; <- *this*
> >
> > + if (ufshpb_mode == HPB_HOST_CONTROL) {
> > + /*
> > + * in host control mode, reads are the main source for
> > + * activation trials.
> > + */
> > + if (reads == ACTIVATION_THRSHLD) {
> If the chunk size is not supported, we can not active this region
> permanently. It may be returned before get this statement.
Yes.
I already noticed that replying to Greg.
Fixed that when I dropped the use of atomic variables.
>
> > diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
> > index 8a34b0f42754..b0e78728af38 100644
> > --- a/drivers/scsi/ufs/ufshpb.h
> > +++ b/drivers/scsi/ufs/ufshpb.h
> > @@ -115,6 +115,9 @@ struct ufshpb_region {
> > /* below information is used by lru */
> > struct list_head list_lru_rgn;
> > unsigned long rgn_flags;
> > +
> > + /* region reads - for host mode */
> > + atomic64_t reads;
> I think 32 bits are suitable, because it is normalized by worker on every
> specific time.
Done.
RE: [PATCH 4/8] scsi: ufshpb: Make eviction depends on region's reads
> > > Hi Avri, > > > + /* > > + * in host control mode, verify that the exiting region > > + * has less reads > > + */ > > + if (ufshpb_mode == HPB_HOST_CONTROL && > > + atomic64_read(&rgn->reads) > (EVICTION_THRSHLD >> 1)) > Why we use shifted value to verify less read? I think we should use another > value to verify. Yes. Will make every logic parameters configurable.
RE: [PATCH 6/8] scsi: ufshpb: Add hpb dev reset response
> > Hi Avri, > > > + list_for_each_entry_safe(rgn, next_rgn, &lru_info->lh_lru_rgn, > > + list_lru_rgn) > How about replace list_for_each_entry_safe to list_for_each_entry? Done. Can also use the relaxed version in the timeout handler as well (patch 7/8). Thanks, Avri
RE: [PATCH 3/8] scsi: ufshpb: Add region's reads counter
> > On Mon, Feb 01, 2021 at 07:12:53AM +, Avri Altman wrote: > > > > +#define WORK_PENDING 0 > > > > +#define ACTIVATION_THRSHLD 4 /* 4 IOs */ > > > Rather than fixing it with macro, how about using sysfs and make it > > > configurable? > > Yes. > > I will add a patch making all the logic configurable. > > As all those are hpb-related parameters, I think module parameters are > more adequate. > > No, this is not the 1990's, please never add new module parameters to > drivers. If not for the basic problem of they do not work on a > per-device basis, but on a per-driver basis, which is what you almost > never want. OK. > > But why would you want to change this value, why can't the driver "just > work" and not need manual intervention? It is. But those are a knobs each vendor may want to tweak, So it'll be optimized with its internal device's implementation. Tweaking the parameters, as well as the entire logic, is really an endless task. Some logic works better for some scenarios, while falling behind on others. How about leaving it for now, to be elaborated it in the future? Maybe even can be a part of a scheme, to make the logic proprietary?
RE: [PATCH 3/8] scsi: ufshpb: Add region's reads counter
> > On Mon, Feb 01, 2021 at 07:51:19AM +, Avri Altman wrote: > > > > > > On Mon, Feb 01, 2021 at 07:12:53AM +, Avri Altman wrote: > > > > > > +#define WORK_PENDING 0 > > > > > > +#define ACTIVATION_THRSHLD 4 /* 4 IOs */ > > > > > Rather than fixing it with macro, how about using sysfs and make it > > > > > configurable? > > > > Yes. > > > > I will add a patch making all the logic configurable. > > > > As all those are hpb-related parameters, I think module parameters are > > > more adequate. > > > > > > No, this is not the 1990's, please never add new module parameters to > > > drivers. If not for the basic problem of they do not work on a > > > per-device basis, but on a per-driver basis, which is what you almost > > > never want. > > OK. > > > > > > > > But why would you want to change this value, why can't the driver "just > > > work" and not need manual intervention? > > It is. > > But those are a knobs each vendor may want to tweak, > > So it'll be optimized with its internal device's implementation. > > > > Tweaking the parameters, as well as the entire logic, is really an endless > task. > > Some logic works better for some scenarios, while falling behind on others. > > Shouldn't the hardware know how to handle this dynamically? If not, how > is a user going to know? There is one "brain". It is either in the device - in device mode, Or in the host - in host mode control. The "brain" decides which region is active, thus carrying the physical address along with the logical - minimizing context switches in the device's RAM. There can be up to N active regions. Activation and deactivation has its overhead. So basically it is a constraint-optimization problem. > > > How about leaving it for now, to be elaborated it in the future? > > I do not care, just do not make it a module parameter for the reason > that does not work on a per-device basis. OK. Will make it a sysfs per hpb-lun, like Daejun proposed. Thanks, Avri
RE: [PATCH 3/8] scsi: ufshpb: Add region's reads counter
> On Mon, Feb 01, 2021 at 08:17:59AM +0000, Avri Altman wrote: > > > > > > On Mon, Feb 01, 2021 at 07:51:19AM +, Avri Altman wrote: > > > > > > > > > > On Mon, Feb 01, 2021 at 07:12:53AM +, Avri Altman wrote: > > > > > > > > +#define WORK_PENDING 0 > > > > > > > > +#define ACTIVATION_THRSHLD 4 /* 4 IOs */ > > > > > > > Rather than fixing it with macro, how about using sysfs and make > > > > > > > it > > > > > > > configurable? > > > > > > Yes. > > > > > > I will add a patch making all the logic configurable. > > > > > > As all those are hpb-related parameters, I think module parameters > are > > > > > more adequate. > > > > > > > > > > No, this is not the 1990's, please never add new module parameters to > > > > > drivers. If not for the basic problem of they do not work on a > > > > > per-device basis, but on a per-driver basis, which is what you almost > > > > > never want. > > > > OK. > > > > > > > > > > > > > > But why would you want to change this value, why can't the driver > "just > > > > > work" and not need manual intervention? > > > > It is. > > > > But those are a knobs each vendor may want to tweak, > > > > So it'll be optimized with its internal device's implementation. > > > > > > > > Tweaking the parameters, as well as the entire logic, is really an > > > > endless > > > task. > > > > Some logic works better for some scenarios, while falling behind on > others. > > > > > > Shouldn't the hardware know how to handle this dynamically? If not, how > > > is a user going to know? > > There is one "brain". > > It is either in the device - in device mode, Or in the host - in host mode > control. > > The "brain" decides which region is active, thus carrying the physical > > address > along with the logical - > > minimizing context switches in the device's RAM. > > > > There can be up to N active regions. > > Activation and deactivation has its overhead. > > So basically it is a constraint-optimization problem. > > So how do you solve it? And how would you expect a user to solve it if > the kernel can not? > > You better document the heck out of these configuration options :) Yes. Will do. Thanks, Avri
RE: [PATCH v19 1/3] scsi: ufs: Introduce HPB feature
Daejun,
> static const struct attribute_group *ufshcd_driver_groups[] = {
> &ufs_sysfs_unit_descriptor_group,
> &ufs_sysfs_lun_attributes_group,
> +#ifdef CONFIG_SCSI_UFS_HPB
> + &ufs_sysfs_hpb_stat_group,
> +#endif
> NULL,
> };
Aren’t you creating a hpb_stats entries for every lun (even wlun)?
This is confusing, even if safe (any non-hpb lun returns NODEV).
Also user-space have no way to know which entry is valid.
Can we group those under ufshpb_lu for valid hpb luns only?
Also need to document the stats? Maybe in a separate sysfs-driver-ufs-features?
Thanks,
Avri
[PATCH v2 1/9] scsi: ufshpb: Cache HPB Control mode on init
We will use it later, when we'll need to differentiate between device
and host control modes.
Signed-off-by: Avri Altman
---
drivers/scsi/ufs/ufshcd.h | 2 ++
drivers/scsi/ufs/ufshpb.c | 8 +---
drivers/scsi/ufs/ufshpb.h | 2 ++
3 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 961fc5b77943..7aeb83b10c37 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -654,6 +654,7 @@ struct ufs_hba_variant_params {
* @srgn_size: device reported HPB sub-region size
* @slave_conf_cnt: counter to check all lu finished initialization
* @hpb_disabled: flag to check if HPB is disabled
+ * @control_mode: either host or device
*/
struct ufshpb_dev_info {
int num_lu;
@@ -661,6 +662,7 @@ struct ufshpb_dev_info {
int srgn_size;
atomic_t slave_conf_cnt;
bool hpb_disabled;
+ u8 control_mode;
};
#endif
diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index 73e7b3ed04a4..46f6a7104e7e 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -1128,6 +1128,9 @@ static void ufshpb_lu_parameter_init(struct ufs_hba *hba,
(hpb->srgn_mem_size / HPB_ENTRY_SIZE));
hpb->pages_per_srgn = DIV_ROUND_UP(hpb->srgn_mem_size, PAGE_SIZE);
+
+ if (hpb_dev_info->control_mode == HPB_HOST_CONTROL)
+ hpb->is_hcm = true;
}
static int ufshpb_alloc_region_tbl(struct ufs_hba *hba, struct ufshpb_lu *hpb)
@@ -1694,10 +1697,9 @@ void ufshpb_get_dev_info(struct ufs_hba *hba, u8
*desc_buf)
{
struct ufshpb_dev_info *hpb_dev_info = &hba->ufshpb_dev;
int version;
- u8 hpb_mode;
- hpb_mode = desc_buf[DEVICE_DESC_PARAM_HPB_CONTROL];
- if (hpb_mode == HPB_HOST_CONTROL) {
+ hpb_dev_info->control_mode = desc_buf[DEVICE_DESC_PARAM_HPB_CONTROL];
+ if (hpb_dev_info->control_mode == HPB_HOST_CONTROL) {
dev_err(hba->dev, "%s: host control mode is not supported.\n",
__func__);
hpb_dev_info->hpb_disabled = true;
diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
index 2c43a03b66b6..afeb6365daf8 100644
--- a/drivers/scsi/ufs/ufshpb.h
+++ b/drivers/scsi/ufs/ufshpb.h
@@ -186,6 +186,8 @@ struct ufshpb_lu {
u32 entries_per_srgn_shift;
u32 pages_per_srgn;
+ bool is_hcm;
+
struct ufshpb_stats stats;
struct kmem_cache *map_req_cache;
--
2.25.1
[PATCH v2 2/9] scsi: ufshpb: Add host control mode support to rsp_upiu
In device control mode, the device may recommend the host to either
activate or inactivate a region, and the host should follow. Meaning
those are not actually recommendations, but more of instructions.
On the contrary, in host control mode, the recommendation protocol is
slightly changed:
a) The device may only recommend the host to update a subregion of an
already-active region. And,
b) The device may *not* recommend to inactivate a region.
Furthermore, in host control mode, the host may choose not to follow any
of the device's recommendations. However, in case of a recommendation to
update an active and clean subregion, it is better to follow those
recommendation because otherwise the host has no other way to know that
some internal relocation took place.
Signed-off-by: Avri Altman
---
drivers/scsi/ufs/ufshpb.c | 35 +++
drivers/scsi/ufs/ufshpb.h | 6 ++
2 files changed, 41 insertions(+)
diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index 46f6a7104e7e..61de80a778a7 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -138,6 +138,8 @@ static void ufshpb_set_ppn_dirty(struct ufshpb_lu *hpb, int
rgn_idx,
else
set_bit_len = cnt;
+ set_bit(RGN_FLAG_DIRTY, &rgn->rgn_flags);
+
if (rgn->rgn_state != HPB_RGN_INACTIVE &&
srgn->srgn_state == HPB_SRGN_VALID)
bitmap_set(srgn->mctx->ppn_dirty, srgn_offset, set_bit_len);
@@ -199,6 +201,11 @@ static bool ufshpb_test_ppn_dirty(struct ufshpb_lu *hpb,
int rgn_idx,
return false;
}
+static inline bool is_rgn_dirty(struct ufshpb_region *rgn)
+{
+ return test_bit(RGN_FLAG_DIRTY, &rgn->rgn_flags);
+}
+
static u64 ufshpb_get_ppn(struct ufshpb_lu *hpb,
struct ufshpb_map_ctx *mctx, int pos, int *error)
{
@@ -380,8 +387,12 @@ static void ufshpb_put_map_req(struct ufshpb_lu *hpb,
static int ufshpb_clear_dirty_bitmap(struct ufshpb_lu *hpb,
struct ufshpb_subregion *srgn)
{
+ struct ufshpb_region *rgn;
+
WARN_ON(!srgn->mctx);
bitmap_zero(srgn->mctx->ppn_dirty, hpb->entries_per_srgn);
+ rgn = hpb->rgn_tbl + srgn->rgn_idx;
+ clear_bit(RGN_FLAG_DIRTY, &rgn->rgn_flags);
return 0;
}
@@ -814,17 +825,39 @@ static void ufshpb_rsp_req_region_update(struct ufshpb_lu
*hpb,
*/
spin_lock(&hpb->rsp_list_lock);
for (i = 0; i < rsp_field->active_rgn_cnt; i++) {
+ struct ufshpb_region *rgn;
+
rgn_idx =
be16_to_cpu(rsp_field->hpb_active_field[i].active_rgn);
srgn_idx =
be16_to_cpu(rsp_field->hpb_active_field[i].active_srgn);
+ rgn = hpb->rgn_tbl + rgn_idx;
+ if (hpb->is_hcm &&
+ (rgn->rgn_state != HPB_RGN_ACTIVE || is_rgn_dirty(rgn))) {
+ /*
+* in host control mode, subregion activation
+* recommendations are only allowed to active regions.
+* Also, ignore recommendations for dirty regions - the
+* host will make decisions concerning those by himself
+*/
+ continue;
+ }
+
dev_dbg(&hpb->sdev_ufs_lu->sdev_dev,
"activate(%d) region %d - %d\n", i, rgn_idx, srgn_idx);
ufshpb_update_active_info(hpb, rgn_idx, srgn_idx);
hpb->stats.rb_active_cnt++;
}
+ if (hpb->is_hcm) {
+ /*
+* in host control mode the device is not allowed to inactivate
+* regions
+*/
+ goto out_unlock;
+ }
+
for (i = 0; i < rsp_field->inactive_rgn_cnt; i++) {
rgn_idx = be16_to_cpu(rsp_field->hpb_inactive_field[i]);
dev_dbg(&hpb->sdev_ufs_lu->sdev_dev,
@@ -832,6 +865,8 @@ static void ufshpb_rsp_req_region_update(struct ufshpb_lu
*hpb,
ufshpb_update_inactive_info(hpb, rgn_idx);
hpb->stats.rb_inactive_cnt++;
}
+
+out_unlock:
spin_unlock(&hpb->rsp_list_lock);
dev_dbg(&hpb->sdev_ufs_lu->sdev_dev, "Noti: #ACT %u #INACT %u\n",
diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
index afeb6365daf8..5ec4023db74d 100644
--- a/drivers/scsi/ufs/ufshpb.h
+++ b/drivers/scsi/ufs/ufshpb.h
@@ -48,6 +48,11 @@ enum UFSHPB_MODE {
HPB_DEVICE_CONTROL,
};
+enum HPB_RGN_FLAGS {
+ RGN_FLAG_UPDATE = 0,
+ RGN_FLAG_DIRTY,
+};
+
enum UFSHPB_STATE {
HPB_PRESENT = 1,
HPB_SUSPEND,
@@ -109,6 +114,7 @@ struct ufshpb_region {
/* below information is used by lru */
struct list_head list_lru_rgn;
+ unsigned long rgn_flags;
};
#define for_each_sub_region(rgn, i, srgn) \
--
2.25.1
[PATCH v2 4/9] scsi: ufshpb: Make eviction depends on region's reads
In host mode, eviction is considered an extreme measure.
verify that the entering region has enough reads, and the exiting
region has much less reads.
Signed-off-by: Avri Altman
---
drivers/scsi/ufs/ufshpb.c | 42 ---
1 file changed, 39 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index de4866d42df0..bae7dca105da 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -18,6 +18,7 @@
#define WORK_PENDING 0
#define ACTIVATION_THRSHLD 4 /* 4 IOs */
+#define EVICTION_THRSHLD (ACTIVATION_THRSHLD << 6) /* 256 IOs */
/* memory management */
static struct kmem_cache *ufshpb_mctx_cache;
@@ -644,6 +645,13 @@ static struct ufshpb_region *ufshpb_victim_lru_info(struct
ufshpb_lu *hpb)
if (ufshpb_check_srgns_issue_state(hpb, rgn))
continue;
+ /*
+* in host control mode, verify that the exiting region
+* has less reads
+*/
+ if (hpb->is_hcm && rgn->reads > (EVICTION_THRSHLD >> 1))
+ continue;
+
victim_rgn = rgn;
break;
}
@@ -799,7 +807,7 @@ static int ufshpb_issue_map_req(struct ufshpb_lu *hpb,
static int ufshpb_add_region(struct ufshpb_lu *hpb, struct ufshpb_region *rgn)
{
- struct ufshpb_region *victim_rgn;
+ struct ufshpb_region *victim_rgn = NULL;
struct victim_select_info *lru_info = &hpb->lru_info;
unsigned long flags;
int ret = 0;
@@ -827,6 +835,16 @@ static int ufshpb_add_region(struct ufshpb_lu *hpb, struct
ufshpb_region *rgn)
* because the device could detect this region
* by not issuing HPB_READ
*/
+
+ /*
+* in host control mode, verify that the entering
+* region has enough reads
+*/
+ if (hpb->is_hcm && rgn->reads < EVICTION_THRSHLD) {
+ ret = -EACCES;
+ goto out;
+ }
+
victim_rgn = ufshpb_victim_lru_info(hpb);
if (!victim_rgn) {
dev_warn(&hpb->sdev_ufs_lu->sdev_dev,
@@ -1034,8 +1052,13 @@ static void ufshpb_run_active_subregion_list(struct
ufshpb_lu *hpb)
rgn = hpb->rgn_tbl + srgn->rgn_idx;
ret = ufshpb_add_region(hpb, rgn);
- if (ret)
- goto active_failed;
+ if (ret) {
+ if (ret == -EACCES) {
+ spin_lock_irqsave(&hpb->rsp_list_lock, flags);
+ continue;
+ }
+ goto add_region_failed;
+ }
ret = ufshpb_issue_map_req(hpb, rgn, srgn);
if (ret) {
@@ -1049,9 +1072,22 @@ static void ufshpb_run_active_subregion_list(struct
ufshpb_lu *hpb)
spin_unlock_irqrestore(&hpb->rsp_list_lock, flags);
return;
+add_region_failed:
+ if (hpb->is_hcm) {
+ /*
+* In host control mode, it is common that eviction trials
+* fail, either because the entering region didn't have enough
+* reads, or a poor-performing exiting region wasn't found.
+* No need to re-insert those regions to the "to-be-activated"
+* list.
+*/
+ return;
+ }
+
active_failed:
dev_err(&hpb->sdev_ufs_lu->sdev_dev, "failed to activate region %d -
%d, will retry\n",
rgn->rgn_idx, srgn->srgn_idx);
+
spin_lock_irqsave(&hpb->rsp_list_lock, flags);
ufshpb_add_active_list(hpb, rgn, srgn);
spin_unlock_irqrestore(&hpb->rsp_list_lock, flags);
--
2.25.1
[PATCH v2 3/9] scsi: ufshpb: Add region's reads counter
In host control mode, reads are the major source of activation trials.
Keep track of those reads counters, for both active as well inactive
regions.
We reset the read counter upon write - we are only interested in "clean"
reads. less intuitive however, is that we also reset it upon region's
deactivation. Region deactivation is often due to the fact that
eviction took place: a region become active on the expense of another.
This is happening when the max-active-regions limit has crossed. If we
don’t reset the counter, we will trigger a lot of trashing of the HPB
database, since few reads (or even one) to the region that was
deactivated, will trigger a re-activation trial.
Keep those counters normalized, as we are using those reads as a
comparative score, to make various decisions.
If during consecutive normalizations an active region has exhaust its
reads - inactivate it.
Signed-off-by: Avri Altman
---
drivers/scsi/ufs/ufshpb.c | 109 --
drivers/scsi/ufs/ufshpb.h | 6 +++
2 files changed, 100 insertions(+), 15 deletions(-)
diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index 61de80a778a7..de4866d42df0 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -16,6 +16,9 @@
#include "ufshpb.h"
#include "../sd.h"
+#define WORK_PENDING 0
+#define ACTIVATION_THRSHLD 4 /* 4 IOs */
+
/* memory management */
static struct kmem_cache *ufshpb_mctx_cache;
static mempool_t *ufshpb_mctx_pool;
@@ -261,6 +264,21 @@ ufshpb_set_hpb_read_to_upiu(struct ufshpb_lu *hpb, struct
ufshcd_lrb *lrbp,
lrbp->cmd->cmd_len = UFS_CDB_SIZE;
}
+static void ufshpb_update_active_info(struct ufshpb_lu *hpb, int rgn_idx,
+ int srgn_idx)
+{
+ struct ufshpb_region *rgn;
+ struct ufshpb_subregion *srgn;
+
+ rgn = hpb->rgn_tbl + rgn_idx;
+ srgn = rgn->srgn_tbl + srgn_idx;
+
+ list_del_init(&rgn->list_inact_rgn);
+
+ if (list_empty(&srgn->list_act_srgn))
+ list_add_tail(&srgn->list_act_srgn, &hpb->lh_act_srgn);
+}
+
/*
* This function will set up HPB read command using host-side L2P map data.
* In HPB v1.0, maximum size of HPB read command is 4KB.
@@ -306,12 +324,45 @@ void ufshpb_prep(struct ufs_hba *hba, struct ufshcd_lrb
*lrbp)
ufshpb_set_ppn_dirty(hpb, rgn_idx, srgn_idx, srgn_offset,
transfer_len);
spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
+
+ if (hpb->is_hcm) {
+ spin_lock_irqsave(&rgn->rgn_lock, flags);
+ rgn->reads = 0;
+ spin_unlock_irqrestore(&rgn->rgn_lock, flags);
+ }
+
return;
}
if (!ufshpb_is_support_chunk(transfer_len))
return;
+ if (hpb->is_hcm) {
+ bool activate = false;
+ /*
+* in host control mode, reads are the main source for
+* activation trials.
+*/
+ spin_lock_irqsave(&rgn->rgn_lock, flags);
+ rgn->reads++;
+ if (rgn->reads == ACTIVATION_THRSHLD)
+ activate = true;
+ spin_unlock_irqrestore(&rgn->rgn_lock, flags);
+ if (activate) {
+ spin_lock_irqsave(&hpb->rsp_list_lock, flags);
+ ufshpb_update_active_info(hpb, rgn_idx, srgn_idx);
+ hpb->stats.rb_active_cnt++;
+ spin_unlock_irqrestore(&hpb->rsp_list_lock, flags);
+ dev_dbg(&hpb->sdev_ufs_lu->sdev_dev,
+ "activate region %d-%d\n", rgn_idx, srgn_idx);
+ }
+
+ /* keep those counters normalized */
+ if (rgn->reads > hpb->entries_per_srgn &&
+ !test_and_set_bit(WORK_PENDING, &hpb->work_data_bits))
+ schedule_work(&hpb->ufshpb_normalization_work);
+ }
+
spin_lock_irqsave(&hpb->rgn_state_lock, flags);
if (ufshpb_test_ppn_dirty(hpb, rgn_idx, srgn_idx, srgn_offset,
transfer_len)) {
@@ -396,21 +447,6 @@ static int ufshpb_clear_dirty_bitmap(struct ufshpb_lu *hpb,
return 0;
}
-static void ufshpb_update_active_info(struct ufshpb_lu *hpb, int rgn_idx,
- int srgn_idx)
-{
- struct ufshpb_region *rgn;
- struct ufshpb_subregion *srgn;
-
- rgn = hpb->rgn_tbl + rgn_idx;
- srgn = rgn->srgn_tbl + srgn_idx;
-
- list_del_init(&rgn->list_inact_rgn);
-
- if (list_empty(&srgn->list_act_srgn))
- list_add_tail(&srgn->list
[PATCH v2 5/9] scsi: ufshpb: Region inactivation in host mode
I host mode, the host is expected to send HPB-WRITE-BUFFER with
buffer-id = 0x1 when it inactivates a region.
Use the map-requests pool as there is no point in assigning a
designated cache for umap-requests.
Signed-off-by: Avri Altman
---
drivers/scsi/ufs/ufshpb.c | 119 +++---
drivers/scsi/ufs/ufshpb.h | 4 ++
2 files changed, 103 insertions(+), 20 deletions(-)
diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index bae7dca105da..49c74de539b7 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -391,49 +391,66 @@ void ufshpb_prep(struct ufs_hba *hba, struct ufshcd_lrb
*lrbp)
hpb->stats.hit_cnt++;
}
+static struct ufshpb_req *ufshpb_get_req(struct ufshpb_lu *hpb,
+int rgn_idx, enum req_opf dir)
+{
+ struct ufshpb_req *rq;
+ struct request *req;
+
+ rq = kmem_cache_alloc(hpb->map_req_cache, GFP_KERNEL);
+ if (!rq)
+ return NULL;
+
+ req = blk_get_request(hpb->sdev_ufs_lu->request_queue,
+ dir, 0);
+ if (IS_ERR(req))
+ goto free_rq;
+
+ rq->hpb = hpb;
+ rq->req = req;
+ rq->rgn_idx = rgn_idx;
+
+ return rq;
+
+free_rq:
+ kmem_cache_free(hpb->map_req_cache, rq);
+ return NULL;
+}
+
+static void ufshpb_put_req(struct ufshpb_lu *hpb, struct ufshpb_req *rq)
+{
+ blk_put_request(rq->req);
+ kmem_cache_free(hpb->map_req_cache, rq);
+}
+
static struct ufshpb_req *ufshpb_get_map_req(struct ufshpb_lu *hpb,
struct ufshpb_subregion *srgn)
{
struct ufshpb_req *map_req;
- struct request *req;
struct bio *bio;
- map_req = kmem_cache_alloc(hpb->map_req_cache, GFP_KERNEL);
+ map_req = ufshpb_get_req(hpb, srgn->rgn_idx, REQ_OP_SCSI_IN);
if (!map_req)
return NULL;
- req = blk_get_request(hpb->sdev_ufs_lu->request_queue,
- REQ_OP_SCSI_IN, 0);
- if (IS_ERR(req))
- goto free_map_req;
-
bio = bio_alloc(GFP_KERNEL, hpb->pages_per_srgn);
if (!bio) {
- blk_put_request(req);
- goto free_map_req;
+ ufshpb_put_req(hpb, map_req);
+ return NULL;
}
- map_req->hpb = hpb;
- map_req->req = req;
map_req->bio = bio;
-
- map_req->rgn_idx = srgn->rgn_idx;
map_req->srgn_idx = srgn->srgn_idx;
map_req->mctx = srgn->mctx;
return map_req;
-
-free_map_req:
- kmem_cache_free(hpb->map_req_cache, map_req);
- return NULL;
}
static void ufshpb_put_map_req(struct ufshpb_lu *hpb,
- struct ufshpb_req *map_req)
+ struct ufshpb_req *map_req)
{
bio_put(map_req->bio);
- blk_put_request(map_req->req);
- kmem_cache_free(hpb->map_req_cache, map_req);
+ ufshpb_put_req(hpb, map_req);
}
static int ufshpb_clear_dirty_bitmap(struct ufshpb_lu *hpb,
@@ -488,6 +505,13 @@ static void ufshpb_activate_subregion(struct ufshpb_lu
*hpb,
srgn->srgn_state = HPB_SRGN_VALID;
}
+static void ufshpb_umap_req_compl_fn(struct request *req, blk_status_t error)
+{
+ struct ufshpb_req *umap_req = (struct ufshpb_req *)req->end_io_data;
+
+ ufshpb_put_req(umap_req->hpb, umap_req);
+}
+
static void ufshpb_map_req_compl_fn(struct request *req, blk_status_t error)
{
struct ufshpb_req *map_req = (struct ufshpb_req *) req->end_io_data;
@@ -506,6 +530,14 @@ static void ufshpb_map_req_compl_fn(struct request *req,
blk_status_t error)
ufshpb_put_map_req(map_req->hpb, map_req);
}
+static void ufshpb_set_write_buf_cmd(unsigned char *cdb, int rgn_idx)
+{
+ cdb[0] = UFSHPB_WRITE_BUFFER;
+ cdb[1] = UFSHPB_WRITE_BUFFER_ID;
+ put_unaligned_be16(rgn_idx, &cdb[2]);
+ cdb[9] = 0x00;
+}
+
static void ufshpb_set_read_buf_cmd(unsigned char *cdb, int rgn_idx,
int srgn_idx, int srgn_mem_size)
{
@@ -519,6 +551,27 @@ static void ufshpb_set_read_buf_cmd(unsigned char *cdb,
int rgn_idx,
cdb[9] = 0x00;
}
+static int ufshpb_execute_umap_req(struct ufshpb_lu *hpb,
+ struct ufshpb_req *umap_req)
+{
+ struct request_queue *q;
+ struct request *req;
+ struct scsi_request *rq;
+
+ q = hpb->sdev_ufs_lu->request_queue;
+ req = umap_req->req;
+ req->timeout = 0;
+ req->end_io_data = (void *)umap_req;
+ rq = scsi_req(req);
+ ufshpb_set_write_buf_cmd(rq->cmd, umap_req->rgn_idx);
+ rq->cmd_len = HPB_WRITE_BUFFER_CMD_LENGTH;
+
+ blk_execute_rq_nowait(q, NULL, req, 1, ufshpb_umap_req_
[PATCH v2 0/9] Add Host control mode to HPB
v1 -> v2: - attend Greg's and Daejun's comments - add patch 9 making host mode parameters configurable - rebase on Daejun's v19 The HPB spec defines 2 control modes - device control mode and host control mode. In oppose to device control mode, in which the host obey to whatever recommendation received from the device - In host control mode, the host uses its own algorithms to decide which regions should be activated or inactivated. We kept the host managed heuristic simple and concise. Aside from adding a by-spec functionality, host control mode entails some further potential benefits: makes the hpb logic transparent and readable, while allow tuning / scaling its various parameters, and utilize system-wide info to optimize HPB potential. This series is based on Samsung's V19 device-control HPB1.0 driver, see msg-id: 20210129052848epcms2p6e5797efd94e6282b76ad9ae6c99e3ab5@epcms2p6 in lore.kernel.org. The patches are also available in wdc ufs repo: https://github.com/westerndigitalcorporation/WDC-UFS-REPO/tree/hpb-v19 This version was tested on Galaxy S20, and Xiaomi Mi10 pro. Your meticulous review and testing is mostly welcome and appreciated. Thanks, Avri Avri Altman (9): scsi: ufshpb: Cache HPB Control mode on init scsi: ufshpb: Add host control mode support to rsp_upiu scsi: ufshpb: Add region's reads counter scsi: ufshpb: Make eviction depends on region's reads scsi: ufshpb: Region inactivation in host mode scsi: ufshpb: Add hpb dev reset response scsi: ufshpb: Add "Cold" regions timer scsi: ufshpb: Add support for host control mode scsi: ufshpb: Make host mode parameters configurable drivers/scsi/ufs/ufshcd.c | 1 + drivers/scsi/ufs/ufshcd.h | 2 + drivers/scsi/ufs/ufshpb.c | 697 +++--- drivers/scsi/ufs/ufshpb.h | 47 +++ 4 files changed, 697 insertions(+), 50 deletions(-) -- 2.25.1
[PATCH v2 6/9] scsi: ufshpb: Add hpb dev reset response
The spec does not define what is the host's recommended response when
the device send hpb dev reset response (oper 0x2).
We will update all active hpb regions: mark them and do that on the next
read.
Signed-off-by: Avri Altman
---
drivers/scsi/ufs/ufshpb.c | 54 ---
drivers/scsi/ufs/ufshpb.h | 1 +
2 files changed, 52 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index 49c74de539b7..28e0025507a1 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -17,6 +17,7 @@
#include "../sd.h"
#define WORK_PENDING 0
+#define RESET_PENDING 1
#define ACTIVATION_THRSHLD 4 /* 4 IOs */
#define EVICTION_THRSHLD (ACTIVATION_THRSHLD << 6) /* 256 IOs */
@@ -349,7 +350,8 @@ void ufshpb_prep(struct ufs_hba *hba, struct ufshcd_lrb
*lrbp)
if (rgn->reads == ACTIVATION_THRSHLD)
activate = true;
spin_unlock_irqrestore(&rgn->rgn_lock, flags);
- if (activate) {
+ if (activate ||
+ test_and_clear_bit(RGN_FLAG_UPDATE, &rgn->rgn_flags)) {
spin_lock_irqsave(&hpb->rsp_list_lock, flags);
ufshpb_update_active_info(hpb, rgn_idx, srgn_idx);
hpb->stats.rb_active_cnt++;
@@ -1068,6 +1070,24 @@ void ufshpb_rsp_upiu(struct ufs_hba *hba, struct
ufshcd_lrb *lrbp)
case HPB_RSP_DEV_RESET:
dev_warn(&hpb->sdev_ufs_lu->sdev_dev,
"UFS device lost HPB information during PM.\n");
+
+ if (hpb->is_hcm) {
+ struct ufshpb_lu *h;
+ struct scsi_device *sdev;
+
+ shost_for_each_device(sdev, hba->host) {
+ h = sdev->hostdata;
+ if (!h)
+ continue;
+
+ if (test_and_set_bit(RESET_PENDING,
+&h->work_data_bits))
+ continue;
+
+ schedule_work(&h->ufshpb_lun_reset_work);
+ }
+ }
+
break;
default:
dev_notice(&hpb->sdev_ufs_lu->sdev_dev,
@@ -1200,6 +1220,27 @@ static void ufshpb_run_inactive_region_list(struct
ufshpb_lu *hpb)
spin_unlock_irqrestore(&hpb->rsp_list_lock, flags);
}
+static void ufshpb_reset_work_handler(struct work_struct *work)
+{
+ struct ufshpb_lu *hpb;
+ struct victim_select_info *lru_info;
+ struct ufshpb_region *rgn;
+ unsigned long flags;
+
+ hpb = container_of(work, struct ufshpb_lu, ufshpb_lun_reset_work);
+
+ lru_info = &hpb->lru_info;
+
+ spin_lock_irqsave(&hpb->rgn_state_lock, flags);
+
+ list_for_each_entry(rgn, &lru_info->lh_lru_rgn, list_lru_rgn)
+ set_bit(RGN_FLAG_UPDATE, &rgn->rgn_flags);
+
+ spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
+
+ clear_bit(RESET_PENDING, &hpb->work_data_bits);
+}
+
static void ufshpb_normalization_work_handler(struct work_struct *work)
{
struct ufshpb_lu *hpb;
@@ -1392,6 +1433,8 @@ static int ufshpb_alloc_region_tbl(struct ufs_hba *hba,
struct ufshpb_lu *hpb)
} else {
rgn->rgn_state = HPB_RGN_INACTIVE;
}
+
+ rgn->rgn_flags = 0;
}
return 0;
@@ -1502,9 +1545,12 @@ static int ufshpb_lu_hpb_init(struct ufs_hba *hba,
struct ufshpb_lu *hpb)
INIT_LIST_HEAD(&hpb->list_hpb_lu);
INIT_WORK(&hpb->map_work, ufshpb_map_work_handler);
- if (hpb->is_hcm)
+ if (hpb->is_hcm) {
INIT_WORK(&hpb->ufshpb_normalization_work,
ufshpb_normalization_work_handler);
+ INIT_WORK(&hpb->ufshpb_lun_reset_work,
+ ufshpb_reset_work_handler);
+ }
hpb->map_req_cache = kmem_cache_create("ufshpb_req_cache",
sizeof(struct ufshpb_req), 0, 0, NULL);
@@ -1591,8 +1637,10 @@ static void ufshpb_discard_rsp_lists(struct ufshpb_lu
*hpb)
static void ufshpb_cancel_jobs(struct ufshpb_lu *hpb)
{
- if (hpb->is_hcm)
+ if (hpb->is_hcm) {
+ cancel_work_sync(&hpb->ufshpb_lun_reset_work);
cancel_work_sync(&hpb->ufshpb_normalization_work);
+ }
cancel_work_sync(&hpb->map_work);
}
diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
index 71b082ee7876..e55892ceb3fc 100644
--- a/drivers/scsi/ufs/ufshpb.h
+++ b/drivers/scsi/ufs/ufshpb.h
@@ -184,6 +184,7 @@ struct ufshpb_lu {
/* for se
[PATCH v2 9/9] scsi: ufshpb: Make host mode parameters configurable
We can make use of this commit, to elaborate some more of the host
control mode logic, explaining what role play each and every variable:
- activation_thld - In host control mode, reads are the major source of
activation trials. once this threshold hs met, the region is added
to the "to-be-activated" list. Since we reset the read counter upon
write, this include sending a rb command updating the region ppn as
well.
- normalization_factor - We think of the regions as "buckets". Those
buckets are being filled with reads, and emptied on write. We use
entries_per_srgn - the amount of blocks in a subregion as our bucket
size. This applies because HPB1.0 only concern a single-block
reads. Once the bucket size is crossed, we trigger a normalization
work - not only to avoid overflow, but mainly because we want to
keep those counters normalized, as we are using those reads as a
comparative score, to make various decisions. The normalization is
dividing (shift right) the read counter by the normalization_factor.
If during consecutive normalizations an active region has exhaust
its reads - inactivate it.
- eviction_thld_enter - Region deactivation is often due to the fact
that eviction took place: a region become active on the expense of
another. This is happening when the max-active-regions limit has
crossed. In host mode, eviction is considered an extreme measure.
We want to verify that the entering region has enough reads, and the
exiting region has much less reads. eviction_thld_enter is the min
reads that a region must have in order to be considered as a
candidate to evict other region.
- eviction_thld_exit - same as above for the exiting region. A region
is consider to be a candidate to be evicted, only if it has less
reads than eviction_thld_exit.
- read_timeout_ms - In order not to hang on to “cold” regions, we
shall inactivate a region that has no READ access for a predefined
amount of time - read_timeout_ms. If read_timeout_ms has expired,
and the region is dirty - it is less likely that we can make any
use of HPB-READing it. So we inactivate it. Still, deactivation
has its overhead, and we may still benefit from HPB-READing this
region if it is clean - see read_timeout_expiries.
- read_timeout_expiries - if the region read timeout has expired, but
the region is clean, just re-wind its timer for another spin. Do
that as long as it is clean and did not exhaust its
read_timeout_expiries threshold.
- timeout_polling_interval_ms - the frequency in which the delayed
worker that checks the read_timeouts is awaken.
Signed-off-by: Avri Altman
---
drivers/scsi/ufs/ufshcd.c | 1 +
drivers/scsi/ufs/ufshpb.c | 284 +++---
drivers/scsi/ufs/ufshpb.h | 22 +++
3 files changed, 290 insertions(+), 17 deletions(-)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 1b521b366067..8dac66783c46 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -8014,6 +8014,7 @@ static const struct attribute_group
*ufshcd_driver_groups[] = {
&ufs_sysfs_lun_attributes_group,
#ifdef CONFIG_SCSI_UFS_HPB
&ufs_sysfs_hpb_stat_group,
+ &ufs_sysfs_hpb_param_group,
#endif
NULL,
};
diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index cec6f641a103..69a742acf0ee 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -351,7 +351,7 @@ void ufshpb_prep(struct ufs_hba *hba, struct ufshcd_lrb
*lrbp)
*/
spin_lock_irqsave(&rgn->rgn_lock, flags);
rgn->reads++;
- if (rgn->reads == ACTIVATION_THRSHLD)
+ if (rgn->reads == hpb->params.activation_thld)
activate = true;
spin_unlock_irqrestore(&rgn->rgn_lock, flags);
if (activate ||
@@ -687,6 +687,7 @@ static void ufshpb_read_to_handler(struct work_struct *work)
struct victim_select_info *lru_info;
struct ufshpb_region *rgn;
unsigned long flags;
+ unsigned int poll;
LIST_HEAD(expired_list);
hpb = container_of(dwork, struct ufshpb_lu, ufshpb_read_to_work);
@@ -713,8 +714,9 @@ static void ufshpb_read_to_handler(struct work_struct *work)
if (dirty || expired)
list_add(&rgn->list_expired_rgn, &expired_list);
else
- rgn->read_timeout = ktime_add_ms(ktime_get(),
-READ_TO_MS);
+ rgn->read_timeout =
+ ktime_add_ms(ktime_get(),
+hpb->params.read_timeout_ms);
}
spin_unlock_irqrestore(&hpb->rgn_state_lock, fl
[PATCH v2 7/9] scsi: ufshpb: Add "Cold" regions timer
In order not to hang on to “cold” regions, we shall inactivate a
region that has no READ access for a predefined amount of time -
READ_TO_MS. For that purpose we shall monitor the active regions list,
polling it on every POLLING_INTERVAL_MS. On timeout expiry we shall add
the region to the "to-be-inactivated" list, unless it is clean and did
not exhaust its READ_TO_EXPIRIES - another parameter.
All this does not apply to pinned regions.
Signed-off-by: Avri Altman
---
drivers/scsi/ufs/ufshpb.c | 76 ++-
drivers/scsi/ufs/ufshpb.h | 6
2 files changed, 81 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index 28e0025507a1..c61fda95e35a 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -18,8 +18,12 @@
#define WORK_PENDING 0
#define RESET_PENDING 1
+#define TIMEOUT_WORK_PENDING 2
#define ACTIVATION_THRSHLD 4 /* 4 IOs */
#define EVICTION_THRSHLD (ACTIVATION_THRSHLD << 6) /* 256 IOs */
+#define READ_TO_MS 1000
+#define READ_TO_EXPIRIES 100
+#define POLLING_INTERVAL_MS 200
/* memory management */
static struct kmem_cache *ufshpb_mctx_cache;
@@ -676,12 +680,69 @@ static int ufshpb_check_srgns_issue_state(struct
ufshpb_lu *hpb,
return 0;
}
+static void ufshpb_read_to_handler(struct work_struct *work)
+{
+ struct delayed_work *dwork = to_delayed_work(work);
+ struct ufshpb_lu *hpb;
+ struct victim_select_info *lru_info;
+ struct ufshpb_region *rgn;
+ unsigned long flags;
+ LIST_HEAD(expired_list);
+
+ hpb = container_of(dwork, struct ufshpb_lu, ufshpb_read_to_work);
+
+ if (test_and_set_bit(TIMEOUT_WORK_PENDING, &hpb->work_data_bits))
+ return;
+
+ spin_lock_irqsave(&hpb->rgn_state_lock, flags);
+
+ lru_info = &hpb->lru_info;
+
+ list_for_each_entry(rgn, &lru_info->lh_lru_rgn, list_lru_rgn) {
+ bool timedout = ktime_after(ktime_get(), rgn->read_timeout);
+ bool dirty, expired = false;
+
+ if (!timedout)
+ continue;
+
+ dirty = is_rgn_dirty(rgn);
+ rgn->read_timeout_expiries--;
+ if (rgn->read_timeout_expiries == 0)
+ expired = true;
+
+ if (dirty || expired)
+ list_add(&rgn->list_expired_rgn, &expired_list);
+ else
+ rgn->read_timeout = ktime_add_ms(ktime_get(),
+READ_TO_MS);
+ }
+
+ spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
+
+ list_for_each_entry(rgn, &expired_list, list_expired_rgn) {
+ list_del_init(&rgn->list_expired_rgn);
+ spin_lock_irqsave(&hpb->rsp_list_lock, flags);
+ ufshpb_update_inactive_info(hpb, rgn->rgn_idx);
+ hpb->stats.rb_inactive_cnt++;
+ spin_unlock_irqrestore(&hpb->rsp_list_lock, flags);
+ }
+
+ clear_bit(TIMEOUT_WORK_PENDING, &hpb->work_data_bits);
+
+ schedule_delayed_work(&hpb->ufshpb_read_to_work,
+ msecs_to_jiffies(POLLING_INTERVAL_MS));
+}
+
static void ufshpb_add_lru_info(struct victim_select_info *lru_info,
- struct ufshpb_region *rgn)
+ struct ufshpb_region *rgn)
{
rgn->rgn_state = HPB_RGN_ACTIVE;
list_add_tail(&rgn->list_lru_rgn, &lru_info->lh_lru_rgn);
atomic_inc(&lru_info->active_cnt);
+ if (rgn->hpb->is_hcm) {
+ rgn->read_timeout = ktime_add_ms(ktime_get(), READ_TO_MS);
+ rgn->read_timeout_expiries = READ_TO_EXPIRIES;
+ }
}
static void ufshpb_hit_lru_info(struct victim_select_info *lru_info,
@@ -1416,6 +1477,7 @@ static int ufshpb_alloc_region_tbl(struct ufs_hba *hba,
struct ufshpb_lu *hpb)
INIT_LIST_HEAD(&rgn->list_inact_rgn);
INIT_LIST_HEAD(&rgn->list_lru_rgn);
+ INIT_LIST_HEAD(&rgn->list_expired_rgn);
if (rgn_idx == hpb->rgns_per_lu - 1)
srgn_cnt = ((hpb->srgns_per_lu - 1) %
@@ -1435,6 +1497,7 @@ static int ufshpb_alloc_region_tbl(struct ufs_hba *hba,
struct ufshpb_lu *hpb)
}
rgn->rgn_flags = 0;
+ rgn->hpb = hpb;
}
return 0;
@@ -1550,6 +1613,8 @@ static int ufshpb_lu_hpb_init(struct ufs_hba *hba, struct
ufshpb_lu *hpb)
ufshpb_normalization_work_handler);
INIT_WORK(&hpb->ufshpb_lun_reset_work,
ufshpb_reset_work_handler);
+ INIT_DELAYED_WORK(&hpb->ufshpb_read_to_work,
+
[PATCH v2 8/9] scsi: ufshpb: Add support for host control mode
Support devices that report they are using host control mode.
Signed-off-by: Avri Altman
---
drivers/scsi/ufs/ufshpb.c | 6 --
1 file changed, 6 deletions(-)
diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index c61fda95e35a..cec6f641a103 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -2050,12 +2050,6 @@ void ufshpb_get_dev_info(struct ufs_hba *hba, u8
*desc_buf)
int version;
hpb_dev_info->control_mode = desc_buf[DEVICE_DESC_PARAM_HPB_CONTROL];
- if (hpb_dev_info->control_mode == HPB_HOST_CONTROL) {
- dev_err(hba->dev, "%s: host control mode is not supported.\n",
- __func__);
- hpb_dev_info->hpb_disabled = true;
- return;
- }
version = get_unaligned_be16(desc_buf + DEVICE_DESC_PARAM_HPB_VER);
if (version != HPB_SUPPORT_VERSION) {
--
2.25.1
RE: [PATCH v2 9/9] scsi: ufshpb: Make host mode parameters configurable
>
> On Tue, Feb 02, 2021 at 10:30:07AM +0200, Avri Altman wrote:
> > +struct attribute_group ufs_sysfs_hpb_param_group = {
> > + .name = "hpb_param_sysfs",
>
> Shouldn't this be "hpb_param"? Why the trailing "_sysfs", doesn't that
> look odd in the directory path?
Done.
RE: [PATCH v2 9/9] scsi: ufshpb: Make host mode parameters configurable
> > > > - timeout_polling_interval_ms - the frequency in which the delayed > > worker that checks the read_timeouts is awaken. > > You create new sysfs files, but fail to document them in > Documentation/ABI/ which is where the above information needs to go :( Done. Will wait to see where Daejun chooses to document the stats entries, and follow.
RE: [PATCH v2 3/9] scsi: ufshpb: Add region's reads counter
> > > > +#define WORK_PENDING 0 > > This should be next to the variable you define that uses this, right? > Otherwise we would think this is a valid value, when in reality it is > the bit number, correct? Done. > > > +#define ACTIVATION_THRSHLD 4 /* 4 IOs */ > > You can spell things out "ACTIVATION_THRESHOLD" :) Done.
RE: [PATCH v2 2/9] scsi: ufshpb: Add host control mode support to rsp_upiu
>
> On Tue, Feb 02, 2021 at 10:30:00AM +0200, Avri Altman wrote:
> > diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
> > index afeb6365daf8..5ec4023db74d 100644
> > --- a/drivers/scsi/ufs/ufshpb.h
> > +++ b/drivers/scsi/ufs/ufshpb.h
> > @@ -48,6 +48,11 @@ enum UFSHPB_MODE {
> > HPB_DEVICE_CONTROL,
> > };
> >
> > +enum HPB_RGN_FLAGS {
> > + RGN_FLAG_UPDATE = 0,
> > + RGN_FLAG_DIRTY,
> > +};
> > +
> > enum UFSHPB_STATE {
> > HPB_PRESENT = 1,
> > HPB_SUSPEND,
> > @@ -109,6 +114,7 @@ struct ufshpb_region {
> >
> > /* below information is used by lru */
> > struct list_head list_lru_rgn;
> > + unsigned long rgn_flags;
>
> Why an unsigned long for a simple enumerated type? And why not make
> this "type safe" by explicitly saying this is an enumerated type
> variable?
I am using it for atomic bit operations.
Thanks,
Avri
RE: [PATCH v2 3/9] scsi: ufshpb: Add region's reads counter
>
>
> On Tue, Feb 02, 2021 at 10:30:01AM +0200, Avri Altman wrote:
> > @@ -175,6 +179,8 @@ struct ufshpb_lu {
> >
> > /* for selecting victim */
> > struct victim_select_info lru_info;
> > + struct work_struct ufshpb_normalization_work;
> > + unsigned long work_data_bits;
>
> You only have 1 "bit" being used here, so perhaps just a u8? Please
> don't use things like "unsigned long" for types, this isn't Windows :)
I am using it for atomic bit operations.
Thanks,
Avri
RE: [PATCH v2 2/9] scsi: ufshpb: Add host control mode support to rsp_upiu
> On Tue, Feb 02, 2021 at 11:24:04AM +0000, Avri Altman wrote:
> >
> > >
> > > On Tue, Feb 02, 2021 at 10:30:00AM +0200, Avri Altman wrote:
> > > > diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
> > > > index afeb6365daf8..5ec4023db74d 100644
> > > > --- a/drivers/scsi/ufs/ufshpb.h
> > > > +++ b/drivers/scsi/ufs/ufshpb.h
> > > > @@ -48,6 +48,11 @@ enum UFSHPB_MODE {
> > > > HPB_DEVICE_CONTROL,
> > > > };
> > > >
> > > > +enum HPB_RGN_FLAGS {
> > > > + RGN_FLAG_UPDATE = 0,
> > > > + RGN_FLAG_DIRTY,
> > > > +};
> > > > +
> > > > enum UFSHPB_STATE {
> > > > HPB_PRESENT = 1,
> > > > HPB_SUSPEND,
> > > > @@ -109,6 +114,7 @@ struct ufshpb_region {
> > > >
> > > > /* below information is used by lru */
> > > > struct list_head list_lru_rgn;
> > > > + unsigned long rgn_flags;
> > >
> > > Why an unsigned long for a simple enumerated type? And why not make
> > > this "type safe" by explicitly saying this is an enumerated type
> > > variable?
> > I am using it for atomic bit operations.
>
> That's not obvious given you have an enumerated type above. Seems like
> an odd mix...
Done.
Will make it clear that those are bit indices.
