Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1692230 > unrolled thread
| Started by | Christophe JAILLET <christophe.jaillet@wanadoo.fr> |
|---|---|
| First post | 2017-07-20 00:20 +0200 |
| Last post | 2017-07-21 19:00 +0200 |
| Articles | 11 — 7 participants |
Back to article view | Back to linux.kernel
[PATCH] dmaengine: ioatdma: Fix error handling path in 'ioat_dma_self_test()' Christophe JAILLET <christophe.jaillet@wanadoo.fr> - 2017-07-20 00:20 +0200
Re: [PATCH] dmaengine: ioatdma: Fix error handling path in 'ioat_dma_self_test()' Dave Jiang <dave.jiang@intel.com> - 2017-07-20 00:30 +0200
Re: [PATCH] dmaengine: ioatdma: Fix error handling path in 'ioat_dma_self_test()' Vinod Koul <vinod.koul@intel.com> - 2017-07-21 08:30 +0200
Re: [PATCH] dmaengine: ioatdma: Fix error handling path in 'ioat_dma_self_test()' "Jiang, Dave" <dave.jiang@intel.com> - 2017-07-21 08:50 +0200
Re: [PATCH] dmaengine: ioatdma: Fix error handling path in 'ioat_dma_self_test()' walter harms <wharms@bfs.de> - 2017-07-20 09:30 +0200
Re: [PATCH] dmaengine: ioatdma: Fix error handling path in 'ioat_dma_self_test()' Dave Jiang <dave.jiang@intel.com> - 2017-07-20 19:00 +0200
Re: [PATCH] dmaengine: ioatdma: Fix error handling path in 'ioat_dma_self_test()' walter harms <wharms@bfs.de> - 2017-07-21 09:30 +0200
Re: [PATCH] dmaengine: ioatdma: Fix error handling path in 'ioat_dma_self_test()' Julia Lawall <julia.lawall@lip6.fr> - 2017-07-21 09:30 +0200
Re: [PATCH] dmaengine: ioatdma: Fix error handling path in 'ioat_dma_self_test()' Dan Carpenter <dan.carpenter@oracle.com> - 2017-07-21 11:40 +0200
Re: [PATCH] dmaengine: ioatdma: Fix error handling path in 'ioat_dma_self_test()' Vinod Koul <vinod.koul@intel.com> - 2017-07-21 10:00 +0200
Re: [PATCH] dmaengine: ioatdma: Fix error handling path in 'ioat_dma_self_test()' Dave Jiang <dave.jiang@intel.com> - 2017-07-21 19:00 +0200
| From | Christophe JAILLET <christophe.jaillet@wanadoo.fr> |
|---|---|
| Date | 2017-07-20 00:20 +0200 |
| Subject | [PATCH] dmaengine: ioatdma: Fix error handling path in 'ioat_dma_self_test()' |
| Message-ID | <u55u2-6uk-7@gated-at.bofh.it> |
If the 'memcmp' fails, free allocated resources as done in all other
error handling paths.
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
Please review carefully, this patch looks "too obvious" to me!
---
drivers/dma/ioat/init.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c
index ed8ed1192775..948fc1f8fb5c 100644
--- a/drivers/dma/ioat/init.c
+++ b/drivers/dma/ioat/init.c
@@ -390,7 +390,7 @@ static int ioat_dma_self_test(struct ioatdma_device *ioat_dma)
if (memcmp(src, dest, IOAT_TEST_SIZE)) {
dev_err(dev, "Self-test copy failed compare, disabling\n");
err = -ENODEV;
- goto free_resources;
+ goto unmap_dma;
}
unmap_dma:
--
2.11.0
[toc] | [next] | [standalone]
| From | Dave Jiang <dave.jiang@intel.com> |
|---|---|
| Date | 2017-07-20 00:30 +0200 |
| Subject | Re: [PATCH] dmaengine: ioatdma: Fix error handling path in 'ioat_dma_self_test()' |
| Message-ID | <u55DH-6xU-7@gated-at.bofh.it> |
| In reply to | #1692230 |
On 07/19/2017 03:16 PM, Christophe JAILLET wrote:
> If the 'memcmp' fails, free allocated resources as done in all other
> error handling paths.
>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Good catch! Thanks.
Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
> Please review carefully, this patch looks "too obvious" to me!
> ---
> drivers/dma/ioat/init.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c
> index ed8ed1192775..948fc1f8fb5c 100644
> --- a/drivers/dma/ioat/init.c
> +++ b/drivers/dma/ioat/init.c
> @@ -390,7 +390,7 @@ static int ioat_dma_self_test(struct ioatdma_device *ioat_dma)
> if (memcmp(src, dest, IOAT_TEST_SIZE)) {
> dev_err(dev, "Self-test copy failed compare, disabling\n");
> err = -ENODEV;
> - goto free_resources;
> + goto unmap_dma;
> }
>
> unmap_dma:
>
[toc] | [prev] | [next] | [standalone]
| From | Vinod Koul <vinod.koul@intel.com> |
|---|---|
| Date | 2017-07-21 08:30 +0200 |
| Subject | Re: [PATCH] dmaengine: ioatdma: Fix error handling path in 'ioat_dma_self_test()' |
| Message-ID | <u5zBM-1do-17@gated-at.bofh.it> |
| In reply to | #1692232 |
On Wed, Jul 19, 2017 at 03:21:23PM -0700, Dave Jiang wrote:
>
>
> On 07/19/2017 03:16 PM, Christophe JAILLET wrote:
> > If the 'memcmp' fails, free allocated resources as done in all other
> > error handling paths.
> >
> > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
You meant acked right..?
>
> Good catch! Thanks.
>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>
> > ---
> > Please review carefully, this patch looks "too obvious" to me!
> > ---
> > drivers/dma/ioat/init.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c
> > index ed8ed1192775..948fc1f8fb5c 100644
> > --- a/drivers/dma/ioat/init.c
> > +++ b/drivers/dma/ioat/init.c
> > @@ -390,7 +390,7 @@ static int ioat_dma_self_test(struct ioatdma_device *ioat_dma)
> > if (memcmp(src, dest, IOAT_TEST_SIZE)) {
> > dev_err(dev, "Self-test copy failed compare, disabling\n");
> > err = -ENODEV;
> > - goto free_resources;
> > + goto unmap_dma;
> > }
> >
> > unmap_dma:
> >
--
~Vinod
[toc] | [prev] | [next] | [standalone]
| From | "Jiang, Dave" <dave.jiang@intel.com> |
|---|---|
| Date | 2017-07-21 08:50 +0200 |
| Subject | Re: [PATCH] dmaengine: ioatdma: Fix error handling path in 'ioat_dma_self_test()' |
| Message-ID | <u5zV7-1jT-7@gated-at.bofh.it> |
| In reply to | #1693407 |
> On Jul 20, 2017, at 11:28 PM, Koul, Vinod <vinod.koul@intel.com> wrote:
>
>> On Wed, Jul 19, 2017 at 03:21:23PM -0700, Dave Jiang wrote:
>>
>>
>>> On 07/19/2017 03:16 PM, Christophe JAILLET wrote:
>>> If the 'memcmp' fails, free allocated resources as done in all other
>>> error handling paths.
>>>
>>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>
> You meant acked right..?
Yes. Typo :)
>
>>
>> Good catch! Thanks.
>>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
>>
>>> ---
>>> Please review carefully, this patch looks "too obvious" to me!
>>> ---
>>> drivers/dma/ioat/init.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c
>>> index ed8ed1192775..948fc1f8fb5c 100644
>>> --- a/drivers/dma/ioat/init.c
>>> +++ b/drivers/dma/ioat/init.c
>>> @@ -390,7 +390,7 @@ static int ioat_dma_self_test(struct ioatdma_device *ioat_dma)
>>> if (memcmp(src, dest, IOAT_TEST_SIZE)) {
>>> dev_err(dev, "Self-test copy failed compare, disabling\n");
>>> err = -ENODEV;
>>> - goto free_resources;
>>> + goto unmap_dma;
>>> }
>>>
>>> unmap_dma:
>>>
>
> --
> ~Vinod
[toc] | [prev] | [next] | [standalone]
| From | walter harms <wharms@bfs.de> |
|---|---|
| Date | 2017-07-20 09:30 +0200 |
| Message-ID | <u5e4i-41p-37@gated-at.bofh.it> |
| In reply to | #1692230 |
Am 20.07.2017 00:16, schrieb Christophe JAILLET:
> If the 'memcmp' fails, free allocated resources as done in all other
> error handling paths.
>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> Please review carefully, this patch looks "too obvious" to me!
> ---
> drivers/dma/ioat/init.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c
> index ed8ed1192775..948fc1f8fb5c 100644
> --- a/drivers/dma/ioat/init.c
> +++ b/drivers/dma/ioat/init.c
> @@ -390,7 +390,7 @@ static int ioat_dma_self_test(struct ioatdma_device *ioat_dma)
> if (memcmp(src, dest, IOAT_TEST_SIZE)) {
> dev_err(dev, "Self-test copy failed compare, disabling\n");
> err = -ENODEV;
> - goto free_resources;
> + goto unmap_dma;
> }
>
> unmap_dma:
^^^^^^^^^^
is the goto needed at all ?
re,
wh
[toc] | [prev] | [next] | [standalone]
| From | Dave Jiang <dave.jiang@intel.com> |
|---|---|
| Date | 2017-07-20 19:00 +0200 |
| Subject | Re: [PATCH] dmaengine: ioatdma: Fix error handling path in 'ioat_dma_self_test()' |
| Message-ID | <u5mXU-1BO-19@gated-at.bofh.it> |
| In reply to | #1692503 |
On 07/20/2017 12:24 AM, walter harms wrote:
>
>
> Am 20.07.2017 00:16, schrieb Christophe JAILLET:
>> If the 'memcmp' fails, free allocated resources as done in all other
>> error handling paths.
>>
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>> Please review carefully, this patch looks "too obvious" to me!
>> ---
>> drivers/dma/ioat/init.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c
>> index ed8ed1192775..948fc1f8fb5c 100644
>> --- a/drivers/dma/ioat/init.c
>> +++ b/drivers/dma/ioat/init.c
>> @@ -390,7 +390,7 @@ static int ioat_dma_self_test(struct ioatdma_device *ioat_dma)
>> if (memcmp(src, dest, IOAT_TEST_SIZE)) {
>> dev_err(dev, "Self-test copy failed compare, disabling\n");
>> err = -ENODEV;
>> - goto free_resources;
>> + goto unmap_dma;
>> }
>>
>> unmap_dma:
>
> ^^^^^^^^^^
>
>
> is the goto needed at all ?
It's not. However, it may be better to stay there if we happen to add
additional code after the if block later on and guard against mistakes.
At least IMO.
>
> re,
> wh
>
[toc] | [prev] | [next] | [standalone]
| From | walter harms <wharms@bfs.de> |
|---|---|
| Date | 2017-07-21 09:30 +0200 |
| Message-ID | <u5AxQ-1LL-13@gated-at.bofh.it> |
| In reply to | #1693096 |
Am 20.07.2017 18:56, schrieb Dave Jiang:
>
>
> On 07/20/2017 12:24 AM, walter harms wrote:
>>
>>
>> Am 20.07.2017 00:16, schrieb Christophe JAILLET:
>>> If the 'memcmp' fails, free allocated resources as done in all other
>>> error handling paths.
>>>
>>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>>> ---
>>> Please review carefully, this patch looks "too obvious" to me!
>>> ---
>>> drivers/dma/ioat/init.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c
>>> index ed8ed1192775..948fc1f8fb5c 100644
>>> --- a/drivers/dma/ioat/init.c
>>> +++ b/drivers/dma/ioat/init.c
>>> @@ -390,7 +390,7 @@ static int ioat_dma_self_test(struct ioatdma_device *ioat_dma)
>>> if (memcmp(src, dest, IOAT_TEST_SIZE)) {
>>> dev_err(dev, "Self-test copy failed compare, disabling\n");
>>> err = -ENODEV;
>>> - goto free_resources;
>>> + goto unmap_dma;
>>> }
>>>
>>> unmap_dma:
>>
>> ^^^^^^^^^^
>>
>>
>> is the goto needed at all ?
>
> It's not. However, it may be better to stay there if we happen to add
> additional code after the if block later on and guard against mistakes.
> At least IMO.
>
If you are happy with that ... its not a big problem. The compiler will
eat that goto anyway but it is unusual so be prepared that other people
may send patches.
re,
wh
[toc] | [prev] | [next] | [standalone]
| From | Julia Lawall <julia.lawall@lip6.fr> |
|---|---|
| Date | 2017-07-21 09:30 +0200 |
| Subject | Re: [PATCH] dmaengine: ioatdma: Fix error handling path in 'ioat_dma_self_test()' |
| Message-ID | <u5AxQ-1LL-17@gated-at.bofh.it> |
| In reply to | #1693446 |
On Fri, 21 Jul 2017, walter harms wrote:
>
>
> Am 20.07.2017 18:56, schrieb Dave Jiang:
> >
> >
> > On 07/20/2017 12:24 AM, walter harms wrote:
> >>
> >>
> >> Am 20.07.2017 00:16, schrieb Christophe JAILLET:
> >>> If the 'memcmp' fails, free allocated resources as done in all other
> >>> error handling paths.
> >>>
> >>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> >>> ---
> >>> Please review carefully, this patch looks "too obvious" to me!
> >>> ---
> >>> drivers/dma/ioat/init.c | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c
> >>> index ed8ed1192775..948fc1f8fb5c 100644
> >>> --- a/drivers/dma/ioat/init.c
> >>> +++ b/drivers/dma/ioat/init.c
> >>> @@ -390,7 +390,7 @@ static int ioat_dma_self_test(struct ioatdma_device *ioat_dma)
> >>> if (memcmp(src, dest, IOAT_TEST_SIZE)) {
> >>> dev_err(dev, "Self-test copy failed compare, disabling\n");
> >>> err = -ENODEV;
> >>> - goto free_resources;
> >>> + goto unmap_dma;
> >>> }
> >>>
> >>> unmap_dma:
> >>
> >> ^^^^^^^^^^
> >>
> >>
> >> is the goto needed at all ?
> >
> > It's not. However, it may be better to stay there if we happen to add
> > additional code after the if block later on and guard against mistakes.
> > At least IMO.
> >
>
> If you are happy with that ... its not a big problem. The compiler will
> eat that goto anyway but it is unusual so be prepared that other people
> may send patches.
I agree with Walter.
julia
>
> re,
> wh
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
[toc] | [prev] | [next] | [standalone]
| From | Dan Carpenter <dan.carpenter@oracle.com> |
|---|---|
| Date | 2017-07-21 11:40 +0200 |
| Subject | Re: [PATCH] dmaengine: ioatdma: Fix error handling path in 'ioat_dma_self_test()' |
| Message-ID | <u5CzF-34w-39@gated-at.bofh.it> |
| In reply to | #1693448 |
I'm with Christophe. ;) I never like it when people get creative with the last test in a series of tests. regards, dan carpenter
[toc] | [prev] | [next] | [standalone]
| From | Vinod Koul <vinod.koul@intel.com> |
|---|---|
| Date | 2017-07-21 10:00 +0200 |
| Subject | Re: [PATCH] dmaengine: ioatdma: Fix error handling path in 'ioat_dma_self_test()' |
| Message-ID | <u5B0R-1Xy-7@gated-at.bofh.it> |
| In reply to | #1693096 |
On Thu, Jul 20, 2017 at 09:56:45AM -0700, Dave Jiang wrote:
>
>
> On 07/20/2017 12:24 AM, walter harms wrote:
> >
> >
> > Am 20.07.2017 00:16, schrieb Christophe JAILLET:
> >> If the 'memcmp' fails, free allocated resources as done in all other
> >> error handling paths.
> >>
> >> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> >> ---
> >> Please review carefully, this patch looks "too obvious" to me!
> >> ---
> >> drivers/dma/ioat/init.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c
> >> index ed8ed1192775..948fc1f8fb5c 100644
> >> --- a/drivers/dma/ioat/init.c
> >> +++ b/drivers/dma/ioat/init.c
> >> @@ -390,7 +390,7 @@ static int ioat_dma_self_test(struct ioatdma_device *ioat_dma)
> >> if (memcmp(src, dest, IOAT_TEST_SIZE)) {
> >> dev_err(dev, "Self-test copy failed compare, disabling\n");
> >> err = -ENODEV;
> >> - goto free_resources;
> >> + goto unmap_dma;
> >> }
> >>
> >> unmap_dma:
> >
> > ^^^^^^^^^^
> >
> >
> > is the goto needed at all ?
>
> It's not. However, it may be better to stay there if we happen to add
> additional code after the if block later on and guard against mistakes.
> At least IMO.
Then lets remove it please, there is no place for dead code, if we need it
people can add it as part of the changes they introduce..
--
~Vinod
[toc] | [prev] | [next] | [standalone]
| From | Dave Jiang <dave.jiang@intel.com> |
|---|---|
| Date | 2017-07-21 19:00 +0200 |
| Subject | Re: [PATCH] dmaengine: ioatdma: Fix error handling path in 'ioat_dma_self_test()' |
| Message-ID | <u5Jrs-7hV-19@gated-at.bofh.it> |
| In reply to | #1693460 |
On 07/21/2017 12:57 AM, Vinod Koul wrote:
> On Thu, Jul 20, 2017 at 09:56:45AM -0700, Dave Jiang wrote:
>>
>>
>> On 07/20/2017 12:24 AM, walter harms wrote:
>>>
>>>
>>> Am 20.07.2017 00:16, schrieb Christophe JAILLET:
>>>> If the 'memcmp' fails, free allocated resources as done in all other
>>>> error handling paths.
>>>>
>>>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>>>> ---
>>>> Please review carefully, this patch looks "too obvious" to me!
>>>> ---
>>>> drivers/dma/ioat/init.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c
>>>> index ed8ed1192775..948fc1f8fb5c 100644
>>>> --- a/drivers/dma/ioat/init.c
>>>> +++ b/drivers/dma/ioat/init.c
>>>> @@ -390,7 +390,7 @@ static int ioat_dma_self_test(struct ioatdma_device *ioat_dma)
>>>> if (memcmp(src, dest, IOAT_TEST_SIZE)) {
>>>> dev_err(dev, "Self-test copy failed compare, disabling\n");
>>>> err = -ENODEV;
>>>> - goto free_resources;
>>>> + goto unmap_dma;
>>>> }
>>>>
>>>> unmap_dma:
>>>
>>> ^^^^^^^^^^
>>>
>>>
>>> is the goto needed at all ?
>>
>> It's not. However, it may be better to stay there if we happen to add
>> additional code after the if block later on and guard against mistakes.
>> At least IMO.
>
> Then lets remove it please, there is no place for dead code, if we need it
> people can add it as part of the changes they introduce..
>
I have no strong opinion in this Christophe. I have seen mistakes and
bugs introduced because of these special optimizations. I've said my
piece and Vinod is for removing it so I'll defer to him.
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web