If the 'memcmp' fails, free allocated resources as done in all other
error handling paths.
Signed-off-by: Christophe JAILLET <[email protected]>
---
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
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 <[email protected]>
Good catch! Thanks.
Signed-off-by: Dave Jiang <[email protected]>
> ---
> 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:
>
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>> ---
>> 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
>
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 <[email protected]>
You meant acked right..?
>
> Good catch! Thanks.
>
> Signed-off-by: Dave Jiang <[email protected]>
>
> > ---
> > 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
> On Jul 20, 2017, at 11:28 PM, Koul, Vinod <[email protected]> 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 <[email protected]>
>
> You meant acked right..?
Yes. Typo :)
>
>>
>> Good catch! Thanks.
>>
>> Signed-off-by: Dave Jiang <[email protected]>
>>
>>> ---
>>> 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
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 <[email protected]>
>>> ---
>>> 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
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 <[email protected]>
> >>> ---
> >>> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
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 <[email protected]>
> >> ---
> >> 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
I'm with Christophe. ;) I never like it when people get creative with
the last test in a series of tests.
regards,
dan carpenter
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 <[email protected]>
>>>> ---
>>>> 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.