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

Submitted by Tejun Heo on Jan. 14, 2010, 7:50 a.m.

Details

Message ID 4B4ECCCD.1040902@kernel.org
State Not Applicable
Delegated to: David Miller
Headers show

Commit Message

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

Comments

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 hide | download patch | download mbox

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