Re: [PATCH 1/1] Fixed to BUG_ON to WARN_ON def
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
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
Signed-off-by: Leon Romanovsky
---
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
We will submit Leon's patch in a new email.
Regards,
Tariq
2.10.2
Re: [PATCH 1/1] Fixed to BUG_ON to WARN_ON def
12.12.2016, 20:18, "Leon Romanovsky" :
> 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
> 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
> Signed-off-by: Leon Romanovsky
> ---
> 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
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
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
Signed-off-by: Leon Romanovsky
---
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
signature.asc
Description: PGP signature
Re: [PATCH 1/1] Fixed to BUG_ON to WARN_ON def
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" : > 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 > > NAK, Leon Romanovsky > > 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
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 NAK, Leon Romanovsky 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 signature.asc Description: PGP signature
Re: [PATCH 1/1] Fixed to BUG_ON to WARN_ON def
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 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
