| Message ID | 20240705085937.1644229-6-huangjunxian6@hisilicon.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RDMA/hns: Bugfixes | expand |
在 2024/7/5 16:59, Junxian Huang 写道: > From: Chengchang Tang <tangchengchang@huawei.com> > > The offset requires 128B alignment and the page size ranges from > 4K to 128M. > > Fixes: 68a997c5d28c ("RDMA/hns: Add FRMR support for hip08") https://patchwork.kernel.org/project/linux-rdma/patch/2eee7e35-504e-4f2a-a364-527e90669108@CMEXHTCAS1.ad.emulex.com/ In the above link, from Bart, it seems that FRMR is renamed to FRWR. " There are already a few drivers upstream in which the fast register memory region work request is abbreviated as FRWR. Please consider renaming FRMR into FRWR in order to avoid confusion and in order to make it easier to find related code with grep in the kernel tree. " So is it possible to rename FRMR to FRWR? > Signed-off-by: Chengchang Tang <tangchengchang@huawei.com> > Signed-off-by: Junxian Huang <huangjunxian6@hisilicon.com> > --- > drivers/infiniband/hw/hns/hns_roce_device.h | 4 ++++ > drivers/infiniband/hw/hns/hns_roce_mr.c | 5 +++++ > 2 files changed, 9 insertions(+) > > diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h b/drivers/infiniband/hw/hns/hns_roce_device.h > index 5a2445f357ab..15b3b978a601 100644 > --- a/drivers/infiniband/hw/hns/hns_roce_device.h > +++ b/drivers/infiniband/hw/hns/hns_roce_device.h > @@ -83,6 +83,7 @@ > #define MR_TYPE_DMA 0x03 > > #define HNS_ROCE_FRMR_MAX_PA 512 > +#define HNS_ROCE_FRMR_ALIGN_SIZE 128 > > #define PKEY_ID 0xffff > #define NODE_DESC_SIZE 64 > @@ -189,6 +190,9 @@ enum { > #define HNS_HW_PAGE_SHIFT 12 > #define HNS_HW_PAGE_SIZE (1 << HNS_HW_PAGE_SHIFT) > > +#define HNS_HW_MAX_PAGE_SHIFT 27 > +#define HNS_HW_MAX_PAGE_SIZE (1 << HNS_HW_MAX_PAGE_SHIFT) > + > struct hns_roce_uar { > u64 pfn; > unsigned long index; > diff --git a/drivers/infiniband/hw/hns/hns_roce_mr.c b/drivers/infiniband/hw/hns/hns_roce_mr.c > index 1a61dceb3319..846da8c78b8b 100644 > --- a/drivers/infiniband/hw/hns/hns_roce_mr.c > +++ b/drivers/infiniband/hw/hns/hns_roce_mr.c > @@ -443,6 +443,11 @@ int hns_roce_map_mr_sg(struct ib_mr *ibmr, struct scatterlist *sg, int sg_nents, > struct hns_roce_mtr *mtr = &mr->pbl_mtr; > int ret, sg_num = 0; > > + if (!IS_ALIGNED(*sg_offset, HNS_ROCE_FRMR_ALIGN_SIZE) || > + ibmr->page_size < HNS_HW_PAGE_SIZE || > + ibmr->page_size > HNS_HW_MAX_PAGE_SIZE) > + return sg_num; > + > mr->npages = 0; > mr->page_list = kvcalloc(mr->pbl_mtr.hem_cfg.buf_pg_count, > sizeof(dma_addr_t), GFP_KERNEL);
On 2024/7/7 17:16, Zhu Yanjun wrote: > 在 2024/7/5 16:59, Junxian Huang 写道: >> From: Chengchang Tang <tangchengchang@huawei.com> >> >> The offset requires 128B alignment and the page size ranges from >> 4K to 128M. >> >> Fixes: 68a997c5d28c ("RDMA/hns: Add FRMR support for hip08") > > https://patchwork.kernel.org/project/linux-rdma/patch/2eee7e35-504e-4f2a-a364-527e90669108@CMEXHTCAS1.ad.emulex.com/ > In the above link, from Bart, it seems that FRMR is renamed to FRWR. > " > There are already a few drivers upstream in which the fast register > memory region work request is abbreviated as FRWR. Please consider > renaming FRMR into FRWR in order to avoid confusion and in order to > make it easier to find related code with grep in the kernel tree. > " > > So is it possible to rename FRMR to FRWR? > I think the rename is irrelevant to this bugfix, and if it needs to be done, we'll need a single patch to rename all existing 'FRMR' in hns driver. So let's leave it as is for now. Thanks, Junxian >> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com> >> Signed-off-by: Junxian Huang <huangjunxian6@hisilicon.com> >> --- >> drivers/infiniband/hw/hns/hns_roce_device.h | 4 ++++ >> drivers/infiniband/hw/hns/hns_roce_mr.c | 5 +++++ >> 2 files changed, 9 insertions(+) >> >> diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h b/drivers/infiniband/hw/hns/hns_roce_device.h >> index 5a2445f357ab..15b3b978a601 100644 >> --- a/drivers/infiniband/hw/hns/hns_roce_device.h >> +++ b/drivers/infiniband/hw/hns/hns_roce_device.h >> @@ -83,6 +83,7 @@ >> #define MR_TYPE_DMA 0x03 >> #define HNS_ROCE_FRMR_MAX_PA 512 >> +#define HNS_ROCE_FRMR_ALIGN_SIZE 128 >> #define PKEY_ID 0xffff >> #define NODE_DESC_SIZE 64 >> @@ -189,6 +190,9 @@ enum { >> #define HNS_HW_PAGE_SHIFT 12 >> #define HNS_HW_PAGE_SIZE (1 << HNS_HW_PAGE_SHIFT) >> +#define HNS_HW_MAX_PAGE_SHIFT 27 >> +#define HNS_HW_MAX_PAGE_SIZE (1 << HNS_HW_MAX_PAGE_SHIFT) >> + >> struct hns_roce_uar { >> u64 pfn; >> unsigned long index; >> diff --git a/drivers/infiniband/hw/hns/hns_roce_mr.c b/drivers/infiniband/hw/hns/hns_roce_mr.c >> index 1a61dceb3319..846da8c78b8b 100644 >> --- a/drivers/infiniband/hw/hns/hns_roce_mr.c >> +++ b/drivers/infiniband/hw/hns/hns_roce_mr.c >> @@ -443,6 +443,11 @@ int hns_roce_map_mr_sg(struct ib_mr *ibmr, struct scatterlist *sg, int sg_nents, >> struct hns_roce_mtr *mtr = &mr->pbl_mtr; >> int ret, sg_num = 0; >> + if (!IS_ALIGNED(*sg_offset, HNS_ROCE_FRMR_ALIGN_SIZE) || >> + ibmr->page_size < HNS_HW_PAGE_SIZE || >> + ibmr->page_size > HNS_HW_MAX_PAGE_SIZE) >> + return sg_num; >> + >> mr->npages = 0; >> mr->page_list = kvcalloc(mr->pbl_mtr.hem_cfg.buf_pg_count, >> sizeof(dma_addr_t), GFP_KERNEL); >
On Mon, Jul 08, 2024 at 10:44:52AM +0800, Junxian Huang wrote: > > > On 2024/7/7 17:16, Zhu Yanjun wrote: > > 在 2024/7/5 16:59, Junxian Huang 写道: > >> From: Chengchang Tang <tangchengchang@huawei.com> > >> > >> The offset requires 128B alignment and the page size ranges from > >> 4K to 128M. > >> > >> Fixes: 68a997c5d28c ("RDMA/hns: Add FRMR support for hip08") > > > > https://patchwork.kernel.org/project/linux-rdma/patch/2eee7e35-504e-4f2a-a364-527e90669108@CMEXHTCAS1.ad.emulex.com/ > > In the above link, from Bart, it seems that FRMR is renamed to FRWR. > > " > > There are already a few drivers upstream in which the fast register > > memory region work request is abbreviated as FRWR. Please consider > > renaming FRMR into FRWR in order to avoid confusion and in order to > > make it easier to find related code with grep in the kernel tree. > > " > > > > So is it possible to rename FRMR to FRWR? > > > > I think the rename is irrelevant to this bugfix, and if it needs to be done, > we'll need a single patch to rename all existing 'FRMR' in hns driver. > > So let's leave it as is for now. +1 > > Thanks, > Junxian > > >> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com> > >> Signed-off-by: Junxian Huang <huangjunxian6@hisilicon.com> > >> --- > >> drivers/infiniband/hw/hns/hns_roce_device.h | 4 ++++ > >> drivers/infiniband/hw/hns/hns_roce_mr.c | 5 +++++ > >> 2 files changed, 9 insertions(+) > >> > >> diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h b/drivers/infiniband/hw/hns/hns_roce_device.h > >> index 5a2445f357ab..15b3b978a601 100644 > >> --- a/drivers/infiniband/hw/hns/hns_roce_device.h > >> +++ b/drivers/infiniband/hw/hns/hns_roce_device.h > >> @@ -83,6 +83,7 @@ > >> #define MR_TYPE_DMA 0x03 > >> #define HNS_ROCE_FRMR_MAX_PA 512 > >> +#define HNS_ROCE_FRMR_ALIGN_SIZE 128 > >> #define PKEY_ID 0xffff > >> #define NODE_DESC_SIZE 64 > >> @@ -189,6 +190,9 @@ enum { > >> #define HNS_HW_PAGE_SHIFT 12 > >> #define HNS_HW_PAGE_SIZE (1 << HNS_HW_PAGE_SHIFT) > >> +#define HNS_HW_MAX_PAGE_SHIFT 27 > >> +#define HNS_HW_MAX_PAGE_SIZE (1 << HNS_HW_MAX_PAGE_SHIFT) > >> + > >> struct hns_roce_uar { > >> u64 pfn; > >> unsigned long index; > >> diff --git a/drivers/infiniband/hw/hns/hns_roce_mr.c b/drivers/infiniband/hw/hns/hns_roce_mr.c > >> index 1a61dceb3319..846da8c78b8b 100644 > >> --- a/drivers/infiniband/hw/hns/hns_roce_mr.c > >> +++ b/drivers/infiniband/hw/hns/hns_roce_mr.c > >> @@ -443,6 +443,11 @@ int hns_roce_map_mr_sg(struct ib_mr *ibmr, struct scatterlist *sg, int sg_nents, > >> struct hns_roce_mtr *mtr = &mr->pbl_mtr; > >> int ret, sg_num = 0; > >> + if (!IS_ALIGNED(*sg_offset, HNS_ROCE_FRMR_ALIGN_SIZE) || > >> + ibmr->page_size < HNS_HW_PAGE_SIZE || > >> + ibmr->page_size > HNS_HW_MAX_PAGE_SIZE) > >> + return sg_num; > >> + > >> mr->npages = 0; > >> mr->page_list = kvcalloc(mr->pbl_mtr.hem_cfg.buf_pg_count, > >> sizeof(dma_addr_t), GFP_KERNEL); > >
在 2024/7/8 10:44, Junxian Huang 写道: > > > On 2024/7/7 17:16, Zhu Yanjun wrote: >> 在 2024/7/5 16:59, Junxian Huang 写道: >>> From: Chengchang Tang <tangchengchang@huawei.com> >>> >>> The offset requires 128B alignment and the page size ranges from >>> 4K to 128M. >>> >>> Fixes: 68a997c5d28c ("RDMA/hns: Add FRMR support for hip08") >> >> https://patchwork.kernel.org/project/linux-rdma/patch/2eee7e35-504e-4f2a-a364-527e90669108@CMEXHTCAS1.ad.emulex.com/ >> In the above link, from Bart, it seems that FRMR is renamed to FRWR. >> " >> There are already a few drivers upstream in which the fast register >> memory region work request is abbreviated as FRWR. Please consider >> renaming FRMR into FRWR in order to avoid confusion and in order to >> make it easier to find related code with grep in the kernel tree. >> " >> >> So is it possible to rename FRMR to FRWR? >> > > I think the rename is irrelevant to this bugfix, and if it needs to be done, > we'll need a single patch to rename all existing 'FRMR' in hns driver. > > So let's leave it as is for now. Exactly. FRMR is obsolete. We do need a single patch to rename all existing "FRMR" to "FRWR" in RDMA. @Leon, please let me know your suggestions. Thanks, Zhu Yanjun > > Thanks, > Junxian > >>> Signed-off-by: Chengchang Tang <tangchengchang@huawei.com> >>> Signed-off-by: Junxian Huang <huangjunxian6@hisilicon.com> >>> --- >>> drivers/infiniband/hw/hns/hns_roce_device.h | 4 ++++ >>> drivers/infiniband/hw/hns/hns_roce_mr.c | 5 +++++ >>> 2 files changed, 9 insertions(+) >>> >>> diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h b/drivers/infiniband/hw/hns/hns_roce_device.h >>> index 5a2445f357ab..15b3b978a601 100644 >>> --- a/drivers/infiniband/hw/hns/hns_roce_device.h >>> +++ b/drivers/infiniband/hw/hns/hns_roce_device.h >>> @@ -83,6 +83,7 @@ >>> #define MR_TYPE_DMA 0x03 >>> #define HNS_ROCE_FRMR_MAX_PA 512 >>> +#define HNS_ROCE_FRMR_ALIGN_SIZE 128 >>> #define PKEY_ID 0xffff >>> #define NODE_DESC_SIZE 64 >>> @@ -189,6 +190,9 @@ enum { >>> #define HNS_HW_PAGE_SHIFT 12 >>> #define HNS_HW_PAGE_SIZE (1 << HNS_HW_PAGE_SHIFT) >>> +#define HNS_HW_MAX_PAGE_SHIFT 27 >>> +#define HNS_HW_MAX_PAGE_SIZE (1 << HNS_HW_MAX_PAGE_SHIFT) >>> + >>> struct hns_roce_uar { >>> u64 pfn; >>> unsigned long index; >>> diff --git a/drivers/infiniband/hw/hns/hns_roce_mr.c b/drivers/infiniband/hw/hns/hns_roce_mr.c >>> index 1a61dceb3319..846da8c78b8b 100644 >>> --- a/drivers/infiniband/hw/hns/hns_roce_mr.c >>> +++ b/drivers/infiniband/hw/hns/hns_roce_mr.c >>> @@ -443,6 +443,11 @@ int hns_roce_map_mr_sg(struct ib_mr *ibmr, struct scatterlist *sg, int sg_nents, >>> struct hns_roce_mtr *mtr = &mr->pbl_mtr; >>> int ret, sg_num = 0; >>> + if (!IS_ALIGNED(*sg_offset, HNS_ROCE_FRMR_ALIGN_SIZE) || >>> + ibmr->page_size < HNS_HW_PAGE_SIZE || >>> + ibmr->page_size > HNS_HW_MAX_PAGE_SIZE) >>> + return sg_num; >>> + >>> mr->npages = 0; >>> mr->page_list = kvcalloc(mr->pbl_mtr.hem_cfg.buf_pg_count, >>> sizeof(dma_addr_t), GFP_KERNEL); >>
On Mon, Jul 08, 2024 at 03:57:49PM +0800, Zhu Yanjun wrote: > 在 2024/7/8 10:44, Junxian Huang 写道: > > > > > > On 2024/7/7 17:16, Zhu Yanjun wrote: > > > 在 2024/7/5 16:59, Junxian Huang 写道: > > > > From: Chengchang Tang <tangchengchang@huawei.com> > > > > > > > > The offset requires 128B alignment and the page size ranges from > > > > 4K to 128M. > > > > > > > > Fixes: 68a997c5d28c ("RDMA/hns: Add FRMR support for hip08") > > > > > > https://patchwork.kernel.org/project/linux-rdma/patch/2eee7e35-504e-4f2a-a364-527e90669108@CMEXHTCAS1.ad.emulex.com/ > > > In the above link, from Bart, it seems that FRMR is renamed to FRWR. > > > " > > > There are already a few drivers upstream in which the fast register > > > memory region work request is abbreviated as FRWR. Please consider > > > renaming FRMR into FRWR in order to avoid confusion and in order to > > > make it easier to find related code with grep in the kernel tree. > > > " > > > > > > So is it possible to rename FRMR to FRWR? > > > > > > > I think the rename is irrelevant to this bugfix, and if it needs to be done, > > we'll need a single patch to rename all existing 'FRMR' in hns driver. > > > > So let's leave it as is for now. > > Exactly. FRMR is obsolete. We do need a single patch to rename all existing > "FRMR" to "FRWR" in RDMA. > > @Leon, please let me know your suggestions. Our preference is to avoid from churn patches which are not part of some larger series. In this case, rename won't give us any benefit, but will create a lot of noise for the backports. There is no need to be cruel to the people who are doing these backports just because "some word was removed from the dictionary". Thanks > Thanks, > > Zhu Yanjun > > > > > Thanks, > > Junxian > > > > > > Signed-off-by: Chengchang Tang <tangchengchang@huawei.com> > > > > Signed-off-by: Junxian Huang <huangjunxian6@hisilicon.com> > > > > --- > > > > drivers/infiniband/hw/hns/hns_roce_device.h | 4 ++++ > > > > drivers/infiniband/hw/hns/hns_roce_mr.c | 5 +++++ > > > > 2 files changed, 9 insertions(+) > > > > > > > > diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h b/drivers/infiniband/hw/hns/hns_roce_device.h > > > > index 5a2445f357ab..15b3b978a601 100644 > > > > --- a/drivers/infiniband/hw/hns/hns_roce_device.h > > > > +++ b/drivers/infiniband/hw/hns/hns_roce_device.h > > > > @@ -83,6 +83,7 @@ > > > > #define MR_TYPE_DMA 0x03 > > > > #define HNS_ROCE_FRMR_MAX_PA 512 > > > > +#define HNS_ROCE_FRMR_ALIGN_SIZE 128 > > > > #define PKEY_ID 0xffff > > > > #define NODE_DESC_SIZE 64 > > > > @@ -189,6 +190,9 @@ enum { > > > > #define HNS_HW_PAGE_SHIFT 12 > > > > #define HNS_HW_PAGE_SIZE (1 << HNS_HW_PAGE_SHIFT) > > > > +#define HNS_HW_MAX_PAGE_SHIFT 27 > > > > +#define HNS_HW_MAX_PAGE_SIZE (1 << HNS_HW_MAX_PAGE_SHIFT) > > > > + > > > > struct hns_roce_uar { > > > > u64 pfn; > > > > unsigned long index; > > > > diff --git a/drivers/infiniband/hw/hns/hns_roce_mr.c b/drivers/infiniband/hw/hns/hns_roce_mr.c > > > > index 1a61dceb3319..846da8c78b8b 100644 > > > > --- a/drivers/infiniband/hw/hns/hns_roce_mr.c > > > > +++ b/drivers/infiniband/hw/hns/hns_roce_mr.c > > > > @@ -443,6 +443,11 @@ int hns_roce_map_mr_sg(struct ib_mr *ibmr, struct scatterlist *sg, int sg_nents, > > > > struct hns_roce_mtr *mtr = &mr->pbl_mtr; > > > > int ret, sg_num = 0; > > > > + if (!IS_ALIGNED(*sg_offset, HNS_ROCE_FRMR_ALIGN_SIZE) || > > > > + ibmr->page_size < HNS_HW_PAGE_SIZE || > > > > + ibmr->page_size > HNS_HW_MAX_PAGE_SIZE) > > > > + return sg_num; > > > > + > > > > mr->npages = 0; > > > > mr->page_list = kvcalloc(mr->pbl_mtr.hem_cfg.buf_pg_count, > > > > sizeof(dma_addr_t), GFP_KERNEL); > > > >
diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h b/drivers/infiniband/hw/hns/hns_roce_device.h index 5a2445f357ab..15b3b978a601 100644 --- a/drivers/infiniband/hw/hns/hns_roce_device.h +++ b/drivers/infiniband/hw/hns/hns_roce_device.h @@ -83,6 +83,7 @@ #define MR_TYPE_DMA 0x03 #define HNS_ROCE_FRMR_MAX_PA 512 +#define HNS_ROCE_FRMR_ALIGN_SIZE 128 #define PKEY_ID 0xffff #define NODE_DESC_SIZE 64 @@ -189,6 +190,9 @@ enum { #define HNS_HW_PAGE_SHIFT 12 #define HNS_HW_PAGE_SIZE (1 << HNS_HW_PAGE_SHIFT) +#define HNS_HW_MAX_PAGE_SHIFT 27 +#define HNS_HW_MAX_PAGE_SIZE (1 << HNS_HW_MAX_PAGE_SHIFT) + struct hns_roce_uar { u64 pfn; unsigned long index; diff --git a/drivers/infiniband/hw/hns/hns_roce_mr.c b/drivers/infiniband/hw/hns/hns_roce_mr.c index 1a61dceb3319..846da8c78b8b 100644 --- a/drivers/infiniband/hw/hns/hns_roce_mr.c +++ b/drivers/infiniband/hw/hns/hns_roce_mr.c @@ -443,6 +443,11 @@ int hns_roce_map_mr_sg(struct ib_mr *ibmr, struct scatterlist *sg, int sg_nents, struct hns_roce_mtr *mtr = &mr->pbl_mtr; int ret, sg_num = 0; + if (!IS_ALIGNED(*sg_offset, HNS_ROCE_FRMR_ALIGN_SIZE) || + ibmr->page_size < HNS_HW_PAGE_SIZE || + ibmr->page_size > HNS_HW_MAX_PAGE_SIZE) + return sg_num; + mr->npages = 0; mr->page_list = kvcalloc(mr->pbl_mtr.hem_cfg.buf_pg_count, sizeof(dma_addr_t), GFP_KERNEL);