[PATCH 1/2] RDMA/erdma: Use the bitmap API to allocate bitmaps

[PATCH 1/2] RDMA/erdma: Use the bitmap API to allocate bitmaps

[Christophe JAILLET]

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(-)

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 = {
-- 
2.34.1

[PATCH 2/2] RDMA/erdma: Use the non-atomic bitmap API when applicable

[Christophe JAILLET]

Usages of the 'comp_wait_bitmap' bitmap is protected with a spinlock, so
non-atomic functions can be used to set/clear bits.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/infiniband/hw/erdma/erdma_cmdq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/hw/erdma/erdma_cmdq.c b/drivers/infiniband/hw/erdma/erdma_cmdq.c
index 0489838d9717..e3d426668788 100644
--- a/drivers/infiniband/hw/erdma/erdma_cmdq.c
+++ b/drivers/infiniband/hw/erdma/erdma_cmdq.c
@@ -47,7 +47,7 @@ static struct erdma_comp_wait *get_comp_wait(struct erdma_cmdq *cmdq)
 		return ERR_PTR(-ENOMEM);
 	}
 
-	set_bit(comp_idx, cmdq->comp_wait_bitmap);
+	__set_bit(comp_idx, cmdq->comp_wait_bitmap);
 	spin_unlock(&cmdq->lock);
 
 	return &cmdq->wait_pool[comp_idx];
@@ -60,7 +60,7 @@ static void put_comp_wait(struct erdma_cmdq *cmdq,
 
 	cmdq->wait_pool[comp_wait->ctx_id].cmd_status = ERDMA_CMD_STATUS_INIT;
 	spin_lock(&cmdq->lock);
-	used = test_and_clear_bit(comp_wait->ctx_id, cmdq->comp_wait_bitmap);
+	used = __test_and_clear_bit(comp_wait->ctx_id, cmdq->comp_wait_bitmap);
 	spin_unlock(&cmdq->lock);
 
 	WARN_ON(!used);
-- 
2.34.1

Re: [PATCH 1/2] RDMA/erdma: Use the bitmap API to allocate bitmaps

[Cheng Xu]

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 = {

Re: [PATCH 1/2] RDMA/erdma: Use the bitmap API to allocate bitmaps

[Dan Carpenter]

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

Re: [PATCH 1/2] RDMA/erdma: Use the bitmap API to allocate bitmaps

[Cheng Xu]

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

Re: [PATCH 1/2] RDMA/erdma: Use the bitmap API to allocate bitmaps

[Jason Gunthorpe]

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

Re: [PATCH 1/2] RDMA/erdma: Use the bitmap API to allocate bitmaps

[Dan Carpenter]

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

Re: [PATCH 1/2] RDMA/erdma: Use the bitmap API to allocate bitmaps

[Christophe JAILLET]

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

Re: [PATCH 1/2] RDMA/erdma: Use the bitmap API to allocate bitmaps

[Jason Gunthorpe]

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

Re: [PATCH 1/2] RDMA/erdma: Use the bitmap API to allocate bitmaps

[Cheng Xu]

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

Re: [PATCH 1/2] RDMA/erdma: Use the bitmap API to allocate bitmaps

[Leon Romanovsky]

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

Re: [PATCH 1/2] RDMA/erdma: Use the bitmap API to allocate bitmaps

[Cheng Xu]

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

Re: [PATCH 1/2] RDMA/erdma: Use the bitmap API to allocate bitmaps

[Leon Romanovsky]

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