Patchwork libata: reduce ata_pci_[sff,bmdma]_init_one() size

login
register
mail settings
Submitter Bartlomiej Zolnierkiewicz
Date Feb. 9, 2011, 2:15 p.m.
Message ID <201102091515.22677.bzolnier@gmail.com>
Download mbox | patch
Permalink /patch/82472/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Bartlomiej Zolnierkiewicz - Feb. 9, 2011, 2:15 p.m.
Turn both helpers (which are used only during LLDs initialization
time and thus are not performance sensitive) into wrappers around
the new ata_pci_init_one() function, this cuts 20 LOC and saves
~1.1k of the output code size (x86-64):

 text    data     bss     dec     hex filename
  21392       0      19   21411    53a3 drivers/ata/libata-sff.o.before
  20256       0      19   20275    4f33 drivers/ata/libata-sff.o.after

Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ata/libata-sff.c |  112 +++++++++++++++++++----------------------------
 1 file changed, 46 insertions(+), 66 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 - Feb. 9, 2011, 8:08 p.m.
Hello.

Bartlomiej Zolnierkiewicz wrote:

> Turn both helpers (which are used only during LLDs initialization
> time and thus are not performance sensitive) into wrappers around
> the new ata_pci_init_one() function, this cuts 20 LOC and saves
> ~1.1k of the output code size (x86-64):

>  text    data     bss     dec     hex filename
>   21392       0      19   21411    53a3 drivers/ata/libata-sff.o.before
>   20256       0      19   20275    4f33 drivers/ata/libata-sff.o.after

> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> ---
>  drivers/ata/libata-sff.c |  112 +++++++++++++++++++----------------------------
>  1 file changed, 46 insertions(+), 66 deletions(-)

> Index: b/drivers/ata/libata-sff.c
> ===================================================================
> --- a/drivers/ata/libata-sff.c
> +++ b/drivers/ata/libata-sff.c
[...]
> @@ -2538,14 +2517,22 @@ int ata_pci_sff_init_one(struct pci_dev
>  	if (rc)
>  		goto out;
>  
> -	/* prepare and activate SFF host */
> -	rc = ata_pci_sff_prepare_host(pdev, ppi, &host);
> +	if (bmdma)
> +		/* prepare and activate BMDMA host */
> +		rc = ata_pci_bmdma_prepare_host(pdev, ppi, &host);
> +	else
> +		/* prepare and activate SFF host */
> +		rc = ata_pci_sff_prepare_host(pdev, ppi, &host);
>  	if (rc)
>  		goto out;
>  	host->private_data = host_priv;
> -	host->flags |= hflag;
> +	host->flags |= hflags;
>  
> -	rc = ata_pci_sff_activate_host(host, ata_sff_interrupt, sht);
> +	if (bmdma) {
> +		pci_set_master(pdev);
> +		rc = ata_pci_sff_activate_host(host, ata_bmdma_interrupt, sht);
> +	} else
> +		rc = ata_pci_sff_activate_host(host, ata_sff_interrupt, sht);

    Perhaps this will save even more:

	if (bmdma)
		pci_set_master(pdev);
	rc = ata_pci_sff_activate_host(host, bmdma ? ata_bmdma_interrupt :
						     ata_sff_interrupt, sht);

> @@ -2554,6 +2541,35 @@ out:
>  
>  	return rc;
>  }
> +
> +/**
> + *	ata_pci_sff_init_one - Initialize/register PIO-only PCI IDE controller
> + *	@pdev: Controller to be initialized
> + *	@ppi: array of port_info, must be enough for two ports
> + *	@sht: scsi_host_template to use when registering the host
> + *	@host_priv: host private_data
> + *	@hflag: host flags
> + *
> + *	This is a helper function which can be called from a driver's
> + *	xxx_init_one() probe function if the hardware uses traditional
> + *	IDE taskfile registers and is PIO only.
> + *
> + *	ASSUMPTION:
> + *	Nobody makes a single channel controller that appears solely as
> + *	the secondary legacy port on PCI.
> + *
> + *	LOCKING:
> + *	Inherited from PCI layer (may sleep).
> + *
> + *	RETURNS:
> + *	Zero on success, negative on errno-based value on error.
> + */
> +int ata_pci_sff_init_one(struct pci_dev *pdev,
> +		 const struct ata_port_info * const *ppi,
> +		 struct scsi_host_template *sht, void *host_priv, int hflag)

    s/hflag/hflags/ for consistency?

> +{
> +	return ata_pci_init_one(pdev, ppi, sht, host_priv, hflag, 0);

    s/0/false/?

> +}
>  EXPORT_SYMBOL_GPL(ata_pci_sff_init_one);
>  
>  #endif /* CONFIG_PCI */
> @@ -3272,43 +3288,7 @@ int ata_pci_bmdma_init_one(struct pci_de
>  			   struct scsi_host_template *sht, void *host_priv,
>  			   int hflags)
>  {
[...]
> +	return ata_pci_init_one(pdev, ppi, sht, host_priv, hflags, 1);

    s/1/true/?

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
Alan Cox - Feb. 9, 2011, 9:17 p.m.
>     Perhaps this will save even more:
> 
> 	if (bmdma)
> 		pci_set_master(pdev);
> 	rc = ata_pci_sff_activate_host(host, bmdma ? ata_bmdma_interrupt :
> 						     ata_sff_interrupt, sht);

gcc is usually smart enough to work that out - and it's definitely easier
to read as Bartlomiej has it.

I agree with the comment that libata-sata rather than ifdefs would be
good for the SATA bit as well btw.

And yes ADMA was designed for PATA
--
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

Patch

Index: b/drivers/ata/libata-sff.c
===================================================================
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -2491,31 +2491,10 @@  static const struct ata_port_info *ata_s
 	return NULL;
 }
 
-/**
- *	ata_pci_sff_init_one - Initialize/register PIO-only PCI IDE controller
- *	@pdev: Controller to be initialized
- *	@ppi: array of port_info, must be enough for two ports
- *	@sht: scsi_host_template to use when registering the host
- *	@host_priv: host private_data
- *	@hflag: host flags
- *
- *	This is a helper function which can be called from a driver's
- *	xxx_init_one() probe function if the hardware uses traditional
- *	IDE taskfile registers and is PIO only.
- *
- *	ASSUMPTION:
- *	Nobody makes a single channel controller that appears solely as
- *	the secondary legacy port on PCI.
- *
- *	LOCKING:
- *	Inherited from PCI layer (may sleep).
- *
- *	RETURNS:
- *	Zero on success, negative on errno-based value on error.
- */
-int ata_pci_sff_init_one(struct pci_dev *pdev,
-		 const struct ata_port_info * const *ppi,
-		 struct scsi_host_template *sht, void *host_priv, int hflag)
+static int ata_pci_init_one(struct pci_dev *pdev,
+		const struct ata_port_info * const *ppi,
+		struct scsi_host_template *sht, void *host_priv,
+		int hflags, bool bmdma)
 {
 	struct device *dev = &pdev->dev;
 	const struct ata_port_info *pi;
@@ -2538,14 +2517,22 @@  int ata_pci_sff_init_one(struct pci_dev
 	if (rc)
 		goto out;
 
-	/* prepare and activate SFF host */
-	rc = ata_pci_sff_prepare_host(pdev, ppi, &host);
+	if (bmdma)
+		/* prepare and activate BMDMA host */
+		rc = ata_pci_bmdma_prepare_host(pdev, ppi, &host);
+	else
+		/* prepare and activate SFF host */
+		rc = ata_pci_sff_prepare_host(pdev, ppi, &host);
 	if (rc)
 		goto out;
 	host->private_data = host_priv;
-	host->flags |= hflag;
+	host->flags |= hflags;
 
-	rc = ata_pci_sff_activate_host(host, ata_sff_interrupt, sht);
+	if (bmdma) {
+		pci_set_master(pdev);
+		rc = ata_pci_sff_activate_host(host, ata_bmdma_interrupt, sht);
+	} else
+		rc = ata_pci_sff_activate_host(host, ata_sff_interrupt, sht);
 out:
 	if (rc == 0)
 		devres_remove_group(&pdev->dev, NULL);
@@ -2554,6 +2541,35 @@  out:
 
 	return rc;
 }
+
+/**
+ *	ata_pci_sff_init_one - Initialize/register PIO-only PCI IDE controller
+ *	@pdev: Controller to be initialized
+ *	@ppi: array of port_info, must be enough for two ports
+ *	@sht: scsi_host_template to use when registering the host
+ *	@host_priv: host private_data
+ *	@hflag: host flags
+ *
+ *	This is a helper function which can be called from a driver's
+ *	xxx_init_one() probe function if the hardware uses traditional
+ *	IDE taskfile registers and is PIO only.
+ *
+ *	ASSUMPTION:
+ *	Nobody makes a single channel controller that appears solely as
+ *	the secondary legacy port on PCI.
+ *
+ *	LOCKING:
+ *	Inherited from PCI layer (may sleep).
+ *
+ *	RETURNS:
+ *	Zero on success, negative on errno-based value on error.
+ */
+int ata_pci_sff_init_one(struct pci_dev *pdev,
+		 const struct ata_port_info * const *ppi,
+		 struct scsi_host_template *sht, void *host_priv, int hflag)
+{
+	return ata_pci_init_one(pdev, ppi, sht, host_priv, hflag, 0);
+}
 EXPORT_SYMBOL_GPL(ata_pci_sff_init_one);
 
 #endif /* CONFIG_PCI */
@@ -3272,43 +3288,7 @@  int ata_pci_bmdma_init_one(struct pci_de
 			   struct scsi_host_template *sht, void *host_priv,
 			   int hflags)
 {
-	struct device *dev = &pdev->dev;
-	const struct ata_port_info *pi;
-	struct ata_host *host = NULL;
-	int rc;
-
-	DPRINTK("ENTER\n");
-
-	pi = ata_sff_find_valid_pi(ppi);
-	if (!pi) {
-		dev_printk(KERN_ERR, &pdev->dev,
-			   "no valid port_info specified\n");
-		return -EINVAL;
-	}
-
-	if (!devres_open_group(dev, NULL, GFP_KERNEL))
-		return -ENOMEM;
-
-	rc = pcim_enable_device(pdev);
-	if (rc)
-		goto out;
-
-	/* prepare and activate BMDMA host */
-	rc = ata_pci_bmdma_prepare_host(pdev, ppi, &host);
-	if (rc)
-		goto out;
-	host->private_data = host_priv;
-	host->flags |= hflags;
-
-	pci_set_master(pdev);
-	rc = ata_pci_sff_activate_host(host, ata_bmdma_interrupt, sht);
- out:
-	if (rc == 0)
-		devres_remove_group(&pdev->dev, NULL);
-	else
-		devres_release_group(&pdev->dev, NULL);
-
-	return rc;
+	return ata_pci_init_one(pdev, ppi, sht, host_priv, hflags, 1);
 }
 EXPORT_SYMBOL_GPL(ata_pci_bmdma_init_one);