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 <[email protected]>
---
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
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 <[email protected]>
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
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" <[email protected]>:
> 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 <[email protected]>
>
> 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
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 <[email protected]>
NAK, Leon Romanovsky <[email protected]>
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
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" <[email protected]>:
> 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 <[email protected]>
>
> NAK, Leon Romanovsky <[email protected]>
>
> 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
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 <[email protected]>
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 <[email protected]>
Signed-off-by: Leon Romanovsky <[email protected]>
---
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
12.12.2016, 20:18, "Leon Romanovsky" <[email protected]>:
> 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 <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Leon Romanovsky <[email protected]>
> ---
> 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
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 <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Leon Romanovsky <[email protected]>
> ---
> 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 <[email protected]>
We will submit Leon's patch in a new email.
Regards,
Tariq
> 2.10.2
>