* [PATCH 1/1] Fixed to BUG_ON to WARN_ON def @ 2016-12-12 10:58 Ozgur Karatas 2016-12-12 11:19 ` Stefan Schmidt 2016-12-12 12:39 ` [PATCH 1/1] Fixed to BUG_ON to WARN_ON def Leon Romanovsky 0 siblings, 2 replies; 8+ messages in thread From: Ozgur Karatas @ 2016-12-12 10:58 UTC (permalink / raw) To: yishaih; +Cc: netdev, linux-kernel 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] Fixed to BUG_ON to WARN_ON def 2016-12-12 10:58 [PATCH 1/1] Fixed to BUG_ON to WARN_ON def Ozgur Karatas @ 2016-12-12 11:19 ` Stefan Schmidt 2016-12-12 11:32 ` [PATCH 1/1] Fixed to BUG_ON to WARN_ON Ozgur Karatas 2016-12-12 12:39 ` [PATCH 1/1] Fixed to BUG_ON to WARN_ON def Leon Romanovsky 1 sibling, 1 reply; 8+ messages in thread From: Stefan Schmidt @ 2016-12-12 11:19 UTC (permalink / raw) To: Ozgur Karatas, yishaih; +Cc: netdev, linux-kernel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] Fixed to BUG_ON to WARN_ON 2016-12-12 11:19 ` Stefan Schmidt @ 2016-12-12 11:32 ` Ozgur Karatas 0 siblings, 0 replies; 8+ messages in thread From: Ozgur Karatas @ 2016-12-12 11:32 UTC (permalink / raw) To: Stefan Schmidt, yishaih@mellanox.com; +Cc: netdev, linux-kernel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] Fixed to BUG_ON to WARN_ON def 2016-12-12 10:58 [PATCH 1/1] Fixed to BUG_ON to WARN_ON def Ozgur Karatas 2016-12-12 11:19 ` Stefan Schmidt @ 2016-12-12 12:39 ` Leon Romanovsky 2016-12-12 13:04 ` Ozgur Karatas 1 sibling, 1 reply; 8+ messages in thread From: Leon Romanovsky @ 2016-12-12 12:39 UTC (permalink / raw) To: Ozgur Karatas; +Cc: yishaih, netdev, linux-kernel [-- 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 --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] Fixed to BUG_ON to WARN_ON def 2016-12-12 12:39 ` [PATCH 1/1] Fixed to BUG_ON to WARN_ON def Leon Romanovsky @ 2016-12-12 13:04 ` Ozgur Karatas 2016-12-12 18:18 ` Leon Romanovsky 0 siblings, 1 reply; 8+ messages in thread From: Ozgur Karatas @ 2016-12-12 13:04 UTC (permalink / raw) To: Leon Romanovsky; +Cc: yishaih@mellanox.com, netdev, linux-kernel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] Fixed to BUG_ON to WARN_ON def 2016-12-12 13:04 ` Ozgur Karatas @ 2016-12-12 18:18 ` Leon Romanovsky 2016-12-12 20:24 ` Ozgur Karatas 2016-12-14 12:58 ` Tariq Toukan 0 siblings, 2 replies; 8+ messages in thread From: Leon Romanovsky @ 2016-12-12 18:18 UTC (permalink / raw) To: Ozgur Karatas, Tariq Toukan; +Cc: yishaih@mellanox.com, netdev, linux-kernel [-- 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 --] ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] Fixed to BUG_ON to WARN_ON def 2016-12-12 18:18 ` Leon Romanovsky @ 2016-12-12 20:24 ` Ozgur Karatas 2016-12-14 12:58 ` Tariq Toukan 1 sibling, 0 replies; 8+ messages in thread From: Ozgur Karatas @ 2016-12-12 20:24 UTC (permalink / raw) To: Leon Romanovsky, Tariq Toukan; +Cc: yishaih@mellanox.com, netdev, linux-kernel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] Fixed to BUG_ON to WARN_ON def 2016-12-12 18:18 ` Leon Romanovsky 2016-12-12 20:24 ` Ozgur Karatas @ 2016-12-14 12:58 ` Tariq Toukan 1 sibling, 0 replies; 8+ messages in thread From: Tariq Toukan @ 2016-12-14 12:58 UTC (permalink / raw) To: Leon Romanovsky, Ozgur Karatas, Tariq Toukan Cc: yishaih@mellanox.com, netdev, linux-kernel 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-12-14 12:58 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-12-12 10:58 [PATCH 1/1] Fixed to BUG_ON to WARN_ON def Ozgur Karatas 2016-12-12 11:19 ` Stefan Schmidt 2016-12-12 11:32 ` [PATCH 1/1] Fixed to BUG_ON to WARN_ON Ozgur Karatas 2016-12-12 12:39 ` [PATCH 1/1] Fixed to BUG_ON to WARN_ON def Leon Romanovsky 2016-12-12 13:04 ` Ozgur Karatas 2016-12-12 18:18 ` Leon Romanovsky 2016-12-12 20:24 ` Ozgur Karatas 2016-12-14 12:58 ` Tariq Toukan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).