diff mbox

ioat: fail self-test if wait_for_completion times out

Message ID 1419763047-15414-1-git-send-email-der.herr@hofr.at (mailing list archive)
State Superseded
Headers show

Commit Message

Nicholas Mc Guire Dec. 28, 2014, 10:37 a.m. UTC 
wait_for_completion_timeout reaching timeout was being ignored,
fail the self-test if timeout condition occurs.

Not sure about the indentations used (CodingStyle:Chapter 2)

this was only compile tested with
x86_64_defconfig + CONFIG_DMA_ENGINE=y + CONFIG_INTEL_IOATDMA=y

patch is against linux-next 3.19.0-rc1 -next-20141226

Signed-off-by: Nicholas Mc Guire <der.herr@hofr.at>
---
 drivers/dma/ioat/dma_v3.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

--
1.7.10.4

--
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

Comments

Prarit Bhargava Jan. 2, 2015, 11:47 a.m. UTC | #1 
On 12/28/2014 05:37 AM, Nicholas Mc Guire wrote:
> wait_for_completion_timeout reaching timeout was being ignored,
> fail the self-test if timeout condition occurs.
> 
> Not sure about the indentations used (CodingStyle:Chapter 2)
> 
> this was only compile tested with
> x86_64_defconfig + CONFIG_DMA_ENGINE=y + CONFIG_INTEL_IOATDMA=y
> 
> patch is against linux-next 3.19.0-rc1 -next-20141226
> 

Seems straightforward to me, although I don't think I've ever seen a failure in
this code.

Acked-by: Prarit Bhargava <prarit@redhat.com>

P.

> Signed-off-by: Nicholas Mc Guire <der.herr@hofr.at>
> ---
>  drivers/dma/ioat/dma_v3.c |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/dma/ioat/dma_v3.c b/drivers/dma/ioat/dma_v3.c
> index be307182..0659215 100644
> --- a/drivers/dma/ioat/dma_v3.c
> +++ b/drivers/dma/ioat/dma_v3.c
> @@ -1311,7 +1311,8 @@ static int ioat_xor_val_self_test(struct ioatdma_device *device)
> 
>  	tmo = wait_for_completion_timeout(&cmp, msecs_to_jiffies(3000));
> 
> -	if (dma->device_tx_status(dma_chan, cookie, NULL) != DMA_COMPLETE) {
> +	if (tmo == 0 || dma->device_tx_status(dma_chan, cookie, NULL)
> +			!= DMA_COMPLETE) {
>  		dev_err(dev, "Self-test xor timed out\n");
>  		err = -ENODEV;
>  		goto dma_unmap;
> @@ -1377,7 +1378,8 @@ static int ioat_xor_val_self_test(struct ioatdma_device *device)
> 
>  	tmo = wait_for_completion_timeout(&cmp, msecs_to_jiffies(3000));
> 
> -	if (dma->device_tx_status(dma_chan, cookie, NULL) != DMA_COMPLETE) {
> +	if (tmo == 0 || dma->device_tx_status(dma_chan, cookie, NULL)
> +			!= DMA_COMPLETE) {
>  		dev_err(dev, "Self-test validate timed out\n");
>  		err = -ENODEV;
>  		goto dma_unmap;
> @@ -1429,7 +1431,8 @@ static int ioat_xor_val_self_test(struct ioatdma_device *device)
> 
>  	tmo = wait_for_completion_timeout(&cmp, msecs_to_jiffies(3000));
> 
> -	if (dma->device_tx_status(dma_chan, cookie, NULL) != DMA_COMPLETE) {
> +	if (tmo == 0 || dma->device_tx_status(dma_chan, cookie, NULL)
> +			!= DMA_COMPLETE) {
>  		dev_err(dev, "Self-test 2nd validate timed out\n");
>  		err = -ENODEV;
>  		goto dma_unmap;
> --
> 1.7.10.4
> 
--
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
Dave Jiang Jan. 5, 2015, 4:51 p.m. UTC | #2 
DQoNCg0KT24gU3VuLCAyMDE0LTEyLTI4IGF0IDEwOjM3ICswMDAwLCBOaWNob2xhcyBNYyBHdWly
ZSB3cm90ZToNCj4gd2FpdF9mb3JfY29tcGxldGlvbl90aW1lb3V0IHJlYWNoaW5nIHRpbWVvdXQg
d2FzIGJlaW5nIGlnbm9yZWQsDQo+IGZhaWwgdGhlIHNlbGYtdGVzdCBpZiB0aW1lb3V0IGNvbmRp
dGlvbiBvY2N1cnMuDQo+IA0KPiBOb3Qgc3VyZSBhYm91dCB0aGUgaW5kZW50YXRpb25zIHVzZWQg
KENvZGluZ1N0eWxlOkNoYXB0ZXIgMikNCj4gDQo+IHRoaXMgd2FzIG9ubHkgY29tcGlsZSB0ZXN0
ZWQgd2l0aA0KPiB4ODZfNjRfZGVmY29uZmlnICsgQ09ORklHX0RNQV9FTkdJTkU9eSArIENPTkZJ
R19JTlRFTF9JT0FURE1BPXkNCj4gDQo+IHBhdGNoIGlzIGFnYWluc3QgbGludXgtbmV4dCAzLjE5
LjAtcmMxIC1uZXh0LTIwMTQxMjI2DQo+IA0KPiBTaWduZWQtb2ZmLWJ5OiBOaWNob2xhcyBNYyBH
dWlyZSA8ZGVyLmhlcnJAaG9mci5hdD4NCj4gLS0tDQo+ICBkcml2ZXJzL2RtYS9pb2F0L2RtYV92
My5jIHwgICAgOSArKysrKystLS0NCj4gIDEgZmlsZSBjaGFuZ2VkLCA2IGluc2VydGlvbnMoKyks
IDMgZGVsZXRpb25zKC0pDQo+IA0KPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9kbWEvaW9hdC9kbWFf
djMuYyBiL2RyaXZlcnMvZG1hL2lvYXQvZG1hX3YzLmMNCj4gaW5kZXggYmUzMDcxODIuLjA2NTky
MTUgMTAwNjQ0DQo+IC0tLSBhL2RyaXZlcnMvZG1hL2lvYXQvZG1hX3YzLmMNCj4gKysrIGIvZHJp
dmVycy9kbWEvaW9hdC9kbWFfdjMuYw0KPiBAQCAtMTMxMSw3ICsxMzExLDggQEAgc3RhdGljIGlu
dCBpb2F0X3hvcl92YWxfc2VsZl90ZXN0KHN0cnVjdCBpb2F0ZG1hX2RldmljZSAqZGV2aWNlKQ0K
PiANCj4gIAl0bW8gPSB3YWl0X2Zvcl9jb21wbGV0aW9uX3RpbWVvdXQoJmNtcCwgbXNlY3NfdG9f
amlmZmllcygzMDAwKSk7DQo+IA0KPiAtCWlmIChkbWEtPmRldmljZV90eF9zdGF0dXMoZG1hX2No
YW4sIGNvb2tpZSwgTlVMTCkgIT0gRE1BX0NPTVBMRVRFKSB7DQo+ICsJaWYgKHRtbyA9PSAwIHx8
IGRtYS0+ZGV2aWNlX3R4X3N0YXR1cyhkbWFfY2hhbiwgY29va2llLCBOVUxMKQ0KPiArCQkJIT0g
RE1BX0NPTVBMRVRFKSB7DQo+ICAJCWRldl9lcnIoZGV2LCAiU2VsZi10ZXN0IHhvciB0aW1lZCBv
dXRcbiIpOw0KPiAgCQllcnIgPSAtRU5PREVWOw0KPiAgCQlnb3RvIGRtYV91bm1hcDsNCj4gQEAg
LTEzNzcsNyArMTM3OCw4IEBAIHN0YXRpYyBpbnQgaW9hdF94b3JfdmFsX3NlbGZfdGVzdChzdHJ1
Y3QgaW9hdGRtYV9kZXZpY2UgKmRldmljZSkNCj4gDQo+ICAJdG1vID0gd2FpdF9mb3JfY29tcGxl
dGlvbl90aW1lb3V0KCZjbXAsIG1zZWNzX3RvX2ppZmZpZXMoMzAwMCkpOw0KPiANCj4gLQlpZiAo
ZG1hLT5kZXZpY2VfdHhfc3RhdHVzKGRtYV9jaGFuLCBjb29raWUsIE5VTEwpICE9IERNQV9DT01Q
TEVURSkgew0KPiArCWlmICh0bW8gPT0gMCB8fCBkbWEtPmRldmljZV90eF9zdGF0dXMoZG1hX2No
YW4sIGNvb2tpZSwgTlVMTCkNCj4gKwkJCSE9IERNQV9DT01QTEVURSkgew0KDQpDYW4geW91IHBs
ZWFzZSBkbzogDQorCWlmICh0bW8gPT0gMCB8fA0KKwkgICAgZG1hLT5kZXZpY2VfdHhfc3RhdHVz
KGRtYV9jaGFuLCBjb29raWUsIE5VTEwpICE9IERNQV9DT01QTEVURSkgew0KDQpPdGhlcndpc2Ug
aXQgbG9va3MgZ29vZC4gQWx0aG91Z2ggSSd2ZSBuZXZlciBoaXQgdGhhdCBlcnJvciBjb25kaXRp
b24gZWl0aGVyLiANCg0KPiAgCQlkZXZfZXJyKGRldiwgIlNlbGYtdGVzdCB2YWxpZGF0ZSB0aW1l
ZCBvdXRcbiIpOw0KPiAgCQllcnIgPSAtRU5PREVWOw0KPiAgCQlnb3RvIGRtYV91bm1hcDsNCj4g
QEAgLTE0MjksNyArMTQzMSw4IEBAIHN0YXRpYyBpbnQgaW9hdF94b3JfdmFsX3NlbGZfdGVzdChz
dHJ1Y3QgaW9hdGRtYV9kZXZpY2UgKmRldmljZSkNCj4gDQo+ICAJdG1vID0gd2FpdF9mb3JfY29t
cGxldGlvbl90aW1lb3V0KCZjbXAsIG1zZWNzX3RvX2ppZmZpZXMoMzAwMCkpOw0KPiANCj4gLQlp
ZiAoZG1hLT5kZXZpY2VfdHhfc3RhdHVzKGRtYV9jaGFuLCBjb29raWUsIE5VTEwpICE9IERNQV9D
T01QTEVURSkgew0KPiArCWlmICh0bW8gPT0gMCB8fCBkbWEtPmRldmljZV90eF9zdGF0dXMoZG1h
X2NoYW4sIGNvb2tpZSwgTlVMTCkNCj4gKwkJCSE9IERNQV9DT01QTEVURSkgew0KPiAgCQlkZXZf
ZXJyKGRldiwgIlNlbGYtdGVzdCAybmQgdmFsaWRhdGUgdGltZWQgb3V0XG4iKTsNCj4gIAkJZXJy
ID0gLUVOT0RFVjsNCj4gIAkJZ290byBkbWFfdW5tYXA7DQo+IC0tDQo+IDEuNy4xMC40DQo+IA0K
--
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
Nicholas Mc Guire Jan. 6, 2015, 12:42 a.m. UTC | #3 
On Mon, 05 Jan 2015, Jiang, Dave wrote:

> 
> 
> 
> On Sun, 2014-12-28 at 10:37 +0000, Nicholas Mc Guire wrote:
> > wait_for_completion_timeout reaching timeout was being ignored,
> > fail the self-test if timeout condition occurs.
> > 
> > Not sure about the indentations used (CodingStyle:Chapter 2)
> > 
> > this was only compile tested with
> > x86_64_defconfig + CONFIG_DMA_ENGINE=y + CONFIG_INTEL_IOATDMA=y
> > 
> > patch is against linux-next 3.19.0-rc1 -next-20141226
> > 
> > Signed-off-by: Nicholas Mc Guire <der.herr@hofr.at>
> > ---
> >  drivers/dma/ioat/dma_v3.c |    9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/dma/ioat/dma_v3.c b/drivers/dma/ioat/dma_v3.c
> > index be307182..0659215 100644
> > --- a/drivers/dma/ioat/dma_v3.c
> > +++ b/drivers/dma/ioat/dma_v3.c
> > @@ -1311,7 +1311,8 @@ static int ioat_xor_val_self_test(struct ioatdma_device *device)
> > 
> >  	tmo = wait_for_completion_timeout(&cmp, msecs_to_jiffies(3000));
> > 
> > -	if (dma->device_tx_status(dma_chan, cookie, NULL) != DMA_COMPLETE) {
> > +	if (tmo == 0 || dma->device_tx_status(dma_chan, cookie, NULL)
> > +			!= DMA_COMPLETE) {
> >  		dev_err(dev, "Self-test xor timed out\n");
> >  		err = -ENODEV;
> >  		goto dma_unmap;
> > @@ -1377,7 +1378,8 @@ static int ioat_xor_val_self_test(struct ioatdma_device *device)
> > 
> >  	tmo = wait_for_completion_timeout(&cmp, msecs_to_jiffies(3000));
> > 
> > -	if (dma->device_tx_status(dma_chan, cookie, NULL) != DMA_COMPLETE) {
> > +	if (tmo == 0 || dma->device_tx_status(dma_chan, cookie, NULL)
> > +			!= DMA_COMPLETE) {
> 
> Can you please do: 
> +	if (tmo == 0 ||
> +	    dma->device_tx_status(dma_chan, cookie, NULL) != DMA_COMPLETE) {

Documentation/CodingStyle:Chapter 2

"Statements longer than 80 columns will be broken into sensible chunks, unless
exceeding 80 columns significantly increases readability and does not hide
information. Descendants are always substantially shorter than the parent and
are placed substantially to the right. The same applies to function headers..."

am I misreading the CodingStyle here ?

> 
> Otherwise it looks good. Although I've never hit that error condition either. 
> 
> >  		dev_err(dev, "Self-test validate timed out\n");
> >  		err = -ENODEV;
> >  		goto dma_unmap;
> > @@ -1429,7 +1431,8 @@ static int ioat_xor_val_self_test(struct ioatdma_device *device)
> > 
> >  	tmo = wait_for_completion_timeout(&cmp, msecs_to_jiffies(3000));
> > 
> > -	if (dma->device_tx_status(dma_chan, cookie, NULL) != DMA_COMPLETE) {
> > +	if (tmo == 0 || dma->device_tx_status(dma_chan, cookie, NULL)
> > +			!= DMA_COMPLETE) {
> >  		dev_err(dev, "Self-test 2nd validate timed out\n");
> >  		err = -ENODEV;
> >  		goto dma_unmap;
> > --
> > 1.7.10.4
> > 

thx!
hofrat
--
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
Dave Jiang Jan. 6, 2015, 3:38 p.m. UTC | #4 
On Tue, 2015-01-06 at 00:42 +0000, Nicholas Mc Guire wrote:
> On Mon, 05 Jan 2015, Jiang, Dave wrote:

> 

> > 

> > 

> > 

> > On Sun, 2014-12-28 at 10:37 +0000, Nicholas Mc Guire wrote:

> > > wait_for_completion_timeout reaching timeout was being ignored,

> > > fail the self-test if timeout condition occurs.

> > > 

> > > Not sure about the indentations used (CodingStyle:Chapter 2)

> > > 

> > > this was only compile tested with

> > > x86_64_defconfig + CONFIG_DMA_ENGINE=y + CONFIG_INTEL_IOATDMA=y

> > > 

> > > patch is against linux-next 3.19.0-rc1 -next-20141226

> > > 

> > > Signed-off-by: Nicholas Mc Guire <der.herr@hofr.at>

> > > ---

> > >  drivers/dma/ioat/dma_v3.c |    9 ++++++---

> > >  1 file changed, 6 insertions(+), 3 deletions(-)

> > > 

> > > diff --git a/drivers/dma/ioat/dma_v3.c b/drivers/dma/ioat/dma_v3.c

> > > index be307182..0659215 100644

> > > --- a/drivers/dma/ioat/dma_v3.c

> > > +++ b/drivers/dma/ioat/dma_v3.c

> > > @@ -1311,7 +1311,8 @@ static int ioat_xor_val_self_test(struct ioatdma_device *device)

> > > 

> > >  	tmo = wait_for_completion_timeout(&cmp, msecs_to_jiffies(3000));

> > > 

> > > -	if (dma->device_tx_status(dma_chan, cookie, NULL) != DMA_COMPLETE) {

> > > +	if (tmo == 0 || dma->device_tx_status(dma_chan, cookie, NULL)

> > > +			!= DMA_COMPLETE) {

> > >  		dev_err(dev, "Self-test xor timed out\n");

> > >  		err = -ENODEV;

> > >  		goto dma_unmap;

> > > @@ -1377,7 +1378,8 @@ static int ioat_xor_val_self_test(struct ioatdma_device *device)

> > > 

> > >  	tmo = wait_for_completion_timeout(&cmp, msecs_to_jiffies(3000));

> > > 

> > > -	if (dma->device_tx_status(dma_chan, cookie, NULL) != DMA_COMPLETE) {

> > > +	if (tmo == 0 || dma->device_tx_status(dma_chan, cookie, NULL)

> > > +			!= DMA_COMPLETE) {

> > 

> > Can you please do: 

> > +	if (tmo == 0 ||

> > +	    dma->device_tx_status(dma_chan, cookie, NULL) != DMA_COMPLETE) {

> 

> Documentation/CodingStyle:Chapter 2

> 

> "Statements longer than 80 columns will be broken into sensible chunks, unless

> exceeding 80 columns significantly increases readability and does not hide

> information. Descendants are always substantially shorter than the parent and

> are placed substantially to the right. The same applies to function headers..."

> 

> am I misreading the CodingStyle here ?


I'm not sure what the issue is here.... What I proposed is still the
same length as the original code. And what I suggested complies with the
existing coding style that's already there.

> 

> > 

> > Otherwise it looks good. Although I've never hit that error condition either. 

> > 

> > >  		dev_err(dev, "Self-test validate timed out\n");

> > >  		err = -ENODEV;

> > >  		goto dma_unmap;

> > > @@ -1429,7 +1431,8 @@ static int ioat_xor_val_self_test(struct ioatdma_device *device)

> > > 

> > >  	tmo = wait_for_completion_timeout(&cmp, msecs_to_jiffies(3000));

> > > 

> > > -	if (dma->device_tx_status(dma_chan, cookie, NULL) != DMA_COMPLETE) {

> > > +	if (tmo == 0 || dma->device_tx_status(dma_chan, cookie, NULL)

> > > +			!= DMA_COMPLETE) {

> > >  		dev_err(dev, "Self-test 2nd validate timed out\n");

> > >  		err = -ENODEV;

> > >  		goto dma_unmap;

> > > --

> > > 1.7.10.4

> > > 

> 

> thx!

> hofrat

> --

> 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
Prarit Bhargava Jan. 7, 2015, 12:46 p.m. UTC | #5 
On 01/06/2015 10:38 AM, Jiang, Dave wrote:
>>>> -	if (dma->device_tx_status(dma_chan, cookie, NULL) != DMA_COMPLETE) {
>>>> +	if (tmo == 0 || dma->device_tx_status(dma_chan, cookie, NULL)
>>>> +			!= DMA_COMPLETE) {
>>>
>>> Can you please do: 
>>> +	if (tmo == 0 ||
>>> +	    dma->device_tx_status(dma_chan, cookie, NULL) != DMA_COMPLETE) {
>>
>> Documentation/CodingStyle:Chapter 2
>>
>> "Statements longer than 80 columns will be broken into sensible chunks, unless
>> exceeding 80 columns significantly increases readability and does not hide
>> information. Descendants are always substantially shorter than the parent and
>> are placed substantially to the right. The same applies to function headers..."
>>
>> am I misreading the CodingStyle here ?
> 
> I'm not sure what the issue is here.... What I proposed is still the
> same length as the original code. And what I suggested complies with the
> existing coding style that's already there.

Ugh ... I missed this obvious CodingStyle error.

What Dave is trying to say is that he (and I'm pretty sure everyone else
for that matter) disagree with you style change because you have not broken
the columns into "sensible chunks".

IOW ... this,

	if (tmo == 0 ||
	    dma->device_tx_status(dma_chan, cookie, NULL) != DMA_COMPLETE) {

is much easier to comprehend than this,

	if (tmo == 0 || dma->device_tx_status(dma_chan, cookie, NULL)
			!= DMA_COMPLETE) {

P.
--
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
Nicholas Mc Guire Jan. 7, 2015, 1:09 p.m. UTC | #6 
On Wed, 07 Jan 2015, Prarit Bhargava wrote:

> 
> 
> On 01/06/2015 10:38 AM, Jiang, Dave wrote:
> >>>> -	if (dma->device_tx_status(dma_chan, cookie, NULL) != DMA_COMPLETE) {
> >>>> +	if (tmo == 0 || dma->device_tx_status(dma_chan, cookie, NULL)
> >>>> +			!= DMA_COMPLETE) {
> >>>
> >>> Can you please do: 
> >>> +	if (tmo == 0 ||
> >>> +	    dma->device_tx_status(dma_chan, cookie, NULL) != DMA_COMPLETE) {
> >>
> >> Documentation/CodingStyle:Chapter 2
> >>
> >> "Statements longer than 80 columns will be broken into sensible chunks, unless
> >> exceeding 80 columns significantly increases readability and does not hide
> >> information. Descendants are always substantially shorter than the parent and
> >> are placed substantially to the right. The same applies to function headers..."
> >>
> >> am I misreading the CodingStyle here ?
> > 
> > I'm not sure what the issue is here.... What I proposed is still the
> > same length as the original code. And what I suggested complies with the
> > existing coding style that's already there.
> 
> Ugh ... I missed this obvious CodingStyle error.
> 
> What Dave is trying to say is that he (and I'm pretty sure everyone else
> for that matter) disagree with you style change because you have not broken
> the columns into "sensible chunks".
> 
> IOW ... this,
> 
> 	if (tmo == 0 ||
> 	    dma->device_tx_status(dma_chan, cookie, NULL) != DMA_COMPLETE) {
> 
> is much easier to comprehend than this,
> 
> 	if (tmo == 0 || dma->device_tx_status(dma_chan, cookie, NULL)
> 			!= DMA_COMPLETE) {
>
agreed - it was just not clear to me how strict things should
be applied - e.g. the indentation with spaces - will cleanup and
resend.

thx!
hofrat 
--
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
Dave Jiang Jan. 7, 2015, 4:22 p.m. UTC | #7 
On Wed, 2015-01-07 at 13:09 +0000, Nicholas Mc Guire wrote:
> On Wed, 07 Jan 2015, Prarit Bhargava wrote:

> 

> > 

> > 

> > On 01/06/2015 10:38 AM, Jiang, Dave wrote:

> > >>>> -	if (dma->device_tx_status(dma_chan, cookie, NULL) != DMA_COMPLETE) {

> > >>>> +	if (tmo == 0 || dma->device_tx_status(dma_chan, cookie, NULL)

> > >>>> +			!= DMA_COMPLETE) {

> > >>>

> > >>> Can you please do: 

> > >>> +	if (tmo == 0 ||

> > >>> +	    dma->device_tx_status(dma_chan, cookie, NULL) != DMA_COMPLETE) {

> > >>

> > >> Documentation/CodingStyle:Chapter 2

> > >>

> > >> "Statements longer than 80 columns will be broken into sensible chunks, unless

> > >> exceeding 80 columns significantly increases readability and does not hide

> > >> information. Descendants are always substantially shorter than the parent and

> > >> are placed substantially to the right. The same applies to function headers..."

> > >>

> > >> am I misreading the CodingStyle here ?

> > > 

> > > I'm not sure what the issue is here.... What I proposed is still the

> > > same length as the original code. And what I suggested complies with the

> > > existing coding style that's already there.

> > 

> > Ugh ... I missed this obvious CodingStyle error.

> > 

> > What Dave is trying to say is that he (and I'm pretty sure everyone else

> > for that matter) disagree with you style change because you have not broken

> > the columns into "sensible chunks".

> > 

> > IOW ... this,

> > 

> > 	if (tmo == 0 ||

> > 	    dma->device_tx_status(dma_chan, cookie, NULL) != DMA_COMPLETE) {

> > 

> > is much easier to comprehend than this,

> > 

> > 	if (tmo == 0 || dma->device_tx_status(dma_chan, cookie, NULL)

> > 			!= DMA_COMPLETE) {

> >

> agreed - it was just not clear to me how strict things should

> be applied - e.g. the indentation with spaces - will cleanup and

> resend.


Thank you. In general as a rule of thumb, apply coding style that's
consistent with the code that's already there. That makes it easy for
readability because it does not deviate from the whole.
diff mbox

Patch

diff --git a/drivers/dma/ioat/dma_v3.c b/drivers/dma/ioat/dma_v3.c
index be307182..0659215 100644
--- a/drivers/dma/ioat/dma_v3.c
+++ b/drivers/dma/ioat/dma_v3.c
@@ -1311,7 +1311,8 @@  static int ioat_xor_val_self_test(struct ioatdma_device *device)

 	tmo = wait_for_completion_timeout(&cmp, msecs_to_jiffies(3000));

-	if (dma->device_tx_status(dma_chan, cookie, NULL) != DMA_COMPLETE) {
+	if (tmo == 0 || dma->device_tx_status(dma_chan, cookie, NULL)
+			!= DMA_COMPLETE) {
 		dev_err(dev, "Self-test xor timed out\n");
 		err = -ENODEV;
 		goto dma_unmap;
@@ -1377,7 +1378,8 @@  static int ioat_xor_val_self_test(struct ioatdma_device *device)

 	tmo = wait_for_completion_timeout(&cmp, msecs_to_jiffies(3000));

-	if (dma->device_tx_status(dma_chan, cookie, NULL) != DMA_COMPLETE) {
+	if (tmo == 0 || dma->device_tx_status(dma_chan, cookie, NULL)
+			!= DMA_COMPLETE) {
 		dev_err(dev, "Self-test validate timed out\n");
 		err = -ENODEV;
 		goto dma_unmap;
@@ -1429,7 +1431,8 @@  static int ioat_xor_val_self_test(struct ioatdma_device *device)

 	tmo = wait_for_completion_timeout(&cmp, msecs_to_jiffies(3000));

-	if (dma->device_tx_status(dma_chan, cookie, NULL) != DMA_COMPLETE) {
+	if (tmo == 0 || dma->device_tx_status(dma_chan, cookie, NULL)
+			!= DMA_COMPLETE) {
 		dev_err(dev, "Self-test 2nd validate timed out\n");
 		err = -ENODEV;
 		goto dma_unmap;