[PATCH 1/1] Fixed to BUG_ON to WARN_ON def

[PATCH 1/1] Fixed to BUG_ON to WARN_ON def

[Ozgur Karatas]

Hello all, 
I think should be use to "WARN_ON" and checkpatch script give to error, I fixed and I think  should don't use "BUG_ON".
Regards,

Signed-off-by: Ozgur Karatas <okaratas@member.fsf.org>
---
drivers/net/ethernet/mellanox/mlx4/icm.c |  4 ++--

diff --git a/drivers/net/ethernet/mellanox/mlx4/icm.c b/drivers/net/ethernet/mellanox/mlx4/icm.c
index 2a9dd46..3fde535 100644
--- a/drivers/net/ethernet/mellanox/mlx4/icm.c
+++ b/drivers/net/ethernet/mellanox/mlx4/icm.c
@@ -119,7 +119,7 @@ static int mlx4_alloc_icm_coherent(struct device *dev, struct scatterlist *mem,
 		return -ENOMEM;
 
 	sg_set_buf(mem, buf, PAGE_SIZE << order);
-	BUG_ON(mem->offset);
+	WARN_ON(mem->offset);
 	sg_dma_len(mem) = PAGE_SIZE << order;
 	return 0;
 }
@@ -133,7 +133,7 @@ struct mlx4_icm *mlx4_alloc_icm(struct mlx4_dev *dev, int npages,
 	int ret;
 
 	/* We use sg_set_buf for coherent allocs, which assumes low memory */
-	BUG_ON(coherent && (gfp_mask & __GFP_HIGHMEM));
+	WARN_ON(coherent && (gfp_mask & __GFP_HIGHMEM));
 
 	icm = kmalloc_node(sizeof(*icm),
 			   gfp_mask & ~(__GFP_HIGHMEM | __GFP_NOWARN),
-- 
2.1.4

Re: [PATCH 1/1] Fixed to BUG_ON to WARN_ON def

[Stefan Schmidt]

Hello.

On 12/12/16 11:58, Ozgur Karatas wrote:
> Hello all,
> I think should be use to "WARN_ON" and checkpatch script give to error, I fixed and I think  should don't use "BUG_ON".
> Regards,
>
> Signed-off-by: Ozgur Karatas <okaratas@member.fsf.org>

I pointed you already before to the Documentation how to prepare a 
commit subject and commit message. You just replied with that you are 
new to contributing patches. That is all fine and many people are new 
each release. Please take the time to read the provided and pointed out 
docs.

If you keep ignoring such suggestions and docs I would think people will 
keep ignoring your patches.

regards
Stefan Schmidt

Re: [PATCH 1/1] Fixed to BUG_ON to WARN_ON

[Ozgur Karatas]

Dear Stefan;

I'm reading to Documentation/SubmittingPatches and I still apologized for misrepresentations my patches. 
I will add a next time good commit message and commit subjects.

Sorry,

Regards

Ozgur Karatas

12.12.2016, 13:20, "Stefan Schmidt" <stefan@osg.samsung.com>:
> Hello.
>
> On 12/12/16 11:58, Ozgur Karatas wrote:
>>  Hello all,
>>  I think should be use to "WARN_ON" and checkpatch script give to error, I fixed and I think should don't use "BUG_ON".
>>  Regards,
>>
>>  Signed-off-by: Ozgur Karatas <okaratas@member.fsf.org>
>
> I pointed you already before to the Documentation how to prepare a
> commit subject and commit message. You just replied with that you are
> new to contributing patches. That is all fine and many people are new
> each release. Please take the time to read the provided and pointed out
> docs.
>
> If you keep ignoring such suggestions and docs I would think people will
> keep ignoring your patches.
>
> regards
> Stefan Schmidt

Re: [PATCH 1/1] Fixed to BUG_ON to WARN_ON def

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1471 bytes --]

On Mon, Dec 12, 2016 at 12:58:59PM +0200, Ozgur Karatas wrote:
> Hello all,
> I think should be use to "WARN_ON" and checkpatch script give to error, I fixed and I think  should don't use "BUG_ON".
> Regards,
>
> Signed-off-by: Ozgur Karatas <okaratas@member.fsf.org>

NAK, Leon Romanovsky <leonro@mellanox.com>

If we put aside commit message issue, which was pointed to you by Stefan, your
proposed change is incorrect. By chnaging BUG_ONs to be WARN_ONs, you
will left the driver in improper state.

Thanks

> ---
> drivers/net/ethernet/mellanox/mlx4/icm.c |  4 ++--
>
> diff --git a/drivers/net/ethernet/mellanox/mlx4/icm.c b/drivers/net/ethernet/mellanox/mlx4/icm.c
> index 2a9dd46..3fde535 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/icm.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/icm.c
> @@ -119,7 +119,7 @@ static int mlx4_alloc_icm_coherent(struct device *dev, struct scatterlist *mem,
>  		return -ENOMEM;
>
>  	sg_set_buf(mem, buf, PAGE_SIZE << order);
> -	BUG_ON(mem->offset);
> +	WARN_ON(mem->offset);
>  	sg_dma_len(mem) = PAGE_SIZE << order;
>  	return 0;
>  }
> @@ -133,7 +133,7 @@ struct mlx4_icm *mlx4_alloc_icm(struct mlx4_dev *dev, int npages,
>  	int ret;
>
>  	/* We use sg_set_buf for coherent allocs, which assumes low memory */
> -	BUG_ON(coherent && (gfp_mask & __GFP_HIGHMEM));
> +	WARN_ON(coherent && (gfp_mask & __GFP_HIGHMEM));
>
>  	icm = kmalloc_node(sizeof(*icm),
>  			   gfp_mask & ~(__GFP_HIGHMEM | __GFP_NOWARN),
> --
> 2.1.4

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/1] Fixed to BUG_ON to WARN_ON def

[Ozgur Karatas]

Dear Romanovsky;

I'm trying to learn english and I apologize for my mistake words and phrases. So, I think the code when call to "sg_set_buf" and next time set memory and buffer. For example, isn't to call "WARN_ON" function, get a error to implicit declaration, right?

Because, you will use to "BUG_ON" get a error implicit declaration of functions.

        sg_set_buf(mem, buf, PAGE_SIZE << order);
        WARN_ON(mem->offset);

Thanks for information and learn to me.

Regards,

Ozgur Karatas

12.12.2016, 14:39, "Leon Romanovsky" <leon@kernel.org>:
> On Mon, Dec 12, 2016 at 12:58:59PM +0200, Ozgur Karatas wrote:
>>  Hello all,
>>  I think should be use to "WARN_ON" and checkpatch script give to error, I fixed and I think should don't use "BUG_ON".
>>  Regards,
>>
>>  Signed-off-by: Ozgur Karatas <okaratas@member.fsf.org>
>
> NAK, Leon Romanovsky <leonro@mellanox.com>
>
> If we put aside commit message issue, which was pointed to you by Stefan, your
> proposed change is incorrect. By chnaging BUG_ONs to be WARN_ONs, you
> will left the driver in improper state.
>
> Thanks
>
>>  ---
>>  drivers/net/ethernet/mellanox/mlx4/icm.c | 4 ++--
>>
>>  diff --git a/drivers/net/ethernet/mellanox/mlx4/icm.c b/drivers/net/ethernet/mellanox/mlx4/icm.c
>>  index 2a9dd46..3fde535 100644
>>  --- a/drivers/net/ethernet/mellanox/mlx4/icm.c
>>  +++ b/drivers/net/ethernet/mellanox/mlx4/icm.c
>>  @@ -119,7 +119,7 @@ static int mlx4_alloc_icm_coherent(struct device *dev, struct scatterlist *mem,
>>                   return -ENOMEM;
>>
>>           sg_set_buf(mem, buf, PAGE_SIZE << order);
>>  - BUG_ON(mem->offset);
>>  + WARN_ON(mem->offset);
>>           sg_dma_len(mem) = PAGE_SIZE << order;
>>           return 0;
>>   }
>>  @@ -133,7 +133,7 @@ struct mlx4_icm *mlx4_alloc_icm(struct mlx4_dev *dev, int npages,
>>           int ret;
>>
>>           /* We use sg_set_buf for coherent allocs, which assumes low memory */
>>  - BUG_ON(coherent && (gfp_mask & __GFP_HIGHMEM));
>>  + WARN_ON(coherent && (gfp_mask & __GFP_HIGHMEM));
>>
>>           icm = kmalloc_node(sizeof(*icm),
>>                              gfp_mask & ~(__GFP_HIGHMEM | __GFP_NOWARN),
>>  --
>>  2.1.4

Re: [PATCH 1/1] Fixed to BUG_ON to WARN_ON def

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2204 bytes --]

On Mon, Dec 12, 2016 at 03:04:28PM +0200, Ozgur Karatas wrote:
> Dear Romanovsky;

Please avoid top-posting in your replies.
Thanks

>
> I'm trying to learn english and I apologize for my mistake words and phrases. So, I think the code when call to "sg_set_buf" and next time set memory and buffer. For example, isn't to call "WARN_ON" function, get a error to implicit declaration, right?
>
> Because, you will use to "BUG_ON" get a error implicit declaration of functions.

I'm not sure that I followed you. mem->offset is set by sg_set_buf from
buf variable returned by dma_alloc_coherent(). HW needs to get very
precise size of this buf, in multiple of pages and aligned to pages
boundaries.

>
>         sg_set_buf(mem, buf, PAGE_SIZE << order);
>         WARN_ON(mem->offset);

See the patch inline which removes this BUG_ON in proper and safe way.

From 7babe807affa2b27d51d3610afb75b693929ea1a Mon Sep 17 00:00:00 2001
From: Leon Romanovsky <leonro@mellanox.com>
Date: Mon, 12 Dec 2016 20:02:45 +0200
Subject: [PATCH] net/mlx4: Remove BUG_ON from ICM allocation routine

This patch removes BUG_ON() macro from mlx4_alloc_icm_coherent()
by checking DMA address aligment in advance and performing proper
folding in case of error.

Fixes: 5b0bf5e25efe ("mlx4_core: Support ICM tables in coherent memory")
Reported-by: Ozgur Karatas <okaratas@member.fsf.org>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx4/icm.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/icm.c b/drivers/net/ethernet/mellanox/mlx4/icm.c
index 2a9dd46..e1f9e7c 100644
--- a/drivers/net/ethernet/mellanox/mlx4/icm.c
+++ b/drivers/net/ethernet/mellanox/mlx4/icm.c
@@ -118,8 +118,13 @@ static int mlx4_alloc_icm_coherent(struct device *dev, struct scatterlist *mem,
 	if (!buf)
 		return -ENOMEM;

+	if (offset_in_page(buf)) {
+		dma_free_coherent(dev, PAGE_SIZE << order,
+				  buf, sg_dma_address(mem));
+		return -ENOMEM;
+	}
+
 	sg_set_buf(mem, buf, PAGE_SIZE << order);
-	BUG_ON(mem->offset);
 	sg_dma_len(mem) = PAGE_SIZE << order;
 	return 0;
 }
--
2.10.2


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/1] Fixed to BUG_ON to WARN_ON def

[Ozgur Karatas]

12.12.2016, 20:18, "Leon Romanovsky" <leon@kernel.org>:
> On Mon, Dec 12, 2016 at 03:04:28PM +0200, Ozgur Karatas wrote:
>>  Dear Romanovsky;
>
> Please avoid top-posting in your replies.
> Thanks

Dear Leon; 
thanks for the information., I will pay attention.

>>  I'm trying to learn english and I apologize for my mistake words and phrases. So, I think the code when call to "sg_set_buf" and next time set memory and buffer. For example, isn't to call "WARN_ON" function, get a error to implicit declaration, right?
>>
>>  Because, you will use to "BUG_ON" get a error implicit declaration of functions.
>
> I'm not sure that I followed you. mem->offset is set by sg_set_buf from
> buf variable returned by dma_alloc_coherent(). HW needs to get very
> precise size of this buf, in multiple of pages and aligned to pages
> boundaries.

I have studied the following your coding and I guess that's the right patchs.
You are the very expert in this matter, thank you for the correct for me.

I learn to your style as an example.

Regards,

Ozgur Karatas

> See the patch inline which removes this BUG_ON in proper and safe way.
>
> From 7babe807affa2b27d51d3610afb75b693929ea1a Mon Sep 17 00:00:00 2001
> From: Leon Romanovsky <leonro@mellanox.com>
> Date: Mon, 12 Dec 2016 20:02:45 +0200
> Subject: [PATCH] net/mlx4: Remove BUG_ON from ICM allocation routine
>
> This patch removes BUG_ON() macro from mlx4_alloc_icm_coherent()
> by checking DMA address aligment in advance and performing proper
> folding in case of error.
>
> Fixes: 5b0bf5e25efe ("mlx4_core: Support ICM tables in coherent memory")
> Reported-by: Ozgur Karatas <okaratas@member.fsf.org>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> ---
>  drivers/net/ethernet/mellanox/mlx4/icm.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx4/icm.c b/drivers/net/ethernet/mellanox/mlx4/icm.c
> index 2a9dd46..e1f9e7c 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/icm.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/icm.c
> @@ -118,8 +118,13 @@ static int mlx4_alloc_icm_coherent(struct device *dev, struct scatterlist *mem,
>          if (!buf)
>                  return -ENOMEM;
>
> + if (offset_in_page(buf)) {
> + dma_free_coherent(dev, PAGE_SIZE << order,
> + buf, sg_dma_address(mem));
> + return -ENOMEM;
> + }
> +
>          sg_set_buf(mem, buf, PAGE_SIZE << order);
> - BUG_ON(mem->offset);
>          sg_dma_len(mem) = PAGE_SIZE << order;
>          return 0;
>  }
> --
> 2.10.2

Re: [PATCH 1/1] Fixed to BUG_ON to WARN_ON def

[Tariq Toukan]

Thanks Ozgur for your report.


On 12/12/2016 8:18 PM, Leon Romanovsky wrote:
> On Mon, Dec 12, 2016 at 03:04:28PM +0200, Ozgur Karatas wrote:
>> Dear Romanovsky;
> Please avoid top-posting in your replies.
> Thanks
>
>> I'm trying to learn english and I apologize for my mistake words and phrases. So, I think the code when call to "sg_set_buf" and next time set memory and buffer. For example, isn't to call "WARN_ON" function, get a error to implicit declaration, right?
>>
>> Because, you will use to "BUG_ON" get a error implicit declaration of functions.
> I'm not sure that I followed you. mem->offset is set by sg_set_buf from
> buf variable returned by dma_alloc_coherent(). HW needs to get very
> precise size of this buf, in multiple of pages and aligned to pages
> boundaries.
>
>>          sg_set_buf(mem, buf, PAGE_SIZE << order);
>>          WARN_ON(mem->offset);
> See the patch inline which removes this BUG_ON in proper and safe way.
>
>  From 7babe807affa2b27d51d3610afb75b693929ea1a Mon Sep 17 00:00:00 2001
> From: Leon Romanovsky <leonro@mellanox.com>
> Date: Mon, 12 Dec 2016 20:02:45 +0200
> Subject: [PATCH] net/mlx4: Remove BUG_ON from ICM allocation routine
>
> This patch removes BUG_ON() macro from mlx4_alloc_icm_coherent()
> by checking DMA address aligment in advance and performing proper
> folding in case of error.
>
> Fixes: 5b0bf5e25efe ("mlx4_core: Support ICM tables in coherent memory")
> Reported-by: Ozgur Karatas <okaratas@member.fsf.org>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> ---
>   drivers/net/ethernet/mellanox/mlx4/icm.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx4/icm.c b/drivers/net/ethernet/mellanox/mlx4/icm.c
> index 2a9dd46..e1f9e7c 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/icm.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/icm.c
> @@ -118,8 +118,13 @@ static int mlx4_alloc_icm_coherent(struct device *dev, struct scatterlist *mem,
>   	if (!buf)
>   		return -ENOMEM;
>
> +	if (offset_in_page(buf)) {
> +		dma_free_coherent(dev, PAGE_SIZE << order,
> +				  buf, sg_dma_address(mem));
> +		return -ENOMEM;
> +	}
> +
>   	sg_set_buf(mem, buf, PAGE_SIZE << order);
> -	BUG_ON(mem->offset);
>   	sg_dma_len(mem) = PAGE_SIZE << order;
>   	return 0;
>   }
> --
Thanks Leon for the patch. It is the right way to do so.
Reviewed-by: Tariq Toukan <tariqt@mellanox.com>

We will submit Leon's patch in a new email.

Regards,
Tariq
> 2.10.2
>