linux-kernel - RE: [PATCH v3] scsi: ufs: mcq: Limit the amount of inflight requests

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <DM6PR04MB6575720218D06C3EF9721BF2FC639@DM6PR04MB6575.namprd04.prod.outlook.com>
Date:   Thu, 20 Apr 2023 10:23:40 +0000
From:   Avri Altman <Avri.Altman@....com>
To:     John Garry <john.g.garry@...cle.com>,
        "James E . J . Bottomley" <jejb@...ux.vnet.ibm.com>,
        "Martin K . Petersen" <martin.petersen@...cle.com>
CC:     Asutosh Das <quic_asutoshd@...cinc.com>,
        "quic_cang@...cinc.com" <quic_cang@...cinc.com>,
        "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Johannes Thumshirn <Johannes.Thumshirn@....com>,
        Bart Van Assche <bvanassche@....org>
Subject: RE: [PATCH v3] scsi: ufs: mcq: Limit the amount of inflight requests

> On 20/04/2023 11:11, Avri Altman wrote:
> > 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.
> >
> > The purpose of this patch is to document the ufs spec restrictions.
> > practically, it impose no functional change because a) the current hw
> > supports much less (merely 64 requests), and b) since
> > shost->host_tagset is set - then the number of inflight IOs is limited
> > by just the HW queue depth.
> >
> 
> I don't know why effectively it has no change, as it seems to me to be checking
> and failing something valid which could happen.
> 
> If it really cannot happen, then we generally don't add what is effectively
> dead code just to provide practical illustrations of the spec. And it is
> misleading to others who may use your driver as a reference.
Better to just drop it then.

Thanks,
Avri

> 
> A code comment would be a better choice, if anything, IMHO.
> 
> Thanks,
> John
> 
> > ---
> > v2 -> v3:
> > fix smatch warnings (Dan Carpenter)
> > elaborate commit log (John Garry)
> >
> > v1 -> v2:
> > Attend Johannes's and Bart's comments
> 
> This is the wrong place for these
> 
> >
> > Reviewed-by: Johannes Thumshirn <johannes.thumshirn@....com>
> > Signed-off-by: Avri Altman <avri.altman@....com>
> > ---
> >   drivers/ufs/core/ufshcd.c | 6 ++++++
> >   1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> > index 03c47f9a2750..9cebec6be35c 100644
> > --- a/drivers/ufs/core/ufshcd.c
> > +++ b/drivers/ufs/core/ufshcd.c
> > @@ -8468,6 +8468,12 @@ static int ufshcd_alloc_mcq(struct ufs_hba *hba)
> >       if (ret)
> >               goto err;
> >
> > +     if (hba->nutrs * hba->nr_hw_queues > SZ_64K - 1) {
> > +             dev_info(hba->dev, "there can be at most 64K inflight requests\n");
> > +             ret = -EINVAL;
> > +             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.