Patchwork [#upstream-fixes,1/2] libata: cleanup ata_sff_interrupt()

login
register
mail settings
Submitter Tejun Heo
Date Jan. 14, 2010, 7:50 a.m.
Message ID <4B4ECCCD.1040902@kernel.org>
Download mbox | patch
Permalink /patch/42852/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Tejun Heo - Jan. 14, 2010, 7:50 a.m.
host->ports[i] is never NULL if i < host->n_ports and non-NULL return
from ata_qc_from_tag() guarantees that the returned qc is active.
Drop unnecessary tests.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 drivers/ata/libata-sff.c |   17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov - Jan. 14, 2010, 10:20 a.m.
Hello.

Tejun Heo wrote:

> host->ports[i] is never NULL if i < host->n_ports and non-NULL return
> from ata_qc_from_tag() guarantees that the returned qc is active.
> Drop unnecessary tests.
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> ---
>  drivers/ata/libata-sff.c |   17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
>
> Index: ata/drivers/ata/libata-sff.c
> ===================================================================
> --- ata.orig/drivers/ata/libata-sff.c
> +++ ata/drivers/ata/libata-sff.c
> @@ -1767,18 +1767,15 @@ irqreturn_t ata_sff_interrupt(int irq, v
>  	spin_lock_irqsave(&host->lock, flags);
>  
>  	for (i = 0; i < host->n_ports; i++) {
> -		struct ata_port *ap;
> +		struct ata_port *ap = host->ports[i];
> +		struct ata_queued_cmd *qc;
>  
> -		ap = host->ports[i];
> -		if (ap &&
> -		    !(ap->flags & ATA_FLAG_DISABLED)) {
> -			struct ata_queued_cmd *qc;
> +		if (unlikely(ap->flags & ATA_FLAG_DISABLED))
> +			continue;
>  
> -			qc = ata_qc_from_tag(ap, ap->link.active_tag);
> -			if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING)) &&
> -			    (qc->flags & ATA_QCFLAG_ACTIVE))
> -				handled |= ata_sff_host_intr(ap, qc);
> -		}
> +		qc = ata_qc_from_tag(ap, ap->link.active_tag);
> +		if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING)))
>   

   () not needed around !x.

WBR, Sergei


--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Garzik - Jan. 14, 2010, 5:38 p.m.
On 01/14/2010 02:50 AM, Tejun Heo wrote:
> host->ports[i] is never NULL if i<  host->n_ports and non-NULL return
> from ata_qc_from_tag() guarantees that the returned qc is active.
> Drop unnecessary tests.
>
> Signed-off-by: Tejun Heo<tj@kernel.org>
> ---
>   drivers/ata/libata-sff.c |   17 +++++++----------
>   1 file changed, 7 insertions(+), 10 deletions(-)

This is more #upstream material, don't you think?  We are pretty deep 
into -rc at this point.

(headed out for the weekend, fyi)

	Jeff



--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo - Jan. 15, 2010, 3:33 a.m.
On 01/15/2010 02:38 AM, Jeff Garzik wrote:
> On 01/14/2010 02:50 AM, Tejun Heo wrote:
>> host->ports[i] is never NULL if i<  host->n_ports and non-NULL return
>> from ata_qc_from_tag() guarantees that the returned qc is active.
>> Drop unnecessary tests.
>>
>> Signed-off-by: Tejun Heo<tj@kernel.org>
>> ---
>>   drivers/ata/libata-sff.c |   17 +++++++----------
>>   1 file changed, 7 insertions(+), 10 deletions(-)
> 
> This is more #upstream material, don't you think?  We are pretty deep
> into -rc at this point.

Yeap, agreed.

Thanks.
Tejun Heo - Jan. 15, 2010, 3:36 a.m.
On 01/14/2010 07:20 PM, Sergei Shtylyov wrote:
>> +        qc = ata_qc_from_tag(ap, ap->link.active_tag);
>> +        if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING)))
>>   
> 
>   () not needed around !x.

Eh... I don't know.  I personally prefer using minimum number of
parantheses but quite a few people dislike not using them when && & ||
| are mixed.  In the end, I don't think it really matters.  It's like
debating about how far the second part of a broken line should be
indented.  Many people strong opinions but in the end it doesn't
really matter.  Anyhow, if it bothers you enough to write an email
about it, I'll be happy to oblige.  :-)

Thanks.

Patch

Index: ata/drivers/ata/libata-sff.c
===================================================================
--- ata.orig/drivers/ata/libata-sff.c
+++ ata/drivers/ata/libata-sff.c
@@ -1767,18 +1767,15 @@  irqreturn_t ata_sff_interrupt(int irq, v
 	spin_lock_irqsave(&host->lock, flags);
 
 	for (i = 0; i < host->n_ports; i++) {
-		struct ata_port *ap;
+		struct ata_port *ap = host->ports[i];
+		struct ata_queued_cmd *qc;
 
-		ap = host->ports[i];
-		if (ap &&
-		    !(ap->flags & ATA_FLAG_DISABLED)) {
-			struct ata_queued_cmd *qc;
+		if (unlikely(ap->flags & ATA_FLAG_DISABLED))
+			continue;
 
-			qc = ata_qc_from_tag(ap, ap->link.active_tag);
-			if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING)) &&
-			    (qc->flags & ATA_QCFLAG_ACTIVE))
-				handled |= ata_sff_host_intr(ap, qc);
-		}
+		qc = ata_qc_from_tag(ap, ap->link.active_tag);
+		if (qc && (!(qc->tf.flags & ATA_TFLAG_POLLING)))
+			handled |= ata_sff_host_intr(ap, qc);
 	}
 
 	spin_unlock_irqrestore(&host->lock, flags);