Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1220539 > unrolled thread

[Bugfix] PCI, x86: Correctly allocate IRQs for PCI devices managed by non-PCI drivers

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [Bugfix] PCI, x86: Correctly allocate IRQs for PCI devices managed by non-PCI drivers Jiang Liu <jiang.liu@linux.intel.com> - 2015-09-08 09:30 +0200
    Re: [Bugfix] PCI, x86: Correctly allocate IRQs for PCI devices managed  by non-PCI drivers Arthur Marsh <arthur.marsh@internode.on.net> - 2015-09-08 11:10 +0200
      Re: [Bugfix] PCI, x86: Correctly allocate IRQs for PCI devices  managed by non-PCI drivers Jiang Liu <jiang.liu@linux.intel.com> - 2015-09-08 11:50 +0200
    Re: [Bugfix] PCI, x86: Correctly allocate IRQs for PCI devices  managed by non-PCI drivers Bjorn Helgaas <bhelgaas@google.com> - 2015-09-08 18:30 +0200
      Re: [Bugfix] PCI, x86: Correctly allocate IRQs for PCI devices  managed by non-PCI drivers Jiang Liu <jiang.liu@linux.intel.com> - 2015-09-08 19:00 +0200
        Re: [Bugfix] PCI, x86: Correctly allocate IRQs for PCI devices  managed by non-PCI drivers Bjorn Helgaas <bhelgaas@google.com> - 2015-09-09 21:20 +0200
          Re: [Bugfix] PCI, x86: Correctly allocate IRQs for PCI devices  managed by non-PCI drivers Jiang Liu <jiang.liu@linux.intel.com> - 2015-09-10 11:00 +0200
          [Bugfix 3/3] eata: Enhance eata driver to support PCI device hot-removal Jiang Liu <jiang.liu@linux.intel.com> - 2015-09-14 05:10 +0200
            Re: [Bugfix 3/3] eata: Enhance eata driver to support PCI device  hot-removal Hannes Reinecke <hare@suse.de> - 2015-09-14 10:30 +0200
              Re: [Bugfix 3/3] eata: Enhance eata driver to support PCI device  hot-removal Jiang Liu <jiang.liu@linux.intel.com> - 2015-09-14 10:40 +0200
              RE: [Bugfix 3/3] eata: Enhance eata driver to support PCI device  hot-removal "Ballabio, Dario" <dario.ballabio@emc.com> - 2015-09-14 10:40 +0200
              Re: [Bugfix 3/3] eata: Enhance eata driver to support PCI device  hot-removal Christoph Hellwig <hch@infradead.org> - 2015-09-16 15:50 +0200
                Re: [Bugfix 3/3] eata: Enhance eata driver to support PCI device  hot-removal Jiang Liu <jiang.liu@linux.intel.com> - 2015-09-17 08:50 +0200
                Re: [Bugfix 3/3] eata: Enhance eata driver to support PCI device  hot-removal Arthur Marsh <arthur.marsh@internode.on.net> - 2015-09-18 17:10 +0200
                [RFT v3] eata: Convert eata driver as normal PCI and platform device drivers Jiang Liu <jiang.liu@linux.intel.com> - 2015-09-22 09:30 +0200
          [Bugfix 0/3] Convert eata driver to a normal PCI device driver Jiang Liu <jiang.liu@linux.intel.com> - 2015-09-14 05:10 +0200
            [Bugfix 2/3] eata: Implement PCI driver to manage eata PCI devices Jiang Liu <jiang.liu@linux.intel.com> - 2015-09-14 05:10 +0200
              Re: [Bugfix 2/3] eata: Implement PCI driver to manage eata PCI  devices Hannes Reinecke <hare@suse.de> - 2015-09-14 10:20 +0200
                Re: [Bugfix 2/3] eata: Implement PCI driver to manage eata PCI  devices Jiang Liu <jiang.liu@linux.intel.com> - 2015-09-14 10:40 +0200
            [Bugfix 1/3] eata: Use IDA to manage eata board IDs Jiang Liu <jiang.liu@linux.intel.com> - 2015-09-14 05:10 +0200
              Re: [Bugfix 1/3] eata: Use IDA to manage eata board IDs Hannes Reinecke <hare@suse.de> - 2015-09-14 10:10 +0200
            Re: [Bugfix 0/3] Convert eata driver to a normal PCI device driver Arthur Marsh <arthur.marsh@internode.on.net> - 2015-09-14 18:10 +0200
              Re: [Bugfix 0/3] Convert eata driver to a normal PCI device driver Jiang Liu <jiang.liu@linux.intel.com> - 2015-09-15 04:40 +0200
                Re: [Bugfix 0/3] Convert eata driver to a normal PCI device driver Jiang Liu <jiang.liu@linux.intel.com> - 2015-09-16 07:10 +0200
                  Re: [Bugfix 0/3] Convert eata driver to a normal PCI device driver Arthur Marsh <arthur.marsh@internode.on.net> - 2015-09-16 09:40 +0200
                    Re: [Bugfix 0/3] Convert eata driver to a normal PCI device driver Jiang Liu <jiang.liu@linux.intel.com> - 2015-09-16 10:30 +0200
                      Re: [Bugfix 0/3] Convert eata driver to a normal PCI device driver Arthur Marsh <arthur.marsh@internode.on.net> - 2015-09-16 13:40 +0200
    Re: [Bugfix] PCI, x86: Correctly allocate IRQs for PCI devices managed  by non-PCI drivers Arthur Marsh <arthur.marsh@internode.on.net> - 2015-09-09 21:10 +0200

Page 1 of 2  [1] 2  Next page →

#1220539 — [Bugfix] PCI, x86: Correctly allocate IRQs for PCI devices managed by non-PCI drivers

Commit 991de2e59090 ("PCI, x86: Implement pcibios_alloc_irq() and
pcibios_free_irq()") changes the way to allocate PCI legacy IRQ
for PCI devices on x86 platforms. Instead of allocating PCI legacy
IRQs when pcibios_enable_device() gets called, now pcibios_alloc_irq()
will be called by pci_device_probe() to allocate PCI legacy IRQs
when binding PCI drivers to PCI devices.

But some device drivers, such as eata, directly access PCI devices
without implementing corresponding PCI drivers, so pcibios_alloc_irq()
won't be called for those PCI devices and wrong IRQ number may be
used to manage the PCI device.

So detect such a case in pcibios_enable_device() by checking
pci_dev->driver is NULL and call pcibios_alloc_irq() to allocate PCI
legacy IRQs.

Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
---
 arch/x86/pci/common.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
index 09d3afc0a181..60b237783582 100644
--- a/arch/x86/pci/common.c
+++ b/arch/x86/pci/common.c
@@ -685,6 +685,16 @@ void pcibios_free_irq(struct pci_dev *dev)
 
 int pcibios_enable_device(struct pci_dev *dev, int mask)
 {
+	/*
+	 * By design, pcibios_alloc_irq() will be called by pci_device_probe()
+	 * when binding a PCI device to a PCI driver. But some device drivers,
+	 * such as eata, directly make use of PCI devices without implementing
+	 * PCI device drivers, so pcibios_alloc_irq() won't be called for those
+	 * PCI devices.
+	 */
+	if (!dev->driver)
+		pcibios_alloc_irq(dev);
+
 	return pci_enable_resources(dev, mask);
 }
 
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1220621 — Re: [Bugfix] PCI, x86: Correctly allocate IRQs for PCI devices managed by non-PCI drivers

Jiang Liu wrote on 08/09/15 16:56:
> Commit 991de2e59090 ("PCI, x86: Implement pcibios_alloc_irq() and
> pcibios_free_irq()") changes the way to allocate PCI legacy IRQ
> for PCI devices on x86 platforms. Instead of allocating PCI legacy
> IRQs when pcibios_enable_device() gets called, now pcibios_alloc_irq()
> will be called by pci_device_probe() to allocate PCI legacy IRQs
> when binding PCI drivers to PCI devices.
>
> But some device drivers, such as eata, directly access PCI devices
> without implementing corresponding PCI drivers, so pcibios_alloc_irq()
> won't be called for those PCI devices and wrong IRQ number may be
> used to manage the PCI device.
>
> So detect such a case in pcibios_enable_device() by checking
> pci_dev->driver is NULL and call pcibios_alloc_irq() to allocate PCI
> legacy IRQs.
>
> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> ---
>   arch/x86/pci/common.c |   10 ++++++++++
>   1 file changed, 10 insertions(+)
>
> diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
> index 09d3afc0a181..60b237783582 100644
> --- a/arch/x86/pci/common.c
> +++ b/arch/x86/pci/common.c
> @@ -685,6 +685,16 @@ void pcibios_free_irq(struct pci_dev *dev)
>
>   int pcibios_enable_device(struct pci_dev *dev, int mask)
>   {
> +	/*
> +	 * By design, pcibios_alloc_irq() will be called by pci_device_probe()
> +	 * when binding a PCI device to a PCI driver. But some device drivers,
> +	 * such as eata, directly make use of PCI devices without implementing
> +	 * PCI device drivers, so pcibios_alloc_irq() won't be called for those
> +	 * PCI devices.
> +	 */
> +	if (!dev->driver)
> +		pcibios_alloc_irq(dev);
> +
>   	return pci_enable_resources(dev, mask);
>   }
>
>

Thanks, I removed the test patch and applied the revised patch and built 
and rebooted the kernel and successfully mounted file systems on a disk 
attached to the DPT 2044W card using the eata driver:

[    0.000000] Linux version 4.2.0+ (root@victoria) (gcc version 5.2.1 
20150903
(Debian 5.2.1-16) ) #31 SMP PREEMPT Tue Sep 8 17:36:28 ACST 2015
...

[   80.691097] EATA0: IRQ 10 mapped to IO-APIC IRQ 17.
[   80.724519] EATA/DMA 2.0x: Copyright (C) 1994-2003 Dario Ballabio.
[   80.752035] EATA config options -> tm:1, lc:y, mq:16, rs:y, et:n, 
ip:n, ep:n, pp:y.
[   80.777063] EATA0: 2.0C, PCI 0xd890, IRQ 17, BMST, SG 122, MB 64.
[   80.802391] EATA0: wide SCSI support enabled, max_id 16, max_lun 8.
[   80.827959] EATA0: SCSI channel 0 enabled, host target ID 7.
[   80.853413] scsi host3: EATA/DMA 2.0x rev. 8.10.00
[   82.445662] scsi 3:0:6:0: Direct-Access     IBM      DCAS-34330W 
  S65A PQ: 0 ANSI: 2
[   82.471584] scsi 3:0:6:0: cmds/lun 16, sorted, simple tags.
[   84.571451] sd 3:0:6:0: Attached scsi generic sg4 type 0
[   84.597572] sd 3:0:6:0: [sdd] 8466688 512-byte logical blocks: (4.33 
GB/4.03 GiB)
[   84.659874] sd 3:0:6:0: [sdd] Write Protect is off
[   84.688543] sd 3:0:6:0: [sdd] Mode Sense: b3 00 00 08
[   84.714021] sd 3:0:6:0: [sdd] Write cache: enabled, read cache: 
enabled, doesn't support DPO or FUA
[   84.817682]  sdd: sdd1 sdd2 < sdd5 >
[   84.919267] sd 3:0:6:0: [sdd] Attached SCSI disk

Arthur.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1220655 — Re: [Bugfix] PCI, x86: Correctly allocate IRQs for PCI devices managed by non-PCI drivers

On 2015/9/8 17:03, Arthur Marsh wrote:
> 
> 
> Jiang Liu wrote on 08/09/15 16:56:
> Thanks, I removed the test patch and applied the revised patch and built
> and rebooted the kernel and successfully mounted file systems on a disk
> attached to the DPT 2044W card using the eata driver:
> 
> [    0.000000] Linux version 4.2.0+ (root@victoria) (gcc version 5.2.1
> 20150903
> (Debian 5.2.1-16) ) #31 SMP PREEMPT Tue Sep 8 17:36:28 ACST 2015
> ...
> 
> [   80.691097] EATA0: IRQ 10 mapped to IO-APIC IRQ 17.
> [   80.724519] EATA/DMA 2.0x: Copyright (C) 1994-2003 Dario Ballabio.
> [   80.752035] EATA config options -> tm:1, lc:y, mq:16, rs:y, et:n,
> ip:n, ep:n, pp:y.
> [   80.777063] EATA0: 2.0C, PCI 0xd890, IRQ 17, BMST, SG 122, MB 64.
> [   80.802391] EATA0: wide SCSI support enabled, max_id 16, max_lun 8.
> [   80.827959] EATA0: SCSI channel 0 enabled, host target ID 7.
> [   80.853413] scsi host3: EATA/DMA 2.0x rev. 8.10.00
> [   82.445662] scsi 3:0:6:0: Direct-Access     IBM      DCAS-34330W
>  S65A PQ: 0 ANSI: 2
> [   82.471584] scsi 3:0:6:0: cmds/lun 16, sorted, simple tags.
> [   84.571451] sd 3:0:6:0: Attached scsi generic sg4 type 0
> [   84.597572] sd 3:0:6:0: [sdd] 8466688 512-byte logical blocks: (4.33
> GB/4.03 GiB)
> [   84.659874] sd 3:0:6:0: [sdd] Write Protect is off
> [   84.688543] sd 3:0:6:0: [sdd] Mode Sense: b3 00 00 08
> [   84.714021] sd 3:0:6:0: [sdd] Write cache: enabled, read cache:
> enabled, doesn't support DPO or FUA
> [   84.817682]  sdd: sdd1 sdd2 < sdd5 >
> [   84.919267] sd 3:0:6:0: [sdd] Attached SCSI disk
Hi Arthur,
Thanks for testing:)

> 
> Arthur.
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1220965 — Re: [Bugfix] PCI, x86: Correctly allocate IRQs for PCI devices managed by non-PCI drivers

Hi Jiang,

I object to subject lines like "Correctly do such and such."  Nobody
writes code to do things *incorrectly*, so the word "correctly" takes
up space without contributing meaning.  In this case, it's at least
debatable whether this is even the "correct" approach; see below.

On Tue, Sep 08, 2015 at 03:26:29PM +0800, Jiang Liu wrote:
> Commit 991de2e59090 ("PCI, x86: Implement pcibios_alloc_irq() and
> pcibios_free_irq()") changes the way to allocate PCI legacy IRQ
> for PCI devices on x86 platforms. Instead of allocating PCI legacy
> IRQs when pcibios_enable_device() gets called, now pcibios_alloc_irq()
> will be called by pci_device_probe() to allocate PCI legacy IRQs
> when binding PCI drivers to PCI devices.
> 
> But some device drivers, such as eata, directly access PCI devices
> without implementing corresponding PCI drivers, so pcibios_alloc_irq()
> won't be called for those PCI devices and wrong IRQ number may be
> used to manage the PCI device.

I'm not sure this is wise.  

We normally call pcibios_alloc_irq() from pci_device_probe(), just
before we call the driver's .probe() method.

The eata driver does not use pci_register_driver(), so there is no
.probe() method (also no .remove(), .suspend(), etc.)  But eata *does*
use pci_enable_device() and other PCI interfaces.  So this patch adds
code in the x86 pci_enable_device() path for this case.

AFAICT, there's no real reason why eata doesn't register a PCI driver;
it's just a case of legacy code where nobody has been motivated to
update it.  I'm not in favor of catering to code like that because
then we have random special cases like this that clutter up the core
code.

I don't think we should necessarily expect the PCI core to support
calls to PCI interfaces when it hasn't had a chance to initialize
itself via driver registration.

> So detect such a case in pcibios_enable_device() by checking
> pci_dev->driver is NULL and call pcibios_alloc_irq() to allocate PCI
> legacy IRQs.
> 
> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> ---
>  arch/x86/pci/common.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
> index 09d3afc0a181..60b237783582 100644
> --- a/arch/x86/pci/common.c
> +++ b/arch/x86/pci/common.c
> @@ -685,6 +685,16 @@ void pcibios_free_irq(struct pci_dev *dev)
>  
>  int pcibios_enable_device(struct pci_dev *dev, int mask)
>  {
> +	/*
> +	 * By design, pcibios_alloc_irq() will be called by pci_device_probe()
> +	 * when binding a PCI device to a PCI driver. But some device drivers,
> +	 * such as eata, directly make use of PCI devices without implementing
> +	 * PCI device drivers, so pcibios_alloc_irq() won't be called for those
> +	 * PCI devices.
> +	 */
> +	if (!dev->driver)
> +		pcibios_alloc_irq(dev);

This is a point fix for x86 only, but I think eata can be built for
any architecture.  Won't other architectures still have the same
problem?

>  	return pci_enable_resources(dev, mask);
>  }
>  
> -- 
> 1.7.10.4
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1220993 — Re: [Bugfix] PCI, x86: Correctly allocate IRQs for PCI devices managed by non-PCI drivers

On 2015/9/9 0:27, Bjorn Helgaas wrote:
> Hi Jiang,
> 
> I object to subject lines like "Correctly do such and such."  Nobody
> writes code to do things *incorrectly*, so the word "correctly" takes
> up space without contributing meaning.  In this case, it's at least
> debatable whether this is even the "correct" approach; see below.
> 
> On Tue, Sep 08, 2015 at 03:26:29PM +0800, Jiang Liu wrote:
>> Commit 991de2e59090 ("PCI, x86: Implement pcibios_alloc_irq() and
>> pcibios_free_irq()") changes the way to allocate PCI legacy IRQ
>> for PCI devices on x86 platforms. Instead of allocating PCI legacy
>> IRQs when pcibios_enable_device() gets called, now pcibios_alloc_irq()
>> will be called by pci_device_probe() to allocate PCI legacy IRQs
>> when binding PCI drivers to PCI devices.
>>
>> But some device drivers, such as eata, directly access PCI devices
>> without implementing corresponding PCI drivers, so pcibios_alloc_irq()
>> won't be called for those PCI devices and wrong IRQ number may be
>> used to manage the PCI device.
> 
> I'm not sure this is wise.  
> 
> We normally call pcibios_alloc_irq() from pci_device_probe(), just
> before we call the driver's .probe() method.
> 
> The eata driver does not use pci_register_driver(), so there is no
> .probe() method (also no .remove(), .suspend(), etc.)  But eata *does*
> use pci_enable_device() and other PCI interfaces.  So this patch adds
> code in the x86 pci_enable_device() path for this case.
> 
> AFAICT, there's no real reason why eata doesn't register a PCI driver;
> it's just a case of legacy code where nobody has been motivated to
> update it.  I'm not in favor of catering to code like that because
> then we have random special cases like this that clutter up the core
> code.
> 
> I don't think we should necessarily expect the PCI core to support
> calls to PCI interfaces when it hasn't had a chance to initialize
> itself via driver registration.
> 
>> So detect such a case in pcibios_enable_device() by checking
>> pci_dev->driver is NULL and call pcibios_alloc_irq() to allocate PCI
>> legacy IRQs.
>>
>> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
>> ---
>>  arch/x86/pci/common.c |   10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
>> index 09d3afc0a181..60b237783582 100644
>> --- a/arch/x86/pci/common.c
>> +++ b/arch/x86/pci/common.c
>> @@ -685,6 +685,16 @@ void pcibios_free_irq(struct pci_dev *dev)
>>  
>>  int pcibios_enable_device(struct pci_dev *dev, int mask)
>>  {
>> +	/*
>> +	 * By design, pcibios_alloc_irq() will be called by pci_device_probe()
>> +	 * when binding a PCI device to a PCI driver. But some device drivers,
>> +	 * such as eata, directly make use of PCI devices without implementing
>> +	 * PCI device drivers, so pcibios_alloc_irq() won't be called for those
>> +	 * PCI devices.
>> +	 */
>> +	if (!dev->driver)
>> +		pcibios_alloc_irq(dev);
> 
> This is a point fix for x86 only, but I think eata can be built for
> any architecture.  Won't other architectures still have the same
> problem?
Hi Bjorn,
	We have used another draft version to fix this issue by changing
eata driver as below. But that needs to export pcibios_alloc_irq. And
I'm not sure whether there are other drivers having the same behavior.
If we think it's a legacy behavior and only a few drivers may have
such a behavior, I prefer changing drivers to fix the issue too.
Thanks!
Gerry
---
 drivers/pci/pci-driver.c |    1 +
 drivers/scsi/eata.c      |    2 ++
 2 files changed, 3 insertions(+)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 52a880ca1768..17d2a0b1de18 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -392,6 +392,7 @@ int __weak pcibios_alloc_irq(struct pci_dev *dev)
 {
 	return 0;
 }
+EXPORT_SYMBOL_GPL(pcibios_alloc_irq);

 void __weak pcibios_free_irq(struct pci_dev *dev)
 {
diff --git a/drivers/scsi/eata.c b/drivers/scsi/eata.c
index 227dd2c2ec2f..7e6eaf867987 100644
--- a/drivers/scsi/eata.c
+++ b/drivers/scsi/eata.c
@@ -1061,6 +1061,7 @@ static void enable_pci_ports(void)
 		       driver_name, dev->bus->number, dev->devfn);
 #endif

+		pcibios_alloc_irq(dev);
 		if (pci_enable_device(dev))
 			printk
 			    ("%s: warning, pci_enable_device failed, bus %d devfn 0x%x.\n",
@@ -1520,6 +1521,7 @@ static void add_pci_ports(void)
 		if (!(dev = pci_get_class(PCI_CLASS_STORAGE_SCSI << 8, dev)))
 			break;

+		pcibios_alloc_irq(dev);
 		if (pci_enable_device(dev)) {
 #if defined(DEBUG_PCI_DETECT)
 			printk


> 
>>  	return pci_enable_resources(dev, mask);
>>  }
>>  
>> -- 
>> 1.7.10.4
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1221640 — Re: [Bugfix] PCI, x86: Correctly allocate IRQs for PCI devices managed by non-PCI drivers

On Tue, Sep 8, 2015 at 11:49 AM, Jiang Liu <jiang.liu@linux.intel.com> wrote:
> Hi Bjorn,
>         We have used another draft version to fix this issue by changing
> eata driver as below. But that needs to export pcibios_alloc_irq. And
> I'm not sure whether there are other drivers having the same behavior.
> If we think it's a legacy behavior and only a few drivers may have
> such a behavior, I prefer changing drivers to fix the issue too.

Did you look at changing the driver to use pci_register_driver()?  I
bet it's pretty straightforward.

>  drivers/pci/pci-driver.c |    1 +
>  drivers/scsi/eata.c      |    2 ++
>  2 files changed, 3 insertions(+)
>
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index 52a880ca1768..17d2a0b1de18 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -392,6 +392,7 @@ int __weak pcibios_alloc_irq(struct pci_dev *dev)
>  {
>         return 0;
>  }
> +EXPORT_SYMBOL_GPL(pcibios_alloc_irq);
>
>  void __weak pcibios_free_irq(struct pci_dev *dev)
>  {
> diff --git a/drivers/scsi/eata.c b/drivers/scsi/eata.c
> index 227dd2c2ec2f..7e6eaf867987 100644
> --- a/drivers/scsi/eata.c
> +++ b/drivers/scsi/eata.c
> @@ -1061,6 +1061,7 @@ static void enable_pci_ports(void)
>                        driver_name, dev->bus->number, dev->devfn);
>  #endif
>
> +               pcibios_alloc_irq(dev);
>                 if (pci_enable_device(dev))
>                         printk
>                             ("%s: warning, pci_enable_device failed, bus %d devfn 0x%x.\n",
> @@ -1520,6 +1521,7 @@ static void add_pci_ports(void)
>                 if (!(dev = pci_get_class(PCI_CLASS_STORAGE_SCSI << 8, dev)))
>                         break;
>
> +               pcibios_alloc_irq(dev);
>                 if (pci_enable_device(dev)) {
>  #if defined(DEBUG_PCI_DETECT)
>                         printk
>
>
>>
>>>      return pci_enable_resources(dev, mask);
>>>  }
>>>
>>> --
>>> 1.7.10.4
>>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1222045 — Re: [Bugfix] PCI, x86: Correctly allocate IRQs for PCI devices managed by non-PCI drivers

On 2015/9/10 3:11, Bjorn Helgaas wrote:
> On Tue, Sep 8, 2015 at 11:49 AM, Jiang Liu <jiang.liu@linux.intel.com> wrote:
>> Hi Bjorn,
>>         We have used another draft version to fix this issue by changing
>> eata driver as below. But that needs to export pcibios_alloc_irq. And
>> I'm not sure whether there are other drivers having the same behavior.
>> If we think it's a legacy behavior and only a few drivers may have
>> such a behavior, I prefer changing drivers to fix the issue too.
> 
> Did you look at changing the driver to use pci_register_driver()?  I
> bet it's pretty straightforward.
Hi Bjorn,
	I have done some investigation. It's doable, but the change
may be a little bigger. Will send the patch out for comments once
it's done.
Thanks!
Gerry

> 
>>  drivers/pci/pci-driver.c |    1 +
>>  drivers/scsi/eata.c      |    2 ++
>>  2 files changed, 3 insertions(+)
>>
>> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
>> index 52a880ca1768..17d2a0b1de18 100644
>> --- a/drivers/pci/pci-driver.c
>> +++ b/drivers/pci/pci-driver.c
>> @@ -392,6 +392,7 @@ int __weak pcibios_alloc_irq(struct pci_dev *dev)
>>  {
>>         return 0;
>>  }
>> +EXPORT_SYMBOL_GPL(pcibios_alloc_irq);
>>
>>  void __weak pcibios_free_irq(struct pci_dev *dev)
>>  {
>> diff --git a/drivers/scsi/eata.c b/drivers/scsi/eata.c
>> index 227dd2c2ec2f..7e6eaf867987 100644
>> --- a/drivers/scsi/eata.c
>> +++ b/drivers/scsi/eata.c
>> @@ -1061,6 +1061,7 @@ static void enable_pci_ports(void)
>>                        driver_name, dev->bus->number, dev->devfn);
>>  #endif
>>
>> +               pcibios_alloc_irq(dev);
>>                 if (pci_enable_device(dev))
>>                         printk
>>                             ("%s: warning, pci_enable_device failed, bus %d devfn 0x%x.\n",
>> @@ -1520,6 +1521,7 @@ static void add_pci_ports(void)
>>                 if (!(dev = pci_get_class(PCI_CLASS_STORAGE_SCSI << 8, dev)))
>>                         break;
>>
>> +               pcibios_alloc_irq(dev);
>>                 if (pci_enable_device(dev)) {
>>  #if defined(DEBUG_PCI_DETECT)
>>                         printk
>>
>>
>>>
>>>>      return pci_enable_resources(dev, mask);
>>>>  }
>>>>
>>>> --
>>>> 1.7.10.4
>>>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1223814 — [Bugfix 3/3] eata: Enhance eata driver to support PCI device hot-removal

Due to having no hardware for testing, this is just a sample code
to support PCI device hot-removal. It just passing compilation,
no any tests.

Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
---
 drivers/scsi/eata.c |   26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/scsi/eata.c b/drivers/scsi/eata.c
index b92e6856f909..f3bd7cbf260e 100644
--- a/drivers/scsi/eata.c
+++ b/drivers/scsi/eata.c
@@ -1474,6 +1474,21 @@ static unsigned int port_probe(unsigned long port_base,
 #ifdef CONFIG_PCI
 static int eata2x_pci_device_count;
 
+/* TODO: need help here to shutdown the scsi host and release resources */
+static void port_remove(unsigned int id, resource_size_t port_base,
+			struct pci_dev *pdev)
+{
+	struct Scsi_Host *shost = sh[id];
+
+	/* TODO: stop scsi device */
+	scsi_unregister(shost);
+	/* TODO: clean up resources allocated by port_detect() */
+	clear_bit(id, eata_board_bitmap);
+	free_irq(shost->irq, &sha[id]);
+	release_region(port_base, REGION_SIZE);
+	ida_simple_remove(&eata_ida, id);
+}
+
 static int eata2x_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
 	int i, ret = -ENXIO;
@@ -1521,6 +1536,16 @@ out_error:
 	return ret;
 }
 
+static void eata2x_pci_remove(struct pci_dev *pdev)
+{
+	int id = (int)(long)dev_get_drvdata(&pdev->dev);
+	resource_size_t port_base;
+
+	port_base = pci_resource_start(pdev, 0) + PCI_BASE_ADDRESS_0;
+	port_remove(id, port_base, pdev);
+	pci_disable_device(pdev);
+}
+
 static struct pci_device_id eata2x_tbl[] = {
 	{ PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_SCSI << 8, PCI_ANY_ID) },
 	{ },
@@ -1531,6 +1556,7 @@ static struct pci_driver eata2x_pci_driver = {
 	.name		= "eata",
 	.id_table	= eata2x_tbl,
 	.probe		= eata2x_pci_probe,
+	.remove		= eata2x_pci_remove,
 };
 
 static int eata2x_probe_pci_devices(struct scsi_host_template *tpnt)
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1223914 — Re: [Bugfix 3/3] eata: Enhance eata driver to support PCI device hot-removal

On 09/14/2015 05:08 AM, Jiang Liu wrote:
> Due to having no hardware for testing, this is just a sample code
> to support PCI device hot-removal. It just passing compilation,
> no any tests.
> 
> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> ---
>  drivers/scsi/eata.c |   26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/scsi/eata.c b/drivers/scsi/eata.c
> index b92e6856f909..f3bd7cbf260e 100644
> --- a/drivers/scsi/eata.c
> +++ b/drivers/scsi/eata.c
> @@ -1474,6 +1474,21 @@ static unsigned int port_probe(unsigned long port_base,
>  #ifdef CONFIG_PCI
>  static int eata2x_pci_device_count;
>  
> +/* TODO: need help here to shutdown the scsi host and release resources */
> +static void port_remove(unsigned int id, resource_size_t port_base,
> +			struct pci_dev *pdev)
> +{
> +	struct Scsi_Host *shost = sh[id];
> +
> +	/* TODO: stop scsi device */
> +	scsi_unregister(shost);
> +	/* TODO: clean up resources allocated by port_detect() */
> +	clear_bit(id, eata_board_bitmap);
> +	free_irq(shost->irq, &sha[id]);
> +	release_region(port_base, REGION_SIZE);
> +	ida_simple_remove(&eata_ida, id);
> +}
> +
>  static int eata2x_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  {
>  	int i, ret = -ENXIO;
> @@ -1521,6 +1536,16 @@ out_error:
>  	return ret;
>  }
>  
> +static void eata2x_pci_remove(struct pci_dev *pdev)
> +{
> +	int id = (int)(long)dev_get_drvdata(&pdev->dev);
> +	resource_size_t port_base;
> +
> +	port_base = pci_resource_start(pdev, 0) + PCI_BASE_ADDRESS_0;
> +	port_remove(id, port_base, pdev);
> +	pci_disable_device(pdev);
> +}
> +
>  static struct pci_device_id eata2x_tbl[] = {
>  	{ PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_SCSI << 8, PCI_ANY_ID) },
>  	{ },
> @@ -1531,6 +1556,7 @@ static struct pci_driver eata2x_pci_driver = {
>  	.name		= "eata",
>  	.id_table	= eata2x_tbl,
>  	.probe		= eata2x_pci_probe,
> +	.remove		= eata2x_pci_remove,
>  };
>  
>  static int eata2x_probe_pci_devices(struct scsi_host_template *tpnt)
> 
Welll ... if you don't have hardware (and I strongly hope you refer to
'hardware able to do hotplugging', not 'hardware for the eata driver'
...) why add the code at all?
Chances are no-one will ever need eata PCI hotplug; SCSI parallel
typically isn't very good at hotplugging, so throwing in PCI hotplug
will only confuse matters more.
Plus due to the sheer mechanics involved here I find it very unlikely
anyone will be using it in real life.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1223917 — Re: [Bugfix 3/3] eata: Enhance eata driver to support PCI device hot-removal

On 2015/9/14 16:31, Ballabio, Dario wrote:
> Agreed, It does not make sense to have  this driver converted to a hot plug api.
Thanks, got the point:)
Originally i thought user may trigger PCI device hot-removal by sysfs
interfaces and there's comment mentioning that eata driver breaks
PCI hotplug, so I tried to solve it. If it's no real use, we may
just drop the third patch.
Thanks!
Gerry

> 
> Cheers,
> 
> 
> 
> ***************************************
> Ph.D. Dario Ballabio
> Principal Field Support Specialist, EMC EMEA 
> Mobile phone: +393487978851
> 
> 
> 
> -----Original Message-----
> From: Hannes Reinecke [mailto:hare@suse.de] 
> Sent: Monday, September 14, 2015 10:21 AM
> To: Jiang Liu; Thomas Gleixner; Bjorn Helgaas; Arthur Marsh; Ballabio, Dario; James E.J. Bottomley
> Cc: linux-kernel@vger.kernel.org; linux-pci@vger.kernel.org; linux-scsi@vger.kernel.org; x86@kernel.org
> Subject: Re: [Bugfix 3/3] eata: Enhance eata driver to support PCI device hot-removal
> 
> On 09/14/2015 05:08 AM, Jiang Liu wrote:
>> Due to having no hardware for testing, this is just a sample code to 
>> support PCI device hot-removal. It just passing compilation, no any 
>> tests.
>>
>> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
>> ---
>>  drivers/scsi/eata.c |   26 ++++++++++++++++++++++++++
>>  1 file changed, 26 insertions(+)
>>
>> diff --git a/drivers/scsi/eata.c b/drivers/scsi/eata.c index 
>> b92e6856f909..f3bd7cbf260e 100644
>> --- a/drivers/scsi/eata.c
>> +++ b/drivers/scsi/eata.c
>> @@ -1474,6 +1474,21 @@ static unsigned int port_probe(unsigned long 
>> port_base,  #ifdef CONFIG_PCI  static int eata2x_pci_device_count;
>>  
>> +/* TODO: need help here to shutdown the scsi host and release 
>> +resources */ static void port_remove(unsigned int id, resource_size_t port_base,
>> +			struct pci_dev *pdev)
>> +{
>> +	struct Scsi_Host *shost = sh[id];
>> +
>> +	/* TODO: stop scsi device */
>> +	scsi_unregister(shost);
>> +	/* TODO: clean up resources allocated by port_detect() */
>> +	clear_bit(id, eata_board_bitmap);
>> +	free_irq(shost->irq, &sha[id]);
>> +	release_region(port_base, REGION_SIZE);
>> +	ida_simple_remove(&eata_ida, id);
>> +}
>> +
>>  static int eata2x_pci_probe(struct pci_dev *dev, const struct 
>> pci_device_id *id)  {
>>  	int i, ret = -ENXIO;
>> @@ -1521,6 +1536,16 @@ out_error:
>>  	return ret;
>>  }
>>  
>> +static void eata2x_pci_remove(struct pci_dev *pdev) {
>> +	int id = (int)(long)dev_get_drvdata(&pdev->dev);
>> +	resource_size_t port_base;
>> +
>> +	port_base = pci_resource_start(pdev, 0) + PCI_BASE_ADDRESS_0;
>> +	port_remove(id, port_base, pdev);
>> +	pci_disable_device(pdev);
>> +}
>> +
>>  static struct pci_device_id eata2x_tbl[] = {
>>  	{ PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_SCSI << 8, PCI_ANY_ID) },
>>  	{ },
>> @@ -1531,6 +1556,7 @@ static struct pci_driver eata2x_pci_driver = {
>>  	.name		= "eata",
>>  	.id_table	= eata2x_tbl,
>>  	.probe		= eata2x_pci_probe,
>> +	.remove		= eata2x_pci_remove,
>>  };
>>  
>>  static int eata2x_probe_pci_devices(struct scsi_host_template *tpnt)
>>
> Welll ... if you don't have hardware (and I strongly hope you refer to 'hardware able to do hotplugging', not 'hardware for the eata driver'
> ...) why add the code at all?
> Chances are no-one will ever need eata PCI hotplug; SCSI parallel typically isn't very good at hotplugging, so throwing in PCI hotplug will only confuse matters more.
> Plus due to the sheer mechanics involved here I find it very unlikely anyone will be using it in real life.
> 
> Cheers,
> 
> Hannes
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1223924 — RE: [Bugfix 3/3] eata: Enhance eata driver to support PCI device hot-removal

Agreed, It does not make sense to have  this driver converted to a hot plug api.

Cheers,



***************************************
Ph.D. Dario Ballabio
Principal Field Support Specialist, EMC EMEA 
Mobile phone: +393487978851



-----Original Message-----
From: Hannes Reinecke [mailto:hare@suse.de] 
Sent: Monday, September 14, 2015 10:21 AM
To: Jiang Liu; Thomas Gleixner; Bjorn Helgaas; Arthur Marsh; Ballabio, Dario; James E.J. Bottomley
Cc: linux-kernel@vger.kernel.org; linux-pci@vger.kernel.org; linux-scsi@vger.kernel.org; x86@kernel.org
Subject: Re: [Bugfix 3/3] eata: Enhance eata driver to support PCI device hot-removal

On 09/14/2015 05:08 AM, Jiang Liu wrote:
> Due to having no hardware for testing, this is just a sample code to 
> support PCI device hot-removal. It just passing compilation, no any 
> tests.
> 
> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> ---
>  drivers/scsi/eata.c |   26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/scsi/eata.c b/drivers/scsi/eata.c index 
> b92e6856f909..f3bd7cbf260e 100644
> --- a/drivers/scsi/eata.c
> +++ b/drivers/scsi/eata.c
> @@ -1474,6 +1474,21 @@ static unsigned int port_probe(unsigned long 
> port_base,  #ifdef CONFIG_PCI  static int eata2x_pci_device_count;
>  
> +/* TODO: need help here to shutdown the scsi host and release 
> +resources */ static void port_remove(unsigned int id, resource_size_t port_base,
> +			struct pci_dev *pdev)
> +{
> +	struct Scsi_Host *shost = sh[id];
> +
> +	/* TODO: stop scsi device */
> +	scsi_unregister(shost);
> +	/* TODO: clean up resources allocated by port_detect() */
> +	clear_bit(id, eata_board_bitmap);
> +	free_irq(shost->irq, &sha[id]);
> +	release_region(port_base, REGION_SIZE);
> +	ida_simple_remove(&eata_ida, id);
> +}
> +
>  static int eata2x_pci_probe(struct pci_dev *dev, const struct 
> pci_device_id *id)  {
>  	int i, ret = -ENXIO;
> @@ -1521,6 +1536,16 @@ out_error:
>  	return ret;
>  }
>  
> +static void eata2x_pci_remove(struct pci_dev *pdev) {
> +	int id = (int)(long)dev_get_drvdata(&pdev->dev);
> +	resource_size_t port_base;
> +
> +	port_base = pci_resource_start(pdev, 0) + PCI_BASE_ADDRESS_0;
> +	port_remove(id, port_base, pdev);
> +	pci_disable_device(pdev);
> +}
> +
>  static struct pci_device_id eata2x_tbl[] = {
>  	{ PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_SCSI << 8, PCI_ANY_ID) },
>  	{ },
> @@ -1531,6 +1556,7 @@ static struct pci_driver eata2x_pci_driver = {
>  	.name		= "eata",
>  	.id_table	= eata2x_tbl,
>  	.probe		= eata2x_pci_probe,
> +	.remove		= eata2x_pci_remove,
>  };
>  
>  static int eata2x_probe_pci_devices(struct scsi_host_template *tpnt)
> 
Welll ... if you don't have hardware (and I strongly hope you refer to 'hardware able to do hotplugging', not 'hardware for the eata driver'
...) why add the code at all?
Chances are no-one will ever need eata PCI hotplug; SCSI parallel typically isn't very good at hotplugging, so throwing in PCI hotplug will only confuse matters more.
Plus due to the sheer mechanics involved here I find it very unlikely anyone will be using it in real life.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1226094 — Re: [Bugfix 3/3] eata: Enhance eata driver to support PCI device hot-removal

On Mon, Sep 14, 2015 at 10:21:14AM +0200, Hannes Reinecke wrote:
> Welll ... if you don't have hardware (and I strongly hope you refer to
> 'hardware able to do hotplugging', not 'hardware for the eata driver'
> ...) why add the code at all?
> Chances are no-one will ever need eata PCI hotplug; SCSI parallel
> typically isn't very good at hotplugging, so throwing in PCI hotplug
> will only confuse matters more.
> Plus due to the sheer mechanics involved here I find it very unlikely
> anyone will be using it in real life.

Because it's used for module removal and we want every driver to use
the standard interface that.

Jiang, you also need to convert the driver to
scsi_add_host/scsi_remove_host from the legacy scsi_register interface,
otherwise the SCSI layer will be very unhappy.

Take a look at commit 0d31f8759109cbc1e6fc196d08e6b0e8a9e93b3f for
example, the change should be straight forward.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1226694 — Re: [Bugfix 3/3] eata: Enhance eata driver to support PCI device hot-removal

On 2015/9/16 21:42, Christoph Hellwig wrote:
> On Mon, Sep 14, 2015 at 10:21:14AM +0200, Hannes Reinecke wrote:
>> Welll ... if you don't have hardware (and I strongly hope you refer to
>> 'hardware able to do hotplugging', not 'hardware for the eata driver'
>> ...) why add the code at all?
>> Chances are no-one will ever need eata PCI hotplug; SCSI parallel
>> typically isn't very good at hotplugging, so throwing in PCI hotplug
>> will only confuse matters more.
>> Plus due to the sheer mechanics involved here I find it very unlikely
>> anyone will be using it in real life.
> 
> Because it's used for module removal and we want every driver to use
> the standard interface that.
> 
> Jiang, you also need to convert the driver to
> scsi_add_host/scsi_remove_host from the legacy scsi_register interface,
> otherwise the SCSI layer will be very unhappy.
> 
> Take a look at commit 0d31f8759109cbc1e6fc196d08e6b0e8a9e93b3f for
> example, the change should be straight forward.
Hi Christoph,
	I have taken a look at the commit 0d31f8759109cb and it seems
that the conversion is not so big. But I have no hardware for testing,
so could only ask for help from community.
Thanks!
Gerry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1227994 — Re: [Bugfix 3/3] eata: Enhance eata driver to support PCI device hot-removal

Christoph Hellwig wrote on 16/09/15 23:12:

> Jiang, you also need to convert the driver to
> scsi_add_host/scsi_remove_host from the legacy scsi_register interface,
> otherwise the SCSI layer will be very unhappy.
>
> Take a look at commit 0d31f8759109cbc1e6fc196d08e6b0e8a9e93b3f for
> example, the change should be straight forward.
>

I am pleased to note that when I tried a Linus git head kernel from the 
last 24 hours, the IRQ routing for my DPT2044W SCSI card using eata 
module worked again, although the shut-down/kexec issue remains.

Arthur.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1229940 — [RFT v3] eata: Convert eata driver as normal PCI and platform device drivers

Previously the eata driver just grabs and accesses eata PCI devices
without implementing a PCI device driver, that causes troubles with
latest IRQ related

Commit 991de2e59090 ("PCI, x86: Implement pcibios_alloc_irq() and
pcibios_free_irq()") changes the way to allocate PCI legacy IRQ
for PCI devices on x86 platforms. Instead of allocating PCI legacy
IRQs when pcibios_enable_device() gets called, now pcibios_alloc_irq()
will be called by pci_device_probe() to allocate PCI legacy IRQs
when binding PCI drivers to PCI devices.

But the eata driver directly accesses PCI devices without implementing
corresponding PCI drivers, so pcibios_alloc_irq() won't be called for
those PCI devices and wrong IRQ number may be used to manage the PCI
device.

This patch implements a PCI device driver to manage eata PCI devices,
so eata driver could properly cooperate with the PCI core. It also
provides headroom for PCI hotplug with eata driver.

It also represents non-PCI eata devices as platform devices, so it could
be managed as normal devices.

Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Ballabio, Dario <dario.ballabio@emc.com>
Cc: Christoph Hellwig <hch@infradead.org>
---
Hi all,
	I have no hardware to test the patch set, so it only passing
compilation and module loading tests. Any help to test this patch on
real hardware are welcomed!
Thanks!
Gerry
---
 drivers/scsi/eata.c |  441 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 262 insertions(+), 179 deletions(-)

diff --git a/drivers/scsi/eata.c b/drivers/scsi/eata.c
index 227dd2c2ec2f..dc2c76b7873e 100644
--- a/drivers/scsi/eata.c
+++ b/drivers/scsi/eata.c
@@ -486,6 +486,8 @@
 #include <linux/interrupt.h>
 #include <linux/stat.h>
 #include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/idr.h>
 #include <linux/init.h>
 #include <linux/ctype.h>
 #include <linux/spinlock.h>
@@ -503,8 +505,6 @@
 #include <scsi/scsi_tcq.h>
 #include <scsi/scsicam.h>
 
-static int eata2x_detect(struct scsi_host_template *);
-static int eata2x_release(struct Scsi_Host *);
 static int eata2x_queuecommand(struct Scsi_Host *, struct scsi_cmnd *);
 static int eata2x_eh_abort(struct scsi_cmnd *);
 static int eata2x_eh_host_reset(struct scsi_cmnd *);
@@ -513,9 +513,9 @@ static int eata2x_bios_param(struct scsi_device *, struct block_device *,
 static int eata2x_slave_configure(struct scsi_device *);
 
 static struct scsi_host_template driver_template = {
+	.module = THIS_MODULE,
+	.proc_name = "eata2x",
 	.name = "EATA/DMA 2.0x rev. 8.10.00 ",
-	.detect = eata2x_detect,
-	.release = eata2x_release,
 	.queuecommand = eata2x_queuecommand,
 	.eh_abort_handler = eata2x_eh_abort,
 	.eh_host_reset_handler = eata2x_eh_host_reset,
@@ -834,12 +834,9 @@ struct hostdata {
 	struct mssp sp;		/* Local copy of sp buffer */
 };
 
-static struct Scsi_Host *sh[MAX_BOARDS];
 static const char *driver_name = "EATA";
-static char sha[MAX_BOARDS];
-
-/* Initialize num_boards so that ihdlr can work while detect is in progress */
-static unsigned int num_boards = MAX_BOARDS;
+static DEFINE_IDA(eata2x_ida);
+static struct platform_device *eata2x_platform_devs[MAX_BOARDS];
 
 static unsigned long io_port[] = {
 
@@ -850,10 +847,6 @@ static unsigned long io_port[] = {
 	/* First ISA */
 	0x1f0,
 
-	/* Space for MAX_PCI ports possibly reported by PCI_BIOS */
-	SKIP, SKIP, SKIP, SKIP, SKIP, SKIP, SKIP, SKIP,
-	SKIP, SKIP, SKIP, SKIP, SKIP, SKIP, SKIP, SKIP,
-
 	/* MAX_EISA ports */
 	0x1c88, 0x2c88, 0x3c88, 0x4c88, 0x5c88, 0x6c88, 0x7c88, 0x8c88,
 	0x9c88, 0xac88, 0xbc88, 0xcc88, 0xdc88, 0xec88, 0xfc88,
@@ -1024,74 +1017,41 @@ static int read_pio(unsigned long iobase, ushort * start, ushort * end)
 	return 0;
 }
 
-static struct pci_dev *get_pci_dev(unsigned long port_base)
-{
-#if defined(CONFIG_PCI)
-	unsigned int addr;
-	struct pci_dev *dev = NULL;
-
-	while ((dev = pci_get_class(PCI_CLASS_STORAGE_SCSI << 8, dev))) {
-		addr = pci_resource_start(dev, 0);
-
-#if defined(DEBUG_PCI_DETECT)
-		printk("%s: get_pci_dev, bus %d, devfn 0x%x, addr 0x%x.\n",
-		       driver_name, dev->bus->number, dev->devfn, addr);
-#endif
-
-		/* we are in so much trouble for a pci hotplug system with this driver
-		 * anyway, so doing this at least lets people unload the driver and not
-		 * cause memory problems, but in general this is a bad thing to do (this
-		 * driver needs to be converted to the proper PCI api someday... */
-		pci_dev_put(dev);
-		if (addr + PCI_BASE_ADDRESS_0 == port_base)
-			return dev;
-	}
-#endif				/* end CONFIG_PCI */
-	return NULL;
-}
-
-static void enable_pci_ports(void)
-{
-#if defined(CONFIG_PCI)
-	struct pci_dev *dev = NULL;
-
-	while ((dev = pci_get_class(PCI_CLASS_STORAGE_SCSI << 8, dev))) {
-#if defined(DEBUG_PCI_DETECT)
-		printk("%s: enable_pci_ports, bus %d, devfn 0x%x.\n",
-		       driver_name, dev->bus->number, dev->devfn);
-#endif
-
-		if (pci_enable_device(dev))
-			printk
-			    ("%s: warning, pci_enable_device failed, bus %d devfn 0x%x.\n",
-			     driver_name, dev->bus->number, dev->devfn);
-	}
-
-#endif				/* end CONFIG_PCI */
-}
-
-static int port_detect(unsigned long port_base, unsigned int j,
-		struct scsi_host_template *tpnt)
+static int port_detect(unsigned long port_base, struct device *dev)
 {
 	unsigned char irq, dma_channel, subversion, i, is_pci = 0;
 	unsigned char protocol_rev;
 	struct eata_info info;
 	char *bus_type, dma_name[16];
-	struct pci_dev *pdev;
+	struct pci_dev *pdev = NULL;
 	/* Allowed DMA channels for ISA (0 indicates reserved) */
 	unsigned char dma_channel_table[4] = { 5, 6, 7, 0 };
 	struct Scsi_Host *shost;
 	struct hostdata *ha;
 	char name[16];
+	int idx, ret = -ENODEV;
 
-	sprintf(name, "%s%d", driver_name, j);
+	idx = ida_simple_get(&eata2x_ida, 0, MAX_BOARDS, GFP_KERNEL);
+	if (idx < 0) {
+		ret = idx;
+		goto fail;
+	}
+
+	shost = scsi_host_alloc(&driver_template, sizeof(struct hostdata));
+	if (shost == NULL) {
+		printk("%s: unable to alloc host, detaching.\n", driver_name);
+		ret = -ENOMEM;
+		goto freeid;
+	}
+
+	sprintf(name, "%s%d", driver_name, idx);
 
 	if (!request_region(port_base, REGION_SIZE, driver_name)) {
 #if defined(DEBUG_DETECT)
 		printk("%s: address 0x%03lx in use, skipping probe.\n", name,
 		       port_base);
 #endif
-		goto fail;
+		goto freeshost;
 	}
 
 	if (do_dma(port_base, 0, READ_CONFIG_PIO)) {
@@ -1199,15 +1159,10 @@ static int port_detect(unsigned long port_base, unsigned int j,
 		    ("%s: warning, LEVEL triggering is suggested for IRQ %u.\n",
 		     name, irq);
 
-	if (is_pci) {
-		pdev = get_pci_dev(port_base);
-		if (!pdev)
-			printk
-			    ("%s: warning, failed to get pci_dev structure.\n",
-			     name);
-	} else
-		pdev = NULL;
-
+	if (dev_is_pci(dev))
+		pdev = to_pci_dev(dev);
+	if (is_pci && !pdev)
+		printk("%s: warning, failed to get pci_dev structure.\n", name);
 	if (pdev && (irq != pdev->irq)) {
 		printk("%s: IRQ %u mapped to IO-APIC IRQ %u.\n", name, irq,
 		       pdev->irq);
@@ -1217,7 +1172,7 @@ static int port_detect(unsigned long port_base, unsigned int j,
 	/* Board detected, allocate its IRQ */
 	if (request_irq(irq, do_interrupt_handler,
 			(subversion == ESA) ? IRQF_SHARED : 0,
-			driver_name, (void *)&sha[j])) {
+			driver_name, shost)) {
 		printk("%s: unable to allocate IRQ %u, detaching.\n", name,
 		       irq);
 		goto freelock;
@@ -1259,12 +1214,6 @@ static int port_detect(unsigned long port_base, unsigned int j,
 	}
 #endif
 
-	sh[j] = shost = scsi_register(tpnt, sizeof(struct hostdata));
-	if (shost == NULL) {
-		printk("%s: unable to register host, detaching.\n", name);
-		goto freedma;
-	}
-
 	shost->io_port = port_base;
 	shost->unique_id = port_base;
 	shost->n_io_port = REGION_SIZE;
@@ -1282,7 +1231,7 @@ static int port_detect(unsigned long port_base, unsigned int j,
 	ha->protocol_rev = protocol_rev;
 	ha->is_pci = is_pci;
 	ha->pdev = pdev;
-	ha->board_number = j;
+	ha->board_number = idx;
 
 	if (ha->subversion == ESA)
 		shost->unchecked_isa_dma = 0;
@@ -1350,7 +1299,7 @@ static int port_detect(unsigned long port_base, unsigned int j,
 			printk
 			    ("%s: kmalloc SGlist failed, mbox %d, detaching.\n",
 			     ha->board_name, i);
-			goto release;
+			goto free_cp_dma_addr;
 		}
 	}
 
@@ -1358,7 +1307,7 @@ static int port_detect(unsigned long port_base, unsigned int j,
 							sizeof(struct mssp),
 							&ha->sp_dma_addr))) {
 		printk("%s: pci_alloc_consistent failed, detaching.\n", ha->board_name);
-		goto release;
+		goto free_sglist;
 	}
 
 	if (max_queue_depth > MAX_TAGGED_CMD_PER_LUN)
@@ -1370,7 +1319,7 @@ static int port_detect(unsigned long port_base, unsigned int j,
 	if (tag_mode != TAG_DISABLED && tag_mode != TAG_SIMPLE)
 		tag_mode = TAG_ORDERED;
 
-	if (j == 0) {
+	if (idx == 0) {
 		printk
 		    ("EATA/DMA 2.0x: Copyright (C) 1994-2003 Dario Ballabio.\n");
 		printk
@@ -1420,21 +1369,65 @@ static int port_detect(unsigned long port_base, unsigned int j,
 			       ha->board_name);
 	}
 
-	return 1;
+	ret = scsi_add_host(shost, NULL);
+	if (!ret) {
+		dev_set_drvdata(dev, shost);
+		scsi_scan_host(shost);
+		return idx;
+	}
 
-      freedma:
+	if (ha->sp_cpu_addr)
+		pci_free_consistent(ha->pdev, sizeof(struct mssp),
+				    ha->sp_cpu_addr, ha->sp_dma_addr);
+free_sglist:
+	for (i = 0; i < shost->can_queue; i++)
+		kfree((&ha->cp[i])->sglist);
+free_cp_dma_addr:
+	for (i = 0; i < shost->can_queue; i++)
+		pci_unmap_single(ha->pdev, ha->cp[i].cp_dma_addr,
+				 sizeof(struct mscp), PCI_DMA_BIDIRECTIONAL);
+#if defined(FORCE_CONFIG)
+freedma:
 	if (subversion == ISA)
 		free_dma(dma_channel);
-      freeirq:
-	free_irq(irq, &sha[j]);
-      freelock:
+#endif
+freeirq:
+	free_irq(irq, shost);
+freelock:
 	release_region(port_base, REGION_SIZE);
-      fail:
-	return 0;
+freeshost:
+	scsi_host_put(shost);
+freeid:
+	ida_simple_remove(&eata2x_ida, idx);
+fail:
+	return ret;
+}
 
-      release:
-	eata2x_release(shost);
-	return 0;
+static void port_remove(struct device *dev)
+{
+	struct Scsi_Host *shost = dev_get_drvdata(dev);
+	struct hostdata *ha;
+	unsigned int i;
+
+	if (!shost)
+		return;
+
+	scsi_remove_host(shost);
+	ha = (struct hostdata *)shost->hostdata;
+	for (i = 0; i < shost->can_queue; i++)
+		kfree((&ha->cp[i])->sglist);
+	for (i = 0; i < shost->can_queue; i++)
+		pci_unmap_single(ha->pdev, ha->cp[i].cp_dma_addr,
+				 sizeof(struct mscp), PCI_DMA_BIDIRECTIONAL);
+	if (ha->sp_cpu_addr)
+		pci_free_consistent(ha->pdev, sizeof(struct mssp),
+				    ha->sp_cpu_addr, ha->sp_dma_addr);
+	if (shost->dma_channel != NO_DMA)
+		free_dma(shost->dma_channel);
+	free_irq(shost->irq, shost);
+	release_region(shost->io_port, shost->n_io_port);
+	ida_simple_remove(&eata2x_ida, ha->board_number);
+	scsi_host_put(shost);
 }
 
 static void internal_setup(char *str, int *ints)
@@ -1509,59 +1502,125 @@ static int option_setup(char *str)
 	return 1;
 }
 
-static void add_pci_ports(void)
+#ifdef CONFIG_PCI
+static int eata2x_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
-#if defined(CONFIG_PCI)
-	unsigned int addr, k;
-	struct pci_dev *dev = NULL;
+	int i;
+	resource_size_t addr;
+	unsigned long port_base;
+
+	if (pci_enable_device(dev)) {
+		if (config_enabled(DEBUG_PCI_DETECT))
+			pr_warn("%s: detect, bus %d, devfn 0x%x, pci_enable_device failed.\n",
+				driver_name, dev->bus->number, dev->devfn);
+		goto out_error;
+	}
 
-	for (k = 0; k < MAX_PCI; k++) {
+	addr = pci_resource_start(dev, 0);
+	port_base = addr + PCI_BASE_ADDRESS_0;
+	if (config_enabled(DEBUG_PCI_DETECT))
+		printk("%s: detect, bus %d, devfn 0x%x, addr 0x%x.\n",
+		       driver_name, dev->bus->number, dev->devfn,
+		       (unsigned int)addr);
+
+	if (setup_done) {
+		/*
+		 * Handle kernel or module parameter
+		 * . probe board if its port is specified by user
+		 * . otherwise ignore the board
+		 */
+		for (i = 1; i < MAX_INT_PARAM; i++)
+			if (io_port[i] == port_base) {
+				io_port[i] = SKIP;
+				break;
+			}
+		if (i >= MAX_INT_PARAM)
+			goto out_disable_device;
+	}
 
-		if (!(dev = pci_get_class(PCI_CLASS_STORAGE_SCSI << 8, dev)))
-			break;
+	if (port_detect(port_base, &dev->dev) >= 0)
+		return 0;
 
-		if (pci_enable_device(dev)) {
-#if defined(DEBUG_PCI_DETECT)
-			printk
-			    ("%s: detect, bus %d, devfn 0x%x, pci_enable_device failed.\n",
-			     driver_name, dev->bus->number, dev->devfn);
-#endif
+out_disable_device:
+	pci_disable_device(dev);
+out_error:
+	return -ENXIO;
+}
 
-			continue;
-		}
+static void eata2x_pci_remove(struct pci_dev *pdev)
+{
+	port_remove(&pdev->dev);
+	pci_disable_device(pdev);
+}
 
-		addr = pci_resource_start(dev, 0);
+static struct pci_device_id eata2x_tbl[] = {
+	{ PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_SCSI << 8, PCI_ANY_ID) },
+	{ },
+};
+MODULE_DEVICE_TABLE(pci, eata2x_tbl);
 
-#if defined(DEBUG_PCI_DETECT)
-		printk("%s: detect, seq. %d, bus %d, devfn 0x%x, addr 0x%x.\n",
-		       driver_name, k, dev->bus->number, dev->devfn, addr);
-#endif
+static struct pci_driver eata2x_pci_driver = {
+	.name		= "eata_pci",
+	.id_table	= eata2x_tbl,
+	.probe		= eata2x_pci_probe,
+	.remove		= eata2x_pci_remove,
+};
 
-		/* Order addresses according to rev_scan value */
-		io_port[MAX_INT_PARAM + (rev_scan ? (MAX_PCI - k) : (1 + k))] =
-		    addr + PCI_BASE_ADDRESS_0;
-	}
+static int eata2x_register_pci_driver(void)
+{
+	if (!pci_probe)
+		return 0;
+	if (!pci_register_driver(&eata2x_pci_driver))
+		return 1;
+	pr_warn("%s: failed to register PCI device driver.\n", driver_name);
+	return -ENODEV;
+}
 
-	pci_dev_put(dev);
-#endif				/* end CONFIG_PCI */
+static void eata2x_unregister_pci_driver(void)
+{
+	pci_unregister_driver(&eata2x_pci_driver);
 }
+#else /* CONFIG_PCI */
+static inline int eata2x_register_pci_driver(void) { return 0; }
+static inline void eata2x_unregister_pci_driver(void) {}
+#endif /* CONFIG_PCI */
 
-static int eata2x_detect(struct scsi_host_template *tpnt)
+static int __init eata2x_platform_probe(struct platform_device *pdev)
 {
-	unsigned int j = 0, k;
+	int ret = -EIO;
+	struct resource *res;
 
-	tpnt->proc_name = "eata2x";
+	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
+	if (!res)
+		dev_warn(&pdev->dev, "failed to get base IOPORT.\n");
+	else
+		ret = port_detect(res->start, &pdev->dev);
 
-	if (strlen(boot_options))
-		option_setup(boot_options);
+	return ret >= 0 ? 0 : ret;
+}
 
-#if defined(MODULE)
-	/* io_port could have been modified when loading as a module */
-	if (io_port[0] != SKIP) {
-		setup_done = 1;
-		io_port[MAX_INT_PARAM] = 0;
-	}
-#endif
+static int __exit eata2x_platform_remove(struct platform_device *pdev)
+{
+	port_remove(&pdev->dev);
+	return 0;
+}
+
+static struct platform_driver eata2x_platform_driver = {
+	.remove = __exit_p(eata2x_platform_remove),
+	.driver = {
+		.name	= "eata_plat",
+		.owner	= THIS_MODULE,
+	},
+};
+
+static int eata2x_probe_platform_devices(void)
+{
+	int k, count = 0, idx = 0, error = 0;
+	struct platform_driver *driver = &eata2x_platform_driver;
+	struct platform_device *pdev;
+	struct resource res = {
+		.flags = IORESOURCE_IO,
+	};
 
 	for (k = MAX_INT_PARAM; io_port[k]; k++)
 		if (io_port[k] == SKIP)
@@ -1574,25 +1633,80 @@ static int eata2x_detect(struct scsi_host_template *tpnt)
 			if (!eisa_probe)
 				io_port[k] = SKIP;
 		}
-
-	if (pci_probe) {
-		if (!setup_done)
-			add_pci_ports();
+	for (k = 0; error == 0 && io_port[k]; k++) {
+		if (io_port[k] == SKIP)
+			continue;
+		res.start = io_port[k];
+		res.end = io_port[k] + REGION_SIZE - 1;
+		pdev = platform_device_register_simple(driver->driver.name,
+						       idx, &res, 1);
+		if (!pdev)
+			error = -ENOMEM;
 		else
-			enable_pci_ports();
+			eata2x_platform_devs[idx++] = pdev;
+	}
+	if (error == 0)
+		error = platform_driver_probe(driver, eata2x_platform_probe);
+	for (k = 0; k < idx; k++) {
+		pdev = eata2x_platform_devs[k];
+		if (error || platform_get_drvdata(pdev) == NULL) {
+			platform_device_unregister(pdev);
+			eata2x_platform_devs[idx] = NULL;;
+		} else {
+			count++;
+		}
 	}
 
-	for (k = 0; io_port[k]; k++) {
+	return count;
+}
 
-		if (io_port[k] == SKIP)
-			continue;
+static void eata2x_remove_platform_devices(void)
+{
+	int idx;
+	struct platform_device *pdev;
+
+	platform_driver_unregister(&eata2x_platform_driver);
+	for (idx = 0; idx < MAX_BOARDS; idx++) {
+		pdev = eata2x_platform_devs[idx];
+		if (pdev) {
+			platform_device_unregister(pdev);
+			eata2x_platform_devs[idx] = NULL;;
+		}
+	}
+}
+
+static int __init eata2x_init(void)
+{
+	int ret, count = 0;
 
-		if (j < MAX_BOARDS && port_detect(io_port[k], j, tpnt))
-			j++;
+	if (strlen(boot_options))
+		option_setup(boot_options);
+
+	/* io_port could have been modified when loading as a module */
+	if (config_enabled(MODULE) && io_port[0] != SKIP) {
+		setup_done = 1;
+		io_port[MAX_INT_PARAM] = 0;
 	}
 
-	num_boards = j;
-	return j;
+	ret = eata2x_register_pci_driver();
+	if (ret >= 0) {
+		count += ret;
+		ret = eata2x_probe_platform_devices();
+		if (ret > 0)
+			count += ret;
+	}
+	if (ret >= 0 && count > 0)
+		return 0;
+	
+	eata2x_remove_platform_devices();
+	eata2x_unregister_pci_driver();
+	return ret < 0 ? ret : -ENODEV;
+}
+
+static void __exit eata2x_exit(void)
+{
+	eata2x_remove_platform_devices();
+	eata2x_unregister_pci_driver();
 }
 
 static void map_dma(unsigned int i, struct hostdata *ha)
@@ -2527,51 +2641,20 @@ static irqreturn_t ihdlr(struct Scsi_Host *shost)
 	return IRQ_NONE;
 }
 
-static irqreturn_t do_interrupt_handler(int dummy, void *shap)
+static irqreturn_t do_interrupt_handler(int dummy, void *data)
 {
-	struct Scsi_Host *shost;
-	unsigned int j;
+	struct Scsi_Host *shost = data;
 	unsigned long spin_flags;
 	irqreturn_t ret;
 
-	/* Check if the interrupt must be processed by this handler */
-	if ((j = (unsigned int)((char *)shap - sha)) >= num_boards)
-		return IRQ_NONE;
-	shost = sh[j];
-
 	spin_lock_irqsave(shost->host_lock, spin_flags);
 	ret = ihdlr(shost);
 	spin_unlock_irqrestore(shost->host_lock, spin_flags);
 	return ret;
 }
 
-static int eata2x_release(struct Scsi_Host *shost)
-{
-	struct hostdata *ha = (struct hostdata *)shost->hostdata;
-	unsigned int i;
-
-	for (i = 0; i < shost->can_queue; i++)
-		kfree((&ha->cp[i])->sglist);
-
-	for (i = 0; i < shost->can_queue; i++)
-		pci_unmap_single(ha->pdev, ha->cp[i].cp_dma_addr,
-				 sizeof(struct mscp), PCI_DMA_BIDIRECTIONAL);
-
-	if (ha->sp_cpu_addr)
-		pci_free_consistent(ha->pdev, sizeof(struct mssp),
-				    ha->sp_cpu_addr, ha->sp_dma_addr);
-
-	free_irq(shost->irq, &sha[ha->board_number]);
-
-	if (shost->dma_channel != NO_DMA)
-		free_dma(shost->dma_channel);
-
-	release_region(shost->io_port, shost->n_io_port);
-	scsi_unregister(shost);
-	return 0;
-}
-
-#include "scsi_module.c"
+module_init(eata2x_init);
+module_exit(eata2x_exit);
 
 #ifndef MODULE
 __setup("eata=", option_setup);
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1223815 — [Bugfix 0/3] Convert eata driver to a normal PCI device driver

Hi Authur,
	As suggested by Bjorn, patch 1-2 set implement a PCI device
driver to manage eata PCI devices. And patch 3 tries to support PCI
device hot-removal for eata, but I have no change to test due to 
limited knowledge about scsi subsystem and lacking of hardware for
tests.
	So you could please help to test patch 1-2? Patch 3 is just
for comments.
Thanks!
Gerry

Jiang Liu (3):
  eata: Use IDA to manage eata board IDs
  eata: Implement PCI driver to manage eata PCI devices
  eata: Enhance eata driver to support PCI device hot-removal

 drivers/scsi/eata.c |  232 +++++++++++++++++++++++++++------------------------
 1 file changed, 125 insertions(+), 107 deletions(-)

-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1223817 — [Bugfix 2/3] eata: Implement PCI driver to manage eata PCI devices

Previously the eata driver just grabs and accesses eata PCI devices
without implementing a PCI device driver, that causes troubles with
latest IRQ related

Commit 991de2e59090 ("PCI, x86: Implement pcibios_alloc_irq() and
pcibios_free_irq()") changes the way to allocate PCI legacy IRQ
for PCI devices on x86 platforms. Instead of allocating PCI legacy
IRQs when pcibios_enable_device() gets called, now pcibios_alloc_irq()
will be called by pci_device_probe() to allocate PCI legacy IRQs
when binding PCI drivers to PCI devices.

But the eata driver directly accesses PCI devices without implementing
corresponding PCI drivers, so pcibios_alloc_irq() won't be called for
those PCI devices and wrong IRQ number may be used to manage the PCI
device.

This patch implements a PCI device driver to manage eata PCI devices,
so eata driver could properly cooperate with the PCI core. It also
provides headroom for PCI hotplug with eata driver.

Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
---
 drivers/scsi/eata.c |  170 ++++++++++++++++++++++-----------------------------
 1 file changed, 74 insertions(+), 96 deletions(-)

diff --git a/drivers/scsi/eata.c b/drivers/scsi/eata.c
index b45d3b532b70..b92e6856f909 100644
--- a/drivers/scsi/eata.c
+++ b/drivers/scsi/eata.c
@@ -850,10 +850,6 @@ static unsigned long io_port[] = {
 	/* First ISA */
 	0x1f0,
 
-	/* Space for MAX_PCI ports possibly reported by PCI_BIOS */
-	SKIP, SKIP, SKIP, SKIP, SKIP, SKIP, SKIP, SKIP,
-	SKIP, SKIP, SKIP, SKIP, SKIP, SKIP, SKIP, SKIP,
-
 	/* MAX_EISA ports */
 	0x1c88, 0x2c88, 0x3c88, 0x4c88, 0x5c88, 0x6c88, 0x7c88, 0x8c88,
 	0x9c88, 0xac88, 0xbc88, 0xcc88, 0xdc88, 0xec88, 0xfc88,
@@ -1024,60 +1020,13 @@ static int read_pio(unsigned long iobase, ushort * start, ushort * end)
 	return 0;
 }
 
-static struct pci_dev *get_pci_dev(unsigned long port_base)
-{
-#if defined(CONFIG_PCI)
-	unsigned int addr;
-	struct pci_dev *dev = NULL;
-
-	while ((dev = pci_get_class(PCI_CLASS_STORAGE_SCSI << 8, dev))) {
-		addr = pci_resource_start(dev, 0);
-
-#if defined(DEBUG_PCI_DETECT)
-		printk("%s: get_pci_dev, bus %d, devfn 0x%x, addr 0x%x.\n",
-		       driver_name, dev->bus->number, dev->devfn, addr);
-#endif
-
-		/* we are in so much trouble for a pci hotplug system with this driver
-		 * anyway, so doing this at least lets people unload the driver and not
-		 * cause memory problems, but in general this is a bad thing to do (this
-		 * driver needs to be converted to the proper PCI api someday... */
-		pci_dev_put(dev);
-		if (addr + PCI_BASE_ADDRESS_0 == port_base)
-			return dev;
-	}
-#endif				/* end CONFIG_PCI */
-	return NULL;
-}
-
-static void enable_pci_ports(void)
-{
-#if defined(CONFIG_PCI)
-	struct pci_dev *dev = NULL;
-
-	while ((dev = pci_get_class(PCI_CLASS_STORAGE_SCSI << 8, dev))) {
-#if defined(DEBUG_PCI_DETECT)
-		printk("%s: enable_pci_ports, bus %d, devfn 0x%x.\n",
-		       driver_name, dev->bus->number, dev->devfn);
-#endif
-
-		if (pci_enable_device(dev))
-			printk
-			    ("%s: warning, pci_enable_device failed, bus %d devfn 0x%x.\n",
-			     driver_name, dev->bus->number, dev->devfn);
-	}
-
-#endif				/* end CONFIG_PCI */
-}
-
 static int port_detect(unsigned long port_base, unsigned int j,
-		struct scsi_host_template *tpnt)
+		       struct scsi_host_template *tpnt, struct pci_dev *pdev)
 {
 	unsigned char irq, dma_channel, subversion, i, is_pci = 0;
 	unsigned char protocol_rev;
 	struct eata_info info;
 	char *bus_type, dma_name[16];
-	struct pci_dev *pdev;
 	/* Allowed DMA channels for ISA (0 indicates reserved) */
 	unsigned char dma_channel_table[4] = { 5, 6, 7, 0 };
 	struct Scsi_Host *shost;
@@ -1199,15 +1148,8 @@ static int port_detect(unsigned long port_base, unsigned int j,
 		    ("%s: warning, LEVEL triggering is suggested for IRQ %u.\n",
 		     name, irq);
 
-	if (is_pci) {
-		pdev = get_pci_dev(port_base);
-		if (!pdev)
-			printk
-			    ("%s: warning, failed to get pci_dev structure.\n",
-			     name);
-	} else
-		pdev = NULL;
-
+	if (is_pci && !pdev)
+		printk("%s: warning, failed to get pci_dev structure.\n", name);
 	if (pdev && (irq != pdev->irq)) {
 		printk("%s: IRQ %u mapped to IO-APIC IRQ %u.\n", name, irq,
 		       pdev->irq);
@@ -1510,14 +1452,17 @@ static int option_setup(char *str)
 }
 
 static unsigned int port_probe(unsigned long port_base,
-			       struct scsi_host_template *tpnt)
+			       struct scsi_host_template *tpnt,
+			       struct pci_dev *pdev)
 {
 	int id;
 
 	id = ida_simple_get(&eata_ida, 0, MAX_BOARDS, GFP_KERNEL);
 	if (id >= 0) {
 		set_bit(id, eata_board_bitmap);
-		if (port_detect(port_base, id, tpnt))
+		if (pdev)
+			dev_set_drvdata(&pdev->dev, (void *)(long)id);
+		if (port_detect(port_base, id, tpnt, pdev))
 			return id;
 		clear_bit(id, eata_board_bitmap);
 		ida_simple_remove(&eata_ida, id);
@@ -1526,42 +1471,81 @@ static unsigned int port_probe(unsigned long port_base,
 	return -1;
 }
 
-static void add_pci_ports(void)
-{
-#if defined(CONFIG_PCI)
-	unsigned int addr, k;
-	struct pci_dev *dev = NULL;
-
-	for (k = 0; k < MAX_PCI; k++) {
+#ifdef CONFIG_PCI
+static int eata2x_pci_device_count;
 
-		if (!(dev = pci_get_class(PCI_CLASS_STORAGE_SCSI << 8, dev)))
-			break;
+static int eata2x_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
+{
+	int i, ret = -ENXIO;
+	resource_size_t addr;
+	unsigned long port_base;
+	struct scsi_host_template *tpnt = (void *)id->driver_data;
 
-		if (pci_enable_device(dev)) {
+	if (pci_enable_device(dev)) {
 #if defined(DEBUG_PCI_DETECT)
-			printk
-			    ("%s: detect, bus %d, devfn 0x%x, pci_enable_device failed.\n",
-			     driver_name, dev->bus->number, dev->devfn);
+		pr_warn("%s: detect, bus %d, devfn 0x%x, pci_enable_device failed.\n",
+			driver_name, dev->bus->number, dev->devfn);
 #endif
+		goto out_error;
+	}
 
-			continue;
-		}
-
-		addr = pci_resource_start(dev, 0);
-
+	addr = pci_resource_start(dev, 0);
+	port_base = addr + PCI_BASE_ADDRESS_0;
 #if defined(DEBUG_PCI_DETECT)
-		printk("%s: detect, seq. %d, bus %d, devfn 0x%x, addr 0x%x.\n",
-		       driver_name, k, dev->bus->number, dev->devfn, addr);
+	printk("%s: detect, bus %d, devfn 0x%x, addr 0x%x.\n",
+	       driver_name, dev->bus->number, dev->devfn, (unsigned int)addr);
 #endif
 
-		/* Order addresses according to rev_scan value */
-		io_port[MAX_INT_PARAM + (rev_scan ? (MAX_PCI - k) : (1 + k))] =
-		    addr + PCI_BASE_ADDRESS_0;
+	if (setup_done) {
+		/*
+		 * Handle kernel or module parameter
+		 * . probe board if its port is specified by user
+		 * . otherwise ignore the board
+		 */
+		for (i = 1; i < MAX_INT_PARAM; i++)
+			if (io_port[i] == port_base) {
+				io_port[i] = SKIP;
+				break;
+			}
+		if (i >= MAX_INT_PARAM)
+			goto out_disable_device;
+	}
+	if (port_probe(port_base, tpnt, dev) >= 0) {
+		eata2x_pci_device_count++;
+		return 0;
 	}
 
-	pci_dev_put(dev);
-#endif				/* end CONFIG_PCI */
+out_disable_device:
+	pci_disable_device(dev);
+out_error:
+	return ret;
+}
+
+static struct pci_device_id eata2x_tbl[] = {
+	{ PCI_DEVICE_CLASS(PCI_CLASS_STORAGE_SCSI << 8, PCI_ANY_ID) },
+	{ },
+};
+MODULE_DEVICE_TABLE(pci, eata2x_tbl);
+
+static struct pci_driver eata2x_pci_driver = {
+	.name		= "eata",
+	.id_table	= eata2x_tbl,
+	.probe		= eata2x_pci_probe,
+};
+
+static int eata2x_probe_pci_devices(struct scsi_host_template *tpnt)
+{
+	eata2x_tbl[0].driver_data = (kernel_ulong_t)tpnt;
+	if (pci_probe && pci_register_driver(&eata2x_pci_driver))
+		pr_warn("eata2x: failed to register PCI device driver.\n");
+	return eata2x_pci_device_count;
+}
+#else /* CONFIG_PCI */
+static inline int eata2x_probe_pci_devices(struct scsi_host_template *tpnt)
+{
+	return 0;
 }
+#endif /* CONFIG_PCI */
 
 static int eata2x_detect(struct scsi_host_template *tpnt)
 {
@@ -1592,16 +1576,10 @@ static int eata2x_detect(struct scsi_host_template *tpnt)
 				io_port[k] = SKIP;
 		}
 
-	if (pci_probe) {
-		if (!setup_done)
-			add_pci_ports();
-		else
-			enable_pci_ports();
-	}
-
+	count += eata2x_probe_pci_devices(tpnt);
 	for (k = 0; io_port[k]; k++)
 		if (io_port[k] != SKIP &&
-		    port_probe(io_port[k], tpnt) >= 0)
+		    port_probe(io_port[k], tpnt, NULL) >= 0)
 			count++;
 
 	return count;
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1223909 — Re: [Bugfix 2/3] eata: Implement PCI driver to manage eata PCI devices

On 09/14/2015 05:08 AM, Jiang Liu wrote:
> Previously the eata driver just grabs and accesses eata PCI devices
> without implementing a PCI device driver, that causes troubles with
> latest IRQ related
> 
> Commit 991de2e59090 ("PCI, x86: Implement pcibios_alloc_irq() and
> pcibios_free_irq()") changes the way to allocate PCI legacy IRQ
> for PCI devices on x86 platforms. Instead of allocating PCI legacy
> IRQs when pcibios_enable_device() gets called, now pcibios_alloc_irq()
> will be called by pci_device_probe() to allocate PCI legacy IRQs
> when binding PCI drivers to PCI devices.
> 
> But the eata driver directly accesses PCI devices without implementing
> corresponding PCI drivers, so pcibios_alloc_irq() won't be called for
> those PCI devices and wrong IRQ number may be used to manage the PCI
> device.
> 
> This patch implements a PCI device driver to manage eata PCI devices,
> so eata driver could properly cooperate with the PCI core. It also
> provides headroom for PCI hotplug with eata driver.
> 
> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> ---
>  drivers/scsi/eata.c |  170 ++++++++++++++++++++++-----------------------------
>  1 file changed, 74 insertions(+), 96 deletions(-)
> 
> diff --git a/drivers/scsi/eata.c b/drivers/scsi/eata.c
> index b45d3b532b70..b92e6856f909 100644
> --- a/drivers/scsi/eata.c
> +++ b/drivers/scsi/eata.c
> @@ -850,10 +850,6 @@ static unsigned long io_port[] = {
>  	/* First ISA */
>  	0x1f0,
>  
> -	/* Space for MAX_PCI ports possibly reported by PCI_BIOS */
> -	SKIP, SKIP, SKIP, SKIP, SKIP, SKIP, SKIP, SKIP,
> -	SKIP, SKIP, SKIP, SKIP, SKIP, SKIP, SKIP, SKIP,
> -
>  	/* MAX_EISA ports */
>  	0x1c88, 0x2c88, 0x3c88, 0x4c88, 0x5c88, 0x6c88, 0x7c88, 0x8c88,
>  	0x9c88, 0xac88, 0xbc88, 0xcc88, 0xdc88, 0xec88, 0xfc88,
> @@ -1024,60 +1020,13 @@ static int read_pio(unsigned long iobase, ushort * start, ushort * end)
>  	return 0;
>  }
>  
> -static struct pci_dev *get_pci_dev(unsigned long port_base)
> -{
> -#if defined(CONFIG_PCI)
> -	unsigned int addr;
> -	struct pci_dev *dev = NULL;
> -
> -	while ((dev = pci_get_class(PCI_CLASS_STORAGE_SCSI << 8, dev))) {
> -		addr = pci_resource_start(dev, 0);
> -
> -#if defined(DEBUG_PCI_DETECT)
> -		printk("%s: get_pci_dev, bus %d, devfn 0x%x, addr 0x%x.\n",
> -		       driver_name, dev->bus->number, dev->devfn, addr);
> -#endif
> -
> -		/* we are in so much trouble for a pci hotplug system with this driver
> -		 * anyway, so doing this at least lets people unload the driver and not
> -		 * cause memory problems, but in general this is a bad thing to do (this
> -		 * driver needs to be converted to the proper PCI api someday... */
> -		pci_dev_put(dev);
> -		if (addr + PCI_BASE_ADDRESS_0 == port_base)
> -			return dev;
> -	}
> -#endif				/* end CONFIG_PCI */
> -	return NULL;
> -}
> -
> -static void enable_pci_ports(void)
> -{
> -#if defined(CONFIG_PCI)
> -	struct pci_dev *dev = NULL;
> -
> -	while ((dev = pci_get_class(PCI_CLASS_STORAGE_SCSI << 8, dev))) {
> -#if defined(DEBUG_PCI_DETECT)
> -		printk("%s: enable_pci_ports, bus %d, devfn 0x%x.\n",
> -		       driver_name, dev->bus->number, dev->devfn);
> -#endif
> -
> -		if (pci_enable_device(dev))
> -			printk
> -			    ("%s: warning, pci_enable_device failed, bus %d devfn 0x%x.\n",
> -			     driver_name, dev->bus->number, dev->devfn);
> -	}
> -
> -#endif				/* end CONFIG_PCI */
> -}
> -
>  static int port_detect(unsigned long port_base, unsigned int j,
> -		struct scsi_host_template *tpnt)
> +		       struct scsi_host_template *tpnt, struct pci_dev *pdev)
>  {
>  	unsigned char irq, dma_channel, subversion, i, is_pci = 0;
>  	unsigned char protocol_rev;
>  	struct eata_info info;
>  	char *bus_type, dma_name[16];
> -	struct pci_dev *pdev;
>  	/* Allowed DMA channels for ISA (0 indicates reserved) */
>  	unsigned char dma_channel_table[4] = { 5, 6, 7, 0 };
>  	struct Scsi_Host *shost;
> @@ -1199,15 +1148,8 @@ static int port_detect(unsigned long port_base, unsigned int j,
>  		    ("%s: warning, LEVEL triggering is suggested for IRQ %u.\n",
>  		     name, irq);
>  
> -	if (is_pci) {
> -		pdev = get_pci_dev(port_base);
> -		if (!pdev)
> -			printk
> -			    ("%s: warning, failed to get pci_dev structure.\n",
> -			     name);
> -	} else
> -		pdev = NULL;
> -
> +	if (is_pci && !pdev)
> +		printk("%s: warning, failed to get pci_dev structure.\n", name);
>  	if (pdev && (irq != pdev->irq)) {
>  		printk("%s: IRQ %u mapped to IO-APIC IRQ %u.\n", name, irq,
>  		       pdev->irq);
> @@ -1510,14 +1452,17 @@ static int option_setup(char *str)
>  }
>  
>  static unsigned int port_probe(unsigned long port_base,
> -			       struct scsi_host_template *tpnt)
> +			       struct scsi_host_template *tpnt,
> +			       struct pci_dev *pdev)
>  {
>  	int id;
>  
>  	id = ida_simple_get(&eata_ida, 0, MAX_BOARDS, GFP_KERNEL);
>  	if (id >= 0) {
>  		set_bit(id, eata_board_bitmap);
> -		if (port_detect(port_base, id, tpnt))
> +		if (pdev)
> +			dev_set_drvdata(&pdev->dev, (void *)(long)id);
> +		if (port_detect(port_base, id, tpnt, pdev))
>  			return id;
>  		clear_bit(id, eata_board_bitmap);
>  		ida_simple_remove(&eata_ida, id);
> @@ -1526,42 +1471,81 @@ static unsigned int port_probe(unsigned long port_base,
>  	return -1;
>  }
>  
> -static void add_pci_ports(void)
> -{
> -#if defined(CONFIG_PCI)
> -	unsigned int addr, k;
> -	struct pci_dev *dev = NULL;
> -
> -	for (k = 0; k < MAX_PCI; k++) {
> +#ifdef CONFIG_PCI
> +static int eata2x_pci_device_count;
>  
> -		if (!(dev = pci_get_class(PCI_CLASS_STORAGE_SCSI << 8, dev)))
> -			break;
> +static int eata2x_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
> +{
> +	int i, ret = -ENXIO;
> +	resource_size_t addr;
> +	unsigned long port_base;
> +	struct scsi_host_template *tpnt = (void *)id->driver_data;
>  
> -		if (pci_enable_device(dev)) {
> +	if (pci_enable_device(dev)) {
>  #if defined(DEBUG_PCI_DETECT)
> -			printk
> -			    ("%s: detect, bus %d, devfn 0x%x, pci_enable_device failed.\n",
> -			     driver_name, dev->bus->number, dev->devfn);
> +		pr_warn("%s: detect, bus %d, devfn 0x%x, pci_enable_device failed.\n",
> +			driver_name, dev->bus->number, dev->devfn);
>  #endif
> +		goto out_error;
> +	}
>  
> -			continue;
> -		}
> -
> -		addr = pci_resource_start(dev, 0);
> -
> +	addr = pci_resource_start(dev, 0);
> +	port_base = addr + PCI_BASE_ADDRESS_0;
>  #if defined(DEBUG_PCI_DETECT)
> -		printk("%s: detect, seq. %d, bus %d, devfn 0x%x, addr 0x%x.\n",
> -		       driver_name, k, dev->bus->number, dev->devfn, addr);
> +	printk("%s: detect, bus %d, devfn 0x%x, addr 0x%x.\n",
> +	       driver_name, dev->bus->number, dev->devfn, (unsigned int)addr);
>  #endif
>  
> -		/* Order addresses according to rev_scan value */
> -		io_port[MAX_INT_PARAM + (rev_scan ? (MAX_PCI - k) : (1 + k))] =
> -		    addr + PCI_BASE_ADDRESS_0;
> +	if (setup_done) {
> +		/*
> +		 * Handle kernel or module parameter
> +		 * . probe board if its port is specified by user
> +		 * . otherwise ignore the board
> +		 */
> +		for (i = 1; i < MAX_INT_PARAM; i++)
> +			if (io_port[i] == port_base) {
> +				io_port[i] = SKIP;
> +				break;
> +			}
> +		if (i >= MAX_INT_PARAM)
> +			goto out_disable_device;
> +	}
Hmm. I must admit I don't like the 'setup_done' thingie. As the driver
is now converted to a 'real' PCI device we should be using driver-core
mechanisms to avoid driver binding, not the prefabricated 'setup_done'
variable.
Can't we just do away with it completely?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1223926 — Re: [Bugfix 2/3] eata: Implement PCI driver to manage eata PCI devices

On 2015/9/14 16:17, Hannes Reinecke wrote:
> On 09/14/2015 05:08 AM, Jiang Liu wrote:
>> Previously the eata driver just grabs and accesses eata PCI devices
>> without implementing a PCI device driver, that causes troubles with
>> latest IRQ related
>>
>> Commit 991de2e59090 ("PCI, x86: Implement pcibios_alloc_irq() and
>> pcibios_free_irq()") changes the way to allocate PCI legacy IRQ
>> for PCI devices on x86 platforms. Instead of allocating PCI legacy
>> IRQs when pcibios_enable_device() gets called, now pcibios_alloc_irq()
>> will be called by pci_device_probe() to allocate PCI legacy IRQs
>> when binding PCI drivers to PCI devices.
>>
>> But the eata driver directly accesses PCI devices without implementing
>> corresponding PCI drivers, so pcibios_alloc_irq() won't be called for
>> those PCI devices and wrong IRQ number may be used to manage the PCI
>> device.
>>
>> This patch implements a PCI device driver to manage eata PCI devices,
>> so eata driver could properly cooperate with the PCI core. It also
>> provides headroom for PCI hotplug with eata driver.
>>
>> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
>> ---
>>  drivers/scsi/eata.c |  170 ++++++++++++++++++++++-----------------------------
>>  1 file changed, 74 insertions(+), 96 deletions(-)
>>
>> diff --git a/drivers/scsi/eata.c b/drivers/scsi/eata.c
>> index b45d3b532b70..b92e6856f909 100644
>> --- a/drivers/scsi/eata.c
>> +++ b/drivers/scsi/eata.c
>> @@ -850,10 +850,6 @@ static unsigned long io_port[] = {
>>  	/* First ISA */
>>  	0x1f0,
>>  
>> -	/* Space for MAX_PCI ports possibly reported by PCI_BIOS */
>> -	SKIP, SKIP, SKIP, SKIP, SKIP, SKIP, SKIP, SKIP,
>> -	SKIP, SKIP, SKIP, SKIP, SKIP, SKIP, SKIP, SKIP,
>> -
>>  	/* MAX_EISA ports */
>>  	0x1c88, 0x2c88, 0x3c88, 0x4c88, 0x5c88, 0x6c88, 0x7c88, 0x8c88,
>>  	0x9c88, 0xac88, 0xbc88, 0xcc88, 0xdc88, 0xec88, 0xfc88,
>> @@ -1024,60 +1020,13 @@ static int read_pio(unsigned long iobase, ushort * start, ushort * end)
>>  	return 0;
>>  }
>>  
>> -static struct pci_dev *get_pci_dev(unsigned long port_base)
>> -{
>> -#if defined(CONFIG_PCI)
>> -	unsigned int addr;
>> -	struct pci_dev *dev = NULL;
>> -
>> -	while ((dev = pci_get_class(PCI_CLASS_STORAGE_SCSI << 8, dev))) {
>> -		addr = pci_resource_start(dev, 0);
>> -
>> -#if defined(DEBUG_PCI_DETECT)
>> -		printk("%s: get_pci_dev, bus %d, devfn 0x%x, addr 0x%x.\n",
>> -		       driver_name, dev->bus->number, dev->devfn, addr);
>> -#endif
>> -
>> -		/* we are in so much trouble for a pci hotplug system with this driver
>> -		 * anyway, so doing this at least lets people unload the driver and not
>> -		 * cause memory problems, but in general this is a bad thing to do (this
>> -		 * driver needs to be converted to the proper PCI api someday... */
>> -		pci_dev_put(dev);
>> -		if (addr + PCI_BASE_ADDRESS_0 == port_base)
>> -			return dev;
>> -	}
>> -#endif				/* end CONFIG_PCI */
>> -	return NULL;
>> -}
>> -
>> -static void enable_pci_ports(void)
>> -{
>> -#if defined(CONFIG_PCI)
>> -	struct pci_dev *dev = NULL;
>> -
>> -	while ((dev = pci_get_class(PCI_CLASS_STORAGE_SCSI << 8, dev))) {
>> -#if defined(DEBUG_PCI_DETECT)
>> -		printk("%s: enable_pci_ports, bus %d, devfn 0x%x.\n",
>> -		       driver_name, dev->bus->number, dev->devfn);
>> -#endif
>> -
>> -		if (pci_enable_device(dev))
>> -			printk
>> -			    ("%s: warning, pci_enable_device failed, bus %d devfn 0x%x.\n",
>> -			     driver_name, dev->bus->number, dev->devfn);
>> -	}
>> -
>> -#endif				/* end CONFIG_PCI */
>> -}
>> -
>>  static int port_detect(unsigned long port_base, unsigned int j,
>> -		struct scsi_host_template *tpnt)
>> +		       struct scsi_host_template *tpnt, struct pci_dev *pdev)
>>  {
>>  	unsigned char irq, dma_channel, subversion, i, is_pci = 0;
>>  	unsigned char protocol_rev;
>>  	struct eata_info info;
>>  	char *bus_type, dma_name[16];
>> -	struct pci_dev *pdev;
>>  	/* Allowed DMA channels for ISA (0 indicates reserved) */
>>  	unsigned char dma_channel_table[4] = { 5, 6, 7, 0 };
>>  	struct Scsi_Host *shost;
>> @@ -1199,15 +1148,8 @@ static int port_detect(unsigned long port_base, unsigned int j,
>>  		    ("%s: warning, LEVEL triggering is suggested for IRQ %u.\n",
>>  		     name, irq);
>>  
>> -	if (is_pci) {
>> -		pdev = get_pci_dev(port_base);
>> -		if (!pdev)
>> -			printk
>> -			    ("%s: warning, failed to get pci_dev structure.\n",
>> -			     name);
>> -	} else
>> -		pdev = NULL;
>> -
>> +	if (is_pci && !pdev)
>> +		printk("%s: warning, failed to get pci_dev structure.\n", name);
>>  	if (pdev && (irq != pdev->irq)) {
>>  		printk("%s: IRQ %u mapped to IO-APIC IRQ %u.\n", name, irq,
>>  		       pdev->irq);
>> @@ -1510,14 +1452,17 @@ static int option_setup(char *str)
>>  }
>>  
>>  static unsigned int port_probe(unsigned long port_base,
>> -			       struct scsi_host_template *tpnt)
>> +			       struct scsi_host_template *tpnt,
>> +			       struct pci_dev *pdev)
>>  {
>>  	int id;
>>  
>>  	id = ida_simple_get(&eata_ida, 0, MAX_BOARDS, GFP_KERNEL);
>>  	if (id >= 0) {
>>  		set_bit(id, eata_board_bitmap);
>> -		if (port_detect(port_base, id, tpnt))
>> +		if (pdev)
>> +			dev_set_drvdata(&pdev->dev, (void *)(long)id);
>> +		if (port_detect(port_base, id, tpnt, pdev))
>>  			return id;
>>  		clear_bit(id, eata_board_bitmap);
>>  		ida_simple_remove(&eata_ida, id);
>> @@ -1526,42 +1471,81 @@ static unsigned int port_probe(unsigned long port_base,
>>  	return -1;
>>  }
>>  
>> -static void add_pci_ports(void)
>> -{
>> -#if defined(CONFIG_PCI)
>> -	unsigned int addr, k;
>> -	struct pci_dev *dev = NULL;
>> -
>> -	for (k = 0; k < MAX_PCI; k++) {
>> +#ifdef CONFIG_PCI
>> +static int eata2x_pci_device_count;
>>  
>> -		if (!(dev = pci_get_class(PCI_CLASS_STORAGE_SCSI << 8, dev)))
>> -			break;
>> +static int eata2x_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
>> +{
>> +	int i, ret = -ENXIO;
>> +	resource_size_t addr;
>> +	unsigned long port_base;
>> +	struct scsi_host_template *tpnt = (void *)id->driver_data;
>>  
>> -		if (pci_enable_device(dev)) {
>> +	if (pci_enable_device(dev)) {
>>  #if defined(DEBUG_PCI_DETECT)
>> -			printk
>> -			    ("%s: detect, bus %d, devfn 0x%x, pci_enable_device failed.\n",
>> -			     driver_name, dev->bus->number, dev->devfn);
>> +		pr_warn("%s: detect, bus %d, devfn 0x%x, pci_enable_device failed.\n",
>> +			driver_name, dev->bus->number, dev->devfn);
>>  #endif
>> +		goto out_error;
>> +	}
>>  
>> -			continue;
>> -		}
>> -
>> -		addr = pci_resource_start(dev, 0);
>> -
>> +	addr = pci_resource_start(dev, 0);
>> +	port_base = addr + PCI_BASE_ADDRESS_0;
>>  #if defined(DEBUG_PCI_DETECT)
>> -		printk("%s: detect, seq. %d, bus %d, devfn 0x%x, addr 0x%x.\n",
>> -		       driver_name, k, dev->bus->number, dev->devfn, addr);
>> +	printk("%s: detect, bus %d, devfn 0x%x, addr 0x%x.\n",
>> +	       driver_name, dev->bus->number, dev->devfn, (unsigned int)addr);
>>  #endif
>>  
>> -		/* Order addresses according to rev_scan value */
>> -		io_port[MAX_INT_PARAM + (rev_scan ? (MAX_PCI - k) : (1 + k))] =
>> -		    addr + PCI_BASE_ADDRESS_0;
>> +	if (setup_done) {
>> +		/*
>> +		 * Handle kernel or module parameter
>> +		 * . probe board if its port is specified by user
>> +		 * . otherwise ignore the board
>> +		 */
>> +		for (i = 1; i < MAX_INT_PARAM; i++)
>> +			if (io_port[i] == port_base) {
>> +				io_port[i] = SKIP;
>> +				break;
>> +			}
>> +		if (i >= MAX_INT_PARAM)
>> +			goto out_disable_device;
>> +	}
> Hmm. I must admit I don't like the 'setup_done' thingie. As the driver
> is now converted to a 'real' PCI device we should be using driver-core
> mechanisms to avoid driver binding, not the prefabricated 'setup_done'
> variable.
> Can't we just do away with it completely?
Hi Hannes,
	Thanks for review. Originally user may specify ports to probe
by kernel or module parameter, no matter it's a ISA, EISA or PCI device.
My concern here is that, if we don't do the setup_done dance here,
it may cause trouble if user load new eata driver with old parameter
configurations, so PCI eata device will be probed twice. So point here
is that we still honor user kernel or module parameter configurations.
Thanks!
Gerry


> 
> Cheers,
> 
> Hannes
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1223819 — [Bugfix 1/3] eata: Use IDA to manage eata board IDs

Use IDA to manage eata board IDs, so we could dynamically allocate
and free board IDs later.

Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
---
 drivers/scsi/eata.c |   46 ++++++++++++++++++++++++++++++----------------
 1 file changed, 30 insertions(+), 16 deletions(-)

diff --git a/drivers/scsi/eata.c b/drivers/scsi/eata.c
index 227dd2c2ec2f..b45d3b532b70 100644
--- a/drivers/scsi/eata.c
+++ b/drivers/scsi/eata.c
@@ -491,6 +491,7 @@
 #include <linux/spinlock.h>
 #include <linux/dma-mapping.h>
 #include <linux/slab.h>
+#include <linux/idr.h>
 #include <asm/byteorder.h>
 #include <asm/dma.h>
 #include <asm/io.h>
@@ -837,9 +838,8 @@ struct hostdata {
 static struct Scsi_Host *sh[MAX_BOARDS];
 static const char *driver_name = "EATA";
 static char sha[MAX_BOARDS];
-
-/* Initialize num_boards so that ihdlr can work while detect is in progress */
-static unsigned int num_boards = MAX_BOARDS;
+static DEFINE_IDA(eata_ida);
+static DECLARE_BITMAP(eata_board_bitmap, MAX_BOARDS);
 
 static unsigned long io_port[] = {
 
@@ -1509,6 +1509,23 @@ static int option_setup(char *str)
 	return 1;
 }
 
+static unsigned int port_probe(unsigned long port_base,
+			       struct scsi_host_template *tpnt)
+{
+	int id;
+
+	id = ida_simple_get(&eata_ida, 0, MAX_BOARDS, GFP_KERNEL);
+	if (id >= 0) {
+		set_bit(id, eata_board_bitmap);
+		if (port_detect(port_base, id, tpnt))
+			return id;
+		clear_bit(id, eata_board_bitmap);
+		ida_simple_remove(&eata_ida, id);
+	}
+
+	return -1;
+}
+
 static void add_pci_ports(void)
 {
 #if defined(CONFIG_PCI)
@@ -1548,7 +1565,7 @@ static void add_pci_ports(void)
 
 static int eata2x_detect(struct scsi_host_template *tpnt)
 {
-	unsigned int j = 0, k;
+	unsigned int k, count = 0;
 
 	tpnt->proc_name = "eata2x";
 
@@ -1582,17 +1599,12 @@ static int eata2x_detect(struct scsi_host_template *tpnt)
 			enable_pci_ports();
 	}
 
-	for (k = 0; io_port[k]; k++) {
+	for (k = 0; io_port[k]; k++)
+		if (io_port[k] != SKIP &&
+		    port_probe(io_port[k], tpnt) >= 0)
+			count++;
 
-		if (io_port[k] == SKIP)
-			continue;
-
-		if (j < MAX_BOARDS && port_detect(io_port[k], j, tpnt))
-			j++;
-	}
-
-	num_boards = j;
-	return j;
+	return count;
 }
 
 static void map_dma(unsigned int i, struct hostdata *ha)
@@ -2530,14 +2542,16 @@ static irqreturn_t ihdlr(struct Scsi_Host *shost)
 static irqreturn_t do_interrupt_handler(int dummy, void *shap)
 {
 	struct Scsi_Host *shost;
-	unsigned int j;
+	unsigned int j = (unsigned int)((char *)shap - sha);
 	unsigned long spin_flags;
 	irqreturn_t ret;
 
 	/* Check if the interrupt must be processed by this handler */
-	if ((j = (unsigned int)((char *)shap - sha)) >= num_boards)
+	if (j >= MAX_BOARDS || !test_bit(j, eata_board_bitmap))
 		return IRQ_NONE;
 	shost = sh[j];
+	if (!shost)
+		return IRQ_NONE;
 
 	spin_lock_irqsave(shost->host_lock, spin_flags);
 	ret = ihdlr(shost);
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web