| Message ID | 20170719221648.30253-1-christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
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: > -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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: > >
> 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 -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org 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 <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'm with Christophe. ;) I never like it when people get creative with the last test in a series of tests. regards, dan carpenter -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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:
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(-)