| Message ID | 20230330131109.5722-1-avri.altman@wdc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: ufs: mcq: Limit the amount of inflight requests | expand |
On 30.03.23 15:11, Avri Altman wrote: > + if (WARN_ONCE(hba->nutrs * hba->nr_hw_queues > (1 << 16) - 1, > + "there can be at most 1<<16 inflight requests\n")) > + goto err; Hmm why not SZ_64K - 1?
On 3/30/23 06:11, Avri Altman wrote: > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > index 35a3bd95c5e4..d529c42a682a 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -8468,6 +8468,10 @@ static int ufshcd_alloc_mcq(struct ufs_hba *hba) > if (ret) > goto err; > > + if (WARN_ONCE(hba->nutrs * hba->nr_hw_queues > (1 << 16) - 1, > + "there can be at most 1<<16 inflight requests\n")) > + goto err; > + > /* > * Previously allocated memory for nutrs may not be enough in MCQ mode. > * Number of supported tags in MCQ mode may be larger than SDB mode. Hi Avri, WARN*() should only be used to report kernel bugs. hba->nutrs * hba->nr_hw_queues being too large is not a kernel bug but a configuration issue. Instead of failing MCQ allocation, shouldn't ufshcd_mcq_decide_queue_depth() be modified such that it restricts hba->nutrs to a value that can be supported? Thanks, Bart.
> On 30.03.23 15:11, Avri Altman wrote: > > + if (WARN_ONCE(hba->nutrs * hba->nr_hw_queues > (1 << 16) - 1, > > + "there can be at most 1<<16 inflight requests\n")) > > + goto err; > > Hmm why not SZ_64K - 1? But of course. Thanks, Avri
> On 3/30/23 06:11, Avri Altman wrote: > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > > index 35a3bd95c5e4..d529c42a682a 100644 > > --- a/drivers/ufs/core/ufshcd.c > > +++ b/drivers/ufs/core/ufshcd.c > > @@ -8468,6 +8468,10 @@ static int ufshcd_alloc_mcq(struct ufs_hba *hba) > > if (ret) > > goto err; > > > > + if (WARN_ONCE(hba->nutrs * hba->nr_hw_queues > (1 << 16) - 1, > > + "there can be at most 1<<16 inflight requests\n")) > > + goto err; > > + > > /* > > * Previously allocated memory for nutrs may not be enough in MCQ > mode. > > * Number of supported tags in MCQ mode may be larger than SDB > mode. > > Hi Avri, > > WARN*() should only be used to report kernel bugs. hba->nutrs * > hba->nr_hw_queues being too large is not a kernel bug but a > configuration issue. Will replace with an appropriate dev_info and a clarifying comment. > > Instead of failing MCQ allocation, shouldn't > ufshcd_mcq_decide_queue_depth() be modified such that it restricts > hba->nutrs to a value that can be supported? The driver's capacity is the product of hba->nutrs * hba->nr_hw_queues, Where the latter is being established only later in ufshcd_mcq_init. Thanks, Avri > > Thanks, > > Bart.
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index 35a3bd95c5e4..d529c42a682a 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -8468,6 +8468,10 @@ static int ufshcd_alloc_mcq(struct ufs_hba *hba) if (ret) goto err; + if (WARN_ONCE(hba->nutrs * hba->nr_hw_queues > (1 << 16) - 1, + "there can be at most 1<<16 inflight requests\n")) + goto err; + /* * Previously allocated memory for nutrs may not be enough in MCQ mode. * Number of supported tags in MCQ mode may be larger than SDB mode.
in UFS, each request is designated via the triplet <iid, lun, task tag>. In UFS4.0 the Initiator ID field is 8 bits wide, comprised of the EXT_IID and IID fields. Together with the task tag (single byte), they limit the driver's hw queues capacity. Signed-off-by: Avri Altman <avri.altman@wdc.com> --- drivers/ufs/core/ufshcd.c | 4 ++++ 1 file changed, 4 insertions(+)