| Message ID | 2764b6e204b32ef8c198a5efaf6c6bc4119f7665.1657301795.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
| Series | [1/2] RDMA/erdma: Use the bitmap API to allocate bitmaps | expand |
On 7/9/22 1:37 AM, Christophe JAILLET wrote: > Use [devm_]bitmap_zalloc()/bitmap_free() instead of hand-writing them. > > It is less verbose and it improves the semantic. > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > drivers/infiniband/hw/erdma/erdma_cmdq.c | 7 +++---- > drivers/infiniband/hw/erdma/erdma_main.c | 9 ++++----- > 2 files changed, 7 insertions(+), 9 deletions(-) > Hi Christophe, Thanks for your two patches of erdma. The erdma code your got is our first upstreaming code, so I would like to squash your changes into the relevant commit in our next patchset to make the commit history cleaner. BTW, the coding style in the patches is OK, but has a little differences with clang-format's result. I will use the format from clang-format to minimize manual adjustments. Thanks, Cheng Xu > diff --git a/drivers/infiniband/hw/erdma/erdma_cmdq.c b/drivers/infiniband/hw/erdma/erdma_cmdq.c > index 0cf5032d4b78..0489838d9717 100644 > --- a/drivers/infiniband/hw/erdma/erdma_cmdq.c > +++ b/drivers/infiniband/hw/erdma/erdma_cmdq.c > @@ -78,10 +78,9 @@ static int erdma_cmdq_wait_res_init(struct erdma_dev *dev, > return -ENOMEM; > > spin_lock_init(&cmdq->lock); > - cmdq->comp_wait_bitmap = > - devm_kcalloc(&dev->pdev->dev, > - BITS_TO_LONGS(cmdq->max_outstandings), > - sizeof(unsigned long), GFP_KERNEL); > + cmdq->comp_wait_bitmap = devm_bitmap_zalloc(&dev->pdev->dev, > + cmdq->max_outstandings, > + GFP_KERNEL); > if (!cmdq->comp_wait_bitmap) { > devm_kfree(&dev->pdev->dev, cmdq->wait_pool); > return -ENOMEM; > diff --git a/drivers/infiniband/hw/erdma/erdma_main.c b/drivers/infiniband/hw/erdma/erdma_main.c > index 27484bea51d9..7e1e27acb404 100644 > --- a/drivers/infiniband/hw/erdma/erdma_main.c > +++ b/drivers/infiniband/hw/erdma/erdma_main.c > @@ -423,9 +423,8 @@ static int erdma_res_cb_init(struct erdma_dev *dev) > for (i = 0; i < ERDMA_RES_CNT; i++) { > dev->res_cb[i].next_alloc_idx = 1; > spin_lock_init(&dev->res_cb[i].lock); > - dev->res_cb[i].bitmap = > - kcalloc(BITS_TO_LONGS(dev->res_cb[i].max_cap), > - sizeof(unsigned long), GFP_KERNEL); > + dev->res_cb[i].bitmap = bitmap_zalloc(dev->res_cb[i].max_cap, > + GFP_KERNEL); > /* We will free the memory in erdma_res_cb_free */ > if (!dev->res_cb[i].bitmap) > goto err; > @@ -435,7 +434,7 @@ static int erdma_res_cb_init(struct erdma_dev *dev) > > err: > for (j = 0; j < i; j++) > - kfree(dev->res_cb[j].bitmap); > + bitmap_free(dev->res_cb[j].bitmap); > > return -ENOMEM; > } > @@ -445,7 +444,7 @@ static void erdma_res_cb_free(struct erdma_dev *dev) > int i; > > for (i = 0; i < ERDMA_RES_CNT; i++) > - kfree(dev->res_cb[i].bitmap); > + bitmap_free(dev->res_cb[i].bitmap); > } > > static const struct ib_device_ops erdma_device_ops = {
On Mon, Jul 11, 2022 at 03:34:56PM +0800, Cheng Xu wrote: > > > On 7/9/22 1:37 AM, Christophe JAILLET wrote: > > Use [devm_]bitmap_zalloc()/bitmap_free() instead of hand-writing them. > > > > It is less verbose and it improves the semantic. > > > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > > --- > > drivers/infiniband/hw/erdma/erdma_cmdq.c | 7 +++---- > > drivers/infiniband/hw/erdma/erdma_main.c | 9 ++++----- > > 2 files changed, 7 insertions(+), 9 deletions(-) > > > > Hi Christophe, > > Thanks for your two patches of erdma. > > The erdma code your got is our first upstreaming code, so I would like to squash your > changes into the relevant commit in our next patchset to make the commit history cleaner. > > BTW, the coding style in the patches is OK, but has a little differences with clang-format's > result. I will use the format from clang-format to minimize manual adjustments. > Best not to use any auto-formatting tools. They are all bad. regards, dan carpenter
On 7/12/22 5:01 PM, Dan Carpenter wrote: > On Mon, Jul 11, 2022 at 03:34:56PM +0800, Cheng Xu wrote: >> >> >> On 7/9/22 1:37 AM, Christophe JAILLET wrote: >>> Use [devm_]bitmap_zalloc()/bitmap_free() instead of hand-writing them. >>> >>> It is less verbose and it improves the semantic. >>> >>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> >>> --- >>> drivers/infiniband/hw/erdma/erdma_cmdq.c | 7 +++---- >>> drivers/infiniband/hw/erdma/erdma_main.c | 9 ++++----- >>> 2 files changed, 7 insertions(+), 9 deletions(-) >>> >> >> Hi Christophe, >> >> Thanks for your two patches of erdma. >> >> The erdma code your got is our first upstreaming code, so I would like to squash your >> changes into the relevant commit in our next patchset to make the commit history cleaner. >> >> BTW, the coding style in the patches is OK, but has a little differences with clang-format's >> result. I will use the format from clang-format to minimize manual adjustments. >> > > Best not to use any auto-formatting tools. They are all bad. > I understand your worry. Tool is not prefect but it's useful to handle large amounts of code in our first upstream, and helps us avoiding style mistakes. While using the clang-format with the config in kernel tree, we also checked all the modifications made by the tool carefully. We won't rely on tools too much with small changes in the future. Thanks, Cheng Xu
On Tue, Jul 12, 2022 at 12:01:10PM +0300, Dan Carpenter wrote:
> Best not to use any auto-formatting tools. They are all bad.
Have you tried clang-format? I wouldn't call it bad..
Jason
On Tue, Jul 19, 2022 at 09:54:34AM -0300, Jason Gunthorpe wrote: > On Tue, Jul 12, 2022 at 12:01:10PM +0300, Dan Carpenter wrote: > > > Best not to use any auto-formatting tools. They are all bad. > > Have you tried clang-format? I wouldn't call it bad.. I prefered Christophe's formatting to clang's. ;) regards, dan carpenter
Le 19/07/2022 à 15:01, Dan Carpenter a écrit : > On Tue, Jul 19, 2022 at 09:54:34AM -0300, Jason Gunthorpe wrote: >> On Tue, Jul 12, 2022 at 12:01:10PM +0300, Dan Carpenter wrote: >> >>> Best not to use any auto-formatting tools. They are all bad. >> >> Have you tried clang-format? I wouldn't call it bad.. > > I prefered Christophe's formatting to clang's. ;) > > regards, > dan carpenter > > Hi, checkpatch.pl only gives hints and should not blindly be taken as THE reference, but: ./scripts/checkpatch.pl -f --strict drivers/infiniband/hw/erdma/erdma_cmdq.c gives: CHECK: Lines should not end with a '(' #81: FILE: drivers/infiniband/hw/erdma/erdma_cmdq.c:81: + cmdq->comp_wait_bitmap = devm_bitmap_zalloc( total: 0 errors, 0 warnings, 1 checks, 493 lines checked (some other files in the same directory also have some checkpatch warning/error) Don't know if it may be an issue for maintainers. CJ
On Tue, Jul 19, 2022 at 05:36:36PM +0200, Christophe JAILLET wrote: > Le 19/07/2022 à 15:01, Dan Carpenter a écrit : > > On Tue, Jul 19, 2022 at 09:54:34AM -0300, Jason Gunthorpe wrote: > > > On Tue, Jul 12, 2022 at 12:01:10PM +0300, Dan Carpenter wrote: > > > > > > > Best not to use any auto-formatting tools. They are all bad. > > > > > > Have you tried clang-format? I wouldn't call it bad.. > > > > I prefered Christophe's formatting to clang's. ;) > > checkpatch.pl only gives hints and should not blindly be taken as THE > reference, but: Oh, I think alot of people don't agree with that one, I know I don't. Jason
On 7/19/22 11:36 PM, Christophe JAILLET wrote: > Le 19/07/2022 à 15:01, Dan Carpenter a écrit : >> On Tue, Jul 19, 2022 at 09:54:34AM -0300, Jason Gunthorpe wrote: >>> On Tue, Jul 12, 2022 at 12:01:10PM +0300, Dan Carpenter wrote: >>> >>>> Best not to use any auto-formatting tools. They are all bad. >>> >>> Have you tried clang-format? I wouldn't call it bad.. >> >> I prefered Christophe's formatting to clang's. ;) >> >> regards, >> dan carpenter >> >> > > Hi, > > (some other files in the same directory also have some checkpatch warning/error) I just double checked the checkpatch results, Two type warnings reported: - WARNING: Missing commit description - Add an appropriate one (for patch 0001) - WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? (for almost all patches except 0001/0011) For the first warning, the change is very simple: add erdma's rdma_driver_id definition, I think the commit title can describe all things, and is enough. For the second warning, I think it is OK for new files before MAINTAINERS being updated in patch 0011. Thanks, Cheng Xu
On Tue, Jul 19, 2022 at 05:36:36PM +0200, Christophe JAILLET wrote: > Le 19/07/2022 à 15:01, Dan Carpenter a écrit : > > On Tue, Jul 19, 2022 at 09:54:34AM -0300, Jason Gunthorpe wrote: > > > On Tue, Jul 12, 2022 at 12:01:10PM +0300, Dan Carpenter wrote: > > > > > > > Best not to use any auto-formatting tools. They are all bad. > > > > > > Have you tried clang-format? I wouldn't call it bad.. > > > > I prefered Christophe's formatting to clang's. ;) > > > > regards, > > dan carpenter > > > > > > Hi, > > checkpatch.pl only gives hints and should not blindly be taken as THE > reference, but: > > ./scripts/checkpatch.pl -f --strict > drivers/infiniband/hw/erdma/erdma_cmdq.c > > gives: > CHECK: Lines should not end with a '(' > #81: FILE: drivers/infiniband/hw/erdma/erdma_cmdq.c:81: > + cmdq->comp_wait_bitmap = devm_bitmap_zalloc( > > total: 0 errors, 0 warnings, 1 checks, 493 lines checked > > (some other files in the same directory also have some checkpatch > warning/error) > > > > Don't know if it may be an issue for maintainers. We don't run with --strict. It is indeed very subjective. Thanks > > CJ
On Wed, Jul 20, 2022 at 09:58:24AM +0800, Cheng Xu wrote: > > > On 7/19/22 11:36 PM, Christophe JAILLET wrote: > > Le 19/07/2022 à 15:01, Dan Carpenter a écrit : > >> On Tue, Jul 19, 2022 at 09:54:34AM -0300, Jason Gunthorpe wrote: > >>> On Tue, Jul 12, 2022 at 12:01:10PM +0300, Dan Carpenter wrote: > >>> > >>>> Best not to use any auto-formatting tools. They are all bad. > >>> > >>> Have you tried clang-format? I wouldn't call it bad.. > >> > >> I prefered Christophe's formatting to clang's. ;) > >> > >> regards, > >> dan carpenter > >> > >> > > > > Hi, > > > > (some other files in the same directory also have some checkpatch warning/error) > > I just double checked the checkpatch results, Two type warnings reported: > > - WARNING: Missing commit description - Add an appropriate one (for patch 0001) > - WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? (for almost all patches except 0001/0011) > > For the first warning, the change is very simple: add erdma's > rdma_driver_id definition, I think the commit title can describe > all things, and is enough. To be clear, our preference is to have commit message in any case, even for simple changes. Thanks
On 7/21/22 3:31 PM, Leon Romanovsky wrote: > On Wed, Jul 20, 2022 at 09:58:24AM +0800, Cheng Xu wrote: >> >> >> On 7/19/22 11:36 PM, Christophe JAILLET wrote: >>> Le 19/07/2022 à 15:01, Dan Carpenter a écrit : >>>> On Tue, Jul 19, 2022 at 09:54:34AM -0300, Jason Gunthorpe wrote: >>>>> On Tue, Jul 12, 2022 at 12:01:10PM +0300, Dan Carpenter wrote: >>>>> >>>>>> Best not to use any auto-formatting tools. They are all bad. >>>>> >>>>> Have you tried clang-format? I wouldn't call it bad.. >>>> >>>> I prefered Christophe's formatting to clang's. ;) >>>> >>>> regards, >>>> dan carpenter >>>> >>>> >>> >>> Hi, >>> >>> (some other files in the same directory also have some checkpatch warning/error) >> >> I just double checked the checkpatch results, Two type warnings reported: >> >> - WARNING: Missing commit description - Add an appropriate one (for patch 0001) >> - WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? (for almost all patches except 0001/0011) >> >> For the first warning, the change is very simple: add erdma's >> rdma_driver_id definition, I think the commit title can describe >> all things, and is enough. > > To be clear, our preference is to have commit message in any case, even > for simple changes. > Sorry for this, I didn't know it previously. Before I sent our patches, I reviewed the EFA/SIW's upstreaming history, and siw only has one line commit title for simply changes, I followed. I will update our patches to fix it in a few days, and collect potential feedback of erdma code in linux-next. Thanks, Cheng Xu
diff --git a/drivers/infiniband/hw/erdma/erdma_cmdq.c b/drivers/infiniband/hw/erdma/erdma_cmdq.c index 0cf5032d4b78..0489838d9717 100644 --- a/drivers/infiniband/hw/erdma/erdma_cmdq.c +++ b/drivers/infiniband/hw/erdma/erdma_cmdq.c @@ -78,10 +78,9 @@ static int erdma_cmdq_wait_res_init(struct erdma_dev *dev, return -ENOMEM; spin_lock_init(&cmdq->lock); - cmdq->comp_wait_bitmap = - devm_kcalloc(&dev->pdev->dev, - BITS_TO_LONGS(cmdq->max_outstandings), - sizeof(unsigned long), GFP_KERNEL); + cmdq->comp_wait_bitmap = devm_bitmap_zalloc(&dev->pdev->dev, + cmdq->max_outstandings, + GFP_KERNEL); if (!cmdq->comp_wait_bitmap) { devm_kfree(&dev->pdev->dev, cmdq->wait_pool); return -ENOMEM; diff --git a/drivers/infiniband/hw/erdma/erdma_main.c b/drivers/infiniband/hw/erdma/erdma_main.c index 27484bea51d9..7e1e27acb404 100644 --- a/drivers/infiniband/hw/erdma/erdma_main.c +++ b/drivers/infiniband/hw/erdma/erdma_main.c @@ -423,9 +423,8 @@ static int erdma_res_cb_init(struct erdma_dev *dev) for (i = 0; i < ERDMA_RES_CNT; i++) { dev->res_cb[i].next_alloc_idx = 1; spin_lock_init(&dev->res_cb[i].lock); - dev->res_cb[i].bitmap = - kcalloc(BITS_TO_LONGS(dev->res_cb[i].max_cap), - sizeof(unsigned long), GFP_KERNEL); + dev->res_cb[i].bitmap = bitmap_zalloc(dev->res_cb[i].max_cap, + GFP_KERNEL); /* We will free the memory in erdma_res_cb_free */ if (!dev->res_cb[i].bitmap) goto err; @@ -435,7 +434,7 @@ static int erdma_res_cb_init(struct erdma_dev *dev) err: for (j = 0; j < i; j++) - kfree(dev->res_cb[j].bitmap); + bitmap_free(dev->res_cb[j].bitmap); return -ENOMEM; } @@ -445,7 +444,7 @@ static void erdma_res_cb_free(struct erdma_dev *dev) int i; for (i = 0; i < ERDMA_RES_CNT; i++) - kfree(dev->res_cb[i].bitmap); + bitmap_free(dev->res_cb[i].bitmap); } static const struct ib_device_ops erdma_device_ops = {
Use [devm_]bitmap_zalloc()/bitmap_free() instead of hand-writing them. It is less verbose and it improves the semantic. Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- drivers/infiniband/hw/erdma/erdma_cmdq.c | 7 +++---- drivers/infiniband/hw/erdma/erdma_main.c | 9 ++++----- 2 files changed, 7 insertions(+), 9 deletions(-)