[PATCH] ahci: add parameter to disable ahci driver on Promise PDC42819

[PATCH] ahci: add parameter to disable ahci driver on Promise PDC42819

[Mark Nelson]

ahci can drive the Promise PDC42819, but obviously it can only use SATA
disks connected to this controller. The controller can actually support
SAS disks as well, but at the moment only with Promise's own binary
t3sas driver.

To allow users to use both the ahci and the t3sas drivers in the same
system (with t3sas controlling the PDC42819) we add a parameter,
promise_enable that can be used to disable ahci on the Promise chip (by
setting it to 0). This way even if the ahci driver is loaded first the
t3sas driver can grab the Promise chip and the user's SAS disks will be
operational.

By default the parameter is 1 so ahci drives the controller.

While we're at it, add a message letting users know that with ahci
driving their Promise chip they won't be able to use their SAS disks.

Signed-off-by: Mark Nelson <mdnelson8@gmail.com>
---
 drivers/ata/ahci.c |   23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

Index: linux-2.6/drivers/ata/ahci.c
===================================================================
--- linux-2.6.orig/drivers/ata/ahci.c
+++ linux-2.6/drivers/ata/ahci.c
@@ -700,6 +700,9 @@ static int marvell_enable = 1;
 module_param(marvell_enable, int, 0644);
 MODULE_PARM_DESC(marvell_enable, "Marvell SATA via AHCI (1 = enabled)");

+static int promise_enable = 1;
+module_param(promise_enable, int, 0644);
+MODULE_PARM_DESC(promise_enable, "Promise PDC42819 via AHCI (1 = enabled)");

 static inline int ahci_nr_ports(u32 cap)
 {
@@ -2988,6 +2991,26 @@ static int ahci_init_one(struct pci_dev
 	if (pdev->vendor == PCI_VENDOR_ID_MARVELL && !marvell_enable)
 		return -ENODEV;

+	/* Promise's PDC42819 is a SAS/SATA controller that has an AHCI mode.
+	 * At the moment, Promise's t3sas driver is required for SAS
+	 * functionality. Disable ahci on this device if the user asked for
+	 * it.
+	 */
+	if (pdev->vendor == PCI_VENDOR_ID_PROMISE) {
+		if (promise_enable) {
+			dev_printk(KERN_INFO, &pdev->dev, "Promise PDC42819 "
+							  "support enabled\n");
+			dev_printk(KERN_INFO, &pdev->dev, "Only SATA devices "
+							  "will function with"
+							  " this driver\n");
+
+		} else {
+			dev_printk(KERN_INFO, &pdev->dev, "Promise PDC42819 "
+							  "support disabled\n");
+			return -ENODEV;
+		}
+	}
+
 	/* acquire resources */
 	rc = pcim_enable_device(pdev);
 	if (rc)

Re: [PATCH] ahci: add parameter to disable ahci driver on Promise PDC42819

[Alan Cox]

On Wed, 11 Nov 2009 20:54:34 +1100
Mark Nelson <mdnelson8@gmail.com> wrote:

> ahci can drive the Promise PDC42819, but obviously it can only use SATA
> disks connected to this controller. The controller can actually support
> SAS disks as well, but at the moment only with Promise's own binary
> t3sas driver.

That would be a promise problem.

> To allow users to use both the ahci and the t3sas drivers in the same

But you said it was a binary driver, so how can you use it. The kernel is
GPL licensed ?

Re: [PATCH] ahci: add parameter to disable ahci driver on Promise PDC42819

[Mark Nelson]

Hi Alan,

On Wed, Nov 11, 2009 at 9:37 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> On Wed, 11 Nov 2009 20:54:34 +1100
> Mark Nelson <mdnelson8@gmail.com> wrote:
>
>> ahci can drive the Promise PDC42819, but obviously it can only use SATA
>> disks connected to this controller. The controller can actually support
>> SAS disks as well, but at the moment only with Promise's own binary
>> t3sas driver.
>
> That would be a promise problem.

It is Promise's problem but seeing as I was the one who added support
for this Promise chip to the ahci driver I felt responsible for the
complaints from people who try to load the driver only to find that it
doesn't work because ahci has grabbed the device; leaving them to
patch and recompile their kernel to be able to use their SAS disks.

>
>> To allow users to use both the ahci and the t3sas drivers in the same
>
> But you said it was a binary driver, so how can you use it. The kernel is
> GPL licensed ?

The driver looks to be the same as all the other binary drivers around
- an open source shim around some proprietary blob (which in theory
might be shared with their other drivers I guess...).

I would personally love to see an open source SAS driver for this chip,
but I'm told that without any documentation from Promise it would be
virtually impossible to create one. Forgive the sidetrack question, but
with tools like mmiotrace and a working binary driver to use it on (and
the knowledge that it has an AHCI mode), how hard would it be?

In retrospect I should have probably put an RFC on the patch because I
wasn't sure if this would be frowned upon, even if it might help a few
users.

Thanks!
Mark

Re: [PATCH] ahci: add parameter to disable ahci driver on Promise PDC42819

[Jeff Garzik]

On 11/11/2009 04:54 AM, Mark Nelson wrote:
> ahci can drive the Promise PDC42819, but obviously it can only use SATA
> disks connected to this controller. The controller can actually support
> SAS disks as well, but at the moment only with Promise's own binary
> t3sas driver.
>
> To allow users to use both the ahci and the t3sas drivers in the same
> system (with t3sas controlling the PDC42819) we add a parameter,
> promise_enable that can be used to disable ahci on the Promise chip (by
> setting it to 0). This way even if the ahci driver is loaded first the
> t3sas driver can grab the Promise chip and the user's SAS disks will be
> operational.
>
> By default the parameter is 1 so ahci drives the controller.
>
> While we're at it, add a message letting users know that with ahci
> driving their Promise chip they won't be able to use their SAS disks.
>
> Signed-off-by: Mark Nelson<mdnelson8@gmail.com>
> ---
>   drivers/ata/ahci.c |   23 +++++++++++++++++++++++
>   1 file changed, 23 insertions(+)
>
> Index: linux-2.6/drivers/ata/ahci.c
> ===================================================================
> --- linux-2.6.orig/drivers/ata/ahci.c
> +++ linux-2.6/drivers/ata/ahci.c
> @@ -700,6 +700,9 @@ static int marvell_enable = 1;
>   module_param(marvell_enable, int, 0644);
>   MODULE_PARM_DESC(marvell_enable, "Marvell SATA via AHCI (1 = enabled)");
>
> +static int promise_enable = 1;
> +module_param(promise_enable, int, 0644);
> +MODULE_PARM_DESC(promise_enable, "Promise PDC42819 via AHCI (1 = enabled)");
>
>   static inline int ahci_nr_ports(u32 cap)
>   {
> @@ -2988,6 +2991,26 @@ static int ahci_init_one(struct pci_dev
>   	if (pdev->vendor == PCI_VENDOR_ID_MARVELL&&  !marvell_enable)
>   		return -ENODEV;
>
> +	/* Promise's PDC42819 is a SAS/SATA controller that has an AHCI mode.
> +	 * At the moment, Promise's t3sas driver is required for SAS
> +	 * functionality. Disable ahci on this device if the user asked for
> +	 * it.
> +	 */
> +	if (pdev->vendor == PCI_VENDOR_ID_PROMISE) {
> +		if (promise_enable) {
> +			dev_printk(KERN_INFO,&pdev->dev, "Promise PDC42819 "
> +							  "support enabled\n");
> +			dev_printk(KERN_INFO,&pdev->dev, "Only SATA devices "
> +							  "will function with"
> +							  " this driver\n");
> +
> +		} else {
> +			dev_printk(KERN_INFO,&pdev->dev, "Promise PDC42819 "
> +							  "support disabled\n");
> +			return -ENODEV;

I don't mind adding a warning "only SATA devices will function with this 
driver."

The promise_enable is not something we do with closed-source drivers, 
though, so that code logic is NAK'd

	Jeff

Re: [PATCH] ahci: add parameter to disable ahci driver on Promise PDC42819

[Mark Nelson]

On Tue, Nov 17, 2009 at 2:11 PM, Jeff Garzik <jeff@garzik.org> wrote:
> On 11/11/2009 04:54 AM, Mark Nelson wrote:
>>
>> ahci can drive the Promise PDC42819, but obviously it can only use SATA
>> disks connected to this controller. The controller can actually support
>> SAS disks as well, but at the moment only with Promise's own binary
>> t3sas driver.
>>
>> To allow users to use both the ahci and the t3sas drivers in the same
>> system (with t3sas controlling the PDC42819) we add a parameter,
>> promise_enable that can be used to disable ahci on the Promise chip (by
>> setting it to 0). This way even if the ahci driver is loaded first the
>> t3sas driver can grab the Promise chip and the user's SAS disks will be
>> operational.
>>
>> By default the parameter is 1 so ahci drives the controller.
>>
>> While we're at it, add a message letting users know that with ahci
>> driving their Promise chip they won't be able to use their SAS disks.
>>
>> Signed-off-by: Mark Nelson<mdnelson8@gmail.com>
>> ---
>>  drivers/ata/ahci.c |   23 +++++++++++++++++++++++
>>  1 file changed, 23 insertions(+)
>>
>> Index: linux-2.6/drivers/ata/ahci.c
>> ===================================================================
>> --- linux-2.6.orig/drivers/ata/ahci.c
>> +++ linux-2.6/drivers/ata/ahci.c
>> @@ -700,6 +700,9 @@ static int marvell_enable = 1;
>>  module_param(marvell_enable, int, 0644);
>>  MODULE_PARM_DESC(marvell_enable, "Marvell SATA via AHCI (1 = enabled)");
>>
>> +static int promise_enable = 1;
>> +module_param(promise_enable, int, 0644);
>> +MODULE_PARM_DESC(promise_enable, "Promise PDC42819 via AHCI (1 =
>> enabled)");
>>
>>  static inline int ahci_nr_ports(u32 cap)
>>  {
>> @@ -2988,6 +2991,26 @@ static int ahci_init_one(struct pci_dev
>>        if (pdev->vendor == PCI_VENDOR_ID_MARVELL&&  !marvell_enable)
>>                return -ENODEV;
>>
>> +       /* Promise's PDC42819 is a SAS/SATA controller that has an AHCI
>> mode.
>> +        * At the moment, Promise's t3sas driver is required for SAS
>> +        * functionality. Disable ahci on this device if the user asked
>> for
>> +        * it.
>> +        */
>> +       if (pdev->vendor == PCI_VENDOR_ID_PROMISE) {
>> +               if (promise_enable) {
>> +                       dev_printk(KERN_INFO,&pdev->dev, "Promise PDC42819
>> "
>> +                                                         "support
>> enabled\n");
>> +                       dev_printk(KERN_INFO,&pdev->dev, "Only SATA
>> devices "
>> +                                                         "will function
>> with"
>> +                                                         " this
>> driver\n");
>> +
>> +               } else {
>> +                       dev_printk(KERN_INFO,&pdev->dev, "Promise PDC42819
>> "
>> +                                                         "support
>> disabled\n");
>> +                       return -ENODEV;
>
> I don't mind adding a warning "only SATA devices will function with this
> driver."

That works for me - with a warning at least users won't be left guessing.

>
> The promise_enable is not something we do with closed-source drivers,
> though, so that code logic is NAK'd

That's completely understandable.

I'll cut the patch down to just add the warning and resend.

Thanks Jeff!

Mark.