* [PATCH] dmaengine: ioatdma: Fix error handling path in 'ioat_dma_self_test()'
@ 2017-07-19 22:16 Christophe JAILLET
2017-07-19 22:21 ` Dave Jiang
2017-07-20 7:24 ` walter harms
0 siblings, 2 replies; 11+ messages in thread
From: Christophe JAILLET @ 2017-07-19 22:16 UTC (permalink / raw)
To: dan.j.williams, vinod.koul, dave.jiang
Cc: dmaengine, linux-kernel, kernel-janitors, 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:
--
2.11.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] dmaengine: ioatdma: Fix error handling path in 'ioat_dma_self_test()'
2017-07-19 22:16 [PATCH] dmaengine: ioatdma: Fix error handling path in 'ioat_dma_self_test()' Christophe JAILLET
@ 2017-07-19 22:21 ` Dave Jiang
2017-07-21 6:31 ` Vinod Koul
2017-07-20 7:24 ` walter harms
1 sibling, 1 reply; 11+ messages in thread
From: Dave Jiang @ 2017-07-19 22:21 UTC (permalink / raw)
To: Christophe JAILLET, Williams, Dan J, Koul, Vinod
Cc: dmaengine, linux-kernel, kernel-janitors
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:
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] dmaengine: ioatdma: Fix error handling path in 'ioat_dma_self_test()'
2017-07-19 22:16 [PATCH] dmaengine: ioatdma: Fix error handling path in 'ioat_dma_self_test()' Christophe JAILLET
2017-07-19 22:21 ` Dave Jiang
@ 2017-07-20 7:24 ` walter harms
2017-07-20 16:56 ` Dave Jiang
1 sibling, 1 reply; 11+ messages in thread
From: walter harms @ 2017-07-20 7:24 UTC (permalink / raw)
To: Christophe JAILLET
Cc: dan.j.williams, vinod.koul, dave.jiang, dmaengine, linux-kernel,
kernel-janitors
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] dmaengine: ioatdma: Fix error handling path in 'ioat_dma_self_test()'
2017-07-20 7:24 ` walter harms
@ 2017-07-20 16:56 ` Dave Jiang
2017-07-21 7:22 ` walter harms
2017-07-21 7:57 ` Vinod Koul
0 siblings, 2 replies; 11+ messages in thread
From: Dave Jiang @ 2017-07-20 16:56 UTC (permalink / raw)
To: wharms, Christophe JAILLET
Cc: Williams, Dan J, Koul, Vinod, dmaengine, linux-kernel, kernel-janitors
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
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] dmaengine: ioatdma: Fix error handling path in 'ioat_dma_self_test()'
2017-07-19 22:21 ` Dave Jiang
@ 2017-07-21 6:31 ` Vinod Koul
2017-07-21 6:44 ` Jiang, Dave
0 siblings, 1 reply; 11+ messages in thread
From: Vinod Koul @ 2017-07-21 6:31 UTC (permalink / raw)
To: Dave Jiang
Cc: Christophe JAILLET, Williams, Dan J, dmaengine, linux-kernel,
kernel-janitors
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] dmaengine: ioatdma: Fix error handling path in 'ioat_dma_self_test()'
2017-07-21 6:31 ` Vinod Koul
@ 2017-07-21 6:44 ` Jiang, Dave
0 siblings, 0 replies; 11+ messages in thread
From: Jiang, Dave @ 2017-07-21 6:44 UTC (permalink / raw)
To: Koul, Vinod
Cc: Christophe JAILLET, Williams, Dan J, dmaengine, linux-kernel,
kernel-janitors
> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] dmaengine: ioatdma: Fix error handling path in 'ioat_dma_self_test()'
2017-07-20 16:56 ` Dave Jiang
@ 2017-07-21 7:22 ` walter harms
2017-07-21 7:23 ` Julia Lawall
2017-07-21 7:57 ` Vinod Koul
1 sibling, 1 reply; 11+ messages in thread
From: walter harms @ 2017-07-21 7:22 UTC (permalink / raw)
To: Dave Jiang
Cc: Christophe JAILLET, Williams, Dan J, Koul, Vinod, dmaengine,
linux-kernel, kernel-janitors
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] dmaengine: ioatdma: Fix error handling path in 'ioat_dma_self_test()'
2017-07-21 7:22 ` walter harms
@ 2017-07-21 7:23 ` Julia Lawall
2017-07-21 9:38 ` Dan Carpenter
0 siblings, 1 reply; 11+ messages in thread
From: Julia Lawall @ 2017-07-21 7:23 UTC (permalink / raw)
To: walter harms
Cc: Dave Jiang, Christophe JAILLET, Williams, Dan J, Koul, Vinod,
dmaengine, linux-kernel, kernel-janitors
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
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] dmaengine: ioatdma: Fix error handling path in 'ioat_dma_self_test()'
2017-07-20 16:56 ` Dave Jiang
2017-07-21 7:22 ` walter harms
@ 2017-07-21 7:57 ` Vinod Koul
2017-07-21 16:50 ` Dave Jiang
1 sibling, 1 reply; 11+ messages in thread
From: Vinod Koul @ 2017-07-21 7:57 UTC (permalink / raw)
To: Dave Jiang
Cc: wharms, Christophe JAILLET, Williams, Dan J, dmaengine,
linux-kernel, kernel-janitors
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] dmaengine: ioatdma: Fix error handling path in 'ioat_dma_self_test()'
2017-07-21 7:23 ` Julia Lawall
@ 2017-07-21 9:38 ` Dan Carpenter
0 siblings, 0 replies; 11+ messages in thread
From: Dan Carpenter @ 2017-07-21 9:38 UTC (permalink / raw)
To: Julia Lawall
Cc: walter harms, Dave Jiang, Christophe JAILLET, Williams, Dan J,
Koul, Vinod, dmaengine, linux-kernel, kernel-janitors
I'm with Christophe. ;) I never like it when people get creative with
the last test in a series of tests.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] dmaengine: ioatdma: Fix error handling path in 'ioat_dma_self_test()'
2017-07-21 7:57 ` Vinod Koul
@ 2017-07-21 16:50 ` Dave Jiang
0 siblings, 0 replies; 11+ messages in thread
From: Dave Jiang @ 2017-07-21 16:50 UTC (permalink / raw)
To: Vinod Koul
Cc: wharms, Christophe JAILLET, Williams, Dan J, dmaengine,
linux-kernel, kernel-janitors
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-07-21 16:51 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-19 22:16 [PATCH] dmaengine: ioatdma: Fix error handling path in 'ioat_dma_self_test()' Christophe JAILLET
2017-07-19 22:21 ` Dave Jiang
2017-07-21 6:31 ` Vinod Koul
2017-07-21 6:44 ` Jiang, Dave
2017-07-20 7:24 ` walter harms
2017-07-20 16:56 ` Dave Jiang
2017-07-21 7:22 ` walter harms
2017-07-21 7:23 ` Julia Lawall
2017-07-21 9:38 ` Dan Carpenter
2017-07-21 7:57 ` Vinod Koul
2017-07-21 16:50 ` Dave Jiang
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).