diff mbox

[1/3] libata: pass host flags into __ata_pci_sff_init_one() helper

Message ID 20100218185922.16594.98925.sendpatchset@localhost
State Not Applicable
Delegated to: David Miller
Headers show

Commit Message

Bartlomiej Zolnierkiewicz Feb. 18, 2010, 6:59 p.m. UTC 
From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Subject: [PATCH] libata: pass host flags into __ata_pci_sff_init_one() helper

This was orginally proposed by Alan Cox but as a change
for ata_pci_sff_init_one() helper function instead of
__ata_pci_sff_init_one() one which casues needless churn
to all host drivers and accidentally breakes few host
drivers which are still on their way upstream.

Allows parallel scan and the like to be set without
having to stop using the existing full helper functions.

Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ata/libata-sff.c    |    4 +++-
 drivers/ata/pata_ali.c      |    2 +-
 drivers/ata/pata_amd.c      |    2 +-
 drivers/ata/pata_artop.c    |    2 +-
 drivers/ata/pata_atiixp.c   |   18 ++----------------
 drivers/ata/pata_cmd640.c   |    2 +-
 drivers/ata/pata_cs5530.c   |    2 +-
 drivers/ata/pata_efar.c     |   18 ++----------------
 drivers/ata/pata_hpt366.c   |    2 +-
 drivers/ata/pata_it821x.c   |    2 +-
 drivers/ata/pata_ns87415.c  |    2 +-
 drivers/ata/pata_sl82c105.c |    2 +-
 include/linux/libata.h      |    4 ++--
 13 files changed, 18 insertions(+), 44 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

Alan Cox Feb. 18, 2010, 9:44 p.m. UTC | #1 
On Thu, 18 Feb 2010 19:59:22 +0100
Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:

> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Subject: [PATCH] libata: pass host flags into __ata_pci_sff_init_one() helper
> 
> This was orginally proposed by Alan Cox but as a change
> for ata_pci_sff_init_one() helper function instead of
> __ata_pci_sff_init_one() one which casues needless churn
> to all host drivers and accidentally breakes few host
> drivers which are still on their way upstream.
> 
> Allows parallel scan and the like to be set without
> having to stop using the existing full helper functions.

NAK - __ is for internal symbol names.

I was split 50/50 on adding ata_pci_sff_init_one_flags() or similar but
the churn, given its a one off and we can then add all sorts of other
future flags without pain, seemed worth it.

I'm ambivalent about whether its best to go with a new function name as
you have or take the hit now (which seems sensible to me). Either way the
__ naming is wrong for an external interface.

Anyway I'd *hope* we can get > 50% of interfaces parallel scanning at
which point it ceases to be more noise anyway !

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
Bartlomiej Zolnierkiewicz Feb. 18, 2010, 10:41 p.m. UTC | #2 
On Thursday 18 February 2010 10:44:48 pm Alan Cox wrote:
> On Thu, 18 Feb 2010 19:59:22 +0100
> Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:
> 
> > From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> > Subject: [PATCH] libata: pass host flags into __ata_pci_sff_init_one() helper
> > 
> > This was orginally proposed by Alan Cox but as a change
> > for ata_pci_sff_init_one() helper function instead of
> > __ata_pci_sff_init_one() one which casues needless churn
> > to all host drivers and accidentally breakes few host
> > drivers which are still on their way upstream.
> > 
> > Allows parallel scan and the like to be set without
> > having to stop using the existing full helper functions.
> 
> NAK - __ is for internal symbol names.

A NAK for an atang specific patch?  :)

- Please read DISCLAIMER in the first mail (it is there for a valid reasons).

- The patch is for atang tree and said __ change happened _months_ ago as
   a part of (very much related) ->init_host work which is not 'upstream'.

- __ has been also used for exactly such purpose in the kernel, i.e.:

$ grep EXPORT_SYMBOL drivers/pci/*c|grep __
drivers/pci/htirq.c:EXPORT_SYMBOL(__ht_create_irq);
drivers/pci/pci.c:EXPORT_SYMBOL_GPL(__pci_complete_power_transition);
drivers/pci/pci.c:EXPORT_SYMBOL_GPL(__pci_reset_function);
drivers/pci/pci-driver.c:EXPORT_SYMBOL(__pci_register_driver);

Just see commit 863b18f introducing __pci_register_driver
(please notice that the change was suggested by Al Viro!)..

--
Bartlomiej Zolnierkiewicz
--
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. 18, 2010, 11:33 p.m. UTC | #3 
> - __ has been also used for exactly such purpose in the kernel, i.e.:

As helpers for inlines and like the yes... 

Matter of style anyway.
--
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 Feb. 19, 2010, midnight UTC | #4 
On 02/18/2010 04:44 PM, Alan Cox wrote:
> On Thu, 18 Feb 2010 19:59:22 +0100
> Bartlomiej Zolnierkiewicz<bzolnier@gmail.com>  wrote:
>
>> From: Bartlomiej Zolnierkiewicz<bzolnier@gmail.com>
>> Subject: [PATCH] libata: pass host flags into __ata_pci_sff_init_one() helper
>>
>> This was orginally proposed by Alan Cox but as a change
>> for ata_pci_sff_init_one() helper function instead of
>> __ata_pci_sff_init_one() one which casues needless churn
>> to all host drivers and accidentally breakes few host
>> drivers which are still on their way upstream.
>>
>> Allows parallel scan and the like to be set without
>> having to stop using the existing full helper functions.
>
> NAK - __ is for internal symbol names.
>
> I was split 50/50 on adding ata_pci_sff_init_one_flags() or similar but
> the churn, given its a one off and we can then add all sorts of other
> future flags without pain, seemed worth it.
>
> I'm ambivalent about whether its best to go with a new function name as
> you have or take the hit now (which seems sensible to me). Either way the
> __ naming is wrong for an external interface.

Your proposed patch from yesterday adding flags to each is fine. 
All-driver patches are just fine, as long as the need is demonstrated 
[and it is, in this case].


> Anyway I'd *hope* we can get>  50% of interfaces parallel scanning at
> which point it ceases to be more noise anyway !
>
> Jeff ?

Most hardware should support parallel scanning, sans caveats like 
chipsets that snoop SET FEATURES - XFER MODE (that command, and only 
that command, needs synchronization)

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

Patch

Index: b/drivers/ata/libata-sff.c
===================================================================
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -3009,6 +3009,7 @@  EXPORT_SYMBOL_GPL(ata_pci_sff_activate_h
  *	@sht: scsi_host_template to use when registering the host
  *	@host_priv: host private_data
  *	@init_host: host initialization method (optional)
+ *	@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
@@ -3031,7 +3032,7 @@  EXPORT_SYMBOL_GPL(ata_pci_sff_activate_h
 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 (*init_host)(struct device *dev))
+			 int (*init_host)(struct device *dev), int hflag)
 {
 	struct device *dev = &pdev->dev;
 	const struct ata_port_info *pi = NULL;
@@ -3068,6 +3069,7 @@  int __ata_pci_sff_init_one(struct pci_de
 
 	host->private_data = host_priv;
 	host->init_host = init_host;
+	host->flags |= hflag;
 
 	pci_set_master(pdev);
 	rc = ata_pci_sff_activate_host(host, ata_sff_interrupt, sht);
Index: b/drivers/ata/pata_ali.c
===================================================================
--- a/drivers/ata/pata_ali.c
+++ b/drivers/ata/pata_ali.c
@@ -316,7 +316,7 @@  static int ali_init_one(struct pci_dev *
 	}
 
 	return __ata_pci_sff_init_one(pdev, ppi, &ali_sht, NULL,
-				      ali_init_chipset);
+				      ali_init_chipset, 0);
 }
 
 static const struct pci_device_id ali[] = {
Index: b/drivers/ata/pata_amd.c
===================================================================
--- a/drivers/ata/pata_amd.c
+++ b/drivers/ata/pata_amd.c
@@ -494,7 +494,7 @@  static int amd_init_one(struct pci_dev *
 	}
 
 	/* And fire it up */
-	return __ata_pci_sff_init_one(pdev, ppi, &amd_sht, hpriv, amd_fixup);
+	return __ata_pci_sff_init_one(pdev, ppi, &amd_sht, hpriv, amd_fixup, 0);
 }
 
 static const struct pci_device_id amd[] = {
Index: b/drivers/ata/pata_artop.c
===================================================================
--- a/drivers/ata/pata_artop.c
+++ b/drivers/ata/pata_artop.c
@@ -177,7 +177,7 @@  static int artop_init_one (struct pci_de
 	atp8xx_fixup(&pdev->dev);
 
 	return __ata_pci_sff_init_one(pdev, ppi, &artop_sht, NULL,
-				      atp8xx_fixup);
+				      atp8xx_fixup, 0);
 }
 
 static const struct pci_device_id artop_pci_tbl[] = {
Index: b/drivers/ata/pata_atiixp.c
===================================================================
--- a/drivers/ata/pata_atiixp.c
+++ b/drivers/ata/pata_atiixp.c
@@ -77,23 +77,9 @@  static int atiixp_init_one(struct pci_de
 		.port_ops = &atiixp_port_ops
 	};
 	const struct ata_port_info *ppi[] = { &info, &info };
-	struct ata_host *host;
-	int rc;
 
-	/* enable device and prepare host */
-	rc = pcim_enable_device(pdev);
-	if (rc)
-		return rc;
-
-	rc = ata_pci_sff_prepare_host(pdev, ppi, &host);
-	if (rc)
-		return rc;
-
-	host->flags |= ATA_HOST_PARALLEL_SCAN;
-
-	pci_set_master(pdev);
-
-	return ata_pci_sff_activate_host(host, ata_sff_interrupt, &atiixp_sht);
+	return __ata_pci_sff_init_one(pdev, ppi, &atiixp_sht, NULL, NULL,
+				      ATA_HOST_PARALLEL_SCAN);
 }
 
 static const struct pci_device_id atiixp[] = {
Index: b/drivers/ata/pata_cmd640.c
===================================================================
--- a/drivers/ata/pata_cmd640.c
+++ b/drivers/ata/pata_cmd640.c
@@ -226,7 +226,7 @@  static int cmd640_init_one(struct pci_de
 	cmd640_hardware_init(&pdev->dev);
 
 	return __ata_pci_sff_init_one(pdev, ppi, &cmd640_sht, NULL,
-				      cmd640_hardware_init);
+				      cmd640_hardware_init, 0);
 }
 
 static const struct pci_device_id cmd640[] = {
Index: b/drivers/ata/pata_cs5530.c
===================================================================
--- a/drivers/ata/pata_cs5530.c
+++ b/drivers/ata/pata_cs5530.c
@@ -150,7 +150,7 @@  static int cs5530_init_one(struct pci_de
 
 	/* Now kick off ATA set up */
 	return __ata_pci_sff_init_one(pdev, ppi, &cs5530_sht, NULL,
-				      cs5530_init_chip);
+				      cs5530_init_chip, 0);
 }
 
 static const struct pci_device_id cs5530[] = {
Index: b/drivers/ata/pata_efar.c
===================================================================
--- a/drivers/ata/pata_efar.c
+++ b/drivers/ata/pata_efar.c
@@ -89,27 +89,13 @@  static int efar_init_one (struct pci_dev
 		.port_ops	= &efar_ops,
 	};
 	const struct ata_port_info *ppi[] = { &info, &info };
-	struct ata_host *host;
-	int rc;
 
 	if (!printed_version++)
 		dev_printk(KERN_DEBUG, &pdev->dev,
 			   "version " DRV_VERSION "\n");
 
-	/* enable device and prepare host */
-	rc = pcim_enable_device(pdev);
-	if (rc)
-		return rc;
-
-	rc = ata_pci_sff_prepare_host(pdev, ppi, &host);
-	if (rc)
-		return rc;
-
-	host->flags |= ATA_HOST_PARALLEL_SCAN;
-
-	pci_set_master(pdev);
-
-	return ata_pci_sff_activate_host(host, ata_sff_interrupt, &efar_sht);
+	return __ata_pci_sff_init_one(pdev, ppi, &efar_sht, NULL, NULL,
+				      ATA_HOST_PARALLEL_SCAN);
 }
 
 static const struct pci_device_id efar_pci_tbl[] = {
Index: b/drivers/ata/pata_hpt366.c
===================================================================
--- a/drivers/ata/pata_hpt366.c
+++ b/drivers/ata/pata_hpt366.c
@@ -413,7 +413,7 @@  static int hpt36x_init_one(struct pci_de
 	}
 	/* Now kick off ATA set up */
 	return __ata_pci_sff_init_one(dev, ppi, &hpt36x_sht, hpriv,
-				      hpt36x_init_chipset);
+				      hpt36x_init_chipset, 0);
 }
 
 static const struct pci_device_id hpt36x[] = {
Index: b/drivers/ata/pata_it821x.c
===================================================================
--- a/drivers/ata/pata_it821x.c
+++ b/drivers/ata/pata_it821x.c
@@ -625,7 +625,7 @@  static int it821x_init_one(struct pci_de
 			ppi[0] = &info_smart;
 	}
 	return __ata_pci_sff_init_one(pdev, ppi, &it821x_sht, NULL,
-				      it821x_fixup);
+				      it821x_fixup, 0);
 }
 
 static const struct pci_device_id it821x[] = {
Index: b/drivers/ata/pata_ns87415.c
===================================================================
--- a/drivers/ata/pata_ns87415.c
+++ b/drivers/ata/pata_ns87415.c
@@ -385,7 +385,7 @@  static int ns87415_init_one (struct pci_
 	ns87415_fixup(&pdev->dev);
 
 	return __ata_pci_sff_init_one(pdev, ppi, &ns87415_sht, NULL,
-				      ns87415_fixup);
+				      ns87415_fixup, 0);
 }
 
 static const struct pci_device_id ns87415_pci_tbl[] = {
Index: b/drivers/ata/pata_sl82c105.c
===================================================================
--- a/drivers/ata/pata_sl82c105.c
+++ b/drivers/ata/pata_sl82c105.c
@@ -222,7 +222,7 @@  static int sl82c105_init_one(struct pci_
 	sl82c105_fixup(&dev->dev);
 
 	return __ata_pci_sff_init_one(dev, ppi, &sl82c105_sht, NULL,
-				      sl82c105_fixup);
+				      sl82c105_fixup, 0);
 }
 
 static const struct pci_device_id sl82c105[] = {
Index: b/include/linux/libata.h
===================================================================
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1605,12 +1605,12 @@  extern int ata_pci_sff_activate_host(str
 extern 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 (*init_host)(struct device *dev));
+				int (*init_host)(struct device *dev), int hflag);
 static inline 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)
 {
-	return __ata_pci_sff_init_one(pdev, ppi, sht, host_priv, NULL);
+	return __ata_pci_sff_init_one(pdev, ppi, sht, host_priv, NULL, 0);
 }
 #endif /* CONFIG_PCI */